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

From: "Nuno Sá" <noname.nuno@gmail.com>
To: "Fabrice Gasnier" <fabrice.gasnier@foss.st.com>,
	"Nuno Sá" <nuno.sa@analog.com>,
	linux-arm-msm@vger.kernel.org, openbmc@lists.ozlabs.org,
	linux-renesas-soc@vger.kernel.org,
	linux-mediatek@lists.infradead.org, linux-imx@nxp.com,
	linux-mips@vger.kernel.org,
	"Lad Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	linux-iio@vger.kernel.org, chrome-platform@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-stm32@st-md-mailman.stormreply.com
Cc: Andy Gross <agross@kernel.org>,
	Nicolas Ferre <nicolas.ferre@microchip.com>,
	 Benson Leung <bleung@chromium.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Tomer Maimon <tmaimon77@gmail.com>,
	Zhang Rui <rui.zhang@intel.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Eugen Hristev <eugen.hristev@microchip.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	 Benjamin Fair <benjaminfair@google.com>,
	Nancy Yuen <yuenn@google.com>,
	Jishnu Prakash <quic_jprakash@quicinc.com>,
	Christophe Branchereau <cbranchereau@gmail.com>,
	 Avi Fishman <avifishman70@gmail.com>,
	Tali Perry <tali.perry1@gmail.com>,
	Michael Hennerich <Michael.Hennerich@analog.com>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	 Claudiu Beznea <claudiu.beznea@microchip.com>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Thara Gopinath <thara.gopinath@linaro.org>,
	Cai Huoqing <cai.huoqing@linux.dev>,
	Fabio Estevam <festevam@gmail.com>,
	Olivier Moysan <olivier.moysan@foss.st.com>,
	Shawn Guo <shawnguo@kernel.org>, Haibo Chen <haibo.chen@nxp.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Patrick Venture <venture@google.com>,
	Amit Kucheria <amitk@kernel.org>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Lorenzo Bianconi <lorenzo@kernel.org>,
	Paul Cercueil <paul@crapouillou.net>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Gwendal Grignou <gwendal@chromium.org>,
	 Bjorn Andersson <bjorn.andersson@linaro.org>,
	Saravanan Sekar <sravanhome@gmail.com>,
	Guenter Roeck <groeck@chromium.org>,
	Jonathan Cameron <jic23@kernel.org>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Linus Walleij <linus.walleij@linaro.org>
Subject: Re: [PATCH v2 13/15] iio: adc: stm32-adc: convert to device properties
Date: Fri, 15 Jul 2022 13:22:18 +0200	[thread overview]
Message-ID: <83d816f52b3d4194b51b20f31b875055f63cd718.camel@gmail.com> (raw)
In-Reply-To: <c4385216-1d9e-34b3-58ea-edf8d9ed4cd8@foss.st.com>

On Wed, 2022-07-13 at 17:48 +0200, Fabrice Gasnier wrote:
> 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 ?
> 

No strong option so I can do as you prefer:

	ret = device_property_count_u32();
	if (ret / 
(sizeof(struct stm32_adc_diff_channel) / sizeof(u32)) > max_chan) {
		...
	}
	...

Probably it's a good idea to store that sizeof() division in a local
variable :)

- Nuno Sá

> > > 
> 