Re: [PATCH v2 05/11] regulator: dt-bindings: mediatek: Add MT6366 PMIC

[Chen-Yu Tsai]

On Wed, Aug 23, 2023 at 3:40 AM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 22/08/2023 21:39, Krzysztof Kozlowski wrote:
> > On 22/08/2023 10:45, Chen-Yu Tsai wrote:
> >> From: Zhiyong Tao <zhiyong.tao@xxxxxxxxxxxx>
> >>
> >> The MediaTek MT6366 PMIC is similar to the MT6358 PMIC. It is designed
> >> to be paired with the MediaTek MT8186 SoC. It has 9 buck regulators and
> >> 29 LDO regulators, not counting ones that feed internally and basically
> >> have no controls. The regulators are named after their intended usage
> >> for the SoC and system design, thus not named generically as ldoX or
> >> dcdcX, but as vcn33 or vgpu.
> >>
> >> Add a binding document describing all the regulators and their supplies.
> >>
> >> Signed-off-by: Zhiyong Tao <zhiyong.tao@xxxxxxxxxxxx>
> >> [wens@xxxxxxxxxxxx: major rework and added commit message]
> >> Signed-off-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
> >> ---
> >> Changes since v1:
> >> - Replaced underscores in supply names to hyphens
> >> - Merged with MT6358 regulator binding
> >> - Added MT6358 fallback compatible to MT6366 regulator
> >>
> >> Changes since Zhiyong's last version (v4) [1]:
> >> - simplified regulator names
> >> - added descriptions to regulators
> >> - removed bogus regulators (*_sshub)
> >> - merged vcn33-wifi and vcn33-bt as vcn33
> >> - added missing regulators (vm18, vmddr, vsram-core)
> >> - cut down examples to a handful of cases and made them complete
> >> - expanded commit message a lot
> >>
> >> [1] https://lore.kernel.org/linux-arm-kernel/20220823123745.14061-1-zhiyong.tao@xxxxxxxxxxxx/
> >>  .../regulator/mediatek,mt6358-regulator.yaml  | 227 +++++++++++++-----
> >>  1 file changed, 168 insertions(+), 59 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/regulator/mediatek,mt6358-regulator.yaml b/Documentation/devicetree/bindings/regulator/mediatek,mt6358-regulator.yaml
> >> index 82328fe17680..b350181f33ff 100644
> >> --- a/Documentation/devicetree/bindings/regulator/mediatek,mt6358-regulator.yaml
> >> +++ b/Documentation/devicetree/bindings/regulator/mediatek,mt6358-regulator.yaml
> >> @@ -16,14 +16,18 @@ description: |
> >>
> >>  properties:
> >>    compatible:
> >> -    const: mediatek,mt6358-regulator
> >> +    oneOf:
> >> +      - const: mediatek,mt6358-regulator
> >> +      - items:
> >> +          - const: mediatek,mt6366-regulator
> >> +          - const: mediatek,mt6358-regulator
> >>
> >>    vsys-ldo1-supply:
> >>      description: Supply for LDOs vfe28, vxo22, vcn28, vaux18, vaud28, vsim1, vusb, vbif28
> >>    vsys-ldo2-supply:
> >> -    description: Supply for LDOs vldo28, vio28, vmc, vmch, vsim2
> >> +    description: Supply for LDOs vldo28 (MT6358 only), vio28, vmc, vmch, vsim2
> >>    vsys-ldo3-supply:
> >> -    description: Supply for LDOs vcn33, vcama1, vcama2, vemc, vibr
> >> +    description: Supply for LDOs vcn33, vcama[12] (MT6358 only), vemc, vibr
> >>    vsys-vcore-supply:
> >>      description: Supply for buck regulator vcore
> >>    vsys-vdram1-supply:
> >> @@ -43,75 +47,138 @@ properties:
> >>    vsys-vs2-supply:
> >>      description: Supply for buck regulator vs2
> >>    vs1-ldo1-supply:
> >> -    description: Supply for LDOs vrf18, vefuse, vcn18, vcamio, vio18
> >> +    description: Supply for LDOs vrf18, vefuse, vcn18, vcamio (MT6358 only), vio18
> >>    vs2-ldo1-supply:
> >> -    description: Supply for LDOs vdram2
> >> +    description: Supply for LDOs vdram2, vmddr (MT6366 only)
> >>    vs2-ldo2-supply:
> >>      description: Supply for LDOs vrf12, va12
> >>    vs2-ldo3-supply:
> >> -    description: Supply for LDOs vsram-gpu, vsram-others, vsram-proc11, vsram-proc12
> >> -  vs2-ldo4-supply:
> >> -    description: Supply for LDO vcamd
> >> -
> >> -patternProperties:
> >> -  "^buck_v(core|dram1|gpu|modem|pa|proc1[12]|s[12])$":
> >> -    description: Buck regulators
> >> -    type: object
> >> -    $ref: regulator.yaml#
> >> -    unevaluatedProperties: false
> >> -
> >> -  "^ldo_v(a|rf)12":
> >> -    description: LDOs with fixed 1.2V output and 0~100/10mV tuning
> >> -    type: object
> >> -    $ref: regulator.yaml#
> >> -    unevaluatedProperties: false
> >> -
> >> -  "^ldo_v((aux|cn|io|rf)18|camio)":
> >> -    description: LDOs with fixed 1.8V output and 0~100/10mV tuning
> >> -    type: object
> >> -    $ref: regulator.yaml#
> >> -    unevaluatedProperties: false
> >> -
> >> -  "^ldo_vxo22":
> >> -    description: LDOs with fixed 2.2V output and 0~100/10mV tuning
> >> -    type: object
> >> -    $ref: regulator.yaml#
> >> -    unevaluatedProperties: false
> >> -
> >> -  "^ldo_v(aud|bif|cn|fe|io)28":
> >> -    description: LDOs with fixed 2.8V output and 0~100/10mV tuning
> >> -    type: object
> >> -    $ref: regulator.yaml#
> >> -    unevaluatedProperties: false
> >> -
> >> -  "^ldo_vusb":
> >> -    description: LDOs with fixed 3.0V output and 0~100/10mV tuning
> >> -    type: object
> >> -    $ref: regulator.yaml#
> >> -    unevaluatedProperties: false
> >> -
> >> -  "^ldo_vsram_(gpu|others|proc1[12])$":
> >> -    description: LDOs with variable output
> >> -    type: object
> >> -    $ref: regulator.yaml#
> >> -    unevaluatedProperties: false
> >> -
> >> -  "^ldo_v(cama[12]|camd|cn33|dram2|efuse|emc|ibr|ldo28|mc|mch|sim[12])$":
> >> -    description: LDOs with variable output and 0~100/10mV tuning
> >> -    type: object
> >> -    $ref: regulator.yaml#
> >> -    unevaluatedProperties: false
> >
> > I don't understand. You just added it and it is already wrong? Please,
> > do not add code which is clearly incorrect.
>
> Sent too early - anyway properties cannot be defined in allOf:. That's
> not the place for them and there is no single reason for it. From which
> regulator binding you got this example?

None. It was simply a way I figured out when I was reading up on JSON
schema syntax. I wanted to split the definitions cleanly, since they
are very different. And with "unevaluatedProperties: false" in the base
schema it did seem to work, successfully evaluating existing device trees
and producing errors when extra properties were added, or if types didn't
match up.

Now that you mention it, I suppose the preferred way to write it is to
have all the properties in the base schema, then negate the ones that
don't belong in the allOf: section? It just seems really repetitive given
the child node names for the chip variants are completely different. OOTH
I guess it would produce better error messages.


ChenYu