Hi Dmitry,
On Thu, Aug 12, 2021 at 2:13 AM Dmitry Antipov <daantipov@xxxxxxxxx> wrote:
>
> Surface Duo uses a touch digitizer that communicates to the main SoC via SPI
> and presents itself as a HID device. This patch contains the changes needed
> for the driver to work as a module: HID Core affordances for SPI devices,
> addition of the new Device IDs, and a new quirk in hid-microsoft. The driver
> itself is being prepared for a submission in the near future.
>
> Signed-off-by: Dmitry Antipov dmanti@xxxxxxxxxxxxx
Though I really appreciate seeing a microsoft.com submission, the
commit description makes me wonder if we should not postpone the
inclusion of this patch with the "submission in the near future".
AFAIK, HID is not SPI aware. So basically, we are introducing dead
code in the upstream kernel if we take this patch.
Why is there a special need for us to take this one without the whole series?
Couple of additions to what Felipe said:
> ---
> drivers/hid/hid-core.c | 3 +++
> drivers/hid/hid-ids.h | 2 ++
> drivers/hid/hid-microsoft.c | 15 +++++++++++++--
> drivers/hid/hid-quirks.c | 2 ++
> include/linux/hid.h | 2 ++
> 5 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 7db332139f7d..123a0e3a6b1a 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2005,6 +2005,9 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
> case BUS_I2C:
> bus = "I2C";
> break;
> + case BUS_SPI:
> + bus = "SPI";
> + break;
I'd like to have the SPI specific changes that are touching hid core
in a separate patch, or with the SPI-HID transport layer.
> case BUS_VIRTUAL:
> bus = "VIRTUAL";
> break;
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 8f1893e68112..5c181d23a7ae 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -885,6 +885,8 @@
> #define USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER 0x02fd
> #define USB_DEVICE_ID_MS_PIXART_MOUSE 0x00cb
> #define USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS 0x02e0
> +#define SPI_DEVICE_ID_MS_SURFACE_G6_0 0x0c1d
> +#define SPI_DEVICE_ID_MS_SURFACE_G6_1 0x0c42
>
> #define USB_VENDOR_ID_MOJO 0x8282
> #define USB_DEVICE_ID_RETRO_ADAPTER 0x3201
> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
> index 071fd093a5f4..50ea1f68c285 100644
> --- a/drivers/hid/hid-microsoft.c
> +++ b/drivers/hid/hid-microsoft.c
> @@ -27,6 +27,7 @@
> #define MS_DUPLICATE_USAGES BIT(5)
> #define MS_SURFACE_DIAL BIT(6)
> #define MS_QUIRK_FF BIT(7)
> +#define MS_NOHIDINPUT BIT(8)
>
> struct ms_data {
> unsigned long quirks;
> @@ -367,6 +368,7 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
> unsigned long quirks = id->driver_data;
> struct ms_data *ms;
> int ret;
> + unsigned int connect_mask;
>
> ms = devm_kzalloc(&hdev->dev, sizeof(*ms), GFP_KERNEL);
> if (ms == NULL)
> @@ -376,20 +378,25 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
>
> hid_set_drvdata(hdev, ms);
>
> + connect_mask = HID_CONNECT_DEFAULT | ((quirks & MS_HIDINPUT) ?
> + HID_CONNECT_HIDINPUT_FORCE : 0);
> +
> if (quirks & MS_NOGET)
> hdev->quirks |= HID_QUIRK_NOGET;
>
> if (quirks & MS_SURFACE_DIAL)
> hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
>
> + if (quirks & MS_NOHIDINPUT)
> + connect_mask &= ~HID_CONNECT_HIDINPUT;
> +
> ret = hid_parse(hdev);
> if (ret) {
> hid_err(hdev, "parse failed\n");
> goto err_free;
> }
>
> - ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT | ((quirks & MS_HIDINPUT) ?
> - HID_CONNECT_HIDINPUT_FORCE : 0));
> + ret = hid_hw_start(hdev, connect_mask);
> if (ret) {
> hid_err(hdev, "hw start failed\n");
> goto err_free;
> @@ -450,6 +457,10 @@ static const struct hid_device_id ms_devices[] = {
> .driver_data = MS_QUIRK_FF },
> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS),
> .driver_data = MS_QUIRK_FF },
> + { HID_SPI_DEVICE(USB_VENDOR_ID_MICROSOFT, SPI_DEVICE_ID_MS_SURFACE_G6_0),
> + .driver_data = MS_NOHIDINPUT },
> + { HID_SPI_DEVICE(USB_VENDOR_ID_MICROSOFT, SPI_DEVICE_ID_MS_SURFACE_G6_1),
> + .driver_data = MS_NOHIDINPUT },
> { }
> };
> MODULE_DEVICE_TABLE(hid, ms_devices);
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> index 51b39bda9a9d..01609e5425b9 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -506,6 +506,8 @@ static const struct hid_device_id hid_have_special_driver[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_DIGITAL_MEDIA_3KV1) },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_POWER_COVER) },
> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_PRESENTER_8K_BT) },
> + { HID_SPI_DEVICE(USB_VENDOR_ID_MICROSOFT, SPI_DEVICE_ID_MS_SURFACE_G6_0) },
> + { HID_SPI_DEVICE(USB_VENDOR_ID_MICROSOFT, SPI_DEVICE_ID_MS_SURFACE_G6_1) },
This hunk should be dropped if it just works without. If it doesn't
work without it, we probably need to fix why.
So, yes, please drop these additions to hid-quirks :)
> #endif
> #if IS_ENABLED(CONFIG_HID_MONTEREY)
> { HID_USB_DEVICE(USB_VENDOR_ID_MONTEREY, USB_DEVICE_ID_GENIUS_KB29E) },
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 9e067f937dbc..32823c6b65f6 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -684,6 +684,8 @@ struct hid_descriptor {
> .bus = BUS_BLUETOOTH, .vendor = (ven), .product = (prod)
> #define HID_I2C_DEVICE(ven, prod) \
> .bus = BUS_I2C, .vendor = (ven), .product = (prod)
> +#define HID_SPI_DEVICE(ven, prod) \
> + .bus = BUS_SPI, .vendor = (ven), .product = (prod)
That one should be separated and merged with the first hunk.
Cheers,
Benjamin
>
> #define HID_REPORT_ID(rep) \
> .report_type = (rep)
> --
> 2.25.1
>
