Re: [PATCH v7 02/13] leds: trigger: Move led_trigger_is_hw_controlled() to the right place

[Rong Zhang <i@rong.moe>]

Hi Lee,

Thanks for your review.

On Thu, 2026-09-24 at 09:37 +0100, Lee Jones wrote:
> --- checkpatch.pl: clean (0 issues) ---
> 
> On Mon, 21 Sep 2026, Rong Zhang wrote:
> 
> > Currently led_trigger_is_hw_controlled() is placed at led-class.c, which
> > is not an right place as it falls into the triggers namespace and does
> > triggers stuff.
> > 
> > Move it into led-triggers.c, and split it into locked and unlocked
> > variant for convenience.
> > 
> > Fixes: b819dc7d8fb2 ("leds: core: Report ENODATA for brightness of hardware controlled LED")
> > Acked-by: Ike Panhc <ikepanhc@gmail.com>
> > Signed-off-by: Rong Zhang <i@rong.moe>
> > ---
> > Changes in v3:
> > - New patch in the series, the dependency of the following patches
> > ---
> >  drivers/leds/led-class.c    | 10 ----------
> >  drivers/leds/led-triggers.c | 19 +++++++++++++++++++
> >  include/linux/leds.h        |  8 ++++++++
> >  3 files changed, 27 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > index 4259533dafa1..7110bfbe3b83 100644
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -27,16 +27,6 @@ static LIST_HEAD(leds_lookup_list);
> >  
> >  static struct workqueue_struct *leds_wq;
> >  
> > -static bool led_trigger_is_hw_controlled(struct led_classdev *led_cdev)
> > -{
> > -#ifdef CONFIG_LEDS_TRIGGERS
> > -	guard(rwsem_read)(&led_cdev->trigger_lock);
> > -	return led_cdev->trigger && led_cdev->trigger->trigger_type;
> > -#else
> > -	return false;
> > -#endif
> > -}
> > -
> >  static ssize_t brightness_show(struct device *dev,
> >  		struct device_attribute *attr, char *buf)
> >  {
> > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> > index b1223218bda1..bf2543538ed0 100644
> > --- a/drivers/leds/led-triggers.c
> > +++ b/drivers/leds/led-triggers.c
> > @@ -7,9 +7,11 @@
> >   * Author: Richard Purdie <rpurdie@openedhand.com>
> >   */
> >  
> > +#include <linux/cleanup.h>
> >  #include <linux/export.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> > +#include <linux/lockdep.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/device.h>
> >  #include <linux/timer.h>
> > @@ -33,6 +35,23 @@ trigger_relevant(struct led_classdev *led_cdev, struct led_trigger *trig)
> >  	return !trig->trigger_type || trig->trigger_type == led_cdev->trigger_type;
> >  }
> >  
> > +static bool __led_trigger_is_hw_controlled(struct led_classdev *led_cdev)
> > +{
> > +	lockdep_assert_held(&led_cdev->trigger_lock);
> > +
> > +	if (!led_cdev->trigger)
> > +		return false;
> > +
> > +	return led_cdev->trigger->trigger_type;
> 
> How does this not cause a compiler warning?

trigger_type is a pointer. Implicitly converting a pointer to boolean via
a return statement is completely valid in C11.

   6.3.1.2 Boolean type
   When any scalar value is converted to _Bool, the result is 0 if the value
   compares equal to 0; otherwise, the result is 1.

   6.8.6.4 The return statement
   ...If the expression has a type different from the return type of the
   function in which it appears, the value is converted as if by
   assignment to an object having the return type of the function.
   
> 
> This needs to be evaluated properly.

If it's preferred to make the intention clearer, how about:

	return led_cdev->trigger->trigger_type != NULL;

Thanks,
Rong

> 
> > +}
> > +
> > +bool led_trigger_is_hw_controlled(struct led_classdev *led_cdev)
> > +{
> > +	guard(rwsem_read)(&led_cdev->trigger_lock);
> > +	return __led_trigger_is_hw_controlled(led_cdev);
> > +}
> > +EXPORT_SYMBOL_GPL(led_trigger_is_hw_controlled);
> > +
> >  ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
> >  			  const struct bin_attribute *bin_attr, char *buf,
> >  			  loff_t pos, size_t count)
> > diff --git a/include/linux/leds.h b/include/linux/leds.h
> > index a515f075c29a..46ad2e03e1c6 100644
> > --- a/include/linux/leds.h
> > +++ b/include/linux/leds.h
> > @@ -534,6 +534,8 @@ void led_trigger_set_default(struct led_classdev *led_cdev);
> >  int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger);
> >  void led_trigger_remove(struct led_classdev *led_cdev);
> >  
> > +bool led_trigger_is_hw_controlled(struct led_classdev *led_cdev);
> > +
> >  static inline void led_set_trigger_data(struct led_classdev *led_cdev,
> >  					void *trigger_data)
> >  {
> > @@ -584,6 +586,12 @@ static inline int led_trigger_set(struct led_classdev *led_cdev,
> >  }
> >  
> >  static inline void led_trigger_remove(struct led_classdev *led_cdev) {}
> > +
> > +static inline bool led_trigger_is_hw_controlled(struct led_classdev *led_cdev)
> > +{
> > +	return false;
> > +}
> > +
> >  static inline void led_set_trigger_data(struct led_classdev *led_cdev) {}
> >  static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
> >  {
> > 
> > -- 
> > 2.55.0
> >