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

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko