Linux Documentation
 help / color / mirror / Atom feed
From: "Nuno Sá" <noname.nuno@gmail.com>
To: Guenter Roeck <linux@roeck-us.net>, nuno.sa@analog.com
Cc: linux-gpio@vger.kernel.org, linux-hwmon@vger.kernel.org,
	 devicetree@vger.kernel.org, linux-doc@vger.kernel.org,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Shuah Khan <skhan@linuxfoundation.org>,
	Linus Walleij <linusw@kernel.org>,
	Bartosz Golaszewski <brgl@kernel.org>
Subject: Re: [PATCH v7 1/3] dt-bindings: hwmon: Document the LTC4283 Swap Controller
Date: Mon, 23 Mar 2026 10:31:42 +0000	[thread overview]
Message-ID: <77cd7e879a10df791d9d5eb1f16f1654e9904199.camel@gmail.com> (raw)
In-Reply-To: <c395fad0-ca24-448a-a77f-ddac1cd9f809@roeck-us.net>

On Mon, 2026-03-16 at 08:59 -0700, Guenter Roeck wrote:
> On Sat, Mar 14, 2026 at 10:52:19AM +0000, Nuno Sá via B4 Relay wrote:
> > From: Nuno Sá <nuno.sa@analog.com>
> > 
> > The LTC4283 is a negative voltage hot swap controller that drives an
> > external N-channel MOSFET to allow a board to be safely inserted and
> > removed from a live backplane.
> > 
> > Special note for the "adi,vpower-drns-enable" property. It allows to choose
> > between the attenuated MOSFET drain voltage or the attenuated input
> > voltage at the RTNS pin (effectively choosing between input or output
> > power). This is a system level decision not really intended to change at
> > runtime and hence is being added as a Firmware property.
> > 
> > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> 
> Some AI review feedback inline. Feel free to ignore if wrong, but please let me know
> to help improve it.
> 
> Thanks,
> Guenter
> 
> > ---
> >  .../devicetree/bindings/hwmon/adi,ltc4283.yaml     | 272 +++++++++++++++++++++
> >  MAINTAINERS                                        |   6 +
> >  2 files changed, 278 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/adi,ltc4283.yaml
> > b/Documentation/devicetree/bindings/hwmon/adi,ltc4283.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..f82fff1ec7e4407ed63d00f8b1281db459d7221b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/adi,ltc4283.yaml
> > @@ -0,0 +1,272 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/adi,ltc4283.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: LTC4283 Negative Voltage Hot Swap Controller
> > +
> > +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.
> > +
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4283.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,ltc4283
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  adi,rsense-nano-ohms:
> > +    description: Value of the sense resistor.
> > +
> > +  adi,current-limit-sense-microvolt:
> > +    description:
> > +      The current limit sense voltage of the chip is adjustable between
> > +      15mV and 30mV in 1mV steps. This effectively limits the current
> > +      on the load.
> > +    minimum: 15000
> > +    maximum: 30000
> > +    default: 15000
> > +
> > +  adi,current-limit-foldback-factor:
> > +    description:
> > +      Specifies the foldback factor for the current limit. The current limit
> > +      can be reduced (folded back) to one of four preset levels. The value
> > +      represents the percentage of the current limit sense voltage to use
> > +      during foldback. A value of 100 means no foldback.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [10, 20, 50, 100]
> > +    default: 100
> > +
> > +  adi,cooling-delay-ms:
> > +    description:
> > +      Cooling time to apply after an overcurrent fault, FET bad or
> > +      external fault.
> > +    enum: [512, 1002, 2005, 4100, 8190, 16400, 32800, 65600]
> > +    default: 512
> > +
> > +  adi,fet-bad-timer-delay-ms:
> > +    description:
> > +      FET bad timer delay. After a FET bad status condition is detected,
> > +      this timer is started. If the condition persists for the
> > +      specified time, the FET is turned off and a fault is logged.
> > +    enum: [256, 512, 1002, 2005]
> > +    default: 256
> > +
> > +  adi,power-good-reset-on-fet:
> > +    description:
> > +      If set, resets the power good status when the MOSFET is turned off.
> > +      Otherwise, it resets when a low output voltage is detected.
> > +    type: boolean
> > +
> > +  adi,fet-turn-off-disable:
> > +    description:
> > +      If set, the MOSFET is turned off immediately when a FET fault is detected.
> > +    type: boolean
> 
> Is there a logic inversion between the property name and its description?
> The property name uses a -disable suffix, but the description says "If set,
> the MOSFET is turned off immediately", which sounds like it is enabling the
> behavior rather than disabling it.

Yes. Misleading description. This has -disable because the default is the -enable case which
indeed is the case when the FET is turned off. Will update the description so that is not
confusing.

> > +
> > +  adi,tmr-pull-down-disable:
> > +    description: Disables 2uA pull-down current on the TMR pin.
> > +    type: boolean
> > +
> > +  adi,dvdt-inrush-control-disable:
> > +    description:
> > +      Enables dV/dt inrush control during startup. In dV/dt mode, the inrush
> > +      current is limited by controlling a constant output voltage ramp rate.
> > +      If not set, the inrush control mechanism is active current limiting.
> > +    type: boolean
> 
> Does this description contradict the property name?
> The -disable suffix implies the property turns off the dV/dt inrush control,
> but the description states that setting the property enables it.
> 
> [ Non-AI note: It seems to me that the description contradicts itself. 
>   It first says "_Enables_ ...", then it says "If _not_ set, the inrush control
>   mechanism is active current limiting" ]

Yeah same thing as above. I will update this one just by s/Enables/Disables/

> 
> > +
> > +  adi,fault-log-enable:
> > +    description:
> > +      If set, enables logging fault registers and ADC data into EEPROM upon a
> > +      fault.
> > +    type: boolean
> > +
> > +  adi,vpower-drns-enable:
> > +    description:
> > +      If set, enables the attenuated MOSFET drain voltage to be monitored. This
> > +      effectively means that the MOSFET power is monitored. If not set, the
> > +      attenuated input voltage (and hence input power) is monitored.
> > +    type: boolean
> > +
> > +  adi,external-fault-fet-off-enable:
> > +    description: Turns MOSFET off following an external fault.
> > +    type: boolean
> > +
> > +  adi,undervoltage-retry-disable:
> > +    description: Do not retry to turn on the MOSFET after an undervoltage fault.
> > +    type: boolean
> > +
> > +  adi,overvoltage-retry-disable:
> > +    description: Do not retry to turn on the MOSFET after an overvoltage fault.
> > +    type: boolean
> > +
> > +  adi,external-fault-retry-enable:
> > +    description: Retry to turn on the MOSFET retry after an external fault.
> > +    type: boolean
> 
> This isn't a bug, but there's a typo in the description where the word
> "retry" is repeated.

ack.

> 
> > +
> > +  adi,overcurrent-retries:
> > +    description: Configures auto-retry following an Overcurrent fault.
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [latch-off, "1", "7", unlimited]
> > +    default: latch-off
> > +
> > +  adi,fet-bad-retries:
> > +    description:
> > +      Configures auto-retry following a FET bad fault and a consequent MOSFET
> > +      turn off.
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [latch-off, "1", "7", unlimited]
> > +    default: latch-off
> > +
> > +  adi,pgio1-func:
> > +    description: Configures the function of the PGIO1 pin.
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [inverted_power_good, power_good, gpio]
> > +    default: inverted_power_good
> > +
> > +  adi,pgio2-func:
> > +    description: Configures the function of the PGIO2 pin.
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [inverted_power_good, power_good, gpio, active_current_limiting]
> > +    default: inverted_power_good
> > +
> > +  adi,pgio3-func:
> > +    description: Configures the function of the PGIO3 pin.
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [inverted_power_good_input, power_good_input, gpio]
> > +    default: inverted_power_good_input
> > +
> > +  adi,pgio4-func:
> > +    description: Configures the function of the PGIO4 pin.
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [inverted_external_fault, external_fault, gpio]
> > +    default: inverted_external_fault
> > +
> > +  adi,gpio-on-adio1:
> > +    description: If set, the ADIO1 pin is used as a GPIO.
> > +    type: boolean
> > +
> > +  adi,gpio-on-adio2:
> > +    description: If set, the ADIO2 pin is used as a GPIO.
> > +    type: boolean
> > +
> > +  adi,gpio-on-adio3:
> > +    description: If set, the ADIO3 pin is used as a GPIO.
> > +    type: boolean
> > +
> > +  adi,gpio-on-adio4:
> > +    description: If set, the ADIO4 pin is used as a GPIO.
> > +    type: boolean
> 
> Does this dependency block force a redundant specification of adi,pgio4-func?
> The default for adi,pgio4-func is inverted_external_fault, which means the
> default hardware state already supports external fault features.
> If a device tree legitimately omits adi,pgio4-func to rely on that default,
> will it fail schema validation here since the dependencies keyword strictly
> checks for the literal presence of properties without injecting defaults?

Fair point. I guess it will fail but the alternative is to not have any constrain at all so
maybe worth it to be explicit in here?

- Nuno Sá

> 
> > +
> > +  gpio-controller: true
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +
> > +dependencies:
> > +  adi,gpio-on-adio1:
> > +    - gpio-controller
> > +    - '#gpio-cells'
> > +  adi,gpio-on-adio2:
> > +    - gpio-controller
> > +    - '#gpio-cells'
> > +  adi,gpio-on-adio3:
> > +    - gpio-controller
> > +    - '#gpio-cells'
> > +  adi,gpio-on-adio4:
> > +    - gpio-controller
> > +    - '#gpio-cells'
> > +  adi,external-fault-retry-enable:
> > +    - adi,pgio4-func
> > +  adi,external-fault-fet-off-enable:
> > +    - adi,pgio4-func
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - adi,rsense-nano-ohms
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        adi,pgio1-func:
> > +          const: gpio
> > +      required:
> > +        - adi,pgio1-func
> > +    then:
> > +      required:
> > +        - gpio-controller
> > +        - '#gpio-cells'
> > +
> > +  - if:
> > +      properties:
> > +        adi,pgio2-func:
> > +          const: gpio
> > +      required:
> > +        - adi,pgio2-func
> > +    then:
> > +      required:
> > +        - gpio-controller
> > +        - '#gpio-cells'
> > +
> > +  - if:
> > +      properties:
> > +        adi,pgio3-func:
> > +          const: gpio
> > +      required:
> > +        - adi,pgio3-func
> > +    then:
> > +      required:
> > +        - gpio-controller
> > +        - '#gpio-cells'
> > +
> > +  - if:
> > +      properties:
> > +        adi,pgio4-func:
> > +          const: gpio
> > +      required:
> > +        - adi,pgio4-func
> > +    then:
> > +      properties:
> > +        adi,external-fault-retry-enable: false
> > +        adi,external-fault-fet-off-enable: false
> > +      required:
> > +        - gpio-controller
> > +        - '#gpio-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        swap-controller@15 {
> > +            compatible = "adi,ltc4283";
> > +            reg = <0x15>;
> > +
> > +            adi,rsense-nano-ohms = <500>;
> > +            adi,current-limit-sense-microvolt = <25000>;
> > +            adi,current-limit-foldback-factor = <10>;
> > +            adi,cooling-delay-ms = <8190>;
> > +            adi,fet-bad-timer-delay-ms = <512>;
> > +
> > +            adi,external-fault-fet-off-enable;
> > +            adi,pgio4-func = "external_fault";
> > +
> > +            adi,gpio-on-adio1;
> > +            adi,pgio1-func = "gpio";
> > +            gpio-controller;
> > +            #gpio-cells = <2>;
> > +        };
> > +    };
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 830c6f076b0029f0ff1abee148ad0e1905a60e82..13ae2f3db449e5fd3a7d0fbac92aabdc01734ba9 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15141,6 +15141,12 @@ F:	Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml
> >  F:	Documentation/hwmon/ltc4282.rst
> >  F:	drivers/hwmon/ltc4282.c
> >  
> > +LTC4283 HARDWARE MONITOR AND GPIO DRIVER
> > +M:	Nuno Sá <nuno.sa@analog.com>
> > +L:	linux-hwmon@vger.kernel.org
> > +S:	Supported
> > +F:	Documentation/devicetree/bindings/hwmon/adi,ltc4283.yaml
> > +
> >  LTC4286 HARDWARE MONITOR DRIVER
> >  M:	Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> >  L:	linux-hwmon@vger.kernel.org
> > 
> > -- 
> > 2.51.0
> > 
> > 
> > 

  reply	other threads:[~2026-03-23 10:30 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-14 10:52 [PATCH v7 0/3] hwmon: Add support for the LTC4283 Hot Swap Controller Nuno Sá via B4 Relay
2026-03-14 10:52 ` [PATCH v7 1/3] dt-bindings: hwmon: Document the LTC4283 " Nuno Sá via B4 Relay
2026-03-16 15:59   ` Guenter Roeck
2026-03-23 10:31     ` Nuno Sá [this message]
2026-03-23 14:33       ` Guenter Roeck
2026-03-23 15:17         ` Nuno Sá
2026-03-23 15:27           ` Guenter Roeck
2026-03-23 16:07             ` Nuno Sá
2026-03-14 10:52 ` [PATCH v7 2/3] hwmon: ltc4283: Add support for " Nuno Sá via B4 Relay
2026-03-18  1:17   ` Guenter Roeck
2026-03-23 10:21     ` Nuno Sá
2026-03-14 10:52 ` [PATCH v7 3/3] gpio: gpio-ltc4283: " Nuno Sá via B4 Relay

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=77cd7e879a10df791d9d5eb1f16f1654e9904199.camel@gmail.com \
    --to=noname.nuno@gmail.com \
    --cc=brgl@kernel.org \
    --cc=conor+dt@kernel.org \
    --cc=corbet@lwn.net \
    --cc=devicetree@vger.kernel.org \
    --cc=krzk+dt@kernel.org \
    --cc=linusw@kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=nuno.sa@analog.com \
    --cc=robh@kernel.org \
    --cc=skhan@linuxfoundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox