linux-kernel - Re: [PATCH v5 02/11] 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: <aErMgh6AKVStF4rQ@smile.fi.intel.com>
Date: Thu, 12 Jun 2025 15:48:02 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Marcelo Schmitt <marcelo.schmitt1@...il.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 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