Re: [PATCH v3 1/3] dt-bindings: hwmon: pmbus: add Delta E50SN12051 binding

Re: [PATCH v3 1/3] dt-bindings: hwmon: pmbus: add Delta E50SN12051 binding

[Guenter Roeck]

On 4/29/26 23:59, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Low] The binding description violates Devicetree principles by describing software instead of hardware. It explicitly states 'The driver supports...', which is an OS-specific software description.
> - [Low] The Devicetree example uses the Linux-specific subsystem name 'hwmon@40' for the node name instead of a generic functional name.
> --
> 
> commit 8f686eaa80cf8986d09fe9ea7dca7debf1c51cb1
> Author: Colin Huang <u8813345@gmail.com>
> 
> dt-bindings: hwmon: pmbus: add Delta E50SN12051 binding
> 
> This commit introduces the devicetree binding schema for the Delta E50SN12051
> PMBus-compliant sensor.
> 
>> diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/delta,e50sn12051.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/delta,e50sn12051.yaml
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/delta,e50sn12051.yaml
> [ ... ]
>> +description: |
>> +  This binding describes the Delta E50SN12051 PMBus sensor.
>> +  The driver supports reading voltage, current, and temperature
>> +  using the PMBus interface.
> 
> This isn't a bug, but should the description focus purely on the hardware
> rather than mentioning what the driver supports? Devicetree bindings are
> generally expected to be OS-agnostic hardware descriptions.

I am not a devicetree maintainer, but I agree.

Guenter

> 
> [ ... ]
>> +examples:
>> +  - |
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        hwmon@40 {
> 
> Is hwmon@40 the most appropriate node name here? The devicetree specification
> usually recommends using generic names that reflect the function of the
> hardware, such as sensor@40, rather than OS-specific subsystem names like
> hwmon.
> 
>> +            compatible = "delta,e50sn12051";
>> +            reg = <0x40>;
>> +        };
>> +    };
>