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.
> > > 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?
> (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?).
> 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 &= (~...);
> > > + 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.
...
> > > + 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).
...
> > > +#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.
...
> > > +/*
> > > + * 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.
> > > + */
--
With Best Regards,
Andy Shevchenko
