On 20/02/2025 14:41, Andy Shevchenko wrote:

On Thu, Feb 20, 2025 at 09:13:00AM +0200, Matti Vaittinen wrote:

quoted

On 19/02/2025 22:41, Andy Shevchenko wrote:

quoted

On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:

...

quoted

quoted

quoted

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

quoted

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

quoted

quoted

quoted

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

...

quoted

quoted

quoted

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

quoted

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

...

quoted

quoted

quoted

+	if (!allowed_types || allowed_types & (~IIO_ADC_CHAN_PROP_TYPE_ALL)) {

Unneeded parentheses around negated value.

quoted

+	if (found_types & (~allowed_types)) {

Ditto.

quoted

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

quoted

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.

quoted

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.

quoted

quoted

quoted

+		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;
+	}

...

quoted

quoted

quoted

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

...

quoted

quoted

quoted

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

...

quoted

quoted

quoted

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

...

quoted

quoted

quoted

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