Hello Benjamin, First of all, apologies for the rookie mistakes in formatting and bundling everything in one patch - this was my first patch sent to the community. I will resend the series of patches once there is agreement on what they should look like. On Thu, Aug 12, 2021 at 9:47 AM Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote: > > 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? This patch allows our spi-hid driver to function correctly as an out of tree module. Prior to Android 12, OEMs were allowed to modify and build their own kernels, but that is changing with Google's GKI initiative. For Android 12 we will need to use the signed image provided by Google, and this patch is meant to prepare us for that. If you are curious, here is a published (older) version of our spi-hid: https://github.com/microsoft/surface-duo-oss-kernel.msm-4..14/tree/surfaceduo/10/2021.622.47/drivers/hid/spi-hid We are working on finalizing the SPI HID spec and making it public, and then preparing the transport driver for submission as a patch. The reason for this first patch is to unblock our development work in the meantime. I see in a branch of this thread Felipe asked some questions seeing if it is possible to get our device installed as hidraw without making some of the code changes in this patch. I will follow that discussion, test suggested approaches and will adjust the v2 of this patch accordingly. Thank you, Dmitry > > 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 > > >
