On Tue, Oct 17, 2023 at 09:35:48AM +0200, Ante Knezic wrote:
> > > + microchip,rmii-clk-internal:
> > > + $ref: /schemas/types.yaml#/definitions/flag
> > > + description:
> > > + Set if the RMII reference clock is provided internally. Otherwise
> > > + reference clock should be provided externally.
> > > +
> > > +if:
> > > + not:
> > > + properties:
> > > + compatible:
> > > + enum:
> > > + - microchip,ksz8863
> > > + - microchip,ksz8873
> > > +then:
> > > + not:
> > > + required:
> > > + - microchip,rmii-clk-internal
I think this bit can become the slightly simpler
then:
properties:
microchip,rmii-clk-internal: false
> > I think that what you want to express is that microchip,rmii-clk-internal
> > is only defined for microchip,ksz8863 and microchip,ksz8873.
> > Can't you describe that as "if: properties: compatible: (...) then:
> > properties: microchip,rmii-clk-internal"?
>
> If I understood you correctly you are refering to a solution like
> if:
> properties:
> compatible:
> enum:
> - microchip,ksz8863
> - microchip,ksz8873
> then:
> properties:
> microchip,rmii-clk-internal:
> $ref: /schemas/types.yaml#/definitions/flag
> description:
> Set if the RMII reference clock is provided internally. Otherwise
> reference clock should be provided externally.
>
> This was already suggested in v1, but was not a satisfactory solution
> according to Mr. Conor Dooley:
Yeah, we prefer not to have the property definitions inside the
conditionals, but rather constrain or allow/disallow them there.
Cheers,
Conor.
>
> >> On Tue, 10 Oct 2023 16:25:55 +0100, Conor Dooley wrote:
> >> > On Tue, Oct 10, 2023 at 03:18:54PM +0200, Ante Knezic wrote:
> >> > > Add documentation for selecting reference rmii clock on KSZ88X3 devices
> >> > >
> >> > > Signed-off-by: Ante Knezic <[email protected]>
> >> > > ---
> >> > > Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 6 ++++++
> >> > > 1 file changed, 6 insertions(+)
> >> > >
> >> > > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> >> > > index e51be1ac0362..3df5d2e72dba 100644
> >> > > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> >> > > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> >> > > @@ -49,6 +49,12 @@ properties:
> >> > > Set if the output SYNCLKO clock should be disabled. Do not mix with
> >> > > microchip,synclko-125.
> >> > >
> >> > > + microchip,rmii-clk-internal:
> >> > > + $ref: /schemas/types.yaml#/definitions/flag
> >> > > + description:
> >> > > + Set if the RMII reference clock should be provided internally.
> >> >
> >> > > Applies only
> >> > > + to KSZ88X3 devices.
> >> >
> >> > This should be enforced by the schema, the example schema in the docs
> >> > should show you how to do this.
> >>
> >> I am guessing you are refering to limiting the property to ksz88x3 devices?
> >> Something like:
> >>
> >> if:
> >> properties:
> >> compatible:
> >> enum:
> >> - microchip,ksz8863
> >> - microchip,ksz8873
> >> then:
> >> properties:
> >> microchip,rmii-clk-internal:
> >> $ref: /schemas/types.yaml#/definitions/flag
> >> description:
> >> Set if the RMII reference clock is provided internally. Otherwise
> >> reference clock should be provided externally.
> >
> >Not quite. The definition of the property should be outside the if/then,
> >but one should be used to allow/disallow the property.
>
On Tue, Oct 17, 2023 at 08:48:27AM +0100, Conor Dooley wrote:
> On Tue, Oct 17, 2023 at 09:35:48AM +0200, Ante Knezic wrote:
> > > > + microchip,rmii-clk-internal:
> > > > + $ref: /schemas/types.yaml#/definitions/flag
> > > > + description:
> > > > + Set if the RMII reference clock is provided internally. Otherwise
> > > > + reference clock should be provided externally.
> > > > +
> > > > +if:
> > > > + not:
> > > > + properties:
> > > > + compatible:
> > > > + enum:
> > > > + - microchip,ksz8863
> > > > + - microchip,ksz8873
> > > > +then:
> > > > + not:
> > > > + required:
> > > > + - microchip,rmii-clk-internal
>
> I think this bit can become the slightly simpler
> then:
> properties:
> microchip,rmii-clk-internal: false
This looks better. I don't understand how the original formulation worked
("not: required:" when the property was never "required" in the first
place - does that do anything?), but I understand how this one does.
> > > I think that what you want to express is that microchip,rmii-clk-internal
> > > is only defined for microchip,ksz8863 and microchip,ksz8873.
> > > Can't you describe that as "if: properties: compatible: (...) then:
> > > properties: microchip,rmii-clk-internal"?
> >
> > If I understood you correctly you are refering to a solution like
> > if:
> > properties:
> > compatible:
> > enum:
> > - microchip,ksz8863
> > - microchip,ksz8873
> > then:
> > properties:
> > microchip,rmii-clk-internal:
> > $ref: /schemas/types.yaml#/definitions/flag
> > description:
> > Set if the RMII reference clock is provided internally. Otherwise
> > reference clock should be provided externally.
> >
> > This was already suggested in v1, but was not a satisfactory solution
> > according to Mr. Conor Dooley:
>
> Yeah, we prefer not to have the property definitions inside the
> conditionals, but rather constrain or allow/disallow them there.
>
> Cheers,
> Conor.
Ok, now you know I didn't open the discussion on v1 :)