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

[Shawn Guo <shengchao.guo@oss.qualcomm.com>]

On Thu, Sep 24, 2026 at 11:23:14AM +0800, Shawn Guo 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.  Keep it enabled for those pads, and key
> pmic_gpio_get_direction() off output_enabled so they still read back as
> outputs.  gpiolib applies PIN_CONFIG_DRIVE_OPEN_DRAIN before calling
> .direction_output(), so pad->buffer_type is up to date there.
> 
> 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 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 | 35 ++++++++++++++++++------
>  1 file changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> index f6dc43e27b38..dd9c212da6ee 100644
> --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> @@ -734,29 +734,48 @@ 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];
> +	unsigned int nconfigs = 0;
>  
> -	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.  Leave the
> +	 * input buffer enabled for those, so that pmic_gpio_get() reports what
> +	 * is on the wire rather than the value last written.  gpiolib applies
> +	 * PIN_CONFIG_DRIVE_OPEN_DRAIN before calling this, so buffer_type is
> +	 * already up to date here.
> +	 */
> +	if (pad->buffer_type == PMIC_GPIO_OUT_BUF_CMOS)
> +		configs[nconfigs++] =
> +			pinconf_to_config_packed(PIN_CONFIG_INPUT_ENABLE, 0);
>  
> -	return pmic_gpio_config_set(state->ctrl, pin, &config, 1);
> +	configs[nconfigs++] = pinconf_to_config_packed(PIN_CONFIG_LEVEL, val);

Sashiko has a valid comment on this (again, damn). I will send v4.

Shawn

> +
> +	return pmic_gpio_config_set(state->ctrl, pin, configs, nconfigs);
>  }
>  
>  static int pmic_gpio_get(struct gpio_chip *chip, unsigned pin)
> -- 
> 2.43.0
>