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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 11 Apr 2025 12:38:35 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.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@analog.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@analog.com>
> > > Co-developed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.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