Re: [PATCH v2] dt-bindings: display: Convert etnaviv to json-schema

[Philippe CORNU <philippe.cornu@st.com>]

Hi Benjamin,


On 1/28/20 1:31 PM, Benjamin GAIGNARD wrote:
> 
> On 1/28/20 1:06 PM, Maxime Ripard wrote:
>> Hi Benjamin,
>>
>> On Tue, Jan 28, 2020 at 09:20:13AM +0100, Benjamin Gaignard wrote:
>>> Convert etnaviv bindings to yaml format.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>>> ---
>>>    .../bindings/display/etnaviv/etnaviv-drm.txt       | 36 -----------
>>>    .../devicetree/bindings/gpu/vivante,gc.yaml        | 72 ++++++++++++++++++++++
>>>    2 files changed, 72 insertions(+), 36 deletions(-)
>>>    delete mode 100644 Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
>>>    create mode 100644 Documentation/devicetree/bindings/gpu/vivante,gc.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt b/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
>>> deleted file mode 100644
>>> index 8def11b16a24..000000000000
>>> --- a/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
>>> +++ /dev/null
>>> @@ -1,36 +0,0 @@
>>> -Vivante GPU core devices
>>> -========================
>>> -
>>> -Required properties:
>>> -- compatible: Should be "vivante,gc"
>>> -  A more specific compatible is not needed, as the cores contain chip
>>> -  identification registers at fixed locations, which provide all the
>>> -  necessary information to the driver.
>>> -- reg: should be register base and length as documented in the
>>> -  datasheet
>>> -- interrupts: Should contain the cores interrupt line
>>> -- clocks: should contain one clock for entry in clock-names
>>> -  see Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> -- clock-names:
>>> -   - "bus":    AXI/master interface clock
>>> -   - "reg":    AHB/slave interface clock
>>> -               (only required if GPU can gate slave interface independently)
>>> -   - "core":   GPU core clock
>>> -   - "shader": Shader clock (only required if GPU has feature PIPE_3D)
>>> -
>>> -Optional properties:
>>> -- power-domains: a power domain consumer specifier according to
>>> -  Documentation/devicetree/bindings/power/power_domain.txt
>>> -
>>> -example:
>>> -
>>> -gpu_3d: gpu@130000 {
>>> -	compatible = "vivante,gc";
>>> -	reg = <0x00130000 0x4000>;
>>> -	interrupts = <0 9 IRQ_TYPE_LEVEL_HIGH>;
>>> -	clocks = <&clks IMX6QDL_CLK_GPU3D_AXI>,
>>> -	         <&clks IMX6QDL_CLK_GPU3D_CORE>,
>>> -	         <&clks IMX6QDL_CLK_GPU3D_SHADER>;
>>> -	clock-names = "bus", "core", "shader";
>>> -	power-domains = <&gpc 1>;
>>> -};
>>> diff --git a/Documentation/devicetree/bindings/gpu/vivante,gc.yaml b/Documentation/devicetree/bindings/gpu/vivante,gc.yaml
>>> new file mode 100644
>>> index 000000000000..c4f549c0d750
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpu/vivante,gc.yaml
>>> @@ -0,0 +1,72 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/gpu/vivante,gc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Vivante GPU Bindings
>>> +
>>> +description: Vivante GPU core devices
>>> +
>>> +maintainers:
>>> +  -  Lucas Stach <l.stach@pengutronix.de>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: vivante,gc
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    items:
>>> +      - description: AXI/master interface clock
>>> +      - description: GPU core clock
>>> +      - description: Shader clock (only required if GPU has feature PIPE_3D)
>>> +      - description: AHB/slave interface clock (only required if GPU can gate slave interface independently)
>> Can you have an AHB slave interface clock without a shader clock?
> 
> No because the items in the list are ordered so you need to have, in
> order: "bus", "core", "shader", "reg"
> 
> If it is needed to allow any number of clock in any order I could write
> it like this:
> 
> clocks:
> 
>     minItems: 1
> 
>     maxItems: 4
> 
> clock-names:
> 
>     items:
> 
>       enum: [ bus, core, shader, reg]
> 
>     minItems: 1
> 
>     maxItems: 4
> 
> Benjamin


Thank you for your patch,

I confirm that your last proposal with enum would be better.

With that,
Reviewed-by: Philippe Cornu <philippe.cornu@st.com>

Philippe :-)


> 
>>
>>> +    minItems: 2
>>> +    maxItems: 4
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: bus
>>> +      - const: core
>>> +      - const: shader
>>> +      - const: reg
>>> +    minItems: 2
>>> +    maxItems: 4
>> If so, that check will fail, since it would expect a clock named
>> shader on the 3rd item.
>>
>> It looks good otherwise, thanks!
>> Maxime