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.
> >>
> >> Two reset signals but what is exactly optional? Each of them? Only the
> >> first? Binding does not allow the first to be optional.
> >>
> > 
> > Hi Krzysztof,
> > 
> > Thank you for the review. 
> > 
> > For the generic snps,dw-apb-timers-pwm2 binding, both reset signals 
> > are now fully optional by not including resets in the required list.
> > 
> > When a single optional reset signal is used, the interface bus reset 
> > (index 0) is used by default.
> > 
> > Keep the YAML as follows:
> > +  resets:
> > +    minItems: 1
> > +    items:
> > +      - description: Interface bus reset
> > +      - description: PWM timer logic reset
> > 
> > Add the following description to the commit message:
> 
> We speak about hardware, not binding. I asked, why your new device has
> only one reset.
> 

Hi Krzysztof,

Thank you for the detailed review.

I don't quite understand the meaning of your sentence: "We speak about 
hardware, not binding. I asked, why your new device has only one reset."
If you mean that the commit message in dt-bindings does not accurately 
describe why the EIC7700 has only one reset, I have pasted below the 
complete commit message that will be included in the next v7 version.

Does this commit message address your question?

The DesignWare PWM includes separate reset signals dedicated to each clock
domain:
The presetn signal resets logic in pclk domain.
The timer_N_resetn signal resets logic in the timer_N_clk domain.
The resets are active-low.

EIC7700 uses DesignWare IP for PWM controllers. Add ESWIN EIC7700 support
in snps,dw-apb-timers-pwm2.yaml.
EIC7700 physically ties presetn signal and timer_N_resetn signal to one reset 
— so exactly 1, required.

> > 
> > Whether each signal is wired on a given SoC is a board integration 
> > decision, so the resets property is optional for snps,dw-apb-timers-pwm2. 
> > When present, up to two handles may be supplied: the bus reset is always 
> > at index 0 and the timer reset at index 1.
> > 
> >>>   - eswin,eic7700-pwm: SoC physically ties both signals to one reset — so
> >>> exactly 1, required.
> >>
> >> Then two would not be right and you need to restrict that.
> >>
> > 
> > For the specific eswin,eic7700-pwm binding, the reset signal is required 
> > and fixed to one via conditional schema (if:then:), with maxItems: 1 
> > and resets added to required. And add an example for eswin,eic7700-pwm.
> > The changes are as follows:
> > 
> > +allOf:
> > +  - $ref: pwm.yaml#
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: eswin,eic7700-pwm
> > +    then:
> > +      properties:
> > +        resets:
> > +          maxItems: 1
> > +      required:
> > +        - resets
> > +
> > 
> > +  - |
> > +    pwm@50818000 {
> > +      compatible = "eswin,eic7700-pwm";
> > +      reg = <0x50818000 0x4000>;
> > +      #pwm-cells = <3>;
> > +      clocks = <&bus>, <&timer>;
> > +      clock-names = "bus", "timer";
> > +      resets = <&reset>;
> > +    };
> > 
> > Then change the binding's subject from "dt-bindings: pwm: dwc: add optional 
> > reset" to "dt-bindings: pwm: dwc: add eswin,eic7700-pwm compatible and resets".
> > 
> > Do these changes look acceptable to you?
> 
> So two resets or one reset? I am completely confused what you are
> replying to.
> 
> Please read writing bindings document.
> 

I think the "resets" in the subject is ambiguous. It could mislead people
into thinking that the EIC7700 has multiple reset signals.
I think the subject should be changed to 
"dt-bindings: pwm: dwc: Add eswin compatible and resets property".

Best regards,
Xuyang Dong