Re: [PATCH] pinctrl: meson: fix drive strength register and bit calculation

[Jerome Brunet <jbrunet@xxxxxxxxxxxx>]

On Thu 11 Jun 2020 at 07:40, Hyeonki Hong <hhk7734@xxxxxxxxx> wrote:

> On Wed, Jun 10, 2020 at 03:09:42PM +0200, Jerome Brunet wrote:
>> 
>> On Wed 10 Jun 2020 at 06:13, hhk7734@xxxxxxxxx wrote:
>> 
>> > From: Hyeonki Hong <hhk7734@xxxxxxxxx>
>> >
>> > If a GPIO bank has greater than 16 pins, PAD_DS_REG is split into two
>> > registers. However, when register and bit were calculated, the first
>> > register defined in the bank was used, and the bit was calculated based
>> > on the first pin. This causes problems in setting the driving strength.
>> >
>> > Solved the problem by changing the bit using a mask and selecting the
>> > next register when the bit exceeds 15.
>> 
>> This fixes the case of GPIOX on g12 which goes up to 18 yes but the same
>> problem will happen again a if bank ever goes past 31 pins. In such case
>> the problem would apply to all reg types.
>> 
>> I would prefer if it was solved in a more generic fashion, like defining
>> a "stride" table with the values of each reg type. This table can common
>> to all aml SoCs for now but eventually it probably need to be SoC
>> specific.
>> 
>> This would allow to :
>> A) handle the case you are reporting in a generic (future proof) way
>> B) remove the weird "bit = bit << 1;" calc in place in the get/set of
>> the drive strengh pinconf
>
> If all amlogic SoC has same register style, I think the code below is fine.
>
> static const unsigned int meson_bit_strides[] = {
> 	0, 0, 0, 0, 0, 1, 0
> };

 the table above is the shift, not the stride.
 Maybe 'width' is a better description
 I would prefer if it was { 1, 1, 1, 1, 1, 2, 1 } and adjust the caculation

A part from this, your proposition is exactly what I meant :) Thx

>
> static void meson_calc_reg_and_bit(struct meson_bank *bank, unsigned int pin,
> 				   enum meson_reg_type reg_type,
> 				   unsigned int *reg, unsigned int *bit)
> {
> 	struct meson_reg_desc *desc = &bank->regs[reg_type];
>
> 	*bit = (desc->bit + pin - bank->first) << meson_bit_strides[reg_type];
> 	*reg = (desc->reg + (*bit / 32)) * 4;
> 	*bit &= 0x1f;
> }
>
> How about this?
>
>> >
>> > Signed-off-by: Hyeonki Hong <hhk7734@xxxxxxxxx>
>> > ---
>> >  drivers/pinctrl/meson/pinctrl-meson.c | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
>> > index bbc919bef2bf..ef66239b7df5 100644
>> > --- a/drivers/pinctrl/meson/pinctrl-meson.c
>> > +++ b/drivers/pinctrl/meson/pinctrl-meson.c
>> > @@ -98,6 +98,13 @@ static void meson_calc_reg_and_bit(struct meson_bank *bank, unsigned int pin,
>> >
>> >  	*reg = desc->reg * 4;
>> >  	*bit = desc->bit + pin - bank->first;
>> > +
>> > +	if (reg_type == REG_DS) {
>> > +		if (*bit > 15) {
>> > +			*bit &= 0xf;
>> > +			*reg += 4;
>> > +		}
>> > +	}
>> >  }
>> >
>> >  static int meson_get_groups_count(struct pinctrl_dev *pcdev)
>>