| Message ID | 20220610084545.547700-33-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | make iio inkern interface firmware agnostic | expand |
On Fri, 10 Jun 2022 10:45:43 +0200 Nuno Sá <nuno.sa@analog.com> 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> One trivial comment inline. In general this particular refactor is complex, so I either want eyes of those familiar with the driver and / or testing or we may want to reduce the scope to just the fwnode_xlate() and leave the more complex conversions for when we can get that testing done. Let's see if we get the necessary review on this more complex patch. I just don't want us to get stalled by trying to do too much at once! Jonathan > --- > drivers/iio/adc/stm32-adc.c | 128 ++++++++++++++++++++---------------- > 1 file changed, 70 insertions(+), 58 deletions(-) > > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c > index a68ecbda6480..87acef31ace1 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" > > @@ -1524,8 +1524,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; > > @@ -1579,7 +1579,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) > @@ -1776,14 +1776,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++) > @@ -1867,11 +1867,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; > @@ -1879,18 +1879,17 @@ 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)); > - if (ret > adc_info->max_channels) { > + ret = device_property_count_u32(dev, "st,adc-diff-channels"); > + if ((ret / 2) > adc_info->max_channels) { hmm. The magic '2' that is introduced here kind of looses some of the meaning that existed for the *_count_elems_of_size() Perhaps introduce a define or a comment on why it is 2? > dev_err(&indio_dev->dev, "Bad st,adc-diff-channels?\n"); > return -EINVAL; > - } else if (ret > 0) { > - adc->num_diff = ret; > - num_channels += ret; > + } else if ((ret / 2) > 0) { > + adc->num_diff = ret / 2; > + num_channels += ret / 2; > } > > /* 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; > @@ -1901,21 +1900,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; > @@ -1936,32 +1934,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); > @@ -2004,22 +2021,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) { > @@ -2044,7 +2060,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; > @@ -2061,7 +2077,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); > @@ -2079,14 +2095,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; > @@ -2096,7 +2111,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; > @@ -2127,7 +2142,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) > @@ -2209,9 +2225,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; > @@ -2220,17 +2233,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; > @@ -2259,7 +2271,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; > > @@ -2276,7 +2288,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; >
On 6/11/22 17:47, Jonathan Cameron wrote: > On Fri, 10 Jun 2022 10:45:43 +0200 > Nuno Sá <nuno.sa@analog.com> 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> > > One trivial comment inline. > > In general this particular refactor is complex, so I either want > eyes of those familiar with the driver and / or testing or we may want > to reduce the scope to just the fwnode_xlate() and leave the more > complex conversions for when we can get that testing done. > > Let's see if we get the necessary review on this more complex patch. > I just don't want us to get stalled by trying to do too much at once! > > Jonathan > > >> --- >> drivers/iio/adc/stm32-adc.c | 128 ++++++++++++++++++++---------------- >> 1 file changed, 70 insertions(+), 58 deletions(-) >> >> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c >> index a68ecbda6480..87acef31ace1 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" >> >> @@ -1524,8 +1524,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; >> >> @@ -1579,7 +1579,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) >> @@ -1776,14 +1776,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++) >> @@ -1867,11 +1867,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; >> @@ -1879,18 +1879,17 @@ 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)); >> - if (ret > adc_info->max_channels) { >> + ret = device_property_count_u32(dev, "st,adc-diff-channels"); >> + if ((ret / 2) > adc_info->max_channels) { > hmm. The magic '2' that is introduced here kind of looses some of the meaning > that existed for the *_count_elems_of_size() > > Perhaps introduce a define or a comment on why it is 2? Hi Nuno, Thanks for your patch, please find some remarks below. First, I agree with Jonathan here. (Just suggesting) Would it be possible to keep a reference to the target struct to ease long term maintenance ? I'm thinking of: sizeof(struct stm32_adc_diff_channel) / sizeof(u32). Or possibly a better approach... > >> dev_err(&indio_dev->dev, "Bad st,adc-diff-channels?\n"); >> return -EINVAL; >> - } else if (ret > 0) { >> - adc->num_diff = ret; >> - num_channels += ret; >> + } else if ((ret / 2) > 0) { >> + adc->num_diff = ret / 2; >> + num_channels += ret / 2; >> } >> >> /* 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; >> @@ -1901,21 +1900,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; >> @@ -1936,32 +1934,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++) { Shouldn't use 'i' variable as is. I tested at my end. There's a bug around here, as the same variable 'i' is used already in next for loop. It creates an endless loop, that ends up in incrementing scan_index indefinitely until some oops occurs. I fixed that locally for quick testing by using another variable. Isn't there a helper to achive this (e.g. like of_property_for_each_u32) ? >> + 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. >> + >> + 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]; Minor remark, I'm wondering if using a switch (nsmps) {.. case} or similar would help to improve understandabilty / readability ? Best Regards, Fabrice >> >> /* Prepare sampling time settings */ >> stm32_adc_smpr_init(adc, channels[i].channel, smp); >> @@ -2004,22 +2021,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) { >> @@ -2044,7 +2060,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; >> @@ -2061,7 +2077,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); >> @@ -2079,14 +2095,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; >> @@ -2096,7 +2111,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; >> @@ -2127,7 +2142,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) >> @@ -2209,9 +2225,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; >> @@ -2220,17 +2233,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; >> @@ -2259,7 +2271,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; >> >> @@ -2276,7 +2288,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; >> >
diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c index a68ecbda6480..87acef31ace1 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" @@ -1524,8 +1524,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; @@ -1579,7 +1579,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) @@ -1776,14 +1776,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++) @@ -1867,11 +1867,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; @@ -1879,18 +1879,17 @@ 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)); - if (ret > adc_info->max_channels) { + ret = device_property_count_u32(dev, "st,adc-diff-channels"); + if ((ret / 2) > adc_info->max_channels) { dev_err(&indio_dev->dev, "Bad st,adc-diff-channels?\n"); return -EINVAL; - } else if (ret > 0) { - adc->num_diff = ret; - num_channels += ret; + } else if ((ret / 2) > 0) { + adc->num_diff = ret / 2; + num_channels += ret / 2; } /* 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; @@ -1901,21 +1900,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; @@ -1936,32 +1934,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); @@ -2004,22 +2021,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) { @@ -2044,7 +2060,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; @@ -2061,7 +2077,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); @@ -2079,14 +2095,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; @@ -2096,7 +2111,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; @@ -2127,7 +2142,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) @@ -2209,9 +2225,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; @@ -2220,17 +2233,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; @@ -2259,7 +2271,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; @@ -2276,7 +2288,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 | 128 ++++++++++++++++++++---------------- 1 file changed, 70 insertions(+), 58 deletions(-)