[PATCH 0/4] Add a generic driver for LED-based backlight

[PATCH 0/4] Add a generic driver for LED-based backlight

[Jean-Jacques Hiblot]

This series aims to add a led-backlight driver, similar to pwm-backlight,
but using a LED class device underneath.

A few years ago (2015), Tomi Valkeinen posted a series implementing a
backlight driver on top of a LED device:
https://patchwork.kernel.org/patch/7293991/
https://patchwork.kernel.org/patch/7294001/
https://patchwork.kernel.org/patch/7293981/

The discussion stopped because Tomi lacked the time to work on it.

This series takes it from there and implements the binding that was
discussed in https://patchwork.kernel.org/patch/7293991/. In this new
binding the backlight device is a child of the LED controller instead of
being another platform device that uses a phandle to reference a LED.

Jean-Jacques Hiblot (2):
  leds: of: create a child device if the LED node contains a
    "compatible" string
  devicetree: Update led binding

Tomi Valkeinen (2):
  backlight: add led-backlight driver
  devicetree: Add led-backlight binding

 .../devicetree/bindings/leds/common.txt       |   3 +
 .../video/backlight/led-backlight.txt         |  39 ++++
 drivers/leds/led-class.c                      |  16 ++
 drivers/video/backlight/Kconfig               |   7 +
 drivers/video/backlight/Makefile              |   1 +
 drivers/video/backlight/led_bl.c              | 217 ++++++++++++++++++
 include/linux/leds.h                          |  11 +
 7 files changed, 294 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
 create mode 100644 drivers/video/backlight/led_bl.c

-- 
2.17.1

[PATCH 1/4] leds: of: create a child device if the LED node contains a "compatible" string

[Jean-Jacques Hiblot]

This allows the LED core to probe a consumer device when the LED is
registered. In that way the LED can be seen like a minimalist bus that
can handle at most one device.
This is useful to manage simple devices, the purpose of which is
mostly to drive a LED.

One example would be a LED-controlled backlight. The device-tree would look
like the following:

tlc59116@40 {
	compatible = "ti,tlc59108";
	reg = <0x40>;

	bl@2 {
		label = "backlight";
		reg = <0x2>;

		compatible = "led-backlight";
		brightness-levels = <0 243 245 247 248 249 251 252 255>;
		default-brightness-level = <8>;
	};
};

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 drivers/leds/led-class.c | 16 ++++++++++++++++
 include/linux/leds.h     | 11 +++++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 4793e77808e2..abf0e63285b9 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -14,6 +14,7 @@
 #include <linux/leds.h>
 #include <linux/list.h>
 #include <linux/module.h>
+#include <linux/of_platform.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
@@ -306,6 +307,17 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
 
 	mutex_unlock(&led_cdev->led_access);
 
+	/* Parse the LED's node in the device-tree and create the consumer
+	 * device if needed.
+	 */
+	if (np) {
+		struct platform_device *pd;
+
+		pd = of_platform_device_create(np, NULL, led_cdev->dev);
+		if (pd)
+			led_cdev->consumer = &pd->dev;
+	}
+
 	dev_dbg(parent, "Registered led device: %s\n",
 			led_cdev->name);
 
@@ -321,6 +333,10 @@ EXPORT_SYMBOL_GPL(of_led_classdev_register);
  */
 void led_classdev_unregister(struct led_classdev *led_cdev)
 {
+	/* destroy the consummer device before removing anything else */
+	if (led_cdev->consumer)
+		of_platform_device_destroy(led_cdev->consumer, NULL);
+
 #ifdef CONFIG_LEDS_TRIGGERS
 	down_write(&led_cdev->trigger_lock);
 	if (led_cdev->trigger)
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 9b2bf574a17a..63feb8495f3e 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -91,6 +91,12 @@ struct led_classdev {
 	int (*pattern_clear)(struct led_classdev *led_cdev);
 
 	struct device		*dev;
+	/*
+	 * The consumer device is a child device created by the LED core if the
+	 * appropriate information (ie "compatible" string) is available in the
+	 * device tree. Its life cycle follows the life cycle of the LED device.
+	 */
+	struct device		*consumer;
 	const struct attribute_group	**groups;
 
 	struct list_head	 node;			/* LED Device list */
@@ -141,6 +147,11 @@ extern void devm_led_classdev_unregister(struct device *parent,
 extern void led_classdev_suspend(struct led_classdev *led_cdev);
 extern void led_classdev_resume(struct led_classdev *led_cdev);
 
+static inline struct led_classdev *to_led_classdev(struct device *dev)
+{
+	return (struct led_classdev *) dev_get_drvdata(dev);
+}
+
 /**
  * led_blink_set - set blinking with software fallback
  * @led_cdev: the LED to start blinking
-- 
2.17.1

[PATCH 2/4] devicetree: Update led binding

[Jean-Jacques Hiblot]

Update the led binding to describe the possibility to add a "compatible"
option to create a child-device, user of the LED.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Cc: devicetree@vger.kernel.org
---
 Documentation/devicetree/bindings/leds/common.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 70876ac11367..2f7882528d97 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -11,6 +11,9 @@ have to be tightly coupled with the LED device binding. They are represented
 by child nodes of the parent LED device binding.
 
 Optional properties for child nodes:
+- compatible : driver name for a child-device. This child-device is the user
+               of the LED. It is created when the LED is registered and
+	       destroyed when the LED is unregistered.
 - led-sources : List of device current outputs the LED is connected to. The
 		outputs are identified by the numbers that must be defined
 		in the LED device binding documentation.
-- 
2.17.1

Re: [PATCH 2/4] devicetree: Update led binding

[Dan Murphy]

JJ

On 7/1/19 10:14 AM, Jean-Jacques Hiblot wrote:
> Update the led binding to describe the possibility to add a "compatible"
> option to create a child-device, user of the LED.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Cc: devicetree@vger.kernel.org
> ---
>   Documentation/devicetree/bindings/leds/common.txt | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index 70876ac11367..2f7882528d97 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -11,6 +11,9 @@ have to be tightly coupled with the LED device binding. They are represented
>   by child nodes of the parent LED device binding.
>   
>   Optional properties for child nodes:
> +- compatible : driver name for a child-device. This child-device is the user
> +               of the LED. It is created when the LED is registered and
> +	       destroyed when the LED is unregistered.

Can you update the example to show usage?

Dan


>   - led-sources : List of device current outputs the LED is connected to. The
>   		outputs are identified by the numbers that must be defined
>   		in the LED device binding documentation.

[PATCH 3/4] backlight: add led-backlight driver

[Jean-Jacques Hiblot]

From: Tomi Valkeinen <tomi.valkeinen@ti.com>

This patch adds a led-backlight driver (led_bl), which is mostly similar to
pwm_bl except the driver uses a LED class driver to adjust the brightness
in the HW.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 drivers/video/backlight/Kconfig  |   7 +
 drivers/video/backlight/Makefile |   1 +
 drivers/video/backlight/led_bl.c | 217 +++++++++++++++++++++++++++++++
 3 files changed, 225 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 000000000000..e699924cc2bc
--- /dev/null
+++ b/drivers/video/backlight/led_bl.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2015-2018 Texas Instruments Incorporated -  http://www.ti.com/
+ * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
+ *
+ * Based on pwm_bl.c
+ */
+
+#include <linux/backlight.h>
+#include <linux/gpio/consumer.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+struct led_bl_data {
+	struct device		*dev;
+	struct backlight_device	*bl_dev;
+
+	unsigned int		*levels;
+	bool			enabled;
+	struct regulator	*power_supply;
+	struct gpio_desc	*enable_gpio;
+
+	struct led_classdev *led_cdev;
+
+	unsigned int max_brightness;
+	unsigned int default_brightness;
+};
+
+static void led_bl_set_brightness(struct led_bl_data *priv, int brightness)
+{
+	int err;
+
+	if (!priv->enabled) {
+		err = regulator_enable(priv->power_supply);
+		if (err < 0)
+			dev_err(priv->dev, "failed to enable power supply\n");
+
+		if (priv->enable_gpio)
+			gpiod_set_value_cansleep(priv->enable_gpio, 1);
+	}
+
+	led_set_brightness(priv->led_cdev, priv->levels[brightness]);
+
+	priv->enabled = true;
+}
+
+static void led_bl_power_off(struct led_bl_data *priv)
+{
+	if (!priv->enabled)
+		return;
+
+	led_set_brightness(priv->led_cdev, LED_OFF);
+
+	if (priv->enable_gpio)
+		gpiod_set_value_cansleep(priv->enable_gpio, 0);
+
+	regulator_disable(priv->power_supply);
+
+	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_parse_dt(struct device *dev,
+			   struct led_bl_data *priv)
+{
+	struct device_node *node = dev->of_node;
+	int num_levels;
+	u32 *levels;
+	u32 value;
+	int ret;
+
+	if (!node)
+		return -ENODEV;
+
+	num_levels = of_property_count_u32_elems(node, "brightness-levels");
+	if (num_levels < 0)
+		return num_levels;
+
+	levels = devm_kzalloc(dev, sizeof(u32) * num_levels, GFP_KERNEL);
+	if (!levels)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(node, "brightness-levels",
+					 levels,
+					 num_levels);
+	if (ret < 0)
+		return ret;
+
+	ret = of_property_read_u32(node, "default-brightness-level", &value);
+	if (ret < 0)
+		return ret;
+
+	if (value >= num_levels) {
+		dev_err(dev, "invalid default-brightness-level\n");
+		return -EINVAL;
+	}
+
+	priv->levels = levels;
+	priv->max_brightness = num_levels - 1;
+	priv->default_brightness = value;
+
+	return 0;
+}
+
+static int led_bl_probe(struct platform_device *pdev)
+{
+	struct backlight_properties props;
+	struct led_bl_data *priv;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+
+	priv->dev = &pdev->dev;
+	priv->led_cdev = to_led_classdev(pdev->dev.parent);
+
+	ret = led_bl_parse_dt(&pdev->dev, priv);
+	if (ret < 0) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "failed to parse DT data\n");
+		return ret;
+	}
+
+	priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
+			    GPIOD_OUT_LOW);
+	if (IS_ERR(priv->enable_gpio)) {
+		ret = PTR_ERR(priv->enable_gpio);
+		goto err;
+	}
+
+	priv->power_supply = devm_regulator_get(&pdev->dev, "power");
+	if (IS_ERR(priv->power_supply)) {
+		ret = PTR_ERR(priv->power_supply);
+		goto err;
+	}
+
+	memset(&props, 0, sizeof(struct backlight_properties));
+	props.type = BACKLIGHT_RAW;
+	props.max_brightness = priv->max_brightness;
+	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
+			&pdev->dev, priv, &led_bl_ops, &props);
+	if (IS_ERR(priv->bl_dev)) {
+		dev_err(&pdev->dev, "failed to register backlight\n");
+		ret = PTR_ERR(priv->bl_dev);
+		goto err;
+	}
+
+	priv->bl_dev->props.brightness = priv->default_brightness;
+	backlight_update_status(priv->bl_dev);
+
+	return 0;
+
+err:
+
+	return ret;
+}
+
+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;
+
+	backlight_device_unregister(bl);
+
+	led_bl_power_off(priv);
+
+	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),
+	},
+	.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

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

[Daniel Thompson]

On Mon, Jul 01, 2019 at 05:14:22PM +0200, Jean-Jacques Hiblot wrote:
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> This patch adds a led-backlight driver (led_bl), which is mostly similar to
> pwm_bl except the driver uses a LED class driver to adjust the brightness
> in the HW.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>  drivers/video/backlight/Kconfig  |   7 +
>  drivers/video/backlight/Makefile |   1 +
>  drivers/video/backlight/led_bl.c | 217 +++++++++++++++++++++++++++++++
>  3 files changed, 225 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 000000000000..e699924cc2bc
> --- /dev/null
> +++ b/drivers/video/backlight/led_bl.c
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2015-2018 Texas Instruments Incorporated -  http://www.ti.com/
> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> + *
> + * Based on pwm_bl.c
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +struct led_bl_data {
> +	struct device		*dev;
> +	struct backlight_device	*bl_dev;
> +
> +	unsigned int		*levels;
> +	bool			enabled;
> +	struct regulator	*power_supply;
> +	struct gpio_desc	*enable_gpio;

For the PWM driver the power_supply and enable_gpio are part of managing
a dumb LED driver device that is downstream of the PWM.

What is their purpose when we wrap an LED device? Put another why why isn't
the LED device driver responsible for this?

> +
> +	struct led_classdev *led_cdev;
> +
> +	unsigned int max_brightness;
> +	unsigned int default_brightness;
> +};
> +
> +static void led_bl_set_brightness(struct led_bl_data *priv, int brightness)
> +{
> +	int err;
> +
> +	if (!priv->enabled) {
> +		err = regulator_enable(priv->power_supply);
> +		if (err < 0)
> +			dev_err(priv->dev, "failed to enable power supply\n");
> +
> +		if (priv->enable_gpio)
> +			gpiod_set_value_cansleep(priv->enable_gpio, 1);
> +	}
> +
> +	led_set_brightness(priv->led_cdev, priv->levels[brightness]);
> +
> +	priv->enabled = true;
> +}
> +
> +static void led_bl_power_off(struct led_bl_data *priv)
> +{
> +	if (!priv->enabled)
> +		return;
> +
> +	led_set_brightness(priv->led_cdev, LED_OFF);
> +
> +	if (priv->enable_gpio)
> +		gpiod_set_value_cansleep(priv->enable_gpio, 0);
> +
> +	regulator_disable(priv->power_supply);
> +
> +	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_parse_dt(struct device *dev,
> +			   struct led_bl_data *priv)
> +{
> +	struct device_node *node = dev->of_node;
> +	int num_levels;
> +	u32 *levels;
> +	u32 value;
> +	int ret;
> +
> +	if (!node)
> +		return -ENODEV;
> +
> +	num_levels = of_property_count_u32_elems(node, "brightness-levels");

Is there any reason that this function cannot use the (more generic)
device property API throughout this function?



Daniel.


> +	if (num_levels < 0)
> +		return num_levels;
> +
> +	levels = devm_kzalloc(dev, sizeof(u32) * num_levels, GFP_KERNEL);
> +	if (!levels)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_array(node, "brightness-levels",
> +					 levels,
> +					 num_levels);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = of_property_read_u32(node, "default-brightness-level", &value);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (value >= num_levels) {
> +		dev_err(dev, "invalid default-brightness-level\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->levels = levels;
> +	priv->max_brightness = num_levels - 1;
> +	priv->default_brightness = value;
> +
> +	return 0;
> +}
> +
> +static int led_bl_probe(struct platform_device *pdev)
> +{
> +	struct backlight_properties props;
> +	struct led_bl_data *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->dev = &pdev->dev;
> +	priv->led_cdev = to_led_classdev(pdev->dev.parent);
> +
> +	ret = led_bl_parse_dt(&pdev->dev, priv);
> +	if (ret < 0) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "failed to parse DT data\n");
> +		return ret;
> +	}
> +
> +	priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> +			    GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->enable_gpio)) {
> +		ret = PTR_ERR(priv->enable_gpio);
> +		goto err;
> +	}
> +
> +	priv->power_supply = devm_regulator_get(&pdev->dev, "power");
> +	if (IS_ERR(priv->power_supply)) {
> +		ret = PTR_ERR(priv->power_supply);
> +		goto err;
> +	}
> +
> +	memset(&props, 0, sizeof(struct backlight_properties));
> +	props.type = BACKLIGHT_RAW;
> +	props.max_brightness = priv->max_brightness;
> +	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
> +			&pdev->dev, priv, &led_bl_ops, &props);
> +	if (IS_ERR(priv->bl_dev)) {
> +		dev_err(&pdev->dev, "failed to register backlight\n");
> +		ret = PTR_ERR(priv->bl_dev);
> +		goto err;
> +	}
> +
> +	priv->bl_dev->props.brightness = priv->default_brightness;
> +	backlight_update_status(priv->bl_dev);
> +
> +	return 0;
> +
> +err:
> +
> +	return ret;
> +}
> +
> +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;
> +
> +	backlight_device_unregister(bl);
> +
> +	led_bl_power_off(priv);
> +
> +	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),
> +	},
> +	.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
> 

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

[Jean-Jacques Hiblot]

Hi Daniel,

On 02/07/2019 11:54, Daniel Thompson wrote:
> On Mon, Jul 01, 2019 at 05:14:22PM +0200, Jean-Jacques Hiblot wrote:
>> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>
>> This patch adds a led-backlight driver (led_bl), which is mostly similar to
>> pwm_bl except the driver uses a LED class driver to adjust the brightness
>> in the HW.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---
>>   drivers/video/backlight/Kconfig  |   7 +
>>   drivers/video/backlight/Makefile |   1 +
>>   drivers/video/backlight/led_bl.c | 217 +++++++++++++++++++++++++++++++
>>   3 files changed, 225 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 000000000000..e699924cc2bc
>> --- /dev/null
>> +++ b/drivers/video/backlight/led_bl.c
>> @@ -0,0 +1,217 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2015-2018 Texas Instruments Incorporated -  http://www.ti.com/
>> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> + *
>> + * Based on pwm_bl.c
>> + */
>> +
>> +#include <linux/backlight.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/slab.h>
>> +
>> +struct led_bl_data {
>> +	struct device		*dev;
>> +	struct backlight_device	*bl_dev;
>> +
>> +	unsigned int		*levels;
>> +	bool			enabled;
>> +	struct regulator	*power_supply;
>> +	struct gpio_desc	*enable_gpio;
> For the PWM driver the power_supply and enable_gpio are part of managing
> a dumb LED driver device that is downstream of the PWM.
>
> What is their purpose when we wrap an LED device? Put another why why isn't
> the LED device driver responsible for this?

This question came up when the patch was first proposed in 2015. Here is 
the answer from Tomi at the time. It is still relevant.

"These are for the backlight, not for the LED chip. So "LED" here is a
chip that produces (most likely) a PWM signal, and "backlight" is the
collection of components that use the PWM to produce the backlight
itself, and use the power-supply and gpios."
  

>
>> +
>> +	struct led_classdev *led_cdev;
>> +
>> +	unsigned int max_brightness;
>> +	unsigned int default_brightness;
>> +};
>> +
>> +static void led_bl_set_brightness(struct led_bl_data *priv, int brightness)
>> +{
>> +	int err;
>> +
>> +	if (!priv->enabled) {
>> +		err = regulator_enable(priv->power_supply);
>> +		if (err < 0)
>> +			dev_err(priv->dev, "failed to enable power supply\n");
>> +
>> +		if (priv->enable_gpio)
>> +			gpiod_set_value_cansleep(priv->enable_gpio, 1);
>> +	}
>> +
>> +	led_set_brightness(priv->led_cdev, priv->levels[brightness]);
>> +
>> +	priv->enabled = true;
>> +}
>> +
>> +static void led_bl_power_off(struct led_bl_data *priv)
>> +{
>> +	if (!priv->enabled)
>> +		return;
>> +
>> +	led_set_brightness(priv->led_cdev, LED_OFF);
>> +
>> +	if (priv->enable_gpio)
>> +		gpiod_set_value_cansleep(priv->enable_gpio, 0);
>> +
>> +	regulator_disable(priv->power_supply);
>> +
>> +	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_parse_dt(struct device *dev,
>> +			   struct led_bl_data *priv)
>> +{
>> +	struct device_node *node = dev->of_node;
>> +	int num_levels;
>> +	u32 *levels;
>> +	u32 value;
>> +	int ret;
>> +
>> +	if (!node)
>> +		return -ENODEV;
>> +
>> +	num_levels = of_property_count_u32_elems(node, "brightness-levels");
> Is there any reason that this function cannot use the (more generic)
> device property API throughout this function?

No reason. The code is a bit old, and can do with an update.

Are you thinking of of_property_read_u32_array(), or another function ?

JJ

>
>
>
> Daniel.
>
>
>> +	if (num_levels < 0)
>> +		return num_levels;
>> +
>> +	levels = devm_kzalloc(dev, sizeof(u32) * num_levels, GFP_KERNEL);
>> +	if (!levels)
>> +		return -ENOMEM;
>> +
>> +	ret = of_property_read_u32_array(node, "brightness-levels",
>> +					 levels,
>> +					 num_levels);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = of_property_read_u32(node, "default-brightness-level", &value);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (value >= num_levels) {
>> +		dev_err(dev, "invalid default-brightness-level\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	priv->levels = levels;
>> +	priv->max_brightness = num_levels - 1;
>> +	priv->default_brightness = value;
>> +
>> +	return 0;
>> +}
>> +
>> +static int led_bl_probe(struct platform_device *pdev)
>> +{
>> +	struct backlight_properties props;
>> +	struct led_bl_data *priv;
>> +	int ret;
>> +
>> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	priv->dev = &pdev->dev;
>> +	priv->led_cdev = to_led_classdev(pdev->dev.parent);
>> +
>> +	ret = led_bl_parse_dt(&pdev->dev, priv);
>> +	if (ret < 0) {
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(&pdev->dev, "failed to parse DT data\n");
>> +		return ret;
>> +	}
>> +
>> +	priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
>> +			    GPIOD_OUT_LOW);
>> +	if (IS_ERR(priv->enable_gpio)) {
>> +		ret = PTR_ERR(priv->enable_gpio);
>> +		goto err;
>> +	}
>> +
>> +	priv->power_supply = devm_regulator_get(&pdev->dev, "power");
>> +	if (IS_ERR(priv->power_supply)) {
>> +		ret = PTR_ERR(priv->power_supply);
>> +		goto err;
>> +	}
>> +
>> +	memset(&props, 0, sizeof(struct backlight_properties));
>> +	props.type = BACKLIGHT_RAW;
>> +	props.max_brightness = priv->max_brightness;
>> +	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
>> +			&pdev->dev, priv, &led_bl_ops, &props);
>> +	if (IS_ERR(priv->bl_dev)) {
>> +		dev_err(&pdev->dev, "failed to register backlight\n");
>> +		ret = PTR_ERR(priv->bl_dev);
>> +		goto err;
>> +	}
>> +
>> +	priv->bl_dev->props.brightness = priv->default_brightness;
>> +	backlight_update_status(priv->bl_dev);
>> +
>> +	return 0;
>> +
>> +err:
>> +
>> +	return ret;
>> +}
>> +
>> +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;
>> +
>> +	backlight_device_unregister(bl);
>> +
>> +	led_bl_power_off(priv);
>> +
>> +	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),
>> +	},
>> +	.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
>>

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

[Daniel Thompson]

On Tue, Jul 02, 2019 at 12:59:53PM +0200, Jean-Jacques Hiblot wrote:
> Hi Daniel,
> 
> On 02/07/2019 11:54, Daniel Thompson wrote:
> > On Mon, Jul 01, 2019 at 05:14:22PM +0200, Jean-Jacques Hiblot wrote:
> > > From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > > 
> > > This patch adds a led-backlight driver (led_bl), which is mostly similar to
> > > pwm_bl except the driver uses a LED class driver to adjust the brightness
> > > in the HW.
> > > 
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > > ---
> > >   drivers/video/backlight/Kconfig  |   7 +
> > >   drivers/video/backlight/Makefile |   1 +
> > >   drivers/video/backlight/led_bl.c | 217 +++++++++++++++++++++++++++++++
> > >   3 files changed, 225 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 000000000000..e699924cc2bc
> > > --- /dev/null
> > > +++ b/drivers/video/backlight/led_bl.c
> > > @@ -0,0 +1,217 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2015-2018 Texas Instruments Incorporated -  http://www.ti.com/
> > > + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > > + *
> > > + * Based on pwm_bl.c
> > > + */
> > > +
> > > +#include <linux/backlight.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/leds.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/slab.h>
> > > +
> > > +struct led_bl_data {
> > > +	struct device		*dev;
> > > +	struct backlight_device	*bl_dev;
> > > +
> > > +	unsigned int		*levels;
> > > +	bool			enabled;
> > > +	struct regulator	*power_supply;
> > > +	struct gpio_desc	*enable_gpio;
> > For the PWM driver the power_supply and enable_gpio are part of managing
> > a dumb LED driver device that is downstream of the PWM.
> > 
> > What is their purpose when we wrap an LED device? Put another why why isn't
> > the LED device driver responsible for this?
> 
> This question came up when the patch was first proposed in 2015. Here is the
> answer from Tomi at the time. It is still relevant.
> 
> "These are for the backlight, not for the LED chip. So "LED" here is a
> chip that produces (most likely) a PWM signal, and "backlight" is the
> collection of components that use the PWM to produce the backlight
> itself, and use the power-supply and gpios."

Expanded significantly in the associated thread, right?
https://patchwork.kernel.org/patch/7293991/

Also still relevant is whether the LED device is being correctly
modelled if the act of turning on the LED doesn't, in fact, turn the LED
on. Is it *really* a correct implementation of an LED device that
setting it to LED_FULL using sysfs doesn't cause it to light up?

Actually there is another area where I think an LED backlight should
perhaps be held to a higher standard than a PWM backlight and that is
handling backlights composed of multiple LEDs.

Using the TLC591xx examples from the thread above... these are
multi-channel (8 or 16) LED controllers and I don't think its
speculative to assume that a backlight could constructed using
one of these could require multiple LEDs.


Daniel.


> 
> > 
> > > +
> > > +	struct led_classdev *led_cdev;
> > > +
> > > +	unsigned int max_brightness;
> > > +	unsigned int default_brightness;
> > > +};
> > > +
> > > +static void led_bl_set_brightness(struct led_bl_data *priv, int brightness)
> > > +{
> > > +	int err;
> > > +
> > > +	if (!priv->enabled) {
> > > +		err = regulator_enable(priv->power_supply);
> > > +		if (err < 0)
> > > +			dev_err(priv->dev, "failed to enable power supply\n");
> > > +
> > > +		if (priv->enable_gpio)
> > > +			gpiod_set_value_cansleep(priv->enable_gpio, 1);
> > > +	}
> > > +
> > > +	led_set_brightness(priv->led_cdev, priv->levels[brightness]);
> > > +
> > > +	priv->enabled = true;
> > > +}
> > > +
> > > +static void led_bl_power_off(struct led_bl_data *priv)
> > > +{
> > > +	if (!priv->enabled)
> > > +		return;
> > > +
> > > +	led_set_brightness(priv->led_cdev, LED_OFF);
> > > +
> > > +	if (priv->enable_gpio)
> > > +		gpiod_set_value_cansleep(priv->enable_gpio, 0);
> > > +
> > > +	regulator_disable(priv->power_supply);
> > > +
> > > +	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_parse_dt(struct device *dev,
> > > +			   struct led_bl_data *priv)
> > > +{
> > > +	struct device_node *node = dev->of_node;
> > > +	int num_levels;
> > > +	u32 *levels;
> > > +	u32 value;
> > > +	int ret;
> > > +
> > > +	if (!node)
> > > +		return -ENODEV;
> > > +
> > > +	num_levels = of_property_count_u32_elems(node, "brightness-levels");
> > Is there any reason that this function cannot use the (more generic)
> > device property API throughout this function?
> 
> No reason. The code is a bit old, and can do with an update.
> 
> Are you thinking of of_property_read_u32_array(), or another function ?
> 
> JJ
> 
> > 
> > 
> > 
> > Daniel.
> > 
> > 
> > > +	if (num_levels < 0)
> > > +		return num_levels;
> > > +
> > > +	levels = devm_kzalloc(dev, sizeof(u32) * num_levels, GFP_KERNEL);
> > > +	if (!levels)
> > > +		return -ENOMEM;
> > > +
> > > +	ret = of_property_read_u32_array(node, "brightness-levels",
> > > +					 levels,
> > > +					 num_levels);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = of_property_read_u32(node, "default-brightness-level", &value);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	if (value >= num_levels) {
> > > +		dev_err(dev, "invalid default-brightness-level\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	priv->levels = levels;
> > > +	priv->max_brightness = num_levels - 1;
> > > +	priv->default_brightness = value;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int led_bl_probe(struct platform_device *pdev)
> > > +{
> > > +	struct backlight_properties props;
> > > +	struct led_bl_data *priv;
> > > +	int ret;
> > > +
> > > +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > > +	if (!priv)
> > > +		return -ENOMEM;
> > > +
> > > +	platform_set_drvdata(pdev, priv);
> > > +
> > > +	priv->dev = &pdev->dev;
> > > +	priv->led_cdev = to_led_classdev(pdev->dev.parent);
> > > +
> > > +	ret = led_bl_parse_dt(&pdev->dev, priv);
> > > +	if (ret < 0) {
> > > +		if (ret != -EPROBE_DEFER)
> > > +			dev_err(&pdev->dev, "failed to parse DT data\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> > > +			    GPIOD_OUT_LOW);
> > > +	if (IS_ERR(priv->enable_gpio)) {
> > > +		ret = PTR_ERR(priv->enable_gpio);
> > > +		goto err;
> > > +	}
> > > +
> > > +	priv->power_supply = devm_regulator_get(&pdev->dev, "power");
> > > +	if (IS_ERR(priv->power_supply)) {
> > > +		ret = PTR_ERR(priv->power_supply);
> > > +		goto err;
> > > +	}
> > > +
> > > +	memset(&props, 0, sizeof(struct backlight_properties));
> > > +	props.type = BACKLIGHT_RAW;
> > > +	props.max_brightness = priv->max_brightness;
> > > +	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
> > > +			&pdev->dev, priv, &led_bl_ops, &props);
> > > +	if (IS_ERR(priv->bl_dev)) {
> > > +		dev_err(&pdev->dev, "failed to register backlight\n");
> > > +		ret = PTR_ERR(priv->bl_dev);
> > > +		goto err;
> > > +	}
> > > +
> > > +	priv->bl_dev->props.brightness = priv->default_brightness;
> > > +	backlight_update_status(priv->bl_dev);
> > > +
> > > +	return 0;
> > > +
> > > +err:
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +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;
> > > +
> > > +	backlight_device_unregister(bl);
> > > +
> > > +	led_bl_power_off(priv);
> > > +
> > > +	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),
> > > +	},
> > > +	.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

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

[Jean-Jacques Hiblot]

Daniel,

On 02/07/2019 15:04, Daniel Thompson wrote:
> On Tue, Jul 02, 2019 at 12:59:53PM +0200, Jean-Jacques Hiblot wrote:
>> Hi Daniel,
>>
>> On 02/07/2019 11:54, Daniel Thompson wrote:
>>> On Mon, Jul 01, 2019 at 05:14:22PM +0200, Jean-Jacques Hiblot wrote:
>>>> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>>
>>>> This patch adds a led-backlight driver (led_bl), which is mostly similar to
>>>> pwm_bl except the driver uses a LED class driver to adjust the brightness
>>>> in the HW.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>>>> ---
>>>>    drivers/video/backlight/Kconfig  |   7 +
>>>>    drivers/video/backlight/Makefile |   1 +
>>>>    drivers/video/backlight/led_bl.c | 217 +++++++++++++++++++++++++++++++
>>>>    3 files changed, 225 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 000000000000..e699924cc2bc
>>>> --- /dev/null
>>>> +++ b/drivers/video/backlight/led_bl.c
>>>> @@ -0,0 +1,217 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (C) 2015-2018 Texas Instruments Incorporated -  http://www.ti.com/
>>>> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>> + *
>>>> + * Based on pwm_bl.c
>>>> + */
>>>> +
>>>> +#include <linux/backlight.h>
>>>> +#include <linux/gpio/consumer.h>
>>>> +#include <linux/leds.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +struct led_bl_data {
>>>> +	struct device		*dev;
>>>> +	struct backlight_device	*bl_dev;
>>>> +
>>>> +	unsigned int		*levels;
>>>> +	bool			enabled;
>>>> +	struct regulator	*power_supply;
>>>> +	struct gpio_desc	*enable_gpio;
>>> For the PWM driver the power_supply and enable_gpio are part of managing
>>> a dumb LED driver device that is downstream of the PWM.
>>>
>>> What is their purpose when we wrap an LED device? Put another why why isn't
>>> the LED device driver responsible for this?
>> This question came up when the patch was first proposed in 2015. Here is the
>> answer from Tomi at the time. It is still relevant.
>>
>> "These are for the backlight, not for the LED chip. So "LED" here is a
>> chip that produces (most likely) a PWM signal, and "backlight" is the
>> collection of components that use the PWM to produce the backlight
>> itself, and use the power-supply and gpios."
> Expanded significantly in the associated thread, right?
> https://patchwork.kernel.org/patch/7293991/
>
> Also still relevant is whether the LED device is being correctly
> modelled if the act of turning on the LED doesn't, in fact, turn the LED
> on. Is it *really* a correct implementation of an LED device that
> setting it to LED_FULL using sysfs doesn't cause it to light up?

What I understood from the discussion between Rob and Tomi is that the 
child-node of the LED controller should be considered a backlight 
device, not a simple LED. I'm not sure if the sysfs interface is still 
relevant in that case. Maybe it should just be disabled by the backlight 
driver (possible with led_sysfs_disable())

>
> Actually there is another area where I think an LED backlight should
> perhaps be held to a higher standard than a PWM backlight and that is
> handling backlights composed of multiple LEDs.
>
> Using the TLC591xx examples from the thread above... these are
> multi-channel (8 or 16) LED controllers and I don't think its
> speculative to assume that a backlight could constructed using
> one of these could require multiple LEDs.

In that case, the device-tree model must be quite different.

Actually the best way to do that is to use the model from Tomi 
https://patchwork.kernel.org/patch/7293991/ and modify it to handle more 
than one LED
<https://patchwork.kernel.org/patch/7293991/>

I'm not completely sure that people would start making real backlight 
this way though. It is much more probable that the ouput of the led ctrl 
is connected to a single control input of a real backlight than to 
actual LEDs.


JJ

>
>
> Daniel.
>
>
>>>> +
>>>> +	struct led_classdev *led_cdev;
>>>> +
>>>> +	unsigned int max_brightness;
>>>> +	unsigned int default_brightness;
>>>> +};
>>>> +
>>>> +static void led_bl_set_brightness(struct led_bl_data *priv, int brightness)
>>>> +{
>>>> +	int err;
>>>> +
>>>> +	if (!priv->enabled) {
>>>> +		err = regulator_enable(priv->power_supply);
>>>> +		if (err < 0)
>>>> +			dev_err(priv->dev, "failed to enable power supply\n");
>>>> +
>>>> +		if (priv->enable_gpio)
>>>> +			gpiod_set_value_cansleep(priv->enable_gpio, 1);
>>>> +	}
>>>> +
>>>> +	led_set_brightness(priv->led_cdev, priv->levels[brightness]);
>>>> +
>>>> +	priv->enabled = true;
>>>> +}
>>>> +
>>>> +static void led_bl_power_off(struct led_bl_data *priv)
>>>> +{
>>>> +	if (!priv->enabled)
>>>> +		return;
>>>> +
>>>> +	led_set_brightness(priv->led_cdev, LED_OFF);
>>>> +
>>>> +	if (priv->enable_gpio)
>>>> +		gpiod_set_value_cansleep(priv->enable_gpio, 0);
>>>> +
>>>> +	regulator_disable(priv->power_supply);
>>>> +
>>>> +	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_parse_dt(struct device *dev,
>>>> +			   struct led_bl_data *priv)
>>>> +{
>>>> +	struct device_node *node = dev->of_node;
>>>> +	int num_levels;
>>>> +	u32 *levels;
>>>> +	u32 value;
>>>> +	int ret;
>>>> +
>>>> +	if (!node)
>>>> +		return -ENODEV;
>>>> +
>>>> +	num_levels = of_property_count_u32_elems(node, "brightness-levels");
>>> Is there any reason that this function cannot use the (more generic)
>>> device property API throughout this function?
>> No reason. The code is a bit old, and can do with an update.
>>
>> Are you thinking of of_property_read_u32_array(), or another function ?
>>
>> JJ
>>
>>>
>>>
>>> Daniel.
>>>
>>>
>>>> +	if (num_levels < 0)
>>>> +		return num_levels;
>>>> +
>>>> +	levels = devm_kzalloc(dev, sizeof(u32) * num_levels, GFP_KERNEL);
>>>> +	if (!levels)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	ret = of_property_read_u32_array(node, "brightness-levels",
>>>> +					 levels,
>>>> +					 num_levels);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	ret = of_property_read_u32(node, "default-brightness-level", &value);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	if (value >= num_levels) {
>>>> +		dev_err(dev, "invalid default-brightness-level\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	priv->levels = levels;
>>>> +	priv->max_brightness = num_levels - 1;
>>>> +	priv->default_brightness = value;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int led_bl_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct backlight_properties props;
>>>> +	struct led_bl_data *priv;
>>>> +	int ret;
>>>> +
>>>> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>>> +	if (!priv)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	platform_set_drvdata(pdev, priv);
>>>> +
>>>> +	priv->dev = &pdev->dev;
>>>> +	priv->led_cdev = to_led_classdev(pdev->dev.parent);
>>>> +
>>>> +	ret = led_bl_parse_dt(&pdev->dev, priv);
>>>> +	if (ret < 0) {
>>>> +		if (ret != -EPROBE_DEFER)
>>>> +			dev_err(&pdev->dev, "failed to parse DT data\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
>>>> +			    GPIOD_OUT_LOW);
>>>> +	if (IS_ERR(priv->enable_gpio)) {
>>>> +		ret = PTR_ERR(priv->enable_gpio);
>>>> +		goto err;
>>>> +	}
>>>> +
>>>> +	priv->power_supply = devm_regulator_get(&pdev->dev, "power");
>>>> +	if (IS_ERR(priv->power_supply)) {
>>>> +		ret = PTR_ERR(priv->power_supply);
>>>> +		goto err;
>>>> +	}
>>>> +
>>>> +	memset(&props, 0, sizeof(struct backlight_properties));
>>>> +	props.type = BACKLIGHT_RAW;
>>>> +	props.max_brightness = priv->max_brightness;
>>>> +	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
>>>> +			&pdev->dev, priv, &led_bl_ops, &props);
>>>> +	if (IS_ERR(priv->bl_dev)) {
>>>> +		dev_err(&pdev->dev, "failed to register backlight\n");
>>>> +		ret = PTR_ERR(priv->bl_dev);
>>>> +		goto err;
>>>> +	}
>>>> +
>>>> +	priv->bl_dev->props.brightness = priv->default_brightness;
>>>> +	backlight_update_status(priv->bl_dev);
>>>> +
>>>> +	return 0;
>>>> +
>>>> +err:
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +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;
>>>> +
>>>> +	backlight_device_unregister(bl);
>>>> +
>>>> +	led_bl_power_off(priv);
>>>> +
>>>> +	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),
>>>> +	},
>>>> +	.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
>>>>

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

[Daniel Thompson]

On Tue, Jul 02, 2019 at 05:17:21PM +0200, Jean-Jacques Hiblot wrote:
> Daniel,
> 
> On 02/07/2019 15:04, Daniel Thompson wrote:
> > On Tue, Jul 02, 2019 at 12:59:53PM +0200, Jean-Jacques Hiblot wrote:
> > > Hi Daniel,
> > > 
> > > On 02/07/2019 11:54, Daniel Thompson wrote:
> > > > On Mon, Jul 01, 2019 at 05:14:22PM +0200, Jean-Jacques Hiblot wrote:
> > > > > From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > > > > 
> > > > > This patch adds a led-backlight driver (led_bl), which is mostly similar to
> > > > > pwm_bl except the driver uses a LED class driver to adjust the brightness
> > > > > in the HW.
> > > > > 
> > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > > > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > > > > ---
> > > > >    drivers/video/backlight/Kconfig  |   7 +
> > > > >    drivers/video/backlight/Makefile |   1 +
> > > > >    drivers/video/backlight/led_bl.c | 217 +++++++++++++++++++++++++++++++
> > > > >    3 files changed, 225 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 000000000000..e699924cc2bc
> > > > > --- /dev/null
> > > > > +++ b/drivers/video/backlight/led_bl.c
> > > > > @@ -0,0 +1,217 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (C) 2015-2018 Texas Instruments Incorporated -  http://www.ti.com/
> > > > > + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > > > > + *
> > > > > + * Based on pwm_bl.c
> > > > > + */
> > > > > +
> > > > > +#include <linux/backlight.h>
> > > > > +#include <linux/gpio/consumer.h>
> > > > > +#include <linux/leds.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +#include <linux/regulator/consumer.h>
> > > > > +#include <linux/slab.h>
> > > > > +
> > > > > +struct led_bl_data {
> > > > > +	struct device		*dev;
> > > > > +	struct backlight_device	*bl_dev;
> > > > > +
> > > > > +	unsigned int		*levels;
> > > > > +	bool			enabled;
> > > > > +	struct regulator	*power_supply;
> > > > > +	struct gpio_desc	*enable_gpio;
> > > > For the PWM driver the power_supply and enable_gpio are part of managing
> > > > a dumb LED driver device that is downstream of the PWM.
> > > > 
> > > > What is their purpose when we wrap an LED device? Put another why why isn't
> > > > the LED device driver responsible for this?
> > > This question came up when the patch was first proposed in 2015. Here is the
> > > answer from Tomi at the time. It is still relevant.
> > > 
> > > "These are for the backlight, not for the LED chip. So "LED" here is a
> > > chip that produces (most likely) a PWM signal, and "backlight" is the
> > > collection of components that use the PWM to produce the backlight
> > > itself, and use the power-supply and gpios."
> > Expanded significantly in the associated thread, right?
> > https://patchwork.kernel.org/patch/7293991/
> > 
> > Also still relevant is whether the LED device is being correctly
> > modelled if the act of turning on the LED doesn't, in fact, turn the LED
> > on. Is it *really* a correct implementation of an LED device that
> > setting it to LED_FULL using sysfs doesn't cause it to light up?
> 
> What I understood from the discussion between Rob and Tomi is that the
> child-node of the LED controller should be considered a backlight device,
> not a simple LED. I'm not sure if the sysfs interface is still relevant in
> that case. Maybe it should just be disabled by the backlight driver
> (possible with led_sysfs_disable())

led_sysfs_disable() sounds like a sensible change but that's not quite
what I mean.

It is more a thought experiment to see if the power control *should* be
implemented by the backlight. Consider what happens if we *don't*
enable CONFIG_BACKLIGHT_LED in the kernel: we would still have an LED
device and it would not work correctly.

In other words I naively expect turning on an LED using the LED API 
(any of them, sysfs or kernel) to result in the LED turning on.
Implementing a workaround in the client for what appears to be
something missing in the LED driver strikes me as odd. Why shouldn't
the regulator be managed in the LED driver?


> > Actually there is another area where I think an LED backlight should
> > perhaps be held to a higher standard than a PWM backlight and that is
> > handling backlights composed of multiple LEDs.
> > 
> > Using the TLC591xx examples from the thread above... these are
> > multi-channel (8 or 16) LED controllers and I don't think its
> > speculative to assume that a backlight could constructed using
> > one of these could require multiple LEDs.
> 
> In that case, the device-tree model must be quite different.
> 
> Actually the best way to do that is to use the model from Tomi
> https://patchwork.kernel.org/patch/7293991/ and modify it to handle more
> than one LED
> <https://patchwork.kernel.org/patch/7293991/>
> 
> I'm not completely sure that people would start making real backlight this
> way though. It is much more probable that the ouput of the led ctrl is
> connected to a single control input of a real backlight than to actual LEDs.

I'm afraid I don't follow this. If you have a backlight composed of mutliple
strings why wouldn't each string be attached directly to the output an of LED
driver chip such as the TLC591xx family.


Daniel.


> 
> 
> JJ
> 
> > 
> > 
> > Daniel.
> > 
> > 
> > > > > +
> > > > > +	struct led_classdev *led_cdev;
> > > > > +
> > > > > +	unsigned int max_brightness;
> > > > > +	unsigned int default_brightness;
> > > > > +};
> > > > > +
> > > > > +static void led_bl_set_brightness(struct led_bl_data *priv, int brightness)
> > > > > +{
> > > > > +	int err;
> > > > > +
> > > > > +	if (!priv->enabled) {
> > > > > +		err = regulator_enable(priv->power_supply);
> > > > > +		if (err < 0)
> > > > > +			dev_err(priv->dev, "failed to enable power supply\n");
> > > > > +
> > > > > +		if (priv->enable_gpio)
> > > > > +			gpiod_set_value_cansleep(priv->enable_gpio, 1);
> > > > > +	}
> > > > > +
> > > > > +	led_set_brightness(priv->led_cdev, priv->levels[brightness]);
> > > > > +
> > > > > +	priv->enabled = true;
> > > > > +}
> > > > > +
> > > > > +static void led_bl_power_off(struct led_bl_data *priv)
> > > > > +{
> > > > > +	if (!priv->enabled)
> > > > > +		return;
> > > > > +
> > > > > +	led_set_brightness(priv->led_cdev, LED_OFF);
> > > > > +
> > > > > +	if (priv->enable_gpio)
> > > > > +		gpiod_set_value_cansleep(priv->enable_gpio, 0);
> > > > > +
> > > > > +	regulator_disable(priv->power_supply);
> > > > > +
> > > > > +	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_parse_dt(struct device *dev,
> > > > > +			   struct led_bl_data *priv)
> > > > > +{
> > > > > +	struct device_node *node = dev->of_node;
> > > > > +	int num_levels;
> > > > > +	u32 *levels;
> > > > > +	u32 value;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (!node)
> > > > > +		return -ENODEV;
> > > > > +
> > > > > +	num_levels = of_property_count_u32_elems(node, "brightness-levels");
> > > > Is there any reason that this function cannot use the (more generic)
> > > > device property API throughout this function?
> > > No reason. The code is a bit old, and can do with an update.
> > > 
> > > Are you thinking of of_property_read_u32_array(), or another function ?
> > > 
> > > JJ
> > > 
> > > > 
> > > > 
> > > > Daniel.
> > > > 
> > > > 
> > > > > +	if (num_levels < 0)
> > > > > +		return num_levels;
> > > > > +
> > > > > +	levels = devm_kzalloc(dev, sizeof(u32) * num_levels, GFP_KERNEL);
> > > > > +	if (!levels)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	ret = of_property_read_u32_array(node, "brightness-levels",
> > > > > +					 levels,
> > > > > +					 num_levels);
> > > > > +	if (ret < 0)
> > > > > +		return ret;
> > > > > +
> > > > > +	ret = of_property_read_u32(node, "default-brightness-level", &value);
> > > > > +	if (ret < 0)
> > > > > +		return ret;
> > > > > +
> > > > > +	if (value >= num_levels) {
> > > > > +		dev_err(dev, "invalid default-brightness-level\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	priv->levels = levels;
> > > > > +	priv->max_brightness = num_levels - 1;
> > > > > +	priv->default_brightness = value;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int led_bl_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +	struct backlight_properties props;
> > > > > +	struct led_bl_data *priv;
> > > > > +	int ret;
> > > > > +
> > > > > +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > > > > +	if (!priv)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	platform_set_drvdata(pdev, priv);
> > > > > +
> > > > > +	priv->dev = &pdev->dev;
> > > > > +	priv->led_cdev = to_led_classdev(pdev->dev.parent);
> > > > > +
> > > > > +	ret = led_bl_parse_dt(&pdev->dev, priv);
> > > > > +	if (ret < 0) {
> > > > > +		if (ret != -EPROBE_DEFER)
> > > > > +			dev_err(&pdev->dev, "failed to parse DT data\n");
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> > > > > +			    GPIOD_OUT_LOW);
> > > > > +	if (IS_ERR(priv->enable_gpio)) {
> > > > > +		ret = PTR_ERR(priv->enable_gpio);
> > > > > +		goto err;
> > > > > +	}
> > > > > +
> > > > > +	priv->power_supply = devm_regulator_get(&pdev->dev, "power");
> > > > > +	if (IS_ERR(priv->power_supply)) {
> > > > > +		ret = PTR_ERR(priv->power_supply);
> > > > > +		goto err;
> > > > > +	}
> > > > > +
> > > > > +	memset(&props, 0, sizeof(struct backlight_properties));
> > > > > +	props.type = BACKLIGHT_RAW;
> > > > > +	props.max_brightness = priv->max_brightness;
> > > > > +	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
> > > > > +			&pdev->dev, priv, &led_bl_ops, &props);
> > > > > +	if (IS_ERR(priv->bl_dev)) {
> > > > > +		dev_err(&pdev->dev, "failed to register backlight\n");
> > > > > +		ret = PTR_ERR(priv->bl_dev);
> > > > > +		goto err;
> > > > > +	}
> > > > > +
> > > > > +	priv->bl_dev->props.brightness = priv->default_brightness;
> > > > > +	backlight_update_status(priv->bl_dev);
> > > > > +
> > > > > +	return 0;
> > > > > +
> > > > > +err:
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +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;
> > > > > +
> > > > > +	backlight_device_unregister(bl);
> > > > > +
> > > > > +	led_bl_power_off(priv);
> > > > > +
> > > > > +	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),
> > > > > +	},
> > > > > +	.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

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

[Jean-Jacques Hiblot]

Daniel,

On 03/07/2019 11:44, Daniel Thompson wrote:
> On Tue, Jul 02, 2019 at 05:17:21PM +0200, Jean-Jacques Hiblot wrote:
>> Daniel,
>>
>> On 02/07/2019 15:04, Daniel Thompson wrote:
>>> On Tue, Jul 02, 2019 at 12:59:53PM +0200, Jean-Jacques Hiblot wrote:
>>>> Hi Daniel,
>>>>
>>>> On 02/07/2019 11:54, Daniel Thompson wrote:
>>>>> On Mon, Jul 01, 2019 at 05:14:22PM +0200, Jean-Jacques Hiblot wrote:
>>>>>> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>>>>
>>>>>> This patch adds a led-backlight driver (led_bl), which is mostly similar to
>>>>>> pwm_bl except the driver uses a LED class driver to adjust the brightness
>>>>>> in the HW.
>>>>>>
>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>>>>>> ---
>>>>>>     drivers/video/backlight/Kconfig  |   7 +
>>>>>>     drivers/video/backlight/Makefile |   1 +
>>>>>>     drivers/video/backlight/led_bl.c | 217 +++++++++++++++++++++++++++++++
>>>>>>     3 files changed, 225 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 000000000000..e699924cc2bc
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/video/backlight/led_bl.c
>>>>>> @@ -0,0 +1,217 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +/*
>>>>>> + * Copyright (C) 2015-2018 Texas Instruments Incorporated -  http://www.ti.com/
>>>>>> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>>>> + *
>>>>>> + * Based on pwm_bl.c
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/backlight.h>
>>>>>> +#include <linux/gpio/consumer.h>
>>>>>> +#include <linux/leds.h>
>>>>>> +#include <linux/module.h>
>>>>>> +#include <linux/platform_device.h>
>>>>>> +#include <linux/regulator/consumer.h>
>>>>>> +#include <linux/slab.h>
>>>>>> +
>>>>>> +struct led_bl_data {
>>>>>> +	struct device		*dev;
>>>>>> +	struct backlight_device	*bl_dev;
>>>>>> +
>>>>>> +	unsigned int		*levels;
>>>>>> +	bool			enabled;
>>>>>> +	struct regulator	*power_supply;
>>>>>> +	struct gpio_desc	*enable_gpio;
>>>>> For the PWM driver the power_supply and enable_gpio are part of managing
>>>>> a dumb LED driver device that is downstream of the PWM.
>>>>>
>>>>> What is their purpose when we wrap an LED device? Put another why why isn't
>>>>> the LED device driver responsible for this?
>>>> This question came up when the patch was first proposed in 2015. Here is the
>>>> answer from Tomi at the time. It is still relevant.
>>>>
>>>> "These are for the backlight, not for the LED chip. So "LED" here is a
>>>> chip that produces (most likely) a PWM signal, and "backlight" is the
>>>> collection of components that use the PWM to produce the backlight
>>>> itself, and use the power-supply and gpios."
>>> Expanded significantly in the associated thread, right?
>>> https://patchwork.kernel.org/patch/7293991/
>>>
>>> Also still relevant is whether the LED device is being correctly
>>> modelled if the act of turning on the LED doesn't, in fact, turn the LED
>>> on. Is it *really* a correct implementation of an LED device that
>>> setting it to LED_FULL using sysfs doesn't cause it to light up?
>> What I understood from the discussion between Rob and Tomi is that the
>> child-node of the LED controller should be considered a backlight device,
>> not a simple LED. I'm not sure if the sysfs interface is still relevant in
>> that case. Maybe it should just be disabled by the backlight driver
>> (possible with led_sysfs_disable())
> led_sysfs_disable() sounds like a sensible change but that's not quite
> what I mean.
>
> It is more a thought experiment to see if the power control *should* be
> implemented by the backlight. Consider what happens if we *don't*
> enable CONFIG_BACKLIGHT_LED in the kernel: we would still have an LED
> device and it would not work correctly.
>
> In other words I naively expect turning on an LED using the LED API
> (any of them, sysfs or kernel) to result in the LED turning on.
> Implementing a workaround in the client for what appears to be
> something missing in the LED driver strikes me as odd. Why shouldn't
> the regulator be managed in the LED driver?

I see your point. Indeed having the regulator handled in the LED-core 
makes sense in a lot of situations

I'll think about it.

>
>
>>> Actually there is another area where I think an LED backlight should
>>> perhaps be held to a higher standard than a PWM backlight and that is
>>> handling backlights composed of multiple LEDs.
>>>
>>> Using the TLC591xx examples from the thread above... these are
>>> multi-channel (8 or 16) LED controllers and I don't think its
>>> speculative to assume that a backlight could constructed using
>>> one of these could require multiple LEDs.
>> In that case, the device-tree model must be quite different.
>>
>> Actually the best way to do that is to use the model from Tomi
>> https://patchwork.kernel.org/patch/7293991/ and modify it to handle more
>> than one LED
>> <https://patchwork.kernel.org/patch/7293991/>
>>
>> I'm not completely sure that people would start making real backlight this
>> way though. It is much more probable that the ouput of the led ctrl is
>> connected to a single control input of a real backlight than to actual LEDs.
> I'm afraid I don't follow this. If you have a backlight composed of mutliple
> strings why wouldn't each string be attached directly to the output an of LED
> driver chip such as the TLC591xx family.

OK. It makes sense.

I'll rework the series in this direction: multiple LED devices handled 
by one backlight device

Thanks,

JJ

>
>
> Daniel.
>
>
>>
>> JJ
>>
>>>
>>> Daniel.
>>>
>>>
>>>>>> +
>>>>>> +	struct led_classdev *led_cdev;
>>>>>> +
>>>>>> +	unsigned int max_brightness;
>>>>>> +	unsigned int default_brightness;
>>>>>> +};
>>>>>> +
>>>>>> +static void led_bl_set_brightness(struct led_bl_data *priv, int brightness)
>>>>>> +{
>>>>>> +	int err;
>>>>>> +
>>>>>> +	if (!priv->enabled) {
>>>>>> +		err = regulator_enable(priv->power_supply);
>>>>>> +		if (err < 0)
>>>>>> +			dev_err(priv->dev, "failed to enable power supply\n");
>>>>>> +
>>>>>> +		if (priv->enable_gpio)
>>>>>> +			gpiod_set_value_cansleep(priv->enable_gpio, 1);
>>>>>> +	}
>>>>>> +
>>>>>> +	led_set_brightness(priv->led_cdev, priv->levels[brightness]);
>>>>>> +
>>>>>> +	priv->enabled = true;
>>>>>> +}
>>>>>> +
>>>>>> +static void led_bl_power_off(struct led_bl_data *priv)
>>>>>> +{
>>>>>> +	if (!priv->enabled)
>>>>>> +		return;
>>>>>> +
>>>>>> +	led_set_brightness(priv->led_cdev, LED_OFF);
>>>>>> +
>>>>>> +	if (priv->enable_gpio)
>>>>>> +		gpiod_set_value_cansleep(priv->enable_gpio, 0);
>>>>>> +
>>>>>> +	regulator_disable(priv->power_supply);
>>>>>> +
>>>>>> +	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_parse_dt(struct device *dev,
>>>>>> +			   struct led_bl_data *priv)
>>>>>> +{
>>>>>> +	struct device_node *node = dev->of_node;
>>>>>> +	int num_levels;
>>>>>> +	u32 *levels;
>>>>>> +	u32 value;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	if (!node)
>>>>>> +		return -ENODEV;
>>>>>> +
>>>>>> +	num_levels = of_property_count_u32_elems(node, "brightness-levels");
>>>>> Is there any reason that this function cannot use the (more generic)
>>>>> device property API throughout this function?
>>>> No reason. The code is a bit old, and can do with an update.
>>>>
>>>> Are you thinking of of_property_read_u32_array(), or another function ?
>>>>
>>>> JJ
>>>>
>>>>>
>>>>> Daniel.
>>>>>
>>>>>
>>>>>> +	if (num_levels < 0)
>>>>>> +		return num_levels;
>>>>>> +
>>>>>> +	levels = devm_kzalloc(dev, sizeof(u32) * num_levels, GFP_KERNEL);
>>>>>> +	if (!levels)
>>>>>> +		return -ENOMEM;
>>>>>> +
>>>>>> +	ret = of_property_read_u32_array(node, "brightness-levels",
>>>>>> +					 levels,
>>>>>> +					 num_levels);
>>>>>> +	if (ret < 0)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	ret = of_property_read_u32(node, "default-brightness-level", &value);
>>>>>> +	if (ret < 0)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	if (value >= num_levels) {
>>>>>> +		dev_err(dev, "invalid default-brightness-level\n");
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>>> +
>>>>>> +	priv->levels = levels;
>>>>>> +	priv->max_brightness = num_levels - 1;
>>>>>> +	priv->default_brightness = value;
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int led_bl_probe(struct platform_device *pdev)
>>>>>> +{
>>>>>> +	struct backlight_properties props;
>>>>>> +	struct led_bl_data *priv;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>>>>> +	if (!priv)
>>>>>> +		return -ENOMEM;
>>>>>> +
>>>>>> +	platform_set_drvdata(pdev, priv);
>>>>>> +
>>>>>> +	priv->dev = &pdev->dev;
>>>>>> +	priv->led_cdev = to_led_classdev(pdev->dev.parent);
>>>>>> +
>>>>>> +	ret = led_bl_parse_dt(&pdev->dev, priv);
>>>>>> +	if (ret < 0) {
>>>>>> +		if (ret != -EPROBE_DEFER)
>>>>>> +			dev_err(&pdev->dev, "failed to parse DT data\n");
>>>>>> +		return ret;
>>>>>> +	}
>>>>>> +
>>>>>> +	priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
>>>>>> +			    GPIOD_OUT_LOW);
>>>>>> +	if (IS_ERR(priv->enable_gpio)) {
>>>>>> +		ret = PTR_ERR(priv->enable_gpio);
>>>>>> +		goto err;
>>>>>> +	}
>>>>>> +
>>>>>> +	priv->power_supply = devm_regulator_get(&pdev->dev, "power");
>>>>>> +	if (IS_ERR(priv->power_supply)) {
>>>>>> +		ret = PTR_ERR(priv->power_supply);
>>>>>> +		goto err;
>>>>>> +	}
>>>>>> +
>>>>>> +	memset(&props, 0, sizeof(struct backlight_properties));
>>>>>> +	props.type = BACKLIGHT_RAW;
>>>>>> +	props.max_brightness = priv->max_brightness;
>>>>>> +	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
>>>>>> +			&pdev->dev, priv, &led_bl_ops, &props);
>>>>>> +	if (IS_ERR(priv->bl_dev)) {
>>>>>> +		dev_err(&pdev->dev, "failed to register backlight\n");
>>>>>> +		ret = PTR_ERR(priv->bl_dev);
>>>>>> +		goto err;
>>>>>> +	}
>>>>>> +
>>>>>> +	priv->bl_dev->props.brightness = priv->default_brightness;
>>>>>> +	backlight_update_status(priv->bl_dev);
>>>>>> +
>>>>>> +	return 0;
>>>>>> +
>>>>>> +err:
>>>>>> +
>>>>>> +	return ret;
>>>>>> +}
>>>>>> +
>>>>>> +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;
>>>>>> +
>>>>>> +	backlight_device_unregister(bl);
>>>>>> +
>>>>>> +	led_bl_power_off(priv);
>>>>>> +
>>>>>> +	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),
>>>>>> +	},
>>>>>> +	.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
>>>>>>

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

[Pavel Machek]

Hi!

> > > > Also still relevant is whether the LED device is being correctly
> > > > modelled if the act of turning on the LED doesn't, in fact, turn the LED
> > > > on. Is it *really* a correct implementation of an LED device that
> > > > setting it to LED_FULL using sysfs doesn't cause it to light up?
> > > What I understood from the discussion between Rob and Tomi is that the
> > > child-node of the LED controller should be considered a backlight device,
> > > not a simple LED. I'm not sure if the sysfs interface is still relevant in
> > > that case. Maybe it should just be disabled by the backlight driver
> > > (possible with led_sysfs_disable())
> > led_sysfs_disable() sounds like a sensible change but that's not quite
> > what I mean.
> > 
> > It is more a thought experiment to see if the power control *should* be
> > implemented by the backlight. Consider what happens if we *don't*
> > enable CONFIG_BACKLIGHT_LED in the kernel: we would still have an LED
> > device and it would not work correctly.
> > 
> > In other words I naively expect turning on an LED using the LED API
> > (any of them, sysfs or kernel) to result in the LED turning on.
> > Implementing a workaround in the client for what appears to be
> > something missing in the LED driver strikes me as odd. Why shouldn't
> > the regulator be managed in the LED driver?
> 
> I see your point. Indeed having the regulator handled in the LED-core makes
> sense in a lot of situations
> 
> I'll think about it.

For the record, I also believe regulator and enable gpio should be
handled in the core.

									Pavel
PS please trim down the quoted text.									
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pavel Machek]

Hi!

> > > > Also still relevant is whether the LED device is being correctly
> > > > modelled if the act of turning on the LED doesn't, in fact, turn the LED
> > > > on. Is it *really* a correct implementation of an LED device that
> > > > setting it to LED_FULL using sysfs doesn't cause it to light up?
> > > What I understood from the discussion between Rob and Tomi is that the
> > > child-node of the LED controller should be considered a backlight device,
> > > not a simple LED. I'm not sure if the sysfs interface is still relevant in
> > > that case. Maybe it should just be disabled by the backlight driver
> > > (possible with led_sysfs_disable())
> > led_sysfs_disable() sounds like a sensible change but that's not quite
> > what I mean.
> > 
> > It is more a thought experiment to see if the power control *should* be
> > implemented by the backlight. Consider what happens if we *don't*
> > enable CONFIG_BACKLIGHT_LED in the kernel: we would still have an LED
> > device and it would not work correctly.
> > 
> > In other words I naively expect turning on an LED using the LED API
> > (any of them, sysfs or kernel) to result in the LED turning on.
> > Implementing a workaround in the client for what appears to be
> > something missing in the LED driver strikes me as odd. Why shouldn't
> > the regulator be managed in the LED driver?
> 
> I see your point. Indeed having the regulator handled in the LED-core makes
> sense in a lot of situations
> 
> I'll think about it.

For the record, I also believe regulator and enable gpio should be
handled in the core.

									Pavel
PS please trim down the quoted text.									
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Jean-Jacques Hiblot]

Pavel

On 05/07/2019 12:08, Pavel Machek wrote:
> Hi!
>
>>>>> Also still relevant is whether the LED device is being correctly
>>>>> modelled if the act of turning on the LED doesn't, in fact, turn the LED
>>>>> on. Is it *really* a correct implementation of an LED device that
>>>>> setting it to LED_FULL using sysfs doesn't cause it to light up?
>>>> What I understood from the discussion between Rob and Tomi is that the
>>>> child-node of the LED controller should be considered a backlight device,
>>>> not a simple LED. I'm not sure if the sysfs interface is still relevant in
>>>> that case. Maybe it should just be disabled by the backlight driver
>>>> (possible with led_sysfs_disable())
>>> led_sysfs_disable() sounds like a sensible change but that's not quite
>>> what I mean.
>>>
>>> It is more a thought experiment to see if the power control *should* be
>>> implemented by the backlight. Consider what happens if we *don't*
>>> enable CONFIG_BACKLIGHT_LED in the kernel: we would still have an LED
>>> device and it would not work correctly.
>>>
>>> In other words I naively expect turning on an LED using the LED API
>>> (any of them, sysfs or kernel) to result in the LED turning on.
>>> Implementing a workaround in the client for what appears to be
>>> something missing in the LED driver strikes me as odd. Why shouldn't
>>> the regulator be managed in the LED driver?
>> I see your point. Indeed having the regulator handled in the LED-core makes
>> sense in a lot of situations
>>
>> I'll think about it.
> For the record, I also believe regulator and enable gpio should be
> handled in the core.

I am working on adding the regulator to the led core.

I don't really want to add a GPIO enable to the core though. If needed 
it can be described as a GPIO-enabled regulator up(/down)stream the 
regular regulator.

JJ


>
> 									Pavel
> PS please trim down the quoted text.									
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jean-Jacques Hiblot]

Pavel

On 05/07/2019 12:08, Pavel Machek wrote:
> Hi!
>
>>>>> Also still relevant is whether the LED device is being correctly
>>>>> modelled if the act of turning on the LED doesn't, in fact, turn the LED
>>>>> on. Is it *really* a correct implementation of an LED device that
>>>>> setting it to LED_FULL using sysfs doesn't cause it to light up?
>>>> What I understood from the discussion between Rob and Tomi is that the
>>>> child-node of the LED controller should be considered a backlight device,
>>>> not a simple LED. I'm not sure if the sysfs interface is still relevant in
>>>> that case. Maybe it should just be disabled by the backlight driver
>>>> (possible with led_sysfs_disable())
>>> led_sysfs_disable() sounds like a sensible change but that's not quite
>>> what I mean.
>>>
>>> It is more a thought experiment to see if the power control *should* be
>>> implemented by the backlight. Consider what happens if we *don't*
>>> enable CONFIG_BACKLIGHT_LED in the kernel: we would still have an LED
>>> device and it would not work correctly.
>>>
>>> In other words I naively expect turning on an LED using the LED API
>>> (any of them, sysfs or kernel) to result in the LED turning on.
>>> Implementing a workaround in the client for what appears to be
>>> something missing in the LED driver strikes me as odd. Why shouldn't
>>> the regulator be managed in the LED driver?
>> I see your point. Indeed having the regulator handled in the LED-core makes
>> sense in a lot of situations
>>
>> I'll think about it.
> For the record, I also believe regulator and enable gpio should be
> handled in the core.

I am working on adding the regulator to the led core.

I don't really want to add a GPIO enable to the core though. If needed 
it can be described as a GPIO-enabled regulator up(/down)stream the 
regular regulator.

JJ


>
> 									Pavel
> PS please trim down the quoted text.									

[PATCH 4/4] devicetree: Add led-backlight binding

[Jean-Jacques Hiblot]

From: Tomi Valkeinen <tomi.valkeinen@ti.com>

Add DT binding for led-backlight.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Cc: devicetree@vger.kernel.org
---
 .../video/backlight/led-backlight.txt         | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt

diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
new file mode 100644
index 000000000000..216cd52d624a
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
@@ -0,0 +1,39 @@
+led-backlight bindings
+
+The node of the backlight driver IS the node of the LED.
+
+Required properties:
+  - compatible: "led-backlight"
+  - brightness-levels: Array of distinct LED brightness levels. These
+      are in the range from 0 to 255, passed to the LED class driver.
+  - default-brightness-level: the default brightness level (index into the
+      array defined by the "brightness-levels" property)
+
+Optional properties:
+  - power-supply: regulator for supply voltage
+  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
+                  and disables the backlight (see GPIO binding[0])
+
+[0]: Documentation/devicetree/bindings/gpio/gpio.txt
+
+Example:
+
+led_ctrl {
+	red_led@1 {
+	        label = "red";
+		reg = <1>;
+	}
+
+	backlight_led@2 {
+		function = LED_FUNCTION_BACKLIGHT;
+		reg = <2>;
+
+		compatible = "led-backlight";
+
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <6>;
+
+		power-supply = <&vdd_bl_reg>;
+		enable-gpios = <&gpio 58 0>;
+	};
+};
-- 
2.17.1

Re: [PATCH 4/4] devicetree: Add led-backlight binding

[Daniel Thompson]

On Mon, Jul 01, 2019 at 05:14:23PM +0200, Jean-Jacques Hiblot wrote:
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Add DT binding for led-backlight.

I think the patchset is in the wrong order; the DT bindings
documentation should appear *before* the binding is
implemented (amoung other things this prevent transient checkpatch
warnings as the patchset is applied).


> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Cc: devicetree@vger.kernel.org
> ---
>  .../video/backlight/led-backlight.txt         | 39 +++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> 
> diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> new file mode 100644
> index 000000000000..216cd52d624a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> @@ -0,0 +1,39 @@
> +led-backlight bindings
> +
> +The node of the backlight driver IS the node of the LED.
> +
> +Required properties:
> +  - compatible: "led-backlight"
> +  - brightness-levels: Array of distinct LED brightness levels. These
> +      are in the range from 0 to 255, passed to the LED class driver.
> +  - default-brightness-level: the default brightness level (index into the
> +      array defined by the "brightness-levels" property)

I think brightness-levels and default-brightness-level could be
optional properties since a default 1:1 mapping seems reasonable given
how constrained the LED brightness values are.


Daniel.


> +
> +Optional properties:
> +  - power-supply: regulator for supply voltage
> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
> +                  and disables the backlight (see GPIO binding[0])
> +
> +[0]: Documentation/devicetree/bindings/gpio/gpio.txt
> +
> +Example:
> +
> +led_ctrl {
> +	red_led@1 {
> +	        label = "red";
> +		reg = <1>;
> +	}
> +
> +	backlight_led@2 {
> +		function = LED_FUNCTION_BACKLIGHT;
> +		reg = <2>;
> +
> +		compatible = "led-backlight";
> +
> +		brightness-levels = <0 4 8 16 32 64 128 255>;
> +		default-brightness-level = <6>;
> +
> +		power-supply = <&vdd_bl_reg>;
> +		enable-gpios = <&gpio 58 0>;
> +	};
> +};
> -- 
> 2.17.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] devicetree: Add led-backlight binding

[Jean-Jacques Hiblot]

Daniel,

On 02/07/2019 11:58, Daniel Thompson wrote:
> On Mon, Jul 01, 2019 at 05:14:23PM +0200, Jean-Jacques Hiblot wrote:
>> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>
>> Add DT binding for led-backlight.
> I think the patchset is in the wrong order; the DT bindings
> documentation should appear *before* the binding is
> implemented (amoung other things this prevent transient checkpatch
> warnings as the patchset is applied).
>
ok
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> Cc: devicetree@vger.kernel.org
>> ---
>>   .../video/backlight/led-backlight.txt         | 39 +++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>
>> diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>> new file mode 100644
>> index 000000000000..216cd52d624a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>> @@ -0,0 +1,39 @@
>> +led-backlight bindings
>> +
>> +The node of the backlight driver IS the node of the LED.
>> +
>> +Required properties:
>> +  - compatible: "led-backlight"
>> +  - brightness-levels: Array of distinct LED brightness levels. These
>> +      are in the range from 0 to 255, passed to the LED class driver.
>> +  - default-brightness-level: the default brightness level (index into the
>> +      array defined by the "brightness-levels" property)
> I think brightness-levels and default-brightness-level could be
> optional properties since a default 1:1 mapping seems reasonable given
> how constrained the LED brightness values are.

That is probably a good idea. Expect it in v2

Thanks,

JJ

>
>
> Daniel.
>
>
>> +
>> +Optional properties:
>> +  - power-supply: regulator for supply voltage
>> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>> +                  and disables the backlight (see GPIO binding[0])
>> +
>> +[0]: Documentation/devicetree/bindings/gpio/gpio.txt
>> +
>> +Example:
>> +
>> +led_ctrl {
>> +	red_led@1 {
>> +	        label = "red";
>> +		reg = <1>;
>> +	}
>> +
>> +	backlight_led@2 {
>> +		function = LED_FUNCTION_BACKLIGHT;
>> +		reg = <2>;
>> +
>> +		compatible = "led-backlight";
>> +
>> +		brightness-levels = <0 4 8 16 32 64 128 255>;
>> +		default-brightness-level = <6>;
>> +
>> +		power-supply = <&vdd_bl_reg>;
>> +		enable-gpios = <&gpio 58 0>;
>> +	};
>> +};
>> -- 
>> 2.17.1
>>

Re: [PATCH 4/4] devicetree: Add led-backlight binding

[Pavel Machek]

Hi!

> Add DT binding for led-backlight.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Cc: devicetree@vger.kernel.org
> ---

> +Required properties:
> +  - compatible: "led-backlight"
> +  - brightness-levels: Array of distinct LED brightness levels. These
> +      are in the range from 0 to 255, passed to the LED class driver.

These days, we support more (or less) than 256 brightness levels for LED.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 4/4] devicetree: Add led-backlight binding

[Pavel Machek]

Hi!

> Add DT binding for led-backlight.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Cc: devicetree@vger.kernel.org
> ---

> +Required properties:
> +  - compatible: "led-backlight"
> +  - brightness-levels: Array of distinct LED brightness levels. These
> +      are in the range from 0 to 255, passed to the LED class driver.

These days, we support more (or less) than 256 brightness levels for LED.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/4] Add a generic driver for LED-based backlight

[Pavel Machek]

On Mon 2019-07-01 17:14:19, Jean-Jacques Hiblot wrote:
> This series aims to add a led-backlight driver, similar to pwm-backlight,
> but using a LED class device underneath.
> 
> A few years ago (2015), Tomi Valkeinen posted a series implementing a
> backlight driver on top of a LED device:
> https://patchwork.kernel.org/patch/7293991/
> https://patchwork.kernel.org/patch/7294001/
> https://patchwork.kernel.org/patch/7293981/
> 
> The discussion stopped because Tomi lacked the time to work on it.
> 
> This series takes it from there and implements the binding that was
> discussed in https://patchwork.kernel.org/patch/7293991/. In this new
> binding the backlight device is a child of the LED controller instead of
> being another platform device that uses a phandle to reference a LED.

Other option would be to have backlight trigger. What are
advantages/disadvantages relative to that?
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/4] Add a generic driver for LED-based backlight

[Pavel Machek]

On Mon 2019-07-01 17:14:19, Jean-Jacques Hiblot wrote:
> This series aims to add a led-backlight driver, similar to pwm-backlight,
> but using a LED class device underneath.
> 
> A few years ago (2015), Tomi Valkeinen posted a series implementing a
> backlight driver on top of a LED device:
> https://patchwork.kernel.org/patch/7293991/
> https://patchwork.kernel.org/patch/7294001/
> https://patchwork.kernel.org/patch/7293981/
> 
> The discussion stopped because Tomi lacked the time to work on it.
> 
> This series takes it from there and implements the binding that was
> discussed in https://patchwork.kernel.org/patch/7293991/. In this new
> binding the backlight device is a child of the LED controller instead of
> being another platform device that uses a phandle to reference a LED.

Other option would be to have backlight trigger. What are
advantages/disadvantages relative to that?
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/4] Add a generic driver for LED-based backlight

[Daniel Thompson]

On Fri, Jul 05, 2019 at 12:14:34PM +0200, Pavel Machek wrote:
> On Mon 2019-07-01 17:14:19, Jean-Jacques Hiblot wrote:
> > This series aims to add a led-backlight driver, similar to pwm-backlight,
> > but using a LED class device underneath.
> > 
> > A few years ago (2015), Tomi Valkeinen posted a series implementing a
> > backlight driver on top of a LED device:
> > https://patchwork.kernel.org/patch/7293991/
> > https://patchwork.kernel.org/patch/7294001/
> > https://patchwork.kernel.org/patch/7293981/
> > 
> > The discussion stopped because Tomi lacked the time to work on it.
> > 
> > This series takes it from there and implements the binding that was
> > discussed in https://patchwork.kernel.org/patch/7293991/. In this new
> > binding the backlight device is a child of the LED controller instead of
> > being another platform device that uses a phandle to reference a LED.
> 
> Other option would be to have backlight trigger. What are
> advantages/disadvantages relative to that?

I spent a little time thinking about that during the recent round of
reviews.

My rough thoughts were that LED triggers would be a good way to
handle it in the kernel code and would trivially solve a backlight
composed of multiple LED devices (since all could attach to the same
trigger). However I think it would be difficult to use the existing DT
bindings for triggers to express things like brightness curves and to
handle systems with multiple heads.

I'm not *too* worried about conflict with the existing backlight trigger
(which isn't actually a backlight... just a hook into the framebuffer
code to operate a binary LED). People like Daniel Vetter are
labouring diligently to get rid of the notifier it depends on so turning
it into a proper backlight device would probably help with that effort.

Anyhow... having thought the above I then shook myself a bit and
figured it was more important to focus on sane bindings that on what
the kernel does under the covers to realize them ;-) and decided to keep
quiet until the next set of bindings is proposed.

However... since you asked...



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

Re: [PATCH 0/4] Add a generic driver for LED-based backlight

[Daniel Thompson]

On Fri, Jul 05, 2019 at 12:14:34PM +0200, Pavel Machek wrote:
> On Mon 2019-07-01 17:14:19, Jean-Jacques Hiblot wrote:
> > This series aims to add a led-backlight driver, similar to pwm-backlight,
> > but using a LED class device underneath.
> > 
> > A few years ago (2015), Tomi Valkeinen posted a series implementing a
> > backlight driver on top of a LED device:
> > https://patchwork.kernel.org/patch/7293991/
> > https://patchwork.kernel.org/patch/7294001/
> > https://patchwork.kernel.org/patch/7293981/
> > 
> > The discussion stopped because Tomi lacked the time to work on it.
> > 
> > This series takes it from there and implements the binding that was
> > discussed in https://patchwork.kernel.org/patch/7293991/. In this new
> > binding the backlight device is a child of the LED controller instead of
> > being another platform device that uses a phandle to reference a LED.
> 
> Other option would be to have backlight trigger. What are
> advantages/disadvantages relative to that?

I spent a little time thinking about that during the recent round of
reviews.

My rough thoughts were that LED triggers would be a good way to
handle it in the kernel code and would trivially solve a backlight
composed of multiple LED devices (since all could attach to the same
trigger). However I think it would be difficult to use the existing DT
bindings for triggers to express things like brightness curves and to
handle systems with multiple heads.

I'm not *too* worried about conflict with the existing backlight trigger
(which isn't actually a backlight... just a hook into the framebuffer
code to operate a binary LED). People like Daniel Vetter are
labouring diligently to get rid of the notifier it depends on so turning
it into a proper backlight device would probably help with that effort.

Anyhow... having thought the above I then shook myself a bit and
figured it was more important to focus on sane bindings that on what
the kernel does under the covers to realize them ;-) and decided to keep
quiet until the next set of bindings is proposed.

However... since you asked...



Daniel.

Re: [PATCH 0/4] Add a generic driver for LED-based backlight

[Jean-Jacques Hiblot]

On 05/07/2019 12:14, Pavel Machek wrote:
> On Mon 2019-07-01 17:14:19, Jean-Jacques Hiblot wrote:
>>
>> This series takes it from there and implements the binding that was
>> discussed in https://patchwork.kernel.org/patch/7293991/. In this new
>> binding the backlight device is a child of the LED controller instead of
>> being another platform device that uses a phandle to reference a LED.
> Other option would be to have backlight trigger. What are
> advantages/disadvantages relative to that?

I don't know how this would fit in the model where multiple panels are 
used, each with its own backlight.

Also the notification is only about blanking.

> 									Pavel
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/4] Add a generic driver for LED-based backlight

[Jean-Jacques Hiblot]

On 05/07/2019 12:14, Pavel Machek wrote:
> On Mon 2019-07-01 17:14:19, Jean-Jacques Hiblot wrote:
>>
>> This series takes it from there and implements the binding that was
>> discussed in https://patchwork.kernel.org/patch/7293991/. In this new
>> binding the backlight device is a child of the LED controller instead of
>> being another platform device that uses a phandle to reference a LED.
> Other option would be to have backlight trigger. What are
> advantages/disadvantages relative to that?

I don't know how this would fit in the model where multiple panels are 
used, each with its own backlight.

Also the notification is only about blanking.

> 									Pavel
>

Re: [PATCH 0/4] Add a generic driver for LED-based backlight

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 1370 bytes --]

Hi,

On Fri, Jul 05, 2019 at 12:14:34PM +0200, Pavel Machek wrote:
> On Mon 2019-07-01 17:14:19, Jean-Jacques Hiblot wrote:
> > This series aims to add a led-backlight driver, similar to pwm-backlight,
> > but using a LED class device underneath.
> > 
> > A few years ago (2015), Tomi Valkeinen posted a series implementing a
> > backlight driver on top of a LED device:
> > https://patchwork.kernel.org/patch/7293991/
> > https://patchwork.kernel.org/patch/7294001/
> > https://patchwork.kernel.org/patch/7293981/
> > 
> > The discussion stopped because Tomi lacked the time to work on it.
> > 
> > This series takes it from there and implements the binding that was
> > discussed in https://patchwork.kernel.org/patch/7293991/. In this new
> > binding the backlight device is a child of the LED controller instead of
> > being another platform device that uses a phandle to reference a LED.
> 
> Other option would be to have backlight trigger. What are
> advantages/disadvantages relative to that?

One advantage of having something like this is the possibility to
reference the backlight in DT. This means the system has an idea
how backlights are mapped. E.g.:

panelA {
    compatible = "random-panel";
    backlight = <&backlight1>;
}

panelB {
    compatible = "random-panel";
    backlight = <&backlight2>;
}

-- Sebastian

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 0/4] Add a generic driver for LED-based backlight

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1370 bytes --]

Hi,

On Fri, Jul 05, 2019 at 12:14:34PM +0200, Pavel Machek wrote:
> On Mon 2019-07-01 17:14:19, Jean-Jacques Hiblot wrote:
> > This series aims to add a led-backlight driver, similar to pwm-backlight,
> > but using a LED class device underneath.
> > 
> > A few years ago (2015), Tomi Valkeinen posted a series implementing a
> > backlight driver on top of a LED device:
> > https://patchwork.kernel.org/patch/7293991/
> > https://patchwork.kernel.org/patch/7294001/
> > https://patchwork.kernel.org/patch/7293981/
> > 
> > The discussion stopped because Tomi lacked the time to work on it.
> > 
> > This series takes it from there and implements the binding that was
> > discussed in https://patchwork.kernel.org/patch/7293991/. In this new
> > binding the backlight device is a child of the LED controller instead of
> > being another platform device that uses a phandle to reference a LED.
> 
> Other option would be to have backlight trigger. What are
> advantages/disadvantages relative to that?

One advantage of having something like this is the possibility to
reference the backlight in DT. This means the system has an idea
how backlights are mapped. E.g.:

panelA {
    compatible = "random-panel";
    backlight = <&backlight1>;
}

panelB {
    compatible = "random-panel";
    backlight = <&backlight2>;
}

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]