RE: [PATCH] gpio: mmio: do not calculate bgpio_bits via "ngpios"

[Asmaa Mnebhi <asmaa@xxxxxxxxxx>]

> -----Original Message-----
> From: Jonas Gorski <jonas.gorski@xxxxxxxxx>
> Sent: Wednesday, June 26, 2024 11:21 AM
> To: Asmaa Mnebhi <asmaa@xxxxxxxxxx>
> Cc: Shiji Yang <yangshiji66@xxxxxxxxxxx>; linux-gpio@xxxxxxxxxxxxxxx; Linus
> Walleij <linus.walleij@xxxxxxxxxx>; Bartosz Golaszewski <brgl@xxxxxxxx>;
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; Mark Mentovai <mark@xxxxxxxxxxxx>; Lóránd
> Horváth <lorand.horvath82@xxxxxxxxx>
> Subject: Re: [PATCH] gpio: mmio: do not calculate bgpio_bits via "ngpios"
> Importance: High
> 
> On Wed, 26 Jun 2024 at 17:00, Asmaa Mnebhi <asmaa@xxxxxxxxxx> wrote:
> >
> > I am not sure this change is needed?
> > When I initially submitted " gpio: mmio: handle "ngpios" properly in
> bgpio_init() ", It was specifically because I have a 32bit reg access but only 16
> gpios. Initially, I did not add the else and so Andy suggested to add it with the
> roundup_pow_of_two to stay backward compatible. If your system is a 32 bit
> arch and you only use 16 Gpio bits, why don't you configure that in your DTS?
> 
> Because the registers in the datasheet are specified as 32 bit wide, so defining
> them as 32 bit in the dts(i) is the most natural way of defining them, even if
> they may use less than half of the register for gpios. And on big endian
> systems you cannot just use smaller accessors, you also must shift the register
> offsets. So this change broke existing devicetrees.
> 
> And as other theoretical arguments against doing that, less than 32 bit
> accesses may be inefficient, or the bus where the registers are may require 32
> bit accesses. And finally, the caller explicitly passed a register width via the sz
> argument, so we should listen to the caller and use that, and not trying to be
> clever by changing the access width based on the number of gpios. At least
> not unless the caller explicitly requested that. Like e.g. make 0 a special value
> for automatically calculating the number of bits based on the number of
> gpios.
> 
> If you only use 16 bits of the 32 bit registers and you want to use 16 bit
> accessors, IMHO it's up to you to pass appropriate values to bgpio_init(), and
> not hope that bgpio_init() will fix this magically up for you.
> 

It was definitely not my intention to change/hack a 32 bits reg access to 16.
I agree with you that bgpio_bits should just not be changed. Maybe the else statement introduces a bug/hack in the case where ngpio is already a power of 2 such as 16 while the register access is a 32 bit access. In this case bgpio_bits would be 16 when it should be 32.

However, Shiji's has a bug and will break other code. My very first patch for "gpio: mmio: handle "ngpios" properly in bgpio_init()" was meant to fix the following problem (that shiji's code will reintroduce):
" bgpio_init uses "sz" argument to populate ngpio, which is not accurate.
Instead, read the "ngpios" property from the DT and if it doesn't 
exist, use the "sz" argument. With this change, drivers no longer need 
to overwrite the ngpio variable after calling bgpio_init."

My v1->v3 patches were only changing ngpio, not bgpio_bit. Please check my v3 patch: https://lore.kernel.org/lkml/202303050354.HH9DhJsr-lkp@xxxxxxxxx/t/

After v3, Andy asked me to also change bgpio_bits. Please see the whole thread:

> > > >+       ret = gpiochip_get_ngpios(gc, dev);
> > > >+       if (ret)
> > >> +               gc->ngpio = gc->bgpio_bits;
> >>
> > >But this doesn't update bgpio_bits in the success case. Can you explain why
> >> it's not a problem (should be at least in the code as a comment).
>>
>> In the success rate, the bgpio_bits would also be equal to "sz * 8" anyways.
>> The argument " unsigned long sz" passed in bgpio_init is specifically for this purpose. That tells the gpio library the gpio register access size.
> >if (!is_power_of_2(sz))
>>                  return -EINVAL;
> > gc->bgpio_bits = sz * 8;
>>
> >If in the success case, we make it dependent on the ngpio value, we would need to round it up anyways to the closest (power of 2 && multiple of 8) which is the same as "sz * 8"
>> I will add a comment in the code in my next patch.

>I believe we should use only a single source for what we need. If we
> rely on ngpios, the bgpio_bits should be recalculated based on it. The
>expression doing this is very simple. Something like round_up(ngpios,
>8);

 Now, if we want to not modify bgpio_bits, we could go back to my v3 patch.
ngpio is the number of gpio pins supported while bgpio_bits is the register access bit type. These are 2 different entities.