| Message ID | cover.1621349813.git.mchehab+huawei@kernel.org |
|---|---|
| Headers | show |
| Series | Adding support for controlling the leds found on Intel NUC | expand |
We have multicolor LED framerwork in Linux. This should be implemented via that framework. Please do not implement your own way for RGB leds. Marek
This should be implemented via the blink trigger or hw_pattern, I think. Have you looked at these? Marek
Hi Marek, Em Wed, 19 May 2021 09:57:13 +0200 Marek Behún <kabel@kernel.org> escreveu: > We have multicolor LED framerwork in Linux. This should be implemented > via that framework. Please do not implement your own way for RGB leds. > > Marek I saw the multicolor LED framework, but IMO it won't fit here. See, Linux doesn't have direct access to the LED. The access is provided via ACPI WMI. The way BIOS reports the type of the led is via a bitmap flag. So, the same LED can be represented with either single-color or multi-color one. See: https://www.intel.com/content/dam/support/us/en/documents/intel-nuc/WMI-Spec-Intel-NUC-NUC10ixFNx.pdf Table 2.2 LED Color Type Bit Number Type 0 Dual-color Blue / Amber 1 Dual-color Blue / White 2 RGB-color 3 Single-color LED Also as each NUC can support up to 7, and they may have a mix of single color, dual color and RGB LEDs, if we would use the multicolor class for the colored ones, that would mean that the code will need to be duplicated, as, depending on what the BIOS reports, the LED would need to be exposed either as via led-multicolor-class or as leds-class. Worse than that, there's even a WMI command that would allow to "switch LED type" (see page 8). On other words, the BIOS can expose a "virtual" single-color LED, but actually implemented in hardware using a RGB one, and this can be changed in real time. (note: I didn't implement support yet for this, as the hardware I have doesn't seem to support the "switch LED type" command). Thanks, Mauro
Em Wed, 19 May 2021 09:58:50 +0200 Marek Behun <marek.behun@nic.cz> escreveu: > This should be implemented via the blink trigger or hw_pattern, I think. > Have you looked at these? The blink trigger and hw_pattern are software implementations for blink. This is not the case on NUC leds, as: 1. It is the hardware/firmware that triggers the blink, not Linux; 2. It blinks even when Linux in suspend/hibernate mode. In a matter of fact, the default usage of blink is to indicate that the machine is suspended. Thanks, Mauro
Hi! > > We have multicolor LED framerwork in Linux. This should be implemented > > via that framework. Please do not implement your own way for RGB leds. > > > > Marek > > I saw the multicolor LED framework, but IMO it won't fit here. > > See, Linux doesn't have direct access to the LED. The access is > provided via ACPI WMI. So? > The way BIOS reports the type of the led is via a bitmap flag. > So, the same LED can be represented with either single-color > or multi-color one. See: > https://www.intel.com/content/dam/support/us/en/documents/intel-nuc/WMI-Spec-Intel-NUC-NUC10ixFNx.pdf > > Table 2.2 LED Color Type > Bit Number Type > 0 Dual-color Blue / Amber > 1 Dual-color Blue / White > 2 RGB-color > 3 Single-color LED > > Also as each NUC can support up to 7, and they may have a mix of > single color, dual color and RGB LEDs, if we would use the > multicolor class for the colored ones, that would mean that the > code will need to be duplicated, as, depending on what the BIOS > reports, the LED would need to be exposed either as via > led-multicolor-class or as leds-class. So? > Worse than that, there's even a WMI command that would allow > to "switch LED type" (see page 8). On other words, the BIOS can > expose a "virtual" single-color LED, but actually implemented > in hardware using a RGB one, and this can be changed in real time. So you simply always use it as RGB one? Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
Hi! > Some models come with single colored or dual-colored LEDs, but high end models > have RGB LEDs. > > Programming them can ether be done via BIOS or by the OS, however, BIOS settings > are limited. So, the vendor offers a Windows application that allows to fully use the > functionality provided by the firmware/hardware. I'm not sure why you are submitting v2 in the middle of interface discussion. Marek and I are saying the same thing -- this needs to use close to existing APIs. If you want to get something merged quickly, please submit basic functionality only (toggling the LED on/off) that completely fits existing APIs. We can review that. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
Em Wed, 19 May 2021 13:07:25 +0200 Pavel Machek <pavel@ucw.cz> escreveu: > Hi! > > > > We have multicolor LED framerwork in Linux. This should be implemented > > > via that framework. Please do not implement your own way for RGB leds. > > > > > > Marek > > > > I saw the multicolor LED framework, but IMO it won't fit here. > > > > See, Linux doesn't have direct access to the LED. The access is > > provided via ACPI WMI. > > So? > > > The way BIOS reports the type of the led is via a bitmap flag. > > So, the same LED can be represented with either single-color > > or multi-color one. See: > > https://www.intel.com/content/dam/support/us/en/documents/intel-nuc/WMI-Spec-Intel-NUC-NUC10ixFNx.pdf > > > > Table 2.2 LED Color Type > > Bit Number Type > > 0 Dual-color Blue / Amber > > 1 Dual-color Blue / White > > 2 RGB-color > > 3 Single-color LED > > > > Also as each NUC can support up to 7, and they may have a mix of > > single color, dual color and RGB LEDs, if we would use the > > multicolor class for the colored ones, that would mean that the > > code will need to be duplicated, as, depending on what the BIOS > > reports, the LED would need to be exposed either as via > > led-multicolor-class or as leds-class. > > So? > > > Worse than that, there's even a WMI command that would allow > > to "switch LED type" (see page 8). On other words, the BIOS can > > expose a "virtual" single-color LED, but actually implemented > > in hardware using a RGB one, and this can be changed in real time. > > So you simply always use it as RGB one? Hmm... are you meaning that I should only use the multicolor led class, even when the BIOS is reporting the LEDs as single color? I can surely do that. > > Best regards, > Pavel Thanks, Mauro
On Wed, 19 May 2021 14:00:40 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > Em Wed, 19 May 2021 13:07:25 +0200 > Pavel Machek <pavel@ucw.cz> escreveu: > > > Hi! > > > > > > We have multicolor LED framerwork in Linux. This should be implemented > > > > via that framework. Please do not implement your own way for RGB leds. > > > > > > > > Marek > > > > > > I saw the multicolor LED framework, but IMO it won't fit here. > > > > > > See, Linux doesn't have direct access to the LED. The access is > > > provided via ACPI WMI. > > > > So? > > > > > The way BIOS reports the type of the led is via a bitmap flag. > > > So, the same LED can be represented with either single-color > > > or multi-color one. See: > > > https://www.intel.com/content/dam/support/us/en/documents/intel-nuc/WMI-Spec-Intel-NUC-NUC10ixFNx.pdf > > > > > > Table 2.2 LED Color Type > > > Bit Number Type > > > 0 Dual-color Blue / Amber > > > 1 Dual-color Blue / White > > > 2 RGB-color > > > 3 Single-color LED > > > > > > Also as each NUC can support up to 7, and they may have a mix of > > > single color, dual color and RGB LEDs, if we would use the > > > multicolor class for the colored ones, that would mean that the > > > code will need to be duplicated, as, depending on what the BIOS > > > reports, the LED would need to be exposed either as via > > > led-multicolor-class or as leds-class. > > > > So? > > > > > Worse than that, there's even a WMI command that would allow > > > to "switch LED type" (see page 8). On other words, the BIOS can > > > expose a "virtual" single-color LED, but actually implemented > > > in hardware using a RGB one, and this can be changed in real time. > > > > So you simply always use it as RGB one? > > Hmm... are you meaning that I should only use the multicolor led class, > even when the BIOS is reporting the LEDs as single color? > > I can surely do that. No. If the BIOS reports that the LED is single color, register a classic LED. If the BIOS reports a RGB LED, register a multi-color LED... Marek
Em Wed, 19 May 2021 13:11:07 +0200 Pavel Machek <pavel@ucw.cz> escreveu: > Hi! > > > Some models come with single colored or dual-colored LEDs, but high end models > > have RGB LEDs. > > > > Programming them can ether be done via BIOS or by the OS, however, BIOS settings > > are limited. So, the vendor offers a Windows application that allows to fully use the > > functionality provided by the firmware/hardware. > > I'm not sure why you are submitting v2 in the middle of interface > discussion. I'll refrain sending a new version while we're discussing the interface. > Marek and I are saying the same thing -- this needs to use close to > existing APIs. Ok, but I'm not seeing an existing API that provides what those LEDs need. > If you want to get something merged quickly, please submit basic > functionality only (toggling the LED on/off) that completely fits > existing APIs. We can review that. If you prefer working this way, I can send an initial patch with just the very basic. Actually, if you apply just patch 2 of this series, it will provide support for for just setting the brightness on NUC8. However, the main reason why someone (including myself) want this driver is to allow to dynamically change what hardware event will be triggering the LED and how, and if suspend will blink or not[1]. Being able to also change the LED color is a plus. [1] Disabling blink at suspend/hibernate is one of the things that I use here: as the machine is at my bedroom, I don't want it to be blinking all night long when the machine is sleeping :-) Thanks, Mauro
Em Wed, 19 May 2021 14:04:51 +0200 Marek Behún <kabel@kernel.org> escreveu: > On Wed, 19 May 2021 14:00:40 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > Em Wed, 19 May 2021 13:07:25 +0200 > > Pavel Machek <pavel@ucw.cz> escreveu: > > > > > Hi! > > > > > > > > We have multicolor LED framerwork in Linux. This should be implemented > > > > > via that framework. Please do not implement your own way for RGB leds. > > > > > > > > > > Marek > > > > > > > > I saw the multicolor LED framework, but IMO it won't fit here. > > > > > > > > See, Linux doesn't have direct access to the LED. The access is > > > > provided via ACPI WMI. > > > > > > So? > > > > > > > The way BIOS reports the type of the led is via a bitmap flag. > > > > So, the same LED can be represented with either single-color > > > > or multi-color one. See: > > > > https://www.intel.com/content/dam/support/us/en/documents/intel-nuc/WMI-Spec-Intel-NUC-NUC10ixFNx.pdf > > > > > > > > Table 2.2 LED Color Type > > > > Bit Number Type > > > > 0 Dual-color Blue / Amber > > > > 1 Dual-color Blue / White > > > > 2 RGB-color > > > > 3 Single-color LED > > > > > > > > Also as each NUC can support up to 7, and they may have a mix of > > > > single color, dual color and RGB LEDs, if we would use the > > > > multicolor class for the colored ones, that would mean that the > > > > code will need to be duplicated, as, depending on what the BIOS > > > > reports, the LED would need to be exposed either as via > > > > led-multicolor-class or as leds-class. > > > > > > So? > > > > > > > Worse than that, there's even a WMI command that would allow > > > > to "switch LED type" (see page 8). On other words, the BIOS can > > > > expose a "virtual" single-color LED, but actually implemented > > > > in hardware using a RGB one, and this can be changed in real time. > > > > > > So you simply always use it as RGB one? > > > > Hmm... are you meaning that I should only use the multicolor led class, > > even when the BIOS is reporting the LEDs as single color? > > > > I can surely do that. > > No. If the BIOS reports that the LED is single color, register a > classic LED. If the BIOS reports a RGB LED, register a multi-color > LED... Ok. I'll try to change the code to work with both APIs. Let's hope it won't result on too much code duplication. - I have one doubt about how to use this API. It is meant just for the RGB LEDs, right? NUC also have dual-color and multi-colored leds that don't allow specifying the intensity of each component. All it lets is to change the color (being, for instance, either blue or white). For those, the API should basically show the names of the supported colors for each LED, letting the user change it to some other color. How should I map such leds? via led-class or via led-class-multicolor? Thanks, Mauro
Hi! > > Marek and I are saying the same thing -- this needs to use close to > > existing APIs. > > Ok, but I'm not seeing an existing API that provides what those > LEDs need. Well, there "close to" part comes into play. > > If you want to get something merged quickly, please submit basic > > functionality only (toggling the LED on/off) that completely fits > > existing APIs. We can review that. > > If you prefer working this way, I can send an initial patch with > just the very basic. Actually, if you apply just patch 2 of this > series, it will provide support for for just setting the brightness > on NUC8. I don't care much. We can discuss minimal interface additions neccessary to support your usecases. But what you proposed was nowhere near close. Note that we don't want to support every crazy feature, just because hardware can do it. > However, the main reason why someone (including myself) want this > driver is to allow to dynamically change what hardware event will > be triggering the LED and how, and if suspend will blink or not[1]. > Being able to also change the LED color is a plus. This one is hard if the LED does not support full color. > [1] Disabling blink at suspend/hibernate is one of the things that > I use here: as the machine is at my bedroom, I don't want it to be > blinking all night long when the machine is sleeping :-) Ok, so lets start with the blink at suspend thing? Having power LED on when machine is on, and slowly "breathing" when machine is suspended is something I have seen before. Is that what your hardware is doing? Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
Em Wed, 19 May 2021 21:41:15 +0200 Pavel Machek <pavel@ucw.cz> escreveu: > Hi! > > > > Marek and I are saying the same thing -- this needs to use close to > > > existing APIs. > > > > Ok, but I'm not seeing an existing API that provides what those > > LEDs need. > > Well, there "close to" part comes into play. > > > > If you want to get something merged quickly, please submit basic > > > functionality only (toggling the LED on/off) that completely fits > > > existing APIs. We can review that. > > > > If you prefer working this way, I can send an initial patch with > > just the very basic. Actually, if you apply just patch 2 of this > > series, it will provide support for for just setting the brightness > > on NUC8. > > I don't care much. We can discuss minimal interface additions > neccessary to support your usecases. > > But what you proposed was nowhere near close. Ok. Let's try to come into an agreement about what's needed. On my discussions with Marek, it sounds to me that the features from the trigger API won't fit, as the attributes there won't be supported by the NUC leds (and vice-versa). Yet, we could try to have something that would look similar. > > Note that we don't want to support every crazy feature, just because > hardware can do it. Neither do I ;-) I don't care much for Software controlled LEDs, nor for a feature that would allow the BIOS to "hide" the LED colors as if it were a single colored one, for instance. Yet, the remaining stuff seems pretty much OK. > > > However, the main reason why someone (including myself) want this > > driver is to allow to dynamically change what hardware event will > > be triggering the LED and how, and if suspend will blink or not[1]. > > > Being able to also change the LED color is a plus. > > This one is hard if the LED does not support full color. Only a subset of devices support RGB colors, but the API has support for dual-color and 8-color LEDs. On those, the color is selected like an enum. The NUC8 device I use her has RGB color LEDs. > > > [1] Disabling blink at suspend/hibernate is one of the things that > > I use here: as the machine is at my bedroom, I don't want it to be > > blinking all night long when the machine is sleeping :-) > > Ok, so lets start with the blink at suspend thing? Yeah, that sounds to be a good start point. > > Having power LED on when machine is on, and slowly "breathing" when > machine is suspended is something I have seen before. Is that what > your hardware is doing? Yes, but see: my device has 6 leds (API supports up to 7 leds). Any of them can be programmed to act as a power LED at runtime. So, the first thing that the API needs is a way to tell what LED is monitoring the device's power state. Then, for each power state (S0, S3, S5), define if the LED will be ON all the times or not. The "slowing breathing" is one of the possible blink patterns. The driver supports 4 other blink patterns - Solid - the LED won't blink; - Breathing - it looks like a sinusoidal wave pattern; - Pulsing - it looks like a square wave pattern; - Strobing - it turns ON suddenly, and then it slowly turns OFF. The speed of the blink is also adjustable, ranging from 0.1 Hz to 1 Hz, on 0.1 Hz steps. --- Let me explain this specific part of the API from my original proposal. Those are the led names from the datasheets (NUC 8 and above), and my proposal for the sysfs class directory name: ============= =============================== LED name sysfs ============= =============================== Skull ``/sys/class/leds/nuc::skull`` Skull eyes ``/sys/class/leds/nuc::eyes`` Power ``/sys/class/leds/nuc::power`` HDD ``/sys/class/leds/nuc::hdd`` Front1 ``/sys/class/leds/nuc::front1`` Front2 ``/sys/class/leds/nuc::front2`` Front3 ``/sys/class/leds/nuc::front3`` ============= =============================== For each of the above, there's the need to identify what hardware function is monitored (if any). My proposal were to add an "indicator" node (the name came from the Intel datasheets) that shows what led will monitor the power state. Then, one blink_behavior and one blink_frequency per power state, e. g.: /sys/class/leds/nuc::front1 |-- indicator |-- s0_blink_behavior |-- s0_blink_frequency |-- s3_blink_behavior |-- s3_blink_frequency |-- s5_blink_behavior `-- s5_blink_frequency PS.: I don't care much about what names we'll use. Feel free to rename them, if you think the above is not clear or generic enough. - To make part of the API complete, there's also the need of a node to control the max brightness that the leds will achieve at the ON state, and another one to control the color on each state, as one could define, let's say, "white" when powered on, "blue" when suspended and "yellow" when hibernating. The colors at the NUC I have are RGB (but other models can use an enum for the supported colors). /sys/class/leds/nuc::front1 |-- s0_brightness |-- s0_color # only shown on colored leds |-- s3_brightness |-- s3_color # only shown on colored leds |-- s0_brightness `-- s5_color # only shown on colored leds Thanks, Mauro
On Thu, 20 May 2021 01:07:20 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > So, the first thing that the API needs is a way to tell what LED > is monitoring the device's power state. If a LED can monitor the device's power state in HW, register a LED private trigger for this LED. If the LED is configured into this state by default, you can set this trigger to be the default_trigger prior registering the LED. The name of this private trigger can be "hw:powerstate" or something like that (I wonder what others will think about this name). > Then, for each power state (S0, S3, S5), define if the LED will > be ON all the times or not. > > The "slowing breathing" is one of the possible blink patterns. > The driver supports 4 other blink patterns > > - Solid - the LED won't blink; > - Breathing - it looks like a sinusoidal wave pattern; > - Pulsing - it looks like a square wave pattern; > - Strobing - it turns ON suddenly, and then it slowly turns OFF. > > The speed of the blink is also adjustable, ranging from 0.1 Hz to 1 Hz, > on 0.1 Hz steps. Is the speed of breathing/strobing also adjustable? Or only when pulsing? When this "hw:powerstate" trigger is enabled for this LED, only then another sysfs files should appear in this LED's sysfs directory. > --- > > Let me explain this specific part of the API from my original proposal. > > Those are the led names from the datasheets (NUC 8 and above), > and my proposal for the sysfs class directory name: > > ============= =============================== > LED name sysfs > ============= =============================== > Skull ``/sys/class/leds/nuc::skull`` > Skull eyes ``/sys/class/leds/nuc::eyes`` > Power ``/sys/class/leds/nuc::power`` > HDD ``/sys/class/leds/nuc::hdd`` > Front1 ``/sys/class/leds/nuc::front1`` > Front2 ``/sys/class/leds/nuc::front2`` > Front3 ``/sys/class/leds/nuc::front3`` > ============= =============================== > > For each of the above, there's the need to identify what > hardware function is monitored (if any). > > My proposal were to add an "indicator" node (the name came from > the Intel datasheets) that shows what led will monitor the power state. > > Then, one blink_behavior and one blink_frequency per power state, > e. g.: > > /sys/class/leds/nuc::front1 > |-- indicator > |-- s0_blink_behavior > |-- s0_blink_frequency > |-- s3_blink_behavior > |-- s3_blink_frequency > |-- s5_blink_behavior > `-- s5_blink_frequency I'd rather use one file for frequencies and one for intervals, and map in to an array, but that is just my preference... > > PS.: I don't care much about what names we'll use. Feel free to > rename them, if you think the above is not clear or generic enough. > > - > > To make part of the API complete, there's also the need of a node > to control the max brightness that the leds will achieve at the > ON state, and another one to control the color on each state, > as one could define, let's say, "white" when powered on, "blue" > when suspended and "yellow" when hibernating. The colors at the > NUC I have are RGB (but other models can use an enum for the > supported colors). > > /sys/class/leds/nuc::front1 > |-- s0_brightness > |-- s0_color # only shown on colored leds > |-- s3_brightness > |-- s3_color # only shown on colored leds > |-- s0_brightness > `-- s5_color # only shown on colored leds If the BIOS reports a LED being full RGB LED, you should register it via multicolor framework. Regarding the enum with 8 colors: are these colors red, yellow, green, cyan, blue, magenta? Because if so, then this is RGB with each channel being binary :) So you can again use multicolor framework.
Em Thu, 20 May 2021 18:19:19 +0200 Marek Behún <kabel@kernel.org> escreveu: > On Thu, 20 May 2021 01:07:20 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > So, the first thing that the API needs is a way to tell what LED > > is monitoring the device's power state. > > If a LED can monitor the device's power state in HW, register a LED > private trigger for this LED. If the LED is configured into this state > by default, you can set this trigger to be the default_trigger prior > registering the LED. The name of this private trigger can be > "hw:powerstate" or something like that (I wonder what others will > think about this name). Ok. So, assuming that we will have one trigger per each hardware state, it could have something like (names subject to change): - hw:powerstate - hw:disk_activity - hw:ethernet_activity - hw:wifi_active - hw:power_limit Right? It still needs to indicate two other possible states: - software controlled led; - led is disabled. Setting led's brightness to zero is different than disabling it. Disabling can be done via BIOS, but BIOS config doesn't allow setting the brightness. There are other difference on BIOS settings: it allow disabling each/all LED controls and/or to disable software control of each LED. So, we need a way at the API to uniquely identify when the LED is software-controlled and when it is disabled. Would it be something like: - hw:disable trigger? or better to implement it on a different way? > > Then, for each power state (S0, S3, S5), define if the LED will > > be ON all the times or not. > > > > The "slowing breathing" is one of the possible blink patterns. > > The driver supports 4 other blink patterns > > > > - Solid - the LED won't blink; > > - Breathing - it looks like a sinusoidal wave pattern; > > - Pulsing - it looks like a square wave pattern; > > - Strobing - it turns ON suddenly, and then it slowly turns OFF. > > > > The speed of the blink is also adjustable, ranging from 0.1 Hz to 1 Hz, > > on 0.1 Hz steps. > > Is the speed of breathing/strobing also adjustable? Or only when > pulsing? Yes, speed is also adjustable, from 0.1 to 1.0 HZ, in 0.1 Hz (NUC 8 and above). The NUC6 API is more limited than NUC8+: it has just two blink patterns (blink, fade), and only 3 frequencies are allowed (0.25 Hz, 0.50 Hz and 1.0 Hz). > When this "hw:powerstate" trigger is enabled for this LED, > only then another sysfs files should appear in this LED's sysfs > directory. OK, makes sense. Out of curiosity: is it reliable to make sysfs nodes appear and disappear dynamically? Does inotify (or something similar) can be used to identify when such nodes appear/disappear? I remember a long time ago I wanted to use something like that at the media (or edac?) subsystem, but someone (Greg, I think) recommended otherwise due to some potential racing issues. > > > --- > > > > Let me explain this specific part of the API from my original proposal. > > > > Those are the led names from the datasheets (NUC 8 and above), > > and my proposal for the sysfs class directory name: > > > > ============= =============================== > > LED name sysfs > > ============= =============================== > > Skull ``/sys/class/leds/nuc::skull`` > > Skull eyes ``/sys/class/leds/nuc::eyes`` > > Power ``/sys/class/leds/nuc::power`` > > HDD ``/sys/class/leds/nuc::hdd`` > > Front1 ``/sys/class/leds/nuc::front1`` > > Front2 ``/sys/class/leds/nuc::front2`` > > Front3 ``/sys/class/leds/nuc::front3`` > > ============= =============================== > > > > For each of the above, there's the need to identify what > > hardware function is monitored (if any). > > > > My proposal were to add an "indicator" node (the name came from > > the Intel datasheets) that shows what led will monitor the power state. > > > > Then, one blink_behavior and one blink_frequency per power state, > > e. g.: > > > > /sys/class/leds/nuc::front1 > > |-- indicator > > |-- s0_blink_behavior > > |-- s0_blink_frequency > > |-- s3_blink_behavior > > |-- s3_blink_frequency > > |-- s5_blink_behavior > > `-- s5_blink_frequency > > I'd rather use one file for frequencies and one for intervals, and map > in to an array, but that is just my preference... By intervals are you meaning 1/frequency? So, basically exposing the frequency as two fields? If so, it sounds overkill to me to have both. Btw, maybe instead of "blink_behavior" it could use "blink_pattern". This would diverge from the datahseet name, but it probably describes better what will be controlled when blink is enabled: - frequency (or inverval) - pattern > > > > > PS.: I don't care much about what names we'll use. Feel free to > > rename them, if you think the above is not clear or generic enough. > > > > - > > > > To make part of the API complete, there's also the need of a node > > to control the max brightness that the leds will achieve at the > > ON state, and another one to control the color on each state, > > as one could define, let's say, "white" when powered on, "blue" > > when suspended and "yellow" when hibernating. The colors at the > > NUC I have are RGB (but other models can use an enum for the > > supported colors). > > > > /sys/class/leds/nuc::front1 > > |-- s0_brightness > > |-- s0_color # only shown on colored leds > > |-- s3_brightness > > |-- s3_color # only shown on colored leds > > |-- s0_brightness > > `-- s5_color # only shown on colored leds > > If the BIOS reports a LED being full RGB LED, you should register it > via multicolor framework. OK. > Regarding the enum with 8 colors: are these > colors red, yellow, green, cyan, blue, magenta? Because if so, then > this is RGB with each channel being binary :) So you can again use > multicolor framework. The dual-colored ones aren't RGB. Two types are supported: - Blue/Amber - Blue/White the only one with 8 colors is at NUC6 API: the ring led. This one can be mapped as RGB with 1 bit per color, as those are the colors: +---------+ | disable | +---------+ | cyan | +---------+ | pink | +---------+ | yellow | +---------+ | blue | +---------+ | red | +---------+ | green | +---------+ | white | +---------+ Thanks, Mauro
On Thu, 20 May 2021 21:16:15 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > So, assuming that we will have one trigger per each hardware > state, it could have something like (names subject to change): > > - hw:powerstate > - hw:disk_activity > - hw:ethernet_activity > - hw:wifi_active > - hw:power_limit > > Right? Yes, but we should really try to map ethernet_activity to netdev and disk_activity to a potential blkdev trigger :-) That's my opinion. > It still needs to indicate two other possible states: > > - software controlled led; > - led is disabled. > > Setting led's brightness to zero is different than disabling > it. > > Disabling can be done via BIOS, but BIOS config doesn't allow > setting the brightness. There are other difference on BIOS settings: > it allow disabling each/all LED controls and/or to disable software > control of each LED. > > So, we need a way at the API to uniquely identify when the LED > is software-controlled and when it is disabled. > Would it be something like: > > - hw:disable > > trigger? or better to implement it on a different way? What is the functional difference (visible to the user) between zero brightness and disabled LED? IMO if user says echo 0 >brightness you can just disable the LED. Or is this impossible? > > Is the speed of breathing/strobing also adjustable? Or only when > > pulsing? > > Yes, speed is also adjustable, from 0.1 to 1.0 HZ, in 0.1 Hz > (NUC 8 and above). > > The NUC6 API is more limited than NUC8+: it has just two > blink patterns (blink, fade), and only 3 frequencies are allowed > (0.25 Hz, 0.50 Hz and 1.0 Hz). > > > When this "hw:powerstate" trigger is enabled for this LED, > > only then another sysfs files should appear in this LED's sysfs > > directory. > > OK, makes sense. > > Out of curiosity: is it reliable to make sysfs nodes appear and > disappear dynamically? Does inotify (or something similar) can > be used to identify when such nodes appear/disappear? > > I remember a long time ago I wanted to use something like that > at the media (or edac?) subsystem, but someone (Greg, I think) > recommended otherwise due to some potential racing issues. No idea, but I would guess yes. > > I'd rather use one file for frequencies and one for intervals, and map > > in to an array, but that is just my preference... > > By intervals are you meaning 1/frequency? So, basically exposing > the frequency as two fields? If so, it sounds overkill to me to have both. Sorry, I meant one file for frequencies and one for patterns. > > Btw, maybe instead of "blink_behavior" it could use "blink_pattern". > > This would diverge from the datahseet name, but it probably describes > better what will be controlled when blink is enabled: > > - frequency (or inverval) > - pattern > > > Regarding the enum with 8 colors: are these > > colors red, yellow, green, cyan, blue, magenta? Because if so, then > > this is RGB with each channel being binary :) So you can again use > > multicolor framework. > > The dual-colored ones aren't RGB. Two types are supported: > - Blue/Amber > - Blue/White These would need a new API, ignore these for now. Marek
Em Thu, 20 May 2021 21:43:56 +0200 Marek Behún <kabel@kernel.org> escreveu: > On Thu, 20 May 2021 21:16:15 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > So, assuming that we will have one trigger per each hardware > > state, it could have something like (names subject to change): > > > > - hw:powerstate > > - hw:disk_activity > > - hw:ethernet_activity > > - hw:wifi_active > > - hw:power_limit > > > > Right? > > Yes, but we should really try to map ethernet_activity to netdev and > disk_activity to a potential blkdev trigger :-) That's my opinion. > > > It still needs to indicate two other possible states: > > > > - software controlled led; > > - led is disabled. > > > > Setting led's brightness to zero is different than disabling > > it. > > > > Disabling can be done via BIOS, but BIOS config doesn't allow > > setting the brightness. There are other difference on BIOS settings: > > it allow disabling each/all LED controls and/or to disable software > > control of each LED. > > > > So, we need a way at the API to uniquely identify when the LED > > is software-controlled and when it is disabled. > > Would it be something like: > > > > - hw:disable > > > > trigger? or better to implement it on a different way? > > What is the functional difference (visible to the user) between zero > brightness and disabled LED? IMO if user says > echo 0 >brightness > you can just disable the LED. Or is this impossible? echo 0 >brightness will turn off the LED, but it won't disable it. A trigger can still be enabled on it. With a disabled LED, depending on how it was disabled, it can't be enabled in runtime. One may need to boot the machine and use BIOS setup to enable it. Trying to change such LED in runtime will return an error. > > > > Is the speed of breathing/strobing also adjustable? Or only when > > > pulsing? > > > > Yes, speed is also adjustable, from 0.1 to 1.0 HZ, in 0.1 Hz > > (NUC 8 and above). > > > > The NUC6 API is more limited than NUC8+: it has just two > > blink patterns (blink, fade), and only 3 frequencies are allowed > > (0.25 Hz, 0.50 Hz and 1.0 Hz). > > > > > When this "hw:powerstate" trigger is enabled for this LED, > > > only then another sysfs files should appear in this LED's sysfs > > > directory. > > > > OK, makes sense. > > > > Out of curiosity: is it reliable to make sysfs nodes appear and > > disappear dynamically? Does inotify (or something similar) can > > be used to identify when such nodes appear/disappear? > > > > I remember a long time ago I wanted to use something like that > > at the media (or edac?) subsystem, but someone (Greg, I think) > > recommended otherwise due to some potential racing issues. > > No idea, but I would guess yes. > > > > I'd rather use one file for frequencies and one for intervals, and map > > > in to an array, but that is just my preference... > > > > By intervals are you meaning 1/frequency? So, basically exposing > > the frequency as two fields? If so, it sounds overkill to me to have both. > > Sorry, I meant one file for frequencies and one for patterns. Ah, makes sense. Yeah, that's how I mapped it. > > Btw, maybe instead of "blink_behavior" it could use "blink_pattern". > > > > This would diverge from the datahseet name, but it probably describes > > better what will be controlled when blink is enabled: > > > > - frequency (or inverval) > > - pattern > > > > > Regarding the enum with 8 colors: are these > > > colors red, yellow, green, cyan, blue, magenta? Because if so, then > > > this is RGB with each channel being binary :) So you can again use > > > multicolor framework. > > > > The dual-colored ones aren't RGB. Two types are supported: > > - Blue/Amber > > - Blue/White > > These would need a new API, ignore these for now. This affects mainly NUC6 part of the API. I'll postpone it. Yet, IMHO, the best here is to do exactly how I did: use the "normal" leds class and add a "color" attribute that can either be "blue" or "amber" written on it (for a blue/amber kind of LED). Thanks, Mauro