linux-kernel - Re: [PATCH v3 3/3] dt-bindings: regulator: Add MAX77675 regulator binding

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d700c50a-272d-4b6b-8c39-615d096ffed4@kernel.org>
Date: Thu, 6 Nov 2025 08:08:36 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Joan Na <joan.na.devcode@...il.com>
Cc: Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, Joan Na <joan.na@...log.com>
Subject: Re: [PATCH v3 3/3] dt-bindings: regulator: Add MAX77675 regulator
 binding

On 06/11/2025 06:29, Joan Na wrote:
>>> +  maxim,bias-low-power-request:
>>> +    type: boolean
>>> +    description: |
>>> +      Request low-power bias mode.
>>> +      When set, the device enters low-power bias mode.
>>> +      Defaults to normal bias mode if this property is not specified.
>>> +    default: false
>>> +
>>> +  maxim,simo-int-ldo-always-on:
>>> +    type: boolean
>>> +    description: |
>>> +      Set internal LDO to always supply 1.8V
>>> +      When set, the internal LDO always supplies 1.8V.
>>> +      By default, the SIMO internal channel supplies 1.8V during low-power mode
>>> +    default: false
>>> +
>>> +  regulators:
>>> +    type: object
>>> +    description: Regulator child nodes
>>> +    patternProperties:
>>> +      "^sbb[0-3]$":
>>> +        type: object
>>> +        $ref: regulator.yaml#
>>> +    properties:
>>> +      maxim,fps-slot:
>>
>> That's not property of regulators. Totally messed indentation.
>>
>>
> 
> The maxim,fps-slot property is specific to the MAX77675 regulators 
> and is used to configure FPS slots individually for each regulator (e.g., sbb0–sbb3). 
> As this represents a device-specific extension rather than a generic regulator property, 
> it is defined under each regulator node.

But you did not define it under each regulator node. That would be fine.
You defined it under regulators. So again that is not a property of
regulators. That's a property of each regulator.

If you bothered to test it, you would see warnings.

> 
>>> +        description: |
>>> +          FPS (Flexible Power Sequencer) slot selection.
>>> +          The Flexible Power Sequencer allows resources to power up under hardware or software control.
>>> +          Additionally, each resource can power up independently or among a group of other regulators
>>> +          with adjustable power-up and power-down slots.
>>> +          This device's regulators provide an additional property to configure the FPS parameters,
>>> +          allowing each regulator to be assigned to an FPS slot for proper power management control.
>>> +          "slot0"   - Assign to FPS Slot 0
>>> +          "slot1"   - Assign to FPS Slot 1
>>> +          "slot2"   - Assign to FPS Slot 2
>>> +          "slot3"   - Assign to FPS Slot 3
>>> +          "default" - Use the default FPS slot value stored in OTP and read from the register
>>> +        $ref: /schemas/types.yaml#/definitions/string
>>> +        enum: ["slot0", "slot1", "slot2", "slot3", "default"]
>>> +        default: default
>>> +
>>> +      maxim,fixed-slew-rate:
>>> +        type: boolean
>>> +        description: |
>>> +          Use fixed slew rate of 2 mV/μs for output voltage transitions.
>>> +          When this property is present, the device uses a constant 2 mV/μs slew rate
>>> +          and ignores any dynamic slew rate configuration.
>>> +          When absent, the device uses the dynamic slew rate specified
>>> +          by 'maxim,dvs-slew-rate-mv-per-us'
>>> +        default: true
>>> +
>>> +    additionalProperties: false
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - regulators
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/regulator/maxim,max77675-regulator.h>
>>> +
>>> +    i2c {
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +
>>> +      max77675: pmic@44 {
>>> +        compatible = "maxim,max77675";
>>> +        reg = <0x44>;
>>> +
>>> +        maxim,en-mode = "slide-switch";
>>> +        maxim,latency-mode = "high";
>>> +        maxim,drv-sbb-strength = "max";
>>> +        maxim,dvs-slew-rate-mv-per-us = <5>;
>>> +        maxim,manual-reset-time-sec = <4>;
>>> +        maxim,en-debounce-time-us = <100>;
>>> +
>>> +        regulators {
>>> +          sbb0: sbb0 {
>>> +            regulator-name = "sbb0";
>>> +            regulator-min-microvolt = <500000>;
>>> +            regulator-max-microvolt = <5500000>;
>>> +            maxim,fps-slot = "default";
>>
>> I don't think this was tested.
>>
>>
>> Best regards,
>> Krzysztof
> 
> Testing on the actual EVKit has been conducted since PATCH V4

I have proofs this was not tested - see email from Rob.

But if you claim it was tested, then please explain me how can you
possible test a binding on EVKit (it is impossible) and how could your
testing miss such obvious errors?


> 


Best regards,
Krzysztof