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

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 03/09/2025 16:29, Andy Shevchenko wrote:
> On Wed, Sep 3, 2025 at 3:14 PM Matti Vaittinen <mazziesaccount@gmail.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.

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

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.

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

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

Yours,
	-- Matti