Re: [PATCH v3 1/3] dt-bindings: net: smsc, lan91c111 convert to schema

[Rui Miguel Silva <rui.silva@linaro.org>]

Hi Andre,
Thanks for your review.

On Thu, Mar 31, 2022 at 04:44:52PM +0100, Andre Przywara wrote:
> On Wed, 30 Mar 2022 14:10:51 +0100
> Rui Miguel Silva <rui.silva@linaro.org> wrote:
> 
> Hi,
> 
> > Convert the smsc lan91c9x and lan91c1xx controller device tree
> > bindings documentation to json-schema.
> > 
> > Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
> > ---
> >  .../bindings/net/smsc,lan91c111.yaml          | 61 +++++++++++++++++++
> >  .../bindings/net/smsc-lan91c111.txt           | 17 ------
> >  2 files changed, 61 insertions(+), 17 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/net/smsc,lan91c111.yaml
> >  delete mode 100644 Documentation/devicetree/bindings/net/smsc-lan91c111.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/net/smsc,lan91c111.yaml b/Documentation/devicetree/bindings/net/smsc,lan91c111.yaml
> > new file mode 100644
> > index 000000000000..1730284430bc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/smsc,lan91c111.yaml
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/smsc,lan91c111.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Smart Mixed-Signal Connectivity (SMSC) LAN91C9x/91C1xx Controller
> > +
> > +maintainers:
> > +  - Nicolas Pitre <nico@fluxnic.net>
> > +
> > +allOf:
> > +  - $ref: ethernet-controller.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: smsc,lan91c111
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  reg-shift: true
> > +
> > +  reg-io-width:
> > +    enum: [ 1, 2, 4 ]
> 
> The old binding spoke of a possible mask, so you could have "6" here to
> signify that your hardware can do both 16 and 32-bit accesses, IIUC.
> And from quickly glancing through the Linux driver it seems to support
> this idea as well.
> So shall this be:
>        minimum: 1
>        maximum: 7
> instead?

I see your point, but going down that rabbit hole, in the smc91x.h
which define the macros for register access, I see, IIUC:
1. 8 or 16 bit access are mandatory (means value 4 only for
   reg-io-width is not valid)
2. All 8 and 16 bit are exclusive, so I think value 3 and 7 here
   don't make sense either.

So, possible values enum: [ 1, 2, 5, 6 ]

let me know your thoughts on this

Cheers,
   Rui
> 
> > +    default: 4
> 
> The old binding said: "If it's omitted or invalid, the size would be 2
> meaning 16-bit access only". That's also what the Linux driver implements.
> So this shall be: "default: 2" then?
> 
> > +
> > +  reset-gpios:
> > +    description: GPIO connected to control RESET pin
> > +    maxItems: 1
> > +
> > +  power-gpios:
> > +    description: GPIO connect to control PWRDEWN pin
> 
>                          connected
> 
> Rest looks fine to me, and passes dt_binding_check.
> 
> Cheers,
> Andre
> 
> > +    maxItems: 1
> > +
> > +  pxa-u16-align4:
> > +    description: put in place the workaround the force all u16 writes to be
> > +      32 bits aligned
> > +    type: boolean
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    ethernet@4010000 {
> > +          compatible = "smsc,lan91c111";
> > +          reg = <0x40100000 0x10000>;
> > +          phy-mode = "mii";
> > +          interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>;
> > +          reg-io-width = <2>;
> > +    };
> > diff --git a/Documentation/devicetree/bindings/net/smsc-lan91c111.txt b/Documentation/devicetree/bindings/net/smsc-lan91c111.txt
> > deleted file mode 100644
> > index 309e37eb7c7c..000000000000
> > --- a/Documentation/devicetree/bindings/net/smsc-lan91c111.txt
> > +++ /dev/null
> > @@ -1,17 +0,0 @@
> > -SMSC LAN91c111 Ethernet mac
> > -
> > -Required properties:
> > -- compatible = "smsc,lan91c111";
> > -- reg : physical address and size of registers
> > -- interrupts : interrupt connection
> > -
> > -Optional properties:
> > -- phy-device : see ethernet.txt file in the same directory
> > -- reg-io-width : Mask of sizes (in bytes) of the IO accesses that
> > -  are supported on the device.  Valid value for SMSC LAN91c111 are
> > -  1, 2 or 4.  If it's omitted or invalid, the size would be 2 meaning
> > -  16-bit access only.
> > -- power-gpios: GPIO to control the PWRDWN pin
> > -- reset-gpios: GPIO to control the RESET pin
> > -- pxa-u16-align4 : Boolean, put in place the workaround the force all
> > -		   u16 writes to be 32 bits aligned
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel