Re: [PATCH v2 1/3] leds: core: Introduce generic pattern interface
Hi! > > Hmm, they are only [brightness duration] tuples, and no definition of > > R, G and B LED device is covered here, so how it can be useful for RGB > > LEDs? > > > > The typical Qualcomm PMIC sports 4-8 LPG channels. The output of these > channels can be configured to some extent, but in theory any combination > of the 8 could be hooked up to the three channels of a RGB LED. > > So looking at the LPG hw block, there's no such thing as RGB. Well, there are certainly RGB leds. And we need kernel to know about them, so user can (for example) ask for white color. > Further more, per the 96boards specification we're expected to have > different triggers on the different "colors" of the TRILED. So the specs is stupid. We may end up allowing that, but lets not introduce a bad design just because of that. [You may get that functionality by lying in the dts and claiming it is three LEDs, not one RGB LED.] You want to be able to set a color of RGB LED... which is not as easy as it looks. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v2 1/3] leds: core: Introduce generic pattern interface
On 07/18/2017 01:39 AM, Bjorn Andersson wrote: > On Mon 17 Jul 14:08 PDT 2017, Jacek Anaszewski wrote: > >> On 07/16/2017 11:14 PM, Bjorn Andersson wrote: >>> On Sun 16 Jul 11:49 PDT 2017, Jacek Anaszewski wrote: On 07/06/2017 05:18 AM, Pavel Machek wrote: > [..] I've been working on addition of RGB LED support to the LED core for some time now, in the way as we agreed upon at [0], but it turns out to be less trivial if we want to do it in an elegant way. >>> >>> Generally 3 predefined LPG blocks are routed to the TRILED current sink. >>> >>> Exposing the TRILED block as a RGB LED instance would make sense in its >>> own, but it doesn't give us a coherent solution for the LPG. >>> >>> The current board I'm working on (DragonBoard820c) has 4 LEDs, 3 of them >>> are connected to the TRILED and the fourth is on a "GPIO" in current >>> sink mode. >>> >>> By having each LPG represented as a LED device gives us a unified view >>> of the hardware even though there are two different types of current >>> sinks. >>> >>> >>> Further more, per the 96boards specification we're expected to have >>> different triggers on the different "colors" of the TRILED. >> >> What is the function of TRILED block then? My first impression was >> that it allows for setting brightness on all three LED synchronously? >> > > It's nothing more than one hardware block providing 3 current sinks. What's the benefit of grouping three sinks? Is there some other gain besides possibility of turning on/off the whole block at once? >>> >>> So I do not agree with imposing this kind of decisions on the board >>> design just to support this higher level feature. >>> Less elegant way would be duplicating led-core functions and changing single enum led_brightness argument with the three ones (or a struct containing three brightness components) I chose to go the elegant way and tried to introduce led_classdev_base type that would be customizable with the set of ops, that would allow for making the number of brightness components to be set at once customizable. This of course entails significant amount of changes in the LED core and some changes in LED Trigger core. >>> >>> I think that the RGB interface has to be a "frontend" of any >>> configurable LED instances and not tied to a particular hardware >>> controller. >> >> That doesn't assure brightness setting synchronization which is >> especially vital in case of triggers like timer. >> > > If you look at any available Android based device you can see what > happens when you set red, then green and then blue brightness > independently - from user space even. The LED might be > red/green/blue/purple whatever, but I would argue that it's not > noticeable due to the short duration between the updates. > The case where synchronization matters is if you have pattern > transitions as you're extending the time of the transition and you can > spot the color variation in the transitions. It would require more thorough analysis across various devices programmed via different buses. Probably in most cases applying the userspace frontend mentioned by you would work just fine. And when it comes to higher frequencies, we have had already attempts of adding hr timer support for "fast LEDs" like the ones driven through GPIOs, and those could benefit from the kernel level synchronization. -- Best regards, Jacek Anaszewski
Re: [PATCH v2 1/3] leds: core: Introduce generic pattern interface
On Mon 17 Jul 14:08 PDT 2017, Jacek Anaszewski wrote: > On 07/16/2017 11:14 PM, Bjorn Andersson wrote: > > On Sun 16 Jul 11:49 PDT 2017, Jacek Anaszewski wrote: > >> On 07/06/2017 05:18 AM, Pavel Machek wrote: [..] > >> I've been working on addition of RGB LED support to the LED core for > >> some time now, in the way as we agreed upon at [0], but it turns out to > >> be less trivial if we want to do it in an elegant way. > >> > > > > Generally 3 predefined LPG blocks are routed to the TRILED current sink. > > > > Exposing the TRILED block as a RGB LED instance would make sense in its > > own, but it doesn't give us a coherent solution for the LPG. > > > > The current board I'm working on (DragonBoard820c) has 4 LEDs, 3 of them > > are connected to the TRILED and the fourth is on a "GPIO" in current > > sink mode. > > > > By having each LPG represented as a LED device gives us a unified view > > of the hardware even though there are two different types of current > > sinks. > > > > > > Further more, per the 96boards specification we're expected to have > > different triggers on the different "colors" of the TRILED. > > What is the function of TRILED block then? My first impression was > that it allows for setting brightness on all three LED synchronously? > It's nothing more than one hardware block providing 3 current sinks. > > > > So I do not agree with imposing this kind of decisions on the board > > design just to support this higher level feature. > > > >> Less elegant way would be duplicating led-core functions and changing > >> single enum led_brightness argument with the three ones (or a struct > >> containing three brightness components) > >> > >> I chose to go the elegant way and tried to introduce led_classdev_base > >> type that would be customizable with the set of ops, that would allow > >> for making the number of brightness components to be set at once > >> customizable. This of course entails significant amount of changes in > >> the LED core and some changes in LED Trigger core. > >> > > > > I think that the RGB interface has to be a "frontend" of any > > configurable LED instances and not tied to a particular hardware > > controller. > > That doesn't assure brightness setting synchronization which is > especially vital in case of triggers like timer. > If you look at any available Android based device you can see what happens when you set red, then green and then blue brightness independently - from user space even. The LED might be red/green/blue/purple whatever, but I would argue that it's not noticeable due to the short duration between the updates. The case where synchronization matters is if you have pattern transitions as you're extending the time of the transition and you can spot the color variation in the transitions. Regards, Bjorn
Re: [PATCH v2 1/3] leds: core: Introduce generic pattern interface
On 07/16/2017 11:14 PM, Bjorn Andersson wrote: > On Sun 16 Jul 11:49 PDT 2017, Jacek Anaszewski wrote: > >> Hi, >> >> On 07/06/2017 05:18 AM, Pavel Machek wrote: >>> Hi! >>> Some LED controllers have support for autonomously controlling brightness over time, according to some preprogrammed pattern or function. This adds a new optional operator that LED class drivers can implement if they support such functionality as well as a new device attribute to configure the pattern for a given LED. >>> @@ -61,3 +61,23 @@ Description: gpio and backlight triggers. In case of the backlight trigger, it is useful when driving a LED which is intended to indicate a device in a standby like state. + +What: /sys/class/leds//pattern +Date: July 2017 +KernelVersion:4.14 +Description: + Specify a pattern for the LED, for LED hardware that support + altering the brightness as a function of time. + + The pattern is given by a series of tuples, of brightness and + duration (ms). The LED is expected to traverse the series and + each brightness value for the specified duration. + + Additionally a repeat marker ":|" can be appended to the + series, which should cause the pattern to be repeated + endlessly. + + As LED hardware might have different capabilities and precision + the requested pattern might be slighly adjusted by the driver + and the resulting pattern of such operation should be returned + when this file is read. >>> >>> Well. I believe this is mostly useful for RGB LEDs. Unfortunately, having >>> patterns >>> per-LED will present opportunity for different channels becoming >>> de-synchronized >>> from each other, which will not look nice. >> >> Hmm, they are only [brightness duration] tuples, and no definition of >> R, G and B LED device is covered here, so how it can be useful for RGB >> LEDs? >> > > The typical Qualcomm PMIC sports 4-8 LPG channels. The output of these > channels can be configured to some extent, but in theory any combination > of the 8 could be hooked up to the three channels of a RGB LED. > > So looking at the LPG hw block, there's no such thing as RGB. > >> I've been working on addition of RGB LED support to the LED core for >> some time now, in the way as we agreed upon at [0], but it turns out to >> be less trivial if we want to do it in an elegant way. >> > > Generally 3 predefined LPG blocks are routed to the TRILED current sink. > > Exposing the TRILED block as a RGB LED instance would make sense in its > own, but it doesn't give us a coherent solution for the LPG. > > The current board I'm working on (DragonBoard820c) has 4 LEDs, 3 of them > are connected to the TRILED and the fourth is on a "GPIO" in current > sink mode. > > By having each LPG represented as a LED device gives us a unified view > of the hardware even though there are two different types of current > sinks. > > > Further more, per the 96boards specification we're expected to have > different triggers on the different "colors" of the TRILED. What is the function of TRILED block then? My first impression was that it allows for setting brightness on all three LED synchronously? > > So I do not agree with imposing this kind of decisions on the board > design just to support this higher level feature. > >> Less elegant way would be duplicating led-core functions and changing >> single enum led_brightness argument with the three ones (or a struct >> containing three brightness components) >> >> I chose to go the elegant way and tried to introduce led_classdev_base >> type that would be customizable with the set of ops, that would allow >> for making the number of brightness components to be set at once >> customizable. This of course entails significant amount of changes in >> the LED core and some changes in LED Trigger core. >> > > I think that the RGB interface has to be a "frontend" of any > configurable LED instances and not tied to a particular hardware > controller. That doesn't assure brightness setting synchronization which is especially vital in case of triggers like timer. > For patterns you need to have some sort of synchronization between the > channels, but for non-pattern cases you can have any combination of LED > drivers to drive the individual components and I believe this should be > possible to expose through our RGB interface. > >> Unfortunately I can't predict how much time it will take >> to submit an RFC due to limited amount of time I can spend working >> on it. >> >> [0] https://www.spinics.net/lists/linux-leds/msg07959.html > > Regards, > Bjorn > -- Best regards, Jacek Anaszewski
Re: [PATCH v2 1/3] leds: core: Introduce generic pattern interface
On Sun 16 Jul 11:49 PDT 2017, Jacek Anaszewski wrote: > Hi, > > On 07/06/2017 05:18 AM, Pavel Machek wrote: > > Hi! > > > >> Some LED controllers have support for autonomously controlling > >> brightness over time, according to some preprogrammed pattern or > >> function. > >> > >> This adds a new optional operator that LED class drivers can implement > >> if they support such functionality as well as a new device attribute to > >> configure the pattern for a given LED. > > > >> @@ -61,3 +61,23 @@ Description: > >>gpio and backlight triggers. In case of the backlight trigger, > >>it is useful when driving a LED which is intended to indicate > >>a device in a standby like state. > >> + > >> +What: /sys/class/leds//pattern > >> +Date: July 2017 > >> +KernelVersion:4.14 > >> +Description: > >> + Specify a pattern for the LED, for LED hardware that support > >> + altering the brightness as a function of time. > >> + > >> + The pattern is given by a series of tuples, of brightness and > >> + duration (ms). The LED is expected to traverse the series and > >> + each brightness value for the specified duration. > >> + > >> + Additionally a repeat marker ":|" can be appended to the > >> + series, which should cause the pattern to be repeated > >> + endlessly. > >> + > >> + As LED hardware might have different capabilities and precision > >> + the requested pattern might be slighly adjusted by the driver > >> + and the resulting pattern of such operation should be returned > >> + when this file is read. > > > > Well. I believe this is mostly useful for RGB LEDs. Unfortunately, having > > patterns > > per-LED will present opportunity for different channels becoming > > de-synchronized > > from each other, which will not look nice. > > Hmm, they are only [brightness duration] tuples, and no definition of > R, G and B LED device is covered here, so how it can be useful for RGB > LEDs? > The typical Qualcomm PMIC sports 4-8 LPG channels. The output of these channels can be configured to some extent, but in theory any combination of the 8 could be hooked up to the three channels of a RGB LED. So looking at the LPG hw block, there's no such thing as RGB. > I've been working on addition of RGB LED support to the LED core for > some time now, in the way as we agreed upon at [0], but it turns out to > be less trivial if we want to do it in an elegant way. > Generally 3 predefined LPG blocks are routed to the TRILED current sink. Exposing the TRILED block as a RGB LED instance would make sense in its own, but it doesn't give us a coherent solution for the LPG. The current board I'm working on (DragonBoard820c) has 4 LEDs, 3 of them are connected to the TRILED and the fourth is on a "GPIO" in current sink mode. By having each LPG represented as a LED device gives us a unified view of the hardware even though there are two different types of current sinks. Further more, per the 96boards specification we're expected to have different triggers on the different "colors" of the TRILED. So I do not agree with imposing this kind of decisions on the board design just to support this higher level feature. > Less elegant way would be duplicating led-core functions and changing > single enum led_brightness argument with the three ones (or a struct > containing three brightness components) > > I chose to go the elegant way and tried to introduce led_classdev_base > type that would be customizable with the set of ops, that would allow > for making the number of brightness components to be set at once > customizable. This of course entails significant amount of changes in > the LED core and some changes in LED Trigger core. > I think that the RGB interface has to be a "frontend" of any configurable LED instances and not tied to a particular hardware controller. For patterns you need to have some sort of synchronization between the channels, but for non-pattern cases you can have any combination of LED drivers to drive the individual components and I believe this should be possible to expose through our RGB interface. > Unfortunately I can't predict how much time it will take > to submit an RFC due to limited amount of time I can spend working > on it. > > [0] https://www.spinics.net/lists/linux-leds/msg07959.html Regards, Bjorn
Re: [PATCH v2 1/3] leds: core: Introduce generic pattern interface
On Wed 05 Jul 20:18 PDT 2017, Pavel Machek wrote: > Hi! > > > Some LED controllers have support for autonomously controlling > > brightness over time, according to some preprogrammed pattern or > > function. > > > > This adds a new optional operator that LED class drivers can implement > > if they support such functionality as well as a new device attribute to > > configure the pattern for a given LED. > > > @@ -61,3 +61,23 @@ Description: > > gpio and backlight triggers. In case of the backlight trigger, > > it is useful when driving a LED which is intended to indicate > > a device in a standby like state. > > + > > +What: /sys/class/leds//pattern > > +Date: July 2017 > > +KernelVersion: 4.14 > > +Description: > > + Specify a pattern for the LED, for LED hardware that support > > + altering the brightness as a function of time. > > + > > + The pattern is given by a series of tuples, of brightness and > > + duration (ms). The LED is expected to traverse the series and > > + each brightness value for the specified duration. > > + > > + Additionally a repeat marker ":|" can be appended to the > > + series, which should cause the pattern to be repeated > > + endlessly. > > + > > + As LED hardware might have different capabilities and precision > > + the requested pattern might be slighly adjusted by the driver > > + and the resulting pattern of such operation should be returned > > + when this file is read. > > Well. I believe this is mostly useful for RGB LEDs. Unfortunately, having > patterns > per-LED will present opportunity for different channels becoming > de-synchronized > from each other, which will not look nice. > The two laptops on my desk indicates suspend state by pulsing a white and a green LED respectively, so there's definitely people out there who found it useful to do single channel patterns. In the case of the Qualcomm LPG each channel (4-8 in a typical Qualcomm PMIC) is implemented as a standalone hardware block. The pattern generator is in a separate hardware block that is shared between the LPGs and to synchronize pattern between channels the "start signal" to the pattern generator can cover multiple channels. But any combination of the LPG channels can be routed to any of the RGB channels and there are use cases where you want to use a single LPG channel to drive a single LED (or some other sink). It therefor doesn't make sense to enforce any such groupings in the driver itself - at least not for the LPG. Similarly it's conceivable to design a board where one uses 3 PWM channels to drive a RBG LED, in which case we would like to use the leds-pwm driver, rather than creating a 3-channel-pwm LED driver. So, based on our previous discussions on this topic, I think it makes sense to have a virtual multi-channel LED driver, that through triggers can have any LED instances associated with its channels. It would then "broadcast" the brightness and pattern requests to the individual LED instances. Regards, Bjorn
Re: [PATCH v2 1/3] leds: core: Introduce generic pattern interface
Hi, On 07/06/2017 05:18 AM, Pavel Machek wrote: > Hi! > >> Some LED controllers have support for autonomously controlling >> brightness over time, according to some preprogrammed pattern or >> function. >> >> This adds a new optional operator that LED class drivers can implement >> if they support such functionality as well as a new device attribute to >> configure the pattern for a given LED. > >> @@ -61,3 +61,23 @@ Description: >> gpio and backlight triggers. In case of the backlight trigger, >> it is useful when driving a LED which is intended to indicate >> a device in a standby like state. >> + >> +What: /sys/class/leds//pattern >> +Date: July 2017 >> +KernelVersion: 4.14 >> +Description: >> +Specify a pattern for the LED, for LED hardware that support >> +altering the brightness as a function of time. >> + >> +The pattern is given by a series of tuples, of brightness and >> +duration (ms). The LED is expected to traverse the series and >> +each brightness value for the specified duration. >> + >> +Additionally a repeat marker ":|" can be appended to the >> +series, which should cause the pattern to be repeated >> +endlessly. >> + >> +As LED hardware might have different capabilities and precision >> +the requested pattern might be slighly adjusted by the driver >> +and the resulting pattern of such operation should be returned >> +when this file is read. > > Well. I believe this is mostly useful for RGB LEDs. Unfortunately, having > patterns > per-LED will present opportunity for different channels becoming > de-synchronized > from each other, which will not look nice. Hmm, they are only [brightness duration] tuples, and no definition of R, G and B LED device is covered here, so how it can be useful for RGB LEDs? I've been working on addition of RGB LED support to the LED core for some time now, in the way as we agreed upon at [0], but it turns out to be less trivial if we want to do it in an elegant way. Less elegant way would be duplicating led-core functions and changing single enum led_brightness argument with the three ones (or a struct containing three brightness components) I chose to go the elegant way and tried to introduce led_classdev_base type that would be customizable with the set of ops, that would allow for making the number of brightness components to be set at once customizable. This of course entails significant amount of changes in the LED core and some changes in LED Trigger core. Unfortunately I can't predict how much time it will take to submit an RFC due to limited amount of time I can spend working on it. [0] https://www.spinics.net/lists/linux-leds/msg07959.html -- Best regards, Jacek Anaszewski
Re: [PATCH v2 1/3] leds: core: Introduce generic pattern interface
Hi! > Some LED controllers have support for autonomously controlling > brightness over time, according to some preprogrammed pattern or > function. > > This adds a new optional operator that LED class drivers can implement > if they support such functionality as well as a new device attribute to > configure the pattern for a given LED. > @@ -61,3 +61,23 @@ Description: > gpio and backlight triggers. In case of the backlight trigger, > it is useful when driving a LED which is intended to indicate > a device in a standby like state. > + > +What:/sys/class/leds//pattern > +Date:July 2017 > +KernelVersion: 4.14 > +Description: > + Specify a pattern for the LED, for LED hardware that support > + altering the brightness as a function of time. > + > + The pattern is given by a series of tuples, of brightness and > + duration (ms). The LED is expected to traverse the series and > + each brightness value for the specified duration. > + > + Additionally a repeat marker ":|" can be appended to the > + series, which should cause the pattern to be repeated > + endlessly. > + > + As LED hardware might have different capabilities and precision > + the requested pattern might be slighly adjusted by the driver > + and the resulting pattern of such operation should be returned > + when this file is read. Well. I believe this is mostly useful for RGB LEDs. Unfortunately, having patterns per-LED will present opportunity for different channels becoming de-synchronized from each other, which will not look nice. Best regards, Pavel
