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

  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(-)
  

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!