On Thu, 06 Feb 2025, Thomas Zimmermann wrote:
> Remove support for fb events from the led backlight trigger. Provide the
> helper ledtrig_backlight_blank() instead. Call it from fbdev to inform
> the trigger of changes to a display's blank state.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
> drivers/leds/trigger/ledtrig-backlight.c | 31 +++++-------------------
> drivers/video/fbdev/core/fbmem.c | 21 +++++++++-------
> include/linux/leds.h | 6 +++++
> 3 files changed, 24 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/leds/trigger/ledtrig-backlight.c b/drivers/leds/trigger/ledtrig-backlight.c
> index f9317f93483b..e3ae4adc29e3 100644
> --- a/drivers/leds/trigger/ledtrig-backlight.c
> +++ b/drivers/leds/trigger/ledtrig-backlight.c
> @@ -10,7 +10,6 @@
> #include <linux/kernel.h>
> #include <linux/slab.h>
> #include <linux/init.h>
> -#include <linux/fb.h>
> #include <linux/leds.h>
> #include "../leds.h"
>
> @@ -21,7 +20,6 @@ struct bl_trig_notifier {
> struct led_classdev *led;
> int brightness;
> int old_status;
> - struct notifier_block notifier;
> unsigned invert;
>
> struct list_head entry;
> @@ -30,7 +28,7 @@ struct bl_trig_notifier {
> static struct list_head ledtrig_backlight_list;
> static struct mutex ledtrig_backlight_list_mutex;
>
> -static void ledtrig_backlight_blank(struct bl_trig_notifier *n, bool on)
> +static void __ledtrig_backlight_blank(struct bl_trig_notifier *n, bool on)
I'm confused. Didn't you just introduce this?
> {
> struct led_classdev *led = n->led;
> int new_status = !on ? BLANK : UNBLANK;
> @@ -48,23 +46,14 @@ static void ledtrig_backlight_blank(struct bl_trig_notifier *n, bool on)
> n->old_status = new_status;
> }
>
> -static int fb_notifier_callback(struct notifier_block *p,
> - unsigned long event, void *data)
> +void ledtrig_backlight_blank(bool on)
> {
> - struct bl_trig_notifier *n = container_of(p,
> - struct bl_trig_notifier, notifier);
> - struct fb_event *fb_event = data;
> - int *blank;
> -
> - /* If we aren't interested in this event, skip it immediately ... */
> - if (event != FB_EVENT_BLANK)
> - return 0;
> -
> - blank = fb_event->data;
> + struct bl_trig_notifier *n;
>
> - ledtrig_backlight_blank(n, !blank[0]);
> + guard(mutex)(&ledtrig_backlight_list_mutex);
>
> - return 0;
> + list_for_each_entry(n, &ledtrig_backlight_list, entry)
> + __ledtrig_backlight_blank(n, on);
> }
>
> static ssize_t bl_trig_invert_show(struct device *dev,
> @@ -110,8 +99,6 @@ ATTRIBUTE_GROUPS(bl_trig);
>
> static int bl_trig_activate(struct led_classdev *led)
> {
> - int ret;
> -
> struct bl_trig_notifier *n;
>
> n = kzalloc(sizeof(struct bl_trig_notifier), GFP_KERNEL);
> @@ -122,11 +109,6 @@ static int bl_trig_activate(struct led_classdev *led)
> n->led = led;
> n->brightness = led->brightness;
> n->old_status = UNBLANK;
> - n->notifier.notifier_call = fb_notifier_callback;
> -
> - ret = fb_register_client(&n->notifier);
> - if (ret)
> - dev_err(led->dev, "unable to register backlight trigger\n");
>
> mutex_lock(&ledtrig_backlight_list_mutex);
> list_add(&n->entry, &ledtrig_backlight_list);
> @@ -143,7 +125,6 @@ static void bl_trig_deactivate(struct led_classdev *led)
> list_del(&n->entry);
> mutex_unlock(&ledtrig_backlight_list_mutex);
>
> - fb_unregister_client(&n->notifier);
> kfree(n);
> }
>
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index fb7ca143a996..92c3fe2a7aff 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -16,6 +16,7 @@
> #include <linux/fb.h>
> #include <linux/fbcon.h>
> #include <linux/lcd.h>
> +#include <linux/leds.h>
>
> #include <video/nomodeset.h>
>
> @@ -373,11 +374,19 @@ static void fb_lcd_notify_blank(struct fb_info *info)
> #endif
> }
>
> +static void fb_ledtrig_backlight_notify_blank(struct fb_info *info)
> +{
> +#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_BACKLIGHT)
#iferry is generally discouraged in C files.
Does is_defined() work for you?
/
> + if (info->blank == FB_BLANK_UNBLANK)
> + ledtrig_backlight_blank(true);
If !CONFIG_LEDS_TRIGGER_BACKLIGHT(), then ledtrig_backlight_blank() is a
noop anyway, right? So why encompass it in the #if at all?
> + else
> + ledtrig_backlight_blank(false);
> +#endif
> +}
> +
> int fb_blank(struct fb_info *info, int blank)
> {
> int old_blank = info->blank;
> - struct fb_event event;
> - int data[2];
> int ret;
>
> if (!info->fbops->fb_blank)
> @@ -386,11 +395,6 @@ int fb_blank(struct fb_info *info, int blank)
> if (blank > FB_BLANK_POWERDOWN)
> blank = FB_BLANK_POWERDOWN;
>
> - data[0] = blank;
> - data[1] = old_blank;
> - event.info = info;
> - event.data = data;
> -
> info->blank = blank;
>
> ret = info->fbops->fb_blank(blank, info);
> @@ -399,8 +403,7 @@ int fb_blank(struct fb_info *info, int blank)
>
> fb_bl_notify_blank(info, old_blank);
> fb_lcd_notify_blank(info);
> -
> - fb_notifier_call_chain(FB_EVENT_BLANK, &event);
> + fb_ledtrig_backlight_notify_blank(info);
>
> return 0;
>
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 98f9719c924c..8c7c41888f7d 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -640,6 +640,12 @@ static inline void ledtrig_flash_ctrl(bool on) {}
> static inline void ledtrig_torch_ctrl(bool on) {}
> #endif
>
> +#if IS_ENABLED(CONFIG_LEDS_TRIGGER_BACKLIGHT)
> +void ledtrig_backlight_blank(bool on);
> +#else
> +static inline void ledtrig_backlight_blank(bool on) {}
> +#endif
> +
> /*
> * Generic LED platform data for describing LED names and default triggers.
> */
> --
> 2.48.1
>
--
/fb_ledtrig_backlight_notify_blankLee Jones [李琼斯]
