linux-kernel - Re: [PATCH v1 2/7] iio: adc: Add basic support for AD4170

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: <20250412171920.531993c1@jic23-huawei>
Date: Sat, 12 Apr 2025 17:19:20 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Marcelo Schmitt <marcelo.schmitt1@...il.com>
Cc: Nuno Sá <noname.nuno@...il.com>, Marcelo Schmitt
 <marcelo.schmitt@...log.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, Ana-Maria Cusco
 <ana-maria.cusco@...log.com>, lars@...afoo.de,
 Michael.Hennerich@...log.com, dlechner@...libre.com, nuno.sa@...log.com,
 andy@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org
Subject: Re: [PATCH v1 2/7] iio: adc: Add basic support for AD4170

On Fri, 11 Apr 2025 12:38:35 -0300
Marcelo Schmitt <marcelo.schmitt1@...il.com> wrote:

> Hi Nuno, thank you for your review.
> I've already applied most of your suggestions.
> Also providing answers to some questions inline.
> 
> Thanks
> 
> On 04/10, Nuno Sá wrote:
> > Hi Marecelo,
> > 
> > First, superficial look...
> > 
> > On Wed, 2025-04-09 at 09:24 -0300, Marcelo Schmitt wrote:  
> > > From: Ana-Maria Cusco <ana-maria.cusco@...log.com>
> > > 
> > > Add support for the AD4170 ADC with the following features:
> > > - Single-shot read.
> > > - Analog front end PGA configuration.
> > > - Digital filter and sampling frequency configuration.
> > > - Calibration gain and offset configuration.
> > > - Differential and pseudo-differential input configuration.
> > > 
> > > Signed-off-by: Ana-Maria Cusco <ana-maria.cusco@...log.com>
> > > Co-developed-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> > > ---  
> ...
> > > +	ret = ad4170_set_channel_enable(st, chan->address, true);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	reinit_completion(&st->completion);  
> > 
> > I would do the above right before wait_for_completion_timeout()...  
> 
> Ack. Changed accordingly.
Hi Marcelo,

A general efficiency comment.  If you agree with a comment, no
need to reply.  Better to crop that bit of the thread out and
reply only to those parts where there is more to add.

Cuts down on what reviewers need to read!  We assume anything
you don't comment on is something you are happy with changing as
suggested!

I normally only point this out to people who are sending a lot
of code as it's just not worth it for one time contributors.
So bad luck you get me being fussier ;)


> > > +	ret = devm_regulator_get_enable_read_voltage(dev, "refin1n");
> > > +	if (ret < 0 && ret != -ENODEV)
> > > +		return dev_err_probe(dev, ret, "Failed to get REFIN- voltage.\n");
> > > +
> > > +	/* Negative supplies are assumed to provide negative voltage */
> > > +	st->vrefs_uv[AD4170_REFIN1N_SUP] = ret == -ENODEV ? -ENODEV : -ret;  
> > 
> > Maybe to early for me but the comment does not make it clear to me why the negation?
> > Won't the regulator return a negative voltage?  
> 
> devm_regulator_get_enable_read_voltage(), regulator_get_voltage(), and anything
> about reading the regulator voltage returns either a positive voltage value or
> a negative error code. I couldn't find out how to read a negative voltage with
> regulator API. So, for now, this is making the simplifying assumption that
> the negative end of external reference supplies is always below GND level (even
> though they could be positive).

Hmm. We went around this a long time back but I can't remember what the outcome was...
https://lore.kernel.org/linux-iio/544AC56F16B56944AEC3BD4E3D59177137546EF3FC@LIMKCMBX1.ad.analog.com/ 
looks like the thread.

Take a look at dac/ad5791.c for example of a negative reference

Michael might remember more of that discussion than I do!

Jonathan