Re: [PATCH 3/4] pinctrl: qcom: spmi-gpio: Rearchitect for flexible group support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect registration sequence breaks pinctrl hogs and exposes an uninitialized controller.
- [Low] Missing Kconfig dependencies for generic pinctrl APIs.
--

commit a1bdf02d93e7a07ac0395bf90452b7604f1cb0ac
Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>

pinctrl: qcom: spmi-gpio: Rearchitect for flexible group support

This commit transitions the spmi-gpio pinctrl driver to use generic pinctrl
groups and functions instead of treating every individual pin as a separate,
fully-featured group.

> diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> index cdd61dae74cf9..f159c56784b4f 100644
> --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
[ ... ]
> @@ -253,139 +254,124 @@ static int pmic_gpio_write(struct pmic_gpio_state *state,
>  	return ret;
>  }
>  
> -static int pmic_gpio_get_groups_count(struct pinctrl_dev *pctldev)
> -{
[ ... ]
>  static const struct pinctrl_ops pmic_gpio_pinctrl_ops = {
> -	.get_groups_count	= pmic_gpio_get_groups_count,
> -	.get_group_name		= pmic_gpio_get_group_name,
> -	.get_group_pins		= pmic_gpio_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,
>  	.dt_node_to_map		= pinconf_generic_dt_node_to_map_group,
>  	.dt_free_map		= pinctrl_utils_free_map,
>  };

[Severity: Low]
The driver was migrated to use generic pinctrl APIs, but does the Kconfig for
CONFIG_PINCTRL_QCOM_SPMI_PMIC select GENERIC_PINCTRL_GROUPS and
GENERIC_PINMUX_FUNCTIONS?

If these are not enabled by another driver during a randconfig build, could
this lead to unresolved symbol errors when using these generic APIs?

[ ... ]
> @@ -1129,11 +1236,11 @@ static int pmic_gpio_probe(struct platform_device *pdev)
[ ... ]
>  	state->chip = pmic_gpio_gpio_template;
>  	state->chip.parent = dev;
>  	state->chip.base = -1;
>  	state->chip.ngpio = npins;
>  	state->chip.label = dev_name(dev);
>  	state->chip.of_gpio_n_cells = 2;
>  	state->chip.can_sleep = false;
>  
>  	state->ctrl = devm_pinctrl_register(dev, pctrldesc, state);
>  	if (IS_ERR(state->ctrl))
>  		return PTR_ERR(state->ctrl);
>  
> +	/* Register pin groups - each GPIO is a group for standard functions */
> +	for (i = 0; i < npins; i++) {
> +		ret = pinctrl_generic_add_group(state->ctrl,

[Severity: High]
Does registering the pinctrl device before adding the generic groups and
functions break pinctrl hogs?

devm_pinctrl_register() internally invokes pinctrl_enable(), which attempts
to immediately parse and claim pinctrl hogs from the device tree via
pinctrl_claim_hogs(). Because the generic group and function radix trees are
empty at this exact moment, pinmux_func_name_to_selector() and
pinctrl_get_group_selector() will fail, silently dropping the hardware hogs.

Could this also expose the pinctrl device to concurrent consumers before it
is fully populated? Should the driver use devm_pinctrl_register_and_init(),
populate its generic structures, and then explicitly call pinctrl_enable()?

> +						pmic_gpio_groups[i],
> +						&pindesc[i].number, 1, NULL);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to register group %s\n",
> +				pmic_gpio_groups[i]);
> +			return ret;
> +		}
> +	}
> +
> +	/* Register standard functions - all GPIOs support these */
> +	for (i = 0; i < ARRAY_SIZE(pmic_gpio_functions); i++) {
> +		ret = pinmux_generic_add_function(state->ctrl,
> +						  pmic_gpio_functions[i],
> +						  pmic_gpio_groups, npins, NULL);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to register function %s\n",
> +				pmic_gpio_functions[i]);
> +			return ret;
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527-pinctrl-level-shifter-v1-0-1965461d0a7c@oss.qualcomm.com?part=3