From: Mauro Carvalho Chehab <email@example.com> To: Pavel Machek <firstname.lastname@example.org> Cc: email@example.com, firstname.lastname@example.org, email@example.com, Mauro Carvalho Chehab <firstname.lastname@example.org>, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org Subject: Re: [PATCH 17/17] staging: nuc-led: update the TODOs Date: Mon, 17 May 2021 08:30:01 +0200 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <20210516182149.GA3666@localhost> Hi Pavel, Em Sun, 16 May 2021 20:21:50 +0200 Pavel Machek <firstname.lastname@example.org> 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
next prev parent reply other threads:[~2021-05-17 6:30 UTC|newest] Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-16 10:53 [PATCH 00/17] Add an experimental driver for Intel NUC leds Mauro Carvalho Chehab 2021-05-16 10:53 ` [PATCH 01/17] staging: add support for NUC WMI LEDs Mauro Carvalho Chehab 2021-05-16 16:12 ` Randy Dunlap 2021-05-17 8:20 ` Greg KH 2021-05-16 10:53 ` [PATCH 02/17] staging: nuc-wmi: detect WMI API detection Mauro Carvalho Chehab 2021-05-17 9:35 ` Dan Carpenter 2021-05-16 10:53 ` [PATCH 03/17] staging: nuc-wmi: add support for changing S0 brightness Mauro Carvalho Chehab 2021-05-16 10:53 ` [PATCH 04/17] staging: nuc-wmi: add all types of brightness Mauro Carvalho Chehab 2021-05-16 10:53 ` [PATCH 05/17] staging: nuc-wmi: allow changing the LED colors Mauro Carvalho Chehab 2021-05-16 10:53 ` [PATCH 06/17] staging: nuc-wmi: add support for WMI API version 1.0 Mauro Carvalho Chehab 2021-05-16 10:53 ` [PATCH 07/17] staging: nuc-wmi: add basic support for NUC6 WMI Mauro Carvalho Chehab 2021-05-17 9:44 ` Dan Carpenter 2021-05-16 10:53 ` [PATCH 08/17] staging: muc-wmi: add brightness and color for NUC6 API Mauro Carvalho Chehab 2021-05-16 10:53 ` [PATCH 09/17] staging: nuc-wmi: Add support to blink behavior for NUC8/10 Mauro Carvalho Chehab 2021-05-16 10:53 ` [PATCH 10/17] staging: nuc-wmi: get rid of an unused variable Mauro Carvalho Chehab 2021-05-16 10:53 ` [PATCH 11/17] staging: nuc-wmi: implement blink control for NUC6 Mauro Carvalho Chehab 2021-05-16 10:53 ` [PATCH 12/17] staging: nuc-wmi: better detect NUC6/NUC7 devices Mauro Carvalho Chehab 2021-05-16 10:53 ` [PATCH 13/17] staging: nuc-led: add support for HDD activity default Mauro Carvalho Chehab 2021-05-16 10:53 ` [PATCH 14/17] staging: nuc-wmi: fix software blink behavior logic Mauro Carvalho Chehab 2021-05-16 10:53 ` [PATCH 15/17] staging: nuc-wmi: add support for changing the ethernet type indicator Mauro Carvalho Chehab 2021-05-16 10:53 ` [PATCH 16/17] staging: nuc-wmi: add support for changing the power limit scheme Mauro Carvalho Chehab 2021-05-16 10:53 ` [PATCH 17/17] staging: nuc-led: update the TODOs Mauro Carvalho Chehab 2021-05-16 18:21 ` Pavel Machek 2021-05-17 6:30 ` Mauro Carvalho Chehab [this message] 2021-05-17 8:05 ` Pavel Machek 2021-05-17 8:57 ` Mauro Carvalho Chehab 2021-05-17 9:12 ` Mauro Carvalho Chehab 2021-05-17 12:19 ` Mauro Carvalho Chehab 2021-05-17 8:18 ` [PATCH 00/17] Add an experimental driver for Intel NUC leds Greg KH 2021-05-17 9:02 ` Mauro Carvalho Chehab 2021-05-17 9:08 ` Greg KH
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH 17/17] staging: nuc-led: update the TODOs' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).