Re: [RFC v2 2/3] pinctrl: Implementation of the generic scmi-pinctrl driver

[andy.shevchenko@xxxxxxxxx]

Wed, Apr 26, 2023 at 01:26:37PM +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.

...

> +#include <linux/device.h>
> +#include <linux/err.h>

> +#include <linux/of.h>

I do not see any user of this header. Do you?

> +#include <linux/module.h>
> +#include <linux/seq_file.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 <linux/scmi_protocol.h>
> +#include <linux/slab.h>

Please, move these two to the upper group of the generic headers.

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

Please, use struct pinfunction.

...

> +struct scmi_pinctrl {

> +	struct scmi_pinctrl_funcs *functions;
> +	unsigned int nr_functions;

> +	char **groups;
> +	unsigned int nr_groups;

I'm not sure what is the difference to what "functions" above represent.

> +};

...

> +static void pinctrl_scmi_pin_dbg_show(struct pinctrl_dev *pctldev,
> +				      struct seq_file *s,
> +				      unsigned int offset)
> +{
> +	seq_puts(s, DRV_NAME);
> +}

What is the usefulness of this method?

...

> +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;
> +
> +	config_type = pinconf_to_config_param(*config);
> +
> +	ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE,
> +				      config_type, (u32 *)&config_value);

Endianess issue. This is, while likely working code, still ugly.

> +	if (ret)
> +		return ret;
> +
> +	*config = pinconf_to_config_packed(config_type, config_value);
> +
> +	return 0;
> +}

...

> + err:

err_free.

> +	kfree(pmx->pins);
> +	pmx->nr_pins = 0;
> +
> +	return ret;

...

> +static const struct scmi_device_id scmi_id_table[] = {
> +	{ SCMI_PROTOCOL_PINCTRL, "pinctrl" },

> +	{ },

No comma for the terminator entry.

> +};

...

> +	pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL,
> +						&ph);

Can be on one line.

> +	if (IS_ERR(pinctrl_ops))
> +		return PTR_ERR(pinctrl_ops);

...

> +	if (pmx->nr_functions) {
> +		pmx->functions =
> +			devm_kcalloc(&sdev->dev, pmx->nr_functions,
> +				     sizeof(*pmx->functions),
> +				     GFP_KERNEL);
> +		if (!pmx->functions) {
> +			ret = -ENOMEM;
> +			goto clean;

Interleaving devm_*() with non-devm_*() in such order is not a good idea.

> +		}
> +	}
> +
> +	if (pmx->nr_groups) {
> +		pmx->groups =
> +			devm_kcalloc(&sdev->dev, pmx->nr_groups,
> +				     sizeof(*pmx->groups),
> +				     GFP_KERNEL);
> +		if (!pmx->groups) {
> +			ret = -ENOMEM;
> +			goto clean;
> +		}
> +	}
> +
> +	return pinctrl_enable(pmx->pctldev);
> +
> +clean:

err_free:

> +	if (pmx) {
> +		kfree(pmx->functions);
> +		kfree(pmx->groups);

Ah, this is simply wrong.

> +	}
> +
> +	kfree(pmx);

-- 
With Best Regards,
Andy Shevchenko