Re: [RFC v3 1/8] dt-bindings: display: convert ingenic, lcd.txt to ingenic, lcd.yaml

["H. Nikolaus Schaller" <hns@goldelico.com>]

Hi Sam,

> Am 09.04.2020 um 09:25 schrieb Sam Ravnborg <sam@ravnborg.org>:
> 
> Hi Nikolaus
> 
> 
> Some comments below that will result in a simplee binding that passes
> the checks.
> Thanks for pushing this.
> 
> 	Sam
> 
> On Sun, Mar 29, 2020 at 07:35:47PM +0200, H. Nikolaus Schaller wrote:
>> and add compatible: jz4780-lcd, including an example how to
>> configure both lcd controllers.
>> 
>> Also fix the clock names and examples.
>> 
>> Based on work by Paul Cercueil <paul@crapouillou.net> and
>> Sam Ravnborg <sam@ravnborg.org>
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> ---
>> .../bindings/display/ingenic,lcd.txt          |  45 ------
>> .../bindings/display/ingenic,lcd.yaml         | 128 ++++++++++++++++++
>> 2 files changed, 128 insertions(+), 45 deletions(-)
>> delete mode 100644 Documentation/devicetree/bindings/display/ingenic,lcd.txt
>> create mode 100644 Documentation/devicetree/bindings/display/ingenic,lcd.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/display/ingenic,lcd.txt b/Documentation/devicetree/bindings/display/ingenic,lcd.txt
>> deleted file mode 100644
>> index 01e3261defb6..000000000000
>> --- a/Documentation/devicetree/bindings/display/ingenic,lcd.txt
>> +++ /dev/null
>> @@ -1,45 +0,0 @@
>> -Ingenic JZ47xx LCD driver
>> -
>> -Required properties:
>> -- compatible: one of:
>> -  * ingenic,jz4740-lcd
>> -  * ingenic,jz4725b-lcd
>> -  * ingenic,jz4770-lcd
>> -- 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.
>> -
>> -Example:
>> -
>> -panel {
>> -	compatible = "sharp,ls020b1dd01d";
>> -
>> -	backlight = <&backlight>;
>> -	power-supply = <&vcc>;
>> -
>> -	port {
>> -		panel_input: endpoint {
>> -			remote-endpoint = <&panel_output>;
>> -		};
>> -	};
>> -};
>> -
>> -
>> -lcd: lcd-controller@13050000 {
>> -	compatible = "ingenic,jz4725b-lcd";
>> -	reg = <0x13050000 0x1000>;
>> -
>> -	interrupt-parent = <&intc>;
>> -	interrupts = <31>;
>> -
>> -	clocks = <&cgu JZ4725B_CLK_LCD>;
>> -	clock-names = "lcd";
>> -
>> -	port {
>> -		panel_output: endpoint {
>> -			remote-endpoint = <&panel_input>;
>> -		};
>> -	};
>> -};
>> diff --git a/Documentation/devicetree/bindings/display/ingenic,lcd.yaml b/Documentation/devicetree/bindings/display/ingenic,lcd.yaml
>> new file mode 100644
>> index 000000000000..8b6467cfc191
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/ingenic,lcd.yaml
>> @@ -0,0 +1,128 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/ingenic,lcd.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Bindings for Ingenic JZ4780 LCD Controller
>> +
>> +maintainers:
>> +  - Paul Cercueil <paul@crapouillou.net>
>> +
>> +description: |
>> +  LCD Controller is the Display Controller for the Ingenic JZ47xx SoC
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +     - const: ingenic,jz4725b-lcd
>> +     - const: ingenic,jz4740-lcd
>> +     - const: ingenic,jz4770-lcd
>> +     - const: ingenic,jz4780-lcd
>> +
>> +  reg:
>> +    maxItems: 1
>> +    description: LCD registers location and length
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +    description: Specifies the interrupt provided by parent
>> +
>> +  clocks:
>> +    maxItems: 2
>> +    description: Clock specifiers for LCD pixclock and device clock.
>> +      The device clock is only required on the JZ4740 and JZ4780
>> +
>> +  clock-names:
>> +    items:
>> +      - const: lcd
>> +      - const: lcd_pclk
>> +
>> +  port:
>> +    type: object
>> +    description: |
>> +      A port node with endpoint definitions as defined in
>> +      Documentation/devicetree/bindings/media/video-interfaces.txt
>> +
>> +required:
>> +    - compatible
>> +    - reg
>> +    - interrupts
>> +    - clocks
>> +    - clock-names
>> +    - port
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/jz4725b-cgu.h>
>> +
>> +    panel {
>> +      compatible = "sharp,ls020b1dd01d";
>> +
>> +      backlight = <&backlight>;
>> +      power-supply = <&vcc>;
>> +
>> +      port {
>> +        panel_input: endpoint {
>> +          remote-endpoint = <&panel_output>;
>> +          };
>> +        };
>> +      };
> The panel part is not needed - better to drop it.

Well, it is needed to fulfill the remote-endpoint below.

> 
> 
>> +
>> +    lcd: lcd-controller@13050000 {
>> +      compatible = "ingenic,jz4725b-lcd";
>> +      reg = <0x13050000 0x1000>;
>> +
>> +      interrupt-parent = <&intc>;
>> +      interrupts = <31>;
>> +
>> +      clocks = <&cgu JZ4725B_CLK_LCD>;
>> +      clock-names = "lcd", "lcd_pclk";
>> +
>> +      port {
>> +        panel_output: endpoint {
>> +          remote-endpoint = <&panel_input>;
>> +          };
>> +        };
>> +      };
> We know this example will not pass the check, as there is only
> one clock specified.
> I suggest to drop this example.
> If it later turns out that jz4725b only have one clock,

Paul already reported that it only wants to see one clock.

> then the binding
> needs to be updated.

Yes, I have that on my to-do list to update the binding to reflect
this minItems/maxItems thing but I am not yet sure about how
to handle the clock-names in that case. I.e. make "lcd" optional
and enforce "lcd_pclk" only.

> But the best guess is that the example is wrong.
> 
> The example below for jz4780-lcd cover all relevant parts - so
> just keep it as the only example.
> 
>> +
>> +  - |
>> +    #include <dt-bindings/clock/jz4780-cgu.h>
>> +
>> +    lcdc0: lcdc0@13050000 {
> Name this lcdc
> And drop "lcdc0@13050000" as this is not relevant for this example.
> 
> Remember - the examples exist to explain the binding. They are
> just examples.
> 
>> +        compatible = "ingenic,jz4780-lcd";
>> +        reg = <0x13050000 0x1800>;
>> +
>> +        clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD0PIXCLK>;
>> +        clock-names = "lcd", "lcd_pclk";
>> +
>> +        interrupt-parent = <&intc>;
>> +        interrupts = <31>;
>> +
>> +        jz4780_lcd_out: port {
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            jz4780_out_hdmi: endpoint@0 {
>> +                reg = <0>;
>> +                remote-endpoint = <&hdmi_in_lcd>;
>> +            };
>> +        };
>> +    };
>> +
> 
> And drop this as it does not add anything extra.

Well, it demonstrates how to add a second lcdc which is disabled.

Showing that it is possible to do so is IMHO the most important
part of the example because it is not at all obvious.

I have also added both SoC to show how differently they can
and should be.

>> +    lcdc1: lcdc1@130a0000 {
>> +        compatible = "ingenic,jz4780-lcd";
>> +        reg = <0x130a0000 0x1800>;
>> +
>> +        clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD1PIXCLK>;
>> +        clock-names = "lcd", "lcd_pclk";
>> +
>> +        interrupt-parent = <&intc>;
>> +        interrupts = <31>;
>> +
>> +        status = "disabled";
>> +    };
> 
> 	Sam

BR and thanks,
Nikolaus