Re: [PATCH v3 2/2] gpio: adg1712: add driver support

[Bartosz Golaszewski <brgl@bgdev.pl>]

On Mon, Nov 17, 2025 at 10:15 AM Antoniu Miclaus
<antoniu.miclaus@analog.com> wrote:
>
> Add driver support for the ADG1712, which contains four independent
> single-pole/single-throw (SPST) switches and operates with a
> low-voltage single supply range from +1.08V to +5.5V or a low-voltage
> dual supply range from ±1.08V to ±2.75V.
>
> The driver configures switches once at probe time based on device tree
> properties and does not expose any userspace interface for runtime control.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> Changes in v3:
> - Remove GPIO controller interface
> - Configure switches from device tree at probe time only
> - Add 'switch-states' property parsing
> - Change from GPIOD_ASIS to GPIOD_OUT_LOW
> ---
>  drivers/gpio/Kconfig        |  9 ++++
>  drivers/gpio/Makefile       |  1 +
>  drivers/gpio/gpio-adg1712.c | 87 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 97 insertions(+)
>  create mode 100644 drivers/gpio/gpio-adg1712.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 7ee3afbc2b05..3fac05823eae 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -157,6 +157,15 @@ config GPIO_74XX_MMIO
>             8 bits:     74244 (Input), 74273 (Output)
>             16 bits:    741624 (Input), 7416374 (Output)
>
> +config GPIO_ADG1712
> +       tristate "Analog Devices ADG1712 quad SPST switch GPIO driver"
> +       depends on GPIOLIB
> +       help
> +         GPIO driver for Analog Devices ADG1712 quad single-pole,
> +         single-throw (SPST) switch. The driver provides a GPIO controller
> +         interface where each GPIO line controls one of the four independent
> +         analog switches on the ADG1712.
> +

I'm finding it hard to understand how this is a GPIO driver. It's a
GPIO consumer but does it really belong under drivers/gpio/?

>  config GPIO_ALTERA
>         tristate "Altera GPIO"
>         select GPIOLIB_IRQCHIP
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index ec296fa14bfd..9043d2d07a15 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -28,6 +28,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_ADG1712)             += gpio-adg1712.o
>  obj-$(CONFIG_GPIO_ADNP)                        += gpio-adnp.o
>  obj-$(CONFIG_GPIO_ADP5520)             += gpio-adp5520.o
>  obj-$(CONFIG_GPIO_ADP5585)             += gpio-adp5585.o
> diff --git a/drivers/gpio/gpio-adg1712.c b/drivers/gpio/gpio-adg1712.c
> new file mode 100644
> index 000000000000..86f8645cf2ad
> --- /dev/null
> +++ b/drivers/gpio/gpio-adg1712.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Analog Devices ADG1712 quad SPST switch driver
> + *
> + * Copyright 2025 Analog Devices Inc.
> + *
> + * Author: Antoniu Miclaus <antoniu.miclaus@analog.com>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +
> +#define ADG1712_NUM_SWITCHES   4
> +
> +struct adg1712 {
> +       struct gpio_descs *switch_gpios;
> +};
> +
> +static int adg1712_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct adg1712 *adg1712;
> +       u32 switch_states[ADG1712_NUM_SWITCHES] = {0}; /* Default all switches off */
> +       int ret, i;
> +
> +       adg1712 = devm_kzalloc(dev, sizeof(*adg1712), GFP_KERNEL);
> +       if (!adg1712)
> +               return -ENOMEM;
> +
> +       adg1712->switch_gpios = devm_gpiod_get_array(dev, "switch", GPIOD_OUT_LOW);
> +       if (IS_ERR(adg1712->switch_gpios))
> +               return dev_err_probe(dev, PTR_ERR(adg1712->switch_gpios),
> +                                    "failed to get switch gpios\n");
> +
> +       if (adg1712->switch_gpios->ndescs != ADG1712_NUM_SWITCHES)
> +               return dev_err_probe(dev, -EINVAL,
> +                                    "expected %d gpios, got %d\n",
> +                                    ADG1712_NUM_SWITCHES,
> +                                    adg1712->switch_gpios->ndescs);
> +
> +       ret = device_property_read_u32_array(dev, "switch-states", switch_states,
> +                                            ADG1712_NUM_SWITCHES);
> +       if (ret && ret != -EINVAL)
> +               return dev_err_probe(dev, ret, "failed to read switch-states\n");
> +
> +       for (i = 0; i < ADG1712_NUM_SWITCHES; i++) {
> +               if (switch_states[i] > 1) {
> +                       dev_warn(dev, "invalid switch state %u for switch %d, using 0\n",
> +                                switch_states[i], i);
> +                       switch_states[i] = 0;
> +               }
> +
> +               ret = gpiod_set_value_cansleep(adg1712->switch_gpios->desc[i],
> +                                              switch_states[i]);

I don't see anything here that cannot be achieved with gpio hogs in
device-tree. Do we really need a separate driver for it? If we really
really need it, you don't really need to implement a new driver, you
could literally just extend gpio-virtuser with a real compatible and
it would request the GPIOs for you.

> +               if (ret)
> +                       return dev_err_probe(dev, ret, "failed to set switch %d\n", i);
> +       }
> +
> +       platform_set_drvdata(pdev, adg1712);

Where is the corresponding platform_get_drvdata()?

> +
> +       dev_info(dev, "ADG1712 switch controller configured\n");

Please remove this, no need to be noisy.

Bart