Message-ID: <aLmOXTcUjw7cj9OK@smile.fi.intel.com>
Date: Thu, 4 Sep 2025 16:04:29 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Andy Shevchenko <andy.shevchenko@...il.com>,
	Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
	Jonathan Cameron <jic23@...nel.org>,
	David Lechner <dlechner@...libre.com>,
	Nuno Sá <nuno.sa@...log.com>,
	Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Marcelo Schmitt <marcelo.schmitt@...log.com>,
	Javier Carrasco <javier.carrasco.cruz@...il.com>,
	Tobias Sperling <tobias.sperling@...ting.com>,
	Antoniu Miclaus <antoniu.miclaus@...log.com>,
	Trevor Gamblin <tgamblin@...libre.com>,
	Esteban Blanc <eblanc@...libre.com>,
	Ramona Alexandra Nechita <ramona.nechita@...log.com>,
	Thomas Bonnefille <thomas.bonnefille@...tlin.com>,
	Hans de Goede <hansg@...nel.org>, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-gpio@...r.kernel.org
Subject: Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

On Thu, Sep 04, 2025 at 03:35:36PM +0300, Matti Vaittinen wrote:
> On 03/09/2025 16:29, Andy Shevchenko wrote:
> > On Wed, Sep 3, 2025 at 3:14 PM Matti Vaittinen <mazziesaccount@...il.com> wrote:
> > > On 03/09/2025 14:23, Andy Shevchenko wrote:
> > > > On Wed, Sep 03, 2025 at 09:52:02AM +0300, Matti Vaittinen wrote:
> > > > > On 02/09/2025 17:15, Andy Shevchenko wrote:
> > > > > > On Tue, Sep 02, 2025 at 03:24:31PM +0300, Matti Vaittinen wrote:

...

> > > > > > > +  data->vref_mv = ret / 1000;
> > > > > > 
> > > > > > (MICRO / MILLI)
> > > > > 
> > > > > I find this much more confusing than plain 1000. (I know we had this type of
> > > > > discussion before. See [1] again).
> > > > 
> > > > Rings a bell, but that's what IIO reviewers suggest to do nowadays as a
> > > > compromise between creating a new bunch of unit (V) related definitions.
> > > 
> > > I am sorry, but this just seems stupid to me. I'd say that it is very
> > > obvious for most of the readers dividing microvolts by 1000 results
> > > millivolts. And if it is not, then having this MICRO / MILLI is likely
> > > to just cause more confusion.
> > 
> > No, it tells that we have a value in microSOMETHING that is converted
> > to MILLIsomething.
> 
> No. I disagree. This tells that 'ret' from the regulator API is divided by
> some unknown value, which is a result of division of two odd defines.
> Especially odd because one would intuitively think MICRO is smaller than
> MILLI. You need to look up the definitions to understand WTF is really going
> on. I think this is plain terrible.
> 
> The fact that we store value in vref_mv should be enough of hint that idea
> is to have value in millivolts. Dividing by 1000 before assigning makes it
> 100% clear the ret is in microvolts even if you didn't know the regulator
> API to return micro volts.

"Dividing by 1000... makes 100% clear...". I disagree on this. It's not clear
and 1000 needs to be counted (it's harder to read, than reader needs to think
what mv and not mV (!) means) and so on. For bare minimum make it mV to
understand the semantics.

So, since it's one of the principal disagreements between us on this particular
issue, I leave it other reviewers and maintainers.

> > > I _really_ dislike these defines. Why is MILLI 1000? Why it isn't 0.001?
> > 
> > You know exactly a few reasons why it's not.
> > 
> > > It makes no sense that KILO and MILLI are the same. Especially not when
> > > we are dealing with physics.
> > 
> > Yes, this is the limitation of computers and particularly of _a_ kernel.
> 
> No. In my opinion, this is an example of, hopefully unintentional,
> obfuscation where blindly following some paradigm like 'avoid plain numbers
> and always use named defines' just results things getting worse. That
> combined with bad naming. If KILO is 1000, then MILLI can't be 1000. That's
> 1 per milli.

You have a point, send a patch!

> And still, the original "mv = uv / 1000" is superior in clarity. Using
> (MICRO / MILLI) there to avoid plain number is just a sign of blindly and
> religiously following some 'golden rule', even when it results worse code.

No, it's not.

> > > This is just an obfuscation compared to using plain 1000. (I kind of
> > > understand having a define for a value like 100000 - where counting the
> > > zeros gets cumbersome, although 100 * 1000 would be equally clear. But
> > > 1000 _is_ really 100% clear, whereas MICRO / MILLI is not).
> > 
> > See above why this way.
> 
> I see no real justification to degrade this - other than "because I say so".

You may (mis)interpret this, up to you.

> Sorry but that's not really a good reason to me.

OK.

...

> > > > > > > +  gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
> > > > > > > +                                    iio_dev->num_channels);
> > > > > > 
> > > > > > > +
> > > > > > 
> > > > > > Instead of leaving this rather unneeded blank line I would move above...
> > > > > > 
> > > > > > > +  /* We're done if all channels are reserved for ADC. */
> > > > > > 
> > > > > > ...to be here
> > > > > > 
> > > > > >      gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
> > > > > >                                        iio_dev->num_channels);
> > > > > 
> > > > > I suppose you mean something like:
> > > > > 
> > > > > register_gpios:
> > > > >       /* We're done if all channels are reserved for ADC. */
> > > > >       gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
> > > > >                                             iio_dev->num_channels);
> > > > >       if (!gpio_pins)
> > > > >               return 0;
> > > > > 
> > > > > right?
> > > > 
> > > > Yes.
> > > > 
> > > > > I don't like this because now the comment suggests we do call
> > > > > bd79112_get_gpio_pins() only to see if all channels were for ADCs. This,
> > > > > however, is not THE reason for this call, only an optimization. I think:
> > > > > having:
> > > > > 
> > > > >           /* We're done if all channels are reserved for ADC. */
> > > > 
> > > > Then you can amend the comment
> > > > 
> > > >            /* If all channels are reserved for ADC, we are done. */
> > > > 
> > > > >           if (!gpio_pins)
> > > > >                   return 0;
> > > > > 
> > > > > is clearer.
> > > > 
> > > > Which makes my approach sustainable.
> > > 
> > > I like your wording better, but placing this comment before the call to
> > > bd79112_get_gpio_pins() is still more confusing that placing it before
> > > the actual check:
> > >          if (!gpio_pins)
> > > is still misleading. Comment applies to the check, not the retrieval.
> > 
> > The variable assignment, or i.o.w. the source of the value we are
> > testing is also part of the equation.
> 
> The comment explains why the check, not why the value is obtained.

Yes, but value is not taken from out-of-nowhere. It belongs to that check.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists