Hi Laurent,
Am Samstag, 15. Juli 2017, 12:23:12 CEST schrieb Laurent Pinchart:
> On Saturday 15 Jul 2017 14:58:40 Jacob Chen wrote:
> > Add DT bindings documentation for Rockchip RGA
> >
> > Signed-off-by: Yakir Yang <ykk@xxxxxxxxxxxxxx>
> > Signed-off-by: Jacob Chen <jacob-chen@xxxxxxxxxx>
> > ---
> > .../devicetree/bindings/media/rockchip-rga.txt | 35 +++++++++++++++++++
> > 1 file changed, 35 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/rockchip-rga.txt
> >
> > diff --git a/Documentation/devicetree/bindings/media/rockchip-rga.txt
> > b/Documentation/devicetree/bindings/media/rockchip-rga.txt new file mode
> > 100644
> > index 0000000..966eba0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/rockchip-rga.txt
> > @@ -0,0 +1,35 @@
> > +device-tree bindings for rockchip 2D raster graphic acceleration controller
> > (RGA)
> > +
> > +RGA is a separate 2D raster graphic acceleration unit. It accelerates 2D
>
> "Separate" from what ? Do you mean "standalone" ?
>
> > +graphics operations, such as point/line drawing, image scaling, rotation,
> > +BitBLT, alpha blending and image blur/sharpness.
> > +
> > +Required properties:
> > +- compatible: value should be one of the following
> > + "rockchip,rk3228-rga";
> > + "rockchip,rk3288-rga";
> > + "rockchip,rk3399-rga";
>
> The driver in patch 2/6 has match entry for rk3328, which is missing from this
> list.
>
> As the implementation of the driver doesn't seem to discriminate between the
> four SoCs, wouldn't it make sense to create a generic compatible string on
> which the driver would match ? You can have both the generic and SoC-specific
> compatible strings in DT if there are differences between the IP core in those
> SoCs that might need to be handled later by the driver.
I think the block is named something like RGA2 in some kernel trees, but
am not sure if that is an actual name, or someone just added a number.
>From short glances at vendor rga-code in the past, it looks like there are
currently 2 basic versions of the in existence I.e. older Rockchip socs
(like the rk3188 or so) use something older.
I think everywhere else we do have only (or at least mostly) soc-specifc
compatibles, but I guess that is more a question what Rob prefers.
Heiko
> > +- interrupts: RGA interrupt number.
>
> This is an "interrupt specifier", not just an "interrupt number" (as you can
> see in the example below there are three numbers)
>
> > +
> > +- clocks: phandle to RGA sclk/hclk/aclk clocks
> > +
> > +- clock-names: should be "aclk" "hclk" and "sclk"
>
> Nitpicking, there should be a comma after "aclk".
>
> > +
> > +- resets: Must contain an entry for each entry in reset-names.
> > + See ../reset/reset.txt for details.
> > +- reset-names: should be "core" "axi" and "ahb"
>
> And a comma after "core".
>
> > +
> > +Example:
> > +SoC specific DT entry:
>
> s/SoC specific/SoC-specific/
>
> > + rga: rga@ff680000 {
> > + compatible = "rockchip,rk3399-rga";
> > + reg = <0xff680000 0x10000>;
> > + interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>;
> > + interrupt-names = "rga";
>
> The interrupt-names property is not described above. Do you really need it ?
>
> > + clocks = <&cru ACLK_RGA>, <&cru HCLK_RGA>, <&cru
> SCLK_RGA_CORE>;
> > + clock-names = "aclk", "hclk", "sclk";
> > +
> > + resets = <&cru SRST_RGA_CORE>, <&cru SRST_A_RGA>, <&cru
> SRST_H_RGA>;
> > + reset-names = "core, "axi", "ahb";
> > + };
>
>
