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

["Nuno Sá" <noname.nuno@gmail.com>]

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á