Hi,
On 1/25/23 12:14, Andy Shevchenko wrote:
> 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)?
Yes I believe so, it has been a while ago and I don't know
where I got the 8k and 4k values from anymore (oops) ...
> 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?)
Yes it means offset, I actually stumbled over this while re-reading
the patch myself too. So definitely something to fix.
>
>> 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?
I believe so, so I think that we also need to clear the other
enable bit (e.g. clear down when enabling up) when enabling
the pull up / down. I'll do this for v2 (after waiting for other
comments first).
>> break;
>>
>> case PIN_CONFIG_DRIVE_STRENGTH:
>
> After your answers I might come with some comments, but FWIW the
> code-wise this seems correct approach.
Thank you for taking a look.
Regards,
Hans
