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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 5 Feb 2022 20:41:14 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Lars-Peter Clausen <lars@...afoo.de>
Subject: Re: [PATCH v2 1/1] iio: chemical: atlas-ezo-sensor: Make use of
 device properties

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@...ux.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@...ux.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