[PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Alexandre Courbot]

This series introduces a way to use pwm-backlight hooks with platforms
that use the device tree through a subdriver system. It also adds support
for the Tegra-based Ventana board, adding the last missing block to enable
its panel. Support for other Tegra board can thus be easily added.

I have something else in mind to properly support this (power
sequences), but this work relies on the GPIO subsystem redesign which will
take some time. The pwm-backlight subdrivers can do the job by the meantime.

There are a few design points that might need to be discussed:
1) Link order is important: subdrivers register themselves in their
module_init function, which must be called before pwm-backlight's probe.
This forbids linking subdrivers as separate modules from pwm-backlight.
2) The subdriver's data is temporarily passed through the backlight
device's driver data. This should not hurt, but maybe there is a better way
to do this.
3) Subdrivers must add themselves into pwm-backlight's own of_device_id
table. It would be cleaner to not have to list subdrivers into
pwm-backlight's main file, but I cannot think of a way to do otherwise.

Suggestions for the 3 points listed above are very welcome - in any case,
I hope to make this converge into something mergeable quickly.

Note that these patches are the last missing block to get a functional
panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
on Ventana is usable out-of-the-box. Yay.

Alexandre Courbot (3):
  pwm-backlight: add subdriver mechanism
  tegra: pwm-backlight: add tegra pwm-bl driver
  tegra: ventana: of: add host1x device to DT

 arch/arm/boot/dts/tegra20-ventana.dts  |  29 +++++-
 arch/arm/configs/tegra_defconfig       |   1 +
 drivers/video/backlight/Kconfig        |   7 ++
 drivers/video/backlight/Makefile       |   4 +
 drivers/video/backlight/pwm_bl.c       |  70 ++++++++++++++-
 drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
 include/linux/pwm_backlight.h          |  15 ++++
 7 files changed, 281 insertions(+), 4 deletions(-)
 create mode 100644 drivers/video/backlight/pwm_bl_tegra.c

-- 
1.8.1.1

[PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Alexandre Courbot]

This series introduces a way to use pwm-backlight hooks with platforms
that use the device tree through a subdriver system. It also adds support
for the Tegra-based Ventana board, adding the last missing block to enable
its panel. Support for other Tegra board can thus be easily added.

I have something else in mind to properly support this (power
sequences), but this work relies on the GPIO subsystem redesign which will
take some time. The pwm-backlight subdrivers can do the job by the meantime.

There are a few design points that might need to be discussed:
1) Link order is important: subdrivers register themselves in their
module_init function, which must be called before pwm-backlight's probe.
This forbids linking subdrivers as separate modules from pwm-backlight.
2) The subdriver's data is temporarily passed through the backlight
device's driver data. This should not hurt, but maybe there is a better way
to do this.
3) Subdrivers must add themselves into pwm-backlight's own of_device_id
table. It would be cleaner to not have to list subdrivers into
pwm-backlight's main file, but I cannot think of a way to do otherwise.

Suggestions for the 3 points listed above are very welcome - in any case,
I hope to make this converge into something mergeable quickly.

Note that these patches are the last missing block to get a functional
panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
on Ventana is usable out-of-the-box. Yay.

Alexandre Courbot (3):
  pwm-backlight: add subdriver mechanism
  tegra: pwm-backlight: add tegra pwm-bl driver
  tegra: ventana: of: add host1x device to DT

 arch/arm/boot/dts/tegra20-ventana.dts  |  29 +++++-
 arch/arm/configs/tegra_defconfig       |   1 +
 drivers/video/backlight/Kconfig        |   7 ++
 drivers/video/backlight/Makefile       |   4 +
 drivers/video/backlight/pwm_bl.c       |  70 ++++++++++++++-
 drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
 include/linux/pwm_backlight.h          |  15 ++++
 7 files changed, 281 insertions(+), 4 deletions(-)
 create mode 100644 drivers/video/backlight/pwm_bl_tegra.c

-- 
1.8.1.1

[PATCH 1/3] pwm-backlight: add subdriver mechanism

[Alexandre Courbot]

PWM-controlled backlights often need additional power control prior
to activating the PWM, typically switching regulators or GPIOs. This has
been done so far through hooks defined in board files, but this
mechanism cannot be used on platforms that rely on the device tree.

This patch introduces a "subdriver" mechanism to the pwm-backlight
driver that allows such hooks to be defined in optionally-compiled
sub-drivers. Every subdriver has its own device tree properties, which
sets the correct hooks to the pwm-backlight driver.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/video/backlight/Makefile |  4 +++
 drivers/video/backlight/pwm_bl.c | 67 +++++++++++++++++++++++++++++++++++++++-
 include/linux/pwm_backlight.h    | 15 +++++++++
 3 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index e7ce729..df97ab1 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -29,6 +29,10 @@ obj-$(CONFIG_BACKLIGHT_LP855X)	+= lp855x_bl.o
 obj-$(CONFIG_BACKLIGHT_OMAP1)	+= omap1_bl.o
 obj-$(CONFIG_BACKLIGHT_PANDORA)	+= pandora_bl.o
 obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
+# pwm-backlight subdrivers must be listed *before* pwm_bl.o.
+# Link order is important as subdrivers must register themselves
+# before pwm-backlight's probe function can be called.
+obj-$(CONFIG_BACKLIGHT_PWM_TEGRA) += pwm_bl_tegra.o
 obj-$(CONFIG_BACKLIGHT_PWM)	+= pwm_bl.o
 obj-$(CONFIG_BACKLIGHT_DA903X)	+= da903x_bl.o
 obj-$(CONFIG_BACKLIGHT_DA9052)	+= da9052_bl.o
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..b65a797 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 
 struct pwm_bl_data {
+	void			*subdriver_data;
 	struct pwm_device	*pwm;
 	struct device		*dev;
 	unsigned int		period;
@@ -35,6 +36,54 @@ struct pwm_bl_data {
 	void			(*exit)(struct device *);
 };
 
+static DEFINE_MUTEX(pwm_backlight_subdrivers_mutex);
+static LIST_HEAD(pwm_backlight_subdrivers);
+
+void pwm_backlight_add_subdriver(struct pwm_backlight_subdriver *driver)
+{
+	mutex_lock(&pwm_backlight_subdrivers_mutex);
+	list_add(&driver->list, &pwm_backlight_subdrivers);
+	mutex_unlock(&pwm_backlight_subdrivers_mutex);
+}
+EXPORT_SYMBOL(pwm_backlight_add_subdriver);
+
+void pwm_backlight_remove_subdriver(struct pwm_backlight_subdriver *driver)
+{
+	mutex_lock(&pwm_backlight_subdrivers_mutex);
+	list_del(&driver->list);
+	mutex_unlock(&pwm_backlight_subdrivers_mutex);
+}
+EXPORT_SYMBOL(pwm_backlight_remove_subdriver);
+
+/**
+ * pwm_backlight_set_subdriver_data - set subdriver data
+ * @dev: backlight device which data is to be set
+ * @data: subdriver data
+ *
+ * This function can be called *only* in the init() hook of the subdriver. The
+ * data will be temporarily set as driver data before being retrieved by
+ * the probe() function and moved to its final place.
+ */
+void pwm_backlight_set_subdriver_data(struct device *dev, void *data)
+{
+	dev_set_drvdata(dev, data);
+}
+EXPORT_SYMBOL(pwm_backlight_set_subdriver_data);
+
+/**
+ * pwm_backlight_get_subdriver_data - retrieve subdriver data
+ * @dev: backlight device to get subdriver data of
+ *
+ * This function can be called in any subdriver hook, excepted init().
+ */
+void *pwm_backlight_get_subdriver_data(struct device *dev)
+{
+	struct backlight_device *bl = dev_get_drvdata(dev);
+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+	return pb->subdriver_data;
+}
+EXPORT_SYMBOL(pwm_backlight_get_subdriver_data);
+
 static int pwm_backlight_update_status(struct backlight_device *bl)
 {
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
@@ -98,6 +147,7 @@ static const struct backlight_ops pwm_backlight_ops = {
 static int pwm_backlight_parse_dt(struct device *dev,
 				  struct platform_pwm_backlight_data *data)
 {
+	struct pwm_backlight_subdriver *subdriver;
 	struct device_node *node = dev->of_node;
 	struct property *prop;
 	int length;
@@ -150,6 +200,17 @@ static int pwm_backlight_parse_dt(struct device *dev,
 	 *       backlight power. Support for specifying these needs to be
 	 *       added.
 	 */
+	mutex_lock(&pwm_backlight_subdrivers_mutex);
+	list_for_each_entry(subdriver, &pwm_backlight_subdrivers, list)
+		if (of_device_is_compatible(node, subdriver->name)) {
+			data->init = subdriver->init;
+			data->exit = subdriver->exit;
+			data->notify = subdriver->notify;
+			data->notify_after = subdriver->notify_after;
+			data->check_fb = subdriver->check_fb;
+			break;
+		}
+	mutex_unlock(&pwm_backlight_subdrivers_mutex);
 
 	return 0;
 }
@@ -201,6 +262,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		goto err_alloc;
 	}
 
+	/* if the init function set subdriver data, move it to correct place */
+	pb->subdriver_data = dev_get_drvdata(&pdev->dev);
+
 	if (data->levels) {
 		max = data->levels[data->max_brightness];
 		pb->levels = data->levels;
@@ -249,10 +313,11 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		goto err_alloc;
 	}
 
+	platform_set_drvdata(pdev, bl);
+
 	bl->props.brightness = data->dft_brightness;
 	backlight_update_status(bl);
 
-	platform_set_drvdata(pdev, bl);
 	return 0;
 
 err_alloc:
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..6abe1ef 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -20,4 +20,19 @@ struct platform_pwm_backlight_data {
 	int (*check_fb)(struct device *dev, struct fb_info *info);
 };
 
+struct pwm_backlight_subdriver {
+	struct list_head list;
+	const char *name;
+	int (*init)(struct device *dev);
+	int (*notify)(struct device *dev, int brightness);
+	void (*notify_after)(struct device *dev, int brightness);
+	void (*exit)(struct device *dev);
+	int (*check_fb)(struct device *dev, struct fb_info *info);
+};
+
+void pwm_backlight_add_subdriver(struct pwm_backlight_subdriver *driver);
+void pwm_backlight_remove_subdriver(struct pwm_backlight_subdriver *driver);
+
+void pwm_backlight_set_subdriver_data(struct device *dev, void *data);
+void *pwm_backlight_get_subdriver_data(struct device *dev);
 #endif
-- 
1.8.1.1

[PATCH 1/3] pwm-backlight: add subdriver mechanism

[Alexandre Courbot]

PWM-controlled backlights often need additional power control prior
to activating the PWM, typically switching regulators or GPIOs. This has
been done so far through hooks defined in board files, but this
mechanism cannot be used on platforms that rely on the device tree.

This patch introduces a "subdriver" mechanism to the pwm-backlight
driver that allows such hooks to be defined in optionally-compiled
sub-drivers. Every subdriver has its own device tree properties, which
sets the correct hooks to the pwm-backlight driver.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/video/backlight/Makefile |  4 +++
 drivers/video/backlight/pwm_bl.c | 67 +++++++++++++++++++++++++++++++++++++++-
 include/linux/pwm_backlight.h    | 15 +++++++++
 3 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index e7ce729..df97ab1 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -29,6 +29,10 @@ obj-$(CONFIG_BACKLIGHT_LP855X)	+= lp855x_bl.o
 obj-$(CONFIG_BACKLIGHT_OMAP1)	+= omap1_bl.o
 obj-$(CONFIG_BACKLIGHT_PANDORA)	+= pandora_bl.o
 obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
+# pwm-backlight subdrivers must be listed *before* pwm_bl.o.
+# Link order is important as subdrivers must register themselves
+# before pwm-backlight's probe function can be called.
+obj-$(CONFIG_BACKLIGHT_PWM_TEGRA) += pwm_bl_tegra.o
 obj-$(CONFIG_BACKLIGHT_PWM)	+= pwm_bl.o
 obj-$(CONFIG_BACKLIGHT_DA903X)	+= da903x_bl.o
 obj-$(CONFIG_BACKLIGHT_DA9052)	+= da9052_bl.o
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..b65a797 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 
 struct pwm_bl_data {
+	void			*subdriver_data;
 	struct pwm_device	*pwm;
 	struct device		*dev;
 	unsigned int		period;
@@ -35,6 +36,54 @@ struct pwm_bl_data {
 	void			(*exit)(struct device *);
 };
 
+static DEFINE_MUTEX(pwm_backlight_subdrivers_mutex);
+static LIST_HEAD(pwm_backlight_subdrivers);
+
+void pwm_backlight_add_subdriver(struct pwm_backlight_subdriver *driver)
+{
+	mutex_lock(&pwm_backlight_subdrivers_mutex);
+	list_add(&driver->list, &pwm_backlight_subdrivers);
+	mutex_unlock(&pwm_backlight_subdrivers_mutex);
+}
+EXPORT_SYMBOL(pwm_backlight_add_subdriver);
+
+void pwm_backlight_remove_subdriver(struct pwm_backlight_subdriver *driver)
+{
+	mutex_lock(&pwm_backlight_subdrivers_mutex);
+	list_del(&driver->list);
+	mutex_unlock(&pwm_backlight_subdrivers_mutex);
+}
+EXPORT_SYMBOL(pwm_backlight_remove_subdriver);
+
+/**
+ * pwm_backlight_set_subdriver_data - set subdriver data
+ * @dev: backlight device which data is to be set
+ * @data: subdriver data
+ *
+ * This function can be called *only* in the init() hook of the subdriver. The
+ * data will be temporarily set as driver data before being retrieved by
+ * the probe() function and moved to its final place.
+ */
+void pwm_backlight_set_subdriver_data(struct device *dev, void *data)
+{
+	dev_set_drvdata(dev, data);
+}
+EXPORT_SYMBOL(pwm_backlight_set_subdriver_data);
+
+/**
+ * pwm_backlight_get_subdriver_data - retrieve subdriver data
+ * @dev: backlight device to get subdriver data of
+ *
+ * This function can be called in any subdriver hook, excepted init().
+ */
+void *pwm_backlight_get_subdriver_data(struct device *dev)
+{
+	struct backlight_device *bl = dev_get_drvdata(dev);
+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+	return pb->subdriver_data;
+}
+EXPORT_SYMBOL(pwm_backlight_get_subdriver_data);
+
 static int pwm_backlight_update_status(struct backlight_device *bl)
 {
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
@@ -98,6 +147,7 @@ static const struct backlight_ops pwm_backlight_ops = {
 static int pwm_backlight_parse_dt(struct device *dev,
 				  struct platform_pwm_backlight_data *data)
 {
+	struct pwm_backlight_subdriver *subdriver;
 	struct device_node *node = dev->of_node;
 	struct property *prop;
 	int length;
@@ -150,6 +200,17 @@ static int pwm_backlight_parse_dt(struct device *dev,
 	 *       backlight power. Support for specifying these needs to be
 	 *       added.
 	 */
+	mutex_lock(&pwm_backlight_subdrivers_mutex);
+	list_for_each_entry(subdriver, &pwm_backlight_subdrivers, list)
+		if (of_device_is_compatible(node, subdriver->name)) {
+			data->init = subdriver->init;
+			data->exit = subdriver->exit;
+			data->notify = subdriver->notify;
+			data->notify_after = subdriver->notify_after;
+			data->check_fb = subdriver->check_fb;
+			break;
+		}
+	mutex_unlock(&pwm_backlight_subdrivers_mutex);
 
 	return 0;
 }
@@ -201,6 +262,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		goto err_alloc;
 	}
 
+	/* if the init function set subdriver data, move it to correct place */
+	pb->subdriver_data = dev_get_drvdata(&pdev->dev);
+
 	if (data->levels) {
 		max = data->levels[data->max_brightness];
 		pb->levels = data->levels;
@@ -249,10 +313,11 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		goto err_alloc;
 	}
 
+	platform_set_drvdata(pdev, bl);
+
 	bl->props.brightness = data->dft_brightness;
 	backlight_update_status(bl);
 
-	platform_set_drvdata(pdev, bl);
 	return 0;
 
 err_alloc:
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..6abe1ef 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -20,4 +20,19 @@ struct platform_pwm_backlight_data {
 	int (*check_fb)(struct device *dev, struct fb_info *info);
 };
 
+struct pwm_backlight_subdriver {
+	struct list_head list;
+	const char *name;
+	int (*init)(struct device *dev);
+	int (*notify)(struct device *dev, int brightness);
+	void (*notify_after)(struct device *dev, int brightness);
+	void (*exit)(struct device *dev);
+	int (*check_fb)(struct device *dev, struct fb_info *info);
+};
+
+void pwm_backlight_add_subdriver(struct pwm_backlight_subdriver *driver);
+void pwm_backlight_remove_subdriver(struct pwm_backlight_subdriver *driver);
+
+void pwm_backlight_set_subdriver_data(struct device *dev, void *data);
+void *pwm_backlight_get_subdriver_data(struct device *dev);
 #endif
-- 
1.8.1.1

[PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alexandre Courbot]

Add a PWM-backlight subdriver for Tegra boards, with support for
Ventana.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
 arch/arm/configs/tegra_defconfig       |   1 +
 drivers/video/backlight/Kconfig        |   7 ++
 drivers/video/backlight/pwm_bl.c       |   3 +
 drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
 5 files changed, 186 insertions(+), 2 deletions(-)
 create mode 100644 drivers/video/backlight/pwm_bl_tegra.c

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index adc4754..a77b529 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -516,6 +516,20 @@
 		bus-width = <8>;
 	};
 
+	backlight {
+		compatible = "pwm-backlight-ventana";
+		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
+		default-brightness-level = <12>;
+
+		pwms = <&pwm 2 5000000>;
+		pwm-names = "backlight";
+
+		power-supply = <&vdd_bl_reg>;
+		panel-supply = <&vdd_pnl_reg>;
+		bl-gpio = <&gpio 28 0>;
+		bl-panel = <&gpio 10 0>;
+	};
+
 	regulators {
 		compatible = "simple-bus";
 		#address-cells = <1>;
@@ -549,7 +563,7 @@
 			enable-active-high;
 		};
 
-		regulator@3 {
+		vdd_pnl_reg: regulator@3 {
 			compatible = "regulator-fixed";
 			reg = <3>;
 			regulator-name = "vdd_pnl";
@@ -559,7 +573,7 @@
 			enable-active-high;
 		};
 
-		regulator@4 {
+		vdd_bl_reg: regulator@4 {
 			compatible = "regulator-fixed";
 			reg = <4>;
 			regulator-name = "vdd_bl";
diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index a7827fd..1c46602 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig
@@ -150,6 +150,7 @@ CONFIG_BACKLIGHT_LCD_SUPPORT=y
 CONFIG_BACKLIGHT_CLASS_DEVICE=y
 # CONFIG_BACKLIGHT_GENERIC is not set
 CONFIG_BACKLIGHT_PWM=y
+CONFIG_BACKLIGHT_PWM_TEGRA=y
 CONFIG_FRAMEBUFFER_CONSOLE=y
 CONFIG_LOGO=y
 CONFIG_SOUND=y
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 765a945..377a409 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -244,6 +244,13 @@ config BACKLIGHT_PWM
 	  If you have a LCD backlight adjustable by PWM, say Y to enable
 	  this driver.
 
+config BACKLIGHT_PWM_TEGRA
+	bool "PWM Backlight Driver for Tegra boards"
+	depends on BACKLIGHT_PWM && ARCH_TEGRA
+	help
+	  Support backlight power sequencing for Tegra boards.
+	  Supported boards: Ventana.
+
 config BACKLIGHT_DA903X
 	tristate "Backlight Driver for DA9030/DA9034 using WLED"
 	depends on PMIC_DA903X
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index b65a797..1a4a9a3 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -217,6 +217,9 @@ static int pwm_backlight_parse_dt(struct device *dev,
 
 static struct of_device_id pwm_backlight_of_match[] = {
 	{ .compatible = "pwm-backlight" },
+#ifdef CONFIG_BACKLIGHT_PWM_TEGRA
+	{ .compatible = "pwm-backlight-ventana" },
+#endif
 	{ }
 };
 
diff --git a/drivers/video/backlight/pwm_bl_tegra.c b/drivers/video/backlight/pwm_bl_tegra.c
new file mode 100644
index 0000000..8f2195b
--- /dev/null
+++ b/drivers/video/backlight/pwm_bl_tegra.c
@@ -0,0 +1,159 @@
+/*
+ * pwm-backlight subdriver for Tegra.
+ *
+ * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/pwm_backlight.h>
+#include <linux/regulator/consumer.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/list.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+
+struct ventana_bl_data {
+	struct regulator *vdd_power;
+	struct regulator *vdd_panel;
+	int bl_gpio;
+	int panel_gpio;
+	bool is_on;
+};
+
+static int init_ventana(struct device *dev)
+{
+	struct ventana_bl_data *data;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(struct ventana_bl_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->vdd_power = devm_regulator_get(dev, "power");
+	if (IS_ERR(data->vdd_power)) {
+		dev_err(dev, "cannot get power regulator!\n");
+		return PTR_ERR(data->vdd_power);
+	}
+
+	data->vdd_panel = devm_regulator_get(dev, "panel");
+	if (IS_ERR(data->vdd_panel)) {
+		dev_err(dev, "cannot get panel regulator!\n");
+		return PTR_ERR(data->vdd_panel);
+	}
+
+	ret = of_get_named_gpio(dev->of_node, "bl-gpio", 0);
+	if (ret < 0) {
+		dev_err(dev, "cannot find backlight GPIO!\n");
+		return ret;
+	}
+	data->bl_gpio = ret;
+
+	ret = of_get_named_gpio(dev->of_node, "bl-panel", 0);
+	if (ret < 0) {
+		dev_err(dev, "cannot find panel GPIO!\n");
+		return ret;
+	}
+	data->panel_gpio = ret;
+
+	ret = devm_gpio_request_one(dev, data->bl_gpio,
+				    GPIOF_DIR_OUT | GPIOF_OUT_INIT_LOW,
+				    "backlight");
+	if (ret < 0) {
+		dev_err(dev, "cannot request backlight GPIO!\n");
+		return ret;
+	}
+
+	ret = devm_gpio_request_one(dev, data->panel_gpio,
+				    GPIOF_DIR_OUT | GPIOF_OUT_INIT_LOW,
+				    "panel");
+	if (ret < 0) {
+		dev_err(dev, "cannot request panel GPIO!\n");
+		return ret;
+	}
+
+	pwm_backlight_set_subdriver_data(dev, data);
+
+	return 0;
+}
+
+static void exit_ventana(struct device *dev)
+{
+	struct ventana_bl_data *data = pwm_backlight_get_subdriver_data(dev);
+
+	devm_gpio_free(dev, data->panel_gpio);
+	devm_gpio_free(dev, data->bl_gpio);
+	devm_regulator_put(data->vdd_panel);
+	devm_regulator_put(data->vdd_power);
+	devm_kfree(dev, data);
+}
+
+static int notify_ventana(struct device *dev, int brightness)
+{
+	struct ventana_bl_data *data = pwm_backlight_get_subdriver_data(dev);
+	if (brightness && !data->is_on) {
+		regulator_enable(data->vdd_panel);
+		gpio_set_value(data->panel_gpio, 1);
+		usleep_range(200000, 200000);
+		regulator_enable(data->vdd_power);
+		usleep_range(10000, 10000);
+	} else if (!brightness && data->is_on) {
+		gpio_set_value(data->bl_gpio, 0);
+	}
+
+	return brightness;
+}
+
+static void notify_after_ventana(struct device *dev, int brightness)
+{
+	struct ventana_bl_data *data = pwm_backlight_get_subdriver_data(dev);
+	if (brightness && !data->is_on) {
+		gpio_set_value(data->bl_gpio, 1);
+		data->is_on = true;
+	} else if (!brightness && data->is_on) {
+		usleep_range(10000, 10000);
+		regulator_disable(data->vdd_power);
+		usleep_range(200000, 200000);
+		gpio_set_value(data->panel_gpio, 0);
+		regulator_disable(data->vdd_panel);
+		data->is_on = false;
+	}
+}
+
+static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
+	.name = "pwm-backlight-ventana",
+	.init = init_ventana,
+	.exit = exit_ventana,
+	.notify = notify_ventana,
+	.notify_after = notify_after_ventana,
+};
+
+static int __init pwm_backlight_tegra_init(void)
+{
+	pwm_backlight_add_subdriver(&pwm_backlight_ventana_subdriver);
+	return 0;
+}
+
+static void __exit pwm_backlight_tegra_exit(void)
+{
+	pwm_backlight_remove_subdriver(&pwm_backlight_ventana_subdriver);
+}
+
+module_init(pwm_backlight_tegra_init);
+module_exit(pwm_backlight_tegra_exit);
+
+MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:pwm-tegra-backlight");
+
+
-- 
1.8.1.1

[PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alexandre Courbot]

Add a PWM-backlight subdriver for Tegra boards, with support for
Ventana.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
 arch/arm/configs/tegra_defconfig       |   1 +
 drivers/video/backlight/Kconfig        |   7 ++
 drivers/video/backlight/pwm_bl.c       |   3 +
 drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
 5 files changed, 186 insertions(+), 2 deletions(-)
 create mode 100644 drivers/video/backlight/pwm_bl_tegra.c

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index adc4754..a77b529 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -516,6 +516,20 @@
 		bus-width = <8>;
 	};
 
+	backlight {
+		compatible = "pwm-backlight-ventana";
+		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
+		default-brightness-level = <12>;
+
+		pwms = <&pwm 2 5000000>;
+		pwm-names = "backlight";
+
+		power-supply = <&vdd_bl_reg>;
+		panel-supply = <&vdd_pnl_reg>;
+		bl-gpio = <&gpio 28 0>;
+		bl-panel = <&gpio 10 0>;
+	};
+
 	regulators {
 		compatible = "simple-bus";
 		#address-cells = <1>;
@@ -549,7 +563,7 @@
 			enable-active-high;
 		};
 
-		regulator@3 {
+		vdd_pnl_reg: regulator@3 {
 			compatible = "regulator-fixed";
 			reg = <3>;
 			regulator-name = "vdd_pnl";
@@ -559,7 +573,7 @@
 			enable-active-high;
 		};
 
-		regulator@4 {
+		vdd_bl_reg: regulator@4 {
 			compatible = "regulator-fixed";
 			reg = <4>;
 			regulator-name = "vdd_bl";
diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index a7827fd..1c46602 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig
@@ -150,6 +150,7 @@ CONFIG_BACKLIGHT_LCD_SUPPORT=y
 CONFIG_BACKLIGHT_CLASS_DEVICE=y
 # CONFIG_BACKLIGHT_GENERIC is not set
 CONFIG_BACKLIGHT_PWM=y
+CONFIG_BACKLIGHT_PWM_TEGRA=y
 CONFIG_FRAMEBUFFER_CONSOLE=y
 CONFIG_LOGO=y
 CONFIG_SOUND=y
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 765a945..377a409 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -244,6 +244,13 @@ config BACKLIGHT_PWM
 	  If you have a LCD backlight adjustable by PWM, say Y to enable
 	  this driver.
 
+config BACKLIGHT_PWM_TEGRA
+	bool "PWM Backlight Driver for Tegra boards"
+	depends on BACKLIGHT_PWM && ARCH_TEGRA
+	help
+	  Support backlight power sequencing for Tegra boards.
+	  Supported boards: Ventana.
+
 config BACKLIGHT_DA903X
 	tristate "Backlight Driver for DA9030/DA9034 using WLED"
 	depends on PMIC_DA903X
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index b65a797..1a4a9a3 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -217,6 +217,9 @@ static int pwm_backlight_parse_dt(struct device *dev,
 
 static struct of_device_id pwm_backlight_of_match[] = {
 	{ .compatible = "pwm-backlight" },
+#ifdef CONFIG_BACKLIGHT_PWM_TEGRA
+	{ .compatible = "pwm-backlight-ventana" },
+#endif
 	{ }
 };
 
diff --git a/drivers/video/backlight/pwm_bl_tegra.c b/drivers/video/backlight/pwm_bl_tegra.c
new file mode 100644
index 0000000..8f2195b
--- /dev/null
+++ b/drivers/video/backlight/pwm_bl_tegra.c
@@ -0,0 +1,159 @@
+/*
+ * pwm-backlight subdriver for Tegra.
+ *
+ * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/pwm_backlight.h>
+#include <linux/regulator/consumer.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/list.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+
+struct ventana_bl_data {
+	struct regulator *vdd_power;
+	struct regulator *vdd_panel;
+	int bl_gpio;
+	int panel_gpio;
+	bool is_on;
+};
+
+static int init_ventana(struct device *dev)
+{
+	struct ventana_bl_data *data;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(struct ventana_bl_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->vdd_power = devm_regulator_get(dev, "power");
+	if (IS_ERR(data->vdd_power)) {
+		dev_err(dev, "cannot get power regulator!\n");
+		return PTR_ERR(data->vdd_power);
+	}
+
+	data->vdd_panel = devm_regulator_get(dev, "panel");
+	if (IS_ERR(data->vdd_panel)) {
+		dev_err(dev, "cannot get panel regulator!\n");
+		return PTR_ERR(data->vdd_panel);
+	}
+
+	ret = of_get_named_gpio(dev->of_node, "bl-gpio", 0);
+	if (ret < 0) {
+		dev_err(dev, "cannot find backlight GPIO!\n");
+		return ret;
+	}
+	data->bl_gpio = ret;
+
+	ret = of_get_named_gpio(dev->of_node, "bl-panel", 0);
+	if (ret < 0) {
+		dev_err(dev, "cannot find panel GPIO!\n");
+		return ret;
+	}
+	data->panel_gpio = ret;
+
+	ret = devm_gpio_request_one(dev, data->bl_gpio,
+				    GPIOF_DIR_OUT | GPIOF_OUT_INIT_LOW,
+				    "backlight");
+	if (ret < 0) {
+		dev_err(dev, "cannot request backlight GPIO!\n");
+		return ret;
+	}
+
+	ret = devm_gpio_request_one(dev, data->panel_gpio,
+				    GPIOF_DIR_OUT | GPIOF_OUT_INIT_LOW,
+				    "panel");
+	if (ret < 0) {
+		dev_err(dev, "cannot request panel GPIO!\n");
+		return ret;
+	}
+
+	pwm_backlight_set_subdriver_data(dev, data);
+
+	return 0;
+}
+
+static void exit_ventana(struct device *dev)
+{
+	struct ventana_bl_data *data = pwm_backlight_get_subdriver_data(dev);
+
+	devm_gpio_free(dev, data->panel_gpio);
+	devm_gpio_free(dev, data->bl_gpio);
+	devm_regulator_put(data->vdd_panel);
+	devm_regulator_put(data->vdd_power);
+	devm_kfree(dev, data);
+}
+
+static int notify_ventana(struct device *dev, int brightness)
+{
+	struct ventana_bl_data *data = pwm_backlight_get_subdriver_data(dev);
+	if (brightness && !data->is_on) {
+		regulator_enable(data->vdd_panel);
+		gpio_set_value(data->panel_gpio, 1);
+		usleep_range(200000, 200000);
+		regulator_enable(data->vdd_power);
+		usleep_range(10000, 10000);
+	} else if (!brightness && data->is_on) {
+		gpio_set_value(data->bl_gpio, 0);
+	}
+
+	return brightness;
+}
+
+static void notify_after_ventana(struct device *dev, int brightness)
+{
+	struct ventana_bl_data *data = pwm_backlight_get_subdriver_data(dev);
+	if (brightness && !data->is_on) {
+		gpio_set_value(data->bl_gpio, 1);
+		data->is_on = true;
+	} else if (!brightness && data->is_on) {
+		usleep_range(10000, 10000);
+		regulator_disable(data->vdd_power);
+		usleep_range(200000, 200000);
+		gpio_set_value(data->panel_gpio, 0);
+		regulator_disable(data->vdd_panel);
+		data->is_on = false;
+	}
+}
+
+static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
+	.name = "pwm-backlight-ventana",
+	.init = init_ventana,
+	.exit = exit_ventana,
+	.notify = notify_ventana,
+	.notify_after = notify_after_ventana,
+};
+
+static int __init pwm_backlight_tegra_init(void)
+{
+	pwm_backlight_add_subdriver(&pwm_backlight_ventana_subdriver);
+	return 0;
+}
+
+static void __exit pwm_backlight_tegra_exit(void)
+{
+	pwm_backlight_remove_subdriver(&pwm_backlight_ventana_subdriver);
+}
+
+module_init(pwm_backlight_tegra_init);
+module_exit(pwm_backlight_tegra_exit);
+
+MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:pwm-tegra-backlight");
+
+
-- 
1.8.1.1

[PATCH 3/3] tegra: ventana: of: add host1x device to DT

[Alexandre Courbot]

Add the host1x device and DDC i2c bus to enable internal panel on
Ventana.

Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra20-ventana.dts | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index a77b529..4477e9c 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -10,6 +10,15 @@
 		reg = <0x00000000 0x40000000>;
 	};
 
+	host1x {
+		dc@54200000 {
+			rgb {
+				status = "okay";
+				nvidia,ddc-i2c-bus = <&lcd_ddc>;
+			};
+		};
+	};
+
 	pinmux {
 		pinctrl-names = "default";
 		pinctrl-0 = <&state_default>;
@@ -341,7 +350,7 @@
 			#size-cells = <0>;
 		};
 
-		i2c@1 {
+		lcd_ddc: i2c@1 {
 			reg = <1>;
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
1.8.1.1

[PATCH 3/3] tegra: ventana: of: add host1x device to DT

[Alexandre Courbot]

Add the host1x device and DDC i2c bus to enable internal panel on
Ventana.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/boot/dts/tegra20-ventana.dts | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index a77b529..4477e9c 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -10,6 +10,15 @@
 		reg = <0x00000000 0x40000000>;
 	};
 
+	host1x {
+		dc@54200000 {
+			rgb {
+				status = "okay";
+				nvidia,ddc-i2c-bus = <&lcd_ddc>;
+			};
+		};
+	};
+
 	pinmux {
 		pinctrl-names = "default";
 		pinctrl-0 = <&state_default>;
@@ -341,7 +350,7 @@
 			#size-cells = <0>;
 		};
 
-		i2c@1 {
+		lcd_ddc: i2c@1 {
 			reg = <1>;
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
1.8.1.1

[PATCH 3/3] tegra: ventana: of: add host1x device to DT

[Alexandre Courbot]

Add the host1x device and DDC i2c bus to enable internal panel on
Ventana.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/boot/dts/tegra20-ventana.dts | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index a77b529..4477e9c 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -10,6 +10,15 @@
 		reg = <0x00000000 0x40000000>;
 	};
 
+	host1x {
+		dc@54200000 {
+			rgb {
+				status = "okay";
+				nvidia,ddc-i2c-bus = <&lcd_ddc>;
+			};
+		};
+	};
+
 	pinmux {
 		pinctrl-names = "default";
 		pinctrl-0 = <&state_default>;
@@ -341,7 +350,7 @@
 			#size-cells = <0>;
 		};
 
-		i2c@1 {
+		lcd_ddc: i2c@1 {
 			reg = <1>;
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
1.8.1.1

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Mark Zhang]

Yeah, thanks Alex. :)

So this is a non power sequence version of backlight & panel enabling,
isn't it? I remember we talked about this several days ago and you
mentioned kernel guys want an ad-hoc version(power sequence logics
inside driver, not in DT) and I believe this is it, right?

I think finally I can enable Tegra30 cardhu's display after this patch
merged.

Mark
On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
> This series introduces a way to use pwm-backlight hooks with platforms
> that use the device tree through a subdriver system. It also adds support
> for the Tegra-based Ventana board, adding the last missing block to enable
> its panel. Support for other Tegra board can thus be easily added.
> 
> I have something else in mind to properly support this (power
> sequences), but this work relies on the GPIO subsystem redesign which will
> take some time. The pwm-backlight subdrivers can do the job by the meantime.
> 
> There are a few design points that might need to be discussed:
> 1) Link order is important: subdrivers register themselves in their
> module_init function, which must be called before pwm-backlight's probe.
> This forbids linking subdrivers as separate modules from pwm-backlight.
> 2) The subdriver's data is temporarily passed through the backlight
> device's driver data. This should not hurt, but maybe there is a better way
> to do this.
> 3) Subdrivers must add themselves into pwm-backlight's own of_device_id
> table. It would be cleaner to not have to list subdrivers into
> pwm-backlight's main file, but I cannot think of a way to do otherwise.
> 
> Suggestions for the 3 points listed above are very welcome - in any case,
> I hope to make this converge into something mergeable quickly.
> 
> Note that these patches are the last missing block to get a functional
> panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
> on Ventana is usable out-of-the-box. Yay.
> 
> Alexandre Courbot (3):
>   pwm-backlight: add subdriver mechanism
>   tegra: pwm-backlight: add tegra pwm-bl driver
>   tegra: ventana: of: add host1x device to DT
> 
>  arch/arm/boot/dts/tegra20-ventana.dts  |  29 +++++-
>  arch/arm/configs/tegra_defconfig       |   1 +
>  drivers/video/backlight/Kconfig        |   7 ++
>  drivers/video/backlight/Makefile       |   4 +
>  drivers/video/backlight/pwm_bl.c       |  70 ++++++++++++++-
>  drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
>  include/linux/pwm_backlight.h          |  15 ++++
>  7 files changed, 281 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
> 

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Mark Zhang]

Yeah, thanks Alex. :)

So this is a non power sequence version of backlight & panel enabling,
isn't it? I remember we talked about this several days ago and you
mentioned kernel guys want an ad-hoc version(power sequence logics
inside driver, not in DT) and I believe this is it, right?

I think finally I can enable Tegra30 cardhu's display after this patch
merged.

Mark
On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
> This series introduces a way to use pwm-backlight hooks with platforms
> that use the device tree through a subdriver system. It also adds support
> for the Tegra-based Ventana board, adding the last missing block to enable
> its panel. Support for other Tegra board can thus be easily added.
> 
> I have something else in mind to properly support this (power
> sequences), but this work relies on the GPIO subsystem redesign which will
> take some time. The pwm-backlight subdrivers can do the job by the meantime.
> 
> There are a few design points that might need to be discussed:
> 1) Link order is important: subdrivers register themselves in their
> module_init function, which must be called before pwm-backlight's probe.
> This forbids linking subdrivers as separate modules from pwm-backlight.
> 2) The subdriver's data is temporarily passed through the backlight
> device's driver data. This should not hurt, but maybe there is a better way
> to do this.
> 3) Subdrivers must add themselves into pwm-backlight's own of_device_id
> table. It would be cleaner to not have to list subdrivers into
> pwm-backlight's main file, but I cannot think of a way to do otherwise.
> 
> Suggestions for the 3 points listed above are very welcome - in any case,
> I hope to make this converge into something mergeable quickly.
> 
> Note that these patches are the last missing block to get a functional
> panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
> on Ventana is usable out-of-the-box. Yay.
> 
> Alexandre Courbot (3):
>   pwm-backlight: add subdriver mechanism
>   tegra: pwm-backlight: add tegra pwm-bl driver
>   tegra: ventana: of: add host1x device to DT
> 
>  arch/arm/boot/dts/tegra20-ventana.dts  |  29 +++++-
>  arch/arm/configs/tegra_defconfig       |   1 +
>  drivers/video/backlight/Kconfig        |   7 ++
>  drivers/video/backlight/Makefile       |   4 +
>  drivers/video/backlight/pwm_bl.c       |  70 ++++++++++++++-
>  drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
>  include/linux/pwm_backlight.h          |  15 ++++
>  7 files changed, 281 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
> 

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Mark Zhang]

Yeah, thanks Alex. :)

So this is a non power sequence version of backlight & panel enabling,
isn't it? I remember we talked about this several days ago and you
mentioned kernel guys want an ad-hoc version(power sequence logics
inside driver, not in DT) and I believe this is it, right?

I think finally I can enable Tegra30 cardhu's display after this patch
merged.

Mark
On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
> This series introduces a way to use pwm-backlight hooks with platforms
> that use the device tree through a subdriver system. It also adds support
> for the Tegra-based Ventana board, adding the last missing block to enable
> its panel. Support for other Tegra board can thus be easily added.
> 
> I have something else in mind to properly support this (power
> sequences), but this work relies on the GPIO subsystem redesign which will
> take some time. The pwm-backlight subdrivers can do the job by the meantime.
> 
> There are a few design points that might need to be discussed:
> 1) Link order is important: subdrivers register themselves in their
> module_init function, which must be called before pwm-backlight's probe.
> This forbids linking subdrivers as separate modules from pwm-backlight.
> 2) The subdriver's data is temporarily passed through the backlight
> device's driver data. This should not hurt, but maybe there is a better way
> to do this.
> 3) Subdrivers must add themselves into pwm-backlight's own of_device_id
> table. It would be cleaner to not have to list subdrivers into
> pwm-backlight's main file, but I cannot think of a way to do otherwise.
> 
> Suggestions for the 3 points listed above are very welcome - in any case,
> I hope to make this converge into something mergeable quickly.
> 
> Note that these patches are the last missing block to get a functional
> panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
> on Ventana is usable out-of-the-box. Yay.
> 
> Alexandre Courbot (3):
>   pwm-backlight: add subdriver mechanism
>   tegra: pwm-backlight: add tegra pwm-bl driver
>   tegra: ventana: of: add host1x device to DT
> 
>  arch/arm/boot/dts/tegra20-ventana.dts  |  29 +++++-
>  arch/arm/configs/tegra_defconfig       |   1 +
>  drivers/video/backlight/Kconfig        |   7 ++
>  drivers/video/backlight/Makefile       |   4 +
>  drivers/video/backlight/pwm_bl.c       |  70 ++++++++++++++-
>  drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
>  include/linux/pwm_backlight.h          |  15 ++++
>  7 files changed, 281 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
> 

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Alexandre Courbot]

On Sun, Jan 20, 2013 at 12:38 PM, Mark Zhang <markz@nvidia.com> wrote:
> So this is a non power sequence version of backlight & panel enabling,
> isn't it? I remember we talked about this several days ago and you
> mentioned kernel guys want an ad-hoc version(power sequence logics
> inside driver, not in DT) and I believe this is it, right?

Basically, yes - I still think power-seqs could be useful here
(especially after seeing the size of these sub-drivers if you want to
do error checking properly) and plan to give it another shot without
DT, but this will not happen soon since we need to do some GPIO
redesign before. You can see what's wrong in the init() function of
the subdriver: we call a device tree function to obtain the GPIO as
there is no get function.

> I think finally I can enable Tegra30 cardhu's display after this patch
> merged.

Yes, feel free to write a subdriver for Cardhu if you like - I'd like
to see all T20 and T30 boards supported by the time this gets merged.

Thanks,
Alex.

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Alexandre Courbot]

On Sun, Jan 20, 2013 at 12:38 PM, Mark Zhang <markz@nvidia.com> wrote:
> So this is a non power sequence version of backlight & panel enabling,
> isn't it? I remember we talked about this several days ago and you
> mentioned kernel guys want an ad-hoc version(power sequence logics
> inside driver, not in DT) and I believe this is it, right?

Basically, yes - I still think power-seqs could be useful here
(especially after seeing the size of these sub-drivers if you want to
do error checking properly) and plan to give it another shot without
DT, but this will not happen soon since we need to do some GPIO
redesign before. You can see what's wrong in the init() function of
the subdriver: we call a device tree function to obtain the GPIO as
there is no get function.

> I think finally I can enable Tegra30 cardhu's display after this patch
> merged.

Yes, feel free to write a subdriver for Cardhu if you like - I'd like
to see all T20 and T30 boards supported by the time this gets merged.

Thanks,
Alex.

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Mark Zhang]

On 01/20/2013 01:26 PM, Alexandre Courbot wrote:
> On Sun, Jan 20, 2013 at 12:38 PM, Mark Zhang <markz@nvidia.com> wrote:
>> So this is a non power sequence version of backlight & panel enabling,
>> isn't it? I remember we talked about this several days ago and you
>> mentioned kernel guys want an ad-hoc version(power sequence logics
>> inside driver, not in DT) and I believe this is it, right?
> 
> Basically, yes - I still think power-seqs could be useful here
> (especially after seeing the size of these sub-drivers if you want to
> do error checking properly) and plan to give it another shot without
> DT, but this will not happen soon since we need to do some GPIO
> redesign before. You can see what's wrong in the init() function of
> the subdriver: we call a device tree function to obtain the GPIO as
> there is no get function.
>

Okay. I think I got the picture. I'll read the codes when I'm free and I
think I'll understand this better after that.

>> I think finally I can enable Tegra30 cardhu's display after this patch
>> merged.
> 
> Yes, feel free to write a subdriver for Cardhu if you like - I'd like
> to see all T20 and T30 boards supported by the time this gets merged.
> 

Yep, I can try to do that. I'll let you know if I have problems.

Mark
> Thanks,
> Alex.
> 

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Mark Zhang]

On 01/20/2013 01:26 PM, Alexandre Courbot wrote:
> On Sun, Jan 20, 2013 at 12:38 PM, Mark Zhang <markz@nvidia.com> wrote:
>> So this is a non power sequence version of backlight & panel enabling,
>> isn't it? I remember we talked about this several days ago and you
>> mentioned kernel guys want an ad-hoc version(power sequence logics
>> inside driver, not in DT) and I believe this is it, right?
> 
> Basically, yes - I still think power-seqs could be useful here
> (especially after seeing the size of these sub-drivers if you want to
> do error checking properly) and plan to give it another shot without
> DT, but this will not happen soon since we need to do some GPIO
> redesign before. You can see what's wrong in the init() function of
> the subdriver: we call a device tree function to obtain the GPIO as
> there is no get function.
>

Okay. I think I got the picture. I'll read the codes when I'm free and I
think I'll understand this better after that.

>> I think finally I can enable Tegra30 cardhu's display after this patch
>> merged.
> 
> Yes, feel free to write a subdriver for Cardhu if you like - I'd like
> to see all T20 and T30 boards supported by the time this gets merged.
> 

Yep, I can try to do that. I'll let you know if I have problems.

Mark
> Thanks,
> Alex.
> 

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Mark Zhang]

Hi Alex,

This patch set applies failed on tot linux-next(0118). Here is the log:

markz@markz-hp6200:~/tegradrm/official-upstream-kernel$ git am
~/Desktop/*.eml
Applying: pwm-backlight: add subdriver mechanism
error: patch failed: drivers/video/backlight/pwm_bl.c:35
error: drivers/video/backlight/pwm_bl.c: patch does not apply
Patch failed at 0001 pwm-backlight: add subdriver mechanism
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

Anyway, I'll try to apply this on 3.8-rc4.

Mark
On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
> This series introduces a way to use pwm-backlight hooks with platforms
> that use the device tree through a subdriver system. It also adds support
> for the Tegra-based Ventana board, adding the last missing block to enable
> its panel. Support for other Tegra board can thus be easily added.
> 
> I have something else in mind to properly support this (power
> sequences), but this work relies on the GPIO subsystem redesign which will
> take some time. The pwm-backlight subdrivers can do the job by the meantime.
> 
> There are a few design points that might need to be discussed:
> 1) Link order is important: subdrivers register themselves in their
> module_init function, which must be called before pwm-backlight's probe.
> This forbids linking subdrivers as separate modules from pwm-backlight.
> 2) The subdriver's data is temporarily passed through the backlight
> device's driver data. This should not hurt, but maybe there is a better way
> to do this.
> 3) Subdrivers must add themselves into pwm-backlight's own of_device_id
> table. It would be cleaner to not have to list subdrivers into
> pwm-backlight's main file, but I cannot think of a way to do otherwise.
> 
> Suggestions for the 3 points listed above are very welcome - in any case,
> I hope to make this converge into something mergeable quickly.
> 
> Note that these patches are the last missing block to get a functional
> panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
> on Ventana is usable out-of-the-box. Yay.
> 
> Alexandre Courbot (3):
>   pwm-backlight: add subdriver mechanism
>   tegra: pwm-backlight: add tegra pwm-bl driver
>   tegra: ventana: of: add host1x device to DT
> 
>  arch/arm/boot/dts/tegra20-ventana.dts  |  29 +++++-
>  arch/arm/configs/tegra_defconfig       |   1 +
>  drivers/video/backlight/Kconfig        |   7 ++
>  drivers/video/backlight/Makefile       |   4 +
>  drivers/video/backlight/pwm_bl.c       |  70 ++++++++++++++-
>  drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
>  include/linux/pwm_backlight.h          |  15 ++++
>  7 files changed, 281 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
> 

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Mark Zhang]

Hi Alex,

This patch set applies failed on tot linux-next(0118). Here is the log:

markz@markz-hp6200:~/tegradrm/official-upstream-kernel$ git am
~/Desktop/*.eml
Applying: pwm-backlight: add subdriver mechanism
error: patch failed: drivers/video/backlight/pwm_bl.c:35
error: drivers/video/backlight/pwm_bl.c: patch does not apply
Patch failed at 0001 pwm-backlight: add subdriver mechanism
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

Anyway, I'll try to apply this on 3.8-rc4.

Mark
On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
> This series introduces a way to use pwm-backlight hooks with platforms
> that use the device tree through a subdriver system. It also adds support
> for the Tegra-based Ventana board, adding the last missing block to enable
> its panel. Support for other Tegra board can thus be easily added.
> 
> I have something else in mind to properly support this (power
> sequences), but this work relies on the GPIO subsystem redesign which will
> take some time. The pwm-backlight subdrivers can do the job by the meantime.
> 
> There are a few design points that might need to be discussed:
> 1) Link order is important: subdrivers register themselves in their
> module_init function, which must be called before pwm-backlight's probe.
> This forbids linking subdrivers as separate modules from pwm-backlight.
> 2) The subdriver's data is temporarily passed through the backlight
> device's driver data. This should not hurt, but maybe there is a better way
> to do this.
> 3) Subdrivers must add themselves into pwm-backlight's own of_device_id
> table. It would be cleaner to not have to list subdrivers into
> pwm-backlight's main file, but I cannot think of a way to do otherwise.
> 
> Suggestions for the 3 points listed above are very welcome - in any case,
> I hope to make this converge into something mergeable quickly.
> 
> Note that these patches are the last missing block to get a functional
> panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
> on Ventana is usable out-of-the-box. Yay.
> 
> Alexandre Courbot (3):
>   pwm-backlight: add subdriver mechanism
>   tegra: pwm-backlight: add tegra pwm-bl driver
>   tegra: ventana: of: add host1x device to DT
> 
>  arch/arm/boot/dts/tegra20-ventana.dts  |  29 +++++-
>  arch/arm/configs/tegra_defconfig       |   1 +
>  drivers/video/backlight/Kconfig        |   7 ++
>  drivers/video/backlight/Makefile       |   4 +
>  drivers/video/backlight/pwm_bl.c       |  70 ++++++++++++++-
>  drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
>  include/linux/pwm_backlight.h          |  15 ++++
>  7 files changed, 281 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
> 

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Mark Zhang]

Hi Alex,

This patch set applies failed on tot linux-next(0118). Here is the log:

markz@markz-hp6200:~/tegradrm/official-upstream-kernel$ git am
~/Desktop/*.eml
Applying: pwm-backlight: add subdriver mechanism
error: patch failed: drivers/video/backlight/pwm_bl.c:35
error: drivers/video/backlight/pwm_bl.c: patch does not apply
Patch failed at 0001 pwm-backlight: add subdriver mechanism
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

Anyway, I'll try to apply this on 3.8-rc4.

Mark
On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
> This series introduces a way to use pwm-backlight hooks with platforms
> that use the device tree through a subdriver system. It also adds support
> for the Tegra-based Ventana board, adding the last missing block to enable
> its panel. Support for other Tegra board can thus be easily added.
> 
> I have something else in mind to properly support this (power
> sequences), but this work relies on the GPIO subsystem redesign which will
> take some time. The pwm-backlight subdrivers can do the job by the meantime.
> 
> There are a few design points that might need to be discussed:
> 1) Link order is important: subdrivers register themselves in their
> module_init function, which must be called before pwm-backlight's probe.
> This forbids linking subdrivers as separate modules from pwm-backlight.
> 2) The subdriver's data is temporarily passed through the backlight
> device's driver data. This should not hurt, but maybe there is a better way
> to do this.
> 3) Subdrivers must add themselves into pwm-backlight's own of_device_id
> table. It would be cleaner to not have to list subdrivers into
> pwm-backlight's main file, but I cannot think of a way to do otherwise.
> 
> Suggestions for the 3 points listed above are very welcome - in any case,
> I hope to make this converge into something mergeable quickly.
> 
> Note that these patches are the last missing block to get a functional
> panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
> on Ventana is usable out-of-the-box. Yay.
> 
> Alexandre Courbot (3):
>   pwm-backlight: add subdriver mechanism
>   tegra: pwm-backlight: add tegra pwm-bl driver
>   tegra: ventana: of: add host1x device to DT
> 
>  arch/arm/boot/dts/tegra20-ventana.dts  |  29 +++++-
>  arch/arm/configs/tegra_defconfig       |   1 +
>  drivers/video/backlight/Kconfig        |   7 ++
>  drivers/video/backlight/Makefile       |   4 +
>  drivers/video/backlight/pwm_bl.c       |  70 ++++++++++++++-
>  drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
>  include/linux/pwm_backlight.h          |  15 ++++
>  7 files changed, 281 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
> 

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Mark Zhang]

Patch is applied OK on 3.8-rc4.

Hmmm.. But I think it's better to make the patch can be applied on
linux-next.

Mark
On 01/21/2013 10:09 AM, Mark Zhang wrote:
> Hi Alex,
> 
> This patch set applies failed on tot linux-next(0118). Here is the log:
> 
> markz@markz-hp6200:~/tegradrm/official-upstream-kernel$ git am
> ~/Desktop/*.eml
> Applying: pwm-backlight: add subdriver mechanism
> error: patch failed: drivers/video/backlight/pwm_bl.c:35
> error: drivers/video/backlight/pwm_bl.c: patch does not apply
> Patch failed at 0001 pwm-backlight: add subdriver mechanism
> When you have resolved this problem run "git am --resolved".
> If you would prefer to skip this patch, instead run "git am --skip".
> To restore the original branch and stop patching run "git am --abort".
> 
> Anyway, I'll try to apply this on 3.8-rc4.
> 
> Mark
> On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
>> This series introduces a way to use pwm-backlight hooks with platforms
>> that use the device tree through a subdriver system. It also adds support
>> for the Tegra-based Ventana board, adding the last missing block to enable
>> its panel. Support for other Tegra board can thus be easily added.
>>
>> I have something else in mind to properly support this (power
>> sequences), but this work relies on the GPIO subsystem redesign which will
>> take some time. The pwm-backlight subdrivers can do the job by the meantime.
>>
>> There are a few design points that might need to be discussed:
>> 1) Link order is important: subdrivers register themselves in their
>> module_init function, which must be called before pwm-backlight's probe.
>> This forbids linking subdrivers as separate modules from pwm-backlight.
>> 2) The subdriver's data is temporarily passed through the backlight
>> device's driver data. This should not hurt, but maybe there is a better way
>> to do this.
>> 3) Subdrivers must add themselves into pwm-backlight's own of_device_id
>> table. It would be cleaner to not have to list subdrivers into
>> pwm-backlight's main file, but I cannot think of a way to do otherwise.
>>
>> Suggestions for the 3 points listed above are very welcome - in any case,
>> I hope to make this converge into something mergeable quickly.
>>
>> Note that these patches are the last missing block to get a functional
>> panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
>> on Ventana is usable out-of-the-box. Yay.
>>
>> Alexandre Courbot (3):
>>   pwm-backlight: add subdriver mechanism
>>   tegra: pwm-backlight: add tegra pwm-bl driver
>>   tegra: ventana: of: add host1x device to DT
>>
>>  arch/arm/boot/dts/tegra20-ventana.dts  |  29 +++++-
>>  arch/arm/configs/tegra_defconfig       |   1 +
>>  drivers/video/backlight/Kconfig        |   7 ++
>>  drivers/video/backlight/Makefile       |   4 +
>>  drivers/video/backlight/pwm_bl.c       |  70 ++++++++++++++-
>>  drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
>>  include/linux/pwm_backlight.h          |  15 ++++
>>  7 files changed, 281 insertions(+), 4 deletions(-)
>>  create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
>>

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Mark Zhang]

Patch is applied OK on 3.8-rc4.

Hmmm.. But I think it's better to make the patch can be applied on
linux-next.

Mark
On 01/21/2013 10:09 AM, Mark Zhang wrote:
> Hi Alex,
> 
> This patch set applies failed on tot linux-next(0118). Here is the log:
> 
> markz@markz-hp6200:~/tegradrm/official-upstream-kernel$ git am
> ~/Desktop/*.eml
> Applying: pwm-backlight: add subdriver mechanism
> error: patch failed: drivers/video/backlight/pwm_bl.c:35
> error: drivers/video/backlight/pwm_bl.c: patch does not apply
> Patch failed at 0001 pwm-backlight: add subdriver mechanism
> When you have resolved this problem run "git am --resolved".
> If you would prefer to skip this patch, instead run "git am --skip".
> To restore the original branch and stop patching run "git am --abort".
> 
> Anyway, I'll try to apply this on 3.8-rc4.
> 
> Mark
> On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
>> This series introduces a way to use pwm-backlight hooks with platforms
>> that use the device tree through a subdriver system. It also adds support
>> for the Tegra-based Ventana board, adding the last missing block to enable
>> its panel. Support for other Tegra board can thus be easily added.
>>
>> I have something else in mind to properly support this (power
>> sequences), but this work relies on the GPIO subsystem redesign which will
>> take some time. The pwm-backlight subdrivers can do the job by the meantime.
>>
>> There are a few design points that might need to be discussed:
>> 1) Link order is important: subdrivers register themselves in their
>> module_init function, which must be called before pwm-backlight's probe.
>> This forbids linking subdrivers as separate modules from pwm-backlight.
>> 2) The subdriver's data is temporarily passed through the backlight
>> device's driver data. This should not hurt, but maybe there is a better way
>> to do this.
>> 3) Subdrivers must add themselves into pwm-backlight's own of_device_id
>> table. It would be cleaner to not have to list subdrivers into
>> pwm-backlight's main file, but I cannot think of a way to do otherwise.
>>
>> Suggestions for the 3 points listed above are very welcome - in any case,
>> I hope to make this converge into something mergeable quickly.
>>
>> Note that these patches are the last missing block to get a functional
>> panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
>> on Ventana is usable out-of-the-box. Yay.
>>
>> Alexandre Courbot (3):
>>   pwm-backlight: add subdriver mechanism
>>   tegra: pwm-backlight: add tegra pwm-bl driver
>>   tegra: ventana: of: add host1x device to DT
>>
>>  arch/arm/boot/dts/tegra20-ventana.dts  |  29 +++++-
>>  arch/arm/configs/tegra_defconfig       |   1 +
>>  drivers/video/backlight/Kconfig        |   7 ++
>>  drivers/video/backlight/Makefile       |   4 +
>>  drivers/video/backlight/pwm_bl.c       |  70 ++++++++++++++-
>>  drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
>>  include/linux/pwm_backlight.h          |  15 ++++
>>  7 files changed, 281 insertions(+), 4 deletions(-)
>>  create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
>>

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Mark Zhang]

Patch is applied OK on 3.8-rc4.

Hmmm.. But I think it's better to make the patch can be applied on
linux-next.

Mark
On 01/21/2013 10:09 AM, Mark Zhang wrote:
> Hi Alex,
> 
> This patch set applies failed on tot linux-next(0118). Here is the log:
> 
> markz@markz-hp6200:~/tegradrm/official-upstream-kernel$ git am
> ~/Desktop/*.eml
> Applying: pwm-backlight: add subdriver mechanism
> error: patch failed: drivers/video/backlight/pwm_bl.c:35
> error: drivers/video/backlight/pwm_bl.c: patch does not apply
> Patch failed at 0001 pwm-backlight: add subdriver mechanism
> When you have resolved this problem run "git am --resolved".
> If you would prefer to skip this patch, instead run "git am --skip".
> To restore the original branch and stop patching run "git am --abort".
> 
> Anyway, I'll try to apply this on 3.8-rc4.
> 
> Mark
> On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
>> This series introduces a way to use pwm-backlight hooks with platforms
>> that use the device tree through a subdriver system. It also adds support
>> for the Tegra-based Ventana board, adding the last missing block to enable
>> its panel. Support for other Tegra board can thus be easily added.
>>
>> I have something else in mind to properly support this (power
>> sequences), but this work relies on the GPIO subsystem redesign which will
>> take some time. The pwm-backlight subdrivers can do the job by the meantime.
>>
>> There are a few design points that might need to be discussed:
>> 1) Link order is important: subdrivers register themselves in their
>> module_init function, which must be called before pwm-backlight's probe.
>> This forbids linking subdrivers as separate modules from pwm-backlight.
>> 2) The subdriver's data is temporarily passed through the backlight
>> device's driver data. This should not hurt, but maybe there is a better way
>> to do this.
>> 3) Subdrivers must add themselves into pwm-backlight's own of_device_id
>> table. It would be cleaner to not have to list subdrivers into
>> pwm-backlight's main file, but I cannot think of a way to do otherwise.
>>
>> Suggestions for the 3 points listed above are very welcome - in any case,
>> I hope to make this converge into something mergeable quickly.
>>
>> Note that these patches are the last missing block to get a functional
>> panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
>> on Ventana is usable out-of-the-box. Yay.
>>
>> Alexandre Courbot (3):
>>   pwm-backlight: add subdriver mechanism
>>   tegra: pwm-backlight: add tegra pwm-bl driver
>>   tegra: ventana: of: add host1x device to DT
>>
>>  arch/arm/boot/dts/tegra20-ventana.dts  |  29 +++++-
>>  arch/arm/configs/tegra_defconfig       |   1 +
>>  drivers/video/backlight/Kconfig        |   7 ++
>>  drivers/video/backlight/Makefile       |   4 +
>>  drivers/video/backlight/pwm_bl.c       |  70 ++++++++++++++-
>>  drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
>>  include/linux/pwm_backlight.h          |  15 ++++
>>  7 files changed, 281 insertions(+), 4 deletions(-)
>>  create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
>>

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Mark Zhang]

On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
> Add a PWM-backlight subdriver for Tegra boards, with support for
> Ventana.
> 
> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
[...]
>  
> +	backlight {
> +		compatible = "pwm-backlight-ventana";
> +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
> +		default-brightness-level = <12>;
> +
> +		pwms = <&pwm 2 5000000>;

After read the codes of tegra pwm driver & pwm framework, I got to know
the meaning of this property. So I think we need to add a doc(e.g:
Documentation/devicetree/bindings/video/backlight/nvidia,tegra20-bl.txt)
to explain this, "Documentation/devicetree/bindings/pwm/pwm.txt" doesn't
explain this, because this may be different between different pwm drivers.

> +		pwm-names = "backlight";
> +
> +		power-supply = <&vdd_bl_reg>;
> +		panel-supply = <&vdd_pnl_reg>;
> +		bl-gpio = <&gpio 28 0>;
> +		bl-panel = <&gpio 10 0>;
> +	};
> +
[...]
> diff --git a/drivers/video/backlight/pwm_bl_tegra.c b/drivers/video/backlight/pwm_bl_tegra.c
> new file mode 100644
> index 0000000..8f2195b
> --- /dev/null
> +++ b/drivers/video/backlight/pwm_bl_tegra.c

So according to the filename, I think we can put all tegra boards codes
here, right? Just like what you do for Ventana, if I wanna add support
for cardhu, I can define similar functions -- let's say "init_cardhu",
"exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?

But I think if we do in this way, the file will become very long soon.
And there are a lot of redundant codes in it. So do you have any
suggestions?

Mark
> @@ -0,0 +1,159 @@
> +/*
> + * pwm-backlight subdriver for Tegra.
> + *
> + * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
[...]
> +MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pwm-tegra-backlight");
> +
> +
> 

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Mark Zhang]

On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
> Add a PWM-backlight subdriver for Tegra boards, with support for
> Ventana.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
[...]
>  
> +	backlight {
> +		compatible = "pwm-backlight-ventana";
> +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
> +		default-brightness-level = <12>;
> +
> +		pwms = <&pwm 2 5000000>;

After read the codes of tegra pwm driver & pwm framework, I got to know
the meaning of this property. So I think we need to add a doc(e.g:
Documentation/devicetree/bindings/video/backlight/nvidia,tegra20-bl.txt)
to explain this, "Documentation/devicetree/bindings/pwm/pwm.txt" doesn't
explain this, because this may be different between different pwm drivers.

> +		pwm-names = "backlight";
> +
> +		power-supply = <&vdd_bl_reg>;
> +		panel-supply = <&vdd_pnl_reg>;
> +		bl-gpio = <&gpio 28 0>;
> +		bl-panel = <&gpio 10 0>;
> +	};
> +
[...]
> diff --git a/drivers/video/backlight/pwm_bl_tegra.c b/drivers/video/backlight/pwm_bl_tegra.c
> new file mode 100644
> index 0000000..8f2195b
> --- /dev/null
> +++ b/drivers/video/backlight/pwm_bl_tegra.c

So according to the filename, I think we can put all tegra boards codes
here, right? Just like what you do for Ventana, if I wanna add support
for cardhu, I can define similar functions -- let's say "init_cardhu",
"exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?

But I think if we do in this way, the file will become very long soon.
And there are a lot of redundant codes in it. So do you have any
suggestions?

Mark
> @@ -0,0 +1,159 @@
> +/*
> + * pwm-backlight subdriver for Tegra.
> + *
> + * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
[...]
> +MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pwm-tegra-backlight");
> +
> +
> 

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Mark Zhang]

On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
> Add a PWM-backlight subdriver for Tegra boards, with support for
> Ventana.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
[...]
>  
> +	backlight {
> +		compatible = "pwm-backlight-ventana";
> +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
> +		default-brightness-level = <12>;
> +
> +		pwms = <&pwm 2 5000000>;

After read the codes of tegra pwm driver & pwm framework, I got to know
the meaning of this property. So I think we need to add a doc(e.g:
Documentation/devicetree/bindings/video/backlight/nvidia,tegra20-bl.txt)
to explain this, "Documentation/devicetree/bindings/pwm/pwm.txt" doesn't
explain this, because this may be different between different pwm drivers.

> +		pwm-names = "backlight";
> +
> +		power-supply = <&vdd_bl_reg>;
> +		panel-supply = <&vdd_pnl_reg>;
> +		bl-gpio = <&gpio 28 0>;
> +		bl-panel = <&gpio 10 0>;
> +	};
> +
[...]
> diff --git a/drivers/video/backlight/pwm_bl_tegra.c b/drivers/video/backlight/pwm_bl_tegra.c
> new file mode 100644
> index 0000000..8f2195b
> --- /dev/null
> +++ b/drivers/video/backlight/pwm_bl_tegra.c

So according to the filename, I think we can put all tegra boards codes
here, right? Just like what you do for Ventana, if I wanna add support
for cardhu, I can define similar functions -- let's say "init_cardhu",
"exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?

But I think if we do in this way, the file will become very long soon.
And there are a lot of redundant codes in it. So do you have any
suggestions?

Mark
> @@ -0,0 +1,159 @@
> +/*
> + * pwm-backlight subdriver for Tegra.
> + *
> + * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
[...]
> +MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pwm-tegra-backlight");
> +
> +
> 

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Thierry Reding]

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

On Sat, Jan 19, 2013 at 07:30:17PM +0900, Alexandre Courbot wrote:
> This series introduces a way to use pwm-backlight hooks with platforms
> that use the device tree through a subdriver system. It also adds support
> for the Tegra-based Ventana board, adding the last missing block to enable
> its panel. Support for other Tegra board can thus be easily added.
> 
> I have something else in mind to properly support this (power
> sequences), but this work relies on the GPIO subsystem redesign which will
> take some time. The pwm-backlight subdrivers can do the job by the meantime.
> 
> There are a few design points that might need to be discussed:
> 1) Link order is important: subdrivers register themselves in their
> module_init function, which must be called before pwm-backlight's probe.
> This forbids linking subdrivers as separate modules from pwm-backlight.
> 2) The subdriver's data is temporarily passed through the backlight
> device's driver data. This should not hurt, but maybe there is a better way
> to do this.
> 3) Subdrivers must add themselves into pwm-backlight's own of_device_id
> table. It would be cleaner to not have to list subdrivers into
> pwm-backlight's main file, but I cannot think of a way to do otherwise.
> 
> Suggestions for the 3 points listed above are very welcome - in any case,
> I hope to make this converge into something mergeable quickly.
> 
> Note that these patches are the last missing block to get a functional
> panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
> on Ventana is usable out-of-the-box. Yay.

Hi Alexandre,

It's great to see you pick this up. I've been meaning to do this myself
but I just can't find the time right now. Generally I think the approach
you've chosen looks good, but I don't think doing it in pwm-backlight is
the right way.

Eventually this should all be covered by the CDF, but since that's not
ready yet we want something ad-hoc to get the hardware supported. As
such I would like to see this go into some sort of minimalistic, Tegra-
specific display/panel framework. I'd prefer to keep the pwm-backlight
driver as simple and generic as possible, that is, a driver for a PWM-
controlled backlight.

Another advantage of moving this into a sort of display framework is
that it may help in defining the requirements for a CDF and that moving
the code to the CDF should be easier once it is done.

Last but not least, abstracting away the panel allows other things such
as physical dimensions and display modes to be properly encapsulated. I
think that power-on/off timing requirements for panels also belong to
this set since they are usually specific to a given panel.

Maybe adding these drivers to tegra-drm for now would be a good option.
That way the corresponding glue can be added without a need for inter-
tree dependencies.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Thierry Reding]

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

On Sat, Jan 19, 2013 at 07:30:17PM +0900, Alexandre Courbot wrote:
> This series introduces a way to use pwm-backlight hooks with platforms
> that use the device tree through a subdriver system. It also adds support
> for the Tegra-based Ventana board, adding the last missing block to enable
> its panel. Support for other Tegra board can thus be easily added.
> 
> I have something else in mind to properly support this (power
> sequences), but this work relies on the GPIO subsystem redesign which will
> take some time. The pwm-backlight subdrivers can do the job by the meantime.
> 
> There are a few design points that might need to be discussed:
> 1) Link order is important: subdrivers register themselves in their
> module_init function, which must be called before pwm-backlight's probe.
> This forbids linking subdrivers as separate modules from pwm-backlight.
> 2) The subdriver's data is temporarily passed through the backlight
> device's driver data. This should not hurt, but maybe there is a better way
> to do this.
> 3) Subdrivers must add themselves into pwm-backlight's own of_device_id
> table. It would be cleaner to not have to list subdrivers into
> pwm-backlight's main file, but I cannot think of a way to do otherwise.
> 
> Suggestions for the 3 points listed above are very welcome - in any case,
> I hope to make this converge into something mergeable quickly.
> 
> Note that these patches are the last missing block to get a functional
> panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
> on Ventana is usable out-of-the-box. Yay.

Hi Alexandre,

It's great to see you pick this up. I've been meaning to do this myself
but I just can't find the time right now. Generally I think the approach
you've chosen looks good, but I don't think doing it in pwm-backlight is
the right way.

Eventually this should all be covered by the CDF, but since that's not
ready yet we want something ad-hoc to get the hardware supported. As
such I would like to see this go into some sort of minimalistic, Tegra-
specific display/panel framework. I'd prefer to keep the pwm-backlight
driver as simple and generic as possible, that is, a driver for a PWM-
controlled backlight.

Another advantage of moving this into a sort of display framework is
that it may help in defining the requirements for a CDF and that moving
the code to the CDF should be easier once it is done.

Last but not least, abstracting away the panel allows other things such
as physical dimensions and display modes to be properly encapsulated. I
think that power-on/off timing requirements for panels also belong to
this set since they are usually specific to a given panel.

Maybe adding these drivers to tegra-drm for now would be a good option.
That way the corresponding glue can be added without a need for inter-
tree dependencies.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Alex Courbot]

Hi Thierry,

On Monday 21 January 2013 15:49:28 Thierry Reding wrote:
> Eventually this should all be covered by the CDF, but since that's not
> ready yet we want something ad-hoc to get the hardware supported. As
> such I would like to see this go into some sort of minimalistic, Tegra-
> specific display/panel framework. I'd prefer to keep the pwm-backlight
> driver as simple and generic as possible, that is, a driver for a PWM-
> controlled backlight.
> 
> Another advantage of moving this into a sort of display framework is
> that it may help in defining the requirements for a CDF and that moving
> the code to the CDF should be easier once it is done.
> 
> Last but not least, abstracting away the panel allows other things such
> as physical dimensions and display modes to be properly encapsulated. I
> think that power-on/off timing requirements for panels also belong to
> this set since they are usually specific to a given panel.
> 
> Maybe adding these drivers to tegra-drm for now would be a good option.
> That way the corresponding glue can be added without a need for inter-
> tree dependencies.

IIRC (because that was a while ago already) having a Tegra-only display 
framework is exactly what we wanted to avoid in the first place. This series 
does nothing but leverage the callbacks mechanism that already exists in pwm-
backlight and make it available to DT systems. If we start making a Tegra-
specific solution, then other architectures will have to reinvent the wheel 
again. I really don't think we want to go that way.

These patches only makes slight changes to pwm_bl.c and do not extend its 
capabilities. I agree that a suitable solution will require the CDF, but by 
the meantime, let's go for the practical route instead of repeating the same 
mistakes (i.e. architecture-specific frameworks) again.

There are certainly better ways to do this, but I'm not convinced at all that 
a Tegra-only solution is one of them.

Alex.

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Alex Courbot]

Hi Thierry,

On Monday 21 January 2013 15:49:28 Thierry Reding wrote:
> Eventually this should all be covered by the CDF, but since that's not
> ready yet we want something ad-hoc to get the hardware supported. As
> such I would like to see this go into some sort of minimalistic, Tegra-
> specific display/panel framework. I'd prefer to keep the pwm-backlight
> driver as simple and generic as possible, that is, a driver for a PWM-
> controlled backlight.
> 
> Another advantage of moving this into a sort of display framework is
> that it may help in defining the requirements for a CDF and that moving
> the code to the CDF should be easier once it is done.
> 
> Last but not least, abstracting away the panel allows other things such
> as physical dimensions and display modes to be properly encapsulated. I
> think that power-on/off timing requirements for panels also belong to
> this set since they are usually specific to a given panel.
> 
> Maybe adding these drivers to tegra-drm for now would be a good option.
> That way the corresponding glue can be added without a need for inter-
> tree dependencies.

IIRC (because that was a while ago already) having a Tegra-only display 
framework is exactly what we wanted to avoid in the first place. This series 
does nothing but leverage the callbacks mechanism that already exists in pwm-
backlight and make it available to DT systems. If we start making a Tegra-
specific solution, then other architectures will have to reinvent the wheel 
again. I really don't think we want to go that way.

These patches only makes slight changes to pwm_bl.c and do not extend its 
capabilities. I agree that a suitable solution will require the CDF, but by 
the meantime, let's go for the practical route instead of repeating the same 
mistakes (i.e. architecture-specific frameworks) again.

There are certainly better ways to do this, but I'm not convinced at all that 
a Tegra-only solution is one of them.

Alex.

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Alex Courbot]

Hi Thierry,

On Monday 21 January 2013 15:49:28 Thierry Reding wrote:
> Eventually this should all be covered by the CDF, but since that's not
> ready yet we want something ad-hoc to get the hardware supported. As
> such I would like to see this go into some sort of minimalistic, Tegra-
> specific display/panel framework. I'd prefer to keep the pwm-backlight
> driver as simple and generic as possible, that is, a driver for a PWM-
> controlled backlight.
> 
> Another advantage of moving this into a sort of display framework is
> that it may help in defining the requirements for a CDF and that moving
> the code to the CDF should be easier once it is done.
> 
> Last but not least, abstracting away the panel allows other things such
> as physical dimensions and display modes to be properly encapsulated. I
> think that power-on/off timing requirements for panels also belong to
> this set since they are usually specific to a given panel.
> 
> Maybe adding these drivers to tegra-drm for now would be a good option.
> That way the corresponding glue can be added without a need for inter-
> tree dependencies.

IIRC (because that was a while ago already) having a Tegra-only display 
framework is exactly what we wanted to avoid in the first place. This series 
does nothing but leverage the callbacks mechanism that already exists in pwm-
backlight and make it available to DT systems. If we start making a Tegra-
specific solution, then other architectures will have to reinvent the wheel 
again. I really don't think we want to go that way.

These patches only makes slight changes to pwm_bl.c and do not extend its 
capabilities. I agree that a suitable solution will require the CDF, but by 
the meantime, let's go for the practical route instead of repeating the same 
mistakes (i.e. architecture-specific frameworks) again.

There are certainly better ways to do this, but I'm not convinced at all that 
a Tegra-only solution is one of them.

Alex.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alex Courbot]

On Monday 21 January 2013 15:35:58 Mark Zhang wrote:
> > +	backlight {
> > +		compatible = "pwm-backlight-ventana";
> > +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 
208
> > 224 240 255>; +		default-brightness-level = <12>;
> > +
> > +		pwms = <&pwm 2 5000000>;
> 
> After read the codes of tegra pwm driver & pwm framework, I got to know
> the meaning of this property. So I think we need to add a doc(e.g:
> Documentation/devicetree/bindings/video/backlight/nvidia,tegra20-bl.txt)
> to explain this, "Documentation/devicetree/bindings/pwm/pwm.txt" doesn't
> explain this, because this may be different between different pwm drivers.

The bindings are in Documentation/devicetree/bindings/video/backlight/pwm-
backlight.txt . But you are right that the power supplies and GPIO will 
require a description of their own - I omitted it for this version because I 
am not sure what the driver should be called.

The panel used on Ventana is a Chunghwa CLAA101WA01A, maybe that's the name we 
should use for the compatible string instead (and rename the driver 
accordingly).

> So according to the filename, I think we can put all tegra boards codes
> here, right? Just like what you do for Ventana, if I wanna add support
> for cardhu, I can define similar functions -- let's say "init_cardhu",
> "exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?

That was my initial intention, yes.

> But I think if we do in this way, the file will become very long soon.
> And there are a lot of redundant codes in it. So do you have any
> suggestions?

If we decide to make a "Tegra" driver, then I don't think the size of the file 
is a big issues, as long as one can easily navigate into it. It will make 
sense to do this since Tegra kernels should include support for all the 
boards.

If we go and name the drivers after their actual panel names, we should 
definitely put them into separate files. The Tegra configuration could then 
include them all by default to make sure all boards are supported.

Alex.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alex Courbot]

On Monday 21 January 2013 15:35:58 Mark Zhang wrote:
> > +	backlight {
> > +		compatible = "pwm-backlight-ventana";
> > +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 
208
> > 224 240 255>; +		default-brightness-level = <12>;
> > +
> > +		pwms = <&pwm 2 5000000>;
> 
> After read the codes of tegra pwm driver & pwm framework, I got to know
> the meaning of this property. So I think we need to add a doc(e.g:
> Documentation/devicetree/bindings/video/backlight/nvidia,tegra20-bl.txt)
> to explain this, "Documentation/devicetree/bindings/pwm/pwm.txt" doesn't
> explain this, because this may be different between different pwm drivers.

The bindings are in Documentation/devicetree/bindings/video/backlight/pwm-
backlight.txt . But you are right that the power supplies and GPIO will 
require a description of their own - I omitted it for this version because I 
am not sure what the driver should be called.

The panel used on Ventana is a Chunghwa CLAA101WA01A, maybe that's the name we 
should use for the compatible string instead (and rename the driver 
accordingly).

> So according to the filename, I think we can put all tegra boards codes
> here, right? Just like what you do for Ventana, if I wanna add support
> for cardhu, I can define similar functions -- let's say "init_cardhu",
> "exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?

That was my initial intention, yes.

> But I think if we do in this way, the file will become very long soon.
> And there are a lot of redundant codes in it. So do you have any
> suggestions?

If we decide to make a "Tegra" driver, then I don't think the size of the file 
is a big issues, as long as one can easily navigate into it. It will make 
sense to do this since Tegra kernels should include support for all the 
boards.

If we go and name the drivers after their actual panel names, we should 
definitely put them into separate files. The Tegra configuration could then 
include them all by default to make sure all boards are supported.

Alex.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Mark Zhang]

On 01/21/2013 04:24 PM, Alex Courbot wrote:
> On Monday 21 January 2013 15:35:58 Mark Zhang wrote:
>>> +	backlight {
>>> +		compatible = "pwm-backlight-ventana";
>>> +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 
> 208
>>> 224 240 255>; +		default-brightness-level = <12>;
>>> +
>>> +		pwms = <&pwm 2 5000000>;
>>
>> After read the codes of tegra pwm driver & pwm framework, I got to know
>> the meaning of this property. So I think we need to add a doc(e.g:
>> Documentation/devicetree/bindings/video/backlight/nvidia,tegra20-bl.txt)
>> to explain this, "Documentation/devicetree/bindings/pwm/pwm.txt" doesn't
>> explain this, because this may be different between different pwm drivers.
> 
> The bindings are in Documentation/devicetree/bindings/video/backlight/pwm-
> backlight.txt . But you are right that the power supplies and GPIO will 
> require a description of their own - I omitted it for this version because I 
> am not sure what the driver should be called.
> 

The description of this property in pwm-backlight.txt is:

"pwms: OF device-tree PWM specification (see PWM binding[0])
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt"

So you can't get any useful infos from that. That's why I propose to add
a tegra specific doc in
"Documentation/devicetree/bindings/video/backlight" directory.

> The panel used on Ventana is a Chunghwa CLAA101WA01A, maybe that's the name we 
> should use for the compatible string instead (and rename the driver 
> accordingly).
> 
>> So according to the filename, I think we can put all tegra boards codes
>> here, right? Just like what you do for Ventana, if I wanna add support
>> for cardhu, I can define similar functions -- let's say "init_cardhu",
>> "exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?
> 
> That was my initial intention, yes.
> 
>> But I think if we do in this way, the file will become very long soon.
>> And there are a lot of redundant codes in it. So do you have any
>> suggestions?
> 
> If we decide to make a "Tegra" driver, then I don't think the size of the file 
> is a big issues, as long as one can easily navigate into it. It will make 
> sense to do this since Tegra kernels should include support for all the 
> boards.
> 
> If we go and name the drivers after their actual panel names, we should 
> definitely put them into separate files. The Tegra configuration could then 
> include them all by default to make sure all boards are supported.

I don't think use panel name instead of board name is a good idea.
Developers may not be familiar with panel names. So if we use panel
name, we have to search and read a lot of manual to find out what the
panel is.

I'd rather putting all stuffs in pwm_bl_tegra.c than separating them.

Mark
> 
> Alex.
> 

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Mark Zhang]

On 01/21/2013 04:24 PM, Alex Courbot wrote:
> On Monday 21 January 2013 15:35:58 Mark Zhang wrote:
>>> +	backlight {
>>> +		compatible = "pwm-backlight-ventana";
>>> +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 
> 208
>>> 224 240 255>; +		default-brightness-level = <12>;
>>> +
>>> +		pwms = <&pwm 2 5000000>;
>>
>> After read the codes of tegra pwm driver & pwm framework, I got to know
>> the meaning of this property. So I think we need to add a doc(e.g:
>> Documentation/devicetree/bindings/video/backlight/nvidia,tegra20-bl.txt)
>> to explain this, "Documentation/devicetree/bindings/pwm/pwm.txt" doesn't
>> explain this, because this may be different between different pwm drivers.
> 
> The bindings are in Documentation/devicetree/bindings/video/backlight/pwm-
> backlight.txt . But you are right that the power supplies and GPIO will 
> require a description of their own - I omitted it for this version because I 
> am not sure what the driver should be called.
> 

The description of this property in pwm-backlight.txt is:

"pwms: OF device-tree PWM specification (see PWM binding[0])
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt"

So you can't get any useful infos from that. That's why I propose to add
a tegra specific doc in
"Documentation/devicetree/bindings/video/backlight" directory.

> The panel used on Ventana is a Chunghwa CLAA101WA01A, maybe that's the name we 
> should use for the compatible string instead (and rename the driver 
> accordingly).
> 
>> So according to the filename, I think we can put all tegra boards codes
>> here, right? Just like what you do for Ventana, if I wanna add support
>> for cardhu, I can define similar functions -- let's say "init_cardhu",
>> "exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?
> 
> That was my initial intention, yes.
> 
>> But I think if we do in this way, the file will become very long soon.
>> And there are a lot of redundant codes in it. So do you have any
>> suggestions?
> 
> If we decide to make a "Tegra" driver, then I don't think the size of the file 
> is a big issues, as long as one can easily navigate into it. It will make 
> sense to do this since Tegra kernels should include support for all the 
> boards.
> 
> If we go and name the drivers after their actual panel names, we should 
> definitely put them into separate files. The Tegra configuration could then 
> include them all by default to make sure all boards are supported.

I don't think use panel name instead of board name is a good idea.
Developers may not be familiar with panel names. So if we use panel
name, we have to search and read a lot of manual to find out what the
panel is.

I'd rather putting all stuffs in pwm_bl_tegra.c than separating them.

Mark
> 
> Alex.
> 

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Mark Zhang]

On 01/21/2013 04:24 PM, Alex Courbot wrote:
> On Monday 21 January 2013 15:35:58 Mark Zhang wrote:
>>> +	backlight {
>>> +		compatible = "pwm-backlight-ventana";
>>> +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 
> 208
>>> 224 240 255>; +		default-brightness-level = <12>;
>>> +
>>> +		pwms = <&pwm 2 5000000>;
>>
>> After read the codes of tegra pwm driver & pwm framework, I got to know
>> the meaning of this property. So I think we need to add a doc(e.g:
>> Documentation/devicetree/bindings/video/backlight/nvidia,tegra20-bl.txt)
>> to explain this, "Documentation/devicetree/bindings/pwm/pwm.txt" doesn't
>> explain this, because this may be different between different pwm drivers.
> 
> The bindings are in Documentation/devicetree/bindings/video/backlight/pwm-
> backlight.txt . But you are right that the power supplies and GPIO will 
> require a description of their own - I omitted it for this version because I 
> am not sure what the driver should be called.
> 

The description of this property in pwm-backlight.txt is:

"pwms: OF device-tree PWM specification (see PWM binding[0])
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt"

So you can't get any useful infos from that. That's why I propose to add
a tegra specific doc in
"Documentation/devicetree/bindings/video/backlight" directory.

> The panel used on Ventana is a Chunghwa CLAA101WA01A, maybe that's the name we 
> should use for the compatible string instead (and rename the driver 
> accordingly).
> 
>> So according to the filename, I think we can put all tegra boards codes
>> here, right? Just like what you do for Ventana, if I wanna add support
>> for cardhu, I can define similar functions -- let's say "init_cardhu",
>> "exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?
> 
> That was my initial intention, yes.
> 
>> But I think if we do in this way, the file will become very long soon.
>> And there are a lot of redundant codes in it. So do you have any
>> suggestions?
> 
> If we decide to make a "Tegra" driver, then I don't think the size of the file 
> is a big issues, as long as one can easily navigate into it. It will make 
> sense to do this since Tegra kernels should include support for all the 
> boards.
> 
> If we go and name the drivers after their actual panel names, we should 
> definitely put them into separate files. The Tegra configuration could then 
> include them all by default to make sure all boards are supported.

I don't think use panel name instead of board name is a good idea.
Developers may not be familiar with panel names. So if we use panel
name, we have to search and read a lot of manual to find out what the
panel is.

I'd rather putting all stuffs in pwm_bl_tegra.c than separating them.

Mark
> 
> Alex.
> 

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Marc Dietrich]

Hi,

> > diff --git a/drivers/video/backlight/pwm_bl_tegra.c
> > b/drivers/video/backlight/pwm_bl_tegra.c new file mode 100644
> > index 0000000..8f2195b
> > --- /dev/null
> > +++ b/drivers/video/backlight/pwm_bl_tegra.c
> 
> So according to the filename, I think we can put all tegra boards codes
> here, right? Just like what you do for Ventana, if I wanna add support
> for cardhu, I can define similar functions -- let's say "init_cardhu",
> "exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?
> 
> But I think if we do in this way, the file will become very long soon.
> And there are a lot of redundant codes in it. So do you have any
> suggestions?

I think we (for PAZ00) will just reuse the ventana code which is sufficient 
for us. But adding "pwm-backlight-ventana" to our DTS may look a bit strange. 
On the other hand, I guess that's why the property is called "compatible".

Marc

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Marc Dietrich]

Hi,

> > diff --git a/drivers/video/backlight/pwm_bl_tegra.c
> > b/drivers/video/backlight/pwm_bl_tegra.c new file mode 100644
> > index 0000000..8f2195b
> > --- /dev/null
> > +++ b/drivers/video/backlight/pwm_bl_tegra.c
> 
> So according to the filename, I think we can put all tegra boards codes
> here, right? Just like what you do for Ventana, if I wanna add support
> for cardhu, I can define similar functions -- let's say "init_cardhu",
> "exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?
> 
> But I think if we do in this way, the file will become very long soon.
> And there are a lot of redundant codes in it. So do you have any
> suggestions?

I think we (for PAZ00) will just reuse the ventana code which is sufficient 
for us. But adding "pwm-backlight-ventana" to our DTS may look a bit strange. 
On the other hand, I guess that's why the property is called "compatible".

Marc

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Marc Dietrich]

Hi,

> > diff --git a/drivers/video/backlight/pwm_bl_tegra.c
> > b/drivers/video/backlight/pwm_bl_tegra.c new file mode 100644
> > index 0000000..8f2195b
> > --- /dev/null
> > +++ b/drivers/video/backlight/pwm_bl_tegra.c
> 
> So according to the filename, I think we can put all tegra boards codes
> here, right? Just like what you do for Ventana, if I wanna add support
> for cardhu, I can define similar functions -- let's say "init_cardhu",
> "exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?
> 
> But I think if we do in this way, the file will become very long soon.
> And there are a lot of redundant codes in it. So do you have any
> suggestions?

I think we (for PAZ00) will just reuse the ventana code which is sufficient 
for us. But adding "pwm-backlight-ventana" to our DTS may look a bit strange. 
On the other hand, I guess that's why the property is called "compatible".

Marc

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Mark Zhang]

On 01/21/2013 04:52 PM, Marc Dietrich wrote:
> Hi,
> 
>>> diff --git a/drivers/video/backlight/pwm_bl_tegra.c
>>> b/drivers/video/backlight/pwm_bl_tegra.c new file mode 100644
>>> index 0000000..8f2195b
>>> --- /dev/null
>>> +++ b/drivers/video/backlight/pwm_bl_tegra.c
>>
>> So according to the filename, I think we can put all tegra boards codes
>> here, right? Just like what you do for Ventana, if I wanna add support
>> for cardhu, I can define similar functions -- let's say "init_cardhu",
>> "exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?
>>
>> But I think if we do in this way, the file will become very long soon.
>> And there are a lot of redundant codes in it. So do you have any
>> suggestions?
> 
> I think we (for PAZ00) will just reuse the ventana code which is sufficient 
> for us. But adding "pwm-backlight-ventana" to our DTS may look a bit strange. 
> On the other hand, I guess that's why the property is called "compatible".
> 

Ah, yeah, that looks strange. :)
Okay, so I know why Alex wants to use panel name while not board name...

> Marc
> 

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Mark Zhang]

On 01/21/2013 04:52 PM, Marc Dietrich wrote:
> Hi,
> 
>>> diff --git a/drivers/video/backlight/pwm_bl_tegra.c
>>> b/drivers/video/backlight/pwm_bl_tegra.c new file mode 100644
>>> index 0000000..8f2195b
>>> --- /dev/null
>>> +++ b/drivers/video/backlight/pwm_bl_tegra.c
>>
>> So according to the filename, I think we can put all tegra boards codes
>> here, right? Just like what you do for Ventana, if I wanna add support
>> for cardhu, I can define similar functions -- let's say "init_cardhu",
>> "exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?
>>
>> But I think if we do in this way, the file will become very long soon.
>> And there are a lot of redundant codes in it. So do you have any
>> suggestions?
> 
> I think we (for PAZ00) will just reuse the ventana code which is sufficient 
> for us. But adding "pwm-backlight-ventana" to our DTS may look a bit strange. 
> On the other hand, I guess that's why the property is called "compatible".
> 

Ah, yeah, that looks strange. :)
Okay, so I know why Alex wants to use panel name while not board name...

> Marc
> 

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Stephen Warren]

On 01/19/2013 03:30 AM, Alexandre Courbot wrote:
> Add a PWM-backlight subdriver for Tegra boards, with support for
> Ventana.
> 
> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
>  arch/arm/configs/tegra_defconfig       |   1 +
>  drivers/video/backlight/Kconfig        |   7 ++
>  drivers/video/backlight/pwm_bl.c       |   3 +
>  drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++

This should be at least 3 separate patches: (1) Driver code (2) Ventana
.dts file (3) Tegra defconfig.

> diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts

> +	backlight {
> +		compatible = "pwm-backlight-ventana";

If this is Ventana-specific, this should have a vendor prefix; "nvidia,"
would be appropriate.

But, why is this Ventana-specific; surely it's at most panel-specific,
or perhaps even generic across any/most LCD panels?

There needs to be binding documentation.

> +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
> +		default-brightness-level = <12>;
> +
> +		pwms = <&pwm 2 5000000>;
> +		pwm-names = "backlight";
> +
> +		power-supply = <&vdd_bl_reg>;

"power" doesn't seem like a good regulator name; power to what? Is this
for the backlight, since I see there's a panel-supply below?

> +		panel-supply = <&vdd_pnl_reg>;

> +		bl-gpio = <&gpio 28 0>;
> +		bl-panel = <&gpio 10 0>;

GPIO names usually have "gpios" in their name, so I assume those should
be bl-enable-gpios, panel-enable-gpios?

> diff --git a/drivers/video/backlight/pwm_bl_tegra.c b/drivers/video/backlight/pwm_bl_tegra.c

> +static void exit_ventana(struct device *dev)
> +{
> +	struct ventana_bl_data *data = pwm_backlight_get_subdriver_data(dev);
> +
> +	devm_gpio_free(dev, data->panel_gpio);
> +	devm_gpio_free(dev, data->bl_gpio);
> +	devm_regulator_put(data->vdd_panel);
> +	devm_regulator_put(data->vdd_power);
> +	devm_kfree(dev, data);
> +}

There shouldn't be a need to explicitly free devm-allocated objects in
almost all cases; that's the whole point of the devm APIs.

> +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
> +	.name = "pwm-backlight-ventana",
> +	.init = init_ventana,
> +	.exit = exit_ventana,
> +	.notify = notify_ventana,
> +	.notify_after = notify_after_ventana,
> +};

It seems like all of that code should be completely generic.

> +static int __init pwm_backlight_tegra_init(void)
> +{
> +	pwm_backlight_add_subdriver(&pwm_backlight_ventana_subdriver);
> +	return 0;
> +}
> +
> +static void __exit pwm_backlight_tegra_exit(void)
> +{
> +	pwm_backlight_remove_subdriver(&pwm_backlight_ventana_subdriver);
> +}
> +
> +module_init(pwm_backlight_tegra_init);
> +module_exit(pwm_backlight_tegra_exit);

Rather than invent some new registration mechanism, if we need
board-/panel-/...-specific drivers, it'd be better to make each of those
specific drivers a full platform device in an of itself (i.e. regular
Linux platform device/driver, have its own probe(), etc.), and have
those specific drivers call into the base PWM backlight code, treating
it like a utility API.

> +MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pwm-tegra-backlight");
> +
> +

Some extra blank lines there.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Stephen Warren]

On 01/19/2013 03:30 AM, Alexandre Courbot wrote:
> Add a PWM-backlight subdriver for Tegra boards, with support for
> Ventana.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
>  arch/arm/configs/tegra_defconfig       |   1 +
>  drivers/video/backlight/Kconfig        |   7 ++
>  drivers/video/backlight/pwm_bl.c       |   3 +
>  drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++

This should be at least 3 separate patches: (1) Driver code (2) Ventana
.dts file (3) Tegra defconfig.

> diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts

> +	backlight {
> +		compatible = "pwm-backlight-ventana";

If this is Ventana-specific, this should have a vendor prefix; "nvidia,"
would be appropriate.

But, why is this Ventana-specific; surely it's at most panel-specific,
or perhaps even generic across any/most LCD panels?

There needs to be binding documentation.

> +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
> +		default-brightness-level = <12>;
> +
> +		pwms = <&pwm 2 5000000>;
> +		pwm-names = "backlight";
> +
> +		power-supply = <&vdd_bl_reg>;

"power" doesn't seem like a good regulator name; power to what? Is this
for the backlight, since I see there's a panel-supply below?

> +		panel-supply = <&vdd_pnl_reg>;

> +		bl-gpio = <&gpio 28 0>;
> +		bl-panel = <&gpio 10 0>;

GPIO names usually have "gpios" in their name, so I assume those should
be bl-enable-gpios, panel-enable-gpios?

> diff --git a/drivers/video/backlight/pwm_bl_tegra.c b/drivers/video/backlight/pwm_bl_tegra.c

> +static void exit_ventana(struct device *dev)
> +{
> +	struct ventana_bl_data *data = pwm_backlight_get_subdriver_data(dev);
> +
> +	devm_gpio_free(dev, data->panel_gpio);
> +	devm_gpio_free(dev, data->bl_gpio);
> +	devm_regulator_put(data->vdd_panel);
> +	devm_regulator_put(data->vdd_power);
> +	devm_kfree(dev, data);
> +}

There shouldn't be a need to explicitly free devm-allocated objects in
almost all cases; that's the whole point of the devm APIs.

> +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
> +	.name = "pwm-backlight-ventana",
> +	.init = init_ventana,
> +	.exit = exit_ventana,
> +	.notify = notify_ventana,
> +	.notify_after = notify_after_ventana,
> +};

It seems like all of that code should be completely generic.

> +static int __init pwm_backlight_tegra_init(void)
> +{
> +	pwm_backlight_add_subdriver(&pwm_backlight_ventana_subdriver);
> +	return 0;
> +}
> +
> +static void __exit pwm_backlight_tegra_exit(void)
> +{
> +	pwm_backlight_remove_subdriver(&pwm_backlight_ventana_subdriver);
> +}
> +
> +module_init(pwm_backlight_tegra_init);
> +module_exit(pwm_backlight_tegra_exit);

Rather than invent some new registration mechanism, if we need
board-/panel-/...-specific drivers, it'd be better to make each of those
specific drivers a full platform device in an of itself (i.e. regular
Linux platform device/driver, have its own probe(), etc.), and have
those specific drivers call into the base PWM backlight code, treating
it like a utility API.

> +MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pwm-tegra-backlight");
> +
> +

Some extra blank lines there.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Stephen Warren]

On 01/19/2013 03:30 AM, Alexandre Courbot wrote:
> Add a PWM-backlight subdriver for Tegra boards, with support for
> Ventana.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
>  arch/arm/configs/tegra_defconfig       |   1 +
>  drivers/video/backlight/Kconfig        |   7 ++
>  drivers/video/backlight/pwm_bl.c       |   3 +
>  drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++

This should be at least 3 separate patches: (1) Driver code (2) Ventana
.dts file (3) Tegra defconfig.

> diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts

> +	backlight {
> +		compatible = "pwm-backlight-ventana";

If this is Ventana-specific, this should have a vendor prefix; "nvidia,"
would be appropriate.

But, why is this Ventana-specific; surely it's at most panel-specific,
or perhaps even generic across any/most LCD panels?

There needs to be binding documentation.

> +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
> +		default-brightness-level = <12>;
> +
> +		pwms = <&pwm 2 5000000>;
> +		pwm-names = "backlight";
> +
> +		power-supply = <&vdd_bl_reg>;

"power" doesn't seem like a good regulator name; power to what? Is this
for the backlight, since I see there's a panel-supply below?

> +		panel-supply = <&vdd_pnl_reg>;

> +		bl-gpio = <&gpio 28 0>;
> +		bl-panel = <&gpio 10 0>;

GPIO names usually have "gpios" in their name, so I assume those should
be bl-enable-gpios, panel-enable-gpios?

> diff --git a/drivers/video/backlight/pwm_bl_tegra.c b/drivers/video/backlight/pwm_bl_tegra.c

> +static void exit_ventana(struct device *dev)
> +{
> +	struct ventana_bl_data *data = pwm_backlight_get_subdriver_data(dev);
> +
> +	devm_gpio_free(dev, data->panel_gpio);
> +	devm_gpio_free(dev, data->bl_gpio);
> +	devm_regulator_put(data->vdd_panel);
> +	devm_regulator_put(data->vdd_power);
> +	devm_kfree(dev, data);
> +}

There shouldn't be a need to explicitly free devm-allocated objects in
almost all cases; that's the whole point of the devm APIs.

> +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
> +	.name = "pwm-backlight-ventana",
> +	.init = init_ventana,
> +	.exit = exit_ventana,
> +	.notify = notify_ventana,
> +	.notify_after = notify_after_ventana,
> +};

It seems like all of that code should be completely generic.

> +static int __init pwm_backlight_tegra_init(void)
> +{
> +	pwm_backlight_add_subdriver(&pwm_backlight_ventana_subdriver);
> +	return 0;
> +}
> +
> +static void __exit pwm_backlight_tegra_exit(void)
> +{
> +	pwm_backlight_remove_subdriver(&pwm_backlight_ventana_subdriver);
> +}
> +
> +module_init(pwm_backlight_tegra_init);
> +module_exit(pwm_backlight_tegra_exit);

Rather than invent some new registration mechanism, if we need
board-/panel-/...-specific drivers, it'd be better to make each of those
specific drivers a full platform device in an of itself (i.e. regular
Linux platform device/driver, have its own probe(), etc.), and have
those specific drivers call into the base PWM backlight code, treating
it like a utility API.

> +MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pwm-tegra-backlight");
> +
> +

Some extra blank lines there.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alex Courbot]

On Tuesday 22 January 2013 01:46:33 Stephen Warren wrote:
> >  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
> >  arch/arm/configs/tegra_defconfig       |   1 +
> >  drivers/video/backlight/Kconfig        |   7 ++
> >  drivers/video/backlight/pwm_bl.c       |   3 +
> >  drivers/video/backlight/pwm_bl_tegra.c | 159
> >  +++++++++++++++++++++++++++++++++
> This should be at least 3 separate patches: (1) Driver code (2) Ventana
> .dts file (3) Tegra defconfig.

Will do that.

> If this is Ventana-specific, this should have a vendor prefix; "nvidia,"
> would be appropriate.
> 
> But, why is this Ventana-specific; surely it's at most panel-specific,
> or perhaps even generic across any/most LCD panels?

Yes, we could use the panel model here instead. Not sure how many other panels 
follow the same powering sequence though.

Making it Ventana-specific would have allowed to group all Tegra board support 
into the same driver, and considering that probably not many devices use the 
same panels as we do this seemed to make sense at first.

> > +		power-supply = <&vdd_bl_reg>;
> 
> "power" doesn't seem like a good regulator name; power to what? Is this
> for the backlight, since I see there's a panel-supply below?
> 
> > +		panel-supply = <&vdd_pnl_reg>;
> > 
> > +		bl-gpio = <&gpio 28 0>;
> > +		bl-panel = <&gpio 10 0>;
> 
> GPIO names usually have "gpios" in their name, so I assume those should
> be bl-enable-gpios, panel-enable-gpios?

Indeed, even though there is only one gpio here. Maybe we could group them 
into a single property and retrieve them by index - that's what the DT GPIO 
APIs seem to be designed for initially.

> > +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
> > +	.name = "pwm-backlight-ventana",
> > +	.init = init_ventana,
> > +	.exit = exit_ventana,
> > +	.notify = notify_ventana,
> > +	.notify_after = notify_after_ventana,
> > +};
> 
> It seems like all of that code should be completely generic.

Sorry, I don't get your point here - could you elaborate?

> Rather than invent some new registration mechanism, if we need
> board-/panel-/...-specific drivers, it'd be better to make each of those
> specific drivers a full platform device in an of itself (i.e. regular
> Linux platform device/driver, have its own probe(), etc.), and have
> those specific drivers call into the base PWM backlight code, treating
> it like a utility API.

That's what would make the most sense indeed, but would require some extra 
changes in pwm-backlight and might go against Thierry's wish to keep it 
simple. On the other hand I totally agree this would be more elegant. Every 
pwm-backlight based driver would just need to invoke pwm_bl's probe/remove 
function from its own. Thierry, would that be an acceptable alternative to the 
sub-driver thing despite the slightly deeper changes this involves?

Thanks,
Alex.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alex Courbot]

On Tuesday 22 January 2013 01:46:33 Stephen Warren wrote:
> >  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
> >  arch/arm/configs/tegra_defconfig       |   1 +
> >  drivers/video/backlight/Kconfig        |   7 ++
> >  drivers/video/backlight/pwm_bl.c       |   3 +
> >  drivers/video/backlight/pwm_bl_tegra.c | 159
> >  +++++++++++++++++++++++++++++++++
> This should be at least 3 separate patches: (1) Driver code (2) Ventana
> .dts file (3) Tegra defconfig.

Will do that.

> If this is Ventana-specific, this should have a vendor prefix; "nvidia,"
> would be appropriate.
> 
> But, why is this Ventana-specific; surely it's at most panel-specific,
> or perhaps even generic across any/most LCD panels?

Yes, we could use the panel model here instead. Not sure how many other panels 
follow the same powering sequence though.

Making it Ventana-specific would have allowed to group all Tegra board support 
into the same driver, and considering that probably not many devices use the 
same panels as we do this seemed to make sense at first.

> > +		power-supply = <&vdd_bl_reg>;
> 
> "power" doesn't seem like a good regulator name; power to what? Is this
> for the backlight, since I see there's a panel-supply below?
> 
> > +		panel-supply = <&vdd_pnl_reg>;
> > 
> > +		bl-gpio = <&gpio 28 0>;
> > +		bl-panel = <&gpio 10 0>;
> 
> GPIO names usually have "gpios" in their name, so I assume those should
> be bl-enable-gpios, panel-enable-gpios?

Indeed, even though there is only one gpio here. Maybe we could group them 
into a single property and retrieve them by index - that's what the DT GPIO 
APIs seem to be designed for initially.

> > +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
> > +	.name = "pwm-backlight-ventana",
> > +	.init = init_ventana,
> > +	.exit = exit_ventana,
> > +	.notify = notify_ventana,
> > +	.notify_after = notify_after_ventana,
> > +};
> 
> It seems like all of that code should be completely generic.

Sorry, I don't get your point here - could you elaborate?

> Rather than invent some new registration mechanism, if we need
> board-/panel-/...-specific drivers, it'd be better to make each of those
> specific drivers a full platform device in an of itself (i.e. regular
> Linux platform device/driver, have its own probe(), etc.), and have
> those specific drivers call into the base PWM backlight code, treating
> it like a utility API.

That's what would make the most sense indeed, but would require some extra 
changes in pwm-backlight and might go against Thierry's wish to keep it 
simple. On the other hand I totally agree this would be more elegant. Every 
pwm-backlight based driver would just need to invoke pwm_bl's probe/remove 
function from its own. Thierry, would that be an acceptable alternative to the 
sub-driver thing despite the slightly deeper changes this involves?

Thanks,
Alex.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alex Courbot]

On Tuesday 22 January 2013 01:46:33 Stephen Warren wrote:
> >  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
> >  arch/arm/configs/tegra_defconfig       |   1 +
> >  drivers/video/backlight/Kconfig        |   7 ++
> >  drivers/video/backlight/pwm_bl.c       |   3 +
> >  drivers/video/backlight/pwm_bl_tegra.c | 159
> >  +++++++++++++++++++++++++++++++++
> This should be at least 3 separate patches: (1) Driver code (2) Ventana
> .dts file (3) Tegra defconfig.

Will do that.

> If this is Ventana-specific, this should have a vendor prefix; "nvidia,"
> would be appropriate.
> 
> But, why is this Ventana-specific; surely it's at most panel-specific,
> or perhaps even generic across any/most LCD panels?

Yes, we could use the panel model here instead. Not sure how many other panels 
follow the same powering sequence though.

Making it Ventana-specific would have allowed to group all Tegra board support 
into the same driver, and considering that probably not many devices use the 
same panels as we do this seemed to make sense at first.

> > +		power-supply = <&vdd_bl_reg>;
> 
> "power" doesn't seem like a good regulator name; power to what? Is this
> for the backlight, since I see there's a panel-supply below?
> 
> > +		panel-supply = <&vdd_pnl_reg>;
> > 
> > +		bl-gpio = <&gpio 28 0>;
> > +		bl-panel = <&gpio 10 0>;
> 
> GPIO names usually have "gpios" in their name, so I assume those should
> be bl-enable-gpios, panel-enable-gpios?

Indeed, even though there is only one gpio here. Maybe we could group them 
into a single property and retrieve them by index - that's what the DT GPIO 
APIs seem to be designed for initially.

> > +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
> > +	.name = "pwm-backlight-ventana",
> > +	.init = init_ventana,
> > +	.exit = exit_ventana,
> > +	.notify = notify_ventana,
> > +	.notify_after = notify_after_ventana,
> > +};
> 
> It seems like all of that code should be completely generic.

Sorry, I don't get your point here - could you elaborate?

> Rather than invent some new registration mechanism, if we need
> board-/panel-/...-specific drivers, it'd be better to make each of those
> specific drivers a full platform device in an of itself (i.e. regular
> Linux platform device/driver, have its own probe(), etc.), and have
> those specific drivers call into the base PWM backlight code, treating
> it like a utility API.

That's what would make the most sense indeed, but would require some extra 
changes in pwm-backlight and might go against Thierry's wish to keep it 
simple. On the other hand I totally agree this would be more elegant. Every 
pwm-backlight based driver would just need to invoke pwm_bl's probe/remove 
function from its own. Thierry, would that be an acceptable alternative to the 
sub-driver thing despite the slightly deeper changes this involves?

Thanks,
Alex.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Thierry Reding]

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

On Tue, Jan 22, 2013 at 12:24:34PM +0900, Alex Courbot wrote:
> On Tuesday 22 January 2013 01:46:33 Stephen Warren wrote:
> > >  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
> > >  arch/arm/configs/tegra_defconfig       |   1 +
> > >  drivers/video/backlight/Kconfig        |   7 ++
> > >  drivers/video/backlight/pwm_bl.c       |   3 +
> > >  drivers/video/backlight/pwm_bl_tegra.c | 159
> > >  +++++++++++++++++++++++++++++++++
> > This should be at least 3 separate patches: (1) Driver code (2) Ventana
> > .dts file (3) Tegra defconfig.
> 
> Will do that.
> 
> > If this is Ventana-specific, this should have a vendor prefix; "nvidia,"
> > would be appropriate.
> > 
> > But, why is this Ventana-specific; surely it's at most panel-specific,
> > or perhaps even generic across any/most LCD panels?
> 
> Yes, we could use the panel model here instead. Not sure how many other panels 
> follow the same powering sequence though.
> 
> Making it Ventana-specific would have allowed to group all Tegra board support 
> into the same driver, and considering that probably not many devices use the 
> same panels as we do this seemed to make sense at first.
> 
> > > +		power-supply = <&vdd_bl_reg>;
> > 
> > "power" doesn't seem like a good regulator name; power to what? Is this
> > for the backlight, since I see there's a panel-supply below?
> > 
> > > +		panel-supply = <&vdd_pnl_reg>;
> > > 
> > > +		bl-gpio = <&gpio 28 0>;
> > > +		bl-panel = <&gpio 10 0>;
> > 
> > GPIO names usually have "gpios" in their name, so I assume those should
> > be bl-enable-gpios, panel-enable-gpios?
> 
> Indeed, even though there is only one gpio here. Maybe we could group them 
> into a single property and retrieve them by index - that's what the DT GPIO 
> APIs seem to be designed for initially.
> 
> > > +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
> > > +	.name = "pwm-backlight-ventana",
> > > +	.init = init_ventana,
> > > +	.exit = exit_ventana,
> > > +	.notify = notify_ventana,
> > > +	.notify_after = notify_after_ventana,
> > > +};
> > 
> > It seems like all of that code should be completely generic.
> 
> Sorry, I don't get your point here - could you elaborate?
> 
> > Rather than invent some new registration mechanism, if we need
> > board-/panel-/...-specific drivers, it'd be better to make each of those
> > specific drivers a full platform device in an of itself (i.e. regular
> > Linux platform device/driver, have its own probe(), etc.), and have
> > those specific drivers call into the base PWM backlight code, treating
> > it like a utility API.
> 
> That's what would make the most sense indeed, but would require some extra 
> changes in pwm-backlight and might go against Thierry's wish to keep it 
> simple. On the other hand I totally agree this would be more elegant. Every 
> pwm-backlight based driver would just need to invoke pwm_bl's probe/remove 
> function from its own. Thierry, would that be an acceptable alternative to the 
> sub-driver thing despite the slightly deeper changes this involves?

I'm confused. Why would you want to call into pwm_bl directly? If we're
going to split this up into separate platform devices, why not look up a
given backlight device and use the backlight API on that? The pieces of
the puzzle are all there: you can use of_find_backlight_by_node() to
obtain a backlight device from a device tree node, so I'd expect the DT
to look something like this:

	backlight: backlight {
		compatible = "pwm-backlight";
		...
	};

	panel: panel {
		compatible = "...";
		...
		backlight = <&backlight>;
		...
	};

After that you can wire it up with host1x using something like:

	host1x {
		dc@54200000 {
			rgb {
				status = "okay";

				nvidia,panel = <&panel>;
			};
		};
	};

Maybe with such a binding, we should move the various display-timings
properties to the panel node as well and have an API to retrieve them
for use by tegra-drm.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Thierry Reding]

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

On Tue, Jan 22, 2013 at 12:24:34PM +0900, Alex Courbot wrote:
> On Tuesday 22 January 2013 01:46:33 Stephen Warren wrote:
> > >  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
> > >  arch/arm/configs/tegra_defconfig       |   1 +
> > >  drivers/video/backlight/Kconfig        |   7 ++
> > >  drivers/video/backlight/pwm_bl.c       |   3 +
> > >  drivers/video/backlight/pwm_bl_tegra.c | 159
> > >  +++++++++++++++++++++++++++++++++
> > This should be at least 3 separate patches: (1) Driver code (2) Ventana
> > .dts file (3) Tegra defconfig.
> 
> Will do that.
> 
> > If this is Ventana-specific, this should have a vendor prefix; "nvidia,"
> > would be appropriate.
> > 
> > But, why is this Ventana-specific; surely it's at most panel-specific,
> > or perhaps even generic across any/most LCD panels?
> 
> Yes, we could use the panel model here instead. Not sure how many other panels 
> follow the same powering sequence though.
> 
> Making it Ventana-specific would have allowed to group all Tegra board support 
> into the same driver, and considering that probably not many devices use the 
> same panels as we do this seemed to make sense at first.
> 
> > > +		power-supply = <&vdd_bl_reg>;
> > 
> > "power" doesn't seem like a good regulator name; power to what? Is this
> > for the backlight, since I see there's a panel-supply below?
> > 
> > > +		panel-supply = <&vdd_pnl_reg>;
> > > 
> > > +		bl-gpio = <&gpio 28 0>;
> > > +		bl-panel = <&gpio 10 0>;
> > 
> > GPIO names usually have "gpios" in their name, so I assume those should
> > be bl-enable-gpios, panel-enable-gpios?
> 
> Indeed, even though there is only one gpio here. Maybe we could group them 
> into a single property and retrieve them by index - that's what the DT GPIO 
> APIs seem to be designed for initially.
> 
> > > +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
> > > +	.name = "pwm-backlight-ventana",
> > > +	.init = init_ventana,
> > > +	.exit = exit_ventana,
> > > +	.notify = notify_ventana,
> > > +	.notify_after = notify_after_ventana,
> > > +};
> > 
> > It seems like all of that code should be completely generic.
> 
> Sorry, I don't get your point here - could you elaborate?
> 
> > Rather than invent some new registration mechanism, if we need
> > board-/panel-/...-specific drivers, it'd be better to make each of those
> > specific drivers a full platform device in an of itself (i.e. regular
> > Linux platform device/driver, have its own probe(), etc.), and have
> > those specific drivers call into the base PWM backlight code, treating
> > it like a utility API.
> 
> That's what would make the most sense indeed, but would require some extra 
> changes in pwm-backlight and might go against Thierry's wish to keep it 
> simple. On the other hand I totally agree this would be more elegant. Every 
> pwm-backlight based driver would just need to invoke pwm_bl's probe/remove 
> function from its own. Thierry, would that be an acceptable alternative to the 
> sub-driver thing despite the slightly deeper changes this involves?

I'm confused. Why would you want to call into pwm_bl directly? If we're
going to split this up into separate platform devices, why not look up a
given backlight device and use the backlight API on that? The pieces of
the puzzle are all there: you can use of_find_backlight_by_node() to
obtain a backlight device from a device tree node, so I'd expect the DT
to look something like this:

	backlight: backlight {
		compatible = "pwm-backlight";
		...
	};

	panel: panel {
		compatible = "...";
		...
		backlight = <&backlight>;
		...
	};

After that you can wire it up with host1x using something like:

	host1x {
		dc@54200000 {
			rgb {
				status = "okay";

				nvidia,panel = <&panel>;
			};
		};
	};

Maybe with such a binding, we should move the various display-timings
properties to the panel node as well and have an API to retrieve them
for use by tegra-drm.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Thierry Reding]

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

On Mon, Jan 21, 2013 at 05:18:11PM +0900, Alex Courbot wrote:
> Hi Thierry,
> 
> On Monday 21 January 2013 15:49:28 Thierry Reding wrote:
> > Eventually this should all be covered by the CDF, but since that's not
> > ready yet we want something ad-hoc to get the hardware supported. As
> > such I would like to see this go into some sort of minimalistic, Tegra-
> > specific display/panel framework. I'd prefer to keep the pwm-backlight
> > driver as simple and generic as possible, that is, a driver for a PWM-
> > controlled backlight.
> > 
> > Another advantage of moving this into a sort of display framework is
> > that it may help in defining the requirements for a CDF and that moving
> > the code to the CDF should be easier once it is done.
> > 
> > Last but not least, abstracting away the panel allows other things such
> > as physical dimensions and display modes to be properly encapsulated. I
> > think that power-on/off timing requirements for panels also belong to
> > this set since they are usually specific to a given panel.
> > 
> > Maybe adding these drivers to tegra-drm for now would be a good option.
> > That way the corresponding glue can be added without a need for inter-
> > tree dependencies.
> 
> IIRC (because that was a while ago already) having a Tegra-only display 
> framework is exactly what we wanted to avoid in the first place. This series 
> does nothing but leverage the callbacks mechanism that already exists in pwm-
> backlight and make it available to DT systems. If we start making a Tegra-
> specific solution, then other architectures will have to reinvent the wheel 
> again. I really don't think we want to go that way.
> 
> These patches only makes slight changes to pwm_bl.c and do not extend its 
> capabilities. I agree that a suitable solution will require the CDF, but by 
> the meantime, let's go for the practical route instead of repeating the same 
> mistakes (i.e. architecture-specific frameworks) again.
> 
> There are certainly better ways to do this, but I'm not convinced at all that 
> a Tegra-only solution is one of them.

Well, your proposal is a Tegra-only solution as well. Anything we come
up with now will be Tegra-only because it will eventually be integrated
with the CDF.

Trying to come up with something generic would be counter-productive.
CDF *is* the generic solution. All we would be doing is add a competing
framework.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Thierry Reding]

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

On Mon, Jan 21, 2013 at 05:18:11PM +0900, Alex Courbot wrote:
> Hi Thierry,
> 
> On Monday 21 January 2013 15:49:28 Thierry Reding wrote:
> > Eventually this should all be covered by the CDF, but since that's not
> > ready yet we want something ad-hoc to get the hardware supported. As
> > such I would like to see this go into some sort of minimalistic, Tegra-
> > specific display/panel framework. I'd prefer to keep the pwm-backlight
> > driver as simple and generic as possible, that is, a driver for a PWM-
> > controlled backlight.
> > 
> > Another advantage of moving this into a sort of display framework is
> > that it may help in defining the requirements for a CDF and that moving
> > the code to the CDF should be easier once it is done.
> > 
> > Last but not least, abstracting away the panel allows other things such
> > as physical dimensions and display modes to be properly encapsulated. I
> > think that power-on/off timing requirements for panels also belong to
> > this set since they are usually specific to a given panel.
> > 
> > Maybe adding these drivers to tegra-drm for now would be a good option.
> > That way the corresponding glue can be added without a need for inter-
> > tree dependencies.
> 
> IIRC (because that was a while ago already) having a Tegra-only display 
> framework is exactly what we wanted to avoid in the first place. This series 
> does nothing but leverage the callbacks mechanism that already exists in pwm-
> backlight and make it available to DT systems. If we start making a Tegra-
> specific solution, then other architectures will have to reinvent the wheel 
> again. I really don't think we want to go that way.
> 
> These patches only makes slight changes to pwm_bl.c and do not extend its 
> capabilities. I agree that a suitable solution will require the CDF, but by 
> the meantime, let's go for the practical route instead of repeating the same 
> mistakes (i.e. architecture-specific frameworks) again.
> 
> There are certainly better ways to do this, but I'm not convinced at all that 
> a Tegra-only solution is one of them.

Well, your proposal is a Tegra-only solution as well. Anything we come
up with now will be Tegra-only because it will eventually be integrated
with the CDF.

Trying to come up with something generic would be counter-productive.
CDF *is* the generic solution. All we would be doing is add a competing
framework.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] pwm-backlight: add subdrivers & Tegra support

[Thierry Reding]

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

On Mon, Jan 21, 2013 at 05:18:11PM +0900, Alex Courbot wrote:
> Hi Thierry,
> 
> On Monday 21 January 2013 15:49:28 Thierry Reding wrote:
> > Eventually this should all be covered by the CDF, but since that's not
> > ready yet we want something ad-hoc to get the hardware supported. As
> > such I would like to see this go into some sort of minimalistic, Tegra-
> > specific display/panel framework. I'd prefer to keep the pwm-backlight
> > driver as simple and generic as possible, that is, a driver for a PWM-
> > controlled backlight.
> > 
> > Another advantage of moving this into a sort of display framework is
> > that it may help in defining the requirements for a CDF and that moving
> > the code to the CDF should be easier once it is done.
> > 
> > Last but not least, abstracting away the panel allows other things such
> > as physical dimensions and display modes to be properly encapsulated. I
> > think that power-on/off timing requirements for panels also belong to
> > this set since they are usually specific to a given panel.
> > 
> > Maybe adding these drivers to tegra-drm for now would be a good option.
> > That way the corresponding glue can be added without a need for inter-
> > tree dependencies.
> 
> IIRC (because that was a while ago already) having a Tegra-only display 
> framework is exactly what we wanted to avoid in the first place. This series 
> does nothing but leverage the callbacks mechanism that already exists in pwm-
> backlight and make it available to DT systems. If we start making a Tegra-
> specific solution, then other architectures will have to reinvent the wheel 
> again. I really don't think we want to go that way.
> 
> These patches only makes slight changes to pwm_bl.c and do not extend its 
> capabilities. I agree that a suitable solution will require the CDF, but by 
> the meantime, let's go for the practical route instead of repeating the same 
> mistakes (i.e. architecture-specific frameworks) again.
> 
> There are certainly better ways to do this, but I'm not convinced at all that 
> a Tegra-only solution is one of them.

Well, your proposal is a Tegra-only solution as well. Anything we come
up with now will be Tegra-only because it will eventually be integrated
with the CDF.

Trying to come up with something generic would be counter-productive.
CDF *is* the generic solution. All we would be doing is add a competing
framework.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Stephen Warren]

On 01/21/2013 08:24 PM, Alex Courbot wrote:
> On Tuesday 22 January 2013 01:46:33 Stephen Warren wrote:
>>>  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
>>>  arch/arm/configs/tegra_defconfig       |   1 +
>>>  drivers/video/backlight/Kconfig        |   7 ++
>>>  drivers/video/backlight/pwm_bl.c       |   3 +
>>>  drivers/video/backlight/pwm_bl_tegra.c | 159

>>> +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
>>> +	.name = "pwm-backlight-ventana",
>>> +	.init = init_ventana,
>>> +	.exit = exit_ventana,
>>> +	.notify = notify_ventana,
>>> +	.notify_after = notify_after_ventana,
>>> +};
>>
>> It seems like all of that code should be completely generic.
> 
> Sorry, I don't get your point here - could you elaborate?

Nothing there (i.e. in the body of any of those functions) seems
remotely specific to Ventana or even Ventana's panel; presumably it
would work for any built-in LCD panel?

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Stephen Warren]

On 01/21/2013 08:24 PM, Alex Courbot wrote:
> On Tuesday 22 January 2013 01:46:33 Stephen Warren wrote:
>>>  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
>>>  arch/arm/configs/tegra_defconfig       |   1 +
>>>  drivers/video/backlight/Kconfig        |   7 ++
>>>  drivers/video/backlight/pwm_bl.c       |   3 +
>>>  drivers/video/backlight/pwm_bl_tegra.c | 159

>>> +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
>>> +	.name = "pwm-backlight-ventana",
>>> +	.init = init_ventana,
>>> +	.exit = exit_ventana,
>>> +	.notify = notify_ventana,
>>> +	.notify_after = notify_after_ventana,
>>> +};
>>
>> It seems like all of that code should be completely generic.
> 
> Sorry, I don't get your point here - could you elaborate?

Nothing there (i.e. in the body of any of those functions) seems
remotely specific to Ventana or even Ventana's panel; presumably it
would work for any built-in LCD panel?

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Stephen Warren]

On 01/21/2013 08:24 PM, Alex Courbot wrote:
> On Tuesday 22 January 2013 01:46:33 Stephen Warren wrote:
>>>  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
>>>  arch/arm/configs/tegra_defconfig       |   1 +
>>>  drivers/video/backlight/Kconfig        |   7 ++
>>>  drivers/video/backlight/pwm_bl.c       |   3 +
>>>  drivers/video/backlight/pwm_bl_tegra.c | 159

>>> +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
>>> +	.name = "pwm-backlight-ventana",
>>> +	.init = init_ventana,
>>> +	.exit = exit_ventana,
>>> +	.notify = notify_ventana,
>>> +	.notify_after = notify_after_ventana,
>>> +};
>>
>> It seems like all of that code should be completely generic.
> 
> Sorry, I don't get your point here - could you elaborate?

Nothing there (i.e. in the body of any of those functions) seems
remotely specific to Ventana or even Ventana's panel; presumably it
would work for any built-in LCD panel?

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alex Courbot]

> I'm confused. Why would you want to call into pwm_bl directly? If we're
> going to split this up into separate platform devices, why not look up a
> given backlight device and use the backlight API on that? The pieces of
> the puzzle are all there: you can use of_find_backlight_by_node() to
> obtain a backlight device from a device tree node, so I'd expect the DT
> to look something like this:
> 
> 	backlight: backlight {
> 		compatible = "pwm-backlight";
> 		...
> 	};

This would still prevent any power control from the backlight driver. I.e. if 
someone sets the brightness to 0 through sysfs, we cannot power the backlight 
off as pwm-backlight cannot control more than the PWM without platform 
callbacks. Backlight could only be powered off as a result of a fb blank event.

> 	panel: panel {
> 		compatible = "...";
> 		...
> 		backlight = <&backlight>;
> 		...
> 	};

So all the power control of both the panel and backlight would be performed 
from this device's driver. How would it plug into tegra-drm? I would see 
tegra_panel as a new member of the tegra_output structure, with one callback 
invoked from tegra_encoder_dpms(). Does that look sane?

> After that you can wire it up with host1x using something like:
> 
> 	host1x {
> 		dc@54200000 {
> 			rgb {
> 				status = "okay";
> 
> 				nvidia,panel = <&panel>;
> 			};
> 		};
> 	};

Indeed. So if we do that, the DRM DPMS functions would take care of the 
panel/backlight powering and the backlight driver will control the PWM after 
this, through the FB notifier. This is a little bit different from the "official" 
power sequence, but I just tested controlling the PWM at the very end of the 
sequence and it works just as well. If you think this looks better I don't 
mind doing it that way, it is actually a good excuse for me to dive into the 
DRM code.

Anyway, this will only be a temporary solution, CDF is the only way to do this 
right.

Alex.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alex Courbot]

> I'm confused. Why would you want to call into pwm_bl directly? If we're
> going to split this up into separate platform devices, why not look up a
> given backlight device and use the backlight API on that? The pieces of
> the puzzle are all there: you can use of_find_backlight_by_node() to
> obtain a backlight device from a device tree node, so I'd expect the DT
> to look something like this:
> 
> 	backlight: backlight {
> 		compatible = "pwm-backlight";
> 		...
> 	};

This would still prevent any power control from the backlight driver. I.e. if 
someone sets the brightness to 0 through sysfs, we cannot power the backlight 
off as pwm-backlight cannot control more than the PWM without platform 
callbacks. Backlight could only be powered off as a result of a fb blank event.

> 	panel: panel {
> 		compatible = "...";
> 		...
> 		backlight = <&backlight>;
> 		...
> 	};

So all the power control of both the panel and backlight would be performed 
from this device's driver. How would it plug into tegra-drm? I would see 
tegra_panel as a new member of the tegra_output structure, with one callback 
invoked from tegra_encoder_dpms(). Does that look sane?

> After that you can wire it up with host1x using something like:
> 
> 	host1x {
> 		dc@54200000 {
> 			rgb {
> 				status = "okay";
> 
> 				nvidia,panel = <&panel>;
> 			};
> 		};
> 	};

Indeed. So if we do that, the DRM DPMS functions would take care of the 
panel/backlight powering and the backlight driver will control the PWM after 
this, through the FB notifier. This is a little bit different from the "official" 
power sequence, but I just tested controlling the PWM at the very end of the 
sequence and it works just as well. If you think this looks better I don't 
mind doing it that way, it is actually a good excuse for me to dive into the 
DRM code.

Anyway, this will only be a temporary solution, CDF is the only way to do this 
right.

Alex.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Leela Krishna Amudala]

Hello Alex,

On Sat, Jan 19, 2013 at 4:00 PM, Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
> Add a PWM-backlight subdriver for Tegra boards, with support for
> Ventana.
>
> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
>  arch/arm/configs/tegra_defconfig       |   1 +
>  drivers/video/backlight/Kconfig        |   7 ++
>  drivers/video/backlight/pwm_bl.c       |   3 +
>  drivers/video/backlight/pwm_bl_tegra.c | 159
> +++++++++++++++++++++++++++++++++
>  5 files changed, 186 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
>
> diff --git a/arch/arm/boot/dts/tegra20-ventana.dts
> b/arch/arm/boot/dts/tegra20-ventana.dts
> index adc4754..a77b529 100644
> --- a/arch/arm/boot/dts/tegra20-ventana.dts
> +++ b/arch/arm/boot/dts/tegra20-ventana.dts
> @@ -516,6 +516,20 @@
>                 bus-width = <8>;
>         };
>
> +       backlight {
> +               compatible = "pwm-backlight-ventana";
> +               brightness-levels = <0 16 32 48 64 80 96 112 128 144 160
> 176 192 208 224 240 255>;
> +               default-brightness-level = <12>;
> +
> +               pwms = <&pwm 2 5000000>;
> +               pwm-names = "backlight";
> +
> +               power-supply = <&vdd_bl_reg>;
> +               panel-supply = <&vdd_pnl_reg>;
> +               bl-gpio = <&gpio 28 0>;
> +               bl-panel = <&gpio 10 0>;
> +       };
> +
>         regulators {
>                 compatible = "simple-bus";
>                 #address-cells = <1>;
> @@ -549,7 +563,7 @@
>                         enable-active-high;
>                 };
>
> -               regulator@3 {
> +               vdd_pnl_reg: regulator@3 {
>                         compatible = "regulator-fixed";
>                         reg = <3>;
>                         regulator-name = "vdd_pnl";
> @@ -559,7 +573,7 @@
>                         enable-active-high;
>                 };
>
> -               regulator@4 {
> +               vdd_bl_reg: regulator@4 {
>                         compatible = "regulator-fixed";
>                         reg = <4>;
>                         regulator-name = "vdd_bl";
> diff --git a/arch/arm/configs/tegra_defconfig
> b/arch/arm/configs/tegra_defconfig
> index a7827fd..1c46602 100644
> --- a/arch/arm/configs/tegra_defconfig
> +++ b/arch/arm/configs/tegra_defconfig
> @@ -150,6 +150,7 @@ CONFIG_BACKLIGHT_LCD_SUPPORT=y
>  CONFIG_BACKLIGHT_CLASS_DEVICE=y
>  # CONFIG_BACKLIGHT_GENERIC is not set
>  CONFIG_BACKLIGHT_PWM=y
> +CONFIG_BACKLIGHT_PWM_TEGRA=y
>  CONFIG_FRAMEBUFFER_CONSOLE=y
>  CONFIG_LOGO=y
>  CONFIG_SOUND=y
> diff --git a/drivers/video/backlight/Kconfig
> b/drivers/video/backlight/Kconfig
> index 765a945..377a409 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -244,6 +244,13 @@ config BACKLIGHT_PWM
>           If you have a LCD backlight adjustable by PWM, say Y to enable
>           this driver.
>
> +config BACKLIGHT_PWM_TEGRA
> +       bool "PWM Backlight Driver for Tegra boards"
> +       depends on BACKLIGHT_PWM && ARCH_TEGRA
> +       help
> +         Support backlight power sequencing for Tegra boards.
> +         Supported boards: Ventana.
> +
>  config BACKLIGHT_DA903X
>         tristate "Backlight Driver for DA9030/DA9034 using WLED"
>         depends on PMIC_DA903X
> diff --git a/drivers/video/backlight/pwm_bl.c
> b/drivers/video/backlight/pwm_bl.c
> index b65a797..1a4a9a3 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -217,6 +217,9 @@ static int pwm_backlight_parse_dt(struct device *dev,
>
>  static struct of_device_id pwm_backlight_of_match[] = {
>         { .compatible = "pwm-backlight" },
> +#ifdef CONFIG_BACKLIGHT_PWM_TEGRA
> +       { .compatible = "pwm-backlight-ventana" },
> +#endif
>         { }
>  };
>
> diff --git a/drivers/video/backlight/pwm_bl_tegra.c
> b/drivers/video/backlight/pwm_bl_tegra.c
> new file mode 100644
> index 0000000..8f2195b
> --- /dev/null
> +++ b/drivers/video/backlight/pwm_bl_tegra.c
> @@ -0,0 +1,159 @@
> +/*
> + * pwm-backlight subdriver for Tegra.
> + *
> + * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/pwm_backlight.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/list.h>
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +
> +struct ventana_bl_data {
> +       struct regulator *vdd_power;
> +       struct regulator *vdd_panel;
> +       int bl_gpio;
> +       int panel_gpio;
> +       bool is_on;
> +};
> +
> +static int init_ventana(struct device *dev)
> +{
> +       struct ventana_bl_data *data;
> +       int ret;
> +
> +       data = devm_kzalloc(dev, sizeof(struct ventana_bl_data),
> GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->vdd_power = devm_regulator_get(dev, "power");
> +       if (IS_ERR(data->vdd_power)) {
> +               dev_err(dev, "cannot get power regulator!\n");
> +               return PTR_ERR(data->vdd_power);
> +       }
> +
> +       data->vdd_panel = devm_regulator_get(dev, "panel");
> +       if (IS_ERR(data->vdd_panel)) {
> +               dev_err(dev, "cannot get panel regulator!\n");
> +               return PTR_ERR(data->vdd_panel);
> +       }
> +
> +       ret = of_get_named_gpio(dev->of_node, "bl-gpio", 0);
> +       if (ret < 0) {
> +               dev_err(dev, "cannot find backlight GPIO!\n");
> +               return ret;
> +       }
> +       data->bl_gpio = ret;
> +
> +       ret = of_get_named_gpio(dev->of_node, "bl-panel", 0);
> +       if (ret < 0) {
> +               dev_err(dev, "cannot find panel GPIO!\n");
> +               return ret;
> +       }
> +       data->panel_gpio = ret;
> +
> +       ret = devm_gpio_request_one(dev, data->bl_gpio,
> +                                   GPIOF_DIR_OUT | GPIOF_OUT_INIT_LOW,
> +                                   "backlight");
> +       if (ret < 0) {
> +               dev_err(dev, "cannot request backlight GPIO!\n");
> +               return ret;
> +       }
> +
> +       ret = devm_gpio_request_one(dev, data->panel_gpio,
> +                                   GPIOF_DIR_OUT | GPIOF_OUT_INIT_LOW,
> +                                   "panel");
> +       if (ret < 0) {
> +               dev_err(dev, "cannot request panel GPIO!\n");
> +               return ret;
> +       }
> +
> +       pwm_backlight_set_subdriver_data(dev, data);

Here you are passing ventana_bl_data pointer as input and in the
pwm_backlight_get_subdriver_data() function you are assigning the
received driver data to backlight_device pointer. As both are two
different structures with different structure fields in it. There can
be a chance for a crash.

Please correct me if I'm wrong.

Best Wishes,
Leela Krishna Amudala.

> +
> +       return 0;
> +}
> +
> +static void exit_ventana(struct device *dev)
> +{
> +       struct ventana_bl_data *data =
> pwm_backlight_get_subdriver_data(dev);
> +
> +       devm_gpio_free(dev, data->panel_gpio);
> +       devm_gpio_free(dev, data->bl_gpio);
> +       devm_regulator_put(data->vdd_panel);
> +       devm_regulator_put(data->vdd_power);
> +       devm_kfree(dev, data);
> +}
> +
> +static int notify_ventana(struct device *dev, int brightness)
> +{
> +       struct ventana_bl_data *data =
> pwm_backlight_get_subdriver_data(dev);
> +       if (brightness && !data->is_on) {
> +               regulator_enable(data->vdd_panel);
> +               gpio_set_value(data->panel_gpio, 1);
> +               usleep_range(200000, 200000);
> +               regulator_enable(data->vdd_power);
> +               usleep_range(10000, 10000);
> +       } else if (!brightness && data->is_on) {
> +               gpio_set_value(data->bl_gpio, 0);
> +       }
> +
> +       return brightness;
> +}
> +
> +static void notify_after_ventana(struct device *dev, int brightness)
> +{
> +       struct ventana_bl_data *data =
> pwm_backlight_get_subdriver_data(dev);
> +       if (brightness && !data->is_on) {
> +               gpio_set_value(data->bl_gpio, 1);
> +               data->is_on = true;
> +       } else if (!brightness && data->is_on) {
> +               usleep_range(10000, 10000);
> +               regulator_disable(data->vdd_power);
> +               usleep_range(200000, 200000);
> +               gpio_set_value(data->panel_gpio, 0);
> +               regulator_disable(data->vdd_panel);
> +               data->is_on = false;
> +       }
> +}
> +
> +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
> +       .name = "pwm-backlight-ventana",
> +       .init = init_ventana,
> +       .exit = exit_ventana,
> +       .notify = notify_ventana,
> +       .notify_after = notify_after_ventana,
> +};
> +
> +static int __init pwm_backlight_tegra_init(void)
> +{
> +       pwm_backlight_add_subdriver(&pwm_backlight_ventana_subdriver);
> +       return 0;
> +}
> +
> +static void __exit pwm_backlight_tegra_exit(void)
> +{
> +       pwm_backlight_remove_subdriver(&pwm_backlight_ventana_subdriver);
> +}
> +
> +module_init(pwm_backlight_tegra_init);
> +module_exit(pwm_backlight_tegra_exit);
> +
> +MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pwm-tegra-backlight");
> +
> +
> --
> 1.8.1.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Leela Krishna Amudala]

Hello Alex,

On Sat, Jan 19, 2013 at 4:00 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> Add a PWM-backlight subdriver for Tegra boards, with support for
> Ventana.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
>  arch/arm/configs/tegra_defconfig       |   1 +
>  drivers/video/backlight/Kconfig        |   7 ++
>  drivers/video/backlight/pwm_bl.c       |   3 +
>  drivers/video/backlight/pwm_bl_tegra.c | 159
> +++++++++++++++++++++++++++++++++
>  5 files changed, 186 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
>
> diff --git a/arch/arm/boot/dts/tegra20-ventana.dts
> b/arch/arm/boot/dts/tegra20-ventana.dts
> index adc4754..a77b529 100644
> --- a/arch/arm/boot/dts/tegra20-ventana.dts
> +++ b/arch/arm/boot/dts/tegra20-ventana.dts
> @@ -516,6 +516,20 @@
>                 bus-width = <8>;
>         };
>
> +       backlight {
> +               compatible = "pwm-backlight-ventana";
> +               brightness-levels = <0 16 32 48 64 80 96 112 128 144 160
> 176 192 208 224 240 255>;
> +               default-brightness-level = <12>;
> +
> +               pwms = <&pwm 2 5000000>;
> +               pwm-names = "backlight";
> +
> +               power-supply = <&vdd_bl_reg>;
> +               panel-supply = <&vdd_pnl_reg>;
> +               bl-gpio = <&gpio 28 0>;
> +               bl-panel = <&gpio 10 0>;
> +       };
> +
>         regulators {
>                 compatible = "simple-bus";
>                 #address-cells = <1>;
> @@ -549,7 +563,7 @@
>                         enable-active-high;
>                 };
>
> -               regulator@3 {
> +               vdd_pnl_reg: regulator@3 {
>                         compatible = "regulator-fixed";
>                         reg = <3>;
>                         regulator-name = "vdd_pnl";
> @@ -559,7 +573,7 @@
>                         enable-active-high;
>                 };
>
> -               regulator@4 {
> +               vdd_bl_reg: regulator@4 {
>                         compatible = "regulator-fixed";
>                         reg = <4>;
>                         regulator-name = "vdd_bl";
> diff --git a/arch/arm/configs/tegra_defconfig
> b/arch/arm/configs/tegra_defconfig
> index a7827fd..1c46602 100644
> --- a/arch/arm/configs/tegra_defconfig
> +++ b/arch/arm/configs/tegra_defconfig
> @@ -150,6 +150,7 @@ CONFIG_BACKLIGHT_LCD_SUPPORT=y
>  CONFIG_BACKLIGHT_CLASS_DEVICE=y
>  # CONFIG_BACKLIGHT_GENERIC is not set
>  CONFIG_BACKLIGHT_PWM=y
> +CONFIG_BACKLIGHT_PWM_TEGRA=y
>  CONFIG_FRAMEBUFFER_CONSOLE=y
>  CONFIG_LOGO=y
>  CONFIG_SOUND=y
> diff --git a/drivers/video/backlight/Kconfig
> b/drivers/video/backlight/Kconfig
> index 765a945..377a409 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -244,6 +244,13 @@ config BACKLIGHT_PWM
>           If you have a LCD backlight adjustable by PWM, say Y to enable
>           this driver.
>
> +config BACKLIGHT_PWM_TEGRA
> +       bool "PWM Backlight Driver for Tegra boards"
> +       depends on BACKLIGHT_PWM && ARCH_TEGRA
> +       help
> +         Support backlight power sequencing for Tegra boards.
> +         Supported boards: Ventana.
> +
>  config BACKLIGHT_DA903X
>         tristate "Backlight Driver for DA9030/DA9034 using WLED"
>         depends on PMIC_DA903X
> diff --git a/drivers/video/backlight/pwm_bl.c
> b/drivers/video/backlight/pwm_bl.c
> index b65a797..1a4a9a3 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -217,6 +217,9 @@ static int pwm_backlight_parse_dt(struct device *dev,
>
>  static struct of_device_id pwm_backlight_of_match[] = {
>         { .compatible = "pwm-backlight" },
> +#ifdef CONFIG_BACKLIGHT_PWM_TEGRA
> +       { .compatible = "pwm-backlight-ventana" },
> +#endif
>         { }
>  };
>
> diff --git a/drivers/video/backlight/pwm_bl_tegra.c
> b/drivers/video/backlight/pwm_bl_tegra.c
> new file mode 100644
> index 0000000..8f2195b
> --- /dev/null
> +++ b/drivers/video/backlight/pwm_bl_tegra.c
> @@ -0,0 +1,159 @@
> +/*
> + * pwm-backlight subdriver for Tegra.
> + *
> + * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/pwm_backlight.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/list.h>
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +
> +struct ventana_bl_data {
> +       struct regulator *vdd_power;
> +       struct regulator *vdd_panel;
> +       int bl_gpio;
> +       int panel_gpio;
> +       bool is_on;
> +};
> +
> +static int init_ventana(struct device *dev)
> +{
> +       struct ventana_bl_data *data;
> +       int ret;
> +
> +       data = devm_kzalloc(dev, sizeof(struct ventana_bl_data),
> GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->vdd_power = devm_regulator_get(dev, "power");
> +       if (IS_ERR(data->vdd_power)) {
> +               dev_err(dev, "cannot get power regulator!\n");
> +               return PTR_ERR(data->vdd_power);
> +       }
> +
> +       data->vdd_panel = devm_regulator_get(dev, "panel");
> +       if (IS_ERR(data->vdd_panel)) {
> +               dev_err(dev, "cannot get panel regulator!\n");
> +               return PTR_ERR(data->vdd_panel);
> +       }
> +
> +       ret = of_get_named_gpio(dev->of_node, "bl-gpio", 0);
> +       if (ret < 0) {
> +               dev_err(dev, "cannot find backlight GPIO!\n");
> +               return ret;
> +       }
> +       data->bl_gpio = ret;
> +
> +       ret = of_get_named_gpio(dev->of_node, "bl-panel", 0);
> +       if (ret < 0) {
> +               dev_err(dev, "cannot find panel GPIO!\n");
> +               return ret;
> +       }
> +       data->panel_gpio = ret;
> +
> +       ret = devm_gpio_request_one(dev, data->bl_gpio,
> +                                   GPIOF_DIR_OUT | GPIOF_OUT_INIT_LOW,
> +                                   "backlight");
> +       if (ret < 0) {
> +               dev_err(dev, "cannot request backlight GPIO!\n");
> +               return ret;
> +       }
> +
> +       ret = devm_gpio_request_one(dev, data->panel_gpio,
> +                                   GPIOF_DIR_OUT | GPIOF_OUT_INIT_LOW,
> +                                   "panel");
> +       if (ret < 0) {
> +               dev_err(dev, "cannot request panel GPIO!\n");
> +               return ret;
> +       }
> +
> +       pwm_backlight_set_subdriver_data(dev, data);

Here you are passing ventana_bl_data pointer as input and in the
pwm_backlight_get_subdriver_data() function you are assigning the
received driver data to backlight_device pointer. As both are two
different structures with different structure fields in it. There can
be a chance for a crash.

Please correct me if I'm wrong.

Best Wishes,
Leela Krishna Amudala.

> +
> +       return 0;
> +}
> +
> +static void exit_ventana(struct device *dev)
> +{
> +       struct ventana_bl_data *data =
> pwm_backlight_get_subdriver_data(dev);
> +
> +       devm_gpio_free(dev, data->panel_gpio);
> +       devm_gpio_free(dev, data->bl_gpio);
> +       devm_regulator_put(data->vdd_panel);
> +       devm_regulator_put(data->vdd_power);
> +       devm_kfree(dev, data);
> +}
> +
> +static int notify_ventana(struct device *dev, int brightness)
> +{
> +       struct ventana_bl_data *data =
> pwm_backlight_get_subdriver_data(dev);
> +       if (brightness && !data->is_on) {
> +               regulator_enable(data->vdd_panel);
> +               gpio_set_value(data->panel_gpio, 1);
> +               usleep_range(200000, 200000);
> +               regulator_enable(data->vdd_power);
> +               usleep_range(10000, 10000);
> +       } else if (!brightness && data->is_on) {
> +               gpio_set_value(data->bl_gpio, 0);
> +       }
> +
> +       return brightness;
> +}
> +
> +static void notify_after_ventana(struct device *dev, int brightness)
> +{
> +       struct ventana_bl_data *data =
> pwm_backlight_get_subdriver_data(dev);
> +       if (brightness && !data->is_on) {
> +               gpio_set_value(data->bl_gpio, 1);
> +               data->is_on = true;
> +       } else if (!brightness && data->is_on) {
> +               usleep_range(10000, 10000);
> +               regulator_disable(data->vdd_power);
> +               usleep_range(200000, 200000);
> +               gpio_set_value(data->panel_gpio, 0);
> +               regulator_disable(data->vdd_panel);
> +               data->is_on = false;
> +       }
> +}
> +
> +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
> +       .name = "pwm-backlight-ventana",
> +       .init = init_ventana,
> +       .exit = exit_ventana,
> +       .notify = notify_ventana,
> +       .notify_after = notify_after_ventana,
> +};
> +
> +static int __init pwm_backlight_tegra_init(void)
> +{
> +       pwm_backlight_add_subdriver(&pwm_backlight_ventana_subdriver);
> +       return 0;
> +}
> +
> +static void __exit pwm_backlight_tegra_exit(void)
> +{
> +       pwm_backlight_remove_subdriver(&pwm_backlight_ventana_subdriver);
> +}
> +
> +module_init(pwm_backlight_tegra_init);
> +module_exit(pwm_backlight_tegra_exit);
> +
> +MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pwm-tegra-backlight");
> +
> +
> --
> 1.8.1.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Leela Krishna Amudala]

Hello Alex,

On Sat, Jan 19, 2013 at 4:00 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> Add a PWM-backlight subdriver for Tegra boards, with support for
> Ventana.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
>  arch/arm/configs/tegra_defconfig       |   1 +
>  drivers/video/backlight/Kconfig        |   7 ++
>  drivers/video/backlight/pwm_bl.c       |   3 +
>  drivers/video/backlight/pwm_bl_tegra.c | 159
> +++++++++++++++++++++++++++++++++
>  5 files changed, 186 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
>
> diff --git a/arch/arm/boot/dts/tegra20-ventana.dts
> b/arch/arm/boot/dts/tegra20-ventana.dts
> index adc4754..a77b529 100644
> --- a/arch/arm/boot/dts/tegra20-ventana.dts
> +++ b/arch/arm/boot/dts/tegra20-ventana.dts
> @@ -516,6 +516,20 @@
>                 bus-width = <8>;
>         };
>
> +       backlight {
> +               compatible = "pwm-backlight-ventana";
> +               brightness-levels = <0 16 32 48 64 80 96 112 128 144 160
> 176 192 208 224 240 255>;
> +               default-brightness-level = <12>;
> +
> +               pwms = <&pwm 2 5000000>;
> +               pwm-names = "backlight";
> +
> +               power-supply = <&vdd_bl_reg>;
> +               panel-supply = <&vdd_pnl_reg>;
> +               bl-gpio = <&gpio 28 0>;
> +               bl-panel = <&gpio 10 0>;
> +       };
> +
>         regulators {
>                 compatible = "simple-bus";
>                 #address-cells = <1>;
> @@ -549,7 +563,7 @@
>                         enable-active-high;
>                 };
>
> -               regulator@3 {
> +               vdd_pnl_reg: regulator@3 {
>                         compatible = "regulator-fixed";
>                         reg = <3>;
>                         regulator-name = "vdd_pnl";
> @@ -559,7 +573,7 @@
>                         enable-active-high;
>                 };
>
> -               regulator@4 {
> +               vdd_bl_reg: regulator@4 {
>                         compatible = "regulator-fixed";
>                         reg = <4>;
>                         regulator-name = "vdd_bl";
> diff --git a/arch/arm/configs/tegra_defconfig
> b/arch/arm/configs/tegra_defconfig
> index a7827fd..1c46602 100644
> --- a/arch/arm/configs/tegra_defconfig
> +++ b/arch/arm/configs/tegra_defconfig
> @@ -150,6 +150,7 @@ CONFIG_BACKLIGHT_LCD_SUPPORT=y
>  CONFIG_BACKLIGHT_CLASS_DEVICE=y
>  # CONFIG_BACKLIGHT_GENERIC is not set
>  CONFIG_BACKLIGHT_PWM=y
> +CONFIG_BACKLIGHT_PWM_TEGRA=y
>  CONFIG_FRAMEBUFFER_CONSOLE=y
>  CONFIG_LOGO=y
>  CONFIG_SOUND=y
> diff --git a/drivers/video/backlight/Kconfig
> b/drivers/video/backlight/Kconfig
> index 765a945..377a409 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -244,6 +244,13 @@ config BACKLIGHT_PWM
>           If you have a LCD backlight adjustable by PWM, say Y to enable
>           this driver.
>
> +config BACKLIGHT_PWM_TEGRA
> +       bool "PWM Backlight Driver for Tegra boards"
> +       depends on BACKLIGHT_PWM && ARCH_TEGRA
> +       help
> +         Support backlight power sequencing for Tegra boards.
> +         Supported boards: Ventana.
> +
>  config BACKLIGHT_DA903X
>         tristate "Backlight Driver for DA9030/DA9034 using WLED"
>         depends on PMIC_DA903X
> diff --git a/drivers/video/backlight/pwm_bl.c
> b/drivers/video/backlight/pwm_bl.c
> index b65a797..1a4a9a3 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -217,6 +217,9 @@ static int pwm_backlight_parse_dt(struct device *dev,
>
>  static struct of_device_id pwm_backlight_of_match[] = {
>         { .compatible = "pwm-backlight" },
> +#ifdef CONFIG_BACKLIGHT_PWM_TEGRA
> +       { .compatible = "pwm-backlight-ventana" },
> +#endif
>         { }
>  };
>
> diff --git a/drivers/video/backlight/pwm_bl_tegra.c
> b/drivers/video/backlight/pwm_bl_tegra.c
> new file mode 100644
> index 0000000..8f2195b
> --- /dev/null
> +++ b/drivers/video/backlight/pwm_bl_tegra.c
> @@ -0,0 +1,159 @@
> +/*
> + * pwm-backlight subdriver for Tegra.
> + *
> + * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/pwm_backlight.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/list.h>
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +
> +struct ventana_bl_data {
> +       struct regulator *vdd_power;
> +       struct regulator *vdd_panel;
> +       int bl_gpio;
> +       int panel_gpio;
> +       bool is_on;
> +};
> +
> +static int init_ventana(struct device *dev)
> +{
> +       struct ventana_bl_data *data;
> +       int ret;
> +
> +       data = devm_kzalloc(dev, sizeof(struct ventana_bl_data),
> GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->vdd_power = devm_regulator_get(dev, "power");
> +       if (IS_ERR(data->vdd_power)) {
> +               dev_err(dev, "cannot get power regulator!\n");
> +               return PTR_ERR(data->vdd_power);
> +       }
> +
> +       data->vdd_panel = devm_regulator_get(dev, "panel");
> +       if (IS_ERR(data->vdd_panel)) {
> +               dev_err(dev, "cannot get panel regulator!\n");
> +               return PTR_ERR(data->vdd_panel);
> +       }
> +
> +       ret = of_get_named_gpio(dev->of_node, "bl-gpio", 0);
> +       if (ret < 0) {
> +               dev_err(dev, "cannot find backlight GPIO!\n");
> +               return ret;
> +       }
> +       data->bl_gpio = ret;
> +
> +       ret = of_get_named_gpio(dev->of_node, "bl-panel", 0);
> +       if (ret < 0) {
> +               dev_err(dev, "cannot find panel GPIO!\n");
> +               return ret;
> +       }
> +       data->panel_gpio = ret;
> +
> +       ret = devm_gpio_request_one(dev, data->bl_gpio,
> +                                   GPIOF_DIR_OUT | GPIOF_OUT_INIT_LOW,
> +                                   "backlight");
> +       if (ret < 0) {
> +               dev_err(dev, "cannot request backlight GPIO!\n");
> +               return ret;
> +       }
> +
> +       ret = devm_gpio_request_one(dev, data->panel_gpio,
> +                                   GPIOF_DIR_OUT | GPIOF_OUT_INIT_LOW,
> +                                   "panel");
> +       if (ret < 0) {
> +               dev_err(dev, "cannot request panel GPIO!\n");
> +               return ret;
> +       }
> +
> +       pwm_backlight_set_subdriver_data(dev, data);

Here you are passing ventana_bl_data pointer as input and in the
pwm_backlight_get_subdriver_data() function you are assigning the
received driver data to backlight_device pointer. As both are two
different structures with different structure fields in it. There can
be a chance for a crash.

Please correct me if I'm wrong.

Best Wishes,
Leela Krishna Amudala.

> +
> +       return 0;
> +}
> +
> +static void exit_ventana(struct device *dev)
> +{
> +       struct ventana_bl_data *data > pwm_backlight_get_subdriver_data(dev);
> +
> +       devm_gpio_free(dev, data->panel_gpio);
> +       devm_gpio_free(dev, data->bl_gpio);
> +       devm_regulator_put(data->vdd_panel);
> +       devm_regulator_put(data->vdd_power);
> +       devm_kfree(dev, data);
> +}
> +
> +static int notify_ventana(struct device *dev, int brightness)
> +{
> +       struct ventana_bl_data *data > pwm_backlight_get_subdriver_data(dev);
> +       if (brightness && !data->is_on) {
> +               regulator_enable(data->vdd_panel);
> +               gpio_set_value(data->panel_gpio, 1);
> +               usleep_range(200000, 200000);
> +               regulator_enable(data->vdd_power);
> +               usleep_range(10000, 10000);
> +       } else if (!brightness && data->is_on) {
> +               gpio_set_value(data->bl_gpio, 0);
> +       }
> +
> +       return brightness;
> +}
> +
> +static void notify_after_ventana(struct device *dev, int brightness)
> +{
> +       struct ventana_bl_data *data > pwm_backlight_get_subdriver_data(dev);
> +       if (brightness && !data->is_on) {
> +               gpio_set_value(data->bl_gpio, 1);
> +               data->is_on = true;
> +       } else if (!brightness && data->is_on) {
> +               usleep_range(10000, 10000);
> +               regulator_disable(data->vdd_power);
> +               usleep_range(200000, 200000);
> +               gpio_set_value(data->panel_gpio, 0);
> +               regulator_disable(data->vdd_panel);
> +               data->is_on = false;
> +       }
> +}
> +
> +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
> +       .name = "pwm-backlight-ventana",
> +       .init = init_ventana,
> +       .exit = exit_ventana,
> +       .notify = notify_ventana,
> +       .notify_after = notify_after_ventana,
> +};
> +
> +static int __init pwm_backlight_tegra_init(void)
> +{
> +       pwm_backlight_add_subdriver(&pwm_backlight_ventana_subdriver);
> +       return 0;
> +}
> +
> +static void __exit pwm_backlight_tegra_exit(void)
> +{
> +       pwm_backlight_remove_subdriver(&pwm_backlight_ventana_subdriver);
> +}
> +
> +module_init(pwm_backlight_tegra_init);
> +module_exit(pwm_backlight_tegra_exit);
> +
> +MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pwm-tegra-backlight");
> +
> +
> --
> 1.8.1.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alex Courbot]

On Wednesday 23 January 2013 18:15:30 Leela Krishna Amudala wrote:
> > +       pwm_backlight_set_subdriver_data(dev, data);
> 
> Here you are passing ventana_bl_data pointer as input and in the
> pwm_backlight_get_subdriver_data() function you are assigning the
> received driver data to backlight_device pointer. As both are two
> different structures with different structure fields in it. There can
> be a chance for a crash.

That's because the following happens later in pwm_backlight_probe():

	pb->subdriver_data = dev_get_drvdata(&pdev->dev);
	...
	bl = backlight_device_register(dev_name(&pdev->dev), &pdev->dev, pb,
				       &pwm_backlight_ops, &props);
	...
	platform_set_drvdata(pdev, bl);

So from then on the result of dev_get_drvdata() is indeed an instance of 
backlight_device from which we can retrieve the subdriver data. I'm not really 
proud of this. But fortunately it seems like we are going to do things 
differently.

Alex.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alex Courbot]

On Wednesday 23 January 2013 18:15:30 Leela Krishna Amudala wrote:
> > +       pwm_backlight_set_subdriver_data(dev, data);
> 
> Here you are passing ventana_bl_data pointer as input and in the
> pwm_backlight_get_subdriver_data() function you are assigning the
> received driver data to backlight_device pointer. As both are two
> different structures with different structure fields in it. There can
> be a chance for a crash.

That's because the following happens later in pwm_backlight_probe():

	pb->subdriver_data = dev_get_drvdata(&pdev->dev);
	...
	bl = backlight_device_register(dev_name(&pdev->dev), &pdev->dev, pb,
				       &pwm_backlight_ops, &props);
	...
	platform_set_drvdata(pdev, bl);

So from then on the result of dev_get_drvdata() is indeed an instance of 
backlight_device from which we can retrieve the subdriver data. I'm not really 
proud of this. But fortunately it seems like we are going to do things 
differently.

Alex.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alex Courbot]

On Wednesday 23 January 2013 18:15:30 Leela Krishna Amudala wrote:
> > +       pwm_backlight_set_subdriver_data(dev, data);
> 
> Here you are passing ventana_bl_data pointer as input and in the
> pwm_backlight_get_subdriver_data() function you are assigning the
> received driver data to backlight_device pointer. As both are two
> different structures with different structure fields in it. There can
> be a chance for a crash.

That's because the following happens later in pwm_backlight_probe():

	pb->subdriver_data = dev_get_drvdata(&pdev->dev);
	...
	bl = backlight_device_register(dev_name(&pdev->dev), &pdev->dev, pb,
				       &pwm_backlight_ops, &props);
	...
	platform_set_drvdata(pdev, bl);

So from then on the result of dev_get_drvdata() is indeed an instance of 
backlight_device from which we can retrieve the subdriver data. I'm not really 
proud of this. But fortunately it seems like we are going to do things 
differently.

Alex.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alex Courbot]

On Wednesday 23 January 2013 17:45:39 Alex Courbot wrote:
> > I'm confused. Why would you want to call into pwm_bl directly? If we're
> > going to split this up into separate platform devices, why not look up a
> > given backlight device and use the backlight API on that? The pieces of
> > the puzzle are all there: you can use of_find_backlight_by_node() to
> > obtain a backlight device from a device tree node, so I'd expect the DT
> > to look something like this:
> > 	backlight: backlight {
> > 	
> > 		compatible = "pwm-backlight";
> > 		...
> > 	
> > 	};
> 
> This would still prevent any power control from the backlight driver. I.e.
> if someone sets the brightness to 0 through sysfs, we cannot power the
> backlight off as pwm-backlight cannot control more than the PWM without
> platform callbacks. Backlight could only be powered off as a result of a fb
> blank event.

In order to solve this, how about adding a backlight notifier call chain to 
broadcast backlight events in a fashion similar to what is done in 
fbmem/fbcon? Then backlight_update_status() could send events like 
BACKLIGHT_EARLY_EVENT_UPDATE and BACKLIGHT_EVENT_UPDATE to which panel drivers 
could subscribe in order to power the backlight up and down as needed.

Then as the backlight is mentioned in the panel's DT node,

>       panel: panel {
>               compatible = "...";
>               ...
>               backlight = <&backlight>;
>               ...
>       };

the panel's driver could listen to backlight-related events and do its stuff 
transparently, without changing anything to the backlight drivers. This would 
be a good way to replace pwm-backlight's callbacks for platforms that use the 
DT, and would also be applicable to any backlight class device.

Generally speaking, having a mean to monitor backlights state in the kernel 
does not seem too crazy, especially since we already have a way to notify user 
space through backlight_generate_event().

Richard, does that sound ok to you?

Alex.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alex Courbot]

On Wednesday 23 January 2013 17:45:39 Alex Courbot wrote:
> > I'm confused. Why would you want to call into pwm_bl directly? If we're
> > going to split this up into separate platform devices, why not look up a
> > given backlight device and use the backlight API on that? The pieces of
> > the puzzle are all there: you can use of_find_backlight_by_node() to
> > obtain a backlight device from a device tree node, so I'd expect the DT
> > to look something like this:
> > 	backlight: backlight {
> > 	
> > 		compatible = "pwm-backlight";
> > 		...
> > 	
> > 	};
> 
> This would still prevent any power control from the backlight driver. I.e.
> if someone sets the brightness to 0 through sysfs, we cannot power the
> backlight off as pwm-backlight cannot control more than the PWM without
> platform callbacks. Backlight could only be powered off as a result of a fb
> blank event.

In order to solve this, how about adding a backlight notifier call chain to 
broadcast backlight events in a fashion similar to what is done in 
fbmem/fbcon? Then backlight_update_status() could send events like 
BACKLIGHT_EARLY_EVENT_UPDATE and BACKLIGHT_EVENT_UPDATE to which panel drivers 
could subscribe in order to power the backlight up and down as needed.

Then as the backlight is mentioned in the panel's DT node,

>       panel: panel {
>               compatible = "...";
>               ...
>               backlight = <&backlight>;
>               ...
>       };

the panel's driver could listen to backlight-related events and do its stuff 
transparently, without changing anything to the backlight drivers. This would 
be a good way to replace pwm-backlight's callbacks for platforms that use the 
DT, and would also be applicable to any backlight class device.

Generally speaking, having a mean to monitor backlights state in the kernel 
does not seem too crazy, especially since we already have a way to notify user 
space through backlight_generate_event().

Richard, does that sound ok to you?

Alex.

Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

[Alex Courbot]

On Wednesday 23 January 2013 17:45:39 Alex Courbot wrote:
> > I'm confused. Why would you want to call into pwm_bl directly? If we're
> > going to split this up into separate platform devices, why not look up a
> > given backlight device and use the backlight API on that? The pieces of
> > the puzzle are all there: you can use of_find_backlight_by_node() to
> > obtain a backlight device from a device tree node, so I'd expect the DT
> > to look something like this:
> > 	backlight: backlight {
> > 	
> > 		compatible = "pwm-backlight";
> > 		...
> > 	
> > 	};
> 
> This would still prevent any power control from the backlight driver. I.e.
> if someone sets the brightness to 0 through sysfs, we cannot power the
> backlight off as pwm-backlight cannot control more than the PWM without
> platform callbacks. Backlight could only be powered off as a result of a fb
> blank event.

In order to solve this, how about adding a backlight notifier call chain to 
broadcast backlight events in a fashion similar to what is done in 
fbmem/fbcon? Then backlight_update_status() could send events like 
BACKLIGHT_EARLY_EVENT_UPDATE and BACKLIGHT_EVENT_UPDATE to which panel drivers 
could subscribe in order to power the backlight up and down as needed.

Then as the backlight is mentioned in the panel's DT node,

>       panel: panel {
>               compatible = "...";
>               ...
>               backlight = <&backlight>;
>               ...
>       };

the panel's driver could listen to backlight-related events and do its stuff 
transparently, without changing anything to the backlight drivers. This would 
be a good way to replace pwm-backlight's callbacks for platforms that use the 
DT, and would also be applicable to any backlight class device.

Generally speaking, having a mean to monitor backlights state in the kernel 
does not seem too crazy, especially since we already have a way to notify user 
space through backlight_generate_event().

Richard, does that sound ok to you?

Alex.