Re: [PATCH v2 2/3] pinctrl: sunxi: Add support for fetching pinconf settings from hardware

[Chen-Yu Tsai]

On Fri, Nov 11, 2016 at 4:36 PM, Maxime Ripard
<maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> On Fri, Nov 11, 2016 at 10:44:54AM +0800, Chen-Yu Tsai wrote:
>> The sunxi pinctrl driver only caches whatever pinconf setting was last
>> set on a given pingroup. This is not particularly helpful, nor is it
>> correct.
>>
>> Fix this by actually reading the hardware registers and returning
>> the correct results or error codes. Also filter out unsupported
>> pinconf settings. Since this driver has a peculiar setup of 1 pin
>> per group, we can support both pin and pingroup pinconf setting
>> read back with the same code. The sunxi_pconf_reg helper and code
>> structure is inspired by pinctrl-msm.
>>
>> With this done we can also claim to support generic pinconf, by
>> setting .is_generic = true in pinconf_ops.
>>
>> Signed-off-by: Chen-Yu Tsai <wens@xxxxxxxx>
>> ---
>>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 84 +++++++++++++++++++++++++++++++++--
>>  1 file changed, 81 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> index e04edda8629d..3e9f7c675d36 100644
>> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> @@ -438,15 +438,91 @@ static const struct pinctrl_ops sunxi_pctrl_ops = {
>>       .get_group_pins         = sunxi_pctrl_get_group_pins,
>>  };
>>
>> +static int sunxi_pconf_reg(unsigned pin, enum pin_config_param param,
>> +                        u32 *offset, u32 *shift, u32 *mask)
>> +{
>> +     switch (param) {
>> +     case PIN_CONFIG_DRIVE_STRENGTH:
>> +             *offset = sunxi_dlevel_reg(pin);
>> +             *shift = sunxi_dlevel_offset(pin);
>> +             *mask = DLEVEL_PINS_MASK;
>> +             break;
>> +
>> +     case PIN_CONFIG_BIAS_PULL_UP:
>> +     case PIN_CONFIG_BIAS_PULL_DOWN:
>> +     case PIN_CONFIG_BIAS_DISABLE:
>> +             *offset = sunxi_pull_reg(pin);
>> +             *shift = sunxi_pull_offset(pin);
>> +             *mask = PULL_PINS_MASK;
>> +             break;
>> +
>> +     default:
>> +             return -ENOTSUPP;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int sunxi_pconf_get(struct pinctrl_dev *pctldev, unsigned pin,
>> +                        unsigned long *config)
>> +{
>> +     struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
>> +     enum pin_config_param param = pinconf_to_config_param(*config);
>> +     u32 offset, shift, mask, val;
>> +     u16 arg;
>> +     int ret;
>> +
>> +     pin -= pctl->desc->pin_base;
>> +
>> +     ret = sunxi_pconf_reg(pin, param, &offset, &shift, &mask);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     val = (readl(pctl->membase + offset) >> shift) & mask;
>> +
>> +     switch (pinconf_to_config_param(*config)) {
>> +     case PIN_CONFIG_DRIVE_STRENGTH:
>> +             arg = (val + 1) * 10;
>> +             break;
>> +
>> +     case PIN_CONFIG_BIAS_PULL_UP:
>> +             if (val != SUN4I_PINCTRL_PULL_UP)
>> +                     return -EINVAL;
>> +             arg = 1; /* hardware is weak pull-up */
>> +             break;
>> +
>> +     case PIN_CONFIG_BIAS_PULL_DOWN:
>> +             if (val != SUN4I_PINCTRL_PULL_DOWN)
>> +                     return -EINVAL;
>> +             arg = 1; /* hardware is weak pull-down */
>> +             break;
>> +
>> +     case PIN_CONFIG_BIAS_DISABLE:
>> +             if (val != SUN4I_PINCTRL_NO_PULL)
>> +                     return -EINVAL;
>> +             arg = 0;
>> +             break;
>> +
>> +     default:
>> +             /* sunxi_pconf_reg should catch anything unsupported */
>> +             WARN_ON(1);
>> +             return -ENOTSUPP;
>> +     }
>> +
>> +     *config = pinconf_to_config_packed(param, arg);
>> +
>> +     return 0;
>> +}
>> +
>>  static int sunxi_pconf_group_get(struct pinctrl_dev *pctldev,
>>                                unsigned group,
>>                                unsigned long *config)
>>  {
>>       struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
>> +     struct sunxi_pinctrl_group *g = &pctl->groups[group];
>>
>> -     *config = pctl->groups[group].config;
>
> Do we still need this variable? Looking at the code, it doesn't look
> that way, and we can remove the caching in the _group_set function and
> the variable itslef in the sunxi_pincttrl_group structure.

It's actually removed in the next patch. :)

ChenYu