[RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Philipp Zabel]

This adds device tree binding documentation for mmio-based syscon
multiplexers controlled by a single bitfield in a syscon register
range.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt

diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
new file mode 100644
index 0000000000000..11d96f5d98583
--- /dev/null
+++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
@@ -0,0 +1,56 @@
+MMIO bitfield-based multiplexer controller bindings
+
+Define a syscon bitfield to be used to control a multiplexer. The parent
+device tree node must be a syscon node to provide register access.
+
+Required properties:
+- compatible : "gpio-mux"
+- reg : register base of the register containing the control bitfield
+- bit-mask : bitmask of the control bitfield in the control register
+- bit-shift : bit offset of the control bitfield in the control register
+- #mux-control-cells : <0>
+* Standard mux-controller bindings as decribed in mux-controller.txt
+
+Optional properties:
+- idle-state : if present, the state the mux will have when idle. The
+	       special state MUX_IDLE_AS_IS is the default.
+
+The multiplexer state is defined as the value of the bitfield described
+by the reg, bit-mask, and bit-shift properties, accessed through the parent
+syscon.
+
+Example:
+
+	syscon {
+		compatible = "syscon";
+
+		mux: mux-controller@3 {
+			compatible = "mmio-mux";
+			reg = <0x3>;
+			bit-mask = <0x1>;
+			bit-shift = <5>;
+			#mux-control-cells = <0>;
+		};
+	};
+
+	video-mux {
+		compatible = "video-mux";
+		mux-controls = <&mux>;
+
+		ports {
+			/* input 0 */
+			port@0 {
+				reg = <0>;
+			};
+
+			/* input 1 */
+			port@1 {
+				reg = <1>;
+			};
+
+			/* output */
+			port@2 {
+				reg = <2>;
+			};
+		};
+	};
-- 
2.11.0

[RFC 2/2] mux: mmio-based syscon mux controller

[Philipp Zabel]

This adds a driver for mmio-based syscon multiplexers controlled by a
single bitfield in a syscon register range.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/mux/Kconfig      |  13 +++++
 drivers/mux/Makefile     |   1 +
 drivers/mux/mux-syscon.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 144 insertions(+)
 create mode 100644 drivers/mux/mux-syscon.c

diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
index 86668b4d2fc52..a5e6a3b01ac24 100644
--- a/drivers/mux/Kconfig
+++ b/drivers/mux/Kconfig
@@ -43,4 +43,17 @@ config MUX_GPIO
 	  To compile the driver as a module, choose M here: the module will
 	  be called mux-gpio.
 
+config MUX_SYSCON
+	tristate "MMIO bitfield-controlled Multiplexer"
+	depends on OF && MFD_SYSCON
+	help
+	  MMIO bitfield-controlled Multiplexer controller.
+
+	  The driver builds a single multiplexer controller using a bitfield
+	  in a syscon 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-syscon.
+
 endif
diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
index b00a7d37d2fbe..234309f6655f7 100644
--- a/drivers/mux/Makefile
+++ b/drivers/mux/Makefile
@@ -5,3 +5,4 @@
 obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
 obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
 obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
+obj-$(CONFIG_MUX_SYSCON)	+= mux-syscon.o
diff --git a/drivers/mux/mux-syscon.c b/drivers/mux/mux-syscon.c
new file mode 100644
index 0000000000000..31cacc61f1439
--- /dev/null
+++ b/drivers/mux/mux-syscon.c
@@ -0,0 +1,130 @@
+/*
+ * syscon bitfield-controlled multiplexer driver
+ *
+ * Copyright (C) 2017 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/err.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/mux.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+struct mux_syscon {
+	struct regmap_field *field;
+};
+
+static int mux_syscon_set(struct mux_control *mux, int state)
+{
+	struct mux_syscon *mux_syscon = mux_chip_priv(mux->chip);
+
+	return regmap_field_write(mux_syscon->field, state);
+}
+
+static const struct mux_control_ops mux_syscon_ops = {
+	.set = mux_syscon_set,
+};
+
+static const struct of_device_id mux_syscon_dt_ids[] = {
+	{ .compatible = "mmio-mux", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mux_syscon_dt_ids);
+
+static int of_get_reg_field(struct device_node *node, struct reg_field *field)
+{
+	u32 bit_mask;
+	int ret;
+
+	ret = of_property_read_u32(node, "reg", &field->reg);
+	if (ret < 0)
+		return ret;
+
+	ret = of_property_read_u32(node, "bit-mask", &bit_mask);
+	if (ret < 0)
+		return ret;
+
+	ret = of_property_read_u32(node, "bit-shift", &field->lsb);
+	if (ret < 0)
+		return ret;
+
+	field->msb = field->lsb + fls(bit_mask) - 1;
+
+	return 0;
+}
+
+static int mux_syscon_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mux_chip *mux_chip;
+	struct mux_syscon *mux_syscon;
+	struct regmap *regmap;
+	struct reg_field field;
+	int bits;
+	s32 idle_state;
+	int ret;
+
+	ret = of_get_reg_field(pdev->dev.of_node, &field);
+	if (ret) {
+		dev_err(&pdev->dev, "missing bit-field properties: %d\n", ret);
+		return ret;
+	}
+
+	regmap = syscon_node_to_regmap(pdev->dev.of_node->parent);
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
+		dev_err(&pdev->dev, "failed to get syscon regmap: %d\n", ret);
+		return ret;
+	}
+
+	mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_syscon));
+	if (!mux_chip)
+		return -ENOMEM;
+
+	mux_syscon = mux_chip_priv(mux_chip);
+	mux_chip->ops = &mux_syscon_ops;
+
+	mux_syscon->field = devm_regmap_field_alloc(&pdev->dev, regmap, field);
+	if (IS_ERR(mux_syscon->field)) {
+		ret = PTR_ERR(mux_syscon->field);
+		dev_err(&pdev->dev, "failed to regmap bit-field: %d\n", ret);
+		return ret;
+	}
+	bits = 1 + field.msb - field.lsb;
+
+	mux_chip->mux->states = 1 << bits;
+
+	ret = device_property_read_u32(dev, "idle-state", (u32 *)&idle_state);
+	if (ret >= 0 && idle_state != MUX_IDLE_AS_IS) {
+		if (idle_state < 0 || idle_state >= mux_chip->mux->states) {
+			dev_err(dev, "invalid idle-state %u\n", idle_state);
+			return -EINVAL;
+		}
+
+		mux_chip->mux->idle_state = idle_state;
+	}
+
+	regmap_field_read(mux_syscon->field, &mux_chip->mux->cached_state);
+
+	return devm_mux_chip_register(dev, mux_chip);
+}
+
+static struct platform_driver mux_syscon_driver = {
+	.driver = {
+		.name = "mmio-mux",
+		.of_match_table	= of_match_ptr(mux_syscon_dt_ids),
+	},
+	.probe = mux_syscon_probe,
+};
+module_platform_driver(mux_syscon_driver);
+
+MODULE_DESCRIPTION("MMIO bitfield-controlled multiplexer driver");
+MODULE_AUTHOR("Philipp Zabel <p.zabel@pengutronix.de>");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

Re: [RFC 2/2] mux: mmio-based syscon mux controller

[Steve Longerbeam]

On 04/13/2017 08:48 AM, Philipp Zabel wrote:
> This adds a driver for mmio-based syscon multiplexers controlled by a
> single bitfield in a syscon register range.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>   drivers/mux/Kconfig      |  13 +++++
>   drivers/mux/Makefile     |   1 +
>   drivers/mux/mux-syscon.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 144 insertions(+)
>   create mode 100644 drivers/mux/mux-syscon.c
>
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index 86668b4d2fc52..a5e6a3b01ac24 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -43,4 +43,17 @@ config MUX_GPIO
>   	  To compile the driver as a module, choose M here: the module will
>   	  be called mux-gpio.
>   
> +config MUX_SYSCON

my preference would be CONFIG_MUX_MMIO.

> +	tristate "MMIO bitfield-controlled Multiplexer"

"MMIO register bitfield-controlled Multiplexer"

The rest looks good to me.

Steve

> +	depends on OF && MFD_SYSCON
> +	help
> +	  MMIO bitfield-controlled Multiplexer controller.
> +
> +	  The driver builds a single multiplexer controller using a bitfield
> +	  in a syscon 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-syscon.
> +
>   endif
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> index b00a7d37d2fbe..234309f6655f7 100644
> --- a/drivers/mux/Makefile
> +++ b/drivers/mux/Makefile
> @@ -5,3 +5,4 @@
>   obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
>   obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
>   obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
> +obj-$(CONFIG_MUX_SYSCON)	+= mux-syscon.o
> diff --git a/drivers/mux/mux-syscon.c b/drivers/mux/mux-syscon.c
> new file mode 100644
> index 0000000000000..31cacc61f1439
> --- /dev/null
> +++ b/drivers/mux/mux-syscon.c
> @@ -0,0 +1,130 @@
> +/*
> + * syscon bitfield-controlled multiplexer driver
> + *
> + * Copyright (C) 2017 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/mux.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +struct mux_syscon {
> +	struct regmap_field *field;
> +};
> +
> +static int mux_syscon_set(struct mux_control *mux, int state)
> +{
> +	struct mux_syscon *mux_syscon = mux_chip_priv(mux->chip);
> +
> +	return regmap_field_write(mux_syscon->field, state);
> +}
> +
> +static const struct mux_control_ops mux_syscon_ops = {
> +	.set = mux_syscon_set,
> +};
> +
> +static const struct of_device_id mux_syscon_dt_ids[] = {
> +	{ .compatible = "mmio-mux", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mux_syscon_dt_ids);
> +
> +static int of_get_reg_field(struct device_node *node, struct reg_field *field)
> +{
> +	u32 bit_mask;
> +	int ret;
> +
> +	ret = of_property_read_u32(node, "reg", &field->reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = of_property_read_u32(node, "bit-mask", &bit_mask);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = of_property_read_u32(node, "bit-shift", &field->lsb);
> +	if (ret < 0)
> +		return ret;
> +
> +	field->msb = field->lsb + fls(bit_mask) - 1;
> +
> +	return 0;
> +}
> +
> +static int mux_syscon_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mux_chip *mux_chip;
> +	struct mux_syscon *mux_syscon;
> +	struct regmap *regmap;
> +	struct reg_field field;
> +	int bits;
> +	s32 idle_state;
> +	int ret;
> +
> +	ret = of_get_reg_field(pdev->dev.of_node, &field);
> +	if (ret) {
> +		dev_err(&pdev->dev, "missing bit-field properties: %d\n", ret);
> +		return ret;
> +	}
> +
> +	regmap = syscon_node_to_regmap(pdev->dev.of_node->parent);
> +	if (IS_ERR(regmap)) {
> +		ret = PTR_ERR(regmap);
> +		dev_err(&pdev->dev, "failed to get syscon regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_syscon));
> +	if (!mux_chip)
> +		return -ENOMEM;
> +
> +	mux_syscon = mux_chip_priv(mux_chip);
> +	mux_chip->ops = &mux_syscon_ops;
> +
> +	mux_syscon->field = devm_regmap_field_alloc(&pdev->dev, regmap, field);
> +	if (IS_ERR(mux_syscon->field)) {
> +		ret = PTR_ERR(mux_syscon->field);
> +		dev_err(&pdev->dev, "failed to regmap bit-field: %d\n", ret);
> +		return ret;
> +	}
> +	bits = 1 + field.msb - field.lsb;
> +
> +	mux_chip->mux->states = 1 << bits;
> +
> +	ret = device_property_read_u32(dev, "idle-state", (u32 *)&idle_state);
> +	if (ret >= 0 && idle_state != MUX_IDLE_AS_IS) {
> +		if (idle_state < 0 || idle_state >= mux_chip->mux->states) {
> +			dev_err(dev, "invalid idle-state %u\n", idle_state);
> +			return -EINVAL;
> +		}
> +
> +		mux_chip->mux->idle_state = idle_state;
> +	}
> +
> +	regmap_field_read(mux_syscon->field, &mux_chip->mux->cached_state);
> +
> +	return devm_mux_chip_register(dev, mux_chip);
> +}
> +
> +static struct platform_driver mux_syscon_driver = {
> +	.driver = {
> +		.name = "mmio-mux",
> +		.of_match_table	= of_match_ptr(mux_syscon_dt_ids),
> +	},
> +	.probe = mux_syscon_probe,
> +};
> +module_platform_driver(mux_syscon_driver);
> +
> +MODULE_DESCRIPTION("MMIO bitfield-controlled multiplexer driver");
> +MODULE_AUTHOR("Philipp Zabel <p.zabel@pengutronix.de>");
> +MODULE_LICENSE("GPL v2");

Re: [RFC 2/2] mux: mmio-based syscon mux controller

[Philipp Zabel]

On Thu, 2017-04-13 at 18:09 -0700, Steve Longerbeam wrote:
> 
> On 04/13/2017 08:48 AM, Philipp Zabel wrote:
> > This adds a driver for mmio-based syscon multiplexers controlled by a
> > single bitfield in a syscon register range.
> >
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >   drivers/mux/Kconfig      |  13 +++++
> >   drivers/mux/Makefile     |   1 +
> >   drivers/mux/mux-syscon.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 144 insertions(+)
> >   create mode 100644 drivers/mux/mux-syscon.c
> >
> > diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> > index 86668b4d2fc52..a5e6a3b01ac24 100644
> > --- a/drivers/mux/Kconfig
> > +++ b/drivers/mux/Kconfig
> > @@ -43,4 +43,17 @@ config MUX_GPIO
> >   	  To compile the driver as a module, choose M here: the module will
> >   	  be called mux-gpio.
> >   
> > +config MUX_SYSCON
> 
> my preference would be CONFIG_MUX_MMIO.
> 
> > +	tristate "MMIO bitfield-controlled Multiplexer"
> 
> "MMIO register bitfield-controlled Multiplexer"
> 
> The rest looks good to me.

I'll change those. mux-syscon.c should probably be renamed to
mux-mmio.c, too.

regards
Philipp

Re: [RFC 2/2] mux: mmio-based syscon mux controller

[Peter Rosin]

On 2017-04-19 13:50, Philipp Zabel wrote:
> On Thu, 2017-04-13 at 18:09 -0700, Steve Longerbeam wrote:
>>
>> On 04/13/2017 08:48 AM, Philipp Zabel wrote:
>>> This adds a driver for mmio-based syscon multiplexers controlled by a
>>> single bitfield in a syscon register range.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>>   drivers/mux/Kconfig      |  13 +++++
>>>   drivers/mux/Makefile     |   1 +
>>>   drivers/mux/mux-syscon.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 144 insertions(+)
>>>   create mode 100644 drivers/mux/mux-syscon.c
>>>
>>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
>>> index 86668b4d2fc52..a5e6a3b01ac24 100644
>>> --- a/drivers/mux/Kconfig
>>> +++ b/drivers/mux/Kconfig
>>> @@ -43,4 +43,17 @@ config MUX_GPIO
>>>   	  To compile the driver as a module, choose M here: the module will
>>>   	  be called mux-gpio.
>>>   
>>> +config MUX_SYSCON
>>
>> my preference would be CONFIG_MUX_MMIO.
>>
>>> +	tristate "MMIO bitfield-controlled Multiplexer"
>>
>> "MMIO register bitfield-controlled Multiplexer"
>>
>> The rest looks good to me.
> 
> I'll change those. mux-syscon.c should probably be renamed to
> mux-mmio.c, too.

I think I disagree. But I'm not familiar with syscon so I don't know.
IIUC, syscon uses regmap to do mmio and this driver requires syscon
to get at the regmap, and in the end this driver doesn't know anything
about mmio. All it knows is syscon/regmap. If some warped syscon
thing shows up that wraps something other than mmio in its regmap,
this driver wouldn't care about it. And syscon is something that
is also known in the DT world. Given that, I think everything in this
driver should be named syscon and not mmio.

Or?

Cheers,
peda

Re: [RFC 2/2] mux: mmio-based syscon mux controller

[Philipp Zabel]

On Wed, 2017-04-19 at 13:58 +0200, Peter Rosin wrote:
> On 2017-04-19 13:50, Philipp Zabel wrote:
> > On Thu, 2017-04-13 at 18:09 -0700, Steve Longerbeam wrote:
> >>
> >> On 04/13/2017 08:48 AM, Philipp Zabel wrote:
> >>> This adds a driver for mmio-based syscon multiplexers controlled by a
> >>> single bitfield in a syscon register range.
> >>>
> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >>> ---
> >>>   drivers/mux/Kconfig      |  13 +++++
> >>>   drivers/mux/Makefile     |   1 +
> >>>   drivers/mux/mux-syscon.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>   3 files changed, 144 insertions(+)
> >>>   create mode 100644 drivers/mux/mux-syscon.c
> >>>
> >>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> >>> index 86668b4d2fc52..a5e6a3b01ac24 100644
> >>> --- a/drivers/mux/Kconfig
> >>> +++ b/drivers/mux/Kconfig
> >>> @@ -43,4 +43,17 @@ config MUX_GPIO
> >>>   	  To compile the driver as a module, choose M here: the module will
> >>>   	  be called mux-gpio.
> >>>   
> >>> +config MUX_SYSCON
> >>
> >> my preference would be CONFIG_MUX_MMIO.
> >>
> >>> +	tristate "MMIO bitfield-controlled Multiplexer"
> >>
> >> "MMIO register bitfield-controlled Multiplexer"
> >>
> >> The rest looks good to me.
> > 
> > I'll change those. mux-syscon.c should probably be renamed to
> > mux-mmio.c, too.
> 
> I think I disagree. But I'm not familiar with syscon so I don't know.
> IIUC, syscon uses regmap to do mmio and this driver requires syscon
> to get at the regmap, and in the end this driver doesn't know anything
> about mmio. All it knows is syscon/regmap.

That is a good point. Right now there is nothing MMIO about the driver
except for the hardware that I want it to handle.

>  If some warped syscon
> thing shows up that wraps something other than mmio in its regmap,
> this driver wouldn't care about it. And syscon is something that
> is also known in the DT world. Given that, I think everything in this
> driver should be named syscon and not mmio.
> 
> Or?

Well, ...

the driver could be extended to do actual MMIO if the syscon is not
found. This would work only if it has exclusive access to its register.

On the other hand, the driver could also be made to match against
    compatible = "bitfield-mux",
for example, and allow handling muxes inside SPI or I2C controlled MFD
devices that provide a syscon regmap, as you describe:

	spi-host {
		mfd-device {
			compatible = "some-spi-regmap-device";

			mux {
				compatible = "bitfield-mux";
			};
		};
	};

regards
Philipp

Re: [RFC 2/2] mux: mmio-based syscon mux controller

[Steve Longerbeam]

On 04/19/2017 08:27 AM, Philipp Zabel wrote:
> On Wed, 2017-04-19 at 13:58 +0200, Peter Rosin wrote:
>> On 2017-04-19 13:50, Philipp Zabel wrote:
>>> On Thu, 2017-04-13 at 18:09 -0700, Steve Longerbeam wrote:
>>>>
>>>> On 04/13/2017 08:48 AM, Philipp Zabel wrote:
>>>>> This adds a driver for mmio-based syscon multiplexers controlled by a
>>>>> single bitfield in a syscon register range.
>>>>>
>>>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>>>> ---
>>>>>   drivers/mux/Kconfig      |  13 +++++
>>>>>   drivers/mux/Makefile     |   1 +
>>>>>   drivers/mux/mux-syscon.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   3 files changed, 144 insertions(+)
>>>>>   create mode 100644 drivers/mux/mux-syscon.c
>>>>>
>>>>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
>>>>> index 86668b4d2fc52..a5e6a3b01ac24 100644
>>>>> --- a/drivers/mux/Kconfig
>>>>> +++ b/drivers/mux/Kconfig
>>>>> @@ -43,4 +43,17 @@ config MUX_GPIO
>>>>>   	  To compile the driver as a module, choose M here: the module will
>>>>>   	  be called mux-gpio.
>>>>>
>>>>> +config MUX_SYSCON
>>>>
>>>> my preference would be CONFIG_MUX_MMIO.
>>>>
>>>>> +	tristate "MMIO bitfield-controlled Multiplexer"
>>>>
>>>> "MMIO register bitfield-controlled Multiplexer"
>>>>
>>>> The rest looks good to me.
>>>
>>> I'll change those. mux-syscon.c should probably be renamed to
>>> mux-mmio.c, too.
>>
>> I think I disagree. But I'm not familiar with syscon so I don't know.
>> IIUC, syscon uses regmap to do mmio and this driver requires syscon
>> to get at the regmap, and in the end this driver doesn't know anything
>> about mmio. All it knows is syscon/regmap.
>
> That is a good point. Right now there is nothing MMIO about the driver
> except for the hardware that I want it to handle.
>
>>  If some warped syscon
>> thing shows up that wraps something other than mmio in its regmap,
>> this driver wouldn't care about it. And syscon is something that
>> is also known in the DT world. Given that, I think everything in this
>> driver should be named syscon and not mmio.
>>

My argument against using the name "syscon" in the device tree is that
it is referring to a subsystem in the Linux kernel. Besides the fact
that "syscon" does not clearly describe, at least to me, what sort of
device this mux is. "regmap" also has similar problem in that it refers
to  a Linux subsystem, although it is clearer to me at least, what the
mux is composed of (a mapped register). Personally I still like mmio,
most embedded systems access registers via MMIO, and the name is not
referring to any specific Linux subsystem.

Steve




>> Or?
>
> Well, ...
>
> the driver could be extended to do actual MMIO if the syscon is not
> found. This would work only if it has exclusive access to its register.
>
> On the other hand, the driver could also be made to match against
>     compatible = "bitfield-mux",
> for example, and allow handling muxes inside SPI or I2C controlled MFD
> devices that provide a syscon regmap, as you describe:
>
> 	spi-host {
> 		mfd-device {
> 			compatible = "some-spi-regmap-device";
>
> 			mux {
> 				compatible = "bitfield-mux";
> 			};
> 		};
> 	};
>
> regards
> Philipp
>

Re: [RFC 2/2] mux: mmio-based syscon mux controller

[Philipp Zabel]

On Wed, 2017-04-19 at 09:23 -0700, Steve Longerbeam wrote:
> 
> On 04/19/2017 08:27 AM, Philipp Zabel wrote:
> > On Wed, 2017-04-19 at 13:58 +0200, Peter Rosin wrote:
> >> On 2017-04-19 13:50, Philipp Zabel wrote:
> >>> On Thu, 2017-04-13 at 18:09 -0700, Steve Longerbeam wrote:
> >>>>
> >>>> On 04/13/2017 08:48 AM, Philipp Zabel wrote:
> >>>>> This adds a driver for mmio-based syscon multiplexers controlled by a
> >>>>> single bitfield in a syscon register range.
> >>>>>
> >>>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >>>>> ---
> >>>>>   drivers/mux/Kconfig      |  13 +++++
> >>>>>   drivers/mux/Makefile     |   1 +
> >>>>>   drivers/mux/mux-syscon.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>   3 files changed, 144 insertions(+)
> >>>>>   create mode 100644 drivers/mux/mux-syscon.c
> >>>>>
> >>>>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> >>>>> index 86668b4d2fc52..a5e6a3b01ac24 100644
> >>>>> --- a/drivers/mux/Kconfig
> >>>>> +++ b/drivers/mux/Kconfig
> >>>>> @@ -43,4 +43,17 @@ config MUX_GPIO
> >>>>>   	  To compile the driver as a module, choose M here: the module will
> >>>>>   	  be called mux-gpio.
> >>>>>
> >>>>> +config MUX_SYSCON
> >>>>
> >>>> my preference would be CONFIG_MUX_MMIO.
> >>>>
> >>>>> +	tristate "MMIO bitfield-controlled Multiplexer"
> >>>>
> >>>> "MMIO register bitfield-controlled Multiplexer"
> >>>>
> >>>> The rest looks good to me.
> >>>
> >>> I'll change those. mux-syscon.c should probably be renamed to
> >>> mux-mmio.c, too.
> >>
> >> I think I disagree. But I'm not familiar with syscon so I don't know.
> >> IIUC, syscon uses regmap to do mmio and this driver requires syscon
> >> to get at the regmap, and in the end this driver doesn't know anything
> >> about mmio. All it knows is syscon/regmap.
> >
> > That is a good point. Right now there is nothing MMIO about the driver
> > except for the hardware that I want it to handle.
> >
> >>  If some warped syscon
> >> thing shows up that wraps something other than mmio in its regmap,
> >> this driver wouldn't care about it. And syscon is something that
> >> is also known in the DT world. Given that, I think everything in this
> >> driver should be named syscon and not mmio.
> >>
> 
> My argument against using the name "syscon" in the device tree is that
> it is referring to a subsystem in the Linux kernel. Besides the fact
> that "syscon" does not clearly describe, at least to me, what sort of
> device this mux is.

If I'm not mistaken, this point was not about the DT compatible
property, just about the driver name.

I'm also in favor of keeping the "syscon" name out of the device tree as
far as it is still possible, for the same reasons. The i.MX6 muxes are
MMIO register bitfield muxes, but not "syscon muxes".

regards
Philipp

Re: [RFC 2/2] mux: mmio-based syscon mux controller

[Peter Rosin]

On 2017-04-19 18:32, Philipp Zabel wrote:
> On Wed, 2017-04-19 at 09:23 -0700, Steve Longerbeam wrote:
>>
>> On 04/19/2017 08:27 AM, Philipp Zabel wrote:
>>> On Wed, 2017-04-19 at 13:58 +0200, Peter Rosin wrote:
>>>> On 2017-04-19 13:50, Philipp Zabel wrote:
>>>>> On Thu, 2017-04-13 at 18:09 -0700, Steve Longerbeam wrote:
>>>>>>
>>>>>> On 04/13/2017 08:48 AM, Philipp Zabel wrote:
>>>>>>> This adds a driver for mmio-based syscon multiplexers controlled by a
>>>>>>> single bitfield in a syscon register range.
>>>>>>>
>>>>>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>>>>>> ---
>>>>>>>   drivers/mux/Kconfig      |  13 +++++
>>>>>>>   drivers/mux/Makefile     |   1 +
>>>>>>>   drivers/mux/mux-syscon.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>   3 files changed, 144 insertions(+)
>>>>>>>   create mode 100644 drivers/mux/mux-syscon.c
>>>>>>>
>>>>>>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
>>>>>>> index 86668b4d2fc52..a5e6a3b01ac24 100644
>>>>>>> --- a/drivers/mux/Kconfig
>>>>>>> +++ b/drivers/mux/Kconfig
>>>>>>> @@ -43,4 +43,17 @@ config MUX_GPIO
>>>>>>>   	  To compile the driver as a module, choose M here: the module will
>>>>>>>   	  be called mux-gpio.
>>>>>>>
>>>>>>> +config MUX_SYSCON
>>>>>>
>>>>>> my preference would be CONFIG_MUX_MMIO.
>>>>>>
>>>>>>> +	tristate "MMIO bitfield-controlled Multiplexer"
>>>>>>
>>>>>> "MMIO register bitfield-controlled Multiplexer"
>>>>>>
>>>>>> The rest looks good to me.
>>>>>
>>>>> I'll change those. mux-syscon.c should probably be renamed to
>>>>> mux-mmio.c, too.
>>>>
>>>> I think I disagree. But I'm not familiar with syscon so I don't know.
>>>> IIUC, syscon uses regmap to do mmio and this driver requires syscon
>>>> to get at the regmap, and in the end this driver doesn't know anything
>>>> about mmio. All it knows is syscon/regmap.
>>>
>>> That is a good point. Right now there is nothing MMIO about the driver
>>> except for the hardware that I want it to handle.
>>>
>>>>  If some warped syscon
>>>> thing shows up that wraps something other than mmio in its regmap,
>>>> this driver wouldn't care about it. And syscon is something that
>>>> is also known in the DT world. Given that, I think everything in this
>>>> driver should be named syscon and not mmio.
>>>>
>>
>> My argument against using the name "syscon" in the device tree is that
>> it is referring to a subsystem in the Linux kernel. Besides the fact
>> that "syscon" does not clearly describe, at least to me, what sort of
>> device this mux is.
> 
> If I'm not mistaken, this point was not about the DT compatible
> property, just about the driver name.
> 
> I'm also in favor of keeping the "syscon" name out of the device tree as
> far as it is still possible, for the same reasons. The i.MX6 muxes are
> MMIO register bitfield muxes, but not "syscon muxes".

The corresponding mux in the i2c-mux "sub-sub-system" is named i2c-mux-reg.
It is about "raw" mmio, i.e. w/o syscon and regmap. Just for the record...

Cheers,
peda

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Steve Longerbeam]

On 04/13/2017 08:48 AM, Philipp Zabel wrote:
> This adds device tree binding documentation for mmio-based syscon
> multiplexers controlled by a single bitfield in a syscon register
> range.

Nice! (you beat me to it, I was about to embark on this myself :)

Looks good to me, just some minor comments below.

>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>   Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
>   1 file changed, 56 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
>
> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> new file mode 100644
> index 0000000000000..11d96f5d98583
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> @@ -0,0 +1,56 @@
> +MMIO bitfield-based multiplexer controller bindings
> +
> +Define a syscon bitfield to be used to control a multiplexer. The parent

I think "Define a register bitfield to be used ..." is more clear.

> +device tree node must be a syscon node to provide register access.
> +
> +Required properties:
> +- compatible : "gpio-mux"

Er, "mmio-mux"

> +- reg : register base of the register containing the control bitfield
> +- bit-mask : bitmask of the control bitfield in the control register
> +- bit-shift : bit offset of the control bitfield in the control register
> +- #mux-control-cells : <0>
> +* Standard mux-controller bindings as decribed in mux-controller.txt
> +
> +Optional properties:
> +- idle-state : if present, the state the mux will have when idle. The
> +	       special state MUX_IDLE_AS_IS is the default.
> +
> +The multiplexer state is defined as the value of the bitfield described
> +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> +syscon.
> +
> +Example:
> +
> +	syscon {
> +		compatible = "syscon";
> +
> +		mux: mux-controller@3 {
> +			compatible = "mmio-mux";
> +			reg = <0x3>;
> +			bit-mask = <0x1>;
> +			bit-shift = <5>;
> +			#mux-control-cells = <0>;
> +		};
> +	};
> +
> +	video-mux {

I like this as an example consumer of a mmio-mux, but just
the same some might argue this doesn't really fit here.

Steve

> +		compatible = "video-mux";
> +		mux-controls = <&mux>;
> +
> +		ports {
> +			/* input 0 */
> +			port@0 {
> +				reg = <0>;
> +			};
> +
> +			/* input 1 */
> +			port@1 {
> +				reg = <1>;
> +			};
> +
> +			/* output */
> +			port@2 {
> +				reg = <2>;
> +			};
> +		};
> +	};

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Philipp Zabel]

On Thu, 2017-04-13 at 18:03 -0700, Steve Longerbeam wrote:
> 
> On 04/13/2017 08:48 AM, Philipp Zabel wrote:
> > This adds device tree binding documentation for mmio-based syscon
> > multiplexers controlled by a single bitfield in a syscon register
> > range.
> 
> Nice! (you beat me to it, I was about to embark on this myself :)
> 
> Looks good to me, just some minor comments below.
> 
[...]
> > +Define a syscon bitfield to be used to control a multiplexer. The parent
> I think "Define a register bitfield to be used ..." is more clear.
[...]
> > +- compatible : "gpio-mux"
> Er, "mmio-mux"

I'll change those, thanks.

[...]
> > +Example:
> > +
> > +	syscon {
> > +		compatible = "syscon";
> > +
> > +		mux: mux-controller@3 {
> > +			compatible = "mmio-mux";
> > +			reg = <0x3>;
> > +			bit-mask = <0x1>;
> > +			bit-shift = <5>;
> > +			#mux-control-cells = <0>;
> > +		};
> > +	};
> > +
> > +	video-mux {
> 
> I like this as an example consumer of a mmio-mux, but just
> the same some might argue this doesn't really fit here.

I don't think this is a problem, of course assuming that this video-mux
binding will actually come into existence.

regards
Philipp

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Philipp Zabel]

On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> This adds device tree binding documentation for mmio-based syscon
> multiplexers controlled by a single bitfield in a syscon register
> range.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> 
> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> new file mode 100644
> index 0000000000000..11d96f5d98583
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> @@ -0,0 +1,56 @@
> +MMIO bitfield-based multiplexer controller bindings
> +
> +Define a syscon bitfield to be used to control a multiplexer. The parent
> +device tree node must be a syscon node to provide register access.
> +
> +Required properties:
> +- compatible : "gpio-mux"
> +- reg : register base of the register containing the control bitfield
> +- bit-mask : bitmask of the control bitfield in the control register
> +- bit-shift : bit offset of the control bitfield in the control register
> +- #mux-control-cells : <0>
> +* Standard mux-controller bindings as decribed in mux-controller.txt
> +
> +Optional properties:
> +- idle-state : if present, the state the mux will have when idle. The
> +	       special state MUX_IDLE_AS_IS is the default.
> +
> +The multiplexer state is defined as the value of the bitfield described
> +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> +syscon.
> +
> +Example:
> +
> +	syscon {
> +		compatible = "syscon";
> +
> +		mux: mux-controller@3 {
> +			compatible = "mmio-mux";
> +			reg = <0x3>;
> +			bit-mask = <0x1>;
> +			bit-shift = <5>;
> +			#mux-control-cells = <0>;
> +		};
> +	};
> +
> +	video-mux {
> +		compatible = "video-mux";
> +		mux-controls = <&mux>;
> +
> +		ports {
> +			/* input 0 */
> +			port@0 {
> +				reg = <0>;
> +			};
> +
> +			/* input 1 */
> +			port@1 {
> +				reg = <1>;
> +			};
> +
> +			/* output */
> +			port@2 {
> +				reg = <2>;
> +			};
> +		};
> +	};

So Pavel (added to Cc:) suggested to merge these into one node for the
video mux, as really we are describing a single hardware entity that
happens to be multiplexing multiple video buses into one:

	syscon {
		compatible = "syscon";

		/* video multiplexer */
		mux: mux-controller@3 {
			compatible = "video-mmio-mux";
			reg = <0x3>;
			bit-mask = <0x1>;
			bit-shift = <5>;
			#mux-control-cells = <0>;
 
			mux-controls = <&mux>;

			ports {
				/* input 0 */
				port@0 {
					reg = <0>;
				};

				/* input 1 */
				port@1 {
					reg = <1>;
				};

				/* output */
				port@2 {
					reg = <2>;
				};
			};
		};
	};

That would not touch on this "general purpose" mmio-mux binding itself,
but would make it necessary to add a separate "video-mmio-mux" and a
"video-gpio-mux" binding that mirror the "mmio-mux" and "gpio-mux"
bindings but add the OF-graph connections.

Also I think in this case the self-referencing mux-controls property
would be superfluous, as the driver binding to this node is expected to
control the mux according to activation of the links described by the
OF-graph bindings.

regards
Philipp

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Sakari Ailus]

Hi Philipp,

On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > This adds device tree binding documentation for mmio-based syscon
> > multiplexers controlled by a single bitfield in a syscon register
> > range.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > new file mode 100644
> > index 0000000000000..11d96f5d98583
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > @@ -0,0 +1,56 @@
> > +MMIO bitfield-based multiplexer controller bindings
> > +
> > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > +device tree node must be a syscon node to provide register access.
> > +
> > +Required properties:
> > +- compatible : "gpio-mux"
> > +- reg : register base of the register containing the control bitfield
> > +- bit-mask : bitmask of the control bitfield in the control register
> > +- bit-shift : bit offset of the control bitfield in the control register
> > +- #mux-control-cells : <0>
> > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > +
> > +Optional properties:
> > +- idle-state : if present, the state the mux will have when idle. The
> > +	       special state MUX_IDLE_AS_IS is the default.
> > +
> > +The multiplexer state is defined as the value of the bitfield described
> > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > +syscon.
> > +
> > +Example:
> > +
> > +	syscon {
> > +		compatible = "syscon";
> > +
> > +		mux: mux-controller@3 {
> > +			compatible = "mmio-mux";
> > +			reg = <0x3>;
> > +			bit-mask = <0x1>;
> > +			bit-shift = <5>;
> > +			#mux-control-cells = <0>;
> > +		};
> > +	};
> > +
> > +	video-mux {
> > +		compatible = "video-mux";
> > +		mux-controls = <&mux>;
> > +
> > +		ports {
> > +			/* input 0 */
> > +			port@0 {
> > +				reg = <0>;
> > +			};
> > +
> > +			/* input 1 */
> > +			port@1 {
> > +				reg = <1>;
> > +			};
> > +
> > +			/* output */
> > +			port@2 {
> > +				reg = <2>;
> > +			};
> > +		};
> > +	};
> 
> So Pavel (added to Cc:) suggested to merge these into one node for the
> video mux, as really we are describing a single hardware entity that
> happens to be multiplexing multiple video buses into one:

Two drivers will be needed in a way or another to disconnect the dependency
between the video switch driver and the MUX implementation. Are there ways
to do that cleanly other than having two devices?

And if there are two devices, shouldn't the video switch device be a child
of the MUX device? I think it'd be odd to have it hanging around in a
completely unrelated part of the device tree.

> 
> 	syscon {
> 		compatible = "syscon";
> 
> 		/* video multiplexer */
> 		mux: mux-controller@3 {
> 			compatible = "video-mmio-mux";
> 			reg = <0x3>;
> 			bit-mask = <0x1>;
> 			bit-shift = <5>;
> 			#mux-control-cells = <0>;
>  
> 			mux-controls = <&mux>;
> 
> 			ports {
> 				/* input 0 */
> 				port@0 {
> 					reg = <0>;
> 				};
> 
> 				/* input 1 */
> 				port@1 {
> 					reg = <1>;
> 				};
> 
> 				/* output */
> 				port@2 {
> 					reg = <2>;
> 				};
> 			};
> 		};
> 	};
> 
> That would not touch on this "general purpose" mmio-mux binding itself,
> but would make it necessary to add a separate "video-mmio-mux" and a
> "video-gpio-mux" binding that mirror the "mmio-mux" and "gpio-mux"
> bindings but add the OF-graph connections.
> 
> Also I think in this case the self-referencing mux-controls property
> would be superfluous, as the driver binding to this node is expected to
> control the mux according to activation of the links described by the
> OF-graph bindings.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3344 bytes --]

On Tue 2017-04-18 13:08:41, Sakari Ailus wrote:
> Hi Philipp,
> 
> On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> > On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > > This adds device tree binding documentation for mmio-based syscon
> > > multiplexers controlled by a single bitfield in a syscon register
> > > range.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > >  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > >  1 file changed, 56 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > new file mode 100644
> > > index 0000000000000..11d96f5d98583
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > @@ -0,0 +1,56 @@
> > > +MMIO bitfield-based multiplexer controller bindings
> > > +
> > > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > > +device tree node must be a syscon node to provide register access.
> > > +
> > > +Required properties:
> > > +- compatible : "gpio-mux"
> > > +- reg : register base of the register containing the control bitfield
> > > +- bit-mask : bitmask of the control bitfield in the control register
> > > +- bit-shift : bit offset of the control bitfield in the control register
> > > +- #mux-control-cells : <0>
> > > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > > +
> > > +Optional properties:
> > > +- idle-state : if present, the state the mux will have when idle. The
> > > +	       special state MUX_IDLE_AS_IS is the default.
> > > +
> > > +The multiplexer state is defined as the value of the bitfield described
> > > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > > +syscon.
> > > +
> > > +Example:
> > > +
> > > +	syscon {
> > > +		compatible = "syscon";
> > > +
> > > +		mux: mux-controller@3 {
> > > +			compatible = "mmio-mux";
> > > +			reg = <0x3>;
> > > +			bit-mask = <0x1>;
> > > +			bit-shift = <5>;
> > > +			#mux-control-cells = <0>;
> > > +		};
> > > +	};
> > > +
> > > +	video-mux {
> > > +		compatible = "video-mux";
> > > +		mux-controls = <&mux>;
> > > +
> > > +		ports {
> > > +			/* input 0 */
> > > +			port@0 {
> > > +				reg = <0>;
> > > +			};
> > > +
> > > +			/* input 1 */
> > > +			port@1 {
> > > +				reg = <1>;
> > > +			};
> > > +
> > > +			/* output */
> > > +			port@2 {
> > > +				reg = <2>;
> > > +			};
> > > +		};
> > > +	};
> > 
> > So Pavel (added to Cc:) suggested to merge these into one node for the
> > video mux, as really we are describing a single hardware entity that
> > happens to be multiplexing multiple video buses into one:
> 
> Two drivers will be needed in a way or another to disconnect the dependency
> between the video switch driver and the MUX implementation. Are there ways
> to do that cleanly other than having two devices?

Yes.

Device tree describes hardware, not the driver structure.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Sakari Ailus]

On Tue, Apr 18, 2017 at 12:34:53PM +0200, Pavel Machek wrote:
> On Tue 2017-04-18 13:08:41, Sakari Ailus wrote:
> > Hi Philipp,
> > 
> > On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> > > On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > > > This adds device tree binding documentation for mmio-based syscon
> > > > multiplexers controlled by a single bitfield in a syscon register
> > > > range.
> > > > 
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > ---
> > > >  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > > >  1 file changed, 56 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > new file mode 100644
> > > > index 0000000000000..11d96f5d98583
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > @@ -0,0 +1,56 @@
> > > > +MMIO bitfield-based multiplexer controller bindings
> > > > +
> > > > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > > > +device tree node must be a syscon node to provide register access.
> > > > +
> > > > +Required properties:
> > > > +- compatible : "gpio-mux"
> > > > +- reg : register base of the register containing the control bitfield
> > > > +- bit-mask : bitmask of the control bitfield in the control register
> > > > +- bit-shift : bit offset of the control bitfield in the control register
> > > > +- #mux-control-cells : <0>
> > > > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > > > +
> > > > +Optional properties:
> > > > +- idle-state : if present, the state the mux will have when idle. The
> > > > +	       special state MUX_IDLE_AS_IS is the default.
> > > > +
> > > > +The multiplexer state is defined as the value of the bitfield described
> > > > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > > > +syscon.
> > > > +
> > > > +Example:
> > > > +
> > > > +	syscon {
> > > > +		compatible = "syscon";
> > > > +
> > > > +		mux: mux-controller@3 {
> > > > +			compatible = "mmio-mux";
> > > > +			reg = <0x3>;
> > > > +			bit-mask = <0x1>;
> > > > +			bit-shift = <5>;
> > > > +			#mux-control-cells = <0>;
> > > > +		};
> > > > +	};
> > > > +
> > > > +	video-mux {
> > > > +		compatible = "video-mux";
> > > > +		mux-controls = <&mux>;
> > > > +
> > > > +		ports {
> > > > +			/* input 0 */
> > > > +			port@0 {
> > > > +				reg = <0>;
> > > > +			};
> > > > +
> > > > +			/* input 1 */
> > > > +			port@1 {
> > > > +				reg = <1>;
> > > > +			};
> > > > +
> > > > +			/* output */
> > > > +			port@2 {
> > > > +				reg = <2>;
> > > > +			};
> > > > +		};
> > > > +	};
> > > 
> > > So Pavel (added to Cc:) suggested to merge these into one node for the
> > > video mux, as really we are describing a single hardware entity that
> > > happens to be multiplexing multiple video buses into one:
> > 
> > Two drivers will be needed in a way or another to disconnect the dependency
> > between the video switch driver and the MUX implementation. Are there ways
> > to do that cleanly other than having two devices?
> 
> Yes.
> 
> Device tree describes hardware, not the driver structure.

I think you you could view the MUX control as a device, too, and that's
separate from the actual video switch.

This isn't really related to the video switch as much as it's got to do with
the MUX framework, so having Peter's opinion here would be very helpful.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4114 bytes --]

On Tue 2017-04-18 13:55:40, Sakari Ailus wrote:
> On Tue, Apr 18, 2017 at 12:34:53PM +0200, Pavel Machek wrote:
> > On Tue 2017-04-18 13:08:41, Sakari Ailus wrote:
> > > Hi Philipp,
> > > 
> > > On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> > > > On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > > > > This adds device tree binding documentation for mmio-based syscon
> > > > > multiplexers controlled by a single bitfield in a syscon register
> > > > > range.
> > > > > 
> > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > > > >  1 file changed, 56 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > > new file mode 100644
> > > > > index 0000000000000..11d96f5d98583
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > > @@ -0,0 +1,56 @@
> > > > > +MMIO bitfield-based multiplexer controller bindings
> > > > > +
> > > > > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > > > > +device tree node must be a syscon node to provide register access.
> > > > > +
> > > > > +Required properties:
> > > > > +- compatible : "gpio-mux"
> > > > > +- reg : register base of the register containing the control bitfield
> > > > > +- bit-mask : bitmask of the control bitfield in the control register
> > > > > +- bit-shift : bit offset of the control bitfield in the control register
> > > > > +- #mux-control-cells : <0>
> > > > > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > > > > +
> > > > > +Optional properties:
> > > > > +- idle-state : if present, the state the mux will have when idle. The
> > > > > +	       special state MUX_IDLE_AS_IS is the default.
> > > > > +
> > > > > +The multiplexer state is defined as the value of the bitfield described
> > > > > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > > > > +syscon.
> > > > > +
> > > > > +Example:
> > > > > +
> > > > > +	syscon {
> > > > > +		compatible = "syscon";
> > > > > +
> > > > > +		mux: mux-controller@3 {
> > > > > +			compatible = "mmio-mux";
> > > > > +			reg = <0x3>;
> > > > > +			bit-mask = <0x1>;
> > > > > +			bit-shift = <5>;
> > > > > +			#mux-control-cells = <0>;
> > > > > +		};
> > > > > +	};
> > > > > +
> > > > > +	video-mux {
> > > > > +		compatible = "video-mux";
> > > > > +		mux-controls = <&mux>;
> > > > > +
> > > > > +		ports {
> > > > > +			/* input 0 */
> > > > > +			port@0 {
> > > > > +				reg = <0>;
> > > > > +			};
> > > > > +
> > > > > +			/* input 1 */
> > > > > +			port@1 {
> > > > > +				reg = <1>;
> > > > > +			};
> > > > > +
> > > > > +			/* output */
> > > > > +			port@2 {
> > > > > +				reg = <2>;
> > > > > +			};
> > > > > +		};
> > > > > +	};
> > > > 
> > > > So Pavel (added to Cc:) suggested to merge these into one node for the
> > > > video mux, as really we are describing a single hardware entity that
> > > > happens to be multiplexing multiple video buses into one:
> > > 
> > > Two drivers will be needed in a way or another to disconnect the dependency
> > > between the video switch driver and the MUX implementation. Are there ways
> > > to do that cleanly other than having two devices?
> > 
> > Yes.
> > 
> > Device tree describes hardware, not the driver structure.
> 
> I think you you could view the MUX control as a device, too, and that's
> separate from the actual video switch.

Actually, I believe what matters here is hardware. There's some chip,
somewhere, that does the switching, and the device tree should should
basically describe that switch.


									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Philipp Zabel]

On Tue, 2017-04-18 at 13:08 +0300, Sakari Ailus wrote:
> Hi Philipp,
> 
> On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> > On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > > This adds device tree binding documentation for mmio-based syscon
> > > multiplexers controlled by a single bitfield in a syscon register
> > > range.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > >  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > >  1 file changed, 56 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > new file mode 100644
> > > index 0000000000000..11d96f5d98583
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > @@ -0,0 +1,56 @@
> > > +MMIO bitfield-based multiplexer controller bindings
> > > +
> > > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > > +device tree node must be a syscon node to provide register access.
> > > +
> > > +Required properties:
> > > +- compatible : "gpio-mux"
> > > +- reg : register base of the register containing the control bitfield
> > > +- bit-mask : bitmask of the control bitfield in the control register
> > > +- bit-shift : bit offset of the control bitfield in the control register
> > > +- #mux-control-cells : <0>
> > > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > > +
> > > +Optional properties:
> > > +- idle-state : if present, the state the mux will have when idle. The
> > > +	       special state MUX_IDLE_AS_IS is the default.
> > > +
> > > +The multiplexer state is defined as the value of the bitfield described
> > > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > > +syscon.
> > > +
> > > +Example:
> > > +
> > > +	syscon {
> > > +		compatible = "syscon";
> > > +
> > > +		mux: mux-controller@3 {
> > > +			compatible = "mmio-mux";
> > > +			reg = <0x3>;
> > > +			bit-mask = <0x1>;
> > > +			bit-shift = <5>;
> > > +			#mux-control-cells = <0>;
> > > +		};
> > > +	};
> > > +
> > > +	video-mux {
> > > +		compatible = "video-mux";
> > > +		mux-controls = <&mux>;
> > > +
> > > +		ports {
> > > +			/* input 0 */
> > > +			port@0 {
> > > +				reg = <0>;
> > > +			};
> > > +
> > > +			/* input 1 */
> > > +			port@1 {
> > > +				reg = <1>;
> > > +			};
> > > +
> > > +			/* output */
> > > +			port@2 {
> > > +				reg = <2>;
> > > +			};
> > > +		};
> > > +	};
> > 
> > So Pavel (added to Cc:) suggested to merge these into one node for the
> > video mux, as really we are describing a single hardware entity that
> > happens to be multiplexing multiple video buses into one:
> 
> Two drivers will be needed in a way or another to disconnect the dependency
> between the video switch driver and the MUX implementation. Are there ways
> to do that cleanly other than having two devices?

We are talking about the device tree bindings, drivers and devices
shouldn't factor into it yet. A video-mmio-mux driver could very well
create a mmio-mux platform device internally, if necessary. Or it could
just use the same library functions that the mmio-mux driver uses,
without creating a second device.

> And if there are two devices, shouldn't the video switch device be a child
> of the MUX device? I think it'd be odd to have it hanging around in a
> completely unrelated part of the device tree.

That boils down to whether you consider the connection between mux
controller and mux to be resource usage that should be described via a
phandle reference, like pwms, gpios, clocks, and so on, or whether you
consider it to be control flow in the device tree sense, which should be
described as a parent-child relationship of the nodes. The mux framework
is designed around the former.

We could embrace this and consider a syscon region that contains
multiple mux bitfields as one mux controller that controls multiple
muxes, with a binding similar to, for example, the
reset/ti-syscon-reset.txt bindings:

	syscon {
		compatible = "syscon";

		mux: mux-controller@4 {
			compatible = "mmio-mux";

			/* register, bit shift, bit mask */
			mux-bits = <0x4 19 0x1>, /* 0: CSI0 mux */
				   <0x4 20 0x1>; /* 1: CSI1 mux */
			#mux-control-cells = <1>;
		};
	};

	/* somewhere else */

	csi0-mux {
		compatible = "video-mux";
		mux-controls = <&mux 0>;

		ports {
			/* ... */
		};
	};

	csi1-mux {
		compatible = "video-mux";
		mux-controls = <&mux 1>;

		ports {
			/* ... */
		};
	};

regards
Philipp

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Rob Herring]

On Thu, Apr 13, 2017 at 05:48:11PM +0200, Philipp Zabel wrote:
> This adds device tree binding documentation for mmio-based syscon
> multiplexers controlled by a single bitfield in a syscon register
> range.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> 
> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> new file mode 100644
> index 0000000000000..11d96f5d98583
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> @@ -0,0 +1,56 @@
> +MMIO bitfield-based multiplexer controller bindings
> +
> +Define a syscon bitfield to be used to control a multiplexer. The parent
> +device tree node must be a syscon node to provide register access.
> +
> +Required properties:
> +- compatible : "gpio-mux"

?

> +- reg : register base of the register containing the control bitfield
> +- bit-mask : bitmask of the control bitfield in the control register
> +- bit-shift : bit offset of the control bitfield in the control register
> +- #mux-control-cells : <0>
> +* Standard mux-controller bindings as decribed in mux-controller.txt
> +
> +Optional properties:
> +- idle-state : if present, the state the mux will have when idle. The
> +	       special state MUX_IDLE_AS_IS is the default.
> +
> +The multiplexer state is defined as the value of the bitfield described
> +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> +syscon.
> +
> +Example:
> +
> +	syscon {
> +		compatible = "syscon";
> +
> +		mux: mux-controller@3 {
> +			compatible = "mmio-mux";
> +			reg = <0x3>;
> +			bit-mask = <0x1>;
> +			bit-shift = <5>;

This pattern doesn't scale once you have multiple fields @ addr 3. I 
also don't really think a node per register field in DT really scales.

I think the parent should be declared as a mux controller instead. You 
could encode the mux addr and bit position in the mux cells.

> +			#mux-control-cells = <0>;
> +		};
> +	};
> +
> +	video-mux {
> +		compatible = "video-mux";
> +		mux-controls = <&mux>;

The mux binding was largely defined for a single control controling 
multiple muxes. This doesn't really fit that, but I guess this is an 
improvement over a custom syscon phandle.

> +
> +		ports {
> +			/* input 0 */
> +			port@0 {
> +				reg = <0>;
> +			};
> +
> +			/* input 1 */
> +			port@1 {
> +				reg = <1>;
> +			};
> +
> +			/* output */
> +			port@2 {
> +				reg = <2>;
> +			};
> +		};
> +	};
> -- 
> 2.11.0
> 

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Philipp Zabel]

Hi Rob,

On Wed, 2017-04-19 at 17:09 -0500, Rob Herring wrote:
> On Thu, Apr 13, 2017 at 05:48:11PM +0200, Philipp Zabel wrote:
> > This adds device tree binding documentation for mmio-based syscon
> > multiplexers controlled by a single bitfield in a syscon register
> > range.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > new file mode 100644
> > index 0000000000000..11d96f5d98583
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > @@ -0,0 +1,56 @@
> > +MMIO bitfield-based multiplexer controller bindings
> > +
> > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > +device tree node must be a syscon node to provide register access.
> > +
> > +Required properties:
> > +- compatible : "gpio-mux"
> 
> ?
>
> > +- reg : register base of the register containing the control bitfield
> > +- bit-mask : bitmask of the control bitfield in the control register
> > +- bit-shift : bit offset of the control bitfield in the control register
> > +- #mux-control-cells : <0>
> > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > +
> > +Optional properties:
> > +- idle-state : if present, the state the mux will have when idle. The
> > +	       special state MUX_IDLE_AS_IS is the default.
> > +
> > +The multiplexer state is defined as the value of the bitfield described
> > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > +syscon.
> > +
> > +Example:
> > +
> > +	syscon {
> > +		compatible = "syscon";
> > +
> > +		mux: mux-controller@3 {
> > +			compatible = "mmio-mux";
> > +			reg = <0x3>;
> > +			bit-mask = <0x1>;
> > +			bit-shift = <5>;
> 
> This pattern doesn't scale once you have multiple fields @ addr 3. I 
> also don't really think a node per register field in DT really scales.

Thanks, I have been a bit uneasy with the separate per-bitfield mux
controller node, so I'm eager to agree. But thit makes me unsure how to
best represent the information that is spelled out above.

> I think the parent should be declared as a mux controller instead.

The syscon node itself should be the mux controller? Would you expect
the mmio-mux driver bind to the syscon node, or should the mux framework
handle creation of the mux controls in this case (i.e. does the syscon
node get a "mmio-mux" added to its compatible list)?

> You could encode the mux addr and bit position in the mux cells.

What about the bit-mask / bitfield width? Just add a cell for it?

	gpr: syscon {
		compatible = "mmio-mux", "syscon", "simple-mfd";
		#mux-control-cells = <3>;

		video-mux {
			compatible = "video-mux";
			/* register 0x3, bits [6:5] */
			mux-controls = <&gpr 0x3 5 0x3>;

			ports {
				/* ports 0..5 */
			};
		};
	};

Or maybe using MSB and LSB would be better to read?

		video-mux {
			/* register 0x3, bits [6:5] */
			mux-control = <&gpr 0x3 6 5>;

			ports {
				/* ports 0..5 */
			};
		};

> > +			#mux-control-cells = <0>;
> > +		};
> > +	};
> > +
> > +	video-mux {
> > +		compatible = "video-mux";
> > +		mux-controls = <&mux>;
> 
> The mux binding was largely defined for a single control controling 
> multiple muxes. This doesn't really fit that, but I guess this is an 
> improvement over a custom syscon phandle.

What I especially like about the mux-controls property is that would
allow me to use the gpio-mux driver (or any other mux controller)
instead of having to code variants of the video-mux for all possible
control schemes.

regards
Philipp

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Peter Rosin]

On 2017-04-20 10:14, Philipp Zabel wrote:
> Hi Rob,
> 
> On Wed, 2017-04-19 at 17:09 -0500, Rob Herring wrote:
>> On Thu, Apr 13, 2017 at 05:48:11PM +0200, Philipp Zabel wrote:
>>> This adds device tree binding documentation for mmio-based syscon
>>> multiplexers controlled by a single bitfield in a syscon register
>>> range.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>>  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
>>>  1 file changed, 56 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
>>> new file mode 100644
>>> index 0000000000000..11d96f5d98583
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
>>> @@ -0,0 +1,56 @@
>>> +MMIO bitfield-based multiplexer controller bindings
>>> +
>>> +Define a syscon bitfield to be used to control a multiplexer. The parent
>>> +device tree node must be a syscon node to provide register access.
>>> +
>>> +Required properties:
>>> +- compatible : "gpio-mux"
>>
>> ?
>>
>>> +- reg : register base of the register containing the control bitfield
>>> +- bit-mask : bitmask of the control bitfield in the control register
>>> +- bit-shift : bit offset of the control bitfield in the control register
>>> +- #mux-control-cells : <0>
>>> +* Standard mux-controller bindings as decribed in mux-controller.txt
>>> +
>>> +Optional properties:
>>> +- idle-state : if present, the state the mux will have when idle. The
>>> +	       special state MUX_IDLE_AS_IS is the default.
>>> +
>>> +The multiplexer state is defined as the value of the bitfield described
>>> +by the reg, bit-mask, and bit-shift properties, accessed through the parent
>>> +syscon.
>>> +
>>> +Example:
>>> +
>>> +	syscon {
>>> +		compatible = "syscon";
>>> +
>>> +		mux: mux-controller@3 {
>>> +			compatible = "mmio-mux";
>>> +			reg = <0x3>;
>>> +			bit-mask = <0x1>;
>>> +			bit-shift = <5>;
>>
>> This pattern doesn't scale once you have multiple fields @ addr 3. I 
>> also don't really think a node per register field in DT really scales.
> 
> Thanks, I have been a bit uneasy with the separate per-bitfield mux
> controller node, so I'm eager to agree. But thit makes me unsure how to
> best represent the information that is spelled out above.
> 
>> I think the parent should be declared as a mux controller instead.
> 
> The syscon node itself should be the mux controller? Would you expect
> the mmio-mux driver bind to the syscon node, or should the mux framework
> handle creation of the mux controls in this case (i.e. does the syscon
> node get a "mmio-mux" added to its compatible list)?
> 
>> You could encode the mux addr and bit position in the mux cells.
> 
> What about the bit-mask / bitfield width? Just add a cell for it?
> 
> 	gpr: syscon {
> 		compatible = "mmio-mux", "syscon", "simple-mfd";
> 		#mux-control-cells = <3>;
> 
> 		video-mux {
> 			compatible = "video-mux";
> 			/* register 0x3, bits [6:5] */
> 			mux-controls = <&gpr 0x3 5 0x3>;
> 
> 			ports {
> 				/* ports 0..5 */
> 			};
> 		};
> 	};
> 
> Or maybe using MSB and LSB would be better to read?
> 
> 		video-mux {
> 			/* register 0x3, bits [6:5] */
> 			mux-control = <&gpr 0x3 6 5>;
> 
> 			ports {
> 				/* ports 0..5 */
> 			};
> 		};

Why do you need three values for one register+field? The shift can be
implied from the mask, if the mask is pre-shifted. I.e. specifying a
mask of 0x60 in this case. What I'm I missing?

>>> +			#mux-control-cells = <0>;
>>> +		};
>>> +	};
>>> +
>>> +	video-mux {
>>> +		compatible = "video-mux";
>>> +		mux-controls = <&mux>;
>>
>> The mux binding was largely defined for a single control controling 
>> multiple muxes. This doesn't really fit that, but I guess this is an 
>> improvement over a custom syscon phandle.
> 
> What I especially like about the mux-controls property is that would
> allow me to use the gpio-mux driver (or any other mux controller)
> instead of having to code variants of the video-mux for all possible
> control schemes.

Yes, when talking about e.g. PWMs or GPIOs, there seem to be little
question that they can sit in their own nodes. I don't get the reluctance
to have MUXes in nodes of their own?

PWMs and GPIOs are controlled from one end and mostly used for something
completely different. Just like MUXes. At least there are many MUXes like
that. Agreed, there are also many MUXes where the MUX is tightly integrated
in some bigger function. But in this case, it appears that there is benefit
in moving the MUX to its own DT node and have the MUX consumer oblivious
of the details of the MUX. Why is that a problem?

Cheers,
peda

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Philipp Zabel]

On Thu, 2017-04-20 at 13:57 +0200, Peter Rosin wrote:
> On 2017-04-20 10:14, Philipp Zabel wrote:
> > Hi Rob,
> > 
> > On Wed, 2017-04-19 at 17:09 -0500, Rob Herring wrote:
> >> On Thu, Apr 13, 2017 at 05:48:11PM +0200, Philipp Zabel wrote:
> >>> This adds device tree binding documentation for mmio-based syscon
> >>> multiplexers controlled by a single bitfield in a syscon register
> >>> range.
> >>>
> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >>> ---
> >>>  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> >>>  1 file changed, 56 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> >>> new file mode 100644
> >>> index 0000000000000..11d96f5d98583
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> >>> @@ -0,0 +1,56 @@
> >>> +MMIO bitfield-based multiplexer controller bindings
> >>> +
> >>> +Define a syscon bitfield to be used to control a multiplexer. The parent
> >>> +device tree node must be a syscon node to provide register access.
> >>> +
> >>> +Required properties:
> >>> +- compatible : "gpio-mux"
> >>
> >> ?
> >>
> >>> +- reg : register base of the register containing the control bitfield
> >>> +- bit-mask : bitmask of the control bitfield in the control register
> >>> +- bit-shift : bit offset of the control bitfield in the control register
> >>> +- #mux-control-cells : <0>
> >>> +* Standard mux-controller bindings as decribed in mux-controller.txt
> >>> +
> >>> +Optional properties:
> >>> +- idle-state : if present, the state the mux will have when idle. The
> >>> +	       special state MUX_IDLE_AS_IS is the default.
> >>> +
> >>> +The multiplexer state is defined as the value of the bitfield described
> >>> +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> >>> +syscon.
> >>> +
> >>> +Example:
> >>> +
> >>> +	syscon {
> >>> +		compatible = "syscon";
> >>> +
> >>> +		mux: mux-controller@3 {
> >>> +			compatible = "mmio-mux";
> >>> +			reg = <0x3>;
> >>> +			bit-mask = <0x1>;
> >>> +			bit-shift = <5>;
> >>
> >> This pattern doesn't scale once you have multiple fields @ addr 3. I 
> >> also don't really think a node per register field in DT really scales.
> > 
> > Thanks, I have been a bit uneasy with the separate per-bitfield mux
> > controller node, so I'm eager to agree. But thit makes me unsure how to
> > best represent the information that is spelled out above.
> > 
> >> I think the parent should be declared as a mux controller instead.
> > 
> > The syscon node itself should be the mux controller? Would you expect
> > the mmio-mux driver bind to the syscon node, or should the mux framework
> > handle creation of the mux controls in this case (i.e. does the syscon
> > node get a "mmio-mux" added to its compatible list)?
> > 
> >> You could encode the mux addr and bit position in the mux cells.
> > 
> > What about the bit-mask / bitfield width? Just add a cell for it?
> > 
> > 	gpr: syscon {
> > 		compatible = "mmio-mux", "syscon", "simple-mfd";
> > 		#mux-control-cells = <3>;
> > 
> > 		video-mux {
> > 			compatible = "video-mux";
> > 			/* register 0x3, bits [6:5] */
> > 			mux-controls = <&gpr 0x3 5 0x3>;
> > 
> > 			ports {
> > 				/* ports 0..5 */
> > 			};
> > 		};
> > 	};
> > 
> > Or maybe using MSB and LSB would be better to read?
> > 
> > 		video-mux {
> > 			/* register 0x3, bits [6:5] */
> > 			mux-control = <&gpr 0x3 6 5>;
> > 
> > 			ports {
> > 				/* ports 0..5 */
> > 			};
> > 		};
> 
> Why do you need three values for one register+field? The shift can be
> implied from the mask, if the mask is pre-shifted. I.e. specifying a
> mask of 0x60 in this case. What I'm I missing?

As long as we have <= 32-bit hardware registers, that would work.
The question then is if things like
	mux-control = <&gpr 0x04 0x00300000>;
are considered readable/reviewable enough. And what happens when we get
64-bit general purpose registers containing muxes? Also a binding like
this would allow non-contiguous bit masks.
The reason I suggested using bit-shift in the first place was that there
are already other bindings using "bit-shift" or "reg-shift" properties.

regards
Philipp

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Rob Herring]

On Thu, Apr 20, 2017 at 10:14:08AM +0200, Philipp Zabel wrote:
> Hi Rob,
> 
> On Wed, 2017-04-19 at 17:09 -0500, Rob Herring wrote:
> > On Thu, Apr 13, 2017 at 05:48:11PM +0200, Philipp Zabel wrote:
> > > This adds device tree binding documentation for mmio-based syscon
> > > multiplexers controlled by a single bitfield in a syscon register
> > > range.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > >  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > >  1 file changed, 56 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > new file mode 100644
> > > index 0000000000000..11d96f5d98583
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > @@ -0,0 +1,56 @@
> > > +MMIO bitfield-based multiplexer controller bindings
> > > +
> > > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > > +device tree node must be a syscon node to provide register access.
> > > +
> > > +Required properties:
> > > +- compatible : "gpio-mux"
> > 
> > ?
> >
> > > +- reg : register base of the register containing the control bitfield
> > > +- bit-mask : bitmask of the control bitfield in the control register
> > > +- bit-shift : bit offset of the control bitfield in the control register
> > > +- #mux-control-cells : <0>
> > > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > > +
> > > +Optional properties:
> > > +- idle-state : if present, the state the mux will have when idle. The
> > > +	       special state MUX_IDLE_AS_IS is the default.
> > > +
> > > +The multiplexer state is defined as the value of the bitfield described
> > > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > > +syscon.
> > > +
> > > +Example:
> > > +
> > > +	syscon {
> > > +		compatible = "syscon";
> > > +
> > > +		mux: mux-controller@3 {
> > > +			compatible = "mmio-mux";
> > > +			reg = <0x3>;
> > > +			bit-mask = <0x1>;
> > > +			bit-shift = <5>;
> > 
> > This pattern doesn't scale once you have multiple fields @ addr 3. I 
> > also don't really think a node per register field in DT really scales.
> 
> Thanks, I have been a bit uneasy with the separate per-bitfield mux
> controller node, so I'm eager to agree. But thit makes me unsure how to
> best represent the information that is spelled out above.
> 
> > I think the parent should be declared as a mux controller instead.
> 
> The syscon node itself should be the mux controller? Would you expect
> the mmio-mux driver bind to the syscon node, or should the mux framework
> handle creation of the mux controls in this case (i.e. does the syscon
> node get a "mmio-mux" added to its compatible list)?

Using compatibles here doesn't scale either. If we had 30 functions in 
a syscon, we'd have 30 different compatibles. The syscon node should 
have a specific compatible which implies it is a mux controller (as does 
#mux-controll-cells). So I would expect either you add compatibles to 
the mmio-mux driver or you have a specific driver for the syscon that 
in turn registers a mux device. Either way, it doesn't affect the binding.

> 
> > You could encode the mux addr and bit position in the mux cells.
> 
> What about the bit-mask / bitfield width? Just add a cell for it?

As Peter said, just a shifted mask should be enough.

> 	gpr: syscon {
> 		compatible = "mmio-mux", "syscon", "simple-mfd";
> 		#mux-control-cells = <3>;
> 
> 		video-mux {
> 			compatible = "video-mux";
> 			/* register 0x3, bits [6:5] */
> 			mux-controls = <&gpr 0x3 5 0x3>;
> 
> 			ports {
> 				/* ports 0..5 */
> 			};
> 		};
> 	};
> 
> Or maybe using MSB and LSB would be better to read?
> 
> 		video-mux {
> 			/* register 0x3, bits [6:5] */
> 			mux-control = <&gpr 0x3 6 5>;
> 
> 			ports {
> 				/* ports 0..5 */
> 			};
> 		};
> 
> > > +			#mux-control-cells = <0>;
> > > +		};
> > > +	};
> > > +
> > > +	video-mux {
> > > +		compatible = "video-mux";
> > > +		mux-controls = <&mux>;
> > 
> > The mux binding was largely defined for a single control controling 
> > multiple muxes. This doesn't really fit that, but I guess this is an 
> > improvement over a custom syscon phandle.
> 
> What I especially like about the mux-controls property is that would
> allow me to use the gpio-mux driver (or any other mux controller)
> instead of having to code variants of the video-mux for all possible
> control schemes.

Yes, that's true.

Rob

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Peter Rosin]

On 2017-04-20 00:09, Rob Herring wrote:
> On Thu, Apr 13, 2017 at 05:48:11PM +0200, Philipp Zabel wrote:
>> This adds device tree binding documentation for mmio-based syscon
>> multiplexers controlled by a single bitfield in a syscon register
>> range.
>>
>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> ---
>>  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
>> new file mode 100644
>> index 0000000000000..11d96f5d98583
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
>> @@ -0,0 +1,56 @@
>> +MMIO bitfield-based multiplexer controller bindings
>> +
>> +Define a syscon bitfield to be used to control a multiplexer. The parent
>> +device tree node must be a syscon node to provide register access.
>> +
>> +Required properties:
>> +- compatible : "gpio-mux"
> 
> ?
> 
>> +- reg : register base of the register containing the control bitfield
>> +- bit-mask : bitmask of the control bitfield in the control register
>> +- bit-shift : bit offset of the control bitfield in the control register
>> +- #mux-control-cells : <0>
>> +* Standard mux-controller bindings as decribed in mux-controller.txt
>> +
>> +Optional properties:
>> +- idle-state : if present, the state the mux will have when idle. The
>> +	       special state MUX_IDLE_AS_IS is the default.
>> +
>> +The multiplexer state is defined as the value of the bitfield described
>> +by the reg, bit-mask, and bit-shift properties, accessed through the parent
>> +syscon.
>> +
>> +Example:
>> +
>> +	syscon {
>> +		compatible = "syscon";
>> +
>> +		mux: mux-controller@3 {
>> +			compatible = "mmio-mux";
>> +			reg = <0x3>;
>> +			bit-mask = <0x1>;
>> +			bit-shift = <5>;
> 
> This pattern doesn't scale once you have multiple fields @ addr 3. I 
> also don't really think a node per register field in DT really scales.
> 
> I think the parent should be declared as a mux controller instead. You 
> could encode the mux addr and bit position in the mux cells.

But then you need to create mux controllers on demand. I have not
succeeded in doing that while also following the rules of the driver
model. I had severe problems with life-time issues when I tried.
I would like to see code before embarking on this path, and I'm
apparently not the one writing it...

So, either you meant that, or that the parent node should somehow
specify the possible mux controllers up front so that they can be
pre-created and ready when the consumers request them. But if you
do that, you can just refer to them by some enumeration from the
mux consumers instead of by some convoluted reg+field notation.

>> +			#mux-control-cells = <0>;
>> +		};
>> +	};
>> +
>> +	video-mux {
>> +		compatible = "video-mux";
>> +		mux-controls = <&mux>;
> 
> The mux binding was largely defined for a single control controling 
> multiple muxes. This doesn't really fit that, but I guess this is an 
> improvement over a custom syscon phandle.
> 
>> +
>> +		ports {
>> +			/* input 0 */
>> +			port@0 {
>> +				reg = <0>;
>> +			};
>> +
>> +			/* input 1 */
>> +			port@1 {
>> +				reg = <1>;
>> +			};
>> +
>> +			/* output */
>> +			port@2 {
>> +				reg = <2>;
>> +			};
>> +		};
>> +	};
>> -- 
>> 2.11.0
>>

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Peter Rosin]

On 2017-04-20 15:32, Peter Rosin wrote:
> On 2017-04-20 00:09, Rob Herring wrote:
>> On Thu, Apr 13, 2017 at 05:48:11PM +0200, Philipp Zabel wrote:
>>> This adds device tree binding documentation for mmio-based syscon
>>> multiplexers controlled by a single bitfield in a syscon register
>>> range.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>>  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
>>>  1 file changed, 56 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
>>> new file mode 100644
>>> index 0000000000000..11d96f5d98583
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
>>> @@ -0,0 +1,56 @@
>>> +MMIO bitfield-based multiplexer controller bindings
>>> +
>>> +Define a syscon bitfield to be used to control a multiplexer. The parent
>>> +device tree node must be a syscon node to provide register access.
>>> +
>>> +Required properties:
>>> +- compatible : "gpio-mux"
>>
>> ?
>>
>>> +- reg : register base of the register containing the control bitfield
>>> +- bit-mask : bitmask of the control bitfield in the control register
>>> +- bit-shift : bit offset of the control bitfield in the control register
>>> +- #mux-control-cells : <0>
>>> +* Standard mux-controller bindings as decribed in mux-controller.txt
>>> +
>>> +Optional properties:
>>> +- idle-state : if present, the state the mux will have when idle. The
>>> +	       special state MUX_IDLE_AS_IS is the default.
>>> +
>>> +The multiplexer state is defined as the value of the bitfield described
>>> +by the reg, bit-mask, and bit-shift properties, accessed through the parent
>>> +syscon.
>>> +
>>> +Example:
>>> +
>>> +	syscon {
>>> +		compatible = "syscon";
>>> +
>>> +		mux: mux-controller@3 {
>>> +			compatible = "mmio-mux";
>>> +			reg = <0x3>;
>>> +			bit-mask = <0x1>;
>>> +			bit-shift = <5>;
>>
>> This pattern doesn't scale once you have multiple fields @ addr 3. I 
>> also don't really think a node per register field in DT really scales.
>>
>> I think the parent should be declared as a mux controller instead. You 
>> could encode the mux addr and bit position in the mux cells.
> 
> But then you need to create mux controllers on demand. I have not
> succeeded in doing that while also following the rules of the driver
> model. I had severe problems with life-time issues when I tried.
> I would like to see code before embarking on this path, and I'm
> apparently not the one writing it...
> 
> So, either you meant that, or that the parent node should somehow
> specify the possible mux controllers up front so that they can be
> pre-created and ready when the consumers request them. But if you
> do that, you can just refer to them by some enumeration from the
> mux consumers instead of by some convoluted reg+field notation.

Ok, thinking some more about this. Sorry for spamming and replying to
self...

How about:

 	syscon {
 		compatible = "syscon", "simple-mfd";

		mux: mux-controllers {
			compatible = "mmio-mux";
	 		#mux-control-cells = <1>;

			/* three mux controllers, one at reg 3 bits 0:2,
			 * one at reg 3 bits 5:6 and one at reg 7 bit 3.
			 */
			mux-reg-masks = <0x3 0x07>, <0x3 0x60>, <0x7 0x08>;
			idle-state = <7>, <MUX_IDLE_AS_IS>, <0>;
		};

 
 		video-mux {
 			compatible = "video-mux";
 			mux-controls = <&mux 1>; /* i.e. reg 3 bits 5:6 */
 
 			ports {
 				/* ports 0..5 */
 			};
 		};
 	};

Optionally using some 64-bit safe 3-value encoding of the register fields
in the mux-reg-masks binding...

Cheers,
peda

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Philipp Zabel]

On Thu, 2017-04-20 at 16:13 +0200, Peter Rosin wrote:
> On 2017-04-20 15:32, Peter Rosin wrote:
> > On 2017-04-20 00:09, Rob Herring wrote:
> >> On Thu, Apr 13, 2017 at 05:48:11PM +0200, Philipp Zabel wrote:
> >>> This adds device tree binding documentation for mmio-based syscon
> >>> multiplexers controlled by a single bitfield in a syscon register
> >>> range.
> >>>
> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >>> ---
> >>>  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> >>>  1 file changed, 56 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> >>> new file mode 100644
> >>> index 0000000000000..11d96f5d98583
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> >>> @@ -0,0 +1,56 @@
> >>> +MMIO bitfield-based multiplexer controller bindings
> >>> +
> >>> +Define a syscon bitfield to be used to control a multiplexer. The parent
> >>> +device tree node must be a syscon node to provide register access.
> >>> +
> >>> +Required properties:
> >>> +- compatible : "gpio-mux"
> >>
> >> ?
> >>
> >>> +- reg : register base of the register containing the control bitfield
> >>> +- bit-mask : bitmask of the control bitfield in the control register
> >>> +- bit-shift : bit offset of the control bitfield in the control register
> >>> +- #mux-control-cells : <0>
> >>> +* Standard mux-controller bindings as decribed in mux-controller.txt
> >>> +
> >>> +Optional properties:
> >>> +- idle-state : if present, the state the mux will have when idle. The
> >>> +	       special state MUX_IDLE_AS_IS is the default.
> >>> +
> >>> +The multiplexer state is defined as the value of the bitfield described
> >>> +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> >>> +syscon.
> >>> +
> >>> +Example:
> >>> +
> >>> +	syscon {
> >>> +		compatible = "syscon";
> >>> +
> >>> +		mux: mux-controller@3 {
> >>> +			compatible = "mmio-mux";
> >>> +			reg = <0x3>;
> >>> +			bit-mask = <0x1>;
> >>> +			bit-shift = <5>;
> >>
> >> This pattern doesn't scale once you have multiple fields @ addr 3. I 
> >> also don't really think a node per register field in DT really scales.
> >>
> >> I think the parent should be declared as a mux controller instead. You 
> >> could encode the mux addr and bit position in the mux cells.
> > 
> > But then you need to create mux controllers on demand. I have not
> > succeeded in doing that while also following the rules of the driver
> > model. I had severe problems with life-time issues when I tried.
> > I would like to see code before embarking on this path, and I'm
> > apparently not the one writing it...
> > 
> > So, either you meant that, or that the parent node should somehow
> > specify the possible mux controllers up front so that they can be
> > pre-created and ready when the consumers request them. But if you
> > do that, you can just refer to them by some enumeration from the
> > mux consumers instead of by some convoluted reg+field notation.
> 
> Ok, thinking some more about this. Sorry for spamming and replying to
> self...
> 
> How about:
> 
>  	syscon {
>  		compatible = "syscon", "simple-mfd";
> 
> 		mux: mux-controllers {
> 			compatible = "mmio-mux";
> 	 		#mux-control-cells = <1>;
> 
> 			/* three mux controllers, one at reg 3 bits 0:2,
> 			 * one at reg 3 bits 5:6 and one at reg 7 bit 3.
> 			 */
> 			mux-reg-masks = <0x3 0x07>, <0x3 0x60>, <0x7 0x08>;
> 			idle-state = <7>, <MUX_IDLE_AS_IS>, <0>;
> 		};
> 
>  
>  		video-mux {
>  			compatible = "video-mux";
>  			mux-controls = <&mux 1>; /* i.e. reg 3 bits 5:6 */
>  
>  			ports {
>  				/* ports 0..5 */
>  			};
>  		};
>  	};
> 
> Optionally using some 64-bit safe 3-value encoding of the register fields
> in the mux-reg-masks binding...

I would prefer this to putting the registers and bit masks into the
phandle cells. The i.MX6Q/D GPR muxes could look like this:

	gpr: iomuxc-gpr@020e0000 {
		compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
		reg = <0x020e0000 0x38>;

		mux: mux-controllers {
			compatible = "mmio-mux";
			#mux-control-cells = <1>;

			/* This list is not complete */
			mux-reg-masks = <0x04 0x00080000>, /* MIPI_IPU1_MUX */
					<0x04 0x00100000>, /* MIPI_IPU2_MUX */
					<0x0c 0x0000000c>, /* HDMI_MUX_CTL */
					<0x0c 0x000000c0>, /* LVDS0_MUX_CTL */
					<0x0c 0x0000030c>, /* LVDS1_MUX_CTL */
					<0x28 0x00000003>, /* DCIC1_MUX_CTL */
					<0x28 0x0000000c>; /* DCIC2_MUX_CTL */
		};

		ipu1_csi0_mux {
			compatible = "video-mux";
			mux-controls = <&mux 0>;
			/* ... */
		};

		ipu2_csi1_mux {
			compatible = "video-mux";
			mux-controls = <&mux 1>;
			/* ... */
		};
	};

and for i.MX6DL/S:

	gpr: iomuxc-gpr@20e0000 {
		compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
		reg = <0x020e0000 0x38>;

		mux: mux-controllers {
			compatible = "mmio-mux";
			#mux-control-cells = <1>;

			mux-reg-masks = <0x34 0x00000007>, /* IPU_CSI0_MUX */
					<0x34 0x00000038>, /* IPU_CSI1_MUX */
					<0x0c 0x0000000c>, /* HDMI_MUX_CTL */
					<0x0c 0x000000c0>, /* LVDS0_MUX_CTL */
					<0x0c 0x0000030c>, /* LVDS1_MUX_CTL */
					<0x28 0x00000003>, /* DCIC1_MUX_CTL */
					<0x28 0x0000000c>; /* DCIC2_MUX_CTL */
		};

		ipu1_csi0_mux {
			compatible = "video-mux";
			mux-controls = <&mux 0>;
			/* ... */
		};

		ipu1_csi1_mux {
			compatible = "video-mux";
			mux-controls = <&mux 1>;
			/* ... */
		};
	};

regards
Philipp

Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Peter Rosin]

On 2017-04-20 16:50, Philipp Zabel wrote:
> 		mux: mux-controllers {
> 			compatible = "mmio-mux";
> 			#mux-control-cells = <1>;
> 
> 			/* This list is not complete */
> 			mux-reg-masks = <0x04 0x00080000>, /* MIPI_IPU1_MUX */
> 					<0x04 0x00100000>, /* MIPI_IPU2_MUX */
> 					<0x0c 0x0000000c>, /* HDMI_MUX_CTL */
> 					<0x0c 0x000000c0>, /* LVDS0_MUX_CTL */
> 					<0x0c 0x0000030c>, /* LVDS1_MUX_CTL */

I hope you mean
 					<0x0c 0x00000300>, /* LVDS1_MUX_CTL */

:-)

> 					<0x28 0x00000003>, /* DCIC1_MUX_CTL */
> 					<0x28 0x0000000c>; /* DCIC2_MUX_CTL */
> 		};

(BTW, same bug in the other example)

Cheers,
peda