Feedback-ID: ifd214418:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-156-g081acc5ed5-fm-20230206.001-g081acc5e
Mime-Version: 1.0
Message-Id: <501a6242-c377-4c71-9659-44ddfac00ba7@app.fastmail.com>
In-Reply-To: <Y+LYwu3Zs13hdVDy@google.com>
References: <Y+LYwu3Zs13hdVDy@google.com>
Date:   Wed, 08 Feb 2023 18:00:27 +1000
From:   Alistair <alistair@alistair23.me>
To:     "Dmitry Torokhov" <dmitry.torokhov@gmail.com>,
        "Jiri Kosina" <jikos@kernel.org>,
        "Benjamin Tissoires" <benjamin.tissoires@redhat.com>
Cc:     "Guenter Roeck" <groeck@chromium.org>,
        "Allen Ballway" <ballway@chromium.org>,
        "Hans de Goede" <hdegoede@redhat.com>, linux-input@vger.kernel.org,
        "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] HID: retain initial quirks set up when creating HID devices
Content-Type: text/plain
Precedence: bulk

On Wed, 8 Feb 2023, at 9:03 AM, Dmitry Torokhov wrote:
> In certain circumstances, such as when creating I2C-connected HID
> devices, we want to pass and retain some quirks (axis inversion, etc).
> The source of such quirks may be device tree, or DMI data, or something
> else not readily available to the HID core itself and therefore cannot
> be reconstructed easily. To allow this, introduce "initial_quirks" field
> in hid_device structure and use it when determining the final set of
> quirks.
> 
> This fixes the problem with i2c-hid setting up device-tree sourced
> quirks too late and losing them on device rebind, and also allows to
> sever the tie between hid-code and i2c-hid when applying DMI-based
> quirks.
> 
> Fixes: b60d3c803d76 ("HID: i2c-hid-of: Expose the touchscreen-inverted properties")
> Fixes: a2f416bf062a ("HID: multitouch: Add quirks for flipped axes")
> Reviewed-by: Guenter Roeck <groeck@chromium.org>
> Tested-by: Allen Ballway <ballway@chromium.org>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Alistair Francis <alistair@alistair23.me>

Alistair

> ---
> 
> v2:
> - corrected spelling/grammar in the commit message per Guenter
> - added reviewed-by/tested-by tags
> 
> 
> drivers/hid/hid-quirks.c                 | 8 +-------
> drivers/hid/i2c-hid/i2c-hid-core.c       | 6 ++++--
> drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c | 1 -
> include/linux/hid.h                      | 1 +
> 4 files changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> index 30e35f79def4..66e64350f138 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -19,7 +19,6 @@
> #include <linux/input/elan-i2c-ids.h>
>  
> #include "hid-ids.h"
> -#include "i2c-hid/i2c-hid.h"
>  
> /*
>   * Alphabetically sorted by vendor then product.
> @@ -1238,7 +1237,7 @@ EXPORT_SYMBOL_GPL(hid_quirks_exit);
> static unsigned long hid_gets_squirk(const struct hid_device *hdev)
> {
> const struct hid_device_id *bl_entry;
> - unsigned long quirks = 0;
> + unsigned long quirks = hdev->initial_quirks;
>  
> if (hid_match_id(hdev, hid_ignore_list))
> quirks |= HID_QUIRK_IGNORE;
> @@ -1299,11 +1298,6 @@ unsigned long hid_lookup_quirk(const struct hid_device *hdev)
> quirks = hid_gets_squirk(hdev);
> mutex_unlock(&dquirks_lock);
>  
> - /* Get quirks specific to I2C devices */
> - if (IS_ENABLED(CONFIG_I2C_DMI_CORE) && IS_ENABLED(CONFIG_DMI) &&
> -     hdev->bus == BUS_I2C)
> - quirks |= i2c_hid_get_dmi_quirks(hdev->vendor, hdev->product);
> -
> return quirks;
> }
> EXPORT_SYMBOL_GPL(hid_lookup_quirk);
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 0ab8f47a84e9..efbba0465eef 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -1025,6 +1025,10 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
> hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
> hid->product = le16_to_cpu(ihid->hdesc.wProductID);
>  
> + hid->initial_quirks = quirks;
> + hid->initial_quirks |= i2c_hid_get_dmi_quirks(hid->vendor,
> +       hid->product);
> +
> snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X",
> client->name, (u16)hid->vendor, (u16)hid->product);
> strscpy(hid->phys, dev_name(&client->dev), sizeof(hid->phys));
> @@ -1038,8 +1042,6 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
> goto err_mem_free;
> }
>  
> - hid->quirks |= quirks;
> -
> return 0;
>  
> err_mem_free:
> diff --git a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
> index 554a7dc28536..210f17c3a0be 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
> @@ -492,4 +492,3 @@ u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
>  
> return quirks;
> }
> -EXPORT_SYMBOL_GPL(i2c_hid_get_dmi_quirks);
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index daaac4d7f370..56dac09c99d9 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -622,6 +622,7 @@ struct hid_device { /* device report descriptor */
> unsigned long status; /* see STAT flags above */
> unsigned claimed; /* Claimed by hidinput, hiddev? */
> unsigned quirks; /* Various quirks the device can pull on us */
> + unsigned initial_quirks; /* Initial set of quirks supplied when creating device */
> bool io_started; /* If IO has started */
>  
> struct list_head inputs; /* The list of inputs */
> -- 
> 2.39.1.519.gcb327c4b5f-goog
> 
> 
> -- 
> Dmitry
>