linux-kernel - Re: [PATCH 3/7] iio: adc: Add support for ad4062

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: <aSQhCKBC36T9t-H1@smile.fi.intel.com>
Date: Mon, 24 Nov 2025 11:10:32 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Jorge Marques <gastmaier@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
	Jorge Marques <jorge.marques@...log.com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	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>, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH 3/7] iio: adc: Add support for ad4062

On Mon, Nov 24, 2025 at 09:57:26AM +0100, Jorge Marques wrote:
> On Mon, Nov 24, 2025 at 09:43:09AM +0200, Andy Shevchenko wrote:
> > On Sun, Nov 23, 2025 at 08:48:09PM +0100, Jorge Marques wrote:
> > > On Sat, Oct 18, 2025 at 05:10:32PM +0100, Jonathan Cameron wrote:
> > > > On Mon, 13 Oct 2025 09:28:01 +0200
> > > > Jorge Marques <jorge.marques@...log.com> wrote:

> > > Mostly acknowledgements and explanations, except a comment on ACQUIRE usage.

...

> > > > > +static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
> > > > > +{
> > > > > +	struct ad4062_state *st = iio_priv(indio_dev);
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> > > > There is a nice new
> > > > 	ACQUIRE()/ACQUIRE_ERR() related set of conditional guards defined that
> > > > let you do this using cleanup.h style.
> > > > 
> > > > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a0abc39450a3123fd52533a662fbd37e0d1508c
> > > > 
> > > > This looks like a perfect example of where those help.
> > > > 
> > > > When I catch up with review backlog I plan to look for other
> > > > places to use that infrastructure in IIO.
> > > > 
> > > I tried implementing, here becomes
> > > 
> > >         ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> > >         ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> > > 
> > > At buffer and monitor, since we put the device as active during the
> > > lifetime of the buffer and monitor mode, either I leave as is, or I bump
> > > the counter with pm_runtime_get_noresume, so when the method leaves, the
> > > counter drops to 1 and not 0, then on disable I drop the counter back to
> > > 0 and queue the autosuspend with pm_runtime_put_autosuspend.
> > > > 
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	ret = ad4062_set_operation_mode(st, st->mode);
> > > > > +	if (ret)
> > > > > +		goto out_error;
> > > > > +
> > > > > +	ret = __ad4062_read_chan_raw(st, val);
> > > > > +
> > > > > +out_error:
> > > > > +	pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > > > > +	return ret;
> > > > > +}
> > 
> > I read the above code, I read it again, I don't understand the reasoning.
> > The ACQUIRE() doesn't change the behaviour of the above code.
> > 
> > If you need to bump the reference counter, it should be done somewhere else
> > where it affects the flow, or this code has a bug.
> > 
> > If I miss something, please elaborate.
> 
> The part highlighted does not require bumping the reference counter, but
> at the buffer acquisition and monitor mode, to not put the device back
> in low power mode during the lifetime of those operations.
> 
> Buffer more:
> 
>   static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
>   {
>           struct ad4062_state *st = iio_priv(indio_dev);
>           int ret;
> 
> 	  // [ Some code ]
> 
>           ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
>           ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
>           if (ret)
>                   return ret;
> 
> 	  // [ More code ]

>           pm_runtime_get_noresume(&st->i3cdev->dev);


Yes, this looks good if it makes the error paths cleaner.
Also consider adding

	struct device *dev = &st->i3cdev->dev;

at the top of the functions that use it, it might make code better to read.

>           return 0;
>   }
> 
>   static int ad4062_triggered_buffer_predisable(struct iio_dev *indio_dev)
>   {
>           struct ad4062_state *st = iio_priv(indio_dev);
> 
>           pm_runtime_put_autosuspend(&st->i3cdev->dev);
>           return 0;
>   }
> 
> Monitor mode:
> 
>   static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
>   {
>           int ret = 0;
> 
>           if (!enable) {
>                   pm_runtime_put_autosuspend(&st->i3cdev->dev);
>                   return 0;
>           }
> 
>           ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
>           ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
>           if (ret)
>                   return ret;
> 
> 	  // [ Some code ]
> 
>           pm_runtime_get_noresume(&st->i3cdev->dev);
>           return 0;
>   }

-- 
With Best Regards,
Andy Shevchenko