lkml.org 
[lkml]   [2020]   [Nov]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 1/4] HID: i2c-hid: Reorganize so ACPI and OF are separate modules
From
Date
Hi,

On 11/10/20 11:17 PM, 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?

Ah I missed that you are adding the i2c_device_id table to
both the new -acpi and -of drivers.

That indeed is not a good idea.

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

Yeah I know about that clever hack the i2c subsys has.

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

Ack, so in the of case there is a reason to keep this. But ACPI binding
does not use anything similar, so the old-style i2c_device_id table
should probably be removed from the -acpi driver.

> For ACPI is there a similar fallback? If not then it seems like it'd
> be easy to remove it from there...
>
>
>> <snip>
>>
>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
>>> index aeff1ffb0c8b..9551ba96dc49 100644
>>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>>> @@ -35,12 +35,8 @@
>>> #include <linux/kernel.h>
>>> #include <linux/hid.h>
>>> #include <linux/mutex.h>
>>> -#include <linux/acpi.h>
>>> -#include <linux/of.h>
>>> #include <linux/regulator/consumer.h>
>>
>> I think you can drop this regulator include here now too ?
>
> Good point.
>
>
>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
>>> new file mode 100644
>>> index 000000000000..15d533be2b24
>>> --- /dev/null
>>> +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
>> <snip>
>>
>>> +static const struct dev_pm_ops i2c_hid_of_pm = {
>>> + SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume)
>>> +};
>>
>> This dev_pm_ops struct is identical to the one in i2c-hid-acpi.c
>> and the one which you introduce later in i2c-hid-of-goodix.c
>> is also identical, so that is 3 copies.
>>
>> Maybe just put a shared dev_pm_ops struct in the i2c-core
>> (and don't export the suspend/resume handlers) ?
>
> Sounds good.

Regards,

Hans

\
 
 \ /
  Last update: 2020-11-11 12:14    [W:0.068 / U:0.784 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site