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

[Andrew Lunn <andrew@lunn.ch>]

> 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.

>  #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