Re: [PATCH v2 1/2] dt-bindings: pwm: add atcpit100-pwm

[Krzysztof Kozlowski <krzk@kernel.org>]

On 03/12/2024 06:41, Ben Zong-You Xie wrote:
> On Mon, Dec 02, 2024 at 08:40:22AM +0100, Krzysztof Kozlowski wrote:
>> [EXTERNAL MAIL]
>>
>> On Mon, Dec 02, 2024 at 02:01:46PM +0800, Ben Zong-You Xie wrote:
>>> Document devicetree bindings for Andes atcpit100-pwm.
>>>
>>> Signed-off-by: Ben Zong-You Xie <ben717@andestech.com>
>>> ---
>>>  .../bindings/pwm/andestech,atcpit100-pwm.yaml | 51 +++++++++++++++++++
>>>  MAINTAINERS                                   |  5 ++
>>>  2 files changed, 56 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml b/Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml
>>> new file mode 100644
>>> index 000000000000..4b707f32ad72
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml
>>> @@ -0,0 +1,51 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/pwm/andestech,atcpit100-pwm.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Andes atcpit100 PWM
>>> +
>>> +maintainers:
>>> +  - Ben Zong-You Xie <ben717@andestech.com>
>>> +
>>> +allOf:
>>> +  - $ref: pwm.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: andestech,atcpit100-pwm
>>
>>
>> Previously, before we removed it in 2022, this was just
>> andestech,atcpit100, so questions:
>>
>> 1. Why are you re-introducing it? Please address all the comments or
>> aspects leading to removal.
>> 2. Why are you using different compatible? Is this one device?
>>
>> Best regards,
>> Krzysztof
>>
> 
> Hi Krzysztof,
> 
> 1. You can first refer to the patch[1].
>    The patch not only removes the support to nds32, but also removes
>    Andes device driver. Though Andes now dedicates our effort on RISC-V,
>    ATCPIT100 is still one of peripheral platform IPs, and that's why we are
>    re-introducing it now.


This should be explained in cover letter and commit msg.

> 
> 2. Yes, they are the same device. ATCPIT100 is a set of compact
>    multi-function timers, which can be used as PWMs or simple timers.
>    I think the example in the YAML file is a little confusing because
>    there are two ATCPIT100 nodes in our DTS file now:

You did not explain it, either...

> 
> 	pit: timer@f0400000 {
> 		compatible = "andestech,atcpit100";
> 		...
> 		...
> 	};
> 	pwm: pwm@f0400000 {
> 		compatible = "andestech,atcpit100-pwm";
> 		...
> 		...
> 	};
> 
>    Is it better to modify our DTS file and the example in the YAML file
>    like below?
> 	
> 	pit: pit@f0400000 {
> 		compatible = "andestech,atcpit100";
> 		reg = <0xf0400000, 0x1000>;
> 		clocks = <&smu 1>, <&smu 7>;
> 		clock-names = "ext", "apb";

One node above.

> 		pwm: pwm {
> 			compatible = "andestech,atcpit100-pwm";

This is not needed.

> 			#pwm-cells = <3>;

And this goes to parent node.



Best regards,
Krzysztof