diff mbox series

[net-next] dt-bindings: net: marvell,orion-mdio: Drop "reg" sizes schema

Message ID 20231213232455.2248056-1-robh@kernel.org (mailing list archive)
State Accepted
Commit 758a8d5b6a64ad63a8c0728f68dd3e21481013db
Delegated to: Netdev Maintainers
Headers show
Series [net-next] dt-bindings: net: marvell,orion-mdio: Drop "reg" sizes schema | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 1 maintainers not CCed: robh+dt@kernel.org
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 28 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Rob Herring (Arm) Dec. 13, 2023, 11:24 p.m. UTC
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(-)

Comments

Conor Dooley Dec. 14, 2023, 4:23 p.m. UTC | #1
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
>
Rob Herring (Arm) Dec. 14, 2023, 6:12 p.m. UTC | #2
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
Andrew Lunn Dec. 15, 2023, 10:18 a.m. UTC | #3
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
Rob Herring (Arm) Dec. 15, 2023, 3:41 p.m. UTC | #4
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
Andrew Lunn Dec. 15, 2023, 3:53 p.m. UTC | #5
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
Andrew Lunn Dec. 15, 2023, 3:57 p.m. UTC | #6
> 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
patchwork-bot+netdevbpf@kernel.org Dec. 16, 2023, 2:10 a.m. UTC | #7
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 mbox series

Patch

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: