Re: Re: [PATCH v7 1/2] dt-bindings: pwm: dwc: Add eswin compatible and resets property

[Xuyang Dong]

> > 
> > EIC7700 use DesignWare IP for PWM controllers. Add ESWIN EIC7700 support
> > in snps,dw-apb-timers-pwm2.yaml.
> > 
> > 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.
> > 
> > The generic snps,dw-apb-timers-pwm2 binding allows one or two optional
> 
> I don't know what is the generic binding, but it does not allow. Open
> the file: there are no resets at all, so it does not allow them. Or you
> mixed tenses here and you wanted to describe the change?
> 

Hi Krzysztof,

Thanks for your comments and time.

Regarding snps,dw-apb-timers-pwm2, we previously intended to add the 
resets property in the same patch. 
However, as you suggested, we will split it into a separate patch.

> The present tense describes current state of source code before applying
> the patch. The patch transform that current state, so you don't use
> present tense to show what will be future.
> 
> Unless you meant here a device, not binding. I would be picky here
> except that your binding is incorrect which made me looking for answers.
> I cannot find these answers.
> 
> > reset lines depending on SoC integration.
> > 
> > On EIC7700, the presetn and timer_N_resetn inputs are physically tied
> > to a single reset line, therefore exactly one reset is required.
> > 
> > Signed-off-by: Xuyang Dong <dongxuyang@xxxxxxxxxxxxxxxxxx>
> > ---
> >  .../bindings/pwm/snps,dw-apb-timers-pwm2.yaml | 38 +++++++++++++++++--
> >  1 file changed, 34 insertions(+), 4 deletions(-)
> > 
> > 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..a4b7929f2e05 100644
> > --- a/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
> > +++ b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
> > @@ -20,12 +20,11 @@ description:
> >    instead of having to encode the IP version number in the device tree
> >    compatible.
> >  
> > -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,12 @@ properties:
> >        - const: bus
> >        - const: timer
> >  
> > +  resets:
> > +    minItems: 1
> > +    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
> > @@ -54,6 +59,22 @@ required:
> >    - clocks
> >    - clock-names
> >  
> > +allOf:
> > +  - $ref: pwm.yaml#
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: eswin,eic7700-pwm
> > +    then:
> > +      properties:
> > +        resets:
> > +          minItems: 1
> 
> Drop
> 

Will fix in the next version.

> > +          maxItems: 1
> 
> So you want to add resets for the existing variant claiming that they
> were missing? Probably we should tell you that earlier, although the
> patch was going odd directions, but you need to split it. First you fix
> existing devices with explanation why. Then you add new compatible with
> constraint for one reset.
> 

Based on your comments, we will split this patch into two. One patch 
explains why we add the resets property, and the other adds the eswin 
compatible string and the limit for resets.

Best regards,
Xuyang Dong