Re: [PATCH v3 2/2] gpio: ds4520: Add ADI DS4520 GPIO Expander Support

[Bartosz Golaszewski]

On Mon, Jul 24, 2023 at 8:34 AM Okan Sahin <okan.sahin@xxxxxxxxxx> wrote:
>
> The DS4520 is a 9-bit nonvolatile (NV) I/O expander.
> It offers users a digitally programmable alternative
> to hardware jumpers and mechanical switches that are
> being used to control digital logic node.
>
> Signed-off-by: Okan Sahin <okan.sahin@xxxxxxxxxx>
> ---

This is such a sweet little driver, I love all these abstraction
layers we now have.

Some nits below.

>  drivers/gpio/Kconfig       | 11 +++++
>  drivers/gpio/Makefile      |  1 +
>  drivers/gpio/gpio-ds4520.c | 89 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 101 insertions(+)
>  create mode 100644 drivers/gpio/gpio-ds4520.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 13be729710f2..5f89e46d6411 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1000,6 +1000,17 @@ config GPIO_ADNP
>           enough to represent all pins, but the driver will assume a
>           register layout for 64 pins (8 registers).
>
> +config GPIO_DS4520
> +       tristate "DS4520 I2C GPIO expander"
> +       select REGMAP_I2C
> +       select GPIO_REGMAP
> +       help
> +         GPIO driver for ADI DS4520 I2C-based GPIO expander.
> +         Say yes here to enable the GPIO driver for the ADI DS4520 chip.
> +
> +         To compile this driver as a module, choose M here: the module will
> +         be called gpio-ds4520.
> +
>  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..6f8656d5d617 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -52,6 +52,7 @@ obj-$(CONFIG_GPIO_DA9052)             += gpio-da9052.o
>  obj-$(CONFIG_GPIO_DA9055)              += gpio-da9055.o
>  obj-$(CONFIG_GPIO_DAVINCI)             += gpio-davinci.o
>  obj-$(CONFIG_GPIO_DLN2)                        += gpio-dln2.o
> +obj-$(CONFIG_GPIO_DS4520)              += gpio-ds4520.o
>  obj-$(CONFIG_GPIO_DWAPB)               += gpio-dwapb.o
>  obj-$(CONFIG_GPIO_EIC_SPRD)            += gpio-eic-sprd.o
>  obj-$(CONFIG_GPIO_EM)                  += gpio-em.o
> diff --git a/drivers/gpio/gpio-ds4520.c b/drivers/gpio/gpio-ds4520.c
> new file mode 100644
> index 000000000000..577ffb4b8c50
> --- /dev/null
> +++ b/drivers/gpio/gpio-ds4520.c
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023 Analog Devices, Inc.
> + * Driver for the DS4520 I/O Expander
> + */
> +
> +#include <linux/device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/regmap.h>
> +#include <linux/i2c.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +#define DS4520_PULLUP0         0xF0
> +#define DS4520_IO_CONTROL0     0xF2
> +#define DS4520_IO_STATUS0      0xF8
> +
> +static const struct regmap_config ds4520_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +};
> +
> +static int ds4520_gpio_probe(struct i2c_client *client)
> +{
> +       struct gpio_regmap_config config = { };
> +       struct device *dev = &client->dev;
> +       struct regmap *regmap;
> +       u32 ngpio;
> +       u32 base;
> +       int ret;
> +
> +       ret = device_property_read_u32(dev, "reg", &base);
> +       if (ret) {
> +               dev_err_probe(dev, ret,
> +                         "Missing 'reg' property.\n");
> +               return -EINVAL;
> +       }

Please add a newline here.

> +       ret = device_property_read_u32(dev, "ngpios", &ngpio);
> +       if (ret) {
> +               dev_err_probe(dev, ret,
> +                         "Missing 'ngpios' property.\n");
> +               return -EINVAL;
> +       }

And here.

> +       regmap = devm_regmap_init_i2c(client, &ds4520_regmap_config);
> +       if (IS_ERR(regmap)) {
> +               ret = PTR_ERR(regmap);
> +               dev_err_probe(dev, ret,
> +                             "Failed to allocate register map\n");
> +               return ret;
> +       }
> +
> +       config.regmap = regmap;
> +       config.parent = dev;
> +       config.ngpio = ngpio;
> +
> +       config.reg_dat_base = base + DS4520_IO_STATUS0;
> +       config.reg_set_base = base + DS4520_PULLUP0;
> +       config.reg_dir_out_base = base + DS4520_IO_CONTROL0;
> +
> +       return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &config));
> +}
> +
> +static const struct of_device_id ds4520_gpio_of_match_table[] = {
> +       {
> +               .compatible = "adi,ds4520-gpio"

Keep this on a single line.

> +       },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, ds4520_gpio_of_match_table);
> +
> +static const struct i2c_device_id ds4520_gpio_id_table[] = {
> +       { "ds4520-gpio" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(i2c, ds4520_gpio_id_table);
> +
> +static struct i2c_driver ds4520_gpio_driver = {
> +       .driver = {
> +               .name = "ds4520-gpio",
> +               .of_match_table = ds4520_gpio_of_match_table,
> +       },
> +       .probe_new = ds4520_gpio_probe,
> +       .id_table = ds4520_gpio_id_table,
> +};
> +module_i2c_driver(ds4520_gpio_driver);
> +
> +MODULE_DESCRIPTION("DS4520 I/O Expander");
> +MODULE_AUTHOR("Okan Sahin <okan.sahin@xxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> --
> 2.30.2
>

Bart