| Message ID | 20210909234212.v3.1.ce74fbdbdc@changeid |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | Add support of an FXL6408 GPIO expander | expand |
Hello Oleksandr, On 09.09.21 22:44, Oleksandr Suvorov wrote: > From: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > > 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> > Reviewed-by: Simon Glass <sjg@chromium.org> > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > Signed-off-by: Oleksandr Suvorov <cryosay@gmail.com> > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io> > --- > > Changes in v3: > - fix a warning: > ...drivers/gpio/gpio-fxl6408.c:348:15: warning: format ‘%ld’ > expects argument of type ‘long int’, but argument 3 has > type ‘int’ [-Wformat=]... > - add Tested-by record. > > drivers/gpio/Kconfig | 7 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-fxl6408.c | 366 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 374 insertions(+) > create mode 100644 drivers/gpio/gpio-fxl6408.c Reviewed-by: Heiko Schocher <hs@denx.de> bye, Heiko
On 9/9/21 10:44 PM, Oleksandr Suvorov wrote: > From: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > > 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> > Reviewed-by: Simon Glass <sjg@chromium.org> > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > Signed-off-by: Oleksandr Suvorov <cryosay@gmail.com> > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io> 3 emails for you. Would be the best to choose only one. > --- > > Changes in v3: > - fix a warning: > ...drivers/gpio/gpio-fxl6408.c:348:15: warning: format ‘%ld’ > expects argument of type ‘long int’, but argument 3 has > type ‘int’ [-Wformat=]... > - add Tested-by record. > > drivers/gpio/Kconfig | 7 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-fxl6408.c | 366 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 374 insertions(+) > create mode 100644 drivers/gpio/gpio-fxl6408.c > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 4a89c1a62b..f56e4cc261 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 expander driver" > + depends on DM_GPIO && DM_I2C > + help > + This driver supports the Fairchild FXL6408 device. FXL6408 is a > + fully configurable 8-bit I2C-controlled GPIO expander. > + > config INTEL_BROADWELL_GPIO > bool "Intel Broadwell GPIO driver" > depends on DM > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > index 58f4704f6b..83d8b5c9d8 100644 > --- a/drivers/gpio/Makefile > +++ b/drivers/gpio/Makefile > @@ -16,6 +16,7 @@ obj-$(CONFIG_AT91_GPIO) += at91_gpio.o > obj-$(CONFIG_ATMEL_PIO4) += atmel_pio4.o > obj-$(CONFIG_BCM6345_GPIO) += bcm6345_gpio.o > obj-$(CONFIG_CORTINA_GPIO) += cortina_gpio.o > +obj-$(CONFIG_FXL6408_GPIO) += gpio-fxl6408.o > obj-$(CONFIG_INTEL_GPIO) += intel_gpio.o > obj-$(CONFIG_INTEL_ICH6_GPIO) += intel_ich6_gpio.o > obj-$(CONFIG_INTEL_BROADWELL_GPIO) += intel_broadwell_gpio.o > diff --git a/drivers/gpio/gpio-fxl6408.c b/drivers/gpio/gpio-fxl6408.c > new file mode 100644 > index 0000000000..dbc68618f9 > --- /dev/null > +++ b/drivers/gpio/gpio-fxl6408.c > @@ -0,0 +1,366 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2021 Toradex > + * Copyright (C) 2016 Broadcom > + */ > + > +/** Below is not kernel-doc format > + * 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@anholt.net>: > + * https://patchwork.kernel.org/patch/9148419/ > + * - the Toradex version by Max Krummenacher <max.krummenacher@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@gmail.com>: > + * drivers/gpio/pca953x_gpio.c > + * > + * TODO: > + * - Add interrupts support > + * - Replace deprecated callbacks direction_input/output() with set_flags() > + */ > + > +#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> please sort it. > + > +#define REG_DEVID_CTRL 0x01 0x1 > +# define SW_RST BIT(0) > +# define RST_INT BIT(1) > +/* 0b101 is the Manufacturer's ID assigned to Fairchild by Nokia */ > +# define MF_ID_FAIRCHILD 5 > + > +/* Bits set here indicate that the GPIO is an output */ > +#define REG_IO_DIR 0x03 0x3 > + > +/* > + * Bits set here, when the corresponding bit of REG_IO_DIR is set, drive > + * the output high instead of low. > + */ > +#define REG_OUT_STATE 0x05 ditto > + > +/* Bits here make the output High-Z, instead of the OUTPUT value */ > +#define REG_OUT_HIGH_Z 0x07 ditto > + > +/* > + * 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 REG_IN_DEFAULT_STATE 0x09 ditto. > + > +/* > + * Bits here enable either pull up or pull down according to > + * REG_PULL_MODE. > + */ > +#define REG_PULL_ENABLE 0x0b ditto. > + > +/* > + * Bits set here selects a pull-up/pull-down state of pin, which > + * is configured as Input and the corresponding REG_PULL_ENABLE bit is > + * set. > + */ > +#define REG_PULL_MODE 0x0d ditto > + > +/* Returns the current status (1 = HIGH) of the input pins */ > +#define REG_IN_STATUS 0x0f ditto > + > +/* Mask of pins which can generate interrupts */ > +#define REG_INT_MASK 0x11 > + > +/* Mask of pins which have generated an interrupt. Cleared on read */ > +#define REG_INT_STATUS 0x13 > + > +/* Manufacturer's ID getting from Device ID & Ctrl register */ > +enum { > + MF_ID_MASK = GENMASK(7, 5), > + MF_ID_SHIFT = 5, This smells. Take a look at include/linux/bitfield.h you should be able to use that macros to get fields you want. > +}; > + > +/* Firmware revision getting from Device ID & Ctrl register */ > +enum { > + FW_REV_MASK = GENMASK(4, 2), > + FW_REV_SHIFT = 2, ditto. > +}; > + > +enum io_direction { > + DIR_IN, > + DIR_OUT, good style is to initialized them. > +}; > + > +/* /** this should be kernel doc. Run this ./scripts/kernel-doc -man -v 1>/dev/null *.c to check that all values are right. > + * struct fxl6408_info - Data for fxl6408 > + * > + * @dev: udevice structure for the device > + * @addr: i2c slave address > + * @reg_io_dir: hold the value of direction register > + * @reg_output: hold the value of output register > + */ > +struct fxl6408_info { > + struct udevice *dev; > + int addr; > + u8 device_id; > + u8 reg_io_dir; > + u8 reg_output; > +}; > + > +static inline int fxl6408_write(struct udevice *dev, int reg, u8 val) > +{ > + return dm_i2c_write(dev, reg, &val, 1); > +} > + > +static int fxl6408_read(struct udevice *dev, int reg) > +{ > + int ret; > + u8 tmp; > + > + ret = dm_i2c_read(dev, reg, &tmp, 1); > + if (!ret) > + ret = tmp; This looks completely wrong. What value are you returning in case of error. If ops->xfer is not defined dm_i2c_read returns -ENOSYS. tmp is not initialized and can have random value that's why here in case or error you will return ramdom value. > + > + return ret; > +} > + > +/* /** > + * fxl6408_is_output() - check whether the gpio configures as either > + * output or input. > + * Return: false - input, true - output. > + */ > +static bool fxl6408_is_output(struct udevice *dev, int offset) > +{ > + 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) ? REG_OUT_STATE : REG_IN_STATUS; > + > + ret = fxl6408_read(dev, reg); > + if (ret < 0) > + return ret; > + > + return !!(ret & 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, REG_OUT_STATE, val); > + if (ret < 0) > + return ret; > + > + info->reg_output = val; > + > + return 0; > +} > + > +static int fxl6408_set_direction(struct udevice *dev, uint offset, > + enum io_direction dir) > +{ > + struct fxl6408_info *info = dev_get_plat(dev); > + u8 val; > + int ret; > + > + if (dir == DIR_IN) > + val = info->reg_io_dir & ~BIT(offset); > + else > + val = info->reg_io_dir | BIT(offset); > + > + ret = fxl6408_write(dev, REG_IO_DIR, val); > + if (ret < 0) > + return ret; > + > + info->reg_io_dir = val; > + > + return 0; > +} > + > +static int fxl6408_direction_input(struct udevice *dev, uint offset) > +{ > + return fxl6408_set_direction(dev, offset, DIR_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 (ret < 0) > + return ret; newline > + /* Configure direction as output */ > + fxl6408_set_direction(dev, offset, DIR_OUT); > + > + return 0; > +} > + > +static int fxl6408_get_function(struct udevice *dev, uint offset) > +{ > + if (fxl6408_is_output(dev, offset)) > + return GPIOF_OUTPUT; > + else you can remove this 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, > + .get_value = fxl6408_get_value, > + .set_value = fxl6408_set_value, > + .get_function = fxl6408_get_function, > + .xlate = fxl6408_xlate, > +}; > + > +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; put them on the same line. > + const u8 *tmp; > + u32 val32; > + > + addr = dev_read_addr(dev); > + if (addr == 0) > + return -EINVAL; newline > + info->addr = addr; > + > + /* > + * Check the device ID register to see if it's responding. > + * This also clears RST_INT as a side effect, so we won't get > + * the "we've been power cycled" interrupt once interrupts > + * being enabled. > + */ > + ret = fxl6408_read(dev, REG_DEVID_CTRL); > + if (ret < 0) { > + dev_err(dev, "FXL6408 probe returned %d\n", ret); > + return ret; > + } > + > + if ((ret & MF_ID_MASK) >> MF_ID_SHIFT != MF_ID_FAIRCHILD) { > + dev_err(dev, "FXL6408 probe: wrong Manufacturer's ID: 0x%02x\n", ret); > + return -ENXIO; > + } > + info->device_id = ret; > + > + /* > + * Disable High-Z of outputs, so that the OUTPUT updates > + * actually take effect. > + */ > + ret = fxl6408_write(dev, REG_OUT_HIGH_Z, (u8)0); > + if (ret < 0) { > + 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 (dev_read_u32(dev, "initial_io_dir", &val32)) { Where do you have dt binding document for this chip? I can't see anything in the linux kernel or in your series. It is good that you read values from dt but you shouldn't do it in probe. You should create platdata and fill it by information from DT and then in probe just read them from platdata structure. This applied to all dt read in probe. > + ret = fxl6408_read(dev, REG_IO_DIR); > + if (ret < 0) { > + dev_err(dev, "Error reading direction register\n"); > + return ret; > + } > + info->reg_io_dir = ret; > + } else { > + info->reg_io_dir = val32 & 0xFF; val32 is not initialized above. dev_read_u32 above fails that's why val32 wont't be initialized and here you do calculation with it. That's quite weird behavior. The same pattern visible below. > + ret = fxl6408_write(dev, REG_IO_DIR, info->reg_io_dir); > + if (ret < 0) { > + dev_err(dev, "Error setting direction register\n"); > + return ret; > + } > + } > + > + if (dev_read_u32(dev, "initial_output", &val32)) { > + ret = fxl6408_read(dev, REG_OUT_STATE); > + if (ret < 0) { > + dev_err(dev, "Error reading output register\n"); > + return ret; > + } > + info->reg_output = ret; > + } else { > + info->reg_output = val32 & 0xFF; > + ret = fxl6408_write(dev, REG_OUT_STATE, info->reg_output); > + if (ret < 0) { > + 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); > + } I see some code around labels in gpio-uclass. I would be surprised if it is not there already because it should be core functionality not driver when labels are defined. > + > + 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. %d) is ready\n", str, > + (info->device_id & MF_ID_MASK) >> MF_ID_SHIFT); > + > + return 0; > +} > + > +static const struct udevice_id fxl6408_ids[] = { > + { .compatible = "fcs,fxl6408", .data = 8 }, nit: Are you going to support more chip that you have data here? > + { } > +}; > + > +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), > +}; > Thanks, Michal
Hi Oleksandr, On Fri, Sep 10, 2021 at 08:34:56AM +0200, Michal Simek wrote: > On 9/9/21 10:44 PM, Oleksandr Suvorov wrote: > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > > 3 emails for you. Would be the best to choose only one. And to use a valid one, oleksandr.suvorov@toradex.com is gone as today, I understand and appreciate the very honest desire to give credit to the company that paid for this work (if I understand correctly), but on the other end using an email address that is going to bounce does not look like the correct solution. Francesco
Hi Michal, On Fri, Sep 10, 2021 at 9:35 AM Michal Simek <michal.simek@xilinx.com> wrote: > > > > On 9/9/21 10:44 PM, Oleksandr Suvorov wrote: > > From: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > > > > 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> > > Reviewed-by: Simon Glass <sjg@chromium.org> > > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > Signed-off-by: Oleksandr Suvorov <cryosay@gmail.com> > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io> > > 3 emails for you. Would be the best to choose only one. Thanks, fixed. That habit to always add '-s' when committing changes :) > > > --- > > > > Changes in v3: > > - fix a warning: > > ...drivers/gpio/gpio-fxl6408.c:348:15: warning: format ‘%ld’ > > expects argument of type ‘long int’, but argument 3 has > > type ‘int’ [-Wformat=]... > > - add Tested-by record. > > > > drivers/gpio/Kconfig | 7 + > > drivers/gpio/Makefile | 1 + > > drivers/gpio/gpio-fxl6408.c | 366 ++++++++++++++++++++++++++++++++++++ > > 3 files changed, 374 insertions(+) > > create mode 100644 drivers/gpio/gpio-fxl6408.c > > > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > > index 4a89c1a62b..f56e4cc261 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 expander driver" > > + depends on DM_GPIO && DM_I2C > > + help > > + This driver supports the Fairchild FXL6408 device. FXL6408 is a > > + fully configurable 8-bit I2C-controlled GPIO expander. > > + > > config INTEL_BROADWELL_GPIO > > bool "Intel Broadwell GPIO driver" > > depends on DM > > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > > index 58f4704f6b..83d8b5c9d8 100644 > > --- a/drivers/gpio/Makefile > > +++ b/drivers/gpio/Makefile > > @@ -16,6 +16,7 @@ obj-$(CONFIG_AT91_GPIO) += at91_gpio.o > > obj-$(CONFIG_ATMEL_PIO4) += atmel_pio4.o > > obj-$(CONFIG_BCM6345_GPIO) += bcm6345_gpio.o > > obj-$(CONFIG_CORTINA_GPIO) += cortina_gpio.o > > +obj-$(CONFIG_FXL6408_GPIO) += gpio-fxl6408.o > > obj-$(CONFIG_INTEL_GPIO) += intel_gpio.o > > obj-$(CONFIG_INTEL_ICH6_GPIO) += intel_ich6_gpio.o > > obj-$(CONFIG_INTEL_BROADWELL_GPIO) += intel_broadwell_gpio.o > > diff --git a/drivers/gpio/gpio-fxl6408.c b/drivers/gpio/gpio-fxl6408.c > > new file mode 100644 > > index 0000000000..dbc68618f9 > > --- /dev/null > > +++ b/drivers/gpio/gpio-fxl6408.c > > @@ -0,0 +1,366 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2021 Toradex > > + * Copyright (C) 2016 Broadcom > > + */ > > + > > +/** > > Below is not kernel-doc format I don't think so. And without formatting comments of this block as a kernel-doc, the kernel-doc tool shows the warning: drivers/gpio/gpio-fxl6408.c:1: warning: no structured comments found Otherwise, it generates the correct description of the file: **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. ... > > + * 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@anholt.net>: > > + * https://patchwork.kernel.org/patch/9148419/ > > + * - the Toradex version by Max Krummenacher <max.krummenacher@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@gmail.com>: > > + * drivers/gpio/pca953x_gpio.c > > + * > > + * TODO: > > + * - Add interrupts support > > + * - Replace deprecated callbacks direction_input/output() with set_flags() > > + */ > > + > > +#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> > > please sort it. Done. > > > + > > +#define REG_DEVID_CTRL 0x01 > > 0x1 Here and for similar cases below: thanks, I'll fix it in the next revision. > > +# define SW_RST BIT(0) > > +# define RST_INT BIT(1) > > +/* 0b101 is the Manufacturer's ID assigned to Fairchild by Nokia */ > > +# define MF_ID_FAIRCHILD 5 > > + > > +/* Bits set here indicate that the GPIO is an output */ > > +#define REG_IO_DIR 0x03 > > 0x3 > > > + > > +/* > > + * Bits set here, when the corresponding bit of REG_IO_DIR is set, drive > > + * the output high instead of low. > > + */ > > +#define REG_OUT_STATE 0x05 > > ditto > > > + > > +/* Bits here make the output High-Z, instead of the OUTPUT value */ > > +#define REG_OUT_HIGH_Z 0x07 > > ditto > > > > + > > +/* > > + * 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 REG_IN_DEFAULT_STATE 0x09 > > ditto. > > > + > > +/* > > + * Bits here enable either pull up or pull down according to > > + * REG_PULL_MODE. > > + */ > > +#define REG_PULL_ENABLE 0x0b > > ditto. > > > + > > +/* > > + * Bits set here selects a pull-up/pull-down state of pin, which > > + * is configured as Input and the corresponding REG_PULL_ENABLE bit is > > + * set. > > + */ > > +#define REG_PULL_MODE 0x0d > > ditto > > > + > > +/* Returns the current status (1 = HIGH) of the input pins */ > > +#define REG_IN_STATUS 0x0f > > ditto > > > + > > +/* Mask of pins which can generate interrupts */ > > +#define REG_INT_MASK 0x11 > > + > > +/* Mask of pins which have generated an interrupt. Cleared on read */ > > +#define REG_INT_STATUS 0x13 > > + > > +/* Manufacturer's ID getting from Device ID & Ctrl register */ > > +enum { > > + MF_ID_MASK = GENMASK(7, 5), > > + MF_ID_SHIFT = 5, > > This smells. Take a look at include/linux/bitfield.h > you should be able to use that macros to get fields you want. > > > +}; > > + > > +/* Firmware revision getting from Device ID & Ctrl register */ > > +enum { > > + FW_REV_MASK = GENMASK(4, 2), > > + FW_REV_SHIFT = 2, > > ditto. > > > +}; > > + > > +enum io_direction { > > + DIR_IN, > > + DIR_OUT, > > good style is to initialized them. Thanks, I'll fix it in the next revision. > > > +}; > > + > > +/* > > /** this should be kernel doc. > > Run this > ./scripts/kernel-doc -man -v 1>/dev/null *.c > > to check that all values are right. The kernel-doc doesn't found errors. > > + * struct fxl6408_info - Data for fxl6408 > > + * > > + * @dev: udevice structure for the device > > + * @addr: i2c slave address > > + * @reg_io_dir: hold the value of direction register > > + * @reg_output: hold the value of output register > > + */ > > +struct fxl6408_info { > > + struct udevice *dev; > > + int addr; > > + u8 device_id; > > + u8 reg_io_dir; > > + u8 reg_output; > > +}; > > + > > +static inline int fxl6408_write(struct udevice *dev, int reg, u8 val) > > +{ > > + return dm_i2c_write(dev, reg, &val, 1); > > +} > > + > > +static int fxl6408_read(struct udevice *dev, int reg) > > +{ > > + int ret; > > + u8 tmp; > > + > > + ret = dm_i2c_read(dev, reg, &tmp, 1); > > + if (!ret) > > + ret = tmp; > > This looks completely wrong. What value are you returning in case of error. Nope. In case of error, I return an error value stored in "ret". > If ops->xfer is not defined dm_i2c_read returns -ENOSYS. tmp is not > initialized and can have random value that's why here in case or error > you will return ramdom value. !(-ENOSYS) == false, thus "if" op fails and doesn't perform "ret = tmp". I don't see any errors here. > > + > > + return ret; > > +} > > + > > +/* > > /** > > > + * fxl6408_is_output() - check whether the gpio configures as either > > + * output or input. > > + * Return: false - input, true - output. > > + */ > > +static bool fxl6408_is_output(struct udevice *dev, int offset) > > +{ > > + 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) ? REG_OUT_STATE : REG_IN_STATUS; > > + > > + ret = fxl6408_read(dev, reg); > > + if (ret < 0) > > + return ret; > > + > > + return !!(ret & 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, REG_OUT_STATE, val); > > + if (ret < 0) > > + return ret; > > + > > + info->reg_output = val; > > + > > + return 0; > > +} > > + > > +static int fxl6408_set_direction(struct udevice *dev, uint offset, > > + enum io_direction dir) > > +{ > > + struct fxl6408_info *info = dev_get_plat(dev); > > + u8 val; > > + int ret; > > + > > + if (dir == DIR_IN) > > + val = info->reg_io_dir & ~BIT(offset); > > + else > > + val = info->reg_io_dir | BIT(offset); > > + > > + ret = fxl6408_write(dev, REG_IO_DIR, val); > > + if (ret < 0) > > + return ret; > > + > > + info->reg_io_dir = val; > > + > > + return 0; > > +} > > + > > +static int fxl6408_direction_input(struct udevice *dev, uint offset) > > +{ > > + return fxl6408_set_direction(dev, offset, DIR_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 (ret < 0) > > + return ret; > > newline Done. > > > + /* Configure direction as output */ > > + fxl6408_set_direction(dev, offset, DIR_OUT); > > + > > + return 0; > > +} > > + > > +static int fxl6408_get_function(struct udevice *dev, uint offset) > > +{ > > + if (fxl6408_is_output(dev, offset)) > > + return GPIOF_OUTPUT; > > + else > > you can remove this else Sure, thank you. > > + 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, > > + .get_value = fxl6408_get_value, > > + .set_value = fxl6408_set_value, > > + .get_function = fxl6408_get_function, > > + .xlate = fxl6408_xlate, > > +}; > > + > > +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; > > put them on the same line. Done, thanks. > > + const u8 *tmp; > > + u32 val32; > > + > > + addr = dev_read_addr(dev); > > + if (addr == 0) > > + return -EINVAL; > > newline Done. > > + info->addr = addr; > > + > > + /* > > + * Check the device ID register to see if it's responding. > > + * This also clears RST_INT as a side effect, so we won't get > > + * the "we've been power cycled" interrupt once interrupts > > + * being enabled. > > + */ > > + ret = fxl6408_read(dev, REG_DEVID_CTRL); > > + if (ret < 0) { > > + dev_err(dev, "FXL6408 probe returned %d\n", ret); > > + return ret; > > + } > > + > > + if ((ret & MF_ID_MASK) >> MF_ID_SHIFT != MF_ID_FAIRCHILD) { > > + dev_err(dev, "FXL6408 probe: wrong Manufacturer's ID: 0x%02x\n", ret); > > + return -ENXIO; > > + } > > + info->device_id = ret; > > + > > + /* > > + * Disable High-Z of outputs, so that the OUTPUT updates > > + * actually take effect. > > + */ > > + ret = fxl6408_write(dev, REG_OUT_HIGH_Z, (u8)0); > > + if (ret < 0) { > > + 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 (dev_read_u32(dev, "initial_io_dir", &val32)) { > > Where do you have dt binding document for this chip? I can't see > anything in the linux kernel or in your series. > > It is good that you read values from dt but you shouldn't do it in > probe. You should create platdata and fill it by information from DT and > then in probe just read them from platdata structure. > This applied to all dt read in probe. > > > > + ret = fxl6408_read(dev, REG_IO_DIR); > > + if (ret < 0) { > > + dev_err(dev, "Error reading direction register\n"); > > + return ret; > > + } > > + info->reg_io_dir = ret; > > + } else { > > + info->reg_io_dir = val32 & 0xFF; > > val32 is not initialized above. dev_read_u32 above fails that's why > val32 wont't be initialized and here you do calculation with it. > That's quite weird behavior. The same pattern visible below. Nope. dev_read_u32() returns 0 on success. The logic is correct. > > + ret = fxl6408_write(dev, REG_IO_DIR, info->reg_io_dir); > > + if (ret < 0) { > > + dev_err(dev, "Error setting direction register\n"); > > + return ret; > > + } > > + } > > + > > + if (dev_read_u32(dev, "initial_output", &val32)) { > > + ret = fxl6408_read(dev, REG_OUT_STATE); > > + if (ret < 0) { > > + dev_err(dev, "Error reading output register\n"); > > + return ret; > > + } > > + info->reg_output = ret; > > + } else { > > + info->reg_output = val32 & 0xFF; > > + ret = fxl6408_write(dev, REG_OUT_STATE, info->reg_output); > > + if (ret < 0) { > > + 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); > > + } > > I see some code around labels in gpio-uclass. I would be surprised if it > is not there already because it should be core functionality not driver > when labels are defined. This code is not about gpios labeling. It's about the bank name, it just sets the uc_priv->bank_name which is used in gpio-uclass. > > + > > + 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. %d) is ready\n", str, > > + (info->device_id & MF_ID_MASK) >> MF_ID_SHIFT); > > + > > + return 0; > > +} > > + > > +static const struct udevice_id fxl6408_ids[] = { > > + { .compatible = "fcs,fxl6408", .data = 8 }, > > nit: Are you going to support more chip that you have data here? I can't be sure - I don't know plans of Fairchild or other manufacturers about gpio devices similar to that one :) > > > + { } > > +}; > > + > > +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), > > +}; > > > > Thanks, > Michal Thanks for such a detailed and helpful review! -- Best regards Oleksandr Oleksandr Suvorov cryosay@gmail.com
On 9/15/21 8:35 PM, Oleksandr Suvorov wrote: > Hi Michal, > > On Fri, Sep 10, 2021 at 9:35 AM Michal Simek <michal.simek@xilinx.com> wrote: >> >> >> >> On 9/9/21 10:44 PM, Oleksandr Suvorov wrote: >>> From: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> >>> >>> 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> >>> Reviewed-by: Simon Glass <sjg@chromium.org> >>> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> >>> Signed-off-by: Oleksandr Suvorov <cryosay@gmail.com> >>> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io> >> >> 3 emails for you. Would be the best to choose only one. > > Thanks, fixed. That habit to always add '-s' when committing changes :) >> >>> --- >>> >>> Changes in v3: >>> - fix a warning: >>> ...drivers/gpio/gpio-fxl6408.c:348:15: warning: format ‘%ld’ >>> expects argument of type ‘long int’, but argument 3 has >>> type ‘int’ [-Wformat=]... >>> - add Tested-by record. >>> >>> drivers/gpio/Kconfig | 7 + >>> drivers/gpio/Makefile | 1 + >>> drivers/gpio/gpio-fxl6408.c | 366 ++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 374 insertions(+) >>> create mode 100644 drivers/gpio/gpio-fxl6408.c >>> >>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig >>> index 4a89c1a62b..f56e4cc261 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 expander driver" >>> + depends on DM_GPIO && DM_I2C >>> + help >>> + This driver supports the Fairchild FXL6408 device. FXL6408 is a >>> + fully configurable 8-bit I2C-controlled GPIO expander. >>> + >>> config INTEL_BROADWELL_GPIO >>> bool "Intel Broadwell GPIO driver" >>> depends on DM >>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile >>> index 58f4704f6b..83d8b5c9d8 100644 >>> --- a/drivers/gpio/Makefile >>> +++ b/drivers/gpio/Makefile >>> @@ -16,6 +16,7 @@ obj-$(CONFIG_AT91_GPIO) += at91_gpio.o >>> obj-$(CONFIG_ATMEL_PIO4) += atmel_pio4.o >>> obj-$(CONFIG_BCM6345_GPIO) += bcm6345_gpio.o >>> obj-$(CONFIG_CORTINA_GPIO) += cortina_gpio.o >>> +obj-$(CONFIG_FXL6408_GPIO) += gpio-fxl6408.o >>> obj-$(CONFIG_INTEL_GPIO) += intel_gpio.o >>> obj-$(CONFIG_INTEL_ICH6_GPIO) += intel_ich6_gpio.o >>> obj-$(CONFIG_INTEL_BROADWELL_GPIO) += intel_broadwell_gpio.o >>> diff --git a/drivers/gpio/gpio-fxl6408.c b/drivers/gpio/gpio-fxl6408.c >>> new file mode 100644 >>> index 0000000000..dbc68618f9 >>> --- /dev/null >>> +++ b/drivers/gpio/gpio-fxl6408.c >>> @@ -0,0 +1,366 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Copyright (C) 2021 Toradex >>> + * Copyright (C) 2016 Broadcom >>> + */ >>> + >>> +/** >> >> Below is not kernel-doc format > > I don't think so. And without formatting comments of this block as a > kernel-doc, the kernel-doc tool shows the warning: > drivers/gpio/gpio-fxl6408.c:1: warning: no structured comments found Because script only checks format starting with /**. <snip> >>> +}; >>> + >>> +/* >> >> /** this should be kernel doc. >> >> Run this >> ./scripts/kernel-doc -man -v 1>/dev/null *.c >> >> to check that all values are right. > > The kernel-doc doesn't found errors. Because it doesn't find any structure to check. Add here /** and run that script again. > >>> + * struct fxl6408_info - Data for fxl6408 >>> + * >>> + * @dev: udevice structure for the device >>> + * @addr: i2c slave address >>> + * @reg_io_dir: hold the value of direction register >>> + * @reg_output: hold the value of output register >>> + */ >>> +struct fxl6408_info { >>> + struct udevice *dev; >>> + int addr; >>> + u8 device_id; >>> + u8 reg_io_dir; >>> + u8 reg_output; >>> +}; >>> + >>> +static inline int fxl6408_write(struct udevice *dev, int reg, u8 val) >>> +{ >>> + return dm_i2c_write(dev, reg, &val, 1); >>> +} >>> + >>> +static int fxl6408_read(struct udevice *dev, int reg) >>> +{ >>> + int ret; >>> + u8 tmp; >>> + >>> + ret = dm_i2c_read(dev, reg, &tmp, 1); >>> + if (!ret) >>> + ret = tmp; >> >> This looks completely wrong. What value are you returning in case of error. > > Nope. In case of error, I return an error value stored in "ret". > >> If ops->xfer is not defined dm_i2c_read returns -ENOSYS. tmp is not >> initialized and can have random value that's why here in case or error >> you will return ramdom value. > > !(-ENOSYS) == false, thus "if" op fails and doesn't perform "ret = tmp". > I don't see any errors here. You are right. I read it incorrectly. <snip> >>> + /* >>> + * If configured, set initial output state and direction, >>> + * otherwise read them from the chip. >>> + */ >>> + if (dev_read_u32(dev, "initial_io_dir", &val32)) { >> >> Where do you have dt binding document for this chip? I can't see >> anything in the linux kernel or in your series. >> >> It is good that you read values from dt but you shouldn't do it in >> probe. You should create platdata and fill it by information from DT and >> then in probe just read them from platdata structure. >> This applied to all dt read in probe. >> >> >>> + ret = fxl6408_read(dev, REG_IO_DIR); >>> + if (ret < 0) { >>> + dev_err(dev, "Error reading direction register\n"); >>> + return ret; >>> + } >>> + info->reg_io_dir = ret; >>> + } else { >>> + info->reg_io_dir = val32 & 0xFF; >> >> val32 is not initialized above. dev_read_u32 above fails that's why >> val32 wont't be initialized and here you do calculation with it. >> That's quite weird behavior. The same pattern visible below. > > Nope. dev_read_u32() returns 0 on success. The logic is correct. You are right again. I normally use reverse logic. >>> + ret = fxl6408_write(dev, REG_IO_DIR, info->reg_io_dir); >>> + if (ret < 0) { >>> + dev_err(dev, "Error setting direction register\n"); >>> + return ret; >>> + } >>> + } >>> + >>> + if (dev_read_u32(dev, "initial_output", &val32)) { >>> + ret = fxl6408_read(dev, REG_OUT_STATE); >>> + if (ret < 0) { >>> + dev_err(dev, "Error reading output register\n"); >>> + return ret; >>> + } >>> + info->reg_output = ret; >>> + } else { >>> + info->reg_output = val32 & 0xFF; >>> + ret = fxl6408_write(dev, REG_OUT_STATE, info->reg_output); >>> + if (ret < 0) { >>> + 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); >>> + } >> >> I see some code around labels in gpio-uclass. I would be surprised if it >> is not there already because it should be core functionality not driver >> when labels are defined. > > This code is not about gpios labeling. It's about the bank name, it > just sets the uc_priv->bank_name which is used in gpio-uclass. Can you please use different variable name then label. If this is bank_name just call it bank name. Thanks, Michal
i Michal, On Thu, Sep 16, 2021 at 10:05 AM Michal Simek <michal.simek@xilinx.com> wrote: > > > > On 9/15/21 8:35 PM, Oleksandr Suvorov wrote: > > Hi Michal, > > > > On Fri, Sep 10, 2021 at 9:35 AM Michal Simek <michal.simek@xilinx.com> wrote: > >> > >> > >> > >> On 9/9/21 10:44 PM, Oleksandr Suvorov wrote: > >>> From: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > >>> > >>> 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> > >>> Reviewed-by: Simon Glass <sjg@chromium.org> > >>> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > >>> Signed-off-by: Oleksandr Suvorov <cryosay@gmail.com> > >>> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io> > >> > >> 3 emails for you. Would be the best to choose only one. > > > > Thanks, fixed. That habit to always add '-s' when committing changes :) > >> > >>> --- > >>> > >>> Changes in v3: > >>> - fix a warning: > >>> ...drivers/gpio/gpio-fxl6408.c:348:15: warning: format ‘%ld’ > >>> expects argument of type ‘long int’, but argument 3 has > >>> type ‘int’ [-Wformat=]... > >>> - add Tested-by record. > >>> > >>> drivers/gpio/Kconfig | 7 + > >>> drivers/gpio/Makefile | 1 + > >>> drivers/gpio/gpio-fxl6408.c | 366 ++++++++++++++++++++++++++++++++++++ > >>> 3 files changed, 374 insertions(+) > >>> create mode 100644 drivers/gpio/gpio-fxl6408.c > >>> > >>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > >>> index 4a89c1a62b..f56e4cc261 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 expander driver" > >>> + depends on DM_GPIO && DM_I2C > >>> + help > >>> + This driver supports the Fairchild FXL6408 device. FXL6408 is a > >>> + fully configurable 8-bit I2C-controlled GPIO expander. > >>> + > >>> config INTEL_BROADWELL_GPIO > >>> bool "Intel Broadwell GPIO driver" > >>> depends on DM > >>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > >>> index 58f4704f6b..83d8b5c9d8 100644 > >>> --- a/drivers/gpio/Makefile > >>> +++ b/drivers/gpio/Makefile > >>> @@ -16,6 +16,7 @@ obj-$(CONFIG_AT91_GPIO) += at91_gpio.o > >>> obj-$(CONFIG_ATMEL_PIO4) += atmel_pio4.o > >>> obj-$(CONFIG_BCM6345_GPIO) += bcm6345_gpio.o > >>> obj-$(CONFIG_CORTINA_GPIO) += cortina_gpio.o > >>> +obj-$(CONFIG_FXL6408_GPIO) += gpio-fxl6408.o > >>> obj-$(CONFIG_INTEL_GPIO) += intel_gpio.o > >>> obj-$(CONFIG_INTEL_ICH6_GPIO) += intel_ich6_gpio.o > >>> obj-$(CONFIG_INTEL_BROADWELL_GPIO) += intel_broadwell_gpio.o > >>> diff --git a/drivers/gpio/gpio-fxl6408.c b/drivers/gpio/gpio-fxl6408.c > >>> new file mode 100644 > >>> index 0000000000..dbc68618f9 > >>> --- /dev/null > >>> +++ b/drivers/gpio/gpio-fxl6408.c > >>> @@ -0,0 +1,366 @@ > >>> +// SPDX-License-Identifier: GPL-2.0 > >>> +/* > >>> + * Copyright (C) 2021 Toradex > >>> + * Copyright (C) 2016 Broadcom > >>> + */ > >>> + > >>> +/** > >> > >> Below is not kernel-doc format > > > > I don't think so. And without formatting comments of this block as a > > kernel-doc, the kernel-doc tool shows the warning: > > drivers/gpio/gpio-fxl6408.c:1: warning: no structured comments found > > Because script only checks format starting with /**. Sure, missed that "little" thing :) Thanks! > > <snip> > > >>> +}; > >>> + > >>> +/* > >> > >> /** this should be kernel doc. > >> > >> Run this > >> ./scripts/kernel-doc -man -v 1>/dev/null *.c > >> > >> to check that all values are right. > > > > The kernel-doc doesn't found errors. > > Because it doesn't find any structure to check. Add here /** and run > that script again. > > > > >>> + * struct fxl6408_info - Data for fxl6408 > >>> + * > >>> + * @dev: udevice structure for the device > >>> + * @addr: i2c slave address > >>> + * @reg_io_dir: hold the value of direction register > >>> + * @reg_output: hold the value of output register > >>> + */ > >>> +struct fxl6408_info { > >>> + struct udevice *dev; > >>> + int addr; > >>> + u8 device_id; > >>> + u8 reg_io_dir; > >>> + u8 reg_output; > >>> +}; > >>> + > >>> +static inline int fxl6408_write(struct udevice *dev, int reg, u8 val) > >>> +{ > >>> + return dm_i2c_write(dev, reg, &val, 1); > >>> +} > >>> + > >>> +static int fxl6408_read(struct udevice *dev, int reg) > >>> +{ > >>> + int ret; > >>> + u8 tmp; > >>> + > >>> + ret = dm_i2c_read(dev, reg, &tmp, 1); > >>> + if (!ret) > >>> + ret = tmp; > >> > >> This looks completely wrong. What value are you returning in case of error. > > > > Nope. In case of error, I return an error value stored in "ret". > > > >> If ops->xfer is not defined dm_i2c_read returns -ENOSYS. tmp is not > >> initialized and can have random value that's why here in case or error > >> you will return ramdom value. > > > > !(-ENOSYS) == false, thus "if" op fails and doesn't perform "ret = tmp". > > I don't see any errors here. > > You are right. I read it incorrectly. > > > <snip> > > >>> + /* > >>> + * If configured, set initial output state and direction, > >>> + * otherwise read them from the chip. > >>> + */ > >>> + if (dev_read_u32(dev, "initial_io_dir", &val32)) { > >> > >> Where do you have dt binding document for this chip? I can't see > >> anything in the linux kernel or in your series. > >> > >> It is good that you read values from dt but you shouldn't do it in > >> probe. You should create platdata and fill it by information from DT and > >> then in probe just read them from platdata structure. > >> This applied to all dt read in probe. > >> > >> > >>> + ret = fxl6408_read(dev, REG_IO_DIR); > >>> + if (ret < 0) { > >>> + dev_err(dev, "Error reading direction register\n"); > >>> + return ret; > >>> + } > >>> + info->reg_io_dir = ret; > >>> + } else { > >>> + info->reg_io_dir = val32 & 0xFF; > >> > >> val32 is not initialized above. dev_read_u32 above fails that's why > >> val32 wont't be initialized and here you do calculation with it. > >> That's quite weird behavior. The same pattern visible below. > > > > Nope. dev_read_u32() returns 0 on success. The logic is correct. > > You are right again. I normally use reverse logic. > > > >>> + ret = fxl6408_write(dev, REG_IO_DIR, info->reg_io_dir); > >>> + if (ret < 0) { > >>> + dev_err(dev, "Error setting direction register\n"); > >>> + return ret; > >>> + } > >>> + } > >>> + > >>> + if (dev_read_u32(dev, "initial_output", &val32)) { > >>> + ret = fxl6408_read(dev, REG_OUT_STATE); > >>> + if (ret < 0) { > >>> + dev_err(dev, "Error reading output register\n"); > >>> + return ret; > >>> + } > >>> + info->reg_output = ret; > >>> + } else { > >>> + info->reg_output = val32 & 0xFF; > >>> + ret = fxl6408_write(dev, REG_OUT_STATE, info->reg_output); > >>> + if (ret < 0) { > >>> + 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); > >>> + } > >> > >> I see some code around labels in gpio-uclass. I would be surprised if it > >> is not there already because it should be core functionality not driver > >> when labels are defined. > > > > This code is not about gpios labeling. It's about the bank name, it > > just sets the uc_priv->bank_name which is used in gpio-uclass. > > Can you please use different variable name then label. If this is > bank_name just call it bank name. It makes sense. I'll rename it in the next revision. Thanks! > > Thanks, > Michal >
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 4a89c1a62b..f56e4cc261 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 expander driver" + depends on DM_GPIO && DM_I2C + help + This driver supports the Fairchild FXL6408 device. FXL6408 is a + fully configurable 8-bit I2C-controlled GPIO expander. + config INTEL_BROADWELL_GPIO bool "Intel Broadwell GPIO driver" depends on DM diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 58f4704f6b..83d8b5c9d8 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_AT91_GPIO) += at91_gpio.o obj-$(CONFIG_ATMEL_PIO4) += atmel_pio4.o obj-$(CONFIG_BCM6345_GPIO) += bcm6345_gpio.o obj-$(CONFIG_CORTINA_GPIO) += cortina_gpio.o +obj-$(CONFIG_FXL6408_GPIO) += gpio-fxl6408.o obj-$(CONFIG_INTEL_GPIO) += intel_gpio.o obj-$(CONFIG_INTEL_ICH6_GPIO) += intel_ich6_gpio.o obj-$(CONFIG_INTEL_BROADWELL_GPIO) += intel_broadwell_gpio.o diff --git a/drivers/gpio/gpio-fxl6408.c b/drivers/gpio/gpio-fxl6408.c new file mode 100644 index 0000000000..dbc68618f9 --- /dev/null +++ b/drivers/gpio/gpio-fxl6408.c @@ -0,0 +1,366 @@ +// 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@anholt.net>: + * https://patchwork.kernel.org/patch/9148419/ + * - the Toradex version by Max Krummenacher <max.krummenacher@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@gmail.com>: + * drivers/gpio/pca953x_gpio.c + * + * TODO: + * - Add interrupts support + * - Replace deprecated callbacks direction_input/output() with set_flags() + */ + +#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 REG_DEVID_CTRL 0x01 +# define SW_RST BIT(0) +# define RST_INT BIT(1) +/* 0b101 is the Manufacturer's ID assigned to Fairchild by Nokia */ +# define MF_ID_FAIRCHILD 5 + +/* Bits set here indicate that the GPIO is an output */ +#define REG_IO_DIR 0x03 + +/* + * Bits set here, when the corresponding bit of REG_IO_DIR is set, drive + * the output high instead of low. + */ +#define REG_OUT_STATE 0x05 + +/* Bits here make the output High-Z, instead of the OUTPUT value */ +#define REG_OUT_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 REG_IN_DEFAULT_STATE 0x09 + +/* + * Bits here enable either pull up or pull down according to + * REG_PULL_MODE. + */ +#define REG_PULL_ENABLE 0x0b + +/* + * Bits set here selects a pull-up/pull-down state of pin, which + * is configured as Input and the corresponding REG_PULL_ENABLE bit is + * set. + */ +#define REG_PULL_MODE 0x0d + +/* Returns the current status (1 = HIGH) of the input pins */ +#define REG_IN_STATUS 0x0f + +/* Mask of pins which can generate interrupts */ +#define REG_INT_MASK 0x11 + +/* Mask of pins which have generated an interrupt. Cleared on read */ +#define REG_INT_STATUS 0x13 + +/* Manufacturer's ID getting from Device ID & Ctrl register */ +enum { + MF_ID_MASK = GENMASK(7, 5), + MF_ID_SHIFT = 5, +}; + +/* Firmware revision getting from Device ID & Ctrl register */ +enum { + FW_REV_MASK = GENMASK(4, 2), + FW_REV_SHIFT = 2, +}; + +enum io_direction { + DIR_IN, + DIR_OUT, +}; + +/* + * struct fxl6408_info - Data for fxl6408 + * + * @dev: udevice structure for the device + * @addr: i2c slave address + * @reg_io_dir: hold the value of direction register + * @reg_output: hold the value of output register + */ +struct fxl6408_info { + struct udevice *dev; + int addr; + u8 device_id; + u8 reg_io_dir; + u8 reg_output; +}; + +static inline int fxl6408_write(struct udevice *dev, int reg, u8 val) +{ + return dm_i2c_write(dev, reg, &val, 1); +} + +static int fxl6408_read(struct udevice *dev, int reg) +{ + int ret; + u8 tmp; + + ret = dm_i2c_read(dev, reg, &tmp, 1); + if (!ret) + ret = tmp; + + return ret; +} + +/* + * fxl6408_is_output() - check whether the gpio configures as either + * output or input. + * Return: false - input, true - output. + */ +static bool fxl6408_is_output(struct udevice *dev, int offset) +{ + 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) ? REG_OUT_STATE : REG_IN_STATUS; + + ret = fxl6408_read(dev, reg); + if (ret < 0) + return ret; + + return !!(ret & 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, REG_OUT_STATE, val); + if (ret < 0) + return ret; + + info->reg_output = val; + + return 0; +} + +static int fxl6408_set_direction(struct udevice *dev, uint offset, + enum io_direction dir) +{ + struct fxl6408_info *info = dev_get_plat(dev); + u8 val; + int ret; + + if (dir == DIR_IN) + val = info->reg_io_dir & ~BIT(offset); + else + val = info->reg_io_dir | BIT(offset); + + ret = fxl6408_write(dev, REG_IO_DIR, val); + if (ret < 0) + return ret; + + info->reg_io_dir = val; + + return 0; +} + +static int fxl6408_direction_input(struct udevice *dev, uint offset) +{ + return fxl6408_set_direction(dev, offset, DIR_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 (ret < 0) + return ret; + /* Configure direction as output */ + fxl6408_set_direction(dev, offset, DIR_OUT); + + return 0; +} + +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, + .get_value = fxl6408_get_value, + .set_value = fxl6408_set_value, + .get_function = fxl6408_get_function, + .xlate = fxl6408_xlate, +}; + +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; + u32 val32; + + addr = dev_read_addr(dev); + if (addr == 0) + return -EINVAL; + info->addr = addr; + + /* + * Check the device ID register to see if it's responding. + * This also clears RST_INT as a side effect, so we won't get + * the "we've been power cycled" interrupt once interrupts + * being enabled. + */ + ret = fxl6408_read(dev, REG_DEVID_CTRL); + if (ret < 0) { + dev_err(dev, "FXL6408 probe returned %d\n", ret); + return ret; + } + + if ((ret & MF_ID_MASK) >> MF_ID_SHIFT != MF_ID_FAIRCHILD) { + dev_err(dev, "FXL6408 probe: wrong Manufacturer's ID: 0x%02x\n", ret); + return -ENXIO; + } + info->device_id = ret; + + /* + * Disable High-Z of outputs, so that the OUTPUT updates + * actually take effect. + */ + ret = fxl6408_write(dev, REG_OUT_HIGH_Z, (u8)0); + if (ret < 0) { + 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 (dev_read_u32(dev, "initial_io_dir", &val32)) { + ret = fxl6408_read(dev, REG_IO_DIR); + if (ret < 0) { + dev_err(dev, "Error reading direction register\n"); + return ret; + } + info->reg_io_dir = ret; + } else { + info->reg_io_dir = val32 & 0xFF; + ret = fxl6408_write(dev, REG_IO_DIR, info->reg_io_dir); + if (ret < 0) { + dev_err(dev, "Error setting direction register\n"); + return ret; + } + } + + if (dev_read_u32(dev, "initial_output", &val32)) { + ret = fxl6408_read(dev, REG_OUT_STATE); + if (ret < 0) { + dev_err(dev, "Error reading output register\n"); + return ret; + } + info->reg_output = ret; + } else { + info->reg_output = val32 & 0xFF; + ret = fxl6408_write(dev, REG_OUT_STATE, info->reg_output); + if (ret < 0) { + 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. %d) is ready\n", str, + (info->device_id & MF_ID_MASK) >> MF_ID_SHIFT); + + 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), +};