On Sat, Mar 23, 2024 at 08:15:17PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@xxxxxxx>
>
> scmi-pinctrl driver implements pinctrl driver interface and using
> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> SCMI platform firmware, which does the changes in HW.
>
Hi,
> Co-developed-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> ---
> MAINTAINERS | 1 +
> drivers/firmware/arm_scmi/pinctrl.c | 1 +
> drivers/pinctrl/Kconfig | 11 +
> drivers/pinctrl/Makefile | 1 +
> drivers/pinctrl/pinctrl-scmi.c | 564 ++++++++++++++++++++++++++++++++++++
> 5 files changed, 578 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4b511a55101c..d8270ac6651a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21457,6 +21457,7 @@ F: drivers/cpufreq/sc[mp]i-cpufreq.c
> F: drivers/firmware/arm_scmi/
> F: drivers/firmware/arm_scpi.c
> F: drivers/hwmon/scmi-hwmon.c
> +F: drivers/pinctrl/pinctrl-scmi.c
> F: drivers/pmdomain/arm/
> F: drivers/powercap/arm_scmi_powercap.c
> F: drivers/regulator/scmi-regulator.c
> diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
> index 87d9b89cab13..0ecefe855432 100644
> --- a/drivers/firmware/arm_scmi/pinctrl.c
> +++ b/drivers/firmware/arm_scmi/pinctrl.c
> @@ -465,6 +465,7 @@ scmi_pinctrl_settings_conf(const struct scmi_protocol_handle *ph,
>
> tx = t->tx.buf;
> tx->identifier = cpu_to_le32(selector);
> + tx->function_id = cpu_to_le32(0xFFFFFFFF);
As already said....does not belong to this patch
> attributes = FIELD_PREP(GENMASK(1, 0), type) |
> FIELD_PREP(GENMASK(9, 2), chunk);
> tx->attributes = cpu_to_le32(attributes);
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index d45657aa986a..4e6f65cf0e76 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -450,6 +450,17 @@ config PINCTRL_ROCKCHIP
> help
> This support pinctrl and GPIO driver for Rockchip SoCs.
>
> +config PINCTRL_SCMI
> + tristate "Pinctrl driver using SCMI protocol 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.
> +
[snip]
> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
> + unsigned int group,
> + unsigned long *config)
> +{
> + int ret;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> + enum pin_config_param config_type;
> + enum scmi_pinctrl_conf_type type;
> + u32 config_value;
> +
> + if (!config)
> + return -EINVAL;
> +
> + config_type = pinconf_to_config_param(*config);
> + ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
> + if (ret) {
> + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
> + return ret;
> + }
> +
> + ret = pinctrl_ops->settings_get(pmx->ph, group, GROUP_TYPE, 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,
> + struct pinctrl_desc *desc)
> +{
> + struct pinctrl_pin_desc *pins;
> + unsigned int npins;
> + int ret, i;
> +
> + npins = pinctrl_ops->count_get(pmx->ph, PIN_TYPE);
> + pins = devm_kmalloc_array(pmx->dev, npins, sizeof(*pins), GFP_KERNEL);
> + if (!pins)
> + return -ENOMEM;
> +
This is fine only if npins != 0, because on zero npins
devm_kmalloc_array will return ZERO_SIZE_PTR which is NOT-NULL so I
would add a check for !npins and bail out..or use ZERO_OR_NULL_PTR()
to check the return value...if from the previous pinctrl patch you
had decided to bail out at the protocol layer when (nr_pins == 0) and so
you will never get here, please add a comment above that npins cannot be
zero...
> + for (i = 0; i < npins; i++) {
> + pins[i].number = i;
> + ret = pinctrl_ops->name_get(pmx->ph, i, PIN_TYPE, &pins[i].name);
> + if (ret)
> + return dev_err_probe(pmx->dev, ret,
> + "Can't get name for pin %d", i);
> + }
> +
> + desc->npins = npins;
> + desc->pins = pins;
> + dev_dbg(pmx->dev, "got pins %d", npins);
> +
> + return 0;
> +}
> +
> +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 device *dev = &sdev->dev;
> + struct scmi_pinctrl *pmx;
> + const struct scmi_handle *handle;
> + struct scmi_protocol_handle *ph;
> +
> + if (!sdev || !sdev->handle)
if (!sdev->handle) is enough...
> + 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(dev, sizeof(*pmx), GFP_KERNEL);
> + if (!pmx)
> + return -ENOMEM;
> +
> + pmx->ph = ph;
> +
> + pmx->dev = 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);
> + if (ret)
> + return ret;
> +
> + ret = devm_pinctrl_register_and_init(dev, &pmx->pctl_desc, pmx,
> + &pmx->pctldev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register pinctrl\n");
> +
> + pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx->pctldev);
> + pmx->functions = devm_kcalloc(dev, pmx->nr_functions,
> + sizeof(*pmx->functions),
> + GFP_KERNEL);
> + if (!pmx->functions)
> + 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);
> +
> +MODULE_AUTHOR("Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>");
> +MODULE_AUTHOR("Peng Fan <peng.fan@xxxxxxx>");
> +MODULE_DESCRIPTION("ARM SCMI pin controller driver");
> +MODULE_LICENSE("GPL");
>
Thanks,
Cristian
