[RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device tree support

[RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device tree support

[Thomas Abraham]

The platform-lcd driver depends on platform-specific callbacks to setup the
lcd panel. These callbacks are supplied using driver's platform data. But
for adding device tree support for platform-lcd driver, providing such
callbacks is not possible (without using auxdata).

Since the callbacks are usually lcd panel specific, it is possible to include
the lcd panel specific setup and control functionality in the platform-lcd
driver itself, thereby eliminating the need for supplying platform specific
callbacks to the driver. The platform-lcd driver can include support for
multiple lcd panels.

This patchset removes the need for platform data for platform-lcd driver and
adds support which can be used to implement lcd panel specific functionality
in the driver. As an example, the support for Hydis hv070wsa lcd panel is added
to the platform-lcd driver which is then used on the Exynos4 based Origen board.
This currently breaks build for other users of platform-lcd driver. Those can be
fixed if this approach is acceptable.

Thomas Abraham (4):
  lcd: platform-lcd: Eliminate need for platform data
  lcd: platform-lcd: Add support for Hydis hv070wsa lcd panel
  ARM: Exynos: Remove platform data of platform-lcd driver
  lcd: platform-lcd: Add device tree support

 arch/arm/mach-exynos/mach-origen.c     |   25 +-----
 drivers/video/backlight/Kconfig        |    6 ++
 drivers/video/backlight/platform_lcd.c |  128 ++++++++++++++++++++++++++++---
 include/video/platform_lcd.h           |   11 +--
 4 files changed, 129 insertions(+), 41 deletions(-)

[RFC][PATCH 1/4] lcd: platform-lcd: Eliminate need for platform data

[Thomas Abraham]

The platform data of platform-lcd driver includes pointers to platform specific
callbacks. These callbacks cannot be supported when adding device tree support.

Looking at all the usage of platform-lcd driver, the callbacks are mainly lcd
panel specific setup functions. Such setup functions can be moved into the
platform-lcd driver as lcd panel support, thereby not depending on platform
specific callbacks to be supplied as platform data.

For each of the lcd panel support that is added to the platform-lcd driver,
a set of panel specific callback functions need to be provided as driver
data. The platform-lcd driver uses these callback functions to setup and
control the lcd panel.

This patch eliminates the need for platform data and provides support for
adding lcd panel specific functionality.

Cc: Ben Dooks <ben-linux@fluff.org>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 drivers/video/backlight/platform_lcd.c |   46 +++++++++++++++++++++++---------
 include/video/platform_lcd.h           |    9 ------
 2 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/drivers/video/backlight/platform_lcd.c b/drivers/video/backlight/platform_lcd.c
index 302330a..707c81f 100644
--- a/drivers/video/backlight/platform_lcd.c
+++ b/drivers/video/backlight/platform_lcd.c
@@ -23,12 +23,20 @@
 struct platform_lcd {
 	struct device		*us;
 	struct lcd_device	*lcd;
-	struct plat_lcd_data	*pdata;
+	void				*lcd_pdata;
+	struct plat_lcd_driver_data	*drv_data;
 
 	unsigned int		 power;
 	unsigned int		 suspended : 1;
 };
 
+struct plat_lcd_driver_data {
+	int	(*lcd_init)(struct platform_lcd *);
+	void	(*lcd_deinit)(struct platform_lcd *);
+	void	(*set_power)(struct platform_lcd *, unsigned int power);
+	int	(*match_fb)(struct platform_lcd *, struct fb_info *);
+};
+
 static inline struct platform_lcd *to_our_lcd(struct lcd_device *lcd)
 {
 	return lcd_get_data(lcd);
@@ -49,7 +57,8 @@ static int platform_lcd_set_power(struct lcd_device *lcd, int power)
 	if (power == FB_BLANK_POWERDOWN || plcd->suspended)
 		lcd_power = 0;
 
-	plcd->pdata->set_power(plcd->pdata, lcd_power);
+	if (plcd->drv_data->set_power)
+		plcd->drv_data->set_power(plcd, lcd_power);
 	plcd->power = power;
 
 	return 0;
@@ -58,10 +67,9 @@ static int platform_lcd_set_power(struct lcd_device *lcd, int power)
 static int platform_lcd_match(struct lcd_device *lcd, struct fb_info *info)
 {
 	struct platform_lcd *plcd = to_our_lcd(lcd);
-	struct plat_lcd_data *pdata = plcd->pdata;
 
-	if (pdata->match_fb)
-		return pdata->match_fb(pdata, info);
+	if (plcd->drv_data->match_fb)
+		return plcd->drv_data->match_fb(plcd, info);
 
 	return plcd->us->parent == info->device;
 }
@@ -72,19 +80,19 @@ static struct lcd_ops platform_lcd_ops = {
 	.check_fb	= platform_lcd_match,
 };
 
+static inline struct plat_lcd_driver_data *platform_lcd_get_driver_data(
+			struct platform_device *pdev)
+{
+	return (struct plat_lcd_driver_data *)
+			platform_get_device_id(pdev)->driver_data;
+}
+
 static int __devinit platform_lcd_probe(struct platform_device *pdev)
 {
-	struct plat_lcd_data *pdata;
 	struct platform_lcd *plcd;
 	struct device *dev = &pdev->dev;
 	int err;
 
-	pdata = pdev->dev.platform_data;
-	if (!pdata) {
-		dev_err(dev, "no platform data supplied\n");
-		return -EINVAL;
-	}
-
 	plcd = kzalloc(sizeof(struct platform_lcd), GFP_KERNEL);
 	if (!plcd) {
 		dev_err(dev, "no memory for state\n");
@@ -92,7 +100,12 @@ static int __devinit platform_lcd_probe(struct platform_device *pdev)
 	}
 
 	plcd->us = dev;
-	plcd->pdata = pdata;
+	plcd->lcd_pdata = plcd->us->platform_data;
+	plcd->drv_data = platform_lcd_get_driver_data(pdev);
+	err = plcd->drv_data->lcd_init(plcd);
+	if (err)
+		goto err_mem;
+
 	plcd->lcd = lcd_device_register(dev_name(dev), dev,
 					plcd, &platform_lcd_ops);
 	if (IS_ERR(plcd->lcd)) {
@@ -116,6 +129,7 @@ static int __devexit platform_lcd_remove(struct platform_device *pdev)
 	struct platform_lcd *plcd = platform_get_drvdata(pdev);
 
 	lcd_device_unregister(plcd->lcd);
+	plcd->drv_data->lcd_deinit(plcd);
 	kfree(plcd);
 
 	return 0;
@@ -146,6 +160,11 @@ static int platform_lcd_resume(struct platform_device *pdev)
 #define platform_lcd_resume NULL
 #endif
 
+
+static struct platform_device_id platform_lcd_driver_ids[] = {
+	{ },
+};
+
 static struct platform_driver platform_lcd_driver = {
 	.driver		= {
 		.name	= "platform-lcd",
@@ -155,6 +174,7 @@ static struct platform_driver platform_lcd_driver = {
 	.remove		= __devexit_p(platform_lcd_remove),
 	.suspend        = platform_lcd_suspend,
 	.resume         = platform_lcd_resume,
+	.id_table	= platform_lcd_driver_ids,
 };
 
 static int __init platform_lcd_init(void)
diff --git a/include/video/platform_lcd.h b/include/video/platform_lcd.h
index ad3bdfe..8e6081a 100644
--- a/include/video/platform_lcd.h
+++ b/include/video/platform_lcd.h
@@ -10,12 +10,3 @@
  * published by the Free Software Foundation.
  *
 */
-
-struct plat_lcd_data;
-struct fb_info;
-
-struct plat_lcd_data {
-	void	(*set_power)(struct plat_lcd_data *, unsigned int power);
-	int	(*match_fb)(struct plat_lcd_data *, struct fb_info *);
-};
-
-- 
1.6.6.rc2

[RFC][PATCH 2/4] lcd: platform-lcd: Add support for Hydis hv070wsa lcd panel

[Thomas Abraham]

Add Hydis hv070wsa lcd panel setup and control support in platform-lcd driver.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 drivers/video/backlight/Kconfig        |    6 ++++
 drivers/video/backlight/platform_lcd.c |   49 ++++++++++++++++++++++++++++++++
 include/video/platform_lcd.h           |    6 ++++
 3 files changed, 61 insertions(+), 0 deletions(-)

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 278aeaa..ef4e9a7 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -86,6 +86,12 @@ config LCD_PLATFORM
 	  This driver provides a platform-device registered LCD power
 	  control interface.
 
+config LCD_HYDIS_HV070WSA
+	tristate "Hydis HV070WSA LCD"
+	depends on LCD_PLATFORM
+	help
+	  Enable support for Hydis HV070WSA lcd panel
+
 config LCD_TOSA
 	tristate "Sharp SL-6000 LCD Driver"
 	depends on SPI && MACH_TOSA
diff --git a/drivers/video/backlight/platform_lcd.c b/drivers/video/backlight/platform_lcd.c
index 707c81f..feb4fd0 100644
--- a/drivers/video/backlight/platform_lcd.c
+++ b/drivers/video/backlight/platform_lcd.c
@@ -17,6 +17,7 @@
 #include <linux/backlight.h>
 #include <linux/lcd.h>
 #include <linux/slab.h>
+#include <linux/gpio.h>
 
 #include <video/platform_lcd.h>
 
@@ -160,10 +161,58 @@ static int platform_lcd_resume(struct platform_device *pdev)
 #define platform_lcd_resume NULL
 #endif
 
+#ifdef CONFIG_LCD_HYDIS_HV070WSA
+static int lcd_hv070wsa_init(struct platform_lcd *plcd)
+{
+	struct plat_lcd_hydis_hv070wsa_pdata *pdata = plcd->lcd_pdata;
+	int err;
+
+	if (!pdata) {
+		dev_err(plcd->us, "no platform data\n");
+		return -EINVAL;
+	}
+
+	err = gpio_request(pdata->gpio, "HV070WSA_GPIO");
+	if (err) {
+		dev_err(plcd->us, "gpio request failed for %d\n", pdata->gpio);
+		return err;
+	}
+
+	return 0;
+}
+
+static void lcd_hv070wsa_deinit(struct platform_lcd *plcd)
+{
+	struct plat_lcd_hydis_hv070wsa_pdata *pdata = plcd->lcd_pdata;
+
+	gpio_free(pdata->gpio);
+}
+
+static void lcd_hv070wsa_set_power(struct platform_lcd *plcd, unsigned int pwr)
+{
+	struct plat_lcd_hydis_hv070wsa_pdata *pdata = plcd->lcd_pdata;
+
+	gpio_direction_output(pdata->gpio, pwr);
+}
+
+static struct plat_lcd_driver_data hv070wsa_driver_data = {
+	.lcd_init	= lcd_hv070wsa_init,
+	.lcd_deinit	= lcd_hv070wsa_deinit,
+	.set_power	= lcd_hv070wsa_set_power,
+};
+#define HV070WSA_DRV_DATA ((kernel_ulong_t)&hv070wsa_driver_data)
+#else
+#define HV070WSA_DRV_DATA (kernel_ulong_t)NULL
+#endif /* CONFIG_LCD_HYDIS_HV070WSA */
 
 static struct platform_device_id platform_lcd_driver_ids[] = {
+	{
+		.name		= "hv070wsa",
+		.driver_data	= HV070WSA_DRV_DATA,
+	},
 	{ },
 };
+MODULE_DEVICE_TABLE(platform, platform_lcd_driver_ids);
 
 static struct platform_driver platform_lcd_driver = {
 	.driver		= {
diff --git a/include/video/platform_lcd.h b/include/video/platform_lcd.h
index 8e6081a..d2fa188 100644
--- a/include/video/platform_lcd.h
+++ b/include/video/platform_lcd.h
@@ -10,3 +10,9 @@
  * published by the Free Software Foundation.
  *
 */
+
+#ifdef CONFIG_LCD_HYDIS_HV070WSA
+struct plat_lcd_hydis_hv070wsa_pdata {
+	unsigned int		gpio;
+};
+#endif
-- 
1.6.6.rc2

[RFC][PATCH 3/4] ARM: Exynos: Remove platform data of platform-lcd driver

[Thomas Abraham]

Remove the platform data for platform-lcd driver and add platform data for
Hydis hv070wsa lcd panel.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 arch/arm/mach-exynos/mach-origen.c |   25 ++++---------------------
 1 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-exynos/mach-origen.c b/arch/arm/mach-exynos/mach-origen.c
index 23fc5cb..0f08113 100644
--- a/arch/arm/mach-exynos/mach-origen.c
+++ b/arch/arm/mach-exynos/mach-origen.c
@@ -554,31 +554,14 @@ static struct platform_device origen_device_gpiokeys = {
 	},
 };
 
-static void lcd_hv070wsa_set_power(struct plat_lcd_data *pd, unsigned int power)
-{
-	int ret;
-
-	if (power)
-		ret = gpio_request_one(EXYNOS4_GPE3(4),
-					GPIOF_OUT_INIT_HIGH, "GPE3_4");
-	else
-		ret = gpio_request_one(EXYNOS4_GPE3(4),
-					GPIOF_OUT_INIT_LOW, "GPE3_4");
-
-	gpio_free(EXYNOS4_GPE3(4));
-
-	if (ret)
-		pr_err("failed to request gpio for LCD power: %d\n", ret);
-}
-
-static struct plat_lcd_data origen_lcd_hv070wsa_data = {
-	.set_power = lcd_hv070wsa_set_power,
+static struct plat_lcd_hydis_hv070wsa_pdata hv070wsa_pdata = {
+	.gpio		= EXYNOS4_GPE3(4),
 };
 
 static struct platform_device origen_lcd_hv070wsa = {
-	.name			= "platform-lcd",
+	.name			= "hv070wsa",
 	.dev.parent		= &s5p_device_fimd0.dev,
-	.dev.platform_data	= &origen_lcd_hv070wsa_data,
+	.dev.platform_data	= &hv070wsa_pdata,
 };
 
 static struct s3c_fb_pd_win origen_fb_win0 = {
-- 
1.6.6.rc2

[RFC][PATCH 4/4] lcd: platform-lcd: Add device tree support

[Thomas Abraham]

Add device tree based initialization for Hydis hv070wsa lcd panel.

Cc: Ben Dooks <ben-linux@fluff.org>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 drivers/video/backlight/platform_lcd.c |   33 ++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/video/backlight/platform_lcd.c b/drivers/video/backlight/platform_lcd.c
index feb4fd0..ebdddfd 100644
--- a/drivers/video/backlight/platform_lcd.c
+++ b/drivers/video/backlight/platform_lcd.c
@@ -18,6 +18,8 @@
 #include <linux/lcd.h>
 #include <linux/slab.h>
 #include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 
 #include <video/platform_lcd.h>
 
@@ -81,9 +83,18 @@ static struct lcd_ops platform_lcd_ops = {
 	.check_fb	= platform_lcd_match,
 };
 
+static const struct of_device_id platform_lcd_dt_match[];
+
 static inline struct plat_lcd_driver_data *platform_lcd_get_driver_data(
 			struct platform_device *pdev)
 {
+#ifdef CONFIG_OF
+	if (pdev->dev.of_node) {
+		const struct of_device_id *match;
+		match = of_match_node(platform_lcd_dt_match, pdev->dev.of_node);
+		return (struct plat_lcd_driver_data *)match->data;
+	}
+#endif
 	return (struct plat_lcd_driver_data *)
 			platform_get_device_id(pdev)->driver_data;
 }
@@ -167,6 +178,19 @@ static int lcd_hv070wsa_init(struct platform_lcd *plcd)
 	struct plat_lcd_hydis_hv070wsa_pdata *pdata = plcd->lcd_pdata;
 	int err;
 
+#ifdef CONFIG_OF
+	if (of_have_populated_dt()) {
+		plcd->lcd_pdata = devm_kzalloc(plcd->us,
+						sizeof(*pdata), GFP_KERNEL);
+		if (!plcd->lcd_pdata) {
+			dev_err(plcd->us, "mem alloc for pdata failed\n");
+			return -ENOMEM;
+		}
+		pdata = plcd->lcd_pdata;
+		pdata->gpio = of_get_gpio(plcd->us->of_node, 0);
+	}
+#endif
+
 	if (!pdata) {
 		dev_err(plcd->us, "no platform data\n");
 		return -EINVAL;
@@ -214,10 +238,19 @@ static struct platform_device_id platform_lcd_driver_ids[] = {
 };
 MODULE_DEVICE_TABLE(platform, platform_lcd_driver_ids);
 
+#ifdef CONFIG_OF
+static const struct of_device_id platform_lcd_dt_match[] = {
+	{ .compatible = "hydis,hv070wsa", .data = (void *)HV070WSA_DRV_DATA},
+	{},
+};
+MODULE_DEVICE_TABLE(of, platform_lcd_dt_match);
+#endif
+
 static struct platform_driver platform_lcd_driver = {
 	.driver		= {
 		.name	= "platform-lcd",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(platform_lcd_dt_match),
 	},
 	.probe		= platform_lcd_probe,
 	.remove		= __devexit_p(platform_lcd_remove),
-- 
1.6.6.rc2

Re: [RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device tree support

[Olof Johansson]

Hi,

On Sun, Jan 1, 2012 at 9:54 PM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
> The platform-lcd driver depends on platform-specific callbacks to setup the
> lcd panel. These callbacks are supplied using driver's platform data. But
> for adding device tree support for platform-lcd driver, providing such
> callbacks is not possible (without using auxdata).
>
> Since the callbacks are usually lcd panel specific, it is possible to include
> the lcd panel specific setup and control functionality in the platform-lcd
> driver itself, thereby eliminating the need for supplying platform specific
> callbacks to the driver. The platform-lcd driver can include support for
> multiple lcd panels.
>
> This patchset removes the need for platform data for platform-lcd driver and
> adds support which can be used to implement lcd panel specific functionality
> in the driver. As an example, the support for Hydis hv070wsa lcd panel is added
> to the platform-lcd driver which is then used on the Exynos4 based Origen board.
> This currently breaks build for other users of platform-lcd driver. Those can be
> fixed if this approach is acceptable.

I think it would be a better idea to provide a generic binding for the
simple LCD panels that just need one or two power GPIOs to be turned
on (some tend to need one to activate and one for power control --
second might in some cases be handled through a regulator I suppose).
I'm sure there will be exceptions to this; panels that have quirks or
requirements on various things that will either require an expanded
binding, or custom probe functions.

Especially in the case you are providing -- that lcd driver is only
using a single gpio, and it would be much more valuable to have a
trivial binding for this that uses shared code for this. Otherwise
every single simple panel will need to be added over time.


-Olof

Re: [RFC][PATCH 4/4] lcd: platform-lcd: Add device tree support

[Grant Likely]

On Mon, Jan 02, 2012 at 11:24:35AM +0530, Thomas Abraham wrote:
> Add device tree based initialization for Hydis hv070wsa lcd panel.
> 
> Cc: Ben Dooks <ben-linux@fluff.org>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  drivers/video/backlight/platform_lcd.c |   33 ++++++++++++++++++++++++++++++++
>  1 files changed, 33 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/backlight/platform_lcd.c b/drivers/video/backlight/platform_lcd.c
> index feb4fd0..ebdddfd 100644
> --- a/drivers/video/backlight/platform_lcd.c
> +++ b/drivers/video/backlight/platform_lcd.c
> @@ -18,6 +18,8 @@
>  #include <linux/lcd.h>
>  #include <linux/slab.h>
>  #include <linux/gpio.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
>  
>  #include <video/platform_lcd.h>
>  
> @@ -81,9 +83,18 @@ static struct lcd_ops platform_lcd_ops = {
>  	.check_fb	= platform_lcd_match,
>  };
>  
> +static const struct of_device_id platform_lcd_dt_match[];
> +
>  static inline struct plat_lcd_driver_data *platform_lcd_get_driver_data(
>  			struct platform_device *pdev)
>  {
> +#ifdef CONFIG_OF
> +	if (pdev->dev.of_node) {
> +		const struct of_device_id *match;
> +		match = of_match_node(platform_lcd_dt_match, pdev->dev.of_node);
> +		return (struct plat_lcd_driver_data *)match->data;

.data is already void*; the cast should not be necessary.

> +	}
> +#endif
>  	return (struct plat_lcd_driver_data *)
>  			platform_get_device_id(pdev)->driver_data;
>  }
> @@ -167,6 +178,19 @@ static int lcd_hv070wsa_init(struct platform_lcd *plcd)
>  	struct plat_lcd_hydis_hv070wsa_pdata *pdata = plcd->lcd_pdata;
>  	int err;
>  
> +#ifdef CONFIG_OF
> +	if (of_have_populated_dt()) {
> +		plcd->lcd_pdata = devm_kzalloc(plcd->us,
> +						sizeof(*pdata), GFP_KERNEL);
> +		if (!plcd->lcd_pdata) {
> +			dev_err(plcd->us, "mem alloc for pdata failed\n");
> +			return -ENOMEM;
> +		}
> +		pdata = plcd->lcd_pdata;
> +		pdata->gpio = of_get_gpio(plcd->us->of_node, 0);
> +	}
> +#endif
> +
>  	if (!pdata) {
>  		dev_err(plcd->us, "no platform data\n");
>  		return -EINVAL;
> @@ -214,10 +238,19 @@ static struct platform_device_id platform_lcd_driver_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(platform, platform_lcd_driver_ids);
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id platform_lcd_dt_match[] = {
> +	{ .compatible = "hydis,hv070wsa", .data = (void *)HV070WSA_DRV_DATA},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, platform_lcd_dt_match);
> +#endif
> +
>  static struct platform_driver platform_lcd_driver = {
>  	.driver		= {
>  		.name	= "platform-lcd",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = of_match_ptr(platform_lcd_dt_match),
>  	},
>  	.probe		= platform_lcd_probe,
>  	.remove		= __devexit_p(platform_lcd_remove),
> -- 
> 1.6.6.rc2
> 

Re: [RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device tree support

[Grant Likely]

On Mon, Jan 02, 2012 at 11:24:31AM +0530, Thomas Abraham wrote:
> The platform-lcd driver depends on platform-specific callbacks to setup the
> lcd panel. These callbacks are supplied using driver's platform data. But
> for adding device tree support for platform-lcd driver, providing such
> callbacks is not possible (without using auxdata).
> 
> Since the callbacks are usually lcd panel specific, it is possible to include
> the lcd panel specific setup and control functionality in the platform-lcd
> driver itself, thereby eliminating the need for supplying platform specific
> callbacks to the driver. The platform-lcd driver can include support for
> multiple lcd panels.
> 
> This patchset removes the need for platform data for platform-lcd driver and
> adds support which can be used to implement lcd panel specific functionality
> in the driver. As an example, the support for Hydis hv070wsa lcd panel is added
> to the platform-lcd driver which is then used on the Exynos4 based Origen board.
> This currently breaks build for other users of platform-lcd driver. Those can be
> fixed if this approach is acceptable.

Approach looks okay to me.  I've not looked too deeply though.

g.

> 
> Thomas Abraham (4):
>   lcd: platform-lcd: Eliminate need for platform data
>   lcd: platform-lcd: Add support for Hydis hv070wsa lcd panel
>   ARM: Exynos: Remove platform data of platform-lcd driver
>   lcd: platform-lcd: Add device tree support
> 
>  arch/arm/mach-exynos/mach-origen.c     |   25 +-----
>  drivers/video/backlight/Kconfig        |    6 ++
>  drivers/video/backlight/platform_lcd.c |  128 ++++++++++++++++++++++++++++---
>  include/video/platform_lcd.h           |   11 +--
>  4 files changed, 129 insertions(+), 41 deletions(-)
> 

Re: [RFC][PATCH 2/4] lcd: platform-lcd: Add support for Hydis hv070wsa lcd panel

[Kyungmin Park]

Hi,

It's not clear to add the panel data at platform_lcd. It's board
specific and it's used the gpio only.
So no need to specific it like HYDIS_HV070WSA. just define
PLATFORM_LCD_GPIO or similar.

How do you think?

Thank you,
Kyungmin Park

On 1/2/12, Thomas Abraham <thomas.abraham@linaro.org> wrote:
> Add Hydis hv070wsa lcd panel setup and control support in platform-lcd
> driver.
>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  drivers/video/backlight/Kconfig        |    6 ++++
>  drivers/video/backlight/platform_lcd.c |   49
> ++++++++++++++++++++++++++++++++
>  include/video/platform_lcd.h           |    6 ++++
>  3 files changed, 61 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/video/backlight/Kconfig
> b/drivers/video/backlight/Kconfig
> index 278aeaa..ef4e9a7 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -86,6 +86,12 @@ config LCD_PLATFORM
>  	  This driver provides a platform-device registered LCD power
>  	  control interface.
>
> +config LCD_HYDIS_HV070WSA
> +	tristate "Hydis HV070WSA LCD"
> +	depends on LCD_PLATFORM
> +	help
> +	  Enable support for Hydis HV070WSA lcd panel
> +
>  config LCD_TOSA
>  	tristate "Sharp SL-6000 LCD Driver"
>  	depends on SPI && MACH_TOSA
> diff --git a/drivers/video/backlight/platform_lcd.c
> b/drivers/video/backlight/platform_lcd.c
> index 707c81f..feb4fd0 100644
> --- a/drivers/video/backlight/platform_lcd.c
> +++ b/drivers/video/backlight/platform_lcd.c
> @@ -17,6 +17,7 @@
>  #include <linux/backlight.h>
>  #include <linux/lcd.h>
>  #include <linux/slab.h>
> +#include <linux/gpio.h>
>
>  #include <video/platform_lcd.h>
>
> @@ -160,10 +161,58 @@ static int platform_lcd_resume(struct platform_device
> *pdev)
>  #define platform_lcd_resume NULL
>  #endif
>
> +#ifdef CONFIG_LCD_HYDIS_HV070WSA
> +static int lcd_hv070wsa_init(struct platform_lcd *plcd)
> +{
> +	struct plat_lcd_hydis_hv070wsa_pdata *pdata = plcd->lcd_pdata;
> +	int err;
> +
> +	if (!pdata) {
> +		dev_err(plcd->us, "no platform data\n");
> +		return -EINVAL;
> +	}
> +
> +	err = gpio_request(pdata->gpio, "HV070WSA_GPIO");
> +	if (err) {
> +		dev_err(plcd->us, "gpio request failed for %d\n", pdata->gpio);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static void lcd_hv070wsa_deinit(struct platform_lcd *plcd)
> +{
> +	struct plat_lcd_hydis_hv070wsa_pdata *pdata = plcd->lcd_pdata;
> +
> +	gpio_free(pdata->gpio);
> +}
> +
> +static void lcd_hv070wsa_set_power(struct platform_lcd *plcd, unsigned int
> pwr)
> +{
> +	struct plat_lcd_hydis_hv070wsa_pdata *pdata = plcd->lcd_pdata;
> +
> +	gpio_direction_output(pdata->gpio, pwr);
> +}
> +
> +static struct plat_lcd_driver_data hv070wsa_driver_data = {
> +	.lcd_init	= lcd_hv070wsa_init,
> +	.lcd_deinit	= lcd_hv070wsa_deinit,
> +	.set_power	= lcd_hv070wsa_set_power,
> +};
> +#define HV070WSA_DRV_DATA ((kernel_ulong_t)&hv070wsa_driver_data)
> +#else
> +#define HV070WSA_DRV_DATA (kernel_ulong_t)NULL
> +#endif /* CONFIG_LCD_HYDIS_HV070WSA */
>
>  static struct platform_device_id platform_lcd_driver_ids[] = {
> +	{
> +		.name		= "hv070wsa",
> +		.driver_data	= HV070WSA_DRV_DATA,
> +	},
>  	{ },
>  };
> +MODULE_DEVICE_TABLE(platform, platform_lcd_driver_ids);
>
>  static struct platform_driver platform_lcd_driver = {
>  	.driver		= {
> diff --git a/include/video/platform_lcd.h b/include/video/platform_lcd.h
> index 8e6081a..d2fa188 100644
> --- a/include/video/platform_lcd.h
> +++ b/include/video/platform_lcd.h
> @@ -10,3 +10,9 @@
>   * published by the Free Software Foundation.
>   *
>  */
> +
> +#ifdef CONFIG_LCD_HYDIS_HV070WSA
> +struct plat_lcd_hydis_hv070wsa_pdata {
> +	unsigned int		gpio;
> +};
> +#endif
> --
> 1.6.6.rc2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC][PATCH 2/4] lcd: platform-lcd: Add support for Hydis hv070wsa lcd panel

[Mark Brown]

On Mon, Jan 02, 2012 at 11:24:33AM +0530, Thomas Abraham wrote:

> +static void lcd_hv070wsa_set_power(struct platform_lcd *plcd, unsigned int pwr)
> +{
> +	struct plat_lcd_hydis_hv070wsa_pdata *pdata = plcd->lcd_pdata;
> +
> +	gpio_direction_output(pdata->gpio, pwr);
> +}

This doesn't look at all specific to this panel - it's just setting a
GPIO - so it should probably just be a generic gpio-lcd driver (or
similar).  It ought to be possible to do a device tree binding for at
least this subset of panels.

Re: [RFC][PATCH 1/4] lcd: platform-lcd: Eliminate need for platform data

[Mark Brown]

On Mon, Jan 02, 2012 at 11:24:32AM +0530, Thomas Abraham wrote:

> @@ -10,12 +10,3 @@
>   * published by the Free Software Foundation.
>   *
>  */
> -
> -struct plat_lcd_data;
> -struct fb_info;
> -
> -struct plat_lcd_data {
> -	void	(*set_power)(struct plat_lcd_data *, unsigned int power);
> -	int	(*match_fb)(struct plat_lcd_data *, struct fb_info *);
> -};

This is going to break all existing users as it will stop them compiling
- they'd either need to be converted en masse to the new style as part
of the commit doing this or the driver would need to support both
platform data and new style configuration simultaneously.

Re: [RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device tree support

[Thomas Abraham]

Hi Olof,

On 2 January 2012 12:18, Olof Johansson <olof@lixom.net> wrote:
> Hi,
>
> On Sun, Jan 1, 2012 at 9:54 PM, Thomas Abraham
> <thomas.abraham@linaro.org> wrote:
>> The platform-lcd driver depends on platform-specific callbacks to setup the
>> lcd panel. These callbacks are supplied using driver's platform data. But
>> for adding device tree support for platform-lcd driver, providing such
>> callbacks is not possible (without using auxdata).
>>
>> Since the callbacks are usually lcd panel specific, it is possible to include
>> the lcd panel specific setup and control functionality in the platform-lcd
>> driver itself, thereby eliminating the need for supplying platform specific
>> callbacks to the driver. The platform-lcd driver can include support for
>> multiple lcd panels.
>>
>> This patchset removes the need for platform data for platform-lcd driver and
>> adds support which can be used to implement lcd panel specific functionality
>> in the driver. As an example, the support for Hydis hv070wsa lcd panel is added
>> to the platform-lcd driver which is then used on the Exynos4 based Origen board.
>> This currently breaks build for other users of platform-lcd driver. Those can be
>> fixed if this approach is acceptable.
>
> I think it would be a better idea to provide a generic binding for the
> simple LCD panels that just need one or two power GPIOs to be turned
> on (some tend to need one to activate and one for power control --
> second might in some cases be handled through a regulator I suppose).
> I'm sure there will be exceptions to this; panels that have quirks or
> requirements on various things that will either require an expanded
> binding, or custom probe functions.

A majority of the existing users of the platform-lcd driver are using
a single gpio to enable or disable the display. As you mentioned, a
generic binding can be added for such lcd panels. There are other
implementations that use one or more gpios but requires toggling those
gpios in a panel specific way (usually with some delay). Such
implementations will require custom panel support included in the
platfrom-lcd driver. I will prepare a patch for these cases.

>
> Especially in the case you are providing -- that lcd driver is only
> using a single gpio, and it would be much more valuable to have a
> trivial binding for this that uses shared code for this. Otherwise
> every single simple panel will need to be added over time.

Yes. I agree. It is better to have a common binding and there are many
panels that can use this generic binding. I will make these changes.

Thanks for your review.

Regards,
Thomas.

>
>
> -Olof

Re: [RFC][PATCH 4/4] lcd: platform-lcd: Add device tree support

[Thomas Abraham]

Hi Grant,

On 2 January 2012 13:04, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Jan 02, 2012 at 11:24:35AM +0530, Thomas Abraham wrote:
>> Add device tree based initialization for Hydis hv070wsa lcd panel.
>>
>> Cc: Ben Dooks <ben-linux@fluff.org>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>>  drivers/video/backlight/platform_lcd.c |   33 ++++++++++++++++++++++++++++++++
>>  1 files changed, 33 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/video/backlight/platform_lcd.c b/drivers/video/backlight/platform_lcd.c

[...]

>> +#ifdef CONFIG_OF
>> +     if (pdev->dev.of_node) {
>> +             const struct of_device_id *match;
>> +             match = of_match_node(platform_lcd_dt_match, pdev->dev.of_node);
>> +             return (struct plat_lcd_driver_data *)match->data;
>
> .data is already void*; the cast should not be necessary.

Thanks. I will remove the cast.

Regards,
Thomas.

[...]

Re: [RFC][PATCH 2/4] lcd: platform-lcd: Add support for Hydis hv070wsa lcd panel

[Thomas Abraham]

Dear Mr. Park,

On 2 January 2012 13:32, Kyungmin Park <kyungmin.park@samsung.com> wrote:
> Hi,
>
> It's not clear to add the panel data at platform_lcd. It's board
> specific and it's used the gpio only.
> So no need to specific it like HYDIS_HV070WSA. just define
> PLATFORM_LCD_GPIO or similar.
>
> How do you think?

Yes, I agree. Looking at other users of platform-lcd driver, it is
better to use config option as you mentioned since many lcd panels use
a single gpio for enable/disable of the panel. I will prepare a new
patch with these changes.

Thanks,
Thomas.

[...]

Re: [RFC][PATCH 2/4] lcd: platform-lcd: Add support for Hydis hv070wsa lcd panel

[Thomas Abraham]

Hi Mark,

On 2 January 2012 17:15, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Jan 02, 2012 at 11:24:33AM +0530, Thomas Abraham wrote:
>
>> +static void lcd_hv070wsa_set_power(struct platform_lcd *plcd, unsigned int pwr)
>> +{
>> +     struct plat_lcd_hydis_hv070wsa_pdata *pdata = plcd->lcd_pdata;
>> +
>> +     gpio_direction_output(pdata->gpio, pwr);
>> +}
>
> This doesn't look at all specific to this panel - it's just setting a
> GPIO - so it should probably just be a generic gpio-lcd driver (or
> similar).  It ought to be possible to do a device tree binding for at
> least this subset of panels.

Right. I should not have made it specific to hv070wsa panel. This type
of functionality is reusable for other lcd panels as well. I will
modify this patch.

Thanks,
Thomas.

Re: [RFC][PATCH 1/4] lcd: platform-lcd: Eliminate need for platform data

[Thomas Abraham]

Hi Mark,

On 2 January 2012 17:29, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Jan 02, 2012 at 11:24:32AM +0530, Thomas Abraham wrote:
>
>> @@ -10,12 +10,3 @@
>>   * published by the Free Software Foundation.
>>   *
>>  */
>> -
>> -struct plat_lcd_data;
>> -struct fb_info;
>> -
>> -struct plat_lcd_data {
>> -     void    (*set_power)(struct plat_lcd_data *, unsigned int power);
>> -     int     (*match_fb)(struct plat_lcd_data *, struct fb_info *);
>> -};
>
> This is going to break all existing users as it will stop them compiling
> - they'd either need to be converted en masse to the new style as part
> of the commit doing this or the driver would need to support both
> platform data and new style configuration simultaneously.

Yes, it is true that it will break other users and is mentioned it in
the cover letter of this patchset.

Your point on maintaining support for both old and new formats is
helpful. But I am not sure if maintaining support for both formats is
the right thing to do. As most of the ARM platforms will switch to
using dt, maybe converting all of them to new format would be better.
I will try doing this and prepare a patch.

Thanks,
Thomas.

Re: [RFC][PATCH 1/4] lcd: platform-lcd: Eliminate need for platform data

[Mark Brown]

On Mon, Jan 02, 2012 at 11:21:46PM +0530, Thomas Abraham wrote:

> Your point on maintaining support for both old and new formats is
> helpful. But I am not sure if maintaining support for both formats is
> the right thing to do. As most of the ARM platforms will switch to
> using dt, maybe converting all of them to new format would be better.
> I will try doing this and prepare a patch.

There's more architectures than ARM supported in Linux and only a
minority of them are using device tree at the minute.  Unless every
architecture switches over platform data based users will still need to
be supported.

Re: [RFC][PATCH 1/4] lcd: platform-lcd: Eliminate need for platform data

[Thomas Abraham]

On 2 January 2012 23:43, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Jan 02, 2012 at 11:21:46PM +0530, Thomas Abraham wrote:
>
>> Your point on maintaining support for both old and new formats is
>> helpful. But I am not sure if maintaining support for both formats is
>> the right thing to do. As most of the ARM platforms will switch to
>> using dt, maybe converting all of them to new format would be better.
>> I will try doing this and prepare a patch.
>
> There's more architectures than ARM supported in Linux and only a
> minority of them are using device tree at the minute.  Unless every
> architecture switches over platform data based users will still need to
> be supported.

I will try to retain the old platform data style also since there are
support for lcd panels in mainline that use 2 or more gpios in a panel
specific way. And it would be difficult to fold them into a generic
lcd type for lcd power control.

Thanks,
Thomas.

Re: [RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device tree support

[Lars-Peter Clausen]

On 01/02/2012 06:54 AM, Thomas Abraham wrote:
> The platform-lcd driver depends on platform-specific callbacks to setup the
> lcd panel. These callbacks are supplied using driver's platform data. But
> for adding device tree support for platform-lcd driver, providing such
> callbacks is not possible (without using auxdata).
> 
> Since the callbacks are usually lcd panel specific, it is possible to include
> the lcd panel specific setup and control functionality in the platform-lcd
> driver itself, thereby eliminating the need for supplying platform specific
> callbacks to the driver. The platform-lcd driver can include support for
> multiple lcd panels.
> 
> This patchset removes the need for platform data for platform-lcd driver and
> adds support which can be used to implement lcd panel specific functionality
> in the driver. As an example, the support for Hydis hv070wsa lcd panel is added
> to the platform-lcd driver which is then used on the Exynos4 based Origen board.
> This currently breaks build for other users of platform-lcd driver. Those can be
> fixed if this approach is acceptable.

The whole approach looks rather backwards to me. The exact purpose of the
platform_lcd driver is to redirect the lcd driver callbacks to board code.
So by removing this support you not only break all the existing driver but
also create a driver which does nothing. Then you add another layer of
abstraction to implement custom drivers in this driver. A better approach in
my opinion is to simply implement these drivers as first level LCD drivers.
So leave the platform-lcd driver as it is and just add a gpio (or maybe
regulator) lcd driver instead.

- Lars

Re: [RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device tree support

[Thomas Abraham]

Hi Lars,

On 3 January 2012 14:36, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 01/02/2012 06:54 AM, Thomas Abraham wrote:
>> The platform-lcd driver depends on platform-specific callbacks to setup the
>> lcd panel. These callbacks are supplied using driver's platform data. But
>> for adding device tree support for platform-lcd driver, providing such
>> callbacks is not possible (without using auxdata).
>>
>> Since the callbacks are usually lcd panel specific, it is possible to include
>> the lcd panel specific setup and control functionality in the platform-lcd
>> driver itself, thereby eliminating the need for supplying platform specific
>> callbacks to the driver. The platform-lcd driver can include support for
>> multiple lcd panels.
>>
>> This patchset removes the need for platform data for platform-lcd driver and
>> adds support which can be used to implement lcd panel specific functionality
>> in the driver. As an example, the support for Hydis hv070wsa lcd panel is added
>> to the platform-lcd driver which is then used on the Exynos4 based Origen board.
>> This currently breaks build for other users of platform-lcd driver. Those can be
>> fixed if this approach is acceptable.
>
> The whole approach looks rather backwards to me. The exact purpose of the
> platform_lcd driver is to redirect the lcd driver callbacks to board code.
> So by removing this support you not only break all the existing driver but
> also create a driver which does nothing. Then you add another layer of
> abstraction to implement custom drivers in this driver. A better approach in
> my opinion is to simply implement these drivers as first level LCD drivers.
> So leave the platform-lcd driver as it is and just add a gpio (or maybe
> regulator) lcd driver instead.

The existing platform-lcd driver mostly depends on the platform
callbacks for lcd panel power controls. Looking at the current users
of platform-lcd driver, a majority of the users of platform-lcd driver
use a gpio to enable/disable the display/power. The gpio is controlled
by the platform callbacks which the platform-lcd driver calls.

Hence, it is possible to extend the platform-lcd driver to include the
functionality of managing the gpio for lcd control. The platform code
is only expected to provide a gpio number to the platform-lcd driver.
This allows consolidation of the all the different platform callbacks
that use a gpio for simple enable/disable of the lcd display.

But there are other users of platform-lcd that do lot more than just
control a single gpio. For such cases, it is possible to reuse the
existing infrastructure of platform-lcd driver and extend it to handle
such lcd panel specific functionality.

The main advantages that I see here is the consolidation of platform
specific callbacks into the driver which inturn allows adding device
tree support without depending on platform data which have pointers to
platform specific functions. In the next version of this patchset, it
will be ensured that no platform breaks due to this change.

Would such a approach be useful and acceptable? Thanks for your
feedback and comments.

Regards,
Thomas.

>
> - Lars

Re: [RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device tree support

[Lars-Peter Clausen]

On 01/03/2012 12:54 PM, Thomas Abraham wrote:
> Hi Lars,
> 
> On 3 January 2012 14:36, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 01/02/2012 06:54 AM, Thomas Abraham wrote:
>>> The platform-lcd driver depends on platform-specific callbacks to setup the
>>> lcd panel. These callbacks are supplied using driver's platform data. But
>>> for adding device tree support for platform-lcd driver, providing such
>>> callbacks is not possible (without using auxdata).
>>>
>>> Since the callbacks are usually lcd panel specific, it is possible to include
>>> the lcd panel specific setup and control functionality in the platform-lcd
>>> driver itself, thereby eliminating the need for supplying platform specific
>>> callbacks to the driver. The platform-lcd driver can include support for
>>> multiple lcd panels.
>>>
>>> This patchset removes the need for platform data for platform-lcd driver and
>>> adds support which can be used to implement lcd panel specific functionality
>>> in the driver. As an example, the support for Hydis hv070wsa lcd panel is added
>>> to the platform-lcd driver which is then used on the Exynos4 based Origen board.
>>> This currently breaks build for other users of platform-lcd driver. Those can be
>>> fixed if this approach is acceptable.
>>
>> The whole approach looks rather backwards to me. The exact purpose of the
>> platform_lcd driver is to redirect the lcd driver callbacks to board code.
>> So by removing this support you not only break all the existing driver but
>> also create a driver which does nothing. Then you add another layer of
>> abstraction to implement custom drivers in this driver. A better approach in
>> my opinion is to simply implement these drivers as first level LCD drivers.
>> So leave the platform-lcd driver as it is and just add a gpio (or maybe
>> regulator) lcd driver instead.
> 
> The existing platform-lcd driver mostly depends on the platform
> callbacks for lcd panel power controls. Looking at the current users
> of platform-lcd driver, a majority of the users of platform-lcd driver
> use a gpio to enable/disable the display/power. The gpio is controlled
> by the platform callbacks which the platform-lcd driver calls.
> 
> Hence, it is possible to extend the platform-lcd driver to include the
> functionality of managing the gpio for lcd control. The platform code
> is only expected to provide a gpio number to the platform-lcd driver.
> This allows consolidation of the all the different platform callbacks
> that use a gpio for simple enable/disable of the lcd display.
> 
> But there are other users of platform-lcd that do lot more than just
> control a single gpio. For such cases, it is possible to reuse the
> existing infrastructure of platform-lcd driver and extend it to handle
> such lcd panel specific functionality.
> 
> The main advantages that I see here is the consolidation of platform
> specific callbacks into the driver which inturn allows adding device
> tree support without depending on platform data which have pointers to
> platform specific functions. In the next version of this patchset, it
> will be ensured that no platform breaks due to this change.

Consolidation of the different implementations which use a GPIO to control
the LCD state is a good idea. But as I wrote above in this series you added
more or less another layer of abstraction. Namely introducing the
platform-lcd driver as a intermediate layer between the actual driver and
the LCD framework. But the layer is so thin that all it does is to call the
plat_lcd_driver_data callback from the lcd_ops callback. There is really no
point in doing this since you can setup the lcd_ops callbacks directly. Also
following your approach we would end up with a bunch of unrelated LCD
drivers in the platform-lcd driver module. The platform-lcd driver is so
generic that basically any LCD driver could be implemented on-top of it, but
this does not mean it has to.

As said before, writing a plain LCD driver which implements the GPIO
handling and leaving the platform-lcd driver as it is, is in my opinion a
better approach.

Re: [RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device tree support

[Thomas Abraham]

On 3 January 2012 17:56, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 01/03/2012 12:54 PM, Thomas Abraham wrote:
>> Hi Lars,
>>
>> On 3 January 2012 14:36, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> On 01/02/2012 06:54 AM, Thomas Abraham wrote:
>>>> The platform-lcd driver depends on platform-specific callbacks to setup the
>>>> lcd panel. These callbacks are supplied using driver's platform data. But
>>>> for adding device tree support for platform-lcd driver, providing such
>>>> callbacks is not possible (without using auxdata).
>>>>
>>>> Since the callbacks are usually lcd panel specific, it is possible to include
>>>> the lcd panel specific setup and control functionality in the platform-lcd
>>>> driver itself, thereby eliminating the need for supplying platform specific
>>>> callbacks to the driver. The platform-lcd driver can include support for
>>>> multiple lcd panels.
>>>>
>>>> This patchset removes the need for platform data for platform-lcd driver and
>>>> adds support which can be used to implement lcd panel specific functionality
>>>> in the driver. As an example, the support for Hydis hv070wsa lcd panel is added
>>>> to the platform-lcd driver which is then used on the Exynos4 based Origen board.
>>>> This currently breaks build for other users of platform-lcd driver. Those can be
>>>> fixed if this approach is acceptable.
>>>
>>> The whole approach looks rather backwards to me. The exact purpose of the
>>> platform_lcd driver is to redirect the lcd driver callbacks to board code.
>>> So by removing this support you not only break all the existing driver but
>>> also create a driver which does nothing. Then you add another layer of
>>> abstraction to implement custom drivers in this driver. A better approach in
>>> my opinion is to simply implement these drivers as first level LCD drivers.
>>> So leave the platform-lcd driver as it is and just add a gpio (or maybe
>>> regulator) lcd driver instead.
>>
>> The existing platform-lcd driver mostly depends on the platform
>> callbacks for lcd panel power controls. Looking at the current users
>> of platform-lcd driver, a majority of the users of platform-lcd driver
>> use a gpio to enable/disable the display/power. The gpio is controlled
>> by the platform callbacks which the platform-lcd driver calls.
>>
>> Hence, it is possible to extend the platform-lcd driver to include the
>> functionality of managing the gpio for lcd control. The platform code
>> is only expected to provide a gpio number to the platform-lcd driver.
>> This allows consolidation of the all the different platform callbacks
>> that use a gpio for simple enable/disable of the lcd display.
>>
>> But there are other users of platform-lcd that do lot more than just
>> control a single gpio. For such cases, it is possible to reuse the
>> existing infrastructure of platform-lcd driver and extend it to handle
>> such lcd panel specific functionality.
>>
>> The main advantages that I see here is the consolidation of platform
>> specific callbacks into the driver which inturn allows adding device
>> tree support without depending on platform data which have pointers to
>> platform specific functions. In the next version of this patchset, it
>> will be ensured that no platform breaks due to this change.
>
> Consolidation of the different implementations which use a GPIO to control
> the LCD state is a good idea. But as I wrote above in this series you added
> more or less another layer of abstraction. Namely introducing the
> platform-lcd driver as a intermediate layer between the actual driver and
> the LCD framework. But the layer is so thin that all it does is to call the
> plat_lcd_driver_data callback from the lcd_ops callback. There is really no
> point in doing this since you can setup the lcd_ops callbacks directly. Also
> following your approach we would end up with a bunch of unrelated LCD
> drivers in the platform-lcd driver module. The platform-lcd driver is so
> generic that basically any LCD driver could be implemented on-top of it, but
> this does not mean it has to.

Yes, this will lead to including support for multiple lcd panels in
the platform-lcd driver. But, we get to reuse most of the
infrastruture in the platform-lcd driver such as module init/cleanup,
suspend/resume, probe and lcd driver registration. There is a plan to
include regulator support in the platform-lcd driver as well. If we go
for independent drivers for all existing users of platform-lcd driver,
then this common code in platform-lcd driver will have to be
duplicated in multiple new drivers.

What would be your suggestion considering this. Actually, I don't
clearly understand the technical/maintainability reasons which do not
favour including support for multiple types of simple lcd panels in
the platform-lcd driver.

>
> As said before, writing a plain LCD driver which implements the GPIO
> handling and leaving the platform-lcd driver as it is, is in my opinion a
> better approach.

Thanks,
Thomas.

Re: [RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device tree support

[Lars-Peter Clausen]

On 01/03/2012 06:07 PM, Thomas Abraham wrote:
> On 3 January 2012 17:56, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 01/03/2012 12:54 PM, Thomas Abraham wrote:
>>> Hi Lars,
>>>
>>> On 3 January 2012 14:36, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>> On 01/02/2012 06:54 AM, Thomas Abraham wrote:
>>>>> The platform-lcd driver depends on platform-specific callbacks to setup the
>>>>> lcd panel. These callbacks are supplied using driver's platform data. But
>>>>> for adding device tree support for platform-lcd driver, providing such
>>>>> callbacks is not possible (without using auxdata).
>>>>>
>>>>> Since the callbacks are usually lcd panel specific, it is possible to include
>>>>> the lcd panel specific setup and control functionality in the platform-lcd
>>>>> driver itself, thereby eliminating the need for supplying platform specific
>>>>> callbacks to the driver. The platform-lcd driver can include support for
>>>>> multiple lcd panels.
>>>>>
>>>>> This patchset removes the need for platform data for platform-lcd driver and
>>>>> adds support which can be used to implement lcd panel specific functionality
>>>>> in the driver. As an example, the support for Hydis hv070wsa lcd panel is added
>>>>> to the platform-lcd driver which is then used on the Exynos4 based Origen board.
>>>>> This currently breaks build for other users of platform-lcd driver. Those can be
>>>>> fixed if this approach is acceptable.
>>>>
>>>> The whole approach looks rather backwards to me. The exact purpose of the
>>>> platform_lcd driver is to redirect the lcd driver callbacks to board code.
>>>> So by removing this support you not only break all the existing driver but
>>>> also create a driver which does nothing. Then you add another layer of
>>>> abstraction to implement custom drivers in this driver. A better approach in
>>>> my opinion is to simply implement these drivers as first level LCD drivers.
>>>> So leave the platform-lcd driver as it is and just add a gpio (or maybe
>>>> regulator) lcd driver instead.
>>>
>>> The existing platform-lcd driver mostly depends on the platform
>>> callbacks for lcd panel power controls. Looking at the current users
>>> of platform-lcd driver, a majority of the users of platform-lcd driver
>>> use a gpio to enable/disable the display/power. The gpio is controlled
>>> by the platform callbacks which the platform-lcd driver calls.
>>>
>>> Hence, it is possible to extend the platform-lcd driver to include the
>>> functionality of managing the gpio for lcd control. The platform code
>>> is only expected to provide a gpio number to the platform-lcd driver.
>>> This allows consolidation of the all the different platform callbacks
>>> that use a gpio for simple enable/disable of the lcd display.
>>>
>>> But there are other users of platform-lcd that do lot more than just
>>> control a single gpio. For such cases, it is possible to reuse the
>>> existing infrastructure of platform-lcd driver and extend it to handle
>>> such lcd panel specific functionality.
>>>
>>> The main advantages that I see here is the consolidation of platform
>>> specific callbacks into the driver which inturn allows adding device
>>> tree support without depending on platform data which have pointers to
>>> platform specific functions. In the next version of this patchset, it
>>> will be ensured that no platform breaks due to this change.
>>
>> Consolidation of the different implementations which use a GPIO to control
>> the LCD state is a good idea. But as I wrote above in this series you added
>> more or less another layer of abstraction. Namely introducing the
>> platform-lcd driver as a intermediate layer between the actual driver and
>> the LCD framework. But the layer is so thin that all it does is to call the
>> plat_lcd_driver_data callback from the lcd_ops callback. There is really no
>> point in doing this since you can setup the lcd_ops callbacks directly. Also
>> following your approach we would end up with a bunch of unrelated LCD
>> drivers in the platform-lcd driver module. The platform-lcd driver is so
>> generic that basically any LCD driver could be implemented on-top of it, but
>> this does not mean it has to.
> 
> Yes, this will lead to including support for multiple lcd panels in
> the platform-lcd driver. But, we get to reuse most of the
> infrastruture in the platform-lcd driver such as module init/cleanup,
> suspend/resume, probe and lcd driver registration. There is a plan to
> include regulator support in the platform-lcd driver as well. If we go
> for independent drivers for all existing users of platform-lcd driver,
> then this common code in platform-lcd driver will have to be
> duplicated in multiple new drivers.

Most of the infrastructure code is driver boiler-plate code. And you'll add
about the same amount of code due to your additional layer redirection. You'll
still have a probe and remove function per driver. You'll have to define a set
of plat_lcd_driver_data per driver. You'll have all these ugly ifdefs.

An exception is the suspend/resume handling. But this does not look like it is
something which is specific to simple displays, more "complex" ones or
non-platform driver lcd drivers might want to use a similar scheme. A good idea
would be to add support for this to the LCD framework. Similar to the backlight
frameworks BL_CORE_SUSPENDRESUME.

> 
> What would be your suggestion considering this. Actually, I don't
> clearly understand the technical/maintainability reasons which do not
> favour including support for multiple types of simple lcd panels in
> the platform-lcd driver.

To me it seems very unidiomatic to put multiple drivers into the same driver.
E.g. where will we draw the line, which kind of device is simple enough so
support for this device should be embedded in the plaform-lcd driver. Once you
have support for multiple types of simple lcd panels in one driver things will
start to get messy. Image how the driver will look like if it has support for
say 5 or 10 different types of simple lcd panels.

- Lars

Re: [RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device tree support

[Thomas Abraham]

Hi Lars,

On 4 January 2012 00:06, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 01/03/2012 06:07 PM, Thomas Abraham wrote:
>> On 3 January 2012 17:56, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> On 01/03/2012 12:54 PM, Thomas Abraham wrote:
>>>> Hi Lars,
>>>>
>>>> On 3 January 2012 14:36, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>>> On 01/02/2012 06:54 AM, Thomas Abraham wrote:
>>>>>> The platform-lcd driver depends on platform-specific callbacks to setup the
>>>>>> lcd panel. These callbacks are supplied using driver's platform data. But
>>>>>> for adding device tree support for platform-lcd driver, providing such
>>>>>> callbacks is not possible (without using auxdata).
>>>>>>
>>>>>> Since the callbacks are usually lcd panel specific, it is possible to include
>>>>>> the lcd panel specific setup and control functionality in the platform-lcd
>>>>>> driver itself, thereby eliminating the need for supplying platform specific
>>>>>> callbacks to the driver. The platform-lcd driver can include support for
>>>>>> multiple lcd panels.
>>>>>>
>>>>>> This patchset removes the need for platform data for platform-lcd driver and
>>>>>> adds support which can be used to implement lcd panel specific functionality
>>>>>> in the driver. As an example, the support for Hydis hv070wsa lcd panel is added
>>>>>> to the platform-lcd driver which is then used on the Exynos4 based Origen board.
>>>>>> This currently breaks build for other users of platform-lcd driver. Those can be
>>>>>> fixed if this approach is acceptable.
>>>>>
>>>>> The whole approach looks rather backwards to me. The exact purpose of the
>>>>> platform_lcd driver is to redirect the lcd driver callbacks to board code.
>>>>> So by removing this support you not only break all the existing driver but
>>>>> also create a driver which does nothing. Then you add another layer of
>>>>> abstraction to implement custom drivers in this driver. A better approach in
>>>>> my opinion is to simply implement these drivers as first level LCD drivers.
>>>>> So leave the platform-lcd driver as it is and just add a gpio (or maybe
>>>>> regulator) lcd driver instead.
>>>>
>>>> The existing platform-lcd driver mostly depends on the platform
>>>> callbacks for lcd panel power controls. Looking at the current users
>>>> of platform-lcd driver, a majority of the users of platform-lcd driver
>>>> use a gpio to enable/disable the display/power. The gpio is controlled
>>>> by the platform callbacks which the platform-lcd driver calls.
>>>>
>>>> Hence, it is possible to extend the platform-lcd driver to include the
>>>> functionality of managing the gpio for lcd control. The platform code
>>>> is only expected to provide a gpio number to the platform-lcd driver.
>>>> This allows consolidation of the all the different platform callbacks
>>>> that use a gpio for simple enable/disable of the lcd display.
>>>>
>>>> But there are other users of platform-lcd that do lot more than just
>>>> control a single gpio. For such cases, it is possible to reuse the
>>>> existing infrastructure of platform-lcd driver and extend it to handle
>>>> such lcd panel specific functionality.
>>>>
>>>> The main advantages that I see here is the consolidation of platform
>>>> specific callbacks into the driver which inturn allows adding device
>>>> tree support without depending on platform data which have pointers to
>>>> platform specific functions. In the next version of this patchset, it
>>>> will be ensured that no platform breaks due to this change.
>>>
>>> Consolidation of the different implementations which use a GPIO to control
>>> the LCD state is a good idea. But as I wrote above in this series you added
>>> more or less another layer of abstraction. Namely introducing the
>>> platform-lcd driver as a intermediate layer between the actual driver and
>>> the LCD framework. But the layer is so thin that all it does is to call the
>>> plat_lcd_driver_data callback from the lcd_ops callback. There is really no
>>> point in doing this since you can setup the lcd_ops callbacks directly. Also
>>> following your approach we would end up with a bunch of unrelated LCD
>>> drivers in the platform-lcd driver module. The platform-lcd driver is so
>>> generic that basically any LCD driver could be implemented on-top of it, but
>>> this does not mean it has to.
>>
>> Yes, this will lead to including support for multiple lcd panels in
>> the platform-lcd driver. But, we get to reuse most of the
>> infrastruture in the platform-lcd driver such as module init/cleanup,
>> suspend/resume, probe and lcd driver registration. There is a plan to
>> include regulator support in the platform-lcd driver as well. If we go
>> for independent drivers for all existing users of platform-lcd driver,
>> then this common code in platform-lcd driver will have to be
>> duplicated in multiple new drivers.
>
> Most of the infrastructure code is driver boiler-plate code. And you'll add
> about the same amount of code due to your additional layer redirection. You'll
> still have a probe and remove function per driver. You'll have to define a set
> of plat_lcd_driver_data per driver. You'll have all these ugly ifdefs.
>
> An exception is the suspend/resume handling. But this does not look like it is
> something which is specific to simple displays, more "complex" ones or
> non-platform driver lcd drivers might want to use a similar scheme. A good idea
> would be to add support for this to the LCD framework. Similar to the backlight
> frameworks BL_CORE_SUSPENDRESUME.
>
>>
>> What would be your suggestion considering this. Actually, I don't
>> clearly understand the technical/maintainability reasons which do not
>> favour including support for multiple types of simple lcd panels in
>> the platform-lcd driver.
>
> To me it seems very unidiomatic to put multiple drivers into the same driver.
> E.g. where will we draw the line, which kind of device is simple enough so
> support for this device should be embedded in the plaform-lcd driver. Once you
> have support for multiple types of simple lcd panels in one driver things will
> start to get messy. Image how the driver will look like if it has support for
> say 5 or 10 different types of simple lcd panels.

Ok, I understand your point now. A new lcd driver will be written
mostly derived from platform-lcd driver (minus the platform specific
callbacks in platform data) that can manage power control for lcd
panels which can be controlled by a single gpio. Also, regulator and
device tree support will be added to this driver. Thanks for your
comments.

Regards,
Thomas.

>
> - Lars
>