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

From: Hans de Goede
Date: Sun Feb 17 2019 - 09:10:56 EST


Hi,

On 2/17/19 1:08 AM, Pavel Machek wrote:

I don't pretend to fully understand it, _but_ hw_pattern should really
describe the pattern LED should do, not whether it reacts to charging
or not.

Then we are back to step 1 of the discussion, that we need another
mechanism outside of the trigger to select if the LED shows the configured
pattern always, or only when the charger is on.

Yep, sorry.

These really are 2 orthogonal settings, there is a pattern which can
be set and the LED can either show that pattern always; or only when
charging the battery. Note that the same pattern is used in both cases.

This is why I previously suggested having a custom sysfs hardware_control
attribute which selects between the "only show pattern when charging"
modes ("hardware_control=1" or "always show the pattern mode"
("hardware_control=0").

I see... and yes, that would be the easiest solution.

But somehow I see "this LED is controlled by charging state" as
primary and "it shows pulses instead of staying on" as secondary
eye-candy.

This week there was another driver for charger LED.. but that one does
not do pulses. Ideally, we'd like consistent interface to the
userland.

(To make it complex, the other driver supports things like:
LED solid on -- fully charged
LED blinking slowly -- charging
LED blinking fast -- charge error
LED off -- not charging).

Something like that could be supported with my original hw_pattern
proposal where we simply encode all of this in the hw-pattern file:

tupple0: charging blinking_on_time
tupple1: charging blinking_off_time
tupple2: charging breathing_time
tupple3: manual blinking_on_time
tupple4: manual blinking_off_time
tupple5: manual breathing_time

So for this chip you mention, we do not need the breathing time (no breathing support),
so we would get the following tupples:

tupple0: not charging blinking_on_time
tupple1: not charging blinking_off_time
tupple2: slow charging blinking_on_time
tupple3: slow charging blinking_off_time
tupple4: fast charging blinking_on_time
tupple5: fast charging blinking_off_time
tupple6: charging error blinking_on_time
tupple7: charging error blinking_off_time

Where by solid on/off can be done by setting one
of the blinking times to 0.

Having hw_pattern ABIs like this where some of
the tupples only activate on certain conditions
might be better then a hardware_control sysfs
file as it offers more flexibility.

Regards,

Hans