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

[Paller, Kim Seer]

Hi Bart,

> -----Original Message-----
> From: Bartosz Golaszewski <brgl@xxxxxxxx>
> Sent: Monday, January 22, 2024 9:00 PM
> To: Paller, Kim Seer <KimSeer.Paller@xxxxxxxxxx>
> Cc: linux-gpio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Linus Walleij <linus.walleij@xxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>
> Subject: Re: [PATCH 2/2] gpio: gpio-adg1414: New driver
> 
> [External]
> 
> 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)

It's most likely to be in plural since the device has 8 individual SPST switches,
as also stated in the datasheet.

> >
> > 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.

I could not seem to figure out how to use the SPI regmap in this case.
Since the number of daisy-chained devices depends on the length of
data transferred with continuous transaction, I could not determine
how to implement that using the SPI regmap. Or maybe I misunderstood
the statement. However, is it still acceptable to use the current approach?

Best regards,
Kim