Re: [PATCH v1 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] lpi_config_set() unconditionally overwrites pull-up, drive strength, and output-enable configurations with default values if they are not explicitly specified in the configs array.
--

commit c207cbbbbf4fca2f1793d2b498400b3981b0254c
Author: Ravi Hothi <ravi.hothi@oss.qualcomm.com>

    pinctrl: qcom: milos-lpass-lpi: Add Eliza LPASS LPI TLMM
    
    Eliza SoC has the same LPASS LPI pin mux functions as Milos but the
    slew rate control is in the same GPIO config register rather than a
    separate register. Add a new variant data struct with updated slew
    offsets and LPI_FLAG_SLEW_RATE_SAME_REG flag, reusing the existing
    pin descriptors and function table from Milos.

> diff --git a/drivers/pinctrl/qcom/pinctrl-milos-lpass-lpi.c b/drivers/pinctrl/qcom/pinctrl-milos-lpass-lpi.c
> index 72b8ffd97860b..cb4934cd6f757 100644
> --- 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 isn't a bug introduced by this patch, but while reviewing the new slew
rate behavior, I noticed an issue in how lpi_config_set() handles the pin
configurations in drivers/pinctrl/qcom/pinctrl-lpass-lpi.c.

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

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

Later in the same function, these defaults are unconditionally applied to the
register value instead of preserving the current hardware state:

    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);

    lpi_gpio_write_reg(pctrl, group, LPI_GPIO_CFG_REG, val);

Does this code accidentally overwrite unrequested settings?

For instance, if a driver updates a GPIO value or if a single configuration 
such as the slew rate for Eliza is applied, the current register state is 
read, but pull-up, drive strength, and output enable fields are unconditionally
replaced with the initialized defaults.

Could this lead to silent clobbering of existing configurations, resulting in
floating pins or output being unintentionally disabled?

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