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

[Xuyang Dong]

> > > > > 
> > > > > 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.
> > > > > 
> > > > > Signed-off-by: Xuyang Dong <dongxuyang@xxxxxxxxxxxxxxxxxx>
> > > > 
> > > > This commit implies that your hardware differs from existing devices,
> > > > I think you should add a device-specific compatible.
> > > > 
> > 
> > Hi Conor and Krzysztof,
> > 
> > The DesignWare PWM Databook for 2.13a says: "The DW_apb_timers includes 
> > separate reset signals dedicated to each clock domain". They are:
> > The presetn signal resets logic in pclk domain (i.e., the bus clock in DT).
> > The timer_N_resetn signal resets logic in the timer_N_clk domain (i.e.,
> > the timer clock in DT).
> > 
> > These reset signals are optional; it is up to the designer's 
> > implementation.
> 
> Right, and it's that "designer's implementation" that warrants a
> device-specific compatible.
> 

Hi Conor,

The YAML update for the new device-specific compatible is as follows:

properties:
  compatible:
    oneOf:
      - const: snps,dw-apb-timers-pwm2
      - items:
          - enum:
              - snps,dw-apb-timers-pwm-2.13a
          - const: snps,dw-apb-timers-pwm2

  reg:
    maxItems: 1

  "#pwm-cells":
    const: 3

  clocks:
    items:
      - description: Interface bus clock
      - description: PWM reference clock

  clock-names:
    items:
      - const: bus
      - const: timer

  resets:
    items:
      - description: Interface bus reset
      - description: PWM timer logic reset

  snps,pwm-number:
    $ref: /schemas/types.yaml#/definitions/uint32
    description: The number of PWM channels configured for this instance
    enum: [1, 2, 3, 4, 5, 6, 7, 8]

required:
  - compatible
  - reg
  - clocks
  - clock-names

allOf:
  - $ref: pwm.yaml#

  - if:
      properties:
        compatible:
          contains:
            const: snps,dw-apb-timers-pwm-2.13a

    then:
      required:
        - resets

In your opinion, is this modification accurate?

Best regards,
Xuyang Dong

> > 
> > According to [1], the applied YAML is also based on 2.13a, so our 
> > hardware is the same as the existing devices. It's just that these two 
> > reset signals were missing from the original YAML binding.
> > 
> > [1] https://lore.kernel.org/linux-pwm/8bb5103d-803e-90d2-fd93-132bb2aac2d6@xxxxxxxxxx/
> > 
> > > > > ---
> > > > >  .../devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml       | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
> > > > > index 7523a89a1773..a8bbad0360f8 100644
> > > > > --- a/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
> > > > > +++ b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
> > > > > @@ -43,6 +43,9 @@ properties:
> > > > >        - const: bus
> > > > >        - const: timer
> > > > >  
> > > > > +  resets:
> > > > > +    maxItems: 2
> > > 
> > > And this should really be listed with description, because order is
> > > fixed.
> > > 
> > 
> > The description of resets will be listed in next version.
> > 
> > Best regards,
> > Xuyang Dong