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