[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

[PATCH v3 2/2] drivers: mux: Add Generic regmap bitfield-based multiplexer in mmio-mux

[Pankaj Bansal]

Generic register bitfield-based multiplexer that controls the multiplexer
producer defined under a parent node.
The driver corresponding to parent node provides register read/write
capabilities.

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

Notes:
    V3:
    - Added the patch in series with device tree binding patch
    - Added the NULL return handling for regmap
    V2:
    - removed seperate driver regmap.c and added the regmap function in mmio.c
      based on compatible field, the syscon or regmap function would be called
    - Modified the KConfig as per Peter's comments

 drivers/mux/Kconfig | 12 ++++++------
 drivers/mux/mmio.c  | 10 +++++++---
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
index 7659d6c5f718..e5c571fd232c 100644
--- a/drivers/mux/Kconfig
+++ b/drivers/mux/Kconfig
@@ -46,14 +46,14 @@ config MUX_GPIO
 	  be called mux-gpio.
 
 config MUX_MMIO
-	tristate "MMIO register bitfield-controlled Multiplexer"
-	depends on (OF && MFD_SYSCON) || COMPILE_TEST
+	tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
+	depends on OF || COMPILE_TEST
 	help
-	  MMIO register bitfield-controlled Multiplexer controller.
+	  MMIO/Regmap register bitfield-controlled Multiplexer controller.
 
-	  The driver builds multiplexer controllers for bitfields in a syscon
-	  register. For N bit wide bitfields, there will be 2^N possible
-	  multiplexer states.
+	  The driver builds multiplexer controllers for bitfields in either
+	  a syscon register or a driver regmap register. For N bit wide
+	  bitfields, there will be 2^N possible multiplexer states.
 
 	  To compile the driver as a module, choose M here: the module will
 	  be called mux-mmio.
diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
index 935ac44aa209..cc02155e4644 100644
--- a/drivers/mux/mmio.c
+++ b/drivers/mux/mmio.c
@@ -28,6 +28,7 @@ static const struct mux_control_ops mux_mmio_ops = {
 
 static const struct of_device_id mux_mmio_dt_ids[] = {
 	{ .compatible = "mmio-mux", },
+	{ .compatible = "reg-mux", },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, mux_mmio_dt_ids);
@@ -43,9 +44,12 @@ static int mux_mmio_probe(struct platform_device *pdev)
 	int ret;
 	int i;
 
-	regmap = syscon_node_to_regmap(np->parent);
-	if (IS_ERR(regmap)) {
-		ret = PTR_ERR(regmap);
+	if (of_device_is_compatible(np, "mmio-mux"))
+		regmap = syscon_node_to_regmap(np->parent);
+	else
+		regmap = dev_get_regmap(dev->parent, NULL);
+	if (IS_ERR_OR_NULL(regmap)) {
+		ret = PTR_ERR_OR_ZERO(regmap) ? PTR_ERR(regmap) : -ENODEV;
 		dev_err(dev, "failed to get regmap: %d\n", ret);
 		return ret;
 	}
-- 
2.17.1

Re: [PATCH v3 2/2] drivers: mux: Add Generic regmap bitfield-based multiplexer in mmio-mux

[Peter Rosin]

On 2019-02-24 09:27, Pankaj Bansal wrote:
> Generic register bitfield-based multiplexer that controls the multiplexer
> producer defined under a parent node.
> The driver corresponding to parent node provides register read/write
> capabilities.
> 
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
> 
> Notes:
>     V3:
>     - Added the patch in series with device tree binding patch
>     - Added the NULL return handling for regmap
>     V2:
>     - removed seperate driver regmap.c and added the regmap function in mmio.c
>       based on compatible field, the syscon or regmap function would be called
>     - Modified the KConfig as per Peter's comments
> 
>  drivers/mux/Kconfig | 12 ++++++------
>  drivers/mux/mmio.c  | 10 +++++++---
>  2 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index 7659d6c5f718..e5c571fd232c 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -46,14 +46,14 @@ config MUX_GPIO
>  	  be called mux-gpio.
>  
>  config MUX_MMIO
> -	tristate "MMIO register bitfield-controlled Multiplexer"
> -	depends on (OF && MFD_SYSCON) || COMPILE_TEST
> +	tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
> +	depends on OF || COMPILE_TEST
>  	help
> -	  MMIO register bitfield-controlled Multiplexer controller.
> +	  MMIO/Regmap register bitfield-controlled Multiplexer controller.
>  
> -	  The driver builds multiplexer controllers for bitfields in a syscon
> -	  register. For N bit wide bitfields, there will be 2^N possible
> -	  multiplexer states.
> +	  The driver builds multiplexer controllers for bitfields in either
> +	  a syscon register or a driver regmap register. For N bit wide
> +	  bitfields, there will be 2^N possible multiplexer states.
>  
>  	  To compile the driver as a module, choose M here: the module will
>  	  be called mux-mmio.
> diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
> index 935ac44aa209..cc02155e4644 100644
> --- a/drivers/mux/mmio.c
> +++ b/drivers/mux/mmio.c
> @@ -28,6 +28,7 @@ static const struct mux_control_ops mux_mmio_ops = {
>  
>  static const struct of_device_id mux_mmio_dt_ids[] = {
>  	{ .compatible = "mmio-mux", },
> +	{ .compatible = "reg-mux", },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, mux_mmio_dt_ids);
> @@ -43,9 +44,12 @@ static int mux_mmio_probe(struct platform_device *pdev)
>  	int ret;
>  	int i;
>  
> -	regmap = syscon_node_to_regmap(np->parent);
> -	if (IS_ERR(regmap)) {
> -		ret = PTR_ERR(regmap);
> +	if (of_device_is_compatible(np, "mmio-mux"))
> +		regmap = syscon_node_to_regmap(np->parent);
> +	else
> +		regmap = dev_get_regmap(dev->parent, NULL);
> +	if (IS_ERR_OR_NULL(regmap)) {
> +		ret = PTR_ERR_OR_ZERO(regmap) ? PTR_ERR(regmap) : -ENODEV;

The above is not correct, this should be better (untested):

		ret = PTR_ERR(regmap) ?: -ENODEV;

Cheers,
Peter

>  		dev_err(dev, "failed to get regmap: %d\n", ret);
>  		return ret;
>  	}
> 

RE: [PATCH v3 2/2] drivers: mux: Add Generic regmap bitfield-based multiplexer in mmio-mux

[Pankaj Bansal]

Hi Peter,

> -----Original Message-----
> From: Peter Rosin [mailto:peda@axentia.se]
> Sent: Monday, 25 February, 2019 08:14 PM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>; Leo Li <leoyang.li@nxp.com>
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 2/2] drivers: mux: Add Generic regmap bitfield-based
> multiplexer in mmio-mux
> 
> On 2019-02-24 09:27, Pankaj Bansal wrote:
> > Generic register bitfield-based multiplexer that controls the
> > multiplexer producer defined under a parent node.
> > The driver corresponding to parent node provides register read/write
> > capabilities.
> >
> > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > ---
> >
> > Notes:
> >     V3:
> >     - Added the patch in series with device tree binding patch
> >     - Added the NULL return handling for regmap
> >     V2:
> >     - removed seperate driver regmap.c and added the regmap function in
> mmio.c
> >       based on compatible field, the syscon or regmap function would be called
> >     - Modified the KConfig as per Peter's comments
> >
> >  drivers/mux/Kconfig | 12 ++++++------  drivers/mux/mmio.c  | 10
> > +++++++---
> >  2 files changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig index
> > 7659d6c5f718..e5c571fd232c 100644
> > --- a/drivers/mux/Kconfig
> > +++ b/drivers/mux/Kconfig
> > @@ -46,14 +46,14 @@ config MUX_GPIO
> >  	  be called mux-gpio.
> >
> >  config MUX_MMIO
> > -	tristate "MMIO register bitfield-controlled Multiplexer"
> > -	depends on (OF && MFD_SYSCON) || COMPILE_TEST
> > +	tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
> > +	depends on OF || COMPILE_TEST
> >  	help
> > -	  MMIO register bitfield-controlled Multiplexer controller.
> > +	  MMIO/Regmap register bitfield-controlled Multiplexer controller.
> >
> > -	  The driver builds multiplexer controllers for bitfields in a syscon
> > -	  register. For N bit wide bitfields, there will be 2^N possible
> > -	  multiplexer states.
> > +	  The driver builds multiplexer controllers for bitfields in either
> > +	  a syscon register or a driver regmap register. For N bit wide
> > +	  bitfields, there will be 2^N possible multiplexer states.
> >
> >  	  To compile the driver as a module, choose M here: the module will
> >  	  be called mux-mmio.
> > diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c index
> > 935ac44aa209..cc02155e4644 100644
> > --- a/drivers/mux/mmio.c
> > +++ b/drivers/mux/mmio.c
> > @@ -28,6 +28,7 @@ static const struct mux_control_ops mux_mmio_ops = {
> >
> >  static const struct of_device_id mux_mmio_dt_ids[] = {
> >  	{ .compatible = "mmio-mux", },
> > +	{ .compatible = "reg-mux", },
> >  	{ /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, mux_mmio_dt_ids); @@ -43,9 +44,12 @@
> static
> > int mux_mmio_probe(struct platform_device *pdev)
> >  	int ret;
> >  	int i;
> >
> > -	regmap = syscon_node_to_regmap(np->parent);
> > -	if (IS_ERR(regmap)) {
> > -		ret = PTR_ERR(regmap);
> > +	if (of_device_is_compatible(np, "mmio-mux"))
> > +		regmap = syscon_node_to_regmap(np->parent);
> > +	else
> > +		regmap = dev_get_regmap(dev->parent, NULL);
> > +	if (IS_ERR_OR_NULL(regmap)) {
> > +		ret = PTR_ERR_OR_ZERO(regmap) ? PTR_ERR(regmap) : -
> ENODEV;
> 
> The above is not correct, this should be better (untested):
> 
> 		ret = PTR_ERR(regmap) ?: -ENODEV;

Omitting the second operand in ternary operator is not standard. https://stackoverflow.com/questions/34559705/ternary-operator-without-the-middle-expression

Although, it *has been* used in kernel in many places
https://livegrep.com/search/linux?q=file%3A%5C.c%24%20%5C%3F%5C%3A&fold_case=auto&regex=true&context=true


> 
> Cheers,
> Peter
> 
> >  		dev_err(dev, "failed to get regmap: %d\n", ret);
> >  		return ret;
> >  	}
> >

Re: [PATCH v3 2/2] drivers: mux: Add Generic regmap bitfield-based multiplexer in mmio-mux

[Peter Rosin]

On 2019-02-26 07:08, Pankaj Bansal wrote:
>>> +	if (IS_ERR_OR_NULL(regmap)) {
>>> +		ret = PTR_ERR_OR_ZERO(regmap) ? PTR_ERR(regmap) : -
>> ENODEV;
>>
>> The above is not correct, this should be better (untested):
>>
>> 		ret = PTR_ERR(regmap) ?: -ENODEV;
> 
> Omitting the second operand in ternary operator is not standard. https://stackoverflow.com/questions/34559705/ternary-operator-without-the-middle-expression

Irrelevant, a great many non-standard features are used in the kernel.

> Although, it *has been* used in kernel in many places
> https://livegrep.com/search/linux?q=file%3A%5C.c%24%20%5C%3F%5C%3A&fold_case=auto&regex=true&context=true

Yes, a local grep suggests that there are plenty of instances in the
kernel. In fact, there are multiple instances even in the kernel/
directory so it's not some fringe thing. My conclusion is that it
makes for readable code and is perfectly safe to use in the kernel,
which apparently does not care about compilers that do not understand
the construct.

$ git grep '?:' *.[ch] | wc
    773    4830   54975

I you still don't buy that and don't want to sign-off on it, then
write

		ret = IS_ERR(regmap) ? PTR_ERR(regmap) : -ENODEV;

But that's just wordy and less readable, methinks.

However, the best thing to do, according to your maintainer, is still
the original suggestion to fixup the NULL as early as possible with

		regmap = dev_get_regmap(dev->parent, NULL) ?: ERR_PTR(-ENODEV);

And leave the error handling block alone. Can you please just do that
and be done with it?

Cheers,
Peter

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