Re: [RFC PATCH v6 01/11] leds: add support for hardware driven LEDs

[Ansuel Smith <ansuelsmth@gmail.com>]

On Thu, May 05, 2022 at 12:19:11AM +0200, Andrew Lunn wrote:
> > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > index 6a8ea94834fa..3516ae3c4c3c 100644
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -164,6 +164,27 @@ static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev)
> >  }
> >  #endif
> >  
> > +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> > +static int led_classdev_check_blink_mode_functions(struct led_classdev *led_cdev)
> > +{
> > +	int mode = led_cdev->blink_mode;
> > +
> 
> We try to avoid #ifdef in code. I suggest you use
> 
>    if (IS_ENABLED(CONFIG_LEDS_HARDWARE_CONTROL)) {
>    }
> 
> You then get compiler coverage independent of if the option is on or
> off.
> 
> > +	if (mode == SOFTWARE_HARDWARE_CONTROLLED &&
> > +	    (!led_cdev->hw_control_status ||
> > +	    !led_cdev->hw_control_start ||
> > +	    !led_cdev->hw_control_stop))
> > +		return -EINVAL;
> > +
> > +	if (mode == SOFTWARE_CONTROLLED &&
> > +	    (led_cdev->hw_control_status ||
> > +	    led_cdev->hw_control_start ||
> > +	    led_cdev->hw_control_stop))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +#endif
> > +
> >  /**
> >   * led_classdev_suspend - suspend an led_classdev.
> >   * @led_cdev: the led_classdev to suspend.
> > @@ -367,6 +388,12 @@ int led_classdev_register_ext(struct device *parent,
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> > +	ret = led_classdev_check_blink_mode_functions(led_cdev);
> > +	if (ret < 0)
> > +		return ret;
> > +#endif
> 
> You can then drop this #ifdef, since it will return 0 by default when
> disabled, and the compiler should optimize it all out.
> 
> > @@ -154,6 +160,32 @@ struct led_classdev {
> >  
> >  	/* LEDs that have private triggers have this set */
> >  	struct led_hw_trigger_type	*trigger_type;
> > +
> > +	/* This report the supported blink_mode. The driver should report the
> > +	 * correct LED capabilities.
> > +	 * With this set to HARDWARE_CONTROLLED, LED is always in offload mode
> > +	 * and triggers can't be simulated by software.
> > +	 * If the led is HARDWARE_CONTROLLED, status/start/stop function
> > +	 * are optional.
> > +	 * By default SOFTWARE_CONTROLLED is set as blink_mode.
> > +	 */
> > +	enum led_blink_modes	blink_mode;
> > +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> > +	/* Ask the LED driver if hardware mode is enabled or not.
> > +	 * If the option is not declared by the LED driver, SOFTWARE_CONTROLLED
> > +	 * is assumed.
> > +	 * Triggers will check if the hardware mode is supported and will be
> > +	 * activated accordingly. If the trigger can't run in hardware mode,
> > +	 * return -EOPNOTSUPP as the blinking can't be simulated by software.
> > +	 */
> > +	bool			(*hw_control_status)(struct led_classdev *led_cdev);
> > +	/* Set LED in hardware mode */
> > +	int			(*hw_control_start)(struct led_classdev *led_cdev);
> > +	/* Disable hardware mode for LED. It's advised to the LED driver to put it to
> > +	 * the old status but that is not mandatory and also putting it off is accepted.
> > +	 */
> > +	int			(*hw_control_stop)(struct led_classdev *led_cdev);
> > +#endif
> 
> I'm surprised this builds. It looked like you accessed these two
> members even when the option was disabled. I would keep them even when
> the option is disabled. Two pointers don't add much overhead, and it
> makes the drivers simpler.
> 

Yhea sorry about this... I proposed this as an RFC as it was old code
that I just refreshed and I'm really checking the implementation...
Will do the ifdef changes in the next version.

> >  #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
> > @@ -215,7 +247,6 @@ extern struct led_classdev *of_led_get(struct device_node *np, int index);
> >  extern void led_put(struct led_classdev *led_cdev);
> >  struct led_classdev *__must_check devm_of_led_get(struct device *dev,
> >  						  int index);
> > -
> 
> Unrelated white space change.
> 
> 	  Andrew

-- 
	Ansuel