Message-ID: <aEnvcaP2ZNPLhzXi@debian-BULLSEYE-live-builder-AMD64>
Date: Wed, 11 Jun 2025 18:04:49 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Marcelo Schmitt <marcelo.schmitt@...log.com>, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-gpio@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Ana-Maria Cusco <ana-maria.cusco@...log.com>, jic23@...nel.org,
	lars@...afoo.de, Michael.Hennerich@...log.com,
	dlechner@...libre.com, nuno.sa@...log.com, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org, linus.walleij@...aro.org,
	brgl@...ev.pl
Subject: Re: [PATCH v5 02/11] iio: adc: Add basic support for AD4170

On 06/11, Andy Shevchenko wrote:
> On Tue, Jun 10, 2025 at 05:31:25PM -0300, Marcelo Schmitt wrote:
> > From: Ana-Maria Cusco <ana-maria.cusco@...log.com>
> > 
> > The AD4170 is a multichannel, low noise, 24-bit precision sigma-delta
> > analog to digital converter. The AD4170 design offers a flexible data
> > acquisition solution with crosspoint multiplexed analog inputs,
> > configurable ADC voltage reference inputs, ultra-low noise integrated PGA,
> > digital filtering, wide range of configurable output data rates, internal
> > oscillator and temperature sensor, four GPIOs, and integrated features for
> > interfacing with load cell weigh scales, RTD, and thermocouple sensors.
> > 
> > Add basic support for the AD4170 ADC with the following features:
> > - Single-shot read.
> > - Analog front end PGA configuration.
> > - Differential and pseudo-differential input configuration.
> 
> ...
> 
> > +	return spi_write(st->spi, st->tx_buf, size + 2);
> 
> ... + sizeof(reg) ?

The size of the specific ADC register is stored in the size variable.
The result of sizeof(reg) can be different on different machines and will
probably not be equal to the size of the register in the ADC chip.

> ...
> 
> > +static bool ad4170_setup_eq(struct ad4170_setup *a, struct ad4170_setup *b)
> > +{
> > +	/*
> > +	 * The use of static_assert() here is to make sure that, if
> > +	 * struct ad4170_setup is ever changed (e.g. a field is added to the
> > +	 * struct's declaration), the comparison below is adapted to keep
> > +	 * comparing each of struct ad4170_setup fields.
> > +	 */
> 
> Okay. But this also will trigger the case when the field just changes the type.
> So, it also brings false positives. I really think this is wrong place to put
> static_assert(). To me it looks like a solving rare problem, if any.

I think it is unlikely that struct ad4170_setup declaration will ever change.
The fields match the registers that are associated with a channel setup and
the their types match the size of the respective registers. So, I do agree
that triggering this assert would be something rare.

> 
> But I leave this to the IIO maintainers.
> 
> In my opinion static_assert() makes only sense when memcmp() is being used.
> Otherwise it has prons and cons.

I think the most relevant reason to have this static_assert would be to keep
some consistency with ad4130, ad7124, and ad7173, but no strong opinion about it.
Actually, I don't get why static_assert() would only matter if memcmp() was
being used. Would it be better to not bother if the fields change type?

Anyway, I'll go with whatever be IIO maintainer's preference.

> 
> > +	static_assert(sizeof(*a) ==
> > +		      sizeof(struct {
> > +				     u16 misc;
> > +				     u16 afe;
> > +				     u16 filter;
> > +				     u16 filter_fs;
> > +				     u32 offset;
> > +				     u32 gain;
> > +			     }));
> > +
> > +	if (a->misc != b->misc ||
> > +	    a->afe != b->afe ||
> > +	    a->filter != b->filter ||
> > +	    a->filter_fs != b->filter_fs ||
> > +	    a->offset != b->offset ||
> > +	    a->gain != b->gain)
> > +		return false;
> > +
> > +	return true;
> > +}
> 
> ...
> 
> > +	/*
> > +	 * Some configurations can lead to invalid setups.
> > +	 * For example, if AVSS = -2.5V, REF_SELECT set to REFOUT (REFOUT/AVSS),
> > +	 * and pseudo-diff channel configuration set, then the input range
> > +	 * should go from 0V to +VREF (single-ended - datasheet pg 10), but
> > +	 * REFOUT/AVSS range would be -2.5V to 0V.
> > +	 * Check the positive reference is higher than 0V for pseudo-diff
> > +	 * channels.
> > +	 */
> 
> Right, the Q is, can refp contain an error code here, rather than negative
> value? The code above hints that in some case it may, but are all those cases
> were caught up already? (Comment can be extended to explain this)

I don't think refp can contain an error code at this point. All regulators are
read at ad4170_regulator_setup(). After that setup,
st->vrefs_uv[AD4170_<SUPPLY>] will either contain the voltage read from the
regulator framework (which is >= 0) or -ENODEV. Then, we check the supply value
at the beginning of ad4170_get_input_range() (this function) and error out if
the value is -ENODEV. Will extend the comment to explain that.

> 
> > +	if (refp <= 0)
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "REF+ <= GND for pseudo-diff chan %u\n",
> > +				     ch_reg);
> 
...
> 
> > +	/* Assume AVSS at GND (0V) if not provided */
> > +	st->vrefs_uv[AD4170_AVSS_SUP] = ret == -ENODEV ? 0 : -ret;
> 
> -ret ?!?!

That's because AVSS is never above system ground level (i.e. AVSS is either GND
or a negative voltage). But we currently don't have support for reading negative
voltages with the regulator framework. So, the current AD4170 support reads
a positive value from the regulator, then inverts signal to make it negative :)

> 
> Even if you know that *now* it can't have any other error code, it's quite
> fragile.

Yeah, I guess ADCs that can take bipolar power supplies are not that common.
I couldn't think of any better way to have that, though.


Thanks,
Marcelo

Powered by blists - more mailing lists