* [PATCH v2 1/1] iio: chemical: atlas-ezo-sensor: Make use of device properties
@ 2022-02-03 16:27 Andy Shevchenko
2022-02-05 16:37 ` Jonathan Cameron
0 siblings, 1 reply; 3+ messages in thread
From: Andy Shevchenko @ 2022-02-03 16:27 UTC (permalink / raw)
To: Andy Shevchenko, linux-iio, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen
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: fixed castings and qualifiers (LKP)
drivers/iio/chemical/atlas-ezo-sensor.c | 44 +++++++++++--------------
1 file changed, 20 insertions(+), 24 deletions(-)
diff --git a/drivers/iio/chemical/atlas-ezo-sensor.c b/drivers/iio/chemical/atlas-ezo-sensor.c
index b1bacfe3c3ce..3f3ea479b474 100644
--- a/drivers/iio/chemical/atlas-ezo-sensor.c
+++ b/drivers/iio/chemical/atlas-ezo-sensor.c
@@ -6,25 +6,21 @@
* Author: Matt Ranostay <matt.ranostay@konsulko.com>
*/
-#include <linux/module.h>
#include <linux/init.h>
#include <linux/delay.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
#include <linux/mutex.h>
+#include <linux/property.h>
#include <linux/err.h>
#include <linux/i2c.h>
-#include <linux/of_device.h>
+
#include <linux/iio/iio.h>
#define ATLAS_EZO_DRV_NAME "atlas-ezo-sensor"
#define ATLAS_INT_TIME_IN_MS 950
#define ATLAS_INT_HUM_TIME_IN_MS 350
-enum {
- ATLAS_CO2_EZO,
- ATLAS_O2_EZO,
- ATLAS_HUM_EZO,
-};
-
struct atlas_ezo_device {
const struct iio_chan_spec *channels;
int num_channels;
@@ -33,7 +29,7 @@ struct atlas_ezo_device {
struct atlas_ezo_data {
struct i2c_client *client;
- struct atlas_ezo_device *chip;
+ const struct atlas_ezo_device *chip;
/* lock to avoid multiple concurrent read calls */
struct mutex lock;
@@ -81,17 +77,17 @@ static const struct iio_chan_spec atlas_hum_ezo_channels[] = {
};
static struct atlas_ezo_device atlas_ezo_devices[] = {
- [ATLAS_CO2_EZO] = {
+ [0] = {
.channels = atlas_co2_ezo_channels,
.num_channels = 1,
.delay = ATLAS_INT_TIME_IN_MS,
},
- [ATLAS_O2_EZO] = {
+ [1] = {
.channels = atlas_o2_ezo_channels,
.num_channels = 1,
.delay = ATLAS_INT_TIME_IN_MS,
},
- [ATLAS_HUM_EZO] = {
+ [2] = {
.channels = atlas_hum_ezo_channels,
.num_channels = 1,
.delay = ATLAS_INT_HUM_TIME_IN_MS,
@@ -184,17 +180,17 @@ static const struct iio_info atlas_info = {
};
static const struct i2c_device_id atlas_ezo_id[] = {
- { "atlas-co2-ezo", ATLAS_CO2_EZO },
- { "atlas-o2-ezo", ATLAS_O2_EZO },
- { "atlas-hum-ezo", ATLAS_HUM_EZO },
+ { "atlas-co2-ezo", (kernel_ulong_t)&atlas_ezo_devices[0] },
+ { "atlas-o2-ezo", (kernel_ulong_t)&atlas_ezo_devices[1] },
+ { "atlas-hum-ezo", (kernel_ulong_t)&atlas_ezo_devices[2] },
{}
};
MODULE_DEVICE_TABLE(i2c, atlas_ezo_id);
static const struct of_device_id atlas_ezo_dt_ids[] = {
- { .compatible = "atlas,co2-ezo", .data = (void *)ATLAS_CO2_EZO, },
- { .compatible = "atlas,o2-ezo", .data = (void *)ATLAS_O2_EZO, },
- { .compatible = "atlas,hum-ezo", .data = (void *)ATLAS_HUM_EZO, },
+ { .compatible = "atlas,co2-ezo", .data = &atlas_ezo_devices[0], },
+ { .compatible = "atlas,o2-ezo", .data = &atlas_ezo_devices[1], },
+ { .compatible = "atlas,hum-ezo", .data = &atlas_ezo_devices[2], },
{}
};
MODULE_DEVICE_TABLE(of, atlas_ezo_dt_ids);
@@ -202,20 +198,20 @@ MODULE_DEVICE_TABLE(of, atlas_ezo_dt_ids);
static int atlas_ezo_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
+ const struct atlas_ezo_device *chip;
struct atlas_ezo_data *data;
- struct atlas_ezo_device *chip;
- const struct of_device_id *of_id;
struct iio_dev *indio_dev;
indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
if (!indio_dev)
return -ENOMEM;
- of_id = of_match_device(atlas_ezo_dt_ids, &client->dev);
- if (!of_id)
- chip = &atlas_ezo_devices[id->driver_data];
+ if (dev_fwnode(&client->dev))
+ chip = device_get_match_data(&client->dev);
else
- chip = &atlas_ezo_devices[(unsigned long)of_id->data];
+ chip = (const struct atlas_ezo_device *)id->driver_data;
+ if (!chip)
+ return -EINVAL;
indio_dev->info = &atlas_info;
indio_dev->name = ATLAS_EZO_DRV_NAME;
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/1] iio: chemical: atlas-ezo-sensor: Make use of device properties
2022-02-03 16:27 [PATCH v2 1/1] iio: chemical: atlas-ezo-sensor: Make use of device properties Andy Shevchenko
@ 2022-02-05 16:37 ` Jonathan Cameron
2022-02-05 18:41 ` Andy Shevchenko
0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Cameron @ 2022-02-05 16:37 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: linux-iio, linux-kernel, Lars-Peter Clausen
On Thu, 3 Feb 2022 18:27:25 +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>
Hi Andy,
Looks fine to me, though I'm a little curious what your logic
was in dropping the enum? Moving to pointers to the array
entry is fine, but without the enum, you have to refer back
and forwards whilst reading to check entries are the right ones.
I wouldn't have bothered commenting on this if reviewing new
code, but here you are removing what I would consider good
practice.
> ---
> v2: fixed castings and qualifiers (LKP)
> drivers/iio/chemical/atlas-ezo-sensor.c | 44 +++++++++++--------------
> 1 file changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/iio/chemical/atlas-ezo-sensor.c b/drivers/iio/chemical/atlas-ezo-sensor.c
> index b1bacfe3c3ce..3f3ea479b474 100644
> --- a/drivers/iio/chemical/atlas-ezo-sensor.c
> +++ b/drivers/iio/chemical/atlas-ezo-sensor.c
> @@ -6,25 +6,21 @@
> * Author: Matt Ranostay <matt.ranostay@konsulko.com>
> */
>
> -#include <linux/module.h>
> #include <linux/init.h>
> #include <linux/delay.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/property.h>
> #include <linux/err.h>
> #include <linux/i2c.h>
> -#include <linux/of_device.h>
> +
> #include <linux/iio/iio.h>
>
> #define ATLAS_EZO_DRV_NAME "atlas-ezo-sensor"
> #define ATLAS_INT_TIME_IN_MS 950
> #define ATLAS_INT_HUM_TIME_IN_MS 350
>
> -enum {
> - ATLAS_CO2_EZO,
> - ATLAS_O2_EZO,
> - ATLAS_HUM_EZO,
> -};
> -
> struct atlas_ezo_device {
> const struct iio_chan_spec *channels;
> int num_channels;
> @@ -33,7 +29,7 @@ struct atlas_ezo_device {
>
> struct atlas_ezo_data {
> struct i2c_client *client;
> - struct atlas_ezo_device *chip;
> + const struct atlas_ezo_device *chip;
>
> /* lock to avoid multiple concurrent read calls */
> struct mutex lock;
> @@ -81,17 +77,17 @@ static const struct iio_chan_spec atlas_hum_ezo_channels[] = {
> };
>
> static struct atlas_ezo_device atlas_ezo_devices[] = {
> - [ATLAS_CO2_EZO] = {
> + [0] = {
I think I would have kept the enum so ...
> .channels = atlas_co2_ezo_channels,
> .num_channels = 1,
> .delay = ATLAS_INT_TIME_IN_MS,
> },
> - [ATLAS_O2_EZO] = {
> + [1] = {
> .channels = atlas_o2_ezo_channels,
> .num_channels = 1,
> .delay = ATLAS_INT_TIME_IN_MS,
> },
> - [ATLAS_HUM_EZO] = {
> + [2] = {
> .channels = atlas_hum_ezo_channels,
> .num_channels = 1,
> .delay = ATLAS_INT_HUM_TIME_IN_MS,
> @@ -184,17 +180,17 @@ static const struct iio_info atlas_info = {
> };
>
> static const struct i2c_device_id atlas_ezo_id[] = {
> - { "atlas-co2-ezo", ATLAS_CO2_EZO },
> - { "atlas-o2-ezo", ATLAS_O2_EZO },
> - { "atlas-hum-ezo", ATLAS_HUM_EZO },
> + { "atlas-co2-ezo", (kernel_ulong_t)&atlas_ezo_devices[0] },
Locally it would have been obvious that
(kernel_ulong_t(&atlas_ezo_devices[ATLAS_CO2_EZO])
was the right one index.
> + { "atlas-o2-ezo", (kernel_ulong_t)&atlas_ezo_devices[1] },
> + { "atlas-hum-ezo", (kernel_ulong_t)&atlas_ezo_devices[2] },
> {}
> };
> MODULE_DEVICE_TABLE(i2c, atlas_ezo_id);
>
> static const struct of_device_id atlas_ezo_dt_ids[] = {
> - { .compatible = "atlas,co2-ezo", .data = (void *)ATLAS_CO2_EZO, },
> - { .compatible = "atlas,o2-ezo", .data = (void *)ATLAS_O2_EZO, },
> - { .compatible = "atlas,hum-ezo", .data = (void *)ATLAS_HUM_EZO, },
> + { .compatible = "atlas,co2-ezo", .data = &atlas_ezo_devices[0], },
> + { .compatible = "atlas,o2-ezo", .data = &atlas_ezo_devices[1], },
> + { .compatible = "atlas,hum-ezo", .data = &atlas_ezo_devices[2], },
> {}
> };
> MODULE_DEVICE_TABLE(of, atlas_ezo_dt_ids);
> @@ -202,20 +198,20 @@ MODULE_DEVICE_TABLE(of, atlas_ezo_dt_ids);
> static int atlas_ezo_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> + const struct atlas_ezo_device *chip;
> struct atlas_ezo_data *data;
> - struct atlas_ezo_device *chip;
> - const struct of_device_id *of_id;
> struct iio_dev *indio_dev;
>
> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> if (!indio_dev)
> return -ENOMEM;
>
> - of_id = of_match_device(atlas_ezo_dt_ids, &client->dev);
> - if (!of_id)
> - chip = &atlas_ezo_devices[id->driver_data];
> + if (dev_fwnode(&client->dev))
> + chip = device_get_match_data(&client->dev);
> else
> - chip = &atlas_ezo_devices[(unsigned long)of_id->data];
> + chip = (const struct atlas_ezo_device *)id->driver_data;
> + if (!chip)
> + return -EINVAL;
>
> indio_dev->info = &atlas_info;
> indio_dev->name = ATLAS_EZO_DRV_NAME;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/1] iio: chemical: atlas-ezo-sensor: Make use of device properties
2022-02-05 16:37 ` Jonathan Cameron
@ 2022-02-05 18:41 ` Andy Shevchenko
0 siblings, 0 replies; 3+ messages in thread
From: Andy Shevchenko @ 2022-02-05 18:41 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, linux-kernel, Lars-Peter Clausen
On Sat, Feb 05, 2022 at 04:37:58PM +0000, Jonathan Cameron wrote:
> On Thu, 3 Feb 2022 18:27:25 +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>
> Hi Andy,
>
> Looks fine to me, though I'm a little curious what your logic
> was in dropping the enum? Moving to pointers to the array
> entry is fine, but without the enum, you have to refer back
> and forwards whilst reading to check entries are the right ones.
>
> I wouldn't have bothered commenting on this if reviewing new
> code, but here you are removing what I would consider good
> practice.
> > static struct atlas_ezo_device atlas_ezo_devices[] = {
> > - [ATLAS_CO2_EZO] = {
> > + [0] = {
>
> I think I would have kept the enum so ...
Even in the original code it's an overkill.
The problems with enums and especially in the cases like this are:
- unnecessary level of indirection when we may use pointers directly
- the casting of the enum in the driver_data is ugly in my opinion
- the enum value 0 used as driver_data can't be read by
*device_get_match_data() APIs.
Or do you mean that use enum for the indices? That's okay.
Let me leave them for the sake of indices.
...
> > + { "atlas-co2-ezo", (kernel_ulong_t)&atlas_ezo_devices[0] },
>
> Locally it would have been obvious that
> (kernel_ulong_t(&atlas_ezo_devices[ATLAS_CO2_EZO])
> was the right one index.
Right.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-02-05 18:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-03 16:27 [PATCH v2 1/1] iio: chemical: atlas-ezo-sensor: Make use of device properties Andy Shevchenko
2022-02-05 16:37 ` Jonathan Cameron
2022-02-05 18:41 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.