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