Re: [PATCH v2 2/2] pinctrl: qcom: milos-lpass-lpi: Add Eliza LPASS LPI TLMM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling pinctrl configurations destructively overwrites unrelated pin state with default values.
--

--- Patch [2]: [PATCH v2 2/2] pinctrl: qcom: milos-lpass-lpi: Add Eliza LPASS LPI TLMM ---
commit 582da1f0b8240dfeb6e4051b30e4e3d9b11be196
Author: Ravi Hothi <ravi.hothi@oss.qualcomm.com>

pinctrl: qcom: milos-lpass-lpi: Add Eliza LPASS LPI TLMM

This commit adds variant data for the Eliza SoC to the Milos LPASS LPI
driver. It updates the slew offsets and adds the LPI_FLAG_SLEW_RATE_SAME_REG
flag, reusing existing pin descriptors and function tables.

> diff --git a/drivers/pinctrl/qcom/pinctrl-milos-lpass-lpi.c b/drivers/pinctrl/qcom/pinctrl-milos-lpass-lpi.c
> --- a/drivers/pinctrl/qcom/pinctrl-milos-lpass-lpi.c
> +++ b/drivers/pinctrl/qcom/pinctrl-milos-lpass-lpi.c

[ ... ]

> @@ -196,8 +223,21 @@ static const struct lpi_pinctrl_variant_data milos_lpi_data = {
>  	.nfunctions = ARRAY_SIZE(milos_functions),
>  };
>  
> +static const struct lpi_pinctrl_variant_data eliza_lpi_data = {
> +	.pins = milos_lpi_pins,
> +	.npins = ARRAY_SIZE(milos_lpi_pins),
> +	.groups = eliza_groups,
> +	.ngroups = ARRAY_SIZE(eliza_groups),
> +	.functions = milos_functions,
> +	.nfunctions = ARRAY_SIZE(milos_functions),
> +	.flags = LPI_FLAG_SLEW_RATE_SAME_REG,
> +};
> +

[Severity: High]
This is a pre-existing issue, but will adding these new pin groups trigger
destructive overwrites in the core lpi_config_set()?

When the pinctrl core or GPIO subsystem invokes lpi_config_set() to change
a pin's configuration (like direction or value), it appears to destructively
overwrite orthogonal pin states with default values.

In drivers/pinctrl/qcom/pinctrl-lpass-lpi.c:lpi_config_set(), the local
variables are initialized to default values:

    unsigned int param, arg, pullup = LPI_GPIO_BIAS_DISABLE, strength = 2;
    bool value, output_enabled = false;

After processing the requested configs, the function reads the current
register but unconditionally overwrites these fields:

    lpi_gpio_read_reg(pctrl, group, LPI_GPIO_CFG_REG, &val);

    u32p_replace_bits(&val, pullup, LPI_GPIO_PULL_MASK);
    u32p_replace_bits(&val, LPI_GPIO_DS_TO_VAL(strength),
                      LPI_GPIO_OUT_STRENGTH_MASK);
    u32p_replace_bits(&val, output_enabled, LPI_GPIO_OE_MASK);

Does this mean merely setting a GPIO output value will reset its drive
strength to 2mA and disable any pull bias?

Similarly, would applying a pull bias on an output pin inadvertently clear
LPI_GPIO_OE_MASK and convert it into an input?

>  static const struct of_device_id lpi_pinctrl_of_match[] = {
>  	{
> +		.compatible = "qcom,eliza-lpass-lpi-pinctrl",
> +		.data = &eliza_lpi_data,
> +	}, {
>  		.compatible = "qcom,milos-lpass-lpi-pinctrl",
>  		.data = &milos_lpi_data,
>  	},

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713121518.2724474-1-ravi.hothi@oss.qualcomm.com?part=2