| Message ID | 20221127224734.885526-7-colin.foster@in-advantage.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | dt-binding preparation for ocelot switches | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 28.11.2022 01:47, Colin Foster wrote: > The description property was located where it applies to every port, not > just ports 5 or 6 (CPU ports). Fix this description. I'm not sure I understand. The description for reg does apply to every port. Both CPU ports and user ports are described. This patch moves the description to under CPU ports only. > > Suggested-by: Rob Herring <robh@kernel.org> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > --- > > v2 -> v3 > * New patch. > > --- > .../bindings/net/dsa/mediatek,mt7530.yaml | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > index 7df4ea1901ce..415e6c40787e 100644 > --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > @@ -156,17 +156,6 @@ patternProperties: > > patternProperties: > "^(ethernet-)?port@[0-9]+$": > - type: object > - description: Ethernet switch ports > - > - unevaluatedProperties: false > - Would be nice to mention these being removed on the patch log. Or remove them while doing ("dt-bindings: net: dsa: utilize base definitions for standard dsa switches"). > - properties: > - reg: > - description: > - Port address described must be 5 or 6 for CPU port and from 0 to 5 > - for user ports. > - > allOf: > - $ref: dsa-port.yaml# > - if: > @@ -174,6 +163,9 @@ patternProperties: > then: > properties: > reg: > + description: > + Port address described must be 5 or 6 for CPU port and from > + 0 to 5 for user ports > enum: > - 5 > - 6 Thank you for your efforts. Arınç
Hi Arınç, On Mon, Nov 28, 2022 at 11:28:31AM +0300, Arınç ÜNAL wrote: > On 28.11.2022 01:47, Colin Foster wrote: > > The description property was located where it applies to every port, not > > just ports 5 or 6 (CPU ports). Fix this description. > > I'm not sure I understand. The description for reg does apply to every port. > Both CPU ports and user ports are described. This patch moves the > description to under CPU ports only. You're right. I misinterpreted what Rob suggested, so the commit message isn't correct. I see now that reg applies to every port, but is only restricted for CPU ports (if: required: [ ethernet ]). I'll clean this message up. > > > > > Suggested-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > > --- > > > > v2 -> v3 > > * New patch. > > > > --- > > .../bindings/net/dsa/mediatek,mt7530.yaml | 14 +++----------- > > 1 file changed, 3 insertions(+), 11 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > > index 7df4ea1901ce..415e6c40787e 100644 > > --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > > +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > > @@ -156,17 +156,6 @@ patternProperties: > > patternProperties: > > "^(ethernet-)?port@[0-9]+$": > > - type: object > > - description: Ethernet switch ports > > - > > - unevaluatedProperties: false > > - > > Would be nice to mention these being removed on the patch log. Or remove > them while doing ("dt-bindings: net: dsa: utilize base definitions for > standard dsa switches"). Agreed. My gut is telling me this wants to be in a separate patch from the generic DSA base definitions patch... but I can't say why for certain. I'll plan to move these to the patch you suggest and add a comment in there about how the type, description, and unevaluatedProperties of mediatek,mt7530 is no longer needed as a result? Keep this patch as more of a "restrict custom port description to CPU ports only" patch? I also see that ("dt-bindings: net: dsa: mediatek,mt7530: remove unnecessary dsa-port reference") should probably be earlier in this patch set. I'll plan to move that earlier in the series, before ("dt-bindings: net: dsa: utilize base definitions for standard dsa switches"). > > > - properties: > > - reg: > > - description: > > - Port address described must be 5 or 6 for CPU port and from 0 to 5 > > - for user ports. > > - > > allOf: > > - $ref: dsa-port.yaml# > > - if: > > @@ -174,6 +163,9 @@ patternProperties: > > then: > > properties: > > reg: > > + description: > > + Port address described must be 5 or 6 for CPU port and from > > + 0 to 5 for user ports > > enum: > > - 5 > > - 6 > > Thank you for your efforts. I greatly appreciate your help and feedback! > > Arınç
Hey Colin, On 29.11.2022 09:32, Colin Foster wrote: > Hi Arınç, > > On Mon, Nov 28, 2022 at 11:28:31AM +0300, Arınç ÜNAL wrote: >> On 28.11.2022 01:47, Colin Foster wrote: >>> The description property was located where it applies to every port, not >>> just ports 5 or 6 (CPU ports). Fix this description. >> >> I'm not sure I understand. The description for reg does apply to every port. >> Both CPU ports and user ports are described. This patch moves the >> description to under CPU ports only. > > You're right. I misinterpreted what Rob suggested, so the commit message > isn't correct. I see now that reg applies to every port, but is only > restricted for CPU ports (if: required: [ ethernet ]). I'll clean this > message up. > >> >>> >>> Suggested-by: Rob Herring <robh@kernel.org> >>> Signed-off-by: Colin Foster <colin.foster@in-advantage.com> >>> --- >>> >>> v2 -> v3 >>> * New patch. >>> >>> --- >>> .../bindings/net/dsa/mediatek,mt7530.yaml | 14 +++----------- >>> 1 file changed, 3 insertions(+), 11 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml >>> index 7df4ea1901ce..415e6c40787e 100644 >>> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml >>> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml >>> @@ -156,17 +156,6 @@ patternProperties: >>> patternProperties: >>> "^(ethernet-)?port@[0-9]+$": >>> - type: object >>> - description: Ethernet switch ports >>> - >>> - unevaluatedProperties: false >>> - >> >> Would be nice to mention these being removed on the patch log. Or remove >> them while doing ("dt-bindings: net: dsa: utilize base definitions for >> standard dsa switches"). > > Agreed. My gut is telling me this wants to be in a separate patch from > the generic DSA base definitions patch... but I can't say why for > certain. I'll plan to move these to the patch you suggest and add a comment If I understand correctly, with ("dt-bindings: net: dsa: utilize base definitions for standard dsa switches"), these properties are now defined on dsa.yaml#/$defs/base and no longer needed to be defined here since mediatek,mt7530.yaml was also made to refer to it. It'd make sense to remove these properties there as there's continuity. > in there about how the type, description, and unevaluatedProperties of > mediatek,mt7530 is no longer needed as a result? Keep this patch as more > of a "restrict custom port description to CPU ports only" patch? I'd say get rid of this patch and do above. Trust your gut though. ;P Arınç
diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml index 7df4ea1901ce..415e6c40787e 100644 --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml @@ -156,17 +156,6 @@ patternProperties: patternProperties: "^(ethernet-)?port@[0-9]+$": - type: object - description: Ethernet switch ports - - unevaluatedProperties: false - - properties: - reg: - description: - Port address described must be 5 or 6 for CPU port and from 0 to 5 - for user ports. - allOf: - $ref: dsa-port.yaml# - if: @@ -174,6 +163,9 @@ patternProperties: then: properties: reg: + description: + Port address described must be 5 or 6 for CPU port and from + 0 to 5 for user ports enum: - 5 - 6
The description property was located where it applies to every port, not just ports 5 or 6 (CPU ports). Fix this description. Suggested-by: Rob Herring <robh@kernel.org> Signed-off-by: Colin Foster <colin.foster@in-advantage.com> --- v2 -> v3 * New patch. --- .../bindings/net/dsa/mediatek,mt7530.yaml | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)