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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aSQ2JUN05vmMQC1I@smile.fi.intel.com>
Date: Mon, 24 Nov 2025 12:40:37 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Jorge Marques <jorge.marques@...log.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Jonathan Cameron <jic23@...nel.org>,
	David Lechner <dlechner@...libre.com>,
	Nuno Sá <nuno.sa@...log.com>,
	Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jonathan Corbet <corbet@....net>,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v2 9/9] iio: adc: ad4062: Add GPIO Controller support

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