mbox series

[v2,0/5] ChromeOS Embedded Controller LED driver

Message ID 20240531-cros_ec-led-v2-0-6cc34408b40d@weissschuh.net
Headers show
Series ChromeOS Embedded Controller LED driver | expand

Message

Thomas Weißschuh May 31, 2024, 4:33 p.m. UTC
Add a LED driver that supports the LED devices exposed by the
ChromeOS Embedded Controller.

Patch 1-3 add a utility function to the led subsystem.
Patch 4 introduces the actual driver.
Patch 5 registers the driver through the cros_ec mfd devices.

Currently the driver introduces some non-standard LED functions.
(See "cros_ec_led_functions")

Tested on a Framework 13 AMD, Firmware 3.05.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Changes in v2:
- Cosmetic cleanups (Tzung-Bi)
- Add trailing comma to MFD cell array
- Rename LEDs and trigger to "chromeos" prefix, to align with kbd
  backlight driver
- Don't use type "rgb" anymore, they are only "multicolor"
- Align commit messages and subject to subsystem standards (Lee)
- Rename led_color_name() to led_get_color_name()
- The same for cros_ec_led_color_name()
- Link to v1: https://lore.kernel.org/r/20240520-cros_ec-led-v1-0-4068fc5c051a@weissschuh.net

---
Thomas Weißschuh (5):
      leds: core: Introduce led_get_color_name() function
      leds: multicolor: Use led_get_color_name() function
      leds: core: Unexport led_colors[] array
      leds: Add ChromeOS EC driver
      mfd: cros_ec: Register LED subdevice

 MAINTAINERS                         |   5 +
 drivers/leds/Kconfig                |  15 ++
 drivers/leds/Makefile               |   1 +
 drivers/leds/led-class-multicolor.c |   2 +-
 drivers/leds/led-core.c             |  12 +-
 drivers/leds/leds-cros_ec.c         | 297 ++++++++++++++++++++++++++++++++++++
 drivers/leds/leds.h                 |   1 -
 drivers/mfd/cros_ec_dev.c           |   9 ++
 include/linux/leds.h                |  10 ++
 9 files changed, 348 insertions(+), 4 deletions(-)
---
base-commit: 4a4be1ad3a6efea16c56615f31117590fd881358
change-id: 20240519-cros_ec-led-3efa24e3991e

Best regards,

Comments

Dustin L. Howett June 2, 2024, 11:30 p.m. UTC | #1
On Fri, May 31, 2024 at 11:33 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> Add a LED driver that supports the LED devices exposed by the
> ChromeOS Embedded Controller.

I've tested this out on the Framework Laptop 13, 11th gen intel core
and AMD Ryzen 7040 editions.

It works fairly well! I found a couple minor issues in day-to-day use:

- Restoring the trigger to chromeos-auto does not always put the EC
back in control, e.g. the side lights no longer return to reporting
charge status.
  I believe this happens when you move from any trigger except "none"
to chromeos-auto, without first setting "none".
- The multicolor intensities report 6x 100 by default; if you set the
brightness with the intensities set as such, it becomes only red. I
would have
  expected intensities of 100 0 0 0 0 0 if that were the case

Thomas, I apologize for the duplicate message; my mail client config
here defaults to "reply" rather than "reply all."

Thanks,
Dustin

>
> Best regards,
> --
> Thomas Weißschuh <linux@weissschuh.net>
>
Thomas Weißschuh June 3, 2024, 8:56 p.m. UTC | #2
On 2024-06-02 18:30:06+0000, Dustin Howett wrote:
> On Fri, May 31, 2024 at 11:33 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
> >
> > Add a LED driver that supports the LED devices exposed by the
> > ChromeOS Embedded Controller.
> 
> I've tested this out on the Framework Laptop 13, 11th gen intel core
> and AMD Ryzen 7040 editions.
> 
> It works fairly well! I found a couple minor issues in day-to-day use:

Thanks!

> - Restoring the trigger to chromeos-auto does not always put the EC
> back in control, e.g. the side lights no longer return to reporting
> charge status.
>   I believe this happens when you move from any trigger except "none"
> to chromeos-auto, without first setting "none".

Thanks for the report, I'll investigate that.

> - The multicolor intensities report 6x 100 by default; if you set the
> brightness with the intensities set as such, it becomes only red. I
> would have
>   expected intensities of 100 0 0 0 0 0 if that were the case

The EC will always use the first nonzero intensity for the color
channel. It isn't a real PWM color mix.
I don't think there are possibilities in the multicolor API to enforce
this. For the next revision I'll need to document this properly.
Also the default intensities could be better indeed.

> Thomas, I apologize for the duplicate message; my mail client config
> here defaults to "reply" rather than "reply all."

No worries!
Lee Jones June 13, 2024, 2:41 p.m. UTC | #3
On Mon, 03 Jun 2024, Thomas Weißschuh wrote:

> On 2024-06-02 18:30:06+0000, Dustin Howett wrote:
> > On Fri, May 31, 2024 at 11:33 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
> > >
> > > Add a LED driver that supports the LED devices exposed by the
> > > ChromeOS Embedded Controller.
> > 
> > I've tested this out on the Framework Laptop 13, 11th gen intel core
> > and AMD Ryzen 7040 editions.
> > 
> > It works fairly well! I found a couple minor issues in day-to-day use:
> 
> Thanks!
> 
> > - Restoring the trigger to chromeos-auto does not always put the EC
> > back in control, e.g. the side lights no longer return to reporting
> > charge status.
> >   I believe this happens when you move from any trigger except "none"
> > to chromeos-auto, without first setting "none".
> 
> Thanks for the report, I'll investigate that.

So am I reviewing this set or waiting for the next version?