Re: [PATCH v3 2/2] gpio: fxl6408: add I2C GPIO expander driver

From: Andy Shevchenko
Date: Mon Mar 13 2023 - 13:36:01 EST

On Mon, Mar 13, 2023 at 7:09 PM Francesco Dolcini <francesco@xxxxxxxxxx> wrote:
>
> From: Emanuele Ghidoli <emanuele.ghidoli@xxxxxxxxxxx>
>
> Add minimal driver for Fairchild FXL6408 8-bit I2C-controlled GPIO expander
> using the generic regmap based GPIO driver (GPIO_REGMAP).
>
> The driver implements setting the GPIO direction, reading inputs
> and writing outputs.
>
> In addition to that the FXL6408 has the following functionalities:
> - allows to monitor input ports for data transitions with an interrupt pin
> - all inputs can be configured with pull-up or pull-down resistors

Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
But see below.

> Datasheet: https://www.onsemi.com/download/data-sheet/pdf/fxl6408-d.pdf
> Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@xxxxxxxxxxx>
> Co-developed-by: Francesco Dolcini <francesco.dolcini@xxxxxxxxxxx>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@xxxxxxxxxxx>
> ---
> v3:
> * add include files: kernel.h and err.h
> * refactor of fxl6408_identify and used dev_err_probe instead of dev_err
> * use FXL6408_REG_INT_STS instead of FXL6408_MAX_REGISTER
>
> v2:
> * remove includes: <linux/gpio.h> and <linux/of_platform.h>
> * add missing and required select REGMAP_I2C in KConfig
> * use dev_err_probe()
> * add "Datasheet:" tag in commit message
> * improve KConfig help section
> * fix newlines, multi-line comments and trailing commas
> ---
> drivers/gpio/Kconfig | 10 +++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-fxl6408.c | 158 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 169 insertions(+)
> create mode 100644 drivers/gpio/gpio-fxl6408.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 13be729710f2..56a73007ebcb 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1000,6 +1000,16 @@ config GPIO_ADNP
> enough to represent all pins, but the driver will assume a
> register layout for 64 pins (8 registers).
>
> +config GPIO_FXL6408
> + tristate "FXL6408 I2C GPIO expander"
> + select GPIO_REGMAP
> + select REGMAP_I2C
> + help
> + GPIO driver for Fairchild Semiconductor FXL6408 GPIO expander.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called gpio-fxl6408.
> +
> config GPIO_GW_PLD
> tristate "Gateworks PLD GPIO Expander"
> depends on OF_GPIO
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index c048ba003367..12027f4c3bee 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -60,6 +60,7 @@ obj-$(CONFIG_GPIO_EP93XX) += gpio-ep93xx.o
> obj-$(CONFIG_GPIO_EXAR) += gpio-exar.o
> obj-$(CONFIG_GPIO_F7188X) += gpio-f7188x.o
> obj-$(CONFIG_GPIO_FTGPIO010) += gpio-ftgpio010.o
> +obj-$(CONFIG_GPIO_FXL6408) += gpio-fxl6408.o
> obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
> obj-$(CONFIG_GPIO_GPIO_MM) += gpio-gpio-mm.o
> obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
> diff --git a/drivers/gpio/gpio-fxl6408.c b/drivers/gpio/gpio-fxl6408.c
> new file mode 100644
> index 000000000000..d7387c0101e2
> --- /dev/null
> +++ b/drivers/gpio/gpio-fxl6408.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * FXL6408 GPIO driver
> + *
> + * Copyright 2023 Toradex
> + *
> + * Author: Emanuele Ghidoli <emanuele.ghidoli@xxxxxxxxxxx>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/gpio/regmap.h>
> +#include <linux/kernel.h>
> +#include <linux/i2c.h>

Seems unordered?

> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#define FXL6408_REG_DEVICE_ID 0x01
> +#define FXL6408_MF_FAIRCHILD 0b101
> +#define FXL6408_MF_SHIFT 5
> +
> +/* Bits set here indicate that the GPIO is an output. */
> +#define FXL6408_REG_IO_DIR 0x03
> +
> +/*
> + * Bits set here, when the corresponding bit of IO_DIR is set, drive
> + * the output high instead of low.
> + */
> +#define FXL6408_REG_OUTPUT 0x05
> +
> +/* Bits here make the output High-Z, instead of the OUTPUT value. */
> +#define FXL6408_REG_OUTPUT_HIGH_Z 0x07
> +
> +/* Returns the current status (1 = HIGH) of the input pins. */
> +#define FXL6408_REG_INPUT_STATUS 0x0f
> +
> +/*
> + * Return the current interrupt status
> + * This bit is HIGH if input GPIO != default state (register 09h).
> + * The flag is cleared after being read (bit returns to 0).
> + * The input must go back to default state and change again before this flag is raised again.
> + */
> +#define FXL6408_REG_INT_STS 0x13
> +
> +#define FXL6408_NGPIO 8
> +
> +static const struct regmap_range rd_range[] = {
> + { FXL6408_REG_DEVICE_ID, FXL6408_REG_DEVICE_ID },
> + { FXL6408_REG_IO_DIR, FXL6408_REG_OUTPUT },
> + { FXL6408_REG_INPUT_STATUS, FXL6408_REG_INPUT_STATUS },
> +};
> +
> +static const struct regmap_range wr_range[] = {
> + { FXL6408_REG_DEVICE_ID, FXL6408_REG_DEVICE_ID },
> + { FXL6408_REG_IO_DIR, FXL6408_REG_OUTPUT },
> + { FXL6408_REG_OUTPUT_HIGH_Z, FXL6408_REG_OUTPUT_HIGH_Z },
> +};
> +
> +static const struct regmap_range volatile_range[] = {
> + { FXL6408_REG_DEVICE_ID, FXL6408_REG_DEVICE_ID },
> + { FXL6408_REG_INPUT_STATUS, FXL6408_REG_INPUT_STATUS },
> +};
> +
> +static const struct regmap_access_table rd_table = {
> + .yes_ranges = rd_range,
> + .n_yes_ranges = ARRAY_SIZE(rd_range),
> +};
> +
> +static const struct regmap_access_table wr_table = {
> + .yes_ranges = wr_range,
> + .n_yes_ranges = ARRAY_SIZE(wr_range),
> +};
> +
> +static const struct regmap_access_table volatile_table = {
> + .yes_ranges = volatile_range,
> + .n_yes_ranges = ARRAY_SIZE(volatile_range),
> +};
> +
> +static const struct regmap_config regmap = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +
> + .max_register = FXL6408_REG_INT_STS,
> + .wr_table = &wr_table,
> + .rd_table = &rd_table,
> + .volatile_table = &volatile_table,
> +
> + .cache_type = REGCACHE_RBTREE,
> + .num_reg_defaults_raw = FXL6408_REG_INT_STS + 1,
> +};
> +
> +static int fxl6408_identify(struct device *dev, struct regmap *regmap)
> +{
> + int val, ret;
> +
> + ret = regmap_read(regmap, FXL6408_REG_DEVICE_ID, &val);
> + if (ret)
> + return dev_err_probe(dev, ret, "error reading DEVICE_ID\n");
> + if (val >> FXL6408_MF_SHIFT != FXL6408_MF_FAIRCHILD)
> + return dev_err_probe(dev, -ENODEV, "invalid device id 0x%02x\n", val);
> +
> + return 0;
> +}
> +
> +static int fxl6408_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + int ret;
> + struct gpio_regmap_config gpio_config = {
> + .parent = dev,
> + .ngpio = FXL6408_NGPIO,
> + .reg_dat_base = GPIO_REGMAP_ADDR(FXL6408_REG_INPUT_STATUS),
> + .reg_set_base = GPIO_REGMAP_ADDR(FXL6408_REG_OUTPUT),
> + .reg_dir_out_base = GPIO_REGMAP_ADDR(FXL6408_REG_IO_DIR),
> + .ngpio_per_reg = FXL6408_NGPIO,
> + };
> +
> + gpio_config.regmap = devm_regmap_init_i2c(client, &regmap);
> + if (IS_ERR(gpio_config.regmap))
> + return dev_err_probe(dev, PTR_ERR(gpio_config.regmap),
> + "failed to allocate register map\n");
> +
> + ret = fxl6408_identify(dev, gpio_config.regmap);
> + if (ret)
> + return ret;
> +
> + /* Disable High-Z of outputs, so that our OUTPUT updates actually take effect. */
> + ret = regmap_write(gpio_config.regmap, FXL6408_REG_OUTPUT_HIGH_Z, 0);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to write 'output high Z' register\n");
> +
> + return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &gpio_config));
> +}
> +
> +static const __maybe_unused struct of_device_id fxl6408_dt_ids[] = {
> + { .compatible = "fcs,fxl6408" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, fxl6408_dt_ids);
> +
> +static const struct i2c_device_id fxl6408_id[] = {
> + { "fxl6408", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, fxl6408_id);
> +
> +static struct i2c_driver fxl6408_driver = {
> + .driver = {
> + .name = "fxl6408",
> + .of_match_table = fxl6408_dt_ids,
> + },
> + .probe_new = fxl6408_probe,
> + .id_table = fxl6408_id,
> +};
> +module_i2c_driver(fxl6408_driver);
> +
> +MODULE_AUTHOR("Emanuele Ghidoli <emanuele.ghidoli@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("FXL6408 GPIO driver");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>


--
With Best Regards,
Andy Shevchenko