Hi Florian
> -----Original Message-----
> From: Florian Fainelli [mailto:[email protected]]
> Sent: Sunday, October 7, 2018 11:32 PM
> To: Pankaj Bansal <[email protected]>; Andrew Lunn <[email protected]>
> Cc: [email protected]
> Subject: Re: [PATCH v2 1/2] dt-bindings: net: add MDIO bus multiplexer driven
> by
> a regmap device
>
>
>
> On 10/07/18 11:24, Pankaj Bansal wrote:
> > Add support for an MDIO bus multiplexer controlled by a regmap device,
> > like an FPGA.
> >
> > Tested on a NXP LX2160AQDS board which uses the "QIXIS" FPGA attached
> > to the i2c bus.
> >
> > Signed-off-by: Pankaj Bansal <[email protected]>
> > ---
> >
> > Notes:
> > V2:
> > - Fixed formatting error caused by using space instead of tab
> > - Add newline between property list and subnode
> > - Add newline between two subnodes
> >
> > .../bindings/net/mdio-mux-regmap.txt | 95 ++++++++++++++++++
> > 1 file changed, 95 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> > b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> > new file mode 100644
> > index 000000000000..88ebac26c1c5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> > @@ -0,0 +1,95 @@
> > +Properties for an MDIO bus multiplexer controlled by a regmap
> > +
> > +This is a special case of a MDIO bus multiplexer. A regmap device,
> > +like an FPGA, is used to control which child bus is connected. The
> > +mdio-mux node must be a child of the device that is controlled by a regmap.
> > +The driver currently only supports devices with upto 32-bit registers.
>
> I would omit any sort of details about Linux constructs designed to support
> specific needs (e.g: regmap) as well as putting driver limitations into a
> binding
> document because it really ought to be restricted to describing hardware.
>
Actually the platform driver mdio-mux-regmap.c, is generalization of
mdio-mux-mmioreg.c
As such, it doesn't describe any new hardware, so no new properties are
described by it.
The only new property is compatible field.
I don't know how to describe this driver otherwise. Can you please help?
> > +
> > +Required properties in addition to the generic multiplexer properties:
> > +
> > +- compatible : string, must contain "mdio-mux-regmap"
> > +
> > +- reg : integer, contains the offset of the register that controls the bus
> > + multiplexer. it can be 32 bit number.
>
> Technically it could be any "reg" property size, the only requirement is that
> it
> must be of the appropriate size with respect to what the parent node of this
> "mdio-mux-regmap" node has, determined by #address-cells/#size-cells width.
We are reading only single cell of this property using "of_propert_read_u32".
That is why I put the size in this.
>
> > +
> > +- mux-mask : integer, contains an 32 bit mask that specifies which
> > + bits in the register control the actual bus multiplexer. The
> > + 'reg' property of each child mdio-mux node must be constrained by
> > + this mask.
>
> Same thing here.
We are reading only single cell of this property using "of_propert_read_u32".
That is why I put the size in this.
>
> Since this is a MDIO mux, I would invite you to specify what the correct
> #address-cells/#size-cells values should be (1, and 0 respectively as your
> example correctly shows).
>
I will mention #address-cells/#size-cells values
> > +
> > +Example:
> > +
> > +The FPGA node defines a i2c connected FPGA with a register space of 0x30
> bytes.
> > +For the "EMI2" MDIO bus, register 0x54 (BRDCFG4) controls the mux on that
> bus.
> > +A bitmask of 0x07 means that bits 0, 1 and 2 (bit 0 is lsb) are the
> > +bits on
> > +BRDCFG4 that control the actual mux.
> > +
> > +i2c@2000000 {
> > + compatible = "fsl,vf610-i2c";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + reg = <0x0 0x2000000 0x0 0x10000>;
> > + interrupts = <0 34 0x4>; // Level high type
> > + clock-names = "i2c";
> > + clocks = <&clockgen 4 7>;
> > + fsl-scl-gpio = <&gpio2 15 0>;
> > + status = "okay";
> > +
> > + /* The FPGA node */
> > + fpga@66 {
> > + compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c";
> > + reg = <0x66>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + mdio1_mux@54 {
> > + compatible = "mdio-mux-regmap", "mdio-mux";
> > + mdio-parent-bus = <&emdio2>; /* MDIO bus */
> > + reg = <0x54>; /* BRDCFG4 */
> > + mux-mask = <0x07>; /* EMI2_MDIO */
> > + #address-cells=<1>;
> > + #size-cells = <0>;
> > +
> > + mdio1_ioslot1@0 { // Slot 1
> > + reg = <0x00>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + phy1@1 {
> > + reg = <1>;
> > + compatible = "ethernet-phy-
> id0210.7441";
> > + };
> > +
> > + phy1@0 {
> > + reg = <0>;
> > + compatible = "ethernet-phy-
> id0210.7441";
> > + };
> > + };
> > +
> > + mdio1_ioslot2@1 { // Slot 2
> > + reg = <0x01>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + };
> > +
> > + mdio1_ioslot3@2 { // Slot 3
> > + reg = <0x02>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + };
> > + };
> > + };
> > +};
> > +
> > + /* The parent MDIO bus. */
> > + emdio2: mdio@0x8B97000 {
> > + compatible = "fsl,fman-memac-mdio";
> > + reg = <0x0 0x8B97000 0x0 0x1000>;
> > + device_type = "mdio";
> > + little-endian;
> > +
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + };
> >
>
> --
> Florian
