Re: [PATCH v2 5/7] iio: adc: ad4170: Add GPIO controller support

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 28 Apr 2025 09:28:55 -0300
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 to make AD4170 GPIO pins controllable through
> the gpiochip interface.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
For GPIO controllers, even ones outside of driver/gpio we generally +CC the maintainer
and list.  They don't have to comment but that at least gets it on their radar if
we are misusing the GPIO chip infrastructure!
Make sure to do that on v3.

In meantime I didn't look hard, but I'm not immediately seeing why we have to disable
the gpios when switching direction.

Jonathan


>
> +
> +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->regmap, AD4170_GPIO_MODE_REG,
> +				BIT(offset * 2 + 1));
> +	if (ret)
> +		goto err_release;
> +
> +	ret = regmap_set_bits(st->regmap, AD4170_GPIO_MODE_REG,
> +			      BIT(offset * 2));
As below. If we have to go via disabled, then we need a comment on why.

> +
> +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;
> +
> +	ret = ad4170_gpio_set(gc, offset, value);
> +	if (ret)
> +		return ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	ret = regmap_clear_bits(st->regmap, AD4170_GPIO_MODE_REG,
> +				BIT(offset * 2));
This is odd enough that if required it needs a comment.
I suspect you can just directly update both bits in one write though
and this should be a regmap_update_bits() setting the two bit
field to the value 2 which is output (vs 1 which is input).

> +	if (ret)
> +		goto err_release;
> +
> +	ret = regmap_set_bits(st->regmap, AD4170_GPIO_MODE_REG,
> +			      BIT(offset * 2 + 1));
> +
> +err_release:
> +	iio_device_release_direct(indio_dev);
> +
> +	return ret;
> +}