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

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

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

quoted

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 :-)

quoted

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.

quoted

quoted

quoted

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.

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.

...

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.

-- 
With Best Regards,
Andy Shevchenko