On 6/11/22 17:47, Jonathan Cameron wrote:
> On Fri, 10 Jun 2022 10:45:43 +0200
> Nuno Sá <nuno.sa@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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;
>>
>
