| Message ID | 20260518-fix-set-value-glitch-v1-1-d350732dc934@amlogic.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: meson: amlogic-a4: fix gpio output glitch | expand |
On 5/18/26 10:26, Xianwei Zhao via B4 Relay wrote: > From: Xianwei Zhao <xianwei.zhao@amlogic.com> > > When the system transitions from bootloader to kernel, the GPIO is > expected to keep driving high. > > However, the Linux kernel first configures the pin direction and then > sets the output value. This may cause a brief low-level glitch on the > GPIO line, which can be problematic for regulator control. > > By configuring the output value before switching the pin direction to > output, the glitch can be avoided. > > This commit fixes the issue by swapping the configuration order. > > Fixes: 6e9be3abb78c ("pinctrl: Add driver support for Amlogic SoCs") > Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> > --- > fix one issue when set gpio line high. > --- > drivers/pinctrl/meson/pinctrl-amlogic-a4.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/pinctrl/meson/pinctrl-amlogic-a4.c b/drivers/pinctrl/meson/pinctrl-amlogic-a4.c > index 35d27626a336..1bd58fbbd26a 100644 > --- a/drivers/pinctrl/meson/pinctrl-amlogic-a4.c > +++ b/drivers/pinctrl/meson/pinctrl-amlogic-a4.c > @@ -548,11 +548,11 @@ static int aml_pinconf_set_output_drive(struct aml_pinctrl *info, > { > int ret; > > - ret = aml_pinconf_set_output(info, pin, true); > + ret = aml_pinconf_set_drive(info, pin, high); > if (ret) > return ret; > > - return aml_pinconf_set_drive(info, pin, high); > + return aml_pinconf_set_output(info, pin, true); > } > > static int aml_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin, > @@ -921,15 +921,14 @@ static int aml_gpio_direction_output(struct gpio_chip *chip, unsigned int gpio, > unsigned int bit, reg; > int ret; > > - aml_gpio_calc_reg_and_bit(bank, AML_REG_DIR, gpio, ®, &bit); > - ret = regmap_update_bits(bank->reg_gpio, reg, BIT(bit), 0); > + aml_gpio_calc_reg_and_bit(bank, AML_REG_OUT, gpio, ®, &bit); > + ret = regmap_update_bits(bank->reg_gpio, reg, BIT(bit), > + value ? BIT(bit) : 0); > if (ret < 0) > return ret; > > - aml_gpio_calc_reg_and_bit(bank, AML_REG_OUT, gpio, ®, &bit); > - > - return regmap_update_bits(bank->reg_gpio, reg, BIT(bit), > - value ? BIT(bit) : 0); > + aml_gpio_calc_reg_and_bit(bank, AML_REG_DIR, gpio, ®, &bit); > + return regmap_update_bits(bank->reg_gpio, reg, BIT(bit), 0); > } > > static int aml_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value) > > --- > base-commit: 73d4991a6949eedb51e442d4e81415017d85975b > change-id: 20260518-fix-set-value-glitch-f43cd366c295 > > Best regards, Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org> Thanks, Neil
On Mon, May 18, 2026 at 10:26 AM Xianwei Zhao via B4 Relay <devnull+xianwei.zhao.amlogic.com@kernel.org> wrote: > From: Xianwei Zhao <xianwei.zhao@amlogic.com> > > When the system transitions from bootloader to kernel, the GPIO is > expected to keep driving high. > > However, the Linux kernel first configures the pin direction and then > sets the output value. This may cause a brief low-level glitch on the > GPIO line, which can be problematic for regulator control. > > By configuring the output value before switching the pin direction to > output, the glitch can be avoided. > > This commit fixes the issue by swapping the configuration order. > > Fixes: 6e9be3abb78c ("pinctrl: Add driver support for Amlogic SoCs") > Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> Is this a regression? I.e. does it cause problems on a supported system with mainline? Linus (the big penguin) is unhappy with too many non-critical fixes so I wanna check this before this goes into fixes. Yours, Linus Walleij
Hi Linus,
Thanks for your review.
On 2026/5/25 16:34, Linus Walleij wrote:
> On Mon, May 18, 2026 at 10:26 AM Xianwei Zhao via B4 Relay
> <devnull+xianwei.zhao.amlogic.com@kernel.org> wrote:
>
>> From: Xianwei Zhao<xianwei.zhao@amlogic.com>
>>
>> When the system transitions from bootloader to kernel, the GPIO is
>> expected to keep driving high.
>>
>> However, the Linux kernel first configures the pin direction and then
>> sets the output value. This may cause a brief low-level glitch on the
>> GPIO line, which can be problematic for regulator control.
>>
>> By configuring the output value before switching the pin direction to
>> output, the glitch can be avoided.
>>
>> This commit fixes the issue by swapping the configuration order.
>>
>> Fixes: 6e9be3abb78c ("pinctrl: Add driver support for Amlogic SoCs")
>> Signed-off-by: Xianwei Zhao<xianwei.zhao@amlogic.com>
> Is this a regression? I.e. does it cause problems on a supported
> system with mainline?
>
> Linus (the big penguin) is unhappy with too many non-critical fixes
> so I wanna check this before this goes into fixes.
>
The issue only occurs when the critical power supply uses GPIO control.
Otherwise, it is not significant.
> Yours,
> Linus Walleij
On Mon, May 18, 2026 at 10:26 AM Xianwei Zhao via B4 Relay <devnull+xianwei.zhao.amlogic.com@kernel.org> wrote: > From: Xianwei Zhao <xianwei.zhao@amlogic.com> > > When the system transitions from bootloader to kernel, the GPIO is > expected to keep driving high. > > However, the Linux kernel first configures the pin direction and then > sets the output value. This may cause a brief low-level glitch on the > GPIO line, which can be problematic for regulator control. > > By configuring the output value before switching the pin direction to > output, the glitch can be avoided. > > This commit fixes the issue by swapping the configuration order. > > Fixes: 6e9be3abb78c ("pinctrl: Add driver support for Amlogic SoCs") > Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> Patch applied as non-critical fix based on my gut feeling. Yours, Linus Walleij
diff --git a/drivers/pinctrl/meson/pinctrl-amlogic-a4.c b/drivers/pinctrl/meson/pinctrl-amlogic-a4.c index 35d27626a336..1bd58fbbd26a 100644 --- a/drivers/pinctrl/meson/pinctrl-amlogic-a4.c +++ b/drivers/pinctrl/meson/pinctrl-amlogic-a4.c @@ -548,11 +548,11 @@ static int aml_pinconf_set_output_drive(struct aml_pinctrl *info, { int ret; - ret = aml_pinconf_set_output(info, pin, true); + ret = aml_pinconf_set_drive(info, pin, high); if (ret) return ret; - return aml_pinconf_set_drive(info, pin, high); + return aml_pinconf_set_output(info, pin, true); } static int aml_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin, @@ -921,15 +921,14 @@ static int aml_gpio_direction_output(struct gpio_chip *chip, unsigned int gpio, unsigned int bit, reg; int ret; - aml_gpio_calc_reg_and_bit(bank, AML_REG_DIR, gpio, ®, &bit); - ret = regmap_update_bits(bank->reg_gpio, reg, BIT(bit), 0); + aml_gpio_calc_reg_and_bit(bank, AML_REG_OUT, gpio, ®, &bit); + ret = regmap_update_bits(bank->reg_gpio, reg, BIT(bit), + value ? BIT(bit) : 0); if (ret < 0) return ret; - aml_gpio_calc_reg_and_bit(bank, AML_REG_OUT, gpio, ®, &bit); - - return regmap_update_bits(bank->reg_gpio, reg, BIT(bit), - value ? BIT(bit) : 0); + aml_gpio_calc_reg_and_bit(bank, AML_REG_DIR, gpio, ®, &bit); + return regmap_update_bits(bank->reg_gpio, reg, BIT(bit), 0); } static int aml_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value)