| Message ID | 20210812001250.1709418-1-dmanti@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | HID: Support Microsoft Surface Duo SPI-based touch controller driver as a module. | expand |
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.
Hi Dmitry, On Thu, Aug 12, 2021 at 2:13 AM Dmitry Antipov <daantipov@gmail.com> 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@microsoft.com 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 >
Hi Benjamin, Benjamin Tissoires <benjamin.tissoires@redhat.com> writes: > Hi Dmitry, > > On Thu, Aug 12, 2021 at 2:13 AM Dmitry Antipov <daantipov@gmail.com> 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@microsoft.com > > 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. Right, unfortunately spec isn't public yet (albeit having products shipped using it and a driver available in a public tree somewhere I couldn't find). I _do_ have one question though: Is there a way to tell hid core that $this device wants hidraw? If we remove the hid-microsoft changes, hid-generic will pick the device as expected, but this really needs hidraw. Any hints?
On Thu, Aug 12, 2021 at 7:16 PM Felipe Balbi <balbi@kernel.org> wrote: > > > Hi Benjamin, > > Benjamin Tissoires <benjamin.tissoires@redhat.com> writes: > > Hi Dmitry, > > > > On Thu, Aug 12, 2021 at 2:13 AM Dmitry Antipov <daantipov@gmail.com> 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@microsoft.com > > > > 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. > > Right, unfortunately spec isn't public yet (albeit having products > shipped using it and a driver available in a public tree somewhere I > couldn't find). > > I _do_ have one question though: > > Is there a way to tell hid core that $this device wants hidraw? If we Depending on what you want and can do I can think of several solutions: - add a quirk for this device (either at boot time, either in hid-quirks.c) There must be one that tells to only bind to hidraw - provide an out of the tree driver that declares the BUS:VID:PID, and start the HID device with HIDRAW only. > remove the hid-microsoft changes, hid-generic will pick the device as > expected, but this really needs hidraw. Any hints? I am fine with the hid-microsoft changes, I just want them in a separate commit. But I don't see why we would take only the first one without the SPI transport and the hid-microsoft code... Basically: not sure why you need hidraw for it. Cheers, Benjamin >
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@redhat.com> wrote: > > Hi Dmitry, > > On Thu, Aug 12, 2021 at 2:13 AM Dmitry Antipov <daantipov@gmail.com> 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@microsoft.com > > 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 > > >
Hi, Benjamin Tissoires <benjamin.tissoires@redhat.com> writes: >> > On Thu, Aug 12, 2021 at 2:13 AM Dmitry Antipov <daantipov@gmail.com> 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@microsoft.com >> > >> > 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. >> >> Right, unfortunately spec isn't public yet (albeit having products >> shipped using it and a driver available in a public tree somewhere I >> couldn't find). >> >> I _do_ have one question though: >> >> Is there a way to tell hid core that $this device wants hidraw? If we > > Depending on what you want and can do I can think of several solutions: > - add a quirk for this device (either at boot time, either in > hid-quirks.c) There must be one that tells to only bind to hidraw > - provide an out of the tree driver that declares the BUS:VID:PID, and > start the HID device with HIDRAW only. sounds good >> remove the hid-microsoft changes, hid-generic will pick the device as >> expected, but this really needs hidraw. Any hints? > > I am fine with the hid-microsoft changes, I just want them in a > separate commit. But I don't see why we would take only the first one > without the SPI transport and the hid-microsoft code... > > Basically: not sure why you need hidraw for it. Yeah, the touch controller is "peculiar". It sends to the host raw frames which needs to be processed by a userspace application. We don't get coordinates, pressure, etc. We get raw values from the touch digitizer; these are passed to a daemon which runs your usual filters (palm rejection, denoising, yada yada yada) and produces the actual touch inputs. Those are, in turn, given to uinput.
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@gmail.com> 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@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 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 >
On Fri, Aug 13, 2021 at 7:13 AM Felipe Balbi <balbi@kernel.org> wrote: > > > Hi, > > Benjamin Tissoires <benjamin.tissoires@redhat.com> writes: > >> > On Thu, Aug 12, 2021 at 2:13 AM Dmitry Antipov <daantipov@gmail.com> 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@microsoft.com > >> > > >> > 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. > >> > >> Right, unfortunately spec isn't public yet (albeit having products > >> shipped using it and a driver available in a public tree somewhere I > >> couldn't find). > >> > >> I _do_ have one question though: > >> > >> Is there a way to tell hid core that $this device wants hidraw? If we > > > > Depending on what you want and can do I can think of several solutions: > > - add a quirk for this device (either at boot time, either in > > hid-quirks.c) There must be one that tells to only bind to hidraw > > - provide an out of the tree driver that declares the BUS:VID:PID, and > > start the HID device with HIDRAW only. > > sounds good I did some more digging this morning. The quirk option is not especially good, there is no "hidraw only" quirk. However, there is a "HID_QUIRK_HAVE_SPECIAL_DRIVER" that prevents hid-generic from binding to your device. This has the same effect as adding the device in the hid-quirks.c in this submission, so for development purposes, if the device is messed up by hid-generic, I would advise to add this quirk (maybe from the development spi transport driver as a temporary work around). To have the device bound to hidraw only, then yes, if you provide a simple out of the tree module that binds to this module, hid-generic will release the device and let this out of the tree module deal with it. AFAICT, the hid-core changes you are asking here should not block the development process if they are not merged. You'll get an "UNKOWN" bus in the logs, and the SPI_HID_DEVICE macro can be defined in the out of the tree driver. > > >> remove the hid-microsoft changes, hid-generic will pick the device as > >> expected, but this really needs hidraw. Any hints? > > > > I am fine with the hid-microsoft changes, I just want them in a > > separate commit. But I don't see why we would take only the first one > > without the SPI transport and the hid-microsoft code... > > > > Basically: not sure why you need hidraw for it. > > Yeah, the touch controller is "peculiar". It sends to the host raw > frames which needs to be processed by a userspace application. We don't > get coordinates, pressure, etc. We get raw values from the touch > digitizer; these are passed to a daemon which runs your usual filters > (palm rejection, denoising, yada yada yada) and produces the actual > touch inputs. Those are, in turn, given to uinput. > In that case, maybe hidraw is not the best way to forward the events. V4L has a capability to export raw touch events. You can have a look at drivers/input/rmi4/rmi_f54.c, drivers/input/touchscreen/sur40.c or drivers/input/touchscreen/atmel_mxt_ts.c for some examples. The nice thing is you'll get parallel processing and DMA between the kernel and userspace. Also, there must be userspace tools around that are already capable of dealing with that kind of input. Though I also understand the need to have your own sauce. Cheers, Benjamin
Hi, Benjamin Tissoires <benjamin.tissoires@redhat.com> writes: >> Benjamin Tissoires <benjamin.tissoires@redhat.com> writes: >> >> > On Thu, Aug 12, 2021 at 2:13 AM Dmitry Antipov <daantipov@gmail.com> 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@microsoft.com >> >> > >> >> > 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. >> >> >> >> Right, unfortunately spec isn't public yet (albeit having products >> >> shipped using it and a driver available in a public tree somewhere I >> >> couldn't find). >> >> >> >> I _do_ have one question though: >> >> >> >> Is there a way to tell hid core that $this device wants hidraw? If we >> > >> > Depending on what you want and can do I can think of several solutions: >> > - add a quirk for this device (either at boot time, either in >> > hid-quirks.c) There must be one that tells to only bind to hidraw >> > - provide an out of the tree driver that declares the BUS:VID:PID, and >> > start the HID device with HIDRAW only. >> >> sounds good > > I did some more digging this morning. > > The quirk option is not especially good, there is no "hidraw only" quirk. > However, there is a "HID_QUIRK_HAVE_SPECIAL_DRIVER" that prevents > hid-generic from binding to your device. This has the same effect as > adding the device in the hid-quirks.c in this submission, so for > development purposes, if the device is messed up by hid-generic, I > would advise to add this quirk (maybe from the development spi > transport driver as a temporary work around). > > To have the device bound to hidraw only, then yes, if you provide a > simple out of the tree module that binds to this module, hid-generic > will release the device and let this out of the tree module deal with > it. > > AFAICT, the hid-core changes you are asking here should not block the > development process if they are not merged. You'll get an "UNKOWN" bus > in the logs, and the SPI_HID_DEVICE macro can be defined in the out of > the tree driver. that's correct, indeed. >> >> remove the hid-microsoft changes, hid-generic will pick the device as >> >> expected, but this really needs hidraw. Any hints? >> > >> > I am fine with the hid-microsoft changes, I just want them in a >> > separate commit. But I don't see why we would take only the first one >> > without the SPI transport and the hid-microsoft code... >> > >> > Basically: not sure why you need hidraw for it. >> >> Yeah, the touch controller is "peculiar". It sends to the host raw >> frames which needs to be processed by a userspace application. We don't >> get coordinates, pressure, etc. We get raw values from the touch >> digitizer; these are passed to a daemon which runs your usual filters >> (palm rejection, denoising, yada yada yada) and produces the actual >> touch inputs. Those are, in turn, given to uinput. >> > > In that case, maybe hidraw is not the best way to forward the events. > > V4L has a capability to export raw touch events. You can have a look > at drivers/input/rmi4/rmi_f54.c, drivers/input/touchscreen/sur40.c or > drivers/input/touchscreen/atmel_mxt_ts.c for some examples. > The nice thing is you'll get parallel processing and DMA between the > kernel and userspace. Also, there must be userspace tools around that > are already capable of dealing with that kind of input. Though I also > understand the need to have your own sauce. Right, I saw those but I'm not sure it applies to spi-hid. OTOH, maybe spi-hid is just a dumb transport and the V4L2 interface would be implemented in a hid-microsoft-surface.c driver, or something. Just to make things clear, the way spi-hid works is like shown below. First the enumeration phase: 1. power on & reset deassert 2. device asserts interrupt 3. Linux sends ack 4. device responds with a header 5. Linux sends ack (containing body size) 6. device responds with Reset Response 7. Linux requests device descriptor 8. device asserts interrupts 9. Linux sends ack 11. device responds with header 12. Linux sends ack (with body size) 13. device responds with device descriptor Then an actual transfer. 1. User touches the digitizer 2. device asserts interrupt 3. Linux sends ack 4. device responds with a header 5. Linux sends ack (containing body size) 6. device responds with Data packet (raw digitizer matrix read) As you can see, there's no polling at a constant rate. Everything is driven by the device's interrrupts. Also, this is a transport layer meaning we can have a touch controller, or a keyboard, or a joystick, or whatever HID device on top of this. There are also some "hazards" allowed by the spec. For example, the interrupt is edge triggered and the interrupt can be reasserted after completion of the ACK prior to the body data portion. I.e., the following is perfectly valid behavior (assuming rising edge IRQs): __ __ ____ IRQ __/ \______________/ \___/ \_______ ACK ACK MOSI __/XXXXX\__________/XXXXX\______________ header body MISO ___________/XXXXXX\_________/XXXXXXXX\__ There's also the possibility that the device can reset at any time. The spec calls these "Device Initiated Reset". The idea for this is to cope with e.g. such as static discharge causing the touch controller to reset and so on. These two little features make it rather hard to make sure that Linux won't loose any interrupt events and still deal with the fact that the device can reset at unexpected times. The way we've been dealing with this in the past was to teardown and recreate the HID device from scratch. This can result in race conditions between the IRQ handler and a workqueue which offloads the work of recreating the device. There's a possibility of race between kernel and userspace here. Remember that we don't know, ahead of time, which device is attached to the bus, we have to fetch the device descriptor to get VID/PID pair in order to start the entire machinery. I'm saying this because it's very likely that when we submit this driver publicly (after cleaning it up a bit) we end up creating a bigger discussion of how to support this new transport layer. We'll work through the comments, of course, but at the same time we want to reduce the amount of out-of-tree code we're keeping (for reasons which should be obvious :-) That being said, we'll have a look at the V4L2 interface for touch devices and check how well it would fit for our touch controller :-) cheers
On Fri, Aug 13, 2021 at 11:16 AM Felipe Balbi <balbi@kernel.org> wrote: > > > Hi, > > Benjamin Tissoires <benjamin.tissoires@redhat.com> writes: > >> Benjamin Tissoires <benjamin.tissoires@redhat.com> writes: > >> >> > On Thu, Aug 12, 2021 at 2:13 AM Dmitry Antipov <daantipov@gmail.com> 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@microsoft.com > >> >> > > >> >> > 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. > >> >> > >> >> Right, unfortunately spec isn't public yet (albeit having products > >> >> shipped using it and a driver available in a public tree somewhere I > >> >> couldn't find). > >> >> > >> >> I _do_ have one question though: > >> >> > >> >> Is there a way to tell hid core that $this device wants hidraw? If we > >> > > >> > Depending on what you want and can do I can think of several solutions: > >> > - add a quirk for this device (either at boot time, either in > >> > hid-quirks.c) There must be one that tells to only bind to hidraw > >> > - provide an out of the tree driver that declares the BUS:VID:PID, and > >> > start the HID device with HIDRAW only. > >> > >> sounds good > > > > I did some more digging this morning. > > > > The quirk option is not especially good, there is no "hidraw only" quirk. > > However, there is a "HID_QUIRK_HAVE_SPECIAL_DRIVER" that prevents > > hid-generic from binding to your device. This has the same effect as > > adding the device in the hid-quirks.c in this submission, so for > > development purposes, if the device is messed up by hid-generic, I > > would advise to add this quirk (maybe from the development spi > > transport driver as a temporary work around). > > > > To have the device bound to hidraw only, then yes, if you provide a > > simple out of the tree module that binds to this module, hid-generic > > will release the device and let this out of the tree module deal with > > it. > > > > AFAICT, the hid-core changes you are asking here should not block the > > development process if they are not merged. You'll get an "UNKOWN" bus > > in the logs, and the SPI_HID_DEVICE macro can be defined in the out of > > the tree driver. > > that's correct, indeed. > > >> >> remove the hid-microsoft changes, hid-generic will pick the device as > >> >> expected, but this really needs hidraw. Any hints? > >> > > >> > I am fine with the hid-microsoft changes, I just want them in a > >> > separate commit. But I don't see why we would take only the first one > >> > without the SPI transport and the hid-microsoft code... > >> > > >> > Basically: not sure why you need hidraw for it. > >> > >> Yeah, the touch controller is "peculiar". It sends to the host raw > >> frames which needs to be processed by a userspace application. We don't > >> get coordinates, pressure, etc. We get raw values from the touch > >> digitizer; these are passed to a daemon which runs your usual filters > >> (palm rejection, denoising, yada yada yada) and produces the actual > >> touch inputs. Those are, in turn, given to uinput. > >> > > > > In that case, maybe hidraw is not the best way to forward the events. > > > > V4L has a capability to export raw touch events. You can have a look > > at drivers/input/rmi4/rmi_f54.c, drivers/input/touchscreen/sur40.c or > > drivers/input/touchscreen/atmel_mxt_ts.c for some examples. > > The nice thing is you'll get parallel processing and DMA between the > > kernel and userspace. Also, there must be userspace tools around that > > are already capable of dealing with that kind of input. Though I also > > understand the need to have your own sauce. > > Right, I saw those but I'm not sure it applies to spi-hid. OTOH, maybe > spi-hid is just a dumb transport and the V4L2 interface would be > implemented in a hid-microsoft-surface.c driver, or something. Right. I think you said it all there: spi-hid is the transport layer, then all per-device logic/behaviour needs to be in the hid driver :) So V4L2 should go into hid-microsoft.c (or another one if that makes it too hard to melt the 2 together). > > Just to make things clear, the way spi-hid works is like shown > below. First the enumeration phase: > > 1. power on & reset deassert > 2. device asserts interrupt > 3. Linux sends ack > 4. device responds with a header > 5. Linux sends ack (containing body size) > 6. device responds with Reset Response > 7. Linux requests device descriptor > 8. device asserts interrupts > 9. Linux sends ack > 11. device responds with header > 12. Linux sends ack (with body size) > 13. device responds with device descriptor > > Then an actual transfer. > > 1. User touches the digitizer > 2. device asserts interrupt > 3. Linux sends ack > 4. device responds with a header > 5. Linux sends ack (containing body size) > 6. device responds with Data packet (raw digitizer matrix read) > > As you can see, there's no polling at a constant rate. Everything is > driven by the device's interrrupts. Also, this is a transport layer > meaning we can have a touch controller, or a keyboard, or a joystick, or > whatever HID device on top of this. Yes, this is pretty much like i2c-hid. > > There are also some "hazards" allowed by the spec. For example, the > interrupt is edge triggered and the interrupt can be reasserted after > completion of the ACK prior to the body data portion. I.e., the > following is perfectly valid behavior (assuming rising edge IRQs): > > __ __ ____ > IRQ __/ \______________/ \___/ \_______ > > ACK ACK > MOSI __/XXXXX\__________/XXXXX\______________ > > header body > MISO ___________/XXXXXX\_________/XXXXXXXX\__ > > > There's also the possibility that the device can reset at any time. The > spec calls these "Device Initiated Reset". The idea for this is to cope > with e.g. such as static discharge causing the touch controller to > reset and so on. > > These two little features make it rather hard to make sure that Linux > won't loose any interrupt events and still deal with the fact that the > device can reset at unexpected times. The way we've been dealing with > this in the past was to teardown and recreate the HID device from > scratch. This can result in race conditions between the IRQ handler and > a workqueue which offloads the work of recreating the device. There's a > possibility of race between kernel and userspace here. Indeed, this is not ideal. IIRC, i2c-hid also has to cope with device initiated resets, but I am not sure I have seen them in real life. So I think we just paper over it :( > > Remember that we don't know, ahead of time, which device is attached to > the bus, we have to fetch the device descriptor to get VID/PID pair in > order to start the entire machinery. Yep, this is exactly what we have for I2C, USB and Bluetooth. > > I'm saying this because it's very likely that when we submit this driver > publicly (after cleaning it up a bit) we end up creating a bigger > discussion of how to support this new transport layer. We'll work > through the comments, of course, but at the same time we want to reduce > the amount of out-of-tree code we're keeping (for reasons which should > be obvious :-) Yeah, no worries. But again, spi-hid should just bring up the device and forward the events. Adding a mechanism to ensure the IRQs are not lost is probably its responsibility, but deciding what to do in case of a device initiated reset is not. It should forward that information to HID core (maybe by adding a new API in hid-core and in hid_drivers), and what needs to be done is decided by the leaf driver (hid-microsoft). I bet hid-generic doesn't care, but hid-microsoft (or hid-multitouch) will. It will then be up to this driver to decide what to do. Unfortunately, unless you work on i2c-hid to detect device initiated resets we won't be able to accept a new HID API without users :( However, nothing prevents you from the out-of-the-tree driver to add a hook to hid-microsoft to directly call some internal API (but that would not be upstreamable, of course). > > That being said, we'll have a look at the V4L2 interface for touch > devices and check how well it would fit for our touch controller :-) Thanks! The one thing I am not entirely sure is if V4L2 requires polling. But with DMA, there is a chance we can just update the buffers as the interrupts come in and let the V4L2 userspace client do a regular polling (sorry, don't know enough of that area). One last thing: please note that there is a HID test-suite at https://gitlab.freedesktop.org/libevdev/hid-tools. Ideally, any change in hid-core should have a matching test added there. The test suite is using uhid, so that also means that you are welcome to add surface tests there too (though there is no V4L2 support yet), but beware that hid-microsoft should be entirely relying on HID for communication and should not directly talk to SPI/USB/I2C because we don't have those transport layers when emulating devices. BTW, there is something I always wanted to use in HID but never found a good implementation: eBPF. If V4L2 doesn't work with your needs, I wonder if you could not get the raw events through BPF in your userspace code. There is a chance you'll get a faster transfer rate than using hidraw. Maybe. Cheers, Benjamin > > cheers > > -- > balbi >
Hi, Benjamin Tissoires <benjamin.tissoires@redhat.com> writes: >> Benjamin Tissoires <benjamin.tissoires@redhat.com> writes: >> >> Benjamin Tissoires <benjamin.tissoires@redhat.com> writes: >> >> >> > On Thu, Aug 12, 2021 at 2:13 AM Dmitry Antipov <daantipov@gmail.com> 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@microsoft.com >> >> >> > >> >> >> > 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. >> >> >> >> >> >> Right, unfortunately spec isn't public yet (albeit having products >> >> >> shipped using it and a driver available in a public tree somewhere I >> >> >> couldn't find). >> >> >> >> >> >> I _do_ have one question though: >> >> >> >> >> >> Is there a way to tell hid core that $this device wants hidraw? If we >> >> > >> >> > Depending on what you want and can do I can think of several solutions: >> >> > - add a quirk for this device (either at boot time, either in >> >> > hid-quirks.c) There must be one that tells to only bind to hidraw >> >> > - provide an out of the tree driver that declares the BUS:VID:PID, and >> >> > start the HID device with HIDRAW only. >> >> >> >> sounds good >> > >> > I did some more digging this morning. >> > >> > The quirk option is not especially good, there is no "hidraw only" quirk. >> > However, there is a "HID_QUIRK_HAVE_SPECIAL_DRIVER" that prevents >> > hid-generic from binding to your device. This has the same effect as >> > adding the device in the hid-quirks.c in this submission, so for >> > development purposes, if the device is messed up by hid-generic, I >> > would advise to add this quirk (maybe from the development spi >> > transport driver as a temporary work around). >> > >> > To have the device bound to hidraw only, then yes, if you provide a >> > simple out of the tree module that binds to this module, hid-generic >> > will release the device and let this out of the tree module deal with >> > it. >> > >> > AFAICT, the hid-core changes you are asking here should not block the >> > development process if they are not merged. You'll get an "UNKOWN" bus >> > in the logs, and the SPI_HID_DEVICE macro can be defined in the out of >> > the tree driver. >> >> that's correct, indeed. >> >> >> >> remove the hid-microsoft changes, hid-generic will pick the device as >> >> >> expected, but this really needs hidraw. Any hints? >> >> > >> >> > I am fine with the hid-microsoft changes, I just want them in a >> >> > separate commit. But I don't see why we would take only the first one >> >> > without the SPI transport and the hid-microsoft code... >> >> > >> >> > Basically: not sure why you need hidraw for it. >> >> >> >> Yeah, the touch controller is "peculiar". It sends to the host raw >> >> frames which needs to be processed by a userspace application. We don't >> >> get coordinates, pressure, etc. We get raw values from the touch >> >> digitizer; these are passed to a daemon which runs your usual filters >> >> (palm rejection, denoising, yada yada yada) and produces the actual >> >> touch inputs. Those are, in turn, given to uinput. >> >> >> > >> > In that case, maybe hidraw is not the best way to forward the events. >> > >> > V4L has a capability to export raw touch events. You can have a look >> > at drivers/input/rmi4/rmi_f54.c, drivers/input/touchscreen/sur40.c or >> > drivers/input/touchscreen/atmel_mxt_ts.c for some examples. >> > The nice thing is you'll get parallel processing and DMA between the >> > kernel and userspace. Also, there must be userspace tools around that >> > are already capable of dealing with that kind of input. Though I also >> > understand the need to have your own sauce. >> >> Right, I saw those but I'm not sure it applies to spi-hid. OTOH, maybe >> spi-hid is just a dumb transport and the V4L2 interface would be >> implemented in a hid-microsoft-surface.c driver, or something. > > Right. I think you said it all there: spi-hid is the transport layer, > then all per-device logic/behaviour needs to be in the hid driver :) > So V4L2 should go into hid-microsoft.c (or another one if that makes > it too hard to melt the 2 together). makes sense to me, thanks :-) >> Just to make things clear, the way spi-hid works is like shown >> below. First the enumeration phase: >> >> 1. power on & reset deassert >> 2. device asserts interrupt >> 3. Linux sends ack >> 4. device responds with a header >> 5. Linux sends ack (containing body size) >> 6. device responds with Reset Response >> 7. Linux requests device descriptor >> 8. device asserts interrupts >> 9. Linux sends ack >> 11. device responds with header >> 12. Linux sends ack (with body size) >> 13. device responds with device descriptor >> >> Then an actual transfer. >> >> 1. User touches the digitizer >> 2. device asserts interrupt >> 3. Linux sends ack >> 4. device responds with a header >> 5. Linux sends ack (containing body size) >> 6. device responds with Data packet (raw digitizer matrix read) >> >> As you can see, there's no polling at a constant rate. Everything is >> driven by the device's interrrupts. Also, this is a transport layer >> meaning we can have a touch controller, or a keyboard, or a joystick, or >> whatever HID device on top of this. > > Yes, this is pretty much like i2c-hid. true, I think i2c-hid doesn't require the IRQ for enumeration, though. >> There are also some "hazards" allowed by the spec. For example, the >> interrupt is edge triggered and the interrupt can be reasserted after >> completion of the ACK prior to the body data portion. I.e., the >> following is perfectly valid behavior (assuming rising edge IRQs): >> >> __ __ ____ >> IRQ __/ \______________/ \___/ \_______ >> >> ACK ACK >> MOSI __/XXXXX\__________/XXXXX\______________ >> >> header body >> MISO ___________/XXXXXX\_________/XXXXXXXX\__ >> >> >> There's also the possibility that the device can reset at any time. The >> spec calls these "Device Initiated Reset". The idea for this is to cope >> with e.g. such as static discharge causing the touch controller to >> reset and so on. >> >> These two little features make it rather hard to make sure that Linux >> won't loose any interrupt events and still deal with the fact that the >> device can reset at unexpected times. The way we've been dealing with >> this in the past was to teardown and recreate the HID device from >> scratch. This can result in race conditions between the IRQ handler and >> a workqueue which offloads the work of recreating the device. There's a >> possibility of race between kernel and userspace here. > > Indeed, this is not ideal. > IIRC, i2c-hid also has to cope with device initiated resets, but I am > not sure I have seen them in real life. So I think we just paper over > it :( yeah, the thing is that it _can_ happen :-) Imagine the user has some considerable static charge (somewhat common during winter months, at least) when they touch the screen they could discharge enough electricity to cause a reset in the touch controller (sure, it's also a function of HW design, but there's always a limit to protection circuitry). >> Remember that we don't know, ahead of time, which device is attached to >> the bus, we have to fetch the device descriptor to get VID/PID pair in >> order to start the entire machinery. > > Yep, this is exactly what we have for I2C, USB and Bluetooth. Right >> I'm saying this because it's very likely that when we submit this driver >> publicly (after cleaning it up a bit) we end up creating a bigger >> discussion of how to support this new transport layer. We'll work >> through the comments, of course, but at the same time we want to reduce >> the amount of out-of-tree code we're keeping (for reasons which should >> be obvious :-) > > Yeah, no worries. But again, spi-hid should just bring up the device > and forward the events. > Adding a mechanism to ensure the IRQs are not lost is probably its > responsibility, but deciding what to do in case of a device initiated > reset is not. I think this the key information that we "missed". We ended up trying to treat the reset inside spi-hid and it's, well, hard :-) > It should forward that information to HID core (maybe by adding a new > API in hid-core and in hid_drivers), and what needs to be done is > decided by the leaf driver (hid-microsoft). fair enough. A device initiated reset will cause reenumeration, btw. Shouldn't we just destroy the hid device? > I bet hid-generic doesn't care, but hid-microsoft (or hid-multitouch) > will. It will then be up to this driver to decide what to do. > > Unfortunately, unless you work on i2c-hid to detect device initiated > resets we won't be able to accept a new HID API without users :( > However, nothing prevents you from the out-of-the-tree driver to add a > hook to hid-microsoft to directly call some internal API (but that > would not be upstreamable, of course). right, we're probably start working with what we have today. >> That being said, we'll have a look at the V4L2 interface for touch >> devices and check how well it would fit for our touch controller :-) > > Thanks! The one thing I am not entirely sure is if V4L2 requires > polling. But with DMA, there is a chance we can just update the > buffers as the interrupts come in and let the V4L2 userspace client do > a regular polling (sorry, don't know enough of that area). hmm, that may create problems. Because the touch controller is forwarding raw buffers, the reports can get pretty big. IIRC, our maximum input report size is 8kiB. If we assume 60Hz frequency on digitizer sampling, that's 480kiB/sec of data (max). > One last thing: please note that there is a HID test-suite at > https://gitlab.freedesktop.org/libevdev/hid-tools. Ideally, any change > in hid-core should have a matching test added there. > The test suite is using uhid, so that also means that you are welcome > to add surface tests there too (though there is no V4L2 support yet), > but beware that hid-microsoft should be entirely relying on HID for > communication and should not directly talk to SPI/USB/I2C because we > don't have those transport layers when emulating devices. nice, that's cool and will be helpful :-) > BTW, there is something I always wanted to use in HID but never found > a good implementation: eBPF. If V4L2 doesn't work with your needs, I > wonder if you could not get the raw events through BPF in your > userspace code. There is a chance you'll get a faster transfer rate > than using hidraw. Maybe. sounds fun, indeed. Need to check on availability of eBPF in the downstream kernels.
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); 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)
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(-)