Re: [PATCH v2 1/1] iio: chemical: atlas-ezo-sensor: Make use of device properties

[Jonathan Cameron <jic23@kernel.org>]

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;