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
