Re: [PATCH v2 9/9] iio: adc: ad4062: Add GPIO Controller support

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

On Mon, Nov 24, 2025 at 10:18:08AM +0100, Jorge Marques wrote:
> When gp0 or gp1 is not taken as an interrupt, expose them as gpo if

GPO

> gpio-contoller is set in the devicetree.

Why can't gpio-regmap be used?

...

> +static int ad4062_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct ad4062_state *st = gpiochip_get_data(gc);
> +	unsigned int reg_val;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD4062_REG_GP_CONF, &reg_val);
> +	if (ret)
> +		return 0;

> +	if (st->gpo_irq[offset])
> +		return -ENODEV;

Consider using valid_mask instead (.init_valid_mask() callback).
Hmm... And it seems it's in place. I didn't get what is here then and
why we need to do it after accessing the HW? If there are side-effects
they must be described.

> +	if (offset)
> +		reg_val = FIELD_GET(AD4062_REG_GP_CONF_MODE_MSK_1, reg_val);
> +	else
> +		reg_val = FIELD_GET(AD4062_REG_GP_CONF_MODE_MSK_0, reg_val);
> +
> +	return reg_val == AD4062_GP_STATIC_HIGH ? 1 : 0;

	return !!(reg_val == AD4062_GP_STATIC_HIGH);

also will work.

> +}

> +static int ad4062_gpio_init_valid_mask(struct gpio_chip *gc,
> +				       unsigned long *valid_mask,
> +				       unsigned int ngpios)
> +{
> +	struct ad4062_state *st = gpiochip_get_data(gc);
> +
> +	bitmap_zero(valid_mask, ngpios);
> +
> +	if (!st->gpo_irq[0])
> +		set_bit(0, valid_mask);
> +	if (!st->gpo_irq[1])
> +		set_bit(1, valid_mask);

Why atomic bit set:s?


> +	return 0;
> +}
> +
> +static int ad4062_gpio_init(struct ad4062_state *st)
> +{
> +	struct device *dev = &st->i3cdev->dev;
> +	struct gpio_chip *gc;
> +	u8 val, mask;
> +	int ret;

> +	if ((st->gpo_irq[0] && st->gpo_irq[1]) ||
> +	    !device_property_read_bool(dev, "gpio-controller"))
> +		return 0;

Do you need this? valid_mask should take care of this.

> +	gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
> +	if (!gc)
> +		return -ENOMEM;
> +
> +	val = 0;
> +	mask = 0;
> +	if (!st->gpo_irq[0]) {
> +		mask |= AD4062_REG_GP_CONF_MODE_MSK_0;
> +		val |= FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_0, AD4062_GP_STATIC_LOW);
> +	}
> +	if (!st->gpo_irq[1]) {
> +		mask |= AD4062_REG_GP_CONF_MODE_MSK_1;
> +		val |= FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_1, AD4062_GP_STATIC_LOW);
> +	}
> +
> +	ret = regmap_update_bits(st->regmap, AD4062_REG_GP_CONF,
> +				 mask, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, ad4062_gpio_disable, st);
> +	if (ret)
> +		return ret;
> +
> +	gc->parent = dev;
> +	gc->label = st->chip->name;
> +	gc->owner = THIS_MODULE;
> +	gc->base = -1;
> +	gc->ngpio = 2;
> +	gc->init_valid_mask = ad4062_gpio_init_valid_mask;
> +	gc->get_direction = ad4062_gpio_get_direction;
> +	gc->set = ad4062_gpio_set;
> +	gc->get = ad4062_gpio_get;
> +	gc->can_sleep = true;
> +
> +	ret = devm_gpiochip_add_data(dev, gc, st);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Unable to register GPIO chip\n");
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko