From: Cristian Marussi <cristian.marussi@arm.com> To: "Peng Fan (OSS)" <peng.fan@oss.nxp.com> Cc: Sudeep Holla <sudeep.holla@arm.com>, Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Linus Walleij <linus.walleij@linaro.org>, Dan Carpenter <dan.carpenter@linaro.org>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-gpio@vger.kernel.org, Peng Fan <peng.fan@nxp.com>, Oleksii Moisieiev <oleksii_moisieiev@epam.com> Subject: Re: [PATCH v6 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver Date: Thu, 28 Mar 2024 16:46:36 +0000 [thread overview] Message-ID: <ZgWe7ISUoCsdn5LB@pluto> (raw) In-Reply-To: <20240323-pinctrl-scmi-v6-4-a895243257c0@nxp.com> On Sat, Mar 23, 2024 at 08:15:17PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > 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@epam.com> > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > 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@epam.com>"); > +MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>"); > +MODULE_DESCRIPTION("ARM SCMI pin controller driver"); > +MODULE_LICENSE("GPL"); > Thanks, Cristian
WARNING: multiple messages have this Message-ID (diff)
From: Cristian Marussi <cristian.marussi@arm.com> To: "Peng Fan (OSS)" <peng.fan@oss.nxp.com> Cc: Sudeep Holla <sudeep.holla@arm.com>, Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Linus Walleij <linus.walleij@linaro.org>, Dan Carpenter <dan.carpenter@linaro.org>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-gpio@vger.kernel.org, Peng Fan <peng.fan@nxp.com>, Oleksii Moisieiev <oleksii_moisieiev@epam.com> Subject: Re: [PATCH v6 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver Date: Thu, 28 Mar 2024 16:46:36 +0000 [thread overview] Message-ID: <ZgWe7ISUoCsdn5LB@pluto> (raw) In-Reply-To: <20240323-pinctrl-scmi-v6-4-a895243257c0@nxp.com> On Sat, Mar 23, 2024 at 08:15:17PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > 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@epam.com> > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > 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@epam.com>"); > +MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>"); > +MODULE_DESCRIPTION("ARM SCMI pin controller driver"); > +MODULE_LICENSE("GPL"); > Thanks, Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2024-03-28 16:46 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-03-23 12:15 [PATCH v6 0/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support Peng Fan (OSS) 2024-03-23 12:15 ` Peng Fan (OSS) 2024-03-23 12:15 ` [PATCH v6 1/4] firmware: arm_scmi: introduce helper get_max_msg_size Peng Fan (OSS) 2024-03-23 12:15 ` Peng Fan (OSS) 2024-03-23 12:15 ` [PATCH v6 2/4] dt-bindings: firmware: arm,scmi: support pinctrl protocol Peng Fan (OSS) 2024-03-23 12:15 ` Peng Fan (OSS) 2024-03-23 12:15 ` [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support Peng Fan (OSS) 2024-03-23 12:15 ` Peng Fan (OSS) 2024-03-27 10:46 ` Dan Carpenter 2024-03-27 10:46 ` Dan Carpenter 2024-03-27 12:20 ` Cristian Marussi 2024-03-27 12:20 ` Cristian Marussi 2024-03-31 12:47 ` Peng Fan 2024-03-31 12:47 ` Peng Fan 2024-03-28 15:47 ` Cristian Marussi 2024-03-28 15:47 ` Cristian Marussi 2024-03-31 13:28 ` Peng Fan 2024-03-31 13:28 ` Peng Fan 2024-03-29 19:00 ` Andy Shevchenko 2024-03-29 19:00 ` Andy Shevchenko 2024-03-31 13:44 ` Peng Fan 2024-03-31 13:44 ` Peng Fan 2024-04-02 7:48 ` Cristian Marussi 2024-04-02 7:48 ` Cristian Marussi 2024-04-02 13:06 ` Andy Shevchenko 2024-04-02 13:06 ` Andy Shevchenko 2024-04-02 14:01 ` Peng Fan 2024-04-02 14:01 ` Peng Fan 2024-04-02 14:35 ` Peng Fan 2024-04-02 14:35 ` Peng Fan 2024-04-02 15:58 ` Cristian Marussi 2024-04-02 15:58 ` Cristian Marussi 2024-04-02 16:39 ` Andy Shevchenko 2024-04-02 16:39 ` Andy Shevchenko 2024-04-03 8:06 ` Cristian Marussi 2024-04-03 8:06 ` Cristian Marussi 2024-04-03 8:44 ` Andy Shevchenko 2024-04-03 8:44 ` Andy Shevchenko 2024-03-23 12:15 ` [PATCH v6 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver Peng Fan (OSS) 2024-03-23 12:15 ` Peng Fan (OSS) 2024-03-27 11:20 ` Dan Carpenter 2024-03-27 11:20 ` Dan Carpenter 2024-03-28 16:46 ` Cristian Marussi [this message] 2024-03-28 16:46 ` Cristian Marussi 2024-03-29 19:10 ` Andy Shevchenko 2024-03-29 19:10 ` Andy Shevchenko 2024-03-28 16:48 ` [PATCH v6 0/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support Cristian Marussi 2024-03-28 16:48 ` Cristian Marussi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=ZgWe7ISUoCsdn5LB@pluto \ --to=cristian.marussi@arm.com \ --cc=conor+dt@kernel.org \ --cc=dan.carpenter@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=oleksii_moisieiev@epam.com \ --cc=peng.fan@nxp.com \ --cc=peng.fan@oss.nxp.com \ --cc=robh@kernel.org \ --cc=sudeep.holla@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.