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

[Maxime Ripard]

Hi Peter,

On Sun, Sep 04, 2016 at 10:03:06PM +0200, Peter Korsgaard wrote:
> > "Maxime" == Maxime Ripard  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 
>  > ---
>  >  .../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
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

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

[Maxime Ripard]

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
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

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

[Peter Korsgaard]

> "Maxime" == Maxime Ripard  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 
 > ---
 >  .../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

[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
 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 
> ---
>  .../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 ..93ded536876b
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun6i_drc.c
> @@ -0,0 +1,117 @@
> +/*
> + * Copyright (C) 2016 Free Electrons
> + *
> + * Maxime Ripard 
> + *
> + * 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +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 

[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 
---
 .../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 ..93ded536876b
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun6i_drc.c
@@ -0,0 +1,117 @@
+/*
+ * Copyright (C) 2016 Free Electrons
+ *
+ * Maxime Ripard 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+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