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

[andy . shevchenko]

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;

-- 
With Best Regards,
Andy Shevchenko