Re: [PATCH v6 06/11] iio: adc: ad7768-1: Add GPIO controller support

[Jonathan Santos <jonath4nns@gmail.com>]

On 04/28, Andy Shevchenko wrote:
> On Mon, Apr 28, 2025 at 3:13 AM Jonathan Santos
> <Jonathan.Santos@analog.com> wrote:
> >
> > The AD7768-1 has the ability to control other local hardware (such as gain
> > stages),to power down other blocks in the signal chain, or read local
> > status signals over the SPI interface.
> >
> > Add direct mode conditional locks in the gpio callbacks to prevent register
> > access when the device is in buffered mode.
> >
> > This change exports the AD7768-1's four gpios and makes them accessible
> > at an upper layer.
> 
> ...
> 
> > +#include <linux/gpio.h>
> 
> No way. This header must not be in any of the code. (Yes, there are
> leftovers in the kernel, but work is ongoing to clean that up)
> 

Sorry about that, will fix it.

> > +#include <linux/gpio/driver.h>
> >  #include <linux/gpio/consumer.h>
> 
> >  #include <linux/kernel.h>
> 
> And since you are doing the big series for the driver, please drop
> this header and replace it (if required) with what is used. No driver
> code should use kernel.h.
> 

Sure, noted.

> >  #include <linux/module.h>
> 
> ...
> 
> > struct ad7768_state {
> 
> >         struct regulator_dev *vcm_rdev;
> >         unsigned int vcm_output_sel;
> >         struct clk *mclk;
> > +       struct gpio_chip gpiochip;
> >         unsigned int mclk_freq;
> >         unsigned int samp_freq;
> >         struct completion completion;
> 
> Btw, have you run `pahole`? Is this the best place for a new field in
> accordance with its output?
> 

I didn't, but I am going to use it.

> ...
> 
> > +static int ad7768_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
> > +{
> > +       struct iio_dev *indio_dev = gpiochip_get_data(chip);
> > +       struct ad7768_state *st = iio_priv(indio_dev);
> > +       unsigned int val;
> > +       int ret;
> > +
> > +       if (!iio_device_claim_direct(indio_dev))
> > +               return -EBUSY;
> > +
> > +       ret = regmap_read(st->regmap, AD7768_REG_GPIO_CONTROL, &val);
> > +       if (ret)
> > +               goto err_release;
> > +
> > +       if (val & BIT(offset))
> > +               ret = regmap_update_bits(st->regmap, AD7768_REG_GPIO_WRITE,
> > +                                        BIT(offset), value << offset);
> 
> And if value happens to be > 1?
> Also consider the use of regmap_assign_bits().
> 

ok!

> > +err_release:
> > +       iio_device_release_direct(indio_dev);
> > +
> > +       return ret;
> > +}
> 
> ...
> 
> > +static int ad7768_gpio_init(struct iio_dev *indio_dev)
> > +{
> > +       struct ad7768_state *st = iio_priv(indio_dev);
> > +       int ret;
> > +
> > +       ret = regmap_write(st->regmap, AD7768_REG_GPIO_CONTROL,
> > +                          AD7768_GPIO_UNIVERSAL_EN);
> > +       if (ret)
> > +               return ret;
> > +
> > +       st->gpiochip = (struct gpio_chip) {
> 
> > +               .label = "ad7768_1_gpios",
> 
> What is '_1' for?
> Also, what will happen if the device has two or more such ADCs
> installed? Will they all provide _the same_ label?!
> 

the '_1' is because the part name is 'ad7768-1'.
Most of similar devices only define a static name, but I could use the
reg field, if you beleive it is better.

> > +               .base = -1,
> > +               .ngpio = 4,
> > +               .parent = &st->spi->dev,
> > +               .can_sleep = true,
> > +               .direction_input = ad7768_gpio_direction_input,
> > +               .direction_output = ad7768_gpio_direction_output,
> > +               .get = ad7768_gpio_get,
> > +               .set_rv = ad7768_gpio_set,
> > +               .owner = THIS_MODULE,
> > +       };
> > +
> > +       return devm_gpiochip_add_data(&st->spi->dev, &st->gpiochip, indio_dev);
> > +}
> 
> ...
> 
> > +       /* Only create a Chip GPIO if flagged for it */
> > +       if (device_property_read_bool(&st->spi->dev, "gpio-controller")) {
> > +               ret = ad7768_gpio_init(indio_dev);
> > +               if (ret < 0)
> 
> Why ' < 0'?
> 
> > +                       return ret;
> > +       }
> 
> -- 
> With Best Regards,
> Andy Shevchenko