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

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

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.

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

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

...

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

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

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

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

...

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

...

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.

...

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.

quoted

quoted

+ */

-- 
With Best Regards,
Andy Shevchenko