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

Oleksandr Suvorov oleksandr.suvorov at toradex.com
Mon Jul 26 00:19:05 CEST 2021


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!

> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 0817b12c5f..5883582a7f 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -123,6 +123,13 @@ config DA8XX_GPIO
> >         help
> >           This driver supports the DA8xx GPIO controller
> >
> > +config FXL6408_GPIO
> > +       bool "FXL6408 I2C GPIO driver"
> > +       depends on DM_GPIO && DM_I2C
> > +       help
> > +         Support for Fairchild Semiconductor FXL6408 I2C 8-bit GPIO
> > +         expander.
>
> Please add some more details here.

Some details are being added to the next version of the patch set.

> [..]
>
> > diff --git a/drivers/gpio/gpio-fxl6408.c b/drivers/gpio/gpio-fxl6408.c
> > new file mode 100644
> > index 0000000000..282ec0a69c
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-fxl6408.c
> > @@ -0,0 +1,371 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + *  Copyright (C) 2021 Toradex
> > + *  Copyright (C) 2016 Broadcom
> > + */
> > +
> > +/**
> > + * DOC: FXL6408 I2C to GPIO expander.
> > + *
> > + * This chip has 8 GPIO lines out of it, and is controlled by an I2C
> > + * bus (a pair of lines), providing 4x expansion of GPIO lines. It
> > + * also provides an interrupt line out for notifying of state changes.
> > + *
> > + * Any preconfigured state will be left in place until the GPIO lines
> > + * get activated. At power on, everything is treated as an input,
> > + * default input is HIGH and pulled-up, all interrupts are masked.
> > + *
> > + * Documentation can be found at:
> > + * https://www.fairchildsemi.com/datasheets/FX/FXL6408.pdf
> > + *
> > + * This driver bases on:
> > + * - the original driver by Eric Anholt <eric at anholt.net>:
> > + *   https://patchwork.kernel.org/patch/9148419/
> > + * - the Toradex version by Max Krummenacher <max.krummenacher at toradex.com>:
> > + *   http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/gpio/gpio-fxl6408.c?h=toradex_5.4-2.3.x-imx
> > + * - the U-boot PCA953x driver by Peng Fan <van.freenix at gmail.com>:
> > + *   drivers/gpio/pca953x_gpio.c
> > + *
> > + * TODO: Add interrupts support
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <dm/device_compat.h>
> > +#include <i2c.h>
> > +#include <log.h>
> > +#include <asm-generic/gpio.h>
> > +#include <asm/global_data.h>
> > +#include <linux/bitops.h>
> > +#include <dt-bindings/gpio/gpio.h>
> > +
> > +#define FXL6408_DEVID_CTRL             0x01
>
> Do you need the FXL6408 prefix?

Actually, no :) Thanks, I renamed all registers in a straight manner.
They'll be introduced in the next patchset version.

> > +# define FXL6408_SW_RST                        BIT(0)
> > +# define FXL6408_RST_INT               BIT(1)
> > +
> > +/* 3-bit manufacturer ID */
> > +# define FXL6408_MF_MASK               GENMASK(7, 5)
> > +# define FXL6408_MF_ID(devid)          (((devid) & FXL6408_MF_MASK) >> 5)
> > +/* 0b101 is for Fairchild assigned by Nokia */
> > +# define FXL6408_FAIRCHILD_MF          5
> > +
> > +/* 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)

> > +
> > +/*
> > + * Bits set here indicate that the GPIO is an output.
>
> single-line comment style is /* ... */

Thanks, fixed!

> > + */
> > +#define FXL6408_DIRECTION      0x03
>
> Then call it DIR_MASK ?

Formally, it is not a mask but a register.
All register names will be tuned in the next patchset version.

> > +
> > +enum {
> > +       FXL6408_DIRECTION_IN,
> > +       FXL6408_DIRECTION_OUT,
> > +};
> > +
> > +/*
> > + * Bits set here, when the corresponding bit of IO_DIR is set, drive
> > + * the output high instead of low.
> > + */
> > +#define FXL6408_OUTPUT         0x05
> > +
> > +/*
> > + * Bits here make the output High-Z, instead of the OUTPUT value.
> > + */
> > +#define FXL6408_OUTPUT_HIGH_Z  0x07
> > +
> > +/*
> > + * Bits here define the expected input state of the GPIO.
> > + * INTERRUPT_STATUS bits will be set when the INPUT transitions away
> > + * from this value.
> > + */
> > +#define FXL6408_INPUT_DEF_STATE        0x09
> > +
> > +/*
> > + * Bits here enable either pull up or pull down according to
> > + * INPUT_PULL_STATE.
> > + */
> > +#define FXL6408_INPUT_PULL_ENABLE      0x0b
> > +
> > +/*
> > + * Bits set here selects a pull-up/pull-down state of pin, which
> > + * is configured as Input and the corresponding PULL_ENABLE bit is
> > + * set.
> > + */
> > +#define FXL6408_INPUT_PULL_STATE       0x0d
> > +
> > +/*
> > + * Returns the current status (1 = HIGH) of the input pins.
> > + */
> > +#define FXL6408_INPUT          0x0f
> > +
> > +/*
> > + * Mask of pins which can generate interrupts.
> > + */
> > +#define FXL6408_INTERRUPT_MASK 0x11
> > +/*
> > + * Mask of pins which have generated an interrupt.
> > + * Cleared on read.
> > + */
> > +#define FXL6408_INTERRUPT_STATUS       0x13
> > +
> > +/*
> > + * struct fxl6408_info - Data for fxl6408
> > + *
> > + * @dev: udevice structure for the device
> > + * @addr: i2c slave address
>
> Please check; these don't match the struct.

Thanks! Fixed.

> > + * @reg_output: hold the value of output register
> > + * @reg_direction: hold the value of direction register
> > + */
> > +struct fxl6408_info {
> > +       struct udevice *dev;
> > +       int addr;
> > +       u8 device_id;
> > +       u8 reg_io_dir;
> > +       u8 reg_output;
> > +};
> > +
> > +static int fxl6408_write(struct udevice *dev, int reg, u8 val)
> > +{
> > +       int ret;
> > +
> > +       ret = dm_i2c_write(dev, reg, &val, 1);
> > +       if (ret)
> > +               dev_err(dev, "%s error: %d\n", __func__, ret);
> > +
> > +       return ret;
> > +}
> > +
> > +static int fxl6408_read(struct udevice *dev, int reg, u8 *val)
>
> You could return the value, or -ve for error, saving a parameter.

Good shot! Reimplemented.

> > +{
> > +       int ret;
> > +       u8 tmp;
> > +
> > +       ret = dm_i2c_read(dev, reg, &tmp, 1);
> > +       if (!ret)
> > +               *val = tmp;
> > +       else
> > +               dev_err(dev, "%s error: %d\n", __func__, ret);
> > +
> > +       return ret;
> > +}
> > +
> > +/*
> > + * fxl6408_is_output() - check whether the gpio configures as either
> > + *                      output or input.
> > + * Return: 0 - input, 1 - output.
> > + */
> > +static int fxl6408_is_output(struct udevice *dev, int offset)
>
> s/int/bool/ and you can drop the !! here and below

Done, thanks!

> > +{
> > +       struct fxl6408_info *info = dev_get_plat(dev);
> > +
> > +       return !!(info->reg_io_dir & BIT(offset));
> > +}
> > +
> > +static int fxl6408_get_value(struct udevice *dev, uint offset)
> > +{
> > +       int ret, reg = fxl6408_is_output(dev, offset) ? FXL6408_OUTPUT : FXL6408_INPUT;
> > +       u8 val = 0;
> > +
> > +       ret = fxl6408_read(dev, reg, &val);
> > +       if (IS_ERR_VALUE(ret))
> > +               return ret;
> > +
> > +       return !!(val & BIT(offset));
> > +}
> > +
> > +static int fxl6408_set_value(struct udevice *dev, uint offset, int value)
> > +{
> > +       struct fxl6408_info *info = dev_get_plat(dev);
> > +       u8 val;
> > +       int ret;
> > +
> > +       if (value)
> > +               val = info->reg_output | BIT(offset);
> > +       else
> > +               val = info->reg_output & ~BIT(offset);
> > +
> > +       ret = fxl6408_write(dev, FXL6408_OUTPUT, val);
> > +       if (!IS_ERR_VALUE(ret))
>
> if (ret < 0) is good enough here
>
> We are not going to change that, and the macros obfuscates things IMO.

Replaced, thanks.

> > +               info->reg_output = val;
> > +
> > +       return ret;
> > +}
> > +
> > +static int fxl6408_set_direction(struct udevice *dev, uint offset, int dir)
> > +{
> > +       struct fxl6408_info *info = dev_get_plat(dev);
> > +       u8 val;
> > +       int ret;
> > +
> > +       if (dir == FXL6408_DIRECTION_IN)
>
> Then shouldn't 'dir' be an enum?

Good point! Done.

> > +               val = info->reg_io_dir & ~BIT(offset);
> > +       else
> > +               val = info->reg_io_dir | BIT(offset);
> > +
> > +       ret = fxl6408_write(dev, FXL6408_DIRECTION, val);
> > +       if (!IS_ERR_VALUE(ret))
> > +               info->reg_io_dir = val;
> > +
> > +       return ret;
> > +}
> > +
> > +static int fxl6408_direction_input(struct udevice *dev, uint offset)
> > +{
> > +       return fxl6408_set_direction(dev, offset, FXL6408_DIRECTION_IN);
> > +}
> > +
> > +static int fxl6408_direction_output(struct udevice *dev, uint offset, int value)
> > +{
> > +       int ret;
> > +
> > +       /* Configure output value. */
> > +       ret = fxl6408_set_value(dev, offset, value);
> > +       if (!IS_ERR_VALUE(ret))
> > +               /* Configure direction as output. */
> > +               fxl6408_set_direction(dev, offset, FXL6408_DIRECTION_OUT);
> > +
> > +       return ret;
> > +}
> > +
> > +static int fxl6408_get_function(struct udevice *dev, uint offset)
> > +{
> > +       if (fxl6408_is_output(dev, offset))
> > +               return GPIOF_OUTPUT;
> > +       else
> > +               return GPIOF_INPUT;
> > +}
> > +
> > +static int fxl6408_xlate(struct udevice *dev, struct gpio_desc *desc,
> > +                        struct ofnode_phandle_args *args)
> > +{
> > +       desc->offset = args->args[0];
> > +       desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dm_gpio_ops fxl6408_ops = {
> > +       .direction_input        = fxl6408_direction_input,
> > +       .direction_output       = fxl6408_direction_output,

I've just found out these callbacks are deprecated. So I'll add
set_flags and get_flags callback's
implementations in the next version.

> > +       .get_value              = fxl6408_get_value,
> > +       .set_value              = fxl6408_set_value,
> > +       .get_function           = fxl6408_get_function,
> > +       .xlate                  = fxl6408_xlate,
> > +};
>
> Can you put that down below with the other struct?

Done.

> > +
> > +static int fxl6408_probe(struct udevice *dev)
> > +{
> > +       struct fxl6408_info *info = dev_get_plat(dev);
> > +       struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> > +       char name[32], label[32], *str;
> > +       int addr;
> > +       int ret;
> > +       int size;
> > +       const u8 *tmp;
> > +       u8 val;
> > +       u32 val32;
> > +
> > +       addr = dev_read_addr(dev);
> > +       if (addr == 0)
> > +               return -ENODEV;
>
> This has a special meaning in drive model. Try -EINVAL instead for
> errors reading the DT.

Thanks! Fixed.

> > +       info->addr = addr;
> > +
> > +       /* Check the device ID register to see if it's responding.
>
> Multi-line comment style is:

Thanks, fixed!

> /*
>  * Check the
>  * ..
>  */
> > +        * This also clears RST_INT as a side effect, so we won't get
> > +        * the "we've been power cycled" interrupt once we enable
> > +        * interrupts.
> > +        */
> > +       ret = fxl6408_read(dev, FXL6408_DEVID_CTRL, &val);
> > +       if (IS_ERR_VALUE(ret)) {
> > +               dev_err(dev, "FXL6408 probe returned %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       if (FXL6408_MF_ID(val) != FXL6408_FAIRCHILD_MF) {
> > +               dev_err(dev, "FXL6408 probe returned DID: 0x%02x\n", val);
> > +               return -ENODEV;
>
> Again you cannot return this, as there is a device. How about -ENXIO?

Sounds logical. Fixed.

> > +       }
> > +       info->device_id = val;
> > +
> > +       /*
> > +        * Disable High-Z of outputs, so that the OUTPUT updates
> > +        * actually take effect.
> > +        */
> > +       ret = fxl6408_write(dev, FXL6408_OUTPUT_HIGH_Z, (u8)0);
> > +       if (IS_ERR_VALUE(ret)) {
> > +               dev_err(dev, "Error writing High-Z register\n");
> > +               return ret;
> > +       }
> > +
> > +       /*
> > +        * If configured, set initial output state and direction,
> > +        * otherwise read them from the chip.
> > +        */
> > +       if (IS_ERR_VALUE(dev_read_u32(dev, "initial_io_dir", &val32))) {
> > +               ret = fxl6408_read(dev, FXL6408_DIRECTION, &info->reg_io_dir);
> > +               if (IS_ERR_VALUE(ret)) {
> > +                       dev_err(dev, "Error reading direction register\n");
> > +                       return ret;
> > +               }
> > +       } else {
> > +               info->reg_io_dir = val32 & 0xFF;
> > +               ret = fxl6408_write(dev, FXL6408_DIRECTION, info->reg_io_dir);
> > +               if (IS_ERR_VALUE(ret)) {
> > +                       dev_err(dev, "Error setting direction register\n");
> > +                       return ret;
> > +               }
> > +       }
> > +
> > +       if (IS_ERR_VALUE(dev_read_u32(dev, "initial_output", &val32))) {
> > +               ret = fxl6408_read(dev, FXL6408_OUTPUT, &info->reg_output);
> > +               if (IS_ERR_VALUE(ret)) {
> > +                       dev_err(dev, "Error reading output register\n");
> > +                       return ret;
> > +               }
> > +       } else {
> > +               info->reg_output = val32 & 0xFF;
> > +               ret = fxl6408_write(dev, FXL6408_OUTPUT, info->reg_output);
> > +               if (IS_ERR_VALUE(ret)) {
> > +                       dev_err(dev, "Error setting output register\n");
> > +                       return ret;
> > +               }
> > +       }
> > +
> > +       tmp = dev_read_prop(dev, "label", &size);
> > +       if (tmp) {
> > +               size = min(size, (int)sizeof(label) - 1);
> > +               memcpy(label, tmp, size);
> > +               label[size] = '\0';
> > +               snprintf(name, sizeof(name), "%s@%x_", label, info->addr);
> > +       } else {
> > +               snprintf(name, sizeof(name), "gpio@%x_", info->addr);
> > +       }
> > +
> > +       str = strdup(name);
> > +       if (!str)
> > +               return -ENOMEM;
> > +
> > +       uc_priv->bank_name = str;
> > +       uc_priv->gpio_count = dev_get_driver_data(dev);
> > +       uc_priv->gpio_base = -1;
> > +
> > +       dev_dbg(dev, "%s (FW rev. %ld) is ready\n", str,
> > +               FXL6408_FW_REV(info->device_id));
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct udevice_id fxl6408_ids[] = {
> > +       { .compatible = "fcs,fxl6408", .data = 8 },
> > +       { }
> > +};
> > +
> > +U_BOOT_DRIVER(fxl6408_gpio) = {
> > +       .name = "fxl6408_gpio",
> > +       .id = UCLASS_GPIO,
> > +       .ops = &fxl6408_ops,
> > +       .probe = fxl6408_probe,
> > +       .of_match = fxl6408_ids,
> > +       .plat_auto = sizeof(struct fxl6408_info),
> > +};
> > --
> > 2.31.1
> >
>
> Regards,
> Simon

Best regards
Oleksandr Suvorov

Toradex AG
Ebenaustrasse 10 | 6048 Horw | Switzerland | T: +41 41 500 48 00


More information about the U-Boot mailing list