Re: [PATCH 2/2] gpio: gpio-adg1414: New driver

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Sun, Jan 21, 2024 at 11:35 AM Kim Seer Paller
<kimseer.paller@xxxxxxxxxx> wrote:
>
> The ADG1414 is a 9.5 Ω RON ±15 V/+12 V/±5 V iCMOS Serially-Controlled
> Octal SPST Switches

Switch? (singular)

>
> Signed-off-by: Kim Seer Paller <kimseer.paller@xxxxxxxxxx>
> ---
>  MAINTAINERS                 |   1 +
>  drivers/gpio/Kconfig        |  10 +++
>  drivers/gpio/Makefile       |   1 +
>  drivers/gpio/gpio-adg1414.c | 141 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 153 insertions(+)
>  create mode 100644 drivers/gpio/gpio-adg1414.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 526145e69..254ba7ea5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -466,6 +466,7 @@ M:  Kim Seer Paller <kimseer.paller@xxxxxxxxxx>
>  S:     Supported
>  W:     https://ez.analog.com/linux-software-drivers
>  F:     Documentation/devicetree/bindings/gpio/gpio-adg1414.yaml
> +F:     drivers/gpio/gpio-adg1414.c
>
>  ADM1025 HARDWARE MONITOR DRIVER
>  M:     Jean Delvare <jdelvare@xxxxxxxx>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 1301cec94..25d467d70 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1728,6 +1728,16 @@ config GPIO_74X164
>           shift registers. This driver can be used to provide access
>           to more GPIO outputs.
>
> +config GPIO_ADG1414
> +       tristate "ADG1414 SPST Switch Driver"
> +       depends on SPI
> +       help
> +         Say yes here to build support for Analog Devices ADG1414 SPST
> +         Switch Driver
> +
> +         To compile this driver as a module, choose M here: the
> +         module will be called gpio-adg1414.
> +
>  config GPIO_MAX3191X
>         tristate "Maxim MAX3191x industrial serializer"
>         select CRC8
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 9e40af196..9ab45d128 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_GPIO_104_IDI_48)         += gpio-104-idi-48.o
>  obj-$(CONFIG_GPIO_104_IDIO_16)         += gpio-104-idio-16.o
>  obj-$(CONFIG_GPIO_74X164)              += gpio-74x164.o
>  obj-$(CONFIG_GPIO_74XX_MMIO)           += gpio-74xx-mmio.o
> +obj-$(CONFIG_GPIO_ADG1414)             += gpio-adg1414.o
>  obj-$(CONFIG_GPIO_ADNP)                        += gpio-adnp.o
>  obj-$(CONFIG_GPIO_ADP5520)             += gpio-adp5520.o
>  obj-$(CONFIG_GPIO_AGGREGATOR)          += gpio-aggregator.o
> diff --git a/drivers/gpio/gpio-adg1414.c b/drivers/gpio/gpio-adg1414.c
> new file mode 100644
> index 000000000..6c0830ee2
> --- /dev/null
> +++ b/drivers/gpio/gpio-adg1414.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ADG1414 SPST Switch Driver
> + *
> + * Copyright 2024 Analog Devices Inc.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/spi/spi.h>
> +#include <linux/module.h>

Can you keep the headers in alphabetical order please?

> +#include <linux/property.h>
> +
> +#define ADG1414_MAX_DEVICES            4
> +
> +struct adg1414_state {
> +       struct spi_device               *spi;
> +       struct gpio_chip                chip;
> +       struct mutex                    lock; /* protect sensor state */

Locking here is simple enough that you could use the SPI regmap and
get it to do the serialization for you. And then you could possibly
reuse the gpio-regmap abstraction and get an even smaller footprint.

> +       u32                             buf;
> +
> +       __be32                          tx __aligned(ARCH_DMA_MINALIGN);
> +};

Please drop the tabs from the struct, I personally don't think they
add to readability.

> +
> +static void adg1414_set(struct gpio_chip *chip,
> +                       unsigned int offset,
> +                       int value)
> +{
> +       struct adg1414_state *st = gpiochip_get_data(chip);
> +       int ret;
> +
> +       struct spi_transfer xfer = {
> +               .tx_buf = &st->tx,
> +               .len = st->chip.ngpio / 8,
> +       };
> +
> +       mutex_lock(&st->lock);
> +
> +       if (value)
> +               st->buf |= BIT(offset);
> +       else
> +               st->buf &= ~BIT(offset);
> +
> +       st->tx = cpu_to_be32(st->buf << (32 - st->chip.ngpio));
> +
> +       ret = spi_sync_transfer(st->spi, &xfer, 1);
> +       if (ret)
> +               goto out;
> +
> +out:
> +       mutex_unlock(&st->lock);
> +}
> +
> +static int adg1414_get(struct gpio_chip *chip,
> +                      unsigned int offset)
> +{
> +       struct adg1414_state *st = gpiochip_get_data(chip);
> +       int value;
> +
> +       mutex_lock(&st->lock);
> +
> +       value = st->buf & BIT(offset);
> +
> +       mutex_unlock(&st->lock);
> +
> +       return value;
> +}
> +
> +static int adg1414_get_direction(struct gpio_chip *chip,
> +                                unsigned int offset)
> +{
> +       return GPIO_LINE_DIRECTION_OUT;
> +}
> +
> +static int adg1414_probe(struct spi_device *spi)
> +{
> +       struct adg1414_state *st;
> +       struct gpio_desc *reset;
> +       struct device *dev = &spi->dev;
> +       u32 num_devices;
> +       int ret;
> +
> +       st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> +       if (!st)
> +               return -ENOMEM;
> +
> +       st->spi = spi;
> +
> +       /* Use reset pin to reset the device */
> +       reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +       if (IS_ERR(reset))
> +               return dev_err_probe(dev, PTR_ERR(reset),
> +                                    "Failed to get reset gpio");
> +
> +       if (reset) {
> +               fsleep(1);
> +               gpiod_set_value_cansleep(reset, 0);
> +       }
> +
> +       num_devices = 1;
> +       ret = device_property_read_u32(dev, "#daisy-chained-devices",
> +                                      &num_devices);
> +       if (!ret) {
> +               if (!num_devices || num_devices > ADG1414_MAX_DEVICES)
> +                       return dev_err_probe(dev, ret,
> +                              "Failed to get daisy-chained-devices property\n");
> +       }
> +
> +       st->chip.label = "adg1414";
> +       st->chip.parent = dev;
> +       st->chip.get_direction = adg1414_get_direction;
> +       st->chip.set = adg1414_set;
> +       st->chip.get = adg1414_get;
> +       st->chip.base = -1;
> +       st->chip.ngpio =  num_devices * 8;
> +       st->chip.can_sleep = true;
> +
> +       mutex_init(&st->lock);
> +
> +       return devm_gpiochip_add_data(dev, &st->chip, st);
> +}
> +
> +static const struct of_device_id adg1414_of_match[] = {
> +       { .compatible = "adi,adg1414" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, adg1414_of_match);
> +
> +static struct spi_driver adg1414_driver = {
> +       .driver = {
> +               .name = "adg1414",
> +               .of_match_table = adg1414_of_match,
> +       },
> +       .probe = adg1414_probe,
> +};
> +module_spi_driver(adg1414_driver);
> +
> +MODULE_AUTHOR("Kim Seer Paller <kimseer.paller@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("ADG1414 SPST Switch Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>

Bart