Re: [PATCH v8 5/5] backlight: add led-backlight driver

2019-10-03 Thread Jean-Jacques Hiblot

Sebastian,

On 03/10/2019 13:47, Sebastian Reichel wrote:

Hi,

On Thu, Oct 03, 2019 at 10:28:12AM +0200, Jean-Jacques Hiblot wrote:

From: Tomi Valkeinen 

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 
Signed-off-by: Jean-Jacques Hiblot 
Acked-by: Pavel Machek 
Reviewed-by: Daniel Thompson 
---

Reviewed-by: Sebastian Reichel 

(with some suggestions below)


[...]



I suggest to restructure:

1. call led_sysfs_disable

2. use devm_add_action_or_reset() to register the
led_sysfs_enable loop

3. use devm_backlight_device_register() to register BL

4. drop the remove function


+
+   backlight_update_status(priv->bl_dev);
+
+   return 0;
+}
+
+static int led_bl_remove(struct platform_device *pdev)
+{
+   struct led_bl_data *priv = platform_get_drvdata(pdev);
+   struct backlight_device *bl = priv->bl_dev;
+   int i;
+
+   backlight_device_unregister(bl);
+
+   led_bl_power_off(priv);
+   for (i = 0; i < priv->nb_leds; i++)
+   led_sysfs_enable(priv->leds[i]);
+
+   return 0;
+}
+
+static const struct of_device_id led_bl_of_match[] = {
+   { .compatible = "led-backlight" },
+   { }
+};
+
+MODULE_DEVICE_TABLE(of, led_bl_of_match);
+
+static struct platform_driver led_bl_driver = {
+   .driver = {
+   .name   = "led-backlight",
+   .of_match_table = of_match_ptr(led_bl_of_match),

You should drop of_match_ptr(). Since the driver depends on OF,
it will always simply return led_bl_of_match.

(Also after removing the OF dependency from the driver it would
either require led_bl_of_match to be marked __maybe_unused or
moving it into a #if CONFIG_OF area to avoid warnings.)


Thanks for the suggestions. I think I'll work on them as a separate 
thing and post them after this is merged if there are no others changes 
required.


JJ



-- Sebastian


+   },
+   .probe  = led_bl_probe,
+   .remove = led_bl_remove,
+};
+
+module_platform_driver(led_bl_driver);
+
+MODULE_DESCRIPTION("LED based Backlight Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:led-backlight");
--
2.17.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v8 5/5] backlight: add led-backlight driver

2019-10-03 Thread Sebastian Reichel
Hi,

On Thu, Oct 03, 2019 at 10:28:12AM +0200, Jean-Jacques Hiblot wrote:
> From: Tomi Valkeinen 
> 
> 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 
> Signed-off-by: Jean-Jacques Hiblot 
> Acked-by: Pavel Machek 
> Reviewed-by: Daniel Thompson 
> ---

Reviewed-by: Sebastian Reichel 

(with some suggestions below)

>  drivers/video/backlight/Kconfig  |   7 +
>  drivers/video/backlight/Makefile |   1 +
>  drivers/video/backlight/led_bl.c | 260 +++
>  3 files changed, 268 insertions(+)
>  create mode 100644 drivers/video/backlight/led_bl.c
> 
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 8b081d61773e..585a1787618c 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -458,6 +458,13 @@ config BACKLIGHT_RAVE_SP
>   help
> Support for backlight control on RAVE SP device.
>  
> +config BACKLIGHT_LED
> + tristate "Generic LED based Backlight Driver"
> + depends on LEDS_CLASS && OF
> + help
> +   If you have a LCD backlight adjustable by LED class driver, say Y
> +   to enable this driver.
> +
>  endif # BACKLIGHT_CLASS_DEVICE
>  
>  endmenu
> diff --git a/drivers/video/backlight/Makefile 
> b/drivers/video/backlight/Makefile
> index 63c507c07437..2a67642966a5 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -57,3 +57,4 @@ obj-$(CONFIG_BACKLIGHT_TPS65217)+= tps65217_bl.o
>  obj-$(CONFIG_BACKLIGHT_WM831X)   += wm831x_bl.o
>  obj-$(CONFIG_BACKLIGHT_ARCXCNN)  += arcxcnn_bl.o
>  obj-$(CONFIG_BACKLIGHT_RAVE_SP)  += rave-sp-backlight.o
> +obj-$(CONFIG_BACKLIGHT_LED)  += led_bl.o
> diff --git a/drivers/video/backlight/led_bl.c 
> b/drivers/video/backlight/led_bl.c
> new file mode 100644
> index ..3f66549997c8
> --- /dev/null
> +++ b/drivers/video/backlight/led_bl.c
> @@ -0,0 +1,260 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2015-2019 Texas Instruments Incorporated -  
> http://www.ti.com/
> + * Author: Tomi Valkeinen 
> + *
> + * Based on pwm_bl.c
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct led_bl_data {
> + struct device   *dev;
> + struct backlight_device *bl_dev;
> + struct led_classdev **leds;
> + boolenabled;
> + int nb_leds;
> + unsigned int*levels;
> + unsigned intdefault_brightness;
> + unsigned intmax_brightness;
> +};
> +
> +static void led_bl_set_brightness(struct led_bl_data *priv, int level)
> +{
> + int i;
> + int bkl_brightness;
> +
> + if (priv->levels)
> + bkl_brightness = priv->levels[level];
> + else
> + bkl_brightness = level;
> +
> + for (i = 0; i < priv->nb_leds; i++)
> + led_set_brightness(priv->leds[i], bkl_brightness);
> +
> + priv->enabled = true;
> +}
> +
> +static void led_bl_power_off(struct led_bl_data *priv)
> +{
> + int i;
> +
> + if (!priv->enabled)
> + return;
> +
> + for (i = 0; i < priv->nb_leds; i++)
> + led_set_brightness(priv->leds[i], LED_OFF);
> +
> + priv->enabled = false;
> +}
> +
> +static int led_bl_update_status(struct backlight_device *bl)
> +{
> + struct led_bl_data *priv = bl_get_data(bl);
> + int brightness = bl->props.brightness;
> +
> + if (bl->props.power != FB_BLANK_UNBLANK ||
> + bl->props.fb_blank != FB_BLANK_UNBLANK ||
> + bl->props.state & BL_CORE_FBBLANK)
> + brightness = 0;
> +
> + if (brightness > 0)
> + led_bl_set_brightness(priv, brightness);
> + else
> + led_bl_power_off(priv);
> +
> + return 0;
> +}
> +
> +static const struct backlight_ops led_bl_ops = {
> + .update_status  = led_bl_update_status,
> +};
> +
> +static int led_bl_get_leds(struct device *dev,
> +struct led_bl_data *priv)
> +{
> + int i, nb_leds, ret;
> + struct device_node *node = dev->of_node;
> + struct led_classdev **leds;
> + unsigned int max_brightness;
> + unsigned int default_brightness;
> +
> + ret = of_count_phandle_with_args(node, "leds", NULL);
> + if (ret < 0) {
> + dev_err(dev, "Unable to get led count\n");
> + return -EINVAL;
> + }
> +
> + nb_leds = ret;
> + if (nb_leds < 1) {
> + dev_err(dev, "At least one LED must be specified!\n");
> + return -EINVAL;
> + }
> +
> + leds = devm_kzalloc(dev, sizeof(struct led_classdev *) * nb_leds,
> + GFP_KERNEL);
> + if (!leds)
> + return -ENOMEM;
> +
> + for (i = 0; i < nb_leds; i++) {
> +