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

[Rob Herring]

On Thu, Sep 26, 2019 at 11:54 AM Maciej Falkowski
<m.falkowski@xxxxxxxxxxx> wrote:
>
>
> On 9/26/19 5:35 PM, Rob Herring wrote:
> > 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.
> Hi Rob,
> Thank you for explaining that.
> >> 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
>
> So when properties are only defined inside if-then scope,
> they are labeled as 'additional' as they are not defined inside
> scope of 'properties'. It is mandatory then to mention 'clock' and
> 'clock-names' there if 'additionalProperties: false' .

Yes, which makes additionalProperties a bit fragile and can't be used
if we include a common schema. There's a fix for that coming in
json-schema draft8 called 'unevaluatedProperties'.

> However I had not set it intentionally as there are additional
> properties in some bindings,
> exactly 'iommu' and 'power-domains' are undocumented.
> Is it a good way to put them in 'properties' just to be able to forbid
> additional properties?

Simply put, they should be documented too.

Rob