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@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?
>
> 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 [李琼斯]
