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