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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <77d41268-30e3-48a5-b611-40f586586ffb@kernel.org>
Date: Sun, 8 Sep 2024 10:13:53 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Junhao Xie <bigfoot@...ssfun.cn>
Cc: devicetree@...r.kernel.org, linux-hwmon@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
 linux-pm@...r.kernel.org, linux-rtc@...r.kernel.org,
 linux-watchdog@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, Jean Delvare <jdelvare@...e.com>,
 Guenter Roeck <linux@...ck-us.net>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>,
 "\"Conor Dooley,\"," <conor+dt@...nel.org>, Pavel Machek <pavel@....cz>,
 Lee Jones <lee@...nel.org>, Sebastian Reichel <sre@...nel.org>,
 Alexandre Belloni <alexandre.belloni@...tlin.com>,
 Wim Van Sebroeck <wim@...ux-watchdog.org>, Heiko Stuebner <heiko@...ech.de>,
 Chukun Pan <amadeus@....edu.cn>
Subject: Re: [PATCH 8/9] dt-bindings: Add documentation for Photonicat PMU

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