[PATCH 0/7] drm/sun4i: Introduce A33 display driver

[PATCH 0/7] drm/sun4i: Introduce A33 display driver

[Maxime Ripard]

Hi everyone,

This serie introduces the support in the sun4i-drm driver for the A33.

Beside the new IPs and special cases for the A33 new IPs, there's
nothing really outstanding, and is now at feature parity with the A13.

This serie is based on my A33 CCU patches posted earlier today here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-September/453208.html

Let me know what you think,
Maxime

Maxime Ripard (7):
  drm/sun4i: support TCONs without channel 1
  drm/sun4i: support A33 tcon
  drm/sun4i: Add SAT and DRC drivers
  drm/panel: Add Sinlinx SinA33 7" panel
  ARM: sun8i: a33: Add display pipeline
  ARM: sun8i: a33: Add RGB666 pins
  ARM: sun8i: sina33: Enable display

 .../bindings/display/sunxi/sun4i-drm.txt           |  44 ++++-
 arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts     |  34 ++++
 arch/arm/boot/dts/sun8i-a33.dtsi                   | 194 +++++++++++++++++++++
 drivers/gpu/drm/panel/panel-simple.c               |  26 +++
 drivers/gpu/drm/sun4i/Makefile                     |   3 +-
 drivers/gpu/drm/sun4i/sun4i_backend.c              |   1 +
 drivers/gpu/drm/sun4i/sun4i_drv.c                  |   8 +-
 drivers/gpu/drm/sun4i/sun4i_tcon.c                 |  42 +++--
 drivers/gpu/drm/sun4i/sun4i_tcon.h                 |   2 +
 drivers/gpu/drm/sun4i/sun6i_drc.c                  | 117 +++++++++++++
 drivers/gpu/drm/sun4i/sun8i_sat.c                  | 105 +++++++++++
 11 files changed, 557 insertions(+), 19 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun6i_drc.c
 create mode 100644 drivers/gpu/drm/sun4i/sun8i_sat.c

-- 
2.9.2

[PATCH 1/7] drm/sun4i: support TCONs without channel 1

[Maxime Ripard]

Some Allwinner SoCs, such as the A33, have a variation of the TCON that
doesn't have a second channel (or it is not wired to anything).

Make sure we can handle that case.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 34 +++++++++++++++++++++-------------
 drivers/gpu/drm/sun4i/sun4i_tcon.h |  2 ++
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 9180e7e7b551..fde6af1230d2 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -59,11 +59,13 @@ void sun4i_tcon_channel_disable(struct sun4i_tcon *tcon, int channel)
 		regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
 				   SUN4I_TCON0_CTL_TCON_ENABLE, 0);
 		clk_disable_unprepare(tcon->dclk);
-	} else if (channel == 1) {
-		regmap_update_bits(tcon->regs, SUN4I_TCON1_CTL_REG,
-				   SUN4I_TCON1_CTL_TCON_ENABLE, 0);
-		clk_disable_unprepare(tcon->sclk1);
+		return;
 	}
+
+	WARN_ON(!tcon->has_channel_1);
+	regmap_update_bits(tcon->regs, SUN4I_TCON1_CTL_REG,
+			   SUN4I_TCON1_CTL_TCON_ENABLE, 0);
+	clk_disable_unprepare(tcon->sclk1);
 }
 EXPORT_SYMBOL(sun4i_tcon_channel_disable);
 
@@ -75,12 +77,14 @@ void sun4i_tcon_channel_enable(struct sun4i_tcon *tcon, int channel)
 				   SUN4I_TCON0_CTL_TCON_ENABLE,
 				   SUN4I_TCON0_CTL_TCON_ENABLE);
 		clk_prepare_enable(tcon->dclk);
-	} else if (channel == 1) {
-		regmap_update_bits(tcon->regs, SUN4I_TCON1_CTL_REG,
-				   SUN4I_TCON1_CTL_TCON_ENABLE,
-				   SUN4I_TCON1_CTL_TCON_ENABLE);
-		clk_prepare_enable(tcon->sclk1);
+		return;
 	}
+
+	WARN_ON(!tcon->has_channel_1);
+	regmap_update_bits(tcon->regs, SUN4I_TCON1_CTL_REG,
+			   SUN4I_TCON1_CTL_TCON_ENABLE,
+			   SUN4I_TCON1_CTL_TCON_ENABLE);
+	clk_prepare_enable(tcon->sclk1);
 }
 EXPORT_SYMBOL(sun4i_tcon_channel_enable);
 
@@ -198,6 +202,8 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon,
 	u8 clk_delay;
 	u32 val;
 
+	WARN_ON(!tcon->has_channel_1);
+
 	/* Adjust clock delay */
 	clk_delay = sun4i_tcon_get_clk_delay(mode, 1);
 	regmap_update_bits(tcon->regs, SUN4I_TCON1_CTL_REG,
@@ -321,10 +327,12 @@ static int sun4i_tcon_init_clocks(struct device *dev,
 		return PTR_ERR(tcon->sclk0);
 	}
 
-	tcon->sclk1 = devm_clk_get(dev, "tcon-ch1");
-	if (IS_ERR(tcon->sclk1)) {
-		dev_err(dev, "Couldn't get the TCON channel 1 clock\n");
-		return PTR_ERR(tcon->sclk1);
+	if (tcon->has_channel_1) {
+		tcon->sclk1 = devm_clk_get(dev, "tcon-ch1");
+		if (IS_ERR(tcon->sclk1)) {
+			dev_err(dev, "Couldn't get the TCON channel 1 clock\n");
+			return PTR_ERR(tcon->sclk1);
+		}
 	}
 
 	return sun4i_dclk_create(dev, tcon);
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index 100bfa093277..12bd48925f4d 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
@@ -164,6 +164,8 @@ struct sun4i_tcon {
 	bool				has_mux;
 
 	struct drm_panel		*panel;
+
+	bool				has_channel_1;
 };
 
 struct drm_bridge *sun4i_tcon_find_bridge(struct device_node *node);
-- 
2.9.2

Re: [PATCH 1/7] drm/sun4i: support TCONs without channel 1

[Chen-Yu Tsai]

On Thu, Sep 1, 2016 at 11:31 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Some Allwinner SoCs, such as the A33, have a variation of the TCON that
> doesn't have a second channel (or it is not wired to anything).
>
> Make sure we can handle that case.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Acked-by: Chen-Yu Tsai <wens@csie.org>

[PATCH 2/7] drm/sun4i: support A33 tcon

[Maxime Ripard]

The A33 has a significantly different pipeline, with components that differ
too.

Make sure we had compatible for them.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt | 7 ++++++-
 drivers/gpu/drm/sun4i/sun4i_backend.c                         | 1 +
 drivers/gpu/drm/sun4i/sun4i_drv.c                             | 8 +++++---
 drivers/gpu/drm/sun4i/sun4i_tcon.c                            | 8 +++++++-
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
index df8f4aeefe4c..d467ea93ac08 100644
--- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
+++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
@@ -26,7 +26,9 @@ TCON
 The TCON acts as a timing controller for RGB, LVDS and TV interfaces.
 
 Required properties:
- - compatible: value should be "allwinner,sun5i-a13-tcon".
+ - compatible: value must be either:
+   * allwinner,sun5i-a13-tcon
+   * allwinner,sun8i-a23-tcon
  - reg: base address and size of memory-mapped region
  - interrupts: interrupt associated to this IP
  - clocks: phandles to the clocks feeding the TCON. Three are needed:
@@ -59,6 +61,7 @@ system.
 Required properties:
   - compatible: value must be one of:
     * allwinner,sun5i-a13-display-backend
+    * allwinner,sun8i-a33-display-backend
   - reg: base address and size of the memory-mapped region.
   - clocks: phandles to the clocks feeding the frontend and backend
     * ahb: the backend interface clock
@@ -80,6 +83,7 @@ deinterlacing and color space conversion.
 Required properties:
   - compatible: value must be one of:
     * allwinner,sun5i-a13-display-frontend
+    * allwinner,sun8i-a33-display-frontend
   - reg: base address and size of the memory-mapped region.
   - interrupts: interrupt associated to this IP
   - clocks: phandles to the clocks feeding the frontend and backend
@@ -104,6 +108,7 @@ extra node.
 Required properties:
   - compatible: value must be one of:
     * allwinner,sun5i-a13-display-engine
+    * allwinner,sun8i-a33-display-engine
 
   - allwinner,pipelines: list of phandle to the display engine
     frontends available.
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index 3ab560450a82..9bfd2e45fceb 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -345,6 +345,7 @@ static int sun4i_backend_remove(struct platform_device *pdev)
 
 static const struct of_device_id sun4i_backend_of_table[] = {
 	{ .compatible = "allwinner,sun5i-a13-display-backend" },
+	{ .compatible = "allwinner,sun8i-a33-display-backend" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sun4i_backend_of_table);
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 942f62e2441c..26431c2b5670 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -199,13 +199,14 @@ static const struct component_master_ops sun4i_drv_master_ops = {
 
 static bool sun4i_drv_node_is_frontend(struct device_node *node)
 {
-	return of_device_is_compatible(node,
-				       "allwinner,sun5i-a13-display-frontend");
+	return of_device_is_compatible(node, "allwinner,sun5i-a13-display-frontend") ||
+		of_device_is_compatible(node, "allwinner,sun8i-a33-display-frontend");
 }
 
 static bool sun4i_drv_node_is_tcon(struct device_node *node)
 {
-	return of_device_is_compatible(node, "allwinner,sun5i-a13-tcon");
+	return of_device_is_compatible(node, "allwinner,sun5i-a13-tcon") ||
+		of_device_is_compatible(node, "allwinner,sun8i-a23-tcon");
 }
 
 static int compare_of(struct device *dev, void *data)
@@ -320,6 +321,7 @@ static int sun4i_drv_remove(struct platform_device *pdev)
 
 static const struct of_device_id sun4i_drv_of_table[] = {
 	{ .compatible = "allwinner,sun5i-a13-display-engine" },
+	{ .compatible = "allwinner,sun8i-a33-display-engine" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sun4i_drv_of_table);
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index fde6af1230d2..078328193168 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -488,8 +488,13 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 	tcon->drm = drm;
 	tcon->dev = dev;
 
-	if (of_device_is_compatible(dev->of_node, "allwinner,sun5i-a13-tcon"))
+	if (of_device_is_compatible(dev->of_node, "allwinner,sun5i-a13-tcon")) {
 		tcon->has_mux = true;
+		tcon->has_channel_1 = true;
+	} else {
+		tcon->has_mux = false;
+		tcon->has_channel_1 = false;
+	}
 
 	tcon->lcd_rst = devm_reset_control_get(dev, "lcd");
 	if (IS_ERR(tcon->lcd_rst)) {
@@ -585,6 +590,7 @@ static int sun4i_tcon_remove(struct platform_device *pdev)
 
 static const struct of_device_id sun4i_tcon_of_table[] = {
 	{ .compatible = "allwinner,sun5i-a13-tcon" },
+	{ .compatible = "allwinner,sun8i-a23-tcon" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sun4i_tcon_of_table);
-- 
2.9.2

Re: [PATCH 2/7] drm/sun4i: support A33 tcon

[Chen-Yu Tsai]

Hi,

On Thu, Sep 1, 2016 at 11:31 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> The A33 has a significantly different pipeline, with components that differ
> too.
>
> Make sure we had compatible for them.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt | 7 ++++++-
>  drivers/gpu/drm/sun4i/sun4i_backend.c                         | 1 +
>  drivers/gpu/drm/sun4i/sun4i_drv.c                             | 8 +++++---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c                            | 8 +++++++-
>  4 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> index df8f4aeefe4c..d467ea93ac08 100644
> --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> @@ -26,7 +26,9 @@ TCON
>  The TCON acts as a timing controller for RGB, LVDS and TV interfaces.
>
>  Required properties:
> - - compatible: value should be "allwinner,sun5i-a13-tcon".
> + - compatible: value must be either:
> +   * allwinner,sun5i-a13-tcon
> +   * allwinner,sun8i-a23-tcon

>From what I can tell from the manuals, the A23 TCON and A33 TCON are
slightly different. The A23 TCON has channel 1, and it also has DMA
input. A33 has neither. (Though the DMA chart still lists the TCON DRQ.)

I think we should have separate compatibles for them. If you think
otherwise you should mention it in the commit message.

The rest looks good though.

Regards
ChenYu

>   - reg: base address and size of memory-mapped region
>   - interrupts: interrupt associated to this IP
>   - clocks: phandles to the clocks feeding the TCON. Three are needed:
> @@ -59,6 +61,7 @@ system.
>  Required properties:
>    - compatible: value must be one of:
>      * allwinner,sun5i-a13-display-backend
> +    * allwinner,sun8i-a33-display-backend
>    - reg: base address and size of the memory-mapped region.
>    - clocks: phandles to the clocks feeding the frontend and backend
>      * ahb: the backend interface clock
> @@ -80,6 +83,7 @@ deinterlacing and color space conversion.
>  Required properties:
>    - compatible: value must be one of:
>      * allwinner,sun5i-a13-display-frontend
> +    * allwinner,sun8i-a33-display-frontend
>    - reg: base address and size of the memory-mapped region.
>    - interrupts: interrupt associated to this IP
>    - clocks: phandles to the clocks feeding the frontend and backend
> @@ -104,6 +108,7 @@ extra node.
>  Required properties:
>    - compatible: value must be one of:
>      * allwinner,sun5i-a13-display-engine
> +    * allwinner,sun8i-a33-display-engine
>
>    - allwinner,pipelines: list of phandle to the display engine
>      frontends available.
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> index 3ab560450a82..9bfd2e45fceb 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -345,6 +345,7 @@ static int sun4i_backend_remove(struct platform_device *pdev)
>
>  static const struct of_device_id sun4i_backend_of_table[] = {
>         { .compatible = "allwinner,sun5i-a13-display-backend" },
> +       { .compatible = "allwinner,sun8i-a33-display-backend" },
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, sun4i_backend_of_table);
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
> index 942f62e2441c..26431c2b5670 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> @@ -199,13 +199,14 @@ static const struct component_master_ops sun4i_drv_master_ops = {
>
>  static bool sun4i_drv_node_is_frontend(struct device_node *node)
>  {
> -       return of_device_is_compatible(node,
> -                                      "allwinner,sun5i-a13-display-frontend");
> +       return of_device_is_compatible(node, "allwinner,sun5i-a13-display-frontend") ||
> +               of_device_is_compatible(node, "allwinner,sun8i-a33-display-frontend");
>  }
>
>  static bool sun4i_drv_node_is_tcon(struct device_node *node)
>  {
> -       return of_device_is_compatible(node, "allwinner,sun5i-a13-tcon");
> +       return of_device_is_compatible(node, "allwinner,sun5i-a13-tcon") ||
> +               of_device_is_compatible(node, "allwinner,sun8i-a23-tcon");
>  }
>
>  static int compare_of(struct device *dev, void *data)
> @@ -320,6 +321,7 @@ static int sun4i_drv_remove(struct platform_device *pdev)
>
>  static const struct of_device_id sun4i_drv_of_table[] = {
>         { .compatible = "allwinner,sun5i-a13-display-engine" },
> +       { .compatible = "allwinner,sun8i-a33-display-engine" },
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, sun4i_drv_of_table);
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index fde6af1230d2..078328193168 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -488,8 +488,13 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>         tcon->drm = drm;
>         tcon->dev = dev;
>
> -       if (of_device_is_compatible(dev->of_node, "allwinner,sun5i-a13-tcon"))
> +       if (of_device_is_compatible(dev->of_node, "allwinner,sun5i-a13-tcon")) {
>                 tcon->has_mux = true;
> +               tcon->has_channel_1 = true;
> +       } else {
> +               tcon->has_mux = false;
> +               tcon->has_channel_1 = false;
> +       }
>
>         tcon->lcd_rst = devm_reset_control_get(dev, "lcd");
>         if (IS_ERR(tcon->lcd_rst)) {
> @@ -585,6 +590,7 @@ static int sun4i_tcon_remove(struct platform_device *pdev)
>
>  static const struct of_device_id sun4i_tcon_of_table[] = {
>         { .compatible = "allwinner,sun5i-a13-tcon" },
> +       { .compatible = "allwinner,sun8i-a23-tcon" },
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, sun4i_tcon_of_table);
> --
> 2.9.2
>

Re: [PATCH 2/7] drm/sun4i: support A33 tcon

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1933 bytes --]

On Fri, Sep 02, 2016 at 02:02:30PM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Thu, Sep 1, 2016 at 11:31 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > The A33 has a significantly different pipeline, with components that differ
> > too.
> >
> > Make sure we had compatible for them.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt | 7 ++++++-
> >  drivers/gpu/drm/sun4i/sun4i_backend.c                         | 1 +
> >  drivers/gpu/drm/sun4i/sun4i_drv.c                             | 8 +++++---
> >  drivers/gpu/drm/sun4i/sun4i_tcon.c                            | 8 +++++++-
> >  4 files changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> > index df8f4aeefe4c..d467ea93ac08 100644
> > --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> > +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> > @@ -26,7 +26,9 @@ TCON
> >  The TCON acts as a timing controller for RGB, LVDS and TV interfaces.
> >
> >  Required properties:
> > - - compatible: value should be "allwinner,sun5i-a13-tcon".
> > + - compatible: value must be either:
> > +   * allwinner,sun5i-a13-tcon
> > +   * allwinner,sun8i-a23-tcon
> 
> From what I can tell from the manuals, the A23 TCON and A33 TCON are
> slightly different. The A23 TCON has channel 1, and it also has DMA
> input. A33 has neither. (Though the DMA chart still lists the TCON DRQ.)
> 
> I think we should have separate compatibles for them. If you think
> otherwise you should mention it in the commit message.

Ack, i will change it in the v2.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[PATCH 3/7] drm/sun4i: Add SAT and DRC drivers

[Maxime Ripard]

The A33 pipeline also has some new components called SAT and DRC. Even
though their exact features and programming model is not known (or
documented), they need to be clocked for the pipeline to carry the video
signal all the way.

Add minimal drivers for those that just claim the needed resources for the
pipeline to operate properly.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 .../bindings/display/sunxi/sun4i-drm.txt           |  37 +++++++
 drivers/gpu/drm/sun4i/Makefile                     |   3 +-
 drivers/gpu/drm/sun4i/sun6i_drc.c                  | 117 +++++++++++++++++++++
 drivers/gpu/drm/sun4i/sun8i_sat.c                  | 105 ++++++++++++++++++
 4 files changed, 261 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun6i_drc.c
 create mode 100644 drivers/gpu/drm/sun4i/sun8i_sat.c

diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
index d467ea93ac08..87c3c8dd34cb 100644
--- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
+++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
@@ -51,6 +51,43 @@ Required properties:
   second the block connected to the TCON channel 1 (usually the TV
   encoder)
 
+SAT
+---
+
+The SAT, found in the A33, allows to do some color correction.
+
+Required properties:
+  - compatible: value must be one of:
+    * allwinner,sun8i-a33-sat
+  - reg: base address and size of the memory-mapped region.
+  - clock: phandles to bus clock feeding the SAT
+  - resets: phandles to the reset line driving the SAT
+
+- ports: A ports node with endpoint definitions as defined in
+  Documentation/devicetree/bindings/media/video-interfaces.txt. The
+  first port should be the input endpoints, the second one the outputs
+
+DRC
+---
+
+The DRC, found in the latest Allwinner SoCs (A31, A23, A33), allows to
+do some backlight control to enhance the power consumption.
+
+Required properties:
+  - compatible: value must be one of:
+    * allwinner,sun8i-a33-drc
+  - reg: base address and size of the memory-mapped region.
+  - interrupts: interrupt associated to this IP
+  - clocks: phandles to the clocks feeding the DRC
+    * ahb: the DRC interface clock
+    * mod: the DRC module clock
+    * ram: the DRC DRAM clock
+  - clock-names: the clock names mentioned above
+  - resets: phandles to the reset line driving the DRC
+
+- ports: A ports node with endpoint definitions as defined in
+  Documentation/devicetree/bindings/media/video-interfaces.txt. The
+  first port should be the input endpoints, the second one the outputs
 
 Display Engine Backend
 ----------------------
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index 58cd55149827..f1d208941a43 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -9,5 +9,6 @@ sun4i-tcon-y += sun4i_dotclock.o
 
 obj-$(CONFIG_DRM_SUN4I)		+= sun4i-drm.o sun4i-tcon.o
 obj-$(CONFIG_DRM_SUN4I)		+= sun4i_backend.o
-
+obj-$(CONFIG_DRM_SUN4I)		+= sun6i_drc.o
+obj-$(CONFIG_DRM_SUN4I)		+= sun8i_sat.o
 obj-$(CONFIG_DRM_SUN4I)		+= sun4i_tv.o
diff --git a/drivers/gpu/drm/sun4i/sun6i_drc.c b/drivers/gpu/drm/sun4i/sun6i_drc.c
new file mode 100644
index 000000000000..93ded536876b
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun6i_drc.c
@@ -0,0 +1,117 @@
+/*
+ * Copyright (C) 2016 Free Electrons
+ *
+ * Maxime Ripard <maxime.ripard@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/component.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+struct sun6i_drc {
+	struct clk		*bus_clk;
+	struct clk		*mod_clk;
+	struct reset_control	*reset;
+};
+
+static int sun6i_drc_bind(struct device *dev, struct device *master,
+			 void *data)
+{
+	struct sun6i_drc *drc;
+	int ret;
+
+	drc = devm_kzalloc(dev, sizeof(*drc), GFP_KERNEL);
+	if (!drc)
+		return -ENOMEM;
+	dev_set_drvdata(dev, drc);
+
+	drc->reset = devm_reset_control_get(dev, NULL);
+	if (IS_ERR(drc->reset)) {
+		dev_err(dev, "Couldn't get our reset line\n");
+		return PTR_ERR(drc->reset);
+	}
+
+	ret = reset_control_deassert(drc->reset);
+	if (ret) {
+		dev_err(dev, "Couldn't deassert our reset line\n");
+		return ret;
+	}
+
+	drc->bus_clk = devm_clk_get(dev, "ahb");
+	if (IS_ERR(drc->bus_clk)) {
+		dev_err(dev, "Couldn't get our bus clock\n");
+		ret = PTR_ERR(drc->bus_clk);
+		goto err_assert_reset;
+	}
+	clk_prepare_enable(drc->bus_clk);
+
+	drc->mod_clk = devm_clk_get(dev, "mod");
+	if (IS_ERR(drc->mod_clk)) {
+		dev_err(dev, "Couldn't get our mod clock\n");
+		ret = PTR_ERR(drc->mod_clk);
+		goto err_disable_bus_clk;
+	}
+
+	return clk_prepare_enable(drc->mod_clk);
+
+err_disable_bus_clk:
+	clk_disable_unprepare(drc->bus_clk);
+err_assert_reset:
+	reset_control_assert(drc->reset);
+	return ret;
+}
+
+static void sun6i_drc_unbind(struct device *dev, struct device *master,
+			    void *data)
+{
+	struct sun6i_drc *drc = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(drc->mod_clk);
+	clk_disable_unprepare(drc->bus_clk);
+	reset_control_assert(drc->reset);
+}
+
+static struct component_ops sun6i_drc_ops = {
+	.bind	= sun6i_drc_bind,
+	.unbind	= sun6i_drc_unbind,
+};
+
+static int sun6i_drc_probe(struct platform_device *pdev)
+{
+	return component_add(&pdev->dev, &sun6i_drc_ops);
+}
+
+static int sun6i_drc_remove(struct platform_device *pdev)
+{
+	component_del(&pdev->dev, &sun6i_drc_ops);
+
+	return 0;
+}
+
+static const struct of_device_id sun6i_drc_of_table[] = {
+	{ .compatible = "allwinner,sun8i-a33-drc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sun6i_drc_of_table);
+
+static struct platform_driver sun6i_drc_platform_driver = {
+	.probe		= sun6i_drc_probe,
+	.remove		= sun6i_drc_remove,
+	.driver		= {
+		.name		= "sun6i-drc",
+		.of_match_table	= sun6i_drc_of_table,
+	},
+};
+module_platform_driver(sun6i_drc_platform_driver);
+
+MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
+MODULE_DESCRIPTION("Allwinner A31 Dynamic Range Control (DRC) Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/sun4i/sun8i_sat.c b/drivers/gpu/drm/sun4i/sun8i_sat.c
new file mode 100644
index 000000000000..fdf07ecbcd49
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun8i_sat.c
@@ -0,0 +1,105 @@
+/*
+ * Copyright (C) 2016 Free Electrons
+ *
+ * Maxime Ripard <maxime.ripard@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/component.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+struct sun8i_sat {
+	struct clk		*clk;
+	struct reset_control	*reset;
+};
+
+static int sun8i_sat_bind(struct device *dev, struct device *master,
+			 void *data)
+{
+	struct sun8i_sat *sat;
+	int ret;
+
+	sat = devm_kzalloc(dev, sizeof(*sat), GFP_KERNEL);
+	if (!sat)
+		return -ENOMEM;
+	dev_set_drvdata(dev, sat);
+
+	sat->reset = devm_reset_control_get(dev, NULL);
+	if (IS_ERR(sat->reset)) {
+		dev_err(dev, "Couldn't get our reset line\n");
+		return PTR_ERR(sat->reset);
+	}
+
+	ret = reset_control_deassert(sat->reset);
+	if (ret) {
+		dev_err(dev, "Couldn't deassert our reset line\n");
+		return ret;
+	}
+
+	sat->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(sat->clk)) {
+		dev_err(dev, "Couldn't get our clock clock\n");
+		ret = PTR_ERR(sat->clk);
+		goto err_assert_reset;
+	}
+
+	return clk_prepare_enable(sat->clk);
+
+err_assert_reset:
+	reset_control_assert(sat->reset);
+	return ret;
+}
+
+static void sun8i_sat_unbind(struct device *dev, struct device *master,
+			    void *data)
+{
+	struct sun8i_sat *sat = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(sat->clk);
+	reset_control_assert(sat->reset);
+}
+
+static struct component_ops sun8i_sat_ops = {
+	.bind	= sun8i_sat_bind,
+	.unbind	= sun8i_sat_unbind,
+};
+
+static int sun8i_sat_probe(struct platform_device *pdev)
+{
+	return component_add(&pdev->dev, &sun8i_sat_ops);
+}
+
+static int sun8i_sat_remove(struct platform_device *pdev)
+{
+	component_del(&pdev->dev, &sun8i_sat_ops);
+
+	return 0;
+}
+
+static const struct of_device_id sun8i_sat_of_table[] = {
+	{ .compatible = "allwinner,sun8i-a33-sat" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sun8i_sat_of_table);
+
+static struct platform_driver sun8i_sat_platform_driver = {
+	.probe		= sun8i_sat_probe,
+	.remove		= sun8i_sat_remove,
+	.driver		= {
+		.name		= "sun8i-sat",
+		.of_match_table	= sun8i_sat_of_table,
+	},
+};
+module_platform_driver(sun8i_sat_platform_driver);
+
+MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
+MODULE_DESCRIPTION("Allwinner A33 Saturation Enhancement (SAT) Driver");
+MODULE_LICENSE("GPL");
-- 
2.9.2

Re: [PATCH 3/7] drm/sun4i: Add SAT and DRC drivers

[Chen-Yu Tsai]

Hi,

On Thu, Sep 1, 2016 at 11:32 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> The A33 pipeline also has some new components called SAT and DRC. Even
> though their exact features and programming model is not known (or
> documented), they need to be clocked for the pipeline to carry the video
> signal all the way.
>
> Add minimal drivers for those that just claim the needed resources for the
> pipeline to operate properly.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  .../bindings/display/sunxi/sun4i-drm.txt           |  37 +++++++
>  drivers/gpu/drm/sun4i/Makefile                     |   3 +-
>  drivers/gpu/drm/sun4i/sun6i_drc.c                  | 117 +++++++++++++++++++++
>  drivers/gpu/drm/sun4i/sun8i_sat.c                  | 105 ++++++++++++++++++
>  4 files changed, 261 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/sun4i/sun6i_drc.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun8i_sat.c
>
> diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> index d467ea93ac08..87c3c8dd34cb 100644
> --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> @@ -51,6 +51,43 @@ Required properties:
>    second the block connected to the TCON channel 1 (usually the TV
>    encoder)
>
> +SAT
> +---
> +
> +The SAT, found in the A33, allows to do some color correction.
> +
> +Required properties:
> +  - compatible: value must be one of:
> +    * allwinner,sun8i-a33-sat
> +  - reg: base address and size of the memory-mapped region.
> +  - clock: phandles to bus clock feeding the SAT
> +  - resets: phandles to the reset line driving the SAT
> +
> +- ports: A ports node with endpoint definitions as defined in
> +  Documentation/devicetree/bindings/media/video-interfaces.txt. The
> +  first port should be the input endpoints, the second one the outputs

See patch 5 for my concerns about the representation of the SAT block.

> +
> +DRC
> +---
> +
> +The DRC, found in the latest Allwinner SoCs (A31, A23, A33), allows to
> +do some backlight control to enhance the power consumption.
> +
> +Required properties:
> +  - compatible: value must be one of:
> +    * allwinner,sun8i-a33-drc

Since this was first introduced in the A31, maybe using that
for the compatible is better?

Or do you want one for each SoC, given these are unknown black
boxes?

> +  - reg: base address and size of the memory-mapped region.
> +  - interrupts: interrupt associated to this IP
> +  - clocks: phandles to the clocks feeding the DRC
> +    * ahb: the DRC interface clock
> +    * mod: the DRC module clock
> +    * ram: the DRC DRAM clock
> +  - clock-names: the clock names mentioned above
> +  - resets: phandles to the reset line driving the DRC
> +
> +- ports: A ports node with endpoint definitions as defined in
> +  Documentation/devicetree/bindings/media/video-interfaces.txt. The
> +  first port should be the input endpoints, the second one the outputs
>
>  Display Engine Backend
>  ----------------------
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index 58cd55149827..f1d208941a43 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -9,5 +9,6 @@ sun4i-tcon-y += sun4i_dotclock.o
>
>  obj-$(CONFIG_DRM_SUN4I)                += sun4i-drm.o sun4i-tcon.o
>  obj-$(CONFIG_DRM_SUN4I)                += sun4i_backend.o
> -
> +obj-$(CONFIG_DRM_SUN4I)                += sun6i_drc.o
> +obj-$(CONFIG_DRM_SUN4I)                += sun8i_sat.o
>  obj-$(CONFIG_DRM_SUN4I)                += sun4i_tv.o
> diff --git a/drivers/gpu/drm/sun4i/sun6i_drc.c b/drivers/gpu/drm/sun4i/sun6i_drc.c
> new file mode 100644
> index 000000000000..93ded536876b
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun6i_drc.c
> @@ -0,0 +1,117 @@
> +/*
> + * Copyright (C) 2016 Free Electrons
> + *
> + * Maxime Ripard <maxime.ripard@free-electrons.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/component.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +
> +struct sun6i_drc {
> +       struct clk              *bus_clk;
> +       struct clk              *mod_clk;
> +       struct reset_control    *reset;
> +};
> +
> +static int sun6i_drc_bind(struct device *dev, struct device *master,
> +                        void *data)
> +{
> +       struct sun6i_drc *drc;
> +       int ret;
> +
> +       drc = devm_kzalloc(dev, sizeof(*drc), GFP_KERNEL);
> +       if (!drc)
> +               return -ENOMEM;
> +       dev_set_drvdata(dev, drc);
> +
> +       drc->reset = devm_reset_control_get(dev, NULL);
> +       if (IS_ERR(drc->reset)) {
> +               dev_err(dev, "Couldn't get our reset line\n");
> +               return PTR_ERR(drc->reset);
> +       }
> +
> +       ret = reset_control_deassert(drc->reset);
> +       if (ret) {
> +               dev_err(dev, "Couldn't deassert our reset line\n");
> +               return ret;
> +       }
> +
> +       drc->bus_clk = devm_clk_get(dev, "ahb");
> +       if (IS_ERR(drc->bus_clk)) {
> +               dev_err(dev, "Couldn't get our bus clock\n");
> +               ret = PTR_ERR(drc->bus_clk);
> +               goto err_assert_reset;
> +       }
> +       clk_prepare_enable(drc->bus_clk);
> +
> +       drc->mod_clk = devm_clk_get(dev, "mod");
> +       if (IS_ERR(drc->mod_clk)) {
> +               dev_err(dev, "Couldn't get our mod clock\n");
> +               ret = PTR_ERR(drc->mod_clk);
> +               goto err_disable_bus_clk;
> +       }
> +
> +       return clk_prepare_enable(drc->mod_clk);

What happens if this fails? No cleanup happens.

> +
> +err_disable_bus_clk:
> +       clk_disable_unprepare(drc->bus_clk);
> +err_assert_reset:
> +       reset_control_assert(drc->reset);
> +       return ret;
> +}
> +
> +static void sun6i_drc_unbind(struct device *dev, struct device *master,
> +                           void *data)
> +{
> +       struct sun6i_drc *drc = dev_get_drvdata(dev);
> +
> +       clk_disable_unprepare(drc->mod_clk);
> +       clk_disable_unprepare(drc->bus_clk);
> +       reset_control_assert(drc->reset);
> +}
> +
> +static struct component_ops sun6i_drc_ops = {
> +       .bind   = sun6i_drc_bind,
> +       .unbind = sun6i_drc_unbind,
> +};
> +
> +static int sun6i_drc_probe(struct platform_device *pdev)
> +{
> +       return component_add(&pdev->dev, &sun6i_drc_ops);
> +}
> +
> +static int sun6i_drc_remove(struct platform_device *pdev)
> +{
> +       component_del(&pdev->dev, &sun6i_drc_ops);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id sun6i_drc_of_table[] = {
> +       { .compatible = "allwinner,sun8i-a33-drc" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, sun6i_drc_of_table);
> +
> +static struct platform_driver sun6i_drc_platform_driver = {
> +       .probe          = sun6i_drc_probe,
> +       .remove         = sun6i_drc_remove,
> +       .driver         = {
> +               .name           = "sun6i-drc",
> +               .of_match_table = sun6i_drc_of_table,
> +       },
> +};
> +module_platform_driver(sun6i_drc_platform_driver);
> +
> +MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
> +MODULE_DESCRIPTION("Allwinner A31 Dynamic Range Control (DRC) Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/sun4i/sun8i_sat.c b/drivers/gpu/drm/sun4i/sun8i_sat.c
> new file mode 100644
> index 000000000000..fdf07ecbcd49
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun8i_sat.c
> @@ -0,0 +1,105 @@
> +/*
> + * Copyright (C) 2016 Free Electrons
> + *
> + * Maxime Ripard <maxime.ripard@free-electrons.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/component.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +
> +struct sun8i_sat {
> +       struct clk              *clk;
> +       struct reset_control    *reset;
> +};
> +
> +static int sun8i_sat_bind(struct device *dev, struct device *master,
> +                        void *data)
> +{
> +       struct sun8i_sat *sat;
> +       int ret;
> +
> +       sat = devm_kzalloc(dev, sizeof(*sat), GFP_KERNEL);
> +       if (!sat)
> +               return -ENOMEM;
> +       dev_set_drvdata(dev, sat);
> +
> +       sat->reset = devm_reset_control_get(dev, NULL);
> +       if (IS_ERR(sat->reset)) {
> +               dev_err(dev, "Couldn't get our reset line\n");
> +               return PTR_ERR(sat->reset);
> +       }
> +
> +       ret = reset_control_deassert(sat->reset);
> +       if (ret) {
> +               dev_err(dev, "Couldn't deassert our reset line\n");
> +               return ret;
> +       }
> +
> +       sat->clk = devm_clk_get(dev, NULL);
> +       if (IS_ERR(sat->clk)) {
> +               dev_err(dev, "Couldn't get our clock clock\n");
> +               ret = PTR_ERR(sat->clk);
> +               goto err_assert_reset;
> +       }
> +
> +       return clk_prepare_enable(sat->clk);

Same here.

Regards
ChenYu

> +
> +err_assert_reset:
> +       reset_control_assert(sat->reset);
> +       return ret;
> +}
> +
> +static void sun8i_sat_unbind(struct device *dev, struct device *master,
> +                           void *data)
> +{
> +       struct sun8i_sat *sat = dev_get_drvdata(dev);
> +
> +       clk_disable_unprepare(sat->clk);
> +       reset_control_assert(sat->reset);
> +}
> +
> +static struct component_ops sun8i_sat_ops = {
> +       .bind   = sun8i_sat_bind,
> +       .unbind = sun8i_sat_unbind,
> +};
> +
> +static int sun8i_sat_probe(struct platform_device *pdev)
> +{
> +       return component_add(&pdev->dev, &sun8i_sat_ops);
> +}
> +
> +static int sun8i_sat_remove(struct platform_device *pdev)
> +{
> +       component_del(&pdev->dev, &sun8i_sat_ops);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id sun8i_sat_of_table[] = {
> +       { .compatible = "allwinner,sun8i-a33-sat" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, sun8i_sat_of_table);
> +
> +static struct platform_driver sun8i_sat_platform_driver = {
> +       .probe          = sun8i_sat_probe,
> +       .remove         = sun8i_sat_remove,
> +       .driver         = {
> +               .name           = "sun8i-sat",
> +               .of_match_table = sun8i_sat_of_table,
> +       },
> +};
> +module_platform_driver(sun8i_sat_platform_driver);
> +
> +MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
> +MODULE_DESCRIPTION("Allwinner A33 Saturation Enhancement (SAT) Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.9.2
>

Re: [PATCH 3/7] drm/sun4i: Add SAT and DRC drivers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]

Hi,

On Fri, Sep 02, 2016 at 02:45:06PM +0800, Chen-Yu Tsai wrote:
> > +
> > +DRC
> > +---
> > +
> > +The DRC, found in the latest Allwinner SoCs (A31, A23, A33), allows to
> > +do some backlight control to enhance the power consumption.
> > +
> > +Required properties:
> > +  - compatible: value must be one of:
> > +    * allwinner,sun8i-a33-drc
> 
> Since this was first introduced in the A31, maybe using that
> for the compatible is better?
> 
> Or do you want one for each SoC, given these are unknown black
> boxes?

Yeah, I'd prefer to be on the safe side here :/

> > +       drc->mod_clk = devm_clk_get(dev, "mod");
> > +       if (IS_ERR(drc->mod_clk)) {
> > +               dev_err(dev, "Couldn't get our mod clock\n");
> > +               ret = PTR_ERR(drc->mod_clk);
> > +               goto err_disable_bus_clk;
> > +       }
> > +
> > +       return clk_prepare_enable(drc->mod_clk);
> 
> What happens if this fails? No cleanup happens.

Indeed, will fix.

Thanks!
Maxime


-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [linux-sunxi] [PATCH 3/7] drm/sun4i: Add SAT and DRC drivers

[Peter Korsgaard]

>>>>> "Maxime" == Maxime Ripard <maxime.ripard@free-electrons.com> writes:

Hi,

 > The A33 pipeline also has some new components called SAT and DRC. Even
 > though their exact features and programming model is not known (or
 > documented), they need to be clocked for the pipeline to carry the video
 > signal all the way.

 > Add minimal drivers for those that just claim the needed resources for the
 > pipeline to operate properly.

 > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
 > ---
 >  .../bindings/display/sunxi/sun4i-drm.txt           |  37 +++++++
 >  drivers/gpu/drm/sun4i/Makefile                     |   3 +-
 >  drivers/gpu/drm/sun4i/sun6i_drc.c                  | 117 +++++++++++++++++++++
 >  drivers/gpu/drm/sun4i/sun8i_sat.c                  | 105 ++++++++++++++++++
 >  4 files changed, 261 insertions(+), 1 deletion(-)
 >  create mode 100644 drivers/gpu/drm/sun4i/sun6i_drc.c
 >  create mode 100644 drivers/gpu/drm/sun4i/sun8i_sat.c

 > diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
 > index d467ea93ac08..87c3c8dd34cb 100644
 > --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
 > +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
 > @@ -51,6 +51,43 @@ Required properties:
 >    second the block connected to the TCON channel 1 (usually the TV
 >    encoder)
 
 > +SAT
 > +---
 > +
 > +The SAT, found in the A33, allows to do some color correction.
 > +
 > +Required properties:
 > +  - compatible: value must be one of:
 > +    * allwinner,sun8i-a33-sat
 > +  - reg: base address and size of the memory-mapped region.
 > +  - clock: phandles to bus clock feeding the SAT
 > +  - resets: phandles to the reset line driving the SAT
 > +
 > +- ports: A ports node with endpoint definitions as defined in
 > +  Documentation/devicetree/bindings/media/video-interfaces.txt. The
 > +  first port should be the input endpoints, the second one the outputs
 > +
 > +DRC
 > +---
 > +
 > +The DRC, found in the latest Allwinner SoCs (A31, A23, A33), allows to
 > +do some backlight control to enhance the power consumption.

'Enhance the power consumption'? That doesn't sound like something you
would want ;) Presumably it is something to allow you to save power by
dynamically adjusting LCD backlight and pixel brightness/contrast
depending on screen content? I believe this is typically called content
adaptive backlight control:

https://www.ecnmag.com/article/2010/04/content-adaptive-lcd-backlight-control

You spell out what DRC and SAT stands for in the driver source code,
perhaps it also makes sense to do it here?

Perhaps rewording it to something like this is clearer:

.. allows to dynamically adjust pixel brightness/contrast based on
histogram measurements for LCD content adaptive backlight control.

-- 
Bye, Peter Korsgaard

Re: [linux-sunxi] [PATCH 3/7] drm/sun4i: Add SAT and DRC drivers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3314 bytes --]

Hi Peter,

On Sun, Sep 04, 2016 at 10:03:06PM +0200, Peter Korsgaard wrote:
> >>>>> "Maxime" == Maxime Ripard <maxime.ripard@free-electrons.com> writes:
> 
> Hi,
> 
>  > The A33 pipeline also has some new components called SAT and DRC. Even
>  > though their exact features and programming model is not known (or
>  > documented), they need to be clocked for the pipeline to carry the video
>  > signal all the way.
> 
>  > Add minimal drivers for those that just claim the needed resources for the
>  > pipeline to operate properly.
> 
>  > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>  > ---
>  >  .../bindings/display/sunxi/sun4i-drm.txt           |  37 +++++++
>  >  drivers/gpu/drm/sun4i/Makefile                     |   3 +-
>  >  drivers/gpu/drm/sun4i/sun6i_drc.c                  | 117 +++++++++++++++++++++
>  >  drivers/gpu/drm/sun4i/sun8i_sat.c                  | 105 ++++++++++++++++++
>  >  4 files changed, 261 insertions(+), 1 deletion(-)
>  >  create mode 100644 drivers/gpu/drm/sun4i/sun6i_drc.c
>  >  create mode 100644 drivers/gpu/drm/sun4i/sun8i_sat.c
> 
>  > diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
>  > index d467ea93ac08..87c3c8dd34cb 100644
>  > --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
>  > +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
>  > @@ -51,6 +51,43 @@ Required properties:
>  >    second the block connected to the TCON channel 1 (usually the TV
>  >    encoder)
>  
>  > +SAT
>  > +---
>  > +
>  > +The SAT, found in the A33, allows to do some color correction.
>  > +
>  > +Required properties:
>  > +  - compatible: value must be one of:
>  > +    * allwinner,sun8i-a33-sat
>  > +  - reg: base address and size of the memory-mapped region.
>  > +  - clock: phandles to bus clock feeding the SAT
>  > +  - resets: phandles to the reset line driving the SAT
>  > +
>  > +- ports: A ports node with endpoint definitions as defined in
>  > +  Documentation/devicetree/bindings/media/video-interfaces.txt. The
>  > +  first port should be the input endpoints, the second one the outputs
>  > +
>  > +DRC
>  > +---
>  > +
>  > +The DRC, found in the latest Allwinner SoCs (A31, A23, A33), allows to
>  > +do some backlight control to enhance the power consumption.
> 
> 'Enhance the power consumption'? That doesn't sound like something you
> would want ;) Presumably it is something to allow you to save power by
> dynamically adjusting LCD backlight and pixel brightness/contrast
> depending on screen content? I believe this is typically called content
> adaptive backlight control:
> 
> https://www.ecnmag.com/article/2010/04/content-adaptive-lcd-backlight-control
> 
> You spell out what DRC and SAT stands for in the driver source code,
> perhaps it also makes sense to do it here?
> 
> Perhaps rewording it to something like this is clearer:
> 
> .. allows to dynamically adjust pixel brightness/contrast based on
> histogram measurements for LCD content adaptive backlight control.

You're right, I changed it.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[PATCH 4/7] drm/panel: Add Sinlinx SinA33 7" panel

[Maxime Ripard]

The SinA33 has an unidentified panel. Add the timings for it under a new
compatible.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpu/drm/panel/panel-simple.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 85143d1b9b31..af142e804245 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1409,6 +1409,29 @@ static const struct panel_desc shelly_sca07010_bfn_lnn = {
 	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
 };
 
+static const struct drm_display_mode sinlinx_sina33_lcd_7_mode = {
+	.clock = 66000,
+	.hdisplay = 1024,
+	.hsync_start = 1024 + 160,
+	.hsync_end = 1024 + 160 + 70,
+	.htotal = 1024 + 160 + 70 + 90,
+	.vdisplay = 600,
+	.vsync_start = 600 + 127,
+	.vsync_end = 600 + 127 + 20,
+	.vtotal = 600 + 127 + 20 + 3,
+	.vrefresh = 60,
+};
+
+static const struct panel_desc sinlinx_sina33_lcd_7 = {
+	.modes = &sinlinx_sina33_lcd_7_mode,
+	.num_modes = 1,
+	.size = {
+		.width = 154,
+		.height = 87,
+	},
+	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
+};
+
 static const struct drm_display_mode starry_kr122ea0sra_mode = {
 	.clock = 147000,
 	.hdisplay = 1920,
@@ -1644,6 +1667,9 @@ static const struct of_device_id platform_of_match[] = {
 		.compatible = "shelly,sca07010-bfn-lnn",
 		.data = &shelly_sca07010_bfn_lnn,
 	}, {
+		.compatible = "sinlinx,sina33-lcd-7",
+		.data = &sinlinx_sina33_lcd_7,
+	}, {
 		.compatible = "starry,kr122ea0sra",
 		.data = &starry_kr122ea0sra,
 	}, {
-- 
2.9.2

Re: [PATCH 4/7] drm/panel: Add Sinlinx SinA33 7" panel

[Chen-Yu Tsai]

On Thu, Sep 1, 2016 at 11:32 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> The SinA33 has an unidentified panel. Add the timings for it under a new
> compatible.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 85143d1b9b31..af142e804245 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1409,6 +1409,29 @@ static const struct panel_desc shelly_sca07010_bfn_lnn = {
>         .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
>  };
>
> +static const struct drm_display_mode sinlinx_sina33_lcd_7_mode = {

Nit on the name: the lcd panel also works with sina31s. The only
difference is the ribbon cable used to connect it to the board.
Maybe sinlinx_sinaxx_lcd_7? Just a suggestion though, the patch
looks fine.

ChenYu

> +       .clock = 66000,
> +       .hdisplay = 1024,
> +       .hsync_start = 1024 + 160,
> +       .hsync_end = 1024 + 160 + 70,
> +       .htotal = 1024 + 160 + 70 + 90,
> +       .vdisplay = 600,
> +       .vsync_start = 600 + 127,
> +       .vsync_end = 600 + 127 + 20,
> +       .vtotal = 600 + 127 + 20 + 3,
> +       .vrefresh = 60,
> +};
> +
> +static const struct panel_desc sinlinx_sina33_lcd_7 = {
> +       .modes = &sinlinx_sina33_lcd_7_mode,
> +       .num_modes = 1,
> +       .size = {
> +               .width = 154,
> +               .height = 87,
> +       },
> +       .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
> +};
> +
>  static const struct drm_display_mode starry_kr122ea0sra_mode = {
>         .clock = 147000,
>         .hdisplay = 1920,
> @@ -1644,6 +1667,9 @@ static const struct of_device_id platform_of_match[] = {
>                 .compatible = "shelly,sca07010-bfn-lnn",
>                 .data = &shelly_sca07010_bfn_lnn,
>         }, {
> +               .compatible = "sinlinx,sina33-lcd-7",
> +               .data = &sinlinx_sina33_lcd_7,
> +       }, {
>                 .compatible = "starry,kr122ea0sra",
>                 .data = &starry_kr122ea0sra,
>         }, {
> --
> 2.9.2
>

Re: [PATCH 4/7] drm/panel: Add Sinlinx SinA33 7" panel

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

Hi,

On Mon, Sep 05, 2016 at 01:03:03AM +0800, Icenowy Zheng wrote:
> Hi Everyone,
> 
> 01.09.2016, 23:40, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> >  The SinA33 has an unidentified panel. Add the timings for it under a new
> >  compatible.
> 
> 
> 
> Excuse me...
> I will ask a question which is not fully related to the patch here...
> If I want to add a generic panel for Q8 tablets, what should it be called?
> "allwinner,q8-lcd-panel-800x480"?

I guess it's more of a question for Thierry, but it seems like the
trend is to put the diagonal rather than the resolution in the
compatibles.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH 4/7] drm/panel: Add Sinlinx SinA33 7" panel

[Chen-Yu Tsai]

On Tue, Sep 6, 2016 at 4:02 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Mon, Sep 05, 2016 at 01:03:03AM +0800, Icenowy Zheng wrote:
>> Hi Everyone,
>>
>> 01.09.2016, 23:40, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
>> >  The SinA33 has an unidentified panel. Add the timings for it under a new
>> >  compatible.
>>
>>
>>
>> Excuse me...
>> I will ask a question which is not fully related to the patch here...
>> If I want to add a generic panel for Q8 tablets, what should it be called?
>> "allwinner,q8-lcd-panel-800x480"?
>
> I guess it's more of a question for Thierry, but it seems like the
> trend is to put the diagonal rather than the resolution in the
> compatibles.

I think Hans (CC-ed) has some q8 tablet that has a 1024x600 LCD.
Both variants are 7". Without the exact model number we would need
the resolution to tell them apart.

ChenYu

Re: [PATCH 4/7] drm/panel: Add Sinlinx SinA33 7" panel

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

On Mon, Sep 05, 2016 at 10:02:48PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Sep 05, 2016 at 01:03:03AM +0800, Icenowy Zheng wrote:
> > Hi Everyone,
> > 
> > 01.09.2016, 23:40, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> > >  The SinA33 has an unidentified panel. Add the timings for it under a new
> > >  compatible.
> > 
> > 
> > 
> > Excuse me...
> > I will ask a question which is not fully related to the patch here...
> > If I want to add a generic panel for Q8 tablets, what should it be called?
> > "allwinner,q8-lcd-panel-800x480"?
> 
> I guess it's more of a question for Thierry, but it seems like the
> trend is to put the diagonal rather than the resolution in the
> compatibles.

Compatible strings should contain the model number of the panel. There
is no such thing as a "generic panel for Q8 tablets".

Also, how is it that these panels are unidentified? Has nobody tried to
open them up and look at the panel to find a model number?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 4/7] drm/panel: Add Sinlinx SinA33 7" panel

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]

On Tue, Sep 06, 2016 at 11:12:24AM +0200, Thierry Reding wrote:
> On Mon, Sep 05, 2016 at 10:02:48PM +0200, Maxime Ripard wrote:
> > Hi,
> > 
> > On Mon, Sep 05, 2016 at 01:03:03AM +0800, Icenowy Zheng wrote:
> > > Hi Everyone,
> > > 
> > > 01.09.2016, 23:40, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> > > >  The SinA33 has an unidentified panel. Add the timings for it under a new
> > > >  compatible.
> > > 
> > > 
> > > 
> > > Excuse me...
> > > I will ask a question which is not fully related to the patch here...
> > > If I want to add a generic panel for Q8 tablets, what should it be called?
> > > "allwinner,q8-lcd-panel-800x480"?
> > 
> > I guess it's more of a question for Thierry, but it seems like the
> > trend is to put the diagonal rather than the resolution in the
> > compatibles.
> 
> Compatible strings should contain the model number of the panel. There
> is no such thing as a "generic panel for Q8 tablets".
> 
> Also, how is it that these panels are unidentified? Has nobody tried to
> open them up and look at the panel to find a model number?

It is not always possible without breaking the device apart. And I can
understand not a lot of people want to do that.

But on that particular patch, I had a bunch of spare ones, and ended
up doing just that. The reference is from an unknown brand, but at
least the part number shows up on Google, I'll use that.

Maxime


-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[PATCH 5/7] ARM: sun8i: a33: Add display pipeline

[Maxime Ripard]

Add all the needed blocks to the A33 DTSI.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/sun8i-a33.dtsi | 184 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 184 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a33.dtsi b/arch/arm/boot/dts/sun8i-a33.dtsi
index deb0cd613e97..5f9dbd17eb50 100644
--- a/arch/arm/boot/dts/sun8i-a33.dtsi
+++ b/arch/arm/boot/dts/sun8i-a33.dtsi
@@ -64,6 +64,42 @@
 	};
 
 	soc@01c00000 {
+		tcon0: lcd-controller@01c0c000 {
+			compatible = "allwinner,sun8i-a23-tcon";
+			reg = <0x01c0c000 0x1000>;
+			interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_LCD>,
+				 <&ccu CLK_LCD_CH0>;
+			clock-names = "ahb",
+				      "tcon-ch0";
+			clock-output-names = "tcon-pixel-clock";
+			resets = <&ccu RST_BUS_LCD>;
+			reset-names = "lcd";
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				tcon0_in: port@0 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <0>;
+
+					tcon0_in_drc0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&drc0_out_tcon0>;
+					};
+				};
+
+				tcon0_out: port@1 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+				};
+			};
+		};
+
 		crypto: crypto-engine@01c15000 {
 			compatible = "allwinner,sun4i-a10-crypto";
 			reg = <0x01c15000 0x1000>;
@@ -104,6 +140,154 @@
 			status = "disabled";
 			#phy-cells = <1>;
 		};
+
+		fe0: display-frontend@01e00000 {
+			compatible = "allwinner,sun8i-a33-display-frontend";
+			reg = <0x01e00000 0x20000>;
+			interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_DE_FE>, <&ccu CLK_DE_FE>,
+				 <&ccu CLK_DRAM_DE_FE>;
+			clock-names = "ahb", "mod",
+				      "ram";
+			resets = <&ccu RST_BUS_DE_FE>;
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				fe0_out: port@1 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+
+					fe0_out_sat0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&sat0_in_fe0>;
+					};
+				};
+			};
+		};
+
+		be0: display-backend@01e60000 {
+			compatible = "allwinner,sun8i-a33-display-backend";
+			reg = <0x01e60000 0x10000>;
+			clocks = <&ccu CLK_BUS_DE_BE>, <&ccu CLK_DE_BE>,
+				 <&ccu CLK_DRAM_DE_BE>;
+			clock-names = "ahb", "mod",
+				      "ram";
+			resets = <&ccu RST_BUS_DE_BE>;
+
+			assigned-clocks = <&ccu CLK_DE_BE>;
+			assigned-clock-rates = <300000000>;
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				be0_in: port@0 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <0>;
+
+					be0_in_sat0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&sat0_out_be0>;
+					};
+				};
+
+				be0_out: port@1 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+
+					be0_out_drc0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&drc0_in_be0>;
+					};
+				};
+			};
+		};
+
+		drc0: drc@01e70000 {
+			compatible = "allwinner,sun8i-a33-drc";
+			reg = <0x01e70000 0x10000>;
+			interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_DRC>, <&ccu CLK_DRC>,
+				 <&ccu CLK_DRAM_DRC>;
+			clock-names = "ahb", "mod", "ram";
+			resets = <&ccu RST_BUS_DRC>;
+
+			assigned-clocks = <&ccu CLK_DRC>;
+			assigned-clock-rates = <300000000>;
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				drc0_in: port@0 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <0>;
+
+					drc0_in_be0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&be0_out_drc0>;
+					};
+				};
+
+				drc0_out: port@1 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+
+					drc0_out_tcon0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&tcon0_in_drc0>;
+					};
+				};
+			};
+		};
+
+		sat0: sat@01e80000 {
+			compatible = "allwinner,sun8i-a33-sat";
+			reg = <0x01e80000 0x1000>;
+			clocks = <&ccu CLK_BUS_SAT>;
+			resets = <&ccu RST_BUS_SAT>;
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				sat0_in: port@0 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <0>;
+
+					sat0_in_fe0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&fe0_out_sat0>;
+					};
+				};
+
+				sat0_out: port@1 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+
+					sat0_out_be0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&be0_in_sat0>;
+					};
+				};
+			};
+		};
+	};
+
+	de: display-engine {
+		compatible = "allwinner,sun8i-a33-display-engine";
+		allwinner,pipelines = <&fe0>;
+		status = "disabled";
 	};
 };
 
-- 
2.9.2

Re: [PATCH 5/7] ARM: sun8i: a33: Add display pipeline

[Chen-Yu Tsai]

Hi,

On Thu, Sep 1, 2016 at 11:32 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Add all the needed blocks to the A33 DTSI.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  arch/arm/boot/dts/sun8i-a33.dtsi | 184 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 184 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun8i-a33.dtsi b/arch/arm/boot/dts/sun8i-a33.dtsi
> index deb0cd613e97..5f9dbd17eb50 100644
> --- a/arch/arm/boot/dts/sun8i-a33.dtsi
> +++ b/arch/arm/boot/dts/sun8i-a33.dtsi
> @@ -64,6 +64,42 @@
>         };
>
>         soc@01c00000 {
> +               tcon0: lcd-controller@01c0c000 {
> +                       compatible = "allwinner,sun8i-a23-tcon";
> +                       reg = <0x01c0c000 0x1000>;
> +                       interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&ccu CLK_BUS_LCD>,
> +                                <&ccu CLK_LCD_CH0>;
> +                       clock-names = "ahb",
> +                                     "tcon-ch0";
> +                       clock-output-names = "tcon-pixel-clock";
> +                       resets = <&ccu RST_BUS_LCD>;
> +                       reset-names = "lcd";
> +                       status = "disabled";
> +
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               tcon0_in: port@0 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <0>;
> +
> +                                       tcon0_in_drc0: endpoint@0 {
> +                                               reg = <0>;
> +                                               remote-endpoint = <&drc0_out_tcon0>;
> +                                       };
> +                               };
> +
> +                               tcon0_out: port@1 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <1>;
> +                               };
> +                       };
> +               };
> +
>                 crypto: crypto-engine@01c15000 {
>                         compatible = "allwinner,sun4i-a10-crypto";
>                         reg = <0x01c15000 0x1000>;
> @@ -104,6 +140,154 @@
>                         status = "disabled";
>                         #phy-cells = <1>;
>                 };
> +
> +               fe0: display-frontend@01e00000 {
> +                       compatible = "allwinner,sun8i-a33-display-frontend";
> +                       reg = <0x01e00000 0x20000>;
> +                       interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&ccu CLK_BUS_DE_FE>, <&ccu CLK_DE_FE>,
> +                                <&ccu CLK_DRAM_DE_FE>;
> +                       clock-names = "ahb", "mod",
> +                                     "ram";
> +                       resets = <&ccu RST_BUS_DE_FE>;
> +                       status = "disabled";
> +
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               fe0_out: port@1 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <1>;
> +
> +                                       fe0_out_sat0: endpoint@0 {
> +                                               reg = <0>;
> +                                               remote-endpoint = <&sat0_in_fe0>;
> +                                       };
> +                               };
> +                       };
> +               };
> +
> +               be0: display-backend@01e60000 {
> +                       compatible = "allwinner,sun8i-a33-display-backend";
> +                       reg = <0x01e60000 0x10000>;

Please also list the interrupt, even though we don't use it yet.
The manual says it's 127 - 32 = 95.

> +                       clocks = <&ccu CLK_BUS_DE_BE>, <&ccu CLK_DE_BE>,
> +                                <&ccu CLK_DRAM_DE_BE>;
> +                       clock-names = "ahb", "mod",
> +                                     "ram";
> +                       resets = <&ccu RST_BUS_DE_BE>;
> +
> +                       assigned-clocks = <&ccu CLK_DE_BE>;
> +                       assigned-clock-rates = <300000000>;
> +
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               be0_in: port@0 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <0>;
> +
> +                                       be0_in_sat0: endpoint@0 {
> +                                               reg = <0>;
> +                                               remote-endpoint = <&sat0_out_be0>;
> +                                       };
> +                               };
> +
> +                               be0_out: port@1 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <1>;
> +
> +                                       be0_out_drc0: endpoint@0 {
> +                                               reg = <0>;
> +                                               remote-endpoint = <&drc0_in_be0>;
> +                                       };
> +                               };
> +                       };
> +               };
> +
> +               drc0: drc@01e70000 {
> +                       compatible = "allwinner,sun8i-a33-drc";
> +                       reg = <0x01e70000 0x10000>;
> +                       interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&ccu CLK_BUS_DRC>, <&ccu CLK_DRC>,
> +                                <&ccu CLK_DRAM_DRC>;
> +                       clock-names = "ahb", "mod", "ram";
> +                       resets = <&ccu RST_BUS_DRC>;
> +
> +                       assigned-clocks = <&ccu CLK_DRC>;
> +                       assigned-clock-rates = <300000000>;
> +
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               drc0_in: port@0 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <0>;
> +
> +                                       drc0_in_be0: endpoint@0 {
> +                                               reg = <0>;
> +                                               remote-endpoint = <&be0_out_drc0>;
> +                                       };
> +                               };
> +
> +                               drc0_out: port@1 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <1>;
> +
> +                                       drc0_out_tcon0: endpoint@0 {
> +                                               reg = <0>;
> +                                               remote-endpoint = <&tcon0_in_drc0>;
> +                                       };
> +                               };
> +                       };
> +               };
> +
> +               sat0: sat@01e80000 {
> +                       compatible = "allwinner,sun8i-a33-sat";
> +                       reg = <0x01e80000 0x1000>;
> +                       clocks = <&ccu CLK_BUS_SAT>;
> +                       resets = <&ccu RST_BUS_SAT>;
> +
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               sat0_in: port@0 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <0>;
> +
> +                                       sat0_in_fe0: endpoint@0 {
> +                                               reg = <0>;
> +                                               remote-endpoint = <&fe0_out_sat0>;
> +                                       };
> +                               };
> +
> +                               sat0_out: port@1 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <1>;
> +
> +                                       sat0_out_be0: endpoint@0 {
> +                                               reg = <0>;
> +                                               remote-endpoint = <&be0_in_sat0>;
> +                                       };
> +                               };

I'm worried about the representation here.

In the user manuals, the SAT is shown as part of the BE.
Look at it this way: if it did come before the BE and is
independent, we shouldn't have to bring the SAT out of
reset for simplefb to work.

For comparison, a similar function unit called "CMU" found on
the other post-sun6i SoCs has the same function description as
SAT on the A33. It uses the reserved registers at the beginning
of the BE address space.

> +                       };
> +               };
> +       };
> +
> +       de: display-engine {
> +               compatible = "allwinner,sun8i-a33-display-engine";
> +               allwinner,pipelines = <&fe0>;
> +               status = "disabled";
>         };

Put this node above soc@? Properly sorted that is.

Everything else looks good.

Regards
ChenYu


>  };
>
> --
> 2.9.2
>

Re: [PATCH 5/7] ARM: sun8i: a33: Add display pipeline

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2716 bytes --]

Hi,

On Fri, Sep 02, 2016 at 02:28:54PM +0800, Chen-Yu Tsai wrote:
> > +               be0: display-backend@01e60000 {
> > +                       compatible = "allwinner,sun8i-a33-display-backend";
> > +                       reg = <0x01e60000 0x10000>;
> 
> Please also list the interrupt, even though we don't use it yet.
> The manual says it's 127 - 32 = 95.

Yep, you're right.

> > +               sat0: sat@01e80000 {
> > +                       compatible = "allwinner,sun8i-a33-sat";
> > +                       reg = <0x01e80000 0x1000>;
> > +                       clocks = <&ccu CLK_BUS_SAT>;
> > +                       resets = <&ccu RST_BUS_SAT>;
> > +
> > +                       ports {
> > +                               #address-cells = <1>;
> > +                               #size-cells = <0>;
> > +
> > +                               sat0_in: port@0 {
> > +                                       #address-cells = <1>;
> > +                                       #size-cells = <0>;
> > +                                       reg = <0>;
> > +
> > +                                       sat0_in_fe0: endpoint@0 {
> > +                                               reg = <0>;
> > +                                               remote-endpoint = <&fe0_out_sat0>;
> > +                                       };
> > +                               };
> > +
> > +                               sat0_out: port@1 {
> > +                                       #address-cells = <1>;
> > +                                       #size-cells = <0>;
> > +                                       reg = <1>;
> > +
> > +                                       sat0_out_be0: endpoint@0 {
> > +                                               reg = <0>;
> > +                                               remote-endpoint = <&be0_in_sat0>;
> > +                                       };
> > +                               };
> 
> I'm worried about the representation here.
> 
> In the user manuals, the SAT is shown as part of the BE.  Look at it
> this way: if it did come before the BE and is independent, we
> shouldn't have to bring the SAT out of reset for simplefb to work.

Indeed.

> For comparison, a similar function unit called "CMU" found on the
> other post-sun6i SoCs has the same function description as SAT on
> the A33. It uses the reserved registers at the beginning of the BE
> address space.

Hmm, ok, so you would essentially, merge the backend and sat nodes?

That wouldn't be very hard to do, i'll do it.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH 5/7] ARM: sun8i: a33: Add display pipeline

[Chen-Yu Tsai]

On Tue, Sep 6, 2016 at 4:21 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Fri, Sep 02, 2016 at 02:28:54PM +0800, Chen-Yu Tsai wrote:
>> > +               be0: display-backend@01e60000 {
>> > +                       compatible = "allwinner,sun8i-a33-display-backend";
>> > +                       reg = <0x01e60000 0x10000>;
>>
>> Please also list the interrupt, even though we don't use it yet.
>> The manual says it's 127 - 32 = 95.
>
> Yep, you're right.
>
>> > +               sat0: sat@01e80000 {
>> > +                       compatible = "allwinner,sun8i-a33-sat";
>> > +                       reg = <0x01e80000 0x1000>;
>> > +                       clocks = <&ccu CLK_BUS_SAT>;
>> > +                       resets = <&ccu RST_BUS_SAT>;
>> > +
>> > +                       ports {
>> > +                               #address-cells = <1>;
>> > +                               #size-cells = <0>;
>> > +
>> > +                               sat0_in: port@0 {
>> > +                                       #address-cells = <1>;
>> > +                                       #size-cells = <0>;
>> > +                                       reg = <0>;
>> > +
>> > +                                       sat0_in_fe0: endpoint@0 {
>> > +                                               reg = <0>;
>> > +                                               remote-endpoint = <&fe0_out_sat0>;
>> > +                                       };
>> > +                               };
>> > +
>> > +                               sat0_out: port@1 {
>> > +                                       #address-cells = <1>;
>> > +                                       #size-cells = <0>;
>> > +                                       reg = <1>;
>> > +
>> > +                                       sat0_out_be0: endpoint@0 {
>> > +                                               reg = <0>;
>> > +                                               remote-endpoint = <&be0_in_sat0>;
>> > +                                       };
>> > +                               };
>>
>> I'm worried about the representation here.
>>
>> In the user manuals, the SAT is shown as part of the BE.  Look at it
>> this way: if it did come before the BE and is independent, we
>> shouldn't have to bring the SAT out of reset for simplefb to work.
>
> Indeed.
>
>> For comparison, a similar function unit called "CMU" found on the
>> other post-sun6i SoCs has the same function description as SAT on
>> the A33. It uses the reserved registers at the beginning of the BE
>> address space.
>
> Hmm, ok, so you would essentially, merge the backend and sat nodes?
>
> That wouldn't be very hard to do, i'll do it.

Yes. That is what I propose.

ChenYu

[PATCH 6/7] ARM: sun8i: a33: Add RGB666 pins

[Maxime Ripard]

The LCD output needs to be muxed. Add the proper pinctrl node.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/sun8i-a33.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a33.dtsi b/arch/arm/boot/dts/sun8i-a33.dtsi
index 5f9dbd17eb50..d5f93c05846f 100644
--- a/arch/arm/boot/dts/sun8i-a33.dtsi
+++ b/arch/arm/boot/dts/sun8i-a33.dtsi
@@ -300,6 +300,16 @@
 	interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>,
 		     <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
 
+	tcon0_rgb666_pins_a: tcon0_rgb666@0 {
+	        allwinner,pins = "PD2", "PD3", "PD4", "PD5", "PD6", "PD7",
+	                         "PD10", "PD11", "PD12", "PD13", "PD14", "PD15",
+	                         "PD18", "PD19", "PD20", "PD21", "PD22", "PD23",
+	                         "PD24", "PD25", "PD26", "PD27";
+		allwinner,function = "lcd0";
+		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+		allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+	};
+
 	uart0_pins_b: uart0@1 {
 		allwinner,pins = "PB0", "PB1";
 		allwinner,function = "uart0";
-- 
2.9.2

Re: [linux-sunxi] [PATCH 6/7] ARM: sun8i: a33: Add RGB666 pins

[Chen-Yu Tsai]

Hi,

On Thu, Sep 1, 2016 at 11:32 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> The LCD output needs to be muxed. Add the proper pinctrl node.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  arch/arm/boot/dts/sun8i-a33.dtsi | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun8i-a33.dtsi b/arch/arm/boot/dts/sun8i-a33.dtsi
> index 5f9dbd17eb50..d5f93c05846f 100644
> --- a/arch/arm/boot/dts/sun8i-a33.dtsi
> +++ b/arch/arm/boot/dts/sun8i-a33.dtsi
> @@ -300,6 +300,16 @@
>         interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>,
>                      <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
>
> +       tcon0_rgb666_pins_a: tcon0_rgb666@0 {

This is the only possible combination. You can drop the _a and @0.

Also this can be shared with A23, as they are pin compatible, and I
also matched the datasheets.

Otherwise

Acked-by: Chen-Yu Tsai <wens@csie.org>

> +               allwinner,pins = "PD2", "PD3", "PD4", "PD5", "PD6", "PD7",
> +                                "PD10", "PD11", "PD12", "PD13", "PD14", "PD15",
> +                                "PD18", "PD19", "PD20", "PD21", "PD22", "PD23",
> +                                "PD24", "PD25", "PD26", "PD27";
> +               allwinner,function = "lcd0";
> +               allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> +               allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> +       };
> +
>         uart0_pins_b: uart0@1 {
>                 allwinner,pins = "PB0", "PB1";
>                 allwinner,function = "uart0";
> --
> 2.9.2
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[PATCH 7/7] ARM: sun8i: sina33: Enable display

[Maxime Ripard]

Enable the display pipeline with the associated 7" panel sold with the
SinA33.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts | 34 ++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts b/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts
index fef6abc0a703..1fa2b5a80ec2 100644
--- a/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts
+++ b/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts
@@ -61,6 +61,27 @@
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
+
+	panel {
+		compatible = "sinlinx,sina33-lcd-7";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			panel_input: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&tcon0_out_panel>;
+			};
+		};
+	};
+};
+
+&de {
+	status = "okay";
 };
 
 &ehci0 {
@@ -207,6 +228,19 @@
 	regulator-name = "vcc-rtc";
 };
 
+&tcon0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&tcon0_rgb666_pins_a>;
+	status = "okay";
+};
+
+&tcon0_out {
+	tcon0_out_panel: endpoint@0 {
+		reg = <0>;
+		remote-endpoint = <&panel_input>;
+	};
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pins_b>;
-- 
2.9.2

Re: [PATCH 0/7] drm/sun4i: Introduce A33 display driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

Hi Icenowy,

On Fri, Sep 02, 2016 at 09:30:05AM +0800, Icenowy Zheng wrote:
> 
> 
> 01.09.2016, 23:40, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> > Hi everyone,
> >
> > This serie introduces the support in the sun4i-drm driver for the A33.
> >
> > Beside the new IPs and special cases for the A33 new IPs, there's
> > nothing really outstanding, and is now at feature parity with the A13.
> 
> How can the driver be modified to support LVDS screen?
> 
> I have an A33 tablet with a 1024x768 LVDS panel. (iNet D978 Rev2 board,
> which I pushed a dt a few days ago)

Awesome, I don't have such a screen myself, so feel free to work on
it.

I haven't looked into the details of LVDS, but it should be something
along the lines of commit 29e57fab97fc ("drm: sun4i: Add RGB output").

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH 0/7] drm/sun4i: Introduce A33 display driver

[Chen-Yu Tsai]

On Sat, Sep 3, 2016 at 3:06 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi Icenowy,
>
> On Fri, Sep 02, 2016 at 09:30:05AM +0800, Icenowy Zheng wrote:
>>
>>
>> 01.09.2016, 23:40, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
>> > Hi everyone,
>> >
>> > This serie introduces the support in the sun4i-drm driver for the A33.
>> >
>> > Beside the new IPs and special cases for the A33 new IPs, there's
>> > nothing really outstanding, and is now at feature parity with the A13.
>>
>> How can the driver be modified to support LVDS screen?
>>
>> I have an A33 tablet with a 1024x768 LVDS panel. (iNet D978 Rev2 board,
>> which I pushed a dt a few days ago)
>
> Awesome, I don't have such a screen myself, so feel free to work on
> it.
>
> I haven't looked into the details of LVDS, but it should be something
> along the lines of commit 29e57fab97fc ("drm: sun4i: Add RGB output").

The implementation might be along the lines of

  1. having multiple output ports, each for a different interface type.
     (Some platforms go this route)

Or

  2. having a DT property describe what the output interface is.

The RGB/TCON driver would then setup the registers accordingly.


ChenYu

Re: [PATCH 0/7] drm/sun4i: Introduce A33 display driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1670 bytes --]

On Sat, Sep 03, 2016 at 09:43:59AM +0800, Chen-Yu Tsai wrote:
> On Sat, Sep 3, 2016 at 3:06 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi Icenowy,
> >
> > On Fri, Sep 02, 2016 at 09:30:05AM +0800, Icenowy Zheng wrote:
> >>
> >>
> >> 01.09.2016, 23:40, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> >> > Hi everyone,
> >> >
> >> > This serie introduces the support in the sun4i-drm driver for the A33.
> >> >
> >> > Beside the new IPs and special cases for the A33 new IPs, there's
> >> > nothing really outstanding, and is now at feature parity with the A13.
> >>
> >> How can the driver be modified to support LVDS screen?
> >>
> >> I have an A33 tablet with a 1024x768 LVDS panel. (iNet D978 Rev2 board,
> >> which I pushed a dt a few days ago)
> >
> > Awesome, I don't have such a screen myself, so feel free to work on
> > it.
> >
> > I haven't looked into the details of LVDS, but it should be something
> > along the lines of commit 29e57fab97fc ("drm: sun4i: Add RGB output").
> 
> The implementation might be along the lines of
> 
>   1. having multiple output ports, each for a different interface type.
>      (Some platforms go this route)
> 
> Or
> 
>   2. having a DT property describe what the output interface is.
> 
> The RGB/TCON driver would then setup the registers accordingly.

Hmmm, yeah, we would need to adjust the bindings too...

I guess I'd prefer 1), but that would also be the most invasive
solution. I'm not sure how the DT maintainers feel about that.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH 0/7] drm/sun4i: Introduce A33 display driver

[Chen-Yu Tsai]

On Tue, Sep 6, 2016 at 4:37 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Sat, Sep 03, 2016 at 09:43:59AM +0800, Chen-Yu Tsai wrote:
>> On Sat, Sep 3, 2016 at 3:06 AM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > Hi Icenowy,
>> >
>> > On Fri, Sep 02, 2016 at 09:30:05AM +0800, Icenowy Zheng wrote:
>> >>
>> >>
>> >> 01.09.2016, 23:40, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
>> >> > Hi everyone,
>> >> >
>> >> > This serie introduces the support in the sun4i-drm driver for the A33.
>> >> >
>> >> > Beside the new IPs and special cases for the A33 new IPs, there's
>> >> > nothing really outstanding, and is now at feature parity with the A13.
>> >>
>> >> How can the driver be modified to support LVDS screen?
>> >>
>> >> I have an A33 tablet with a 1024x768 LVDS panel. (iNet D978 Rev2 board,
>> >> which I pushed a dt a few days ago)
>> >
>> > Awesome, I don't have such a screen myself, so feel free to work on
>> > it.
>> >
>> > I haven't looked into the details of LVDS, but it should be something
>> > along the lines of commit 29e57fab97fc ("drm: sun4i: Add RGB output").
>>
>> The implementation might be along the lines of
>>
>>   1. having multiple output ports, each for a different interface type.
>>      (Some platforms go this route)
>>
>> Or
>>
>>   2. having a DT property describe what the output interface is.
>>
>> The RGB/TCON driver would then setup the registers accordingly.
>
> Hmmm, yeah, we would need to adjust the bindings too...
>
> I guess I'd prefer 1), but that would also be the most invasive
> solution. I'm not sure how the DT maintainers feel about that.

I wonder if the TCON could use its 2 channels simultaneously?
Like output to one LCD, then mirror through HDMI/VGA?
The first option would be able to cover this better?

And you still need to add outgoing endpoints for the HDMI block.

In addition we'll have to rework the TV encoder binding as well.

The 2 TV encoders (on the A20) each have four DACs, which map
onto 4 external pins. The address space includes a not so easy
to use mux. More importantly, the binding needs to specify which
pin is used for what signal (RGB, YUV, S/Video, composite).
There seems to be an implicit rule that 1 pin is always used
for composite, and the 3 others RGB, though.


Regards
ChenYu

Re: [PATCH 0/7] drm/sun4i: Introduce A33 display driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1908 bytes --]

On Tue, Sep 06, 2016 at 10:50:09AM +0800, Chen-Yu Tsai wrote:
> >> The implementation might be along the lines of
> >>
> >>   1. having multiple output ports, each for a different interface type.
> >>      (Some platforms go this route)
> >>
> >> Or
> >>
> >>   2. having a DT property describe what the output interface is.
> >>
> >> The RGB/TCON driver would then setup the registers accordingly.
> >
> > Hmmm, yeah, we would need to adjust the bindings too...
> >
> > I guess I'd prefer 1), but that would also be the most invasive
> > solution. I'm not sure how the DT maintainers feel about that.
> 
> I wonder if the TCON could use its 2 channels simultaneously?

No, it's mutually exclusive.

> Like output to one LCD, then mirror through HDMI/VGA?
> The first option would be able to cover this better?

Even if it wasn't exclusive, that wouldn't be possible
unfortunately. Or rather, this would be possible if the LCD and the
HDMI screen had the same timings, which is very unlikely.

> And you still need to add outgoing endpoints for the HDMI block.

Indeed.

> In addition we'll have to rework the TV encoder binding as well.
> 
> The 2 TV encoders (on the A20) each have four DACs, which map
> onto 4 external pins. The address space includes a not so easy
> to use mux. More importantly, the binding needs to specify which
> pin is used for what signal (RGB, YUV, S/Video, composite).
> There seems to be an implicit rule that 1 pin is always used
> for composite, and the 3 others RGB, though.

I'm not sure why we would need to rework this one though. We have no
way to detect whether the screen is connected or not on either
connectors, and we can't have both output running at the same time for
the same reason than mention above.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH 0/7] drm/sun4i: Introduce A33 display driver

[Chen-Yu Tsai]

On Wed, Sep 7, 2016 at 2:54 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Tue, Sep 06, 2016 at 10:50:09AM +0800, Chen-Yu Tsai wrote:
>> >> The implementation might be along the lines of
>> >>
>> >>   1. having multiple output ports, each for a different interface type.
>> >>      (Some platforms go this route)
>> >>
>> >> Or
>> >>
>> >>   2. having a DT property describe what the output interface is.
>> >>
>> >> The RGB/TCON driver would then setup the registers accordingly.
>> >
>> > Hmmm, yeah, we would need to adjust the bindings too...
>> >
>> > I guess I'd prefer 1), but that would also be the most invasive
>> > solution. I'm not sure how the DT maintainers feel about that.
>>
>> I wonder if the TCON could use its 2 channels simultaneously?
>
> No, it's mutually exclusive.

I don't see how though. Are you referring to the IO_Map_Sel bit?
I assume that only controls the external output pins though.

>> Like output to one LCD, then mirror through HDMI/VGA?
>> The first option would be able to cover this better?
>
> Even if it wasn't exclusive, that wouldn't be possible
> unfortunately. Or rather, this would be possible if the LCD and the
> HDMI screen had the same timings, which is very unlikely.

What about an LCD-VGA bridge + HDMI in mirror mode on sun6i?
That should work.

>> And you still need to add outgoing endpoints for the HDMI block.
>
> Indeed.

On second thought, this particular one shouldn't affect the binding.

>
>> In addition we'll have to rework the TV encoder binding as well.
>>
>> The 2 TV encoders (on the A20) each have four DACs, which map
>> onto 4 external pins. The address space includes a not so easy
>> to use mux. More importantly, the binding needs to specify which
>> pin is used for what signal (RGB, YUV, S/Video, composite).
>> There seems to be an implicit rule that 1 pin is always used
>> for composite, and the 3 others RGB, though.
>
> I'm not sure why we would need to rework this one though. We have no
> way to detect whether the screen is connected or not on either
> connectors, and we can't have both output running at the same time for
> the same reason than mention above.

I would like to add connector nodes. At least we can specify stuff
like what DAC outputs are used for which connector, what type of
connector, and a ddc bus for VGA connectors. I haven't worked out
the details though.

Regards
ChenYu

Re: [PATCH 0/7] drm/sun4i: Introduce A33 display driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3179 bytes --]

Hi,

On Wed, Sep 07, 2016 at 12:49:58PM +0800, Chen-Yu Tsai wrote:
> On Wed, Sep 7, 2016 at 2:54 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Tue, Sep 06, 2016 at 10:50:09AM +0800, Chen-Yu Tsai wrote:
> >> >> The implementation might be along the lines of
> >> >>
> >> >>   1. having multiple output ports, each for a different interface type.
> >> >>      (Some platforms go this route)
> >> >>
> >> >> Or
> >> >>
> >> >>   2. having a DT property describe what the output interface is.
> >> >>
> >> >> The RGB/TCON driver would then setup the registers accordingly.
> >> >
> >> > Hmmm, yeah, we would need to adjust the bindings too...
> >> >
> >> > I guess I'd prefer 1), but that would also be the most invasive
> >> > solution. I'm not sure how the DT maintainers feel about that.
> >>
> >> I wonder if the TCON could use its 2 channels simultaneously?
> >
> > No, it's mutually exclusive.
> 
> I don't see how though. Are you referring to the IO_Map_Sel bit?

Yes.

> I assume that only controls the external output pins though.

As far as I know, channel 1 has no external pins, it's always one of
the blocks using the channel 1 that have external pins.

> >> Like output to one LCD, then mirror through HDMI/VGA?
> >> The first option would be able to cover this better?
> >
> > Even if it wasn't exclusive, that wouldn't be possible
> > unfortunately. Or rather, this would be possible if the LCD and the
> > HDMI screen had the same timings, which is very unlikely.
> 
> What about an LCD-VGA bridge + HDMI in mirror mode on sun6i?
> That should work.

The same resolution doesn't mean you have the same timings. The
porches and sync length might be different, in which case you'll have
to have a different pixel clock and different register set ups.

> >> In addition we'll have to rework the TV encoder binding as well.
> >>
> >> The 2 TV encoders (on the A20) each have four DACs, which map
> >> onto 4 external pins. The address space includes a not so easy
> >> to use mux. More importantly, the binding needs to specify which
> >> pin is used for what signal (RGB, YUV, S/Video, composite).
> >> There seems to be an implicit rule that 1 pin is always used
> >> for composite, and the 3 others RGB, though.
> >
> > I'm not sure why we would need to rework this one though. We have no
> > way to detect whether the screen is connected or not on either
> > connectors, and we can't have both output running at the same time for
> > the same reason than mention above.
> 
> I would like to add connector nodes. At least we can specify stuff
> like what DAC outputs are used for which connector, what type of
> connector, and a ddc bus for VGA connectors. I haven't worked out
> the details though.

The DAC really don't matter. It's purely internal to the driver
itself, it doesn't change from one SoC to the other, so it doesn't
really make sense to have it in the DT.

And is there any design that uses an i2c bus for DDC together with the
TV Encoder?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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