Re: [PATCH 12/13] leds: backlight trigger: Replace fb events with a dedicated function call

[Lee Jones <lee@kernel.org>]

On Thu, 13 Feb 2025, Thomas Zimmermann wrote:

> Hi
> 
> Am 11.02.25 um 14:57 schrieb Lee Jones:
> > 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@suse.de>
> > > ---
> > >   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?
> 
> It's being renamed here; probably something to avoid.
> 
> 
> > 
> > >   {
> > >   	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?
> 
> I don't think so. This ifdef covers the case that fbdev is built in, but led
> is a module (i.e., fbdev=y && led=m).
> 
> > /
> > > +	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?
> 
> Because of (fbdev=y && led=m) again. ledtrig_backlight_blank() would be
> defined then, but not linkable from fbdev. Preferably, I'd rather leave out
> the ifdef in the led header file.

#ifdefs are not generally welcome in C-files.  Please rework it.

-- 
Lee Jones [李琼斯]