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

[Paul Cercueil <paul@crapouillou.net>]

Hi Sam,

Le sam. 16 mars 2019 à 21:59, Sam Ravnborg <sam@ravnborg.org> 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@crapouillou.net>
>>  Tested-by: Artur Rojek <contact@artur-rojek.eu>
>>  ---
>> 
>>  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