Re: [PATCH 1/2] dt-bindings: hwmon: Add LTC4282 bindings

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 9655 bytes --]

Yo,

On Fri, Nov 10, 2023 at 04:18:45PM +0100, Nuno Sa wrote:
> Add bindings for the LTC4282 High Current Hot Swap Controller with I2C
> Compatible Monitoring.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  .../bindings/hwmon/adi,ltc4282.yaml           | 228 ++++++++++++++++++
>  MAINTAINERS                                   |   6 +
>  2 files changed, 234 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml b/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml
> new file mode 100644
> index 000000000000..0a5d540f014e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml
> @@ -0,0 +1,228 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/adi,ltc4282.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices LTC4282 I2C High Current Hot Swap Controller over I2C
> +
> +maintainers:
> +  - Nuno Sa <nuno.sa@analog.com>
> +
> +description: |
> +  Analog Devices LTC4282 I2C High Current Hot Swap Controller over I2C.
> +
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4282.pdf
> +
> +

Extra blank line here FYI.

> +properties:
> +  compatible:
> +    enum:
> +      - adi,ltc4282
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +  clocks:
> +    maxItems: 1
> +
> +  adi,clkout-mode:
> +    description: |
> +      Controls in which mode the CLKOUT PIN should work:
> +        0 - Configures the CLKOUT pin to output the internal system clock
> +        1 - Configures the CLKOUT pin to output the internal conversion time
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]

I really am not a fan of these types of properties. Part of me says that
if you're outputting clocks from this device, then you should be a clock
controller. How do consumers of this @clkout@ pin get the rate of the
clock?
I'd kinda be expecting to see a clocks property with a maxItems of 1 and
clock-names with two, mutually exclusive, options.

The other part says, and it applies in multiple places here, that having
integer properties with non-integer meanings is a poor ABI. I'd be vastly
happier if the various instances in this file became enums of strings,
or $re┤evant-unit so that a dts containing these properties is
immediately understandable.

Cheers,
COnor.

> +
> +  adi,rsense-nano-ohms:
> +    description: Value of the sense resistor.
> +
> +  adi,vin-mode-microvolt:
> +    description:
> +      Selects operating range for the Undervoltage, Overvoltage and Foldback
> +      pins. Also for the ADC. Should be set to the nominal input voltage.
> +    enum: [3300000, 5000000, 12000000, 24000000]
> +    default: 12000000
> +
> +  adi,fet-bad-timeout-ms:
> +    description:
> +      From the moment a FET bad conditions is present, this property selects the
> +      wait time/timeout for a FET-bad fault to be signaled. Setting this to 0,
> +      disables FET bad faults to be reported.
> +    default: 255
> +    maximum: 255
> +
> +  adi,overvoltage-dividers:
> +    description: |
> +      Select which dividers to use for VDD Overvoltage detection. Note that
> +      when the internal dividers are used the threshold is referenced to VDD.
> +      The percentages in the datasheet are misleading since the actual values
> +      to look for are in the "Absolute Maximum Ratings" table in the
> +      "Comparator Inputs" section. In there there's a line for each of the 5%,
> +      10% and 15% settings with the actual min, typical and max tolerances.
> +        0 - Use the external dividers.
> +        1 - Internal dividers 5%
> +        2 - Internal dividers 10%
> +        3 - Internal dividers 15%
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2, 3]
> +    default: 0
> +
> +  adi,undervoltage-dividers:
> +    description: |
> +      Select which dividers to use for VDD Overvoltage detection. Note that
> +      when the internal dividers are used the threshold is referenced to VDD.
> +      The percentages in the datasheet are misleading since the actual values
> +      to look for are in the "Absolute Maximum Ratings" table in the
> +      "Comparator Inputs" section. In there there's a line for each of the 5%,
> +      10% and 15% settings with the actual min, typical and max tolerances.
> +        0 - Use the external dividers.
> +        1 - Internal dividers 5%
> +        2 - Internal dividers 10%
> +        3 - Internal dividers 15%
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2, 3]
> +    default: 0
> +
> +  adi,current-limit-microvolt:
> +    description:
> +      The current limit sense voltage of the chip is adjustable between
> +      12.5mV and 34.4mV in 3.1mV steps. This effectively limits the current
> +      on the load.
> +    enum: [12500, 15625, 18750, 21875, 25000, 28125, 31250, 34375]
> +    default: 25000
> +
> +  adi,overcurrent-retry:
> +    description:
> +      If set, enables the chip to auto-retry 256 timer cycles after an
> +      Overcurrent fault.
> +    type: boolean
> +
> +  adi,overvoltage-retry-disable:
> +    description:
> +      If set, disables the chip to auto-retry 50ms after an Overvoltage fault.
> +      It's enabled by default.
> +    type: boolean
> +
> +  adi,undervoltage-retry-disable:
> +    description:
> +      If set, disables the chip to auto-retry 50ms after an Undervoltage fault.
> +      It's enabled by default.
> +    type: boolean
> +
> +  adi,fault-log-enable:
> +    description:
> +      If set, enables the FAULT_LOG and ADC_ALERT_LOG registers to be written
> +      to the EEPROM when a fault bit transitions high and hence, will be
> +      available after a power cycle (the chip loads the contents of
> +      the EE_FAULT_LOG register - the one in EEPROM - into FAULT_LOG at boot).
> +    type: boolean
> +
> +  adi,gpio-alert:
> +    description: Use the ALERT pin as a GPIO.
> +    type: boolean
> +
> +  adi,gpio1-mode:
> +    description: |
> +      Defines the function of the Pin.
> +          0 - GPIO Mode.
> +          1 - Power Bad. Goes into high-z to indicate that the output power is
> +              no longer good.
> +          2 - Power Good. Pulls low to indicate that the output power is no
> +              longer good.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maximum: 2
> +
> +  adi,gpio2-mode:
> +    description: |
> +      Defines the function of the Pin.
> +          0 - GPIO Mode.
> +          1 - Acts as an input pin and it is feeded into the ADC.
> +          2 - Pulls Low when the MOSFET is dissipating power (stress).
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maximum: 2
> +
> +  adi,gpio3-mode:
> +    description: |
> +      Defines the function of the Pin.
> +          0 - GPIO Mode.
> +          1 - Acts as an input pin and it is feeded into the ADC.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maximum: 1
> +
> +  gpio-controller:
> +    description:
> +      This property applies if some of the pins are used as GPIOs.
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - adi,rsense-nano-ohms
> +
> +dependencies:
> +  adi,alert-as-gpio: [gpio-controller, '#gpio-cells']
> +
> +allOf:
> +  - if:
> +      required:
> +        - adi,gpio1-mode
> +    then:
> +      allOf:
> +        - if:
> +            properties:
> +              adi,gpio0-mode:
> +                const: 0
> +          then:
> +            dependencies:
> +              adi,gpio0-mode: [gpio-controller, '#gpio-cells']
> +  - if:
> +      required:
> +        - adi,gpio2-mode
> +    then:
> +      allOf:
> +        - if:
> +            properties:
> +              adi,gpio1-mode:
> +                const: 0
> +          then:
> +            dependencies:
> +              adi,gpio1-mode: [gpio-controller, '#gpio-cells']
> +  - if:
> +      required:
> +        - adi,gpio3-mode
> +    then:
> +      allOf:
> +        - if:
> +            properties:
> +              adi,gpio2-mode:
> +                const: 0
> +          then:
> +            dependencies:
> +              adi,gpio2-mode: [gpio-controller, '#gpio-cells']
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        hwmon@50 {
> +            compatible = "adi,ltc4282";
> +            reg = <0x50>;
> +            adi,rsense-nano-ohms = <500>;
> +
> +            gpio-controller;
> +            #gpio-cells = <2>;
> +
> +            adi,gpio1-mode = <2>;
> +            adi,gpio2-mode = <0>;
> +        };
> +    };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 43121073390c..9f9527f6057b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12481,6 +12481,12 @@ S:	Maintained
>  F:	Documentation/hwmon/ltc4261.rst
>  F:	drivers/hwmon/ltc4261.c
>  
> +LTC4282 HARDWARE MONITOR DRIVER
> +M:	Nuno Sa <nuno.sa@analog.com>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Supported
> +F:	Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml
> +
>  LTC4306 I2C MULTIPLEXER DRIVER
>  M:	Michael Hennerich <michael.hennerich@analog.com>
>  L:	linux-i2c@vger.kernel.org
> -- 
> 2.42.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]