[PATCH] video: ARM CLCD: Added dt support to set tim2 register

[PATCH] video: ARM CLCD: Added dt support to set tim2 register

[Arun Ramamurthy]

Added code based on linaro tree:
http://git.linaro.org/kernel/linux-linaro-stable.git
with commit id:6846e7822c4cab5a84672baace3b768c2d0db142
at drivers/video/amba-clcd.c. This lets the driver set
certain tim2 register bits after reading them from
device tree.

Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
---
 .../devicetree/bindings/video/arm,pl11x.txt        | 17 ++++++++-
 drivers/video/fbdev/amba-clcd.c                    | 41 ++++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
index 3e3039a..14d6f87 100644
--- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
+++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
@@ -35,6 +35,21 @@ Optional properties:
 	cell's memory interface can handle; if not present, the memory
 	interface is fast enough to handle all possible video modes
 
+- tim2: Used to set certain bits in LCDTiming2 register.
+	It can be TIM2_CLKSEL or TIM2_IOE or both
+
+	TIM2_CLKSEL: This bit drives the CLCDCLKSEL signal. It is the select
+	signal for the external LCD clock multiplexor.
+
+	TIM2_IOE: Invert output enable:
+	0 = CLAC output pin is active HIGH in TFT mode
+	1 = CLAC output pin is active LOW in TFT mode.
+	This bit selects the active polarity of the output enable signal in
+	TFT mode. In this mode, the CLAC pin is an enable that indicates to
+	the LCD panel when valid display data is available. In active
+	display mode, data is driven onto the LCD data lines at the
+	programmed edge of CLCP when CLAC is in its active state.
+
 Required sub-nodes:
 
 - port: describes LCD panel signals, following the common binding
@@ -76,7 +91,7 @@ Example:
 		clocks = <&oscclk1>, <&oscclk2>;
 		clock-names = "clcdclk", "apb_pclk";
 		max-memory-bandwidth = <94371840>; /* Bps, 1024x768@60 16bpp */
-
+		tim2 = "TIM2_CLKSEL";
 		port {
 			clcd_pads: endpoint {
 				remote-endpoint = <&clcd_panel>;
diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index 32c0b6b..4e4e50f 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -41,6 +41,44 @@
 /* This is limited to 16 characters when displayed by X startup */
 static const char *clcd_name = "CLCD FB";
 
+struct string_lookup {
+	const char	*string;
+	const u32	val;
+};
+
+static const struct string_lookup tim2_lookups[] = {
+	{ "TIM2_CLKSEL",	TIM2_CLKSEL},
+	{ "TIM2_IOE",		TIM2_IOE},
+	{ NULL, 0},
+};
+
+static u32 parse_setting(const struct string_lookup *lookup, const char *name)
+{
+	int i = 0;
+
+	while (lookup[i].string != NULL) {
+		if (strcmp(lookup[i].string, name) == 0)
+			return lookup[i].val;
+		++i;
+	}
+	return 0;
+}
+
+static u32 get_string_lookup(struct device_node *node, const char *name,
+		      const struct string_lookup *lookup)
+{
+	const char *string;
+	int count, i;
+	u32 ret = 0;
+
+	count = of_property_count_strings(node, name);
+	if (count >= 0)
+		for (i = 0; i < count; i++)
+			if (of_property_read_string_index(node, name, i,
+					&string) == 0)
+				ret |= parse_setting(lookup, string);
+	return ret;
+}
 /*
  * Unfortunately, the enable/disable functions may be called either from
  * process or IRQ context, and we _need_ to delay.  This is _not_ good.
@@ -626,6 +664,9 @@ static int clcdfb_of_init_tft_panel(struct clcd_fb *fb, u32 r0, u32 g0, u32 b0)
 	/* Bypass pixel clock divider, data output on the falling edge */
 	fb->panel->tim2 = TIM2_BCD | TIM2_IPC;
 
+	fb->panel->tim2 |= get_string_lookup(fb->dev->dev.of_node,
+					"tim2", tim2_lookups);
+
 	/* TFT display, vert. comp. interrupt at the start of the back porch */
 	fb->panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1);
 
-- 
2.3.0

[PATCH] video: ARM CLCD: Added support for FBIOPAN_DISPLAY and virtual y resolution

[Arun Ramamurthy]

Added code to support FBIOPAN_DISPLAY. Also added yres_virtual
parameter to device tree to set the virtual y resolution

Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
---
 .../devicetree/bindings/video/arm,pl11x.txt        |  4 +++
 drivers/video/fbdev/amba-clcd.c                    | 31 +++++++++++++++++++---
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
index 14d6f87..2262cdb 100644
--- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
+++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
@@ -50,6 +50,10 @@ Optional properties:
 	display mode, data is driven onto the LCD data lines at the
 	programmed edge of CLCP when CLAC is in its active state.
 
+- yres_virtual: Virtual Y resolution,
+	It can be used to configure a virtual y resolution. It
+	must be a value larger than the actual y resolution.
+
 Required sub-nodes:
 
 - port: describes LCD panel signals, following the common binding
diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index 4e4e50f..3bc09ad 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -454,6 +454,18 @@ static int clcdfb_mmap(struct fb_info *info,
 	return ret;
 }
 
+static int clcdfb_pan_display(struct fb_var_screeninfo *var,
+			      struct fb_info *info)
+{
+	struct clcd_fb *fb;
+
+	info->var = *var;
+	fb = to_clcd(info);
+	clcdfb_set_start(fb);
+
+	return 0;
+}
+
 static struct fb_ops clcdfb_ops = {
 	.owner		= THIS_MODULE,
 	.fb_check_var	= clcdfb_check_var,
@@ -464,6 +476,7 @@ static struct fb_ops clcdfb_ops = {
 	.fb_copyarea	= cfb_copyarea,
 	.fb_imageblit	= cfb_imageblit,
 	.fb_mmap	= clcdfb_mmap,
+	.fb_pan_display	= clcdfb_pan_display,
 };
 
 static int clcdfb_register(struct clcd_fb *fb)
@@ -517,14 +530,16 @@ static int clcdfb_register(struct clcd_fb *fb)
 	fb->fb.fix.type		= FB_TYPE_PACKED_PIXELS;
 	fb->fb.fix.type_aux	= 0;
 	fb->fb.fix.xpanstep	= 0;
-	fb->fb.fix.ypanstep	= 0;
+	if (fb->fb.var.yres_virtual > fb->panel->mode.yres)
+		fb->fb.fix.ypanstep = 1;
+	else
+		fb->fb.fix.ypanstep = 0;
 	fb->fb.fix.ywrapstep	= 0;
 	fb->fb.fix.accel	= FB_ACCEL_NONE;
 
 	fb->fb.var.xres		= fb->panel->mode.xres;
 	fb->fb.var.yres		= fb->panel->mode.yres;
 	fb->fb.var.xres_virtual	= fb->panel->mode.xres;
-	fb->fb.var.yres_virtual	= fb->panel->mode.yres;
 	fb->fb.var.bits_per_pixel = fb->panel->bpp;
 	fb->fb.var.grayscale	= fb->panel->grayscale;
 	fb->fb.var.pixclock	= fb->panel->mode.pixclock;
@@ -690,7 +705,7 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
 	struct device_node *endpoint;
 	int err;
 	unsigned int bpp;
-	u32 max_bandwidth;
+	u32 max_bandwidth, yres_virtual;
 	u32 tft_r0b0g0[3];
 
 	fb->panel = devm_kzalloc(&fb->dev->dev, sizeof(*fb->panel), GFP_KERNEL);
@@ -730,6 +745,14 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
 	fb->panel->width = -1;
 	fb->panel->height = -1;
 
+	/* if yres_virtual property is not specified in device tree,
+	 * set it as the actual y resolution */
+	if (of_property_read_u32(fb->dev->dev.of_node,
+				"yres_virtual", &yres_virtual))
+		fb->fb.var.yres_virtual = fb->panel->mode.yres;
+	else
+		fb->fb.var.yres_virtual = yres_virtual;
+
 	if (of_property_read_u32_array(endpoint,
 			"arm,pl11x,tft-r0g0b0-pads",
 			tft_r0b0g0, ARRAY_SIZE(tft_r0b0g0)) == 0)
@@ -797,7 +820,7 @@ static int clcdfb_of_dma_setup(struct clcd_fb *fb)
 	if (err)
 		return err;
 
-	framesize = fb->panel->mode.xres * fb->panel->mode.yres *
+	framesize = fb->panel->mode.xres * fb->fb.var.yres_virtual *
 			fb->panel->bpp / 8;
 	fb->fb.screen_base = dma_alloc_coherent(&fb->dev->dev, framesize,
 			&dma, GFP_KERNEL);
-- 
2.3.0

[PATCH] video: ARM CLCD: Added support for FBIO_WAITFORVSYNC

[Arun Ramamurthy]

Added ioctl and interrupt handler functions to support FBIO_WAITFORVSYNC
Also corrected documentation to make interrupts and interrupt-names
optional as they are not required properties.

Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>0
---
 .../devicetree/bindings/video/arm,pl11x.txt        | 11 +--
 drivers/video/fbdev/amba-clcd.c                    | 82 ++++++++++++++++++++++
 include/linux/amba/clcd.h                          |  4 ++
 3 files changed, 89 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
index 2262cdb..7d19024 100644
--- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
+++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
@@ -10,14 +10,6 @@ Required properties:
 
 - reg: base address and size of the control registers block
 
-- interrupt-names: either the single entry "combined" representing a
-	combined interrupt output (CLCDINTR), or the four entries
-	"mbe", "vcomp", "lnbu", "fuf" representing the individual
-	CLCDMBEINTR, CLCDVCOMPINTR, CLCDLNBUINTR, CLCDFUFINTR interrupts
-
-- interrupts: contains an interrupt specifier for each entry in
-	interrupt-names
-
 - clock-names: should contain "clcdclk" and "apb_pclk"
 
 - clocks: contains phandle and clock specifier pairs for the entries
@@ -54,6 +46,9 @@ Optional properties:
 	It can be used to configure a virtual y resolution. It
 	must be a value larger than the actual y resolution.
 
+- interrupts: contains an interrupt specifier for the clcd vcomp interrupt
+	 This is required for the driver to handle FBIO_WAITFORVSYNC ioctls.
+
 Required sub-nodes:
 
 - port: describes LCD panel signals, following the common binding
diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index 3bc09ad..9f7a58c 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -30,6 +30,8 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_graph.h>
+#include <linux/of_irq.h>
+#include <linux/interrupt.h>
 #include <video/display_timing.h>
 #include <video/of_display_timing.h>
 #include <video/videomode.h>
@@ -466,6 +468,73 @@ static int clcdfb_pan_display(struct fb_var_screeninfo *var,
 	return 0;
 }
 
+static int clcdfb_ioctl(struct fb_info *info,
+			unsigned int cmd, unsigned long args)
+{
+	struct clcd_fb *fb = to_clcd(info);
+	int retval = 0;
+	u32 val, ienb_val;
+
+	switch (cmd) {
+	case FBIO_WAITFORVSYNC:{
+		if (fb->lcd_irq <= 0) {
+			retval = -EINVAL;
+			break;
+		}
+		/* disable Vcomp interrupts */
+		ienb_val = readl(fb->regs + fb->off_ienb);
+		ienb_val &= ~CLCD_PL111_IENB_VCOMP;
+		writel(ienb_val, fb->regs + fb->off_ienb);
+
+		/* clear Vcomp interrupt */
+		writel(CLCD_PL111_IENB_VCOMP, fb->regs + CLCD_PL111_ICR);
+
+		/* Generate Interrupt at the start of Vsync */
+		reinit_completion(&fb->wait);
+		val = readl(fb->regs +  fb->off_cntl);
+		val &= ~(CNTL_LCDVCOMP(3));
+		writel(val, fb->regs + fb->off_cntl);
+
+		/* enable Vcomp interrupt */
+		ienb_val = readl(fb->regs + fb->off_ienb);
+		ienb_val |= CLCD_PL111_IENB_VCOMP;
+		writel(ienb_val, fb->regs + fb->off_ienb);
+		if (!wait_for_completion_interruptible_timeout
+			(&fb->wait, HZ/10))
+			retval = -ETIMEDOUT;
+		break;
+	}
+	default:
+		retval = -ENOIOCTLCMD;
+		break;
+	}
+	return retval;
+}
+
+static irqreturn_t clcd_interrupt(int irq, void *data)
+{
+	struct clcd_fb *fb = data;
+	u32 val;
+
+	/* check for vsync interrupt */
+	val = readl(fb->regs + CLCD_PL111_MIS);
+
+	if (val & CLCD_PL111_IENB_VCOMP) {
+		val = readl(fb->regs + fb->off_ienb);
+		val &= ~CLCD_PL111_IENB_VCOMP;
+
+		/* disable Vcomp interrupts */
+		writel(val, fb->regs + fb->off_ienb);
+
+		/* clear Vcomp interrupt */
+		writel(CLCD_PL111_IENB_VCOMP, fb->regs + CLCD_PL111_ICR);
+
+		complete(&fb->wait);
+		return IRQ_HANDLED;
+	}
+	return IRQ_NONE;
+}
+
 static struct fb_ops clcdfb_ops = {
 	.owner		= THIS_MODULE,
 	.fb_check_var	= clcdfb_check_var,
@@ -477,6 +546,7 @@ static struct fb_ops clcdfb_ops = {
 	.fb_imageblit	= cfb_imageblit,
 	.fb_mmap	= clcdfb_mmap,
 	.fb_pan_display	= clcdfb_pan_display,
+	.fb_ioctl	= clcdfb_ioctl,
 };
 
 static int clcdfb_register(struct clcd_fb *fb)
@@ -753,6 +823,18 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
 	else
 		fb->fb.var.yres_virtual = yres_virtual;
 
+	fb->lcd_irq = irq_of_parse_and_map(fb->dev->dev.of_node, 0);
+	if (fb->lcd_irq > 0) {
+		err = devm_request_irq(&fb->dev->dev,
+					fb->lcd_irq, clcd_interrupt,
+					IRQF_SHARED, "clcd", fb);
+		if (err < 0) {
+			dev_err(&fb->dev->dev, "unable to register CLCD interrupt\n");
+			return err;
+		}
+		init_completion(&fb->wait);
+	}
+
 	if (of_property_read_u32_array(endpoint,
 			"arm,pl11x,tft-r0g0b0-pads",
 			tft_r0b0g0, ARRAY_SIZE(tft_r0b0g0)) == 0)
diff --git a/include/linux/amba/clcd.h b/include/linux/amba/clcd.h
index e82e3ee..6a3bc2d 100644
--- a/include/linux/amba/clcd.h
+++ b/include/linux/amba/clcd.h
@@ -28,6 +28,8 @@
 #define CLCD_PL110_UCUR		0x00000028
 #define CLCD_PL110_LCUR		0x0000002C
 
+#define CLCD_PL111_IENB_VCOMP	(1 << 3)
+
 #define CLCD_PL111_CNTL		0x00000018
 #define CLCD_PL111_IENB		0x0000001c
 #define CLCD_PL111_RIS		0x00000020
@@ -184,6 +186,8 @@ struct clcd_fb {
 	u32			clcd_cntl;
 	u32			cmap[16];
 	bool			clk_enabled;
+	struct completion	wait;
+	int			lcd_irq;
 };
 
 static inline void clcdfb_decode(struct clcd_fb *fb, struct clcd_regs *regs)
-- 
2.3.0

[PATCH] video: ARM CLCD: Correcting timing checks for STN and TFT dispalys

[Arun Ramamurthy]

The minimum values for timing parameters such as left margin,
right margin etc are different for STN and TFT dispalys.
This commit fixes a check that does not account for
this difference.

Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
---
 include/linux/amba/clcd.h | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/include/linux/amba/clcd.h b/include/linux/amba/clcd.h
index 6a3bc2d..0fe8a17 100644
--- a/include/linux/amba/clcd.h
+++ b/include/linux/amba/clcd.h
@@ -288,16 +288,28 @@ static inline int clcdfb_check(struct clcd_fb *fb, struct fb_var_screeninfo *var
 	var->xres_virtual = var->xres = (var->xres + 15) & ~15;
 	var->yres_virtual = var->yres = (var->yres + 1) & ~1;
 
-#define CHECK(e,l,h) (var->e < l || var->e > h)
-	if (CHECK(right_margin, (5+1), 256) ||	/* back porch */
-	    CHECK(left_margin, (5+1), 256) ||	/* front porch */
-	    CHECK(hsync_len, (5+1), 256) ||
-	    var->xres > 4096 ||
-	    var->lower_margin > 255 ||		/* back porch */
-	    var->upper_margin > 255 ||		/* front porch */
-	    var->vsync_len > 32 ||
-	    var->yres > 1024)
-		return -EINVAL;
+#define CHECK(e, l, h) (var->e < l || var->e > h)
+	if (!(fb->panel->cntl & CNTL_LCDTFT)) {
+		if (CHECK(right_margin, (5+1), 256) ||	/* back porch */
+		CHECK(left_margin, (5+1), 256) ||	/* front porch */
+		CHECK(hsync_len, (5+1), 256) ||
+		var->xres > 4096 ||
+		var->lower_margin > 255 ||	/* back porch */
+		var->upper_margin > 255 ||	/* front porch */
+		var->vsync_len > 32 ||
+		var->yres > 1024)
+			return -EINVAL;
+	} else {
+		if (CHECK(right_margin, 1, 256) ||	/* back porch */
+		CHECK(left_margin, 1, 256) ||	/* front porch */
+		CHECK(hsync_len, 1, 256) ||
+		var->xres > 4096 ||
+		var->lower_margin > 255 ||	/* back porch */
+		var->upper_margin > 255 ||	/* front porch */
+		var->vsync_len > 32 ||
+		var->yres > 1024)
+			return -EINVAL;
+	}
 #undef CHECK
 
 	/* single panel mode: PCD = max(PCD, 1) */
-- 
2.3.0

Re: [PATCH] video: ARM CLCD: Added support for FBIO_WAITFORVSYNC

[Pawel Moll]

On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote:
> Added ioctl and interrupt handler functions to support FBIO_WAITFORVSYNC
> Also corrected documentation to make interrupts and interrupt-names
> optional as they are not required properties.

You may not be aware of this fact, but its the "documentation" what
defines what properties are required...

> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>0
> ---
>  .../devicetree/bindings/video/arm,pl11x.txt        | 11 +--
>  drivers/video/fbdev/amba-clcd.c                    | 82 ++++++++++++++++++++++
>  include/linux/amba/clcd.h                          |  4 ++
>  3 files changed, 89 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
> index 2262cdb..7d19024 100644
> --- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
> +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
> @@ -10,14 +10,6 @@ Required properties:
>  
>  - reg: base address and size of the control registers block
>  
> -- interrupt-names: either the single entry "combined" representing a
> -	combined interrupt output (CLCDINTR), or the four entries
> -	"mbe", "vcomp", "lnbu", "fuf" representing the individual
> -	CLCDMBEINTR, CLCDVCOMPINTR, CLCDLNBUINTR, CLCDFUFINTR interrupts
> -
> -- interrupts: contains an interrupt specifier for each entry in
> -	interrupt-names
> -
>  - clock-names: should contain "clcdclk" and "apb_pclk"
>  
>  - clocks: contains phandle and clock specifier pairs for the entries

So no, you can't do that.

Pawel

Re: [PATCH] video: ARM CLCD: Added support for FBIOPAN_DISPLAY and virtual y resolution

[Pawel Moll]

On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote:
> Added code to support FBIOPAN_DISPLAY. Also added yres_virtual
> parameter to device tree to set the virtual y resolution
> 
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> ---
>  .../devicetree/bindings/video/arm,pl11x.txt        |  4 +++
>  drivers/video/fbdev/amba-clcd.c                    | 31 +++++++++++++++++++---
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
> index 14d6f87..2262cdb 100644
> --- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
> +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
> @@ -50,6 +50,10 @@ Optional properties:
>  	display mode, data is driven onto the LCD data lines at the
>  	programmed edge of CLCP when CLAC is in its active state.
>  
> +- yres_virtual: Virtual Y resolution,
> +	It can be used to configure a virtual y resolution. It
> +	must be a value larger than the actual y resolution.
> +

I'm not sure about this... The word "virtual" never works well with
device tree nodes defined as "hardware description".

I understand what you're doing, but adding this property to the display
controller's node doesn't sound right. How does this describe hardware?
If anywhere, it's more like a job for the panel node?

Pawel

Re: [PATCH] video: ARM CLCD: Added support for FBIOPAN_DISPLAY and virtual y resolution

[Russell King - ARM Linux]

On Mon, Mar 02, 2015 at 04:08:29PM +0000, Pawel Moll wrote:
> I'm not sure about this... The word "virtual" never works well with
> device tree nodes defined as "hardware description".
> 
> I understand what you're doing, but adding this property to the display
> controller's node doesn't sound right. How does this describe hardware?
> If anywhere, it's more like a job for the panel node?

A better description (and implementation) would be to describe the size
of the RAM available for video purposes.  The driver can then use the
requested virtual X resolution to limit (and/or compute) the virtual Y
resolution to allow Y panning/wrapping of the display.

This would match some hardware where the video RAM is indeed a separate
physical set of RAM (such as the IM-PD/1).

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] video: ARM CLCD: Added dt support to set tim2 register

[Pawel Moll]

On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote:
> Added code based on linaro tree:
> http://git.linaro.org/kernel/linux-linaro-stable.git
> with commit id:6846e7822c4cab5a84672baace3b768c2d0db142
> at drivers/video/amba-clcd.c. This lets the driver set
> certain tim2 register bits after reading them from
> device tree.
> 
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> ---
>  .../devicetree/bindings/video/arm,pl11x.txt        | 17 ++++++++-
>  drivers/video/fbdev/amba-clcd.c                    | 41 ++++++++++++++++++++++
>  2 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
> index 3e3039a..14d6f87 100644
> --- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
> +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
> @@ -35,6 +35,21 @@ Optional properties:
>  	cell's memory interface can handle; if not present, the memory
>  	interface is fast enough to handle all possible video modes
>  
> +- tim2: Used to set certain bits in LCDTiming2 register.
> +	It can be TIM2_CLKSEL or TIM2_IOE or both
> +
> +	TIM2_CLKSEL: This bit drives the CLCDCLKSEL signal. It is the select
> +	signal for the external LCD clock multiplexor.
> +
> +	TIM2_IOE: Invert output enable:
> +	0 = CLAC output pin is active HIGH in TFT mode
> +	1 = CLAC output pin is active LOW in TFT mode.
> +	This bit selects the active polarity of the output enable signal in
> +	TFT mode. In this mode, the CLAC pin is an enable that indicates to
> +	the LCD panel when valid display data is available. In active
> +	display mode, data is driven onto the LCD data lines at the
> +	programmed edge of CLCP when CLAC is in its active state.
> +

The existing bindings intentionally avoided quoting internal registers -
they are supposed to describe how the hardware is wired up...

So how about something like "arm,pl11x,tft-invert-clac"? Then the driver
sets the bit or not, depending on the property existance?

Pawel

Re: [PATCH] video: ARM CLCD: Added support for FBIO_WAITFORVSYNC

[Arun Ramamurthy]

On 15-03-02 08:00 AM, Pawel Moll wrote:
> On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote:
>> Added ioctl and interrupt handler functions to support FBIO_WAITFORVSYNC
>> Also corrected documentation to make interrupts and interrupt-names
>> optional as they are not required properties.
>
> You may not be aware of this fact, but its the "documentation" what
> defines what properties are required...
>
Pawel, I was not aware of that. Since the driver code did not require 
the interrupts or interrupt-names bindings to load properly, I moved it 
out of the required properties. I can remove that change.

Any other comments on this change? Thanks
>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>0
>> ---
>>   .../devicetree/bindings/video/arm,pl11x.txt        | 11 +--
>>   drivers/video/fbdev/amba-clcd.c                    | 82 ++++++++++++++++++++++
>>   include/linux/amba/clcd.h                          |  4 ++
>>   3 files changed, 89 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> index 2262cdb..7d19024 100644
>> --- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> @@ -10,14 +10,6 @@ Required properties:
>>
>>   - reg: base address and size of the control registers block
>>
>> -- interrupt-names: either the single entry "combined" representing a
>> -	combined interrupt output (CLCDINTR), or the four entries
>> -	"mbe", "vcomp", "lnbu", "fuf" representing the individual
>> -	CLCDMBEINTR, CLCDVCOMPINTR, CLCDLNBUINTR, CLCDFUFINTR interrupts
>> -
>> -- interrupts: contains an interrupt specifier for each entry in
>> -	interrupt-names
>> -
>>   - clock-names: should contain "clcdclk" and "apb_pclk"
>>
>>   - clocks: contains phandle and clock specifier pairs for the entries
>
> So no, you can't do that.

>
> Pawel
>

Re: [PATCH] video: ARM CLCD: Added support for FBIOPAN_DISPLAY and virtual y resolution

[Arun Ramamurthy]

On 15-03-02 08:11 AM, Russell King - ARM Linux wrote:
> On Mon, Mar 02, 2015 at 04:08:29PM +0000, Pawel Moll wrote:
>> I'm not sure about this... The word "virtual" never works well with
>> device tree nodes defined as "hardware description".
>>
>> I understand what you're doing, but adding this property to the display
>> controller's node doesn't sound right. How does this describe hardware?
>> If anywhere, it's more like a job for the panel node?
>
I see what you are saying Pawel, I can follow Russell's recommendation 
of adding a RAM size node called max-memory-available or something similar
> A better description (and implementation) would be to describe the size
> of the RAM available for video purposes.  The driver can then use the
> requested virtual X resolution to limit (and/or compute) the virtual Y
> resolution to allow Y panning/wrapping of the display.
>

In this scenario, where would I specify the virtual X resolution? I am 
assuming it would be in the panel-timing node as Pawel suggested?

> This would match some hardware where the video RAM is indeed a separate
> physical set of RAM (such as the IM-PD/1).
>

Re: [PATCH] video: ARM CLCD: Added dt support to set tim2 register

[Arun Ramamurthy]

On 15-03-02 08:11 AM, Pawel Moll wrote:
> On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote:
>> Added code based on linaro tree:
>> http://git.linaro.org/kernel/linux-linaro-stable.git
>> with commit id:6846e7822c4cab5a84672baace3b768c2d0db142
>> at drivers/video/amba-clcd.c. This lets the driver set
>> certain tim2 register bits after reading them from
>> device tree.
>>
>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
>> ---
>>   .../devicetree/bindings/video/arm,pl11x.txt        | 17 ++++++++-
>>   drivers/video/fbdev/amba-clcd.c                    | 41 ++++++++++++++++++++++
>>   2 files changed, 57 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> index 3e3039a..14d6f87 100644
>> --- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> @@ -35,6 +35,21 @@ Optional properties:
>>   	cell's memory interface can handle; if not present, the memory
>>   	interface is fast enough to handle all possible video modes
>>
>> +- tim2: Used to set certain bits in LCDTiming2 register.
>> +	It can be TIM2_CLKSEL or TIM2_IOE or both
>> +
>> +	TIM2_CLKSEL: This bit drives the CLCDCLKSEL signal. It is the select
>> +	signal for the external LCD clock multiplexor.
>> +
>> +	TIM2_IOE: Invert output enable:
>> +	0 = CLAC output pin is active HIGH in TFT mode
>> +	1 = CLAC output pin is active LOW in TFT mode.
>> +	This bit selects the active polarity of the output enable signal in
>> +	TFT mode. In this mode, the CLAC pin is an enable that indicates to
>> +	the LCD panel when valid display data is available. In active
>> +	display mode, data is driven onto the LCD data lines at the
>> +	programmed edge of CLCP when CLAC is in its active state.
>> +
>
> The existing bindings intentionally avoided quoting internal registers -
> they are supposed to describe how the hardware is wired up...
>
> So how about something like "arm,pl11x,tft-invert-clac"? Then the driver
> sets the bit or not, depending on the property existance?
>
Sure, I can change it to two properties called arm,pl11x,tft-invert-clac 
and arm,pl11x,tft-clksel. Would that be acceptable?

> Pawel
>

Re: [PATCH] video: ARM CLCD: Added support for FBIOPAN_DISPLAY and virtual y resolution

[Russell King - ARM Linux]

On Mon, Mar 02, 2015 at 11:09:51AM -0800, Arun Ramamurthy wrote:
> 
> 
> On 15-03-02 08:11 AM, Russell King - ARM Linux wrote:
> >On Mon, Mar 02, 2015 at 04:08:29PM +0000, Pawel Moll wrote:
> >>I'm not sure about this... The word "virtual" never works well with
> >>device tree nodes defined as "hardware description".
> >>
> >>I understand what you're doing, but adding this property to the display
> >>controller's node doesn't sound right. How does this describe hardware?
> >>If anywhere, it's more like a job for the panel node?
> >
> I see what you are saying Pawel, I can follow Russell's recommendation of
> adding a RAM size node called max-memory-available or something similar
> >A better description (and implementation) would be to describe the size
> >of the RAM available for video purposes.  The driver can then use the
> >requested virtual X resolution to limit (and/or compute) the virtual Y
> >resolution to allow Y panning/wrapping of the display.
> >
> 
> In this scenario, where would I specify the virtual X resolution? I am
> assuming it would be in the panel-timing node as Pawel suggested?

virtual X should default to real X if it's not set, otherwise it
should come from the selected mode.

Drivers which get this right are things like acornfb, where we've
used ywrap scolling since it was merged.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] video: ARM CLCD: Added support for FBIOPAN_DISPLAY and virtual y resolution

[Rob Herring]

On Mon, Mar 2, 2015 at 1:09 PM, Arun Ramamurthy
<arun.ramamurthy@broadcom.com> wrote:
>
>
> On 15-03-02 08:11 AM, Russell King - ARM Linux wrote:
>>
>> On Mon, Mar 02, 2015 at 04:08:29PM +0000, Pawel Moll wrote:
>>>
>>> I'm not sure about this... The word "virtual" never works well with
>>> device tree nodes defined as "hardware description".
>>>
>>> I understand what you're doing, but adding this property to the display
>>> controller's node doesn't sound right. How does this describe hardware?
>>> If anywhere, it's more like a job for the panel node?
>>
>>
> I see what you are saying Pawel, I can follow Russell's recommendation of
> adding a RAM size node called max-memory-available or something similar

We've already got a binding for reserved memory regions for this
purpose. And there is also simplefb binding.

Rob

Re: [PATCH] video: ARM CLCD: Added support for FBIO_WAITFORVSYNC

[Rob Herring]

On Mon, Mar 2, 2015 at 10:00 AM, Pawel Moll <pawel.moll@arm.com> wrote:
> On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote:
>> Added ioctl and interrupt handler functions to support FBIO_WAITFORVSYNC
>> Also corrected documentation to make interrupts and interrupt-names
>> optional as they are not required properties.
>
> You may not be aware of this fact, but its the "documentation" what
> defines what properties are required...

Except when docs are wrong. Then dts files win.

>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>0
>> ---
>>  .../devicetree/bindings/video/arm,pl11x.txt        | 11 +--
>>  drivers/video/fbdev/amba-clcd.c                    | 82 ++++++++++++++++++++++
>>  include/linux/amba/clcd.h                          |  4 ++
>>  3 files changed, 89 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> index 2262cdb..7d19024 100644
>> --- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> @@ -10,14 +10,6 @@ Required properties:
>>
>>  - reg: base address and size of the control registers block
>>
>> -- interrupt-names: either the single entry "combined" representing a
>> -     combined interrupt output (CLCDINTR), or the four entries
>> -     "mbe", "vcomp", "lnbu", "fuf" representing the individual
>> -     CLCDMBEINTR, CLCDVCOMPINTR, CLCDLNBUINTR, CLCDFUFINTR interrupts
>> -
>> -- interrupts: contains an interrupt specifier for each entry in
>> -     interrupt-names
>> -
>>  - clock-names: should contain "clcdclk" and "apb_pclk"
>>
>>  - clocks: contains phandle and clock specifier pairs for the entries
>
> So no, you can't do that.

You can't do the other way around (making optional ones required), but
I think this is okay if the h/w interrupt lines are not physically
connected. However, if it is simply because the driver doesn't use
them, then I agree this should not be changed.

Rob

Re: [PATCH] video: ARM CLCD: Added support for FBIO_WAITFORVSYNC

[Rob Herring]

On Wed, Feb 25, 2015 at 3:01 PM, Arun Ramamurthy
<arun.ramamurthy@broadcom.com> wrote:
> Added ioctl and interrupt handler functions to support FBIO_WAITFORVSYNC
> Also corrected documentation to make interrupts and interrupt-names
> optional as they are not required properties.
>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>0
> ---
>  .../devicetree/bindings/video/arm,pl11x.txt        | 11 +--

Please split bindings to separate patches.

>  drivers/video/fbdev/amba-clcd.c                    | 82 ++++++++++++++++++++++
>  include/linux/amba/clcd.h                          |  4 ++
>  3 files changed, 89 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
> index 2262cdb..7d19024 100644
> --- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
> +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
> @@ -10,14 +10,6 @@ Required properties:
>
>  - reg: base address and size of the control registers block
>
> -- interrupt-names: either the single entry "combined" representing a
> -       combined interrupt output (CLCDINTR), or the four entries
> -       "mbe", "vcomp", "lnbu", "fuf" representing the individual
> -       CLCDMBEINTR, CLCDVCOMPINTR, CLCDLNBUINTR, CLCDFUFINTR interrupts
> -
> -- interrupts: contains an interrupt specifier for each entry in
> -       interrupt-names
> -
>  - clock-names: should contain "clcdclk" and "apb_pclk"
>
>  - clocks: contains phandle and clock specifier pairs for the entries
> @@ -54,6 +46,9 @@ Optional properties:
>         It can be used to configure a virtual y resolution. It
>         must be a value larger than the actual y resolution.
>
> +- interrupts: contains an interrupt specifier for the clcd vcomp interrupt
> +        This is required for the driver to handle FBIO_WAITFORVSYNC ioctls.
> +

What happened to interrupt-names?

Rob

Re: [PATCH] video: ARM CLCD: Added support for FBIO_WAITFORVSYNC

[Pawel Moll]

On Mon, 2015-03-02 at 19:09 +0000, Arun Ramamurthy wrote:
> On 15-03-02 08:00 AM, Pawel Moll wrote:
> > On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote:
> >> Added ioctl and interrupt handler functions to support FBIO_WAITFORVSYNC
> >> Also corrected documentation to make interrupts and interrupt-names
> >> optional as they are not required properties.
> >
> > You may not be aware of this fact, but its the "documentation" what
> > defines what properties are required...
> >
> Pawel, I was not aware of that. Since the driver code did not require 
> the interrupts or interrupt-names bindings to load properly, I moved it 
> out of the required properties. I can remove that change.

Cool. Drivers don't have to use all available properties :-) The
interrupt names are required because CLCD can be wired up with a single,
combined interrupt or with 4 separate interrupt lines (see "A.4. On-chip
signals", in the TRM) and the binding has to provide ways of describing
this. Yes, one could say "1 number == combined, 4 numbers == split", but
I personally prefer to be explicit than implicit.

Just request the interrupt by name and you'll be fine.

> Any other comments on this change? Thanks

I have no experience with the vsync ioctl, so can't really comment on
it. One minor thing I did spot is your use of curly brackets in one of
the switch cases:

On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote:
@@ -466,6 +468,73 @@ static int clcdfb_pan_display(struct fb_var_screeninfo *var,
>  	return 0;
>  }
>  
> +static int clcdfb_ioctl(struct fb_info *info,
> +			unsigned int cmd, unsigned long args)
> +{
> +	struct clcd_fb *fb = to_clcd(info);
> +	int retval = 0;
> +	u32 val, ienb_val;
> +
> +	switch (cmd) {
> +	case FBIO_WAITFORVSYNC:{

In the line above...

> +		if (fb->lcd_irq <= 0) {
> +			retval = -EINVAL;
> +			break;
> +		}
> +		/* disable Vcomp interrupts */
> +		ienb_val = readl(fb->regs + fb->off_ienb);
> +		ienb_val &= ~CLCD_PL111_IENB_VCOMP;
> +		writel(ienb_val, fb->regs + fb->off_ienb);
> +
> +		/* clear Vcomp interrupt */
> +		writel(CLCD_PL111_IENB_VCOMP, fb->regs + CLCD_PL111_ICR);
> +
> +		/* Generate Interrupt at the start of Vsync */
> +		reinit_completion(&fb->wait);
> +		val = readl(fb->regs +  fb->off_cntl);
> +		val &= ~(CNTL_LCDVCOMP(3));
> +		writel(val, fb->regs + fb->off_cntl);
> +
> +		/* enable Vcomp interrupt */
> +		ienb_val = readl(fb->regs + fb->off_ienb);
> +		ienb_val |= CLCD_PL111_IENB_VCOMP;
> +		writel(ienb_val, fb->regs + fb->off_ienb);
> +		if (!wait_for_completion_interruptible_timeout
> +			(&fb->wait, HZ/10))
> +			retval = -ETIMEDOUT;
> +		break;
> +	}

... and here.

Not sure it's needed?

Pawel

Re: [PATCH] video: ARM CLCD: Added dt support to set tim2 register

[Pawel Moll]

On Mon, 2015-03-02 at 19:09 +0000, Arun Ramamurthy wrote:
> > The existing bindings intentionally avoided quoting internal registers -
> > they are supposed to describe how the hardware is wired up...
> >
> > So how about something like "arm,pl11x,tft-invert-clac"? Then the driver
> > sets the bit or not, depending on the property existance?
> >
> Sure, I can change it to two properties called arm,pl11x,tft-invert-clac 
> and arm,pl11x,tft-clksel. Would that be acceptable?

That would be fine by me :-)

Pawel

Re: [PATCH] video: ARM CLCD: Added dt support to set tim2 register

[Pawel Moll]

On Tue, 2015-03-03 at 10:02 +0000, Pawel Moll wrote:
> On Mon, 2015-03-02 at 19:09 +0000, Arun Ramamurthy wrote:
> > > The existing bindings intentionally avoided quoting internal registers -
> > > they are supposed to describe how the hardware is wired up...
> > >
> > > So how about something like "arm,pl11x,tft-invert-clac"? Then the driver
> > > sets the bit or not, depending on the property existance?
> > >
> > Sure, I can change it to two properties called arm,pl11x,tft-invert-clac 
> > and arm,pl11x,tft-clksel. Would that be acceptable?
> 
> That would be fine by me :-)

Or (after having a look at the TRM) I should rather say: the invert-clac
is fine by me :-) but the tft-clksel doesn't work, I afraid.

If I'm not mistaken, there are two problems with it.

Number one: it's not TFT-specific, is it? So it certainly should not
have the "tft-" bit.

Number two: setting this bit says "do not use CLCDCLK for the logic; use
HCLK instead", correct? If so, have a look at the clock properties. They
say:

- clock-names: should contain "clcdclk" and "apb_pclk"

- clocks: contains phandle and clock specifier pairs for the entries
        in the clock-names property. See

So if your hardware has the reference clock wired to HCLK, and you
defining the clocks as "clcdclk", you are (no offence meant ;-)
lying :-)

So how about solving the problem by extending the clock-names definition
like this (feel free to use own wording):

- clock-names: should contain two clocks, either "clcdclk" or "hclk"
               (depending on which input is to be used as a reference
               clock by the controller logic) and "apb_pclk"

That way you're precisely describing the way the hardware is wired up.
And the driver simply tries to get clcdclk first, if it's defined -
cool, set clksel to 1, if not - try hclk and set clksel to 0. If neither
of them is present - bail out.

Does this make any sense?

Pawel

Re: [PATCH] video: ARM CLCD: Added support for FBIOPAN_DISPLAY and virtual y resolution

[Arun Ramamurthy]

On 15-03-02 03:22 PM, Rob Herring wrote:
> On Mon, Mar 2, 2015 at 1:09 PM, Arun Ramamurthy
> <arun.ramamurthy@broadcom.com> wrote:
>>
>>
>> On 15-03-02 08:11 AM, Russell King - ARM Linux wrote:
>>>
>>> On Mon, Mar 02, 2015 at 04:08:29PM +0000, Pawel Moll wrote:
>>>>
>>>> I'm not sure about this... The word "virtual" never works well with
>>>> device tree nodes defined as "hardware description".
>>>>
>>>> I understand what you're doing, but adding this property to the display
>>>> controller's node doesn't sound right. How does this describe hardware?
>>>> If anywhere, it's more like a job for the panel node?
>>>
>>>
>> I see what you are saying Pawel, I can follow Russell's recommendation of
>> adding a RAM size node called max-memory-available or something similar
>
> We've already got a binding for reserved memory regions for this
> purpose. And there is also simplefb binding.
>
Rob, I am not sure what binding you are referring to, could you be more 
specific? Also how does the simplefb bindings apply to me considering 
that is a separate driver? Thanks
> Rob
>

Re: [PATCH] video: ARM CLCD: Added support for FBIO_WAITFORVSYNC

[Arun Ramamurthy]

On 15-03-02 03:27 PM, Rob Herring wrote:
> On Mon, Mar 2, 2015 at 10:00 AM, Pawel Moll <pawel.moll@arm.com> wrote:
>> On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote:
>>> Added ioctl and interrupt handler functions to support FBIO_WAITFORVSYNC
>>> Also corrected documentation to make interrupts and interrupt-names
>>> optional as they are not required properties.
>>
>> You may not be aware of this fact, but its the "documentation" what
>> defines what properties are required...
>
> Except when docs are wrong. Then dts files win.
>
>>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>>> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>0
>>> ---
>>>   .../devicetree/bindings/video/arm,pl11x.txt        | 11 +--
>>>   drivers/video/fbdev/amba-clcd.c                    | 82 ++++++++++++++++++++++
>>>   include/linux/amba/clcd.h                          |  4 ++
>>>   3 files changed, 89 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
>>> index 2262cdb..7d19024 100644
>>> --- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
>>> +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
>>> @@ -10,14 +10,6 @@ Required properties:
>>>
>>>   - reg: base address and size of the control registers block
>>>
>>> -- interrupt-names: either the single entry "combined" representing a
>>> -     combined interrupt output (CLCDINTR), or the four entries
>>> -     "mbe", "vcomp", "lnbu", "fuf" representing the individual
>>> -     CLCDMBEINTR, CLCDVCOMPINTR, CLCDLNBUINTR, CLCDFUFINTR interrupts
>>> -
>>> -- interrupts: contains an interrupt specifier for each entry in
>>> -     interrupt-names
>>> -
>>>   - clock-names: should contain "clcdclk" and "apb_pclk"
>>>
>>>   - clocks: contains phandle and clock specifier pairs for the entries
>>
>> So no, you can't do that.
>
> You can't do the other way around (making optional ones required), but
> I think this is okay if the h/w interrupt lines are not physically
> connected. However, if it is simply because the driver doesn't use
> them, then I agree this should not be changed.
>
Agreed, I will undo this change in V2
> Rob
>

Re: [PATCH] video: ARM CLCD: Added support for FBIO_WAITFORVSYNC

[Arun Ramamurthy]

On 15-03-02 03:29 PM, Rob Herring wrote:
> On Wed, Feb 25, 2015 at 3:01 PM, Arun Ramamurthy
> <arun.ramamurthy@broadcom.com> wrote:
>> Added ioctl and interrupt handler functions to support FBIO_WAITFORVSYNC
>> Also corrected documentation to make interrupts and interrupt-names
>> optional as they are not required properties.
>>
>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>0
>> ---
>>   .../devicetree/bindings/video/arm,pl11x.txt        | 11 +--
>
> Please split bindings to separate patches.
Rob, I looked at the history of patches to this driver and all them 
included changes to the documentation in the same patch so I followed a 
similar model. I assumed that a separate patch was only required when 
the documentation was being added from scratch and not when modifying a 
few lines. Is this not the case?
>
>>   drivers/video/fbdev/amba-clcd.c                    | 82 ++++++++++++++++++++++
>>   include/linux/amba/clcd.h                          |  4 ++
>>   3 files changed, 89 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> index 2262cdb..7d19024 100644
>> --- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> @@ -10,14 +10,6 @@ Required properties:
>>
>>   - reg: base address and size of the control registers block
>>
>> -- interrupt-names: either the single entry "combined" representing a
>> -       combined interrupt output (CLCDINTR), or the four entries
>> -       "mbe", "vcomp", "lnbu", "fuf" representing the individual
>> -       CLCDMBEINTR, CLCDVCOMPINTR, CLCDLNBUINTR, CLCDFUFINTR interrupts
>> -
>> -- interrupts: contains an interrupt specifier for each entry in
>> -       interrupt-names
>> -
>>   - clock-names: should contain "clcdclk" and "apb_pclk"
>>
>>   - clocks: contains phandle and clock specifier pairs for the entries
>> @@ -54,6 +46,9 @@ Optional properties:
>>          It can be used to configure a virtual y resolution. It
>>          must be a value larger than the actual y resolution.
>>
>> +- interrupts: contains an interrupt specifier for the clcd vcomp interrupt
>> +        This is required for the driver to handle FBIO_WAITFORVSYNC ioctls.
>> +
>
> What happened to interrupt-names?
As mentioned before, these changes to the documentation  will be undone 
in V2. Thanks
>
> Rob
>

Re: [PATCH] video: ARM CLCD: Added support for FBIO_WAITFORVSYNC

[Arun Ramamurthy]

On 15-03-03 02:01 AM, Pawel Moll wrote:
> On Mon, 2015-03-02 at 19:09 +0000, Arun Ramamurthy wrote:
>> On 15-03-02 08:00 AM, Pawel Moll wrote:
>>> On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote:
>>>> Added ioctl and interrupt handler functions to support FBIO_WAITFORVSYNC
>>>> Also corrected documentation to make interrupts and interrupt-names
>>>> optional as they are not required properties.
>>>
>>> You may not be aware of this fact, but its the "documentation" what
>>> defines what properties are required...
>>>
>> Pawel, I was not aware of that. Since the driver code did not require
>> the interrupts or interrupt-names bindings to load properly, I moved it
>> out of the required properties. I can remove that change.
>
> Cool. Drivers don't have to use all available properties :-) The
> interrupt names are required because CLCD can be wired up with a single,
> combined interrupt or with 4 separate interrupt lines (see "A.4. On-chip
> signals", in the TRM) and the binding has to provide ways of describing
> this. Yes, one could say "1 number == combined, 4 numbers == split", but
> I personally prefer to be explicit than implicit.
>
> Just request the interrupt by name and you'll be fine.
>
Ok got it, will make the necessary changes
>> Any other comments on this change? Thanks
>
> I have no experience with the vsync ioctl, so can't really comment on
> it. One minor thing I did spot is your use of curly brackets in one of
> the switch cases:
>
> On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote:
> @@ -466,6 +468,73 @@ static int clcdfb_pan_display(struct fb_var_screeninfo *var,
>>   	return 0;
>>   }
>>
>> +static int clcdfb_ioctl(struct fb_info *info,
>> +			unsigned int cmd, unsigned long args)
>> +{
>> +	struct clcd_fb *fb = to_clcd(info);
>> +	int retval = 0;
>> +	u32 val, ienb_val;
>> +
>> +	switch (cmd) {
>> +	case FBIO_WAITFORVSYNC:{
>
> In the line above...
>
>> +		if (fb->lcd_irq <= 0) {
>> +			retval = -EINVAL;
>> +			break;
>> +		}
>> +		/* disable Vcomp interrupts */
>> +		ienb_val = readl(fb->regs + fb->off_ienb);
>> +		ienb_val &= ~CLCD_PL111_IENB_VCOMP;
>> +		writel(ienb_val, fb->regs + fb->off_ienb);
>> +
>> +		/* clear Vcomp interrupt */
>> +		writel(CLCD_PL111_IENB_VCOMP, fb->regs + CLCD_PL111_ICR);
>> +
>> +		/* Generate Interrupt at the start of Vsync */
>> +		reinit_completion(&fb->wait);
>> +		val = readl(fb->regs +  fb->off_cntl);
>> +		val &= ~(CNTL_LCDVCOMP(3));
>> +		writel(val, fb->regs + fb->off_cntl);
>> +
>> +		/* enable Vcomp interrupt */
>> +		ienb_val = readl(fb->regs + fb->off_ienb);
>> +		ienb_val |= CLCD_PL111_IENB_VCOMP;
>> +		writel(ienb_val, fb->regs + fb->off_ienb);
>> +		if (!wait_for_completion_interruptible_timeout
>> +			(&fb->wait, HZ/10))
>> +			retval = -ETIMEDOUT;
>> +		break;
>> +	}
>
> ... and here.
>
> Not sure it's needed?
>
No its not needed, I can remove it.
> Pawel
>
>

Re: [PATCH] video: ARM CLCD: Added dt support to set tim2 register

[Arun Ramamurthy]

On 15-03-03 02:22 AM, Pawel Moll wrote:
> On Tue, 2015-03-03 at 10:02 +0000, Pawel Moll wrote:
>> On Mon, 2015-03-02 at 19:09 +0000, Arun Ramamurthy wrote:
>>>> The existing bindings intentionally avoided quoting internal registers -
>>>> they are supposed to describe how the hardware is wired up...
>>>>
>>>> So how about something like "arm,pl11x,tft-invert-clac"? Then the driver
>>>> sets the bit or not, depending on the property existance?
>>>>
>>> Sure, I can change it to two properties called arm,pl11x,tft-invert-clac
>>> and arm,pl11x,tft-clksel. Would that be acceptable?
>>
>> That would be fine by me :-)
>
> Or (after having a look at the TRM) I should rather say: the invert-clac
> is fine by me :-) but the tft-clksel doesn't work, I afraid.
>
> If I'm not mistaken, there are two problems with it.
>
> Number one: it's not TFT-specific, is it? So it certainly should not
> have the "tft-" bit.
>
> Number two: setting this bit says "do not use CLCDCLK for the logic; use
> HCLK instead", correct? If so, have a look at the clock properties. They
> say:
>
> - clock-names: should contain "clcdclk" and "apb_pclk"
>
> - clocks: contains phandle and clock specifier pairs for the entries
>          in the clock-names property. See
>
> So if your hardware has the reference clock wired to HCLK, and you
> defining the clocks as "clcdclk", you are (no offence meant ;-)
> lying :-)
>
No offense taken :)
> So how about solving the problem by extending the clock-names definition
> like this (feel free to use own wording):
>
> - clock-names: should contain two clocks, either "clcdclk" or "hclk"
>                 (depending on which input is to be used as a reference
>                 clock by the controller logic) and "apb_pclk"
>
> That way you're precisely describing the way the hardware is wired up.
> And the driver simply tries to get clcdclk first, if it's defined -
> cool, set clksel to 1, if not - try hclk and set clksel to 0. If neither
> of them is present - bail out.
>
> Does this make any sense?
>
This makes sense to me, thank you for the suggestions. I will fix it all 
up in V2
> Pawel
>

Re: [PATCH] video: ARM CLCD: Added dt support to set tim2 register

[Pawel Moll]

On Wed, 2015-03-04 at 00:37 +0000, Arun Ramamurthy wrote:
> > That way you're precisely describing the way the hardware is wired up.
> > And the driver simply tries to get clcdclk first, if it's defined -
> > cool, set clksel to 1, if not - try hclk and set clksel to 0. If neither
> > of them is present - bail out.
> >
> > Does this make any sense?
> >
> This makes sense to me, thank you for the suggestions. I will fix it all 
> up in V2

Cool. Just a word of comment to my own words ;-) The "bail out" case was
a bad idea - the non-DT use cases more likely than not will have no
clock name defined. So, to maintain backward compatibility, the driver
will still have to work when no named clock is available. I think the
simplest way to do that is to check if "hclk" is available, and use it
if it is (setting the clksel accordingly). Otherwise - proceed as it was
the case previously.

Pawel

Re: [PATCH] video: ARM CLCD: Added dt support to set tim2 register

[Russell King - ARM Linux]

On Tue, Mar 03, 2015 at 10:22:07AM +0000, Pawel Moll wrote:
> On Tue, 2015-03-03 at 10:02 +0000, Pawel Moll wrote:
> > On Mon, 2015-03-02 at 19:09 +0000, Arun Ramamurthy wrote:
> > > > The existing bindings intentionally avoided quoting internal registers -
> > > > they are supposed to describe how the hardware is wired up...
> > > >
> > > > So how about something like "arm,pl11x,tft-invert-clac"? Then the driver
> > > > sets the bit or not, depending on the property existance?
> > > >
> > > Sure, I can change it to two properties called arm,pl11x,tft-invert-clac 
> > > and arm,pl11x,tft-clksel. Would that be acceptable?
> > 
> > That would be fine by me :-)
> 
> Or (after having a look at the TRM) I should rather say: the invert-clac
> is fine by me :-) but the tft-clksel doesn't work, I afraid.
> 
> If I'm not mistaken, there are two problems with it.
> 
> Number one: it's not TFT-specific, is it? So it certainly should not
> have the "tft-" bit.
> 
> Number two: setting this bit says "do not use CLCDCLK for the logic; use
> HCLK instead", correct? If so, have a look at the clock properties. They
> say:
> 
> - clock-names: should contain "clcdclk" and "apb_pclk"
> 
> - clocks: contains phandle and clock specifier pairs for the entries
>         in the clock-names property. See
> 
> So if your hardware has the reference clock wired to HCLK, and you
> defining the clocks as "clcdclk", you are (no offence meant ;-)
> lying :-)

No.  The CLCD block always takes two clock signals - the AHB bus clock
(HCLK) for the slave interface, and a CLCD clock.

The CLCDCLKSEL is a bit which affects a signal sent to the world outside
of the CLCD block, which is used to drive an _external_ multiplexer to
select the CLCD clock source.  (See the description for bit 5 of the
LCDTiming2 register.)

So, the clock is still input to the CLCDCLK input, even if it is ultimately
derived from HCLK.

Remember, the clock API does not deal with names describing the source of
the clock, but the consumer of the clock.  The consumer in this case is
the PL11x CLCD block, which takes a CLCDCLK input.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] video: ARM CLCD: Added dt support to set tim2 register

[Linus Walleij]

On Wed, Feb 25, 2015 at 10:01 PM, Arun Ramamurthy
<arun.ramamurthy@broadcom.com> wrote:

> Added code based on linaro tree:
> http://git.linaro.org/kernel/linux-linaro-stable.git
> with commit id:6846e7822c4cab5a84672baace3b768c2d0db142
> at drivers/video/amba-clcd.c. This lets the driver set
> certain tim2 register bits after reading them from
> device tree.
>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>

I have now implemented this properly in this patch:
http://marc.info/?l=linux-fbdev&m=145459469913983&w=2

Please provide your Tested-by/Reviewed-by/Ack if you're
still working on this.

Yours,
Linus Walleij

Re: [PATCH] video: ARM CLCD: Added dt support to set tim2 register

[Ray Jui]

Hi Linus,

On 2/10/2016 5:58 AM, Linus Walleij wrote:
> On Wed, Feb 25, 2015 at 10:01 PM, Arun Ramamurthy
> <arun.ramamurthy@broadcom.com> wrote:
>
>> Added code based on linaro tree:
>> http://git.linaro.org/kernel/linux-linaro-stable.git
>> with commit id:6846e7822c4cab5a84672baace3b768c2d0db142
>> at drivers/video/amba-clcd.c. This lets the driver set
>> certain tim2 register bits after reading them from
>> device tree.
>>
>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
>
> I have now implemented this properly in this patch:
> http://marc.info/?l=linux-fbdev&m=145459469913983&w=2
>
> Please provide your Tested-by/Reviewed-by/Ack if you're
> still working on this.

Could you please add me to the email thread and I can review it there (I 
won't have time to test, but I can help to review the code and find time 
to test later)?

This may be a dumb question, is there any way for me to directly reply 
to the thread here?

http://marc.info/?l=linux-fbdev&m=145459469913983&w=2

>
> Yours,
> Linus Walleij
>

Re: [PATCH] video: ARM CLCD: Added dt support to set tim2 register

[Linus Walleij]

On Wed, Feb 10, 2016 at 6:48 PM, Ray Jui <ray.jui@broadcom.com> wrote:

> Could you please add me to the email thread and I can review it there (I
> won't have time to test, but I can help to review the code and find time to
> test later)?

OK I will add you to subsequent postings, if any.

> This may be a dumb question, is there any way for me to directly reply to
> the thread here?

Not easily, I guess it is possible to conjure an SMTP mail
with the right in-reply-to message ID but that is so complex
hacking that I have no clue how to do it, just ever reached
the limit of "a little knowledge is dangerous".

Yours,
Linus Walleij

Re: [PATCH] video: ARM CLCD: Added dt support to set tim2 register

[Ray Jui]

On 2/16/2016 11:32 AM, Dmitry Torokhov wrote:
>
>
> On Mon, Feb 15, 2016 at 5:25 AM, Linus Walleij <linus.walleij@linaro.org
> <mailto:linus.walleij@linaro.org>> wrote:
>
>     On Wed, Feb 10, 2016 at 6:48 PM, Ray Jui <ray.jui@broadcom.com
>     <mailto:ray.jui@broadcom.com>> wrote:
>
>     > Could you please add me to the email thread and I can review it there (I
>     > won't have time to test, but I can help to review the code and find time to
>     > test later)?
>
>     OK I will add you to subsequent postings, if any.
>
>     > This may be a dumb question, is there any way for me to directly reply to
>     > the thread here?
>
>     Not easily, I guess it is possible to conjure an SMTP mail
>     with the right in-reply-to message ID but that is so complex
>     hacking that I have no clue how to do it, just ever reached
>     the limit of "a little knowledge is dangerous".
>
>
> To reply to a patch find it in patchwork, download it as mbox, open
> downloaded file with mutt and reply as usual.
>
> Thanks,
> Dmitry

Got it, thanks!

Ray