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
