| Message ID | 20210809122910.11580-1-phil@nwl.cc |
|---|---|
| State | New |
| Headers | show |
| Series | leds: trigger: Add invert attribute to ledtrig-audio | expand |
Hi Pavel, On Mon, Aug 09, 2021 at 08:11:18PM +0200, Pavel Machek wrote: > > Inverting micmute LED used to be possible via a mixer setting, but > > conversion to LEDs class (probably) killed it. Re-establish the old > > functionality via sysfs attribute in audio LED triggers. > > So we have both invert and inverted attributes. Fun :-). Hmm! :) Are you talking about LED_BLINK_INVERT flag? I see a few triggers allow inversion but didn't find LED drivers exporting such a property. > See sysfs-class-led and sysfs-class-led-trigger-oneshot. I think I "copied" from oneshot trigger when writing this patch. > We definitely want this documented. We probably want this for most > triggers, maybe it should get one implementation in library somewhere? Should this be an implicit attribute of simple triggers only or all? In the latter case (which could simplify some triggers) I guess the value inversion has to take place in led_set_brightness_nopm(), the lowest level function triggers may use. How does inversion work, actually? LED_OFF <-> LED_ON is trivial, but what about LED_HALF and LED_FULL? Leaving LED_HALF as-is seems logical, but the opposite of LED_OFF might be LED_ON or LED_FULL. Does max_brightness determine that? > > Otherwise it makes sense. > > > +static ssize_t do_invert_store(enum led_audio type, > > + const char *buf, size_t size) > > +{ > > + unsigned long state; > > + int ret; > > + > > + ret = kstrtoul(buf, 0, &state); > > + if (ret) > > + return ret; > > + > > + led_invert[type] = !!state; > > + ledtrig_audio_set(type, audio_state[type]); > > Accepting 42 as valid value sounds wrong. Anyway, this should do what > oneshot trigger does. Similarities to oneshot are not a coincidence. :) Cheers, Phil
Hi! > > On Mon, Aug 09, 2021 at 08:11:18PM +0200, Pavel Machek wrote: > > > Inverting micmute LED used to be possible via a mixer setting, but > > > conversion to LEDs class (probably) killed it. Re-establish the old > > > functionality via sysfs attribute in audio LED triggers. > > > > So we have both invert and inverted attributes. Fun :-). > > Hmm! :) > > Are you talking about LED_BLINK_INVERT flag? I see a few triggers allow > inversion but didn't find LED drivers exporting such a property. > > > See sysfs-class-led and sysfs-class-led-trigger-oneshot. > > I think I "copied" from oneshot trigger when writing this patch. > > > We definitely want this documented. We probably want this for most > > triggers, maybe it should get one implementation in library somewhere? > > Should this be an implicit attribute of simple triggers only or all? In > the latter case (which could simplify some triggers) I guess the value > inversion has to take place in led_set_brightness_nopm(), the lowest > level function triggers may use. Actual inversion is trivial (so it may not make sense to share), but sysfs interface is not. Sharing that would be good. > How does inversion work, actually? LED_OFF <-> LED_ON is trivial, but > what about LED_HALF and LED_FULL? Leaving LED_HALF as-is seems logical, > but the opposite of LED_OFF might be LED_ON or LED_FULL. Does > max_brightness determine that? max_brightness determines that, and it would be simply if (->inverted) b = ->max_brighntess-b; Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
diff --git a/drivers/leds/trigger/ledtrig-audio.c b/drivers/leds/trigger/ledtrig-audio.c index f76621e88482d..319ac4f514dfd 100644 --- a/drivers/leds/trigger/ledtrig-audio.c +++ b/drivers/leds/trigger/ledtrig-audio.c @@ -6,9 +6,12 @@ #include <linux/kernel.h> #include <linux/leds.h> #include <linux/module.h> +#include <linux/slab.h> +#include <linux/sysfs.h> static struct led_trigger *ledtrig_audio[NUM_AUDIO_LEDS]; static enum led_brightness audio_state[NUM_AUDIO_LEDS]; +static bool led_invert[NUM_AUDIO_LEDS]; enum led_brightness ledtrig_audio_get(enum led_audio type) { @@ -18,17 +21,112 @@ EXPORT_SYMBOL_GPL(ledtrig_audio_get); void ledtrig_audio_set(enum led_audio type, enum led_brightness state) { + if (led_invert[type]) + state = !state; + audio_state[type] = state; led_trigger_event(ledtrig_audio[type], state); } EXPORT_SYMBOL_GPL(ledtrig_audio_set); +static ssize_t do_invert_show(enum led_audio type, char *buf) +{ + return sprintf(buf, "%u\n", led_invert[type]); +} + +static ssize_t mute_invert_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return do_invert_show(LED_AUDIO_MUTE, buf); +} + +static ssize_t micmute_invert_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return do_invert_show(LED_AUDIO_MICMUTE, buf); +} + +static ssize_t do_invert_store(enum led_audio type, + const char *buf, size_t size) +{ + unsigned long state; + int ret; + + ret = kstrtoul(buf, 0, &state); + if (ret) + return ret; + + led_invert[type] = !!state; + ledtrig_audio_set(type, audio_state[type]); + + return size; +} + +static ssize_t mute_invert_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + return do_invert_store(LED_AUDIO_MUTE, buf, size); +} + +static ssize_t micmute_invert_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + return do_invert_store(LED_AUDIO_MICMUTE, buf, size); +} + +static struct device_attribute dev_attr_mute_invert = + __ATTR(invert, 0644, mute_invert_show, mute_invert_store); + +static struct attribute *audio_mute_trig_attrs[] = { + &dev_attr_mute_invert.attr, + NULL +}; +ATTRIBUTE_GROUPS(audio_mute_trig); + +static struct device_attribute dev_attr_micmute_invert = + __ATTR(invert, 0644, micmute_invert_show, micmute_invert_store); + +static struct attribute *audio_micmute_trig_attrs[] = { + &dev_attr_micmute_invert.attr, + NULL +}; +ATTRIBUTE_GROUPS(audio_micmute_trig); + +static void __init do_ledtrig_audio_init(const char *name, + enum led_audio type, + const struct attribute_group **groups) +{ + struct led_trigger *trigger; + int err; + + trigger = kzalloc(sizeof(struct led_trigger), GFP_KERNEL); + if (!trigger) { + pr_warn("LED trigger %s failed to register (no memory)\n", + name); + goto out; + } + + trigger->name = name; + trigger->groups = groups; + + err = led_trigger_register(trigger); + if (err < 0) { + kfree(trigger); + trigger = NULL; + pr_warn("LED trigger %s failed to register (%d)\n", name, err); + } +out: + ledtrig_audio[type] = trigger; +} + static int __init ledtrig_audio_init(void) { - led_trigger_register_simple("audio-mute", - &ledtrig_audio[LED_AUDIO_MUTE]); - led_trigger_register_simple("audio-micmute", - &ledtrig_audio[LED_AUDIO_MICMUTE]); + do_ledtrig_audio_init("audio-mute", LED_AUDIO_MUTE, + audio_mute_trig_groups); + do_ledtrig_audio_init("audio-micmute", LED_AUDIO_MICMUTE, + audio_micmute_trig_groups); return 0; } module_init(ledtrig_audio_init);
Inverting micmute LED used to be possible via a mixer setting, but conversion to LEDs class (probably) killed it. Re-establish the old functionality via sysfs attribute in audio LED triggers. Signed-off-by: Phil Sutter <phil@nwl.cc> --- drivers/leds/trigger/ledtrig-audio.c | 106 ++++++++++++++++++++++++++- 1 file changed, 102 insertions(+), 4 deletions(-)