Re: [PATCH 3/3] dt-bindings: regulator: add regulator-fixed-clock binding
On Wed, Sep 4, 2019 at 9:07 AM Philippe Schenker wrote: > > On Tue, 2019-09-03 at 09:45 +0100, Rob Herring wrote: > > On Tue, Sep 3, 2019 at 9:03 AM Philippe Schenker > > 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 > > > > > > --- > > > > > > .../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
Re: [PATCH 3/3] dt-bindings: regulator: add regulator-fixed-clock binding
On Tue, 2019-09-03 at 09:45 +0100, Rob Herring wrote: > On Tue, Sep 3, 2019 at 9:03 AM Philippe Schenker > 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 > > > > --- > > > > .../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 > >
Re: [PATCH 3/3] dt-bindings: regulator: add regulator-fixed-clock binding
On Tue, Sep 3, 2019 at 9:03 AM Philippe Schenker wrote: > > This adds the documentation to the compatible regulator-fixed-clock Please explain what that is in this patch. > > Signed-off-by: Philippe Schenker > > --- > > .../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 >
