Re: [PATCH] platform/chrome: Add Wilco EC keyboard backlight LEDs support

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 2323 bytes --]

On Mon 2019-03-11 09:36:20, Nick Crews wrote:
> On Fri, Mar 8, 2019 at 3:13 PM Pavel Machek <pavel@ucw.cz> wrote:
> >
> > Hi!
> >
> > > > This patch is meant to be applied on top of the for-next
> > > > branch of the platform/chrome repository, as it uses some of
> > > > the code staged there.
> > > >
> > > > The EC is in charge of controlling the keyboard backlight on
> > > > the Wilco platform. We expose a standard LED class device at
> > > > /sys/class/leds/wilco::kbd_backlight. This driver is modeled
> > > > after the standard Chrome OS keyboard backlight driver at
> > > > drivers/platform/chrome/cros_kbd_led_backlight.c
> > > >
> > > > Some Wilco devices do not support a keyboard backlight. This
> > > > is checked in probe(), and in this case the sysfs entry will
> > > > not appear, and everything will behave normally.
> > >
> > > It would be even better if we did not register platform device if EC
> > > does not support backlight.
> 
> Good point Dmitry. That would require making the core driver
> dependent upon this keyboard backlight driver, though. Do you
> think that added complexity is worth it? I don't see performance
> concerns with adding one unused platform device, so is your rationale
> that it would just be cleaner code?
> 

You probably wanted to mail dmitry. You should have probably cc-ed
Dmitry and the lists.

> > > > +       data->led.brightness_set = keyboard_led_set_brightness;
> > > > +       data->led.brightness_get = keyboard_led_get_brightness;
> > >
> > > wilco_ec_mailbox() may sleep, so you need to assign it to
> > > led.brightness_set_blocking.
> >
> > Hmm. Seeing get method there... can the EC change the brightness
> > without command from kernel?
> 
> No, the EC won't change the brightness without getting told to by
> the kernel. Do you think that means that we could get away with just
> caching the brightness from the previous set_brightness(), and not
> actually sending a mailbox) command to the EC on get_brightness()?

Something like that. Even better ... don't implement brightness_get at
all, and check that core does the right thing checking it.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]