Some LED controllers may come with an internal HW triggering mechanism
for the LED and an ability to switch between user control of the LED,
or the internal control. One such example is AXP20X PMIC, that allows
wither for user control of the LED, or for internal control based on
the state of the battery charger.
Add support for registering per-LED device trigger.
Names of private triggers need to be globally unique, but may clash
with other private triggers. This is enforced during trigger
registration. Developers can register private triggers just like
the normal triggers, by setting private_led to a classdev
of the LED the trigger is associated with.
Signed-off-by: Ondrej Jirman <[email protected]>
---
drivers/leds/led-triggers.c | 12 +++++++++---
include/linux/leds.h | 3 +++
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 79e30d2cb7a5..3011e2658404 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -50,7 +50,8 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
down_read(&triggers_list_lock);
list_for_each_entry(trig, &trigger_list, next_trig) {
- if (sysfs_streq(buf, trig->name)) {
+ if (sysfs_streq(buf, trig->name) &&
+ (!trig->private_led || trig->private_led == led_cdev)) {
down_write(&led_cdev->trigger_lock);
led_trigger_set(led_cdev, trig);
up_write(&led_cdev->trigger_lock);
@@ -96,6 +97,9 @@ static int led_trigger_format(char *buf, size_t size,
bool hit = led_cdev->trigger &&
!strcmp(led_cdev->trigger->name, trig->name);
+ if (trig->private_led && trig->private_led != led_cdev)
+ continue;
+
len += led_trigger_snprintf(buf + len, size - len,
" %s%s%s", hit ? "[" : "",
trig->name, hit ? "]" : "");
@@ -243,7 +247,8 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
down_read(&triggers_list_lock);
down_write(&led_cdev->trigger_lock);
list_for_each_entry(trig, &trigger_list, next_trig) {
- if (!strcmp(led_cdev->default_trigger, trig->name)) {
+ if (!strcmp(led_cdev->default_trigger, trig->name) &&
+ (!trig->private_led || trig->private_led == led_cdev)) {
led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
led_trigger_set(led_cdev, trig);
break;
@@ -280,7 +285,8 @@ int led_trigger_register(struct led_trigger *trig)
down_write(&triggers_list_lock);
/* Make sure the trigger's name isn't already in use */
list_for_each_entry(_trig, &trigger_list, next_trig) {
- if (!strcmp(_trig->name, trig->name)) {
+ if (!strcmp(_trig->name, trig->name) &&
+ (!_trig->private_led || _trig->private_led == trig->private_led)) {
up_write(&triggers_list_lock);
return -EEXIST;
}
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 2451962d1ec5..0d7577c752ad 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -345,6 +345,9 @@ struct led_trigger {
int (*activate)(struct led_classdev *led_cdev);
void (*deactivate)(struct led_classdev *led_cdev);
+ /* LED-private triggers have the LED device set here. */
+ struct led_classdev *private_led;
+
/* LEDs under control by this trigger (for simple triggers) */
rwlock_t leddev_list_lock;
struct list_head led_cdevs;
--
2.27.0
Hi,
On Thu, Jul 02, 2020 at 04:47:11PM +0200, megous hlavni wrote:
> Some LED controllers may come with an internal HW triggering mechanism
> for the LED and an ability to switch between user control of the LED,
> or the internal control. One such example is AXP20X PMIC, that allows
> wither for user control of the LED, or for internal control based on
> the state of the battery charger.
>
> Add support for registering per-LED device trigger.
>
> Names of private triggers need to be globally unique, but may clash
> with other private triggers. This is enforced during trigger
> registration. Developers can register private triggers just like
> the normal triggers, by setting private_led to a classdev
> of the LED the trigger is associated with.
I've prepared this patch based on the note here:
https://www.kernel.org/doc/html/latest/leds/leds-class.html
Future Development:
At the moment, a trigger can’t be created specifically for a single LED. There
are a number of cases where a trigger might only be mappable to a particular
LED (ACPI?). The addition of triggers provided by the LED driver should cover
this option and be possible to add without breaking the current interface.
Does it look good?
I plan to use this interface in the axp20x driver I've sent previously,
if this patch is acceptable.
thank you and regards,
o.
> Signed-off-by: Ondrej Jirman <[email protected]>
> ---
> drivers/leds/led-triggers.c | 12 +++++++++---
> include/linux/leds.h | 3 +++
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 79e30d2cb7a5..3011e2658404 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -50,7 +50,8 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
>
> down_read(&triggers_list_lock);
> list_for_each_entry(trig, &trigger_list, next_trig) {
> - if (sysfs_streq(buf, trig->name)) {
> + if (sysfs_streq(buf, trig->name) &&
> + (!trig->private_led || trig->private_led == led_cdev)) {
> down_write(&led_cdev->trigger_lock);
> led_trigger_set(led_cdev, trig);
> up_write(&led_cdev->trigger_lock);
> @@ -96,6 +97,9 @@ static int led_trigger_format(char *buf, size_t size,
> bool hit = led_cdev->trigger &&
> !strcmp(led_cdev->trigger->name, trig->name);
>
> + if (trig->private_led && trig->private_led != led_cdev)
> + continue;
> +
> len += led_trigger_snprintf(buf + len, size - len,
> " %s%s%s", hit ? "[" : "",
> trig->name, hit ? "]" : "");
> @@ -243,7 +247,8 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
> down_read(&triggers_list_lock);
> down_write(&led_cdev->trigger_lock);
> list_for_each_entry(trig, &trigger_list, next_trig) {
> - if (!strcmp(led_cdev->default_trigger, trig->name)) {
> + if (!strcmp(led_cdev->default_trigger, trig->name) &&
> + (!trig->private_led || trig->private_led == led_cdev)) {
> led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
> led_trigger_set(led_cdev, trig);
> break;
> @@ -280,7 +285,8 @@ int led_trigger_register(struct led_trigger *trig)
> down_write(&triggers_list_lock);
> /* Make sure the trigger's name isn't already in use */
> list_for_each_entry(_trig, &trigger_list, next_trig) {
> - if (!strcmp(_trig->name, trig->name)) {
> + if (!strcmp(_trig->name, trig->name) &&
> + (!_trig->private_led || _trig->private_led == trig->private_led)) {
> up_write(&triggers_list_lock);
> return -EEXIST;
> }
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 2451962d1ec5..0d7577c752ad 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -345,6 +345,9 @@ struct led_trigger {
> int (*activate)(struct led_classdev *led_cdev);
> void (*deactivate)(struct led_classdev *led_cdev);
>
> + /* LED-private triggers have the LED device set here. */
> + struct led_classdev *private_led;
I'm not sure what the best descriptive name is here. ^
regards,
o.
> /* LEDs under control by this trigger (for simple triggers) */
> rwlock_t leddev_list_lock;
> struct list_head led_cdevs;
> --
> 2.27.0
>
On Thu, 2 Jul 2020 16:47:11 +0200
Ondrej Jirman <[email protected]> wrote:
> Some LED controllers may come with an internal HW triggering mechanism
> for the LED and an ability to switch between user control of the LED,
> or the internal control. One such example is AXP20X PMIC, that allows
> wither for user control of the LED, or for internal control based on
> the state of the battery charger.
>
> Add support for registering per-LED device trigger.
>
> Names of private triggers need to be globally unique, but may clash
> with other private triggers. This is enforced during trigger
> registration. Developers can register private triggers just like
> the normal triggers, by setting private_led to a classdev
> of the LED the trigger is associated with.
>
> Signed-off-by: Ondrej Jirman <[email protected]>
> ---
> drivers/leds/led-triggers.c | 12 +++++++++---
> include/linux/leds.h | 3 +++
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 79e30d2cb7a5..3011e2658404 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -50,7 +50,8 @@ ssize_t led_trigger_write(struct file *filp, struct
> kobject *kobj,
> down_read(&triggers_list_lock);
> list_for_each_entry(trig, &trigger_list, next_trig) {
> - if (sysfs_streq(buf, trig->name)) {
> + if (sysfs_streq(buf, trig->name) &&
> + (!trig->private_led || trig->private_led ==
> led_cdev)) { down_write(&led_cdev->trigger_lock);
> led_trigger_set(led_cdev, trig);
> up_write(&led_cdev->trigger_lock);
> @@ -96,6 +97,9 @@ static int led_trigger_format(char *buf, size_t
> size, bool hit = led_cdev->trigger &&
> !strcmp(led_cdev->trigger->name, trig->name);
>
> + if (trig->private_led && trig->private_led !=
> led_cdev)
> + continue;
> +
> len += led_trigger_snprintf(buf + len, size - len,
> " %s%s%s", hit ? "[" :
> "", trig->name, hit ? "]" : "");
> @@ -243,7 +247,8 @@ void led_trigger_set_default(struct led_classdev
> *led_cdev) down_read(&triggers_list_lock);
> down_write(&led_cdev->trigger_lock);
> list_for_each_entry(trig, &trigger_list, next_trig) {
> - if (!strcmp(led_cdev->default_trigger, trig->name)) {
> + if (!strcmp(led_cdev->default_trigger, trig->name) &&
> + (!trig->private_led || trig->private_led ==
> led_cdev)) { led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
> led_trigger_set(led_cdev, trig);
> break;
> @@ -280,7 +285,8 @@ int led_trigger_register(struct led_trigger *trig)
> down_write(&triggers_list_lock);
> /* Make sure the trigger's name isn't already in use */
> list_for_each_entry(_trig, &trigger_list, next_trig) {
> - if (!strcmp(_trig->name, trig->name)) {
> + if (!strcmp(_trig->name, trig->name) &&
> + (!_trig->private_led || _trig->private_led ==
> trig->private_led)) { up_write(&triggers_list_lock);
> return -EEXIST;
> }
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 2451962d1ec5..0d7577c752ad 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -345,6 +345,9 @@ struct led_trigger {
> int (*activate)(struct led_classdev
> *led_cdev); void (*deactivate)(struct led_classdev
> *led_cdev);
> + /* LED-private triggers have the LED device set here. */
> + struct led_classdev *private_led;
> +
> /* LEDs under control by this trigger (for simple triggers)
> */ rwlock_t leddev_list_lock;
> struct list_head led_cdevs;
Hi Ondrej,
Some criticism to this approach to HW triggers:
- every hw trigger for each LED has to be registered via current trigger
API. This will grow code size and memory footprint once this API is
widely used
- one HW trigger can only master one LED device (via private_led
member). So if I have, for example an ethernet switch with 8 ports,
and each port has 2 LEDs, and each LED has 10 possible HW triggering
mechanisms, with your proposed API one would need to register 8*2*10
= 160 triggers
I too have been thinking about an API for HW LED triggers, and I
tinkered with it a little. Some time ago I sent some emails, with
subjects:
"RFC: LED hw triggering API"
"about my trigger-sources work"
My current thoughts about how HW LED triggers could work nicely is as
such:
- these members (maybe with different names) shall be added to struct
led_classdev:
available_hw_triggers()
- shall return a NULL terminated list of HW trigger names
available for this LED
set_hw_trigger()
- sets HW trigger for this LED. The LED triggering API shall
call this method after previous LED trigger is unset. If
called with NULL parameter, unsets HW trigger
current_hw_trigger
- name of the currently set HW LED trigger for this LED
- the driver registering the LED cdev informs abouth the LED being
capable of HW triggering - members available_hw_triggers and
set_hw_trigger must be set
- SW LED trigger and HW LED trigger are mutualy exclusive on one LED
- the trigger file in sysfs (/sys/class/leds/LED/trigger) shall first
list the available SW triggers, and then available hw triggers for
this LED, prefixed with "hw:"
When written, if the written trigger name starts with "hw:",
instead of setting SW trigger, a HW trigger is set via
set_hw_trigger() method
This could also nicely work with trigger-source DTS property - when the
driver registers a LED, it can determine what other device is
triggering the LED (for example a netdevice), and correspondingly set
the correct name for the LED. Code for that could be shared between HW
and SW triggering API.
Marek
Hello Marek,
On Fri, Jul 03, 2020 at 12:06:02PM +0200, Marek Beh?n wrote:
> On Thu, 2 Jul 2020 16:47:11 +0200
> Ondrej Jirman <[email protected]> wrote:
>
> > Some LED controllers may come with an internal HW triggering mechanism
> > for the LED and an ability to switch between user control of the LED,
> > or the internal control. One such example is AXP20X PMIC, that allows
> > wither for user control of the LED, or for internal control based on
> > the state of the battery charger.
> >
> > Add support for registering per-LED device trigger.
> >
> > Names of private triggers need to be globally unique, but may clash
> > with other private triggers. This is enforced during trigger
> > registration. Developers can register private triggers just like
> > the normal triggers, by setting private_led to a classdev
> > of the LED the trigger is associated with.
> >
[...]
>
> Hi Ondrej,
>
> Some criticism to this approach to HW triggers:
> - every hw trigger for each LED has to be registered via current trigger
> API. This will grow code size and memory footprint once this API is
> widely used
> - one HW trigger can only master one LED device (via private_led
> member). So if I have, for example an ethernet switch with 8 ports,
> and each port has 2 LEDs, and each LED has 10 possible HW triggering
> mechanisms, with your proposed API one would need to register 8*2*10
> = 160 triggers
Do you have such a switch? Also what's your real usecase?
My usecase is a PMIC which has either a user-controllable or self-working mode
for a single LED it controls. I want to be able to switch between those quickly
and easily.
I want the LED to be mostly controlled by PMIC, because that way PMIC can signal
events that are not exposed to OS like overvoltage, overheating, etc. ... all
automagically, but also be able to control it sometimes via SW (for non PMIC
related notifications, eg.).
So in my mindset LED is either controlled by Linux via various SW triggers, or
it's self-working in some arbitrary device specific configuration that doesn't
need any passing of the data via CPU for the LED to reflect some HW state.
So I'd expose a 'hw-trigger' only on the LED device that allows this, that you
can select among all the other regular triggers as you do now, and then
configure its precise mode/options in sysfs (on the trigger kobj). The driver
would come with some sane device specific defaults for the self-working mode.
User can then select hw-trigger, in the triggers and would get a nice PMIC LED
behavior controlled by HW, or a common LED behavior of the ehternet port, or
on the wireless card, or whatever.
From the perspective of this use case the interface is nice and generic:
- you set LED to hw-trigger mode on boot
- you set trigger to none and poke the LED with a pattern you want for the
notification and put it back to hw-trigger mode again afterwards
We can standardize on hw-trigger, or self-controlled, or some other name
for this kind of private LED trigger, and then the userspace would not need
to even care about the specific LED device type, when sitching between
SW controlled and self-working modes.
You'd be able to take SW control of the ethernet PHY controlled LEDs this way
just the same as the PMIC LED with the same interface, described above. And
if you don't like the default self-controled mode, you can change it via
sysfs attributes on the trigger.
It would also allow the user to switch between SW and HW control, without
having to remember the previous HW triggering mode, because that could be
persisted by the LED HW trigger device. So you can go back to previous
HW triggering mode just by 'echo hw-trigger > your-led/trigger'.
I've read through the discussions, and this seems like a workable interface.
Most of the LED devices would just add one extra private trigger to the
triggers_list, so it would not explode in the way you describe above.
Also benefit of this approach is that it fits quite nicely with the existing
code, and requires minimal changes. The trigger already allows for specifying
sysfs attributes, too.
regards,
o.
> I too have been thinking about an API for HW LED triggers, and I
> tinkered with it a little. Some time ago I sent some emails, with
> subjects:
> "RFC: LED hw triggering API"
> "about my trigger-sources work"
> My current thoughts about how HW LED triggers could work nicely is as
> such:
> - these members (maybe with different names) shall be added to struct
> led_classdev:
> available_hw_triggers()
> - shall return a NULL terminated list of HW trigger names
> available for this LED
> set_hw_trigger()
> - sets HW trigger for this LED. The LED triggering API shall
> call this method after previous LED trigger is unset. If
> called with NULL parameter, unsets HW trigger
> current_hw_trigger
> - name of the currently set HW LED trigger for this LED
> - the driver registering the LED cdev informs abouth the LED being
> capable of HW triggering - members available_hw_triggers and
> set_hw_trigger must be set
> - SW LED trigger and HW LED trigger are mutualy exclusive on one LED
> - the trigger file in sysfs (/sys/class/leds/LED/trigger) shall first
> list the available SW triggers, and then available hw triggers for
> this LED, prefixed with "hw:"
> When written, if the written trigger name starts with "hw:",
> instead of setting SW trigger, a HW trigger is set via
> set_hw_trigger() method
>
> This could also nicely work with trigger-source DTS property - when the
> driver registers a LED, it can determine what other device is
> triggering the LED (for example a netdevice), and correspondingly set
> the correct name for the LED. Code for that could be shared between HW
> and SW triggering API.
>
> Marek
Hi!
> Add support for registering per-LED device trigger.
>
> Names of private triggers need to be globally unique, but may clash
> with other private triggers. This is enforced during trigger
Globally unique name is going to be a problem, no? If you have two
keyboards with automatical backlight support...
Otherwise... yes, we need something like this.
Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Hello,
On Sat, Jul 04, 2020 at 02:04:59PM +0200, Pavel Machek wrote:
> Hi!
>
> > Add support for registering per-LED device trigger.
> >
> > Names of private triggers need to be globally unique, but may clash
> > with other private triggers. This is enforced during trigger
>
> Globally unique name is going to be a problem, no? If you have two
> keyboards with automatical backlight support...
Only globally unique in a sense that they must not clash with non
per-LED trigger names. So you can have two keyboards with 'self-working'
trigger on their LED devices in sysfs.
This requirement only comes from the fact that this shares the
same sysfs configuration interface as regular non-private triggers.
regards,
o.
> Otherwise... yes, we need something like this.
>
> Best regards,
> Pavel
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Hi!
> Some criticism to this approach to HW triggers:
> - every hw trigger for each LED has to be registered via current trigger
> API. This will grow code size and memory footprint once this API is
> widely used
> - one HW trigger can only master one LED device (via private_led
> member). So if I have, for example an ethernet switch with 8 ports,
> and each port has 2 LEDs, and each LED has 10 possible HW triggering
> mechanisms, with your proposed API one would need to register 8*2*10
> = 160 triggers
Well, code is simple, and so far I have seen 2 HW triggering
mechanisms, not 10. Maybe we should have a function to regiter a hw
trigger for a LED, so that internal implementation can be changed
more easily.
Ondrej: You already have code using this, right? Can we get an example?
> I too have been thinking about an API for HW LED triggers, and I
> tinkered with it a little. Some time ago I sent some emails, with
> subjects:
> "RFC: LED hw triggering API"
> "about my trigger-sources work"
Perhaps it is time to send them one more time, so Ondrej can say if it
works for him/looks okay for him?
> My current thoughts about how HW LED triggers could work nicely is as
> such:
> - these members (maybe with different names) shall be added to struct
> led_classdev:
> available_hw_triggers()
> - shall return a NULL terminated list of HW trigger names
> available for this LED
> set_hw_trigger()
> - sets HW trigger for this LED. The LED triggering API shall
> call this method after previous LED trigger is unset. If
> called with NULL parameter, unsets HW trigger
> current_hw_trigger
> - name of the currently set HW LED trigger for this LED
> - the driver registering the LED cdev informs abouth the LED being
> capable of HW triggering - members available_hw_triggers and
> set_hw_trigger must be set
> - SW LED trigger and HW LED trigger are mutualy exclusive on one LED
> - the trigger file in sysfs (/sys/class/leds/LED/trigger) shall first
> list the available SW triggers, and then available hw triggers for
> this LED, prefixed with "hw:"
> When written, if the written trigger name starts with "hw:",
> instead of setting SW trigger, a HW trigger is set via
> set_hw_trigger() method
This does not sound bad, either.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> > > Add support for registering per-LED device trigger.
> > >
> > > Names of private triggers need to be globally unique, but may clash
> > > with other private triggers. This is enforced during trigger
> >
> > Globally unique name is going to be a problem, no? If you have two
> > keyboards with automatical backlight support...
>
> Only globally unique in a sense that they must not clash with non
> per-LED trigger names. So you can have two keyboards with 'self-working'
> trigger on their LED devices in sysfs.
>
> This requirement only comes from the fact that this shares the
> same sysfs configuration interface as regular non-private triggers.
Ok. That looks sane.
And if you tweak code a bit (don't compare pointers to struct led;
have struct hw_trigger_group, and compare pointers to that), you
should be able to fix the uglyness Marek mentioned without major changes.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Ondrej,
I overlooked your reply in my inbox, sorry this took so long.
On Fri, 3 Jul 2020 15:08:09 +0200
Ondřej Jirman <[email protected]> wrote:
> Do you have such a switch? Also what's your real usecase?
Yes, on Turris MOX three 8-port ethernet switches can be plugged,
resulting in 24 ethernet ports, each having 2 LEDs.
The current driver does not expose these LEDs via sysfs, but I want to
add the support there. Each of these LEDs can be controlled by
software, or can be put into one of these HW controlled modes:
- Link (with three submodes: 10/100, Gb, 10Gb)
- Link activity (again with three submodes as above)
- PTP activity
- Force blink
> My usecase is a PMIC which has either a user-controllable or
> self-working mode for a single LED it controls. I want to be able to
> switch between those quickly and easily.
I understand your usecase completely. This is the same thing I want. I
just have reservations about how you want to implement this.
Marek
> I want the LED to be mostly controlled by PMIC, because that way PMIC
> can signal events that are not exposed to OS like overvoltage,
> overheating, etc. ... all automagically, but also be able to control
> it sometimes via SW (for non PMIC related notifications, eg.).
>
> So in my mindset LED is either controlled by Linux via various SW
> triggers, or it's self-working in some arbitrary device specific
> configuration that doesn't need any passing of the data via CPU for
> the LED to reflect some HW state.
>
> So I'd expose a 'hw-trigger' only on the LED device that allows this,
> that you can select among all the other regular triggers as you do
> now, and then configure its precise mode/options in sysfs (on the
> trigger kobj). The driver would come with some sane device specific
> defaults for the self-working mode.
>
> User can then select hw-trigger, in the triggers and would get a nice
> PMIC LED behavior controlled by HW, or a common LED behavior of the
> ehternet port, or on the wireless card, or whatever.
>
> From the perspective of this use case the interface is nice and
> generic:
>
> - you set LED to hw-trigger mode on boot
> - you set trigger to none and poke the LED with a pattern you want
> for the notification and put it back to hw-trigger mode again
> afterwards
>
> We can standardize on hw-trigger, or self-controlled, or some other
> name for this kind of private LED trigger, and then the userspace
> would not need to even care about the specific LED device type, when
> sitching between SW controlled and self-working modes.
>
> You'd be able to take SW control of the ethernet PHY controlled LEDs
> this way just the same as the PMIC LED with the same interface,
> described above. And if you don't like the default self-controled
> mode, you can change it via sysfs attributes on the trigger.
>
> It would also allow the user to switch between SW and HW control,
> without having to remember the previous HW triggering mode, because
> that could be persisted by the LED HW trigger device. So you can go
> back to previous HW triggering mode just by 'echo hw-trigger >
> your-led/trigger'.
>
> I've read through the discussions, and this seems like a workable
> interface.
>
> Most of the LED devices would just add one extra private trigger to
> the triggers_list, so it would not explode in the way you describe
> above.
>
> Also benefit of this approach is that it fits quite nicely with the
> existing code, and requires minimal changes. The trigger already
> allows for specifying sysfs attributes, too.
>
> regards,
> o.
On Sat, 4 Jul 2020 14:59:00 +0200
Pavel Machek <[email protected]> wrote:
> Hi!
>
> > Some criticism to this approach to HW triggers:
> > - every hw trigger for each LED has to be registered via current
> > trigger API. This will grow code size and memory footprint once
> > this API is widely used
> > - one HW trigger can only master one LED device (via private_led
> > member). So if I have, for example an ethernet switch with 8
> > ports, and each port has 2 LEDs, and each LED has 10 possible HW
> > triggering mechanisms, with your proposed API one would need to
> > register 8*2*10 = 160 triggers
>
> Well, code is simple, and so far I have seen 2 HW triggering
> mechanisms, not 10. Maybe we should have a function to regiter a hw
> trigger for a LED, so that internal implementation can be changed
> more easily.
>
> Ondrej: You already have code using this, right? Can we get an
> example?
>
> > I too have been thinking about an API for HW LED triggers, and I
> > tinkered with it a little. Some time ago I sent some emails, with
> > subjects:
> > "RFC: LED hw triggering API"
> > "about my trigger-sources work"
>
> Perhaps it is time to send them one more time, so Ondrej can say if it
> works for him/looks okay for him?
>
> > My current thoughts about how HW LED triggers could work nicely is
> > as such:
> > - these members (maybe with different names) shall be added to
> > struct led_classdev:
> > available_hw_triggers()
> > - shall return a NULL terminated list of HW trigger names
> > available for this LED
> > set_hw_trigger()
> > - sets HW trigger for this LED. The LED triggering API shall
> > call this method after previous LED trigger is unset. If
> > called with NULL parameter, unsets HW trigger
> > current_hw_trigger
> > - name of the currently set HW LED trigger for this LED
> > - the driver registering the LED cdev informs abouth the LED being
> > capable of HW triggering - members available_hw_triggers and
> > set_hw_trigger must be set
> > - SW LED trigger and HW LED trigger are mutualy exclusive on one
> > LED
> > - the trigger file in sysfs (/sys/class/leds/LED/trigger) shall
> > first list the available SW triggers, and then available hw
> > triggers for this LED, prefixed with "hw:"
> > When written, if the written trigger name starts with "hw:",
> > instead of setting SW trigger, a HW trigger is set via
> > set_hw_trigger() method
>
> This does not sound bad, either.
>
> Best regards,
> Pavel
Hi Pavel
I shall try to implement this and send a proposal within 2 weeks.
Marek
Hi!
> Some LED controllers may come with an internal HW triggering mechanism
> for the LED and an ability to switch between user control of the LED,
> or the internal control. One such example is AXP20X PMIC, that allows
> wither for user control of the LED, or for internal control based on
> the state of the battery charger.
>
> Add support for registering per-LED device trigger.
>
> Names of private triggers need to be globally unique, but may clash
> with other private triggers. This is enforced during trigger
> registration. Developers can register private triggers just like
> the normal triggers, by setting private_led to a classdev
> of the LED the trigger is associated with.
What about this? Should address Marek's concerns about resource use...
Best regards,
Pavel
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 79e30d2cb7a5..e8333675959c 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -27,6 +27,12 @@ LIST_HEAD(trigger_list);
/* Used by LED Class */
+static inline bool
+trigger_relevant(struct led_classdev *led_cdev, struct led_trigger *trig)
+{
+ return !trig->trigger_type || trig->trigger_type == led_cdev->trigger_type;
+}
+
ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
struct bin_attribute *bin_attr, char *buf,
loff_t pos, size_t count)
@@ -50,7 +56,8 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
down_read(&triggers_list_lock);
list_for_each_entry(trig, &trigger_list, next_trig) {
- if (sysfs_streq(buf, trig->name)) {
+ if (sysfs_streq(buf, trig->name) &&
+ trigger_relevant(led_cdev, trig)) {
down_write(&led_cdev->trigger_lock);
led_trigger_set(led_cdev, trig);
up_write(&led_cdev->trigger_lock);
@@ -96,6 +103,9 @@ static int led_trigger_format(char *buf, size_t size,
bool hit = led_cdev->trigger &&
!strcmp(led_cdev->trigger->name, trig->name);
+ if (!trigger_relevant(led_cdev, trig))
+ continue;
+
len += led_trigger_snprintf(buf + len, size - len,
" %s%s%s", hit ? "[" : "",
trig->name, hit ? "]" : "");
@@ -243,7 +253,8 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
down_read(&triggers_list_lock);
down_write(&led_cdev->trigger_lock);
list_for_each_entry(trig, &trigger_list, next_trig) {
- if (!strcmp(led_cdev->default_trigger, trig->name)) {
+ if (!strcmp(led_cdev->default_trigger, trig->name) &&
+ trigger_relevant(led_cdev, trig)) {
led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
led_trigger_set(led_cdev, trig);
break;
@@ -280,7 +291,8 @@ int led_trigger_register(struct led_trigger *trig)
down_write(&triggers_list_lock);
/* Make sure the trigger's name isn't already in use */
list_for_each_entry(_trig, &trigger_list, next_trig) {
- if (!strcmp(_trig->name, trig->name)) {
+ if (!strcmp(_trig->name, trig->name) &&
+ (!_trig->private_led || _trig->private_led == trig->private_led)) {
up_write(&triggers_list_lock);
return -EEXIST;
}
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 2451962d1ec5..cba52714558f 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -57,6 +57,10 @@ struct led_init_data {
bool devname_mandatory;
};
+struct led_hw_trigger_type {
+ int dummy;
+}
+
struct led_classdev {
const char *name;
enum led_brightness brightness;
@@ -150,6 +154,8 @@ struct led_classdev {
/* Ensures consistent access to the LED Flash Class device */
struct mutex led_access;
+
+ struct led_hw_trigger_type *trigger_type;
};
/**
@@ -345,6 +351,9 @@ struct led_trigger {
int (*activate)(struct led_classdev *led_cdev);
void (*deactivate)(struct led_classdev *led_cdev);
+ /* LED-private triggers have this set. */
+ struct led_hw_trigger_type *trigger_type;
+
/* LEDs under control by this trigger (for simple triggers) */
rwlock_t leddev_list_lock;
struct list_head led_cdevs;
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hello Pavel,
On Sat, Jul 11, 2020 at 12:04:09PM +0200, Pavel Machek wrote:
> Hi!
>
> > Some LED controllers may come with an internal HW triggering mechanism
> > for the LED and an ability to switch between user control of the LED,
> > or the internal control. One such example is AXP20X PMIC, that allows
> > wither for user control of the LED, or for internal control based on
> > the state of the battery charger.
> >
> > Add support for registering per-LED device trigger.
> >
> > Names of private triggers need to be globally unique, but may clash
> > with other private triggers. This is enforced during trigger
> > registration. Developers can register private triggers just like
> > the normal triggers, by setting private_led to a classdev
> > of the LED the trigger is associated with.
>
> What about this? Should address Marek's concerns about resource use...
What concerns? Marek's concerns seem to be about case where we register
a trigger for (each led * self-working configuration) which I admit
can be quite a lot of triggers if there are many functions. But that's
not my proposal.
My proposal is to only register on trigger per LED at most. So on my
system that's 1 extra trigger and on Marek's system that'd be 48 new
triggers. Neither seems like a meaningful problem from resource
use perspective.
regards,
o.
> Best regards,
> Pavel
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 79e30d2cb7a5..e8333675959c 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -27,6 +27,12 @@ LIST_HEAD(trigger_list);
>
> /* Used by LED Class */
>
> +static inline bool
> +trigger_relevant(struct led_classdev *led_cdev, struct led_trigger *trig)
> +{
> + return !trig->trigger_type || trig->trigger_type == led_cdev->trigger_type;
> +}
> +
> ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
> struct bin_attribute *bin_attr, char *buf,
> loff_t pos, size_t count)
> @@ -50,7 +56,8 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
>
> down_read(&triggers_list_lock);
> list_for_each_entry(trig, &trigger_list, next_trig) {
> - if (sysfs_streq(buf, trig->name)) {
> + if (sysfs_streq(buf, trig->name) &&
> + trigger_relevant(led_cdev, trig)) {
> down_write(&led_cdev->trigger_lock);
> led_trigger_set(led_cdev, trig);
> up_write(&led_cdev->trigger_lock);
> @@ -96,6 +103,9 @@ static int led_trigger_format(char *buf, size_t size,
> bool hit = led_cdev->trigger &&
> !strcmp(led_cdev->trigger->name, trig->name);
>
> + if (!trigger_relevant(led_cdev, trig))
> + continue;
> +
> len += led_trigger_snprintf(buf + len, size - len,
> " %s%s%s", hit ? "[" : "",
> trig->name, hit ? "]" : "");
> @@ -243,7 +253,8 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
> down_read(&triggers_list_lock);
> down_write(&led_cdev->trigger_lock);
> list_for_each_entry(trig, &trigger_list, next_trig) {
> - if (!strcmp(led_cdev->default_trigger, trig->name)) {
> + if (!strcmp(led_cdev->default_trigger, trig->name) &&
> + trigger_relevant(led_cdev, trig)) {
> led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
> led_trigger_set(led_cdev, trig);
> break;
> @@ -280,7 +291,8 @@ int led_trigger_register(struct led_trigger *trig)
> down_write(&triggers_list_lock);
> /* Make sure the trigger's name isn't already in use */
> list_for_each_entry(_trig, &trigger_list, next_trig) {
> - if (!strcmp(_trig->name, trig->name)) {
> + if (!strcmp(_trig->name, trig->name) &&
> + (!_trig->private_led || _trig->private_led == trig->private_led)) {
> up_write(&triggers_list_lock);
> return -EEXIST;
> }
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 2451962d1ec5..cba52714558f 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -57,6 +57,10 @@ struct led_init_data {
> bool devname_mandatory;
> };
>
> +struct led_hw_trigger_type {
> + int dummy;
> +}
> +
> struct led_classdev {
> const char *name;
> enum led_brightness brightness;
> @@ -150,6 +154,8 @@ struct led_classdev {
>
> /* Ensures consistent access to the LED Flash Class device */
> struct mutex led_access;
> +
> + struct led_hw_trigger_type *trigger_type;
> };
>
> /**
> @@ -345,6 +351,9 @@ struct led_trigger {
> int (*activate)(struct led_classdev *led_cdev);
> void (*deactivate)(struct led_classdev *led_cdev);
>
> + /* LED-private triggers have this set. */
> + struct led_hw_trigger_type *trigger_type;
> +
> /* LEDs under control by this trigger (for simple triggers) */
> rwlock_t leddev_list_lock;
> struct list_head led_cdevs;
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat 2020-07-11 23:01:11, Ondřej Jirman wrote:
> Hello Pavel,
>
> On Sat, Jul 11, 2020 at 12:04:09PM +0200, Pavel Machek wrote:
> > Hi!
> >
> > > Some LED controllers may come with an internal HW triggering mechanism
> > > for the LED and an ability to switch between user control of the LED,
> > > or the internal control. One such example is AXP20X PMIC, that allows
> > > wither for user control of the LED, or for internal control based on
> > > the state of the battery charger.
> > >
> > > Add support for registering per-LED device trigger.
> > >
> > > Names of private triggers need to be globally unique, but may clash
> > > with other private triggers. This is enforced during trigger
> > > registration. Developers can register private triggers just like
> > > the normal triggers, by setting private_led to a classdev
> > > of the LED the trigger is associated with.
> >
> > What about this? Should address Marek's concerns about resource use...
>
> What concerns? Marek's concerns seem to be about case where we register
> a trigger for (each led * self-working configuration) which I admit
> can be quite a lot of triggers if there are many functions. But that's
> not my proposal.
>
> My proposal is to only register on trigger per LED at most. So on my
> system that's 1 extra trigger and on Marek's system that'd be 48 new
> triggers. Neither seems like a meaningful problem from resource
> use perspective.
So.. 48 triggers on Marek's systems means I'll not apply your patch.
Please take a look at my version, it is as simple and avoids that
problem.
If it works for you, you can submit it properly and I'll likely accept
it.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hello,
On Sun, Jul 12, 2020 at 09:25:54AM +0200, Pavel Machek wrote:
> On Sat 2020-07-11 23:01:11, Ondřej Jirman wrote:
> > Hello Pavel,
> >
> > On Sat, Jul 11, 2020 at 12:04:09PM +0200, Pavel Machek wrote:
> > > Hi!
> > >
> > > > Some LED controllers may come with an internal HW triggering mechanism
> > > > for the LED and an ability to switch between user control of the LED,
> > > > or the internal control. One such example is AXP20X PMIC, that allows
> > > > wither for user control of the LED, or for internal control based on
> > > > the state of the battery charger.
> > > >
> > > > Add support for registering per-LED device trigger.
> > > >
> > > > Names of private triggers need to be globally unique, but may clash
> > > > with other private triggers. This is enforced during trigger
> > > > registration. Developers can register private triggers just like
> > > > the normal triggers, by setting private_led to a classdev
> > > > of the LED the trigger is associated with.
> > >
> > > What about this? Should address Marek's concerns about resource use...
> >
> > What concerns? Marek's concerns seem to be about case where we register
> > a trigger for (each led * self-working configuration) which I admit
> > can be quite a lot of triggers if there are many functions. But that's
> > not my proposal.
> >
> > My proposal is to only register on trigger per LED at most. So on my
> > system that's 1 extra trigger and on Marek's system that'd be 48 new
> > triggers. Neither seems like a meaningful problem from resource
> > use perspective.
>
> So.. 48 triggers on Marek's systems means I'll not apply your patch.
>
> Please take a look at my version, it is as simple and avoids that
> problem.
I would, but I don't see your version linked or mentioned in this thread.
thank you and regards,
o.
> If it works for you, you can submit it properly and I'll likely accept
> it.
>
> Best regards,
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> > > > > Some LED controllers may come with an internal HW triggering mechanism
> > > > > for the LED and an ability to switch between user control of the LED,
> > > > > or the internal control. One such example is AXP20X PMIC, that allows
> > > > > wither for user control of the LED, or for internal control based on
> > > > > the state of the battery charger.
> > > > >
> > > > > Add support for registering per-LED device trigger.
> > > > >
> > > > > Names of private triggers need to be globally unique, but may clash
> > > > > with other private triggers. This is enforced during trigger
> > > > > registration. Developers can register private triggers just like
> > > > > the normal triggers, by setting private_led to a classdev
> > > > > of the LED the trigger is associated with.
> > > >
> > > > What about this? Should address Marek's concerns about resource use...
> > >
> > > What concerns? Marek's concerns seem to be about case where we register
> > > a trigger for (each led * self-working configuration) which I admit
> > > can be quite a lot of triggers if there are many functions. But that's
> > > not my proposal.
> > >
> > > My proposal is to only register on trigger per LED at most. So on my
> > > system that's 1 extra trigger and on Marek's system that'd be 48 new
> > > triggers. Neither seems like a meaningful problem from resource
> > > use perspective.
> >
> > So.. 48 triggers on Marek's systems means I'll not apply your patch.
> >
> > Please take a look at my version, it is as simple and avoids that
> > problem.
>
> I would, but I don't see your version linked or mentioned in this
> thread.
Ah! Sorry about that. Here it is. (I verified it compiles in the
meantime).
Best regards,
Pavel
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 79e30d2cb7a5..e8333675959c 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -27,6 +27,12 @@ LIST_HEAD(trigger_list);
/* Used by LED Class */
+static inline bool
+trigger_relevant(struct led_classdev *led_cdev, struct led_trigger *trig)
+{
+ return !trig->trigger_type || trig->trigger_type == led_cdev->trigger_type;
+}
+
ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
struct bin_attribute *bin_attr, char *buf,
loff_t pos, size_t count)
@@ -50,7 +56,8 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
down_read(&triggers_list_lock);
list_for_each_entry(trig, &trigger_list, next_trig) {
- if (sysfs_streq(buf, trig->name)) {
+ if (sysfs_streq(buf, trig->name) &&
+ trigger_relevant(led_cdev, trig)) {
down_write(&led_cdev->trigger_lock);
led_trigger_set(led_cdev, trig);
up_write(&led_cdev->trigger_lock);
@@ -96,6 +103,9 @@ static int led_trigger_format(char *buf, size_t size,
bool hit = led_cdev->trigger &&
!strcmp(led_cdev->trigger->name, trig->name);
+ if (!trigger_relevant(led_cdev, trig))
+ continue;
+
len += led_trigger_snprintf(buf + len, size - len,
" %s%s%s", hit ? "[" : "",
trig->name, hit ? "]" : "");
@@ -243,7 +253,8 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
down_read(&triggers_list_lock);
down_write(&led_cdev->trigger_lock);
list_for_each_entry(trig, &trigger_list, next_trig) {
- if (!strcmp(led_cdev->default_trigger, trig->name)) {
+ if (!strcmp(led_cdev->default_trigger, trig->name) &&
+ trigger_relevant(led_cdev, trig)) {
led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
led_trigger_set(led_cdev, trig);
break;
@@ -280,7 +291,8 @@ int led_trigger_register(struct led_trigger *trig)
down_write(&triggers_list_lock);
/* Make sure the trigger's name isn't already in use */
list_for_each_entry(_trig, &trigger_list, next_trig) {
- if (!strcmp(_trig->name, trig->name)) {
+ if (!strcmp(_trig->name, trig->name) &&
+ (!_trig->private_led || _trig->private_led == trig->private_led)) {
up_write(&triggers_list_lock);
return -EEXIST;
}
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 2451962d1ec5..cba52714558f 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -57,6 +57,10 @@ struct led_init_data {
bool devname_mandatory;
};
+struct led_hw_trigger_type {
+ int dummy;
+}
+
struct led_classdev {
const char *name;
enum led_brightness brightness;
@@ -150,6 +154,8 @@ struct led_classdev {
/* Ensures consistent access to the LED Flash Class device */
struct mutex led_access;
+
+ struct led_hw_trigger_type *trigger_type;
};
/**
@@ -345,6 +351,9 @@ struct led_trigger {
int (*activate)(struct led_classdev *led_cdev);
void (*deactivate)(struct led_classdev *led_cdev);
+ /* LED-private triggers have this set. */
+ struct led_hw_trigger_type *trigger_type;
+
/* LEDs under control by this trigger (for simple triggers) */
rwlock_t leddev_list_lock;
struct list_head led_cdevs;
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sun, 12 Jul 2020 21:11:11 +0200
Pavel Machek <[email protected]> wrote:
> Hi!
>
> > > > > > Some LED controllers may come with an internal HW triggering mechanism
> > > > > > for the LED and an ability to switch between user control of the LED,
> > > > > > or the internal control. One such example is AXP20X PMIC, that allows
> > > > > > wither for user control of the LED, or for internal control based on
> > > > > > the state of the battery charger.
> > > > > >
> > > > > > Add support for registering per-LED device trigger.
> > > > > >
> > > > > > Names of private triggers need to be globally unique, but may clash
> > > > > > with other private triggers. This is enforced during trigger
> > > > > > registration. Developers can register private triggers just like
> > > > > > the normal triggers, by setting private_led to a classdev
> > > > > > of the LED the trigger is associated with.
> > > > >
> > > > > What about this? Should address Marek's concerns about resource use...
> > > >
> > > > What concerns? Marek's concerns seem to be about case where we register
> > > > a trigger for (each led * self-working configuration) which I admit
> > > > can be quite a lot of triggers if there are many functions. But that's
> > > > not my proposal.
> > > >
> > > > My proposal is to only register on trigger per LED at most. So on my
> > > > system that's 1 extra trigger and on Marek's system that'd be 48 new
> > > > triggers. Neither seems like a meaningful problem from resource
> > > > use perspective.
> > >
> > > So.. 48 triggers on Marek's systems means I'll not apply your patch.
> > >
> > > Please take a look at my version, it is as simple and avoids that
> > > problem.
> >
> > I would, but I don't see your version linked or mentioned in this
> > thread.
>
> Ah! Sorry about that. Here it is. (I verified it compiles in the
> meantime).
>
> Best regards,
> Pavel
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 79e30d2cb7a5..e8333675959c 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -27,6 +27,12 @@ LIST_HEAD(trigger_list);
>
> /* Used by LED Class */
>
> +static inline bool
> +trigger_relevant(struct led_classdev *led_cdev, struct led_trigger *trig)
> +{
> + return !trig->trigger_type || trig->trigger_type == led_cdev->trigger_type;
> +}
> +
> ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
> struct bin_attribute *bin_attr, char *buf,
> loff_t pos, size_t count)
> @@ -50,7 +56,8 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
>
> down_read(&triggers_list_lock);
> list_for_each_entry(trig, &trigger_list, next_trig) {
> - if (sysfs_streq(buf, trig->name)) {
> + if (sysfs_streq(buf, trig->name) &&
> + trigger_relevant(led_cdev, trig)) {
> down_write(&led_cdev->trigger_lock);
> led_trigger_set(led_cdev, trig);
> up_write(&led_cdev->trigger_lock);
> @@ -96,6 +103,9 @@ static int led_trigger_format(char *buf, size_t size,
> bool hit = led_cdev->trigger &&
> !strcmp(led_cdev->trigger->name, trig->name);
>
> + if (!trigger_relevant(led_cdev, trig))
> + continue;
> +
> len += led_trigger_snprintf(buf + len, size - len,
> " %s%s%s", hit ? "[" : "",
> trig->name, hit ? "]" : "");
> @@ -243,7 +253,8 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
> down_read(&triggers_list_lock);
> down_write(&led_cdev->trigger_lock);
> list_for_each_entry(trig, &trigger_list, next_trig) {
> - if (!strcmp(led_cdev->default_trigger, trig->name)) {
> + if (!strcmp(led_cdev->default_trigger, trig->name) &&
> + trigger_relevant(led_cdev, trig)) {
> led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
> led_trigger_set(led_cdev, trig);
> break;
> @@ -280,7 +291,8 @@ int led_trigger_register(struct led_trigger *trig)
> down_write(&triggers_list_lock);
> /* Make sure the trigger's name isn't already in use */
> list_for_each_entry(_trig, &trigger_list, next_trig) {
> - if (!strcmp(_trig->name, trig->name)) {
> + if (!strcmp(_trig->name, trig->name) &&
> + (!_trig->private_led || _trig->private_led == trig->private_led)) {
> up_write(&triggers_list_lock);
> return -EEXIST;
> }
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 2451962d1ec5..cba52714558f 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -57,6 +57,10 @@ struct led_init_data {
> bool devname_mandatory;
> };
>
> +struct led_hw_trigger_type {
> + int dummy;
> +}
> +
> struct led_classdev {
> const char *name;
> enum led_brightness brightness;
> @@ -150,6 +154,8 @@ struct led_classdev {
>
> /* Ensures consistent access to the LED Flash Class device */
> struct mutex led_access;
> +
> + struct led_hw_trigger_type *trigger_type;
> };
>
> /**
> @@ -345,6 +351,9 @@ struct led_trigger {
> int (*activate)(struct led_classdev *led_cdev);
> void (*deactivate)(struct led_classdev *led_cdev);
>
> + /* LED-private triggers have this set. */
> + struct led_hw_trigger_type *trigger_type;
> +
> /* LEDs under control by this trigger (for simple triggers) */
> rwlock_t leddev_list_lock;
> struct list_head led_cdevs;
>
Hmm, this could actually work and is nicer than my proposal, since it
does not require to differentiate between a HW and SW trigger when
changing them.
Marek
Hello Pavel,
On Sun, Jul 12, 2020 at 09:11:11PM +0200, Pavel Machek wrote:
> Hi!
>
> > > > > > Some LED controllers may come with an internal HW triggering mechanism
> > > > > > for the LED and an ability to switch between user control of the LED,
> > > > > > or the internal control. One such example is AXP20X PMIC, that allows
> > > > > > wither for user control of the LED, or for internal control based on
> > > > > > the state of the battery charger.
> > > > > >
> > > > > > Add support for registering per-LED device trigger.
> > > > > >
> > > > > > Names of private triggers need to be globally unique, but may clash
> > > > > > with other private triggers. This is enforced during trigger
> > > > > > registration. Developers can register private triggers just like
> > > > > > the normal triggers, by setting private_led to a classdev
> > > > > > of the LED the trigger is associated with.
> > > > >
> > > > > What about this? Should address Marek's concerns about resource use...
> > > >
> > > > What concerns? Marek's concerns seem to be about case where we register
> > > > a trigger for (each led * self-working configuration) which I admit
> > > > can be quite a lot of triggers if there are many functions. But that's
> > > > not my proposal.
> > > >
> > > > My proposal is to only register on trigger per LED at most. So on my
> > > > system that's 1 extra trigger and on Marek's system that'd be 48 new
> > > > triggers. Neither seems like a meaningful problem from resource
> > > > use perspective.
> > >
> > > So.. 48 triggers on Marek's systems means I'll not apply your patch.
> > >
> > > Please take a look at my version, it is as simple and avoids that
> > > problem.
> >
> > I would, but I don't see your version linked or mentioned in this
> > thread.
>
> Ah! Sorry about that. Here it is. (I verified it compiles in the
> meantime).
>
> Best regards,
> Pavel
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 79e30d2cb7a5..e8333675959c 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -27,6 +27,12 @@ LIST_HEAD(trigger_list);
>
> /* Used by LED Class */
>
> +static inline bool
> +trigger_relevant(struct led_classdev *led_cdev, struct led_trigger *trig)
> +{
> + return !trig->trigger_type || trig->trigger_type == led_cdev->trigger_type;
> +}
> +
> ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
> struct bin_attribute *bin_attr, char *buf,
> loff_t pos, size_t count)
> @@ -50,7 +56,8 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
>
> down_read(&triggers_list_lock);
> list_for_each_entry(trig, &trigger_list, next_trig) {
> - if (sysfs_streq(buf, trig->name)) {
> + if (sysfs_streq(buf, trig->name) &&
> + trigger_relevant(led_cdev, trig)) {
> down_write(&led_cdev->trigger_lock);
> led_trigger_set(led_cdev, trig);
> up_write(&led_cdev->trigger_lock);
> @@ -96,6 +103,9 @@ static int led_trigger_format(char *buf, size_t size,
> bool hit = led_cdev->trigger &&
> !strcmp(led_cdev->trigger->name, trig->name);
>
> + if (!trigger_relevant(led_cdev, trig))
> + continue;
> +
> len += led_trigger_snprintf(buf + len, size - len,
> " %s%s%s", hit ? "[" : "",
> trig->name, hit ? "]" : "");
> @@ -243,7 +253,8 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
> down_read(&triggers_list_lock);
> down_write(&led_cdev->trigger_lock);
> list_for_each_entry(trig, &trigger_list, next_trig) {
> - if (!strcmp(led_cdev->default_trigger, trig->name)) {
> + if (!strcmp(led_cdev->default_trigger, trig->name) &&
> + trigger_relevant(led_cdev, trig)) {
> led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
> led_trigger_set(led_cdev, trig);
> break;
> @@ -280,7 +291,8 @@ int led_trigger_register(struct led_trigger *trig)
> down_write(&triggers_list_lock);
> /* Make sure the trigger's name isn't already in use */
> list_for_each_entry(_trig, &trigger_list, next_trig) {
> - if (!strcmp(_trig->name, trig->name)) {
> + if (!strcmp(_trig->name, trig->name) &&
> + (!_trig->private_led || _trig->private_led == trig->private_led)) {
> up_write(&triggers_list_lock);
> return -EEXIST;
> }
This would not compile, probably some stale code.
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 2451962d1ec5..cba52714558f 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -57,6 +57,10 @@ struct led_init_data {
> bool devname_mandatory;
> };
>
> +struct led_hw_trigger_type {
> + int dummy;
> +}
> +
> struct led_classdev {
> const char *name;
> enum led_brightness brightness;
> @@ -150,6 +154,8 @@ struct led_classdev {
>
> /* Ensures consistent access to the LED Flash Class device */
> struct mutex led_access;
> +
> + struct led_hw_trigger_type *trigger_type;
> };
>
> /**
> @@ -345,6 +351,9 @@ struct led_trigger {
> int (*activate)(struct led_classdev *led_cdev);
> void (*deactivate)(struct led_classdev *led_cdev);
>
> + /* LED-private triggers have this set. */
> + struct led_hw_trigger_type *trigger_type;
> +
> /* LEDs under control by this trigger (for simple triggers) */
> rwlock_t leddev_list_lock;
> struct list_head led_cdevs;
I like this proposal. I'll try to use it in my code. Thank you!
regards,
o.
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hello,
On Sun, Jul 12, 2020 at 09:11:11PM +0200, Pavel Machek wrote:
> Hi!
>
[....]
> }
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 2451962d1ec5..cba52714558f 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -57,6 +57,10 @@ struct led_init_data {
> bool devname_mandatory;
> };
>
> +struct led_hw_trigger_type {
> + int dummy;
> +}
> +
> struct led_classdev {
> const char *name;
> enum led_brightness brightness;
> @@ -150,6 +154,8 @@ struct led_classdev {
>
> /* Ensures consistent access to the LED Flash Class device */
> struct mutex led_access;
> +
> + struct led_hw_trigger_type *trigger_type;
> };
>
> /**
> @@ -345,6 +351,9 @@ struct led_trigger {
> int (*activate)(struct led_classdev *led_cdev);
> void (*deactivate)(struct led_classdev *led_cdev);
>
> + /* LED-private triggers have this set. */
> + struct led_hw_trigger_type *trigger_type;
> +
> /* LEDs under control by this trigger (for simple triggers) */
> rwlock_t leddev_list_lock;
> struct list_head led_cdevs;
So after trying to use this, this seems to disallow the use of multiple HW
triggers per LED. That's fine by me, because using one HW sysfs configured
trigger per LED that use case is my proposal, but is it desireable in general?
Drivers would be forced to use just one HW trigger + sysfs config, instead
of having freedom between choosing either that or expressing multiple hw
triggering modes via multiple differently named HW triggers.
regards,
o.
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, 13 Jul 2020 00:38:21 +0200
Ondřej Jirman <[email protected]> wrote:
> Hello,
>
> On Sun, Jul 12, 2020 at 09:11:11PM +0200, Pavel Machek wrote:
> > Hi!
> >
>
> [....]
>
> > }
> > diff --git a/include/linux/leds.h b/include/linux/leds.h
> > index 2451962d1ec5..cba52714558f 100644
> > --- a/include/linux/leds.h
> > +++ b/include/linux/leds.h
> > @@ -57,6 +57,10 @@ struct led_init_data {
> > bool devname_mandatory;
> > };
> >
> > +struct led_hw_trigger_type {
> > + int dummy;
> > +}
> > +
> > struct led_classdev {
> > const char *name;
> > enum led_brightness brightness;
> > @@ -150,6 +154,8 @@ struct led_classdev {
> >
> > /* Ensures consistent access to the LED Flash Class device */
> > struct mutex led_access;
> > +
> > + struct led_hw_trigger_type *trigger_type;
> > };
> >
> > /**
> > @@ -345,6 +351,9 @@ struct led_trigger {
> > int (*activate)(struct led_classdev *led_cdev);
> > void (*deactivate)(struct led_classdev *led_cdev);
> >
> > + /* LED-private triggers have this set. */
> > + struct led_hw_trigger_type *trigger_type;
> > +
> > /* LEDs under control by this trigger (for simple triggers) */
> > rwlock_t leddev_list_lock;
> > struct list_head led_cdevs;
>
> So after trying to use this, this seems to disallow the use of multiple HW
> triggers per LED. That's fine by me, because using one HW sysfs configured
> trigger per LED that use case is my proposal, but is it desireable in general?
Why? If you register one LED and several triggers, all sharing the same
trigger_type pointer, I think it should work.
Marek
On Mon, Jul 13, 2020 at 01:15:44AM +0200, Marek Behun wrote:
> On Mon, 13 Jul 2020 00:38:21 +0200
> Ondřej Jirman <[email protected]> wrote:
>
> > Hello,
> >
> > On Sun, Jul 12, 2020 at 09:11:11PM +0200, Pavel Machek wrote:
> > > Hi!
> > >
> >
> > [....]
> >
> > > }
> > > diff --git a/include/linux/leds.h b/include/linux/leds.h
> > > index 2451962d1ec5..cba52714558f 100644
> > > --- a/include/linux/leds.h
> > > +++ b/include/linux/leds.h
> > > @@ -57,6 +57,10 @@ struct led_init_data {
> > > bool devname_mandatory;
> > > };
> > >
> > > +struct led_hw_trigger_type {
> > > + int dummy;
> > > +}
> > > +
> > > struct led_classdev {
> > > const char *name;
> > > enum led_brightness brightness;
> > > @@ -150,6 +154,8 @@ struct led_classdev {
> > >
> > > /* Ensures consistent access to the LED Flash Class device */
> > > struct mutex led_access;
> > > +
> > > + struct led_hw_trigger_type *trigger_type;
> > > };
> > >
> > > /**
> > > @@ -345,6 +351,9 @@ struct led_trigger {
> > > int (*activate)(struct led_classdev *led_cdev);
> > > void (*deactivate)(struct led_classdev *led_cdev);
> > >
> > > + /* LED-private triggers have this set. */
> > > + struct led_hw_trigger_type *trigger_type;
> > > +
> > > /* LEDs under control by this trigger (for simple triggers) */
> > > rwlock_t leddev_list_lock;
> > > struct list_head led_cdevs;
> >
> > So after trying to use this, this seems to disallow the use of multiple HW
> > triggers per LED. That's fine by me, because using one HW sysfs configured
> > trigger per LED that use case is my proposal, but is it desireable in general?
>
> Why? If you register one LED and several triggers, all sharing the same
> trigger_type pointer, I think it should work.
Ah, you're right. :)
thanks,
o.
> Marek
On Mon, 13 Jul 2020 01:15:44 +0200
Marek Behun <[email protected]> wrote:
> On Mon, 13 Jul 2020 00:38:21 +0200
> Ondřej Jirman <[email protected]> wrote:
>
> > So after trying to use this, this seems to disallow the use of multiple HW
> > triggers per LED. That's fine by me, because using one HW sysfs configured
> > trigger per LED that use case is my proposal, but is it desireable in general?
>
> Why? If you register one LED and several triggers, all sharing the same
> trigger_type pointer, I think it should work.
>
> Marek
The problem arises when I have two LEDs and two HW triggers, and the
hardware allows setting one HW trigger on both LEDs and other HW
trigger only on one LED. But this could simply be ignored - the
set_trigger function could simply return -ENOTSUPP or something.
Marek
Hello Pavel,
On Sun, Jul 12, 2020 at 09:11:11PM +0200, Pavel Machek wrote:
> > > So.. 48 triggers on Marek's systems means I'll not apply your patch.
> > >
> > > Please take a look at my version, it is as simple and avoids that
> > > problem.
> >
> > I would, but I don't see your version linked or mentioned in this
> > thread.
>
> Ah! Sorry about that. Here it is. (I verified it compiles in the
> meantime).
I tried implementing a driver using this API:
https://megous.com/git/linux/commit/?h=tbs-a711-5.8&id=adda29f57685155ac35bd810b51befbfb266da48
and it works pretty nicely for me.
thank you and regards,
o.
On Mon 2020-07-13 01:18:41, Marek Behun wrote:
> On Mon, 13 Jul 2020 01:15:44 +0200
> Marek Behun <[email protected]> wrote:
>
> > On Mon, 13 Jul 2020 00:38:21 +0200
> > Ondřej Jirman <[email protected]> wrote:
> >
> > > So after trying to use this, this seems to disallow the use of multiple HW
> > > triggers per LED. That's fine by me, because using one HW sysfs configured
> > > trigger per LED that use case is my proposal, but is it desireable in general?
> >
> > Why? If you register one LED and several triggers, all sharing the same
> > trigger_type pointer, I think it should work.
> >
> > Marek
>
> The problem arises when I have two LEDs and two HW triggers, and the
> hardware allows setting one HW trigger on both LEDs and other HW
> trigger only on one LED. But this could simply be ignored - the
> set_trigger function could simply return -ENOTSUPP or something.
In this case you should have two trigger_type pointers (since two LEDs
are different), and yes, you'll have duplication for one of the
triggers. I don't think thats a problem.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat, 11 Jul 2020 12:04:09 +0200
Pavel Machek <[email protected]> wrote:
> What about this? Should address Marek's concerns about resource use...
>
> Best regards,
> Pavel
>
...
> @@ -280,7 +291,8 @@ int led_trigger_register(struct led_trigger *trig)
> down_write(&triggers_list_lock);
> /* Make sure the trigger's name isn't already in use */
> list_for_each_entry(_trig, &trigger_list, next_trig) {
> - if (!strcmp(_trig->name, trig->name)) {
> + if (!strcmp(_trig->name, trig->name) &&
> + (!_trig->private_led || _trig->private_led ==
> trig->private_led)) { up_write(&triggers_list_lock);
> return -EEXIST;
> }
Hi Pavel,
Your proposal does not add private_led member to struct led_trigger. I
think you forgot to change this from Ondrej's proposal.
This should instead check:
the names are same and both trigger have the same type (either none
or same). In that case return -EEXIST.
Also a couple of lines below there is code for enabling this trigger
for LEDs that have it set as default trigger. There should also be a
check whether the trigger is relevant.
In the linux/leds.h header the trigger_type in led_classdev should be
inside the CONFIG_LEDS_TRIGGERS block.
I will send new version with an example usage for a Marvell PHY driver.
Marek
On Wed, 15 Jul 2020 19:07:27 +0200
Marek Behún <[email protected]> wrote:
> This should instead check:
> the names are same and both trigger have the same type (either none
> or same). In that case return -EEXIST.
My bad, this is of course wrong. -EEXIST should be returned if the
names are same and triggers have same type or one of them has no type.