Message-ID: <cd0b36f4-f31d-4a65-868c-72b3c7021f14@classfun.cn>
Date: Sat, 7 Sep 2024 22:27:26 +0800
From: Junhao Xie <bigfoot@...ssfun.cn>
To: Krzysztof Kozlowski <krzk@...nel.org>
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>, Junhao Xie <bigfoot@...ssfun.cn>
Subject: Re: [PATCH 8/9] dt-bindings: Add documentation for Photonicat PMU

On 2024/9/6 17:51, Krzysztof Kozlowski wrote:
> On 06/09/2024 11:36, Junhao Xie wrote:
>> Add device tree binding documentation for Photonicat PMU MFD, LEDs,
>> hardware monitor, power off, power supply, real-time clock watchdog.
[...]
>> diff --git a/Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml
[...]>> +  label:
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    description: Label for hwmon device
> 
> No resources here. Fold it into parent binding.
> 
>> +
>> +required:
>> +  - compatible
>> +  - label
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +      serial {
>> +          mcu {
>> +              compatible = "ariaboard,photonicat-pmu";
> 
> Drop, no need.
> 
>> +
> 
> Messed indentation.
> 
> Entire example is redundant. Merge it to parent binding.
> 

I will fix them.

> 
[...]
>> diff --git a/Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml b/Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml
[...]
>> +properties:
>> +  compatible:
>> +    const: ariaboard,photonicat-pmu-leds
> 
> Your compatibles per device do not make much sense. You organized
> bindings per drivers, but that's not what we want.
> 
>> +
>> +  label: true
> 
> Drop

I will drop it.

> 
[...]
>> diff --git a/Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml b/Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml
[...]
>> +description:
>> +  Driver for the Power Management MCU in the Ariaboard Photonicat,
> 
> Bindings are for hardware, not drivers. Drop it everywhere and explain
> hardware.
> 

I will edit it.
Maybe the following looks better?
  The Power Management MCU in Ariaboard Photonicat, which
  provides battery and charger power supply real-time clock,
  watchdog, hardware shutdown, status LEDs.

>> +  which provides battery and charger power supply, real-time clock,
>> +  watchdog, hardware shutdown.
>> +
>> +properties:
>> +  compatible:
>> +    const: ariaboard,photonicat-pmu
> 
> That's the only compatible you should have. Drop all others.
> 
>> +
[...]
>> +  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?

> 
>> +
>> +  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?

> 
> 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?

> 
> I finish the review here.
> 
> Best regards,
> Krzysztof
> 

Thanks for your review, I will fix all problems in next version!

Best regards,
Junhao

Powered by blists - more mailing lists