On Sat, 24 Nov 2018 00:33:09 +0100, Pavel Machek wrote:
Hi!
> > You have general-purpose LED, yet you are treating it as "something > > special". That means ugly code (quoted above) and lack of flexibility. > >
I'd prefer this to be normal LED and "mic muted" to become normal trigger.
But how would you solve the existing problem?
As already mentioned, you'll need to hook the LED trigger and the actual mixer value change. This is the biggest missing piece, and it's the very reason we have the exported symbol from the platform driver side.
So, if you prefer in that way, please implement that for the existing driver (thinkpad_acpi and dell-laptop) at first. I'll be really happy to get rid of the present ugly solution! But it's been there just because it's not so trivial at all. FWIW, this must be all done inside the kernel; otherwise you'll hit a regression.
Ok, what about something like this?
Tested, and it did not work. I guess I hooked it up at the wrong place in LED subsystem... or maybe thinkpad x60 is wrong machine to test on.
Anyway, it looks less ugly than current code in alsa. We should not really be using mixer settings to turn LED on and off.
Plus, it works in similar way triggers and LEDs "usually" do, and has all the flexibility.
Yes, thanks, that's something similar as what I had in mind, too. I guess it's just a matter of thinkpad_acpi side implementation that differs from your expectation.
However, one remaining problem is that the state will be inconsistent depending on the driver module order, if we get rid of the dynamic symbol binding. Then both modules become completely individual and thinkpad_acpi can be loaded at anytime later than HD-audio codec. If a mixer state is already changed before loading thinkpad_acpi, this event is lost and the state may be different in both sides.
So, we'd need to record the state in the mute-trigger side, and add a function to return the current state. Then thinkpad_acpi will query the state at the initialization time.
Also, we'll need also the normal mute LED in addition to the mic mute LED, so there need to be two triggers.
In anyway, moving to this direction requires the leds class implementation for dell-laptop.c as well as the new huawei stuff. So, I'd really like to have the "already good working" code before actually hacking this, so that we can see and track the functional regression more obviously.
I can start hacking this in the next week. At least I have a Dell laptop and I can check the part of the changes locally.
Since the changes will be fairly cross-tree, it's better to have an immutable branch to start with. I'd branch off at 4.20-rc3 (or rc4), apply the current patches, so that it can be merged to all relevant trees cleanly.
Andy, could you give your sign-off for the platform part of Huawei bits?
thanks,
Takashi
Signed-off-by: Pavel Machek pavel@ucw.cz
commit e7d6d170f2f45ea7a42c9ebb31869f440382e3ad
leds: ledtrig-sound: provide indication of muted microphone Signed-off-by: Pavel Machek <pavel@ucw.cz>diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile index 9bcb64e..c5ea19e 100644 --- a/drivers/leds/trigger/Makefile +++ b/drivers/leds/trigger/Makefile @@ -14,3 +14,4 @@ obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o obj-$(CONFIG_LEDS_TRIGGER_NETDEV) += ledtrig-netdev.o obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o +obj-y += ledtrig-sound.o diff --git a/drivers/leds/trigger/ledtrig-sound.c b/drivers/leds/trigger/ledtrig-sound.c new file mode 100644 index 0000000..608df35 --- /dev/null +++ b/drivers/leds/trigger/ledtrig-sound.c @@ -0,0 +1,23 @@ +/* GPLv2+.
- Copyright 2018 Pavel Machek pavel@ucw.cz
- */
+#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/leds.h>
+DEFINE_LED_TRIGGER(ledtrig_micmute);
+void ledtrig_mic_muted(bool muted) +{
- led_trigger_event(ledtrig_micmute, muted ? LED_FULL : LED_OFF);
+} +EXPORT_SYMBOL(ledtrig_mic_muted);
+static int __init ledtrig_micmute_init(void) +{
- led_trigger_register_simple("mic-muted", &ledtrig_micmute);
- return 0;
+} +device_initcall(ledtrig_micmute_init); diff --git a/include/linux/leds.h b/include/linux/leds.h index e4a9a00..c30e1cb 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -494,6 +494,14 @@ static inline void ledtrig_cpu(enum cpu_led_event evt) } #endif
+#ifdef CONFIG_LEDS_TRIGGERS +extern void ledtrig_mic_muted(bool m); +#else +static inline void ledtrig_mic_muted(bool m) {} +#endif
#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED extern void led_classdev_notify_brightness_hw_changed( struct led_classdev *led_cdev, enum led_brightness brightness); diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c index 276150f..3ec1f61 100644 --- a/sound/pci/hda/hda_generic.c +++ b/sound/pci/hda/hda_generic.c @@ -29,6 +29,7 @@ #include <linux/string.h> #include <linux/bitops.h> #include <linux/module.h> +#include <linux/leds.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/tlv.h> @@ -3929,6 +3930,7 @@ static void call_micmute_led_update(struct hda_codec *codec) val = !spec->micmute_led.capture; break; }
ledtrig_mic_muted(!spec->micmute_led.capture);
if (val == spec->micmute_led.led_value) return;
diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c index 568575b..f1b2265 100644 --- a/sound/pci/hda/thinkpad_helper.c +++ b/sound/pci/hda/thinkpad_helper.c @@ -23,6 +23,8 @@ static void update_tpacpi_mute_led(void *private_data, int enabled) if (old_vmaster_hook) old_vmaster_hook(private_data, enabled);
- printk("mute led... %d\n", !enabled);
- if (led_set_func) led_set_func(TPACPI_LED_MUTE, !enabled);
}
-- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [2 Digital signature <application/pgp-signature (7bit)>]