Re: [PATCH v12 4/4] gpio: xilinx: Utilize generic bitmap_get_value and _set_value

[Syed Nayyar Waris]

On Tue, Nov 10, 2020 at 6:05 PM William Breathitt Gray
<vilhelm.gray@xxxxxxxxx> wrote:
>
> On Tue, Nov 10, 2020 at 11:02:43AM +0100, Michal Simek wrote:
> >
> >
> > On 09. 11. 20 18:31, William Breathitt Gray wrote:
> > > On Mon, Nov 09, 2020 at 07:22:20PM +0200, Andy Shevchenko wrote:
> > >> On Mon, Nov 09, 2020 at 12:11:40PM -0500, William Breathitt Gray wrote:
> > >>> On Mon, Nov 09, 2020 at 10:15:29PM +0530, Syed Nayyar Waris wrote:
> > >>>> On Mon, Nov 09, 2020 at 03:41:53PM +0100, Arnd Bergmann wrote:
> > >>
> > >> ...
> > >>
> > >>>>  static inline void bitmap_set_value(unsigned long *map,
> > >>>> -                                    unsigned long value,
> > >>>> +                                    unsigned long value, const size_t length,
> > >>>>                                      unsigned long start, unsigned long nbits)
> > >>>>  {
> > >>>>          const size_t index = BIT_WORD(start);
> > >>>> @@ -15,6 +15,10 @@ static inline void bitmap_set_value(unsigned long *map,
> > >>>>          } else {
> > >>>>                  map[index + 0] &= ~BITMAP_FIRST_WORD_MASK(start);
> > >>>>                  map[index + 0] |= value << offset;
> > >>>> +
> > >>>> +               if (index + 1 >= length)
> > >>>> +                       __builtin_unreachable();
> > >>>> +
> > >>>>                  map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
> > >>>>                  map[index + 1] |= value >> space;
> > >>>>          }
> > >>>
> > >>> Hi Syed,
> > >>>
> > >>> Let's rename 'length' to 'nbits' as Arnd suggested, and rename 'nbits'
> > >>> to value_width.
> > >>
> > >> length here is in longs. I guess this is the point of entire patch.
> > >
> > > Ah yes, this should become 'const unsigned long nbits' and represent the
> > > length of the bitmap in bits and not longs.

Hi William, Andy and All,

Thank You for reviewing. I was looking into the review comments and I
have a question on the above.

Actually, in bitmap_set_value(), the intended comparison is to be made
between 'index + 1' and 'length' (which is now renamed as 'nbits').
That is, the comparison would look-like as follows:
if (index + 1 >= nbits)

The 'index' is getting populated with BIT_WORD(start).
The 'index' variable in above is the actual index of the bitmap array,
while in previous mail it is suggested to use 'nbits' which represent
the length of the bitmap in bits and not longs.

Isn't it comparing two different things? index of array (not the
bit-wise-length) on left hand side and nbits (bit-wise-length) on
right hand side?

Have I misunderstood something? If yes, request to clarify.

Or do I have to first divide 'nbits' by BITS_PER_LONG and then compare
it with 'index + 1'? Something like this?

Regards
Syed Nayyar Waris

> > >
> > >> But to me sounds like it would be better to have simply bitmap_set_value64() /
> > >> bitmap_set_value32() with proper optimization done and forget about variadic
> > >> ones for now.
> > >
> > > The gpio-xilinx driver can have arbitrary sizes for width[0] and
> > > width[1], so unfortunately that means we don't know the start position
> > > nor the width of the value beforehand.
> >
> > Start position should be all the time zero. You can't configure this IP
> > to start from bit 2. Width can vary but start is IMHO all the time from
> > 0 bit.
> >
> > Thanks,
> > Michal
>
> Hi Michal,
>
> I'm referring to the mask creation, not the data bus transfer; see the
> implementation of the xgpio_set_multiple() function in linux-next for
> reference:
> <https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpio/gpio-xilinx.c?h=akpm>.
>
> To generate the old mask we call the following:
>
>         bitmap_set_value(old, state[0], 0, width[0]);
>         bitmap_set_value(old, state[1], width[0], width[1]);
>
> Here, width[0] and width[1] can vary, which makes the exact values of
> the start and nbits parameters unknown beforehand (although we do know
> they are within the bitmap boundary).
>
> Regardless, this is not an issue because we know the bitmap_set_value()
> is supposed to be called with valid values. We just need a way to hint
> to GCC that this is the case, without increasing the latency of the
> function -- which I think is possible if we use __builtin_unreachable()
> for the conditional path checking the index against the length of the
> bitmap.
>
> William Breathitt Gray