Re: [PATCH v2 2/6] pinctrl: Ingenic: Add support for read the pin configuration of X1830.

[Zhou Yanjie <zhouyanjie@wanyeetech.com>]

Hi Paul,

On 2021/3/12 下午9:31, Paul Cercueil wrote:
> Hi Zhou,
>
> Le jeu. 11 mars 2021 à 23:21, 周琰杰 (Zhou Yanjie) 
> <zhouyanjie@wanyeetech.com> a écrit :
>> Add X1830 support in "ingenic_pinconf_get()", so that it can read the
>> configuration of X1830 SoC correctly.
>>
>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>
> This is a fix, so it needs a Fixes: tag, and you need to Cc linux-stable.
>

Sure.


>> ---
>>
>> Notes:
>>     v2:
>>     New patch.
>>
>>  drivers/pinctrl/pinctrl-ingenic.c | 76 
>> +++++++++++++++++++++++++++++----------
>>  1 file changed, 57 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-ingenic.c 
>> b/drivers/pinctrl/pinctrl-ingenic.c
>> index 05dfa0a..0a88aab 100644
>> --- a/drivers/pinctrl/pinctrl-ingenic.c
>> +++ b/drivers/pinctrl/pinctrl-ingenic.c
>> @@ -2109,31 +2109,69 @@ static int ingenic_pinconf_get(struct 
>> pinctrl_dev *pctldev,
>>      enum pin_config_param param = pinconf_to_config_param(*config);
>>      unsigned int idx = pin % PINS_PER_GPIO_CHIP;
>>      unsigned int offt = pin / PINS_PER_GPIO_CHIP;
>> +    unsigned int bias;
>>      bool pull;
>>
>> -    if (jzpc->info->version >= ID_JZ4770)
>> -        pull = !ingenic_get_pin_config(jzpc, pin, JZ4770_GPIO_PEN);
>> -    else
>> -        pull = !ingenic_get_pin_config(jzpc, pin, 
>> JZ4740_GPIO_PULL_DIS);
>> +    if (jzpc->info->version >= ID_X1830) {
>> +        unsigned int half = PINS_PER_GPIO_CHIP / 2;
>> +        unsigned int idxh = pin % half * 2;
>>
>> -    switch (param) {
>> -    case PIN_CONFIG_BIAS_DISABLE:
>> -        if (pull)
>> -            return -EINVAL;
>> -        break;
>> +        if (idx < half)
>> +            regmap_read(jzpc->map, offt * jzpc->info->reg_offset +
>> +                    X1830_GPIO_PEL, &bias);
>> +        else
>> +            regmap_read(jzpc->map, offt * jzpc->info->reg_offset +
>> +                    X1830_GPIO_PEH, &bias);
>>
>> -    case PIN_CONFIG_BIAS_PULL_UP:
>> -        if (!pull || !(jzpc->info->pull_ups[offt] & BIT(idx)))
>> -            return -EINVAL;
>> -        break;
>> +        bias = (bias >> idxh) & 3;
>
> You can do:
>
> u32 mask = GENMASK(idxh + 1, idxh);
>
> bias = FIELD_GET(mask, bias);
>
> (macros in <linux/bitfield.h>)
>

Sure.


>>
>> -    case PIN_CONFIG_BIAS_PULL_DOWN:
>> -        if (!pull || !(jzpc->info->pull_downs[offt] & BIT(idx)))
>> -            return -EINVAL;
>> -        break;
>> +        switch (param) {
>> +        case PIN_CONFIG_BIAS_DISABLE:
>> +            if (bias)
>> +                return -EINVAL;
>> +            break;
>>
>> -    default:
>> -        return -ENOTSUPP;
>> +        case PIN_CONFIG_BIAS_PULL_UP:
>> +            if ((bias != PIN_CONFIG_BIAS_PULL_UP) ||
>> +                    !(jzpc->info->pull_ups[offt] & BIT(idx)))
>
> "bias" is a 2-bit value (because of the & 3 mask), and 
> PIN_CONFIG_BIAS_PULL_UP == 5.
>
> So this clearly won't work. You are comparing hardware values with 
> public API enums.


OK, I will fix it in the next version.


>
>> +                return -EINVAL;
>> +            break;
>> +
>> +        case PIN_CONFIG_BIAS_PULL_DOWN:
>> +            if ((bias != PIN_CONFIG_BIAS_PULL_DOWN) ||
>> +                    !(jzpc->info->pull_downs[offt] & BIT(idx)))
>> +                return -EINVAL;
>> +            break;
>> +
>> +        default:
>> +            return -ENOTSUPP;
>> +        }
>> +
>> +    } else {
>> +        if (jzpc->info->version >= ID_JZ4770)
>> +            pull = !ingenic_get_pin_config(jzpc, pin, JZ4770_GPIO_PEN);
>> +        else
>> +            pull = !ingenic_get_pin_config(jzpc, pin, 
>> JZ4740_GPIO_PULL_DIS);
>
> I think you can keep the switch outside the if/else block, if you use 
> pullup/pulldown variables.
>
> These can be initialized (in the non-X1830 case) to:
>
> pullup = pull && (jzpc->info->pull_ups[offt] & BIT(idx));
> pulldown = pull && (jzpc->info->pull_downs[offt] & BIT(idx));
>
> In the X1830 case you'd initialize these variables from 'bias'.


Sure, I will do this in the next version.


>
>> +
>> +        switch (param) {
>> +        case PIN_CONFIG_BIAS_DISABLE:
>> +            if (pull)
>
> Here would change to if (pullup || pulldown)
>

OK.


>> +                return -EINVAL;
>> +            break;
>> +
>> +        case PIN_CONFIG_BIAS_PULL_UP:
>> +            if (!pull || !(jzpc->info->pull_ups[offt] & BIT(idx)))
>
> if (!pullup)
>

Sure.


>> +                return -EINVAL;
>> +            break;
>> +
>> +        case PIN_CONFIG_BIAS_PULL_DOWN:
>> +            if (!pull || !(jzpc->info->pull_downs[offt] & BIT(idx)))
>
> if (!pulldown)
>

Sure.


Thanks and best regards!


> Cheers,
> -Paul
>
>> +                return -EINVAL;
>> +            break;
>> +
>> +        default:
>> +            return -ENOTSUPP;
>> +        }
>>      }
>>
>>      *config = pinconf_to_config_packed(param, 1);
>> -- 
>> 2.7.4
>>
>