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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> 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
