[09/15] dt-bindings: reset: Document ma35d1 reset controller bindings

  From: Jacky Huang <ychuang3@nuvoton.com>

Add documentation to describe nuvoton ma35d1 reset driver bindings.

Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
---
 .../bindings/reset/nuvoton,ma35d1-reset.yaml  | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
  

On 15/03/2023 08:28, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@nuvoton.com>
> 
> Add documentation to describe nuvoton ma35d1 reset driver bindings.

Subject: drop second/last, redundant "bindings". The "dt-bindings"
prefix is already stating that these are bindings.

> 
> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
> ---
>  .../bindings/reset/nuvoton,ma35d1-reset.yaml  | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
> new file mode 100644
> index 000000000000..f66c566c6dce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/nuvoton,ma35d1-reset.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton MA35D1 Reset Controller
> +
> +maintainers:
> +  - Chi-Fang Li <cfli0@nuvoton.com>
> +  - Jacky Huang <ychuang3@nuvoton.com>
> +
> +description:
> +  The system reset controller can be used to reset various peripheral
> +  controllers in MA35D1 SoC.
> +
> +properties:
> +  compatible:
> +    const: nuvoton,ma35d1-reset
> +
> +  regmap:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Phandle to the register map node.

You need to be specific what is this. As you can easily check, there is
no such property in any devices. I don't understand why do you need it
in the first place.

> +
> +  '#reset-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - regmap
> +  - '#reset-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  # system reset controller node:
> +  - |
> +    #include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
> +
> +    sys: system-management@40460000 {
> +        compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";

And your patchset is not bisectable.... Test for bisectability before
sending.


Best regards,
Krzysztof
  

On 16/03/2023 08:37, Krzysztof Kozlowski wrote:
> On 15/03/2023 08:28, Jacky Huang wrote:
>> From: Jacky Huang <ychuang3@nuvoton.com>
>>
>> Add documentation to describe nuvoton ma35d1 reset driver bindings.
> 
> Subject: drop second/last, redundant "bindings". The "dt-bindings"
> prefix is already stating that these are bindings.
> 
>>
>> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
>> ---
>>  .../bindings/reset/nuvoton,ma35d1-reset.yaml  | 50 +++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>> new file mode 100644
>> index 000000000000..f66c566c6dce
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>> @@ -0,0 +1,50 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/reset/nuvoton,ma35d1-reset.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Nuvoton MA35D1 Reset Controller
>> +
>> +maintainers:
>> +  - Chi-Fang Li <cfli0@nuvoton.com>
>> +  - Jacky Huang <ychuang3@nuvoton.com>
>> +
>> +description:
>> +  The system reset controller can be used to reset various peripheral
>> +  controllers in MA35D1 SoC.
>> +
>> +properties:
>> +  compatible:
>> +    const: nuvoton,ma35d1-reset
>> +
>> +  regmap:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: Phandle to the register map node.
> 
> You need to be specific what is this. As you can easily check, there is
> no such property in any devices. I don't understand why do you need it
> in the first place.
> 
>> +
>> +  '#reset-cells':
>> +    const: 1
>> +
>> +required:
>> +  - compatible
>> +  - regmap
>> +  - '#reset-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  # system reset controller node:
>> +  - |
>> +    #include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
>> +
>> +    sys: system-management@40460000 {
>> +        compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
> 
> And your patchset is not bisectable.... Test for bisectability before
> sending.

Ah, no, it's correct. I see the compatible in previous patch. You need
to clearly describe the dependencies and merging strategy/requirements
in cover letter.

Best regards,
Krzysztof
  

Dear Krzysztof,


Thanks for your advice.


On 2023/3/16 下午 03:39, Krzysztof Kozlowski wrote:
> On 16/03/2023 08:37, Krzysztof Kozlowski wrote:
>> On 15/03/2023 08:28, Jacky Huang wrote:
>>> From: Jacky Huang <ychuang3@nuvoton.com>
>>>
>>> Add documentation to describe nuvoton ma35d1 reset driver bindings.
>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>> prefix is already stating that these are bindings.


OK, I will fix it.


>>> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
>>> ---
>>>   .../bindings/reset/nuvoton,ma35d1-reset.yaml  | 50 +++++++++++++++++++
>>>   1 file changed, 50 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>>> new file mode 100644
>>> index 000000000000..f66c566c6dce
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>>> @@ -0,0 +1,50 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/reset/nuvoton,ma35d1-reset.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Nuvoton MA35D1 Reset Controller
>>> +
>>> +maintainers:
>>> +  - Chi-Fang Li <cfli0@nuvoton.com>
>>> +  - Jacky Huang <ychuang3@nuvoton.com>
>>> +
>>> +description:
>>> +  The system reset controller can be used to reset various peripheral
>>> +  controllers in MA35D1 SoC.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: nuvoton,ma35d1-reset
>>> +
>>> +  regmap:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: Phandle to the register map node.
>> You need to be specific what is this. As you can easily check, there is
>> no such property in any devices. I don't understand why do you need it
>> in the first place.

         reset: reset-controller {
             compatible = "nuvoton,ma35d1-reset";
             regmap = <&sys>;
             #reset-cells = <1>;
         };

The dt_binding_check check report an error about the above "regmap".

I found that add this can pass the test.



>>> +
>>> +  '#reset-cells':
>>> +    const: 1
>>> +
>>> +required:
>>> +  - compatible
>>> +  - regmap
>>> +  - '#reset-cells'
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  # system reset controller node:
>>> +  - |
>>> +    #include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
>>> +
>>> +    sys: system-management@40460000 {
>>> +        compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
>> And your patchset is not bisectable.... Test for bisectability before
>> sending.
> Ah, no, it's correct. I see the compatible in previous patch. You need
> to clearly describe the dependencies and merging strategy/requirements
> in cover letter.
>
> Best regards,
> Krzysztof

Sorry for the confusion.

I will add history to the cover letter.


Best regards,

Jacky Huang
  

On 18/03/2023 05:30, Jacky Huang wrote:
> Dear Krzysztof,
> 
> 
> Thanks for your advice.
> 
> 
> On 2023/3/16 下午 03:39, Krzysztof Kozlowski wrote:
>> On 16/03/2023 08:37, Krzysztof Kozlowski wrote:
>>> On 15/03/2023 08:28, Jacky Huang wrote:
>>>> From: Jacky Huang <ychuang3@nuvoton.com>
>>>>
>>>> Add documentation to describe nuvoton ma35d1 reset driver bindings.
>>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>>> prefix is already stating that these are bindings.
> 
> 
> OK, I will fix it.
> 
> 
>>>> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
>>>> ---
>>>>   .../bindings/reset/nuvoton,ma35d1-reset.yaml  | 50 +++++++++++++++++++
>>>>   1 file changed, 50 insertions(+)
>>>>   create mode 100644 Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>>>> new file mode 100644
>>>> index 000000000000..f66c566c6dce
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>>>> @@ -0,0 +1,50 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/reset/nuvoton,ma35d1-reset.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Nuvoton MA35D1 Reset Controller
>>>> +
>>>> +maintainers:
>>>> +  - Chi-Fang Li <cfli0@nuvoton.com>
>>>> +  - Jacky Huang <ychuang3@nuvoton.com>
>>>> +
>>>> +description:
>>>> +  The system reset controller can be used to reset various peripheral
>>>> +  controllers in MA35D1 SoC.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: nuvoton,ma35d1-reset
>>>> +
>>>> +  regmap:
>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>> +    description: Phandle to the register map node.
>>> You need to be specific what is this. As you can easily check, there is
>>> no such property in any devices. I don't understand why do you need it
>>> in the first place.
> 
>          reset: reset-controller {
>              compatible = "nuvoton,ma35d1-reset";
>              regmap = <&sys>;
>              #reset-cells = <1>;
>          };
> 
> The dt_binding_check check report an error about the above "regmap".
> 
> I found that add this can pass the test.

Do not add properties to bindings to "pass the test". That's not the
goal of bindings. Add there properties because they make sense...

Anyway, you did not answer my question at all. So one by one - address them:
1. As you can easily check, there is no such property in any devices.
Explanation: do you see it anywhere in existing bindings?

2. I don't understand why do you need it in the first place.
Explanation: your binding suggest this is not needed. If you think
otherwise, you need to provide rationale.



Best regards,
Krzysztof
  

On 2023/3/19 下午 07:05, Krzysztof Kozlowski wrote:
> On 18/03/2023 05:30, Jacky Huang wrote:
>> Dear Krzysztof,
>>
>>
>> Thanks for your advice.
>>
>>
>> On 2023/3/16 下午 03:39, Krzysztof Kozlowski wrote:
>>> On 16/03/2023 08:37, Krzysztof Kozlowski wrote:
>>>> On 15/03/2023 08:28, Jacky Huang wrote:
>>>>> From: Jacky Huang <ychuang3@nuvoton.com>
>>>>>
>>>>> Add documentation to describe nuvoton ma35d1 reset driver bindings.
>>>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>>>> prefix is already stating that these are bindings.
>>
>> OK, I will fix it.
>>
>>
>>>>> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
>>>>> ---
>>>>>    .../bindings/reset/nuvoton,ma35d1-reset.yaml  | 50 +++++++++++++++++++
>>>>>    1 file changed, 50 insertions(+)
>>>>>    create mode 100644 Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..f66c566c6dce
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>>>>> @@ -0,0 +1,50 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/reset/nuvoton,ma35d1-reset.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Nuvoton MA35D1 Reset Controller
>>>>> +
>>>>> +maintainers:
>>>>> +  - Chi-Fang Li <cfli0@nuvoton.com>
>>>>> +  - Jacky Huang <ychuang3@nuvoton.com>
>>>>> +
>>>>> +description:
>>>>> +  The system reset controller can be used to reset various peripheral
>>>>> +  controllers in MA35D1 SoC.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: nuvoton,ma35d1-reset
>>>>> +
>>>>> +  regmap:
>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>>> +    description: Phandle to the register map node.
>>>> You need to be specific what is this. As you can easily check, there is
>>>> no such property in any devices. I don't understand why do you need it
>>>> in the first place.
>>           reset: reset-controller {
>>               compatible = "nuvoton,ma35d1-reset";
>>               regmap = <&sys>;
>>               #reset-cells = <1>;
>>           };
>>
>> The dt_binding_check check report an error about the above "regmap".
>>
>> I found that add this can pass the test.
> Do not add properties to bindings to "pass the test". That's not the
> goal of bindings. Add there properties because they make sense...
>
> Anyway, you did not answer my question at all. So one by one - address them:
> 1. As you can easily check, there is no such property in any devices.
> Explanation: do you see it anywhere in existing bindings?

Yes, I cannot find it in all bindings. I know it's wrong.

> 2. I don't understand why do you need it in the first place.
> Explanation: your binding suggest this is not needed. If you think
> otherwise, you need to provide rationale.
>
>
>
> Best regards,
> Krzysztof
>

Now we have removed regmap and modify the dtsi as:

     sys: system-management@40460000 {
         compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
         reg = <0x0 0x40460000 0x0 0x200>;

         reset: reset-controller {
             compatible = "nuvoton,ma35d1-reset";
             #reset-cells = <1>;
         };
     };

In the reset driver, we obtain the regmap by parent node:
     parent = of_get_parent(dev->of_node); /* parent should be syscon 
node */
     reset_data->regmap = syscon_node_to_regmap(parent);
     of_node_put(parent);

We have it tested OK on ma35d1 SOM board.
And it pass the dt_binding_check and dtbs_check.

Best regards,
Jacky Huang