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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 14-02-19 12:14, Pavel Machek wrote:
> Hi!

<snip>

>> 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? :-).

Which works until the system freezes one time. I believe that
if we are going to do a LED driver for the charging LED on these
devices, we MUST offer a way to put it back in its original
state, even if the state is foo-barred at bootup.

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

As you explained in the parts which I snipped, there are many
devices which have a similar choice for a LED being under hw or
user control. I can see how this looks like a trigger and how we
could use the trigger API for this.

I believe though, that if we implement a "virtual" (for lack of
a better word) trigger for this, that this should be done in the
LED core. I can envision this working like this:

1) Add a:

hw_control_set(struct led_classdev *led_cdev, bool enable_hw_control);

Callback to struct led_classdev which when implemented by a driver
like the current PMIC LED controller would do what it says.

2) Have the core create and register a virtual hardware trigger the
first time a LED cdev which has this callback gets registered.

When configured as the trigger for this LED device this trigger calls
hw_control_set(cdev, true) and when unregistered calls
hw_control_set(cdev, false)

Taking a quick look at the trigger code, a problem with this is
that normally any trigger can work with any led device, so this
"hardware" trigger will show up in the list of possible
triggers for each device.

This problem can be solved by making the activate method for the
hardware trigger check the classdev has a hw_control_set callback
and if not return -EINVAL, or maybe -ENXIO but still this is somewhat
inconsistent with other triggers, which AFAIK work with any LED.

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

Hmm, maybe call it "hardware_controlled" instead ? Otherwise this
would work for me and I would personally prefer this solution. This
could even be done in the LED core using the hw_control_set callback
I proposed, to make sure it is handled consistently between devices.

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

Right, I see that the triggers attribute already uses the fmt where
on "cat" all options are listed and the current active one has [] around it,
so I think the pattern and frequency attributes I proposed should work
well, although thinking more about this I believe the freq. attribute should
be called pattern_freq to make clear it applies to blinking / breathing
set through the pattern attribute.

Regards,

Hans