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' .
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?
Best regards,
Maciej Falkowski
>
