On Wed, Jan 25, 2023 at 12:39 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> PIN_CONFIG_BIAS_PULL_UP is documented as follows:
>
> @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
> impedance to VDD). If the argument is != 0 pull-up is enabled,
> if it is 0, pull-up is total, i.e. the pin is connected to VDD.
>
> This patch fixes 2 issues with how the AMD pinctrl code was handling this:
>
> 1. amd_pinconf_set() was setting the PULL_UP_ENABLE bit as follows:
> pin_reg &= ~BIT(PULL_UP_ENABLE_OFF);
> pin_reg |= ((arg>>1) & BIT(0)) << PULL_UP_ENABLE_OFF;
> When called from gpio_set_bias() for ACPI enumerated GPIOs arg == 1,
> so the pull-up enable bit would be cleared instead of being set.
> It seems unnecessary to say that this is BAD.
>
> There is no real convention for the meaning of arg other then that
than
> a value != 0 means the pull-up should be enabled (which was being
> violated here). Looking at other drivers the Intel pinctrl drivers
> all treat 1 (as used by gpio_set_bias()) as indictating that the
indicating
> driver should pick the pull-up strength; and all other values are
> interpreted as the amount of ohm with which to pull-up, with non
> supported values being rejected with -EINVAL.
>
> This patch changes the AMD pinctrl code to match this behavior so
> that the behavior of all x86 pinctrl drivers is consistent.
>
> 2. arg == 0 does not mean that the pull-up/-down is disabled as the
> old code was assuming. Rather it means that the "pull-up is total,
> i.e. the pin is connected to VDD". The correct way for
> amd_pinconf_get() to indicate that the pull-up/-down is not enabled
> is to return -EINVAL. I've checked a whole bunch of pinctrl drivers
> and they all behave this way. This patch brings the AMD pinctrl driver
> in line with this.
>
> Fixes: dbad75dd1f25 ("pinctrl: add AMD GPIO driver support.")
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=212379
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> drivers/pinctrl/pinctrl-amd.c | 37 +++++++++++++++++++++++++++--------
> 1 file changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index 9bc6e3922e78..88174195b5c8 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -744,11 +744,19 @@ static int amd_pinconf_get(struct pinctrl_dev *pctldev,
> break;
>
> case PIN_CONFIG_BIAS_PULL_DOWN:
> - arg = (pin_reg >> PULL_DOWN_ENABLE_OFF) & BIT(0);
> + if (!(pin_reg & BIT(PULL_DOWN_ENABLE_OFF)))
> + return -EINVAL;
> + arg = 1;
> break;
>
> case PIN_CONFIG_BIAS_PULL_UP:
> - arg = (pin_reg >> PULL_UP_SEL_OFF) & (BIT(0) | BIT(1));
> + if (!(pin_reg & BIT(PULL_UP_ENABLE_OFF)))
> + return -EINVAL;
> +
> + if (pin_reg & BIT(PULL_UP_SEL_OFF))
> + arg = 8000;
> + else
> + arg = 4000;
> break;
Do I understand correctly that there is only one bias value possible
for Pdown (4k?) and two for Pup (4k & 8k)?
(of course excluding cases when either is disabled).
Also I have stumbled over _OFF. Does it actually mean "offset"? Can we
rename to avoid (my) confusion with OFF as something being "off"?
(Maybe a separate patch?)
> case PIN_CONFIG_DRIVE_STRENGTH:
> @@ -790,15 +798,28 @@ static int amd_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> break;
>
> case PIN_CONFIG_BIAS_PULL_DOWN:
> - pin_reg &= ~BIT(PULL_DOWN_ENABLE_OFF);
> - pin_reg |= (arg & BIT(0)) << PULL_DOWN_ENABLE_OFF;
> + pin_reg |= BIT(PULL_DOWN_ENABLE_OFF);
> break;
>
> case PIN_CONFIG_BIAS_PULL_UP:
> - pin_reg &= ~BIT(PULL_UP_SEL_OFF);
> - pin_reg |= (arg & BIT(0)) << PULL_UP_SEL_OFF;
> - pin_reg &= ~BIT(PULL_UP_ENABLE_OFF);
> - pin_reg |= ((arg>>1) & BIT(0)) << PULL_UP_ENABLE_OFF;
> + /* Set default ohm value in case none is given */
> + if (arg == 1)
> + arg = 4000;
> +
> + switch (arg) {
> + case 4000:
> + pin_reg &= ~BIT(PULL_UP_SEL_OFF);
> + pin_reg |= BIT(PULL_UP_ENABLE_OFF);
> + break;
> + case 8000:
> + pin_reg |= BIT(PULL_UP_SEL_OFF);
> + pin_reg |= BIT(PULL_UP_ENABLE_OFF);
> + break;
> + default:
> + dev_err(&gpio_dev->pdev->dev,
> + "Invalid pull-up arg %u\n", arg);
> + ret = -EINVAL;
> + }
Can Pup and Pdown be enabled simultaneously?
> break;
>
> case PIN_CONFIG_DRIVE_STRENGTH:
After your answers I might come with some comments, but FWIW the
code-wise this seems correct approach.
--
With Best Regards,
Andy Shevchenko
