Re: [PATCH v4] pinctrl: qcom: spmi-gpio: make direction changes exclusive

[Linus Walleij]

Hi Shawn,

thanks for your patch!

On Thu, Sep 24, 2026 at 9:03 AM Shawn Guo
<shengchao.guo@xxxxxxxxxxxxxxxx> wrote:

> pmic_gpio_populate() seeds pad->input_enabled and pad->output_enabled from
> the hardware MODE_CTL register, so a pad left in DIGITAL_INPUT or
> DIGITAL_INPUT_OUTPUT mode by the bootloader starts out with the input
> buffer enabled.  Neither direction callback clears the opposite buffer:
> .direction_output() only packs PIN_CONFIG_LEVEL, which sets
> output_enabled, and .direction_input() only packs PIN_CONFIG_INPUT_ENABLE,
> which sets input_enabled.  Requesting either direction on such a pad
> therefore programs MODE_DIGITAL_INPUT_OUTPUT rather than the requested
> direction.
>
> That silently breaks both directions.  After gpiod_direction_input() the
> pad keeps driving the line.  And after gpiod_direction_output()
> pmic_gpio_get_direction() still reports GPIO_LINE_DIRECTION_IN, because it
> cannot tell plain input from input+output, which makes gpiolib consider
> the line an input while the driver is driving it.  On a board where
> several regulator-fixed nodes share one PMIC GPIO the shared GPIO proxy
> reads that direction back and rejects every consumer after the first:
>
>   reg-fixed-voltage regulator-wcn-core-vm-1p35: setup of GPIO (default) failed: -1
>   reg-fixed-voltage regulator-wcn-core-vm-1p35: error -EPERM: can't get GPIO
>
> Pack the opposite buffer's PIN_CONFIG_*_ENABLE along with the requested
> direction so that the resulting MODE_CTL is DIGITAL_INPUT or
> DIGITAL_OUTPUT, never both.  pmic_gpio_config_set() programs the registers
> once after walking all configs, so this stays a single register write.
>
> Open-drain and open-source outputs are the exception: such a pad only ever
> drives one rail, so DIGITAL_INPUT_OUTPUT is physically correct for it, and
> pmic_gpio_get() needs the input buffer to sample the line rather than
> return the value last written.  So .direction_output() programs the input
> buffer from pad->buffer_type in either case, rather than only clearing it
> for a CMOS pad: the buffer may have been left off by the bootloader or by
> an earlier direction change made with a different buffer type, and an
> open-drain pad that keeps it off would fall back to reporting the value
> last written.  gpiolib applies PIN_CONFIG_DRIVE_OPEN_DRAIN before calling
> .direction_output(), so pad->buffer_type is up to date there.  Key
> pmic_gpio_get_direction() off output_enabled so that these pads still read
> back as outputs.
>
> An input must not drive the line whatever the buffer type, so
> .direction_input() clears output_enabled unconditionally.
>
> Pads that are genuinely bidirectional can still be described that way
> through pinconf, which is the interface that has always been able to
> express it; the gpiolib direction callbacks now mean what gpiolib says
> they mean.
>
> Fixes: 263447532463 ("pinctrl: qcom: spmi-gpio: implement .get_direction()")
> Assisted-by: LLM
> Signed-off-by: Shawn Guo <shengchao.guo@xxxxxxxxxxxxxxxx>

It's a bit in the style of LLM:s to write overly verbose commit
logs, can you put this into your AGENTS.md file:

- Use terse commit messages.

>  static int pmic_gpio_direction_output(struct gpio_chip *chip,
>                                       unsigned pin, int val)
>  {
>         struct pmic_gpio_state *state = gpiochip_get_data(chip);
> -       unsigned long config;
> +       struct pmic_gpio_pad *pad = state->ctrl->desc->pins[pin].drv_data;
> +       unsigned long configs[2];
>
> -       config = pinconf_to_config_packed(PIN_CONFIG_LEVEL, val);
> +       /*
> +        * An open-drain or open-source pad only ever drives one rail, so the
> +        * line can still be sampled while the pad is an output.  Keep the input
> +        * buffer enabled for those, so that pmic_gpio_get() reports what is on
> +        * the wire rather than the value last written, and disable it for a
> +        * CMOS pad, so that the pad ends up in DIGITAL_OUTPUT rather than
> +        * DIGITAL_INPUT_OUTPUT.  Program it either way, as the buffer may have
> +        * been left in the opposite state by the bootloader or by an earlier
> +        * direction change with a different buffer type.  gpiolib applies
> +        * PIN_CONFIG_DRIVE_OPEN_DRAIN before calling this, so buffer_type is
> +        * already up to date here.
> +        */
> +       configs[0] = pinconf_to_config_packed(PIN_CONFIG_INPUT_ENABLE,
> +                                            pad->buffer_type != PMIC_GPIO_OUT_BUF_CMOS);
> +       configs[1] = pinconf_to_config_packed(PIN_CONFIG_LEVEL, val);

It is also typical for LLMs to insert verbose comments like that.

BUT! I like the comment, so it can stay!

Reviewed-by: Linus Walleij <linusw@xxxxxxxxxx>

Yours,
Linus Walleij