Re: [PATCH v2 2/3] pinctrl: scmi: Replace pinctrl ops get group info with generics

[Sudeep Holla]

On Thu, Sep 17, 2026 at 03:55:56PM -0700, Alex Tran wrote:
> During probe, populate the pinctrl device with group info
> so that the generic callbacks can be used to fetch group
> count, name, and pins.
> 
> Signed-off-by: Alex Tran <alex.tran@xxxxxxxxxxxxxxxx>
> ---
>  drivers/pinctrl/pinctrl-scmi.c | 81 +++++++++++++++++++++++-------------------
>  1 file changed, 45 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
> index 1d85a16f300d..c93d61dfa282 100644
> --- a/drivers/pinctrl/pinctrl-scmi.c
> +++ b/drivers/pinctrl/pinctrl-scmi.c
> @@ -40,43 +40,10 @@ struct scmi_pinctrl {
>  	struct pinctrl_desc pctl_desc;
>  };
>  
> -static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev)
> -{
> -	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> -
> -	return pinctrl_ops->count_get(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 = pinctrl_dev_get_drvdata(pctldev);
> -
> -	ret = pinctrl_ops->name_get(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 = pinctrl_dev_get_drvdata(pctldev);
> -
> -	return pinctrl_ops->group_pins_get(pmx->ph, selector, pins, num_pins);
> -}
> -
>  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,
> +	.get_groups_count = pinctrl_generic_get_group_count,
> +	.get_group_name = pinctrl_generic_get_group_name,
> +	.get_group_pins = pinctrl_generic_get_group_pins,
>  #ifdef CONFIG_OF
>  	.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
>  	.dt_free_map = pinconf_generic_dt_free_map,
> @@ -460,6 +427,44 @@ static int pinctrl_scmi_get_functions(struct scmi_pinctrl *pmx)
>  	return 0;
>  }
>  
> +static int pinctrl_scmi_get_groups(struct scmi_pinctrl *pmx)
> +{
> +	unsigned int nr_groups, nr_pins, i;
> +	const unsigned int *pins;
> +	const char *gname;
> +	int ret;
> +
> +	nr_groups = pinctrl_ops->count_get(pmx->ph, GROUP_TYPE);
> +
> +	for (i = 0; i < nr_groups; i++) {
> +		ret = pinctrl_ops->name_get(pmx->ph, i, GROUP_TYPE, &gname);
> +		if (ret)
> +			return ret;
> +
> +		ret = pinctrl_ops->group_pins_get(pmx->ph, i, &pins,
> +						  &nr_pins);
> +		if (ret)
> +			return ret;
> +
> +		if (!nr_pins)
> +			dev_warn(pmx->dev,
> +				 "Group %s contains no pins\n", gname);

I think sashiko warning on the above can be ignored as it is not true.

> +
> +		ret = pinctrl_generic_add_group(pmx->pctldev, gname, pins,
> +						nr_pins, NULL);
> +		if (ret < 0)
> +			return ret;

Similar question about use of pinctrl_generic_remove_group(), but I did
check the driver and absence of driver remove() callback is the hint I
believe and devm_* takes care of cleanup I assume. Sashiko also points
at some possible race with devm_* cleanup. I will leave that to you as
I don't fully understand how devm_* cleanup works in pinctl.

Anyways, with fix if needed or without if not required,

Acked-by: Sudeep Holla <sudeep.holla@xxxxxxxxxx>

-- 
Regards,
Sudeep