| Message ID | 20190903080336.32288-4-philippe.schenker@toradex.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add new binding regulator-fixed-clock to regulator-fixed | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
On Tue, Sep 3, 2019 at 9:03 AM Philippe Schenker <philippe.schenker@toradex.com> wrote: > > This adds the documentation to the compatible regulator-fixed-clock Please explain what that is in this patch. > > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > > --- > > .../bindings/regulator/fixed-regulator.yaml | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml b/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml > index a650b457085d..5fd081e80b43 100644 > --- a/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml > +++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml > @@ -19,9 +19,19 @@ description: > allOf: > - $ref: "regulator.yaml#" > > +select: > + properties: > + compatible: > + contains: > + const: regulator-fixed-clock > + required: > + - clocks You don't need this. If you add a new compatible, then this should probably be a new schema doc. Is the 'gpio' property valid in this case as if a clock is the enable, can you also have a gpio enable? That said, it seems like the new compatible is only for validating the DT in the driver. You could just use a clock if present and default to current behavior if not. It's not the kernel's job to validate DTs. > > properties: > compatible: > - const: regulator-fixed > + items: > + - const: regulator-fixed > + - const: regulator-fixed-clock This says you must have 'compatible = "regulator-fixed", "regulator-fixed-clock";'. What you want is: enum: - regulator-fixed - regulator-fixed-clock > regulator-name: true > > @@ -29,6 +39,12 @@ properties: > description: gpio to use for enable control > maxItems: 1 > > + clocks: > + description: > + clock to use for enable control. This binding is only available if > + the compatible is chosen to regulator-fixed-clock. The clock binding > + is mandatory if compatible is chosen to regulator-fixed-clock. Need to define how many clocks (maxItems: 1). > + > startup-delay-us: > description: startup time in microseconds > $ref: /schemas/types.yaml#/definitions/uint32 > -- > 2.23.0 >
On Tue, 2019-09-03 at 09:45 +0100, Rob Herring wrote: > On Tue, Sep 3, 2019 at 9:03 AM Philippe Schenker > <philippe.schenker@toradex.com> wrote: > > This adds the documentation to the compatible regulator-fixed-clock > > Please explain what that is in this patch. Hi Rob and thanks for your comments. I will change this commit message for a possible v2 into this: This adds the documentation to the compatible regulator-fixed-clock. This binding is a special binding of regulator-fixed and adds the ability to add a clock to regulator-fixed, so the regulator can be enabled and disabled with that clock. If the special compatible regulator-fixed-clock is used it is mandatory to supply a clock. > > > > > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > > > > --- > > > > .../bindings/regulator/fixed-regulator.yaml | 18 > > +++++++++++++++++- > > 1 file changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/regulator/fixed- > > regulator.yaml b/Documentation/devicetree/bindings/regulator/fixed- > > regulator.yaml > > index a650b457085d..5fd081e80b43 100644 > > --- a/Documentation/devicetree/bindings/regulator/fixed- > > regulator.yaml > > +++ b/Documentation/devicetree/bindings/regulator/fixed- > > regulator.yaml > > @@ -19,9 +19,19 @@ description: > > allOf: > > - $ref: "regulator.yaml#" > > > > +select: > > + properties: > > + compatible: > > + contains: > > + const: regulator-fixed-clock > > + required: > > + - clocks > > You don't need this. > > If you add a new compatible, then this should probably be a new schema > doc. Is the 'gpio' property valid in this case as if a clock is the > enable, can you also have a gpio enable? That said, it seems like the > new compatible is only for validating the DT in the driver. You could > just use a clock if present and default to current behavior if not. > It's not the kernel's job to validate DTs. The gpio property is valid with this compatible. I added regulator- fixed-clock to regulator-fixed hence I also don't want to create a new schema file. With the above select statement I wanted to state clocks as required when the compatible regulator-fixed-clock is given. > > > properties: > > compatible: > > - const: regulator-fixed > > + items: > > + - const: regulator-fixed > > + - const: regulator-fixed-clock > > This says you must have 'compatible = "regulator-fixed", > "regulator-fixed-clock";'. > > What you want is: > > enum: > - regulator-fixed > - regulator-fixed-clock Thanks, this was exactly what I wanted to do. > > > regulator-name: true > > > > @@ -29,6 +39,12 @@ properties: > > description: gpio to use for enable control > > maxItems: 1 > > > > + clocks: > > + description: > > + clock to use for enable control. This binding is only > > available if > > + the compatible is chosen to regulator-fixed-clock. The clock > > binding > > + is mandatory if compatible is chosen to regulator-fixed- > > clock. > > Need to define how many clocks (maxItems: 1). Will do for a possible v2. I want to wait what Mark Brown says about the addition in general, maybe I have to change all over how this speciality is added into regulator subsystem and therefore also the binding will change. Philippe > > > + > > startup-delay-us: > > description: startup time in microseconds > > $ref: /schemas/types.yaml#/definitions/uint32 > > -- > > 2.23.0 > >
On Wed, Sep 4, 2019 at 9:07 AM Philippe Schenker <philippe.schenker@toradex.com> wrote: > > On Tue, 2019-09-03 at 09:45 +0100, Rob Herring wrote: > > On Tue, Sep 3, 2019 at 9:03 AM Philippe Schenker > > <philippe.schenker@toradex.com> wrote: > > > This adds the documentation to the compatible regulator-fixed-clock > > > > Please explain what that is in this patch. > > Hi Rob and thanks for your comments. I will change this commit message > for a possible v2 into this: > > This adds the documentation to the compatible regulator-fixed-clock. > This binding is a special binding of regulator-fixed and adds the > ability to add a clock to regulator-fixed, so the regulator can be > enabled and disabled with that clock. > If the special compatible regulator-fixed-clock is used it is mandatory > to supply a clock. > > > > > > > > > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> > > > > > > --- > > > > > > .../bindings/regulator/fixed-regulator.yaml | 18 > > > +++++++++++++++++- > > > 1 file changed, 17 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/regulator/fixed- > > > regulator.yaml b/Documentation/devicetree/bindings/regulator/fixed- > > > regulator.yaml > > > index a650b457085d..5fd081e80b43 100644 > > > --- a/Documentation/devicetree/bindings/regulator/fixed- > > > regulator.yaml > > > +++ b/Documentation/devicetree/bindings/regulator/fixed- > > > regulator.yaml > > > @@ -19,9 +19,19 @@ description: > > > allOf: > > > - $ref: "regulator.yaml#" > > > > > > +select: > > > + properties: > > > + compatible: > > > + contains: > > > + const: regulator-fixed-clock > > > + required: > > > + - clocks > > > > You don't need this. > > > > If you add a new compatible, then this should probably be a new schema > > doc. Is the 'gpio' property valid in this case as if a clock is the > > enable, can you also have a gpio enable? That said, it seems like the > > new compatible is only for validating the DT in the driver. You could > > just use a clock if present and default to current behavior if not. > > It's not the kernel's job to validate DTs. > > The gpio property is valid with this compatible. I added regulator- > fixed-clock to regulator-fixed hence I also don't want to create a new > schema file. Okay, if all the other properties are valid then adding to this file is fine. > With the above select statement I wanted to state clocks as required > when the compatible regulator-fixed-clock is given. select is not the right way to do that. select is for whether to apply the schema to a node or not. What you have will silently not apply the schema if 'clocks' is missing or compatible is 'regulator-fixed'. Essentially what you need to do here is: if: properties: compatible: contains: const: regulator-fixed-clock then: required: - clocks Rob
diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml b/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml index a650b457085d..5fd081e80b43 100644 --- a/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml +++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml @@ -19,9 +19,19 @@ description: allOf: - $ref: "regulator.yaml#" +select: + properties: + compatible: + contains: + const: regulator-fixed-clock + required: + - clocks + properties: compatible: - const: regulator-fixed + items: + - const: regulator-fixed + - const: regulator-fixed-clock regulator-name: true @@ -29,6 +39,12 @@ properties: description: gpio to use for enable control maxItems: 1 + clocks: + description: + clock to use for enable control. This binding is only available if + the compatible is chosen to regulator-fixed-clock. The clock binding + is mandatory if compatible is chosen to regulator-fixed-clock. + startup-delay-us: description: startup time in microseconds $ref: /schemas/types.yaml#/definitions/uint32
This adds the documentation to the compatible regulator-fixed-clock Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com> --- .../bindings/regulator/fixed-regulator.yaml | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)