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

[Shawn Guo]

On Sun, Sep 27, 2026 at 12:35:13AM +0200, Linus Walleij wrote:
> 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.

Ah, nice tip! I will update the commit log.

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

Thanks Linus!

Shawn