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

From: Hans de Goede
Date: Sat Feb 16 2019 - 15:56:11 EST


On 2/16/19 8:37 PM, Pavel Machek wrote:

I think that should work fine, which means that we can use the timer and
pattern trigger support for the blinking and breathing modes.

That still leaves the switching between user and hw-control modes,
as discussed the hw-controlled mode could be modelled as a new "hardware"
trigger, but then we cannot choose between on/blink/breathing when
in hw-controlled mode. As Pavel mentioned, that would require some
sort of composed trigger, where we have both the hardware and
timer triggers active for example.

I think it might be easier to just allow turning on/off the hardware
control mode through a special "hardware_control" sysfs attribute and
then use the existing timer and pattern triggers for blinking / breathing.

Pattern trigger exposes pattern file by default and hw_pattern if
pattern_set/get ops are provided. Writing them enables software and
hardware pattern respectively.

This is not about software vs hardware pattern.

There are 2 *orthogonal*, separate problems/challenges with this LED controller:

1) It has hardware blinking and breathing, as discussed this can be
controlled through the timer and pattern triggers, so this problem
is solved.

2) It has 2 operating modes:

a) Automatic/hardware controlled, in this mode the LED is turned
off or on (where on can be continues on, blinking or breathing)
by the hardware itself, when in this mode we / userspace is not
in control of the LED

b) Manual/user controlled mode, in this mode we / userspace can
control of the LED.

Currently there is no API in the ledclass to switch a LED from
automatic controlled to user controlled and back, This is what
the proposed hardware trigger was for, to switch to automatic
mode. A problem with this is that we still want to be able
to chose between continues on, blinking or breathing (when on),
configure the max brightness, etc.

Yes, we do have the API to switch a LED from automatic (hardware
accelerated) control to software control and back. This is pattern
trigger, which exposes two files for setting pattern: pattern
and hw_pattern. Writing pattern file switches the device to software
control mode and writing hw_pattern switches it to the hardware control,
with the possibility of defining device specific ABI syntax to enable
particular pattern (blinking, breathing or event permanently on
in case of this device).

OK, I see. So we would use the hw_pattern for this and the driver
would implement the pattern_set led_classdev callback.

The pattern_set callback would then expect 6 brightness/time tuples
with the following meaning for the time part of each tupple

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

Where only the times in tupple 0-2; or the times in 3-5 can be
non-zero. Having non zero times for both some charging and some
manual values is not allowed.

If a breathing time is set, none of the other times may be non
0. If blinkig_on and blinking_off are used then breathing_time
must be 0.

When configured to blink then blinking_off must be either 0
(continuously on); or it must be the same as blinking_on.

I believe this will work, does this sound ok to you ?

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.

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"