diff mbox series

[v4] dt-bindings: rtc: isl1208: Convert to json-schema

Message ID 20230509131249.80456-1-biju.das.jz@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series [v4] dt-bindings: rtc: isl1208: Convert to json-schema | expand

Commit Message

Biju Das May 9, 2023, 1:12 p.m. UTC 
Convert the isl1208 RTC device tree binding documentation to json-schema.

Update the example to match reality.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
v3->v4:
 * Added Rb tag from Krzysztof Kozlowski.
 * Dropped | from description 
 * Replaced the pin name #EVDET->EVDET in description.
 * Dropped oneOf from compatible.
v2->v3:
 * Updated interrupt-names property by keeping the list of names.
 * Removed Interrupts from required property as it may not be wired.
 * Removed isil,ev-evienb from required property.
RFC->v2:
 * Updated maintainers list
 * Updated description from original bindings
 * removed default from isil,ev-evienb properties to match with the original
   bindings.
 * Added conditional check for interrupts.
---
 .../devicetree/bindings/rtc/isil,isl1208.txt  | 38 ---------
 .../devicetree/bindings/rtc/isil,isl1208.yaml | 80 +++++++++++++++++++
 2 files changed, 80 insertions(+), 38 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/rtc/isil,isl1208.txt
 create mode 100644 Documentation/devicetree/bindings/rtc/isil,isl1208.yaml

Comments

Trent Piepho May 9, 2023, 7:03 p.m. UTC | #1 
On Tue, May 9, 2023 at 6:12 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> +
> +  interrupt-names:

Shouldn't this have minItems: 1 and maxItems: 2 as well?

> +    items:
> +      - const: irq
> +      - const: evident


> +
> +  isil,ev-evienb:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    description: |
> +      Enable or disable internal pull on EVIN pin
> +      Default will leave the non-volatile configuration of the pullup
> +      as is.
> +        <0> : Enables internal pull-up on evin pin
> +        <1> : Disables internal pull-up on evin pin

It appears this was not clear at first.  The default is not to use the
reset value, which is 0, but to leave the existing value unchanged.
The RTC settings are battery-backed and the RTC is not reset on boot
by the kernel.  The value might have been set on a previous boot, or
might have already been configured by the bootloader or BIOS.  This
was a common design in Linux RTC drivers.  The bootloader would
configure the RTC and then Linux driver was only design to be able to
get/set the time and alarms.  Stuff like programming the interrupt
pull-ups or setting calibration registers wasn't done by the kernel.

> +    then:
> +      properties:
> +        interrupts:
> +          maxItems: 2
> +    else:
> +      properties:
> +        interrupts:
> +          maxItems: 1

Add interrupt-names here too.

> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        rtc_twi: rtc@6f {
> +            compatible = "isil,isl1208";
> +            reg = <0x6f>;
> +        };
> +    };

I agree with Geert's original comment about switching from the most
complex to the simplest example.  It's better to show as much as
possible.

If it's not possible to make a valid example that shows interrupts and
evdet pull enable, then doesn't that mean the bindings don't work?
Geert Uytterhoeven May 10, 2023, 6:52 a.m. UTC | #2 
Hi Trent,

On Tue, May 9, 2023 at 9:03 PM Trent Piepho <tpiepho@gmail.com> wrote:
> On Tue, May 9, 2023 at 6:12 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > +
> > +  interrupt-names:
>
> Shouldn't this have minItems: 1 and maxItems: 2 as well?

> > +    then:
> > +      properties:
> > +        interrupts:
> > +          maxItems: 2
> > +    else:
> > +      properties:
> > +        interrupts:
> > +          maxItems: 1
>
> Add interrupt-names here too.

Isn't the relation interrupts <=> interrupt-names enforced by the
tooling?

Gr{oetje,eeting}s,

                        Geert
Krzysztof Kozlowski May 10, 2023, 6:58 a.m. UTC | #3 
On 10/05/2023 08:52, Geert Uytterhoeven wrote:
> Hi Trent,
> 
> On Tue, May 9, 2023 at 9:03 PM Trent Piepho <tpiepho@gmail.com> wrote:
>> On Tue, May 9, 2023 at 6:12 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>>> +
>>> +  interrupt-names:
>>
>> Shouldn't this have minItems: 1 and maxItems: 2 as well?
> 
>>> +    then:
>>> +      properties:
>>> +        interrupts:
>>> +          maxItems: 2
>>> +    else:
>>> +      properties:
>>> +        interrupts:
>>> +          maxItems: 1
>>
>> Add interrupt-names here too.
> 
> Isn't the relation interrupts <=> interrupt-names enforced by the
> tooling?

No, every constrain or schema code for one should be duplicated for
second. These can be done however in different ways, e.g.
interrupts:
  minItems: 1
  maxitems: 2
interrupt-names:
  minItems: 1
  items:
    - foo
    - bar

but the outcome - so how many items are expected - must be the same in
every branch/condition.

Best regards,
Krzysztof
Biju Das May 10, 2023, 11:37 a.m. UTC | #4 
Hi Krzysztof Kozlowski,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Wednesday, May 10, 2023 7:58 AM
> To: Geert Uytterhoeven <geert@linux-m68k.org>; Trent Piepho
> <tpiepho@gmail.com>
> Cc: Biju Das <biju.das.jz@bp.renesas.com>; Alessandro Zummo
> <a.zummo@towertech.it>; Alexandre Belloni <alexandre.belloni@bootlin.com>;
> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; linux-rtc@vger.kernel.org;
> devicetree@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>;
> Fabrizio Castro <fabrizio.castro.jz@renesas.com>; linux-renesas-
> soc@vger.kernel.org
> Subject: Re: [PATCH v4] dt-bindings: rtc: isl1208: Convert to json-schema
> 
> On 10/05/2023 08:52, Geert Uytterhoeven wrote:
> > Hi Trent,
> >
> > On Tue, May 9, 2023 at 9:03 PM Trent Piepho <tpiepho@gmail.com> wrote:
> >> On Tue, May 9, 2023 at 6:12 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> >>> +
> >>> +  interrupt-names:
> >>
> >> Shouldn't this have minItems: 1 and maxItems: 2 as well?
> >
> >>> +    then:
> >>> +      properties:
> >>> +        interrupts:
> >>> +          maxItems: 2
> >>> +    else:
> >>> +      properties:
> >>> +        interrupts:
> >>> +          maxItems: 1
> >>
> >> Add interrupt-names here too.
> >
> > Isn't the relation interrupts <=> interrupt-names enforced by the
> > tooling?
> 
> No, every constrain or schema code for one should be duplicated for second.
> These can be done however in different ways, e.g.
> interrupts:
>   minItems: 1
>   maxitems: 2
> interrupt-names:
>   minItems: 1
>   items:
>     - foo
>     - bar
> 
> but the outcome - so how many items are expected - must be the same in every
> branch/condition.

But this will result in duplication of items in 2 places right?

One like the above and other one in conditional branch.

Eg:

 interrupt-names:
   minItems: 1
   items:
     - foo
     - bar

and
...

+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - isil,isl1209
+              - isil,isl1219
+    then:
+      properties:
+        interrupts:
+          maxItems: 2
         interrupt-names:
           items:
             - foo
             - bar

Cheers,
Biju
Biju Das May 10, 2023, 12:42 p.m. UTC | #5 
Hi Trent Piepho,

> -----Original Message-----
> From: Trent Piepho <tpiepho@gmail.com>
> Sent: Tuesday, May 9, 2023 8:04 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Rob Herring <robh+dt@kernel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; linux-rtc@vger.kernel.org;
> devicetree@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>;
> Fabrizio Castro <fabrizio.castro.jz@renesas.com>; linux-renesas-
> soc@vger.kernel.org; Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Subject: Re: [PATCH v4] dt-bindings: rtc: isl1208: Convert to json-schema
> 
> On Tue, May 9, 2023 at 6:12 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > +
> > +  interrupt-names:
> 
> Shouldn't this have minItems: 1 and maxItems: 2 as well?
> 
> > +    items:
> > +      - const: irq
> > +      - const: evident
> 
> 
> > +
> > +  isil,ev-evienb:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [ 0, 1 ]
> > +    description: |
> > +      Enable or disable internal pull on EVIN pin
> > +      Default will leave the non-volatile configuration of the pullup
> > +      as is.
> > +        <0> : Enables internal pull-up on evin pin
> > +        <1> : Disables internal pull-up on evin pin
> 
> It appears this was not clear at first.  The default is not to use the reset
> value, which is 0, but to leave the existing value unchanged.
> The RTC settings are battery-backed and the RTC is not reset on boot by the
> kernel.  The value might have been set on a previous boot, or might have
> already been configured by the bootloader or BIOS.  This was a common design
> in Linux RTC drivers.  The bootloader would configure the RTC and then Linux
> driver was only design to be able to get/set the time and alarms.  Stuff
> like programming the interrupt pull-ups or setting calibration registers
> wasn't done by the kernel.

Thanks for the detailed explanation. So currently are you ok with the description
right? or Do you want to change this with detailed explanation? We can always add
incremental patch to update this section later. This patch is just conversion
from txt to yaml and the content should be same as the original one as much as possible.

Please let me know.

> 
> > +    then:
> > +      properties:
> > +        interrupts:
> > +          maxItems: 2
> > +    else:
> > +      properties:
> > +        interrupts:
> > +          maxItems: 1
> 
> Add interrupt-names here too.

Based on the other mail thread, will update this.

> 
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        rtc_twi: rtc@6f {
> > +            compatible = "isil,isl1208";
> > +            reg = <0x6f>;
> > +        };
> > +    };
> 
> I agree with Geert's original comment about switching from the most complex
> to the simplest example.  It's better to show as much as possible.
> 
> If it's not possible to make a valid example that shows interrupts and evdet
> pull enable, then doesn't that mean the bindings don't work?

That is the normal practice right? When there is a board dts with complex case,
It is normal to add that case in example section.

I started with complex case and tested the bindings., later found that there is
no board dts with complex case. So switched to simple case.

Cheers,
Biju
Krzysztof Kozlowski May 10, 2023, 1:05 p.m. UTC | #6 
On 10/05/2023 13:37, Biju Das wrote:
> Hi Krzysztof Kozlowski,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Wednesday, May 10, 2023 7:58 AM
>> To: Geert Uytterhoeven <geert@linux-m68k.org>; Trent Piepho
>> <tpiepho@gmail.com>
>> Cc: Biju Das <biju.das.jz@bp.renesas.com>; Alessandro Zummo
>> <a.zummo@towertech.it>; Alexandre Belloni <alexandre.belloni@bootlin.com>;
>> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; linux-rtc@vger.kernel.org;
>> devicetree@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>;
>> Fabrizio Castro <fabrizio.castro.jz@renesas.com>; linux-renesas-
>> soc@vger.kernel.org
>> Subject: Re: [PATCH v4] dt-bindings: rtc: isl1208: Convert to json-schema
>>
>> On 10/05/2023 08:52, Geert Uytterhoeven wrote:
>>> Hi Trent,
>>>
>>> On Tue, May 9, 2023 at 9:03 PM Trent Piepho <tpiepho@gmail.com> wrote:
>>>> On Tue, May 9, 2023 at 6:12 AM Biju Das <biju.das.jz@bp.renesas.com>
>> wrote:
>>>>> +
>>>>> +  interrupt-names:
>>>>
>>>> Shouldn't this have minItems: 1 and maxItems: 2 as well?
>>>
>>>>> +    then:
>>>>> +      properties:
>>>>> +        interrupts:
>>>>> +          maxItems: 2
>>>>> +    else:
>>>>> +      properties:
>>>>> +        interrupts:
>>>>> +          maxItems: 1
>>>>
>>>> Add interrupt-names here too.
>>>
>>> Isn't the relation interrupts <=> interrupt-names enforced by the
>>> tooling?
>>
>> No, every constrain or schema code for one should be duplicated for second.
>> These can be done however in different ways, e.g.
>> interrupts:
>>   minItems: 1
>>   maxitems: 2
>> interrupt-names:
>>   minItems: 1
>>   items:
>>     - foo
>>     - bar
>>
>> but the outcome - so how many items are expected - must be the same in every
>> branch/condition.
> 
> But this will result in duplication of items in 2 places right?
> 
> One like the above and other one in conditional branch.


No. Constraints must be the same, so for example minItems.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/rtc/isil,isl1208.txt b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
deleted file mode 100644
index 51f003006f04..000000000000
--- a/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
+++ /dev/null
@@ -1,38 +0,0 @@ 
-Intersil ISL1209/19 I2C RTC/Alarm chip with event in
-
-ISL12X9 have additional pins EVIN and #EVDET for tamper detection, while the
-ISL1208 and ISL1218 do not.  They are all use the same driver with the bindings
-described here, with chip specific properties as noted.
-
-Required properties supported by the device:
- - "compatible": Should be one of the following:
-		- "isil,isl1208"
-		- "isil,isl1209"
-		- "isil,isl1218"
-		- "isil,isl1219"
- - "reg": I2C bus address of the device
-
-Optional properties:
- - "interrupt-names": list which may contains "irq" and "evdet"
-	evdet applies to isl1209 and isl1219 only
- - "interrupts": list of interrupts for "irq" and "evdet"
-	evdet applies to isl1209 and isl1219 only
- - "isil,ev-evienb": Enable or disable internal pull on EVIN pin
-	Applies to isl1209 and isl1219 only
-	Possible values are 0 and 1
-	Value 0 enables internal pull-up on evin pin, 1 disables it.
-	Default will leave the non-volatile configuration of the pullup
-	as is.
-
-Example isl1219 node with #IRQ pin connected to SoC gpio1 pin12 and #EVDET pin
-connected to SoC gpio2 pin 24 and internal pull-up enabled in EVIN pin.
-
-	isl1219: rtc@68 {
-		compatible = "isil,isl1219";
-		reg = <0x68>;
-		interrupt-names = "irq", "evdet";
-		interrupts-extended = <&gpio1 12 IRQ_TYPE_EDGE_FALLING>,
-			<&gpio2 24 IRQ_TYPE_EDGE_FALLING>;
-		isil,ev-evienb = <1>;
-	};
-
diff --git a/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml b/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
new file mode 100644
index 000000000000..d11c41f90a3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
@@ -0,0 +1,80 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/isil,isl1208.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intersil ISL1209/19 I2C RTC/Alarm chip with event in
+
+maintainers:
+  - Trent Piepho <tpiepho@gmail.com>
+
+description:
+  ISL12X9 have additional pins EVIN and EVDET for tamper detection, while the
+  ISL1208 and ISL1218 do not.
+
+properties:
+  compatible:
+    enum:
+      - isil,isl1208
+      - isil,isl1209
+      - isil,isl1218
+      - isil,isl1219
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+
+  interrupt-names:
+    items:
+      - const: irq
+      - const: evdet
+
+  isil,ev-evienb:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    description: |
+      Enable or disable internal pull on EVIN pin
+      Default will leave the non-volatile configuration of the pullup
+      as is.
+        <0> : Enables internal pull-up on evin pin
+        <1> : Disables internal pull-up on evin pin
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: rtc.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - isil,isl1209
+              - isil,isl1219
+    then:
+      properties:
+        interrupts:
+          maxItems: 2
+    else:
+      properties:
+        interrupts:
+          maxItems: 1
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        rtc_twi: rtc@6f {
+            compatible = "isil,isl1208";
+            reg = <0x6f>;
+        };
+    };