Re: [PATCH] HID: multitouch: enable the Surface 4 Type Cover Pro (JP) to report multitouch data

Re: [PATCH] HID: multitouch: enable the Surface 4 Type Cover Pro (JP) to report multitouch data

[benjamin.tissoires]

Hi,

Please resend this patch to Jiri Kosina (Cc-ed),
linux-input@vger.kernel.org and myself so we can include it upstream.

On Dec 15 2016 or thereabouts, Yuta Kobayashi wrote:
> From: Yuta Kobayashi <alu.ula@outlook.com>
> 
> Signed-off-by: Yuta Kobayashi <alu.ula@outlook.com>
> Signed-off-by: alu <alu@ninja.co.jp>
> ---
>  drivers/hid/hid-core.c          | 2 --
>  drivers/hid/hid-ids.h           | 1 -
>  drivers/hid/hid-microsoft.c     | 2 --
>  drivers/hid/usbhid/hid-quirks.c | 1 -
>  4 files changed, 6 deletions(-)

Otherwise, that's the kind of patches I love :)

Cheers,
Benjamin

> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index cff060b..db87d91 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -729,7 +729,6 @@ static void hid_scan_collection(struct hid_parser *parser, unsigned type)
>  	     hid->product == USB_DEVICE_ID_MS_TYPE_COVER_PRO_3_JP ||
>  	     hid->product == USB_DEVICE_ID_MS_TYPE_COVER_PRO_4 ||
>  	     hid->product == USB_DEVICE_ID_MS_TYPE_COVER_PRO_4_2 ||
> -	     hid->product == USB_DEVICE_ID_MS_TYPE_COVER_PRO_4_JP ||
>  	     hid->product == USB_DEVICE_ID_MS_POWER_COVER) &&
>  	    hid->group == HID_GROUP_MULTITOUCH)
>  		hid->group = HID_GROUP_GENERIC;
> @@ -1990,7 +1989,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_PRO_3_JP) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_PRO_4) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_PRO_4_2) },
> -	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_PRO_4_JP) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_DIGITAL_MEDIA_7K) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_DIGITAL_MEDIA_600) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_DIGITAL_MEDIA_3KV1) },
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index ec277b9..55758f7 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -723,7 +723,6 @@
>  #define USB_DEVICE_ID_MS_TYPE_COVER_PRO_3_JP 0x07dd
>  #define USB_DEVICE_ID_MS_TYPE_COVER_PRO_4 0x07e4
>  #define USB_DEVICE_ID_MS_TYPE_COVER_PRO_4_2 0x07e8
> -#define USB_DEVICE_ID_MS_TYPE_COVER_PRO_4_JP 0x07e9
>  #define USB_DEVICE_ID_MS_POWER_COVER     0x07da
>  
>  #define USB_VENDOR_ID_MOJO		0x8282
> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
> index 74b7b84..2b706b5 100644
> --- a/drivers/hid/hid-microsoft.c
> +++ b/drivers/hid/hid-microsoft.c
> @@ -284,8 +284,6 @@ static const struct hid_device_id ms_devices[] = {
>  		.driver_data = MS_HIDINPUT },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_PRO_4_2),
>  		.driver_data = MS_HIDINPUT },
> -	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_PRO_4_JP),
> -		.driver_data = MS_HIDINPUT },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_POWER_COVER),
>  		.driver_data = MS_HIDINPUT },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_COMFORT_KEYBOARD),
> diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
> index b3e01c8..ebb8e95 100644
> --- a/drivers/hid/usbhid/hid-quirks.c
> +++ b/drivers/hid/usbhid/hid-quirks.c
> @@ -105,7 +105,6 @@ static const struct hid_blacklist {
>  	{ USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_PRO_3_JP, HID_QUIRK_NO_INIT_REPORTS },
>  	{ USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_PRO_4, HID_QUIRK_NO_INIT_REPORTS },
>  	{ USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_PRO_4_2, HID_QUIRK_NO_INIT_REPORTS },
> -	{ USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_PRO_4_JP, HID_QUIRK_NO_INIT_REPORTS },
>  	{ USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_POWER_COVER, HID_QUIRK_NO_INIT_REPORTS },
>  	{ USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GT683R_LED_PANEL, HID_QUIRK_NO_INIT_REPORTS },
>  	{ USB_VENDOR_ID_NEXIO, USB_DEVICE_ID_NEXIO_MULTITOUCH_PTI0750, HID_QUIRK_NO_INIT_REPORTS },
> -- 
> 2.5.5
> 

Re: [PATCH] HID: multitouch: enable the Surface 4 Type Cover Pro (JP) to report multitouch data

[benjamin.tissoires]

On Dec 15 2016 or thereabouts, Yuta Kobayashi wrote:
> From: Yuta Kobayashi <alu.ula@outlook.com>
> 
> Signed-off-by: Yuta Kobayashi <alu.ula@outlook.com>
> ---
>  drivers/hid/hid-core.c          | 2 --
>  drivers/hid/hid-ids.h           | 1 -
>  drivers/hid/hid-microsoft.c     | 2 --
>  drivers/hid/usbhid/hid-quirks.c | 1 -
>  4 files changed, 6 deletions(-)

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

Re: [PATCH] HID: multitouch: enable the Surface 4 Type Cover Pro (JP) to report multitouch data

[Jiri Kosina]

On Thu, 15 Dec 2016, benjamin.tissoires@redhat.com wrote:

> On Dec 15 2016 or thereabouts, Yuta Kobayashi wrote:
> > From: Yuta Kobayashi <alu.ula@outlook.com>
> > 
> > Signed-off-by: Yuta Kobayashi <alu.ula@outlook.com>
> > ---
> >  drivers/hid/hid-core.c          | 2 --
> >  drivers/hid/hid-ids.h           | 1 -
> >  drivers/hid/hid-microsoft.c     | 2 --
> >  drivers/hid/usbhid/hid-quirks.c | 1 -
> >  4 files changed, 6 deletions(-)
> 
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Thanks. I still have one request though -- Yuta-san, could you please 
resend the patch with a brief description in the patch changelog (what the 
patch is achieving and why this way is the correct way to do so).

We generally want brief description even for the simpliest changes; 
future will be grateful.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: multitouch: enable the Surface 4 Type Cover Pro (JP) to report multitouch data

[benjamin.tissoires]

Hi,

On Dec 16 2016 or thereabouts, Yuta Kobayashi wrote:
> Thank you for advice.
> 
> I found this thread.
> http://www.spinics.net/lists/linux-input/msg48065.html
> 
> I think special drivers for Type Cover will not be needed by the patches.
> So i removed some defines  such as patch below.
> http://www.spinics.net/lists/linux-input/msg48068.html 
> 
> Will not this be ground?
> 
> If so, my idea is just wrong, sorry.

Actually, your idea is right. The thing Jiri is asking is just a small
paragraph in the patch explaining why this is required. Both Jiri and I
know this is the good approach but it is purely for documentation
purpose.

Something along the lines would be:
---
Since commit 8fe89ef076fa1 ("HID: multitouch: enable the Surface 3 Type
Cover to report multitouch data"), the TypeCover can be properly handled
by hid-multitouch and don't require any special quirk in the kernel.

Remove the support of the Surface 4 Type Cover Pro (JP) from
hid-microsoft so it can properly report multitouch from the touchpad.
---

Add this paragraph below a blank line after the title of the commit and
before your signed-off-by and my reviewed-by, resubmit and we are good
to go.

Cheers,
Benjamin

> 
> Thanks.
> 
> 2016-12-16 15:46 GMT+09:00 Yuta Kobayashi <alu.ula@outlook.com>:
> > On Thu, 15 Dec 2016, benjamin.tissoires@redhat.com wrote:
> >
> >> On Dec 15 2016 or thereabouts, Yuta Kobayashi wrote:
> >> > From: Yuta Kobayashi <alu.ula@outlook.com>
> >> >
> >> > Signed-off-by: Yuta Kobayashi <alu.ula@outlook.com>
> >> > ---
> >> >  drivers/hid/hid-core.c          | 2 --
> >> >  drivers/hid/hid-ids.h           | 1 -
> >> >  drivers/hid/hid-microsoft.c     | 2 --
> >> >  drivers/hid/usbhid/hid-quirks.c | 1 -
> >> >  4 files changed, 6 deletions(-)
> >>
> >> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
> > Thanks. I still have one request though -- Yuta-san, could you please
> > resend the patch with a brief description in the patch changelog (what the
> > patch is achieving and why this way is the correct way to do so).
> >
> > We generally want brief description even for the simpliest changes;
> > future will be grateful.
> >
> > Thanks,
> >
> > --
> > Jiri Kosina
> > SUSE Labs
> >