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
