| Message ID | 6c5b678526e227488592d004c315a967b9809701.1739967040.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support ROHM BD79124 ADC | expand |
On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote: > There are ADC ICs which may have some of the AIN pins usable for other > functions. These ICs may have some of the AIN pins wired so that they > should not be used for ADC. > > (Preferred?) way for marking pins which can be used as ADC inputs is to > add corresponding channels@N nodes in the device tree as described in > the ADC binding yaml. > > Add couple of helper functions which can be used to retrieve the channel > information from the device node. ... > - Rename iio_adc_device_get_channels() as as? ... > 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? > obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o > obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o > obj-$(CONFIG_IMX93_ADC) += imx93_adc.o ... + bitops.h > +#include <linux/device.h> > +#include <linux/errno.h> + export.h + module.h > +#include <linux/property.h> + types.h ... > +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels); No namespace? ... > + if (!allowed_types || allowed_types & (~IIO_ADC_CHAN_PROP_TYPE_ALL)) { Unneeded parentheses around negated value. > + dev_dbg(dev, "Invalid adc allowed prop types 0x%lx\n", > + allowed_types); > + > + return -EINVAL; > + } > + 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? > + 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; > + } ... > +int iio_adc_device_channels_by_property(struct device *dev, int *channels, > + int max_channels, const struct iio_adc_props *expected_props) > +{ > + int num_chan = 0, ret; > + > + device_for_each_child_node_scoped(dev, child) { > + u32 ch, diff[2], se; > + struct iio_adc_props tmp; > + int chtypes_found = 0; > + > + if (!fwnode_name_eq(child, "channel")) > + continue; > + > + if (num_chan == max_channels) > + return -EINVAL; > + > + ret = fwnode_property_read_u32(child, "reg", &ch); > + if (ret) > + return ret; > + > + ret = fwnode_property_read_u32_array(child, "diff-channels", > + &diff[0], 2); diff, ARRAY_SIZE(diff)); (will require array_size.h) > + if (!ret) > + chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF; > + > + ret = fwnode_property_read_u32(child, "single-channel", &se); > + if (!ret) > + chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED; > + > + tmp = *expected_props; > + /* > + * We don't bother reading the "common-mode-channel" here as it > + * doesn't really affect on the primary channel ID. We remove > + * it from the required properties to allow the sanity check > + * pass here also for drivers which require it. > + */ > + tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON)); Redundant outer parentheses. What's the point, please? > + ret = iio_adc_prop_type_check_sanity(dev, &tmp, chtypes_found); > + if (ret) > + return ret; > + > + if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF) > + ch = diff[0]; > + else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED) > + ch = se; > + > + /* > + * We assume the channel IDs start from 0. If it seems this is > + * not a sane assumption, then we can relax this check or add > + * 'allowed ID range' parameter. > + * > + * Let's just start with this simple assumption. > + */ > + if (ch >= max_channels) > + return -ERANGE; > + > + channels[num_chan] = ch; > + num_chan++; > + } > + > + return num_chan; > + > +} ... > +int devm_iio_adc_device_alloc_chaninfo(struct device *dev, > + const struct iio_chan_spec *template, > + struct iio_chan_spec **cs, > + const struct iio_adc_props *expected_props) > +{ > + struct iio_chan_spec *chan; > + int num_chan = 0, ret; > + > + num_chan = iio_adc_device_num_channels(dev); > + if (num_chan < 1) > + return num_chan; > + > + *cs = devm_kcalloc(dev, num_chan, sizeof(**cs), GFP_KERNEL); > + if (!*cs) > + return -ENOMEM; > + > + chan = &(*cs)[0]; This and above and below will be easier to read if you introduce a temporary variable which will be used locally and assigned to the output later on. Also the current approach breaks the rule that infiltrates the output even in the error cases. > + device_for_each_child_node_scoped(dev, child) { > + u32 ch, diff[2], se, common; > + int chtypes_found = 0; > + > + if (!fwnode_name_eq(child, "channel")) > + continue; > + > + ret = fwnode_property_read_u32(child, "reg", &ch); > + if (ret) > + return ret; > + > + ret = fwnode_property_read_u32_array(child, "diff-channels", > + &diff[0], 2); As per above. > + if (!ret) > + chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF; > + > + ret = fwnode_property_read_u32(child, "single-channel", &se); > + if (!ret) > + chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED; > + ret = fwnode_property_read_u32(child, "common-mode-channel", > + &common); I believe this is okay to have on a single line, > + if (!ret) > + chtypes_found |= BIT(IIO_ADC_CHAN_PROP_COMMON); > + > + ret = iio_adc_prop_type_check_sanity(dev, expected_props, > + chtypes_found); > + if (ret) > + return ret; > + > + *chan = *template; > + chan->channel = ch; > + > + if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF) { > + chan->differential = 1; > + chan->channel = diff[0]; > + chan->channel2 = diff[1]; > + > + } else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED) { > + chan->channel = se; > + if (chtypes_found & BIT(IIO_ADC_CHAN_PROP_COMMON)) > + chan->channel2 = common; > + } > + > + /* > + * We assume the channel IDs start from 0. If it seems this is > + * not a sane assumption, then we have to add 'allowed ID ranges' > + * to the struct iio_adc_props because some of the callers may > + * rely on the IDs being in this range - and have arrays indexed > + * by the ID. > + */ > + if (chan->channel >= num_chan) > + return -ERANGE; > + > + chan++; > + } > + > + return num_chan; > +} ... > +#ifndef _INDUSTRIAL_IO_ADC_HELPERS_H_ > +#define _INDUSTRIAL_IO_ADC_HELPERS_H_ + bits.h > +#include <linux/iio/iio.h> I'm failing to see how this is being used in this header. > +struct device; > +struct fwnode_handle; > + > +enum { > + IIO_ADC_CHAN_PROP_REG, > + IIO_ADC_CHAN_PROP_SINGLE_ENDED, > + IIO_ADC_CHAN_PROP_DIFF, > + IIO_ADC_CHAN_PROP_COMMON, > + IIO_ADC_CHAN_NUM_PROP_TYPES > +}; > + > +/* > + * Channel property types to be used with iio_adc_device_get_channels, > + * devm_iio_adc_device_alloc_chaninfo, ... Looks like unfinished sentence... > + */ > +#define IIO_ADC_CHAN_PROP_TYPE_REG BIT(IIO_ADC_CHAN_PROP_REG) > +#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED) > +#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_COMMON \ > + (BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED) | BIT(IIO_ADC_CHAN_PROP_COMMON)) > +#define IIO_ADC_CHAN_PROP_TYPE_DIFF BIT(IIO_ADC_CHAN_PROP_DIFF) > +#define IIO_ADC_CHAN_PROP_TYPE_ALL GENMASK(IIO_ADC_CHAN_NUM_PROP_TYPES - 1, 0) > +int devm_iio_adc_device_alloc_chaninfo(struct device *dev, > + const struct iio_chan_spec *template, > + struct iio_chan_spec **cs, > + const struct iio_adc_props *expected_props); > + > +int iio_adc_device_channels_by_property(struct device *dev, int *channels, > + int max_channels, > + const struct iio_adc_props *expected_props); > +#endif /* _INDUSTRIAL_IO_ADC_HELPERS_H_ */
Hi Andy, Long time no hear ;) First of all, thanks for the review! On 19/02/2025 22:41, Andy Shevchenko wrote: > On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote: >> There are ADC ICs which may have some of the AIN pins usable for other >> functions. These ICs may have some of the AIN pins wired so that they >> should not be used for ADC. >> >> (Preferred?) way for marking pins which can be used as ADC inputs is to >> add corresponding channels@N nodes in the device tree as described in >> the ADC binding yaml. >> >> Add couple of helper functions which can be used to retrieve the channel >> information from the device node. > > ... > >> - Rename iio_adc_device_get_channels() as > > as? Oh, looks like I got interrupted :) Thanks! > > ... > >> 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. >> obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o >> obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o >> obj-$(CONFIG_IMX93_ADC) += imx93_adc.o > > ... > > > + bitops.h > >> +#include <linux/device.h> >> +#include <linux/errno.h> > > + export.h > > + module.h > >> +#include <linux/property.h> > > + types.h Thanks! > ... > >> +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. (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. > >> + dev_dbg(dev, "Invalid adc allowed prop types 0x%lx\n", >> + allowed_types); >> + >> + return -EINVAL; >> + } >> + 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. 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. 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. > >> + 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; >> + } > > ... > >> +int iio_adc_device_channels_by_property(struct device *dev, int *channels, >> + int max_channels, const struct iio_adc_props *expected_props) >> +{ >> + int num_chan = 0, ret; >> + >> + device_for_each_child_node_scoped(dev, child) { >> + u32 ch, diff[2], se; >> + struct iio_adc_props tmp; >> + int chtypes_found = 0; >> + >> + if (!fwnode_name_eq(child, "channel")) >> + continue; >> + >> + if (num_chan == max_channels) >> + return -EINVAL; >> + >> + ret = fwnode_property_read_u32(child, "reg", &ch); >> + if (ret) >> + return ret; >> + >> + ret = fwnode_property_read_u32_array(child, "diff-channels", >> + &diff[0], 2); > > diff, ARRAY_SIZE(diff)); > > (will require array_size.h) thanks :) And thanks for being helpful with the header - and there is no sarcasm! >> + if (!ret) >> + chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF; >> + >> + ret = fwnode_property_read_u32(child, "single-channel", &se); >> + if (!ret) >> + chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED; >> + >> + tmp = *expected_props; >> + /* >> + * We don't bother reading the "common-mode-channel" here as it >> + * doesn't really affect on the primary channel ID. We remove >> + * it from the required properties to allow the sanity check >> + * pass here also for drivers which require it. >> + */ >> + tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON)); > > Redundant outer parentheses. What's the point, please? Zero need to think of precedence. >> + ret = iio_adc_prop_type_check_sanity(dev, &tmp, chtypes_found); >> + if (ret) >> + return ret; >> + >> + if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF) >> + ch = diff[0]; >> + else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED) >> + ch = se; >> + >> + /* >> + * We assume the channel IDs start from 0. If it seems this is >> + * not a sane assumption, then we can relax this check or add >> + * 'allowed ID range' parameter. >> + * >> + * Let's just start with this simple assumption. >> + */ >> + if (ch >= max_channels) >> + return -ERANGE; >> + >> + channels[num_chan] = ch; >> + num_chan++; >> + } >> + >> + return num_chan; >> + >> +} > > ... > >> +int devm_iio_adc_device_alloc_chaninfo(struct device *dev, >> + const struct iio_chan_spec *template, >> + struct iio_chan_spec **cs, >> + const struct iio_adc_props *expected_props) >> +{ >> + struct iio_chan_spec *chan; >> + int num_chan = 0, ret; >> + >> + num_chan = iio_adc_device_num_channels(dev); >> + if (num_chan < 1) >> + return num_chan; >> + >> + *cs = devm_kcalloc(dev, num_chan, sizeof(**cs), GFP_KERNEL); >> + if (!*cs) >> + return -ENOMEM; >> + >> + chan = &(*cs)[0]; > > This and above and below will be easier to read if you introduce a temporary > variable which will be used locally and assigned to the output later on. > Also the current approach breaks the rule that infiltrates the output even in > the error cases. Agree. Thanks. > >> + device_for_each_child_node_scoped(dev, child) { >> + u32 ch, diff[2], se, common; >> + int chtypes_found = 0; >> + >> + if (!fwnode_name_eq(child, "channel")) >> + continue; >> + >> + ret = fwnode_property_read_u32(child, "reg", &ch); >> + if (ret) >> + return ret; >> + >> + ret = fwnode_property_read_u32_array(child, "diff-channels", >> + &diff[0], 2); > > As per above. > >> + if (!ret) >> + chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF; >> + >> + ret = fwnode_property_read_u32(child, "single-channel", &se); >> + if (!ret) >> + chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED; > >> + 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 ;) > >> + if (!ret) >> + chtypes_found |= BIT(IIO_ADC_CHAN_PROP_COMMON); >> + >> + ret = iio_adc_prop_type_check_sanity(dev, expected_props, >> + chtypes_found); >> + if (ret) >> + return ret; >> + >> + *chan = *template; >> + chan->channel = ch; >> + >> + if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF) { >> + chan->differential = 1; >> + chan->channel = diff[0]; >> + chan->channel2 = diff[1]; >> + >> + } else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED) { >> + chan->channel = se; >> + if (chtypes_found & BIT(IIO_ADC_CHAN_PROP_COMMON)) >> + chan->channel2 = common; >> + } >> + >> + /* >> + * We assume the channel IDs start from 0. If it seems this is >> + * not a sane assumption, then we have to add 'allowed ID ranges' >> + * to the struct iio_adc_props because some of the callers may >> + * rely on the IDs being in this range - and have arrays indexed >> + * by the ID. >> + */ >> + if (chan->channel >= num_chan) >> + return -ERANGE; >> + >> + chan++; >> + } >> + >> + return num_chan; >> +} > > ... > >> +#ifndef _INDUSTRIAL_IO_ADC_HELPERS_H_ >> +#define _INDUSTRIAL_IO_ADC_HELPERS_H_ > > + bits.h > >> +#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. > >> +struct device; >> +struct fwnode_handle; >> + >> +enum { >> + IIO_ADC_CHAN_PROP_REG, >> + IIO_ADC_CHAN_PROP_SINGLE_ENDED, >> + IIO_ADC_CHAN_PROP_DIFF, >> + IIO_ADC_CHAN_PROP_COMMON, >> + IIO_ADC_CHAN_NUM_PROP_TYPES >> +}; >> + >> +/* >> + * 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 ... > >> + */ >> +#define IIO_ADC_CHAN_PROP_TYPE_REG BIT(IIO_ADC_CHAN_PROP_REG) >> +#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED) >> +#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_COMMON \ >> + (BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED) | BIT(IIO_ADC_CHAN_PROP_COMMON)) >> +#define IIO_ADC_CHAN_PROP_TYPE_DIFF BIT(IIO_ADC_CHAN_PROP_DIFF) >> +#define IIO_ADC_CHAN_PROP_TYPE_ALL GENMASK(IIO_ADC_CHAN_NUM_PROP_TYPES - 1, 0) > >> +int devm_iio_adc_device_alloc_chaninfo(struct device *dev, >> + const struct iio_chan_spec *template, >> + struct iio_chan_spec **cs, >> + const struct iio_adc_props *expected_props); >> + >> +int iio_adc_device_channels_by_property(struct device *dev, int *channels, >> + int max_channels, >> + const struct iio_adc_props *expected_props); >> +#endif /* _INDUSTRIAL_IO_ADC_HELPERS_H_ */ > >
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. > > > + */
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
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? Or a new opaque namespace for the mentioned cases, something like IIO_HELPERS. > > > (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. There are very obvious cases like below. > > > 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 :-) > foo &= ~bar; > > and having to google the priorities. Again, this is something a (regular) kernel developer keeps refreshed. Or even wider, C-language developer. > > > > > + 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 :-) ... > > > > > +#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? ... > 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.
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
On Thu, Feb 20, 2025 at 04:21:37PM +0200, Matti Vaittinen wrote: > 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. Probably I was not clear, I mean to put this under a given namespace. There is no a such, we have currently: IIO_BACKEND IIO_DMA_BUFFER IIO_DMAENGINE_BUFFER IIO_GTS_HELPER IIO_RESCALE > > > > > (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... ... > > > 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. I'm just surprised by seeing that style from a 25y experienced C developer, that's all.
On 20/02/2025 16:56, Andy Shevchenko wrote: > On Thu, Feb 20, 2025 at 04:21:37PM +0200, Matti Vaittinen wrote: >> 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. > > Probably I was not clear, I mean to put this under a given namespace. There is > no a such, we have currently: > > IIO_BACKEND > IIO_DMA_BUFFER > IIO_DMAENGINE_BUFFER > IIO_GTS_HELPER > IIO_RESCALE Ah. So, the IIO core stuff is still not in a namespace. Those listed above are all too specific (I believe, in general, and definitely to carry ADC helpers). Adding 'ADC_HELPERS' would just add yet another way too specific one. So, currently there is no suitable namespace for these helpers, and I still believe they fit best to where the rest of the IIO-core stuff is. If we want really play the namespace game, then the existing IIO stuff should be put in a IIO_CORE-namespace instead of creating more new small ones. I am afraid that adding all existing IIO core to a IIO_CORE namespace and converting all existing users to use the IIO_CORE is not a reasonable request for a person trying to: 1. Write a driver 2. Add a small helper to aid others (instead of just melding it all in the given new driver - which does not benefit anyone else and just leads to code duplication in the long run...) >>>>>> (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... > > ... > >>>> 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. > > I'm just surprised by seeing that style from a 25y experienced C developer, > that's all. I am not. If something, these 25 years have taught me to understand that even if something is simple and obvious to me, it may not be simple and obvious to someone else. Similarly, something obvious to someone else, is not obvious to me. Hence, I am very careful when telling people that: >>> Again, this is something a (regular) kernel developer keeps refreshed. >>> Or even wider, C-language developer. I may however say that "this is something _I_ keep refreshed (as a kernel/C-developer)". As an example, >>>> foo &= (~bar); This is something _I_ find very clear and exact, with zero doubt if negation is applied before &=. For _me_ the parenthesis there _help_, and for _me_ the parenthesis cause no confusion when reading the code. I won't go and tell that I'd expect any C or kernel developer to be able to fluently parse "foo &= (~bar)". (Whether I think they should is another matter). Oh well, let's wait and see what Jonathan thinks of these helpers in general. We can continue the parenthesis discussion when we know whether the code is going to stay. Yours, -- Matti
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 849c90203071..37b70a65da6f 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -6,6 +6,9 @@ menu "Analog to digital converters" +config IIO_ADC_HELPER + tristate + config AB8500_GPADC bool "ST-Ericsson AB8500 GPADC driver" depends on AB8500_CORE && REGULATOR_AB8500 diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index ee19afba62b7..956c121a7544 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -57,6 +57,7 @@ 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 obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o obj-$(CONFIG_IMX93_ADC) += imx93_adc.o diff --git a/drivers/iio/adc/industrialio-adc.c b/drivers/iio/adc/industrialio-adc.c new file mode 100644 index 000000000000..0281d64ae112 --- /dev/null +++ b/drivers/iio/adc/industrialio-adc.c @@ -0,0 +1,304 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Helpers for parsing common ADC information from a firmware node. + * + * Copyright (c) 2025 Matti Vaittinen <mazziesaccount@gmail.com> + */ + +#include <linux/device.h> +#include <linux/errno.h> +#include <linux/property.h> + +#include <linux/iio/adc-helpers.h> + +int iio_adc_device_num_channels(struct device *dev) +{ + int num_chan = 0; + + device_for_each_child_node_scoped(dev, child) + if (fwnode_name_eq(child, "channel")) + num_chan++; + + return num_chan; +} +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels); + +static const char *iio_adc_type2prop(int type) +{ + switch (type) { + case IIO_ADC_CHAN_PROP_TYPE_REG: + return "reg"; + case IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED: + return "single-channel"; + case IIO_ADC_CHAN_PROP_TYPE_DIFF: + return "diff-channels"; + case IIO_ADC_CHAN_PROP_COMMON: + return "common-mode-channel"; + default: + return "unknown"; + } +} + +/* + * Sanity check. Ensure that: + * - At least some type(s) are allowed + * - All types found are also expected + * - If plain "reg" is not allowed, either single-ended or differential + * properties are found. + */ +static int iio_adc_prop_type_check_sanity(struct device *dev, + const struct iio_adc_props *expected_props, int found_types) +{ + unsigned long allowed_types = expected_props->allowed | + expected_props->required; + + if (!allowed_types || allowed_types & (~IIO_ADC_CHAN_PROP_TYPE_ALL)) { + dev_dbg(dev, "Invalid adc allowed prop types 0x%lx\n", + allowed_types); + + return -EINVAL; + } + if (found_types & (~allowed_types)) { + long unknown_types = found_types & (~allowed_types); + 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; + } + + /* + * The IIO_ADC_CHAN_PROP_TYPE_REG is special. We always require it to + * be found in the dt. (If not, we'll error out before calling this + * function.) However, listing it in 'allowed' types means the "reg" + * alone can be used to indicate the channel ID. + * + * Thus, we don't add it in the found properties either - so check for + * found and allowed properties passes even if user hasn't explicitly + * added the 'IIO_ADC_CHAN_PROP_TYPE_REG' to be allowed. (This is the + * case if either differential or single-ended property is required). + * + * Hence, for this check we need to explicitly add the + * IIO_ADC_CHAN_PROP_TYPE_REG to 'found' properties to make the check + * pass when "reg" is the property which is required to have the + * channel ID. + * + * We could of course always add the IIO_ADC_CHAN_PROP_TYPE_REG in + * allowed types and found types - but then we wouldn't catch the case + * where user says the "reg" alone is not sufficient. + */ + if ((~(found_types | IIO_ADC_CHAN_PROP_TYPE_REG)) & expected_props->required) { + long missing_types; + int type; + + missing_types = (~found_types) & expected_props->required; + + for_each_set_bit(type, &missing_types, + IIO_ADC_CHAN_NUM_PROP_TYPES - 1) { + dev_err(dev, "required channel specifier '%s' not found\n", + iio_adc_type2prop(type)); + } + + return -EINVAL; + } + + /* Check if we require something else but the "reg" property */ + if (!(allowed_types & IIO_ADC_CHAN_PROP_TYPE_REG)) { + if (found_types & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED || + found_types & IIO_ADC_CHAN_PROP_TYPE_DIFF) + return 0; + + dev_err(dev, "channel specifier not found\n"); + + return -EINVAL; + } + + return 0; +} + +/** + * iio_adc_device_channels_by_property - get ADC channel IDs + * + * Scan the device node for ADC channel information. Return an array of found + * IDs. Caller needs to provide the memory for the array and provide maximum + * number of IDs the array can store. + * + * @dev: Pointer to the ADC device + * @channels: Array where the found IDs will be stored. + * @max_channels: Number of IDs that fit in the array. + * @expected_props: Bitmaps of channel property types (for checking). + * + * Return: Number of found channels on succes. 0 if no channels + * was found. Negative value to indicate failure. + */ +int iio_adc_device_channels_by_property(struct device *dev, int *channels, + int max_channels, const struct iio_adc_props *expected_props) +{ + int num_chan = 0, ret; + + device_for_each_child_node_scoped(dev, child) { + u32 ch, diff[2], se; + struct iio_adc_props tmp; + int chtypes_found = 0; + + if (!fwnode_name_eq(child, "channel")) + continue; + + if (num_chan == max_channels) + return -EINVAL; + + ret = fwnode_property_read_u32(child, "reg", &ch); + if (ret) + return ret; + + ret = fwnode_property_read_u32_array(child, "diff-channels", + &diff[0], 2); + if (!ret) + chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF; + + ret = fwnode_property_read_u32(child, "single-channel", &se); + if (!ret) + chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED; + + tmp = *expected_props; + /* + * We don't bother reading the "common-mode-channel" here as it + * doesn't really affect on the primary channel ID. We remove + * it from the required properties to allow the sanity check + * pass here also for drivers which require it. + */ + tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON)); + + ret = iio_adc_prop_type_check_sanity(dev, &tmp, chtypes_found); + if (ret) + return ret; + + if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF) + ch = diff[0]; + else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED) + ch = se; + + /* + * We assume the channel IDs start from 0. If it seems this is + * not a sane assumption, then we can relax this check or add + * 'allowed ID range' parameter. + * + * Let's just start with this simple assumption. + */ + if (ch >= max_channels) + return -ERANGE; + + channels[num_chan] = ch; + num_chan++; + } + + return num_chan; + +} +EXPORT_SYMBOL_GPL(iio_adc_device_channels_by_property); + +/** + * devm_iio_adc_device_alloc_chaninfo - allocate and fill iio_chan_spec for adc + * + * Scan the device node for ADC channel information. Allocate and populate the + * iio_chan_spec structure corresponding to channels that are found. The memory + * for iio_chan_spec structure will be freed upon device detach. Try parent + * device node if given device has no fwnode associated to cover also MFD + * devices. + * + * @dev: Pointer to the ADC device. + * @template: Template iio_chan_spec from which the fields of all + * found and allocated channels are initialized. + * @cs: Location where pointer to allocated iio_chan_spec + * should be stored. + * @expected_props: Bitmaps of channel property types (for checking). + * + * Return: Number of found channels on succes. Negative value to indicate + * failure. + */ +int devm_iio_adc_device_alloc_chaninfo(struct device *dev, + const struct iio_chan_spec *template, + struct iio_chan_spec **cs, + const struct iio_adc_props *expected_props) +{ + struct iio_chan_spec *chan; + int num_chan = 0, ret; + + num_chan = iio_adc_device_num_channels(dev); + if (num_chan < 1) + return num_chan; + + *cs = devm_kcalloc(dev, num_chan, sizeof(**cs), GFP_KERNEL); + if (!*cs) + return -ENOMEM; + + chan = &(*cs)[0]; + + device_for_each_child_node_scoped(dev, child) { + u32 ch, diff[2], se, common; + int chtypes_found = 0; + + if (!fwnode_name_eq(child, "channel")) + continue; + + ret = fwnode_property_read_u32(child, "reg", &ch); + if (ret) + return ret; + + ret = fwnode_property_read_u32_array(child, "diff-channels", + &diff[0], 2); + if (!ret) + chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF; + + ret = fwnode_property_read_u32(child, "single-channel", &se); + if (!ret) + chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED; + + ret = fwnode_property_read_u32(child, "common-mode-channel", + &common); + if (!ret) + chtypes_found |= BIT(IIO_ADC_CHAN_PROP_COMMON); + + ret = iio_adc_prop_type_check_sanity(dev, expected_props, + chtypes_found); + if (ret) + return ret; + + *chan = *template; + chan->channel = ch; + + if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF) { + chan->differential = 1; + chan->channel = diff[0]; + chan->channel2 = diff[1]; + + } else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED) { + chan->channel = se; + if (chtypes_found & BIT(IIO_ADC_CHAN_PROP_COMMON)) + chan->channel2 = common; + } + + /* + * We assume the channel IDs start from 0. If it seems this is + * not a sane assumption, then we have to add 'allowed ID ranges' + * to the struct iio_adc_props because some of the callers may + * rely on the IDs being in this range - and have arrays indexed + * by the ID. + */ + if (chan->channel >= num_chan) + return -ERANGE; + + chan++; + } + + return num_chan; +} +EXPORT_SYMBOL_GPL(devm_iio_adc_device_alloc_chaninfo); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>"); +MODULE_DESCRIPTION("IIO ADC fwnode parsing helpers"); diff --git a/include/linux/iio/adc-helpers.h b/include/linux/iio/adc-helpers.h new file mode 100644 index 000000000000..f7791d45dbd2 --- /dev/null +++ b/include/linux/iio/adc-helpers.h @@ -0,0 +1,56 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +/* The industrial I/O ADC helpers + * + * Copyright (c) 2025 Matti Vaittinen <mazziesaccount@gmail.com> + */ + +#ifndef _INDUSTRIAL_IO_ADC_HELPERS_H_ +#define _INDUSTRIAL_IO_ADC_HELPERS_H_ + +#include <linux/iio/iio.h> + +struct device; +struct fwnode_handle; + +enum { + IIO_ADC_CHAN_PROP_REG, + IIO_ADC_CHAN_PROP_SINGLE_ENDED, + IIO_ADC_CHAN_PROP_DIFF, + IIO_ADC_CHAN_PROP_COMMON, + IIO_ADC_CHAN_NUM_PROP_TYPES +}; + +/* + * Channel property types to be used with iio_adc_device_get_channels, + * devm_iio_adc_device_alloc_chaninfo, ... + */ +#define IIO_ADC_CHAN_PROP_TYPE_REG BIT(IIO_ADC_CHAN_PROP_REG) +#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED) +#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_COMMON \ + (BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED) | BIT(IIO_ADC_CHAN_PROP_COMMON)) +#define IIO_ADC_CHAN_PROP_TYPE_DIFF BIT(IIO_ADC_CHAN_PROP_DIFF) +#define IIO_ADC_CHAN_PROP_TYPE_ALL GENMASK(IIO_ADC_CHAN_NUM_PROP_TYPES - 1, 0) + +/** + * iio_adc_chan_props - information of expected device-tree channel properties + * + * @required: Bitmask of property definitions of required channel properties + * @allowed: Bitmask of property definitions of optional channel properties. + * Listing of required properties is not needed here. + */ +struct iio_adc_props { + unsigned long required; + unsigned long allowed; +}; + +int iio_adc_device_num_channels(struct device *dev); +int devm_iio_adc_device_alloc_chaninfo(struct device *dev, + const struct iio_chan_spec *template, + struct iio_chan_spec **cs, + const struct iio_adc_props *expected_props); + +int iio_adc_device_channels_by_property(struct device *dev, int *channels, + int max_channels, + const struct iio_adc_props *expected_props); +#endif /* _INDUSTRIAL_IO_ADC_HELPERS_H_ */
There are ADC ICs which may have some of the AIN pins usable for other functions. These ICs may have some of the AIN pins wired so that they should not be used for ADC. (Preferred?) way for marking pins which can be used as ADC inputs is to add corresponding channels@N nodes in the device tree as described in the ADC binding yaml. Add couple of helper functions which can be used to retrieve the channel information from the device node. Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> --- Revision history: v2 => v3: Mostly based on review comments by Jonathan - Support differential and single-ended channels(*) - Rename iio_adc_device_get_channels() as - Improve spelling - Drop support for cases where DT comes from parent device's node - Decrease loop indent by reverting node name check conditions - Don't set 'chan->indexed' by number of channels to keep the interface consistent no matter how many channels are connected. - Fix ID range check and related comment RFC v1 => v2: - New patch (*) The support for single-ended and differential channels is 100% untested. I am not convinced it is actually an improvement over the *_simple() helpers which only supported getting the ID from the "reg". In theory they could be used. In practice, while I skimmed through the in-tree ADC drivers which used the for_each_child_node() construct - it seemed that most of those which supported differential inputs had also some other per-channel properties to read. Those users would in any case need to loop through the nodes to get those other properties. If I am once more allowed to go back to proposing the _simple() variant which only covers the case: "chan ID in 'reg' property"... Dropping support for differential and single-ended channels would simplify this helper a lot. It'd allow dropping the sanity check as well as the extra parameters callers need to pass to tell what kind of properties they expect. That'd (in my opinion) made the last patches (to actual ADC drivers) in this series a much more lean and worthy ;) Finally, we could add own functions for differential/single-ended/all channels when the next driver which uses differential or single-ended channels - and which does not need other per-channel properties - lands in tree. That would still simplify the helper API usage for those drivers touched at the end of this series. I (still) think it might be nice to have helpers for fetching also the other generic (non vendor specific) ADC properties (as listed in the Documentation/devicetree/bindings/iio/adc/adc.yaml) - but as I don't have use for those in BD79124 driver (at least not for now), I don't imnplement them yet. Anyways, this commit creates a place for such helpers. --- drivers/iio/adc/Kconfig | 3 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/industrialio-adc.c | 304 +++++++++++++++++++++++++++++ include/linux/iio/adc-helpers.h | 56 ++++++ 4 files changed, 364 insertions(+) create mode 100644 drivers/iio/adc/industrialio-adc.c create mode 100644 include/linux/iio/adc-helpers.h