[PATCH] video: fbdev: mxsfb: fix pixelclock polarity

The PIXDATA flags of the display_flags enum are controller centric,
e.g. NEGEDGE means the controller shall drive the data signals on
pixelclocks negative edge. However, the drivers flag is display
centric: Sample the data on negative (falling) edge.

Therefore, change the if statement to check for the POSEDGE flag
(which is typically not set):
Drive on positive edge => sample on negative edge

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Hi all

Shawn, I would like to have at least your Ack on this before merge.

It seems that this has been wrong since the driver is able to use
the timings from the device tree, introduced with 669406534b4a
("video: mxsfb: get display timings from device tree").

Not sure how many device trees actually specify the wrong pixel clock
polarity due to that. At least the initial flag convertion from the
old platform data structures done with 0d9f8217db15 ("ARM: mxs: move
display timing configurations into device tree") seems to be affected
and would need to be changed accordingly...

Not sure how we should handle this, maybe just invert all
pixelclk-active properties where the mxsfb driver is in use...?

--
Stefan

 drivers/video/fbdev/mxsfb.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/mxsfb.c b/drivers/video/fbdev/mxsfb.c
index 4e6608c..38898a9 100644
--- a/drivers/video/fbdev/mxsfb.c
+++ b/drivers/video/fbdev/mxsfb.c

@@ -150,7 +150,7 @@
 #define STMLCDIF_24BIT 3 /** pixel data bus to the display is of 24 bit width */
 
 #define MXSFB_SYNC_DATA_ENABLE_HIGH_ACT	(1 << 6)
-#define MXSFB_SYNC_DOTCLK_FALLING_ACT	(1 << 7) /* negtive edge sampling */
+#define MXSFB_SYNC_DOTCLK_FALLING_ACT	(1 << 7) /* negative edge sampling */
 
 enum mxsfb_devtype {
 	MXSFB_V3,

@@ -788,7 +788,16 @@ static int mxsfb_init_fbinfo_dt(struct mxsfb_info *host,
 
 	if (vm.flags & DISPLAY_FLAGS_DE_HIGH)
 		host->sync |= MXSFB_SYNC_DATA_ENABLE_HIGH_ACT;
-	if (vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE)
+
+	/*
+	 * The PIXDATA flags of the display_flags enum are controller
+	 * centric, e.g. NEGEDGE means drive data on negative edge.
+	 * However, the drivers flag is display centric: Sample the
+	 * data on negative (falling) edge. Therefore, check for the
+	 * POSEDGE flag:
+	 * drive on positive edge => sample on negative edge
+	 */
+	if (vm.flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
 		host->sync |= MXSFB_SYNC_DOTCLK_FALLING_ACT;
 
 put_display_node:

-- 
2.7.0

Hi Shawn,

On 2016-01-25 17:29, Stefan Agner wrote:

The PIXDATA flags of the display_flags enum are controller centric,
e.g. NEGEDGE means the controller shall drive the data signals on
pixelclocks negative edge. However, the drivers flag is display
centric: Sample the data on negative (falling) edge.

Therefore, change the if statement to check for the POSEDGE flag
(which is typically not set):
Drive on positive edge => sample on negative edge

Any comment on that patch?

--
Stefan

Hi all,

what is the state of this patch? Two days ago I had trouble running a
TFT panel properly on a iMX6 board because of problem that this patch
aims to solve.

I saw that an affine patch from same author was accepted into drm/fbdev
(commit 53990e416bb7adaa59d045f325a47f31a11b75ee "drm: mxsfb: fix pixel
clock polarity" on mainline branch).

Thanks in advance, regards

Mauro

Hi Shawn,

On 2016-01-25 17:29, Stefan Agner wrote:

quoted

The PIXDATA flags of the display_flags enum are controller centric,
e.g. NEGEDGE means the controller shall drive the data signals on
pixelclocks negative edge. However, the drivers flag is display
centric: Sample the data on negative (falling) edge.

Therefore, change the if statement to check for the POSEDGE flag
(which is typically not set):
Drive on positive edge => sample on negative edge

Any comment on that patch?

--
Stefan

quoted

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Hi all

Shawn, I would like to have at least your Ack on this before merge.

It seems that this has been wrong since the driver is able to use
the timings from the device tree, introduced with 669406534b4a
("video: mxsfb: get display timings from device tree").

Not sure how many device trees actually specify the wrong pixel

clock

quoted

polarity due to that. At least the initial flag convertion from the
old platform data structures done with 0d9f8217db15 ("ARM: mxs:

move

quoted

display timing configurations into device tree") seems to be

affected

quoted

and would need to be changed accordingly...

Not sure how we should handle this, maybe just invert all
pixelclk-active properties where the mxsfb driver is in use...?

--
Stefan

 drivers/video/fbdev/mxsfb.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/mxsfb.c

b/drivers/video/fbdev/mxsfb.c

quoted

index 4e6608c..38898a9 100644

--- a/drivers/video/fbdev/mxsfb.c
+++ b/drivers/video/fbdev/mxsfb.c

@@ -150,7 +150,7 @@

 #define STMLCDIF_24BIT 3 /** pixel data bus to the display is of

24

quoted

bit width */
 
 #define MXSFB_SYNC_DATA_ENABLE_HIGH_ACT	(1 << 6)
-#define MXSFB_SYNC_DOTCLK_FALLING_ACT	(1 << 7) /* negtive

edge sampling */

quoted

+#define MXSFB_SYNC_DOTCLK_FALLING_ACT	(1 << 7) /* negative

edge sampling */

quoted

 
 enum mxsfb_devtype {
 	MXSFB_V3,

@@ -788,7 +788,16 @@ static int mxsfb_init_fbinfo_dt(struct

mxsfb_info *host,

quoted

 
 	if (vm.flags & DISPLAY_FLAGS_DE_HIGH)
 		host->sync |= MXSFB_SYNC_DATA_ENABLE_HIGH_ACT;
-	if (vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE)
+
+	/*
+	 * The PIXDATA flags of the display_flags enum are

controller

quoted

+	 * centric, e.g. NEGEDGE means drive data on negative

edge.

quoted

+	 * However, the drivers flag is display centric: Sample

the

quoted

+	 * data on negative (falling) edge. Therefore, check for

the

quoted

+	 * POSEDGE flag:
+	 * drive on positive edge => sample on negative edge
+	 */
+	if (vm.flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
 		host->sync |= MXSFB_SYNC_DOTCLK_FALLING_ACT;
 
 put_display_node:

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

On 2016-01-25 17:29, Stefan Agner wrote:

The PIXDATA flags of the display_flags enum are controller centric,
e.g. NEGEDGE means the controller shall drive the data signals on
pixelclocks negative edge. However, the drivers flag is display
centric: Sample the data on negative (falling) edge.

Therefore, change the if statement to check for the POSEDGE flag
(which is typically not set):
Drive on positive edge => sample on negative edge

Any comment on that patch?

--
Stefan

b/drivers/video/fbdev/mxsfb.c

edge sampling */

+#define MXSFB_SYNC_DOTCLK_FALLING_ACT	(1 << 7) /* negative

edge sampling */

mxsfb_info *host,

 
 	if (vm.flags & DISPLAY_FLAGS_DE_HIGH)
 		host->sync |= MXSFB_SYNC_DATA_ENABLE_HIGH_ACT;
-	if (vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE)
+
+	/*
+	 * The PIXDATA flags of the display_flags enum are

controller

+	 * centric, e.g. NEGEDGE means drive data on negative edge.
+	 * However, the drivers flag is display centric: Sample the
+	 * data on negative (falling) edge. Therefore, check for the
+	 * POSEDGE flag:
+	 * drive on positive edge => sample on negative edge
+	 */
+	if (vm.flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
 		host->sync |= MXSFB_SYNC_DOTCLK_FALLING_ACT;
 
 put_display_node:

Hi,

Stefan/Shawn:

Should I merge this patch? It seems to be still needed:

https://www.spinics.net/lists/linux-fbdev/msg23112.html

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

On Friday, May 06, 2016 01:29:59 PM Stefan Agner wrote:

Hi Shawn,

On 2016-01-25 17:29, Stefan Agner wrote:

quoted

The PIXDATA flags of the display_flags enum are controller centric,
e.g. NEGEDGE means the controller shall drive the data signals on
pixelclocks negative edge. However, the drivers flag is display
centric: Sample the data on negative (falling) edge.

Therefore, change the if statement to check for the POSEDGE flag
(which is typically not set):
Drive on positive edge => sample on negative edge

Any comment on that patch?

--
Stefan

quoted

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Hi all

Shawn, I would like to have at least your Ack on this before merge.

It seems that this has been wrong since the driver is able to use
the timings from the device tree, introduced with 669406534b4a
("video: mxsfb: get display timings from device tree").

Not sure how many device trees actually specify the wrong pixel clock
polarity due to that. At least the initial flag convertion from the
old platform data structures done with 0d9f8217db15 ("ARM: mxs: move
display timing configurations into device tree") seems to be affected
and would need to be changed accordingly...

Not sure how we should handle this, maybe just invert all
pixelclk-active properties where the mxsfb driver is in use...?

--
Stefan

 drivers/video/fbdev/mxsfb.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/mxsfb.c b/drivers/video/fbdev/mxsfb.c
index 4e6608c..38898a9 100644
--- a/drivers/video/fbdev/mxsfb.c
+++ b/drivers/video/fbdev/mxsfb.c

@@ -150,7 +150,7 @@
 #define STMLCDIF_24BIT 3 /** pixel data bus to the display is of 24

bit width */
 
 #define MXSFB_SYNC_DATA_ENABLE_HIGH_ACT	(1 << 6)
-#define MXSFB_SYNC_DOTCLK_FALLING_ACT	(1 << 7) /* negtive edge sampling */
+#define MXSFB_SYNC_DOTCLK_FALLING_ACT	(1 << 7) /* negative edge sampling */
 
 enum mxsfb_devtype {
 	MXSFB_V3,

@@ -788,7 +788,16 @@ static int mxsfb_init_fbinfo_dt(struct mxsfb_info *host,
 
 	if (vm.flags & DISPLAY_FLAGS_DE_HIGH)
 		host->sync |= MXSFB_SYNC_DATA_ENABLE_HIGH_ACT;
-	if (vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE)
+
+	/*
+	 * The PIXDATA flags of the display_flags enum are controller
+	 * centric, e.g. NEGEDGE means drive data on negative edge.
+	 * However, the drivers flag is display centric: Sample the
+	 * data on negative (falling) edge. Therefore, check for the
+	 * POSEDGE flag:
+	 * drive on positive edge => sample on negative edge
+	 */
+	if (vm.flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
 		host->sync |= MXSFB_SYNC_DOTCLK_FALLING_ACT;
 
 put_display_node:

On 2017-10-17 15:56, Bartlomiej Zolnierkiewicz wrote:

Hi,

Stefan/Shawn:

Should I merge this patch? It seems to be still needed:

https://www.spinics.net/lists/linux-fbdev/msg23112.html

In my opinion yes, it fixes an issue... 

FWIW, I normally use the DRM driver when using mainline which does not
suffer from this issue, so I did not mind much that this fix never made
it...

--
Stefan

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

On Friday, May 06, 2016 01:29:59 PM Stefan Agner wrote:

quoted

Hi Shawn,

On 2016-01-25 17:29, Stefan Agner wrote:

quoted

The PIXDATA flags of the display_flags enum are controller centric,
e.g. NEGEDGE means the controller shall drive the data signals on
pixelclocks negative edge. However, the drivers flag is display
centric: Sample the data on negative (falling) edge.

Therefore, change the if statement to check for the POSEDGE flag
(which is typically not set):
Drive on positive edge => sample on negative edge

Any comment on that patch?

--
Stefan

quoted

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Hi all

Shawn, I would like to have at least your Ack on this before merge.

It seems that this has been wrong since the driver is able to use
the timings from the device tree, introduced with 669406534b4a
("video: mxsfb: get display timings from device tree").

Not sure how many device trees actually specify the wrong pixel clock
polarity due to that. At least the initial flag convertion from the
old platform data structures done with 0d9f8217db15 ("ARM: mxs: move
display timing configurations into device tree") seems to be affected
and would need to be changed accordingly...

Not sure how we should handle this, maybe just invert all
pixelclk-active properties where the mxsfb driver is in use...?

--
Stefan

 drivers/video/fbdev/mxsfb.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/mxsfb.c b/drivers/video/fbdev/mxsfb.c
index 4e6608c..38898a9 100644
--- a/drivers/video/fbdev/mxsfb.c
+++ b/drivers/video/fbdev/mxsfb.c

@@ -150,7 +150,7 @@
 #define STMLCDIF_24BIT 3 /** pixel data bus to the display is of 24

bit width */

 #define MXSFB_SYNC_DATA_ENABLE_HIGH_ACT	(1 << 6)
-#define MXSFB_SYNC_DOTCLK_FALLING_ACT	(1 << 7) /* negtive edge sampling */
+#define MXSFB_SYNC_DOTCLK_FALLING_ACT	(1 << 7) /* negative edge sampling */

 enum mxsfb_devtype {
 	MXSFB_V3,

@@ -788,7 +788,16 @@ static int mxsfb_init_fbinfo_dt(struct mxsfb_info *host,

 	if (vm.flags & DISPLAY_FLAGS_DE_HIGH)
 		host->sync |= MXSFB_SYNC_DATA_ENABLE_HIGH_ACT;
-	if (vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE)
+
+	/*
+	 * The PIXDATA flags of the display_flags enum are controller
+	 * centric, e.g. NEGEDGE means drive data on negative edge.
+	 * However, the drivers flag is display centric: Sample the
+	 * data on negative (falling) edge. Therefore, check for the
+	 * POSEDGE flag:
+	 * drive on positive edge => sample on negative edge
+	 */
+	if (vm.flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
 		host->sync |= MXSFB_SYNC_DOTCLK_FALLING_ACT;

 put_display_node:

On Tue, Oct 17, 2017 at 03:56:19PM +0200, Bartlomiej Zolnierkiewicz wrote:

Hi,

Stefan/Shawn:

Should I merge this patch? It seems to be still needed:

Acked-by: Shawn Guo <shawnguo@kernel.org>

On Tuesday, October 17, 2017 06:01:58 PM Stefan Agner wrote:

On 2017-10-17 15:56, Bartlomiej Zolnierkiewicz wrote:

quoted

Hi,

Stefan/Shawn:

Should I merge this patch? It seems to be still needed:

https://www.spinics.net/lists/linux-fbdev/msg23112.html

In my opinion yes, it fixes an issue... 

FWIW, I normally use the DRM driver when using mainline which does not
suffer from this issue, so I did not mind much that this fix never made
it...

I queued your patch (with Shawn's ACK) for 4.15, thanks!

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

--
Stefan

quoted

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

On Friday, May 06, 2016 01:29:59 PM Stefan Agner wrote:

quoted

Hi Shawn,

On 2016-01-25 17:29, Stefan Agner wrote:

quoted

The PIXDATA flags of the display_flags enum are controller centric,
e.g. NEGEDGE means the controller shall drive the data signals on
pixelclocks negative edge. However, the drivers flag is display
centric: Sample the data on negative (falling) edge.

Therefore, change the if statement to check for the POSEDGE flag
(which is typically not set):
Drive on positive edge => sample on negative edge

Any comment on that patch?

--
Stefan

quoted

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Hi all

Shawn, I would like to have at least your Ack on this before merge.

It seems that this has been wrong since the driver is able to use
the timings from the device tree, introduced with 669406534b4a
("video: mxsfb: get display timings from device tree").

Not sure how many device trees actually specify the wrong pixel clock
polarity due to that. At least the initial flag convertion from the
old platform data structures done with 0d9f8217db15 ("ARM: mxs: move
display timing configurations into device tree") seems to be affected
and would need to be changed accordingly...

Not sure how we should handle this, maybe just invert all
pixelclk-active properties where the mxsfb driver is in use...?

--
Stefan

 drivers/video/fbdev/mxsfb.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/mxsfb.c b/drivers/video/fbdev/mxsfb.c
index 4e6608c..38898a9 100644
--- a/drivers/video/fbdev/mxsfb.c
+++ b/drivers/video/fbdev/mxsfb.c

@@ -150,7 +150,7 @@
 #define STMLCDIF_24BIT 3 /** pixel data bus to the display is of 24

bit width */

 #define MXSFB_SYNC_DATA_ENABLE_HIGH_ACT	(1 << 6)
-#define MXSFB_SYNC_DOTCLK_FALLING_ACT	(1 << 7) /* negtive edge sampling */
+#define MXSFB_SYNC_DOTCLK_FALLING_ACT	(1 << 7) /* negative edge sampling */

 enum mxsfb_devtype {
 	MXSFB_V3,

@@ -788,7 +788,16 @@ static int mxsfb_init_fbinfo_dt(struct mxsfb_info *host,

 	if (vm.flags & DISPLAY_FLAGS_DE_HIGH)
 		host->sync |= MXSFB_SYNC_DATA_ENABLE_HIGH_ACT;
-	if (vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE)
+
+	/*
+	 * The PIXDATA flags of the display_flags enum are controller
+	 * centric, e.g. NEGEDGE means drive data on negative edge.
+	 * However, the drivers flag is display centric: Sample the
+	 * data on negative (falling) edge. Therefore, check for the
+	 * POSEDGE flag:
+	 * drive on positive edge => sample on negative edge
+	 */
+	if (vm.flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
 		host->sync |= MXSFB_SYNC_DOTCLK_FALLING_ACT;

 put_display_node: