On 03/09/2025 16:29, Andy Shevchenko wrote:

On Wed, Sep 3, 2025 at 3:14 PM Matti Vaittinen [off-list ref] wrote:

quoted

On 03/09/2025 14:23, Andy Shevchenko wrote:

quoted

On Wed, Sep 03, 2025 at 09:52:02AM +0300, Matti Vaittinen wrote:

quoted

On 02/09/2025 17:15, Andy Shevchenko wrote:

quoted

On Tue, Sep 02, 2025 at 03:24:31PM +0300, Matti Vaittinen wrote:

...

quoted

quoted

quoted

quoted

quoted

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

quoted

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.

quoted

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.

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.

quoted

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". Sorry but that's not really a good reason to me.

...

quoted

quoted

quoted

quoted

quoted

+  gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
+                                    iio_dev->num_channels);

quoted

+

Instead of leaving this rather unneeded blank line I would move above...

quoted

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

quoted

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

quoted

          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.

Yours,
	-- Matti