All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] HID: google: modify HID device groups of eel
@ 2022-01-07  9:13 Zhengqiao Xia
  2022-01-07  9:31 ` Zhengqiao Xia
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Zhengqiao Xia @ 2022-01-07  9:13 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, linux-input, linux-kernel
  Cc: dianders, Zhengqiao Xia

If HID_GROUP of eel is set to HID_GROUP_GENERIC, Whiskers Tablet
Mode Switch of eel hammer will not be detected by system. when it
is set to HID_GROUP_VIVALDI, system will detect Whiskers Tablet
Mode Switch successfully.

Signed-off-by: Zhengqiao Xia <xiazhengqiao@huaqin.corp-partner.google.com>
---
 drivers/hid/hid-google-hammer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-google-hammer.c b/drivers/hid/hid-google-hammer.c
index 0403beb3104b..e5acd15f4a55 100644
--- a/drivers/hid/hid-google-hammer.c
+++ b/drivers/hid/hid-google-hammer.c
@@ -585,7 +585,7 @@ static void hammer_remove(struct hid_device *hdev)
 static const struct hid_device_id hammer_devices[] = {
 	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
 		     USB_VENDOR_ID_GOOGLE, USB_DEVICE_ID_GOOGLE_DON) },
-	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
+	{ HID_DEVICE(BUS_USB, HID_GROUP_VIVALDI,
 		     USB_VENDOR_ID_GOOGLE, USB_DEVICE_ID_GOOGLE_EEL) },
 	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
 		     USB_VENDOR_ID_GOOGLE, USB_DEVICE_ID_GOOGLE_HAMMER) },
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] HID: google: modify HID device groups of eel
  2022-01-07  9:13 [PATCH v2] HID: google: modify HID device groups of eel Zhengqiao Xia
@ 2022-01-07  9:31 ` Zhengqiao Xia
  2022-01-14  8:38 ` Jiri Kosina
  2022-01-14 11:33 ` Re " xiazhengqiao
  2 siblings, 0 replies; 8+ messages in thread
From: Zhengqiao Xia @ 2022-01-07  9:31 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, linux-input, linux-kernel; +Cc: dianders

Hi Jiri,

  As your suggestions in https://patchwork.kernel.org/project/linux-input/patch/20211228114650.31981-1-xiazhengqiao@huaqin.corp-partner.google.com/ ,

I add verbose changelog, and change my email name, please help to review.

thanks.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] HID: google: modify HID device groups of eel
  2022-01-07  9:13 [PATCH v2] HID: google: modify HID device groups of eel Zhengqiao Xia
  2022-01-07  9:31 ` Zhengqiao Xia
@ 2022-01-14  8:38 ` Jiri Kosina
  2022-01-14 19:25   ` Stephen Boyd
  2022-01-14 11:33 ` Re " xiazhengqiao
  2 siblings, 1 reply; 8+ messages in thread
From: Jiri Kosina @ 2022-01-14  8:38 UTC (permalink / raw)
  To: Zhengqiao Xia
  Cc: benjamin.tissoires, linux-input, linux-kernel, dianders,
	Wei-Ning Huang, Dmitry Torokhov, Stephen Boyd, Nicolas Boichat,
	Sean O'Brien

On Fri, 7 Jan 2022, Zhengqiao Xia wrote:

> If HID_GROUP of eel is set to HID_GROUP_GENERIC, Whiskers Tablet
> Mode Switch of eel hammer will not be detected by system. when it
> is set to HID_GROUP_VIVALDI, system will detect Whiskers Tablet
> Mode Switch successfully.
> 
> Signed-off-by: Zhengqiao Xia <xiazhengqiao@huaqin.corp-partner.google.com>
> ---
>  drivers/hid/hid-google-hammer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-google-hammer.c b/drivers/hid/hid-google-hammer.c
> index 0403beb3104b..e5acd15f4a55 100644
> --- a/drivers/hid/hid-google-hammer.c
> +++ b/drivers/hid/hid-google-hammer.c
> @@ -585,7 +585,7 @@ static void hammer_remove(struct hid_device *hdev)
>  static const struct hid_device_id hammer_devices[] = {
>  	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
>  		     USB_VENDOR_ID_GOOGLE, USB_DEVICE_ID_GOOGLE_DON) },
> -	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
> +	{ HID_DEVICE(BUS_USB, HID_GROUP_VIVALDI,
>  		     USB_VENDOR_ID_GOOGLE, USB_DEVICE_ID_GOOGLE_EEL) },
>  	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,

Color me confused, but anything with HID_GROUP_VIVALDI should be matched 
by hid-vivaldi driver, so what is this about?

Adding some more CCs.

Thanks,

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re Re: [PATCH v2] HID: google: modify HID device groups of eel
  2022-01-07  9:13 [PATCH v2] HID: google: modify HID device groups of eel Zhengqiao Xia
  2022-01-07  9:31 ` Zhengqiao Xia
  2022-01-14  8:38 ` Jiri Kosina
@ 2022-01-14 11:33 ` xiazhengqiao
  2 siblings, 0 replies; 8+ messages in thread
From: xiazhengqiao @ 2022-01-14 11:33 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, linux-input, linux-kernel; +Cc: phoenixshen

Hi Jiri,

> Color me confused, but anything with HID_GROUP_VIVALDI should be matched 
> by hid-vivaldi driver, so what is this about?

yes, HID_GROUP_VIVALDI will be matched by hid-vivaldi driver. 

CONFIG_HID_VIVALDI will be set to y when it is used.  

thanks.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] HID: google: modify HID device groups of eel
  2022-01-14  8:38 ` Jiri Kosina
@ 2022-01-14 19:25   ` Stephen Boyd
  2022-01-14 19:59     ` Dmitry Torokhov
  0 siblings, 1 reply; 8+ messages in thread
From: Stephen Boyd @ 2022-01-14 19:25 UTC (permalink / raw)
  To: Jiri Kosina, Zhengqiao Xia
  Cc: benjamin.tissoires, linux-input, linux-kernel, dianders,
	Wei-Ning Huang, Dmitry Torokhov, Nicolas Boichat,
	Sean O'Brien, phoenixshen

Quoting Jiri Kosina (2022-01-14 00:38:23)
> On Fri, 7 Jan 2022, Zhengqiao Xia wrote:
>
> > If HID_GROUP of eel is set to HID_GROUP_GENERIC, Whiskers Tablet
> > Mode Switch of eel hammer will not be detected by system. when it
> > is set to HID_GROUP_VIVALDI, system will detect Whiskers Tablet
> > Mode Switch successfully.
> >
> > Signed-off-by: Zhengqiao Xia <xiazhengqiao@huaqin.corp-partner.google.com>
> > ---
> >  drivers/hid/hid-google-hammer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/hid-google-hammer.c b/drivers/hid/hid-google-hammer.c
> > index 0403beb3104b..e5acd15f4a55 100644
> > --- a/drivers/hid/hid-google-hammer.c
> > +++ b/drivers/hid/hid-google-hammer.c
> > @@ -585,7 +585,7 @@ static void hammer_remove(struct hid_device *hdev)
> >  static const struct hid_device_id hammer_devices[] = {
> >       { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
> >                    USB_VENDOR_ID_GOOGLE, USB_DEVICE_ID_GOOGLE_DON) },
> > -     { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
> > +     { HID_DEVICE(BUS_USB, HID_GROUP_VIVALDI,
> >                    USB_VENDOR_ID_GOOGLE, USB_DEVICE_ID_GOOGLE_EEL) },
> >       { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
>
> Color me confused, but anything with HID_GROUP_VIVALDI should be matched
> by hid-vivaldi driver, so what is this about?
>

My understanding is that 'vivaldi' is mostly a keyboard layout and
'hammer' is a detachable keyboard. We want to prevent the hid-vivaldi
driver from probing this particular device because the hid-vivaldi
driver doesn't know about detachable keyboards. Hammer devices also
support 360 degree wraparound so we know that the keyboard has been put
behind the screen or that it's being used to stand up the device on a
table.

Given all that, I'm still confused. If we make the hid-google-hammer
driver probe this device and the keyboard layout is vivaldi then we'd
want the part of the vivaldi driver that exposes the
function_row_physmap through sysfs. Otherwise userspace won't know how
to handle the function row properly. I think we need the device to stack
two drivers here. Does that happen with HID?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] HID: google: modify HID device groups of eel
  2022-01-14 19:25   ` Stephen Boyd
@ 2022-01-14 19:59     ` Dmitry Torokhov
  2022-01-14 20:55       ` Stephen Boyd
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Torokhov @ 2022-01-14 19:59 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Jiri Kosina, Zhengqiao Xia, benjamin.tissoires, linux-input,
	linux-kernel, dianders, Wei-Ning Huang, Nicolas Boichat,
	Sean O'Brien, phoenixshen

On Fri, Jan 14, 2022 at 01:25:12PM -0600, Stephen Boyd wrote:
> Quoting Jiri Kosina (2022-01-14 00:38:23)
> > On Fri, 7 Jan 2022, Zhengqiao Xia wrote:
> >
> > > If HID_GROUP of eel is set to HID_GROUP_GENERIC, Whiskers Tablet
> > > Mode Switch of eel hammer will not be detected by system. when it
> > > is set to HID_GROUP_VIVALDI, system will detect Whiskers Tablet
> > > Mode Switch successfully.
> > >
> > > Signed-off-by: Zhengqiao Xia <xiazhengqiao@huaqin.corp-partner.google.com>
> > > ---
> > >  drivers/hid/hid-google-hammer.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hid/hid-google-hammer.c b/drivers/hid/hid-google-hammer.c
> > > index 0403beb3104b..e5acd15f4a55 100644
> > > --- a/drivers/hid/hid-google-hammer.c
> > > +++ b/drivers/hid/hid-google-hammer.c
> > > @@ -585,7 +585,7 @@ static void hammer_remove(struct hid_device *hdev)
> > >  static const struct hid_device_id hammer_devices[] = {
> > >       { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
> > >                    USB_VENDOR_ID_GOOGLE, USB_DEVICE_ID_GOOGLE_DON) },
> > > -     { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
> > > +     { HID_DEVICE(BUS_USB, HID_GROUP_VIVALDI,
> > >                    USB_VENDOR_ID_GOOGLE, USB_DEVICE_ID_GOOGLE_EEL) },
> > >       { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
> >
> > Color me confused, but anything with HID_GROUP_VIVALDI should be matched
> > by hid-vivaldi driver, so what is this about?

We need this particular chunk because hid_scan_collection() forces all
devices that declare usage 0x01 from the Google Vendor page to be marked
as HID_GROUP_VIVALDI, so without it the hammer driver will not match
eel.

However this change (I believe) will break vivaldi functionality
(reporting of the top row map) so we can't apply this as is.

> >
> 
> My understanding is that 'vivaldi' is mostly a keyboard layout and
> 'hammer' is a detachable keyboard. We want to prevent the hid-vivaldi
> driver from probing this particular device because the hid-vivaldi
> driver doesn't know about detachable keyboards. Hammer devices also
> support 360 degree wraparound so we know that the keyboard has been put
> behind the screen or that it's being used to stand up the device on a
> table.
> 
> Given all that, I'm still confused. If we make the hid-google-hammer
> driver probe this device and the keyboard layout is vivaldi then we'd
> want the part of the vivaldi driver that exposes the
> function_row_physmap through sysfs. Otherwise userspace won't know how
> to handle the function row properly. I think we need the device to stack
> two drivers here. Does that happen with HID?

As far as I know HID does not easily allow "stacking" drivers like that.

Probably the easiest way would be to export vivaldi_feature_mapping()
and the show method for the physical row map and call them from the
hammer driver.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] HID: google: modify HID device groups of eel
  2022-01-14 19:59     ` Dmitry Torokhov
@ 2022-01-14 20:55       ` Stephen Boyd
  2022-01-14 21:40         ` Dmitry Torokhov
  0 siblings, 1 reply; 8+ messages in thread
From: Stephen Boyd @ 2022-01-14 20:55 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Jiri Kosina, Zhengqiao Xia, benjamin.tissoires, linux-input,
	linux-kernel, dianders, Wei-Ning Huang, Nicolas Boichat,
	Sean O'Brien, phoenixshen

Quoting Dmitry Torokhov (2022-01-14 11:59:50)
> On Fri, Jan 14, 2022 at 01:25:12PM -0600, Stephen Boyd wrote:
> >
> > My understanding is that 'vivaldi' is mostly a keyboard layout and
> > 'hammer' is a detachable keyboard. We want to prevent the hid-vivaldi
> > driver from probing this particular device because the hid-vivaldi
> > driver doesn't know about detachable keyboards. Hammer devices also
> > support 360 degree wraparound so we know that the keyboard has been put
> > behind the screen or that it's being used to stand up the device on a
> > table.
> >
> > Given all that, I'm still confused. If we make the hid-google-hammer
> > driver probe this device and the keyboard layout is vivaldi then we'd
> > want the part of the vivaldi driver that exposes the
> > function_row_physmap through sysfs. Otherwise userspace won't know how
> > to handle the function row properly. I think we need the device to stack
> > two drivers here. Does that happen with HID?
>
> As far as I know HID does not easily allow "stacking" drivers like that.

Ok.

>
> Probably the easiest way would be to export vivaldi_feature_mapping()
> and the show method for the physical row map and call them from the
> hammer driver.
>

I worry about builtin vs. modular drivers so probably ought to make some
hid-vivaldi-common.c file that has the physmap code and then have both
drivers call that mini-library. The 'vivaldi_data' structure would need
to be figured out too. The hammer driver stores 'hammer_kbd_leds' in the
hid_get_drvdata() whereas the vivaldi driver stores 'vivalid_data' so we
can't simply call the show method for the sysfs attribute without some
minor surgery.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] HID: google: modify HID device groups of eel
  2022-01-14 20:55       ` Stephen Boyd
@ 2022-01-14 21:40         ` Dmitry Torokhov
  0 siblings, 0 replies; 8+ messages in thread
From: Dmitry Torokhov @ 2022-01-14 21:40 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Jiri Kosina, Zhengqiao Xia, benjamin.tissoires, linux-input,
	linux-kernel, dianders, Wei-Ning Huang, Nicolas Boichat,
	Sean O'Brien, phoenixshen

On Fri, Jan 14, 2022 at 02:55:49PM -0600, Stephen Boyd wrote:
> Quoting Dmitry Torokhov (2022-01-14 11:59:50)
> > On Fri, Jan 14, 2022 at 01:25:12PM -0600, Stephen Boyd wrote:
> > >
> > > My understanding is that 'vivaldi' is mostly a keyboard layout and
> > > 'hammer' is a detachable keyboard. We want to prevent the hid-vivaldi
> > > driver from probing this particular device because the hid-vivaldi
> > > driver doesn't know about detachable keyboards. Hammer devices also
> > > support 360 degree wraparound so we know that the keyboard has been put
> > > behind the screen or that it's being used to stand up the device on a
> > > table.
> > >
> > > Given all that, I'm still confused. If we make the hid-google-hammer
> > > driver probe this device and the keyboard layout is vivaldi then we'd
> > > want the part of the vivaldi driver that exposes the
> > > function_row_physmap through sysfs. Otherwise userspace won't know how
> > > to handle the function row properly. I think we need the device to stack
> > > two drivers here. Does that happen with HID?
> >
> > As far as I know HID does not easily allow "stacking" drivers like that.
> 
> Ok.
> 
> >
> > Probably the easiest way would be to export vivaldi_feature_mapping()
> > and the show method for the physical row map and call them from the
> > hammer driver.
> >
> 
> I worry about builtin vs. modular drivers so probably ought to make some

Just make hid-hammer depend on hid-vivaldi, hid-vivaldi is tiny
otherwise.

> hid-vivaldi-common.c file that has the physmap code and then have both
> drivers call that mini-library. The 'vivaldi_data' structure would need
> to be figured out too. The hammer driver stores 'hammer_kbd_leds' in the
> hid_get_drvdata() whereas the vivaldi driver stores 'vivalid_data' so we
> can't simply call the show method for the sysfs attribute without some
> minor surgery.

Yeah, we'll need to convert them into helpers into which callers can
pass buffers.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-01-14 21:40 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-07  9:13 [PATCH v2] HID: google: modify HID device groups of eel Zhengqiao Xia
2022-01-07  9:31 ` Zhengqiao Xia
2022-01-14  8:38 ` Jiri Kosina
2022-01-14 19:25   ` Stephen Boyd
2022-01-14 19:59     ` Dmitry Torokhov
2022-01-14 20:55       ` Stephen Boyd
2022-01-14 21:40         ` Dmitry Torokhov
2022-01-14 11:33 ` Re " xiazhengqiao

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.