Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Feb 20, 2025 at 09:13:00AM +0200, Matti Vaittinen wrote:
> On 19/02/2025 22:41, Andy Shevchenko wrote:
> > On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:

...

> > > obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
> > >   obj-$(CONFIG_GEHC_PMC_ADC) += gehc-pmc-adc.o
> > >   obj-$(CONFIG_HI8435) += hi8435.o
> > >   obj-$(CONFIG_HX711) += hx711.o
> > 
> > > +obj-$(CONFIG_IIO_ADC_HELPER) += industrialio-adc.o
> > 
> > Shouldn't this be grouped with other IIO core related objects?
> 
> I was unsure where to put this. The 'adc' subfolder contained no other IIO
> core files, so there really was no group. I did consider putting it on top
> of the file but then just decided to go with the alphabetical order. Not
> sure what is the right way though.

I think it would be nice to have it grouped even if this one becomes
the first one.

> > >   obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
> > >   obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
> > >   obj-$(CONFIG_IMX93_ADC) += imx93_adc.o

...

> > > +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
> > 
> > No namespace?
> 
> I was considering also this. The IIO core functions don't belong into a
> namespace - so I followed the convention to keep these similar to other IIO
> core stuff.

But it's historically. We have already started using namespaces
in the parts of IIO, haven't we?

> (Sometimes I have a feeling that the trend today is to try make things
> intentionally difficult in the name of the safety. Like, "more difficult I
> make this, more experience points I gain in the name of the safety".)
> 
> Well, I suppose I could add a namespace for these functions - if this
> approach stays - but I'd really prefer having all IIO core stuff in some
> global IIO namespace and not to have dozens of fine-grained namespaces for
> an IIO driver to use...

...

> > > +	if (!allowed_types || allowed_types & (~IIO_ADC_CHAN_PROP_TYPE_ALL)) {
> > 
> > Unneeded parentheses around negated value.
> > 
> > > +	if (found_types & (~allowed_types)) {
> > 
> > Ditto.
> > 
> > > +		long unknown_types = found_types & (~allowed_types);
> > 
> > Ditto and so on...
> > 
> > Where did you get this style from? I think I see it first time in your
> > contributions. Is it a new preferences? Why?
> 
> Last autumn I found out my house was damaged by water. I had to empty half
> of the rooms and finally move out for 2.5 months.

Sad to hear that... Hope that your house had been recovered (to some extent?).

> Now I'm finally back, but
> during the moves I lost my printed list of operator precedences which I used
> to have on my desk. I've been writing C for 25 years or so, and I still
> don't remember the precedence rules for all bitwise operations - and I am
> fairly convinced I am not the only one.

~ (a.k.a. negation) is higher priority in bitops and it's idiomatic
(at least in LK project).

> What I understood is that I don't really have to have a printed list at
> home, or go googling when away from home. I can just make it very, very
> obvious :) Helps me a lot.

Makes code harder to read, especially in the undoubtful cases like

	foo &= (~...);

> > > +		int type;
> > > +
> > > +		for_each_set_bit(type, &unknown_types,
> > > +				 IIO_ADC_CHAN_NUM_PROP_TYPES - 1) {
> > > +			dev_err(dev, "Unsupported channel property %s\n",
> > > +				iio_adc_type2prop(type));
> > > +		}
> > > +
> > > +		return -EINVAL;
> > > +	}

...

> > > +		tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON));
> > 
> > Redundant outer parentheses. What's the point, please?
> 
> Zero need to think of precedence.

Huh? See above.
Everything with equal sign is less precedence than normal ops.

...

> > > +		ret = fwnode_property_read_u32(child, "common-mode-channel",
> > > +					       &common);
> > 
> > I believe this is okay to have on a single line,
> 
> I try to keep things under 80 chars. It really truly helps me as I'd like to
> have 3 parallel terminals open when writing code. Furthermore, I hate to
> admit it but during the last two years my near vision has deteriorated... :/
> 40 is getting more distant and 50 is approaching ;)

It's only 86 altogether with better readability.
We are in the second quarter of 21st century,
the 80 should be left in 80s...

(and yes, I deliberately put the above too short).

...

> > > +#include <linux/iio/iio.h>
> > 
> > I'm failing to see how this is being used in this header.
> 
> I suppose it was the struct iio_chan_spec. Yep, forward declaration could
> do, but I guess there would be no benefit because anyone using this header
> is more than likely to use the iio.h as well.

Still, it will be a beast to motivate people not thinking about what they are
doing. I strongly prefer avoiding the use of the "proxy" or dangling headers.

...

> > > +/*
> > > + * Channel property types to be used with iio_adc_device_get_channels,
> > > + * devm_iio_adc_device_alloc_chaninfo, ...
> > 
> > Looks like unfinished sentence...
> 
> Intention was to just give user an example of functions where this gets
> used, and leave room for more functions to be added. Reason is that lists
> like this tend to end up being incomplete anyways. Hence the ...

At least you may add ').' (without quotes) to that to make it clear.

> > > + */

-- 
With Best Regards,
Andy Shevchenko