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

[Andy Shevchenko]

On Wed, Apr 9, 2025 at 3:26 PM Marcelo Schmitt
<marcelo.schmitt@xxxxxxxxxx> 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