OMAPDSS: Add support for panel ls037v7dw01

Otherwise we can get often errors like the following and the display won't come on:

omapdss APPLY error: FIFO UNDERFLOW on gfx, disabling the overlay omapdss APPLY error: SYNC_LOST on channel lcd, restarting the output with video overlays disabled

There are some earlier references to this issue:

http://www.spinics.net/lists/linux-omap/msg59511.html
http://www.spinics.net/lists/linux-omap/msg59724.html

It seems that it's safe to set the lower values even for 3630. If we can confirm that 3630 works with the higher values reliably we can add further detection.

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/video/fbdev/omap2/dss/dss.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/video/fbdev/omap2/dss/dss.c +3 -2

index d55266c..ad6561f 100644
@@ -707,9 +707,10 @@ static const struct dss_features omap34xx_dss_feats __initconst = {
 	.dpi_select_source	=	&dss_dpi_select_source_omap2_omap3,
 };
 
+/* Supposedly 3630 can use div 32 mult 2, but that needs to be rechecked */
 static const struct dss_features omap3630_dss_feats __initconst = {
-	.fck_div_max		=	32,
-	.dss_fck_multiplier	=	1,
+	.fck_div_max		=	16,
+	.dss_fck_multiplier	=	2,
 	.parent_clk_name	=	"dpll4_ck",
 	.dpi_select_source	=	&dss_dpi_select_source_omap2_omap3,
 };

Signature · 6 lines

1.8.1.1

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 7 lines · Tony Lindgren

Otherwise we can get often errors like the following and the display won't come on:

omapdss APPLY error: FIFO UNDERFLOW on gfx, disabling the overlay omapdss APPLY error: SYNC_LOST on channel lcd, restarting the output with video overlays disabled

Quoted 8 lines

There are some earlier references to this issue:

http://www.spinics.net/lists/linux-omap/msg59511.html
http://www.spinics.net/lists/linux-omap/msg59724.html

It seems that it's safe to set the lower values even for 3630. If we can confirm that 3630 works with the higher values reliably we can add further detection.

BTW, I'm also seeing this warning on 3730-evm it may be related:

[    3.523101] ------------[ cut here ]------------
[    3.528015] WARNING: CPU: 0 PID: 6 at drivers/video/fbdev/omap2/dss/dss.c:483 dss_set_fck_rate+0x6c/0x8c()
[    3.538360] clk rate mismatch: 108000000 != 115200000
[    3.543518] Modules linked in:

Quoted 19 lines · Tony Lindgren

Quoted 7 lines · Tony Lindgren

Otherwise we can get often errors like the following and the display won't come on:

omapdss APPLY error: FIFO UNDERFLOW on gfx, disabling the overlay omapdss APPLY error: SYNC_LOST on channel lcd, restarting the output with video overlays disabled

Quoted 8 lines

There are some earlier references to this issue:

http://www.spinics.net/lists/linux-omap/msg59511.html
http://www.spinics.net/lists/linux-omap/msg59724.html

It seems that it's safe to set the lower values even for 3630. If we can confirm that 3630 works with the higher values reliably we can add further detection.

BTW, I'm also seeing this warning on 3730-evm it may be related:

Quoted 4 lines

[    3.523101] ------------[ cut here ]------------
[    3.528015] WARNING: CPU: 0 PID: 6 at drivers/video/fbdev/omap2/dss/dss.c:483 dss_set_fck_rate+0x6c/0x8c()
[    3.538360] clk rate mismatch: 108000000 != 115200000
[    3.543518] Modules linked in:

Hmm... Can you paste the clk_summary from debugfs? Somehow the clock framework calculates the clock rate differently than the dss. Or do we have different clock.dts files used for 3730 and 3630?

Quoted 25 lines · Tomi Valkeinen

Quoted 23 lines · Tony Lindgren

Quoted 7 lines · Tony Lindgren

Otherwise we can get often errors like the following and the display won't come on:

omapdss APPLY error: FIFO UNDERFLOW on gfx, disabling the overlay omapdss APPLY error: SYNC_LOST on channel lcd, restarting the output with video overlays disabled

Quoted 8 lines

There are some earlier references to this issue:

http://www.spinics.net/lists/linux-omap/msg59511.html
http://www.spinics.net/lists/linux-omap/msg59724.html

It seems that it's safe to set the lower values even for 3630. If we can confirm that 3630 works with the higher values reliably we can add further detection.

BTW, I'm also seeing this warning on 3730-evm it may be related:

[    3.523101] ------------[ cut here ]------------
[    3.528015] WARNING: CPU: 0 PID: 6 at drivers/video/fbdev/omap2/dss/dss.c:483 dss_set_fck_rate+0x6c/0x8c()
[    3.538360] clk rate mismatch: 108000000 != 115200000
[    3.543518] Modules linked in:

Quoted 3 lines

Hmm... Can you paste the clk_summary from debugfs? Somehow the clock framework calculates the clock rate differently than the dss. Or do we have different clock.dts files used for 3730 and 3630?

OK pasted to the other email in this thread.

Otherwise we can get often errors like the following and the display won't come on:

omapdss APPLY error: FIFO UNDERFLOW on gfx, disabling the overlay omapdss APPLY error: SYNC_LOST on channel lcd, restarting the output with video overlays disabled

There are some earlier references to this issue:

http://www.spinics.net/lists/linux-omap/msg59511.html
http://www.spinics.net/lists/linux-omap/msg59724.html

It seems that it's safe to set the lower values even for 3630. If we can confirm that 3630 works with the higher values reliably we can add further detection.

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/video/fbdev/omap2/dss/dss.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/video/fbdev/omap2/dss/dss.c +0 -0

index d55266c..ad6561f 100644

Quoted 7 lines · Tony Lindgren

Otherwise we can get often errors like the following and the display won't come on:

omapdss APPLY error: FIFO UNDERFLOW on gfx, disabling the overlay omapdss APPLY error: SYNC_LOST on channel lcd, restarting the output with video overlays disabled

Quoted 4 lines

There are some earlier references to this issue:

http://www.spinics.net/lists/linux-omap/msg59511.html
http://www.spinics.net/lists/linux-omap/msg59724.html

Those don't sound like the same issue, but it's hard to say. What kind of clock rates do you get? Cat you paste debugfs/omapdss/clk, with and without this patch?

What resolution do you have? If it's a very high resolution (say, DVI output to a monitor), it could just be an issue of not-enough-memory-bandwidth.

Quoted 14 lines

It seems that it's safe to set the lower values even for 3630. If we can confirm that 3630 works with the higher values reliably we can add further detection.

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/video/fbdev/omap2/dss/dss.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/video/fbdev/omap2/dss/dss.c +0 -0

index d55266c..ad6561f 100644

drivers/video/fbdev/omap2/dss/dss.c static const struct dss_features omap34xx_dss_feats __initconst = { · line 713

Quoted 9 lines

drivers/video/fbdev/omap2/dss/dss.c +3 -2

@@ -707,9 +707,10 @@ static const struct dss_features omap34xx_dss_feats __initconst = {
 	.dpi_select_source	=	&dss_dpi_select_source_omap2_omap3,
 };
 
+/* Supposedly 3630 can use div 32 mult 2, but that needs to be rechecked */
 static const struct dss_features omap3630_dss_feats __initconst = {
-	.fck_div_max		=	32,
-	.dss_fck_multiplier	=	1,
+	.fck_div_max		=	16,
+	.dss_fck_multiplier	=	2,

These values tell about the clock hardware, they are not settings that can be changed to change the clock. OMAP3630 has a fixed x2 multiplier and a divider with maximum value of 16.

Tomi

Quoted 8 lines · Tomi Valkeinen

Quoted 7 lines · Tony Lindgren

Otherwise we can get often errors like the following and the display won't come on:

omapdss APPLY error: FIFO UNDERFLOW on gfx, disabling the overlay omapdss APPLY error: SYNC_LOST on channel lcd, restarting the output with video overlays disabled

Quoted 8 lines

Quoted 4 lines

There are some earlier references to this issue:

http://www.spinics.net/lists/linux-omap/msg59511.html
http://www.spinics.net/lists/linux-omap/msg59724.html

Those don't sound like the same issue, but it's hard to say. What kind of clock rates do you get? Cat you paste debugfs/omapdss/clk, with and without this patch?

Without this patch:

# cat /sys/kernel/debug/omapdss/clk 
[   24.833831] DSS: dss_runtime_get
[   24.837554] DSS: dss_runtime_put
[   24.840972] DISPC: dispc_runtime_get
[   24.844757] DISPC: dispc_runtime_put
- DSS -
DSS_FCK (DSS1_ALWON_FCLK) = 57600000
- DISPC -
dispc fclk source = DSS_FCK (DSS1_ALWON_FCLK)
fck             57600000        
- LCD -
LCD clk source = DSS_FCK (DSS1_ALWON_FCLK)
lck             57600000        lck div 1
pck             19200000        pck div 3

With this patch:

# cat /sys/kernel/debug/omapdss/clk 
[   34.580688] DSS: dss_runtime_get
[   34.584197] DSS: dss_runtime_put
[   34.587768] DISPC: dispc_runtime_get
[   34.591552] DISPC: dispc_runtime_put
- DSS -
DSS_FCK (DSS1_ALWON_FCLK) = 108000000
- DISPC -
dispc fclk source = DSS_FCK (DSS1_ALWON_FCLK)
fck             108000000       
- LCD -
LCD clk source = DSS_FCK (DSS1_ALWON_FCLK)
lck             108000000       lck div 1
pck             18000000        pck div 6

Quoted 3 lines

What resolution do you have? If it's a very high resolution (say, DVI output to a monitor), it could just be an issue of not-enough-memory-bandwidth.

This is just the 3730-evm with the Sharp VGA panel mentioned in this series.

Quoted 3 lines · Tony Lindgren

This is just the 3730-evm with the Sharp VGA panel mentioned in this series.

Hmm, well, those both look fine. The fck is well below the maximum, which is somewhere around 170MHz-180MHz. The lck/pck ratio is higher with this patch, but that should affect the GFX overlay.

So you're just booting, and there are no applications that use the framebuffer? And there is no rotation or such configured?

Quoted 5 lines · Tomi Valkeinen

Quoted 3 lines · Tony Lindgren

This is just the 3730-evm with the Sharp VGA panel mentioned in this series.

Quoted 6 lines

Hmm, well, those both look fine. The fck is well below the maximum, which is somewhere around 170MHz-180MHz. The lck/pck ratio is higher with this patch, but that should affect the GFX overlay.

So you're just booting, and there are no applications that use the framebuffer? And there is no rotation or such configured?

Right. The rotation is set to 3 though.

Quoted 10 lines · Tony Lindgren

Quoted 9 lines · Tomi Valkeinen

Quoted 3 lines · Tony Lindgren

This is just the 3730-evm with the Sharp VGA panel mentioned in this series.

Hmm, well, those both look fine. The fck is well below the maximum, which is somewhere around 170MHz-180MHz. The lck/pck ratio is higher with this patch, but that should affect the GFX overlay.

Quoted 4 lines

Quoted 2 lines

So you're just booting, and there are no applications that use the framebuffer? And there is no rotation or such configured?

Right. The rotation is set to 3 though.

Hmm, that's probably the issue then. VRFB rotation is very heavy on the memory bandwidth, and is generally a very easy way to get sync lost errors.

will handle it fine with the clock rates and bandwidth usage you have for your use cases.

Quoted 11 lines · Tomi Valkeinen

Quoted 10 lines · Tony Lindgren

Quoted 9 lines · Tomi Valkeinen

Quoted 3 lines · Tony Lindgren

This is just the 3730-evm with the Sharp VGA panel mentioned in this series.

Hmm, well, those both look fine. The fck is well below the maximum, which is somewhere around 170MHz-180MHz. The lck/pck ratio is higher with this patch, but that should affect the GFX overlay.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

So you're just booting, and there are no applications that use the framebuffer? And there is no rotation or such configured?

Right. The rotation is set to 3 though.

Hmm, that's probably the issue then. VRFB rotation is very heavy on the memory bandwidth, and is generally a very easy way to get sync lost errors.

I found another case without rotation where the error always triggers. By forcing 3730-evm LCD panel to QVGA mode it always seems to happen even without rotation.

Without this patch with errors and no image:

# cat /sys/kernel/debug/omapdss/clk 
[   55.185729] DSS: dss_runtime_get
[   55.189422] DSS: dss_runtime_put
[   55.192810] DISPC: dispc_runtime_get
[   55.196685] DISPC: dispc_runtime_put
- DSS -
DSS_FCK (DSS1_ALWON_FCLK) = 27000000
- DISPC -
dispc fclk source = DSS_FCK (DSS1_ALWON_FCLK)
fck             27000000        
- LCD -
LCD clk source = DSS_FCK (DSS1_ALWON_FCLK)
lck             27000000        lck div 1
pck             5400000         pck div 5

With this patch without errors and penguin showing:

# cat /sys/kernel/debug/omapdss/clk
[   19.905792] DSS: dss_runtime_get
[   19.909545] DSS: dss_runtime_put
[   19.912933] DISPC: dispc_runtime_get
[   19.916778] DISPC: dispc_runtime_put
- DSS -
DSS_FCK (DSS1_ALWON_FCLK) = 108000000
- DISPC -
dispc fclk source = DSS_FCK (DSS1_ALWON_FCLK)
fck             108000000       
- LCD -
LCD clk source = DSS_FCK (DSS1_ALWON_FCLK)
lck             108000000       lck div 1
pck             5400000         pck div 20

In this case the errors are different too:

DISPC: channel 0 xres 240 yres 320
DISPC: pck 5400000
DISPC: hsw 3 hfp 3 hbp 39 vsw 1 vfp 2 vbp 7
DISPC: vsync_level 1 hsync_level 1 data_pclk_edge 1 de_level 0 sync_pclk_edge 0
DISPC: hsync 18947Hz, vsync 57Hz
DISPC: lck = 27000000 (1)
DISPC: pck = 5400000 (5)
APPLY: DISPC IRQ: 0x60: GFX_FIFO_UNDERFLOW
APPLY: DISPC IRQ: 0x4062: GFX_FIFO_UNDERFLOW SYNC_LOST
DISPC: dispc_runtime_get
omapdss APPLY error: FIFO UNDERFLOW on gfx, disabling the overlay

Regarding rotation, it does look like that with VGA mode enabling rotation makes the error trigger quite often on 3730.

Quoted 2 lines

will handle it fine with the clock rates and bandwidth usage you have for your use cases.

I don't think it's all because of rotation. And rotating my head

makes my neck hurt!

Quoted 3 lines · Tony Lindgren

Otherwise we can get often errors like the following and the display won't come on:

Quoted 8 lines

omapdss APPLY error: FIFO UNDERFLOW on gfx, disabling the overlay omapdss APPLY error: SYNC_LOST on channel lcd, restarting the output with video overlays disabled

There are some earlier references to this issue:

http://www.spinics.net/lists/linux-omap/msg59511.html
http://www.spinics.net/lists/linux-omap/msg59724.html

Those don't sound like the same issue, but it's hard to say. What kind of clock rates do you get? Cat you paste debugfs/omapdss/clk, with and without this patch?

What resolution do you have? If it's a very high resolution (say, DVI output to a monitor), it could just be an issue of not-enough-memory-bandwidth.

Quoted 14 lines

It seems that it's safe to set the lower values even for 3630. If we can confirm that 3630 works with the higher values reliably we can add further detection.

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/video/fbdev/omap2/dss/dss.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/video/fbdev/omap2/dss/dss.c +0 -0

index d55266c..ad6561f 100644

drivers/video/fbdev/omap2/dss/dss.c static const struct dss_features omap34xx_dss_feats __initconst = { · line 713

Quoted 9 lines

drivers/video/fbdev/omap2/dss/dss.c +3 -2

@@ -707,9 +707,10 @@ static const struct dss_features omap34xx_dss_feats __initconst = {
      .dpi_select_source      =       &dss_dpi_select_source_omap2_omap3,
 };
 
+/* Supposedly 3630 can use div 32 mult 2, but that needs to be rechecked */
 static const struct dss_features omap3630_dss_feats __initconst = {
-     .fck_div_max            =       32,
-     .dss_fck_multiplier     =       1,
+     .fck_div_max            =       16,
+     .dss_fck_multiplier     =       2,

These values tell about the clock hardware, they are not settings that can be changed to change the clock. OMAP3630 has a fixed x2 multiplier and a divider with maximum value of 16.

Tomi

Quoted 4 lines · Tomi Valkeinen

Quoted 3 lines · Tony Lindgren

Otherwise we can get often errors like the following and the display won't come on:

Quoted 9 lines

Quoted 8 lines

omapdss APPLY error: FIFO UNDERFLOW on gfx, disabling the overlay omapdss APPLY error: SYNC_LOST on channel lcd, restarting the output with video overlays disabled

There are some earlier references to this issue:

http://www.spinics.net/lists/linux-omap/msg59511.html
http://www.spinics.net/lists/linux-omap/msg59724.html

resend - my client had HTML enabled...

FWIW: I have had issues long time ago [1]. With mainline 3.14.3 I had still the reboot problem on old 600MHz OMAP 3530. Applying this patch solved the issues. For other versions I had no chance to reproduce the original wakup issue mentioned in old thread

[1] http://marc.info/?l=linux-omap&m=136250904607413&w=2

Andreas

Quoted 39 lines

Those don't sound like the same issue, but it's hard to say. What kind of clock rates do you get? Cat you paste debugfs/omapdss/clk, with and without this patch?

What resolution do you have? If it's a very high resolution (say, DVI output to a monitor), it could just be an issue of not-enough-memory-bandwidth.

Quoted 14 lines

It seems that it's safe to set the lower values even for 3630. If we can confirm that 3630 works with the higher values reliably we can add further detection.

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/video/fbdev/omap2/dss/dss.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/video/fbdev/omap2/dss/dss.c +0 -0

index d55266c..ad6561f 100644

drivers/video/fbdev/omap2/dss/dss.c static const struct dss_features omap34xx_dss_feats __initconst = { · line 713

Quoted 9 lines

drivers/video/fbdev/omap2/dss/dss.c +3 -2

@@ -707,9 +707,10 @@ static const struct dss_features omap34xx_dss_feats __initconst = {
      .dpi_select_source      =       &dss_dpi_select_source_omap2_omap3,
 };
 
+/* Supposedly 3630 can use div 32 mult 2, but that needs to be rechecked */
 static const struct dss_features omap3630_dss_feats __initconst = {
-     .fck_div_max            =       32,
-     .dss_fck_multiplier     =       1,
+     .fck_div_max            =       16,
+     .dss_fck_multiplier     =       2,

These values tell about the clock hardware, they are not settings that can be changed to change the clock. OMAP3630 has a fixed x2 multiplier and a divider with maximum value of 16.

Tomi

Quoted 14 lines · Andreas M?ller

Quoted 4 lines · Tomi Valkeinen

Quoted 3 lines · Tony Lindgren

Otherwise we can get often errors like the following and the display won't come on:

Quoted 9 lines

Quoted 8 lines

omapdss APPLY error: FIFO UNDERFLOW on gfx, disabling the overlay omapdss APPLY error: SYNC_LOST on channel lcd, restarting the output with video overlays disabled

There are some earlier references to this issue:

http://www.spinics.net/lists/linux-omap/msg59511.html
http://www.spinics.net/lists/linux-omap/msg59724.html

Quoted 6 lines

resend - my client had HTML enabled...

FWIW: I have had issues long time ago [1]. With mainline 3.14.3 I had still the reboot problem on old 600MHz OMAP 3530. Applying this patch solved the issues. For other versions I had no chance to reproduce the original wakup issue mentioned in old thread

Sorry I'm a bit confused now. Is the reboot issue a separate issue related to the twl4030 generic scripts for 3530?

And then this patch fixes dm3730 wake-up (from suspend?) issues?

Or do we have some other bug where we wrongly hit omap3630_dss_feats on 3530 somehow?

Quoted 22 lines · Tony Lindgren

Quoted 14 lines · Andreas M?ller

Quoted 4 lines · Tomi Valkeinen

Quoted 3 lines · Tony Lindgren

Otherwise we can get often errors like the following and the display won't come on:

Quoted 9 lines

Quoted 8 lines

omapdss APPLY error: FIFO UNDERFLOW on gfx, disabling the overlay omapdss APPLY error: SYNC_LOST on channel lcd, restarting the output with video overlays disabled

There are some earlier references to this issue:

http://www.spinics.net/lists/linux-omap/msg59511.html
http://www.spinics.net/lists/linux-omap/msg59724.html

Quoted 6 lines

resend - my client had HTML enabled...

FWIW: I have had issues long time ago [1]. With mainline 3.14.3 I had still the reboot problem on old 600MHz OMAP 3530. Applying this patch solved the issues. For other versions I had no chance to reproduce the original wakup issue mentioned in old thread

Quoted 7 lines

Sorry I'm a bit confused now. Is the reboot issue a separate issue related to the twl4030 generic scripts for 3530?

And then this patch fixes dm3730 wake-up (from suspend?) issues?

Or do we have some other bug where we wrongly hit omap3630_dss_feats on 3530 somehow?

(What I think) I have done: Have 3.14.3 still booting with legacy board init (please don't kill me for that). Without applying your patch old OMAP3530 crashed at every reboot with SYNCH_LOST.

After applying your patch reboot works fine.

Looking into id-code makes me wonder what changes on 36xx-features do to 35xx devices (treated as 34xx - right?). The only way that this happens is for unknown hawkeye fallthough. I suggest you simply forget my note and I will find out what really 'fixed' dss reboot problem.

Using gpiod will make it easier to add device tree support for this panel in the following patches.

Note that all the GPIOs for this panel are optional, any of the the GPIOs could be configured with external pulls instead of GPIOs, so let's not error out if GPIOs are not found to make the panel more generic.

Signed-off-by Tony Lindgren <[email protected]>

---
 .../omap2/displays-new/panel-sharp-ls037v7dw01.c   | 92 +++++++++-------------
 1 file changed, 38 insertions(+), 54 deletions(-)

Review

Quoted 9 lines · Tony Lindgren

(quoted diff) +9 -0

+static struct gpio_desc *
+sharp_ls_get_gpio(struct device *dev, int gpio, unsigned long flags,
+		  char *desc)
+{
+	int r;
+
+	r = devm_gpio_request_one(dev, gpio, flags, desc);
+	if (r) {
+		dev_err(dev, "no %s gpio\n", desc);

As it's fine to not have a gpio, I guess this error print should be removed.

Also, if the error is EPROBE_DEFER, the driver should fail and return that. The same goes for the DT version in the next patch.

We can pass the GPIO configuration for ls037v7dw01 in a standard gpios property.

Signed-off-by Tony Lindgren <[email protected]>

---
 .../bindings/panel/sharp,ls037v7dw01.txt           | 53 ++++++++++++++
 .../omap2/displays-new/panel-sharp-ls037v7dw01.c   | 84 ++++++++++++++++++++--
 2 files changed, 133 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt

Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt new file +19 -0

new file mode 100644
index 0000000..7f6f5e1
@@ -0,0 +1,53 @@
+SHARP LS037V7DW01 TFT-LCD panel
+
+Required properties:
+- compatible: should be "sharp,ls037v7dw01"
+
+Optional properties:
+- reset-gpios: a GPIO spec for the optional reset pin
+- enable-gpios: a GPIO array for the optional configuration GPIOs
+  ordered MO, LR, UD, INI as specified in the LS037V7DW01.pdf file.
+
+This binding is compatible with the simple-panel binding, which is specified
+in simple-panel.txt in this directory.
+
+This panel can have zero to five GPIOs to configure
+to change configuration between QVGA and VGA mode
+and the scan direction. As these pins can be also
+configured with external pulls, all the GPIOs are
+considered optional with holes in the array.
+

Quoted 41 lines · Tony Lindgren

We can pass the GPIO configuration for ls037v7dw01 in a standard gpios property.

Signed-off-by Tony Lindgren <[email protected]>

---
 .../bindings/panel/sharp,ls037v7dw01.txt           | 53 ++++++++++++++
 .../omap2/displays-new/panel-sharp-ls037v7dw01.c   | 84 ++++++++++++++++++++--
 2 files changed, 133 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt

Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt new file +25 -0

new file mode 100644
index 0000000..7f6f5e1
@@ -0,0 +1,53 @@
+SHARP LS037V7DW01 TFT-LCD panel
+
+Required properties:
+- compatible: should be "sharp,ls037v7dw01"
+
+Optional properties:
+- reset-gpios: a GPIO spec for the optional reset pin
+- enable-gpios: a GPIO array for the optional configuration GPIOs
+  ordered MO, LR, UD, INI as specified in the LS037V7DW01.pdf file.
+
+This binding is compatible with the simple-panel binding, which is specified
+in simple-panel.txt in this directory.
+
+This panel can have zero to five GPIOs to configure
+to change configuration between QVGA and VGA mode
+and the scan direction. As these pins can be also
+configured with external pulls, all the GPIOs are
+considered optional with holes in the array.
+
+Example when connected to a omap2+ based device:
+
+	lcd0: display {
+		compatible = "sharp,ls037v7dw01";
+		power-supply = <&lcd_3v3>;
+		reset-gpios = <&gpio5 27 GPIO_ACTIVE_HIGH>;	/* gpio155, lcd RESB */

Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt line 29

Quoted 4 lines

Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt new file +4 -0

+		enable-gpios = <&gpio5 26 GPIO_ACTIVE_HIGH	/* gpio154, lcd MO */
+				&gpio1 2 GPIO_ACTIVE_HIGH	/* gpio2, lcd LR */
+				&gpio1 3 GPIO_ACTIVE_HIGH	/* gpio3, lcd UD */
+				&gpio5 24 GPIO_ACTIVE_HIGH>;	/* gpio152, lcd INI */

Why are most of the gpios in 'enable-gpios'? MO, LR and UD don't really have anything to do with enabling the panel. Why not just have one 'gpios' array with all the gpios, as they are quite a mixed bunch?

Quoted 4 lines

Quoted 4 lines

Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt new file +4 -0

+	lcd0: display {
+		compatible = "sharp,ls037v7dw01";
+		power-supply = <&lcd_3v3>;
+		reset-gpios = <&gpio5 27 GPIO_ACTIVE_HIGH>;	/* gpio155, lcd RESB */

Quoted 8 lines

Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt line 29

Quoted 4 lines

Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt new file +4 -0

+		enable-gpios = <&gpio5 26 GPIO_ACTIVE_HIGH	/* gpio154, lcd MO */
+				&gpio1 2 GPIO_ACTIVE_HIGH	/* gpio2, lcd LR */
+				&gpio1 3 GPIO_ACTIVE_HIGH	/* gpio3, lcd UD */
+				&gpio5 24 GPIO_ACTIVE_HIGH>;	/* gpio152, lcd INI */

Why are most of the gpios in 'enable-gpios'? MO, LR and UD don't really have anything to do with enabling the panel. Why not just have one 'gpios' array with all the gpios, as they are quite a mixed bunch?

Yeah that's what I had initially, but reading simple-panel.txt noticed there are some standardized GPIOs for reset and enable.

How about we set up one reset-gpios, one enable-gpios and an array of three unlabelled GPIOs?

Or if we want to add something generic, we could add new mode-gpios property that other panels might be able to use too?

BTW, I'm also personally fine with all five gpios showing in a single gpios property, I'm not too exited about naming anything in DT..

Quoted 13 lines · Tony Lindgren

Quoted 4 lines

Quoted 4 lines

Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt new file +4 -0

+	lcd0: display {
+		compatible = "sharp,ls037v7dw01";
+		power-supply = <&lcd_3v3>;
+		reset-gpios = <&gpio5 27 GPIO_ACTIVE_HIGH>;	/* gpio155, lcd RESB */

Quoted 8 lines

Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt line 29

Quoted 4 lines

Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt new file +4 -0

+		enable-gpios = <&gpio5 26 GPIO_ACTIVE_HIGH	/* gpio154, lcd MO */
+				&gpio1 2 GPIO_ACTIVE_HIGH	/* gpio2, lcd LR */
+				&gpio1 3 GPIO_ACTIVE_HIGH	/* gpio3, lcd UD */
+				&gpio5 24 GPIO_ACTIVE_HIGH>;	/* gpio152, lcd INI */

Why are most of the gpios in 'enable-gpios'? MO, LR and UD don't really have anything to do with enabling the panel. Why not just have one 'gpios' array with all the gpios, as they are quite a mixed bunch?

Quoted 2 lines

Yeah that's what I had initially, but reading simple-panel.txt noticed there are some standardized GPIOs for reset and enable.

I'm not sure if it's worth to try to make it compatible with simple-panel. simple-panel bindings do not work with omapdss anyway, and simple-panel doesn't have the support for input ports/endpoints.

Can the panel be used with only one gpio (I guess that would be INI or RESB)? If not, simple-panel doesn't work.

Quoted 6 lines

How about we set up one reset-gpios, one enable-gpios and an array of three unlabelled GPIOs?

Or if we want to add something generic, we could add new mode-gpios property that other panels might be able to use too?

Quoted 2 lines

BTW, I'm also personally fine with all five gpios showing in a single gpios property, I'm not too exited about naming anything in DT..

I don't have a strong opinion here. I don't have much experience with DT, especially with making bindings compatible with other ones.

I'd just forget the simple-panel, and have single gpio array.

Quoted 5 lines · Tomi Valkeinen

Quoted 3 lines · Tony Lindgren

BTW, I'm also personally fine with all five gpios showing in a single gpios property, I'm not too exited about naming anything in DT..

Quoted 4 lines

I don't have a strong opinion here. I don't have much experience with DT, especially with making bindings compatible with other ones.

I'd just forget the simple-panel, and have single gpio array.

Well if it's a don't care flag for both of us, let's try to use the existing standard for simple-panel.txt and add mode-gpios property. I'll post a patch for that.

We can pass the GPIO configuration for ls037v7dw01 in a standard gpios property.

Signed-off-by Tony Lindgren <[email protected]>

Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt new file +5 -0

@@ -0,0 +1,56 @@
+SHARP LS037V7DW01 TFT-LCD panel
+
+Required properties:
+- compatible: should be "sharp,ls037v7dw01"
+

Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt new file +22 -0

+This panel can have zero to five GPIOs to configure
+to change configuration between QVGA and VGA mode
+and the scan direction. As these pins can be also
+configured with external pulls, all the GPIOs are
+considered optional with holes in the array.
+
+Example when connected to a omap2+ based device:
+
+	lcd0: display {
+		compatible = "sharp,ls037v7dw01";
+		power-supply = <&lcd_3v3>;
+		enable-gpios = <&gpio5 24 GPIO_ACTIVE_HIGH>;	/* gpio152, lcd INI */
+		reset-gpios = <&gpio5 27 GPIO_ACTIVE_HIGH>;	/* gpio155, lcd RESB */
+		mode-gpios = <&gpio5 26 GPIO_ACTIVE_HIGH	/* gpio154, lcd MO */
+			      &gpio1 2 GPIO_ACTIVE_HIGH		/* gpio2, lcd LR */
+			      &gpio1 3 GPIO_ACTIVE_HIGH>;	/* gpio3, lcd UD */
+
+		panel-timing {
+			clock-frequency = <19200000>;
+			hback-porch = <28>;
+			hactive = <480>;
+			hfront-porch = <1>;

Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt new file +10 -0

+static int sharp_ls_probe_of(struct platform_device *pdev)
+{
+	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+	struct device_node *node = pdev->dev.of_node;
+	struct omap_dss_device *in;
+	struct display_timing timing;
+	struct videomode vm;
+	int r;
+
+	ddata->vcc = devm_regulator_get(&pdev->dev, "envdd");

Quoted 24 lines · Tony Lindgren

Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt new file +24 -0

+Example when connected to a omap2+ based device:
+
+	lcd0: display {
+		compatible = "sharp,ls037v7dw01";
+		power-supply = <&lcd_3v3>;
+		enable-gpios = <&gpio5 24 GPIO_ACTIVE_HIGH>;	/* gpio152, lcd INI */
+		reset-gpios = <&gpio5 27 GPIO_ACTIVE_HIGH>;	/* gpio155, lcd RESB */
+		mode-gpios = <&gpio5 26 GPIO_ACTIVE_HIGH	/* gpio154, lcd MO */
+			      &gpio1 2 GPIO_ACTIVE_HIGH		/* gpio2, lcd LR */
+			      &gpio1 3 GPIO_ACTIVE_HIGH>;	/* gpio3, lcd UD */
+
+		panel-timing {
+			clock-frequency = <19200000>;
+			hback-porch = <28>;
+			hactive = <480>;
+			hfront-porch = <1>;
+			hsync-len = <2>;
+			vback-porch = <1>;
+			vactive = <640>;
+			vfront-porch = <1>;
+			vsync-len = <1>;
+			hsync-active = <0>;
+			vsync-active = <0>;
+			de-active = <1>;

Review

Quoted 2 lines

(quoted diff) +2 -0

+			pixelclk-active = <1>;
+		};

I don't think we should define panel-timing here. We know it's sharp,ls037v7dw01, so the driver knows the video timings. Also, if we would extend the driver to support both resolution modes, it needs to support two different timings, so the above doesn't work in that case either.

Although if the MO gpio is not controlled by the driver, we should tell the driver whether that gpio is high or low.

At the moment our display drivers are OMAP specific, and for that reason we should prefix the compatible strings with "omapdss,". For example, drivers/video/fbdev/omap2/displays-new/panel-dsi-cm.c:

	{ .compatible = "omapdss,panel-dsi-cm", },

But we should still have the right compatible string in the .dts, so we convert the compatible name in arch/arm/mach-omap2/display.c, with 'dss_compat_conv_list' array, to which this panel's name should be added.

Quoted 2 lines

Although if the MO gpio is not controlled by the driver, we should tell the driver whether that gpio is high or low.

What do you have in mind for telling that? We should also tell the orientation of the panel:

EVM VGA omapfb.rotate=3
LDP QVGA omapfb.rotate=0

Do you have something in mind for that?

Quoted 9 lines

At the moment our display drivers are OMAP specific, and for that reason we should prefix the compatible strings with "omapdss,". For example, drivers/video/fbdev/omap2/displays-new/panel-dsi-cm.c:

	{ .compatible = "omapdss,panel-dsi-cm", },

But we should still have the right compatible string in the .dts, so we convert the compatible name in arch/arm/mach-omap2/display.c, with 'dss_compat_conv_list' array, to which this panel's name should be added.

Oh so what do you want to have in the .dts file then?

Quoted 10 lines · Tony Lindgren

Quoted 2 lines

Although if the MO gpio is not controlled by the driver, we should tell the driver whether that gpio is high or low.

What do you have in mind for telling that? We should also tell the orientation of the panel:

EVM   VGA     omapfb.rotate=3
LDP   QVGA    omapfb.rotate=0

Do you have something in mind for that?

Hmm, right. I guess all we can do is have boolean flags in the .dts for MO, LR and UD, which tells if the respective pins are hard-wires high or low. And say in the documentation that you must either have a proper GPIO, or use the flag, but not both.

The panel mounting orientation is a different matter. But I think it is also something we should specify in the .dts. However, we don't have any SW support to handle it, and it's a bit unclear to me how it should be handled, so I think that has to be left for later.

Quoted 11 lines

Quoted 9 lines

At the moment our display drivers are OMAP specific, and for that reason we should prefix the compatible strings with "omapdss,". For example, drivers/video/fbdev/omap2/displays-new/panel-dsi-cm.c:

     { .compatible = "omapdss,panel-dsi-cm", },

But we should still have the right compatible string in the .dts, so we convert the compatible name in arch/arm/mach-omap2/display.c, with 'dss_compat_conv_list' array, to which this panel's name should be added.

Oh so what do you want to have in the .dts file then?

The .dts should have the proper names. The idea of this hackery is that in the .dts we can have the proper compatible string. So in the .dts, we have, say:

"sony,panel-foobar"

Then, at boot time, omap's platform code changes that to:

"omapdss,sony,panel-foobar".

And our (omap specific) panel-foobar driver use that "omapdss,sony,panel-foobar" string.

This way some other platform could do the same, and have their platform specific drivers handle the panel.

At some point in the future we hopefully will have common panel drivers, and at that point we can remove that hackery. The .dts files will already be correct.

Tomi

Maybe we can remove this hackery by relying on the fact that a compatible string can have a set of values that goes from more specific to more general. So you can have something like:

compatible = "sony,panel-foobar", "omapdss,panel-foobar"

So right now only "omapdss,panel-foobar" will be matched and later when we have common panel drivers then that driver could handle the "sony,panel-foobar" compatible string.

Other platforms could do something similar and have

compatible = "sony,panel-foobar", "baz,panel-foobar"

That way you won't break DT backward compatible but still not require hacks on arch/arm/mach-omap2/display.c.

We do the same for OMAP boards, we now have the following compatible string:

compatible = "isee,omap3-igep0020", "ti,omap36xx", "ti,omap3";

There isn't a struct machine_desc that matches "isee,omap3-igep0020" but later if we find that we need some board specific initialization we could add one without breaking existing DTS. In fact it used to be a single machine_desc that matched "ti,omap3" for both omap36xx and omap34xx but later when some DT clocks changes were introduced we needed to split both SoC families.

Quoted 19 lines · Javier Martinez Canillas

Maybe we can remove this hackery by relying on the fact that a compatible string can have a set of values that goes from more specific to more general. So you can have something like:

compatible = "sony,panel-foobar", "omapdss,panel-foobar"

So right now only "omapdss,panel-foobar" will be matched and later when we have common panel drivers then that driver could handle the "sony,panel-foobar" compatible string.

Other platforms could do something similar and have

compatible = "sony,panel-foobar", "baz,panel-foobar"

That way you won't break DT backward compatible but still not require hacks on arch/arm/mach-omap2/display.c.

We do the same for OMAP boards, we now have the following compatible string:

Quoted 8 lines

compatible = "isee,omap3-igep0020", "ti,omap36xx", "ti,omap3";

There isn't a struct machine_desc that matches "isee,omap3-igep0020" but later if we find that we need some board specific initialization we could add one without breaking existing DTS. In fact it used to be a single machine_desc that matched "ti,omap3" for both omap36xx and omap34xx but later when some DT clocks changes were introduced we needed to split both SoC families.

I think that's a different thing. "isee,omap3-igep0020" is a proper compatible string, if the board is "isee,...". No matter what software you run, that's correct and fine.

The omapdss case is different, there the "omapdss" points to a sw thing, it does not describe the hardware. It's only needed as we don't have proper sw drivers for the devices.

That said, I think what you describe would work. But I would rather keep the .dts files clean and correct, and keep that hacks hidden inside the kernel.

Tomi

Hello Tomi,

Quoted 34 lines · Tomi Valkeinen

Quoted 19 lines · Javier Martinez Canillas

Maybe we can remove this hackery by relying on the fact that a compatible string can have a set of values that goes from more specific to more general. So you can have something like:

compatible = "sony,panel-foobar", "omapdss,panel-foobar"

So right now only "omapdss,panel-foobar" will be matched and later when we have common panel drivers then that driver could handle the "sony,panel-foobar" compatible string.

Other platforms could do something similar and have

compatible = "sony,panel-foobar", "baz,panel-foobar"

That way you won't break DT backward compatible but still not require hacks on arch/arm/mach-omap2/display.c.

We do the same for OMAP boards, we now have the following compatible string:

Quoted 8 lines

compatible = "isee,omap3-igep0020", "ti,omap36xx", "ti,omap3";

There isn't a struct machine_desc that matches "isee,omap3-igep0020" but later if we find that we need some board specific initialization we could add one without breaking existing DTS. In fact it used to be a single machine_desc that matched "ti,omap3" for both omap36xx and omap34xx but later when some DT clocks changes were introduced we needed to split both SoC families.

I think that's a different thing. "isee,omap3-igep0020" is a proper compatible string, if the board is "isee,...". No matter what software you run, that's correct and fine.

Quoted 8 lines

The omapdss case is different, there the "omapdss" points to a sw thing, it does not describe the hardware. It's only needed as we don't have proper sw drivers for the devices.

That said, I think what you describe would work. But I would rather keep the .dts files clean and correct, and keep that hacks hidden inside the kernel.

Thanks for the explanation. Since DT are meant to describe hardware and not software I agree with you that we shouldn't leak an implementation detail to the DeviceTrees.

And after all fortunately we don't have a stable API in the kernel like the one that is enforced in the DT so we can fix it later ;-)

Quoted 13 lines

Quoted 8 lines

The omapdss case is different, there the "omapdss" points to a sw thing, it does not describe the hardware. It's only needed as we don't have proper sw drivers for the devices.

That said, I think what you describe would work. But I would rather keep the .dts files clean and correct, and keep that hacks hidden inside the kernel.

Thanks for the explanation. Since DT are meant to describe hardware and not software I agree with you that we shouldn't leak an implementation detail to the DeviceTrees.

Quoted 2 lines

And after all fortunately we don't have a stable API in the kernel like the one that is enforced in the DT so we can fix it later ;-)

This remapping of compatible flags sounds smelly to me, please discuss it first with the device tree people.

I think we're better off just using the compatible properties limited to the simple-panel.txt for now and actually describe the real hardware. The fact that the panel is connected to DSS should be possible to find out based on the phandles.

Quoted 13 lines · Tony Lindgren

Quoted 13 lines

Quoted 8 lines

The omapdss case is different, there the "omapdss" points to a sw thing, it does not describe the hardware. It's only needed as we don't have proper sw drivers for the devices.

That said, I think what you describe would work. But I would rather keep the .dts files clean and correct, and keep that hacks hidden inside the kernel.

Thanks for the explanation. Since DT are meant to describe hardware and not software I agree with you that we shouldn't leak an implementation detail to the DeviceTrees.

Quoted 5 lines

Quoted 2 lines

And after all fortunately we don't have a stable API in the kernel like the one that is enforced in the DT so we can fix it later ;-)

This remapping of compatible flags sounds smelly to me, please discuss it first with the device tree people.

It's already in v3.15.

I agree it's smelly, but it smelly only inside the kernel, not visible anywhere, and we can remove it when we have common panel drivers, without any modifications to .dts files.

Quoted 4 lines

I think we're better off just using the compatible properties limited to the simple-panel.txt for now and actually describe the real hardware. The fact that the panel is connected to DSS should be possible to find out based on the phandles.

I'm not sure what you mean. We do describe only the real hardware. The compatible string in the .dts file is "sharp,ls037v7dw01".

Prepending the "omapdss" to the compatible string is required for the device/driver matching to work, because we can't use the "sharp,ls037v7dw01" compatible string in our omap specific panel driver.

I'm not sure what it would give us to try to be compatible with simple-panel.txt. The simple-panel bindings won't probably be compatible with the future common display drivers in any case, as the simple-panel binding is just too limited and doesn't describe the hardware fully.

Quoted 1 line

It's already in v3.15.

Oh great.. And you dumped it into arch/arm/mach-omap2 too and I somehow even acked it :( Looks like there's also the custom mux hacks. And custom hwmod hacks. And ongoing soc_is_xxx tinkering. Now let's not add

_any_ new crap into arch/arm/mach-omap2/display.c.

So please consider this a huge eternal NAK to add any more crap to arch/arm/mach-omap2/display.c from me. No more new soc_is beyond the soc_is_am43xx() you have in linux next. No more adding of_find_compatible_node() beyond ti,omap5-dss you have in linux next.

And no more new panel remapping in this file as soon as you have found a better solution. Preferrably by v3.17 merge window. This file just needs to disappear. Yuk.

Quoted 4 lines

I'm not sure what it would give us to try to be compatible with simple-panel.txt. The simple-panel bindings won't probably be compatible with the future common display drivers in any case, as the simple-panel binding is just too limited and doesn't describe the hardware fully.

Hmm what else does a panel need where the existing binding cannot be extended?

Quoted 5 lines · Tony Lindgren

Quoted 1 line

It's already in v3.15.

Oh great.. And you dumped it into arch/arm/mach-omap2 too and I somehow even acked it :( Looks like there's also the custom mux hacks. And custom hwmod hacks. And ongoing soc_is_xxx tinkering. Now let's not add

The omap2_dss_hwmod_data, create_dss_pdev and create_simple_dss_pdev, omap_display_init stuff can be removed when the DT conversion has been done.

For the omap4_dsi_mux_pads (or the omap5 dsi muxing that was recently discussed) I still have no solution, but I haven't spent time on it. I have dropped the omap5 dsi muxing from the latest series for that reason.

dispc_disable_outputs and omap_dss_reset are needed because omap_device doesn't handle resetting DSS properly, as special steps need to be done for that. omap_dss_reset is called from the hwmod/omap_device code, so I don't think this code can be anywhere else.

For the omap_display_get_version() I have no good solution. Making separate .dts versions for all the needed OMAP ES versions seems like a huge hassle to me, but that is one solution.

I think that would mean having separate .dtsi files for omapdss for omap3_es1, omap3_es3plus, omap4_es1, omap4_es2, omap4_es3plus, and then having separate omapxxxx.dtsi files for all of those, and then separate board .dts files for the ES versions used on each board.

Or is there some sane way to define such things in dts?

Quoted 10 lines

_any_ new crap into arch/arm/mach-omap2/display.c.

So please consider this a huge eternal NAK to add any more crap to arch/arm/mach-omap2/display.c from me. No more new soc_is beyond the soc_is_am43xx() you have in linux next. No more adding of_find_compatible_node() beyond ti,omap5-dss you have in linux next.

And no more new panel remapping in this file as soon as you have found a better solution. Preferrably by v3.17 merge window. This file just needs to disappear. Yuk.

Do you object to the compatible string remapping as such, or just that it's in arch/arm/mach-omap2?

I guess nothing prevents me from moving it to drivers/, and having some early-ish initcall doing the job.

If the remapping as such is horrible, I'm all ears for better ideas. I spent quite a lot of time on it, and that's the best I could come up with.

It's rather simple prefixing of the compatible strings for selected devices. The purpose of that is to have proper data in the .dts files (which I think is very important) with the cost of having some rather simple internal hacks in the kernel, which can be removed immediately when we have a common display driver framework.

Quoted 7 lines

Quoted 4 lines

I'm not sure what it would give us to try to be compatible with simple-panel.txt. The simple-panel bindings won't probably be compatible with the future common display drivers in any case, as the simple-panel binding is just too limited and doesn't describe the hardware fully.

Hmm what else does a panel need where the existing binding cannot be extended?

The existing simple-panel binding doesn't describe the connections of the panel, i.e. its video input. I guess it can be extended, but I don't see what the benefit is of trying to create new panel bindings compatible with the simple-panel bindings. As I see, the simple-panel bindings work only with very limited use cases, where the drivers make assumptions. Simple panel bindings cannot be used with omapdss, nor can it be used with the future common display framework.

But I'm not really familiar with using extending current bindings, and making new bindings compatible with old ones. Can you explain why it'd be good to have the sharp panel bindings compatible with simple-panel? In what kind of scenario would it be used?

Tomi

Quoted 10 lines · Tomi Valkeinen

Quoted 5 lines · Tony Lindgren

Quoted 1 line

It's already in v3.15.

Oh great.. And you dumped it into arch/arm/mach-omap2 too and I somehow even acked it :( Looks like there's also the custom mux hacks. And custom hwmod hacks. And ongoing soc_is_xxx tinkering. Now let's not add

The omap2_dss_hwmod_data, create_dss_pdev and create_simple_dss_pdev, omap_display_init stuff can be removed when the DT conversion has been done.

Quoted 3 lines

For the omap4_dsi_mux_pads (or the omap5 dsi muxing that was recently discussed) I still have no solution, but I haven't spent time on it. I have dropped the omap5 dsi muxing from the latest series for that reason.

Quoted 4 lines

dispc_disable_outputs and omap_dss_reset are needed because omap_device doesn't handle resetting DSS properly, as special steps need to be done for that. omap_dss_reset is called from the hwmod/omap_device code, so I don't think this code can be anywhere else.

Quoted 11 lines

For the omap_display_get_version() I have no good solution. Making separate .dts versions for all the needed OMAP ES versions seems like a huge hassle to me, but that is one solution.

I think that would mean having separate .dtsi files for omapdss for omap3_es1, omap3_es3plus, omap4_es1, omap4_es2, omap4_es3plus, and then having separate omapxxxx.dtsi files for all of those, and then separate board .dts files for the ES versions used on each board.

Or is there some sane way to define such things in dts?

Unfortunately there isn't.

I have a similar problem with the IGEP boards since there are just too many variations of them (e.g: DM3730 SoC vs OMAP3530 SoC, NAND vs OneNAND, 256MB vs 512MB flash memory sizes, with and without wifi chip, etc).

Since dts are supposed to describe the hardware, each revision with a slighter variation forces to have a different dts. My solution was to not try to have a dts for every single variation in mainline but only one for the most common revision and that could be used as a reference to modify and ship on each device. Unfortunately that is not possible to you since each DSS IP block is used by many boards.

Quoted 7 lines

Quoted 7 lines

_any_ new crap into arch/arm/mach-omap2/display.c.

So please consider this a huge eternal NAK to add any more crap to arch/arm/mach-omap2/display.c from me. No more new soc_is beyond the soc_is_am43xx() you have in linux next. No more adding of_find_compatible_node() beyond ti,omap5-dss you have in linux next.

Quoted 6 lines

Quoted 3 lines

And no more new panel remapping in this file as soon as you have found a better solution. Preferrably by v3.17 merge window. This file just needs to disappear. Yuk.

Do you object to the compatible string remapping as such, or just that it's in arch/arm/mach-omap2?

Quoted 2 lines

I guess nothing prevents me from moving it to drivers/, and having some early-ish initcall doing the job.

Quoted 9 lines

If the remapping as such is horrible, I'm all ears for better ideas. I spent quite a lot of time on it, and that's the best I could come up with.

It's rather simple prefixing of the compatible strings for selected devices. The purpose of that is to have proper data in the .dts files (which I think is very important) with the cost of having some rather simple internal hacks in the kernel, which can be removed immediately when we have a common display driver framework.

Even though the compatible trick that I said before could be an alternative, it has the problem that does not describes the hardware as you said. The restriction of the DT being a stable API and get it right from the beginning forces us to make these kind of technical decisions.

So, since we can change the kernel later but not the DTS, I agree with you that the remapping is the least bad of our options.

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

I'm not sure what it would give us to try to be compatible with simple-panel.txt. The simple-panel bindings won't probably be compatible with the future common display drivers in any case, as the simple-panel binding is just too limited and doesn't describe the hardware fully.

Hmm what else does a panel need where the existing binding cannot be extended?

Quoted 7 lines

The existing simple-panel binding doesn't describe the connections of the panel, i.e. its video input. I guess it can be extended, but I don't see what the benefit is of trying to create new panel bindings compatible with the simple-panel bindings. As I see, the simple-panel bindings work only with very limited use cases, where the drivers make assumptions. Simple panel bindings cannot be used with omapdss, nor can it be used with the future common display framework.

Quoted 4 lines

But I'm not really familiar with using extending current bindings, and making new bindings compatible with old ones. Can you explain why it'd be good to have the sharp panel bindings compatible with simple-panel? In what kind of scenario would it be used?

Well ideally the revision info for a device would come from device revision registers rather using the SoC revision. In the DSS case when the SoC revision is needed by a device it maybe it can be deciphered from a combination of compatible flag and the clock rate for example?

Quoted 2 lines

Quoted 2 lines

Do you object to the compatible string remapping as such, or just that it's in arch/arm/mach-omap2?

It's something I'd rather not have under mach-omap2 as that means that I may need to deal with it too to some extent. And I don't think we need to do such remapping, we should be able to use the panel compatible strings as they are just fine. It should be possible to figure out from the device tree properties what controller the panel belongs to. Or for now, use the panel registration to figure out what display controller it belongs to.

Quoted 2 lines

Quoted 2 lines

I guess nothing prevents me from moving it to drivers/, and having some early-ish initcall doing the job.

/me likes this idea if you need it at all. Stuff like this tends to stay and spread, so I'd rather not do the remapping of compatible strings at all.

Quoted 2 lines

So, since we can change the kernel later but not the DTS, I agree with you that the remapping is the least bad of our options.

Yes the binding for the panel should just describe the panel so it can be used with whatever display controller. But we do have quite a few buses probing devices. How about set up the panel probing the same way?

For the panels on display controller, just do the usual for_each_child_of_node(pdev->dev.of_node, child) and probe them?

It seems the remapping of compatible strings is not needed in this as we're only picking up panels that are children of the display controller.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

I'm not sure what it would give us to try to be compatible with simple-panel.txt. The simple-panel bindings won't probably be compatible with the future common display drivers in any case, as the simple-panel binding is just too limited and doesn't describe the hardware fully.

Hmm what else does a panel need where the existing binding cannot be extended?

Quoted 7 lines

Quoted 7 lines

The existing simple-panel binding doesn't describe the connections of the panel, i.e. its video input. I guess it can be extended, but I don't see what the benefit is of trying to create new panel bindings compatible with the simple-panel bindings. As I see, the simple-panel bindings work only with very limited use cases, where the drivers make assumptions. Simple panel bindings cannot be used with omapdss, nor can it be used with the future common display framework.

Well it seems at least the reset and enable pin standard from that binding can be kept.

Quoted 4 lines

Quoted 4 lines

But I'm not really familiar with using extending current bindings, and making new bindings compatible with old ones. Can you explain why it'd be good to have the sharp panel bindings compatible with simple-panel? In what kind of scenario would it be used?

Ideally the panel binding just describes the panel and it should not matter which display controller it is a child of.

Quoted 4 lines · Tony Lindgren

Well ideally the revision info for a device would come from device revision registers rather using the SoC revision. In the DSS case when the SoC revision is needed by a device it maybe it can be deciphered from a combination of compatible flag and the clock rate for example?

I've been trying that. The HW guys didn't bother to update the DSS revision registers, so they are useless. And, for example, the OMAP3 ES difference is only about bitfield widths in two registers.

I tried writing "too long" value to the register on the earlier ES version, hoping that the extra bits would be kept at zero, but that wasn't the case. So I just don't see a way to detect this from the DSS's point of view.

Quoted 14 lines

Quoted 2 lines

Quoted 2 lines

Do you object to the compatible string remapping as such, or just that it's in arch/arm/mach-omap2?

It's something I'd rather not have under mach-omap2 as that means that I may need to deal with it too to some extent. And I don't think we need to do such remapping, we should be able to use the panel compatible strings as they are just fine. It should be possible to figure out from the device tree properties what controller the panel belongs to. Or for now, use the panel registration to figure out what display controller it belongs to.

Quoted 2 lines

Quoted 2 lines

I guess nothing prevents me from moving it to drivers/, and having some early-ish initcall doing the job.

Quoted 3 lines

/me likes this idea if you need it at all. Stuff like this tends to stay and spread, so I'd rather not do the remapping of compatible strings at all.

Yep. I'll look to this. Thinking about it now, it kind of makes more sense to have it in the omapdss's directory.

Quoted 10 lines

Quoted 2 lines

So, since we can change the kernel later but not the DTS, I agree with you that the remapping is the least bad of our options.

Yes the binding for the panel should just describe the panel so it can be used with whatever display controller. But we do have quite a few buses probing devices. How about set up the panel probing the same way?

For the panels on display controller, just do the usual for_each_child_of_node(pdev->dev.of_node, child) and probe them?

Quoted 3 lines

It seems the remapping of compatible strings is not needed in this as we're only picking up panels that are children of the display controller.

The panels (or display encoders) are not (usually) children of the display controller. They are children of their respective control bus. Say, an i2c panel is a child of i2c bus. If there's no control bus, like is the case with the sharp panel, it's a platform device.

The video paths of the panels and encoders are connected using the v4l2 style ports/endpoints. We can use those to see what display controller a panel is connected to, but only after the panel driver has already probed. We don't have control for the actual probing, as that happens with whatever the control bus is for the display component.

Quoted 16 lines

Quoted 15 lines

Quoted 15 lines

Quoted 7 lines

Quoted 4 lines

I'm not sure what it would give us to try to be compatible with simple-panel.txt. The simple-panel bindings won't probably be compatible with the future common display drivers in any case, as the simple-panel binding is just too limited and doesn't describe the hardware fully.

Hmm what else does a panel need where the existing binding cannot be extended?

The existing simple-panel binding doesn't describe the connections of the panel, i.e. its video input. I guess it can be extended, but I don't see what the benefit is of trying to create new panel bindings compatible with the simple-panel bindings. As I see, the simple-panel bindings work only with very limited use cases, where the drivers make assumptions. Simple panel bindings cannot be used with omapdss, nor can it be used with the future common display framework.

Quoted 2 lines

Well it seems at least the reset and enable pin standard from that binding can be kept.

Only enable gpio there. But even that's vague. Do you turn on the power before or after setting the enable gpio? How long delay should be between the power and the gpio? Different panels have different rules for the power-up.

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

But I'm not really familiar with using extending current bindings, and making new bindings compatible with old ones. Can you explain why it'd be good to have the sharp panel bindings compatible with simple-panel? In what kind of scenario would it be used?

Quoted 2 lines

Ideally the panel binding just describes the panel and it should not matter which display controller it is a child of.

Yes, but that means the panel bindings need to have enough information so that all display controllers can use it. Simple-panel bindings do not have enough information. The simple-panel bindings do not have information about the video bus input, and it doesn't even have information about the resolution or bitdepth of the panel.

So I'm still asking, if we create sharp bindings that use the same properties as the simple-panel bindings, and define that sharp panel is compatible with simple-panel, what kind of scenario in practice would it be used in?

Would the scenario be some other OS, that doesn't have a driver for the sharp panel, but has a driver for the simple-panel? That would only work if the sharp panel hardware is setup so that only the enable gpio is needed, so that quite a narrow definition of "compatible".

Or is there some other scenario in which it could be used?

Quoted 5 lines

The video paths of the panels and encoders are connected using the v4l2 style ports/endpoints. We can use those to see what display controller a panel is connected to, but only after the panel driver has already probed. We don't have control for the actual probing, as that happens with whatever the control bus is for the display component.

OK. So with generic panels, you can just let the panels probe with the right compatible flag then and let the ports/endpoints registration to figure out if the panel is usable for the display controller in question.

Quoted 7 lines

Quoted 2 lines

Well it seems at least the reset and enable pin standard from that binding can be kept.

Only enable gpio there. But even that's vague. Do you turn on the power before or after setting the enable gpio? How long delay should be between the power and the gpio? Different panels have different rules for the power-up.

Sure, it's a complex problem. But for the enable gpio..

Maybe the enable GPIO should be treated as a regulator? That would allow specifying first the source regulator startup delay, and then the panel has it's own startup delay.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

But I'm not really familiar with using extending current bindings, and making new bindings compatible with old ones. Can you explain why it'd be good to have the sharp panel bindings compatible with simple-panel? In what kind of scenario would it be used?

Quoted 8 lines

Quoted 2 lines

Ideally the panel binding just describes the panel and it should not matter which display controller it is a child of.

Yes, but that means the panel bindings need to have enough information so that all display controllers can use it. Simple-panel bindings do not have enough information. The simple-panel bindings do not have information about the video bus input, and it doesn't even have information about the resolution or bitdepth of the panel.

Some of that you can hide into the panel driver based on the compatible flag. So why not already do something like this in the .dts files instead of the remapping:

compatible = "sharp,ls037v7dw01-omap-dss", "sharp,ls037v7dw01";

And drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c would only claim to be compatible with "sharp,ls037v7dw01-omap-dss".

Then when the common panel framework is available, you can stop parsing sharp,ls037v7dw01-omap-dss but the .dts files don't need to be changed and it's fine to keep "sharp,ls037v7dw01-omap-dss" in the .dts files.

Quoted 4 lines

So I'm still asking, if we create sharp bindings that use the same properties as the simple-panel bindings, and define that sharp panel is compatible with simple-panel, what kind of scenario in practice would it be used in?

Well with the above example, just by dss with "sharp,ls037v7dw01-omap-dss" until some other SoC notices it can use the GPIO parts of the panel code at least :)

Quoted 4 lines

Would the scenario be some other OS, that doesn't have a driver for the sharp panel, but has a driver for the simple-panel? That would only work if the sharp panel hardware is setup so that only the enable gpio is needed, so that quite a narrow definition of "compatible".

That's where we can use the compatible flags and just avoid parsing the generic compatible flag unless some common framework is available.

Quoted 10 lines · Tony Lindgren

Quoted 5 lines

The video paths of the panels and encoders are connected using the v4l2 style ports/endpoints. We can use those to see what display controller a panel is connected to, but only after the panel driver has already probed. We don't have control for the actual probing, as that happens with whatever the control bus is for the display component.

OK. So with generic panels, you can just let the panels probe with the right compatible flag then and let the ports/endpoints registration to figure out if the panel is usable for the display controller in question.

I'm not sure what you mean here.

Do you mean with the future common display framework? There's no need to figure out anything there, as supposedly the .dts has been written correctly and the panel and the display controller work together.

If you mean with the current kernel, there's still nothing to figure out. We can have only single driver with a particular compatible string. And as our current drivers are omap specific, it makes sense to have their compatible string be something omap-ish. And if the .dts file connects the display controller and the panel, then they must be usable together.

Quoted 10 lines

Quoted 7 lines

Quoted 2 lines

Well it seems at least the reset and enable pin standard from that binding can be kept.

Only enable gpio there. But even that's vague. Do you turn on the power before or after setting the enable gpio? How long delay should be between the power and the gpio? Different panels have different rules for the power-up.

Sure, it's a complex problem. But for the enable gpio..

Quoted 3 lines

Maybe the enable GPIO should be treated as a regulator? That would allow specifying first the source regulator startup delay, and then the panel has it's own startup delay.

Well... I don't know. Sounds rather hacky to me. We have the option to have a specific driver for this panel, and that driver can handle all the delays and power-up sequences just right in a clean manner.

Quoted 20 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

But I'm not really familiar with using extending current bindings, and making new bindings compatible with old ones. Can you explain why it'd be good to have the sharp panel bindings compatible with simple-panel? In what kind of scenario would it be used?

Quoted 8 lines

Quoted 2 lines

Ideally the panel binding just describes the panel and it should not matter which display controller it is a child of.

Yes, but that means the panel bindings need to have enough information so that all display controllers can use it. Simple-panel bindings do not have enough information. The simple-panel bindings do not have information about the video bus input, and it doesn't even have information about the resolution or bitdepth of the panel.

Some of that you can hide into the panel driver based on the compatible flag. So why not already do something like this in the .dts files instead of the remapping:

compatible = "sharp,ls037v7dw01-omap-dss", "sharp,ls037v7dw01";

Quoted 7 lines

And drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c would only claim to be compatible with "sharp,ls037v7dw01-omap-dss".

Then when the common panel framework is available, you can stop parsing sharp,ls037v7dw01-omap-dss but the .dts files don't need to be changed and it's fine to keep "sharp,ls037v7dw01-omap-dss" in the .dts files.

Hmm, I don't see how this relates to the simple-panel bindings.

But you're right, having "sharp,ls037v7dw01-omap-dss" in the .dts is an alternative for the compatible-string conversion we do now. I guess it's a matter of taste, but I rather hide it inside the kernel, in an internal omapdss file, than pollute the .dts files with those compatible strings.

Quoted 9 lines

Quoted 4 lines

So I'm still asking, if we create sharp bindings that use the same properties as the simple-panel bindings, and define that sharp panel is compatible with simple-panel, what kind of scenario in practice would it be used in?

Well with the above example, just by dss with "sharp,ls037v7dw01-omap-dss" until some other SoC notices it can use the GPIO parts of the panel code at least :)

Quoted 7 lines

Quoted 4 lines

Would the scenario be some other OS, that doesn't have a driver for the sharp panel, but has a driver for the simple-panel? That would only work if the sharp panel hardware is setup so that only the enable gpio is needed, so that quite a narrow definition of "compatible".

That's where we can use the compatible flags and just avoid parsing the generic compatible flag unless some common framework is available.

Hmm, sorry, I don't follow. My question was about the simple-panel bindings, not common display framework.

You were saying that the sharp bindings should be compatible with simple-panel bindings. I'm still trying to understand what benefit does that give us.

As I see it, the sharp panel could be used with the simple-panel bindings only in certain special case, where all the mode pins and the reset are hardwired in the board hardware, and they are hardwired in a certain state (all hardwired low, probably), which matches what the simple-panel driver expects.

Quoted 5 lines

But you're right, having "sharp,ls037v7dw01-omap-dss" in the .dts is an alternative for the compatible-string conversion we do now. I guess it's a matter of taste, but I rather hide it inside the kernel, in an internal omapdss file, than pollute the .dts files with those compatible strings.

Well it avoid you parsing through all the nodes during booting and leaves out the function to do remapping. And removes the need for maintaining a custom display mapping table. I'd say that's a pretty good list of advantages right there :)

Quoted 10 lines · Tony Lindgren

Quoted 5 lines

But you're right, having "sharp,ls037v7dw01-omap-dss" in the .dts is an alternative for the compatible-string conversion we do now. I guess it's a matter of taste, but I rather hide it inside the kernel, in an internal omapdss file, than pollute the .dts files with those compatible strings.

Well it avoid you parsing through all the nodes during booting and leaves out the function to do remapping. And removes the need for maintaining a custom display mapping table. I'd say that's a pretty good list of advantages right there :)

Yep... I don't know. Maybe I'm being too careful about doing wrong things with .dts. I just like it more if any hacks are in kernel code, which I can remove without anyone noticing.

Anyway, we already have board.dts files using the non-omapified compatible strings in the mainline, so if I would now add the omapified compatible strings to .dts files, those old board.dts files would break.

So I guess the choice has already been made.

Quoted 20 lines · Tomi Valkeinen

Quoted 10 lines · Tony Lindgren

Quoted 5 lines

But you're right, having "sharp,ls037v7dw01-omap-dss" in the .dts is an alternative for the compatible-string conversion we do now. I guess it's a matter of taste, but I rather hide it inside the kernel, in an internal omapdss file, than pollute the .dts files with those compatible strings.

Well it avoid you parsing through all the nodes during booting and leaves out the function to do remapping. And removes the need for maintaining a custom display mapping table. I'd say that's a pretty good list of advantages right there :)

Yep... I don't know. Maybe I'm being too careful about doing wrong things with .dts. I just like it more if any hacks are in kernel code, which I can remove without anyone noticing.

Anyway, we already have board.dts files using the non-omapified compatible strings in the mainline, so if I would now add the omapified compatible strings to .dts files, those old board.dts files would break.

Quoted 1 line

So I guess the choice has already been made.

I really think you should remove this misuse of device tree code ASAP. It's better to fix it up now than carry the hack for next two years and keep on adding to it.

Quoted 21 lines · Tony Lindgren

Quoted 20 lines · Tomi Valkeinen

Quoted 10 lines · Tony Lindgren

Quoted 5 lines

But you're right, having "sharp,ls037v7dw01-omap-dss" in the .dts is an alternative for the compatible-string conversion we do now. I guess it's a matter of taste, but I rather hide it inside the kernel, in an internal omapdss file, than pollute the .dts files with those compatible strings.

Well it avoid you parsing through all the nodes during booting and leaves out the function to do remapping. And removes the need for maintaining a custom display mapping table. I'd say that's a pretty good list of advantages right there :)

Yep... I don't know. Maybe I'm being too careful about doing wrong things with .dts. I just like it more if any hacks are in kernel code, which I can remove without anyone noticing.

Anyway, we already have board.dts files using the non-omapified compatible strings in the mainline, so if I would now add the omapified compatible strings to .dts files, those old board.dts files would break.

Quoted 5 lines

Quoted 1 line

So I guess the choice has already been made.

I really think you should remove this misuse of device tree code ASAP. It's better to fix it up now than carry the hack for next two years and keep on adding to it.

IMHO appending -omap-dss to a random device is an even bigger hack, since its adding lots of bloat to the API. Let's assume there is another OS using DT for ARM, but has no proper API for SPI controllers and it introduces your hack to SPI devices. That would mean each SPI device has -omap-spi appended (or -exynos-spi, -foo-spi, ...). At least I would blame them for creating a huge unmaintainable mess.

I think Tomi's workaround is a much better hack, since it keeps the API clean. If the code simply prefixes "omapdss," to all "child"-devices of omapdss it can be left mostly untouched, too.

Quoted 28 lines · Sebastian Reichel

Quoted 21 lines · Tony Lindgren

Quoted 20 lines · Tomi Valkeinen

Quoted 10 lines · Tony Lindgren

Quoted 5 lines

But you're right, having "sharp,ls037v7dw01-omap-dss" in the .dts is an alternative for the compatible-string conversion we do now. I guess it's a matter of taste, but I rather hide it inside the kernel, in an internal omapdss file, than pollute the .dts files with those compatible strings.

Well it avoid you parsing through all the nodes during booting and leaves out the function to do remapping. And removes the need for maintaining a custom display mapping table. I'd say that's a pretty good list of advantages right there :)

Yep... I don't know. Maybe I'm being too careful about doing wrong things with .dts. I just like it more if any hacks are in kernel code, which I can remove without anyone noticing.

Anyway, we already have board.dts files using the non-omapified compatible strings in the mainline, so if I would now add the omapified compatible strings to .dts files, those old board.dts files would break.

Quoted 5 lines

Quoted 1 line

So I guess the choice has already been made.

I really think you should remove this misuse of device tree code ASAP. It's better to fix it up now than carry the hack for next two years and keep on adding to it.

Quoted 7 lines

IMHO appending -omap-dss to a random device is an even bigger hack, since its adding lots of bloat to the API. Let's assume there is another OS using DT for ARM, but has no proper API for SPI controllers and it introduces your hack to SPI devices. That would mean each SPI device has -omap-spi appended (or -exynos-spi, -foo-spi, ...). At least I would blame them for creating a huge unmaintainable mess.

I think you're misunderstanding. I do not want the naming to be Linux specific. The naming should naturally be as hardware specific as possible. In this case something like:

compatible = "sharp,ls037v7dw01-dss", "sharp,ls037v7dw01";

Or we should probably use:

compatible = "sharp,ls037v7dw01-dpi", "sharp,ls037v7dw01";

As dpi here reflects the hardware it's connected to. The dss is probably a Linux name.

Not use what you're after with the SPI example though, but sounds like that's something different.

Quoted 3 lines

I think Tomi's workaround is a much better hack, since it keeps the API clean. If the code simply prefixes "omapdss," to all "child"-devices of omapdss it can be left mostly untouched, too.

AFAIK that remapping not needed at all. All that is already available with the compatible flags.

Quoted 2 lines

AFAIK that remapping not needed at all. All that is already available with the compatible flags.

And alternatively we can also just use the sharp,ls037v7dw01 in the panel driver(s). We can have the panels bail out in driver probe based on of_machine_is_compatible if nothing else is available.

Also, currently the remapping code also probably keeps prepending more and more omapdss,omapdss,omapdss,... on each kexec reboot? And

it would be quite silly for each display controller to have to do their own remapping for shared panels?

If we still still despite all these reasons decide to go with the remapping of the compatible strings, it should really be a Linux generic (or drivers/video generic function), not implemented for each display controller.

Quoted 6 lines · Tony Lindgren

Quoted 2 lines

AFAIK that remapping not needed at all. All that is already available with the compatible flags.

And alternatively we can also just use the sharp,ls037v7dw01 in the panel driver(s). We can have the panels bail out in driver probe based on of_machine_is_compatible if nothing else is available.

I don't think that would work. Then we could have two panel drivers, both loaded into the kernel and both having the same driver name and the same compatible-string.

I'm not sure if it's even possible to load both of those drivers at the same time, but if it was, and the first one would get probe() called for a device, and the driver would return an error, I'm quite sure the driver framework won't continue trying with the other driver, as the first driver already matched for the device.

Quoted 2 lines

Also, currently the remapping code also probably keeps prepending more and more omapdss,omapdss,omapdss,... on each kexec reboot? And

I don't think so. The code looks for, say, "ti,tfp410", and it wouldn't match for "omapdss,ti,tfp410".

Quoted 2 lines

it would be quite silly for each display controller to have to do their own remapping for shared panels?

It would, but wouldn't it be just as silly (well, even more silly in my opinion) to have each display controller require driver specific compatible strings for the panels used?

Until we have a common display framework, we need _some_ way to handle this problem. Either we mess up the .dts files as you suggest, or we have a driver internal hack.

Quoted 4 lines

If we still still despite all these reasons decide to go with the remapping of the compatible strings, it should really be a Linux generic (or drivers/video generic function), not implemented for each display controller.

Well, I sure hope nobody else has to implement similar thing.

The reason why we have this code, and others do not, is mainly that omapdss is maybe the first driver to implement the display components properly, both in the SW side and in the .dts side, and that I wanted omapdss to behave correctly even if there are other display subsystems/panels compiled into the same kernel.

To open that up a bit, traditionally other display driver architectures have not had drivers for each display "entity", like the panels. So you would really just have the display subsystem in the .dts, and some raw data there (panel timings) to get the panel running.

Now, with omapdss, we have separate drivers for each display entity. Unfortunately those drivers are all tied to the omapdss driver's API (and cannot thus be used on anything else than OMAP), as there's not yet a driver framework for display entities (DRM is a bit different thing, in my opinion).

Everything with that was simple with platform data, as the omap board files created the panel devices, and there were no name clashing problems with other platforms.

With DT that changed, as the bindings are common, and thus the compatible strings are common also. But we still have omapdss specific panel drivers. This is the reason we do the compatible-string conversion hack. As soon as we can create platform agnostic panel drivers, we can remove the hack.

And, I want to remind, it's an omapdss internal hack (after the patch I sent), not visible to anyone else. The .dts files, the arch/arm files, they are all not touched. So I don't quite understand why you see it as such a bad thing. It's ugly, sure, but what harm does it do (except some maintainer burden, for me)?

No need to load multiple drivers at this point. That's why I'm saying you can bail out on the undesired display controller probes using of_machine_is_compatible test. It's not that uncommon for drivers to do:

$ git grep of_machine_is_compatible drivers/ | wc -l
116

Quoted 5 lines

I'm not sure if it's even possible to load both of those drivers at the same time, but if it was, and the first one would get probe() called for a device, and the driver would return an error, I'm quite sure the driver framework won't continue trying with the other driver, as the first driver already matched for the device.

No need to load multiple drivers at this point. That's why I'm saying you can bail out on the undesired display controller probes using of_machine_is_compatible test. It's not that uncommon for drivers to do:

Quoted 6 lines

Quoted 2 lines

it would be quite silly for each display controller to have to do their own remapping for shared panels?

It would, but wouldn't it be just as silly (well, even more silly in my opinion) to have each display controller require driver specific compatible strings for the panels used?

Not driver specific, but hardware package specific. That's being done all the time. For example, compatible strings like "nvidia,tegra124", "ti,omap5" describe a package of an ARM core and various devices.

But by using of_machine_is_compatible in the display drivers, you can use the right compatible flags from the start if you want to go that way.

Quoted 1 line

Well, I sure hope nobody else has to implement similar thing.

Suuuure.. I've heard that about 20 times before and guess what happenened? Things never got fixed until some poor maintainer had to start fixing it all over the place about three years later.

Quoted 4 lines

To open that up a bit, traditionally other display driver architectures have not had drivers for each display "entity", like the panels. So you would really just have the display subsystem in the .dts, and some raw data there (panel timings) to get the panel running.

Right, I believe that is the way to go, no doubt about it.

Here's a list of things that bothers me:

1. Searching through the dtb from a driver instead of relying on the driver probe mechanism for the components in question. If the parsing of the dtb needs to be done it should be done in a Linux generic way from some framework instead.

2. Having to do this all early before we even have any debug console available. We are trying to move everything to happen later on to avoid all the issues related to initializing things early.

3. Having to maintain a database in kernel of display mappings separately in addition to the .dts files.

4. Having this hack limited to device tree based booting while we are trying to unify the functions for drivers to use to get their resources.

5. Seeing the possibility of this spreading to other drivers.

Quoted 9 lines · Tony Lindgren

Quoted 5 lines

I'm not sure if it's even possible to load both of those drivers at the same time, but if it was, and the first one would get probe() called for a device, and the driver would return an error, I'm quite sure the driver framework won't continue trying with the other driver, as the first driver already matched for the device.

No need to load multiple drivers at this point. That's why I'm saying you can bail out on the undesired display controller probes using of_machine_is_compatible test. It's not that uncommon for drivers to do:

Hmm, I don't follow. If both, say, omap and exynos have a panel driver for the same sharp panel, both need a driver for it (presuming exynos has a proper display component driver system, which may not be true).

Both drivers would be loaded at boot time for the probe to be even to be possible. Even if the other one bails out from a device probe using of_machine_is_compatible, afaik the driver framework will stop probing at that point, as that driver reports being compatible with the device in question.

...

Ah, ok, now I see. I think you mean module init, not probe? For some reason I was just thinking about the probe stage.

Maybe that would work. I need to test tomorrow.

Quoted 10 lines

Quoted 6 lines

Quoted 2 lines

it would be quite silly for each display controller to have to do their own remapping for shared panels?

It would, but wouldn't it be just as silly (well, even more silly in my opinion) to have each display controller require driver specific compatible strings for the panels used?

Not driver specific, but hardware package specific. That's being done all the time. For example, compatible strings like "nvidia,tegra124", "ti,omap5" describe a package of an ARM core and various devices.

It feels to me rather wrong if I'd specify the compatible string for an external piece of hardware based on the SoC it's connected to.

Quoted 3 lines

But by using of_machine_is_compatible in the display drivers, you can use the right compatible flags from the start if you want to go that way.

Yes, with that we could have right compatible flags in the .dts.

Quoted 2 lines

Quoted 1 line

Well, I sure hope nobody else has to implement similar thing.

Quoted 3 lines

Suuuure.. I've heard that about 20 times before and guess what happenened? Things never got fixed until some poor maintainer had to start fixing it all over the place about three years later.

The poor maintainer here being me =). And it's still a case of pick-your-poison. We need to hack around the issue, in some form or another. So in any case the poor maintainer has to fix it at some point in the future.

But all this is fixed when we have a common display framework. And that's something that is really badly needed in some form or another, the sooner the better.

So the "fix" here is not just some internal thing that could be left at that and forgotten.

Quoted 6 lines

Quoted 4 lines

To open that up a bit, traditionally other display driver architectures have not had drivers for each display "entity", like the panels. So you would really just have the display subsystem in the .dts, and some raw data there (panel timings) to get the panel running.

Right, I believe that is the way to go, no doubt about it.

No, that's exactly _not_ the way to go =). We need proper drivers for display components, instead of trying to avoid that and embed panel data into the display controller's data.

Quoted 6 lines

Here's a list of things that bothers me:

1. Searching through the dtb from a driver instead of relying on the driver probe mechanism for the components in question. If the parsing of the dtb needs to be done it should be done in a Linux generic way from some framework instead.

The reason I haven't made the conversion code available for others to use is that there are no other users at the moment. And I hope there will be no other user until we get the common display framework. If there is, we'll need to move the code to a generic place.

Quoted 3 lines

2. Having to do this all early before we even have any debug console available. We are trying to move everything to happen later on to avoid all the issues related to initializing things early.

It worked fine for me when doing it as subsys_init level. Why do you say it'd be early, before debug console?

All the drivers with converted compatible strings are normal drivers, so afaik things work fine if we just convert the strings before the module init level.

Quoted 2 lines

3. Having to maintain a database in kernel of display mappings separately in addition to the .dts files.

Yes, that's slightly annoying. Not really a big burden, though, as it's quite rare to get a new encoder or panel driver.

Quoted 3 lines

4. Having this hack limited to device tree based booting while we are trying to unify the functions for drivers to use to get their resources.

I don't understand this one. With non-DT boot we don't have the issue at all, there's no need for hacks.

Quoted 1 line

5. Seeing the possibility of this spreading to other drivers.

Well, until we have a common display framework, one of the hacky options we've discussed will spread to other drivers if they need to have their own drivers for the same hardware devices.

Which is not nice, but not something we can escape. And using the of_machine_is_compatible or having the compatible strings in .dts appended with "dss" or such doesn't change that, it just changes which hack may spread.

Quoted 6 lines

Quoted 3 lines

4. Having this hack limited to device tree based booting while we are trying to unify the functions for drivers to use to get their resources.

I don't understand this one. With non-DT boot we don't have the issue at all, there's no need for hacks.

Hmm can't we still have multiarch booting happening with the same panel name being used by two different display drivers?

Quoted 6 lines

Quoted 1 line

5. Seeing the possibility of this spreading to other drivers.

Well, until we have a common display framework, one of the hacky options we've discussed will spread to other drivers if they need to have their own drivers for the same hardware devices.

Quoted 4 lines

Which is not nice, but not something we can escape. And using the of_machine_is_compatible or having the compatible strings in .dts appended with "dss" or such doesn't change that, it just changes which hack may spread.

Yes I agree they are all hacks, but your version seems to carry some extra early init baggage with it as I noted above :)

I'll try and see how the other options work, which are:

- Bailing out from module_init in each display driver. The reason I don't like this (although I haven't tried it) is that all the display drivers need the modification, and because I need to catch the module_init, I cannot use the helpers like module_platform_driver(), so it adds multiple lines to every driver.

- Traveling the video graph, starting from omapdss. This one is possibly better performance-wise than my original version, as we only need to search for the omapdss node and can then follow the links. But the code will be more complex.

Quoted 2 lines

3. Having to maintain a database in kernel of display mappings separately in addition to the .dts files.

Yes, that's slightly annoying. Not really a big burden, though, as it's quite rare to get a new encoder or panel driver.

Hi,

Quoted 5 lines · Tomi Valkeinen

Quoted 2 lines

3. Having to maintain a database in kernel of display mappings separately in addition to the .dts files.

Yes, that's slightly annoying. Not really a big burden, though, as it's quite rare to get a new encoder or panel driver.

I wonder if the "omapdss," prefix could be added to *all* remote endpoints seen by omapdss. Pseudocode:

for each port in dss {

    for each endpoint in port {
        panel = get_phandle(endpoint, "remote-endpoint");

        /* only add prefix if it's not already there (kexec) */
        if (panel.compatible != "omapdss,.*")
            panel.compatible = "omapdss," + panel.compatible;
    }
}

Quoted 7 lines

IMHO appending -omap-dss to a random device is an even bigger hack, since its adding lots of bloat to the API. Let's assume there is another OS using DT for ARM, but has no proper API for SPI controllers and it introduces your hack to SPI devices. That would mean each SPI device has -omap-spi appended (or -exynos-spi, -foo-spi, ...). At least I would blame them for creating a huge unmaintainable mess.

I think you're misunderstanding. I do not want the naming to be Linux specific. The naming should naturally be as hardware specific as possible. In this case something like:

compatible = "sharp,ls037v7dw01-dss", "sharp,ls037v7dw01";

Or we should probably use:

compatible = "sharp,ls037v7dw01-dpi", "sharp,ls037v7dw01";

As dpi here reflects the hardware it's connected to. The dss is probably a Linux name.

Not use what you're after with the SPI example though, but sounds like that's something different.

Quoted 12 lines · Tony Lindgren

Quoted 7 lines

IMHO appending -omap-dss to a random device is an even bigger hack, since its adding lots of bloat to the API. Let's assume there is another OS using DT for ARM, but has no proper API for SPI controllers and it introduces your hack to SPI devices. That would mean each SPI device has -omap-spi appended (or -exynos-spi, -foo-spi, ...). At least I would blame them for creating a huge unmaintainable mess.

I think you're misunderstanding. I do not want the naming to be Linux specific. The naming should naturally be as hardware specific as possible. In this case something like:

Quoted 8 lines

compatible = "sharp,ls037v7dw01-dss", "sharp,ls037v7dw01";

Or we should probably use:

compatible = "sharp,ls037v7dw01-dpi", "sharp,ls037v7dw01";

As dpi here reflects the hardware it's connected to. The dss is probably a Linux name.

Well, "dss" or "omapdss" is as much a hardware term as "dpi". And "dpi" wouldn't really be a good extension, as what we want is an omapdss driver specific compatible string. So I think "-omapdss" is the best extension if that method is used.

But I don't think that's really the point. The point is that the panel's compatible string should be "sharp,ls037v7dw01", nothing else.

All the variations of "sharp,ls037v7dw01-dss" are not correct, and are only made for Linux SW reasons. So I would say they are Linux specific SW names, even if the words themselves are also HW terms.

Quoted 2 lines

Not use what you're after with the SPI example though, but sounds like that's something different.

I think Sebastien's example is just like the issue here.

In many cases however we do have multiple compatible strings that describe how the device is wired. See drivers/tty/serial/of_serial.c for example. It has "ns16550" but then it also has additional "nvidia,tegra20-uart", "nxp,lpc3220-uart" and "ibm,qpace-nwp-serial".

Quoted 4 lines

Quoted 2 lines

Not use what you're after with the SPI example though, but sounds like that's something different.

I think Sebastien's example is just like the issue here.

Hmm is there some existing example in the kernel like that?

Quoted 4 lines · Tony Lindgren

In many cases however we do have multiple compatible strings that describe how the device is wired. See drivers/tty/serial/of_serial.c for example. It has "ns16550" but then it also has additional "nvidia,tegra20-uart", "nxp,lpc3220-uart" and "ibm,qpace-nwp-serial".

All those sound like SoC components. In that case it sounds fine to have the device compatible contain the SoC name. We're talking here about external, detachable devices.

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

Not use what you're after with the SPI example though, but sounds like that's something different.

Quoted 3 lines

Quoted 1 line

I think Sebastien's example is just like the issue here.

Hmm is there some existing example in the kernel like that?

No, Sebastien's example was just a hypothetical case. Here, using your way of having SoC specific data in the .dts, we would have "sharp,ls037v7dw01-omap-dss", and in Sebastien's example with a touch sensor we'd have, say, "synaptics,xyz123-omap-spi".

Quoted 10 lines · Tomi Valkeinen

Quoted 4 lines · Tony Lindgren

In many cases however we do have multiple compatible strings that describe how the device is wired. See drivers/tty/serial/of_serial.c for example. It has "ns16550" but then it also has additional "nvidia,tegra20-uart", "nxp,lpc3220-uart" and "ibm,qpace-nwp-serial".

All those sound like SoC components. In that case it sounds fine to have the device compatible contain the SoC name. We're talking here about external, detachable devices.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

Not use what you're after with the SPI example though, but sounds like that's something different.

I think Sebastien's example is just like the issue here.

Quoted 6 lines

Quoted 1 line

Hmm is there some existing example in the kernel like that?

No, Sebastien's example was just a hypothetical case. Here, using your way of having SoC specific data in the .dts, we would have "sharp,ls037v7dw01-omap-dss", and in Sebastien's example with a touch sensor we'd have, say, "synaptics,xyz123-omap-spi".

Yes, that's what I wanted to say :)

Quoted 11 lines · Sebastian Reichel

Quoted 10 lines · Tomi Valkeinen

Quoted 4 lines · Tony Lindgren

In many cases however we do have multiple compatible strings that describe how the device is wired. See drivers/tty/serial/of_serial.c for example. It has "ns16550" but then it also has additional "nvidia,tegra20-uart", "nxp,lpc3220-uart" and "ibm,qpace-nwp-serial".

All those sound like SoC components. In that case it sounds fine to have the device compatible contain the SoC name. We're talking here about external, detachable devices.

Yes I agree it's also hackish. But playing with the compatible flags avoids adding other custom workarounds.

Quoted 12 lines

Quoted 11 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Not use what you're after with the SPI example though, but sounds like that's something different.

I think Sebastien's example is just like the issue here.

Hmm is there some existing example in the kernel like that?

No, Sebastien's example was just a hypothetical case. Here, using your way of having SoC specific data in the .dts, we would have "sharp,ls037v7dw01-omap-dss", and in Sebastien's example with a touch sensor we'd have, say, "synaptics,xyz123-omap-spi".

Quoted 1 line

Yes, that's what I wanted to say :)

Hmm I think we need to add few things to the hypothetical SPI case to get it even close to the panel example. Like assume that no SPI bus exists, [...]

[...] and that potentially multiple different Synaptics drivers are trying to share the same compatible flag for a non-existing generic binding.

I think then it's getting close to the panel example :)

Quoted 6 lines · Tony Lindgren

Quoted 5 lines · Tomi Valkeinen

Quoted 3 lines · Tony Lindgren

BTW, I'm also personally fine with all five gpios showing in a single gpios property, I'm not too exited about naming anything in DT..

Quoted 8 lines

Quoted 4 lines

I don't have a strong opinion here. I don't have much experience with DT, especially with making bindings compatible with other ones.

I'd just forget the simple-panel, and have single gpio array.

Well if it's a don't care flag for both of us, let's try to use the existing standard for simple-panel.txt and add mode-gpios property. I'll post a patch for that.

Here's an updated version using enable-gpios, reset-gpios and mode-gpios. So it follows simple-panel.txt and adds mode-gpios that's currently specific to this panel only.

Also updated for -EPROBE_DEFER handling, tested that by changing one of the GPIOs to be a twl4030 GPIO.

From f8360778e8bc96096cbb1793a18a8c240376ca09 Mon Sep 17 00:00:00 2001

Quoted 20 lines · Tony Lindgren

Quoted 6 lines · Tony Lindgren

Quoted 5 lines · Tomi Valkeinen

Quoted 3 lines · Tony Lindgren

BTW, I'm also personally fine with all five gpios showing in a single gpios property, I'm not too exited about naming anything in DT..

Quoted 8 lines

Quoted 4 lines

I don't have a strong opinion here. I don't have much experience with DT, especially with making bindings compatible with other ones.

I'd just forget the simple-panel, and have single gpio array.

Well if it's a don't care flag for both of us, let's try to use the existing standard for simple-panel.txt and add mode-gpios property. I'll post a patch for that.

Here's an updated version using enable-gpios, reset-gpios and mode-gpios. So it follows simple-panel.txt and adds mode-gpios that's currently specific to this panel only.

Quoted 2 lines

Also updated for -EPROBE_DEFER handling, tested that by changing one of the GPIOs to be a twl4030 GPIO.

To speed things up a bit, I made the changes I suggested. Compile tested only.

Quoted 21 lines · Tomi Valkeinen

Quoted 20 lines · Tony Lindgren

Quoted 6 lines · Tony Lindgren

Quoted 5 lines · Tomi Valkeinen

Quoted 3 lines · Tony Lindgren

BTW, I'm also personally fine with all five gpios showing in a single gpios property, I'm not too exited about naming anything in DT..

Quoted 8 lines

Quoted 4 lines

I don't have a strong opinion here. I don't have much experience with DT, especially with making bindings compatible with other ones.

I'd just forget the simple-panel, and have single gpio array.

Well if it's a don't care flag for both of us, let's try to use the existing standard for simple-panel.txt and add mode-gpios property. I'll post a patch for that.

Here's an updated version using enable-gpios, reset-gpios and mode-gpios. So it follows simple-panel.txt and adds mode-gpios that's currently specific to this panel only.

Quoted 5 lines

Quoted 2 lines

Also updated for -EPROBE_DEFER handling, tested that by changing one of the GPIOs to be a twl4030 GPIO.

To speed things up a bit, I made the changes I suggested. Compile tested only.

OK thanks did not get the penguin with it so need to look at it a bit more.

drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c static int sharp_ls_probe_pdata(struct platform_device *pdev) · line 328

Quoted 8 lines · Tony Lindgren

drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c +8 -0

+	/* lcd MO */
+	ddata->mo_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 1, "mode");
+	if (PTR_ERR(ddata->mo_gpio) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (!IS_ERR(ddata->mo_gpio))
+		if (gpiod_get_raw_value_cansleep(ddata->mo_gpio))
+			ddata->flags |= SHARP_LS_QVGA;

Shouldn't there be an explicit flag in the DT data for this? If the panel's MO pin is hardwired to, say, pull up, then the mode-gpios won't have MO gpio, right? So something like:

mode-gpios = <0 /* high, lcd MO */

	      &gpio1 2 GPIO_ACTIVE_HIGH		/* gpio2, lcd LR */
	      &gpio1 3 GPIO_ACTIVE_HIGH>;	/* gpio3, lcd UD */

vga-mode; /* MO hardwired high */

Btw, the gpio.txt has each gpio inside <>:

chipsel-gpios = <&gpio1 12 0>,

		 <&gpio1 13 0>,
		 <0>, /* holes are permitted, means no GPIO 2 */
		 <&gpio2 2>;

Is that equivalent to having all gpios inside <>?

Quoted 20 lines · Tomi Valkeinen

drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c static int sharp_ls_probe_pdata(struct platform_device *pdev) · line 328

Quoted 8 lines · Tony Lindgren

drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c +8 -0

+	/* lcd MO */
+	ddata->mo_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 1, "mode");
+	if (PTR_ERR(ddata->mo_gpio) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (!IS_ERR(ddata->mo_gpio))
+		if (gpiod_get_raw_value_cansleep(ddata->mo_gpio))
+			ddata->flags |= SHARP_LS_QVGA;

Shouldn't there be an explicit flag in the DT data for this? If the panel's MO pin is hardwired to, say, pull up, then the mode-gpios won't have MO gpio, right? So something like:

mode-gpios = <0 /* high, lcd MO */

	      &gpio1 2 GPIO_ACTIVE_HIGH		/* gpio2, lcd LR */
	      &gpio1 3 GPIO_ACTIVE_HIGH>;	/* gpio3, lcd UD */

Quoted 1 line

vga-mode; /* MO hardwired high */

Yeah holes there are just fine. I figured let's keep the custom vga-mode property out of the way until we actually run into a panel that's not using a GPIO for mode.

Quoted 23 lines · Tony Lindgren

Quoted 21 lines · Tomi Valkeinen

drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c static int sharp_ls_probe_pdata(struct platform_device *pdev) · line 328

Quoted 8 lines · Tony Lindgren

drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c +8 -0

+	/* lcd MO */
+	ddata->mo_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 1, "mode");
+	if (PTR_ERR(ddata->mo_gpio) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (!IS_ERR(ddata->mo_gpio))
+		if (gpiod_get_raw_value_cansleep(ddata->mo_gpio))
+			ddata->flags |= SHARP_LS_QVGA;

Shouldn't there be an explicit flag in the DT data for this? If the panel's MO pin is hardwired to, say, pull up, then the mode-gpios won't have MO gpio, right? So something like:

mode-gpios = <0 /* high, lcd MO */

	      &gpio1 2 GPIO_ACTIVE_HIGH		/* gpio2, lcd LR */
	      &gpio1 3 GPIO_ACTIVE_HIGH>;	/* gpio3, lcd UD */

vga-mode; /* MO hardwired high */

Quoted 3 lines

Yeah holes there are just fine. I figured let's keep the custom vga-mode property out of the way until we actually run into a panel that's not using a GPIO for mode.

Ok, I'm fine with that, but in that case I think we have to use all the panels in the same mode, i.e. the driver either sets the MO gpio always low and uses VGA mode, or sets the MO always high and uses QVGA mode.

In your omap3-evm.dts patch, you set the MO gpio ACTIVE_LOW in order to change the mode, even if it really should be ACTIVE_HIGH. But if you do that, and we later add the support to the panel driver to manage the MO gpio dynamically (i.e. the user can select VGA/QVGA at runtime), then the panel won't work as the driver uses wrong polarity for the pin.

drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c +6 -0

+static int sharp_ls_probe_of(struct platform_device *pdev)
+{
+	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+	struct device_node *node = pdev->dev.of_node;
+	struct omap_dss_device *in;
+

Quoted 6 lines · Tony Lindgren

drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c +6 -0

+static int sharp_ls_probe_of(struct platform_device *pdev)
+{
+	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+	struct device_node *node = pdev->dev.of_node;
+	struct omap_dss_device *in;
+

drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c static int sharp_ls_probe_pdata(struct platform_device *pdev) · line 314

Quoted 10 lines

drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c +10 -0

+	ddata->vcc = devm_regulator_get(&pdev->dev, "envdd");
+	if (IS_ERR(ddata->vcc)) {
+		dev_err(&pdev->dev, "failed to get regulator\n");
+		return PTR_ERR(ddata->vcc);
+	}
+
+	/* lcd INI */
+	ddata->ini_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 0, "enable");
+	if (PTR_ERR(ddata->ini_gpio) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;

Hmm, the GPIOs are optional, but shouldn't we react somehow to real errors? I guess we should do something like:

ddata->ini_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 0, "enable");

if (IS_ERR(ddata->ini_gpio) {
	int err = PTR_ERR(ddata->ini_gpio);
	if (err == -EPROBE_DEFER || err != -ENOENT)
		return err;
}

Looks like quite a few omaps have sharp ls037v7dw01 that's configured as various panel dpi entries for whatever legacy reasons. For device tree based support, let's just configure these properly for panel ls037v7dw01 instead of panel dpi.

This patch creates a common file for panel ls037v7dw01, and makes boards ldp and omap3-evm to use it. The panel for ldp is configured in the qvga mode and omap3-evm panel in vga mode.

The ls037v7dw01 also seems to be coupled with an ad7846 touchscreen controller for the omaps, so let's add a basic configuration for the touchscreen also using the default values.

Note that we can now remove the regulator-name = "vdds_dsi" entry for ldp, that's no longer needed as we have the entry for vdds_dsi-supply = <&vpll2>.

Signed-off-by Tony Lindgren <[email protected]>

---
 .../arm/boot/dts/omap-panel-sharp-ls037v7dw01.dtsi | 82 ++++++++++++++++++++++
 arch/arm/boot/dts/omap3-evm-37xx.dts               | 50 +++++++++++++
 arch/arm/boot/dts/omap3-evm-common.dtsi            | 47 +++++++++++++
 arch/arm/boot/dts/omap3-ldp.dts                    | 31 ++++++--
 4 files changed, 205 insertions(+), 5 deletions(-)
 create mode 100644 arch/arm/boot/dts/omap-panel-sharp-ls037v7dw01.dtsi

arch/arm/boot/dts/omap3-ldp.dts +1 -0

index 0abe986..50fdac9 100644
@@ -164,6 +164,7 @@
 
 #include "twl4030.dtsi"
 #include "twl4030_omap3.dtsi"
+#include "omap-panel-sharp-ls037v7dw01.dtsi"
 
 &i2c2 {
        clock-frequency = <400000>;

@@ -173,6 +174,31 @@
        clock-frequency = <400000>;

Quoted 42 lines · Tony Lindgren

Looks like quite a few omaps have sharp ls037v7dw01 that's configured as various panel dpi entries for whatever legacy reasons. For device tree based support, let's just configure these properly for panel ls037v7dw01 instead of panel dpi.

This patch creates a common file for panel ls037v7dw01, and makes boards ldp and omap3-evm to use it. The panel for ldp is configured in the qvga mode and omap3-evm panel in vga mode.

The ls037v7dw01 also seems to be coupled with an ad7846 touchscreen controller for the omaps, so let's add a basic configuration for the touchscreen also using the default values.

Note that we can now remove the regulator-name = "vdds_dsi" entry for ldp, that's no longer needed as we have the entry for vdds_dsi-supply = <&vpll2>.

Signed-off-by Tony Lindgren <[email protected]>

---
 .../arm/boot/dts/omap-panel-sharp-ls037v7dw01.dtsi | 82 ++++++++++++++++++++++
 arch/arm/boot/dts/omap3-evm-37xx.dts               | 50 +++++++++++++
 arch/arm/boot/dts/omap3-evm-common.dtsi            | 47 +++++++++++++
 arch/arm/boot/dts/omap3-ldp.dts                    | 31 ++++++--
 4 files changed, 205 insertions(+), 5 deletions(-)
 create mode 100644 arch/arm/boot/dts/omap-panel-sharp-ls037v7dw01.dtsi

arch/arm/boot/dts/omap3-ldp.dts +1 -0

index 0abe986..50fdac9 100644
@@ -164,6 +164,7 @@
 
 #include "twl4030.dtsi"
 #include "twl4030_omap3.dtsi"
+#include "omap-panel-sharp-ls037v7dw01.dtsi"
 
 &i2c2 {
        clock-frequency = <400000>;

@@ -173,6 +174,31 @@
        clock-frequency = <400000>;
 };
 

arch/arm/boot/dts/omap3-ldp.dts line 184

Quoted 8 lines

arch/arm/boot/dts/omap3-ldp.dts +8 -0

+&lcd_3v3 {
+       gpio = <&twl_gpio 7 GPIO_ACTIVE_HIGH>;
+       enable-active-high;
+};
+
+&lcd0 {
+       reset-gpios = <&gpio2 23 GPIO_ACTIVE_HIGH>;     /* gpio55, lcd RESB */
+       gpios = <&gpio2 24 GPIO_ACTIVE_LOW      /* gpio56, lcd MO */

enable-gpios ?

Quoted 43 lines · Joachim Eastwood

Quoted 42 lines · Tony Lindgren

Looks like quite a few omaps have sharp ls037v7dw01 that's configured as various panel dpi entries for whatever legacy reasons. For device tree based support, let's just configure these properly for panel ls037v7dw01 instead of panel dpi.

This patch creates a common file for panel ls037v7dw01, and makes boards ldp and omap3-evm to use it. The panel for ldp is configured in the qvga mode and omap3-evm panel in vga mode.

The ls037v7dw01 also seems to be coupled with an ad7846 touchscreen controller for the omaps, so let's add a basic configuration for the touchscreen also using the default values.

Note that we can now remove the regulator-name = "vdds_dsi" entry for ldp, that's no longer needed as we have the entry for vdds_dsi-supply = <&vpll2>.

Signed-off-by Tony Lindgren <[email protected]>

---
 .../arm/boot/dts/omap-panel-sharp-ls037v7dw01.dtsi | 82 ++++++++++++++++++++++
 arch/arm/boot/dts/omap3-evm-37xx.dts               | 50 +++++++++++++
 arch/arm/boot/dts/omap3-evm-common.dtsi            | 47 +++++++++++++
 arch/arm/boot/dts/omap3-ldp.dts                    | 31 ++++++--
 4 files changed, 205 insertions(+), 5 deletions(-)
 create mode 100644 arch/arm/boot/dts/omap-panel-sharp-ls037v7dw01.dtsi

arch/arm/boot/dts/omap3-ldp.dts +1 -0

index 0abe986..50fdac9 100644
@@ -164,6 +164,7 @@
 
 #include "twl4030.dtsi"
 #include "twl4030_omap3.dtsi"
+#include "omap-panel-sharp-ls037v7dw01.dtsi"
 
 &i2c2 {
        clock-frequency = <400000>;

@@ -173,6 +174,31 @@
        clock-frequency = <400000>;
 };
 

Quoted 10 lines

arch/arm/boot/dts/omap3-ldp.dts line 184

Quoted 8 lines

arch/arm/boot/dts/omap3-ldp.dts +8 -0

+&lcd_3v3 {
+       gpio = <&twl_gpio 7 GPIO_ACTIVE_HIGH>;
+       enable-active-high;
+};
+
+&lcd0 {
+       reset-gpios = <&gpio2 23 GPIO_ACTIVE_HIGH>;     /* gpio55, lcd RESB */
+       gpios = <&gpio2 24 GPIO_ACTIVE_LOW      /* gpio56, lcd MO */

enable-gpios ?

Oops yes, changed from gpios to enable-gpios while reading the panel binding doc, probably forgot to commit the change, will update.

Quoted 5 lines · Tony Lindgren

Quoted 4 lines · Joachim Eastwood

Quoted 2 lines · Tony Lindgren

(quoted diff) +2 -0

+       reset-gpios = <&gpio2 23 GPIO_ACTIVE_HIGH>;     /* gpio55, lcd RESB */
+       gpios = <&gpio2 24 GPIO_ACTIVE_LOW      /* gpio56, lcd MO */

Quoted 4 lines

Quoted 1 line

enable-gpios ?

Oops yes, changed from gpios to enable-gpios while reading the panel binding doc, probably forgot to commit the change, will update.

Here's an updated version of this one.

arch/arm/boot/dts/omap-panel-sharp-ls037v7dw01.dtsi new file +7 -0

+/ {
+	aliases {
+		display0 = &lcd0;
+	};
+
+	backlight0: backlight {
+		compatible = "gpio-backlight";

arch/arm/boot/dts/omap-panel-sharp-ls037v7dw01.dtsi line 7

Quoted 10 lines · Tony Lindgren

arch/arm/boot/dts/omap-panel-sharp-ls037v7dw01.dtsi new file +7 -0

@@ -0,0 +1,82 @@
+/*
+ * Common file for omap dpi panels with QVGA and reset pins
+ *
+ * Note that the board specifc DTS file needs to specify
+ * at minimum the GPIO enable-gpios for display, and
+ * gpios for gpio-backlight.
+ */

This looks very board specific to me... The regulator and the use of mcspi1 depend on the board, so this file can't be used on just any omap board with the same panel. And this can (probably) only be used on boards with a single display. Do those boards have tv-out?

So I have nothing against having common files, but shouldn't this be named something more specific? If the boards involved are TI's OMAP3 development boards, maybe this should be something like... omap3-ti-dev-panel-sharp-ls037v7dw01.dtsi. Well, that's a quite long one.

Quoted 11 lines

arch/arm/boot/dts/omap-panel-sharp-ls037v7dw01.dtsi new file +11 -0

+/ {
+	aliases {
+		display0 = &lcd0;
+	};
+
+	backlight0: backlight {
+		compatible = "gpio-backlight";
+	};
+
+	/* 3.3V GPIO controlled regulator for LCD_ENVDD */
+	lcd_3v3: regulator-lcd-3v3 {

arch/arm/boot/dts/omap-panel-sharp-ls037v7dw01.dtsi line 25

Quoted 6 lines

arch/arm/boot/dts/omap-panel-sharp-ls037v7dw01.dtsi new file +6 -0

+		compatible = "regulator-fixed";
+		regulator-name = "lcd_3v3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		startup-delay-us = <70000>;
+		regulator-always-on;

Why always-on?

Quoted 2 lines · Tomi Valkeinen

On 09/05/14 02:36, Tony Lindgren wrote:
Why always-on?

Oops, yeah that should not be there. The GPIO is board specific.

arch/arm/boot/dts/omap3-panel-sharp-ls037v7dw01.dtsi line 50

Quoted 10 lines · Tony Lindgren

+&dss {

arch/arm/boot/dts/omap3-panel-sharp-ls037v7dw01.dtsi new file +9 -0

+	status = "ok";
+	vdds_dsi-supply = <&vpll2>;
+	port {
+		dpi_out: endpoint {
+			remote-endpoint = <&lcd_in>;
+			data-lines = <18>;
+		};
+	};
+};

I just noticed the vdds_dsi-supply there. While the driver currently uses that if available, I think it should be removed, and done the same way Florian did with his overo patches:

/* Needed to power the DPI pins */

&vpll2 {
	regulator-always-on;
};

That supply is not DSS's supply, but it's used to power up the pins. If the pins were used as GPIOs, that power should be enabled.