Re: [PATCH 17/17] staging: nuc-led: update the TODOs
Em Mon, 17 May 2021 10:05:27 +0200 Pavel Machek escreveu: > No. Take a look at triggers; for example hdd monitor should look very > much like existing disk trigger. Hmm... after looking at triggers, I'm not sure if this is the right interface, nor if we're talking about the same thing. See, at least the way ledtrig-disk.c uses it, two drivers are triggering the LED based on software events: drivers/ata/libata-core.c: ledtrig_disk_activity(!!(qc->tf.flags & ATA_TFLAG_WRITE)); drivers/ide/ide-disk.c: ledtrig_disk_activity(rq_data_dir(rq) == WRITE); This is not how the NUC LEDs are work. On NUC, the hardware itself triggers the event and/or blinks the LED(*). (*) By default, blink is enabled only when the device is suspended or it is hibernating. There's no need to do any software emulation. The API is meant to just program the hardware (and/or the firmware) for it to do the behavior expected by the user. So, for instance, setting the indicator event that will trigger the LED is done by sending a WMI message for this GUID object: 8C5DA44C-CDC3-46b3-8619-4E26D34390B7 (somewhat similar to using the way ACPI works, but WMI is a different firmware interface). The method at the WMI API which sets the LED indicator is this one: 0x05 message (Set an Indicator option for the LED type) Such method receives two parameters. The first one is the LED number, which can be: 0 - Power button LED 1 - HDD LED 2 - Skull LED 3 - Eyes LED 4 - Front LED 1 5 - Front LED 1 6 - Front LED 1 The second one tells which hardware event will trigger the LED: = == === Value Indicator type Meaning = == === 0 Power State Shows if the device is powered and what power level it is (e. g. if the device is suspended or not, and on which kind of suspended level). 1 HDD ActivityIndicates if the LED is measuring the hard disk (or SDD) activity. 2 EthernetIndicates the activity Ethernet adapter(s) 3 WiFiIndicates if WiFi is enabled 4 SoftwareDoesn't indicate any hardware level. Instead, the LED status is controlled via software. 5 Power Limit Changes the LED color when the computer is throttling its power limits. 6 Disable The LED was disabled (either by BIOS or via WMI). = == === There is an example at page 7 of the datasheet: https://raw.githubusercontent.com/nomego/intel_nuc_led/master/specs/INTEL_WMI_LED_0.64.pdf Where it shows what happens if the WMI message is filled with: <0x05> <0x00> <0x01> On such example, it changes the behavior of the button named "Power button" to change it to monitor the disk activity, instead of monitoring if the device is powered on or not. Such setting is even persistent after rebooting, and the event is hardware/firmware triggered. So, IMO, it would only makes sense to use something else from the LED class if are there a way for the sysfs nodes to dynamically be shown/hidden when the indicator changes. At least on my eyes, that doesn't seem to be what triggers do. Thanks, Mauro ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 17/17] staging: nuc-led: update the TODOs
Em Mon, 17 May 2021 10:57:49 +0200 Mauro Carvalho Chehab escreveu: > Em Mon, 17 May 2021 10:05:27 +0200 > Pavel Machek escreveu: > > No. Take a look at triggers; for example hdd monitor should look very > > much like existing disk trigger. Btw, is there a way to trigger brightness? When a LED is monitoring the power state, brightness should be hidden, as, instead of a single brightness parameter, the device will now have one brightness per different power state, e. g.: When the LED indicator is measuring *Power State*, the following parameters may be available: = === Parameter Meaning = === _brightnessBrightness in percent (from 0 to 100) _blink_behaviortype of blink. See :ref:`nuc_blink_behavior`. _blink_frequency Blink frequency. See :ref:`nuc_blink_behavior`. _color LED color See :ref:`nuc_color`. = === Where is different, depending on the WMI API version: On version 0.64 (NUC8/9): ++ | s0 | ++ | s3 | ++ | s5 | ++ | ready_mode | ++ Btw, I've no idea what "ready mode" is, as the specs don't explain it. This particular mode is disabled on my NUC8 device, so I can't test it. On version 1.0 (NUC10+): ++ | s0 | ++ | s3 | ++ | standby| ++ Note: At the specs, "Standby" is actually "Modern Standby". I ended simplifying it, as just "standby_brightness" sounds good enough. Thanks, Mauro ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 17/17] staging: nuc-led: update the TODOs
Em Mon, 17 May 2021 10:05:27 +0200 Pavel Machek escreveu: > Hi! > > > > > - Need to validate the uAPI and document it before moving > > > >this driver out of staging. > > > > > > > - Stabilize and document its sysfs interface. > > > > > > Would you mind starting with this one? > > > > Do you mean writing the ABI document for it? Surely I can do that, > > but I'm not sure where to put such document while it is on staging. > > No need for formal ABI just yet, but it needs to be clear what the interface > is. > > > > We should have existing APIs > > > for most of functionality described... > > > > I tried to stay as close as possible to the existing API, but > > there are some things that required a different one. > > I believe it should be possible to move _way_ closer to existing APIs. > > > For instance, with WMI rev 0.64 and 1.0, any LED of the device > > can be programmed to be a power indicator. > > > > When a LED is programmed this way, there are up to 3 (on rev 1.0) or up > > to 4 (on rev 0.64) different brightness level of the LED, and those > > are associated with a power status (like S0, S3, S5, "ready mode"). > > I'll need a description how this works. > > > /sys/class/leds/nuc::front1/ > > ├── blink_behavior > > ├── blink_frequency > > We have timer trigger for these. Not really. The LED blink behavior is provided by the hardware itself. The LEDs can blink *even* when the device is suspended or is hibernating. That's something that a timer trigger can't do ;-) See below for a draft of the ABI description. > > > ├── ethernet_type > > ├── hdd_default > > ├── indicator > > ├── ready_mode_blink_behavior > > ├── ready_mode_blink_frequency > > ├── ready_mode_brightness > > ├── s0_blink_behavior > > ├── s0_blink_frequency > > ├── s0_brightness > > ├── s3_blink_behavior > > ├── s3_blink_frequency > > ├── s3_brightness > > ├── s5_blink_behavior > > ├── s5_blink_frequency > > ├── s5_brightness > > No. Take a look at triggers; for example hdd monitor should look very > much like existing disk trigger. Ok, I'll double-check how this works. Yeah, it would be a way better if the sysfs nodes could be hidden when changing the indicator type. For instance, when monitoring disk activity, only those parameters may be available: = === Parameter Meaning = === brightness Brightness in percent (from 0 to 100) color LED color. See :ref:`nuc_color`. hdd_default Default is LED turned ON or OFF. When set toOFF, the LED will turn on at disk activity. When set to ON, the LED will be turned on by default, turning off at disk activity. = === (color is only available for multi-colored or RGB leds). > > > On other words, the "indicator" tells what type of hardware event > > the LED is currently measuring: > > > > $ cat /sys/class/leds/nuc\:\:front1/indicator > > Power State [HDD Activity] Ethernet WiFi Software Power Limit > > Disable > > So this will use existing "trigger" infrastructure. Ok, I will take a look on that. Are there any driver that I could use as an example, using it in a configurable way? > > That should likely be easier to discuss if any changes at the > > ABI would be needed. Before moving it out of staging, I would > > add another one under Documentation/ABI describing the meaning > > of each sysfs node. > > Lets get reasonable ABI before moving it _into_ tree, staging or > otherwise. I'm enclosing a document that I started to write today, describing the way the current ABI was designed. The document doesn't describe in full the NUC6 variant (which is really limited: just two LEDs with fixed behavior). Thanks, Mauro == Intel NUC WMI LEDs == Some models of the Intel Next Unit of Computing (NUC) may have programmable LEDs on its panel via its BIOS. A subset of those may also be programmed on user space. There are currently three different APIs on such devices, depending on the NUC generation: * NUC 6/7: https://www.intel.com/content/www/us/en/support/articles/23426/intel-nuc/intel-nuc-kits.html * NUC 8/9: https://raw.githubusercontent.com/nomego/intel_nuc_led/master/specs/INTEL_WMI_LED_0.64.pdf * NUC 10 and newer: https://www.intel.com/content/dam/support/us/en/documents/intel-nuc/WMI-Spec-Intel-NUC-NUC10ixFNx.pdf This document
Re: [PATCH 17/17] staging: nuc-led: update the TODOs
Hi! > > > - Need to validate the uAPI and document it before moving > > >this driver out of staging. > > > > > - Stabilize and document its sysfs interface. > > > > Would you mind starting with this one? > > Do you mean writing the ABI document for it? Surely I can do that, > but I'm not sure where to put such document while it is on staging. No need for formal ABI just yet, but it needs to be clear what the interface is. > > We should have existing APIs > > for most of functionality described... > > I tried to stay as close as possible to the existing API, but > there are some things that required a different one. I believe it should be possible to move _way_ closer to existing APIs. > For instance, with WMI rev 0.64 and 1.0, any LED of the device > can be programmed to be a power indicator. > > When a LED is programmed this way, there are up to 3 (on rev 1.0) or up > to 4 (on rev 0.64) different brightness level of the LED, and those > are associated with a power status (like S0, S3, S5, "ready mode"). I'll need a description how this works. > /sys/class/leds/nuc::front1/ > ├── blink_behavior > ├── blink_frequency We have timer trigger for these. > ├── ethernet_type > ├── hdd_default > ├── indicator > ├── ready_mode_blink_behavior > ├── ready_mode_blink_frequency > ├── ready_mode_brightness > ├── s0_blink_behavior > ├── s0_blink_frequency > ├── s0_brightness > ├── s3_blink_behavior > ├── s3_blink_frequency > ├── s3_brightness > ├── s5_blink_behavior > ├── s5_blink_frequency > ├── s5_brightness No. Take a look at triggers; for example hdd monitor should look very much like existing disk trigger. > On other words, the "indicator" tells what type of hardware event > the LED is currently measuring: > > $ cat /sys/class/leds/nuc\:\:front1/indicator > Power State [HDD Activity] Ethernet WiFi Software Power Limit > Disable So this will use existing "trigger" infrastructure. > That should likely be easier to discuss if any changes at the > ABI would be needed. Before moving it out of staging, I would > add another one under Documentation/ABI describing the meaning > of each sysfs node. Lets get reasonable ABI before moving it _into_ tree, staging or otherwise. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek signature.asc Description: Digital signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 17/17] staging: nuc-led: update the TODOs
Hi Pavel, Em Sun, 16 May 2021 20:21:50 +0200 Pavel Machek escreveu: > Hi! > > > - Need to validate the uAPI and document it before moving > >this driver out of staging. > > > - Stabilize and document its sysfs interface. > > Would you mind starting with this one? Do you mean writing the ABI document for it? Surely I can do that, but I'm not sure where to put such document while it is on staging. > We should have existing APIs > for most of functionality described... I tried to stay as close as possible to the existing API, but there are some things that required a different one. For instance, with WMI rev 0.64 and 1.0, any LED of the device can be programmed to be a power indicator. When a LED is programmed this way, there are up to 3 (on rev 1.0) or up to 4 (on rev 0.64) different brightness level of the LED, and those are associated with a power status (like S0, S3, S5, "ready mode"). So, the LED API standard "brightness" is meaningless. On the other hand, when the same LED is programmed to monitor, let's say, the WiFi or one of the two Ethernets (or both at the same time), the standard "brightness" level makes sense. > > We really don't want to merge code with bad API, not even to staging. See, this is the API that it is exposed on with a NUC8: $ tree /sys/class/leds/nuc\:\:front1/ /sys/class/leds/nuc::front1/ ├── blink_behavior ├── blink_frequency ├── brightness ├── color ├── device -> ../../../8C5DA44C-CDC3-46B3-8619-4E26D34390B7 ├── ethernet_type ├── hdd_default ├── indicator ├── max_brightness ├── power │ ├── autosuspend_delay_ms │ ├── control │ ├── runtime_active_time │ ├── runtime_status │ └── runtime_suspended_time ├── power_limit_scheme ├── ready_mode_blink_behavior ├── ready_mode_blink_frequency ├── ready_mode_brightness ├── s0_blink_behavior ├── s0_blink_frequency ├── s0_brightness ├── s3_blink_behavior ├── s3_blink_frequency ├── s3_brightness ├── s5_blink_behavior ├── s5_blink_frequency ├── s5_brightness ├── subsystem -> ../../../../../../../../class/leds ├── trigger └── uevent As the behavior of the LEDs can be dynamically changed, each LED expose parameters for all types of hardware event it can deal, but only the ones that are applied to its current indicator type can be seen/changed. On other words, the "indicator" tells what type of hardware event the LED is currently measuring: $ cat /sys/class/leds/nuc\:\:front1/indicator Power State [HDD Activity] Ethernet WiFi Software Power Limit Disable In this case, as it is measuring the HDD activity. If one tries to read/write something to, let's say, the Ethernet type, a -EINVAL is returned: $ cat /sys/class/leds/nuc\:\:front1/ethernet_type cat: '/sys/class/leds/nuc::front1/ethernet_type': Invalid argument So, before being able to use the ethernet_type, the indicator needs to be changed: $ echo Ethernet > /sys/class/leds/nuc\:\:front1/indicator $ cat /sys/class/leds/nuc\:\:front1/ethernet_type LAN1 LAN2 [LAN1+LAN2] Anyway, I suspect that besides a document under ABI, it would make sense to add a .rst file describing it under admin-guide, explaining how to use the ABI. That should likely be easier to discuss if any changes at the ABI would be needed. Before moving it out of staging, I would add another one under Documentation/ABI describing the meaning of each sysfs node. Would that work for you? Thanks, Mauro ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 17/17] staging: nuc-led: update the TODOs
Hi! > - Need to validate the uAPI and document it before moving >this driver out of staging. > - Stabilize and document its sysfs interface. Would you mind starting with this one? We should have existing APIs for most of functionality described... We really don't want to merge code with bad API, not even to staging. Best regards, Pavel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
