Re: [PATCH] pinctrl: single: support GPIO for bits pinctrl

[Jun Nie <jun.nie@linaro.org>]

2015-06-17 15:17 GMT+08:00 Tony Lindgren <tony@atomide.com>:
> * Jun Nie <jun.nie@linaro.org> [150616 18:58]:
>> Support GPIO for one register control multiple pins case
>> with calculating register offset first, then bit offset.
>>
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> ---
>>  drivers/pinctrl/pinctrl-single.c | 22 ++++++++++++++++++----
>>  1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
>> index 13b45f2..bd69d9a 100644
>> --- a/drivers/pinctrl/pinctrl-single.c
>> +++ b/drivers/pinctrl/pinctrl-single.c
>> @@ -494,7 +494,7 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev,
>>       struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
>>       struct pcs_gpiofunc_range *frange = NULL;
>>       struct list_head *pos, *tmp;
>> -     int mux_bytes = 0;
>> +     int offset, mux_bytes = 0;
>>       unsigned data;
>>
>>       /* If function mask is null, return directly. */
>> @@ -507,9 +507,23 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev,
>>                       || pin < frange->offset)
>>                       continue;
>>               mux_bytes = pcs->width / BITS_PER_BYTE;
>> -             data = pcs->read(pcs->base + pin * mux_bytes) & ~pcs->fmask;
>> -             data |= frange->gpiofunc;
>> -             pcs->write(data, pcs->base + pin * mux_bytes);
>> +             if (pcs->bits_per_mux) {
>> +                     int pin_pos, byte_num, num_pins_in_register;
>> +
>> +                     num_pins_in_register = pcs->width / pcs->bits_per_pin;
>> +                     byte_num = (pcs->bits_per_pin * pin) / BITS_PER_BYTE;
>> +                     offset = (byte_num / mux_bytes) * mux_bytes;
>> +                     pin_pos = pin % num_pins_in_register;
>> +                     pin_pos *= pcs->bits_per_pin;
>> +                     data = pcs->read(pcs->base + offset) &
>> +                             ~(pcs->fmask << pin_pos);
>
> Should you check the pcs->fmask here too in case some bits are reserved?
>
Did not catch your idea? Those bits set in fmask are dedicated for one
pin mux control and should be clear before set as desired value per my
understanding. Do you mean some bits may be reserved and not for any
function?

>> +                     data |= frange->gpiofunc << pin_pos;
>> +             } else {
>> +                     offset = pin * mux_bytes;
>> +                     data = pcs->read(pcs->base + offset) & ~pcs->fmask;
>> +                     data |= frange->gpiofunc;
>> +             }
>> +             pcs->write(data, pcs->base + offset);
>>               break;
>>       }
>>       return 0;
>
> Other than that looks OK to me, would be good to also wait for Haojian's
> comments here.
>
> Regards,
>
> Tony