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
