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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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