[PATCH 1/2] GPIO: fxl6408: Add support for FXL6408 GPIO expander

[Simon Glass]

Hi Oleksandr,

On Sun, 25 Jul 2021 at 16:19, Oleksandr Suvorov
<oleksandr.suvorov at toradex.com> wrote:
>
> Hi Simon,
>
> On Sun, Jul 25, 2021 at 1:01 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Oleksandr,
> >
> > On Wed, 21 Jul 2021 at 06:21, Oleksandr Suvorov
> > <oleksandr.suvorov at toradex.com> wrote:
> > >
> > > Initial support for Fairchild's 8 bit I2C gpio expander FXL6408.
> > > The CONFIG_FXL6408_GPIO define enables support for such devices.
> > >
> > > Based on: https://patchwork.kernel.org/patch/9148419/
> > >
> > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov at toradex.com>
> > > ---
> > >
> > >  drivers/gpio/Kconfig        |   7 +
> > >  drivers/gpio/Makefile       |   1 +
> > >  drivers/gpio/gpio-fxl6408.c | 371 ++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 379 insertions(+)
> > >  create mode 100644 drivers/gpio/gpio-fxl6408.c
> >
> > Reviewed-by: Simon Glass <sjg at chromium.org>
> >
> > Lots of nits below
>
> Thanks for your detailed and extremely useful review!
>

[..]

> > > +/* 3-bit firmware revision */
> > > +# define FXL6408_FW_MASK               GENMASK(4, 2)
> > > +# define FXL6408_FW_REV(devid)         (((devid) & FXL6408_FW_MASK) >> 2)
> >
> > This is only used once, so why not include that code inline below?
>
> This way is clearer to read as for me. Moreover, if we include only
> FW_REV()'s code inline,
> the shifting and the purpose of such shifting were in different places.
> Don't you mind leaving it as is? (with some names simplifying)

Yes that's fine, you have my review tag for the next version. FYI we
tend to do things like:

enum {
   FW_MASK = GENMASK(4, 2),
   FW_SHIFT = 2,
};

then open-code the assignment or reading. Often there is only one read
and one write in the source code, since registers are typically
handled in only a few places:

reg_val = (old_val & ~FW_MASK) | (val << FW_SHIFT);

val = (reg_val & FW_MASK) >> FW_SHIFT;

Regards,
Simon