Re: [PATCH v2 1/2] regulator: dt-bindings: rtq2208: Add Richtek RTQ2208 SubPMIC

[Alina Yu <alina_yu@richtek.com>]

On Wed, Jul 05, 2023 at 07:58:53PM +0200, Krzysztof Kozlowski wrote:
> On 05/07/2023 17:27, alina_yu@richtek.com wrote:
> > From: alinayu <alina_yu@richtek.com>
> > 
> > Add bindings for Richtek RTQ2208 IC controlled SubPMIC
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC.  It might happen, that command when run on an older
> kernel, gives you outdated entries.  Therefore please be sure you base
> your patches on recent Linux kernel.
> 
> You missed at least DT list (maybe more), so this won't be tested by our
> tools. Performing review on untested code might be a waste of time, thus
> I will skip this patch entirely till you follow the process allowing the
> patch to be tested.
> 
> Please kindly resend and include all necessary To/Cc entries.
> 
> Limited review follows.
> 

Sorry, I will add all necessary people to CC list to v3.

...

> > +      Buck and ldo vout selection is based on this value.
> > +      There are two independently programmable voltage settings named as mtp-sel0 and
> > +      mtp-sel1 for RTQ2208 bucks vout voltage. 0 which means this property isn't present
> > +      and 1 which means this property is present corresponds to different adjustable registers.
> > +
> > +      0 - DVS0 registers to adjust buck vout and BUCK_[A-H]_EN_NR_MTP_SEL0 register to en/disable vout.
> > +      1 - DVS1 registers to adjust buck vout and BUCK_[A-H]_EN_NR_MTP_SEL1 register to en/disable vout.
> 
> I read it three times and still don't understand. This is bool, not 0/1,
> so are these "0" refer to DVS0 or to presence of the property? Maybe
> Mark will understand it, I don't get it.
> 

Yes, "0" refers to DVS0 registers, and "1" refers to DVS1.
and there is only DVS0 and DVS1, so I use boolean to check which one is used.

Is it more understandable if I modify that to enum ? And description will be like this

richtek,mtp-sel:
  enum: [0, 1]
    description: |
      vout register selection besed on this value.
      0 - Using DVS0 register setting to adjust vout
      1 - Using DVS1 register setting to adjust vout

...

> > +
> > +      regulator-state-(mem):
> 
> That's not a pattern.
>

Should I revise that like this ?

patternProperties:
  "^regulator-state-mem$":


> > +        type: object
> > +        additionalProperties: true
> 
> Why?

Does "additionalProperties: true" mean I need to define my own property ?
If yes, I misunderstood additionalProperties as properties like "regulator-on-in-suspend" or "regulator-mode".

> 
> > +        properties:
> > +          regulator-on-in-suspend: false
> > +          regulator-mode: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - regulators
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      rtq2208@10 {
> 
> Node names should be generic. See also explanation and list of examples
> in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 

I'll modify the node name to 

regulator@10

in v3

> > +        compatible = "richtek,rtq2208";
> > +        reg = <0x10>;
> > +        interrupts-extended = <&gpio26 0 IRQ_TYPE_LEVEL_LOW>;
> > +        richtek,mtp-sel;
> > +
> > +        regulators {
> > +         BUCK_A:buck_A {
> 
> Drop labels.
> 
> 

BR,
Alina