| Message ID | 20220711123835.811358-14-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | make iio inkern interface firmware agnostic | expand |
On 7/11/22 14:38, Nuno Sá wrote: > Make the conversion to firmware agnostic device properties. As part of > the conversion the IIO inkern interface 'of_xlate()' is also converted to > 'fwnode_xlate()'. The goal is to completely drop 'of_xlate' and hence OF > dependencies from IIO. > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > --- > drivers/iio/adc/stm32-adc.c | 121 ++++++++++++++++++++---------------- > 1 file changed, 67 insertions(+), 54 deletions(-) > > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c > index 3985fe972892..e55859113df8 100644 > --- a/drivers/iio/adc/stm32-adc.c > +++ b/drivers/iio/adc/stm32-adc.c > @@ -21,11 +21,11 @@ > #include <linux/io.h> > #include <linux/iopoll.h> > #include <linux/module.h> > +#include <linux/mod_devicetable.h> > #include <linux/nvmem-consumer.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > -#include <linux/of.h> > -#include <linux/of_device.h> > +#include <linux/property.h> > > #include "stm32-adc-core.h" > > @@ -1530,8 +1530,8 @@ static int stm32_adc_update_scan_mode(struct iio_dev *indio_dev, > return ret; > } > > -static int stm32_adc_of_xlate(struct iio_dev *indio_dev, > - const struct of_phandle_args *iiospec) > +static int stm32_adc_fwnode_xlate(struct iio_dev *indio_dev, > + const struct fwnode_reference_args *iiospec) > { > int i; > > @@ -1585,7 +1585,7 @@ static const struct iio_info stm32_adc_iio_info = { > .hwfifo_set_watermark = stm32_adc_set_watermark, > .update_scan_mode = stm32_adc_update_scan_mode, > .debugfs_reg_access = stm32_adc_debugfs_reg_access, > - .of_xlate = stm32_adc_of_xlate, > + .fwnode_xlate = stm32_adc_fwnode_xlate, > }; > > static unsigned int stm32_adc_dma_residue(struct stm32_adc *adc) > @@ -1782,14 +1782,14 @@ static const struct iio_chan_spec_ext_info stm32_adc_ext_info[] = { > {}, > }; > > -static int stm32_adc_of_get_resolution(struct iio_dev *indio_dev) > +static int stm32_adc_fw_get_resolution(struct iio_dev *indio_dev) > { > - struct device_node *node = indio_dev->dev.of_node; > + struct device *dev = &indio_dev->dev; > struct stm32_adc *adc = iio_priv(indio_dev); > unsigned int i; > u32 res; > > - if (of_property_read_u32(node, "assigned-resolution-bits", &res)) > + if (device_property_read_u32(dev, "assigned-resolution-bits", &res)) > res = adc->cfg->adc_info->resolutions[0]; > > for (i = 0; i < adc->cfg->adc_info->num_res; i++) > @@ -1873,11 +1873,11 @@ static void stm32_adc_chan_init_one(struct iio_dev *indio_dev, > > static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm32_adc *adc) > { > - struct device_node *node = indio_dev->dev.of_node; > + struct device *dev = &indio_dev->dev; > const struct stm32_adc_info *adc_info = adc->cfg->adc_info; > int num_channels = 0, ret; > > - ret = of_property_count_u32_elems(node, "st,adc-channels"); > + ret = device_property_count_u32(dev, "st,adc-channels"); > if (ret > adc_info->max_channels) { > dev_err(&indio_dev->dev, "Bad st,adc-channels?\n"); > return -EINVAL; > @@ -1885,8 +1885,8 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm > num_channels += ret; > } > > - ret = of_property_count_elems_of_size(node, "st,adc-diff-channels", > - sizeof(struct stm32_adc_diff_channel)); > + /* each st,adc-diff-channels is a group of 2 u32 */ > + ret = device_property_count_u64(dev, "st,adc-diff-channels"); > if (ret > adc_info->max_channels) { > dev_err(&indio_dev->dev, "Bad st,adc-diff-channels?\n"); > return -EINVAL; > @@ -1896,7 +1896,7 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm > } > > /* Optional sample time is provided either for each, or all channels */ > - ret = of_property_count_u32_elems(node, "st,min-sample-time-nsecs"); > + ret = device_property_count_u32(dev, "st,min-sample-time-nsecs"); > if (ret > 1 && ret != num_channels) { > dev_err(&indio_dev->dev, "Invalid st,min-sample-time-nsecs\n"); > return -EINVAL; > @@ -1907,21 +1907,20 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm > > static int stm32_adc_legacy_chan_init(struct iio_dev *indio_dev, > struct stm32_adc *adc, > - struct iio_chan_spec *channels) > + struct iio_chan_spec *channels, > + int nchans) > { > - struct device_node *node = indio_dev->dev.of_node; > const struct stm32_adc_info *adc_info = adc->cfg->adc_info; > struct stm32_adc_diff_channel diff[STM32_ADC_CH_MAX]; > + struct device *dev = &indio_dev->dev; > u32 num_diff = adc->num_diff; > int size = num_diff * sizeof(*diff) / sizeof(u32); > - int scan_index = 0, val, ret, i; > - struct property *prop; > - const __be32 *cur; > - u32 smp = 0; > + int scan_index = 0, ret, i; > + u32 smp = 0, nsmps, smps[STM32_ADC_CH_MAX], chans[STM32_ADC_CH_MAX]; > > if (num_diff) { > - ret = of_property_read_u32_array(node, "st,adc-diff-channels", > - (u32 *)diff, size); > + ret = device_property_read_u32_array(dev, "st,adc-diff-channels", > + (u32 *)diff, size); > if (ret) { > dev_err(&indio_dev->dev, "Failed to get diff channels %d\n", ret); > return ret; > @@ -1942,32 +1941,51 @@ static int stm32_adc_legacy_chan_init(struct iio_dev *indio_dev, > } > } > > - of_property_for_each_u32(node, "st,adc-channels", prop, cur, val) { > - if (val >= adc_info->max_channels) { > - dev_err(&indio_dev->dev, "Invalid channel %d\n", val); > + ret = device_property_read_u32_array(dev, "st,adc-channels", chans, > + nchans); > + if (ret) > + return ret; > + > + for (i = 0; i < nchans; i++) { Hi Nuno, There's an endless loop issue here due to the two for loops are using the same index 'i'. (Please look at my earlier comments in v1) for (i = 0; i < nchans; i++) { ... // Here i gets cleared, num_diff may vary (example with 0 here) for (i = 0; i < num_diff; i++) { ... } stm32_adc_chan_init_one(.. &channels[scan_index].. scan_index..) scan_index++; // Still, scan index gets incremented indefinitly } There's an out of boudary situation with scan_index above, wich ends-up in crashing in stm32_adc_chan_init_one(). > + if (chans[i] >= adc_info->max_channels) { > + dev_err(&indio_dev->dev, "Invalid channel %d\n", > + chans[i]); > return -EINVAL; > } > > /* Channel can't be configured both as single-ended & diff */ > for (i = 0; i < num_diff; i++) { > - if (val == diff[i].vinp) { > - dev_err(&indio_dev->dev, "channel %d misconfigured\n", val); > + if (chans[i] == diff[i].vinp) { > + dev_err(&indio_dev->dev, "channel %d misconfigured\n", chans[i]); > return -EINVAL; > } > } > - stm32_adc_chan_init_one(indio_dev, &channels[scan_index], val, > - 0, scan_index, false); > + stm32_adc_chan_init_one(indio_dev, &channels[scan_index], > + chans[i], 0, scan_index, false); > scan_index++; > } > > + nsmps = device_property_count_u32(dev, "st,min-sample-time-nsecs"); > + if (nsmps) { > + if (nsmps >= nchans) nit: if (nsmps > nchans) > + nsmps = nchans; > + There's a bit of redundancy in checking nsmps, "st,min-sample-time-nsecs" is already sanitized in stm32_adc_get_legacy_chan_count(): /* Optional sample time is provided either for each, or all channels */ ret = device_property_count_u32(dev, "st,min-sample-time-nsecs"); if (ret > 1 && ret != num_channels) { dev_err(... So just sharing my thoughts here: - Maybe this could be dropped ? (Thinking loudly) The earliest this gets sanitized, the less un-needed initialisations happen before failing? - Or the earlier check could be moved here ? I've no strong opinion. Best Regards, Fabrice > + ret = device_property_read_u32_array(dev, "st,min-sample-time-nsecs", > + smps, nsmps); > + if (ret) > + return ret; > + } > + > for (i = 0; i < scan_index; i++) { > /* > - * Using of_property_read_u32_index(), smp value will only be > - * modified if valid u32 value can be decoded. This allows to > - * get either no value, 1 shared value for all indexes, or one > - * value per channel. > + * This check is used with the above logic so that smp value > + * will only be modified if valid u32 value can be decoded. This > + * allows to get either no value, 1 shared value for all indexes, > + * or one value per channel. The point is to have the same > + * behavior as 'of_property_read_u32_index()'. > */ > - of_property_read_u32_index(node, "st,min-sample-time-nsecs", i, &smp); > + if (i < nsmps) > + smp = smps[i]; > > /* Prepare sampling time settings */ > stm32_adc_smpr_init(adc, channels[i].channel, smp); > @@ -2010,22 +2028,21 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev, > struct stm32_adc *adc, > struct iio_chan_spec *channels) > { > - struct device_node *node = indio_dev->dev.of_node; > const struct stm32_adc_info *adc_info = adc->cfg->adc_info; > - struct device_node *child; > + struct fwnode_handle *child; > const char *name; > int val, scan_index = 0, ret; > bool differential; > u32 vin[2]; > > - for_each_available_child_of_node(node, child) { > - ret = of_property_read_u32(child, "reg", &val); > + device_for_each_child_node(&indio_dev->dev, child) { > + ret = fwnode_property_read_u32(child, "reg", &val); > if (ret) { > dev_err(&indio_dev->dev, "Missing channel index %d\n", ret); > goto err; > } > > - ret = of_property_read_string(child, "label", &name); > + ret = fwnode_property_read_string(child, "label", &name); > /* label is optional */ > if (!ret) { > if (strlen(name) >= STM32_ADC_CH_SZ) { > @@ -2050,7 +2067,7 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev, > } > > differential = false; > - ret = of_property_read_u32_array(child, "diff-channels", vin, 2); > + ret = fwnode_property_read_u32_array(child, "diff-channels", vin, 2); > /* diff-channels is optional */ > if (!ret) { > differential = true; > @@ -2067,7 +2084,7 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev, > stm32_adc_chan_init_one(indio_dev, &channels[scan_index], val, > vin[1], scan_index, differential); > > - ret = of_property_read_u32(child, "st,min-sample-time-ns", &val); > + ret = fwnode_property_read_u32(child, "st,min-sample-time-ns", &val); > /* st,min-sample-time-ns is optional */ > if (!ret) { > stm32_adc_smpr_init(adc, channels[scan_index].channel, val); > @@ -2085,14 +2102,13 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev, > return scan_index; > > err: > - of_node_put(child); > + fwnode_handle_put(child); > > return ret; > } > > -static int stm32_adc_chan_of_init(struct iio_dev *indio_dev, bool timestamping) > +static int stm32_adc_chan_fw_init(struct iio_dev *indio_dev, bool timestamping) > { > - struct device_node *node = indio_dev->dev.of_node; > struct stm32_adc *adc = iio_priv(indio_dev); > const struct stm32_adc_info *adc_info = adc->cfg->adc_info; > struct iio_chan_spec *channels; > @@ -2102,7 +2118,7 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev, bool timestamping) > for (i = 0; i < STM32_ADC_INT_CH_NB; i++) > adc->int_ch[i] = STM32_ADC_INT_CH_NONE; > > - num_channels = of_get_available_child_count(node); > + num_channels = device_get_child_node_count(&indio_dev->dev); > /* If no channels have been found, fallback to channels legacy properties. */ > if (!num_channels) { > legacy = true; > @@ -2133,7 +2149,8 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev, bool timestamping) > return -ENOMEM; > > if (legacy) > - ret = stm32_adc_legacy_chan_init(indio_dev, adc, channels); > + ret = stm32_adc_legacy_chan_init(indio_dev, adc, channels, > + num_channels); > else > ret = stm32_adc_generic_chan_init(indio_dev, adc, channels); > if (ret < 0) > @@ -2215,9 +2232,6 @@ static int stm32_adc_probe(struct platform_device *pdev) > bool timestamping = false; > int ret; > > - if (!pdev->dev.of_node) > - return -ENODEV; > - > indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc)); > if (!indio_dev) > return -ENOMEM; > @@ -2226,17 +2240,16 @@ static int stm32_adc_probe(struct platform_device *pdev) > adc->common = dev_get_drvdata(pdev->dev.parent); > spin_lock_init(&adc->lock); > init_completion(&adc->completion); > - adc->cfg = (const struct stm32_adc_cfg *) > - of_match_device(dev->driver->of_match_table, dev)->data; > + adc->cfg = device_get_match_data(dev); > > indio_dev->name = dev_name(&pdev->dev); > - indio_dev->dev.of_node = pdev->dev.of_node; > + device_set_node(&indio_dev->dev, dev_fwnode(&pdev->dev)); > indio_dev->info = &stm32_adc_iio_info; > indio_dev->modes = INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED; > > platform_set_drvdata(pdev, indio_dev); > > - ret = of_property_read_u32(pdev->dev.of_node, "reg", &adc->offset); > + ret = device_property_read_u32(dev, "reg", &adc->offset); > if (ret != 0) { > dev_err(&pdev->dev, "missing reg property\n"); > return -EINVAL; > @@ -2265,7 +2278,7 @@ static int stm32_adc_probe(struct platform_device *pdev) > } > } > > - ret = stm32_adc_of_get_resolution(indio_dev); > + ret = stm32_adc_fw_get_resolution(indio_dev); > if (ret < 0) > return ret; > > @@ -2282,7 +2295,7 @@ static int stm32_adc_probe(struct platform_device *pdev) > timestamping = true; > } > > - ret = stm32_adc_chan_of_init(indio_dev, timestamping); > + ret = stm32_adc_chan_fw_init(indio_dev, timestamping); > if (ret < 0) > goto err_dma_disable; >
Hi Fabrice, Nice that someone in ST is looking at this one :) On Mon, 2022-07-11 at 16:04 +0200, Fabrice Gasnier wrote: > On 7/11/22 14:38, Nuno Sá wrote: > > Make the conversion to firmware agnostic device properties. As part > > of > > the conversion the IIO inkern interface 'of_xlate()' is also > > converted to > > 'fwnode_xlate()'. The goal is to completely drop 'of_xlate' and > > hence OF > > dependencies from IIO. > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > --- > > drivers/iio/adc/stm32-adc.c | 121 ++++++++++++++++++++------------ > > ---- > > 1 file changed, 67 insertions(+), 54 deletions(-) > > > > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32- > > adc.c > > index 3985fe972892..e55859113df8 100644 > > --- a/drivers/iio/adc/stm32-adc.c > > +++ b/drivers/iio/adc/stm32-adc.c > > @@ -21,11 +21,11 @@ > > #include <linux/io.h> > > #include <linux/iopoll.h> > > #include <linux/module.h> > > +#include <linux/mod_devicetable.h> > > #include <linux/nvmem-consumer.h> > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > -#include <linux/of.h> > > -#include <linux/of_device.h> > > +#include <linux/property.h> > > > > #include "stm32-adc-core.h" > > > > @@ -1530,8 +1530,8 @@ static int stm32_adc_update_scan_mode(struct > > iio_dev *indio_dev, > > return ret; > > } > > > > -static int stm32_adc_of_xlate(struct iio_dev *indio_dev, > > - const struct of_phandle_args > > *iiospec) > > +static int stm32_adc_fwnode_xlate(struct iio_dev *indio_dev, > > + const struct > > fwnode_reference_args *iiospec) > > { > > int i; > > > > @@ -1585,7 +1585,7 @@ static const struct iio_info > > stm32_adc_iio_info = { > > .hwfifo_set_watermark = stm32_adc_set_watermark, > > .update_scan_mode = stm32_adc_update_scan_mode, > > .debugfs_reg_access = stm32_adc_debugfs_reg_access, > > - .of_xlate = stm32_adc_of_xlate, > > + .fwnode_xlate = stm32_adc_fwnode_xlate, > > }; > > > > static unsigned int stm32_adc_dma_residue(struct stm32_adc *adc) > > @@ -1782,14 +1782,14 @@ static const struct iio_chan_spec_ext_info > > stm32_adc_ext_info[] = { > > {}, > > }; > > > > -static int stm32_adc_of_get_resolution(struct iio_dev *indio_dev) > > +static int stm32_adc_fw_get_resolution(struct iio_dev *indio_dev) > > { > > - struct device_node *node = indio_dev->dev.of_node; > > + struct device *dev = &indio_dev->dev; > > struct stm32_adc *adc = iio_priv(indio_dev); > > unsigned int i; > > u32 res; > > > > - if (of_property_read_u32(node, "assigned-resolution-bits", > > &res)) > > + if (device_property_read_u32(dev, "assigned-resolution- > > bits", &res)) > > res = adc->cfg->adc_info->resolutions[0]; > > > > for (i = 0; i < adc->cfg->adc_info->num_res; i++) > > @@ -1873,11 +1873,11 @@ static void stm32_adc_chan_init_one(struct > > iio_dev *indio_dev, > > > > static int stm32_adc_get_legacy_chan_count(struct iio_dev > > *indio_dev, struct stm32_adc *adc) > > { > > - struct device_node *node = indio_dev->dev.of_node; > > + struct device *dev = &indio_dev->dev; > > const struct stm32_adc_info *adc_info = adc->cfg->adc_info; > > int num_channels = 0, ret; > > > > - ret = of_property_count_u32_elems(node, "st,adc-channels"); > > + ret = device_property_count_u32(dev, "st,adc-channels"); > > if (ret > adc_info->max_channels) { > > dev_err(&indio_dev->dev, "Bad st,adc-channels?\n"); > > return -EINVAL; > > @@ -1885,8 +1885,8 @@ static int > > stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct > > stm > > num_channels += ret; > > } > > > > - ret = of_property_count_elems_of_size(node, "st,adc-diff- > > channels", > > - sizeof(struct > > stm32_adc_diff_channel)); > > + /* each st,adc-diff-channels is a group of 2 u32 */ > > + ret = device_property_count_u64(dev, "st,adc-diff- > > channels"); Are you fine with this change (still does not have any reference to the target struct but the comment might be helpful and there's no magic 2)? > > if (ret > adc_info->max_channels) { > > dev_err(&indio_dev->dev, "Bad st,adc-diff- > > channels?\n"); > > return -EINVAL; > > @@ -1896,7 +1896,7 @@ static int > > stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct > > stm > > } > > > > /* Optional sample time is provided either for each, or all > > channels */ > > - ret = of_property_count_u32_elems(node, "st,min-sample- > > time-nsecs"); > > + ret = device_property_count_u32(dev, "st,min-sample-time- > > nsecs"); > > if (ret > 1 && ret != num_channels) { > > dev_err(&indio_dev->dev, "Invalid st,min-sample- > > time-nsecs\n"); > > return -EINVAL; > > @@ -1907,21 +1907,20 @@ static int > > stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct > > stm > > > > static int stm32_adc_legacy_chan_init(struct iio_dev *indio_dev, > > struct stm32_adc *adc, > > - struct iio_chan_spec > > *channels) > > + struct iio_chan_spec > > *channels, > > + int nchans) > > { > > - struct device_node *node = indio_dev->dev.of_node; > > const struct stm32_adc_info *adc_info = adc->cfg->adc_info; > > struct stm32_adc_diff_channel diff[STM32_ADC_CH_MAX]; > > + struct device *dev = &indio_dev->dev; > > u32 num_diff = adc->num_diff; > > int size = num_diff * sizeof(*diff) / sizeof(u32); > > - int scan_index = 0, val, ret, i; > > - struct property *prop; > > - const __be32 *cur; > > - u32 smp = 0; > > + int scan_index = 0, ret, i; > > + u32 smp = 0, nsmps, smps[STM32_ADC_CH_MAX], > > chans[STM32_ADC_CH_MAX]; > > > > if (num_diff) { > > - ret = of_property_read_u32_array(node, "st,adc- > > diff-channels", > > - (u32 *)diff, > > size); > > + ret = device_property_read_u32_array(dev, "st,adc- > > diff-channels", > > + (u32 *)diff, > > size); > > if (ret) { > > dev_err(&indio_dev->dev, "Failed to get > > diff channels %d\n", ret); > > return ret; > > @@ -1942,32 +1941,51 @@ static int > > stm32_adc_legacy_chan_init(struct iio_dev *indio_dev, > > } > > } > > > > - of_property_for_each_u32(node, "st,adc-channels", prop, > > cur, val) { > > - if (val >= adc_info->max_channels) { > > - dev_err(&indio_dev->dev, "Invalid channel > > %d\n", val); > > + ret = device_property_read_u32_array(dev, "st,adc- > > channels", chans, > > + nchans); > > + if (ret) > > + return ret; > > + > > + for (i = 0; i < nchans; i++) { > > Hi Nuno, > > There's an endless loop issue here due to the two for loops are using > the same index 'i'. (Please look at my earlier comments in v1) > Ouch... Regarding your v1 comments (that for some reason I missed), I do not think there's any helper like "of_property_for_each_u32()". For now we have to live with this... > for (i = 0; i < nchans; i++) { > ... > // Here i gets cleared, num_diff may vary (example with 0 > here) > for (i = 0; i < num_diff; i++) { > ... > } > > stm32_adc_chan_init_one(.. &channels[scan_index].. > scan_index..) > > scan_index++; // Still, scan index gets incremented > indefinitly > } > > There's an out of boudary situation with scan_index above, wich ends- > up > in crashing in stm32_adc_chan_init_one(). > > > + if (chans[i] >= adc_info->max_channels) { > > + dev_err(&indio_dev->dev, "Invalid channel > > %d\n", > > + chans[i]); > > return -EINVAL; > > } > > > > /* Channel can't be configured both as single-ended > > & diff */ > > for (i = 0; i < num_diff; i++) { > > - if (val == diff[i].vinp) { > > - dev_err(&indio_dev->dev, "channel > > %d misconfigured\n", val); > > + if (chans[i] == diff[i].vinp) { > > + dev_err(&indio_dev->dev, "channel > > %d misconfigured\n", chans[i]); > > return -EINVAL; > > } > > } > > - stm32_adc_chan_init_one(indio_dev, > > &channels[scan_index], val, > > - 0, scan_index, false); > > + stm32_adc_chan_init_one(indio_dev, > > &channels[scan_index], > > + chans[i], 0, scan_index, > > false); > > scan_index++; > > } > > > > + nsmps = device_property_count_u32(dev, "st,min-sample-time- > > nsecs"); > > + if (nsmps) { > > + if (nsmps >= nchans) > > nit: if (nsmps > nchans) > > > + nsmps = nchans; > > + > > There's a bit of redundancy in checking nsmps, > "st,min-sample-time-nsecs" is already sanitized in > stm32_adc_get_legacy_chan_count(): > > /* Optional sample time is provided either for each, or all channels > */ > ret = device_property_count_u32(dev, "st,min-sample-time-nsecs"); > if (ret > 1 && ret != num_channels) { > dev_err(... > > So just sharing my thoughts here: > - Maybe this could be dropped ? > (Thinking loudly) The earliest this gets sanitized, the less un- > needed > initialisations happen before failing? > - Or the earlier check could be moved here ? > > I've no strong opinion. > Ahh, didn't noticed the first call to 'device_property_count_u32(dev, "st,min-sample-time-nsecs")'. So I do agree with you that we should bail as soon as possible but I'm also not a big fan of calling device_property_count_u32() for the same purpose. So I would suggest: 1) Your option 2. 2) Option 1 but we would store 'nsmps' in the first call (making it a member of struct stm32_adc) and then use it here accordingly. Thought? > Best Regards, > Fabrice > > > + ret = device_property_read_u32_array(dev, "st,min- > > sample-time-nsecs", > > + smps, nsmps); > > + if (ret) > > + return ret; > > + } > > + > > for (i = 0; i < scan_index; i++) { > > /* > > - * Using of_property_read_u32_index(), smp value > > will only be > > - * modified if valid u32 value can be decoded. This > > allows to > > - * get either no value, 1 shared value for all > > indexes, or one > > - * value per channel. > > + * This check is used with the above logic so that > > smp value > > + * will only be modified if valid u32 value can be > > decoded. This > > + * allows to get either no value, 1 shared value > > for all indexes, > > + * or one value per channel. The point is to have > > the same > > + * behavior as 'of_property_read_u32_index()'. > > */ > > - of_property_read_u32_index(node, "st,min-sample- > > time-nsecs", i, &smp); > > + if (i < nsmps) > > + smp = smps[i]; > > Regarding your comment on v1, I would prefer to keep this as-is. I think it's enough together with the comment. Can change it though if you fell too strong about it :) Thanks! - Nuno Sá
On 7/12/22 12:33, Nuno Sá wrote: > Hi Fabrice, > > Nice that someone in ST is looking at this one :) Hi Nuno, Thank you for taking care of converting all these drivers to device properties, including this one :-). > > On Mon, 2022-07-11 at 16:04 +0200, Fabrice Gasnier wrote: >> On 7/11/22 14:38, Nuno Sá wrote: >>> Make the conversion to firmware agnostic device properties. As part >>> of >>> the conversion the IIO inkern interface 'of_xlate()' is also >>> converted to >>> 'fwnode_xlate()'. The goal is to completely drop 'of_xlate' and >>> hence OF >>> dependencies from IIO. >>> >>> Signed-off-by: Nuno Sá <nuno.sa@analog.com> >>> --- >>> drivers/iio/adc/stm32-adc.c | 121 ++++++++++++++++++++------------ >>> ---- >>> 1 file changed, 67 insertions(+), 54 deletions(-) >>> >>> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32- >>> adc.c >>> index 3985fe972892..e55859113df8 100644 >>> --- a/drivers/iio/adc/stm32-adc.c >>> +++ b/drivers/iio/adc/stm32-adc.c >>> @@ -21,11 +21,11 @@ >>> #include <linux/io.h> >>> #include <linux/iopoll.h> >>> #include <linux/module.h> >>> +#include <linux/mod_devicetable.h> >>> #include <linux/nvmem-consumer.h> >>> #include <linux/platform_device.h> >>> #include <linux/pm_runtime.h> >>> -#include <linux/of.h> >>> -#include <linux/of_device.h> >>> +#include <linux/property.h> >>> >>> #include "stm32-adc-core.h" >>> >>> @@ -1530,8 +1530,8 @@ static int stm32_adc_update_scan_mode(struct >>> iio_dev *indio_dev, >>> return ret; >>> } >>> >>> -static int stm32_adc_of_xlate(struct iio_dev *indio_dev, >>> - const struct of_phandle_args >>> *iiospec) >>> +static int stm32_adc_fwnode_xlate(struct iio_dev *indio_dev, >>> + const struct >>> fwnode_reference_args *iiospec) >>> { >>> int i; >>> >>> @@ -1585,7 +1585,7 @@ static const struct iio_info >>> stm32_adc_iio_info = { >>> .hwfifo_set_watermark = stm32_adc_set_watermark, >>> .update_scan_mode = stm32_adc_update_scan_mode, >>> .debugfs_reg_access = stm32_adc_debugfs_reg_access, >>> - .of_xlate = stm32_adc_of_xlate, >>> + .fwnode_xlate = stm32_adc_fwnode_xlate, >>> }; >>> >>> static unsigned int stm32_adc_dma_residue(struct stm32_adc *adc) >>> @@ -1782,14 +1782,14 @@ static const struct iio_chan_spec_ext_info >>> stm32_adc_ext_info[] = { >>> {}, >>> }; >>> >>> -static int stm32_adc_of_get_resolution(struct iio_dev *indio_dev) >>> +static int stm32_adc_fw_get_resolution(struct iio_dev *indio_dev) >>> { >>> - struct device_node *node = indio_dev->dev.of_node; >>> + struct device *dev = &indio_dev->dev; >>> struct stm32_adc *adc = iio_priv(indio_dev); >>> unsigned int i; >>> u32 res; >>> >>> - if (of_property_read_u32(node, "assigned-resolution-bits", >>> &res)) >>> + if (device_property_read_u32(dev, "assigned-resolution- >>> bits", &res)) >>> res = adc->cfg->adc_info->resolutions[0]; >>> >>> for (i = 0; i < adc->cfg->adc_info->num_res; i++) >>> @@ -1873,11 +1873,11 @@ static void stm32_adc_chan_init_one(struct >>> iio_dev *indio_dev, >>> >>> static int stm32_adc_get_legacy_chan_count(struct iio_dev >>> *indio_dev, struct stm32_adc *adc) >>> { >>> - struct device_node *node = indio_dev->dev.of_node; >>> + struct device *dev = &indio_dev->dev; >>> const struct stm32_adc_info *adc_info = adc->cfg->adc_info; >>> int num_channels = 0, ret; >>> >>> - ret = of_property_count_u32_elems(node, "st,adc-channels"); >>> + ret = device_property_count_u32(dev, "st,adc-channels"); >>> if (ret > adc_info->max_channels) { >>> dev_err(&indio_dev->dev, "Bad st,adc-channels?\n"); >>> return -EINVAL; >>> @@ -1885,8 +1885,8 @@ static int >>> stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct >>> stm >>> num_channels += ret; >>> } >>> >>> - ret = of_property_count_elems_of_size(node, "st,adc-diff- >>> channels", >>> - sizeof(struct >>> stm32_adc_diff_channel)); >>> + /* each st,adc-diff-channels is a group of 2 u32 */ >>> + ret = device_property_count_u64(dev, "st,adc-diff- >>> channels"); > > Are you fine with this change (still does not have any reference to the > target struct but the comment might be helpful and there's no magic 2)? Since you added that comment, this sounds better. IMHO, This still looks a bit weird. I'd feel more comfortable by using u32 API for a 'uint32-matrix' as defined in dt-bindings. Strictly speaking, something like sizeof(struct stm32_adc_diff_channel) / sizeof(u32) could be used, along with this comment and device_property_count_u32() to make it clear ? Maybe Jonathan has an opinion, as he first raised a comment here ? > >>> if (ret > adc_info->max_channels) { >>> dev_err(&indio_dev->dev, "Bad st,adc-diff- >>> channels?\n"); >>> return -EINVAL; >>> @@ -1896,7 +1896,7 @@ static int >>> stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct >>> stm >>> } >>> >>> /* Optional sample time is provided either for each, or all >>> channels */ >>> - ret = of_property_count_u32_elems(node, "st,min-sample- >>> time-nsecs"); >>> + ret = device_property_count_u32(dev, "st,min-sample-time- >>> nsecs"); >>> if (ret > 1 && ret != num_channels) { >>> dev_err(&indio_dev->dev, "Invalid st,min-sample- >>> time-nsecs\n"); >>> return -EINVAL; >>> @@ -1907,21 +1907,20 @@ static int >>> stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct >>> stm >>> >>> static int stm32_adc_legacy_chan_init(struct iio_dev *indio_dev, >>> struct stm32_adc *adc, >>> - struct iio_chan_spec >>> *channels) >>> + struct iio_chan_spec >>> *channels, >>> + int nchans) >>> { >>> - struct device_node *node = indio_dev->dev.of_node; >>> const struct stm32_adc_info *adc_info = adc->cfg->adc_info; >>> struct stm32_adc_diff_channel diff[STM32_ADC_CH_MAX]; >>> + struct device *dev = &indio_dev->dev; >>> u32 num_diff = adc->num_diff; >>> int size = num_diff * sizeof(*diff) / sizeof(u32); >>> - int scan_index = 0, val, ret, i; >>> - struct property *prop; >>> - const __be32 *cur; >>> - u32 smp = 0; >>> + int scan_index = 0, ret, i; >>> + u32 smp = 0, nsmps, smps[STM32_ADC_CH_MAX], >>> chans[STM32_ADC_CH_MAX]; >>> >>> if (num_diff) { >>> - ret = of_property_read_u32_array(node, "st,adc- >>> diff-channels", >>> - (u32 *)diff, >>> size); >>> + ret = device_property_read_u32_array(dev, "st,adc- >>> diff-channels", >>> + (u32 *)diff, >>> size); >>> if (ret) { >>> dev_err(&indio_dev->dev, "Failed to get >>> diff channels %d\n", ret); >>> return ret; >>> @@ -1942,32 +1941,51 @@ static int >>> stm32_adc_legacy_chan_init(struct iio_dev *indio_dev, >>> } >>> } >>> >>> - of_property_for_each_u32(node, "st,adc-channels", prop, >>> cur, val) { >>> - if (val >= adc_info->max_channels) { >>> - dev_err(&indio_dev->dev, "Invalid channel >>> %d\n", val); >>> + ret = device_property_read_u32_array(dev, "st,adc- >>> channels", chans, >>> + nchans); >>> + if (ret) >>> + return ret; >>> + >>> + for (i = 0; i < nchans; i++) { >> >> Hi Nuno, >> >> There's an endless loop issue here due to the two for loops are using >> the same index 'i'. (Please look at my earlier comments in v1) >> > > Ouch... > > Regarding your v1 comments (that for some reason I missed), I do not > think there's any helper like "of_property_for_each_u32()". For now we > have to live with this... Indeed, still need to fix this for loop. > >> for (i = 0; i < nchans; i++) { >> ... >> // Here i gets cleared, num_diff may vary (example with 0 >> here) >> for (i = 0; i < num_diff; i++) { >> ... >> } >> >> stm32_adc_chan_init_one(.. &channels[scan_index].. >> scan_index..) >> >> scan_index++; // Still, scan index gets incremented >> indefinitly >> } >> >> There's an out of boudary situation with scan_index above, wich ends- >> up >> in crashing in stm32_adc_chan_init_one(). >> >>> + if (chans[i] >= adc_info->max_channels) { >>> + dev_err(&indio_dev->dev, "Invalid channel >>> %d\n", >>> + chans[i]); >>> return -EINVAL; >>> } >>> >>> /* Channel can't be configured both as single-ended >>> & diff */ >>> for (i = 0; i < num_diff; i++) { >>> - if (val == diff[i].vinp) { >>> - dev_err(&indio_dev->dev, "channel >>> %d misconfigured\n", val); >>> + if (chans[i] == diff[i].vinp) { >>> + dev_err(&indio_dev->dev, "channel >>> %d misconfigured\n", chans[i]); >>> return -EINVAL; >>> } >>> } >>> - stm32_adc_chan_init_one(indio_dev, >>> &channels[scan_index], val, >>> - 0, scan_index, false); >>> + stm32_adc_chan_init_one(indio_dev, >>> &channels[scan_index], >>> + chans[i], 0, scan_index, >>> false); >>> scan_index++; >>> } >>> >>> + nsmps = device_property_count_u32(dev, "st,min-sample-time- >>> nsecs"); >>> + if (nsmps) { >>> + if (nsmps >= nchans) >> >> nit: if (nsmps > nchans) >> >>> + nsmps = nchans; >>> + >> >> There's a bit of redundancy in checking nsmps, >> "st,min-sample-time-nsecs" is already sanitized in >> stm32_adc_get_legacy_chan_count(): >> >> /* Optional sample time is provided either for each, or all channels >> */ >> ret = device_property_count_u32(dev, "st,min-sample-time-nsecs"); >> if (ret > 1 && ret != num_channels) { >> dev_err(... >> >> So just sharing my thoughts here: >> - Maybe this could be dropped ? >> (Thinking loudly) The earliest this gets sanitized, the less un- >> needed >> initialisations happen before failing? >> - Or the earlier check could be moved here ? >> >> I've no strong opinion. >> > > Ahh, didn't noticed the first call to 'device_property_count_u32(dev, > "st,min-sample-time-nsecs")'. So I do agree with you that we should > bail as soon as possible but I'm also not a big fan of calling > device_property_count_u32() for the same purpose. So I would suggest: > > 1) Your option 2. > 2) Option 1 but we would store 'nsmps' in the first call (making it a > member of struct stm32_adc) and then use it here accordingly. > > Thought? I'd prefer Your option 2) to follow the logic of the current code (e.g. get count then init chans). It has the advantage to tackle errors as soon as possible, and store 'nsmps' at once, when sanitized. Its avoids redundancy here, only cost is an extra variable indeed. Best Regards, Fabrice > >> Best Regards, >> Fabrice >> >>> + ret = device_property_read_u32_array(dev, "st,min- >>> sample-time-nsecs", >>> + smps, nsmps); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> for (i = 0; i < scan_index; i++) { >>> /* >>> - * Using of_property_read_u32_index(), smp value >>> will only be >>> - * modified if valid u32 value can be decoded. This >>> allows to >>> - * get either no value, 1 shared value for all >>> indexes, or one >>> - * value per channel. >>> + * This check is used with the above logic so that >>> smp value >>> + * will only be modified if valid u32 value can be >>> decoded. This >>> + * allows to get either no value, 1 shared value >>> for all indexes, >>> + * or one value per channel. The point is to have >>> the same >>> + * behavior as 'of_property_read_u32_index()'. >>> */ >>> - of_property_read_u32_index(node, "st,min-sample- >>> time-nsecs", i, &smp); >>> + if (i < nsmps) >>> + smp = smps[i]; >>> > > Regarding your comment on v1, I would prefer to keep this as-is. I > think it's enough together with the comment. Can change it though if > you fell too strong about it :) > > Thanks! > - Nuno Sá >
On Wed, 2022-07-13 at 17:48 +0200, Fabrice Gasnier wrote: > On 7/12/22 12:33, Nuno Sá wrote: > > Hi Fabrice, > > > > Nice that someone in ST is looking at this one :) > > Hi Nuno, > > Thank you for taking care of converting all these drivers to device > properties, including this one :-). > > > > > On Mon, 2022-07-11 at 16:04 +0200, Fabrice Gasnier wrote: > > > On 7/11/22 14:38, Nuno Sá wrote: > > > > Make the conversion to firmware agnostic device properties. As > > > > part > > > > of > > > > the conversion the IIO inkern interface 'of_xlate()' is also > > > > converted to > > > > 'fwnode_xlate()'. The goal is to completely drop 'of_xlate' and > > > > hence OF > > > > dependencies from IIO. > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > --- > > > > drivers/iio/adc/stm32-adc.c | 121 ++++++++++++++++++++-------- > > > > ---- > > > > ---- > > > > 1 file changed, 67 insertions(+), 54 deletions(-) > > > > > > > > diff --git a/drivers/iio/adc/stm32-adc.c > > > > b/drivers/iio/adc/stm32- > > > > adc.c > > > > index 3985fe972892..e55859113df8 100644 > > > > --- a/drivers/iio/adc/stm32-adc.c > > > > +++ b/drivers/iio/adc/stm32-adc.c > > > > @@ -21,11 +21,11 @@ > > > > #include <linux/io.h> > > > > #include <linux/iopoll.h> > > > > #include <linux/module.h> > > > > +#include <linux/mod_devicetable.h> > > > > #include <linux/nvmem-consumer.h> > > > > #include <linux/platform_device.h> > > > > #include <linux/pm_runtime.h> > > > > -#include <linux/of.h> > > > > -#include <linux/of_device.h> > > > > +#include <linux/property.h> > > > > > > > > #include "stm32-adc-core.h" > > > > > > > > @@ -1530,8 +1530,8 @@ static int > > > > stm32_adc_update_scan_mode(struct > > > > iio_dev *indio_dev, > > > > return ret; > > > > } > > > > > > > > -static int stm32_adc_of_xlate(struct iio_dev *indio_dev, > > > > - const struct of_phandle_args > > > > *iiospec) > > > > +static int stm32_adc_fwnode_xlate(struct iio_dev *indio_dev, > > > > + const struct > > > > fwnode_reference_args *iiospec) > > > > { > > > > int i; > > > > > > > > @@ -1585,7 +1585,7 @@ static const struct iio_info > > > > stm32_adc_iio_info = { > > > > .hwfifo_set_watermark = stm32_adc_set_watermark, > > > > .update_scan_mode = stm32_adc_update_scan_mode, > > > > .debugfs_reg_access = stm32_adc_debugfs_reg_access, > > > > - .of_xlate = stm32_adc_of_xlate, > > > > + .fwnode_xlate = stm32_adc_fwnode_xlate, > > > > }; > > > > > > > > static unsigned int stm32_adc_dma_residue(struct stm32_adc > > > > *adc) > > > > @@ -1782,14 +1782,14 @@ static const struct > > > > iio_chan_spec_ext_info > > > > stm32_adc_ext_info[] = { > > > > {}, > > > > }; > > > > > > > > -static int stm32_adc_of_get_resolution(struct iio_dev > > > > *indio_dev) > > > > +static int stm32_adc_fw_get_resolution(struct iio_dev > > > > *indio_dev) > > > > { > > > > - struct device_node *node = indio_dev->dev.of_node; > > > > + struct device *dev = &indio_dev->dev; > > > > struct stm32_adc *adc = iio_priv(indio_dev); > > > > unsigned int i; > > > > u32 res; > > > > > > > > - if (of_property_read_u32(node, "assigned-resolution- > > > > bits", > > > > &res)) > > > > + if (device_property_read_u32(dev, "assigned-resolution- > > > > bits", &res)) > > > > res = adc->cfg->adc_info->resolutions[0]; > > > > > > > > for (i = 0; i < adc->cfg->adc_info->num_res; i++) > > > > @@ -1873,11 +1873,11 @@ static void > > > > stm32_adc_chan_init_one(struct > > > > iio_dev *indio_dev, > > > > > > > > static int stm32_adc_get_legacy_chan_count(struct iio_dev > > > > *indio_dev, struct stm32_adc *adc) > > > > { > > > > - struct device_node *node = indio_dev->dev.of_node; > > > > + struct device *dev = &indio_dev->dev; > > > > const struct stm32_adc_info *adc_info = adc->cfg- > > > > >adc_info; > > > > int num_channels = 0, ret; > > > > > > > > - ret = of_property_count_u32_elems(node, "st,adc- > > > > channels"); > > > > + ret = device_property_count_u32(dev, "st,adc- > > > > channels"); > > > > if (ret > adc_info->max_channels) { > > > > dev_err(&indio_dev->dev, "Bad st,adc- > > > > channels?\n"); > > > > return -EINVAL; > > > > @@ -1885,8 +1885,8 @@ static int > > > > stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, > > > > struct > > > > stm > > > > num_channels += ret; > > > > } > > > > > > > > - ret = of_property_count_elems_of_size(node, "st,adc- > > > > diff- > > > > channels", > > > > - sizeof(struct > > > > stm32_adc_diff_channel)); > > > > + /* each st,adc-diff-channels is a group of 2 u32 */ > > > > + ret = device_property_count_u64(dev, "st,adc-diff- > > > > channels"); > > > > Are you fine with this change (still does not have any reference to > > the > > target struct but the comment might be helpful and there's no magic > > 2)? > > > Since you added that comment, this sounds better. IMHO, This still > looks > a bit weird. I'd feel more comfortable by using u32 API for a > 'uint32-matrix' as defined in dt-bindings. > Strictly speaking, something like > sizeof(struct stm32_adc_diff_channel) / sizeof(u32) could be used, > along > with this comment and device_property_count_u32() to make it clear ? > No strong option so I can do as you prefer: ret = device_property_count_u32(); if (ret / (sizeof(struct stm32_adc_diff_channel) / sizeof(u32)) > max_chan) { ... } ... Probably it's a good idea to store that sizeof() division in a local variable :) - Nuno Sá > > > >
diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c index 3985fe972892..e55859113df8 100644 --- a/drivers/iio/adc/stm32-adc.c +++ b/drivers/iio/adc/stm32-adc.c @@ -21,11 +21,11 @@ #include <linux/io.h> #include <linux/iopoll.h> #include <linux/module.h> +#include <linux/mod_devicetable.h> #include <linux/nvmem-consumer.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> -#include <linux/of.h> -#include <linux/of_device.h> +#include <linux/property.h> #include "stm32-adc-core.h" @@ -1530,8 +1530,8 @@ static int stm32_adc_update_scan_mode(struct iio_dev *indio_dev, return ret; } -static int stm32_adc_of_xlate(struct iio_dev *indio_dev, - const struct of_phandle_args *iiospec) +static int stm32_adc_fwnode_xlate(struct iio_dev *indio_dev, + const struct fwnode_reference_args *iiospec) { int i; @@ -1585,7 +1585,7 @@ static const struct iio_info stm32_adc_iio_info = { .hwfifo_set_watermark = stm32_adc_set_watermark, .update_scan_mode = stm32_adc_update_scan_mode, .debugfs_reg_access = stm32_adc_debugfs_reg_access, - .of_xlate = stm32_adc_of_xlate, + .fwnode_xlate = stm32_adc_fwnode_xlate, }; static unsigned int stm32_adc_dma_residue(struct stm32_adc *adc) @@ -1782,14 +1782,14 @@ static const struct iio_chan_spec_ext_info stm32_adc_ext_info[] = { {}, }; -static int stm32_adc_of_get_resolution(struct iio_dev *indio_dev) +static int stm32_adc_fw_get_resolution(struct iio_dev *indio_dev) { - struct device_node *node = indio_dev->dev.of_node; + struct device *dev = &indio_dev->dev; struct stm32_adc *adc = iio_priv(indio_dev); unsigned int i; u32 res; - if (of_property_read_u32(node, "assigned-resolution-bits", &res)) + if (device_property_read_u32(dev, "assigned-resolution-bits", &res)) res = adc->cfg->adc_info->resolutions[0]; for (i = 0; i < adc->cfg->adc_info->num_res; i++) @@ -1873,11 +1873,11 @@ static void stm32_adc_chan_init_one(struct iio_dev *indio_dev, static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm32_adc *adc) { - struct device_node *node = indio_dev->dev.of_node; + struct device *dev = &indio_dev->dev; const struct stm32_adc_info *adc_info = adc->cfg->adc_info; int num_channels = 0, ret; - ret = of_property_count_u32_elems(node, "st,adc-channels"); + ret = device_property_count_u32(dev, "st,adc-channels"); if (ret > adc_info->max_channels) { dev_err(&indio_dev->dev, "Bad st,adc-channels?\n"); return -EINVAL; @@ -1885,8 +1885,8 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm num_channels += ret; } - ret = of_property_count_elems_of_size(node, "st,adc-diff-channels", - sizeof(struct stm32_adc_diff_channel)); + /* each st,adc-diff-channels is a group of 2 u32 */ + ret = device_property_count_u64(dev, "st,adc-diff-channels"); if (ret > adc_info->max_channels) { dev_err(&indio_dev->dev, "Bad st,adc-diff-channels?\n"); return -EINVAL; @@ -1896,7 +1896,7 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm } /* Optional sample time is provided either for each, or all channels */ - ret = of_property_count_u32_elems(node, "st,min-sample-time-nsecs"); + ret = device_property_count_u32(dev, "st,min-sample-time-nsecs"); if (ret > 1 && ret != num_channels) { dev_err(&indio_dev->dev, "Invalid st,min-sample-time-nsecs\n"); return -EINVAL; @@ -1907,21 +1907,20 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm static int stm32_adc_legacy_chan_init(struct iio_dev *indio_dev, struct stm32_adc *adc, - struct iio_chan_spec *channels) + struct iio_chan_spec *channels, + int nchans) { - struct device_node *node = indio_dev->dev.of_node; const struct stm32_adc_info *adc_info = adc->cfg->adc_info; struct stm32_adc_diff_channel diff[STM32_ADC_CH_MAX]; + struct device *dev = &indio_dev->dev; u32 num_diff = adc->num_diff; int size = num_diff * sizeof(*diff) / sizeof(u32); - int scan_index = 0, val, ret, i; - struct property *prop; - const __be32 *cur; - u32 smp = 0; + int scan_index = 0, ret, i; + u32 smp = 0, nsmps, smps[STM32_ADC_CH_MAX], chans[STM32_ADC_CH_MAX]; if (num_diff) { - ret = of_property_read_u32_array(node, "st,adc-diff-channels", - (u32 *)diff, size); + ret = device_property_read_u32_array(dev, "st,adc-diff-channels", + (u32 *)diff, size); if (ret) { dev_err(&indio_dev->dev, "Failed to get diff channels %d\n", ret); return ret; @@ -1942,32 +1941,51 @@ static int stm32_adc_legacy_chan_init(struct iio_dev *indio_dev, } } - of_property_for_each_u32(node, "st,adc-channels", prop, cur, val) { - if (val >= adc_info->max_channels) { - dev_err(&indio_dev->dev, "Invalid channel %d\n", val); + ret = device_property_read_u32_array(dev, "st,adc-channels", chans, + nchans); + if (ret) + return ret; + + for (i = 0; i < nchans; i++) { + if (chans[i] >= adc_info->max_channels) { + dev_err(&indio_dev->dev, "Invalid channel %d\n", + chans[i]); return -EINVAL; } /* Channel can't be configured both as single-ended & diff */ for (i = 0; i < num_diff; i++) { - if (val == diff[i].vinp) { - dev_err(&indio_dev->dev, "channel %d misconfigured\n", val); + if (chans[i] == diff[i].vinp) { + dev_err(&indio_dev->dev, "channel %d misconfigured\n", chans[i]); return -EINVAL; } } - stm32_adc_chan_init_one(indio_dev, &channels[scan_index], val, - 0, scan_index, false); + stm32_adc_chan_init_one(indio_dev, &channels[scan_index], + chans[i], 0, scan_index, false); scan_index++; } + nsmps = device_property_count_u32(dev, "st,min-sample-time-nsecs"); + if (nsmps) { + if (nsmps >= nchans) + nsmps = nchans; + + ret = device_property_read_u32_array(dev, "st,min-sample-time-nsecs", + smps, nsmps); + if (ret) + return ret; + } + for (i = 0; i < scan_index; i++) { /* - * Using of_property_read_u32_index(), smp value will only be - * modified if valid u32 value can be decoded. This allows to - * get either no value, 1 shared value for all indexes, or one - * value per channel. + * This check is used with the above logic so that smp value + * will only be modified if valid u32 value can be decoded. This + * allows to get either no value, 1 shared value for all indexes, + * or one value per channel. The point is to have the same + * behavior as 'of_property_read_u32_index()'. */ - of_property_read_u32_index(node, "st,min-sample-time-nsecs", i, &smp); + if (i < nsmps) + smp = smps[i]; /* Prepare sampling time settings */ stm32_adc_smpr_init(adc, channels[i].channel, smp); @@ -2010,22 +2028,21 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev, struct stm32_adc *adc, struct iio_chan_spec *channels) { - struct device_node *node = indio_dev->dev.of_node; const struct stm32_adc_info *adc_info = adc->cfg->adc_info; - struct device_node *child; + struct fwnode_handle *child; const char *name; int val, scan_index = 0, ret; bool differential; u32 vin[2]; - for_each_available_child_of_node(node, child) { - ret = of_property_read_u32(child, "reg", &val); + device_for_each_child_node(&indio_dev->dev, child) { + ret = fwnode_property_read_u32(child, "reg", &val); if (ret) { dev_err(&indio_dev->dev, "Missing channel index %d\n", ret); goto err; } - ret = of_property_read_string(child, "label", &name); + ret = fwnode_property_read_string(child, "label", &name); /* label is optional */ if (!ret) { if (strlen(name) >= STM32_ADC_CH_SZ) { @@ -2050,7 +2067,7 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev, } differential = false; - ret = of_property_read_u32_array(child, "diff-channels", vin, 2); + ret = fwnode_property_read_u32_array(child, "diff-channels", vin, 2); /* diff-channels is optional */ if (!ret) { differential = true; @@ -2067,7 +2084,7 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev, stm32_adc_chan_init_one(indio_dev, &channels[scan_index], val, vin[1], scan_index, differential); - ret = of_property_read_u32(child, "st,min-sample-time-ns", &val); + ret = fwnode_property_read_u32(child, "st,min-sample-time-ns", &val); /* st,min-sample-time-ns is optional */ if (!ret) { stm32_adc_smpr_init(adc, channels[scan_index].channel, val); @@ -2085,14 +2102,13 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev, return scan_index; err: - of_node_put(child); + fwnode_handle_put(child); return ret; } -static int stm32_adc_chan_of_init(struct iio_dev *indio_dev, bool timestamping) +static int stm32_adc_chan_fw_init(struct iio_dev *indio_dev, bool timestamping) { - struct device_node *node = indio_dev->dev.of_node; struct stm32_adc *adc = iio_priv(indio_dev); const struct stm32_adc_info *adc_info = adc->cfg->adc_info; struct iio_chan_spec *channels; @@ -2102,7 +2118,7 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev, bool timestamping) for (i = 0; i < STM32_ADC_INT_CH_NB; i++) adc->int_ch[i] = STM32_ADC_INT_CH_NONE; - num_channels = of_get_available_child_count(node); + num_channels = device_get_child_node_count(&indio_dev->dev); /* If no channels have been found, fallback to channels legacy properties. */ if (!num_channels) { legacy = true; @@ -2133,7 +2149,8 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev, bool timestamping) return -ENOMEM; if (legacy) - ret = stm32_adc_legacy_chan_init(indio_dev, adc, channels); + ret = stm32_adc_legacy_chan_init(indio_dev, adc, channels, + num_channels); else ret = stm32_adc_generic_chan_init(indio_dev, adc, channels); if (ret < 0) @@ -2215,9 +2232,6 @@ static int stm32_adc_probe(struct platform_device *pdev) bool timestamping = false; int ret; - if (!pdev->dev.of_node) - return -ENODEV; - indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc)); if (!indio_dev) return -ENOMEM; @@ -2226,17 +2240,16 @@ static int stm32_adc_probe(struct platform_device *pdev) adc->common = dev_get_drvdata(pdev->dev.parent); spin_lock_init(&adc->lock); init_completion(&adc->completion); - adc->cfg = (const struct stm32_adc_cfg *) - of_match_device(dev->driver->of_match_table, dev)->data; + adc->cfg = device_get_match_data(dev); indio_dev->name = dev_name(&pdev->dev); - indio_dev->dev.of_node = pdev->dev.of_node; + device_set_node(&indio_dev->dev, dev_fwnode(&pdev->dev)); indio_dev->info = &stm32_adc_iio_info; indio_dev->modes = INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED; platform_set_drvdata(pdev, indio_dev); - ret = of_property_read_u32(pdev->dev.of_node, "reg", &adc->offset); + ret = device_property_read_u32(dev, "reg", &adc->offset); if (ret != 0) { dev_err(&pdev->dev, "missing reg property\n"); return -EINVAL; @@ -2265,7 +2278,7 @@ static int stm32_adc_probe(struct platform_device *pdev) } } - ret = stm32_adc_of_get_resolution(indio_dev); + ret = stm32_adc_fw_get_resolution(indio_dev); if (ret < 0) return ret; @@ -2282,7 +2295,7 @@ static int stm32_adc_probe(struct platform_device *pdev) timestamping = true; } - ret = stm32_adc_chan_of_init(indio_dev, timestamping); + ret = stm32_adc_chan_fw_init(indio_dev, timestamping); if (ret < 0) goto err_dma_disable;
Make the conversion to firmware agnostic device properties. As part of the conversion the IIO inkern interface 'of_xlate()' is also converted to 'fwnode_xlate()'. The goal is to completely drop 'of_xlate' and hence OF dependencies from IIO. Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- drivers/iio/adc/stm32-adc.c | 121 ++++++++++++++++++++---------------- 1 file changed, 67 insertions(+), 54 deletions(-)