[PATCH v2 1/1] iio: multiplexer: Make use of device properties

[PATCH v2 1/1] iio: multiplexer: Make use of device properties

[Andy Shevchenko]

Convert the module to be property provider agnostic and allow
it to be used on non-OF platforms.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: dropped depends on OF
 drivers/iio/multiplexer/Kconfig   |  1 -
 drivers/iio/multiplexer/iio-mux.c | 48 ++++++++++++++-----------------
 2 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/multiplexer/Kconfig b/drivers/iio/multiplexer/Kconfig
index a1e1332d1206..928f424a1ed3 100644
--- a/drivers/iio/multiplexer/Kconfig
+++ b/drivers/iio/multiplexer/Kconfig
@@ -9,7 +9,6 @@ menu "Multiplexers"
 config IIO_MUX
 	tristate "IIO multiplexer driver"
 	select MULTIPLEXER
-	depends on OF || COMPILE_TEST
 	help
 	  Say yes here to build support for the IIO multiplexer.
 
diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
index f422d44377df..e6f0fef96494 100644
--- a/drivers/iio/multiplexer/iio-mux.c
+++ b/drivers/iio/multiplexer/iio-mux.c
@@ -10,11 +10,12 @@
 #include <linux/err.h>
 #include <linux/iio/consumer.h>
 #include <linux/iio/iio.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/mux/consumer.h>
-#include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 
 struct mux_ext_info_cache {
 	char *data;
@@ -324,37 +325,20 @@ static int mux_configure_channel(struct device *dev, struct mux *mux,
 	return 0;
 }
 
-/*
- * Same as of_property_for_each_string(), but also keeps track of the
- * index of each string.
- */
-#define of_property_for_each_string_index(np, propname, prop, s, i)	\
-	for (prop = of_find_property(np, propname, NULL),		\
-	     s = of_prop_next_string(prop, NULL),			\
-	     i = 0;							\
-	     s;								\
-	     s = of_prop_next_string(prop, s),				\
-	     i++)
-
 static int mux_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *np = pdev->dev.of_node;
 	struct iio_dev *indio_dev;
 	struct iio_channel *parent;
 	struct mux *mux;
-	struct property *prop;
-	const char *label;
+	const char **labels;
 	u32 state;
+	int children, all_children;
 	int sizeof_ext_info;
-	int children;
 	int sizeof_priv;
 	int i;
 	int ret;
 
-	if (!np)
-		return -ENODEV;
-
 	parent = devm_iio_channel_get(dev, "parent");
 	if (IS_ERR(parent))
 		return dev_err_probe(dev, PTR_ERR(parent),
@@ -366,9 +350,21 @@ static int mux_probe(struct platform_device *pdev)
 		sizeof_ext_info *= sizeof(*mux->ext_info);
 	}
 
+	all_children = device_property_count_string_array(dev, "channels");
+	if (all_children < 0)
+		return all_children;
+
+	labels = devm_kmalloc_array(dev, all_children, sizeof(char *), GFP_KERNEL);
+	if (!labels)
+		return -ENOMEM;
+
+	ret = device_property_read_string_array(dev, "channels", labels, all_children);
+	if (ret < 0)
+		return ret;
+
 	children = 0;
-	of_property_for_each_string(np, "channels", prop, label) {
-		if (*label)
+	for (state = 0; state < all_children; state++) {
+		if (*labels[state])
 			children++;
 	}
 	if (children <= 0) {
@@ -395,7 +391,7 @@ static int mux_probe(struct platform_device *pdev)
 	mux->cached_state = -1;
 
 	mux->delay_us = 0;
-	of_property_read_u32(np, "settle-time-us", &mux->delay_us);
+	device_property_read_u32(dev, "settle-time-us", &mux->delay_us);
 
 	indio_dev->name = dev_name(dev);
 	indio_dev->info = &mux_info;
@@ -426,11 +422,11 @@ static int mux_probe(struct platform_device *pdev)
 	}
 
 	i = 0;
-	of_property_for_each_string_index(np, "channels", prop, label, state) {
-		if (!*label)
+	for (state = 0; state < all_children; state++) {
+		if (!*labels[state])
 			continue;
 
-		ret = mux_configure_channel(dev, mux, state, label, i++);
+		ret = mux_configure_channel(dev, mux, state, labels[state], i++);
 		if (ret < 0)
 			return ret;
 	}
-- 
2.34.1

Re: [PATCH v2 1/1] iio: multiplexer: Make use of device properties

[Jonathan Cameron]

On Wed,  2 Feb 2022 22:44:27 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Convert the module to be property provider agnostic and allow
> it to be used on non-OF platforms.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Looks good to me, but as it's complex I'd like Peter + anyone else interested
to have a bit more time to take a look before I apply this one.

Thanks,

Jonathan

> ---
> v2: dropped depends on OF
>  drivers/iio/multiplexer/Kconfig   |  1 -
>  drivers/iio/multiplexer/iio-mux.c | 48 ++++++++++++++-----------------
>  2 files changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iio/multiplexer/Kconfig b/drivers/iio/multiplexer/Kconfig
> index a1e1332d1206..928f424a1ed3 100644
> --- a/drivers/iio/multiplexer/Kconfig
> +++ b/drivers/iio/multiplexer/Kconfig
> @@ -9,7 +9,6 @@ menu "Multiplexers"
>  config IIO_MUX
>  	tristate "IIO multiplexer driver"
>  	select MULTIPLEXER
> -	depends on OF || COMPILE_TEST
>  	help
>  	  Say yes here to build support for the IIO multiplexer.
>  
> diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
> index f422d44377df..e6f0fef96494 100644
> --- a/drivers/iio/multiplexer/iio-mux.c
> +++ b/drivers/iio/multiplexer/iio-mux.c
> @@ -10,11 +10,12 @@
>  #include <linux/err.h>
>  #include <linux/iio/consumer.h>
>  #include <linux/iio/iio.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/mux/consumer.h>
> -#include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  
>  struct mux_ext_info_cache {
>  	char *data;
> @@ -324,37 +325,20 @@ static int mux_configure_channel(struct device *dev, struct mux *mux,
>  	return 0;
>  }
>  
> -/*
> - * Same as of_property_for_each_string(), but also keeps track of the
> - * index of each string.
> - */
> -#define of_property_for_each_string_index(np, propname, prop, s, i)	\
> -	for (prop = of_find_property(np, propname, NULL),		\
> -	     s = of_prop_next_string(prop, NULL),			\
> -	     i = 0;							\
> -	     s;								\
> -	     s = of_prop_next_string(prop, s),				\
> -	     i++)
> -
>  static int mux_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct device_node *np = pdev->dev.of_node;
>  	struct iio_dev *indio_dev;
>  	struct iio_channel *parent;
>  	struct mux *mux;
> -	struct property *prop;
> -	const char *label;
> +	const char **labels;
>  	u32 state;
> +	int children, all_children;
>  	int sizeof_ext_info;
> -	int children;
>  	int sizeof_priv;
>  	int i;
>  	int ret;
>  
> -	if (!np)
> -		return -ENODEV;
> -
>  	parent = devm_iio_channel_get(dev, "parent");
>  	if (IS_ERR(parent))
>  		return dev_err_probe(dev, PTR_ERR(parent),
> @@ -366,9 +350,21 @@ static int mux_probe(struct platform_device *pdev)
>  		sizeof_ext_info *= sizeof(*mux->ext_info);
>  	}
>  
> +	all_children = device_property_count_string_array(dev, "channels");
> +	if (all_children < 0)
> +		return all_children;
> +
> +	labels = devm_kmalloc_array(dev, all_children, sizeof(char *), GFP_KERNEL);
> +	if (!labels)
> +		return -ENOMEM;
> +
> +	ret = device_property_read_string_array(dev, "channels", labels, all_children);
> +	if (ret < 0)
> +		return ret;
> +
>  	children = 0;
> -	of_property_for_each_string(np, "channels", prop, label) {
> -		if (*label)
> +	for (state = 0; state < all_children; state++) {
> +		if (*labels[state])
>  			children++;
>  	}
>  	if (children <= 0) {
> @@ -395,7 +391,7 @@ static int mux_probe(struct platform_device *pdev)
>  	mux->cached_state = -1;
>  
>  	mux->delay_us = 0;
> -	of_property_read_u32(np, "settle-time-us", &mux->delay_us);
> +	device_property_read_u32(dev, "settle-time-us", &mux->delay_us);
>  
>  	indio_dev->name = dev_name(dev);
>  	indio_dev->info = &mux_info;
> @@ -426,11 +422,11 @@ static int mux_probe(struct platform_device *pdev)
>  	}
>  
>  	i = 0;
> -	of_property_for_each_string_index(np, "channels", prop, label, state) {
> -		if (!*label)
> +	for (state = 0; state < all_children; state++) {
> +		if (!*labels[state])
>  			continue;
>  
> -		ret = mux_configure_channel(dev, mux, state, label, i++);
> +		ret = mux_configure_channel(dev, mux, state, labels[state], i++);
>  		if (ret < 0)
>  			return ret;
>  	}

Re: [PATCH v2 1/1] iio: multiplexer: Make use of device properties

[Andy Shevchenko]

On Sat, Feb 05, 2022 at 05:38:54PM +0000, Jonathan Cameron wrote:
> On Wed,  2 Feb 2022 22:44:27 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Convert the module to be property provider agnostic and allow
> > it to be used on non-OF platforms.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Looks good to me, but as it's complex I'd like Peter + anyone else interested
> to have a bit more time to take a look before I apply this one.

Sure, I would love to see more eyes and hear comments!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] iio: multiplexer: Make use of device properties

[Jonathan Cameron]

On Sat, 5 Feb 2022 20:30:18 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sat, Feb 05, 2022 at 05:38:54PM +0000, Jonathan Cameron wrote:
> > On Wed,  2 Feb 2022 22:44:27 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >   
> > > Convert the module to be property provider agnostic and allow
> > > it to be used on non-OF platforms.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>  
> > Looks good to me, but as it's complex I'd like Peter + anyone else interested
> > to have a bit more time to take a look before I apply this one.  
> 
> Sure, I would love to see more eyes and hear comments!
> 

Peter, do you have time to take a look at this one?
No problem if you are too busy, I'll just take another look myself and apply
it if I think it looks fine.

Thanks,

Jonathan

Re: [PATCH v2 1/1] iio: multiplexer: Make use of device properties

[Peter Rosin]

Hi!

Sorry for the late reply.

On 2022-02-02 21:44, Andy Shevchenko wrote:
> Convert the module to be property provider agnostic and allow
> it to be used on non-OF platforms.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: dropped depends on OF
>  drivers/iio/multiplexer/Kconfig   |  1 -
>  drivers/iio/multiplexer/iio-mux.c | 48 ++++++++++++++-----------------
>  2 files changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iio/multiplexer/Kconfig b/drivers/iio/multiplexer/Kconfig
> index a1e1332d1206..928f424a1ed3 100644
> --- a/drivers/iio/multiplexer/Kconfig
> +++ b/drivers/iio/multiplexer/Kconfig
> @@ -9,7 +9,6 @@ menu "Multiplexers"
>  config IIO_MUX
>  	tristate "IIO multiplexer driver"
>  	select MULTIPLEXER
> -	depends on OF || COMPILE_TEST
>  	help
>  	  Say yes here to build support for the IIO multiplexer.
>  
> diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
> index f422d44377df..e6f0fef96494 100644
> --- a/drivers/iio/multiplexer/iio-mux.c
> +++ b/drivers/iio/multiplexer/iio-mux.c
> @@ -10,11 +10,12 @@
>  #include <linux/err.h>
>  #include <linux/iio/consumer.h>
>  #include <linux/iio/iio.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/mux/consumer.h>
> -#include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  
>  struct mux_ext_info_cache {
>  	char *data;
> @@ -324,37 +325,20 @@ static int mux_configure_channel(struct device *dev, struct mux *mux,
>  	return 0;
>  }
>  
> -/*
> - * Same as of_property_for_each_string(), but also keeps track of the
> - * index of each string.
> - */
> -#define of_property_for_each_string_index(np, propname, prop, s, i)	\
> -	for (prop = of_find_property(np, propname, NULL),		\
> -	     s = of_prop_next_string(prop, NULL),			\
> -	     i = 0;							\
> -	     s;								\
> -	     s = of_prop_next_string(prop, s),				\
> -	     i++)
> -
>  static int mux_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct device_node *np = pdev->dev.of_node;
>  	struct iio_dev *indio_dev;
>  	struct iio_channel *parent;
>  	struct mux *mux;
> -	struct property *prop;
> -	const char *label;
> +	const char **labels;
>  	u32 state;
> +	int children, all_children;

I dislike that the pattern with one declaration per line is broken here.
There are perhaps a few too many variables, but introducing one instance
of two declarations on one line is disturbing for me.

So, with that fixed,

Acked-by: Peter Rosin <peda@axentia.se>

>  	int sizeof_ext_info;
> -	int children;
>  	int sizeof_priv;
>  	int i;
>  	int ret;
>  
> -	if (!np)
> -		return -ENODEV;
> -
>  	parent = devm_iio_channel_get(dev, "parent");
>  	if (IS_ERR(parent))
>  		return dev_err_probe(dev, PTR_ERR(parent),
> @@ -366,9 +350,21 @@ static int mux_probe(struct platform_device *pdev)
>  		sizeof_ext_info *= sizeof(*mux->ext_info);
>  	}
>  
> +	all_children = device_property_count_string_array(dev, "channels");
> +	if (all_children < 0)
> +		return all_children;
> +
> +	labels = devm_kmalloc_array(dev, all_children, sizeof(char *), GFP_KERNEL);

I'm a bit sad to see the copy of the labels array. No big deal,
but it seems inelegant. But the of_property_for_each_string_index
macro was also misplaced and therefore inelegant. My guess is
that the device properties should be fairly static and that the
copy is a waste?

So, anyway, I'm just whining here, and the labels copy is ok.

Cheers,
Peter

> +	if (!labels)
> +		return -ENOMEM;
> +
> +	ret = device_property_read_string_array(dev, "channels", labels, all_children);
> +	if (ret < 0)
> +		return ret;
> +
>  	children = 0;
> -	of_property_for_each_string(np, "channels", prop, label) {
> -		if (*label)
> +	for (state = 0; state < all_children; state++) {
> +		if (*labels[state])
>  			children++;
>  	}
>  	if (children <= 0) {
> @@ -395,7 +391,7 @@ static int mux_probe(struct platform_device *pdev)
>  	mux->cached_state = -1;
>  
>  	mux->delay_us = 0;
> -	of_property_read_u32(np, "settle-time-us", &mux->delay_us);
> +	device_property_read_u32(dev, "settle-time-us", &mux->delay_us);
>  
>  	indio_dev->name = dev_name(dev);
>  	indio_dev->info = &mux_info;
> @@ -426,11 +422,11 @@ static int mux_probe(struct platform_device *pdev)
>  	}
>  
>  	i = 0;
> -	of_property_for_each_string_index(np, "channels", prop, label, state) {
> -		if (!*label)
> +	for (state = 0; state < all_children; state++) {
> +		if (!*labels[state])
>  			continue;
>  
> -		ret = mux_configure_channel(dev, mux, state, label, i++);
> +		ret = mux_configure_channel(dev, mux, state, labels[state], i++);
>  		if (ret < 0)
>  			return ret;
>  	}

Re: [PATCH v2 1/1] iio: multiplexer: Make use of device properties

[Jonathan Cameron]

On Sun, 27 Feb 2022 03:28:15 +0100
Peter Rosin <peda@axentia.se> wrote:

> Hi!
> 
> Sorry for the late reply.
> 
> On 2022-02-02 21:44, Andy Shevchenko wrote:
> > Convert the module to be property provider agnostic and allow
> > it to be used on non-OF platforms.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > v2: dropped depends on OF
> >  drivers/iio/multiplexer/Kconfig   |  1 -
> >  drivers/iio/multiplexer/iio-mux.c | 48 ++++++++++++++-----------------
> >  2 files changed, 22 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/iio/multiplexer/Kconfig b/drivers/iio/multiplexer/Kconfig
> > index a1e1332d1206..928f424a1ed3 100644
> > --- a/drivers/iio/multiplexer/Kconfig
> > +++ b/drivers/iio/multiplexer/Kconfig
> > @@ -9,7 +9,6 @@ menu "Multiplexers"
> >  config IIO_MUX
> >  	tristate "IIO multiplexer driver"
> >  	select MULTIPLEXER
> > -	depends on OF || COMPILE_TEST
> >  	help
> >  	  Say yes here to build support for the IIO multiplexer.
> >  
> > diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
> > index f422d44377df..e6f0fef96494 100644
> > --- a/drivers/iio/multiplexer/iio-mux.c
> > +++ b/drivers/iio/multiplexer/iio-mux.c
> > @@ -10,11 +10,12 @@
> >  #include <linux/err.h>
> >  #include <linux/iio/consumer.h>
> >  #include <linux/iio/iio.h>
> > +#include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> >  #include <linux/mux/consumer.h>
> > -#include <linux/of.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/property.h>
> >  
> >  struct mux_ext_info_cache {
> >  	char *data;
> > @@ -324,37 +325,20 @@ static int mux_configure_channel(struct device *dev, struct mux *mux,
> >  	return 0;
> >  }
> >  
> > -/*
> > - * Same as of_property_for_each_string(), but also keeps track of the
> > - * index of each string.
> > - */
> > -#define of_property_for_each_string_index(np, propname, prop, s, i)	\
> > -	for (prop = of_find_property(np, propname, NULL),		\
> > -	     s = of_prop_next_string(prop, NULL),			\
> > -	     i = 0;							\
> > -	     s;								\
> > -	     s = of_prop_next_string(prop, s),				\
> > -	     i++)
> > -
> >  static int mux_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > -	struct device_node *np = pdev->dev.of_node;
> >  	struct iio_dev *indio_dev;
> >  	struct iio_channel *parent;
> >  	struct mux *mux;
> > -	struct property *prop;
> > -	const char *label;
> > +	const char **labels;
> >  	u32 state;
> > +	int children, all_children;  
> 
> I dislike that the pattern with one declaration per line is broken here.
> There are perhaps a few too many variables, but introducing one instance
> of two declarations on one line is disturbing for me.
> 
> So, with that fixed,
> 
> Acked-by: Peter Rosin <peda@axentia.se>
> 
> >  	int sizeof_ext_info;
> > -	int children;
> >  	int sizeof_priv;
> >  	int i;
> >  	int ret;
> >  
> > -	if (!np)
> > -		return -ENODEV;
> > -
> >  	parent = devm_iio_channel_get(dev, "parent");
> >  	if (IS_ERR(parent))
> >  		return dev_err_probe(dev, PTR_ERR(parent),
> > @@ -366,9 +350,21 @@ static int mux_probe(struct platform_device *pdev)
> >  		sizeof_ext_info *= sizeof(*mux->ext_info);
> >  	}
> >  
> > +	all_children = device_property_count_string_array(dev, "channels");
> > +	if (all_children < 0)
> > +		return all_children;

Function seems to be called device_property_string_array_count() so this will need
a v3 anyway as it doesn't currently build.

Thanks,

Jonathan



> > +
> > +	labels = devm_kmalloc_array(dev, all_children, sizeof(char *), GFP_KERNEL);  
> 
> I'm a bit sad to see the copy of the labels array. No big deal,
> but it seems inelegant. But the of_property_for_each_string_index
> macro was also misplaced and therefore inelegant. My guess is
> that the device properties should be fairly static and that the
> copy is a waste?
> 
> So, anyway, I'm just whining here, and the labels copy is ok.
> 
> Cheers,
> Peter
> 
> > +	if (!labels)
> > +		return -ENOMEM;
> > +
> > +	ret = device_property_read_string_array(dev, "channels", labels, all_children);
> > +	if (ret < 0)
> > +		return ret;
> > +
> >  	children = 0;
> > -	of_property_for_each_string(np, "channels", prop, label) {
> > -		if (*label)
> > +	for (state = 0; state < all_children; state++) {
> > +		if (*labels[state])
> >  			children++;
> >  	}
> >  	if (children <= 0) {
> > @@ -395,7 +391,7 @@ static int mux_probe(struct platform_device *pdev)
> >  	mux->cached_state = -1;
> >  
> >  	mux->delay_us = 0;
> > -	of_property_read_u32(np, "settle-time-us", &mux->delay_us);
> > +	device_property_read_u32(dev, "settle-time-us", &mux->delay_us);
> >  
> >  	indio_dev->name = dev_name(dev);
> >  	indio_dev->info = &mux_info;
> > @@ -426,11 +422,11 @@ static int mux_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	i = 0;
> > -	of_property_for_each_string_index(np, "channels", prop, label, state) {
> > -		if (!*label)
> > +	for (state = 0; state < all_children; state++) {
> > +		if (!*labels[state])
> >  			continue;
> >  
> > -		ret = mux_configure_channel(dev, mux, state, label, i++);
> > +		ret = mux_configure_channel(dev, mux, state, labels[state], i++);
> >  		if (ret < 0)
> >  			return ret;
> >  	}