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

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

[Shawn Guo]

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@oss.qualcomm.com>
---
Changes for v4:
- .direction_output() now programs PIN_CONFIG_INPUT_ENABLE unconditionally
  from pad->buffer_type, instead of only clearing it for a CMOS pad,
  so an open-drain or open-source pad whose input buffer was already off
  gets it re-enabled rather than falling back to reporting the value last
  written.
- Link to v3: https://lore.kernel.org/all/20260924032314.948638-1-shengchao.guo@oss.qualcomm.com/

Changes for v3:
- .direction_output() clears PIN_CONFIG_INPUT_ENABLE only for
  PMIC_GPIO_OUT_BUF_CMOS pads; open-drain/open-source keep the input
  buffer so pmic_gpio_get() samples the wire.
- pmic_gpio_get_direction() now keys on output_enabled instead of
  input_enabled - required, or an open-drain output reports IN again and
  the -EPERM regression returns.
- Link to v2: https://lore.kernel.org/all/20260922065908.477523-1-shengchao.guo@oss.qualcomm.com/

Changes for v2:
- Drop GPIO shared-proxy patch
- Update Fixes tag (Thanks Neil!)
- Link to v1: https://lore.kernel.org/all/20260915014447.282121-1-shengchao.guo@oss.qualcomm.com/

 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 37 +++++++++++++++++++-----
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index f6dc43e27b38..9163bb14d703 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -734,29 +734,50 @@ static int pmic_gpio_get_direction(struct gpio_chip *chip, unsigned pin)
 	    (!pad->input_enabled && !pad->output_enabled))
 		return -EINVAL;
 
-	/* Make sure the state is aligned on what pmic_gpio_get() returns */
-	return pad->input_enabled ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
+	/*
+	 * An open-drain or open-source pad keeps its input buffer enabled while
+	 * driving, so the output buffer is what tells the direction.
+	 */
+	return pad->output_enabled ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
 }
 
 static int pmic_gpio_direction_input(struct gpio_chip *chip, unsigned pin)
 {
 	struct pmic_gpio_state *state = gpiochip_get_data(chip);
-	unsigned long config;
+	unsigned long configs[2];
 
-	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_ENABLE, 1);
+	configs[0] = pinconf_to_config_packed(PIN_CONFIG_OUTPUT_ENABLE, 0);
+	configs[1] = pinconf_to_config_packed(PIN_CONFIG_INPUT_ENABLE, 1);
 
-	return pmic_gpio_config_set(state->ctrl, pin, &config, 1);
+	return pmic_gpio_config_set(state->ctrl, pin, configs,
+				    ARRAY_SIZE(configs));
 }
 
 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);
 
-	return pmic_gpio_config_set(state->ctrl, pin, &config, 1);
+	return pmic_gpio_config_set(state->ctrl, pin, configs,
+				    ARRAY_SIZE(configs));
 }
 
 static int pmic_gpio_get(struct gpio_chip *chip, unsigned pin)
-- 
2.43.0

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@oss.qualcomm.com> 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@oss.qualcomm.com>

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@kernel.org>

Yours,
Linus Walleij

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@oss.qualcomm.com> 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@oss.qualcomm.com>
> 
> 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@kernel.org>

Thanks Linus!

Shawn