Re: [PATCH v5 1/4] HID: i2c-hid: Reorganize so ACPI and OF are separate modules

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Tue, Nov 10, 2020 at 02:17:27PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Tue, Nov 10, 2020 at 1:01 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi,
> >
> > On 11/9/20 10:36 PM, Douglas Anderson wrote:
> > > This patch rejiggers the i2c-hid code so that the OF (Open Firmware
> > > aka Device Tree) and ACPI support is separated out a bit.  The OF and
> > > ACPI drivers are now separate modules that wrap the core module.
> > >
> > > Essentially, what we're doing here:
> > > * Make "power up" and "power down" a function that can be (optionally)
> > >   implemented by a given user of the i2c-hid core.
> > > * The OF and ACPI modules are drivers on their own, so they implement
> > >   probe / remove / suspend / resume / shutdown.  The core code
> > >   provides implementations that OF and ACPI can call into.
> > >
> > > We'll organize this so that we now have 3 modules: the old i2c-hid
> > > module becomes the "core" module and two new modules will depend on
> > > it, handling probing the specific device.
> > >
> > > As part of this work, we'll remove the i2c-hid "platform data"
> > > concept since it's not needed.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > >
> > > Changes in v5:
> > > - Add shutdown_tail op and use it in ACPI.
> > > - i2chid_subclass_data => i2chid_ops.
> > > - power_up_device => power_up (same with power_down).
> > > - subclass => ops.
> > >
> >
> > Thanks this looks good to now, 2 small remarks below (since you are
> > going to do a v6 anyways). Feel free to ignore these remarks if
> > you prefer to keep things as is.
> >
> > And feel free to add my reviewed-by to v6 of this patch:
> >
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> Thanks!
> 
> 
> > > +static const struct i2c_device_id i2c_hid_acpi_id_table[] = {
> > > +     { "hid", 0 },
> > > +     { "hid-over-i2c", 0 },
> > > +     { },
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, i2c_hid_acpi_id_table);
> >
> > Hmm, I do not think these old-style i2c-ids are necessarry at
> > all in this driver. I expect all use-cases to use either
> > of or acpi matches.
> >
> > This was already present in the code before though, so
> > please ignore this remark. This is just something which
> > I noticed and thought was worth while pointing out as
> > a future cleanup.
> 
> Yeah, I wasn't sure if there was anyone using them.
> 
> Hrm.  Thinking about it, though, is it really OK for two drivers to
> both have the same table listed?  I'm not sure how that would work.
> Do you know?
> 
> I don't know a ton about ACPI, but for device tree I know i2c has a
> fallback mode.  Specifically having this table means that we'll match
> compatible strings such as:
> 
>   "zipzapzing,hid"
>   "kapowzers,hid-over-i2c"
> 
> In other words it'll ignore the vendor part and just match on the
> second half.  Just to make sure I wasn't remembering that from a dream
> I tried it and it worked.  I don't see any mainline device trees that
> look like that, though.  I could delete it, though it doesn't really
> take up much space and it seems nice to keep it working in case anyone
> was relying on it?
> 
> For ACPI is there a similar fallback?  If not then it seems like it'd
> be easy to remove it from there...

Just a random thought - will all this still be working with ACPI PRP0001
and DT-style compatible string and properties in _DSD?

Thanks.

-- 
Dmitry