The AD4170 has four multifunctional pins that can be used as GPIOs. The
GPIO functionality can be accessed when the AD4170 chip is not busy
performing continuous data capture or handling any other register
read/write request. Also, the AD4170 does not provide any interrupt based
on GPIO pin states so AD4170 GPIOs can't be used as interrupt sources.

Implement gpio_chip callbacks so to make AD4170 GPIO pins controllable
through the gpiochip interface.

Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
 drivers/iio/adc/ad4170.c | 167 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 166 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
index 97cf4465038f..b382e7f3dbe0 100644
--- a/drivers/iio/adc/ad4170.c
+++ b/drivers/iio/adc/ad4170.c
@@ -12,6 +12,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/gpio/driver.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -79,6 +80,7 @@
 #define AD4170_FIR_CTRL					0x141
 #define AD4170_COEFF_DATA_REG				0x14A
 #define AD4170_COEFF_ADDR_REG				0x14C
+#define AD4170_GPIO_MODE_REG				0x191
 #define AD4170_GPIO_OUTPUT_REG				0x193
 #define AD4170_GPIO_INPUT_REG				0x195
 
@@ -189,6 +191,7 @@
 /* Device properties and auxiliary constants */
 
 #define AD4170_NUM_ANALOG_PINS				9
+#define AD4170_NUM_GPIO_PINS				4
 #define AD4170_MAX_CHANNELS				16
 #define AD4170_MAX_ANALOG_PINS				8
 #define AD4170_MAX_SETUPS				8
@@ -340,6 +343,7 @@ struct ad4170_state {
 	struct clk *ext_clk;
 	struct clk_hw int_clk_hw;
 	int pins_fn[AD4170_NUM_ANALOG_PINS];
+	struct gpio_chip gpiochip;
 	u32 int_pin_sel;
 	int sps_tbl[ARRAY_SIZE(ad4170_filt_names)][AD4170_MAX_FS_TBL_SIZE][2];
 	struct completion completion;
@@ -1553,6 +1557,156 @@ static int ad4170_soft_reset(struct ad4170_state *st)
 	return 0;
 }
 
+static int ad4170_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct iio_dev *indio_dev = gpiochip_get_data(gc);
+	struct ad4170_state *st = iio_priv(indio_dev);
+	unsigned int val;
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = regmap_read(st->regmap16, AD4170_GPIO_MODE_REG, &val);
+	if (ret)
+		goto err_release;
+
+	/*
+	 * If the GPIO is configured as an input, read the current value from
+	 * AD4170_GPIO_INPUT_REG. Otherwise, read the input value from
+	 * AD4170_GPIO_OUTPUT_REG.
+	 */
+	if (val & BIT(offset * 2))
+		ret = regmap_read(st->regmap16, AD4170_GPIO_INPUT_REG, &val);
+	else
+		ret = regmap_read(st->regmap16, AD4170_GPIO_OUTPUT_REG, &val);
+	if (ret)
+		goto err_release;
+
+	ret = !!(val & BIT(offset));
+err_release:
+	iio_device_release_direct(indio_dev);
+
+	return ret;
+}
+
+static int ad4170_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct iio_dev *indio_dev = gpiochip_get_data(gc);
+	struct ad4170_state *st = iio_priv(indio_dev);
+	unsigned int val;
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = regmap_read(st->regmap16, AD4170_GPIO_MODE_REG, &val);
+	if (ret)
+		goto err_release;
+
+	if (val & BIT(offset * 2 + 1))
+		ret = regmap_update_bits(st->regmap16, AD4170_GPIO_OUTPUT_REG,
+					 BIT(offset), value << offset);
+
+err_release:
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static int ad4170_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct iio_dev *indio_dev = gpiochip_get_data(gc);
+	struct ad4170_state *st = iio_priv(indio_dev);
+	unsigned int val;
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = regmap_read(st->regmap16, AD4170_GPIO_MODE_REG, &val);
+	if (ret)
+		goto err_release;
+
+	if (val & BIT(offset * 2 + 1))
+		ret = GPIO_LINE_DIRECTION_OUT;
+	else
+		ret = GPIO_LINE_DIRECTION_IN;
+
+err_release:
+	iio_device_release_direct(indio_dev);
+
+	return ret;
+}
+
+static int ad4170_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+	struct iio_dev *indio_dev = gpiochip_get_data(gc);
+	struct ad4170_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = regmap_clear_bits(st->regmap16, AD4170_GPIO_MODE_REG,
+				BIT(offset * 2 + 1));
+	if (ret)
+		goto err_release;
+
+	ret = regmap_set_bits(st->regmap16, AD4170_GPIO_MODE_REG,
+			      BIT(offset * 2));
+
+err_release:
+	iio_device_release_direct(indio_dev);
+
+	return ret;
+}
+
+static int ad4170_gpio_direction_output(struct gpio_chip *gc,
+					unsigned int offset, int value)
+{
+	struct iio_dev *indio_dev = gpiochip_get_data(gc);
+	struct ad4170_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = regmap_clear_bits(st->regmap16, AD4170_GPIO_MODE_REG,
+				BIT(offset * 2));
+	if (ret)
+		goto err_release;
+
+	ret = regmap_set_bits(st->regmap16, AD4170_GPIO_MODE_REG,
+			      BIT(offset * 2 + 1));
+
+err_release:
+	iio_device_release_direct(indio_dev);
+
+	ad4170_gpio_set(gc, offset, value);
+	return ret;
+}
+
+static int ad4170_gpio_init(struct iio_dev *indio_dev)
+{
+	struct ad4170_state *st = iio_priv(indio_dev);
+
+	st->gpiochip = (struct gpio_chip) {
+		.label = "ad4170_gpios",
+		.base = -1,
+		.ngpio = 4,
+		.parent = &st->spi->dev,
+		.can_sleep = true,
+		.get_direction = ad4170_gpio_get_direction,
+		.direction_input = ad4170_gpio_direction_input,
+		.direction_output = ad4170_gpio_direction_output,
+		.get = ad4170_gpio_get,
+		.set_rv = ad4170_gpio_set,
+		.owner = THIS_MODULE,
+	};
+
+	return devm_gpiochip_add_data(&st->spi->dev, &st->gpiochip, indio_dev);
+}
+
 static int ad4170_parse_reference(struct ad4170_state *st,
 				  struct fwnode_handle *child,
 				  struct ad4170_setup *setup)
@@ -1855,7 +2009,18 @@ static int ad4170_parse_firmware(struct iio_dev *indio_dev)
 	if (ret)
 		return ret;
 
-	return ad4170_parse_channels(indio_dev);
+	ret = ad4170_parse_channels(indio_dev);
+	if (ret)
+		return ret;
+
+	/* Only create a GPIO chip if flagged for it */
+	if (device_property_read_bool(&st->spi->dev, "gpio-controller")) {
+		ret = ad4170_gpio_init(indio_dev);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
 }
 
 static int ad4170_initial_config(struct iio_dev *indio_dev)
-- 
2.47.2

On Wed, Apr 9, 2025 at 3:26 PM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> The AD4170 has four multifunctional pins that can be used as GPIOs. The
> GPIO functionality can be accessed when the AD4170 chip is not busy
> performing continuous data capture or handling any other register
> read/write request. Also, the AD4170 does not provide any interrupt based
> on GPIO pin states so AD4170 GPIOs can't be used as interrupt sources.
>
> Implement gpio_chip callbacks so to make AD4170 GPIO pins controllable

callbacks to

> through the gpiochip interface.

...

> +static int ad4170_gpio_direction_output(struct gpio_chip *gc,
> +                                       unsigned int offset, int value)
> +{
> +       struct iio_dev *indio_dev = gpiochip_get_data(gc);
> +       struct ad4170_state *st = iio_priv(indio_dev);
> +       int ret;
> +
> +       if (!iio_device_claim_direct(indio_dev))
> +               return -EBUSY;
> +
> +       ret = regmap_clear_bits(st->regmap16, AD4170_GPIO_MODE_REG,
> +                               BIT(offset * 2));
> +       if (ret)
> +               goto err_release;
> +
> +       ret = regmap_set_bits(st->regmap16, AD4170_GPIO_MODE_REG,
> +                             BIT(offset * 2 + 1));
> +
> +err_release:
> +       iio_device_release_direct(indio_dev);
> +
> +       ad4170_gpio_set(gc, offset, value);

This is incorrect ordering, you will have glitches. Can you set the
value beforehands? Or is it broken hardware?

> +       return ret;
> +}

...

> +static int ad4170_gpio_init(struct iio_dev *indio_dev)
> +{
> +       struct ad4170_state *st = iio_priv(indio_dev);

> +       st->gpiochip = (struct gpio_chip) {
> +               .label = "ad4170_gpios",
> +               .base = -1,
> +               .ngpio = 4,
> +               .parent = &st->spi->dev,
> +               .can_sleep = true,
> +               .get_direction = ad4170_gpio_get_direction,
> +               .direction_input = ad4170_gpio_direction_input,
> +               .direction_output = ad4170_gpio_direction_output,
> +               .get = ad4170_gpio_get,
> +               .set_rv = ad4170_gpio_set,
> +               .owner = THIS_MODULE,
> +       };

I think it would be better to have it field by field initialised.

> +       return devm_gpiochip_add_data(&st->spi->dev, &st->gpiochip, indio_dev);
> +}

...

> +       /* Only create a GPIO chip if flagged for it */
> +       if (device_property_read_bool(&st->spi->dev, "gpio-controller")) {
> +               ret = ad4170_gpio_init(indio_dev);
> +               if (ret < 0)

< 0 ? What is the meaning of the positive values that you expect from
this function?

> +                       return ret;
> +       }

-- 
With Best Regards,
Andy Shevchenko

On Wed, 2025-04-09 at 09:25 -0300, Marcelo Schmitt wrote:
> The AD4170 has four multifunctional pins that can be used as GPIOs. The
> GPIO functionality can be accessed when the AD4170 chip is not busy
> performing continuous data capture or handling any other register
> read/write request. Also, the AD4170 does not provide any interrupt based
> on GPIO pin states so AD4170 GPIOs can't be used as interrupt sources.
> 
> Implement gpio_chip callbacks so to make AD4170 GPIO pins controllable
> through the gpiochip interface.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---

Just some doubts, see below...

>  drivers/iio/adc/ad4170.c | 167 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 166 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
> index 97cf4465038f..b382e7f3dbe0 100644
> --- a/drivers/iio/adc/ad4170.c
> +++ b/drivers/iio/adc/ad4170.c
> @@ -12,6 +12,7 @@
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> +#include <linux/gpio/driver.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -79,6 +80,7 @@
>  #define AD4170_FIR_CTRL					0x141
>  #define AD4170_COEFF_DATA_REG				0x14A
>  #define AD4170_COEFF_ADDR_REG				0x14C
> +#define AD4170_GPIO_MODE_REG				0x191
>  #define AD4170_GPIO_OUTPUT_REG				0x193
>  #define AD4170_GPIO_INPUT_REG				0x195
>  
> @@ -189,6 +191,7 @@
>  /* Device properties and auxiliary constants */
>  
>  #define AD4170_NUM_ANALOG_PINS				9
> +#define AD4170_NUM_GPIO_PINS				4
>  #define AD4170_MAX_CHANNELS				16
>  #define AD4170_MAX_ANALOG_PINS				8
>  #define AD4170_MAX_SETUPS				8
> @@ -340,6 +343,7 @@ struct ad4170_state {
>  	struct clk *ext_clk;
>  	struct clk_hw int_clk_hw;
>  	int pins_fn[AD4170_NUM_ANALOG_PINS];
> +	struct gpio_chip gpiochip;
>  	u32 int_pin_sel;
>  	int
> sps_tbl[ARRAY_SIZE(ad4170_filt_names)][AD4170_MAX_FS_TBL_SIZE][2];
>  	struct completion completion;
> @@ -1553,6 +1557,156 @@ static int ad4170_soft_reset(struct ad4170_state *st)
>  	return 0;
>  }
>  
> +static int ad4170_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct iio_dev *indio_dev = gpiochip_get_data(gc);
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +	unsigned int val;
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	ret = regmap_read(st->regmap16, AD4170_GPIO_MODE_REG, &val);
> +	if (ret)
> +		goto err_release;
> +
> +	/*
> +	 * If the GPIO is configured as an input, read the current value from
> +	 * AD4170_GPIO_INPUT_REG. Otherwise, read the input value from
> +	 * AD4170_GPIO_OUTPUT_REG.
> +	 */
> +	if (val & BIT(offset * 2))
> +		ret = regmap_read(st->regmap16, AD4170_GPIO_INPUT_REG, &val);
> +	else
> +		ret = regmap_read(st->regmap16, AD4170_GPIO_OUTPUT_REG,
> &val);
> +	if (ret)
> +		goto err_release;
> +
> +	ret = !!(val & BIT(offset));
> +err_release:
> +	iio_device_release_direct(indio_dev);
> +
> +	return ret;
> +}
> +
> +static int ad4170_gpio_set(struct gpio_chip *gc, unsigned int offset, int
> value)
> +{
> +	struct iio_dev *indio_dev = gpiochip_get_data(gc);
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +	unsigned int val;
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	ret = regmap_read(st->regmap16, AD4170_GPIO_MODE_REG, &val);
> +	if (ret)
> +		goto err_release;
> +
> +	if (val & BIT(offset * 2 + 1))

Why do we need this? Are we checking if it's a GPO? If so, we should return
-EPERM in case we have a GPI?
 
> +		ret = regmap_update_bits(st->regmap16,
> AD4170_GPIO_OUTPUT_REG,
> +					 BIT(offset), value << offset);
> +
> +err_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}
> +
> +static int ad4170_gpio_get_direction(struct gpio_chip *gc, unsigned int
> offset)
> +{
> +	struct iio_dev *indio_dev = gpiochip_get_data(gc);
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +	unsigned int val;
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +

This claim_direct() makes me wonder if there's any overlap between the GPIO func
and normal readings? Like, imagine a consumer requests a gpio and no buffering
is happening so all is good. However, there's nothing stopping us for enabling
buffering afterwards, right? Wouldn't that be an issue? If there are shared
pins, I can see this also being an issue even for single shot reading...
Otherwise, I wonder why we have this iio_device_claim_direct() calls? Is it just
for using the internal IIO lock?

At this point, I did not checked the datasheet so I can be completely
misunderstanding things...

- Nuno Sá

...
> > +static int ad4170_gpio_get_direction(struct gpio_chip *gc, unsigned int
> > offset)
> > +{
> > +	struct iio_dev *indio_dev = gpiochip_get_data(gc);
> > +	struct ad4170_state *st = iio_priv(indio_dev);
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> 
> This claim_direct() makes me wonder if there's any overlap between the GPIO func
> and normal readings? Like, imagine a consumer requests a gpio and no buffering
> is happening so all is good. However, there's nothing stopping us for enabling
> buffering afterwards, right? Wouldn't that be an issue? If there are shared
> pins, I can see this also being an issue even for single shot reading...
> Otherwise, I wonder why we have this iio_device_claim_direct() calls? Is it just
> for using the internal IIO lock?

We need read/write to AD4170 registers to configure/set/read GPIOs so the intent
of claiming direct mode here is to avoid that from happening while we are doing
something else (e.g. a buffered capture). I'm now considering to also lock on the
state mutex to prevent concurrent run with the single-shot read routine.

Thanks,
Marcelo