Messages in this thread | | | Subject | Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs | From | Jacek Anaszewski <> | Date | Fri, 15 Feb 2019 22:41:31 +0100 |
| |
Hi Pavel,
On 2/14/19 1:28 PM, Pavel Machek wrote: > Hi! > > Jacek, could we get you to comment here? I'd prefer "hardware" trigger...
What prevents use from using pattern trigger with its hw_pattern file?
Do you remember drivers/leds/leds-sc27xx-bltc.c and its breathing mode, for which pattern trigger was introduced?
>>>> 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: > > Agreed about the LED core. > >> 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. > > I guess other option is to modify core so that it does not list > "hardware" trigger for leds that don't support it.
Pattern trigger will not show hw_pattern file if pattern_set/get ops are not provided.
>>> 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. > > This should be in LED core, yes. > >>> 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. > > Take a look at blinking trigger. It can already do hardware > acceleration, but uses different format than what you proposed. > > Best regards, > Pavel >
-- Best regards, Jacek Anaszewski
| |