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

[Matti Vaittinen]

On 20/02/2025 14:41, Andy Shevchenko wrote:
> 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.

I will move this on top of the file. (If these helpers stay. I think I 
wrote somewhere - maybe in the cover letter - that people are not sure 
if this is worth or if every driver should just use the fwnode APIs. 
Reviewers may want to save energy and do more accurate review only to 
the next version...)

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

Yes. But as I wrote, I don't think adding new namespaces for every 
helper file with a function or two exported will scale. We either need 
something common for IIO (or IIO "subsystems" like "adc", "accel", 
"light", ... ), or then we just keep these small helpers same as most of 
the IIO core.

> > (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?).

Thanks. I finalized rebuilding last weekend. Just moved back and now I'm 
trying to carry things back to right places... :rolleyes:

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

I know there are well established, accurate rules. Problem is that I 
never remember these without looking.

> > 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 &= (~...);

This is not undoubtful case for me :) And believe me, reading and 
deciphering the

foo &= (~bar);

is _much_ faster than seeing:

foo &= ~bar;

and having to google the priorities.

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

Sure. It's obvious if you remember that "Everything with equal sign is 
less precedence than normal ops". But as I said, I truly have hard time 
remembering these rules. When I try "going by memory" I end up having 
odd errors and suggestions to add parenthesis from the compiler...

By the way, do you know why anyone has bothered to add these 
warnings/suggestions about adding the parenthesis to the compiler? My 
guess is that I am not only one who needs the precedence charts ;)

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

I didn't even notice you had squeezed the lines :)

But yeah, I truly have problems fitting even 3 80 column terminals on 
screen with my current monitor. And when working on laptop screen it 
becomes impossible. Hence I strongly prefer keeping the 80 chars limit.

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

Ehh. There will be no IIO user who does not include the iio.h. And, I 
need the iio_chan_spec here.

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

Thanks. I agree, that's a good idea.

And as I said, I suggest saving some of the energy for reviewing the 
next version. I doubt the "property type" -flags and bitwise operations 
stay, and it may be all of this will be just meld in the bd79124 code - 
depending on what Jonathan & others think of it.

Yours,
	-- Matti