RE: [PATCH 04/10] input: keyboard: adp5588-keys: add support for fw properties

["Sa, Nuno" <Nuno.Sa@xxxxxxxxxx>]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Friday, July 8, 2022 4:56 PM
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: devicetree <devicetree@xxxxxxxxxxxxxxx>; open list:GPIO
> SUBSYSTEM <linux-gpio@xxxxxxxxxxxxxxx>; linux-input <linux-
> input@xxxxxxxxxxxxxxx>; Dmitry Torokhov
> <dmitry.torokhov@xxxxxxxxx>; Bartosz Golaszewski
> <brgl@xxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Linus Walleij
> <linus.walleij@xxxxxxxxxx>
> Subject: Re: [PATCH 04/10] input: keyboard: adp5588-keys: add
> support for fw properties
> 
> [External]
> 
> On Fri, Jul 8, 2022 at 11:37 AM Nuno Sá <nuno.sa@xxxxxxxxxx> wrote:
> >
> > Use firmware properties (eg: OF) to get the device specific
> > configuration. This change just replaces the platform data since there
> > was no platform using it and so, it makes no sense having both.
> >
> > Special note to the PULL-UP disable setting that is now supported as
> > part of the gpio subsystem (using 'set_config()' callback).
> 
> ...
> 
> > +#define ADP5588_DEVICE_ID_MASK 0xF
> > +
> > + /* Configuration Register1 */
> > +#define ADP5588_AUTO_INC       (1 << 7)
> > +#define ADP5588_GPIEM_CFG      (1 << 6)
> > +#define ADP5588_OVR_FLOW_M     (1 << 5)
> > +#define ADP5588_INT_CFG                (1 << 4)
> > +#define ADP5588_OVR_FLOW_IEN   (1 << 3)
> > +#define ADP5588_K_LCK_IM       (1 << 2)
> > +#define ADP5588_GPI_IEN                (1 << 1)
> > +#define ADP5588_KE_IEN         (1 << 0)
> 
> Okay, you add something in the wrong form and then fix it in the other
> patch in the very same series? Please no ping-pong type of changes.
> Squash / rebase your series accordingly.

Well, I thought to just copy it as it was on the platform file and then just fix
it with the rest of the coding styles changes. But I'm fine in fixing it already
in this patch. In fact, there's a lot of defines that are not used (it's just
defining the complete register map) so I can as well get rid of all the stuff 
that is not used anywhere in the driver.

> ...
> 
> > -       ret = adp5588_write(client, KP_GPIO2, KP_SEL(pdata->cols) &
> 0xFF);
> > +       ret = adp5588_write(client, KP_GPIO2, KP_SEL(kpad->cols) &
> 0xFF);
> 
> Do you need these " & 0xFF" parts?

Not sure but probably not. I just kept as it was...

> ...
> 
> > +               /*
> > +                * fw properties keys start from 0 but on the device they
> 
> Firmware
> 

ack...

- Nuno Sá