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 [off-list ref] wrote:

quoted

Hi,

On 11/9/20 10:36 PM, Douglas Anderson wrote:

quoted

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 <redacted>

Thanks!

quoted

quoted

+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