linux-kernel - Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

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: <CAHp75Ve4vgU5kK3z3bZyGqDOPVkMbW7RUd6_EA3jjZSeruWs=Q@mail.gmail.com>
Date: Wed, 3 Sep 2025 16:29:51 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Andy Shevchenko <andriy.shevchenko@...el.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 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:

...

> >>>> +static int bd79112_probe(struct spi_device *spi)
> >>>> +{
> >>>> +  /* ADC channels as named in the data-sheet */
> >>>> +  static const char * const chan_names[] = {
> >>>> +          "AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", "AGIO4A", "AGIO5A",
> >>>> +          "AGIO6A", "AGIO7A", "AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A",
> >>>> +          "AGIO11A", "AGIO12A", "AGIO13A", "AGIO14A", "AGIO15A",
> >>>> +          "AGIO0B", "AGIO1B", "AGIO2B", "AGIO3B", "AGIO4B", "AGIO5B",
> >>>> +          "AGIO6B", "AGIO7B", "AGIO8B", "AGIO9B", "AGIO10B", "AGIO11B",
> >>>> +          "AGIO11B", "AGIO12B", "AGIO13B", "AGIO14B", "AGIO15B",
> >>>
> >>> Can you make all of the lines to be the same in terms of amount of entries?
> >>
> >> Maybe :) I would like to know why? As you know, I prefer to keep lines short
> >> to fit multiple terminals in parallel, so this will probably make the entry
> >> to consume more rows. Thus, I would like to have a solid reason.
> >
> > Sure, the array above is unindexed. It's prone to errors and typos.
>
> Ha. Thanks :) I see it now when I count the entries :) Should be 32,
> was 34. I agree this would have been easier to spot!
>
> > Moreover, it's really hard to follow in case one needs to debug such
> > a typo and see which value needs to be fixed (imagine you typed twice
> > the same name).
>
> Or, if I typed twice the same name twice ;) Thanks!

TBH, I even haven't noticed that the array _has_ already mistakes :-)

> > Recommended way is to use power-of-two per line (and even add a comment
> > at the end), like
> >
> > static const char * const chan_names[] = {
> >       "AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A",         /*  0 -  3 */
> >       "AGIO4A", "AGIO5A", "AGIO6A", "AGIO7A",         /*  4 -  7 */
> >       "AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A",       /*  8 - 11 */
> >       ...
> >
> > (or hexadecimal offsets, whatever is better and more in accordance with
> >   the SW / data sheet).
>
> Ok, This makes sense now.
>
> >>>> +  };

...

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

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

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

...

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

-- 
With Best Regards,
Andy Shevchenko