Message-ID: <9018e23c-da28-41b0-b774-1598b946a2a1@gmail.com>
Date: Thu, 20 Feb 2025 16:21:37 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
 Chen-Yu Tsai <wens@...e.org>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Samuel Holland <samuel@...lland.org>,
 Hugo Villeneuve <hvilleneuve@...onoff.com>, Nuno Sa <nuno.sa@...log.com>,
 David Lechner <dlechner@...libre.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-renesas-soc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

On 20/02/2025 16:04, Andy Shevchenko wrote:
> On Thu, Feb 20, 2025 at 03:40:30PM +0200, Matti Vaittinen wrote:
>> 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:
> 
> ...
> 
>>>>>> +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.
> 
> It can be still pushed to IIO_CORE namespace. Do you see an issue with that?

No. I've missed the fact we have IIO_CORE O_o. Thanks for pointing it out!

> Or a new opaque namespace for the mentioned cases, something like IIO_HELPERS.

I am unsure if it really benefits to split this out of the IIO_CORE. 
I've a feeling it falls into the category of making things harder for 
user with no apparent reason. But yes, the IIO_CORE makes sense.

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

...

>>>> 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.
> 
> There are very obvious cases like below.

I think we just disagree on if this is obvious.

>>>> 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:
> 
> Strongly disagree. One need to parse an additional pair of parentheses,
> and especially when it's a big statement inside with nested ones along
> with understanding what the heck is going on that you need them in the
> first place.
> 
> On top of that, we have a common practices in the LK project and
> with our history of communication it seems you are trying to do differently
> from time to time. Sounds like a rebellion to me :-)

I only rebel when I (in my opinion) have a solid reason :)

>> foo &= ~bar;
>>
>> and having to google the priorities.
> 
> Again, this is something a (regular) kernel developer keeps refreshed.
> Or even wider, C-language developer.

Ha. As I mentioned, I've been writing C on a daily bases for almost 25 
years. I wonder if you intent to say I am not a kernel/C-language 
developer? Bold claim.

>>>>>> +		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...
> 
> The hardest to remember probably the
> 
> 	foo && bar | baz
> 
> case and alike. These are the only ones that I totally agree on with you.
> But negation.
> 
>> 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 ;)
> 
> Maybe someone programmed too much in LISP?.. (it's a rhetorical one)
> 
> ...
> 
>>>>>> +		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.
> 
> Maybe you need a bigger monitor after all? (lurking now :-)

Wouldn't fit my table :)

> ...
> 
>>>>>> +#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.
> 
> It's not your concern. That's the idea of making C units as much independent
> and modular as possible (with common sense in mind). And in this case I see
> no point of including this header. Again, the main problem is this will call
> people to use the new header as a "proxy" and that's what I fully against to.
> 
>> And, I need the iio_chan_spec here.
> 
> Do you really need it or is it just a pointer?

Just a pointer. Forward declaration could do it. Hmm. I didn't think of 
people using this header as a proxy. I guess you have a point here :)

> 
> ...
> 
>> 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.
> 
> Whenever this code will be trying to land, the review comments still apply.

Sure! But chances are plenty of this code gets erased :) I just wanted 
to warn you that some of the effort on this version is likely to get 
wasted. I did consider reverting this back to a RFC - but going back'n 
forth with the RFC status felt odd...

Yours,
	-- Matti

Powered by blists - more mailing lists