Re: Re: [PATCH v6 1/2] dt-bindings: pwm: dwc: add optional reset

[Xuyang Dong]

> >>>  
> >>> +allOf:
> >>> +  - $ref: pwm.yaml#
> >>> +
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: eswin,eic7700-pwm
> >>
> >> Same problem as v3 which I commented. I do not understand why your new
> >> device has also 1 reset.
> >>
> >> Your commit msg MUST explain why 1 reset is valid.
> >>
> > 
> > Hi Krzysztof,
> > 
> > Although the PWM IP supports two clock domains, each requiring a reset, 
> > the EIC7700 implementation uses the same clock domain for both clock 
> > signals. Therefore, the eic7700-pwm only supports one reset.
> >
> 
> If we speak about eic7700, explain why it has two resets now, according
> to schema, even though you say it has not.
> 
> But I was speaking about dw-apb-timers-pwm, which has one reset as well!
> Why you are not having proper constraints? Please read writing bindings
> document.
> 

Hi Krzysztof,

Let me clarify the reset signals.
  - snps,dw-apb-timers-pwm2: IP spec has 2 optional reset signals (one per
clock domain), SoC vendor decides whether to wire them — so maxItems: 2, 
optional in required.
  - eswin,eic7700-pwm: SoC physically ties both signals to one reset — so
exactly 1, required.

So I think the correct patch should be like this:
Modify the commit message as follows:

The DesignWare PWM IP has up to two optional reset signals, one per clock
domain (presetn for pclk, timer_N_resetn for timer_N_clk).
SoC vendors decides to wire them, so maxItems: 2 is the upper bound for 
the generic snps,dw-apb-timers-pwm2 compatible.

The ESWIN EIC7700 ties both reset domains to a single physical reset 
signal, so exactly one reset is required. Add maxItems: 1 to the if-then 
block for eswin,eic7700-pwm to enforce this.

Modify the YAML as follows:

-allOf:
-  - $ref: pwm.yaml#
-
 properties:
   compatible:
-    const: snps,dw-apb-timers-pwm2
+    enum:
+      - snps,dw-apb-timers-pwm2
+      - eswin,eic7700-pwm

   reg:
     maxItems: 1
@@ -43,6 +42,13 @@ properties:
       - const: bus
       - const: timer

+  resets:
+    minItems: 1
+    maxItems: 2
+
   snps,pwm-number:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: The number of PWM channels configured for this instance
@@ -54,6 +60,21 @@ required:
   - clocks
   - clock-names

+allOf:
+  - $ref: pwm.yaml#
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: eswin,eic7700-pwm
+    then:
+      properties:
+        resets:
+          maxItems: 1
+      required:
+        - resets
+
 additionalProperties: false

Do you think this modification is more appropriate?

Best regards,
Xuyang Dong