Hi Oleksandr, On Sun, 25 Jul 2021 at 16:19, Oleksandr Suvorov <oleksandr.suvo...@toradex.com> wrote: > > Hi Simon, > > On Sun, Jul 25, 2021 at 1:01 AM Simon Glass <s...@chromium.org> wrote: > > > > Hi Oleksandr, > > > > On Wed, 21 Jul 2021 at 06:21, Oleksandr Suvorov > > <oleksandr.suvo...@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.suvo...@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 <s...@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