Re: [PATCH 1/2] dt-bindings: memory: convert Marvell MVEBU SDRAM controller to dtschema

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 17/08/2021 12:58, Jan Lübbe wrote:
> On Tue, 2021-08-17 at 12:52 +0200, Krzysztof Kozlowski wrote:
>> On 17/08/2021 12:46, Jan Lübbe wrote:
>>> On Tue, 2021-08-17 at 11:38 +0200, Krzysztof Kozlowski wrote:
>>>> Convert Marvell MVEBU SDRAM controller bindings to DT schema format
>>>> using json-schema.
>>>>
>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>>>> ---
>>>>  .../marvell,mvebu-sdram-controller.yaml       | 31 +++++++++++++++++++
>>>>  .../mvebu-sdram-controller.txt                | 21 -------------
>>>>  2 files changed, 31 insertions(+), 21 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/marvell,mvebu-sdram-controller.yaml
>>>>  delete mode 100644 Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/marvell,mvebu-sdram-controller.yaml b/Documentation/devicetree/bindings/memory-controllers/marvell,mvebu-sdram-controller.yaml
>>>> new file mode 100644
>>>> index 000000000000..14a6bc8f421f
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/marvell,mvebu-sdram-controller.yaml
>>>> @@ -0,0 +1,31 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/memory-controllers/marvell,mvebu-sdram-controller.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Marvell MVEBU SDRAM controller
>>>> +
>>>> +maintainers:
>>>> +  - Jan Luebbe <jlu@pengutronix.de>
>>>> +  - Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: marvell,armada-xp-sdram-controller
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    memory-controller@1400 {
>>>> +        compatible = "marvell,armada-xp-sdram-controller";
>>>> +        reg = <0x1400 0x500>;
>>>> +    };
>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt b/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
>>>> deleted file mode 100644
>>>> index 89657d1d4cd4..000000000000
>>>> --- a/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
>>>> +++ /dev/null
>>>> @@ -1,21 +0,0 @@
>>>> -Device Tree bindings for MVEBU SDRAM controllers
>>>> -
>>>> -The Marvell EBU SoCs all have a SDRAM controller. The SDRAM controller
>>>> -differs from one SoC variant to another, but they also share a number
>>>> -of commonalities.
>>>> -
>>>> -For now, this Device Tree binding documentation only documents the
>>>> -Armada XP SDRAM controller.
>>>
>>> Please keep the description, otherwise it would be confusing why the binding is
>>> named marvell,mvebu-sdram-controller.yaml although it (currenly) only applies to
>>> the Armada XP.
>>>
>>
>> The description does not explain why it only documents Armada XP, so it
>> just duplicates what is in "compatible" part of bindings. How is the
>> confusion removed by saying "we document currently only Armada XP"? What
>> changes?
> 
> Then perhaps the binding should be named marvell,armada-xp-sdram-
> controller.yaml? It find it unlikely that support for the other SoC's
> controllers will be added, given their age.

Sure, could be like this but there is no confusion here and no need to
fix anything. It is understandable and common to name bindings file as
general (e.g. for group of devices) even though only limited amount of
devices are currently documented/described. It's almost everywhere like
this, when the bindings are not complete. I don't see where is the
confusion.

Look at: arm/marvell/armada-cpu-reset.txt which mentions several SoCs
while only three compatibles are documented.
arm/marvell/armada-cpu-reset.txt has generic name and describes only one
reset controller (370) while being used on multiple SoC (370, 375, 38x,
39x). I could go like this all over the bindings...


Best regards,
Krzysztof