[v3,2/4] dt-bindings: regulator: add document bindings for mpq7920

Add device tree binding information for mpq7920 regulator driver.
Example bindings for mpq7920 are added.

Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
---
 .../bindings/regulator/mpq7920.yaml           | 143 ++++++++++++++++++
 1 file changed, 143 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/mpq7920.yaml

On Sun, Dec 22, 2019 at 09:45:05PM +0100, Saravanan Sekar wrote:
> Add device tree binding information for mpq7920 regulator driver.
> Example bindings for mpq7920 are added.
>
> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> ---
>  .../bindings/regulator/mpq7920.yaml           | 143 ++++++++++++++++++
>  1 file changed, 143 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/mpq7920.yaml
>
> diff --git a/Documentation/devicetree/bindings/regulator/mpq7920.yaml b/Documentation/devicetree/bindings/regulator/mpq7920.yaml
> new file mode 100644
> index 000000000000..d173ba1fb28d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/mpq7920.yaml
> @@ -0,0 +1,143 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/mpq7920.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Monolithic Power System MPQ7920 PMIC
> +
> +maintainers:
> +  - Saravanan Sekar <sravanhome@gmail.com>
> +
> +properties:
> +  $nodename:
> +    pattern: "pmic@[0-9a-f]{1,2}"
> +  compatible:
> +    enum:
> +      - mps,mpq7920
> +
> +  reg:
> +    maxItems: 1
> +
> +  mps,time-slot:
> +    description:
> +      each regulator output shall be delayed during power on/off sequence which
> +      based on configurable time slot value, must be one of following corresponding
> +      value 0.5ms, 2ms, 8ms, 16ms
> +    allOf:
> +      - $ref: "/schemas/types.yaml#/definitions/uint8"
> +      - enum: [ 0, 1, 2, 3 ]
> +      - default: 0
> +
> +  mps,fixed-on-time:
> +     description:
> +       select power on sequence with fixed time output delay mentioned in
> +       time-slot reg for all the regulators.
> +     type: boolean
> +
> +  mps,fixed-off-time:
> +     description:
> +        select power off sequence with fixed time output delay mentioned in
> +        time-slot reg for all the regulators.
> +     type: boolean

I'm not sure what this fixed-on-time and fixed-off-time property is
supposed to be doing. Why not just get rid of the time slot property,
and set the power on / power off time in fixed-on-time /
fixed-off-time property?

> +  mps,inc-off-time:
> +     description: |
> +        mutually exclusive to mps,fixed-off-time an array of 8, linearly increase
> +        output delay during power off sequence based on factor of time slot/interval
> +        for each regulator.
> +     allOf:
> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       - minimum: 0
> +       - maximum: 15
> +       - default: [ 0, 6, 0, 6, 7, 7, 7, 9 ]

You should check the size of the array too, but if it's a property of
the regulators, why not have it in the regulators node?

> +  mps,inc-on-time:
> +     description: |
> +        mutually exclusive to mps,fixed-on-time an array of 8, linearly increase
> +        output delay during power on sequence based on factor of time slot/interval
> +        for each regulator.
> +     allOf:
> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       - minimum: 0
> +       - maximum: 15
> +       - default: [ 0, 6, 0, 6, 7, 7, 7, 9 ]
> +
> +  mps,switch-freq:
> +     description: |
> +        switching frequency must be one of following corresponding value
> +        1.1MHz, 1.65MHz, 2.2MHz, 2.75MHz
> +     allOf:
> +       - $ref: "/schemas/types.yaml#/definitions/uint8"
> +       - enum: [ 0, 1, 2, 3 ]
> +       - default: 2
> +
> +  mps,buck-softstart:
> +     description: |
> +        An array of 4 contains soft start time of each buck, must be one of
> +        following corresponding values 150us, 300us, 610us, 920us
> +     allOf:
> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       - enum: [ 0, 1, 2, 3 ]
> +       - default: [ 1, 1, 1, 1 ]
> +
> +  mps,buck-ovp:
> +     description: |
> +        An array of 4 contains over voltage protection of each buck, must be
> +        one of above values
> +     allOf:
> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       - enum: [ 0, 1 ]
> +       - default: [ 1, 1, 1, 1 ]
> +
> +  mps,buck-phase-delay:
> +     description: |
> +        An array of 4 contains phase delay of each buck must be one of above values
> +        corresponding to 0deg, 90deg, 180deg, 270deg
> +     allOf:
> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       - enum: [ 0, 1, 2, 3 ]
> +       - default: [ 0, 0, 1, 1 ]
> +
> +  regulators:
> +    type: object
> +    description:
> +      list of regulators provided by this controller, must be named
> +      after their hardware counterparts BUCK[1-4], one LDORTC, and LDO[2-5]
> +      The valid names for regulators are
> +      buck1, buck2, buck3, buck4, ldortc, ldo2, ldo3, ldo4, ldo5

For the third times now, the names should be validated using
propertyNames.

Maxime
>

On 23/12/19 11:50 am, Maxime Ripard wrote:

> On Sun, Dec 22, 2019 at 09:45:05PM +0100, Saravanan Sekar wrote:
>> Add device tree binding information for mpq7920 regulator driver.
>> Example bindings for mpq7920 are added.
>>
>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>> ---
>>   .../bindings/regulator/mpq7920.yaml           | 143 ++++++++++++++++++
>>   1 file changed, 143 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/regulator/mpq7920.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/mpq7920.yaml b/Documentation/devicetree/bindings/regulator/mpq7920.yaml
>> new file mode 100644
>> index 000000000000..d173ba1fb28d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/mpq7920.yaml
>> @@ -0,0 +1,143 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/mpq7920.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Monolithic Power System MPQ7920 PMIC
>> +
>> +maintainers:
>> +  - Saravanan Sekar <sravanhome@gmail.com>
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: "pmic@[0-9a-f]{1,2}"
>> +  compatible:
>> +    enum:
>> +      - mps,mpq7920
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  mps,time-slot:
>> +    description:
>> +      each regulator output shall be delayed during power on/off sequence which
>> +      based on configurable time slot value, must be one of following corresponding
>> +      value 0.5ms, 2ms, 8ms, 16ms
>> +    allOf:
>> +      - $ref: "/schemas/types.yaml#/definitions/uint8"
>> +      - enum: [ 0, 1, 2, 3 ]
>> +      - default: 0
>> +
>> +  mps,fixed-on-time:
>> +     description:
>> +       select power on sequence with fixed time output delay mentioned in
>> +       time-slot reg for all the regulators.
>> +     type: boolean
>> +
>> +  mps,fixed-off-time:
>> +     description:
>> +        select power off sequence with fixed time output delay mentioned in
>> +        time-slot reg for all the regulators.
>> +     type: boolean
> I'm not sure what this fixed-on-time and fixed-off-time property is

the time slot register holds the time interval of Vout when enable the
each regulator.
fixed-on-time property is to specify each regulator shares same time
interval mention in time slot register.
variable on-time defines the factor or multiples of value in time slot
register.


> supposed to be doing. Why not just get rid of the time slot property,
> and set the power on / power off time in fixed-on-time /
> fixed-off-time property?

1. if fixed-on-time is needed with default time slot register settings,
then time slot property is not needed
2. if variable time is needed with modified time slot, then both
variable time factor & time slot property is needed,
Hope above explanations answers the necessary of all the above property

>
>> +  mps,inc-off-time:
>> +     description: |
>> +        mutually exclusive to mps,fixed-off-time an array of 8, linearly increase
>> +        output delay during power off sequence based on factor of time slot/interval
>> +        for each regulator.
>> +     allOf:
>> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
>> +       - minimum: 0
>> +       - maximum: 15
>> +       - default: [ 0, 6, 0, 6, 7, 7, 7, 9 ]
> You should check the size of the array too, but if it's a property of
> the regulators, why not have it in the regulators node?

the node regulators & sub-node of regulators are parsed (initdata) by
regulator framework during regulator registration,
so it would be redundant parsing all the node if mentioned under each
regulator node and this is optional. If you still not
convinced I will change.

>> +  mps,inc-on-time:
>> +     description: |
>> +        mutually exclusive to mps,fixed-on-time an array of 8, linearly increase
>> +        output delay during power on sequence based on factor of time slot/interval
>> +        for each regulator.
>> +     allOf:
>> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
>> +       - minimum: 0
>> +       - maximum: 15
>> +       - default: [ 0, 6, 0, 6, 7, 7, 7, 9 ]
>> +
>> +  mps,switch-freq:
>> +     description: |
>> +        switching frequency must be one of following corresponding value
>> +        1.1MHz, 1.65MHz, 2.2MHz, 2.75MHz
>> +     allOf:
>> +       - $ref: "/schemas/types.yaml#/definitions/uint8"
>> +       - enum: [ 0, 1, 2, 3 ]
>> +       - default: 2
>> +
>> +  mps,buck-softstart:
>> +     description: |
>> +        An array of 4 contains soft start time of each buck, must be one of
>> +        following corresponding values 150us, 300us, 610us, 920us
>> +     allOf:
>> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
>> +       - enum: [ 0, 1, 2, 3 ]
>> +       - default: [ 1, 1, 1, 1 ]
>> +
>> +  mps,buck-ovp:
>> +     description: |
>> +        An array of 4 contains over voltage protection of each buck, must be
>> +        one of above values
>> +     allOf:
>> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
>> +       - enum: [ 0, 1 ]
>> +       - default: [ 1, 1, 1, 1 ]
>> +
>> +  mps,buck-phase-delay:
>> +     description: |
>> +        An array of 4 contains phase delay of each buck must be one of above values
>> +        corresponding to 0deg, 90deg, 180deg, 270deg
>> +     allOf:
>> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
>> +       - enum: [ 0, 1, 2, 3 ]
>> +       - default: [ 0, 0, 1, 1 ]
>> +
>> +  regulators:
>> +    type: object
>> +    description:
>> +      list of regulators provided by this controller, must be named
>> +      after their hardware counterparts BUCK[1-4], one LDORTC, and LDO[2-5]
>> +      The valid names for regulators are
>> +      buck1, buck2, buck3, buck4, ldortc, ldo2, ldo3, ldo4, ldo5
> For the third times now, the names should be validated using
> propertyNames.

Not sure did I understand your question correctly.
all the node name under regulators node are parsed by regulator
framework & validated against
name in regulator descriptors.

>
> Maxime


Thanks,

Saravanan

On 23/12/19 11:50 am, Maxime Ripard wrote:
> On Sun, Dec 22, 2019 at 09:45:05PM +0100, Saravanan Sekar wrote:
>> Add device tree binding information for mpq7920 regulator driver.
>> Example bindings for mpq7920 are added.
>>
>> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
>> ---
>>   .../bindings/regulator/mpq7920.yaml           | 143 ++++++++++++++++++
>>   1 file changed, 143 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/regulator/mpq7920.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/mpq7920.yaml b/Documentation/devicetree/bindings/regulator/mpq7920.yaml
>> new file mode 100644
>> index 000000000000..d173ba1fb28d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/mpq7920.yaml
>> @@ -0,0 +1,143 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/mpq7920.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Monolithic Power System MPQ7920 PMIC
>> +
>> +maintainers:
>> +  - Saravanan Sekar <sravanhome@gmail.com>
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: "pmic@[0-9a-f]{1,2}"
>> +  compatible:
>> +    enum:
>> +      - mps,mpq7920
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  mps,time-slot:
>> +    description:
>> +      each regulator output shall be delayed during power on/off sequence which
>> +      based on configurable time slot value, must be one of following corresponding
>> +      value 0.5ms, 2ms, 8ms, 16ms
>> +    allOf:
>> +      - $ref: "/schemas/types.yaml#/definitions/uint8"
>> +      - enum: [ 0, 1, 2, 3 ]
>> +      - default: 0
>> +
>> +  mps,fixed-on-time:
>> +     description:
>> +       select power on sequence with fixed time output delay mentioned in
>> +       time-slot reg for all the regulators.
>> +     type: boolean
>> +
>> +  mps,fixed-off-time:
>> +     description:
>> +        select power off sequence with fixed time output delay mentioned in
>> +        time-slot reg for all the regulators.
>> +     type: boolean
> I'm not sure what this fixed-on-time and fixed-off-time property is
> supposed to be doing. Why not just get rid of the time slot property,
> and set the power on / power off time in fixed-on-time /
> fixed-off-time property?
>
>> +  mps,inc-off-time:
>> +     description: |
>> +        mutually exclusive to mps,fixed-off-time an array of 8, linearly increase
>> +        output delay during power off sequence based on factor of time slot/interval
>> +        for each regulator.
>> +     allOf:
>> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
>> +       - minimum: 0
>> +       - maximum: 15
>> +       - default: [ 0, 6, 0, 6, 7, 7, 7, 9 ]
> You should check the size of the array too, but if it's a property of
> the regulators, why not have it in the regulators node?
>
>> +  mps,inc-on-time:
>> +     description: |
>> +        mutually exclusive to mps,fixed-on-time an array of 8, linearly increase
>> +        output delay during power on sequence based on factor of time slot/interval
>> +        for each regulator.
>> +     allOf:
>> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
>> +       - minimum: 0
>> +       - maximum: 15
>> +       - default: [ 0, 6, 0, 6, 7, 7, 7, 9 ]
>> +
>> +  mps,switch-freq:
>> +     description: |
>> +        switching frequency must be one of following corresponding value
>> +        1.1MHz, 1.65MHz, 2.2MHz, 2.75MHz
>> +     allOf:
>> +       - $ref: "/schemas/types.yaml#/definitions/uint8"
>> +       - enum: [ 0, 1, 2, 3 ]
>> +       - default: 2
>> +
>> +  mps,buck-softstart:
>> +     description: |
>> +        An array of 4 contains soft start time of each buck, must be one of
>> +        following corresponding values 150us, 300us, 610us, 920us
>> +     allOf:
>> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
>> +       - enum: [ 0, 1, 2, 3 ]
>> +       - default: [ 1, 1, 1, 1 ]
>> +
>> +  mps,buck-ovp:
>> +     description: |
>> +        An array of 4 contains over voltage protection of each buck, must be
>> +        one of above values
>> +     allOf:
>> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
>> +       - enum: [ 0, 1 ]
>> +       - default: [ 1, 1, 1, 1 ]
>> +
>> +  mps,buck-phase-delay:
>> +     description: |
>> +        An array of 4 contains phase delay of each buck must be one of above values
>> +        corresponding to 0deg, 90deg, 180deg, 270deg
>> +     allOf:
>> +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
>> +       - enum: [ 0, 1, 2, 3 ]
>> +       - default: [ 0, 0, 1, 1 ]
>> +
>> +  regulators:
>> +    type: object
>> +    description:
>> +      list of regulators provided by this controller, must be named
>> +      after their hardware counterparts BUCK[1-4], one LDORTC, and LDO[2-5]
>> +      The valid names for regulators are
>> +      buck1, buck2, buck3, buck4, ldortc, ldo2, ldo3, ldo4, ldo5
> For the third times now, the names should be validated using
> propertyNames.
>
> Maxime

On Thu, Dec 26, 2019 at 11:23:36PM +0100, saravanan sekar wrote:
> On 23/12/19 11:50 am, Maxime Ripard wrote:
> > On Sun, Dec 22, 2019 at 09:45:05PM +0100, Saravanan Sekar wrote:

> > > +  mps,fixed-off-time:
> > > +     description:
> > > +        select power off sequence with fixed time output delay mentioned in
> > > +        time-slot reg for all the regulators.
> > > +     type: boolean

> > I'm not sure what this fixed-on-time and fixed-off-time property is

> the time slot register holds the time interval of Vout when enable the
> each regulator.
> fixed-on-time property is to specify each regulator shares same time
> interval mention in time slot register.
> variable on-time defines the factor or multiples of value in time slot
> register.

This really isn't very clear from the bindings document.  You
need an explanation like the above in there.

> > supposed to be doing. Why not just get rid of the time slot property,
> > and set the power on / power off time in fixed-on-time /
> > fixed-off-time property?

> 1. if fixed-on-time is needed with default time slot register settings,
> then time slot property is not needed
> 2. if variable time is needed with modified time slot, then both
> variable time factor & time slot property is needed,
> Hope above explanations answers the necessary of all the above property

Same here, though I'm still a bit unclear about what "needed with
modified time slot" means.

> > > +  regulators:
> > > +    type: object
> > > +    description:
> > > +      list of regulators provided by this controller, must be named
> > > +      after their hardware counterparts BUCK[1-4], one LDORTC, and LDO[2-5]
> > > +      The valid names for regulators are
> > > +      buck1, buck2, buck3, buck4, ldortc, ldo2, ldo3, ldo4, ldo5

> > For the third times now, the names should be validated using
> > propertyNames.

> Not sure did I understand your question correctly.
> all the node name under regulators node are parsed by regulator
> framework & validated against
> name in regulator descriptors.

That validates at kernel runtime but doesn't let bindings
validation at the time the DTS is built verify things, Maxime is
asking you to spell things out in the DT binding document so the
DT can be validated independently of the kernel.

On Thu, Dec 26, 2019 at 11:23:36PM +0100, saravanan sekar wrote:
> > > +  mps,inc-off-time:
> > > +     description: |
> > > +        mutually exclusive to mps,fixed-off-time an array of 8, linearly increase
> > > +        output delay during power off sequence based on factor of time slot/interval
> > > +        for each regulator.
> > > +     allOf:
> > > +       - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> > > +       - minimum: 0
> > > +       - maximum: 15
> > > +       - default: [ 0, 6, 0, 6, 7, 7, 7, 9 ]
> > You should check the size of the array too, but if it's a property of
> > the regulators, why not have it in the regulators node?
>
> the node regulators & sub-node of regulators are parsed (initdata) by
> regulator framework during regulator registration,
> so it would be redundant parsing all the node if mentioned under each
> regulator node and this is optional. If you still not
> convinced I will change.

It's not really redundant, since the regulator framework will ignore
whatever custom property you would put there, and your driver would
ignore any generic property in those nodes.

> > > +  regulators:
> > > +    type: object
> > > +    description:
> > > +      list of regulators provided by this controller, must be named
> > > +      after their hardware counterparts BUCK[1-4], one LDORTC, and LDO[2-5]
> > > +      The valid names for regulators are
> > > +      buck1, buck2, buck3, buck4, ldortc, ldo2, ldo3, ldo4, ldo5
> > For the third times now, the names should be validated using
> > propertyNames.
>
> Not sure did I understand your question correctly.
> all the node name under regulators node are parsed by regulator
> framework & validated against
> name in regulator descriptors.

Yes, and the point of the bindings in YAML is to make sure all the
constraints we might have can be catched at compilation / validation
time.

The names of the nodes are a constraint, and propertyNames allows you
to express it.

Maxime