Re: [PATCH] HID: Support Microsoft Surface Duo SPI-based touch controller driver as a module.
flat view
From: Benjamin Tissoires <hidden>
Date: 2021-08-13 07:53:39
Hi Dmitry, I made a second pass at this, and I have a few more comments. On Thu, Aug 12, 2021 at 2:13 AM Dmitry Antipov [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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@microsoft.com --- 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; 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 0x3201diff --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);
I think it would be clearer to group all connect_mask together, and remove the '?' which hides a bit what we expect to have. How about?: --- + connect_mask = HID_CONNECT_DEFAULT; + if (quirks & MS_HIDINPUT) + connect_mask |= HID_CONNECT_HIDINPUT_FORCE; + if (quirks & MS_NOHIDINPUT) + connect_mask &= ~HID_CONNECT_HIDINPUT; + + ret = hid_hw_start(hdev, connect_mask); --- Cheers, Benjamin
quoted hunk ↗ jump to hunk
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) }, #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) #define HID_REPORT_ID(rep) \ .report_type = (rep) --2.25.1