Re: [PATCH v10 1/9] dt-bindings: regulator: Document MediaTek MT6316 PMIC Regulators

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 24/10/25 18:29, Conor Dooley ha scritto:
> On Fri, Oct 24, 2025 at 10:32:13AM +0200, AngeloGioacchino Del Regno wrote:
>> Add bindings for the regulators found in the MediaTek MT6316 PMIC,
>> usually found in board designs using the MT6991 Dimensity 9400 and
>> on MT8196 Kompanio SoC for Chromebooks.
>>
>> This chip is fully controlled by SPMI and has multiple variants
>> providing different phase configurations.
>>
>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   .../regulator/mediatek,mt6316b-regulator.yaml | 78 +++++++++++++++++++
>>   .../regulator/mediatek,mt6316c-regulator.yaml | 78 +++++++++++++++++++
>>   .../regulator/mediatek,mt6316d-regulator.yaml | 77 ++++++++++++++++++
>>   3 files changed, 233 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/regulator/mediatek,mt6316b-regulator.yaml
>>   create mode 100644 Documentation/devicetree/bindings/regulator/mediatek,mt6316c-regulator.yaml
>>   create mode 100644 Documentation/devicetree/bindings/regulator/mediatek,mt6316d-regulator.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/mediatek,mt6316b-regulator.yaml b/Documentation/devicetree/bindings/regulator/mediatek,mt6316b-regulator.yaml
>> new file mode 100644
>> index 000000000000..65b70dd90728
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/mediatek,mt6316b-regulator.yaml
>> @@ -0,0 +1,78 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/mediatek,mt6316b-regulator.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MediaTek MT6316 BP/VP SPMI PMIC Regulators
>> +
>> +maintainers:
>> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> +
>> +description:
>> +  The MediaTek MT6316BP/VP PMICs are fully controlled by SPMI interface, both
>> +  feature four step-down DC/DC (buck) converters, and provides 2+2 Phases,
>> +  joining Buck 1+2 for the first phase, and Buck 3+4 for the second phase.
>> +
>> +properties:
>> +  compatible:
>> +    const: mediatek,mt6316b-regulator
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +patternProperties:
>> +  "^vbuck(12|34)$":
>> +    type: object
>> +    $ref: regulator.yaml#
>> +    unevaluatedProperties: false
>> +    properties:
>> +      regulator-allowed-modes:
>> +        description: |
>> +          Allowed Buck regulator operating modes allowed. Valid values below.
>> +            0 - Normal mode with automatic power saving, reducing the switching
>> +                frequency when light load conditions are detected
>> +            1 - Forced Continuous Conduction mode (FCCM) for improved voltage
>> +                regulation accuracy with constant switching frequency but lower
>> +                regulator efficiency
>> +            2 - Forced Low Power mode for improved regulator efficiency, used
>> +                when no heavy load is expected, will shut down unnecessary IP
>> +                blocks and secondary phases to reduce quiescent current.
>> +                This mode does not limit the maximum output current but unless
>> +                only a light load is applied, there will be regulation accuracy
>> +                and efficiency losses.
>> +        minItems: 1
>> +        maxItems: 3
>> +        items:
>> +          enum: [ 0, 1, 2 ]
> 
> This property has no default, and the property is not required. Is one
> of these modes the default, or is there another mode beyond what's here
> that is used if the property is absent? Or are all modes allowed with no
> property?
> 

The default is what the bootloader sets before jumping to the kernel... this may
vary from one to the other.

Even though "in theory" the default should be 0, I can't guarantee that this will
really be the default for when Linux boots.... and "resetting" is not possible
(either as a real reset or forcing a default) because those are CPU regulators
and if anything goes wrong the CPUs may freeze execution.

It should be kinda safe to force the default to 0 at boot, but I'm not sure that
I should really take that assumption for granted - I prefer leaving this untouched
and without any "forced" default unless DT specifies some.

So, well, that's why there's no default, and that's why this is not a required
property at the end of the day.

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - '#address-cells'
> 
> Why is address-cells required here? Your bucks don't have addresses.

Ugh. I forgot it there. No, that's not intentional.

The #address-cells makes no sense here.

> If it is actually required, Rob's bot has pointed out that the property
> isn't defined for the device anyway.
> 
> pw-bot: changes-requested
> 
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/spmi/spmi.h>
>> +
>> +    spmi {
>> +      #address-cells = <2>;
>> +      #size-cells = <0>;
>> +
>> +      pmic@8 {
>> +        compatible = "mediatek,mt6316b-regulator";
>> +        reg = <0x8 SPMI_USID>;
>> +        #address-cells = <0>;
>> +
>> +        vbuck12 {
>> +          regulator-name = "dvdd_core";
>> +          regulator-min-microvolt = <450000>;
>> +          regulator-max-microvolt = <965000>;
>> +          regulator-allowed-modes = <0 1 2>;
>> +          regulator-enable-ramp-delay = <256>;
>> +        };
>> +      };
>> +    };
>> +...