Re: [PATCH 1/3] dt-bindings: hwmon: ltc4283: add ltc4284

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: hwmon: ltc4283: requiring rsense2 breaks single-driver mode
- [Low] dt-bindings: hwmon: ltc4283: ltc4284 example sense resistors below min 50000
--

commit 9676b294cf2ad1168e5ce0d8de5c0cd58e68d9ed
Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

dt-bindings: hwmon: ltc4283: add ltc4284

This commit adds device tree binding support for the Analog Devices LTC4284
negative voltage hot swap controller to the adi,ltc4283 schema, including
new properties for its second sense resistor and ADC resistance.

> diff --git a/Documentation/devicetree/bindings/hwmon/adi,ltc4283.yaml b/Documentation/devicetree/bindings/hwmon/adi,ltc4283.yaml
> index 05e2132ad4d86..c850390ddc4a3 100644
> --- a/Documentation/devicetree/bindings/hwmon/adi,ltc4283.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/adi,ltc4283.yaml
> @@ -10,22 +10,42 @@ maintainers:
>    - Nuno Sá <nuno.sa@analog.com>
>  
>  description: |
> -  The LTC4283 negative voltage hot swap controller drives an external N-channel
> -  MOSFET to allow a board to be safely inserted and removed from a live
> -  backplane.
> +  The LTC4283 and LTC4284 are negative voltage hot swap controllers that drive
> +  external N-channel MOSFETs to allow a board to be safely inserted and removed
> +  from a live backplane.
> +
> +  The LTC4284 features dual-gate drivers with four operating modes (single
> +  driver, parallel, high stress staged start, and low stress staged start)
> +  configured via the hardware MODE pin. The MODE pin must be connected to the
> +  appropriate voltage level (VEE, VIN, INTVCC, or left open) and cannot be
> +  controlled via software or GPIO.
>  
>    https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4283.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4284.pdf
>  
>  properties:
>    compatible:
>      enum:
>        - adi,ltc4283
> +      - adi,ltc4284
>  
>    reg:
>      maxItems: 1
>  
>    adi,rsense-nano-ohms:
> -    description: Value of the sense resistor.
> +    description:
> +      Value of the sense resistor. For LTC4283, this is the resistor connected
> +      between SENSE+/SENSE- pins. For LTC4284, this is the resistor connected
> +      between SENSE1+/SENSE1- pins.
> +
> +  adi,rsense2-nano-ohms:
> +    description:
> +      Value of the SENSE2 current sense resistor. This resistor is connected
> +      between SENSE2+ and SENSE2- pins.
> +
> +  adi,radc-nano-ohms:
> +    description:
> +      Value of the effective resistance seen by the ADC+/ADC- pins.
>  
>    adi,current-limit-sense-microvolt:
>      description:
> @@ -195,6 +215,20 @@ required:
>    - adi,rsense-nano-ohms
>  
>  allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: adi,ltc4284
> +    then:
> +      required:
> +        - adi,rsense2-nano-ohms
> +        - adi,radc-nano-ohms
> +    else:
> +      properties:
> +        adi,rsense2-nano-ohms: false
> +        adi,radc-nano-ohms: false

[Severity: Medium]
As noted in the binding description above, the LTC4284 supports four
hardware operating modes configured via the MODE pin, including single
driver mode.

In single driver mode, only the first MOSFET path (GATE1 and SENSE1+/SENSE1-)
is used, and no second sense resistor is populated between SENSE2+ and
SENSE2-.

If adi,rsense2-nano-ohms is unconditionally required in the allOf block for
adi,ltc4284, wouldn't boards wired in single driver mode fail schema
validation unless they specify a nonexistent adi,rsense2-nano-ohms property?

> @@ -269,4 +303,19 @@ examples:
>              #gpio-cells = <2>;
>          };
>      };
> +
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        swap-controller@20 {
> +            compatible = "adi,ltc4284";
> +            reg = <0x20>;
> +
> +            adi,rsense-nano-ohms = <1000>;
> +            adi,rsense2-nano-ohms = <1000>;
> +            adi,radc-nano-ohms = <500>;
> +        };

[Severity: Low]
In drivers/hwmon/ltc4283.c, ltc4283_setup() validates the sense resistor
properties against [LTC4283_MIN_RSENSE, LTC4283_MAX_RSENSE], where
LTC4283_MIN_RSENSE is 50000 nano-ohms (50 micro-ohms) and LTC4283_MAX_RSENSE
is 1677721599 nano-ohms:

    if (st->rsense < LTC4283_MIN_RSENSE || st->rsense > LTC4283_MAX_RSENSE)
        return dev_err_probe(dev, -EINVAL,
                             "adi,rsense-nano-ohms(%u) too small or too large [%u %u]\n",
                             st->rsense, LTC4283_MIN_RSENSE, LTC4283_MAX_RSENSE);

The example above sets adi,rsense-nano-ohms = <1000>,
adi,rsense2-nano-ohms = <1000>, and adi,radc-nano-ohms = <500> (1 micro-ohm
and 0.5 micro-ohm), which will cause ltc4283_setup() to fail probe with
-EINVAL.

Were these example values intended to be 1000 micro-ohms (<1000000>
nano-ohms) and 500 micro-ohms (<500000> nano-ohms), and should
adi,rsense-nano-ohms, adi,rsense2-nano-ohms, and adi,radc-nano-ohms specify
minimum: 50000 and maximum: 1677721599 in the schema?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-staging-ltc4284-v1-0-49fce67ebc48@analog.com?part=1