Re: [PATCH 2/5] regulator: dt-bindings: Add TI LM51772 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] regulator: dt-bindings: ti,lm51772: missing additionalProperties on regulators
- [Medium] regulator: dt-bindings: ti,lm51772: invalid default: false on boolean properties
- [Medium] regulator: dt-bindings: ti,lm51772: non-standard unit suffixes on vendor properties
- [Medium] regulator: dt-bindings: ti,lm51772: gap in ti,ivp-thres-millivolt range
- [Low] regulator: dt-bindings: ti,lm51772: 10x typo in regulator-max-microamp maximum
- [Low] regulator: dt-bindings: ti,lm51772: example fails dt-check-style strict checks
--

commit 791e5dd89a97fb0d36e798526e83309f0104b7b6
Author: Paul Louvel <paul.louvel@bootlin.com>

regulator: dt-bindings: Add TI LM51772 support

This patch adds the devicetree binding schema for the Texas Instruments
lm51772 buck-boost voltage regulator and its auxiliary fixed 5v regulator.

> diff --git a/Documentation/devicetree/bindings/regulator/ti,lm51772.yaml b/Documentation/devicetree/bindings/regulator/ti,lm51772.yaml
> new file mode 100644
> index 0000000000000..1e72ac13ddfc3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/ti,lm51772.yaml
> @@ -0,0 +1,239 @@

[ ... ]

> +  The peak current limit threshold is programmed through software, between 0.5A
> +  to 7A.
> +  The LM51772 also provides an auxiliary fixed 5 V regulator output
> +  (VCC1).
> +
> +properties:
> +  compatible:
> +    const: ti,lm51772
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +    description: Fault / warning irq.
> +
> +  vin-supply:
> +    description: Regulator supply for the chip.
> +
> +  regulators:
> +    type: object
> +    description: List of regulators provided by this PMIC.
> +
> +    properties:
> +      vout:
> +        type: object
> +        $ref: regulator.yaml#
> +        description: Main output voltage.
> +
> +        properties:
> +          ti,active-discharge-strength-milliamp:
> +            description: Discharge current for VOUT.
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            multipleOf: 25
> +            minimum: 25
> +            maximum: 75
> +            default: 25
> +
> +          ti,iset-pin:
> +            description: |
> +              Select the ISET pin as the source of the current limit, instead of
> +              the ILIM DAC. When this property is present, the ISET pin is forced
> +              in its active configuration and the ILIM DAC is disabled, so the
> +              current limit can no longer be programmed through the
> +              dedicated register.
> +            type: boolean
> +            default: false

[ ... ]

> +          ti,sense-res-milliohm:
> +            description: |
> +              Value in milliohms of the current sense resistor connected between the
> +              ISNSP and ISNSN pins, used to translate the ILIM threshold voltage into
> +              a current limit and vice versa.
> +              TI recommands a value of 10 milliohms.
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            minimum: 1
> +            default: 10

[Severity: Medium]
In Documentation/devicetree/bindings/regulator/ti,lm51772.yaml,
ti,active-discharge-strength-milliamp, ti,sense-res-milliohm, and
ti,ivp-thres-millivolt use -milliamp, -milliohm, and -millivolt suffixes
with an explicit uint32 type reference.

When running make dt_binding_check, dtschema meta-schemas/vendor-props.yaml
rejects new vendor properties that use deprecated or non-standard unit
suffixes instead of standard suffixes from property-units.yaml (-microamp,
-micro-ohms, and -microvolt, which also provide the uint32 type
automatically).

Could ti,active-discharge-strength-milliamp and ti,ivp-thres-millivolt use
the standard -microamp and -microvolt suffixes, and could
ti,sense-res-milliohm use the standard shunt-resistor-micro-ohms property?

[ ... ]

> +          regulator-min-microamp:
> +            minimum: 500000
> +            maximum: 70000000
> +
> +          regulator-max-microamp:
> +            minimum: 500000
> +            maximum: 70000000

[Severity: Low]
Under properties:regulators:properties:vout in
Documentation/devicetree/bindings/regulator/ti,lm51772.yaml, both
regulator-min-microamp and regulator-max-microamp set maximum to 70000000
(70 amperes), whereas the description at the top of the file states that
the peak current limit threshold is programmable between 0.5A to 7A.

Should maximum on regulator-min-microamp and regulator-max-microamp be
7000000 instead of 70000000 so that invalid current limits above 7 amperes
do not pass schema validation?

> +
> +        unevaluatedProperties: false
> +
> +      vcc1:
> +        type: object
> +        $ref: regulator.yaml#
> +        description: |
> +          LDO regulator for auxiliary use.
> +          It can used to power, for example, a PD controller that will drive this PMIC.
> +
> +        properties:
> +          regulator-min-microvolt:
> +            minimum: 5000000
> +            maximum: 5000000
> +
> +          regulator-max-microvolt:
> +            minimum: 5000000
> +            maximum: 5000000
> +
> +        unevaluatedProperties: false

[Severity: Medium]
In Documentation/devicetree/bindings/regulator/ti,lm51772.yaml,
properties:regulators is defined with type: object and child properties
vout and vcc1, but the regulators object itself does not specify
additionalProperties: false.

Without additionalProperties: false on properties:regulators, running make
dt_binding_check fails meta-schema validation and undocumented child nodes
under regulators silently pass validation. Should additionalProperties: false
be added to the regulators object?

[ ... ]

> +  ti,ivp-thres-millivolt:
> +    description: |
> +      Input Overvoltage Protection and Regulation Threshold.
> +      It shall avoid any damage to the device in case the current flows from the
> +      output to the input and the input source cannot sink current.
> +      This threshold is used for IVP (Input Voltage Protection) and IVR (Input
> +      Voltage Regulation).
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    oneOf:
> +      - multipleOf: 125
> +        minimum: 4750
> +        maximum: 23250
> +      - multipleOf: 250
> +        minimum: 23750
> +        maximum: 50000
> +    default: 50000

[Severity: Medium]
In Documentation/devicetree/bindings/regulator/ti,lm51772.yaml, the oneOf
constraints on properties:ti,ivp-thres-millivolt cover 4750 to 23250 in
steps of 125 and 23750 to 50000 in steps of 250, which leaves a gap at
23500.

In drivers/regulator/lm51772-regulator.c (from commit "regulator: Add TI
LM51772 support"), lm51772_of_parse() defines the hardware linear ranges
as:

  LINEAR_RANGE(4750000, 0x00, 0x94, 125000),
  LINEAR_RANGE(23500000, 0x95, 0xff, 250000)

where selector 0x94 is 23250 millivolts and selector 0x95 is 23500
millivolts. Should the minimum of the second oneOf branch be 23500 (or
23500000 microvolts) so that the valid 23.5 volt threshold is not rejected
by schema validation?

> +
> +  ti,ivp:
> +    description: Enable the IVP feature of the regulator.
> +    type: boolean
> +    default: false
> +
> +  ti,ivr:
> +    description: Enable the IVR feature of the regulator.
> +    type: boolean
> +    default: false
> +
> +  ti,nint:
> +    description: |
> +      Configures the nFLT/nINT pin to act as an interrupt pin.
> +
> +      nFLT mode: The pin remains low until the cause of the status change ceases.
> +      In this mode use an level irq trigger.
> +
> +      nINT mode: The pin is asserted low to signal a status change.
> +      In this mode use an edge irq trigger.
> +    type: boolean
> +    default: false
> +
> +  ti,cable-drop-compensation:
> +    description: |
> +      The cable drop compensation feature helps to keep the output voltage at the
> +      nominal value over a wide range of load current without the need for
> +      additional remote sensing. The cable drop compensation measures the
> +      current and offsets the output voltage proportionally to the measured
> +      current.
> +    type: boolean
> +    default: false

[Severity: Medium]
In Documentation/devicetree/bindings/regulator/ti,lm51772.yaml, the boolean
properties ti,iset-pin, ti,ivp, ti,ivr, ti,nint, and
ti,cable-drop-compensation all specify default: false alongside
type: boolean.

Because dtschema meta-schemas/boolean.yaml sets additionalProperties: false
and does not allow the default keyword on boolean flag properties, running
make dt_binding_check fails validation on these five properties. Should the
default: false lines be removed from ti,iset-pin, ti,ivp, ti,ivr, ti,nint,
and ti,cable-drop-compensation?

[ ... ]

> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        pmic@6a {
> +            compatible = "ti,lm51772";
> +            reg = <0x6a>;
> +            interrupt-parent = <&irqc>;
> +            interrupts = <1 IRQ_TYPE_EDGE_FALLING>;
> +
> +            ti,nint;
> +
> +            regulators {
> +              vout {
> +                  regulator-name = "vout";
> +                  regulator-min-microvolt = <5000000>;
> +                  regulator-max-microvolt = <20000000>;
> +                  regulator-ov-protection-microvolt = <1000000>;
> +                  regulator-min-microamp = <1000000>;
> +                  regulator-max-microamp = <5000000>;
> +                  regulator-temp-warn-kelvin = <368>;
> +              };
> +
> +              vcc1 {
> +                  regulator-name = "vcc1";
> +                  regulator-min-microvolt = <5000000>;
> +                  regulator-max-microvolt = <5000000>;
> +              };
> +            };
> +        };
> +    };

[Severity: Low]
In the examples section of
Documentation/devicetree/bindings/regulator/ti,lm51772.yaml, the vout and
vcc1 child nodes under regulators are indented by 2 spaces instead of 4
spaces relative to regulators, and the child nodes and regulator-*
properties are not in canonical sort order.

Running scripts/dtc/dt-check-style --mode=strict reports indent-consistent,
child-name-order, and property-order warnings here. Could the example node
indentation and ordering be aligned with dt-check-style?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-paul-lm51772-v1-0-25f0ffbffa8d@bootlin.com?part=2