| Message ID | 20231213232455.2248056-1-robh@kernel.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [net-next] dt-bindings: net: marvell,orion-mdio: Drop "reg" sizes schema | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Wed, Dec 13, 2023 at 05:24:55PM -0600, Rob Herring wrote: > Defining the size of register regions is not really in scope of what > bindings need to cover. The schema for this is also not completely correct > as a reg entry can be variable number of cells for the address and size, > but the schema assumes 1 cell. > > Signed-off-by: Rob Herring <robh@kernel.org> Does this not also remove restrictions on what the number in the reg entry is actually allowed to be? > --- > .../bindings/net/marvell,orion-mdio.yaml | 22 ------------------- > 1 file changed, 22 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml b/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml > index e35da8b01dc2..73429855d584 100644 > --- a/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml > +++ b/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml > @@ -39,28 +39,6 @@ required: > allOf: > - $ref: mdio.yaml# > > - - if: > - required: > - - interrupts > - > - then: > - properties: > - reg: > - items: > - - items: > - - $ref: /schemas/types.yaml#/definitions/cell > - - const: 0x84 > - > - else: > - properties: > - reg: > - items: > - - items: > - - $ref: /schemas/types.yaml#/definitions/cell > - - enum: > - - 0x4 > - - 0x10 > - > unevaluatedProperties: false > > examples: > -- > 2.43.0 >
On Thu, Dec 14, 2023 at 10:23 AM Conor Dooley <conor@kernel.org> wrote: > > On Wed, Dec 13, 2023 at 05:24:55PM -0600, Rob Herring wrote: > > Defining the size of register regions is not really in scope of what > > bindings need to cover. The schema for this is also not completely correct > > as a reg entry can be variable number of cells for the address and size, > > but the schema assumes 1 cell. > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > Does this not also remove restrictions on what the number in the reg > entry is actually allowed to be? Yes, that's what I mean with the first sentence. We don't do this anywhere else with the exception of some I2C devices with fixed addresses. Keying off of the interrupt property also seems questionable. If the register size is different, that should be a different compatible. I only noticed this when I happened to remove "definitions/cell" and this broke. That wasn't really intended to be public. Rob
On Thu, Dec 14, 2023 at 12:12:42PM -0600, Rob Herring wrote: > On Thu, Dec 14, 2023 at 10:23 AM Conor Dooley <conor@kernel.org> wrote: > > > > On Wed, Dec 13, 2023 at 05:24:55PM -0600, Rob Herring wrote: > > > Defining the size of register regions is not really in scope of what > > > bindings need to cover. The schema for this is also not completely correct > > > as a reg entry can be variable number of cells for the address and size, > > > but the schema assumes 1 cell. > > > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > > > Does this not also remove restrictions on what the number in the reg > > entry is actually allowed to be? > > Yes, that's what I mean with the first sentence. We don't do this > anywhere else with the exception of some I2C devices with fixed > addresses. Keying off of the interrupt property also seems > questionable. If the register size is different, that should be a > different compatible. Reading the code, it appears the hardware always supported interrupts, however the first version of the driver never used them. It seems like some DT blobs had the register space cover just the needed registers for polling, and excluded the interrupt control register. When interrupt support was added, all in-tree DT files were updated with the extended register space, but to allow backwards compatibility, the driver checks the length of the register space and will not enable interrupts if its too small. I'm guessing that since the hardware did not change, a new compatible was not used when adding interrupt support. And the yaml is there to help when old out of tree .dts files are merged into the tree and have the old register space. This is and old driver, and its usage of DT is from long before many of the current best practices where determined, or yaml was even an idea. So i'm not surprised it has a few odd quirks. I don't see a reason not to remove these constraints, as i said, the driver should do the right thing if the register space it too small and YAML does not warn about it. Andrew
On Fri, Dec 15, 2023 at 4:18 AM Andrew Lunn <andrew@lunn.ch> wrote: > > On Thu, Dec 14, 2023 at 12:12:42PM -0600, Rob Herring wrote: > > On Thu, Dec 14, 2023 at 10:23 AM Conor Dooley <conor@kernel.org> wrote: > > > > > > On Wed, Dec 13, 2023 at 05:24:55PM -0600, Rob Herring wrote: > > > > Defining the size of register regions is not really in scope of what > > > > bindings need to cover. The schema for this is also not completely correct > > > > as a reg entry can be variable number of cells for the address and size, > > > > but the schema assumes 1 cell. > > > > > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > > > > > Does this not also remove restrictions on what the number in the reg > > > entry is actually allowed to be? > > > > Yes, that's what I mean with the first sentence. We don't do this > > anywhere else with the exception of some I2C devices with fixed > > addresses. Keying off of the interrupt property also seems > > questionable. If the register size is different, that should be a > > different compatible. > > Reading the code, it appears the hardware always supported interrupts, > however the first version of the driver never used them. It seems like > some DT blobs had the register space cover just the needed registers > for polling, and excluded the interrupt control register. When > interrupt support was added, all in-tree DT files were updated with > the extended register space, but to allow backwards compatibility, the > driver checks the length of the register space and will not enable > interrupts if its too small. > > I'm guessing that since the hardware did not change, a new compatible > was not used when adding interrupt support. And the yaml is there to > help when old out of tree .dts files are merged into the tree and have > the old register space. > > This is and old driver, and its usage of DT is from long before many > of the current best practices where determined, or yaml was even an > idea. So i'm not surprised it has a few odd quirks. > > I don't see a reason not to remove these constraints, as i said, the > driver should do the right thing if the register space it too small > and YAML does not warn about it. Is that an Ack? I almost read your double negative as a Nak and that's what the maintainers read because it is now "Rejected" in PW. Rob
On Wed, Dec 13, 2023 at 05:24:55PM -0600, Rob Herring wrote: > Defining the size of register regions is not really in scope of what > bindings need to cover. The schema for this is also not completely correct > as a reg entry can be variable number of cells for the address and size, > but the schema assumes 1 cell. > > Signed-off-by: Rob Herring <robh@kernel.org> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
> Is that an Ack? I almost read your double negative as a Nak and that's > what the maintainers read because it is now "Rejected" in PW. I don't know if this will work, but we can give it a try. pw-bot: new Andrew
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 13 Dec 2023 17:24:55 -0600 you wrote: > Defining the size of register regions is not really in scope of what > bindings need to cover. The schema for this is also not completely correct > as a reg entry can be variable number of cells for the address and size, > but the schema assumes 1 cell. > > Signed-off-by: Rob Herring <robh@kernel.org> > > [...] Here is the summary with links: - [net-next] dt-bindings: net: marvell,orion-mdio: Drop "reg" sizes schema https://git.kernel.org/netdev/net-next/c/758a8d5b6a64 You are awesome, thank you!
diff --git a/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml b/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml index e35da8b01dc2..73429855d584 100644 --- a/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml +++ b/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml @@ -39,28 +39,6 @@ required: allOf: - $ref: mdio.yaml# - - if: - required: - - interrupts - - then: - properties: - reg: - items: - - items: - - $ref: /schemas/types.yaml#/definitions/cell - - const: 0x84 - - else: - properties: - reg: - items: - - items: - - $ref: /schemas/types.yaml#/definitions/cell - - enum: - - 0x4 - - 0x10 - unevaluatedProperties: false examples:
Defining the size of register regions is not really in scope of what bindings need to cover. The schema for this is also not completely correct as a reg entry can be variable number of cells for the address and size, but the schema assumes 1 cell. Signed-off-by: Rob Herring <robh@kernel.org> --- .../bindings/net/marvell,orion-mdio.yaml | 22 ------------------- 1 file changed, 22 deletions(-)