Re: [PATCH v2] dt-bindings: gpu: Convert Samsung Image Scaler to dt-schema

[Rob Herring]

On Thu, Sep 26, 2019 at 9:47 AM Maciej Falkowski
<m.falkowski@xxxxxxxxxxx> wrote:
>
>
> On 9/26/19 4:03 PM, Krzysztof Kozlowski wrote:
> > On Thu, Sep 26, 2019 at 02:56:14PM +0200, Marek Szyprowski wrote:
> >> From: Maciej Falkowski <m.falkowski@xxxxxxxxxxx>
> >>
> >> Convert Samsung Image Scaler to newer dt-schema format.
> >>
> >> Signed-off-by: Maciej Falkowski <m.falkowski@xxxxxxxxxxx>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> >> ---
> >> v2:
> >> - Removed quotation marks from string in 'compatible' property
> >> - Added if-then statement for 'clocks' and 'clock-names' property
> >> - Added include directive to example
> >> - Added GIC_SPI macro to example
> >>
> >> Best regards,
> >> Maciej Falkowski
> >> ---
> >>   .../bindings/gpu/samsung-scaler.txt           | 27 -------
> >>   .../bindings/gpu/samsung-scaler.yaml          | 71 +++++++++++++++++++
> >>   2 files changed, 71 insertions(+), 27 deletions(-)
> >>   delete mode 100644 Documentation/devicetree/bindings/gpu/samsung-scaler.txt
> >>   create mode 100644 Documentation/devicetree/bindings/gpu/samsung-scaler.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/gpu/samsung-scaler.txt b/Documentation/devicetree/bindings/gpu/samsung-scaler.txt
> >> deleted file mode 100644
> >> index 9c3d98105dfd..000000000000
> >> --- a/Documentation/devicetree/bindings/gpu/samsung-scaler.txt
> >> +++ /dev/null
> >> @@ -1,27 +0,0 @@
> >> -* Samsung Exynos Image Scaler
> >> -
> >> -Required properties:
> >> -  - compatible : value should be one of the following:
> >> -    (a) "samsung,exynos5420-scaler" for Scaler IP in Exynos5420
> >> -    (b) "samsung,exynos5433-scaler" for Scaler IP in Exynos5433
> >> -
> >> -  - reg : Physical base address of the IP registers and length of memory
> >> -      mapped region.
> >> -
> >> -  - interrupts : Interrupt specifier for scaler interrupt, according to format
> >> -             specific to interrupt parent.
> >> -
> >> -  - clocks : Clock specifier for scaler clock, according to generic clock
> >> -         bindings. (See Documentation/devicetree/bindings/clock/exynos*.txt)
> >> -
> >> -  - clock-names : Names of clocks. For exynos scaler, it should be "mscl"
> >> -              on 5420 and "pclk", "aclk" and "aclk_xiu" on 5433.
> >> -
> >> -Example:
> >> -    scaler@12800000 {
> >> -            compatible = "samsung,exynos5420-scaler";
> >> -            reg = <0x12800000 0x1294>;
> >> -            interrupts = <0 220 IRQ_TYPE_LEVEL_HIGH>;
> >> -            clocks = <&clock CLK_MSCL0>;
> >> -            clock-names = "mscl";
> >> -    };
> >> diff --git a/Documentation/devicetree/bindings/gpu/samsung-scaler.yaml b/Documentation/devicetree/bindings/gpu/samsung-scaler.yaml
> >> new file mode 100644
> >> index 000000000000..af19930d052e
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/gpu/samsung-scaler.yaml
> >> @@ -0,0 +1,71 @@
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +%YAML 1.2
> >> +---
> >> +$id: https://protect2.fireeye.com/url?k=1ffa720fd467d028.1ffbf940-9a5a550397b4da2b&u=http://devicetree.org/schemas/gpu/samsung-scaler.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Samsung Exynos SoC Image Scaler
> >> +
> >> +maintainers:
> >> +  - Inki Dae <inki.dae@xxxxxxxxxxx>
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - samsung,exynos5420-scaler
> >> +      - samsung,exynos5433-scaler
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  interrupts:
> >> +    maxItems: 1
> >> +
>
> Hi Krzysztof,

Please work on your quoting. Reply below what you are replying to.

>
> By "Midgard" I assume that you referred to
> 'Documentation/devicetree/bindings/gpu/arm,mali-midgard.yaml'.
>
> I think that 'clocks' and 'clock-names' properties before if statement
> serve different purpose in this schema.
> It totally has about 10 different compatibles grouped in five pairs.
> Then schema declares for 'clocks' minItems as one and maxItems as two and
> later it overrides this boundaries with if statement for particular
> compatibles.

It's not an override, but an AND. So what's under 'properties' has to
be the looser constraints than what is under an if/then schema.

> Well, then clearly, the purpose is to declare boundaries for all of
> pairs and
> not to provide easy-to-find definition for this properties.
>
> In my schema I directly set boundaries per compatible with single
> if-else statement.
> I didn't know what to put before then as if statement is already
> self-explanatory.
>
> Best regards,
> Maciej Falkowski
>
> > I am repeating myself... leave the clocks and clock-names.
> >
> > "I think it is worth to leave the clocks and clock-names here (could be
> > empty or with min/max values for number of items). This makes it easy to
> > find the properties by humans.

I agree.

Let me put it another way. You need to add an 'additionalProperties:
false' and (I think) to make that work you'll need them listed here.

Rob