> 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
