Re: [PATCH 8/9] dt-bindings: Add documentation for Photonicat PMU

[Krzysztof Kozlowski <krzk@kernel.org>]

On 07/09/2024 16:27, Junhao Xie wrote:

>>> +
> [...]
>>> +  local-address:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 1
>>> +    maximum: 127
>>> +    default: 1
>>> +    description: CPU board address
>>
>> Address of what? In which notation? It's part of this hardware.
>>
> 
> Photonicat's MCU protocol documentation says it supports multiple hosts.
> But Photonicat only uses one.
> Is it necessary to remove local-address and use a fixed address?

I don't understand what this "address" is for.

> 
>>
>>> +
>>> +  remote-address:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 1
>>> +    maximum: 127
>>> +    default: 1
>>> +    description: PMU board address
>>
>> Eee, no. Your board knows its address. You do not have to tell it.
> 
> I will remove remote-address.
> 
>>
> [...]
>>> +
>>> +patternProperties:
>>> +  '^leds-(status)':
>>
>> That's not a pattern.
>>
> 
> I originally wanted to keep it for extensions, but it didn't seem like a good idea.
> I will move it to properties.
> 
>>> +    $ref: /schemas/leds/ariaboard,photonicat-pmu-leds.yaml
>>> +
>>> +  '^supply-(battery|charger)$':
>>> +    $ref: /schemas/power/supply/ariaboard,photonicat-pmu-supply.yaml
>>
>> Why two nodes?
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +      serial {
>>> +          photonicat-pmu {
>>> +              compatible = "ariaboard,photonicat-pmu";
>>> +              current-speed = <115200>;
>>> +              local-address = <1>;
>>> +              remote-address = <1>;
>>> +
>>> +              supply-battery {
>>> +                  compatible = "ariaboard,photonicat-pmu-supply";
>>> +                  label = "battery";
>>
>> Nope, drop label.
>>
>>> +                  type = "battery";
>>
>> No, there is no type property.
> 
> There are two supplies here, one is the charger and the other is the battery.
> Is it necessary to change to use different compatible ones like
> ariaboard,photonicat-pmu-battery and ariaboard,photonicat-pmu-charger?

Are the devices different? Why do you even need the type?

> 
>>
>> Missing monitored battery.
>>
> 
> I will add it.
> 
>>> +              };
>>> +
> [...]
>>> +
>>> +              watchdog {
>>> +                  compatible = "ariaboard,photonicat-pmu-watchdog";
>>> +              };
>>
>> These are seriously redundant and useless nodes.  There is nothing
>> beneficial from the nodes above - they are all empty, without resources.
>> Drop all of them.
> 
> How should I describe these devices? Using mfd_cell?

You mean drivers? MFD or auxiliary bus.

Best regards,
Krzysztof