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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 11/05/2026 09:10, Xuyang Dong wrote:
>>
>> On 29/04/2026 11:30, Xuyang Dong wrote:
>>>>>>>  
>>>>>>> +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.

> 
> 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.

Best regards,
Krzysztof