[PATCH] platform/chrome: Create new USB driver for RGB keyboard in ChromeOS devices

[PATCH] platform/chrome: Create new USB driver for RGB keyboard in ChromeOS devices

[Won Chung]

Without any driver bound to RGB keyboard, it may not be suspended
properly, preventing USB xHCI to be suspended and causing power drain.
Create new USB driver for RGB keyboard so that it can be suspended
properly.

Signed-off-by: Won Chung <wonchung@google.com>
---
 drivers/platform/chrome/Kconfig             |  8 ++++
 drivers/platform/chrome/Makefile            |  1 +
 drivers/platform/chrome/cros_rgb_keyboard.c | 44 +++++++++++++++++++++
 3 files changed, 53 insertions(+)
 create mode 100644 drivers/platform/chrome/cros_rgb_keyboard.c

diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index c45fb376d653..a7c36df99432 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -265,6 +265,14 @@ config CHROMEOS_PRIVACY_SCREEN
 	  this should probably always be built into the kernel to avoid or
 	  minimize drm probe deferral.
 
+config CROS_RGB_KEYBOARD
+	tristate "ChromeOS RGB keyboard"
+	depends on USB
+	help
+	  This driver supports RGB keyboard in some ChromeOS devices. This shall be
+	  enabled if RGB keyboard is present, otherwise it may not be suspended
+	  properly.
+
 source "drivers/platform/chrome/wilco_ec/Kconfig"
 
 # Kunit test cases
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index f7e74a845afc..e4ffa17c57fc 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_CROS_EC_SENSORHUB)		+= cros-ec-sensorhub.o
 obj-$(CONFIG_CROS_EC_SYSFS)		+= cros_ec_sysfs.o
 obj-$(CONFIG_CROS_USBPD_LOGGER)		+= cros_usbpd_logger.o
 obj-$(CONFIG_CROS_USBPD_NOTIFY)		+= cros_usbpd_notify.o
+obj-$(CONFIG_CROS_RGB_KEYBOARD)		+= cros_rgb_keyboard.o
 
 obj-$(CONFIG_WILCO_EC)			+= wilco_ec/
 
diff --git a/drivers/platform/chrome/cros_rgb_keyboard.c b/drivers/platform/chrome/cros_rgb_keyboard.c
new file mode 100644
index 000000000000..1d53fc832d76
--- /dev/null
+++ b/drivers/platform/chrome/cros_rgb_keyboard.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0
+// RGB keyboard driver for ChromeOS
+//
+// Copyright (C) 2022 Google, Inc.
+
+#include <linux/module.h>
+#include <linux/usb.h>
+
+#define DRV_NAME "cros-rgb-keyboard"
+
+/* vendor ID */
+#define ID_GOOGLE 0x18d1
+/* product ID */
+#define ID_PRISM 0x5022
+
+static const struct usb_device_id cros_rgb_keyboard_table[] = {
+	{USB_DEVICE(ID_GOOGLE, ID_PRISM)},
+	{} // terminating null entry
+};
+
+static int cros_rgb_keyboard_probe(struct usb_interface *interface, const struct usb_device_id *id)
+{
+	struct usb_device *udev = interface_to_usbdev(interface);
+
+	usb_enable_autosuspend(udev);
+	return 0;
+}
+
+static void cros_rgb_keyboard_disconnect(struct usb_interface *interface)
+{
+}
+
+/* usb specific object needed to register this driver with the usb subsystem */
+static struct usb_driver cros_rgb_keyboard_driver = {
+	.name = DRV_NAME,
+	.id_table = cros_rgb_keyboard_table,
+	.probe = cros_rgb_keyboard_probe,
+	.disconnect = cros_rgb_keyboard_disconnect,
+};
+
+module_usb_driver(cros_rgb_keyboard_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ChromeOS RGB keyboard driver");
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

Re: [PATCH] platform/chrome: Create new USB driver for RGB keyboard in ChromeOS devices

[Prashant Malani]

Hi Won,

On Wed, Nov 30, 2022 at 3:19 PM Won Chung <wonchung@google.com> wrote:
>
> Without any driver bound to RGB keyboard, it may not be suspended
> properly, preventing USB xHCI to be suspended and causing power drain.
> Create new USB driver for RGB keyboard so that it can be suspended
> properly.

This seems like overkill. Can't you set this from USB's sysfs nodes
like power/control [1] ?

[1] https://www.kernel.org/doc/html/latest/driver-api/usb/power-management.html#the-user-interface-for-dynamic-pm


Best regards,

-Prashant

Re: [PATCH] platform/chrome: Create new USB driver for RGB keyboard in ChromeOS devices

[Benson Leung]

Hi Prashant,


On Thu, Dec 1, 2022 at 12:10 PM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Won,
>
> On Wed, Nov 30, 2022 at 3:19 PM Won Chung <wonchung@google.com> wrote:
> >
> > Without any driver bound to RGB keyboard, it may not be suspended
> > properly, preventing USB xHCI to be suspended and causing power drain.
> > Create new USB driver for RGB keyboard so that it can be suspended
> > properly.
>
> This seems like overkill. Can't you set this from USB's sysfs nodes
> like power/control [1] ?
>
> [1] https://www.kernel.org/doc/html/latest/driver-api/usb/power-management.html#the-user-interface-for-dynamic-pm
>
>
> Best regards,
>
> -Prashant

We're seeing some behavior where a bound driver is needed in order for
this USB device to properly enter suspend state. Just manipulating the
power/control and other sysfs nodes for this usb device when there's
no driver in the kernel doesn't seem to affect the device's ability to
drop into a usb low power state.

Also, I synced with Won about this offline, but the primary concern is
not this prism usb device runtime suspending, it's actually it's
ability to enter suspend state during system suspend. Right now, this
internal usb device is keeping the whole system from entering lower
S0iX states because it's not sleeping. This driver patch doesn't
address that yet, but I'd like Won to dig down and see if he can get
it suspending at suspend time too.


-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

Re: [PATCH] platform/chrome: Create new USB driver for RGB keyboard in ChromeOS devices

[Prashant Malani]

Hey Benson,

On Thu, Dec 1, 2022 at 1:00 PM Benson Leung <bleung@chromium.org> wrote:
>
> Hi Prashant,
>
>
> On Thu, Dec 1, 2022 at 12:10 PM Prashant Malani <pmalani@chromium.org> wrote:
> >
> > Hi Won,
> >
> > On Wed, Nov 30, 2022 at 3:19 PM Won Chung <wonchung@google.com> wrote:
> > >
> > > Without any driver bound to RGB keyboard, it may not be suspended
> > > properly, preventing USB xHCI to be suspended and causing power drain.
> > > Create new USB driver for RGB keyboard so that it can be suspended
> > > properly.
> >
> > This seems like overkill. Can't you set this from USB's sysfs nodes
> > like power/control [1] ?
> >
> > [1] https://www.kernel.org/doc/html/latest/driver-api/usb/power-management.html#the-user-interface-for-dynamic-pm
> >
> >
> > Best regards,
> >
> > -Prashant
>
> We're seeing some behavior where a bound driver is needed in order for
> this USB device to properly enter suspend state. Just manipulating the
> power/control and other sysfs nodes for this usb device when there's
> no driver in the kernel doesn't seem to affect the device's ability to
> drop into a usb low power state.

That seems like an issue with the device then, which should be debugged
from the device side and/or its interaction with the USB subsystem.

>
> Also, I synced with Won about this offline, but the primary concern is
> not this prism usb device runtime suspending, it's actually it's
> ability to enter suspend state during system suspend. Right now, this
> internal usb device is keeping the whole system from entering lower
> S0iX states because it's not sleeping. This driver patch doesn't
> address that yet, but I'd like Won to dig down and see if he can get
> it suspending at suspend time too.

Auto-suspend / dynamic PM should take care of that FWIU (but I may be mistaken).

Re: [PATCH] platform/chrome: Create new USB driver for RGB keyboard in ChromeOS devices

[Won Chung]

On Thu, Dec 1, 2022 at 1:36 PM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hey Benson,
>
> On Thu, Dec 1, 2022 at 1:00 PM Benson Leung <bleung@chromium.org> wrote:
> >
> > Hi Prashant,
> >
> >
> > On Thu, Dec 1, 2022 at 12:10 PM Prashant Malani <pmalani@chromium.org> wrote:
> > >
> > > Hi Won,
> > >
> > > On Wed, Nov 30, 2022 at 3:19 PM Won Chung <wonchung@google.com> wrote:
> > > >
> > > > Without any driver bound to RGB keyboard, it may not be suspended
> > > > properly, preventing USB xHCI to be suspended and causing power drain.
> > > > Create new USB driver for RGB keyboard so that it can be suspended
> > > > properly.
> > >
> > > This seems like overkill. Can't you set this from USB's sysfs nodes
> > > like power/control [1] ?
> > >
> > > [1] https://www.kernel.org/doc/html/latest/driver-api/usb/power-management.html#the-user-interface-for-dynamic-pm
> > >
> > >
> > > Best regards,
> > >
> > > -Prashant
> >
> > We're seeing some behavior where a bound driver is needed in order for
> > this USB device to properly enter suspend state. Just manipulating the
> > power/control and other sysfs nodes for this usb device when there's
> > no driver in the kernel doesn't seem to affect the device's ability to
> > drop into a usb low power state.
>
> That seems like an issue with the device then, which should be debugged
> from the device side and/or its interaction with the USB subsystem.
>

Hi Prashant,

As Benson mentioned, I can check on my test Vell device that changing
power/control does not suspend the device.
Should it be controllable even without a driver bound?

The RGB keyboard seems to be able to enter sleep with a suspend signal.
Fyi, here are the related bugs for more context: b/242087721 and b/249173368

Thank you,
Won

> >
> > Also, I synced with Won about this offline, but the primary concern is
> > not this prism usb device runtime suspending, it's actually it's
> > ability to enter suspend state during system suspend. Right now, this
> > internal usb device is keeping the whole system from entering lower
> > S0iX states because it's not sleeping. This driver patch doesn't
> > address that yet, but I'd like Won to dig down and see if he can get
> > it suspending at suspend time too.
>
> Auto-suspend / dynamic PM should take care of that FWIU (but I may be mistaken).

Re: [PATCH] platform/chrome: Create new USB driver for RGB keyboard in ChromeOS devices

[Prashant Malani]

On Thu, Dec 1, 2022 at 2:52 PM Won Chung <wonchung@google.com> wrote:
>
> On Thu, Dec 1, 2022 at 1:36 PM Prashant Malani <pmalani@chromium.org> wrote:
> > >
> > > We're seeing some behavior where a bound driver is needed in order for
> > > this USB device to properly enter suspend state. Just manipulating the
> > > power/control and other sysfs nodes for this usb device when there's
> > > no driver in the kernel doesn't seem to affect the device's ability to
> > > drop into a usb low power state.
> >
> > That seems like an issue with the device then, which should be debugged
> > from the device side and/or its interaction with the USB subsystem.
> >
>
> Hi Prashant,
>
> As Benson mentioned, I can check on my test Vell device that changing
> power/control does not suspend the device.
> Should it be controllable even without a driver bound?

Um, it should be(?); usb_enable_autosuspend() [1] and `echo auto >
power/control` [2]
both seem to call the same runtime PM function. I've not looked into
runtime PM code
deeper than that, but it may be worthwhile to examine what's causing the suspend
to get blocked.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/usb/core/driver.c#L1637
[2] https://elixir.bootlin.com/linux/latest/source/drivers/base/power/sysfs.c#L113