Message-ID: <aQwyH4rYlcWNxGxI@HYB-iPCgmhaB8Cy.ad.analog.com>
Date: Thu, 6 Nov 2025 14:29:03 +0900
From: Joan Na <joan.na.devcode@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
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 Tue, Oct 14, 2025 at 10:14:15AM +0200, Krzysztof Kozlowski wrote:
> On 14/10/2025 07:31, Joan-Na-adi wrote:
> > From: Joan Na <joan.na@...log.com>
> > 
> > Add device tree binding YAML schema for the Maxim MAX77675 PMIC regulator.
> > This defines the node properties and supported regulator names for use
> > in device tree sources.

Hello Krzysztof,

Thank you again for your helpful feedback.

> 
> A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
> prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 

I plan to update the patch subject to avoid redundancy. The new title is:
dt-bindings: regulator: Add support for MAX77675 PMIC


> > 
> > Fixed:
> > - Missing explanation of `maxim,fps-slot` default value
> > - Updated DT binding enums to use string values (e.g., "low", "high") instead of integers
> > - Converted several binary properties to boolean type
> > - Renamed time-based properties to use standard unit suffixes (e.g., "-sec", "-us")
> > - Added default values for properties
> 
> 
> Probably you wanted a changelog? Changelog goes to --- with details
> against EACH version. Please start using b4 if canonical patch format is
> tricky. b4 would help.
>

I have applied this from PATCH V4.

> > 
> > Signed-off-by: Joan Na <joan.na@...log.com>
> > ---
> >  .../bindings/regulator/maxim,max77675.yaml    | 205 ++++++++++++++++++
> >  1 file changed, 205 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/regulator/maxim,max77675.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/regulator/maxim,max77675.yaml b/Documentation/devicetree/bindings/regulator/maxim,max77675.yaml
> > new file mode 100644
> > index 000000000000..6be29eced039
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/maxim,max77675.yaml
> > @@ -0,0 +1,205 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/regulator/maxim,max77675.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Maxim MAX77675 PMIC Regulator
> > +
> > +maintainers:
> > +  - Joan Na <joan.na@...log.com>
> > +
> > +description: |
> > +  The MAX77675 is a PMIC providing multiple switching buck regulators
> > +  (SBB0–SBB3), accessible via I2C. Each SBB can be configured individually
> > +  in the Device Tree. Additional PMIC settings can be configured through
> > +  device-specific properties.
> > +  Users should use the macros from dt-bindings/regulator/maxim,max77675-regulator.h
> > +
> > +allOf:
> > +  - $ref: regulator.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: maxim,max77675
> 
> Please organize the patch documenting compatible (DT bindings) before
> their user.
> See also:
> https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46
> 

I have changed the commit order starting from PATCH V5.

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  maxim,en-mode:
> > +    description: |
> > +      Enable mode configuration.
> > +      "push-button"  - Push button
> > +      "slide-switch" - Slide switch
> > +      "logic" - Logic mode
> 
> You just repeat the name in the description. Say something useful.
> 

Updated the DT property description as follows:
description: |
      Enable mode configuration.
      The debounce time set by 'adi,debounce-time-us' applies to
      both push-button and slide-switch modes.
      "push-button"  - A long press triggers power-on or power-down
      "slide-switch" - Low level powers on, high level powers down
      "logic"        - Low level powers on, high level powers down (no debounce time)

> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: ["push-button", "slide-switch", "logic"]
> > +    default: "slide-switch"
> > +
> > +  maxim,latency-mode:
> > +    description: |
> > +      Latency mode for voltage transition:
> > +      "high" - High latency (100μs)
> > +      "low"  - Low latency (10μs)
> 
> So use values 100 or 10, and proper type (-us)
> 
>

I have applied this from PATCH V5.

> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: ["high", "low"]
> > +    default: "high"
> > +
> > +  maxim,drv-sbb-strength:
> > +    description: |
> > +      SIMO Buck-Boost Drive Strength Trim.
> > +      Controls the drive strength of the SIMO regulator's power MOSFETs.
> > +      This setting affects the switching speed, which impacts power efficiency and EMI.
> > +      "max"  – Maximum drive strength (~0.6 ns transition time)
> > +      "high" – High drive strength (~1.2 ns transition time)
> > +      "low"  – Low drive strength (~1.8 ns transition time)
> > +      "min"  – Minimum drive strength (~8 ns transition time)
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: ["max", "high", "low", "min"]
> > +    default: "max"
> > +
> > +  maxim,dvs-slew-rate-mv-per-us:
> 
> ... like here.
> 
> > +    description: |
> > +      Dynamic rising slew rate for output voltage transitions, in mV/μs.
> 
> Except you said mv/us, not us. Confusing.
> 

Would it be acceptable to keep the current name "mv-per-us"?

> > +      This setting is only used when 'maxim,fixed-slew-rate' is not present.
> > +    enum: [5, 10]
> > +    default: 5
> > +
> > +  maxim,en-debounce-time-us:
> > +    description: |
> > +      Debounce time for the enable pin, in microseconds
> > +    enum: [100, 30000]
> > +    default: 100
> > +
> > +  maxim,manual-reset-time-sec:
> > +    description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 

I have applied this from PATCH V5.

> > +      Manual reset time in seconds:
> > +    enum: [4, 8, 12, 16]
> > +    default: 4
> > +
> > +  maxim,en-pullup-disable:
> > +    type: boolean
> > +    description: |
> > +      Disable internal pull-up for EN pin.
> > +      When set, the internal pull-up is disabled.
> > +      Defaults to enabled if this property is not specified.
> 
> This and maybe few more properties look like pinctrl bindings. Maybe
> properties from pincfg are applicable?
> 

The maxim,en-pullup-disable property disables the internal pull-up of the EN pin.
While it looks similar to a pinctrl configuration,
it is actually handled by a device-specific control in the max77675 register,
not through the SoC’s pinctrl subsystem.
So, would it be acceptable to keep the current naming and implementation?

> > +    default: false
> > +
> > +  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.

> > +        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

-- 
Best regards,
Joan Na

Powered by blists - more mailing lists