linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] HID: google: whiskers: more robust tablet mode detection
@ 2019-09-13 22:03 Dmitry Torokhov
  2019-09-13 22:03 ` [PATCH 2/3] HID: google: whiskers: signal tablet mode switch on disconnect Dmitry Torokhov
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Dmitry Torokhov @ 2019-09-13 22:03 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires
  Cc: Nicolas Boichat, Benson Leung, linux-input, linux-kernel

The USB interface may get detected before the platform/EC one, so let's
note the state of the base (if we receive event) and use it to correctly
initialize the tablet mode switch state.

Also let's start the HID interface immediately when probing, this will
ensure that we correctly process "base folded" events that may be sent
as we initialize the base. Note that this requires us to add a release()
function where we stop and close the hardware and switch the LED
registration away from devm interface as we need to make sure that we
destroy the LED instance before we stop the hardware.

Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
---
 drivers/hid/hid-google-hammer.c | 71 ++++++++++++++++++++++++++-------
 1 file changed, 56 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/hid-google-hammer.c b/drivers/hid/hid-google-hammer.c
index 84f8c127ebdc..4f64f93ddfcb 100644
--- a/drivers/hid/hid-google-hammer.c
+++ b/drivers/hid/hid-google-hammer.c
@@ -35,6 +35,7 @@ struct cbas_ec {
 	struct device *dev;	/* The platform device (EC) */
 	struct input_dev *input;
 	bool base_present;
+	bool base_folded;
 	struct notifier_block notifier;
 };
 
@@ -208,7 +209,14 @@ static int __cbas_ec_probe(struct platform_device *pdev)
 		return error;
 	}
 
-	input_report_switch(input, SW_TABLET_MODE, !cbas_ec.base_present);
+	if (!cbas_ec.base_present)
+		cbas_ec.base_folded = false;
+
+	dev_dbg(&pdev->dev, "%s: base: %d, folded: %d\n", __func__,
+		cbas_ec.base_present, cbas_ec.base_folded);
+
+	input_report_switch(input, SW_TABLET_MODE,
+			    !cbas_ec.base_present || cbas_ec.base_folded);
 
 	cbas_ec_set_input(input);
 
@@ -322,10 +330,9 @@ static int hammer_kbd_brightness_set_blocking(struct led_classdev *cdev,
 static int hammer_register_leds(struct hid_device *hdev)
 {
 	struct hammer_kbd_leds *kbd_backlight;
+	int error;
 
-	kbd_backlight = devm_kzalloc(&hdev->dev,
-				     sizeof(*kbd_backlight),
-				     GFP_KERNEL);
+	kbd_backlight = kzalloc(sizeof(*kbd_backlight), GFP_KERNEL);
 	if (!kbd_backlight)
 		return -ENOMEM;
 
@@ -339,7 +346,26 @@ static int hammer_register_leds(struct hid_device *hdev)
 	/* Set backlight to 0% initially. */
 	hammer_kbd_brightness_set_blocking(&kbd_backlight->cdev, 0);
 
-	return devm_led_classdev_register(&hdev->dev, &kbd_backlight->cdev);
+	error = led_classdev_register(&hdev->dev, &kbd_backlight->cdev);
+	if (error)
+		goto err_free_mem;
+
+	hid_set_drvdata(hdev, kbd_backlight);
+	return 0;
+
+err_free_mem:
+	kfree(kbd_backlight);
+	return error;
+}
+
+static void hammer_unregister_leds(struct hid_device *hdev)
+{
+	struct hammer_kbd_leds *kbd_backlight = hid_get_drvdata(hdev);
+
+	if (kbd_backlight) {
+		led_classdev_unregister(&kbd_backlight->cdev);
+		kfree(kbd_backlight);
+	}
 }
 
 #define HID_UP_GOOGLEVENDOR	0xffd10000
@@ -376,8 +402,9 @@ static int hammer_event(struct hid_device *hid, struct hid_field *field,
 	    usage->hid == WHISKERS_KBD_FOLDED) {
 		spin_lock_irqsave(&cbas_ec_lock, flags);
 
+		cbas_ec.base_folded = value;
 		hid_dbg(hid, "%s: base: %d, folded: %d\n", __func__,
-			cbas_ec.base_present, value);
+			cbas_ec.base_present, cbas_ec.base_folded);
 
 		/*
 		 * We should not get event if base is detached, but in case
@@ -436,6 +463,14 @@ static int hammer_probe(struct hid_device *hdev,
 {
 	int error;
 
+	error = hid_parse(hdev);
+	if (error)
+		return error;
+
+	error = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	if (error)
+		return error;
+
 	/*
 	 * We always want to poll for, and handle tablet mode events from
 	 * Whiskers, even when nobody has opened the input device. This also
@@ -443,16 +478,12 @@ static int hammer_probe(struct hid_device *hdev,
 	 * the device.
 	 */
 	if (hdev->product == USB_DEVICE_ID_GOOGLE_WHISKERS &&
-			hammer_is_keyboard_interface(hdev))
+	    hammer_is_keyboard_interface(hdev)) {
 		hdev->quirks |= HID_QUIRK_ALWAYS_POLL;
-
-	error = hid_parse(hdev);
-	if (error)
-		return error;
-
-	error = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
-	if (error)
-		return error;
+		error = hid_hw_open(hdev);
+		if (error)
+			return error;
+	}
 
 	if (hammer_has_backlight_control(hdev)) {
 		error = hammer_register_leds(hdev);
@@ -465,6 +496,15 @@ static int hammer_probe(struct hid_device *hdev,
 	return 0;
 }
 
+static void hammer_remove(struct hid_device *hdev)
+{
+	if (hdev->product == USB_DEVICE_ID_GOOGLE_WHISKERS &&
+			hammer_is_keyboard_interface(hdev))
+		hid_hw_close(hdev);
+
+	hammer_unregister_leds(hdev);
+	hid_hw_stop(hdev);
+}
 
 static const struct hid_device_id hammer_devices[] = {
 	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
@@ -483,6 +523,7 @@ static struct hid_driver hammer_driver = {
 	.name = "hammer",
 	.id_table = hammer_devices,
 	.probe = hammer_probe,
+	.remove = hammer_remove,
 	.input_mapping = hammer_input_mapping,
 	.event = hammer_event,
 };
-- 
2.23.0.237.gc6a4ce50a0-goog


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

* [PATCH 2/3] HID: google: whiskers: signal tablet mode switch on disconnect
  2019-09-13 22:03 [PATCH 1/3] HID: google: whiskers: more robust tablet mode detection Dmitry Torokhov
@ 2019-09-13 22:03 ` Dmitry Torokhov
  2019-09-13 22:03 ` [PATCH 3/3] HID: google: whiskers: signal tablet mode on connect Dmitry Torokhov
  2019-09-16  3:26 ` [PATCH 1/3] HID: google: whiskers: more robust tablet mode detection Nicolas Boichat
  2 siblings, 0 replies; 7+ messages in thread
From: Dmitry Torokhov @ 2019-09-13 22:03 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires
  Cc: Nicolas Boichat, Benson Leung, linux-input, linux-kernel

Currently, the tablet mode switch that takes two signals as its input:
base attached switch from the EC and some GMR signal from whiskers when
it's folded over. This tablet mode switch is then sent to Chrome, which
changes the UI.

However, there are some units which have a lot of leakage on the ADC
pins that the EC uses to determine whether or not a base is attached.
This can result in the base being physically detached, but the EC
thinking that it's still attached. The user would then be stuck in
laptop mode and wouldn't be able to rotate their display.

To work around this let's send "tablet mode" signal when we remove HID
interface, which normally happens when we physically disconnect
whiskers.

Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
---
 drivers/hid/hid-google-hammer.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-google-hammer.c b/drivers/hid/hid-google-hammer.c
index 4f64f93ddfcb..3dc6116c8f76 100644
--- a/drivers/hid/hid-google-hammer.c
+++ b/drivers/hid/hid-google-hammer.c
@@ -498,11 +498,33 @@ static int hammer_probe(struct hid_device *hdev,
 
 static void hammer_remove(struct hid_device *hdev)
 {
+	unsigned long flags;
+
 	if (hdev->product == USB_DEVICE_ID_GOOGLE_WHISKERS &&
-			hammer_is_keyboard_interface(hdev))
+			hammer_is_keyboard_interface(hdev)) {
 		hid_hw_close(hdev);
 
+		/*
+		 * If we are disconnecting then most likely Whiskers is
+		 * being removed. Even if it is not removed, without proper
+		 * keyboard we should not stay in clamshell mode.
+		 *
+		 * The reason for doing it here and not waiting for signal
+		 * from EC, is that on some devices there are high leakage
+		 * on Whiskers pins and we do not detect disconnect reliably,
+		 * resulting in devices being stuck in clamshell mode.
+		 */
+		spin_lock_irqsave(&cbas_ec_lock, flags);
+		if (cbas_ec.input && cbas_ec.base_present) {
+			input_report_switch(cbas_ec.input, SW_TABLET_MODE, 1);
+			input_sync(cbas_ec.input);
+		}
+		cbas_ec.base_present = false;
+		spin_unlock_irqrestore(&cbas_ec_lock, flags);
+	}
+
 	hammer_unregister_leds(hdev);
+
 	hid_hw_stop(hdev);
 }
 
-- 
2.23.0.237.gc6a4ce50a0-goog


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

* [PATCH 3/3] HID: google: whiskers: signal tablet mode on connect
  2019-09-13 22:03 [PATCH 1/3] HID: google: whiskers: more robust tablet mode detection Dmitry Torokhov
  2019-09-13 22:03 ` [PATCH 2/3] HID: google: whiskers: signal tablet mode switch on disconnect Dmitry Torokhov
@ 2019-09-13 22:03 ` Dmitry Torokhov
  2019-09-16  3:26 ` [PATCH 1/3] HID: google: whiskers: more robust tablet mode detection Nicolas Boichat
  2 siblings, 0 replies; 7+ messages in thread
From: Dmitry Torokhov @ 2019-09-13 22:03 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires
  Cc: Nicolas Boichat, Benson Leung, linux-input, linux-kernel

When we receive "keyboard position" event from Whiskers we can
conclude that the base is attached, even if we did not get EC event
for that. We may miss EC event because there are some units which
have a lot of leakage on the ADC pins that the EC uses to determine
whether or not a base is attached.

Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
---
 drivers/hid/hid-google-hammer.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-google-hammer.c b/drivers/hid/hid-google-hammer.c
index 3dc6116c8f76..31e4a39946f5 100644
--- a/drivers/hid/hid-google-hammer.c
+++ b/drivers/hid/hid-google-hammer.c
@@ -402,16 +402,16 @@ static int hammer_event(struct hid_device *hid, struct hid_field *field,
 	    usage->hid == WHISKERS_KBD_FOLDED) {
 		spin_lock_irqsave(&cbas_ec_lock, flags);
 
+		/*
+		 * If we are getting events from Whiskers that means that it
+		 * is attached to the lid.
+		 */
+		cbas_ec.base_present = true;
 		cbas_ec.base_folded = value;
 		hid_dbg(hid, "%s: base: %d, folded: %d\n", __func__,
 			cbas_ec.base_present, cbas_ec.base_folded);
 
-		/*
-		 * We should not get event if base is detached, but in case
-		 * we happen to service HID and EC notifications out of order
-		 * let's still check the "base present" flag.
-		 */
-		if (cbas_ec.input && cbas_ec.base_present) {
+		if (cbas_ec.input) {
 			input_report_switch(cbas_ec.input,
 					    SW_TABLET_MODE, value);
 			input_sync(cbas_ec.input);
-- 
2.23.0.237.gc6a4ce50a0-goog


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

* Re: [PATCH 1/3] HID: google: whiskers: more robust tablet mode detection
  2019-09-13 22:03 [PATCH 1/3] HID: google: whiskers: more robust tablet mode detection Dmitry Torokhov
  2019-09-13 22:03 ` [PATCH 2/3] HID: google: whiskers: signal tablet mode switch on disconnect Dmitry Torokhov
  2019-09-13 22:03 ` [PATCH 3/3] HID: google: whiskers: signal tablet mode on connect Dmitry Torokhov
@ 2019-09-16  3:26 ` Nicolas Boichat
  2019-09-16  3:36   ` Dmitry Torokhov
  2 siblings, 1 reply; 7+ messages in thread
From: Nicolas Boichat @ 2019-09-16  3:26 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Jiri Kosina, Benjamin Tissoires, Benson Leung,
	open list:HID CORE LAYER, lkml

On Sat, Sep 14, 2019 at 6:03 AM Dmitry Torokhov <dtor@chromium.org> wrote:
>
> The USB interface may get detected before the platform/EC one, so let's
> note the state of the base (if we receive event) and use it to correctly
> initialize the tablet mode switch state.
>
> Also let's start the HID interface immediately when probing, this will
> ensure that we correctly process "base folded" events that may be sent
> as we initialize the base. Note that this requires us to add a release()

s/release/remove/ ?

> function where we stop and close the hardware and switch the LED
> registration away from devm interface as we need to make sure that we
> destroy the LED instance before we stop the hardware.
>
> Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
> ---
>  drivers/hid/hid-google-hammer.c | 71 ++++++++++++++++++++++++++-------
>  1 file changed, 56 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hid/hid-google-hammer.c b/drivers/hid/hid-google-hammer.c
> index 84f8c127ebdc..4f64f93ddfcb 100644
> --- a/drivers/hid/hid-google-hammer.c
> +++ b/drivers/hid/hid-google-hammer.c
> @@ -208,7 +209,14 @@ static int __cbas_ec_probe(struct platform_device *pdev)
>                 return error;
>         }
>
> -       input_report_switch(input, SW_TABLET_MODE, !cbas_ec.base_present);
> +       if (!cbas_ec.base_present)
> +               cbas_ec.base_folded = false;

I'm not sure to see why this is necessary? The folded base state
should be in bss and false anyway, and even if it was true, it would
not change the result of the expression below (!cbas_ec.base_present
|| cbas_ec.base_folded).

> +
> +       dev_dbg(&pdev->dev, "%s: base: %d, folded: %d\n", __func__,
> +               cbas_ec.base_present, cbas_ec.base_folded);
> +
> +       input_report_switch(input, SW_TABLET_MODE,
> +                           !cbas_ec.base_present || cbas_ec.base_folded);
>
>         cbas_ec_set_input(input);
>

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

* Re: [PATCH 1/3] HID: google: whiskers: more robust tablet mode detection
  2019-09-16  3:26 ` [PATCH 1/3] HID: google: whiskers: more robust tablet mode detection Nicolas Boichat
@ 2019-09-16  3:36   ` Dmitry Torokhov
  2019-09-23  9:09     ` Jiri Kosina
  0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Torokhov @ 2019-09-16  3:36 UTC (permalink / raw)
  To: Nicolas Boichat
  Cc: Jiri Kosina, Benjamin Tissoires, Benson Leung,
	open list:HID CORE LAYER, lkml

On Sun, Sep 15, 2019 at 8:26 PM Nicolas Boichat <drinkcat@chromium.org> wrote:
>
> On Sat, Sep 14, 2019 at 6:03 AM Dmitry Torokhov <dtor@chromium.org> wrote:
> >
> > The USB interface may get detected before the platform/EC one, so let's
> > note the state of the base (if we receive event) and use it to correctly
> > initialize the tablet mode switch state.
> >
> > Also let's start the HID interface immediately when probing, this will
> > ensure that we correctly process "base folded" events that may be sent
> > as we initialize the base. Note that this requires us to add a release()
>
> s/release/remove/ ?

You are right.

>
> > function where we stop and close the hardware and switch the LED
> > registration away from devm interface as we need to make sure that we
> > destroy the LED instance before we stop the hardware.
> >
> > Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
> > ---
> >  drivers/hid/hid-google-hammer.c | 71 ++++++++++++++++++++++++++-------
> >  1 file changed, 56 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/hid/hid-google-hammer.c b/drivers/hid/hid-google-hammer.c
> > index 84f8c127ebdc..4f64f93ddfcb 100644
> > --- a/drivers/hid/hid-google-hammer.c
> > +++ b/drivers/hid/hid-google-hammer.c
> > @@ -208,7 +209,14 @@ static int __cbas_ec_probe(struct platform_device *pdev)
> >                 return error;
> >         }
> >
> > -       input_report_switch(input, SW_TABLET_MODE, !cbas_ec.base_present);
> > +       if (!cbas_ec.base_present)
> > +               cbas_ec.base_folded = false;
>
> I'm not sure to see why this is necessary? The folded base state
> should be in bss and false anyway, and even if it was true, it would
> not change the result of the expression below (!cbas_ec.base_present
> || cbas_ec.base_folded).

To have a more accurate printout generated by the line below. The fact
that it is in bss it not relevant, as I can unbind and rebind the
driver via sysfs, so it could have stale data from the previous run.

>
> > +
> > +       dev_dbg(&pdev->dev, "%s: base: %d, folded: %d\n", __func__,
> > +               cbas_ec.base_present, cbas_ec.base_folded);
> > +
> > +       input_report_switch(input, SW_TABLET_MODE,
> > +                           !cbas_ec.base_present || cbas_ec.base_folded);
> >
> >         cbas_ec_set_input(input);
> >

Thanks,
Dmitry

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

* Re: [PATCH 1/3] HID: google: whiskers: more robust tablet mode detection
  2019-09-16  3:36   ` Dmitry Torokhov
@ 2019-09-23  9:09     ` Jiri Kosina
  2019-10-01 14:16       ` Jiri Kosina
  0 siblings, 1 reply; 7+ messages in thread
From: Jiri Kosina @ 2019-09-23  9:09 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Nicolas Boichat, Benjamin Tissoires, Benson Leung,
	open list:HID CORE LAYER, lkml

On Sun, 15 Sep 2019, Dmitry Torokhov wrote:

> > > The USB interface may get detected before the platform/EC one, so let's
> > > note the state of the base (if we receive event) and use it to correctly
> > > initialize the tablet mode switch state.
> > >
> > > Also let's start the HID interface immediately when probing, this will
> > > ensure that we correctly process "base folded" events that may be sent
> > > as we initialize the base. Note that this requires us to add a release()
> >
> > s/release/remove/ ?
> 
> You are right.

I'll fix that up when I am applying it (once 5.4 merge window is over).

Thanks,

-- 
Jiri Kosina
SUSE Labs


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

* Re: [PATCH 1/3] HID: google: whiskers: more robust tablet mode detection
  2019-09-23  9:09     ` Jiri Kosina
@ 2019-10-01 14:16       ` Jiri Kosina
  0 siblings, 0 replies; 7+ messages in thread
From: Jiri Kosina @ 2019-10-01 14:16 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Nicolas Boichat, Benjamin Tissoires, Benson Leung,
	open list:HID CORE LAYER, lkml

On Mon, 23 Sep 2019, Jiri Kosina wrote:

> > > > The USB interface may get detected before the platform/EC one, so let's
> > > > note the state of the base (if we receive event) and use it to correctly
> > > > initialize the tablet mode switch state.
> > > >
> > > > Also let's start the HID interface immediately when probing, this will
> > > > ensure that we correctly process "base folded" events that may be sent
> > > > as we initialize the base. Note that this requires us to add a release()
> > >
> > > s/release/remove/ ?
> > 
> > You are right.
> 
> I'll fix that up when I am applying it (once 5.4 merge window is over).

Now applied (with the fixup made). Thanks,

-- 
Jiri Kosina
SUSE Labs


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

end of thread, other threads:[~2019-10-01 14:16 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-13 22:03 [PATCH 1/3] HID: google: whiskers: more robust tablet mode detection Dmitry Torokhov
2019-09-13 22:03 ` [PATCH 2/3] HID: google: whiskers: signal tablet mode switch on disconnect Dmitry Torokhov
2019-09-13 22:03 ` [PATCH 3/3] HID: google: whiskers: signal tablet mode on connect Dmitry Torokhov
2019-09-16  3:26 ` [PATCH 1/3] HID: google: whiskers: more robust tablet mode detection Nicolas Boichat
2019-09-16  3:36   ` Dmitry Torokhov
2019-09-23  9:09     ` Jiri Kosina
2019-10-01 14:16       ` Jiri Kosina

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).