[v3,3/4] pinctrl: Implementation of the generic scmi-pinctrl driver

scmi-pinctrl driver implements pinctrl driver interface and using
SCMI protocol to redirect messages from pinctrl subsystem SDK to
SCP firmware, which does the changes in HW.

This setup expects SCP firmware (or similar system, such as ATF)
to be installed on the platform, which implements pinctrl driver
for the specific platform.

SCMI-Pinctrl driver should be configured from the device-tree and uses
generic device-tree mappings for the configuration.

Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
---
 MAINTAINERS                    |   1 +
 drivers/pinctrl/Kconfig        |  11 +
 drivers/pinctrl/Makefile       |   1 +
 drivers/pinctrl/pinctrl-scmi.c | 554 +++++++++++++++++++++++++++++++++
 4 files changed, 567 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-scmi.c

Tue, Jun 06, 2023 at 04:22:28PM +0000, Oleksii Moisieiev kirjoitti:
> scmi-pinctrl driver implements pinctrl driver interface and using
> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> SCP firmware, which does the changes in HW.
> 
> This setup expects SCP firmware (or similar system, such as ATF)
> to be installed on the platform, which implements pinctrl driver
> for the specific platform.
> 
> SCMI-Pinctrl driver should be configured from the device-tree and uses
> generic device-tree mappings for the configuration.

...

> +++ b/drivers/pinctrl/Kconfig
> @@ -546,4 +546,15 @@ source "drivers/pinctrl/uniphier/Kconfig"
>  source "drivers/pinctrl/visconti/Kconfig"
>  source "drivers/pinctrl/vt8500/Kconfig"
>  
> +config PINCTRL_SCMI
> +	tristate "Pinctrl driver controlled via SCMI interface"
> +	depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> +	select PINMUX
> +	select GENERIC_PINCONF
> +	help
> +	  This driver provides support for pinctrl which is controlled
> +	  by firmware that implements the SCMI interface.
> +	  It uses SCMI Message Protocol to interact with the
> +	  firmware providing all the pinctrl controls.

Sounds to me that u and v should be after S. Decrypting for your convenience,
the above is ordered and proposed change misses that.

>  endif

Btw, what is this endif for and how does it affect your Kconfig option?

...

> +++ b/drivers/pinctrl/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
>  obj-$(CONFIG_PINCTRL_TB10X)	+= pinctrl-tb10x.o
>  obj-$(CONFIG_PINCTRL_ZYNQMP)	+= pinctrl-zynqmp.o
>  obj-$(CONFIG_PINCTRL_ZYNQ)	+= pinctrl-zynq.o
> +obj-$(CONFIG_PINCTRL_SCMI)	+= pinctrl-scmi.o

Ditto.

>  obj-y				+= actions/
>  obj-$(CONFIG_ARCH_ASPEED)	+= aspeed/

...

> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/seq_file.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>

> +struct scmi_pinctrl_funcs {
> +	unsigned int num_groups;
> +	const char **groups;
> +};

struct pinfunction

...

> +struct scmi_pinctrl {
> +	struct device *dev;
> +	struct scmi_protocol_handle *ph;
> +	struct pinctrl_dev *pctldev;
> +	struct pinctrl_desc pctl_desc;
> +	struct scmi_pinctrl_funcs *functions;
> +	unsigned int nr_functions;
> +	char **groups;

struct pingroup ?

> +	unsigned int nr_groups;
> +	struct pinctrl_pin_desc *pins;
> +	unsigned int nr_pins;
> +};

...

> +	pmx = pinctrl_dev_get_drvdata(pctldev);

> +

Redundant blank line.

> +	if (!pmx || !pmx->ph)
> +		return NULL;

...

> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph)
> +		return -EINVAL;

Ditto. And so on in a few more places.

...

> +		pmx->functions[selector].groups[i] =

> +			pinctrl_scmi_get_group_name(pmx->pctldev,
> +						    group_ids[i]);

It's okay to have this on a single line which takes only 81 character.


...

> +error:

Labels shoud be self-explanatory, i.e. they should tell what _will_ be when goto.

> +	devm_kfree(pmx->dev, pmx->functions[selector].groups);

Red Flag. Please, elaborate.

> +
> +	return ret;

...

> +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
> +				    unsigned int _pin,
> +				    unsigned long *configs,
> +				    unsigned int num_configs)
> +{
> +	int i, ret;
> +	struct scmi_pinctrl *pmx;
> +	enum pin_config_param config_type;
> +	unsigned long config_value;

> +	if (!pctldev)
> +		return -EINVAL;

Huh?! When this is not a dead code?

Ditto for other places.

> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph || !configs || num_configs == 0)
> +		return -EINVAL;
> +
> +	for (i = 0; i < num_configs; i++) {
> +		config_type = pinconf_to_config_param(configs[i]);
> +		config_value = pinconf_to_config_argument(configs[i]);
> +
> +		ret = pinctrl_ops->set_config(pmx->ph, _pin, PIN_TYPE, config_type,
> +					      config_value);
> +		if (ret) {
> +			dev_err(pmx->dev, "Error parsing config %ld\n",
> +				configs[i]);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}

...

> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
> +					  unsigned int _pin,

Why this strange parameter name?

> +					  unsigned long *config)

...

> + err_free:

This is better, but shows the inconsistency with the other goto label namings.

> +	devm_kfree(pmx->dev, pmx->pins);

Red Flag. Please, elaborate.

> +	pmx->nr_pins = 0;
> +
> +	return ret;

...

> +	ret = devm_pinctrl_register_and_init(&sdev->dev, &pmx->pctl_desc, pmx,
> +					     &pmx->pctldev);
> +	if (ret) {
> +		dev_err_probe(&sdev->dev, ret, "Failed to register pinctrl\n");
> +		return ret;

		return dev_err_probe(...);

> +	}

...

> +		pmx->functions =
> +			devm_kcalloc(&sdev->dev, pmx->nr_functions,

This is perfectly a signle line.

Also with

	struct device *dev = &sdev->dev;

at the top you may make the entire ->probe() look neater.

> +				     sizeof(*pmx->functions),
> +				     GFP_KERNEL);
> +		if (!pmx->functions)
> +			return -ENOMEM;

On Tue, Jun 06, 2023 at 04:22:28PM +0000, Oleksii Moisieiev wrote:
> scmi-pinctrl driver implements pinctrl driver interface and using
> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> SCP firmware, which does the changes in HW.

I would drop any reference to SCP, which is a reference implementation
BUT not neccesarily the only one.... "SCMI platform firware" should be
generic enough.

> 
> This setup expects SCP firmware (or similar system, such as ATF)
> to be installed on the platform, which implements pinctrl driver
> for the specific platform.
> 
> SCMI-Pinctrl driver should be configured from the device-tree and uses
> generic device-tree mappings for the configuration.

This should be obvious, I would drop any reference to the fact that you
need an SCMI server somewhere replying to your requests and to the need
of a DT configuration thing which is anyeway standard.

> 
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> ---
>  MAINTAINERS                    |   1 +
>  drivers/pinctrl/Kconfig        |  11 +
>  drivers/pinctrl/Makefile       |   1 +
>  drivers/pinctrl/pinctrl-scmi.c | 554 +++++++++++++++++++++++++++++++++
>  4 files changed, 567 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-scmi.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 297b2512963d..91883955fc1a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20527,6 +20527,7 @@ M:	Oleksii Moisieiev <oleksii_moisieiev@epam.com>
>  L:	linux-arm-kernel@lists.infradead.org
>  S:	Maintained
>  F:	drivers/firmware/arm_scmi/pinctrl.c
> +F:	drivers/pinctrl/pinctrl-scmi.c
>  
>  SYSTEM RESET/SHUTDOWN DRIVERS
>  M:	Sebastian Reichel <sre@kernel.org>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 5787c579dcf6..c4680a2c5e13 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -546,4 +546,15 @@ source "drivers/pinctrl/uniphier/Kconfig"
>  source "drivers/pinctrl/visconti/Kconfig"
>  source "drivers/pinctrl/vt8500/Kconfig"
>  
> +config PINCTRL_SCMI
> +	tristate "Pinctrl driver controlled via SCMI interface"

"using SCMI protocol interface" maybe is more fitting...it is the pin
that is controlled via SCMI not the driver, which is indeed the
controller of the pin via SCMI O_o

> +	depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> +	select PINMUX
> +	select GENERIC_PINCONF
> +	help
> +	  This driver provides support for pinctrl which is controlled
> +	  by firmware that implements the SCMI interface.
> +	  It uses SCMI Message Protocol to interact with the
> +	  firmware providing all the pinctrl controls.
> +
>  endif
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index e196c6e324ad..b932a116e6a0 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
>  obj-$(CONFIG_PINCTRL_TB10X)	+= pinctrl-tb10x.o
>  obj-$(CONFIG_PINCTRL_ZYNQMP)	+= pinctrl-zynqmp.o
>  obj-$(CONFIG_PINCTRL_ZYNQ)	+= pinctrl-zynq.o
> +obj-$(CONFIG_PINCTRL_SCMI)	+= pinctrl-scmi.o
>  

Probably needs to be alphabetically ordered.

>  obj-y				+= actions/
>  obj-$(CONFIG_ARCH_ASPEED)	+= aspeed/
> diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
> new file mode 100644
> index 000000000000..e46dffa652c6
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-scmi.c
> @@ -0,0 +1,554 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Power Interface (SCMI) Protocol based pinctrl driver
> + *
> + * Copyright (C) 2023 EPAM
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/seq_file.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +
> +#include <linux/pinctrl/machine.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +
> +#include "pinctrl-utils.h"
> +#include "core.h"
> +#include "pinconf.h"
> +
> +#define DRV_NAME "scmi-pinctrl"
> +
> +static const struct scmi_pinctrl_proto_ops *pinctrl_ops;
> +
> +struct scmi_pinctrl_funcs {
> +	unsigned int num_groups;
> +	const char **groups;
> +};
> +
> +struct scmi_pinctrl {
> +	struct device *dev;
> +	struct scmi_protocol_handle *ph;
> +	struct pinctrl_dev *pctldev;
> +	struct pinctrl_desc pctl_desc;
> +	struct scmi_pinctrl_funcs *functions;
> +	unsigned int nr_functions;
> +	char **groups;
> +	unsigned int nr_groups;
> +	struct pinctrl_pin_desc *pins;
> +	unsigned int nr_pins;
> +};
> +
> +static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +	struct scmi_pinctrl *pmx;
> +
> +	if (!pctldev)
> +		return -EINVAL;
> +
> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph)
> +		return -EINVAL;

This check is superfluos right ? If you registered successfully with the
Pinctrl subsys and you passed your pmx driver_data as properly
initialized in _probe you can never get a NULL here for pmx or pmx->ph.

> +
> +	return pinctrl_ops->get_count(pmx->ph, GROUP_TYPE);
> +}
> +
> +static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev,
> +					       unsigned int selector)
> +{
> +	int ret;
> +	const char *name;
> +	struct scmi_pinctrl *pmx;
> +
> +	if (!pctldev)
> +		return NULL;
> +
> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph)
> +		return NULL;
> +

Ditto.

> +	ret = pinctrl_ops->get_name(pmx->ph, selector, GROUP_TYPE, &name);
> +	if (ret) {
> +		dev_err(pmx->dev, "get name failed with err %d", ret);
> +		return NULL;
> +	}
> +
> +	return name;
> +}
> +
> +static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev,
> +				       unsigned int selector,
> +				       const unsigned int **pins,
> +				       unsigned int *num_pins)
> +{
> +	struct scmi_pinctrl *pmx;
> +
> +	if (!pctldev)
> +		return -EINVAL;
> +
> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph)
> +		return -EINVAL;
> +

Ditto.

> +	return pinctrl_ops->get_group_pins(pmx->ph, selector,
> +					   pins, num_pins);
> +}
> +
> +#ifdef CONFIG_OF
> +static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev,
> +				       struct device_node *np_config,
> +				       struct pinctrl_map **map,
> +				       u32 *num_maps)
> +{
> +	return pinconf_generic_dt_node_to_map(pctldev, np_config, map,
> +					      num_maps, PIN_MAP_TYPE_INVALID);
> +}
> +
> +static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev,
> +				     struct pinctrl_map *map, u32 num_maps)
> +{
> +	kfree(map);
> +}
> +
> +#endif /* CONFIG_OF */
> +
> +static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = {
> +	.get_groups_count = pinctrl_scmi_get_groups_count,
> +	.get_group_name = pinctrl_scmi_get_group_name,
> +	.get_group_pins = pinctrl_scmi_get_group_pins,
> +#ifdef CONFIG_OF
> +	.dt_node_to_map = pinctrl_scmi_dt_node_to_map,
> +	.dt_free_map = pinctrl_scmi_dt_free_map,
> +#endif
> +};
> +
> +static int pinctrl_scmi_get_functions_count(struct pinctrl_dev *pctldev)
> +{
> +	struct scmi_pinctrl *pmx;
> +
> +	if (!pctldev)
> +		return -EINVAL;
> +
> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph)
> +		return -EINVAL;
> +

Ditto.

> +	return pinctrl_ops->get_count(pmx->ph, FUNCTION_TYPE);
> +}
> +
> +static const char *pinctrl_scmi_get_function_name(struct pinctrl_dev *pctldev,
> +						  unsigned int selector)
> +{
> +	int ret;
> +	const char *name;
> +	struct scmi_pinctrl *pmx;
> +
> +	if (!pctldev)
> +		return NULL;
> +
> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph)
> +		return NULL;
> +

Ditto.

> +	ret = pinctrl_ops->get_name(pmx->ph, selector, FUNCTION_TYPE, &name);
> +	if (ret) {
> +		dev_err(pmx->dev, "get name failed with err %d", ret);
> +		return NULL;
> +	}
> +
> +	return name;
> +}
> +
> +static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev,
> +					    unsigned int selector,
> +					    const char * const **groups,
> +					    unsigned int * const num_groups)
> +{
> +	const unsigned int *group_ids;
> +	int ret, i;
> +	struct scmi_pinctrl *pmx;
> +
> +	if (!pctldev)
> +		return -EINVAL;
> +
> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph || !groups || !num_groups)
> +		return -EINVAL;
> +

Ditto for pmx/pmx->ph.

> +	if (selector < pmx->nr_functions &&
> +	    pmx->functions[selector].num_groups) {
> +		*groups = (const char * const *)pmx->functions[selector].groups;
> +		*num_groups = pmx->functions[selector].num_groups;
> +		return 0;
> +	}
> +
> +	ret = pinctrl_ops->get_function_groups(pmx->ph, selector,
> +					       &pmx->functions[selector].num_groups,
> +					       &group_ids);
> +	if (ret) {
> +		dev_err(pmx->dev, "Unable to get function groups, err %d", ret);
> +		return ret;
> +	}
> +
> +	*num_groups = pmx->functions[selector].num_groups;
> +	if (!*num_groups)
> +		return -EINVAL;
> +
> +	pmx->functions[selector].groups =
> +		devm_kcalloc(pmx->dev, *num_groups,
> +			     sizeof(*pmx->functions[selector].groups),
> +			     GFP_KERNEL);
> +	if (!pmx->functions[selector].groups)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < *num_groups; i++) {
> +		pmx->functions[selector].groups[i] =
> +			pinctrl_scmi_get_group_name(pmx->pctldev,
> +						    group_ids[i]);
> +		if (!pmx->functions[selector].groups[i]) {
> +			ret = -ENOMEM;
> +			goto error;
> +		}
> +	}
> +
> +	*groups = (const char * const *)pmx->functions[selector].groups;
> +
> +	return 0;
> +
> +error:
> +	devm_kfree(pmx->dev, pmx->functions[selector].groups);
> +
> +	return ret;
> +}
> +
> +static int pinctrl_scmi_func_set_mux(struct pinctrl_dev *pctldev,
> +				     unsigned int selector, unsigned int group)
> +{
> +	struct scmi_pinctrl *pmx;
> +
> +	if (!pctldev)
> +		return -EINVAL;
> +
> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph)
> +		return -EINVAL;

Ditto.

> +
> +	return pinctrl_ops->set_mux(pmx->ph, selector, group);
> +}
> +
> +static int pinctrl_scmi_request(struct pinctrl_dev *pctldev,
> +				unsigned int offset)
> +{
> +	struct scmi_pinctrl *pmx;
> +
> +	if (!pctldev)
> +		return -EINVAL;
> +
> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph)
> +		return -EINVAL;

Ditto.

> +
> +	return pinctrl_ops->request_pin(pmx->ph, offset);
> +}
> +
> +static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int offset)
> +{
> +	struct scmi_pinctrl *pmx;
> +
> +	if (!pctldev)
> +		return -EINVAL;
> +
> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph)
> +		return -EINVAL;

Ditto.

> +
> +	return pinctrl_ops->free_pin(pmx->ph, offset);
> +}
> +
> +static const struct pinmux_ops pinctrl_scmi_pinmux_ops = {
> +	.request = pinctrl_scmi_request,
> +	.free = pinctrl_scmi_free,
> +	.get_functions_count = pinctrl_scmi_get_functions_count,
> +	.get_function_name = pinctrl_scmi_get_function_name,
> +	.get_function_groups = pinctrl_scmi_get_function_groups,
> +	.set_mux = pinctrl_scmi_func_set_mux,
> +};
> +
> +static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
> +				    unsigned int _pin,
> +				    unsigned long *config)
> +{
> +	int ret;
> +	struct scmi_pinctrl *pmx;
> +	enum pin_config_param config_type;
> +	unsigned long config_value;
> +
> +	if (!pctldev)
> +		return -EINVAL;
> +
> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph || !config)
> +		return -EINVAL;
> +

Ditto for pmx/pmx->ph.

> +	config_type = pinconf_to_config_param(*config);
> +
> +	ret = pinctrl_ops->get_config(pmx->ph, _pin, PIN_TYPE, config_type,
> +				      &config_value);
> +	if (ret)
> +		return ret;
> +
> +	*config = pinconf_to_config_packed(config_type, config_value);
> +
> +	return 0;
> +}
> +
> +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
> +				    unsigned int _pin,
> +				    unsigned long *configs,
> +				    unsigned int num_configs)
> +{
> +	int i, ret;
> +	struct scmi_pinctrl *pmx;
> +	enum pin_config_param config_type;
> +	unsigned long config_value;
> +
> +	if (!pctldev)
> +		return -EINVAL;
> +
> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph || !configs || num_configs == 0)
> +		return -EINVAL;

Ditto for pmx/pmx->ph.

> +
> +	for (i = 0; i < num_configs; i++) {
> +		config_type = pinconf_to_config_param(configs[i]);
> +		config_value = pinconf_to_config_argument(configs[i]);
> +
> +		ret = pinctrl_ops->set_config(pmx->ph, _pin, PIN_TYPE, config_type,
> +					      config_value);
> +		if (ret) {
> +			dev_err(pmx->dev, "Error parsing config %ld\n",
> +				configs[i]);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
> +					  unsigned int group,
> +					  unsigned long *configs,
> +					  unsigned int num_configs)
> +{
> +	int i, ret;
> +	struct scmi_pinctrl *pmx;
> +	enum pin_config_param config_type;
> +	unsigned long config_value;
> +
> +	if (!pctldev)
> +		return -EINVAL;
> +
> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph || !configs || num_configs == 0)
> +		return -EINVAL;

Ditto for pmx/pmx->ph.

> +
> +	for (i = 0; i < num_configs; i++) {
> +		config_type = pinconf_to_config_param(configs[i]);
> +		config_value = pinconf_to_config_argument(configs[i]);
> +
> +		ret = pinctrl_ops->set_config(pmx->ph, group, GROUP_TYPE,
> +					      config_type, config_value);
> +		if (ret) {
> +			dev_err(pmx->dev, "Error parsing config = %ld",
> +				configs[i]);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +};
> +
> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
> +					  unsigned int _pin,
> +					  unsigned long *config)
> +{
> +	int ret;
> +	struct scmi_pinctrl *pmx;
> +	enum pin_config_param config_type;
> +	unsigned long config_value;
> +
> +	if (!pctldev)
> +		return -EINVAL;
> +
> +	pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!pmx || !pmx->ph || !config)
> +		return -EINVAL;

Ditto for pmx/pmx->ph.

> +
> +	config_type = pinconf_to_config_param(*config);
> +
> +	ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE,
> +				      config_type, &config_value);
> +	if (ret)
> +		return ret;
> +
> +	*config = pinconf_to_config_packed(config_type, config_value);
> +
> +	return 0;
> +}
> +
> +static const struct pinconf_ops pinctrl_scmi_pinconf_ops = {
> +	.is_generic = true,
> +	.pin_config_get = pinctrl_scmi_pinconf_get,
> +	.pin_config_set = pinctrl_scmi_pinconf_set,
> +	.pin_config_group_set = pinctrl_scmi_pinconf_group_set,
> +	.pin_config_group_get = pinctrl_scmi_pinconf_group_get,
> +	.pin_config_config_dbg_show = pinconf_generic_dump_config,
> +};
> +
> +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
> +				 unsigned int *nr_pins,
> +				 const struct pinctrl_pin_desc **pins)
> +{
> +	int ret, i;
> +
> +	if (!pmx || !pmx->ph)
> +		return -EINVAL;

Ditto.

> +
> +	if (!pins || !nr_pins)
> +		return -EINVAL;
> +
> +	if (pmx->nr_pins) {
> +		*pins = pmx->pins;
> +		*nr_pins = pmx->nr_pins;
> +		return 0;
> +	}
> +
> +	*nr_pins = pinctrl_ops->get_count(pmx->ph, PIN_TYPE);
> +
> +	pmx->nr_pins = *nr_pins;
> +	pmx->pins = devm_kmalloc_array(pmx->dev, *nr_pins, sizeof(*pmx->pins),
> +				       GFP_KERNEL);
> +	if (!pmx->pins)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < *nr_pins; i++) {
> +		pmx->pins[i].number = i;
> +		ret = pinctrl_ops->get_name(pmx->ph, i, PIN_TYPE,
> +					    &pmx->pins[i].name);
> +		if (ret) {
> +			dev_err(pmx->dev, "Can't get name for pin %d: rc %d",
> +				i, ret);
> +			goto err_free;
> +		}
> +	}
> +
> +	*pins = pmx->pins;
> +	dev_dbg(pmx->dev, "got pins %d", *nr_pins);
> +
> +	return 0;
> + err_free:
> +	devm_kfree(pmx->dev, pmx->pins);
> +	pmx->nr_pins = 0;
> +
> +	return ret;
> +}
> +
> +static const struct scmi_device_id scmi_id_table[] = {
> +	{ SCMI_PROTOCOL_PINCTRL, "pinctrl" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> +
> +static int scmi_pinctrl_probe(struct scmi_device *sdev)
> +{
> +	int ret;
> +	struct scmi_pinctrl *pmx;
> +	const struct scmi_handle *handle;
> +	struct scmi_protocol_handle *ph;
> +
> +	if (!sdev || !sdev->handle)
> +		return -EINVAL;
> +
> +	handle = sdev->handle;
> +
> +	pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, &ph);
> +	if (IS_ERR(pinctrl_ops))
> +		return PTR_ERR(pinctrl_ops);
> +
> +	pmx = devm_kzalloc(&sdev->dev, sizeof(*pmx), GFP_KERNEL);
> +	if (!pmx)
> +		return -ENOMEM;
> +
> +	pmx->ph = ph;
> +
> +	pmx->dev = &sdev->dev;
> +	pmx->pctl_desc.name = DRV_NAME;
> +	pmx->pctl_desc.owner = THIS_MODULE;
> +	pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops;
> +	pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops;
> +	pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops;
> +
> +	ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc.npins,
> +				    &pmx->pctl_desc.pins);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_pinctrl_register_and_init(&sdev->dev, &pmx->pctl_desc, pmx,
> +					     &pmx->pctldev);
> +	if (ret) {
> +		dev_err_probe(&sdev->dev, ret, "Failed to register pinctrl\n");
> +		return ret;
> +	}
> +
> +	pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx->pctldev);
> +	pmx->nr_groups = pinctrl_scmi_get_groups_count(pmx->pctldev);
> +
> +	if (pmx->nr_functions) {
> +		pmx->functions =
> +			devm_kcalloc(&sdev->dev, pmx->nr_functions,
> +				     sizeof(*pmx->functions),
> +				     GFP_KERNEL);
> +		if (!pmx->functions)
> +			return -ENOMEM;
> +	}
> +
> +	if (pmx->nr_groups) {
> +		pmx->groups =
> +			devm_kcalloc(&sdev->dev, pmx->nr_groups,
> +				     sizeof(*pmx->groups),
> +				     GFP_KERNEL);
> +		if (!pmx->groups)
> +			return -ENOMEM;
> +	}
> +
> +	return pinctrl_enable(pmx->pctldev);
> +}
> +
> +static struct scmi_driver scmi_pinctrl_driver = {
> +	.name = DRV_NAME,
> +	.probe = scmi_pinctrl_probe,
> +	.id_table = scmi_id_table,
> +};
> +module_scmi_driver(scmi_pinctrl_driver);
> +

Thanks,
Cristian

Hi Andy,

andy.shevchenko@gmail.com writes:

> Tue, Jun 06, 2023 at 04:22:28PM +0000, Oleksii Moisieiev kirjoitti:
>> scmi-pinctrl driver implements pinctrl driver interface and using
>> SCMI protocol to redirect messages from pinctrl subsystem SDK to
>> SCP firmware, which does the changes in HW.
>> 
>> This setup expects SCP firmware (or similar system, such as ATF)
>> to be installed on the platform, which implements pinctrl driver
>> for the specific platform.
>> 
>> SCMI-Pinctrl driver should be configured from the device-tree and uses
>> generic device-tree mappings for the configuration.

[snip]

> ...
>
>> +error:
>
> Labels shoud be self-explanatory, i.e. they should tell what _will_ be when goto.
>
>> +	devm_kfree(pmx->dev, pmx->functions[selector].groups);
>
> Red Flag. Please, elaborate.
>

Thank you for the review.
I did some research regarding this and now I'm confused. Could you
please explain to me why it's a red flag?
IIUC devm_alloc/free functions are the calls to the resource-managed
alloc/free command, which is bound to the device.
pinctrl-scmi driver does devm_pinctrl_register_and_init which does
devres_alloc and doesn't open devres_group like
scmi_alloc_init_protocol_instance (thanks to Cristian detailed
explanation).

As was mentioned in Documentation/driver-api/driver-model/devres.rst:

```
No matter what, all devres entries are released on driver detach.  On
release, the associated release function is invoked and then the
devres entry is freed.
```

Also there is devm_pinctrl_get call listed in the managed interfaces.

My understanding is that all resources, bound to the particular device
will be freed on driver detach.

Also I found some examples of using devm_alloc/free like from dt_node_to_map
call in pinctrl-simple.c driver.

I agree that I need to implement .remove callback with proper cleanup,
but why can't I use devm_* here?
Maybe I've misunderstood your point.

On Thu, Jul 20, 2023 at 4:40 PM Oleksii Moisieiev
<Oleksii_Moisieiev@epam.com> wrote:
> andy.shevchenko@gmail.com writes:
> > Tue, Jun 06, 2023 at 04:22:28PM +0000, Oleksii Moisieiev kirjoitti:

...

> >> +    devm_kfree(pmx->dev, pmx->functions[selector].groups);
> >
> > Red Flag. Please, elaborate.
>
> Thank you for the review.
> I did some research regarding this and now I'm confused. Could you
> please explain to me why it's a red flag?
> IIUC devm_alloc/free functions are the calls to the resource-managed
> alloc/free command, which is bound to the device.
> pinctrl-scmi driver does devm_pinctrl_register_and_init which does
> devres_alloc and doesn't open devres_group like
> scmi_alloc_init_protocol_instance (thanks to Cristian detailed
> explanation).
>
> As was mentioned in Documentation/driver-api/driver-model/devres.rst:
>
> ```
> No matter what, all devres entries are released on driver detach.  On
> release, the associated release function is invoked and then the
> devres entry is freed.
> ```

Precisely. So, why do you intervene in this?

> Also there is devm_pinctrl_get call listed in the managed interfaces.
>
> My understanding is that all resources, bound to the particular device
> will be freed on driver detach.
>
> Also I found some examples of using devm_alloc/free like from dt_node_to_map
> call in pinctrl-simple.c driver.
>
> I agree that I need to implement .remove callback with proper cleanup,
> but why can't I use devm_* here?

You can use devm_*(), but what's the point if you call release
yourself? That's quite a red flag usually shows a bigger issue
(misunderstanding of the objects lifetimes and their interaction).

> Maybe I've misunderstood your point.

Hi Andy,

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Thu, Jul 20, 2023 at 4:40 PM Oleksii Moisieiev
> <Oleksii_Moisieiev@epam.com> wrote:
>> andy.shevchenko@gmail.com writes:
>> > Tue, Jun 06, 2023 at 04:22:28PM +0000, Oleksii Moisieiev kirjoitti:
>
> ...
>
>> >> +    devm_kfree(pmx->dev, pmx->functions[selector].groups);
>> >
>> > Red Flag. Please, elaborate.
>>
>> Thank you for the review.
>> I did some research regarding this and now I'm confused. Could you
>> please explain to me why it's a red flag?
>> IIUC devm_alloc/free functions are the calls to the resource-managed
>> alloc/free command, which is bound to the device.
>> pinctrl-scmi driver does devm_pinctrl_register_and_init which does
>> devres_alloc and doesn't open devres_group like
>> scmi_alloc_init_protocol_instance (thanks to Cristian detailed
>> explanation).
>>
>> As was mentioned in Documentation/driver-api/driver-model/devres.rst:
>>
>> ```
>> No matter what, all devres entries are released on driver detach.  On
>> release, the associated release function is invoked and then the
>> devres entry is freed.
>> ```
>
> Precisely. So, why do you intervene in this?
>
>> Also there is devm_pinctrl_get call listed in the managed interfaces.
>>
>> My understanding is that all resources, bound to the particular device
>> will be freed on driver detach.
>>
>> Also I found some examples of using devm_alloc/free like from dt_node_to_map
>> call in pinctrl-simple.c driver.
>>
>> I agree that I need to implement .remove callback with proper cleanup,
>> but why can't I use devm_* here?
>
> You can use devm_*(), but what's the point if you call release
> yourself? That's quite a red flag usually shows a bigger issue
> (misunderstanding of the objects lifetimes and their interaction).
>

The idea was to follow the way of how pinctrl subsystem manages
resources. It assumes that functions, groups and pins should be
registered using helper functions
pinmux_generic_add_function, pinmux_generic_remove_function,
pinconf_generic_add_group, pinconf_generic_remove_group, etc. Which has
data as the input parameter and should be freed on pinctrl_unregister
call. So pins, groups and functions should live until pinctrl_unregister
is called (from remove callback or from devm_pinctrl_dev_release)

Unfortunately, I can't use this helpers because pins, funcs and groups should
have selector which is understandable by SCMI.

pinctrl_scmi_get_function_groups returns pointer to the allocated
resources to the caller, so I'm allocating managed resources to be sure
that they should be freed on detach.
devm_kfree is called only if scmi_get_group_name call was failed while
converting group_ids to group_names. I count that as a lack of memory,
so I clean allocated groups to give caller a chance to free additional
memory and repeat the call.

So IMHO devm_* fits here good. What do you think?

Sorry for being annoying but I'm trying to understand...