| Message ID | 20220202204427.57506-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/1] iio: multiplexer: Make use of device properties | expand |
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; > }
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!
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
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; > }
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; > > }
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; }
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(-)