Re: [PATCH v5 02/11] iio: adc: Add basic support for AD4170

[Andy Shevchenko]

On Wed, Jun 11, 2025 at 06:04:49PM -0300, Marcelo Schmitt wrote:
> On 06/11, Andy Shevchenko wrote:
> > On Tue, Jun 10, 2025 at 05:31:25PM -0300, Marcelo Schmitt wrote:

...

> > > +	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.

Hmm... But shouldn't we have a variable type that respects the sizeof() of the
register in HW to keep it there? 2 is magic.

...

> > > +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.

Yep, which thinks to me as an unneeded noise in the code, making it harder to
read and maintain (in _this_ case).

> > 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.

I would argue that those needs to be revisited for the same reasons as above.

> 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;
> > > +}

...

> > > +	/* 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 :)

This needs a good comment and ideally a TODO item in the regulator framework.
(It might be easy to implement by adding a flag without changing the type of
 the field, if it's unsigned.)

> > 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.

-- 
With Best Regards,
Andy Shevchenko