Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Pavel Machek <pavel@ucw.cz>]

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

Hi!

> >>Basically the charge led has 3 states:
> >>
> >>1) Off
> >>2) On
> >>3) On when charging
> >>
> >>And then when on it has 4 patterns:
> >>
> >>1) Permanently off (so still not really on)
> >>2) Permanently on
> >>3) Blinking
> >>4) Breathing
> >
> >Ok, so you don't really need to support _both_ off methods.
> >
> >Still sounds like a normal LED, with special "yoga-charging" and
> >"yoga-breathing" triggers. (All the normal triggers should still work,
> >too.)
> 
> Except that when in yoga-charging mode, the user should
> still be able to select if the LED is simply on when charging,
> blinking when charging, or breathing when charging.
> 
> Given that there are 2 independent settings model-ing this
> as a trigger does not work well. Also the trigger names should
> not contain yoga as the PMIC in question is used in other
> devices too.
> 
> I really think we should not try to shoe-horn special cases
> like this into the generic API and just use custom sysfs
> attributes for this. Like for example how the kbd-backlight

This is not really that much of a special case. I have LEDs on
thinkpads that can be either hardware controlled or sw controlled with
various functions. Both Droid 4 and N900 have hardware support for
showing charging state on the notification LED. N900 additionaly has
debug functionality for keyboard backlight...

> led classdev from the dell-laptop has custom attributes to
> select if the timeout for going off on keyboard/mouse activity
> and another custom attribute to select if only the keyboard or
> both the keyboard and mouse count as relevant activity.

Yeah well more stuff to fix :-(.

> Triggers are great for when we actually want to add a link
> between an event coming from some part of code in the kernel,
> to a LED classdevice, so that that event can control the LED.

Well, triggers are also useful because userspace should already know
about them.... and because your hardware already behaves as if it had
"trigger" implemented in hardware...

> >Anyway, in such case I'd propose having rmmod/reboot/poweroff hook
> >that just sets it to breathing. No need to expose it to userspace.
> 
> That assumes that breathing is the default setting on all hardware
> and again I don't see why not to export this functionality to

Save the status on boot, then restore it on rmmod/reboot/poweroff? :-).

> userspace. Just because something does not fit in the existing
> API is IMHO not a good reason to not expose it to userspace.
> 
> I suggest that we deal with this special case by adding 3 custom
> sysfs attributes:
> 
> 1) "mode" which when read, prints, e.g. :
> manual [on-when-charging]

While this allows _user on console_ to control everything using echo,
it is not suitable for applications trying to control LEDs.

As there's nothing special about the case here, I believe we should
have generic solution here.

My preffered solution would be "hardware" trigger that leaves the LED
in hardware control.

Alternatively I could imagine "hardware" sysfs attribute, containing
0/1, with 0==software controlled, 1==hardware controlled.

The rest of attributes would have to be Cove-specific, yes (but still
should fit with the rest of LED subsystem).

									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 --]