Re: [PATCH v8 2/5] dt-bindings: soc: sophgo: Add Sophgo system control module

[Krzysztof Kozlowski]

On 16/01/2024 12:37, Chen Wang wrote:
> 
> On 2024/1/16 18:06, Krzysztof Kozlowski wrote:
>> On 16/01/2024 08:21, Chen Wang wrote:
>>> From: Chen Wang <unicorn_wang@xxxxxxxxxxx>
>>>
>>> Add documentation to describe Sophgo System Control for SG2042.
>>>
>>> Signed-off-by: Chen Wang <unicorn_wang@xxxxxxxxxxx>
>>> ---
>>>   .../soc/sophgo/sophgo,sg2042-sysctrl.yaml     | 46 +++++++++++++++++++
>>>   1 file changed, 46 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>> new file mode 100644
>>> index 000000000000..7b50bb56b4cf
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>> @@ -0,0 +1,46 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/soc/sophgo/sophgo,sg2042-sysctrl.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Sophgo SG2042 SoC system control
>>> +
>>> +maintainers:
>>> +  - Chen Wang <unicorn_wang@xxxxxxxxxxx>
>>> +
>>> +description:
>>> +  The Sophgo system control is a registers block (SYS_CTRL), providing multiple
>>> +  low level platform functions like chip configuration, clock control, etc.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: sophgo,sg2042-sysctrl
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clock-controller:
>>> +    # Child node
>> Drop the comment, it is obvious. It cannot be anything else.
>>
>>> +    $ref: /schemas/clock/sophgo,sg2042-sysclk.yaml#
>>> +    type: object
>> Why isn't this merged here? You do not need the child node really...
>> unless the clock inputs are specific to that clock controller and you
>> will have here more devices? But where are they in such case?
> I don't see more devices will be included later. It should be ok to 
> merge them into one.
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - clock-controller
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    system-control@30010000 {
>> Why did you change the name? Please provide detailed changelog with
>> explanation of such changes.
> 
> I changed the name due to I find the 
> TRM(https://github.com/sophgo/sophgo-doc/blob/main/SG2042/TRM/source/system-control.rst) 
> call it "system control",  so I changed it in v8.
> 
> Which one do you prefer? I'm not sure if there are any requirements for 
> this?

Node names should be generic and follow common guidelines, not match
your TRM. Please use the same name all other devices use for the same class.

Best regards,
Krzysztof