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
