Re: [PATCH v2 13/15] iio: adc: stm32-adc: convert to device properties

[Fabrice Gasnier <fabrice.gasnier@foss.st.com>]

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á
>