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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d51318d1-5d26-f840-2651-42a1134d407b@redhat.com>
Date:   Tue, 10 Nov 2020 10:01:10 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Douglas Anderson <dianders@...omium.org>, jkosina@...e.cz,
        benjamin.tissoires@...hat.com, gregkh@...uxfoundation.org,
        Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     kai.heng.feng@...onical.com, robh+dt@...nel.org,
        linux-input@...r.kernel.org, swboyd@...omium.org,
        andrea@...gia.bo.it, Jiri Kosina <jikos@...nel.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Pavel Balan <admin@...ma.net>,
        Xiaofei Tan <tanxiaofei@...wei.com>,
        You-Sheng Yang <vicamo.yang@...onical.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/4] HID: i2c-hid: Reorganize so ACPI and OF are
 separate modules

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@...omium.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@...hat.com>

<snip>

> diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> new file mode 100644
> index 000000000000..5f09635d00ce
> --- /dev/null
> +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> @@ -0,0 +1,170 @@

<snip>

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

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


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

Regards,

Hans