Re: [PATCH v2 1/6] dt-bindings: rtc: Convert snps,dw-apb-timer to DT schema

On Tue, Mar 24, 2020 at 08:43:20PM +0300, Sergey.Semin@baikalelectronics.ru wrote:

From: Serge Semin <redacted>

Modern device tree bindings are supposed to be created as YAML-files
in accordance with DT schema. This commit replaces Synopsys DW Timer
legacy bare text binding with YAML file. As before the binding file
states that the corresponding dts node is supposed to be compatible
with generic DW APB Timer indicated by the "snps,dw-apb-timer"
compatible string and to provide a mandatory registers memory range,
one timer interrupt, either reference clock source or a fixed clock
rate value. It may also have an optional APB bus reference clock
phandle specified.

Signed-off-by: Serge Semin <redacted>
Cc: Alexey Malahov <redacted>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <redacted>
Cc: Alessandro Zummo <redacted>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Daniel Lezcano <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Cc: linux-rtc@vger.kernel.org

---

I have doubts that this binding file belongs to the bindings/rtc
directory seeing it's a pure timer with no rtc facilities like
days/months/years counting and alarms. What about moving it to the
"Documentation/devicetree/bindings/timer/" directory?

+1

I also don't know who is the corresponding driver maintainer, so I added
Daniel Lezcano to the maintainers schema. Any idea what email should be
specified there instead?

Please also note, that "oneOf: - required: ..." pattern isn't working
here. So if you omit any of the clock-related property the
dt_binding_check procedure won't fail. Seeing the anyOf schema is working
I suppose this happens due to the dtschema/lib.py script, which replaces
the global oneOf with a fixup for the interrupts/interrupts-extended
properties:

quoted

def fixup_interrupts(schema):
   # Supporting 'interrupts' implies 'interrupts-extended' is also supported.
   if not 'interrupts' in schema['properties'].keys():
       return

   # Any node with 'interrupts' can have 'interrupt-parent'
   schema['properties']['interrupt-parent'] = True

   schema['properties']['interrupts-extended'] = { "$ref": "#/properties/interrupts" };

   if not ('required' in schema.keys() and 'interrupts' in schema['required']):
       return

!>    # Currently no better way to express either 'interrupts' or 'interrupts-extended'
!>    # is required. If this fails validation, the error reporting is the whole
!>    # schema file fails validation
!>    schema['oneOf'] = [ {'required': ['interrupts']}, {'required': ['interrupts-extended']} ]

I'll fix this. I'll have to check for 'oneOf' and if it exists then put 
it under an 'allOf'.

---
 .../devicetree/bindings/rtc/dw-apb.txt        | 32 -------
 .../bindings/rtc/snps,dw-apb-timer.yaml       | 88 +++++++++++++++++++
 2 files changed, 88 insertions(+), 32 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/rtc/dw-apb.txt
 create mode 100644 Documentation/devicetree/bindings/rtc/snps,dw-apb-timer.yaml

Otherwise, looks good.

Reviewed-by: Rob Herring <robh@kernel.org>