Re: [PATCH RESEND] input: adp5589: Add gpio_set_multiple interface

[Togorean, Bogdan]

Hi Dmitry,

Thank you for your review

On Thu, 2019-05-23 at 00:18 -0700, Dmitry Torokhov wrote:
> [External]
> 
> 
> Hi Bogdan,
> 
> On Tue, May 21, 2019 at 11:38:22AM +0300, Bogdan Togorean wrote:
> > This patch implements the gpio_set_multiple interface for ADP558x
> > chip.
> > 
> > Signed-off-by: Bogdan Togorean <bogdan.togorean@xxxxxxxxxx>
> > ---
> >  drivers/input/keyboard/adp5589-keys.c | 25
> > +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/drivers/input/keyboard/adp5589-keys.c
> > b/drivers/input/keyboard/adp5589-keys.c
> > index 2835fba71c33..143871bd60ef 100644
> > --- a/drivers/input/keyboard/adp5589-keys.c
> > +++ b/drivers/input/keyboard/adp5589-keys.c
> > @@ -416,6 +416,30 @@ static void adp5589_gpio_set_value(struct
> > gpio_chip *chip,
> >       mutex_unlock(&kpad->gpio_lock);
> >  }
> > 
> > +static void adp5589_gpio_set_multiple(struct gpio_chip *chip,
> > +                                   unsigned long *mask, unsigned
> > long *bits)
> > +{
> > +     struct adp5589_kpad *kpad = container_of(chip, struct
> > adp5589_kpad, gc);
> > +     u8 bank, reg_mask, reg_bits;
> > +
> > +     mutex_lock(&kpad->gpio_lock);
> > +
> > +     for (bank = 0; bank <= kpad->var->bank(kpad->var->maxgpio);
> > bank++) {
> > +             if (bank > kpad->var->bank(get_bitmask_order(*mask) -
> > 1))
> > +                     break;
> 
> I wonder if we should have:
> 
>         last_gpio = min(kpad->var->maxgpio, get_bitmask_order(*mask)
> - 1);
>         last_bank = kpad->var->bank(last_bank);
>         for (bank = 0; bank <= last_bank; bank++) {
>                 ...
>         }
I agree this can be made more clear in the way you proposed.
> 
> > +             reg_mask = mask[bank / sizeof(*mask)] >>
> > +                        ((bank % sizeof(*mask)) * BITS_PER_BYTE);
> > +             reg_bits = bits[bank / sizeof(*bits)] >>
> > +                        ((bank % sizeof(*bits)) * BITS_PER_BYTE);
> 
> This s really hard to parse. We know that "bank" is a byte, and mask
> is
> long, we do not have to be this roundabout it.
Here main reasons for doing this complexity were to support 64bit/32bit
architectures (differet long size) and to avoid use of magic values
(BITS_PER_BYTE)
> 
> > +             kpad->dat_out[bank] &= ~reg_mask;
> > +             kpad->dat_out[bank] |= reg_bits & reg_mask;
> > +             adp5589_write(kpad->client, kpad->var-
> > >reg(ADP5589_GPO_DATA_OUT_A) + bank,
> > +                           kpad->dat_out[bank]);
> > +     }
> 
> However the biggest issue is that this implementation seems to ignore
> the kpad->gpiomap that translates GPIO numbers as seen by gpiolib to
> GPIO numbers used by the chip. You need to reshuffle the mask and
> bits,
> and only then do the writes.
> 
> Given the complexities, does set_multiple really save anything?
> 
We have a critical application where we need to set multiple GPIOs that
are on the same bank simultaneously. No delay accepted. So this was the
usecase which led to implementation of set_multiple_interface for this
chip.
> Thanks.
> 
> --
> Dmitry
Thank you,
Bogdan