Re: [PATCH v4 4/4] backlight: add led-backlight driver

[Daniel Thompson]

On Wed, Jul 17, 2019 at 04:15:14PM +0200, Jean-Jacques Hiblot wrote:
> From: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
> 
> This patch adds a led-backlight driver (led_bl), which is similar to
> pwm_bl except the driver uses a LED class driver to adjust the
> brightness in the HW. Multiple LEDs can be used for a single backlight.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@xxxxxx>
> Acked-by: Pavel Machek <pavel@xxxxxx>
> ---
>  drivers/video/backlight/Kconfig  |   7 +
>  drivers/video/backlight/Makefile |   1 +
>  drivers/video/backlight/led_bl.c | 268 +++++++++++++++++++++++++++++++
>  3 files changed, 276 insertions(+)
>  create mode 100644 drivers/video/backlight/led_bl.c
> 
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> new file mode 100644
> index 000000000000..ac5ff78e7859
> --- /dev/null
> +++ b/drivers/video/backlight/led_bl.c
> @@ -0,0 +1,268 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2015-2019 Texas Instruments Incorporated -  http://www.ti.com/
> + * Author: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
> + *
> + * Based on pwm_bl.c
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/gpio/consumer.h>

Why do we need this header file?

> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define BKL_FULL_BRIGHTNESS 255

If we really need to have a full intensity constant then shouldn't we
use LED_FULL directly.

> +
> +struct led_bl_data {
> +	struct device		*dev;
> +	struct backlight_device	*bl_dev;
> +	struct led_classdev	**leds;
> +	bool			enabled;
> +	int			nb_leds;
> +	unsigned int		*levels;
> +	unsigned int		default_brightness;
> +	unsigned int		max_brightness;
> +};
> +
> +static int to_led_brightness(struct led_classdev *led, int value)
> +{
> +	return (value * led->max_brightness) / BKL_FULL_BRIGHTNESS;

This code looks broken.

For example led->max_brightness is 127 then the value this
function will pick values is in the interval 0..63 which is
wrong since we are not using the full range of the LED.

Similarly led->max_brightness is > 255 then we'll generate values
that are out-of-range


Daniel.