Re: [PATCH v2 1/3] dt-bindings: Add doc for the ingenic-drm driver

[Paul Cercueil]

Hi Sam,

Le sam. 16 mars 2019 à 21:59, Sam Ravnborg <sam@xxxxxxxxxxxx> a écrit 
:
> Hi Paul.
>
> On Fri, Mar 15, 2019 at 06:06:59PM +0100, Paul Cercueil wrote:
> >  Add documentation for the devicetree bindings of the DRM driver for 
> > the
> >  JZ47xx family of SoCs from Ingenic.
> >
> >  Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> >  Tested-by: Artur Rojek <contact@xxxxxxxxxxxxxx>
> >  ---
> >
> >  Notes:
> >      v2: Remove ingenic,panel property.
> >
> >   .../devicetree/bindings/display/ingenic,drm.txt    | 29 
> > ++++++++++++++++++++++
> >   1 file changed, 29 insertions(+)
> >   create mode 100644 
> > Documentation/devicetree/bindings/display/ingenic,drm.txt
> >
> >  diff --git 
> > a/Documentation/devicetree/bindings/display/ingenic,drm.txt 
> > b/Documentation/devicetree/bindings/display/ingenic,drm.txt
> >  new file mode 100644
> >  index 000000000000..f57a4ae0f7ac
> >  --- /dev/null
> >  +++ b/Documentation/devicetree/bindings/display/ingenic,drm.txt
> >  @@ -0,0 +1,29 @@
> >  +Ingenic JZ47xx DRM driver
> >  +
> >  +Required properties:
> >  +- compatible: one of:
> >  +  * ingenic,jz4740-drm
> >  +  * ingenic,jz4725b-drm
> >  +- reg: LCD registers location and length
> >  +- clocks: LCD pixclock and device clock specifiers.
> >  +	   The device clock is only required on the JZ4740.
> >  +- clock-names: "lcd_pclk" and "lcd"
> >  +- interrupts: Specifies the interrupt line the LCD controller is 
> > connected to.
> >  +
> >  +Optional properties:
> >  +- ingenic,lcd-mode: LCD mode to use with the display panel.
> >  +		    See <dt-bindings/display/ingenic,drm.h> for all the
> >  +		    possible values.
>
> The value of the ingenic,lcd-mode depends on the panel used.
> So this looks like a panel property and not a display property.
>
> The panel should be specified using an OF graph binding
> in a port sub-node.
> The remote-endpoint will specifify the panel properties
> and if the panel requires something special from the display driver,
> the panle should specify this.
> So one can change panel without touching the display node
> in the DT.

Then the 'ingenic,lcd-mode' property would be in the panel node,
even though it's not used by the panel driver but by the DRM driver?
That sounds strange to me.

> >  +
> >  +Example:
> >  +
> >  +lcd: lcd-controller@13050000 {
> >  +	compatible = "ingenic,jz4725b-drm";
> >  +	reg = <0x13050000 0x1000>;
> >  +
> >  +	interrupt-parent = <&intc>;
> >  +	interrupts = <31>;
> >  +
> >  +	clocks = <&cgu JZ4725B_CLK_LCD>;
> >  +	clock-names = "lcd";
> >  +};
>
> As hinted above the panel is not specified above and to follow the
> modern way to do so would be to use a OF graph binding.

It's done this way in the DRM driver now, but I didn't update the
example. I will update it in the next version.

-Paul