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

["Nuno Sá" <noname.nuno@gmail.com>]

On Fri, 2025-10-31 at 16:07 +0000, Antoniu Miclaus 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.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
>  drivers/gpio/Kconfig        |   9 +++
>  drivers/gpio/Makefile       |   1 +
>  drivers/gpio/gpio-adg1712.c | 146 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 156 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.
> +
>  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..f8d3481ac9d0
> --- /dev/null
> +++ b/drivers/gpio/gpio-adg1712.c
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Analog Devices ADG1712 quad SPST switch GPIO driver
> + *
> + * Copyright 2025 Analog Devices Inc.
> + *
> + * Author: Antoniu Miclaus <antoniu.miclaus@analog.com>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +
> +#define ADG1712_NUM_GPIOS	4
> +
> +struct adg1712 {
> +	struct gpio_chip chip;
> +	struct gpio_desc *switch_gpios[ADG1712_NUM_GPIOS];
> +};
> +
> +static int adg1712_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +	return GPIO_LINE_DIRECTION_OUT;
> +}
> +
> +static int adg1712_direction_input(struct gpio_chip *chip, unsigned int offset)
> +{
> +	return -EINVAL;
> +}

Did not checked gpiolib for this but do we need the above given that we always
return GPIO_LINE_DIRECTION_OUT?

> +
> +static int adg1712_direction_output(struct gpio_chip *chip, unsigned int offset,
> +				    int value)
> +{
> +	struct adg1712 *adg1712 = gpiochip_get_data(chip);
> +
> +	if (offset >= ADG1712_NUM_GPIOS)
> +		return -EINVAL;

I don't think above can happen.

> +
> +	gpiod_set_value_cansleep(adg1712->switch_gpios[offset], value);

return gpiod_set_value_cansleep().

> +	return 0;
> +}
> +
> +static int adg1712_set(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> +	struct adg1712 *adg1712 = gpiochip_get_data(chip);
> +
> +	if (offset >= ADG1712_NUM_GPIOS)
> +		return -EINVAL;

Ditto

> +
> +	gpiod_set_value_cansleep(adg1712->switch_gpios[offset], value);
> +	return 0;
> +}
> +
> +static int adg1712_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct adg1712 *adg1712 = gpiochip_get_data(chip);
> +
> +	if (offset >= ADG1712_NUM_GPIOS)
> +		return -EINVAL;
> +
> +	return gpiod_get_value_cansleep(adg1712->switch_gpios[offset]);
> +}
> +
> +static int adg1712_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> +				 unsigned long *bits)
> +{
> +	struct adg1712 *adg1712 = gpiochip_get_data(chip);
> +	int i;
> +
> +	for_each_set_bit(i, mask, ADG1712_NUM_GPIOS) {
> +		gpiod_set_value_cansleep(adg1712->switch_gpios[i],
> +					 test_bit(i, bits));

Error handling.

> +	}
> +
> +	return 0;
> +}
> +
> +static const struct gpio_chip adg1712_gpio_chip = {
> +	.label			= "adg1712",
> +	.owner			= THIS_MODULE,
> +	.get_direction		= adg1712_get_direction,
> +	.direction_input	= adg1712_direction_input,
> +	.direction_output	= adg1712_direction_output,
> +	.get			= adg1712_get,
> +	.set			= adg1712_set,
> +	.set_multiple		= adg1712_set_multiple,
> +	.base			= -1,
> +	.ngpio			= ADG1712_NUM_GPIOS,
> +	.can_sleep		= true,
> +};
> +
> +static int adg1712_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct adg1712 *adg1712;
> +	int ret, i;
> +	char gpio_name[16];
> +
> +	adg1712 = devm_kzalloc(dev, sizeof(*adg1712), GFP_KERNEL);
> +	if (!adg1712)
> +		return -ENOMEM;
> +
> +	adg1712->chip = adg1712_gpio_chip;
> +	adg1712->chip.parent = dev;
> +
> +	for (i = 0; i < ADG1712_NUM_GPIOS; i++) {
> +		snprintf(gpio_name, sizeof(gpio_name), "switch%d", i + 1);

Just a suggestion. Instead of the snprintf(), you could have a const array of
strings and just go over it. Not a big deal to me though. You could also
consider devm_gpiod_get_array()

> +		adg1712->switch_gpios[i] = devm_gpiod_get(dev, gpio_name,
> +							  GPIOD_OUT_LOW);

Should we make assumptions on the initial value? Not sure if GPIO_ASIS would
make sense here.

> +		if (IS_ERR(adg1712->switch_gpios[i]))
> +			return dev_err_probe(dev, PTR_ERR(adg1712->switch_gpios[i]),
> +					     "failed to get %s gpio\n", gpio_name);
> +	}
> +
> +	ret = devm_gpiochip_add_data(dev, &adg1712->chip, adg1712);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to add gpio chip\n");
> +
> +	dev_info(dev, "ADG1712 %u-GPIO expander registered\n",
> +		 adg1712->chip.ngpio);

Drop the above or turn it into dev_dbg()

- Nuno Sá
>