[PATCH v3 1/2] dt-bindings: add register based devices' mux controller DT bindings

[PATCH v3 1/2] dt-bindings: add register based devices' mux controller DT bindings

[Pankaj Bansal]

This adds device tree binding documentation for generic register based
multiplexer controlled by a bitfields in a parent device's register range.

Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---

Notes:
    V3:
    - Added the patch in series with the driver patch
    - Fixed the node value out of bitfield error
    - removed the "parent register r/w functions" line
    V2:
    - Removed syscon reference from txt file
    - Removed loading zeroes from hex numbers
    - Fixed the depth of dts nodes
    - fixed minor formatting errors

 .../devicetree/bindings/mux/reg-mux.txt      | 83 ++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/Documentation/devicetree/bindings/mux/reg-mux.txt b/Documentation/devicetree/bindings/mux/reg-mux.txt
new file mode 100644
index 000000000000..8bea6129c113
--- /dev/null
+++ b/Documentation/devicetree/bindings/mux/reg-mux.txt
@@ -0,0 +1,83 @@
+Generic register bitfield-based multiplexer controller bindings
+
+Define register bitfields to be used to control multiplexers. The parent
+device tree node must be a device node to provide register r/w access.
+
+Required properties:
+- compatible : "reg-mux"
+- #mux-control-cells : <1>
+- mux-reg-masks : an array of register offset and pre-shifted bitfield mask
+                  pairs, each describing a single mux control.
+* Standard mux-controller bindings as decribed in mux-controller.txt
+
+Optional properties:
+- idle-states : if present, the state the muxes will have when idle. The
+		special state MUX_IDLE_AS_IS is the default.
+
+The multiplexer state of each multiplexer is defined as the value of the
+bitfield described by the corresponding register offset and bitfield mask pair
+in the mux-reg-masks array.
+
+Example:
+
+&i2c0 {
+	fpga@66 { // fpga connected to i2c
+		compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c",
+			     "simple-mfd";
+		reg = <0x66>;
+
+		mux: mux-controller { // Mux Producer
+			compatible = "reg-mux";
+			#mux-control-cells = <1>;
+			mux-reg-masks = <0x54 0xf8>, /* 0: reg 0x54, bits 7:3 */
+					<0x54 0x07>; /* 1: reg 0x54, bits 2:0 */
+		};
+	};
+};
+
+mdio-mux-1 { // Mux consumer
+	compatible = "mdio-mux";
+	mux-controls = <&mux 0>;
+	mdio-parent-bus = <&emdio1>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	mdio@0 {
+		reg = <0x0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+
+	mdio@8 {
+		reg = <0x8>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+
+	..
+	..
+};
+
+mdio-mux-2 { // Mux consumer
+	compatible = "mdio-mux";
+	mux-controls = <&mux 1>;
+	mdio-parent-bus = <&emdio2>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	mdio@0 {
+		reg = <0x0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+
+	mdio@1 {
+		reg = <0x1>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+
+	..
+	..
+};
+
-- 
2.17.1

Re: [PATCH v3 1/2] dt-bindings: add register based devices' mux controller DT bindings

[Rob Herring]

On Sun, Feb 24, 2019 at 08:27:23AM +0000, Pankaj Bansal wrote:
> This adds device tree binding documentation for generic register based
> multiplexer controlled by a bitfields in a parent device's register range.
> 
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
> 
> Notes:
>     V3:
>     - Added the patch in series with the driver patch
>     - Fixed the node value out of bitfield error
>     - removed the "parent register r/w functions" line
>     V2:
>     - Removed syscon reference from txt file
>     - Removed loading zeroes from hex numbers
>     - Fixed the depth of dts nodes
>     - fixed minor formatting errors
> 
>  .../devicetree/bindings/mux/reg-mux.txt      | 83 ++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mux/reg-mux.txt b/Documentation/devicetree/bindings/mux/reg-mux.txt
> new file mode 100644
> index 000000000000..8bea6129c113
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mux/reg-mux.txt
> @@ -0,0 +1,83 @@
> +Generic register bitfield-based multiplexer controller bindings
> +
> +Define register bitfields to be used to control multiplexers. The parent
> +device tree node must be a device node to provide register r/w access.

We generally avoid register bit level bindings like this...

What happens when I need 8 or 16-bit accesses. Or some quirky encoding 
of the bits. Or non-contiguous bit fields... It's an endless extending 
of the binding to try to handle different cases.

> +
> +Required properties:
> +- compatible : "reg-mux"
> +- #mux-control-cells : <1>
> +- mux-reg-masks : an array of register offset and pre-shifted bitfield mask
> +                  pairs, each describing a single mux control.
> +* Standard mux-controller bindings as decribed in mux-controller.txt
> +
> +Optional properties:
> +- idle-states : if present, the state the muxes will have when idle. The
> +		special state MUX_IDLE_AS_IS is the default.
> +
> +The multiplexer state of each multiplexer is defined as the value of the
> +bitfield described by the corresponding register offset and bitfield mask pair
> +in the mux-reg-masks array.
> +
> +Example:
> +
> +&i2c0 {
> +	fpga@66 { // fpga connected to i2c
> +		compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c",
> +			     "simple-mfd";
> +		reg = <0x66>;
> +
> +		mux: mux-controller { // Mux Producer
> +			compatible = "reg-mux";

> +			#mux-control-cells = <1>;
> +			mux-reg-masks = <0x54 0xf8>, /* 0: reg 0x54, bits 7:3 */
> +					<0x54 0x07>; /* 1: reg 0x54, bits 2:0 */

You can accomplish the same thing by moving these 2 properties to the 
parent. The parent driver can register a generic mux if that's 
appropriate.

Rob

Re: [PATCH v3 1/2] dt-bindings: add register based devices' mux controller DT bindings

[Li Yang]

On Tue, Feb 26, 2019 at 1:54 PM Rob Herring <robh@kernel.org> wrote:
>
> On Sun, Feb 24, 2019 at 08:27:23AM +0000, Pankaj Bansal wrote:
> > This adds device tree binding documentation for generic register based
> > multiplexer controlled by a bitfields in a parent device's register range.
> >
> > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > ---
> >
> > Notes:
> >     V3:
> >     - Added the patch in series with the driver patch
> >     - Fixed the node value out of bitfield error
> >     - removed the "parent register r/w functions" line
> >     V2:
> >     - Removed syscon reference from txt file
> >     - Removed loading zeroes from hex numbers
> >     - Fixed the depth of dts nodes
> >     - fixed minor formatting errors
> >
> >  .../devicetree/bindings/mux/reg-mux.txt      | 83 ++++++++++++++++++
> >  1 file changed, 83 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mux/reg-mux.txt b/Documentation/devicetree/bindings/mux/reg-mux.txt
> > new file mode 100644
> > index 000000000000..8bea6129c113
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mux/reg-mux.txt
> > @@ -0,0 +1,83 @@
> > +Generic register bitfield-based multiplexer controller bindings
> > +
> > +Define register bitfields to be used to control multiplexers. The parent
> > +device tree node must be a device node to provide register r/w access.
>
> We generally avoid register bit level bindings like this...
>
> What happens when I need 8 or 16-bit accesses. Or some quirky encoding
> of the bits. Or non-contiguous bit fields... It's an endless extending
> of the binding to try to handle different cases.

I think the intention here is to mimic the existing mmio-mux binding
(Documentation/devicetree/bindings/mux/mmio-mux.txt) so that the
existing mmio-mux driver can be extended to cover both MMIO and
non-MMIO register based muxes.  The bit-width of the register access
will be taken care of by the regmap framework.  It is probably true
that this can not cover all the register based mux device if they do
have quircky register layout, but I think it should be good enough to
cover majority of the cases.  If there are devices that cannot be
covered by the generic binding and driver, they could create their own
bindings and drivers.

>
> > +
> > +Required properties:
> > +- compatible : "reg-mux"
> > +- #mux-control-cells : <1>
> > +- mux-reg-masks : an array of register offset and pre-shifted bitfield mask
> > +                  pairs, each describing a single mux control.
> > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > +
> > +Optional properties:
> > +- idle-states : if present, the state the muxes will have when idle. The
> > +             special state MUX_IDLE_AS_IS is the default.
> > +
> > +The multiplexer state of each multiplexer is defined as the value of the
> > +bitfield described by the corresponding register offset and bitfield mask pair
> > +in the mux-reg-masks array.
> > +
> > +Example:
> > +
> > +&i2c0 {
> > +     fpga@66 { // fpga connected to i2c
> > +             compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c",
> > +                          "simple-mfd";
> > +             reg = <0x66>;
> > +
> > +             mux: mux-controller { // Mux Producer
> > +                     compatible = "reg-mux";
>
> > +                     #mux-control-cells = <1>;
> > +                     mux-reg-masks = <0x54 0xf8>, /* 0: reg 0x54, bits 7:3 */
> > +                                     <0x54 0x07>; /* 1: reg 0x54, bits 2:0 */
>
> You can accomplish the same thing by moving these 2 properties to the
> parent. The parent driver can register a generic mux if that's
> appropriate.

Again, the intention is to reuse the generic mux binding and driver.

Regards,
Leo

Re: [PATCH v3 1/2] dt-bindings: add register based devices' mux controller DT bindings

[Rob Herring]

On Tue, Feb 26, 2019 at 02:28:12PM -0600, Li Yang wrote:
> On Tue, Feb 26, 2019 at 1:54 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Sun, Feb 24, 2019 at 08:27:23AM +0000, Pankaj Bansal wrote:
> > > This adds device tree binding documentation for generic register based
> > > multiplexer controlled by a bitfields in a parent device's register range.
> > >
> > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > > ---
> > >
> > > Notes:
> > >     V3:
> > >     - Added the patch in series with the driver patch
> > >     - Fixed the node value out of bitfield error
> > >     - removed the "parent register r/w functions" line
> > >     V2:
> > >     - Removed syscon reference from txt file
> > >     - Removed loading zeroes from hex numbers
> > >     - Fixed the depth of dts nodes
> > >     - fixed minor formatting errors
> > >
> > >  .../devicetree/bindings/mux/reg-mux.txt      | 83 ++++++++++++++++++
> > >  1 file changed, 83 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/mux/reg-mux.txt b/Documentation/devicetree/bindings/mux/reg-mux.txt
> > > new file mode 100644
> > > index 000000000000..8bea6129c113
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mux/reg-mux.txt
> > > @@ -0,0 +1,83 @@
> > > +Generic register bitfield-based multiplexer controller bindings
> > > +
> > > +Define register bitfields to be used to control multiplexers. The parent
> > > +device tree node must be a device node to provide register r/w access.
> >
> > We generally avoid register bit level bindings like this...
> >
> > What happens when I need 8 or 16-bit accesses. Or some quirky encoding
> > of the bits. Or non-contiguous bit fields... It's an endless extending
> > of the binding to try to handle different cases.
> 
> I think the intention here is to mimic the existing mmio-mux binding
> (Documentation/devicetree/bindings/mux/mmio-mux.txt) so that the
> existing mmio-mux driver can be extended to cover both MMIO and
> non-MMIO register based muxes.  The bit-width of the register access
> will be taken care of by the regmap framework.  It is probably true
> that this can not cover all the register based mux device if they do
> have quircky register layout, but I think it should be good enough to
> cover majority of the cases.  If there are devices that cannot be
> covered by the generic binding and driver, they could create their own
> bindings and drivers.

Other than the somewhat misleading name, why not just reuse the existing 
binding? Worst case, just add another compatible to it and renane the 
file. That would have saved me reviewing the whole thing again... 

But really, you should just be able to use what is there and determine 
the type of access from the parent node (or I suppose regmap does that 
for you).

Rob

RE: [PATCH v3 1/2] dt-bindings: add register based devices' mux controller DT bindings

[Pankaj Bansal]

Hi Rob,

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Wednesday, 27 February, 2019 03:31 AM
> To: Leo Li <leoyang.li@nxp.com>
> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>; Peter Rosin <peda@axentia.se>;
> Frank Rowand <frowand.list@gmail.com>; devicetree@vger.kernel.org
> Subject: Re: [PATCH v3 1/2] dt-bindings: add register based devices' mux
> controller DT bindings
> 
> On Tue, Feb 26, 2019 at 02:28:12PM -0600, Li Yang wrote:
> > On Tue, Feb 26, 2019 at 1:54 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Sun, Feb 24, 2019 at 08:27:23AM +0000, Pankaj Bansal wrote:
> > > > This adds device tree binding documentation for generic register
> > > > based multiplexer controlled by a bitfields in a parent device's register
> range.
> > > >
> > > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > > > ---
> > > >
> > > > Notes:
> > > >     V3:
> > > >     - Added the patch in series with the driver patch
> > > >     - Fixed the node value out of bitfield error
> > > >     - removed the "parent register r/w functions" line
> > > >     V2:
> > > >     - Removed syscon reference from txt file
> > > >     - Removed loading zeroes from hex numbers
> > > >     - Fixed the depth of dts nodes
> > > >     - fixed minor formatting errors
> > > >
> > > >  .../devicetree/bindings/mux/reg-mux.txt      | 83 ++++++++++++++++++
> > > >  1 file changed, 83 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mux/reg-mux.txt
> > > > b/Documentation/devicetree/bindings/mux/reg-mux.txt
> > > > new file mode 100644
> > > > index 000000000000..8bea6129c113
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mux/reg-mux.txt
> > > > @@ -0,0 +1,83 @@
> > > > +Generic register bitfield-based multiplexer controller bindings
> > > > +
> > > > +Define register bitfields to be used to control multiplexers. The
> > > > +parent device tree node must be a device node to provide register r/w
> access.
> > >
> > > We generally avoid register bit level bindings like this...
> > >
> > > What happens when I need 8 or 16-bit accesses. Or some quirky
> > > encoding of the bits. Or non-contiguous bit fields... It's an
> > > endless extending of the binding to try to handle different cases.
> >
> > I think the intention here is to mimic the existing mmio-mux binding
> > (Documentation/devicetree/bindings/mux/mmio-mux.txt) so that the
> > existing mmio-mux driver can be extended to cover both MMIO and
> > non-MMIO register based muxes.  The bit-width of the register access
> > will be taken care of by the regmap framework.  It is probably true
> > that this can not cover all the register based mux device if they do
> > have quircky register layout, but I think it should be good enough to
> > cover majority of the cases.  If there are devices that cannot be
> > covered by the generic binding and driver, they could create their own
> > bindings and drivers.
> 
> Other than the somewhat misleading name, why not just reuse the existing
> binding? Worst case, just add another compatible to it and renane the file. That
> would have saved me reviewing the whole thing again...
> 

I guess I can rename mmio-mux to reg-mux and add the bindings in it. Because really, the
MMIO mux is a special case of register based mux, where the register access is memory mapped.
@Peter Rosin : Are you okay with this?

> But really, you should just be able to use what is there and determine the type of
> access from the parent node (or I suppose regmap does that for you).

The intention is to have an indication that the parent devices produces one (or many) mux
Producers, which are controlled by parent devices' registers.
Which is why I chose compatible "reg-mux".

The parent device's register can be written in any way (MMIO / SPI / I2C) etc.

> 
> Rob

Re: [PATCH v3 1/2] dt-bindings: add register based devices' mux controller DT bindings

[Peter Rosin]

On 2019-02-27 09:30, Pankaj Bansal wrote:
> Hi Rob,
> 
>> -----Original Message-----
>> From: Rob Herring [mailto:robh@kernel.org]
>> Sent: Wednesday, 27 February, 2019 03:31 AM
>> To: Leo Li <leoyang.li@nxp.com>
>> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>; Peter Rosin <peda@axentia.se>;
>> Frank Rowand <frowand.list@gmail.com>; devicetree@vger.kernel.org
>> Subject: Re: [PATCH v3 1/2] dt-bindings: add register based devices' mux
>> controller DT bindings
>>
>> On Tue, Feb 26, 2019 at 02:28:12PM -0600, Li Yang wrote:
>>> On Tue, Feb 26, 2019 at 1:54 PM Rob Herring <robh@kernel.org> wrote:
>>>>
>>>> On Sun, Feb 24, 2019 at 08:27:23AM +0000, Pankaj Bansal wrote:
>>>>> This adds device tree binding documentation for generic register
>>>>> based multiplexer controlled by a bitfields in a parent device's register
>> range.
>>>>>
>>>>> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>>     V3:
>>>>>     - Added the patch in series with the driver patch
>>>>>     - Fixed the node value out of bitfield error
>>>>>     - removed the "parent register r/w functions" line
>>>>>     V2:
>>>>>     - Removed syscon reference from txt file
>>>>>     - Removed loading zeroes from hex numbers
>>>>>     - Fixed the depth of dts nodes
>>>>>     - fixed minor formatting errors
>>>>>
>>>>>  .../devicetree/bindings/mux/reg-mux.txt      | 83 ++++++++++++++++++
>>>>>  1 file changed, 83 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mux/reg-mux.txt
>>>>> b/Documentation/devicetree/bindings/mux/reg-mux.txt
>>>>> new file mode 100644
>>>>> index 000000000000..8bea6129c113
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/mux/reg-mux.txt
>>>>> @@ -0,0 +1,83 @@
>>>>> +Generic register bitfield-based multiplexer controller bindings
>>>>> +
>>>>> +Define register bitfields to be used to control multiplexers. The
>>>>> +parent device tree node must be a device node to provide register r/w
>> access.
>>>>
>>>> We generally avoid register bit level bindings like this...
>>>>
>>>> What happens when I need 8 or 16-bit accesses. Or some quirky
>>>> encoding of the bits. Or non-contiguous bit fields... It's an
>>>> endless extending of the binding to try to handle different cases.
>>>
>>> I think the intention here is to mimic the existing mmio-mux binding
>>> (Documentation/devicetree/bindings/mux/mmio-mux.txt) so that the
>>> existing mmio-mux driver can be extended to cover both MMIO and
>>> non-MMIO register based muxes.  The bit-width of the register access
>>> will be taken care of by the regmap framework.  It is probably true
>>> that this can not cover all the register based mux device if they do
>>> have quircky register layout, but I think it should be good enough to
>>> cover majority of the cases.  If there are devices that cannot be
>>> covered by the generic binding and driver, they could create their own
>>> bindings and drivers.
>>
>> Other than the somewhat misleading name, why not just reuse the existing
>> binding? Worst case, just add another compatible to it and renane the file. That
>> would have saved me reviewing the whole thing again...
>>
> 
> I guess I can rename mmio-mux to reg-mux and add the bindings in it. Because really, the
> MMIO mux is a special case of register based mux, where the register access is memory mapped.
> @Peter Rosin : Are you okay with this?

It's so obvious now that Rob points it out. Sorry for not realizing this
all by myself when I reviewed the initial revisions. I made the connection
for the driver itself, but not so for the binding. How silly, and sorry
again for the trouble.

So, please rename the file with the binding, and hack the text to cover
both compatibles (mmio-mux and reg-mux), because I think it would be
very confusing to reuse the mmio-mux compatible for a "reg-mux" that is
not mmio based.

>> But really, you should just be able to use what is there and determine the type of
>> access from the parent node (or I suppose regmap does that for you).
> 
> The intention is to have an indication that the parent devices produces one (or many) mux
> Producers, which are controlled by parent devices' registers.
> Which is why I chose compatible "reg-mux".

This is not what Rob is talking about, he is talking about /not/ adding
a new compatible at all, and have the driver simply try both of
syscon_node_to_regmap(np->parent) and dev_get_regmap(dev->parent, NULL)
(in some other) and go with anything that is returned.

However, as I said above, that leads to confusing naming in the dts,
and adding a new compatible feels like it's worth it.

Cheers,
Peter

> The parent device's register can be written in any way (MMIO / SPI / I2C) etc.

Re: [PATCH v3 1/2] dt-bindings: add register based devices' mux controller DT bindings

[Peter Rosin]

On 2019-02-27 09:42, Peter Rosin wrote:
> This is not what Rob is talking about, he is talking about /not/ adding
> a new compatible at all, and have the driver simply try both of
> syscon_node_to_regmap(np->parent) and dev_get_regmap(dev->parent, NULL)
> (in some other) and go with anything that is returned.

"(in some order)", sigh...

Cheers,
Peter