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

[Jonathan Cameron]

On Fri, 18 Feb 2022 13:27:17 +0100
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:

> On Fri, Feb 18, 2022 at 1:03 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > On Fri, 18 Feb 2022 09:39:14 +0100
> > Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:  
> > > On Fri, Feb 18, 2022 at 4:46 AM Kai-Heng Feng
> > > <kai.heng.feng@xxxxxxxxxxxxx> wrote:  
> > > > On Thu, Feb 17, 2022 at 6:57 PM Andy Shevchenko
> > > > <andy.shevchenko@xxxxxxxxx> wrote:  
> > > > > On Thursday, February 17, 2022, Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> 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.  
> 
> Thanks for chiming in.
> So, can we simply use dev_name() then? Or would it be too bad to have
> the device instance name there?

I'd rather it wasn't the device instance.  All the documentation etc
says part number for this so that's what will be expected by most
users.  The docs are deliberately vague (Typically a part number)
because some devices don't have one and we have those historical parts
where I missed they were using the instance name when reviewing.

Jonathan



>