linux-kernel - Re: [PATCH v1 5/7] iio: adc: ad4170: Add GPIO controller support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6669da27c78714d76ff21f810cd57045e90d701a.camel@gmail.com>
Date: Thu, 10 Apr 2025 10:53:14 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>, linux-iio@...r.kernel.org,
 	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: jic23@...nel.org, lars@...afoo.de, Michael.Hennerich@...log.com, 
	dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
 robh@...nel.org, 	krzk+dt@...nel.org, conor+dt@...nel.org,
 marcelo.schmitt1@...il.com
Subject: Re: [PATCH v1 5/7] iio: adc: ad4170: Add GPIO controller support

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@...log.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á