| Message ID | 30d49088-283c-40f3-b97b-fd5f5174a467@gmail.com |
|---|---|
| Headers | show |
| Series | leds: trigger: Improve handling of led_trigger_event() and simplify mute audio trigger | expand |
On Tue, 13 Feb 2024 08:30:30 +0100, Heiner Kallweit wrote: > > If a simple trigger is assigned to a LED, then the LED may be off until > the next led_trigger_event() call. This may be an issue for simple > triggers with rare led_trigger_event() calls, e.g. power supply > charging indicators (drivers/power/supply/power_supply_leds.c). > Therefore persist the brightness value of the last led_trigger_event() > call and use this value if the trigger is assigned to a LED. > This change allows to use simple triggers in more cases. > As a first use case simplify handling of the mute audio trigger. > > This series touches few subsystems. I'd propose to handle it via > the LED subsystem. > > Heiner Kallweit (4): > leds: trigger: Store brightness set by led_trigger_event() > ALSA: control-led: Integrate mute led trigger > Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER > leds: trigger: audio: Remove this trigger LGTM. Reviewed-by: Takashi Iwai <tiwai@suse.de> One thing I'm not 100% sure is the movement from ledtrig:audio-mute and ledtrig:audio-micmute alias into snd-ctl-led module. Who would use/process those aliases? I don't think this would be a problem, but it might change the loading order. Thanks! Takashi
On 15.02.2024 13:29, Takashi Iwai wrote: > On Tue, 13 Feb 2024 08:30:30 +0100, > Heiner Kallweit wrote: >> >> If a simple trigger is assigned to a LED, then the LED may be off until >> the next led_trigger_event() call. This may be an issue for simple >> triggers with rare led_trigger_event() calls, e.g. power supply >> charging indicators (drivers/power/supply/power_supply_leds.c). >> Therefore persist the brightness value of the last led_trigger_event() >> call and use this value if the trigger is assigned to a LED. >> This change allows to use simple triggers in more cases. >> As a first use case simplify handling of the mute audio trigger. >> >> This series touches few subsystems. I'd propose to handle it via >> the LED subsystem. >> >> Heiner Kallweit (4): >> leds: trigger: Store brightness set by led_trigger_event() >> ALSA: control-led: Integrate mute led trigger >> Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER >> leds: trigger: audio: Remove this trigger > > LGTM. > > Reviewed-by: Takashi Iwai <tiwai@suse.de> > > One thing I'm not 100% sure is the movement from ledtrig:audio-mute > and ledtrig:audio-micmute alias into snd-ctl-led module. Who would > use/process those aliases? I don't think this would be a problem, but > it might change the loading order. > The ledtrig:% aliases are used when a LED device is registered that has a default trigger. Like in the case here with the input leds (patch 3). There might also be DT-defined LEDs with a audio mute default trigger. snd-ctl-led has a dependency on snd, so at least wrt snd the load order doesn't change. > > Thanks! > > Takashi Heiner
On Tue, 13 Feb 2024, Heiner Kallweit wrote: > If a simple trigger is assigned to a LED, then the LED may be off until > the next led_trigger_event() call. This may be an issue for simple > triggers with rare led_trigger_event() calls, e.g. power supply > charging indicators (drivers/power/supply/power_supply_leds.c). > Therefore persist the brightness value of the last led_trigger_event() > call and use this value if the trigger is assigned to a LED. > This change allows to use simple triggers in more cases. > As a first use case simplify handling of the mute audio trigger. > > This series touches few subsystems. I'd propose to handle it via > the LED subsystem. > > Heiner Kallweit (4): > leds: trigger: Store brightness set by led_trigger_event() > ALSA: control-led: Integrate mute led trigger > Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER > leds: trigger: audio: Remove this trigger > > arch/mips/configs/ci20_defconfig | 1 - > drivers/input/input-leds.c | 8 +--- > drivers/leds/led-triggers.c | 6 ++- > drivers/leds/trigger/Kconfig | 7 --- > drivers/leds/trigger/Makefile | 1 - > drivers/leds/trigger/ledtrig-audio.c | 67 ---------------------------- > include/linux/leds.h | 29 ++++++------ > sound/core/Kconfig | 1 - > sound/core/control_led.c | 20 +++++++-- > 9 files changed, 37 insertions(+), 103 deletions(-) > delete mode 100644 drivers/leds/trigger/ledtrig-audio.c Are the sound maintainers on-board with this?
On Fri, 23 Feb 2024 16:45:59 +0100, Lee Jones wrote: > > On Tue, 13 Feb 2024, Heiner Kallweit wrote: > > > If a simple trigger is assigned to a LED, then the LED may be off until > > the next led_trigger_event() call. This may be an issue for simple > > triggers with rare led_trigger_event() calls, e.g. power supply > > charging indicators (drivers/power/supply/power_supply_leds.c). > > Therefore persist the brightness value of the last led_trigger_event() > > call and use this value if the trigger is assigned to a LED. > > This change allows to use simple triggers in more cases. > > As a first use case simplify handling of the mute audio trigger. > > > > This series touches few subsystems. I'd propose to handle it via > > the LED subsystem. > > > > Heiner Kallweit (4): > > leds: trigger: Store brightness set by led_trigger_event() > > ALSA: control-led: Integrate mute led trigger > > Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER > > leds: trigger: audio: Remove this trigger > > > > arch/mips/configs/ci20_defconfig | 1 - > > drivers/input/input-leds.c | 8 +--- > > drivers/leds/led-triggers.c | 6 ++- > > drivers/leds/trigger/Kconfig | 7 --- > > drivers/leds/trigger/Makefile | 1 - > > drivers/leds/trigger/ledtrig-audio.c | 67 ---------------------------- > > include/linux/leds.h | 29 ++++++------ > > sound/core/Kconfig | 1 - > > sound/core/control_led.c | 20 +++++++-- > > 9 files changed, 37 insertions(+), 103 deletions(-) > > delete mode 100644 drivers/leds/trigger/ledtrig-audio.c > > Are the sound maintainers on-board with this? See https://lore.kernel.org/r/87zfw1ewrd.wl-tiwai@suse.de thanks, Takashi
On Fri, 23 Feb 2024, Takashi Iwai wrote: > On Fri, 23 Feb 2024 16:45:59 +0100, > Lee Jones wrote: > > > > On Tue, 13 Feb 2024, Heiner Kallweit wrote: > > > > > If a simple trigger is assigned to a LED, then the LED may be off until > > > the next led_trigger_event() call. This may be an issue for simple > > > triggers with rare led_trigger_event() calls, e.g. power supply > > > charging indicators (drivers/power/supply/power_supply_leds.c). > > > Therefore persist the brightness value of the last led_trigger_event() > > > call and use this value if the trigger is assigned to a LED. > > > This change allows to use simple triggers in more cases. > > > As a first use case simplify handling of the mute audio trigger. > > > > > > This series touches few subsystems. I'd propose to handle it via > > > the LED subsystem. > > > > > > Heiner Kallweit (4): > > > leds: trigger: Store brightness set by led_trigger_event() > > > ALSA: control-led: Integrate mute led trigger > > > Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER > > > leds: trigger: audio: Remove this trigger > > > > > > arch/mips/configs/ci20_defconfig | 1 - > > > drivers/input/input-leds.c | 8 +--- > > > drivers/leds/led-triggers.c | 6 ++- > > > drivers/leds/trigger/Kconfig | 7 --- > > > drivers/leds/trigger/Makefile | 1 - > > > drivers/leds/trigger/ledtrig-audio.c | 67 ---------------------------- > > > include/linux/leds.h | 29 ++++++------ > > > sound/core/Kconfig | 1 - > > > sound/core/control_led.c | 20 +++++++-- > > > 9 files changed, 37 insertions(+), 103 deletions(-) > > > delete mode 100644 drivers/leds/trigger/ledtrig-audio.c > > > > Are the sound maintainers on-board with this? > > See > https://lore.kernel.org/r/87zfw1ewrd.wl-tiwai@suse.de Were you happy with Heiner's response?
On Tue, 13 Feb 2024, Heiner Kallweit wrote: > If a simple trigger is assigned to a LED, then the LED may be off until > the next led_trigger_event() call. This may be an issue for simple > triggers with rare led_trigger_event() calls, e.g. power supply > charging indicators (drivers/power/supply/power_supply_leds.c). > Therefore persist the brightness value of the last led_trigger_event() > call and use this value if the trigger is assigned to a LED. > This change allows to use simple triggers in more cases. > As a first use case simplify handling of the mute audio trigger. > > This series touches few subsystems. I'd propose to handle it via > the LED subsystem. > > Heiner Kallweit (4): > leds: trigger: Store brightness set by led_trigger_event() > ALSA: control-led: Integrate mute led trigger > Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER Dmitry, you happy too? > leds: trigger: audio: Remove this trigger > > arch/mips/configs/ci20_defconfig | 1 - > drivers/input/input-leds.c | 8 +--- > drivers/leds/led-triggers.c | 6 ++- > drivers/leds/trigger/Kconfig | 7 --- > drivers/leds/trigger/Makefile | 1 - > drivers/leds/trigger/ledtrig-audio.c | 67 ---------------------------- > include/linux/leds.h | 29 ++++++------ > sound/core/Kconfig | 1 - > sound/core/control_led.c | 20 +++++++-- > 9 files changed, 37 insertions(+), 103 deletions(-) > delete mode 100644 drivers/leds/trigger/ledtrig-audio.c > > -- > 2.43.1 >
On Fri, 23 Feb 2024 17:04:15 +0100, Lee Jones wrote: > > On Fri, 23 Feb 2024, Takashi Iwai wrote: > > > On Fri, 23 Feb 2024 16:45:59 +0100, > > Lee Jones wrote: > > > > > > On Tue, 13 Feb 2024, Heiner Kallweit wrote: > > > > > > > If a simple trigger is assigned to a LED, then the LED may be off until > > > > the next led_trigger_event() call. This may be an issue for simple > > > > triggers with rare led_trigger_event() calls, e.g. power supply > > > > charging indicators (drivers/power/supply/power_supply_leds.c). > > > > Therefore persist the brightness value of the last led_trigger_event() > > > > call and use this value if the trigger is assigned to a LED. > > > > This change allows to use simple triggers in more cases. > > > > As a first use case simplify handling of the mute audio trigger. > > > > > > > > This series touches few subsystems. I'd propose to handle it via > > > > the LED subsystem. > > > > > > > > Heiner Kallweit (4): > > > > leds: trigger: Store brightness set by led_trigger_event() > > > > ALSA: control-led: Integrate mute led trigger > > > > Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER > > > > leds: trigger: audio: Remove this trigger > > > > > > > > arch/mips/configs/ci20_defconfig | 1 - > > > > drivers/input/input-leds.c | 8 +--- > > > > drivers/leds/led-triggers.c | 6 ++- > > > > drivers/leds/trigger/Kconfig | 7 --- > > > > drivers/leds/trigger/Makefile | 1 - > > > > drivers/leds/trigger/ledtrig-audio.c | 67 ---------------------------- > > > > include/linux/leds.h | 29 ++++++------ > > > > sound/core/Kconfig | 1 - > > > > sound/core/control_led.c | 20 +++++++-- > > > > 9 files changed, 37 insertions(+), 103 deletions(-) > > > > delete mode 100644 drivers/leds/trigger/ledtrig-audio.c > > > > > > Are the sound maintainers on-board with this? > > > > See > > https://lore.kernel.org/r/87zfw1ewrd.wl-tiwai@suse.de > > Were you happy with Heiner's response? Yes. Takashi
On Tue, 13 Feb 2024, Heiner Kallweit wrote: > If a simple trigger is assigned to a LED, then the LED may be off until > the next led_trigger_event() call. This may be an issue for simple > triggers with rare led_trigger_event() calls, e.g. power supply > charging indicators (drivers/power/supply/power_supply_leds.c). > Therefore persist the brightness value of the last led_trigger_event() > call and use this value if the trigger is assigned to a LED. > This change allows to use simple triggers in more cases. > As a first use case simplify handling of the mute audio trigger. > > This series touches few subsystems. I'd propose to handle it via > the LED subsystem. > > Heiner Kallweit (4): > leds: trigger: Store brightness set by led_trigger_event() > ALSA: control-led: Integrate mute led trigger > Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER > leds: trigger: audio: Remove this trigger > > arch/mips/configs/ci20_defconfig | 1 - > drivers/input/input-leds.c | 8 +--- This does not apply. Please rebase onto v6.8-rc1. > drivers/leds/led-triggers.c | 6 ++- > drivers/leds/trigger/Kconfig | 7 --- > drivers/leds/trigger/Makefile | 1 - > drivers/leds/trigger/ledtrig-audio.c | 67 ---------------------------- > include/linux/leds.h | 29 ++++++------ > sound/core/Kconfig | 1 - > sound/core/control_led.c | 20 +++++++-- > 9 files changed, 37 insertions(+), 103 deletions(-) > delete mode 100644 drivers/leds/trigger/ledtrig-audio.c > > -- > 2.43.1 >
On 29.02.2024 18:26, Lee Jones wrote: > On Tue, 13 Feb 2024, Heiner Kallweit wrote: > >> If a simple trigger is assigned to a LED, then the LED may be off until >> the next led_trigger_event() call. This may be an issue for simple >> triggers with rare led_trigger_event() calls, e.g. power supply >> charging indicators (drivers/power/supply/power_supply_leds.c). >> Therefore persist the brightness value of the last led_trigger_event() >> call and use this value if the trigger is assigned to a LED. >> This change allows to use simple triggers in more cases. >> As a first use case simplify handling of the mute audio trigger. >> >> This series touches few subsystems. I'd propose to handle it via >> the LED subsystem. >> >> Heiner Kallweit (4): >> leds: trigger: Store brightness set by led_trigger_event() >> ALSA: control-led: Integrate mute led trigger >> Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER >> leds: trigger: audio: Remove this trigger >> >> arch/mips/configs/ci20_defconfig | 1 - > >> drivers/input/input-leds.c | 8 +--- > > This does not apply. > > Please rebase onto v6.8-rc1. > Since v6.8-rc1 the following has been added, which is touched by my series: 698b43780ba2 ("Input: leds - set default-trigger for mute") Rebasing onto v6.8-rc1 would mean: - remove the change to input-leds from the series - resubmit this change via input subsystem This would affect bisectability, because for the time being input-leds would reference a config symbol that doesn't exist any longer. We'd be fine only if the change to input-leds is applied first. I think that's the best way to go, if you can't accept a series based on linux-next. >> drivers/leds/led-triggers.c | 6 ++- >> drivers/leds/trigger/Kconfig | 7 --- >> drivers/leds/trigger/Makefile | 1 - >> drivers/leds/trigger/ledtrig-audio.c | 67 ---------------------------- >> include/linux/leds.h | 29 ++++++------ >> sound/core/Kconfig | 1 - >> sound/core/control_led.c | 20 +++++++-- >> 9 files changed, 37 insertions(+), 103 deletions(-) >> delete mode 100644 drivers/leds/trigger/ledtrig-audio.c >> >> -- >> 2.43.1 >> >
On Sat, 02 Mar 2024, Heiner Kallweit wrote: > On 29.02.2024 18:26, Lee Jones wrote: > > On Tue, 13 Feb 2024, Heiner Kallweit wrote: > > > >> If a simple trigger is assigned to a LED, then the LED may be off until > >> the next led_trigger_event() call. This may be an issue for simple > >> triggers with rare led_trigger_event() calls, e.g. power supply > >> charging indicators (drivers/power/supply/power_supply_leds.c). > >> Therefore persist the brightness value of the last led_trigger_event() > >> call and use this value if the trigger is assigned to a LED. > >> This change allows to use simple triggers in more cases. > >> As a first use case simplify handling of the mute audio trigger. > >> > >> This series touches few subsystems. I'd propose to handle it via > >> the LED subsystem. > >> > >> Heiner Kallweit (4): > >> leds: trigger: Store brightness set by led_trigger_event() > >> ALSA: control-led: Integrate mute led trigger > >> Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER > >> leds: trigger: audio: Remove this trigger > >> > >> arch/mips/configs/ci20_defconfig | 1 - > > > >> drivers/input/input-leds.c | 8 +--- > > > > This does not apply. > > > > Please rebase onto v6.8-rc1. > > > Since v6.8-rc1 the following has been added, which is touched by > my series: > 698b43780ba2 ("Input: leds - set default-trigger for mute") > > Rebasing onto v6.8-rc1 would mean: > - remove the change to input-leds from the series > - resubmit this change via input subsystem > > This would affect bisectability, because for the time being > input-leds would reference a config symbol that doesn't exist > any longer. > > We'd be fine only if the change to input-leds is applied first. > I think that's the best way to go, if you can't accept a series > based on linux-next. Then it's going to have to wait until v6.10.
On Tue, 05 Mar 2024 10:55:39 +0100, Lee Jones wrote: > > On Sat, 02 Mar 2024, Heiner Kallweit wrote: > > > On 29.02.2024 18:26, Lee Jones wrote: > > > On Tue, 13 Feb 2024, Heiner Kallweit wrote: > > > > > >> If a simple trigger is assigned to a LED, then the LED may be off until > > >> the next led_trigger_event() call. This may be an issue for simple > > >> triggers with rare led_trigger_event() calls, e.g. power supply > > >> charging indicators (drivers/power/supply/power_supply_leds.c). > > >> Therefore persist the brightness value of the last led_trigger_event() > > >> call and use this value if the trigger is assigned to a LED. > > >> This change allows to use simple triggers in more cases. > > >> As a first use case simplify handling of the mute audio trigger. > > >> > > >> This series touches few subsystems. I'd propose to handle it via > > >> the LED subsystem. > > >> > > >> Heiner Kallweit (4): > > >> leds: trigger: Store brightness set by led_trigger_event() > > >> ALSA: control-led: Integrate mute led trigger > > >> Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER > > >> leds: trigger: audio: Remove this trigger > > >> > > >> arch/mips/configs/ci20_defconfig | 1 - > > > > > >> drivers/input/input-leds.c | 8 +--- > > > > > > This does not apply. > > > > > > Please rebase onto v6.8-rc1. > > > > > Since v6.8-rc1 the following has been added, which is touched by > > my series: > > 698b43780ba2 ("Input: leds - set default-trigger for mute") > > > > Rebasing onto v6.8-rc1 would mean: > > - remove the change to input-leds from the series > > - resubmit this change via input subsystem > > > > This would affect bisectability, because for the time being > > input-leds would reference a config symbol that doesn't exist > > any longer. > > > > We'd be fine only if the change to input-leds is applied first. > > I think that's the best way to go, if you can't accept a series > > based on linux-next. > > Then it's going to have to wait until v6.10. Or merging via input tree? The changes are relatively small and easy, after all. thanks, Takashi
On Tue, 05 Mar 2024, Takashi Iwai wrote: > On Tue, 05 Mar 2024 10:55:39 +0100, > Lee Jones wrote: > > > > On Sat, 02 Mar 2024, Heiner Kallweit wrote: > > > > > On 29.02.2024 18:26, Lee Jones wrote: > > > > On Tue, 13 Feb 2024, Heiner Kallweit wrote: > > > > > > > >> If a simple trigger is assigned to a LED, then the LED may be off until > > > >> the next led_trigger_event() call. This may be an issue for simple > > > >> triggers with rare led_trigger_event() calls, e.g. power supply > > > >> charging indicators (drivers/power/supply/power_supply_leds.c). > > > >> Therefore persist the brightness value of the last led_trigger_event() > > > >> call and use this value if the trigger is assigned to a LED. > > > >> This change allows to use simple triggers in more cases. > > > >> As a first use case simplify handling of the mute audio trigger. > > > >> > > > >> This series touches few subsystems. I'd propose to handle it via > > > >> the LED subsystem. > > > >> > > > >> Heiner Kallweit (4): > > > >> leds: trigger: Store brightness set by led_trigger_event() > > > >> ALSA: control-led: Integrate mute led trigger > > > >> Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER > > > >> leds: trigger: audio: Remove this trigger > > > >> > > > >> arch/mips/configs/ci20_defconfig | 1 - > > > > > > > >> drivers/input/input-leds.c | 8 +--- > > > > > > > > This does not apply. > > > > > > > > Please rebase onto v6.8-rc1. > > > > > > > Since v6.8-rc1 the following has been added, which is touched by > > > my series: > > > 698b43780ba2 ("Input: leds - set default-trigger for mute") > > > > > > Rebasing onto v6.8-rc1 would mean: > > > - remove the change to input-leds from the series > > > - resubmit this change via input subsystem > > > > > > This would affect bisectability, because for the time being > > > input-leds would reference a config symbol that doesn't exist > > > any longer. > > > > > > We'd be fine only if the change to input-leds is applied first. > > > I think that's the best way to go, if you can't accept a series > > > based on linux-next. > > > > Then it's going to have to wait until v6.10. > > Or merging via input tree? > The changes are relatively small and easy, after all. That's likely to culminate in similar merge-conflicts when further changes are made to: drivers/leds/led-triggers.c drivers/leds/trigger/Kconfig drivers/leds/trigger/Makefile drivers/leds/trigger/ledtrig-audio.c include/linux/leds.h What happens if I take all but the Input change? If this doesn't cause build-failures and the Input change will definitely land in v6.9, it could work.
On Tue, 13 Feb 2024 08:30:30 +0100, Heiner Kallweit wrote: > If a simple trigger is assigned to a LED, then the LED may be off until > the next led_trigger_event() call. This may be an issue for simple > triggers with rare led_trigger_event() calls, e.g. power supply > charging indicators (drivers/power/supply/power_supply_leds.c). > Therefore persist the brightness value of the last led_trigger_event() > call and use this value if the trigger is assigned to a LED. > This change allows to use simple triggers in more cases. > As a first use case simplify handling of the mute audio trigger. > > [...] Applied, thanks! [1/4] leds: trigger: Store brightness set by led_trigger_event() commit: 575129855dee0e364af7df84a77ab5cca54b1442 [2/4] ALSA: control-led: Integrate mute led trigger commit: ba8adb1646ee498029ac12b20e792d9d0dd17920 [3/4] Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER (no commit info) [4/4] leds: trigger: audio: Remove this trigger (no commit info) -- Lee Jones [李琼斯]