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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

Hi Cristian,

Please see below.

On 05.05.23 23:01, Cristian Marussi wrote:
> On Wed, Apr 26, 2023 at 01:26:37PM +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.
>>
>> 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.
>>
> 
> Hi Oleksii,
> 
> just a few remarks down below.
> 
>> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
>> ---
>>   MAINTAINERS                    |   1 +
>>   drivers/pinctrl/Kconfig        |   9 +
>>   drivers/pinctrl/Makefile       |   1 +
>>   drivers/pinctrl/pinctrl-scmi.c | 578 +++++++++++++++++++++++++++++++++
>>   4 files changed, 589 insertions(+)
>>   create mode 100644 drivers/pinctrl/pinctrl-scmi.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0d251ebac437..ba9e3aea6176 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -20322,6 +20322,7 @@ M:	Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
>>   L:	linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>>   S:	Maintained
>>   F:	drivers/firmware/arm_scmi/pinctrl.c
>> +F:	drivers/pinctrl/pinctrl-scmi.c
>>   
>>   SYSTEM RESET/SHUTDOWN DRIVERS
>>   M:	Sebastian Reichel <sre@xxxxxxxxxx>
>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
>> index dcb53c4a9584..16bf2c67f095 100644
>> --- a/drivers/pinctrl/Kconfig
>> +++ b/drivers/pinctrl/Kconfig
>> @@ -552,4 +552,13 @@ source "drivers/pinctrl/uniphier/Kconfig"
>>   source "drivers/pinctrl/visconti/Kconfig"
>>   source "drivers/pinctrl/vt8500/Kconfig"
>>   
>> +config PINCTRL_SCMI
>> +	bool "Pinctrl driver controlled via SCMI interface"
> 
> Why not a tristate ? The core SCMI stack can be compiled as module with
> all the protocols...at least for testing is useful to have this driver
> as M.
> 

Yes, thanks. Will fix in v3.

>> +	depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
>> +	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 d5939840bb2a..21366db4f4f4 100644
>> --- a/drivers/pinctrl/Makefile
>> +++ b/drivers/pinctrl/Makefile
>> @@ -51,6 +51,7 @@ obj-$(CONFIG_PINCTRL_TB10X)	+= pinctrl-tb10x.o
>>   obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o
>>   obj-$(CONFIG_PINCTRL_ZYNQMP)	+= pinctrl-zynqmp.o
>>   obj-$(CONFIG_PINCTRL_ZYNQ)	+= pinctrl-zynq.o
>> +obj-$(CONFIG_PINCTRL_SCMI)	+= pinctrl-scmi.o
>>   
>>   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..8401db1d030b
>> --- /dev/null
>> +++ b/drivers/pinctrl/pinctrl-scmi.c
>> @@ -0,0 +1,578 @@
>> +// 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/of.h>
>> +#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>
>> +
>> +#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;
>> +
>> +	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;
>> +
>> +	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;
>> +
>> +	return pinctrl_ops->get_group_pins(pmx->ph, selector,
>> +					   pins, num_pins);
>> +}
>> +
>> +static void pinctrl_scmi_pin_dbg_show(struct pinctrl_dev *pctldev,
>> +				      struct seq_file *s,
>> +				      unsigned int offset)
>> +{
>> +	seq_puts(s, DRV_NAME);
>> +}
>> +
>> +#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,
>> +	.pin_dbg_show = pinctrl_scmi_pin_dbg_show,
>> +#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;
>> +
>> +	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;
>> +
>> +	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;
>> +
>> +	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:
>> +	kfree(pmx->functions[selector].groups);
> 
> devm_kfree ?
> 
Thanks, fixed.
>> +
>> +	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;
>> +
>> +	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;
>> +
>> +	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;
>> +
>> +	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;
>> +
>> +	config_type = pinconf_to_config_param(*config);
>> +
>> +	ret = pinctrl_ops->get_config(pmx->ph, _pin, PIN_TYPE, config_type,
>> +				      (u32 *)&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;
>> +
>> +	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;
>> +
>> +	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;
>> +
>> +	config_type = pinconf_to_config_param(*config);
>> +
>> +	ret = pinctrl_ops->get_config(pmx->ph, _pin, GROUP_TYPE,
>> +				      config_type, (u32 *)&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;
>> +
>> +	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;
>> +		}
>> +	}
>> +
>> +	*pins = pmx->pins;
>> +	dev_dbg(pmx->dev, "got pins %d", *nr_pins);
>> +
>> +	return 0;
>> + err:
>> +	kfree(pmx->pins);
> devm_kfree ? but anyway when this fails it will trigger the _probe to
> fail so all devres will be released..so not needed probably at the end.
> 
>> +	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)
>> +		goto clean;
>> +
>> +	ret = devm_pinctrl_register_and_init(&sdev->dev, &pmx->pctl_desc, pmx,
>> +					     &pmx->pctldev);
>> +	if (ret) {
>> +		dev_err(&sdev->dev, "could not register: %i\n", ret);
>> +		goto clean;
>> +	}
>> +
>> +	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) {
>> +			ret = -ENOMEM;
>> +			goto clean;
>> +		}
>> +	}
>> +
>> +	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:
>> +	if (pmx) {
>> +		kfree(pmx->functions);
>> +		kfree(pmx->groups);
>> +	}
>> +
>> +	kfree(pmx);
> 
> All of these are devres allocated...it does not seem to me that they
> require explicit freeing on the _probe() failure path.
> (indeed you dont need even a .remove function)
> 

Removed this block.

> Thanks,
> Cristian