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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11/11/20 1:04 AM, Dmitry Torokhov wrote:
> 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?

That should keep working. Unless someone mixes a DT-style compatible string
with the PNP0C50 ACPI HID specific DSM for getting the hid_descriptor_address
(instead of a DT style property). But that would be a really weird mix to
use and obviously would go against both the PNP0C50 and the PRP0001 specs.

Regards,

Hans