Re: [PATCH 5/6] gpio: ad7768: Add AD7768 GPIO auxiliary driver

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Thu, Jul 09, 2026 at 10:50:16AM +0200, Janani Sunil wrote:
> The AD7768/AD7768-4 ADC exposes 5 general-purpose I/O pins that can be
> independently configured as inputs or outputs. Add an auxiliary bus driver
> to expose these pins as a GPIO chip, registered by the parent IIO driver.
> 
> The driver uses the parent's regmap for register access and delegates
> runtime power management to the parent device.

...

> +config GPIO_AD7768
> +	tristate "Analog Devices AD7768 GPIO support"
> +	depends on AD7768 && GPIOLIB

Make depend on GPIOLIB on a separate line (it helps a lot when grepping for
the users of GPIOLIB).

> +	help
> +	  Say yes here to expose the AD7768 utility pins as GPIOs when the
> +	  device tree node is marked as a GPIO controller.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called gpio-ad7768.

>  config GPIO_LTC4283

Shouldn't Kconfig entry be aligned by order with Makefile ordering?

...

> +#include <linux/auxiliary_bus.h>

+ bits.h

> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>

Please, IWYU! I believe AD knows this very well and again same mistake from AD!

...

> +struct ad7768_gpio_state {
> +	struct device *parent;
> +	struct regmap *regmap;

As far as I can see these two are dups. One may be derived from the other.
Try both and check (with probably bloat-o-meter) which one is better.

> +	struct mutex lock; /* protects regmap accesses */

This is not fully correct comment. This protects GPIO IO which may require more
than one regmap call in a row.

> +	struct gpio_chip gc;
> +};

...

> +static int ad7768_gpio_direction_input(struct gpio_chip *chip,
> +				       unsigned int offset)
> +{
> +	struct ad7768_gpio_state *st = gpiochip_get_data(chip);
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(st->parent, pm);

> +	int ret = PM_RUNTIME_ACQUIRE_ERR(&pm);

No, declare it usual way.

> +

And drop this blank line.

> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(st->regmap, AD7768_REG_GPIO_CONTROL,
> +				  BIT(offset), AD7768_GPIO_INPUT);
> +}

...

So, I briefly looked at the implementation and I don't understand why
gpio-regmap can't be used. Do you need PM runtime there? It can be
done for all (if absent).

...

> +static int ad7768_gpio_probe(struct auxiliary_device *adev,
> +			     const struct auxiliary_device_id *id)
> +{
> +	struct device *dev = &adev->dev;
> +	const char *label = dev_get_platdata(dev);
> +	struct ad7768_gpio_state *st;
> +	struct gpio_chip *gc;
> +	int ret;
> +
> +	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> +	if (!st)
> +		return -ENOMEM;
> +
> +	st->parent = dev->parent;
> +	st->regmap = dev_get_regmap(dev->parent, NULL);
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_resume_and_get(st->parent);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_update_bits(st->regmap, AD7768_REG_GPIO_CONTROL,
> +				 AD7768_GPIO_UGPIO_ENABLE,
> +				 AD7768_GPIO_UGPIO_ENABLE);
> +

> +	pm_runtime_mark_last_busy(st->parent);

For a few releases this is a dup call.

> +	pm_runtime_put_autosuspend(st->parent);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	gc = &st->gc;
> +	gc->label = label;
> +	gc->base = -1;
> +	gc->ngpio = AD7768_NUM_GPIOS;
> +	gc->parent = dev;
> +	gc->owner = THIS_MODULE;
> +	gc->can_sleep = true;
> +	gc->get_direction = ad7768_gpio_get_direction;
> +	gc->direction_input = ad7768_gpio_direction_input;
> +	gc->direction_output = ad7768_gpio_direction_output;
> +	gc->get = ad7768_gpio_get;
> +	gc->set = ad7768_gpio_set;
> +
> +	return devm_gpiochip_add_data(dev, &st->gc, st);
> +}

-- 
With Best Regards,
Andy Shevchenko