Re: [PATCH v4] iio: accel: adxl345: Add ACPI HID table

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 18 Feb 2022 09:39:14 +0100
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Feb 18, 2022 at 4:46 AM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
> > On Thu, Feb 17, 2022 at 6:57 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:  
> > > On Thursday, February 17, 2022, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:  
> 
> ...
> 
> > >> +               acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
> > >> +               if (acpi_id) {
> > >> +                       type = acpi_id->driver_data;
> > >> +                       name = acpi_id->id;
> > >> +               } else
> > >> +                       return -ENODEV;  
> > >
> > > Thanks, but can we do this in ACPI agnostic way?
> > >
> > > Can be as simple as
> > >
> > > if (id)
> > >   ...
> > > else {
> > >   match = device_get_match_data(dev);
> > >   if (!match)
> > >     return -ENODEV;
> > > }
> > >
> > > Note, it might require to reconsider what is put in the driver data (either convert to pointers, or be sure that valid type is never a 0/NULL).  
> >
> > Unlike acpi_match_device(), device_get_match_data() only get
> > driver_data, so we need a new struct to provide both name and type.  
> 
> It's unfortunate. Let me think about it a bit more.
Usual solution is just to add that name to a per device type structure.
In this particular case there isn't one so far though and an enum is used
in the one place we might otherwise have used a part number specific structure.

Probably the easiest thing to do is use the enum to do a lookup in an array
of structures and have the string there.

> 
> > > Also note, in both cases using ID name for name us fragile. Probably we have to fix that first. Let me check today’s evening.  
> >
> > Can you please explain more on this? How does ID name make it fragile?  
> 
> I thought this one is used somehow by userspace to distinguish the
> instance of the device, but looking into the rest of the IIO drivers
> it seems more or less  a field for part number. That said, the ID is
> okay to use. I hope Jonathan may correct me.
> 
Should be part number.  Instances are distinguished via label rather than
name (or via the device parent on older kernels where we didn't have
label).

There are a few places where we accidentally let though IDs that aren't
always simply the part number and they became part of the ABI so we
couldn't really fix them after the event.

Jonathan