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