| Message ID | 20221130231936.1666390-1-wonchung@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | platform/chrome: Create new USB driver for RGB keyboard in ChromeOS devices | expand |
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
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.
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).
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).
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
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");
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