linux-kernel - Re: [PATCH v3 8/8] iio: adc: Add events support to ad4052

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <2uknsmgz57wie4cv2tll3ttfyiw7lyjyaryc74nd3o5fteoazk@vbgdt5ofkn5r>
Date: Mon, 16 Jun 2025 15:54:56 +0200
From: Jorge Marques <gastmaier@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Jorge Marques <jorge.marques@...log.com>, 
	Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>, 
	David Lechner <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, Uwe Kleine-König <ukleinek@...nel.org>, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v3 8/8] iio: adc: Add events support to ad4052

On Sat, Jun 14, 2025 at 11:36:16AM +0100, Jonathan Cameron wrote:
> On Tue, 10 Jun 2025 09:34:41 +0200
> Jorge Marques <jorge.marques@...log.com> wrote:
> 
> > The AD4052 family supports autonomous monitoring readings for threshold
> > crossings. Add support for catching the GPIO interrupt and expose as an IIO
> > event. The device allows to set either, rising and falling directions. Only
> > either threshold crossing is implemented.
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@...log.com>
Hi Jonathan,
> Hi Jorge,
> 
> A few comments inline.
> 
> Jonathan
> 
> >
> > +
> > +static int ad4052_write_event_config(struct iio_dev *indio_dev,
> > +				     const struct iio_chan_spec *chan,
> > +				     enum iio_event_type type,
> > +				     enum iio_event_direction dir,
> > +				     bool state)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +	if (st->wait_event == state) {
> > +		ret = 0;
> 
> Feels like a case where init ret at declaration would be reasonable.
> 
Ack.
> > +		goto out_release;
> > +	}
> > +
> > +	if (state)
> > +		ret = ad4052_monitor_mode_enable(st);
> > +	else
> > +		ret = ad4052_monitor_mode_disable(st);
> > +
> > +	if (!ret)
> > +		st->wait_event = state;
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret;
> > +}
> 
> > +
> > +static int ad4052_read_event_value(struct iio_dev *indio_dev,
> > +				   const struct iio_chan_spec *chan,
> > +				   enum iio_event_type type,
> > +				   enum iio_event_direction dir,
> > +				   enum iio_event_info info, int *val,
> > +				   int *val2)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> > +		ret = -EBUSY;
> > +		goto out_release;
> 

Below are two distinct options with different implications.
> Not being able to read event parameters whilst monitoring them seems
> very restrictive.  Can't we cache the values?  Either play games to ensure
> we get them from the regmap cache or just cache these few values in st.
> 
> Checking what you are monitoring for feels like the sort of thing
> userspace might well do.

(1)
I agree, I can investigate regcache_cache_only and the other cache
options to achieve this. If I come to the conclusion it is not possible,
storing into st will achieve the same.

> 
> Even blocking changing the monitoring parameters is unusually strict.
> Why not just drop out of monitor mode, update them and go back in?
> 
(2)
The core point of the blocking behaviour is to not have hidden downtimes
in the monitoring for the user. An early driver used to do what you
describe and it was a design decision.

Since a custom regmap_bus was necessary to restrict the regmap access
speed (ADC access is faster), bringing back this by behavior embedding
it in the custom regmap now seems plausible, with proper explanation in
the rst page. This should fully dismiss the st->wait_event -> -EBUSY.

Considering (1) and (2), what is the preferred approach?

Regards,
Jorge
> > +	}
> > +
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		ret = __ad4052_read_event_info_value(st, dir, val);
> > +		break;
> > +	case IIO_EV_INFO_HYSTERESIS:
> > +		ret = __ad4052_read_event_info_hysteresis(st, dir, val);
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret ? ret : IIO_VAL_INT;
> > +}