Re: [PATCH 1/2] dt-bindings: hwmon: pmbus: adp1050 : add bindings

[Guenter Roeck]

On 3/18/24 04:21, Radu Sabau wrote:
> Add dt-bindings for adp1050 digital controller for isolated power supply
> with pmbus interface voltage, current and temperature monitor.
>
> Signed-off-by: Radu Sabau <radu.sabau@xxxxxxxxxx>
> ---
>   .../bindings/hwmon/pmbus/adi,adp1050.yaml     | 65 +++++++++++++++++++
>   MAINTAINERS                                   |  8 +++
>   2 files changed, 73 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
> new file mode 100644
> index 000000000000..e3162d0df0e2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: htpps://devicetree.org/schemas/hwmon/pmbus/adi,adp1050.yaml#
> +$schema: htpps://devicetree.org/meta-schemes/core.yaml#
> +
> +title: Analog Devices ADP1050 digital controller with PMBus interface
> +
> +maintainers:
> +  - Radu Sabau <radu.sabau@xxxxxxxxxx>
> +
> +description: |
> +   The ADP1050 is used to monitor system voltages, currents and temperatures.
> +   Through the PMBus interface, the ADP1050 targets isolated power supplies
> +   and has four individual monitors for input/output voltage, input current
> +   and temperature.
> +   Datasheet:
> +     https://www.analog.com/en/products/adp1050.html
> +properties:
> +  compatbile:
> +    const: adi,adp1050
> +
> +  reg:
> +    maxItems: 1
> +
> +  vcc-supply: true
> +
> +  adi,vin-scale-monitor:
> +    description:
> +      The value of the input voltage scale used by the internal ADP1050 ADC in
> +      order to read correct voltage values.
> +    $ref: /schemas/typees.yaml#/definitions/uint16
> +  adi,iin-scale-monitor:
> +    description:
> +      The value of the input current scale used by the internal ADP1050 ADC in
> +      order to read carrect current values.
> +    $ref: /schemas/typees.yaml#/definitions/uint16
> +

I don't see value in "-monitor" in those property names.

Also, I don't see why those properties should be mandatory since the chip
has the ability to store its configuration in eeprom.

The proposed driver code disables vin and iin monitoring if the properties
are set to 0 or not provided. I disagree that this should be possible in
the first place (I don't see its value), but if disabling vin and iin
monitoring is supported it should be documented.

Last but not least, I think those values should be abstracted with some
scale instead of reflecting raw (and unexplained) register values.

Thanks,
Guenter