Re: [PATCH] HID: Support Microsoft Surface Duo SPI-based touch controller driver as a module.

[Felipe Balbi <balbi@kernel.org>]

Hi Dmitry,

Dmitry Antipov <daantipov@gmail.com> writes:
> 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.

commit log should be broken down at 72 characters

> Signed-off-by: Dmitry Antipov dmanti@microsoft.com

this is not the correct way of adding your Signed-off-by line, I'm
afraid. It should look like this:

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;

this should come as its own patch since it's not directly related to $subject

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

it looks like adding connect_mask could also be done as a separate
patch where the first patch just converts the existing code to use
connect_mask and addition for both G6 IDs is done separately

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

Adding this helper should be done as a seperate too.

-- 
balbi