[PATCH 0/2] ARM: davinci: initial infrastructure for LCDC

[PATCH 0/2] ARM: davinci: initial infrastructure for LCDC

[Bartosz Golaszewski]

After discussing the matter with Laurent Pinchart it turned out that
using ti,tilcdc,panel was wrong and we should go with the new
simple-vga-dac driver proposed by Maxime Ripard and currently being
reviewed.

The da850-lcdk board on which I'm working has a THS8135 video DAC for
which the new driver seems to be best suited and we'll be able to
query the connected display for supported modes instead of hardcoding
them in the dt as is needed for the panel driver.

In the meantime I'm posting two patches based on Karl Beldan's
previous work that can already be merged.

The first one adds OF_DEV_AUXDATA entry to da8xx-dt.c. I changed the
compatible string to the new one we're introducing in the tilcdc
driver.

The second adds the lcd pins and the display node to da850.dtsi. As
suggested by Sekhar: I moved the pins node, which was previously in
da850-lcdk.dts, to da850.dtsi. I also squashed Karl's two patches and
removed the panel node.

Tested on a da850-lcdk with an LCD display connected over VGA with
two patches already posted to the drm mailing list:

  drm: tilcdc: add a da850-specific compatible string
  drm: tilcdc: add a workaround for failed clk_set_rate()

and some additional work-in-progress/hacks on top of that.

Karl Beldan (2):
  ARM: davinci: da8xx-dt: add OF_DEV_AUXDATA entry for lcdc
  ARM: dts: da850: add a node for the LCD controller

 arch/arm/boot/dts/da850.dtsi     | 29 +++++++++++++++++++++++++++++
 arch/arm/mach-davinci/da8xx-dt.c |  1 +
 2 files changed, 30 insertions(+)

-- 
2.9.3

[PATCH 0/2] ARM: davinci: initial infrastructure for LCDC

[Bartosz Golaszewski]

After discussing the matter with Laurent Pinchart it turned out that
using ti,tilcdc,panel was wrong and we should go with the new
simple-vga-dac driver proposed by Maxime Ripard and currently being
reviewed.

The da850-lcdk board on which I'm working has a THS8135 video DAC for
which the new driver seems to be best suited and we'll be able to
query the connected display for supported modes instead of hardcoding
them in the dt as is needed for the panel driver.

In the meantime I'm posting two patches based on Karl Beldan's
previous work that can already be merged.

The first one adds OF_DEV_AUXDATA entry to da8xx-dt.c. I changed the
compatible string to the new one we're introducing in the tilcdc
driver.

The second adds the lcd pins and the display node to da850.dtsi. As
suggested by Sekhar: I moved the pins node, which was previously in
da850-lcdk.dts, to da850.dtsi. I also squashed Karl's two patches and
removed the panel node.

Tested on a da850-lcdk with an LCD display connected over VGA with
two patches already posted to the drm mailing list:

  drm: tilcdc: add a da850-specific compatible string
  drm: tilcdc: add a workaround for failed clk_set_rate()

and some additional work-in-progress/hacks on top of that.

Karl Beldan (2):
  ARM: davinci: da8xx-dt: add OF_DEV_AUXDATA entry for lcdc
  ARM: dts: da850: add a node for the LCD controller

 arch/arm/boot/dts/da850.dtsi     | 29 +++++++++++++++++++++++++++++
 arch/arm/mach-davinci/da8xx-dt.c |  1 +
 2 files changed, 30 insertions(+)

-- 
2.9.3

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

[PATCH 0/2] ARM: davinci: initial infrastructure for LCDC

[Bartosz Golaszewski]

After discussing the matter with Laurent Pinchart it turned out that
using ti,tilcdc,panel was wrong and we should go with the new
simple-vga-dac driver proposed by Maxime Ripard and currently being
reviewed.

The da850-lcdk board on which I'm working has a THS8135 video DAC for
which the new driver seems to be best suited and we'll be able to
query the connected display for supported modes instead of hardcoding
them in the dt as is needed for the panel driver.

In the meantime I'm posting two patches based on Karl Beldan's
previous work that can already be merged.

The first one adds OF_DEV_AUXDATA entry to da8xx-dt.c. I changed the
compatible string to the new one we're introducing in the tilcdc
driver.

The second adds the lcd pins and the display node to da850.dtsi. As
suggested by Sekhar: I moved the pins node, which was previously in
da850-lcdk.dts, to da850.dtsi. I also squashed Karl's two patches and
removed the panel node.

Tested on a da850-lcdk with an LCD display connected over VGA with
two patches already posted to the drm mailing list:

  drm: tilcdc: add a da850-specific compatible string
  drm: tilcdc: add a workaround for failed clk_set_rate()

and some additional work-in-progress/hacks on top of that.

Karl Beldan (2):
  ARM: davinci: da8xx-dt: add OF_DEV_AUXDATA entry for lcdc
  ARM: dts: da850: add a node for the LCD controller

 arch/arm/boot/dts/da850.dtsi     | 29 +++++++++++++++++++++++++++++
 arch/arm/mach-davinci/da8xx-dt.c |  1 +
 2 files changed, 30 insertions(+)

-- 
2.9.3

[PATCH 1/2] ARM: davinci: da8xx-dt: add OF_DEV_AUXDATA entry for lcdc

[Bartosz Golaszewski]

From: Karl Beldan <kbeldan@baylibre.com>

This is required for tilcdc to be able to acquire a functional clock
on da850 SoCs.

Signed-off-by: Karl Beldan <kbeldan@baylibre.com>
[Bartosz:
  - added the commit description
  - changed the compatible string to 'ti,da850-tilcdc']
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/da8xx-dt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
index c9f7e92..697da3d 100644
--- a/arch/arm/mach-davinci/da8xx-dt.c
+++ b/arch/arm/mach-davinci/da8xx-dt.c
@@ -38,6 +38,7 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
 		       NULL),
 	OF_DEV_AUXDATA("ti,da830-mcasp-audio", 0x01d00000, "davinci-mcasp.0", NULL),
 	OF_DEV_AUXDATA("ti,da850-aemif", 0x68000000, "ti-aemif", NULL),
+	OF_DEV_AUXDATA("ti,da850-tilcdc", 0x01e13000, "da8xx_lcdc.0", NULL),
 	{}
 };
 
-- 
2.9.3

[PATCH 1/2] ARM: davinci: da8xx-dt: add OF_DEV_AUXDATA entry for lcdc

[Bartosz Golaszewski]

From: Karl Beldan <kbeldan@baylibre.com>

This is required for tilcdc to be able to acquire a functional clock
on da850 SoCs.

Signed-off-by: Karl Beldan <kbeldan@baylibre.com>
[Bartosz:
  - added the commit description
  - changed the compatible string to 'ti,da850-tilcdc']
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/da8xx-dt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
index c9f7e92..697da3d 100644
--- a/arch/arm/mach-davinci/da8xx-dt.c
+++ b/arch/arm/mach-davinci/da8xx-dt.c
@@ -38,6 +38,7 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
 		       NULL),
 	OF_DEV_AUXDATA("ti,da830-mcasp-audio", 0x01d00000, "davinci-mcasp.0", NULL),
 	OF_DEV_AUXDATA("ti,da850-aemif", 0x68000000, "ti-aemif", NULL),
+	OF_DEV_AUXDATA("ti,da850-tilcdc", 0x01e13000, "da8xx_lcdc.0", NULL),
 	{}
 };
 
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] ARM: davinci: da8xx-dt: add OF_DEV_AUXDATA entry for lcdc

[Bartosz Golaszewski]

From: Karl Beldan <kbeldan@baylibre.com>

This is required for tilcdc to be able to acquire a functional clock
on da850 SoCs.

Signed-off-by: Karl Beldan <kbeldan@baylibre.com>
[Bartosz:
  - added the commit description
  - changed the compatible string to 'ti,da850-tilcdc']
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/da8xx-dt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
index c9f7e92..697da3d 100644
--- a/arch/arm/mach-davinci/da8xx-dt.c
+++ b/arch/arm/mach-davinci/da8xx-dt.c
@@ -38,6 +38,7 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
 		       NULL),
 	OF_DEV_AUXDATA("ti,da830-mcasp-audio", 0x01d00000, "davinci-mcasp.0", NULL),
 	OF_DEV_AUXDATA("ti,da850-aemif", 0x68000000, "ti-aemif", NULL),
+	OF_DEV_AUXDATA("ti,da850-tilcdc", 0x01e13000, "da8xx_lcdc.0", NULL),
 	{}
 };
 
-- 
2.9.3

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Bartosz Golaszewski]

From: Karl Beldan <kbeldan@baylibre.com>

Add pins used by the LCD controller and a disabled LCDC node to be
reused in device trees including da850.dtsi.

Signed-off-by: Karl Beldan <kbeldan@baylibre.com>
[Bartosz:
  - added the commit description
  - changed the dt node name to a generic one
  - added a da850-specific compatible string
  - removed the tilcdc,panel node
  - moved the pins definitions to da850.dtsi as suggested by
    Sekhar Nori (was in: da850-lcdk.dts)]
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850.dtsi | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index f79e1b9..32908ae 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -186,6 +186,27 @@
 					0xc 0x88888888 0xffffffff
 				>;
 			};
+			lcd_pins: pinmux_lcd_pins {
+				pinctrl-single,bits = <
+					/*
+					 * LCD_D[2], LCD_D[3], LCD_D[4], LCD_D[5],
+					 * LCD_D[6], LCD_D[7]
+					 */
+					0x40 0x22222200 0xffffff00
+					/*
+					 * LCD_D[10], LCD_D[11], LCD_D[12], LCD_D[13],
+					 * LCD_D[14], LCD_D[15], LCD_D[0], LCD_D[1]
+					 */
+					0x44 0x22222222 0xffffffff
+					/* LCD_D[8], LCD_D[9] */
+					0x48 0x00000022 0x000000ff
+
+					/* LCD_PCLK */
+					0x48 0x02000000 0x0f000000
+					/* LCD_AC_ENB_CS, LCD_VSYNC, LCD_HSYNC */
+					0x4c 0x02000022 0x0f0000ff
+				>;
+			};
 
 		};
 		edma0: edma@0 {
@@ -399,6 +420,14 @@
 				<&edma0 0 1>;
 			dma-names = "tx", "rx";
 		};
+
+		display: display@213000 {
+			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
+			reg = <0x213000 0x1000>;
+			interrupt-parent = <&intc>;
+			interrupts = <52>;
+			status = "disabled";
+		};
 	};
 	aemif: aemif@68000000 {
 		compatible = "ti,da850-aemif";
-- 
2.9.3

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Bartosz Golaszewski]

From: Karl Beldan <kbeldan@baylibre.com>

Add pins used by the LCD controller and a disabled LCDC node to be
reused in device trees including da850.dtsi.

Signed-off-by: Karl Beldan <kbeldan@baylibre.com>
[Bartosz:
  - added the commit description
  - changed the dt node name to a generic one
  - added a da850-specific compatible string
  - removed the tilcdc,panel node
  - moved the pins definitions to da850.dtsi as suggested by
    Sekhar Nori (was in: da850-lcdk.dts)]
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850.dtsi | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index f79e1b9..32908ae 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -186,6 +186,27 @@
 					0xc 0x88888888 0xffffffff
 				>;
 			};
+			lcd_pins: pinmux_lcd_pins {
+				pinctrl-single,bits = <
+					/*
+					 * LCD_D[2], LCD_D[3], LCD_D[4], LCD_D[5],
+					 * LCD_D[6], LCD_D[7]
+					 */
+					0x40 0x22222200 0xffffff00
+					/*
+					 * LCD_D[10], LCD_D[11], LCD_D[12], LCD_D[13],
+					 * LCD_D[14], LCD_D[15], LCD_D[0], LCD_D[1]
+					 */
+					0x44 0x22222222 0xffffffff
+					/* LCD_D[8], LCD_D[9] */
+					0x48 0x00000022 0x000000ff
+
+					/* LCD_PCLK */
+					0x48 0x02000000 0x0f000000
+					/* LCD_AC_ENB_CS, LCD_VSYNC, LCD_HSYNC */
+					0x4c 0x02000022 0x0f0000ff
+				>;
+			};
 
 		};
 		edma0: edma@0 {
@@ -399,6 +420,14 @@
 				<&edma0 0 1>;
 			dma-names = "tx", "rx";
 		};
+
+		display: display@213000 {
+			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
+			reg = <0x213000 0x1000>;
+			interrupt-parent = <&intc>;
+			interrupts = <52>;
+			status = "disabled";
+		};
 	};
 	aemif: aemif@68000000 {
 		compatible = "ti,da850-aemif";
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Bartosz Golaszewski]

From: Karl Beldan <kbeldan@baylibre.com>

Add pins used by the LCD controller and a disabled LCDC node to be
reused in device trees including da850.dtsi.

Signed-off-by: Karl Beldan <kbeldan@baylibre.com>
[Bartosz:
  - added the commit description
  - changed the dt node name to a generic one
  - added a da850-specific compatible string
  - removed the tilcdc,panel node
  - moved the pins definitions to da850.dtsi as suggested by
    Sekhar Nori (was in: da850-lcdk.dts)]
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850.dtsi | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index f79e1b9..32908ae 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -186,6 +186,27 @@
 					0xc 0x88888888 0xffffffff
 				>;
 			};
+			lcd_pins: pinmux_lcd_pins {
+				pinctrl-single,bits = <
+					/*
+					 * LCD_D[2], LCD_D[3], LCD_D[4], LCD_D[5],
+					 * LCD_D[6], LCD_D[7]
+					 */
+					0x40 0x22222200 0xffffff00
+					/*
+					 * LCD_D[10], LCD_D[11], LCD_D[12], LCD_D[13],
+					 * LCD_D[14], LCD_D[15], LCD_D[0], LCD_D[1]
+					 */
+					0x44 0x22222222 0xffffffff
+					/* LCD_D[8], LCD_D[9] */
+					0x48 0x00000022 0x000000ff
+
+					/* LCD_PCLK */
+					0x48 0x02000000 0x0f000000
+					/* LCD_AC_ENB_CS, LCD_VSYNC, LCD_HSYNC */
+					0x4c 0x02000022 0x0f0000ff
+				>;
+			};
 
 		};
 		edma0: edma at 0 {
@@ -399,6 +420,14 @@
 				<&edma0 0 1>;
 			dma-names = "tx", "rx";
 		};
+
+		display: display at 213000 {
+			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
+			reg = <0x213000 0x1000>;
+			interrupt-parent = <&intc>;
+			interrupts = <52>;
+			status = "disabled";
+		};
 	};
 	aemif: aemif at 68000000 {
 		compatible = "ti,da850-aemif";
-- 
2.9.3

Re: [PATCH 0/2] ARM: davinci: initial infrastructure for LCDC

[Karl Beldan]

On Wed, Oct 05, 2016 at 03:05:30PM +0200, Bartosz Golaszewski wrote:
> After discussing the matter with Laurent Pinchart it turned out that
> using ti,tilcdc,panel was wrong and we should go with the new
> simple-vga-dac driver proposed by Maxime Ripard and currently being
> reviewed.
> 
> The da850-lcdk board on which I'm working has a THS8135 video DAC for
> which the new driver seems to be best suited and we'll be able to
> query the connected display for supported modes instead of hardcoding
> them in the dt as is needed for the panel driver.
> 

I meant to point to these new changes but then it slipped my mind, it is
clearly the way to go.

Regards, 
Karl


> In the meantime I'm posting two patches based on Karl Beldan's
> previous work that can already be merged.
> 
> The first one adds OF_DEV_AUXDATA entry to da8xx-dt.c. I changed the
> compatible string to the new one we're introducing in the tilcdc
> driver.
> 
> The second adds the lcd pins and the display node to da850.dtsi. As
> suggested by Sekhar: I moved the pins node, which was previously in
> da850-lcdk.dts, to da850.dtsi. I also squashed Karl's two patches and
> removed the panel node.
> 
> Tested on a da850-lcdk with an LCD display connected over VGA with
> two patches already posted to the drm mailing list:
> 
>   drm: tilcdc: add a da850-specific compatible string
>   drm: tilcdc: add a workaround for failed clk_set_rate()
> 
> and some additional work-in-progress/hacks on top of that.
> 
> Karl Beldan (2):
>   ARM: davinci: da8xx-dt: add OF_DEV_AUXDATA entry for lcdc
>   ARM: dts: da850: add a node for the LCD controller
> 
>  arch/arm/boot/dts/da850.dtsi     | 29 +++++++++++++++++++++++++++++
>  arch/arm/mach-davinci/da8xx-dt.c |  1 +
>  2 files changed, 30 insertions(+)
> 
> -- 
> 2.9.3
> 

Re: [PATCH 0/2] ARM: davinci: initial infrastructure for LCDC

[Karl Beldan]

On Wed, Oct 05, 2016 at 03:05:30PM +0200, Bartosz Golaszewski wrote:
> After discussing the matter with Laurent Pinchart it turned out that
> using ti,tilcdc,panel was wrong and we should go with the new
> simple-vga-dac driver proposed by Maxime Ripard and currently being
> reviewed.
> 
> The da850-lcdk board on which I'm working has a THS8135 video DAC for
> which the new driver seems to be best suited and we'll be able to
> query the connected display for supported modes instead of hardcoding
> them in the dt as is needed for the panel driver.
> 

I meant to point to these new changes but then it slipped my mind, it is
clearly the way to go.

Regards, 
Karl


> In the meantime I'm posting two patches based on Karl Beldan's
> previous work that can already be merged.
> 
> The first one adds OF_DEV_AUXDATA entry to da8xx-dt.c. I changed the
> compatible string to the new one we're introducing in the tilcdc
> driver.
> 
> The second adds the lcd pins and the display node to da850.dtsi. As
> suggested by Sekhar: I moved the pins node, which was previously in
> da850-lcdk.dts, to da850.dtsi. I also squashed Karl's two patches and
> removed the panel node.
> 
> Tested on a da850-lcdk with an LCD display connected over VGA with
> two patches already posted to the drm mailing list:
> 
>   drm: tilcdc: add a da850-specific compatible string
>   drm: tilcdc: add a workaround for failed clk_set_rate()
> 
> and some additional work-in-progress/hacks on top of that.
> 
> Karl Beldan (2):
>   ARM: davinci: da8xx-dt: add OF_DEV_AUXDATA entry for lcdc
>   ARM: dts: da850: add a node for the LCD controller
> 
>  arch/arm/boot/dts/da850.dtsi     | 29 +++++++++++++++++++++++++++++
>  arch/arm/mach-davinci/da8xx-dt.c |  1 +
>  2 files changed, 30 insertions(+)
> 
> -- 
> 2.9.3
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/2] ARM: davinci: initial infrastructure for LCDC

[Karl Beldan]

On Wed, Oct 05, 2016 at 03:05:30PM +0200, Bartosz Golaszewski wrote:
> After discussing the matter with Laurent Pinchart it turned out that
> using ti,tilcdc,panel was wrong and we should go with the new
> simple-vga-dac driver proposed by Maxime Ripard and currently being
> reviewed.
> 
> The da850-lcdk board on which I'm working has a THS8135 video DAC for
> which the new driver seems to be best suited and we'll be able to
> query the connected display for supported modes instead of hardcoding
> them in the dt as is needed for the panel driver.
> 

I meant to point to these new changes but then it slipped my mind, it is
clearly the way to go.

Regards, 
Karl


> In the meantime I'm posting two patches based on Karl Beldan's
> previous work that can already be merged.
> 
> The first one adds OF_DEV_AUXDATA entry to da8xx-dt.c. I changed the
> compatible string to the new one we're introducing in the tilcdc
> driver.
> 
> The second adds the lcd pins and the display node to da850.dtsi. As
> suggested by Sekhar: I moved the pins node, which was previously in
> da850-lcdk.dts, to da850.dtsi. I also squashed Karl's two patches and
> removed the panel node.
> 
> Tested on a da850-lcdk with an LCD display connected over VGA with
> two patches already posted to the drm mailing list:
> 
>   drm: tilcdc: add a da850-specific compatible string
>   drm: tilcdc: add a workaround for failed clk_set_rate()
> 
> and some additional work-in-progress/hacks on top of that.
> 
> Karl Beldan (2):
>   ARM: davinci: da8xx-dt: add OF_DEV_AUXDATA entry for lcdc
>   ARM: dts: da850: add a node for the LCD controller
> 
>  arch/arm/boot/dts/da850.dtsi     | 29 +++++++++++++++++++++++++++++
>  arch/arm/mach-davinci/da8xx-dt.c |  1 +
>  2 files changed, 30 insertions(+)
> 
> -- 
> 2.9.3
> 

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Wednesday 05 October 2016 06:35 PM, Bartosz Golaszewski wrote:
> From: Karl Beldan <kbeldan@baylibre.com>
> 
> Add pins used by the LCD controller and a disabled LCDC node to be
> reused in device trees including da850.dtsi.
> 
> Signed-off-by: Karl Beldan <kbeldan@baylibre.com>
> [Bartosz:
>   - added the commit description
>   - changed the dt node name to a generic one
>   - added a da850-specific compatible string
>   - removed the tilcdc,panel node
>   - moved the pins definitions to da850.dtsi as suggested by
>     Sekhar Nori (was in: da850-lcdk.dts)]
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  arch/arm/boot/dts/da850.dtsi | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index f79e1b9..32908ae 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi

> @@ -399,6 +420,14 @@
>  				<&edma0 0 1>;
>  			dma-names = "tx", "rx";
>  		};
> +
> +		display: display@213000 {
> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";

This should instead be:

compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";

as the closest match should appear first in the list.

> +			reg = <0x213000 0x1000>;
> +			interrupt-parent = <&intc>

No need of specifying the interrupt-parent as it is assumed to be that
from the parent node (soc) if left unspecified.

I made these two fixes locally and pushed the two patches in this series
to v4.10/dt branch of my tree (for URL see MAINTAINERS). Can you take a
look and make sure I did not mess anything up?

Regards,
Sekhar

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Wednesday 05 October 2016 06:35 PM, Bartosz Golaszewski wrote:
> From: Karl Beldan <kbeldan-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> 
> Add pins used by the LCD controller and a disabled LCDC node to be
> reused in device trees including da850.dtsi.
> 
> Signed-off-by: Karl Beldan <kbeldan-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> [Bartosz:
>   - added the commit description
>   - changed the dt node name to a generic one
>   - added a da850-specific compatible string
>   - removed the tilcdc,panel node
>   - moved the pins definitions to da850.dtsi as suggested by
>     Sekhar Nori (was in: da850-lcdk.dts)]
> Signed-off-by: Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
>  arch/arm/boot/dts/da850.dtsi | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index f79e1b9..32908ae 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi

> @@ -399,6 +420,14 @@
>  				<&edma0 0 1>;
>  			dma-names = "tx", "rx";
>  		};
> +
> +		display: display@213000 {
> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";

This should instead be:

compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";

as the closest match should appear first in the list.

> +			reg = <0x213000 0x1000>;
> +			interrupt-parent = <&intc>

No need of specifying the interrupt-parent as it is assumed to be that
from the parent node (soc) if left unspecified.

I made these two fixes locally and pushed the two patches in this series
to v4.10/dt branch of my tree (for URL see MAINTAINERS). Can you take a
look and make sure I did not mess anything up?

Regards,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Wednesday 05 October 2016 06:35 PM, Bartosz Golaszewski wrote:
> From: Karl Beldan <kbeldan@baylibre.com>
> 
> Add pins used by the LCD controller and a disabled LCDC node to be
> reused in device trees including da850.dtsi.
> 
> Signed-off-by: Karl Beldan <kbeldan@baylibre.com>
> [Bartosz:
>   - added the commit description
>   - changed the dt node name to a generic one
>   - added a da850-specific compatible string
>   - removed the tilcdc,panel node
>   - moved the pins definitions to da850.dtsi as suggested by
>     Sekhar Nori (was in: da850-lcdk.dts)]
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  arch/arm/boot/dts/da850.dtsi | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index f79e1b9..32908ae 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi

> @@ -399,6 +420,14 @@
>  				<&edma0 0 1>;
>  			dma-names = "tx", "rx";
>  		};
> +
> +		display: display at 213000 {
> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";

This should instead be:

compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";

as the closest match should appear first in the list.

> +			reg = <0x213000 0x1000>;
> +			interrupt-parent = <&intc>

No need of specifying the interrupt-parent as it is assumed to be that
from the parent node (soc) if left unspecified.

I made these two fixes locally and pushed the two patches in this series
to v4.10/dt branch of my tree (for URL see MAINTAINERS). Can you take a
look and make sure I did not mess anything up?

Regards,
Sekhar

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Bartosz Golaszewski]

2016-10-15 19:42 GMT+02:00 Sekhar Nori <nsekhar@ti.com>:
> On Wednesday 05 October 2016 06:35 PM, Bartosz Golaszewski wrote:
>> From: Karl Beldan <kbeldan@baylibre.com>
>>
>> Add pins used by the LCD controller and a disabled LCDC node to be
>> reused in device trees including da850.dtsi.
>>
>> Signed-off-by: Karl Beldan <kbeldan@baylibre.com>
>> [Bartosz:
>>   - added the commit description
>>   - changed the dt node name to a generic one
>>   - added a da850-specific compatible string
>>   - removed the tilcdc,panel node
>>   - moved the pins definitions to da850.dtsi as suggested by
>>     Sekhar Nori (was in: da850-lcdk.dts)]
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>>  arch/arm/boot/dts/da850.dtsi | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>> index f79e1b9..32908ae 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
>
>> @@ -399,6 +420,14 @@
>>                               <&edma0 0 1>;
>>                       dma-names = "tx", "rx";
>>               };
>> +
>> +             display: display@213000 {
>> +                     compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>
> This should instead be:
>
> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>
> as the closest match should appear first in the list.
>
>> +                     reg = <0x213000 0x1000>;
>> +                     interrupt-parent = <&intc>
>
> No need of specifying the interrupt-parent as it is assumed to be that
> from the parent node (soc) if left unspecified.
>
> I made these two fixes locally and pushed the two patches in this series
> to v4.10/dt branch of my tree (for URL see MAINTAINERS). Can you take a
> look and make sure I did not mess anything up?
>

Looks good, thanks!

Bartosz Golaszewski

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Bartosz Golaszewski]

2016-10-15 19:42 GMT+02:00 Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>:
> On Wednesday 05 October 2016 06:35 PM, Bartosz Golaszewski wrote:
>> From: Karl Beldan <kbeldan-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>
>> Add pins used by the LCD controller and a disabled LCDC node to be
>> reused in device trees including da850.dtsi.
>>
>> Signed-off-by: Karl Beldan <kbeldan-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> [Bartosz:
>>   - added the commit description
>>   - changed the dt node name to a generic one
>>   - added a da850-specific compatible string
>>   - removed the tilcdc,panel node
>>   - moved the pins definitions to da850.dtsi as suggested by
>>     Sekhar Nori (was in: da850-lcdk.dts)]
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/da850.dtsi | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>> index f79e1b9..32908ae 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
>
>> @@ -399,6 +420,14 @@
>>                               <&edma0 0 1>;
>>                       dma-names = "tx", "rx";
>>               };
>> +
>> +             display: display@213000 {
>> +                     compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>
> This should instead be:
>
> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>
> as the closest match should appear first in the list.
>
>> +                     reg = <0x213000 0x1000>;
>> +                     interrupt-parent = <&intc>
>
> No need of specifying the interrupt-parent as it is assumed to be that
> from the parent node (soc) if left unspecified.
>
> I made these two fixes locally and pushed the two patches in this series
> to v4.10/dt branch of my tree (for URL see MAINTAINERS). Can you take a
> look and make sure I did not mess anything up?
>

Looks good, thanks!

Bartosz Golaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Bartosz Golaszewski]

2016-10-15 19:42 GMT+02:00 Sekhar Nori <nsekhar@ti.com>:
> On Wednesday 05 October 2016 06:35 PM, Bartosz Golaszewski wrote:
>> From: Karl Beldan <kbeldan@baylibre.com>
>>
>> Add pins used by the LCD controller and a disabled LCDC node to be
>> reused in device trees including da850.dtsi.
>>
>> Signed-off-by: Karl Beldan <kbeldan@baylibre.com>
>> [Bartosz:
>>   - added the commit description
>>   - changed the dt node name to a generic one
>>   - added a da850-specific compatible string
>>   - removed the tilcdc,panel node
>>   - moved the pins definitions to da850.dtsi as suggested by
>>     Sekhar Nori (was in: da850-lcdk.dts)]
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>>  arch/arm/boot/dts/da850.dtsi | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>> index f79e1b9..32908ae 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
>
>> @@ -399,6 +420,14 @@
>>                               <&edma0 0 1>;
>>                       dma-names = "tx", "rx";
>>               };
>> +
>> +             display: display at 213000 {
>> +                     compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>
> This should instead be:
>
> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>
> as the closest match should appear first in the list.
>
>> +                     reg = <0x213000 0x1000>;
>> +                     interrupt-parent = <&intc>
>
> No need of specifying the interrupt-parent as it is assumed to be that
> from the parent node (soc) if left unspecified.
>
> I made these two fixes locally and pushed the two patches in this series
> to v4.10/dt branch of my tree (for URL see MAINTAINERS). Can you take a
> look and make sure I did not mess anything up?
>

Looks good, thanks!

Bartosz Golaszewski

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 735 bytes --]

On 15/10/16 20:42, Sekhar Nori wrote:

>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>> index f79e1b9..32908ae 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
> 
>> @@ -399,6 +420,14 @@
>>  				<&edma0 0 1>;
>>  			dma-names = "tx", "rx";
>>  		};
>> +
>> +		display: display@213000 {
>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
> 
> This should instead be:
> 
> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
> 
> as the closest match should appear first in the list.

Actually I don't think that's correct. The LCDC on da850 is not
compatible with the LCDC on AM335x. I think it should be just
"ti,da850-tilcdc".

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 735 bytes --]

On 15/10/16 20:42, Sekhar Nori wrote:

>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>> index f79e1b9..32908ae 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
> 
>> @@ -399,6 +420,14 @@
>>  				<&edma0 0 1>;
>>  			dma-names = "tx", "rx";
>>  		};
>> +
>> +		display: display@213000 {
>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
> 
> This should instead be:
> 
> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
> 
> as the closest match should appear first in the list.

Actually I don't think that's correct. The LCDC on da850 is not
compatible with the LCDC on AM335x. I think it should be just
"ti,da850-tilcdc".

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Tomi Valkeinen]

On 15/10/16 20:42, Sekhar Nori wrote:

>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>> index f79e1b9..32908ae 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
> 
>> @@ -399,6 +420,14 @@
>>  				<&edma0 0 1>;
>>  			dma-names = "tx", "rx";
>>  		};
>> +
>> +		display: display at 213000 {
>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
> 
> This should instead be:
> 
> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
> 
> as the closest match should appear first in the list.

Actually I don't think that's correct. The LCDC on da850 is not
compatible with the LCDC on AM335x. I think it should be just
"ti,da850-tilcdc".

 Tomi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161017/a26c6f6e/attachment.sig>

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
> On 15/10/16 20:42, Sekhar Nori wrote:
> 
>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>> index f79e1b9..32908ae 100644
>>> --- a/arch/arm/boot/dts/da850.dtsi
>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>
>>> @@ -399,6 +420,14 @@
>>>  				<&edma0 0 1>;
>>>  			dma-names = "tx", "rx";
>>>  		};
>>> +
>>> +		display: display@213000 {
>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>
>> This should instead be:
>>
>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>
>> as the closest match should appear first in the list.
> 
> Actually I don't think that's correct. The LCDC on da850 is not
> compatible with the LCDC on AM335x. I think it should be just
> "ti,da850-tilcdc".

So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
the case, I wonder how the patch passed testing. Bartosz?

Thanks,
Sekhar

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
> On 15/10/16 20:42, Sekhar Nori wrote:
> 
>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>> index f79e1b9..32908ae 100644
>>> --- a/arch/arm/boot/dts/da850.dtsi
>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>
>>> @@ -399,6 +420,14 @@
>>>  				<&edma0 0 1>;
>>>  			dma-names = "tx", "rx";
>>>  		};
>>> +
>>> +		display: display@213000 {
>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>
>> This should instead be:
>>
>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>
>> as the closest match should appear first in the list.
> 
> Actually I don't think that's correct. The LCDC on da850 is not
> compatible with the LCDC on AM335x. I think it should be just
> "ti,da850-tilcdc".

So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
the case, I wonder how the patch passed testing. Bartosz?

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
> On 15/10/16 20:42, Sekhar Nori wrote:
> 
>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>> index f79e1b9..32908ae 100644
>>> --- a/arch/arm/boot/dts/da850.dtsi
>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>
>>> @@ -399,6 +420,14 @@
>>>  				<&edma0 0 1>;
>>>  			dma-names = "tx", "rx";
>>>  		};
>>> +
>>> +		display: display at 213000 {
>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>
>> This should instead be:
>>
>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>
>> as the closest match should appear first in the list.
> 
> Actually I don't think that's correct. The LCDC on da850 is not
> compatible with the LCDC on AM335x. I think it should be just
> "ti,da850-tilcdc".

So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
the case, I wonder how the patch passed testing. Bartosz?

Thanks,
Sekhar

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Bartosz Golaszewski]

2016-10-17 9:12 GMT+02:00 Sekhar Nori <nsekhar@ti.com>:
> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>> On 15/10/16 20:42, Sekhar Nori wrote:
>>
>>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>>> index f79e1b9..32908ae 100644
>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>
>>>> @@ -399,6 +420,14 @@
>>>>                             <&edma0 0 1>;
>>>>                     dma-names = "tx", "rx";
>>>>             };
>>>> +
>>>> +           display: display@213000 {
>>>> +                   compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>
>>> This should instead be:
>>>
>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>
>>> as the closest match should appear first in the list.
>>
>> Actually I don't think that's correct. The LCDC on da850 is not
>> compatible with the LCDC on AM335x. I think it should be just
>> "ti,da850-tilcdc".
>
> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
> the case, I wonder how the patch passed testing. Bartosz?
>

DA850 uses revision 1 of the IP while am33xx is equipped with rev 2.
The driver reads the appropriate register, detects the revision and
sets the corresponding field in struct tilcdc_drm_private in
tilcdc_load().

Thanks,
Bartosz

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Bartosz Golaszewski]

2016-10-17 9:12 GMT+02:00 Sekhar Nori <nsekhar@ti.com>:
> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>> On 15/10/16 20:42, Sekhar Nori wrote:
>>
>>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>>> index f79e1b9..32908ae 100644
>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>
>>>> @@ -399,6 +420,14 @@
>>>>                             <&edma0 0 1>;
>>>>                     dma-names = "tx", "rx";
>>>>             };
>>>> +
>>>> +           display: display@213000 {
>>>> +                   compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>
>>> This should instead be:
>>>
>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>
>>> as the closest match should appear first in the list.
>>
>> Actually I don't think that's correct. The LCDC on da850 is not
>> compatible with the LCDC on AM335x. I think it should be just
>> "ti,da850-tilcdc".
>
> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
> the case, I wonder how the patch passed testing. Bartosz?
>

DA850 uses revision 1 of the IP while am33xx is equipped with rev 2.
The driver reads the appropriate register, detects the revision and
sets the corresponding field in struct tilcdc_drm_private in
tilcdc_load().

Thanks,
Bartosz
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Bartosz Golaszewski]

2016-10-17 9:12 GMT+02:00 Sekhar Nori <nsekhar@ti.com>:
> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>> On 15/10/16 20:42, Sekhar Nori wrote:
>>
>>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>>> index f79e1b9..32908ae 100644
>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>
>>>> @@ -399,6 +420,14 @@
>>>>                             <&edma0 0 1>;
>>>>                     dma-names = "tx", "rx";
>>>>             };
>>>> +
>>>> +           display: display at 213000 {
>>>> +                   compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>
>>> This should instead be:
>>>
>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>
>>> as the closest match should appear first in the list.
>>
>> Actually I don't think that's correct. The LCDC on da850 is not
>> compatible with the LCDC on AM335x. I think it should be just
>> "ti,da850-tilcdc".
>
> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
> the case, I wonder how the patch passed testing. Bartosz?
>

DA850 uses revision 1 of the IP while am33xx is equipped with rev 2.
The driver reads the appropriate register, detects the revision and
sets the corresponding field in struct tilcdc_drm_private in
tilcdc_load().

Thanks,
Bartosz

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1436 bytes --]

On 17/10/16 10:12, Sekhar Nori wrote:
> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>> On 15/10/16 20:42, Sekhar Nori wrote:
>>
>>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>>> index f79e1b9..32908ae 100644
>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>
>>>> @@ -399,6 +420,14 @@
>>>>  				<&edma0 0 1>;
>>>>  			dma-names = "tx", "rx";
>>>>  		};
>>>> +
>>>> +		display: display@213000 {
>>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>
>>> This should instead be:
>>>
>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>
>>> as the closest match should appear first in the list.
>>
>> Actually I don't think that's correct. The LCDC on da850 is not
>> compatible with the LCDC on AM335x. I think it should be just
>> "ti,da850-tilcdc".
> 
> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
> the case, I wonder how the patch passed testing. Bartosz?

AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
similar, but different.

The driver gets the version number from LCDC's register, and acts based
on that, so afaik the compatible string doesn't really affect the
functionality (as long as it matches).

But even if it works with the current driver, I don't think
"ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1436 bytes --]

On 17/10/16 10:12, Sekhar Nori wrote:
> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>> On 15/10/16 20:42, Sekhar Nori wrote:
>>
>>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>>> index f79e1b9..32908ae 100644
>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>
>>>> @@ -399,6 +420,14 @@
>>>>  				<&edma0 0 1>;
>>>>  			dma-names = "tx", "rx";
>>>>  		};
>>>> +
>>>> +		display: display@213000 {
>>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>
>>> This should instead be:
>>>
>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>
>>> as the closest match should appear first in the list.
>>
>> Actually I don't think that's correct. The LCDC on da850 is not
>> compatible with the LCDC on AM335x. I think it should be just
>> "ti,da850-tilcdc".
> 
> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
> the case, I wonder how the patch passed testing. Bartosz?

AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
similar, but different.

The driver gets the version number from LCDC's register, and acts based
on that, so afaik the compatible string doesn't really affect the
functionality (as long as it matches).

But even if it works with the current driver, I don't think
"ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Tomi Valkeinen]

On 17/10/16 10:12, Sekhar Nori wrote:
> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>> On 15/10/16 20:42, Sekhar Nori wrote:
>>
>>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>>> index f79e1b9..32908ae 100644
>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>
>>>> @@ -399,6 +420,14 @@
>>>>  				<&edma0 0 1>;
>>>>  			dma-names = "tx", "rx";
>>>>  		};
>>>> +
>>>> +		display: display at 213000 {
>>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>
>>> This should instead be:
>>>
>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>
>>> as the closest match should appear first in the list.
>>
>> Actually I don't think that's correct. The LCDC on da850 is not
>> compatible with the LCDC on AM335x. I think it should be just
>> "ti,da850-tilcdc".
> 
> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
> the case, I wonder how the patch passed testing. Bartosz?

AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
similar, but different.

The driver gets the version number from LCDC's register, and acts based
on that, so afaik the compatible string doesn't really affect the
functionality (as long as it matches).

But even if it works with the current driver, I don't think
"ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.

 Tomi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161017/b6ff5ea1/attachment.sig>

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Laurent Pinchart]

Hello,

On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
> On 17/10/16 10:12, Sekhar Nori wrote:
>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>>> On 15/10/16 20:42, Sekhar Nori wrote:
>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>>>> b/arch/arm/boot/dts/da850.dtsi
>>>>> index f79e1b9..32908ae 100644
>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>> @@ -399,6 +420,14 @@
>>>>>  				<&edma0 0 1>;
>>>>>  			dma-names = "tx", "rx";
>>>>>  		};
>>>>> +
>>>>> +		display: display@213000 {
>>>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>> 
>>>> This should instead be:
>>>> 
>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>> 
>>>> as the closest match should appear first in the list.
>>> 
>>> Actually I don't think that's correct. The LCDC on da850 is not
>>> compatible with the LCDC on AM335x. I think it should be just
>>> "ti,da850-tilcdc".
>> 
>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
>> the case, I wonder how the patch passed testing. Bartosz?
> 
> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
> similar, but different.
> 
> The driver gets the version number from LCDC's register, and acts based
> on that, so afaik the compatible string doesn't really affect the
> functionality (as long as it matches).
> 
> But even if it works with the current driver, I don't think
> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.

If the hardware provides IP revision information, how about just "ti,lcdc" ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Laurent Pinchart]

Hello,

On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
> On 17/10/16 10:12, Sekhar Nori wrote:
>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>>> On 15/10/16 20:42, Sekhar Nori wrote:
>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>>>> b/arch/arm/boot/dts/da850.dtsi
>>>>> index f79e1b9..32908ae 100644
>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>> @@ -399,6 +420,14 @@
>>>>>  				<&edma0 0 1>;
>>>>>  			dma-names = "tx", "rx";
>>>>>  		};
>>>>> +
>>>>> +		display: display@213000 {
>>>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>> 
>>>> This should instead be:
>>>> 
>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>> 
>>>> as the closest match should appear first in the list.
>>> 
>>> Actually I don't think that's correct. The LCDC on da850 is not
>>> compatible with the LCDC on AM335x. I think it should be just
>>> "ti,da850-tilcdc".
>> 
>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
>> the case, I wonder how the patch passed testing. Bartosz?
> 
> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
> similar, but different.
> 
> The driver gets the version number from LCDC's register, and acts based
> on that, so afaik the compatible string doesn't really affect the
> functionality (as long as it matches).
> 
> But even if it works with the current driver, I don't think
> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.

If the hardware provides IP revision information, how about just "ti,lcdc" ?

-- 
Regards,

Laurent Pinchart

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

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Laurent Pinchart]

Hello,

On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
> On 17/10/16 10:12, Sekhar Nori wrote:
>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>>> On 15/10/16 20:42, Sekhar Nori wrote:
>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>>>> b/arch/arm/boot/dts/da850.dtsi
>>>>> index f79e1b9..32908ae 100644
>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>> @@ -399,6 +420,14 @@
>>>>>  				<&edma0 0 1>;
>>>>>  			dma-names = "tx", "rx";
>>>>>  		};
>>>>> +
>>>>> +		display: display at 213000 {
>>>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>> 
>>>> This should instead be:
>>>> 
>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>> 
>>>> as the closest match should appear first in the list.
>>> 
>>> Actually I don't think that's correct. The LCDC on da850 is not
>>> compatible with the LCDC on AM335x. I think it should be just
>>> "ti,da850-tilcdc".
>> 
>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
>> the case, I wonder how the patch passed testing. Bartosz?
> 
> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
> similar, but different.
> 
> The driver gets the version number from LCDC's register, and acts based
> on that, so afaik the compatible string doesn't really affect the
> functionality (as long as it matches).
> 
> But even if it works with the current driver, I don't think
> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.

If the hardware provides IP revision information, how about just "ti,lcdc" ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 2220 bytes --]

On 17/10/16 14:40, Laurent Pinchart wrote:
> Hello,
> 
> On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
>> On 17/10/16 10:12, Sekhar Nori wrote:
>>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>>>> On 15/10/16 20:42, Sekhar Nori wrote:
>>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>>>>> b/arch/arm/boot/dts/da850.dtsi
>>>>>> index f79e1b9..32908ae 100644
>>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>>> @@ -399,6 +420,14 @@
>>>>>>  				<&edma0 0 1>;
>>>>>>  			dma-names = "tx", "rx";
>>>>>>  		};
>>>>>> +
>>>>>> +		display: display@213000 {
>>>>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>>>
>>>>> This should instead be:
>>>>>
>>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>>>
>>>>> as the closest match should appear first in the list.
>>>>
>>>> Actually I don't think that's correct. The LCDC on da850 is not
>>>> compatible with the LCDC on AM335x. I think it should be just
>>>> "ti,da850-tilcdc".
>>>
>>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
>>> the case, I wonder how the patch passed testing. Bartosz?
>>
>> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
>> similar, but different.
>>
>> The driver gets the version number from LCDC's register, and acts based
>> on that, so afaik the compatible string doesn't really affect the
>> functionality (as long as it matches).
>>
>> But even if it works with the current driver, I don't think
>> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.
> 
> If the hardware provides IP revision information, how about just "ti,lcdc" ?

Maybe, and I agree that's the "correct" way, but looking at the history,
it's not just once or twice when we've suddenly found out some
difference or bug or such in an IP revision, or the integration to a
SoC, that can't be found based on the IP revision.

That's why I feel it's usually safer to have the SoC revision there in
the compatible string.

That said, we have only a few different old SoCs with LCDC (compared to,
say, OMAP DSS) so in this case perhaps just "ti,lcdc" would be fine.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 2220 bytes --]

On 17/10/16 14:40, Laurent Pinchart wrote:
> Hello,
> 
> On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
>> On 17/10/16 10:12, Sekhar Nori wrote:
>>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>>>> On 15/10/16 20:42, Sekhar Nori wrote:
>>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>>>>> b/arch/arm/boot/dts/da850.dtsi
>>>>>> index f79e1b9..32908ae 100644
>>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>>> @@ -399,6 +420,14 @@
>>>>>>  				<&edma0 0 1>;
>>>>>>  			dma-names = "tx", "rx";
>>>>>>  		};
>>>>>> +
>>>>>> +		display: display@213000 {
>>>>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>>>
>>>>> This should instead be:
>>>>>
>>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>>>
>>>>> as the closest match should appear first in the list.
>>>>
>>>> Actually I don't think that's correct. The LCDC on da850 is not
>>>> compatible with the LCDC on AM335x. I think it should be just
>>>> "ti,da850-tilcdc".
>>>
>>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
>>> the case, I wonder how the patch passed testing. Bartosz?
>>
>> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
>> similar, but different.
>>
>> The driver gets the version number from LCDC's register, and acts based
>> on that, so afaik the compatible string doesn't really affect the
>> functionality (as long as it matches).
>>
>> But even if it works with the current driver, I don't think
>> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.
> 
> If the hardware provides IP revision information, how about just "ti,lcdc" ?

Maybe, and I agree that's the "correct" way, but looking at the history,
it's not just once or twice when we've suddenly found out some
difference or bug or such in an IP revision, or the integration to a
SoC, that can't be found based on the IP revision.

That's why I feel it's usually safer to have the SoC revision there in
the compatible string.

That said, we have only a few different old SoCs with LCDC (compared to,
say, OMAP DSS) so in this case perhaps just "ti,lcdc" would be fine.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Tomi Valkeinen]

On 17/10/16 14:40, Laurent Pinchart wrote:
> Hello,
> 
> On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
>> On 17/10/16 10:12, Sekhar Nori wrote:
>>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>>>> On 15/10/16 20:42, Sekhar Nori wrote:
>>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>>>>> b/arch/arm/boot/dts/da850.dtsi
>>>>>> index f79e1b9..32908ae 100644
>>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>>> @@ -399,6 +420,14 @@
>>>>>>  				<&edma0 0 1>;
>>>>>>  			dma-names = "tx", "rx";
>>>>>>  		};
>>>>>> +
>>>>>> +		display: display at 213000 {
>>>>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>>>
>>>>> This should instead be:
>>>>>
>>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>>>
>>>>> as the closest match should appear first in the list.
>>>>
>>>> Actually I don't think that's correct. The LCDC on da850 is not
>>>> compatible with the LCDC on AM335x. I think it should be just
>>>> "ti,da850-tilcdc".
>>>
>>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
>>> the case, I wonder how the patch passed testing. Bartosz?
>>
>> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
>> similar, but different.
>>
>> The driver gets the version number from LCDC's register, and acts based
>> on that, so afaik the compatible string doesn't really affect the
>> functionality (as long as it matches).
>>
>> But even if it works with the current driver, I don't think
>> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.
> 
> If the hardware provides IP revision information, how about just "ti,lcdc" ?

Maybe, and I agree that's the "correct" way, but looking at the history,
it's not just once or twice when we've suddenly found out some
difference or bug or such in an IP revision, or the integration to a
SoC, that can't be found based on the IP revision.

That's why I feel it's usually safer to have the SoC revision there in
the compatible string.

That said, we have only a few different old SoCs with LCDC (compared to,
say, OMAP DSS) so in this case perhaps just "ti,lcdc" would be fine.

 Tomi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161017/6c32126e/attachment.sig>

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Laurent Pinchart]

Hi Tomi,

On Monday 17 Oct 2016 15:29:23 Tomi Valkeinen wrote:
> On 17/10/16 14:40, Laurent Pinchart wrote:
> > On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
> >> On 17/10/16 10:12, Sekhar Nori wrote:
> >>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
> >>>> On 15/10/16 20:42, Sekhar Nori wrote:
> >>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
> >>>>>> b/arch/arm/boot/dts/da850.dtsi
> >>>>>> index f79e1b9..32908ae 100644
> >>>>>> --- a/arch/arm/boot/dts/da850.dtsi
> >>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
> >>>>>> @@ -399,6 +420,14 @@
> >>>>>>  				<&edma0 0 1>;
> >>>>>>  			dma-names = "tx", "rx";
> >>>>>>  		};
> >>>>>> +
> >>>>>> +		display: display@213000 {
> >>>>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-
tilcdc";
> >>>>> 
> >>>>> This should instead be:
> >>>>> 
> >>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
> >>>>> 
> >>>>> as the closest match should appear first in the list.
> >>>> 
> >>>> Actually I don't think that's correct. The LCDC on da850 is not
> >>>> compatible with the LCDC on AM335x. I think it should be just
> >>>> "ti,da850-tilcdc".
> >>> 
> >>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
> >>> the case, I wonder how the patch passed testing. Bartosz?
> >> 
> >> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
> >> similar, but different.
> >> 
> >> The driver gets the version number from LCDC's register, and acts based
> >> on that, so afaik the compatible string doesn't really affect the
> >> functionality (as long as it matches).
> >> 
> >> But even if it works with the current driver, I don't think
> >> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.
> > 
> > If the hardware provides IP revision information, how about just "ti,lcdc"
> > ?
>
> Maybe, and I agree that's the "correct" way, but looking at the history,
> it's not just once or twice when we've suddenly found out some
> difference or bug or such in an IP revision, or the integration to a
> SoC, that can't be found based on the IP revision.
> 
> That's why I feel it's usually safer to have the SoC revision there in
> the compatible string.
> 
> That said, we have only a few different old SoCs with LCDC (compared to,
> say, OMAP DSS) so in this case perhaps just "ti,lcdc" would be fine.

You obviously know more than I do on this topic so I'll trust your opinion. If 
the version register isn't enough I'm fine with multiple compatible strings.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Laurent Pinchart]

Hi Tomi,

On Monday 17 Oct 2016 15:29:23 Tomi Valkeinen wrote:
> On 17/10/16 14:40, Laurent Pinchart wrote:
> > On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
> >> On 17/10/16 10:12, Sekhar Nori wrote:
> >>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
> >>>> On 15/10/16 20:42, Sekhar Nori wrote:
> >>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
> >>>>>> b/arch/arm/boot/dts/da850.dtsi
> >>>>>> index f79e1b9..32908ae 100644
> >>>>>> --- a/arch/arm/boot/dts/da850.dtsi
> >>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
> >>>>>> @@ -399,6 +420,14 @@
> >>>>>>  				<&edma0 0 1>;
> >>>>>>  			dma-names = "tx", "rx";
> >>>>>>  		};
> >>>>>> +
> >>>>>> +		display: display@213000 {
> >>>>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-
tilcdc";
> >>>>> 
> >>>>> This should instead be:
> >>>>> 
> >>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
> >>>>> 
> >>>>> as the closest match should appear first in the list.
> >>>> 
> >>>> Actually I don't think that's correct. The LCDC on da850 is not
> >>>> compatible with the LCDC on AM335x. I think it should be just
> >>>> "ti,da850-tilcdc".
> >>> 
> >>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
> >>> the case, I wonder how the patch passed testing. Bartosz?
> >> 
> >> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
> >> similar, but different.
> >> 
> >> The driver gets the version number from LCDC's register, and acts based
> >> on that, so afaik the compatible string doesn't really affect the
> >> functionality (as long as it matches).
> >> 
> >> But even if it works with the current driver, I don't think
> >> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.
> > 
> > If the hardware provides IP revision information, how about just "ti,lcdc"
> > ?
>
> Maybe, and I agree that's the "correct" way, but looking at the history,
> it's not just once or twice when we've suddenly found out some
> difference or bug or such in an IP revision, or the integration to a
> SoC, that can't be found based on the IP revision.
> 
> That's why I feel it's usually safer to have the SoC revision there in
> the compatible string.
> 
> That said, we have only a few different old SoCs with LCDC (compared to,
> say, OMAP DSS) so in this case perhaps just "ti,lcdc" would be fine.

You obviously know more than I do on this topic so I'll trust your opinion. If 
the version register isn't enough I'm fine with multiple compatible strings.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Laurent Pinchart]

Hi Tomi,

On Monday 17 Oct 2016 15:29:23 Tomi Valkeinen wrote:
> On 17/10/16 14:40, Laurent Pinchart wrote:
> > On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
> >> On 17/10/16 10:12, Sekhar Nori wrote:
> >>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
> >>>> On 15/10/16 20:42, Sekhar Nori wrote:
> >>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
> >>>>>> b/arch/arm/boot/dts/da850.dtsi
> >>>>>> index f79e1b9..32908ae 100644
> >>>>>> --- a/arch/arm/boot/dts/da850.dtsi
> >>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
> >>>>>> @@ -399,6 +420,14 @@
> >>>>>>  				<&edma0 0 1>;
> >>>>>>  			dma-names = "tx", "rx";
> >>>>>>  		};
> >>>>>> +
> >>>>>> +		display: display at 213000 {
> >>>>>> +			compatible = "ti,am33xx-tilcdc", "ti,da850-
tilcdc";
> >>>>> 
> >>>>> This should instead be:
> >>>>> 
> >>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
> >>>>> 
> >>>>> as the closest match should appear first in the list.
> >>>> 
> >>>> Actually I don't think that's correct. The LCDC on da850 is not
> >>>> compatible with the LCDC on AM335x. I think it should be just
> >>>> "ti,da850-tilcdc".
> >>> 
> >>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
> >>> the case, I wonder how the patch passed testing. Bartosz?
> >> 
> >> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
> >> similar, but different.
> >> 
> >> The driver gets the version number from LCDC's register, and acts based
> >> on that, so afaik the compatible string doesn't really affect the
> >> functionality (as long as it matches).
> >> 
> >> But even if it works with the current driver, I don't think
> >> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.
> > 
> > If the hardware provides IP revision information, how about just "ti,lcdc"
> > ?
>
> Maybe, and I agree that's the "correct" way, but looking at the history,
> it's not just once or twice when we've suddenly found out some
> difference or bug or such in an IP revision, or the integration to a
> SoC, that can't be found based on the IP revision.
> 
> That's why I feel it's usually safer to have the SoC revision there in
> the compatible string.
> 
> That said, we have only a few different old SoCs with LCDC (compared to,
> say, OMAP DSS) so in this case perhaps just "ti,lcdc" would be fine.

You obviously know more than I do on this topic so I'll trust your opinion. If 
the version register isn't enough I'm fine with multiple compatible strings.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Bartosz Golaszewski]

2016-10-17 14:29 GMT+02:00 Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On 17/10/16 14:40, Laurent Pinchart wrote:
>> Hello,
>>
>> On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
>>> On 17/10/16 10:12, Sekhar Nori wrote:
>>>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>>>>> On 15/10/16 20:42, Sekhar Nori wrote:
>>>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>>>>>> b/arch/arm/boot/dts/da850.dtsi
>>>>>>> index f79e1b9..32908ae 100644
>>>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>>>> @@ -399,6 +420,14 @@
>>>>>>>                                  <&edma0 0 1>;
>>>>>>>                          dma-names = "tx", "rx";
>>>>>>>                  };
>>>>>>> +
>>>>>>> +                display: display@213000 {
>>>>>>> +                        compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>>>>
>>>>>> This should instead be:
>>>>>>
>>>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>>>>
>>>>>> as the closest match should appear first in the list.
>>>>>
>>>>> Actually I don't think that's correct. The LCDC on da850 is not
>>>>> compatible with the LCDC on AM335x. I think it should be just
>>>>> "ti,da850-tilcdc".
>>>>
>>>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
>>>> the case, I wonder how the patch passed testing. Bartosz?
>>>
>>> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
>>> similar, but different.
>>>
>>> The driver gets the version number from LCDC's register, and acts based
>>> on that, so afaik the compatible string doesn't really affect the
>>> functionality (as long as it matches).
>>>
>>> But even if it works with the current driver, I don't think
>>> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.
>>
>> If the hardware provides IP revision information, how about just "ti,lcdc" ?
>
> Maybe, and I agree that's the "correct" way, but looking at the history,
> it's not just once or twice when we've suddenly found out some
> difference or bug or such in an IP revision, or the integration to a
> SoC, that can't be found based on the IP revision.
>
> That's why I feel it's usually safer to have the SoC revision there in
> the compatible string.
>
> That said, we have only a few different old SoCs with LCDC (compared to,
> say, OMAP DSS) so in this case perhaps just "ti,lcdc" would be fine.
>
>  Tomi
>

I Sekhar is ok with this, I'll send a follow-up patch for that.

Thanks,
Bartosz

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Bartosz Golaszewski]

2016-10-17 14:29 GMT+02:00 Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On 17/10/16 14:40, Laurent Pinchart wrote:
>> Hello,
>>
>> On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
>>> On 17/10/16 10:12, Sekhar Nori wrote:
>>>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>>>>> On 15/10/16 20:42, Sekhar Nori wrote:
>>>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>>>>>> b/arch/arm/boot/dts/da850.dtsi
>>>>>>> index f79e1b9..32908ae 100644
>>>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>>>> @@ -399,6 +420,14 @@
>>>>>>>                                  <&edma0 0 1>;
>>>>>>>                          dma-names = "tx", "rx";
>>>>>>>                  };
>>>>>>> +
>>>>>>> +                display: display@213000 {
>>>>>>> +                        compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>>>>
>>>>>> This should instead be:
>>>>>>
>>>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>>>>
>>>>>> as the closest match should appear first in the list.
>>>>>
>>>>> Actually I don't think that's correct. The LCDC on da850 is not
>>>>> compatible with the LCDC on AM335x. I think it should be just
>>>>> "ti,da850-tilcdc".
>>>>
>>>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
>>>> the case, I wonder how the patch passed testing. Bartosz?
>>>
>>> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
>>> similar, but different.
>>>
>>> The driver gets the version number from LCDC's register, and acts based
>>> on that, so afaik the compatible string doesn't really affect the
>>> functionality (as long as it matches).
>>>
>>> But even if it works with the current driver, I don't think
>>> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.
>>
>> If the hardware provides IP revision information, how about just "ti,lcdc" ?
>
> Maybe, and I agree that's the "correct" way, but looking at the history,
> it's not just once or twice when we've suddenly found out some
> difference or bug or such in an IP revision, or the integration to a
> SoC, that can't be found based on the IP revision.
>
> That's why I feel it's usually safer to have the SoC revision there in
> the compatible string.
>
> That said, we have only a few different old SoCs with LCDC (compared to,
> say, OMAP DSS) so in this case perhaps just "ti,lcdc" would be fine.
>
>  Tomi
>

I Sekhar is ok with this, I'll send a follow-up patch for that.

Thanks,
Bartosz
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Bartosz Golaszewski]

2016-10-17 14:29 GMT+02:00 Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On 17/10/16 14:40, Laurent Pinchart wrote:
>> Hello,
>>
>> On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
>>> On 17/10/16 10:12, Sekhar Nori wrote:
>>>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>>>>> On 15/10/16 20:42, Sekhar Nori wrote:
>>>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>>>>>> b/arch/arm/boot/dts/da850.dtsi
>>>>>>> index f79e1b9..32908ae 100644
>>>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>>>> @@ -399,6 +420,14 @@
>>>>>>>                                  <&edma0 0 1>;
>>>>>>>                          dma-names = "tx", "rx";
>>>>>>>                  };
>>>>>>> +
>>>>>>> +                display: display at 213000 {
>>>>>>> +                        compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>>>>
>>>>>> This should instead be:
>>>>>>
>>>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>>>>
>>>>>> as the closest match should appear first in the list.
>>>>>
>>>>> Actually I don't think that's correct. The LCDC on da850 is not
>>>>> compatible with the LCDC on AM335x. I think it should be just
>>>>> "ti,da850-tilcdc".
>>>>
>>>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
>>>> the case, I wonder how the patch passed testing. Bartosz?
>>>
>>> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
>>> similar, but different.
>>>
>>> The driver gets the version number from LCDC's register, and acts based
>>> on that, so afaik the compatible string doesn't really affect the
>>> functionality (as long as it matches).
>>>
>>> But even if it works with the current driver, I don't think
>>> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.
>>
>> If the hardware provides IP revision information, how about just "ti,lcdc" ?
>
> Maybe, and I agree that's the "correct" way, but looking at the history,
> it's not just once or twice when we've suddenly found out some
> difference or bug or such in an IP revision, or the integration to a
> SoC, that can't be found based on the IP revision.
>
> That's why I feel it's usually safer to have the SoC revision there in
> the compatible string.
>
> That said, we have only a few different old SoCs with LCDC (compared to,
> say, OMAP DSS) so in this case perhaps just "ti,lcdc" would be fine.
>
>  Tomi
>

I Sekhar is ok with this, I'll send a follow-up patch for that.

Thanks,
Bartosz

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Monday 17 October 2016 07:31 PM, Bartosz Golaszewski wrote:
> 2016-10-17 14:29 GMT+02:00 Tomi Valkeinen <tomi.valkeinen@ti.com>:
>> On 17/10/16 14:40, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
>>>> On 17/10/16 10:12, Sekhar Nori wrote:
>>>>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>>>>>> On 15/10/16 20:42, Sekhar Nori wrote:
>>>>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>>>>>>> b/arch/arm/boot/dts/da850.dtsi
>>>>>>>> index f79e1b9..32908ae 100644
>>>>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>>>>> @@ -399,6 +420,14 @@
>>>>>>>>                                  <&edma0 0 1>;
>>>>>>>>                          dma-names = "tx", "rx";
>>>>>>>>                  };
>>>>>>>> +
>>>>>>>> +                display: display@213000 {
>>>>>>>> +                        compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>>>>>
>>>>>>> This should instead be:
>>>>>>>
>>>>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>>>>>
>>>>>>> as the closest match should appear first in the list.
>>>>>>
>>>>>> Actually I don't think that's correct. The LCDC on da850 is not
>>>>>> compatible with the LCDC on AM335x. I think it should be just
>>>>>> "ti,da850-tilcdc".
>>>>>
>>>>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
>>>>> the case, I wonder how the patch passed testing. Bartosz?
>>>>
>>>> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
>>>> similar, but different.
>>>>
>>>> The driver gets the version number from LCDC's register, and acts based
>>>> on that, so afaik the compatible string doesn't really affect the
>>>> functionality (as long as it matches).
>>>>
>>>> But even if it works with the current driver, I don't think
>>>> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.
>>>
>>> If the hardware provides IP revision information, how about just "ti,lcdc" ?
>>
>> Maybe, and I agree that's the "correct" way, but looking at the history,
>> it's not just once or twice when we've suddenly found out some
>> difference or bug or such in an IP revision, or the integration to a
>> SoC, that can't be found based on the IP revision.
>>
>> That's why I feel it's usually safer to have the SoC revision there in
>> the compatible string.

I agree with Tomi here. Lets keep the soc part number in the compatible
string. More often than not, some undocumented, undiscovered issue pops
up over time. Its much safer to have the SoC revision in there.

>>
>> That said, we have only a few different old SoCs with LCDC (compared to,
>> say, OMAP DSS) so in this case perhaps just "ti,lcdc" would be fine.
>>
>>  Tomi
>>
> 
> I Sekhar is ok with this, I'll send a follow-up patch for that.

Per me, compatible property is an ordered list precisely for the reason
that things should continue to "work" with as closely matched driver as
possible. So even if someone is running a kernel which does not
recognize "ti,da850-tilcdc", it should still be able to probe the driver
based on "ti,am33xx-tilcdc" and provide as close to full functionality
as possible.

That said, I will not insist on keeping it around if Tomi is
uncomfortable. And having read the binding documentation accepted by
Jyri, it actually says the compatible property should be __one of__
"ti,am33xx-tilcdc" or "ti,da850-tilcdc".

Thanks,
Sekhar

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Monday 17 October 2016 07:31 PM, Bartosz Golaszewski wrote:
> 2016-10-17 14:29 GMT+02:00 Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>:
>> On 17/10/16 14:40, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
>>>> On 17/10/16 10:12, Sekhar Nori wrote:
>>>>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>>>>>> On 15/10/16 20:42, Sekhar Nori wrote:
>>>>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>>>>>>> b/arch/arm/boot/dts/da850.dtsi
>>>>>>>> index f79e1b9..32908ae 100644
>>>>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>>>>> @@ -399,6 +420,14 @@
>>>>>>>>                                  <&edma0 0 1>;
>>>>>>>>                          dma-names = "tx", "rx";
>>>>>>>>                  };
>>>>>>>> +
>>>>>>>> +                display: display@213000 {
>>>>>>>> +                        compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>>>>>
>>>>>>> This should instead be:
>>>>>>>
>>>>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>>>>>
>>>>>>> as the closest match should appear first in the list.
>>>>>>
>>>>>> Actually I don't think that's correct. The LCDC on da850 is not
>>>>>> compatible with the LCDC on AM335x. I think it should be just
>>>>>> "ti,da850-tilcdc".
>>>>>
>>>>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
>>>>> the case, I wonder how the patch passed testing. Bartosz?
>>>>
>>>> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
>>>> similar, but different.
>>>>
>>>> The driver gets the version number from LCDC's register, and acts based
>>>> on that, so afaik the compatible string doesn't really affect the
>>>> functionality (as long as it matches).
>>>>
>>>> But even if it works with the current driver, I don't think
>>>> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.
>>>
>>> If the hardware provides IP revision information, how about just "ti,lcdc" ?
>>
>> Maybe, and I agree that's the "correct" way, but looking at the history,
>> it's not just once or twice when we've suddenly found out some
>> difference or bug or such in an IP revision, or the integration to a
>> SoC, that can't be found based on the IP revision.
>>
>> That's why I feel it's usually safer to have the SoC revision there in
>> the compatible string.

I agree with Tomi here. Lets keep the soc part number in the compatible
string. More often than not, some undocumented, undiscovered issue pops
up over time. Its much safer to have the SoC revision in there.

>>
>> That said, we have only a few different old SoCs with LCDC (compared to,
>> say, OMAP DSS) so in this case perhaps just "ti,lcdc" would be fine.
>>
>>  Tomi
>>
> 
> I Sekhar is ok with this, I'll send a follow-up patch for that.

Per me, compatible property is an ordered list precisely for the reason
that things should continue to "work" with as closely matched driver as
possible. So even if someone is running a kernel which does not
recognize "ti,da850-tilcdc", it should still be able to probe the driver
based on "ti,am33xx-tilcdc" and provide as close to full functionality
as possible.

That said, I will not insist on keeping it around if Tomi is
uncomfortable. And having read the binding documentation accepted by
Jyri, it actually says the compatible property should be __one of__
"ti,am33xx-tilcdc" or "ti,da850-tilcdc".

Thanks,
Sekhar

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

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Monday 17 October 2016 07:31 PM, Bartosz Golaszewski wrote:
> 2016-10-17 14:29 GMT+02:00 Tomi Valkeinen <tomi.valkeinen@ti.com>:
>> On 17/10/16 14:40, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
>>>> On 17/10/16 10:12, Sekhar Nori wrote:
>>>>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>>>>>> On 15/10/16 20:42, Sekhar Nori wrote:
>>>>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>>>>>>> b/arch/arm/boot/dts/da850.dtsi
>>>>>>>> index f79e1b9..32908ae 100644
>>>>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>>>>> @@ -399,6 +420,14 @@
>>>>>>>>                                  <&edma0 0 1>;
>>>>>>>>                          dma-names = "tx", "rx";
>>>>>>>>                  };
>>>>>>>> +
>>>>>>>> +                display: display at 213000 {
>>>>>>>> +                        compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>>>>>>
>>>>>>> This should instead be:
>>>>>>>
>>>>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>>>>>
>>>>>>> as the closest match should appear first in the list.
>>>>>>
>>>>>> Actually I don't think that's correct. The LCDC on da850 is not
>>>>>> compatible with the LCDC on AM335x. I think it should be just
>>>>>> "ti,da850-tilcdc".
>>>>>
>>>>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
>>>>> the case, I wonder how the patch passed testing. Bartosz?
>>>>
>>>> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
>>>> similar, but different.
>>>>
>>>> The driver gets the version number from LCDC's register, and acts based
>>>> on that, so afaik the compatible string doesn't really affect the
>>>> functionality (as long as it matches).
>>>>
>>>> But even if it works with the current driver, I don't think
>>>> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.
>>>
>>> If the hardware provides IP revision information, how about just "ti,lcdc" ?
>>
>> Maybe, and I agree that's the "correct" way, but looking at the history,
>> it's not just once or twice when we've suddenly found out some
>> difference or bug or such in an IP revision, or the integration to a
>> SoC, that can't be found based on the IP revision.
>>
>> That's why I feel it's usually safer to have the SoC revision there in
>> the compatible string.

I agree with Tomi here. Lets keep the soc part number in the compatible
string. More often than not, some undocumented, undiscovered issue pops
up over time. Its much safer to have the SoC revision in there.

>>
>> That said, we have only a few different old SoCs with LCDC (compared to,
>> say, OMAP DSS) so in this case perhaps just "ti,lcdc" would be fine.
>>
>>  Tomi
>>
> 
> I Sekhar is ok with this, I'll send a follow-up patch for that.

Per me, compatible property is an ordered list precisely for the reason
that things should continue to "work" with as closely matched driver as
possible. So even if someone is running a kernel which does not
recognize "ti,da850-tilcdc", it should still be able to probe the driver
based on "ti,am33xx-tilcdc" and provide as close to full functionality
as possible.

That said, I will not insist on keeping it around if Tomi is
uncomfortable. And having read the binding documentation accepted by
Jyri, it actually says the compatible property should be __one of__
"ti,am33xx-tilcdc" or "ti,da850-tilcdc".

Thanks,
Sekhar

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1351 bytes --]

On 20/10/16 13:07, Sekhar Nori wrote:

> Per me, compatible property is an ordered list precisely for the reason
> that things should continue to "work" with as closely matched driver as
> possible. So even if someone is running a kernel which does not
> recognize "ti,da850-tilcdc", it should still be able to probe the driver
> based on "ti,am33xx-tilcdc" and provide as close to full functionality
> as possible.
> 
> That said, I will not insist on keeping it around if Tomi is
> uncomfortable. And having read the binding documentation accepted by
> Jyri, it actually says the compatible property should be __one of__
> "ti,am33xx-tilcdc" or "ti,da850-tilcdc".

Well, they are just not compatible as far as I know. If the LCDC on
DA850 would be identified as AM335x LCDC, and used as such, it would not
work at all. They have different registers, AM335x LCDC has registers
that do not exist on DA850.

With our driver it happens to work, because the driver looks at the IP
revision in the registers, and then decides that this IP is not AM335x
LCDC even if the dts says so. But I see that as a driver "feature",
nothing that the .dts can depend on.

Perhaps it might work the other way around, using DA850 driver on
AM335x, as DA850 LCDC is a kind of subset of AM335x LCDC. But I'm not
sure even about that.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1351 bytes --]

On 20/10/16 13:07, Sekhar Nori wrote:

> Per me, compatible property is an ordered list precisely for the reason
> that things should continue to "work" with as closely matched driver as
> possible. So even if someone is running a kernel which does not
> recognize "ti,da850-tilcdc", it should still be able to probe the driver
> based on "ti,am33xx-tilcdc" and provide as close to full functionality
> as possible.
> 
> That said, I will not insist on keeping it around if Tomi is
> uncomfortable. And having read the binding documentation accepted by
> Jyri, it actually says the compatible property should be __one of__
> "ti,am33xx-tilcdc" or "ti,da850-tilcdc".

Well, they are just not compatible as far as I know. If the LCDC on
DA850 would be identified as AM335x LCDC, and used as such, it would not
work at all. They have different registers, AM335x LCDC has registers
that do not exist on DA850.

With our driver it happens to work, because the driver looks at the IP
revision in the registers, and then decides that this IP is not AM335x
LCDC even if the dts says so. But I see that as a driver "feature",
nothing that the .dts can depend on.

Perhaps it might work the other way around, using DA850 driver on
AM335x, as DA850 LCDC is a kind of subset of AM335x LCDC. But I'm not
sure even about that.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Tomi Valkeinen]

On 20/10/16 13:07, Sekhar Nori wrote:

> Per me, compatible property is an ordered list precisely for the reason
> that things should continue to "work" with as closely matched driver as
> possible. So even if someone is running a kernel which does not
> recognize "ti,da850-tilcdc", it should still be able to probe the driver
> based on "ti,am33xx-tilcdc" and provide as close to full functionality
> as possible.
> 
> That said, I will not insist on keeping it around if Tomi is
> uncomfortable. And having read the binding documentation accepted by
> Jyri, it actually says the compatible property should be __one of__
> "ti,am33xx-tilcdc" or "ti,da850-tilcdc".

Well, they are just not compatible as far as I know. If the LCDC on
DA850 would be identified as AM335x LCDC, and used as such, it would not
work at all. They have different registers, AM335x LCDC has registers
that do not exist on DA850.

With our driver it happens to work, because the driver looks at the IP
revision in the registers, and then decides that this IP is not AM335x
LCDC even if the dts says so. But I see that as a driver "feature",
nothing that the .dts can depend on.

Perhaps it might work the other way around, using DA850 driver on
AM335x, as DA850 LCDC is a kind of subset of AM335x LCDC. But I'm not
sure even about that.

 Tomi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161020/b0a52fc5/attachment.sig>

Re: [PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Thursday 20 October 2016 03:51 PM, Tomi Valkeinen wrote:
> On 20/10/16 13:07, Sekhar Nori wrote:
> 
>> Per me, compatible property is an ordered list precisely for the reason
>> that things should continue to "work" with as closely matched driver as
>> possible. So even if someone is running a kernel which does not
>> recognize "ti,da850-tilcdc", it should still be able to probe the driver
>> based on "ti,am33xx-tilcdc" and provide as close to full functionality
>> as possible.
>>
>> That said, I will not insist on keeping it around if Tomi is
>> uncomfortable. And having read the binding documentation accepted by
>> Jyri, it actually says the compatible property should be __one of__
>> "ti,am33xx-tilcdc" or "ti,da850-tilcdc".
> 
> Well, they are just not compatible as far as I know. If the LCDC on
> DA850 would be identified as AM335x LCDC, and used as such, it would not
> work at all. They have different registers, AM335x LCDC has registers
> that do not exist on DA850.
> 
> With our driver it happens to work, because the driver looks at the IP
> revision in the registers, and then decides that this IP is not AM335x
> LCDC even if the dts says so. But I see that as a driver "feature",
> nothing that the .dts can depend on.
> 
> Perhaps it might work the other way around, using DA850 driver on
> AM335x, as DA850 LCDC is a kind of subset of AM335x LCDC. But I'm not
> sure even about that.

Alright, thanks for the detailed explanation. I dropped the "ti,am33xx-
tilcdc" from the list and here is the updated patch I am queuing for
reference.

Thanks,
Sekhar

--8<--
Author:     Karl Beldan <kbeldan@baylibre.com>
AuthorDate: Wed Oct 5 15:05:32 2016 +0200
Commit:     Sekhar Nori <nsekhar@ti.com>
CommitDate: Thu Oct 20 15:57:21 2016 +0530

    ARM: dts: da850: add a node for the LCD controller
    
    Add pins used by the LCD controller and a disabled LCDC node to be
    reused in device trees including da850.dtsi.
    
    Signed-off-by: Karl Beldan <kbeldan@baylibre.com>
    [Bartosz:
      - added the commit description
      - changed the dt node name to a generic one
      - added a da850-specific compatible string
      - removed the tilcdc,panel node
      - moved the pins definitions to da850.dtsi as suggested by
        Sekhar Nori (was in: da850-lcdk.dts)]
    Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
    [nsekhar@ti.com: fix compatible property and remove interrupt-parent]
    Signed-off-by: Sekhar Nori <nsekhar@ti.com>

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index f79e1b91c680..901d5c98d5f0 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -186,6 +186,27 @@
 					0xc 0x88888888 0xffffffff
 				>;
 			};
+			lcd_pins: pinmux_lcd_pins {
+				pinctrl-single,bits = <
+					/*
+					 * LCD_D[2], LCD_D[3], LCD_D[4], LCD_D[5],
+					 * LCD_D[6], LCD_D[7]
+					 */
+					0x40 0x22222200 0xffffff00
+					/*
+					 * LCD_D[10], LCD_D[11], LCD_D[12], LCD_D[13],
+					 * LCD_D[14], LCD_D[15], LCD_D[0], LCD_D[1]
+					 */
+					0x44 0x22222222 0xffffffff
+					/* LCD_D[8], LCD_D[9] */
+					0x48 0x00000022 0x000000ff
+
+					/* LCD_PCLK */
+					0x48 0x02000000 0x0f000000
+					/* LCD_AC_ENB_CS, LCD_VSYNC, LCD_HSYNC */
+					0x4c 0x02000022 0x0f0000ff
+				>;
+			};
 
 		};
 		edma0: edma@0 {
@@ -399,6 +420,13 @@
 				<&edma0 0 1>;
 			dma-names = "tx", "rx";
 		};
+
+		display: display@213000 {
+			compatible = "ti,da850-tilcdc";
+			reg = <0x213000 0x1000>;
+			interrupts = <52>;
+			status = "disabled";
+		};
 	};
 	aemif: aemif@68000000 {
 		compatible = "ti,da850-aemif";

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Thursday 20 October 2016 03:51 PM, Tomi Valkeinen wrote:
> On 20/10/16 13:07, Sekhar Nori wrote:
> 
>> Per me, compatible property is an ordered list precisely for the reason
>> that things should continue to "work" with as closely matched driver as
>> possible. So even if someone is running a kernel which does not
>> recognize "ti,da850-tilcdc", it should still be able to probe the driver
>> based on "ti,am33xx-tilcdc" and provide as close to full functionality
>> as possible.
>>
>> That said, I will not insist on keeping it around if Tomi is
>> uncomfortable. And having read the binding documentation accepted by
>> Jyri, it actually says the compatible property should be __one of__
>> "ti,am33xx-tilcdc" or "ti,da850-tilcdc".
> 
> Well, they are just not compatible as far as I know. If the LCDC on
> DA850 would be identified as AM335x LCDC, and used as such, it would not
> work at all. They have different registers, AM335x LCDC has registers
> that do not exist on DA850.
> 
> With our driver it happens to work, because the driver looks at the IP
> revision in the registers, and then decides that this IP is not AM335x
> LCDC even if the dts says so. But I see that as a driver "feature",
> nothing that the .dts can depend on.
> 
> Perhaps it might work the other way around, using DA850 driver on
> AM335x, as DA850 LCDC is a kind of subset of AM335x LCDC. But I'm not
> sure even about that.

Alright, thanks for the detailed explanation. I dropped the "ti,am33xx-
tilcdc" from the list and here is the updated patch I am queuing for
reference.

Thanks,
Sekhar

--8<--
Author:     Karl Beldan <kbeldan@baylibre.com>
AuthorDate: Wed Oct 5 15:05:32 2016 +0200
Commit:     Sekhar Nori <nsekhar@ti.com>
CommitDate: Thu Oct 20 15:57:21 2016 +0530

    ARM: dts: da850: add a node for the LCD controller
    
    Add pins used by the LCD controller and a disabled LCDC node to be
    reused in device trees including da850.dtsi.
    
    Signed-off-by: Karl Beldan <kbeldan@baylibre.com>
    [Bartosz:
      - added the commit description
      - changed the dt node name to a generic one
      - added a da850-specific compatible string
      - removed the tilcdc,panel node
      - moved the pins definitions to da850.dtsi as suggested by
        Sekhar Nori (was in: da850-lcdk.dts)]
    Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
    [nsekhar at ti.com: fix compatible property and remove interrupt-parent]
    Signed-off-by: Sekhar Nori <nsekhar@ti.com>

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index f79e1b91c680..901d5c98d5f0 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -186,6 +186,27 @@
 					0xc 0x88888888 0xffffffff
 				>;
 			};
+			lcd_pins: pinmux_lcd_pins {
+				pinctrl-single,bits = <
+					/*
+					 * LCD_D[2], LCD_D[3], LCD_D[4], LCD_D[5],
+					 * LCD_D[6], LCD_D[7]
+					 */
+					0x40 0x22222200 0xffffff00
+					/*
+					 * LCD_D[10], LCD_D[11], LCD_D[12], LCD_D[13],
+					 * LCD_D[14], LCD_D[15], LCD_D[0], LCD_D[1]
+					 */
+					0x44 0x22222222 0xffffffff
+					/* LCD_D[8], LCD_D[9] */
+					0x48 0x00000022 0x000000ff
+
+					/* LCD_PCLK */
+					0x48 0x02000000 0x0f000000
+					/* LCD_AC_ENB_CS, LCD_VSYNC, LCD_HSYNC */
+					0x4c 0x02000022 0x0f0000ff
+				>;
+			};
 
 		};
 		edma0: edma at 0 {
@@ -399,6 +420,13 @@
 				<&edma0 0 1>;
 			dma-names = "tx", "rx";
 		};
+
+		display: display at 213000 {
+			compatible = "ti,da850-tilcdc";
+			reg = <0x213000 0x1000>;
+			interrupts = <52>;
+			status = "disabled";
+		};
 	};
 	aemif: aemif at 68000000 {
 		compatible = "ti,da850-aemif";