[PATCH 0/2] MUX: Add support for mux-states

[PATCH 0/2] MUX: Add support for mux-states

[Aswath Govindraju]

The following series of patches add support for reading mux
state from the device tree.

Aswath Govindraju (2):
  dt-bindings: mux: Document mux-states property
  mux: Add support for reading mux state from consumer DT node

 .../devicetree/bindings/mux/gpio-mux.yaml     |  11 +-
 .../devicetree/bindings/mux/mux-consumer.yaml |  14 ++
 .../bindings/mux/mux-controller.yaml          |  26 ++-
 drivers/mux/core.c                            | 213 ++++++++++++++++--
 include/linux/mux/consumer.h                  |  19 +-
 5 files changed, 260 insertions(+), 23 deletions(-)

-- 
2.17.1

[PATCH 1/2] dt-bindings: mux: Document mux-states property

[Aswath Govindraju]

In some cases, it is required to provide the state to which the mux
controller has be set to, from the consumer device tree node. Document the
property mux-states that can be used for adding this support.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
 .../devicetree/bindings/mux/gpio-mux.yaml     | 11 ++++++--
 .../devicetree/bindings/mux/mux-consumer.yaml | 14 ++++++++++
 .../bindings/mux/mux-controller.yaml          | 26 ++++++++++++++++++-
 3 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.yaml b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
index 0a7c8d64981a..ee4de9fbaf4d 100644
--- a/Documentation/devicetree/bindings/mux/gpio-mux.yaml
+++ b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
@@ -26,7 +26,10 @@ properties:
       List of gpios used to control the multiplexer, least significant bit first.
 
   '#mux-control-cells':
-    const: 0
+    enum: [ 0, 1 ]
+
+  '#mux-state-cells':
+    enum: [ 1, 2 ]
 
   idle-state:
     default: -1
@@ -34,7 +37,11 @@ properties:
 required:
   - compatible
   - mux-gpios
-  - "#mux-control-cells"
+anyOf:
+  - required:
+      - "#mux-control-cells"
+  - required:
+      - "#mux-state-cells"
 
 additionalProperties: false
 
diff --git a/Documentation/devicetree/bindings/mux/mux-consumer.yaml b/Documentation/devicetree/bindings/mux/mux-consumer.yaml
index 7af93298ab5c..64f353714227 100644
--- a/Documentation/devicetree/bindings/mux/mux-consumer.yaml
+++ b/Documentation/devicetree/bindings/mux/mux-consumer.yaml
@@ -25,6 +25,11 @@ description: |
   strings to label each of the mux controllers listed in the "mux-controls"
   property.
 
+  If it is required to provide the state that the mux controller needs to
+  be set to, the property "mux-states" must be used. An optional property
+  "mux-state-names" can be used to provide a list of strings, to label
+  each of the mux controllers listed in the "mux-states" property.
+
   mux-ctrl-specifier typically encodes the chip-relative mux controller number.
   If the mux controller chip only provides a single mux controller, the
   mux-ctrl-specifier can typically be left out.
@@ -35,12 +40,21 @@ properties:
   mux-controls:
     $ref: /schemas/types.yaml#/definitions/phandle-array
 
+  mux-states:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+
   mux-control-names:
     description:
       Devices that use more than a single mux controller can use the
       "mux-control-names" property to map the name of the requested mux
       controller to an index into the list given by the "mux-controls" property.
 
+  mux-state-names:
+    description:
+      Devices that use more than a single mux controller can use the
+      "mux-state-names" property to map the name of the requested mux
+      controller to an index into the list given by the "mux-states" property.
+
 additionalProperties: true
 
 ...
diff --git a/Documentation/devicetree/bindings/mux/mux-controller.yaml b/Documentation/devicetree/bindings/mux/mux-controller.yaml
index 736a84c3b6a5..b29dbf521f01 100644
--- a/Documentation/devicetree/bindings/mux/mux-controller.yaml
+++ b/Documentation/devicetree/bindings/mux/mux-controller.yaml
@@ -25,7 +25,9 @@ description: |
   --------------------
 
   Mux controller nodes must specify the number of cells used for the
-  specifier using the '#mux-control-cells' property.
+  specifier using the '#mux-control-cells' or 'mux-state-cells'
+  property. Value of '#mux-state-cells' will always be one greater then
+  the value of '#mux-control-cells'.
 
   Optionally, mux controller nodes can also specify the state the mux should
   have when it is idle. The idle-state property is used for this. If the
@@ -67,6 +69,8 @@ select:
           pattern: '^mux-controller'
     - required:
         - '#mux-control-cells'
+    - required:
+        - '#mux-state-cells'
 
 properties:
   $nodename:
@@ -75,6 +79,9 @@ properties:
   '#mux-control-cells':
     enum: [ 0, 1 ]
 
+  '#mux-state-cells':
+    enum: [ 1, 2 ]
+
   idle-state:
     $ref: /schemas/types.yaml#/definitions/int32
     minimum: -2
@@ -179,4 +186,21 @@ examples:
             };
         };
     };
+
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    mux1: mux-controller {
+        compatible = "gpio-mux";
+        #mux-state-cells = <1>;
+        mux-gpios = <&exp_som 2 GPIO_ACTIVE_HIGH>;
+    };
+
+    transceiver4: can-phy4 {
+        compatible = "ti,tcan1042";
+        #phy-cells = <0>;
+        max-bitrate = <5000000>;
+        standby-gpios = <&exp_som 7 GPIO_ACTIVE_HIGH>;
+        mux-states = <&mux1 1>;
+    };
 ...
-- 
2.17.1

[PATCH 2/2] mux: Add support for reading mux state from consumer DT node

[Aswath Govindraju]

In some cases, we might need to provide the state of the mux to be set for
the operation of a given peripheral. Therefore, pass this information using
mux-states property.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
 drivers/mux/core.c           | 213 +++++++++++++++++++++++++++++++----
 include/linux/mux/consumer.h |  19 +++-
 2 files changed, 212 insertions(+), 20 deletions(-)

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 22f4709768d1..38869c3680a8 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -29,6 +29,19 @@
  */
 #define MUX_CACHE_UNKNOWN MUX_IDLE_AS_IS
 
+/**
+ * struct mux_state -	Represents a mux controller specific to a given device
+ * @mux:		Pointer to a mux controller
+ * @state		State of the mux to be set
+ *
+ * This structure is specific to a device that acquires it and has information
+ * specific to the device.
+ */
+struct mux_state {
+	struct mux_control *mux;
+	unsigned int state;
+};
+
 static struct class mux_class = {
 	.name = "mux",
 	.owner = THIS_MODULE,
@@ -370,6 +383,30 @@ int mux_control_select_delay(struct mux_control *mux, unsigned int state,
 }
 EXPORT_SYMBOL_GPL(mux_control_select_delay);
 
+/**
+ * mux_state_select_delay() - Select mux-state
+ * @mux: The mux-state to select
+ * @delay_us: The time to delay (in microseconds) if the mux control
+ *            changes state on select
+ *
+ * On successfully selecting the mux-state, it will be locked until
+ * there is a call to mux_state_deselect(). If the mux-control is already
+ * selected when mux_state_select() is called, the caller will be blocked
+ * until mux_state_deselect() is called (by someone else).
+ *
+ * Therefore, make sure to call mux_state_deselect() when the operation is
+ * complete and the mux-control is free for others to use, but do not call
+ * mux_state_deselect() if mux_state_select() fails.
+ *
+ * Return: 0 when the mux-state has been selected or a negative
+ * errno on error.
+ */
+int mux_state_select_delay(struct mux_state *mstate, unsigned int delay_us)
+{
+	return mux_control_select_delay(mstate->mux, mstate->state, delay_us);
+}
+EXPORT_SYMBOL_GPL(mux_state_select_delay);
+
 /**
  * mux_control_try_select_delay() - Try to select the given multiplexer state.
  * @mux: The mux-control to request a change of state from.
@@ -405,6 +442,27 @@ int mux_control_try_select_delay(struct mux_control *mux, unsigned int state,
 }
 EXPORT_SYMBOL_GPL(mux_control_try_select_delay);
 
+/**
+ * mux_state_try_select_delay() - Try to select the mux-state.
+ * @mux: The mux-state to select
+ * @delay_us: The time to delay (in microseconds) if the mux state is changed.
+ *
+ * On successfully selecting the mux-state, it will be locked until
+ * mux_state_deselect() called.
+ *
+ * Therefore, make sure to call mux_state_deselect() when the operation is
+ * complete and the mux-control is free for others to use, but do not call
+ * mux_state_deselect() if mux_state_try_select() fails.
+ *
+ * Return: 0 when the mux-control state has the requested state or a negative
+ * errno on error. Specifically -EBUSY if the mux-control is contended.
+ */
+int mux_state_try_select_delay(struct mux_state *mstate, unsigned int delay_us)
+{
+	return mux_control_try_select_delay(mstate->mux, mstate->state, delay_us);
+}
+EXPORT_SYMBOL_GPL(mux_state_try_select_delay);
+
 /**
  * mux_control_deselect() - Deselect the previously selected multiplexer state.
  * @mux: The mux-control to deselect.
@@ -431,6 +489,24 @@ int mux_control_deselect(struct mux_control *mux)
 }
 EXPORT_SYMBOL_GPL(mux_control_deselect);
 
+/**
+ * mux_state_deselect() - Deselect the previously selected multiplexer state.
+ * @mux: The mux-state to deselect.
+ *
+ * It is required that a single call is made to mux_state_deselect() for
+ * each and every successful call made to either of mux_state_select() or
+ * mux_state_try_select().
+ *
+ * Return: 0 on success and a negative errno on error. An error can only
+ * occur if the mux has an idle state. Note that even if an error occurs, the
+ * mux-control is unlocked and is thus free for the next access.
+ */
+int mux_state_deselect(struct mux_state *mstate)
+{
+	return mux_control_deselect(mstate->mux);
+}
+EXPORT_SYMBOL_GPL(mux_state_deselect);
+
 /* Note this function returns a reference to the mux_chip dev. */
 static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
 {
@@ -442,13 +518,15 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
 }
 
 /**
- * mux_control_get() - Get the mux-control for a device.
+ * mux_get() - Get the mux-control for a device.
  * @dev: The device that needs a mux-control.
  * @mux_name: The name identifying the mux-control.
+ * @state: The variable to store the enable state for the requested device
  *
  * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
  */
-struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+static struct mux_control *mux_get(struct device *dev, const char *mux_name,
+				   unsigned int *state)
 {
 	struct device_node *np = dev->of_node;
 	struct of_phandle_args args;
@@ -458,8 +536,12 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
 	int ret;
 
 	if (mux_name) {
-		index = of_property_match_string(np, "mux-control-names",
-						 mux_name);
+		if (state)
+			index = of_property_match_string(np, "mux-state-names",
+							 mux_name);
+		else
+			index = of_property_match_string(np, "mux-control-names",
+							 mux_name);
 		if (index < 0) {
 			dev_err(dev, "mux controller '%s' not found\n",
 				mux_name);
@@ -467,12 +549,17 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
 		}
 	}
 
-	ret = of_parse_phandle_with_args(np,
-					 "mux-controls", "#mux-control-cells",
-					 index, &args);
+	if (state)
+		ret = of_parse_phandle_with_args(np,
+						 "mux-states", "#mux-state-cells",
+						 index, &args);
+	else
+		ret = of_parse_phandle_with_args(np,
+						 "mux-controls", "#mux-control-cells",
+						 index, &args);
 	if (ret) {
-		dev_err(dev, "%pOF: failed to get mux-control %s(%i)\n",
-			np, mux_name ?: "", index);
+		dev_err(dev, "%pOF: failed to get mux-%s %s(%i)\n",
+			np, state ? "state" : "control", mux_name ?: "", index);
 		return ERR_PTR(ret);
 	}
 
@@ -481,17 +568,35 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
 	if (!mux_chip)
 		return ERR_PTR(-EPROBE_DEFER);
 
-	if (args.args_count > 1 ||
-	    (!args.args_count && (mux_chip->controllers > 1))) {
-		dev_err(dev, "%pOF: wrong #mux-control-cells for %pOF\n",
-			np, args.np);
-		put_device(&mux_chip->dev);
-		return ERR_PTR(-EINVAL);
-	}
-
 	controller = 0;
-	if (args.args_count)
-		controller = args.args[0];
+	if (state) {
+		if (args.args_count > 2 || args.args_count == 0 ||
+		    (args.args_count < 2 && mux_chip->controllers > 1)) {
+			dev_err(dev, "%pOF: wrong #mux-state-cells for %pOF\n",
+				np, args.np);
+			put_device(&mux_chip->dev);
+			return ERR_PTR(-EINVAL);
+		}
+
+		if (args.args_count == 2) {
+			controller = args.args[0];
+			*state = args.args[1];
+		} else {
+			*state = args.args[0];
+		}
+
+	} else {
+		if (args.args_count > 1 ||
+		    (!args.args_count && mux_chip->controllers > 1)) {
+			dev_err(dev, "%pOF: wrong #mux-control-cells for %pOF\n",
+				np, args.np);
+			put_device(&mux_chip->dev);
+			return ERR_PTR(-EINVAL);
+		}
+
+		if (args.args_count)
+			controller = args.args[0];
+	}
 
 	if (controller >= mux_chip->controllers) {
 		dev_err(dev, "%pOF: bad mux controller %u specified in %pOF\n",
@@ -502,6 +607,18 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
 
 	return &mux_chip->mux[controller];
 }
+
+/**
+ * mux_control_get() - Get the mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+{
+	return mux_get(dev, mux_name, NULL);
+}
 EXPORT_SYMBOL_GPL(mux_control_get);
 
 /**
@@ -523,6 +640,26 @@ static void devm_mux_control_release(struct device *dev, void *res)
 	mux_control_put(mux);
 }
 
+/**
+ * mux_state_put() - Put away the mux-state for good.
+ * @mux: The mux-state to put away.
+ *
+ * mux_control_put() reverses the effects of mux_control_get().
+ */
+void mux_state_put(struct mux_state *mstate)
+{
+	mux_control_put(mstate->mux);
+	kfree(mstate);
+}
+EXPORT_SYMBOL_GPL(mux_state_put);
+
+static void devm_mux_state_release(struct device *dev, void *res)
+{
+	struct mux_state *mstate = *(struct mux_state **)res;
+
+	mux_state_put(mstate);
+}
+
 /**
  * devm_mux_control_get() - Get the mux-control for a device, with resource
  *			    management.
@@ -553,6 +690,44 @@ struct mux_control *devm_mux_control_get(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_mux_control_get);
 
+/**
+ * devm_mux_state_get() - Get the mux-state for a device, with resource
+ *			  management.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: Pointer to the mux-state, or an ERR_PTR with a negative errno.
+ */
+struct mux_state *devm_mux_state_get(struct device *dev,
+				     const char *mux_name)
+{
+	struct mux_state **ptr, *mstate;
+	struct mux_control *mux_ctrl;
+	int state;
+
+	mstate = devm_kzalloc(dev, sizeof(struct mux_state), GFP_KERNEL);
+	if (!mstate)
+		return ERR_PTR(-ENOMEM);
+
+	ptr = devres_alloc(devm_mux_state_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	mux_ctrl = mux_get(dev, mux_name, &state);
+	if (IS_ERR(mux_ctrl)) {
+		devres_free(ptr);
+		return (struct mux_state *)mux_ctrl;
+	}
+
+	mstate->mux = mux_ctrl;
+	mstate->state = state;
+	*ptr = mstate;
+	devres_add(dev, ptr);
+
+	return mstate;
+}
+EXPORT_SYMBOL_GPL(devm_mux_state_get);
+
 /*
  * Using subsys_initcall instead of module_init here to try to ensure - for
  * the non-modular case - that the subsystem is initialized when mux consumers
diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
index 7a09b040ac39..bf5abf062c21 100644
--- a/include/linux/mux/consumer.h
+++ b/include/linux/mux/consumer.h
@@ -14,33 +14,50 @@
 
 struct device;
 struct mux_control;
+struct mux_state;
 
 unsigned int mux_control_states(struct mux_control *mux);
 int __must_check mux_control_select_delay(struct mux_control *mux,
 					  unsigned int state,
 					  unsigned int delay_us);
+int __must_check mux_state_select_delay(struct mux_state *mstate,
+					unsigned int delay_us);
 int __must_check mux_control_try_select_delay(struct mux_control *mux,
 					      unsigned int state,
 					      unsigned int delay_us);
-
+int __must_check mux_state_try_select_delay(struct mux_state *mstate,
+					    unsigned int delay_us);
 static inline int __must_check mux_control_select(struct mux_control *mux,
 						  unsigned int state)
 {
 	return mux_control_select_delay(mux, state, 0);
 }
 
+static inline int __must_check mux_state_select(struct mux_state *mstate)
+{
+	return mux_state_select_delay(mstate, 0);
+}
 static inline int __must_check mux_control_try_select(struct mux_control *mux,
 						      unsigned int state)
 {
 	return mux_control_try_select_delay(mux, state, 0);
 }
 
+static inline int __must_check mux_state_try_select(struct mux_state *mstate)
+{
+	return mux_state_try_select_delay(mstate, 0);
+}
+
 int mux_control_deselect(struct mux_control *mux);
+int mux_state_deselect(struct mux_state *mstate);
 
 struct mux_control *mux_control_get(struct device *dev, const char *mux_name);
 void mux_control_put(struct mux_control *mux);
+void mux_state_put(struct mux_state *mstate);
 
 struct mux_control *devm_mux_control_get(struct device *dev,
 					 const char *mux_name);
+struct mux_state *devm_mux_state_get(struct device *dev,
+				     const char *mux_name);
 
 #endif /* _LINUX_MUX_CONSUMER_H */
-- 
2.17.1

Re: [PATCH 1/2] dt-bindings: mux: Document mux-states property

[Peter Rosin]

Hi Aswath!

Some language nits. But other than that it looks good to me!

On 2021-11-30 13:18, Aswath Govindraju wrote:
> In some cases, it is required to provide the state to which the mux
> controller has be set to, from the consumer device tree node. Document the
s/has be/has to be/

> property mux-states that can be used for adding this support.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  .../devicetree/bindings/mux/gpio-mux.yaml     | 11 ++++++--
>  .../devicetree/bindings/mux/mux-consumer.yaml | 14 ++++++++++
>  .../bindings/mux/mux-controller.yaml          | 26 ++++++++++++++++++-
>  3 files changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.yaml b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
> index 0a7c8d64981a..ee4de9fbaf4d 100644
> --- a/Documentation/devicetree/bindings/mux/gpio-mux.yaml
> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
> @@ -26,7 +26,10 @@ properties:
>        List of gpios used to control the multiplexer, least significant bit first.
>  
>    '#mux-control-cells':
> -    const: 0
> +    enum: [ 0, 1 ]
> +
> +  '#mux-state-cells':
> +    enum: [ 1, 2 ]
>  
>    idle-state:
>      default: -1
> @@ -34,7 +37,11 @@ properties:
>  required:
>    - compatible
>    - mux-gpios
> -  - "#mux-control-cells"
> +anyOf:
> +  - required:
> +      - "#mux-control-cells"
> +  - required:
> +      - "#mux-state-cells"
>  
>  additionalProperties: false
>  
> diff --git a/Documentation/devicetree/bindings/mux/mux-consumer.yaml b/Documentation/devicetree/bindings/mux/mux-consumer.yaml
> index 7af93298ab5c..64f353714227 100644
> --- a/Documentation/devicetree/bindings/mux/mux-consumer.yaml
> +++ b/Documentation/devicetree/bindings/mux/mux-consumer.yaml
> @@ -25,6 +25,11 @@ description: |
>    strings to label each of the mux controllers listed in the "mux-controls"
>    property.
>  
> +  If it is required to provide the state that the mux controller needs to
> +  be set to, the property "mux-states" must be used. An optional property
> +  "mux-state-names" can be used to provide a list of strings, to label
> +  each of the mux controllers listed in the "mux-states" property.

s/mux controllers/multiplexer states/

> +
>    mux-ctrl-specifier typically encodes the chip-relative mux controller number.
>    If the mux controller chip only provides a single mux controller, the
>    mux-ctrl-specifier can typically be left out.
> @@ -35,12 +40,21 @@ properties:
>    mux-controls:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>  
> +  mux-states:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +
>    mux-control-names:
>      description:
>        Devices that use more than a single mux controller can use the
>        "mux-control-names" property to map the name of the requested mux
>        controller to an index into the list given by the "mux-controls" property.
>  
> +  mux-state-names:
> +    description:
> +      Devices that use more than a single mux controller can use the

s/mux controller/multiplexer state/

> +      "mux-state-names" property to map the name of the requested mux
> +      controller to an index into the list given by the "mux-states" property.

And again, over the line break.

> +
>  additionalProperties: true
>  
>  ...
> diff --git a/Documentation/devicetree/bindings/mux/mux-controller.yaml b/Documentation/devicetree/bindings/mux/mux-controller.yaml
> index 736a84c3b6a5..b29dbf521f01 100644
> --- a/Documentation/devicetree/bindings/mux/mux-controller.yaml
> +++ b/Documentation/devicetree/bindings/mux/mux-controller.yaml
> @@ -25,7 +25,9 @@ description: |
>    --------------------
>  
>    Mux controller nodes must specify the number of cells used for the
> -  specifier using the '#mux-control-cells' property.
> +  specifier using the '#mux-control-cells' or 'mux-state-cells'

'#mux-state-cells' (missing #)

> +  property. Value of '#mux-state-cells' will always be one greater then

s/Value/The value/
s/then/than/

Cheers,
Peter

> +  the value of '#mux-control-cells'.
>  
>    Optionally, mux controller nodes can also specify the state the mux should
>    have when it is idle. The idle-state property is used for this. If the
> @@ -67,6 +69,8 @@ select:
>            pattern: '^mux-controller'
>      - required:
>          - '#mux-control-cells'
> +    - required:
> +        - '#mux-state-cells'
>  
>  properties:
>    $nodename:
> @@ -75,6 +79,9 @@ properties:
>    '#mux-control-cells':
>      enum: [ 0, 1 ]
>  
> +  '#mux-state-cells':
> +    enum: [ 1, 2 ]
> +
>    idle-state:
>      $ref: /schemas/types.yaml#/definitions/int32
>      minimum: -2
> @@ -179,4 +186,21 @@ examples:
>              };
>          };
>      };
> +
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    mux1: mux-controller {
> +        compatible = "gpio-mux";
> +        #mux-state-cells = <1>;
> +        mux-gpios = <&exp_som 2 GPIO_ACTIVE_HIGH>;
> +    };
> +
> +    transceiver4: can-phy4 {
> +        compatible = "ti,tcan1042";
> +        #phy-cells = <0>;
> +        max-bitrate = <5000000>;
> +        standby-gpios = <&exp_som 7 GPIO_ACTIVE_HIGH>;
> +        mux-states = <&mux1 1>;
> +    };
>  ...
> 

Re: [PATCH 2/2] mux: Add support for reading mux state from consumer DT node

[Peter Rosin]

Hi!

Looks good, I think. I have some nits, but nothing major...

On 2021-11-30 13:18, Aswath Govindraju wrote:
> In some cases, we might need to provide the state of the mux to be set for
> the operation of a given peripheral. Therefore, pass this information using
> mux-states property.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  drivers/mux/core.c           | 213 +++++++++++++++++++++++++++++++----
>  include/linux/mux/consumer.h |  19 +++-
>  2 files changed, 212 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
> index 22f4709768d1..38869c3680a8 100644
> --- a/drivers/mux/core.c
> +++ b/drivers/mux/core.c
> @@ -29,6 +29,19 @@
>   */
>  #define MUX_CACHE_UNKNOWN MUX_IDLE_AS_IS
>  
> +/**
> + * struct mux_state -	Represents a mux controller specific to a given device
> + * @mux:		Pointer to a mux controller
> + * @state		State of the mux to be set
> + *
> + * This structure is specific to a device that acquires it and has information
> + * specific to the device.
> + */
> +struct mux_state {
> +	struct mux_control *mux;
> +	unsigned int state;
> +};
> +
>  static struct class mux_class = {
>  	.name = "mux",
>  	.owner = THIS_MODULE,
> @@ -370,6 +383,30 @@ int mux_control_select_delay(struct mux_control *mux, unsigned int state,
>  }
>  EXPORT_SYMBOL_GPL(mux_control_select_delay);
>  
> +/**
> + * mux_state_select_delay() - Select mux-state
> + * @mux: The mux-state to select

@mstate

> + * @delay_us: The time to delay (in microseconds) if the mux control
> + *            changes state on select
> + *
> + * On successfully selecting the mux-state, it will be locked until
> + * there is a call to mux_state_deselect(). If the mux-control is already
> + * selected when mux_state_select() is called, the caller will be blocked
> + * until mux_state_deselect() is called (by someone else).

If the other consumer has the whole mux control, that consumer will
deselect with mux_control_deselect() (i.e. not mux_state_deselect()).

> + *
> + * Therefore, make sure to call mux_state_deselect() when the operation is
> + * complete and the mux-control is free for others to use, but do not call
> + * mux_state_deselect() if mux_state_select() fails.
> + *
> + * Return: 0 when the mux-state has been selected or a negative
> + * errno on error.
> + */
> +int mux_state_select_delay(struct mux_state *mstate, unsigned int delay_us)
> +{
> +	return mux_control_select_delay(mstate->mux, mstate->state, delay_us);
> +}
> +EXPORT_SYMBOL_GPL(mux_state_select_delay);
> +
>  /**
>   * mux_control_try_select_delay() - Try to select the given multiplexer state.
>   * @mux: The mux-control to request a change of state from.
> @@ -405,6 +442,27 @@ int mux_control_try_select_delay(struct mux_control *mux, unsigned int state,
>  }
>  EXPORT_SYMBOL_GPL(mux_control_try_select_delay);
>  
> +/**
> + * mux_state_try_select_delay() - Try to select the mux-state.
> + * @mux: The mux-state to select

@mstate

> + * @delay_us: The time to delay (in microseconds) if the mux state is changed.
> + *
> + * On successfully selecting the mux-state, it will be locked until
> + * mux_state_deselect() called.
> + *
> + * Therefore, make sure to call mux_state_deselect() when the operation is
> + * complete and the mux-control is free for others to use, but do not call
> + * mux_state_deselect() if mux_state_try_select() fails.
> + *
> + * Return: 0 when the mux-control state has the requested state or a negative

 * Return: 0 when the mux-state has been selected or a negative

> + * errno on error. Specifically -EBUSY if the mux-control is contended.
> + */
> +int mux_state_try_select_delay(struct mux_state *mstate, unsigned int delay_us)
> +{
> +	return mux_control_try_select_delay(mstate->mux, mstate->state, delay_us);
> +}
> +EXPORT_SYMBOL_GPL(mux_state_try_select_delay);
> +
>  /**
>   * mux_control_deselect() - Deselect the previously selected multiplexer state.
>   * @mux: The mux-control to deselect.
> @@ -431,6 +489,24 @@ int mux_control_deselect(struct mux_control *mux)
>  }
>  EXPORT_SYMBOL_GPL(mux_control_deselect);
>  
> +/**
> + * mux_state_deselect() - Deselect the previously selected multiplexer state.
> + * @mux: The mux-state to deselect.

@mstate

> + *
> + * It is required that a single call is made to mux_state_deselect() for
> + * each and every successful call made to either of mux_state_select() or
> + * mux_state_try_select().
> + *
> + * Return: 0 on success and a negative errno on error. An error can only
> + * occur if the mux has an idle state. Note that even if an error occurs, the
> + * mux-control is unlocked and is thus free for the next access.
> + */
> +int mux_state_deselect(struct mux_state *mstate)
> +{
> +	return mux_control_deselect(mstate->mux);
> +}
> +EXPORT_SYMBOL_GPL(mux_state_deselect);
> +
>  /* Note this function returns a reference to the mux_chip dev. */
>  static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
>  {
> @@ -442,13 +518,15 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
>  }
>  
>  /**

I think this should be an ordinary comment now that this function is refactored
into a static helper, so just go with a plain old /* instead of /**

> - * mux_control_get() - Get the mux-control for a device.
> + * mux_get() - Get the mux-control for a device.
>   * @dev: The device that needs a mux-control.
>   * @mux_name: The name identifying the mux-control.
> + * @state: The variable to store the enable state for the requested device

 * @state: Pointer to where the requested state is returned, or NULL when the
           required multiplexer states are handled by other means.

>   *
>   * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
>   */
> -struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> +static struct mux_control *mux_get(struct device *dev, const char *mux_name,
> +				   unsigned int *state)
>  {
>  	struct device_node *np = dev->of_node;
>  	struct of_phandle_args args;
> @@ -458,8 +536,12 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>  	int ret;
>  
>  	if (mux_name) {
> -		index = of_property_match_string(np, "mux-control-names",
> -						 mux_name);
> +		if (state)
> +			index = of_property_match_string(np, "mux-state-names",
> +							 mux_name);
> +		else
> +			index = of_property_match_string(np, "mux-control-names",
> +							 mux_name);
>  		if (index < 0) {
>  			dev_err(dev, "mux controller '%s' not found\n",
>  				mux_name);
> @@ -467,12 +549,17 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>  		}
>  	}
>  
> -	ret = of_parse_phandle_with_args(np,
> -					 "mux-controls", "#mux-control-cells",
> -					 index, &args);
> +	if (state)
> +		ret = of_parse_phandle_with_args(np,
> +						 "mux-states", "#mux-state-cells",
> +						 index, &args);
> +	else
> +		ret = of_parse_phandle_with_args(np,
> +						 "mux-controls", "#mux-control-cells",
> +						 index, &args);
>  	if (ret) {
> -		dev_err(dev, "%pOF: failed to get mux-control %s(%i)\n",
> -			np, mux_name ?: "", index);
> +		dev_err(dev, "%pOF: failed to get mux-%s %s(%i)\n",
> +			np, state ? "state" : "control", mux_name ?: "", index);
>  		return ERR_PTR(ret);
>  	}
>  
> @@ -481,17 +568,35 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>  	if (!mux_chip)
>  		return ERR_PTR(-EPROBE_DEFER);
>  
> -	if (args.args_count > 1 ||
> -	    (!args.args_count && (mux_chip->controllers > 1))) {
> -		dev_err(dev, "%pOF: wrong #mux-control-cells for %pOF\n",
> -			np, args.np);
> -		put_device(&mux_chip->dev);
> -		return ERR_PTR(-EINVAL);
> -	}
> -
>  	controller = 0;
> -	if (args.args_count)
> -		controller = args.args[0];
> +	if (state) {
> +		if (args.args_count > 2 || args.args_count == 0 ||
> +		    (args.args_count < 2 && mux_chip->controllers > 1)) {
> +			dev_err(dev, "%pOF: wrong #mux-state-cells for %pOF\n",
> +				np, args.np);
> +			put_device(&mux_chip->dev);
> +			return ERR_PTR(-EINVAL);
> +		}
> +
> +		if (args.args_count == 2) {
> +			controller = args.args[0];
> +			*state = args.args[1];
> +		} else {
> +			*state = args.args[0];
> +		}
> +
> +	} else {
> +		if (args.args_count > 1 ||
> +		    (!args.args_count && mux_chip->controllers > 1)) {
> +			dev_err(dev, "%pOF: wrong #mux-control-cells for %pOF\n",
> +				np, args.np);
> +			put_device(&mux_chip->dev);
> +			return ERR_PTR(-EINVAL);
> +		}
> +
> +		if (args.args_count)
> +			controller = args.args[0];
> +	}
>  
>  	if (controller >= mux_chip->controllers) {
>  		dev_err(dev, "%pOF: bad mux controller %u specified in %pOF\n",
> @@ -502,6 +607,18 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>  
>  	return &mux_chip->mux[controller];
>  }
> +
> +/**
> + * mux_control_get() - Get the mux-control for a device.
> + * @dev: The device that needs a mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> +{
> +	return mux_get(dev, mux_name, NULL);
> +}
>  EXPORT_SYMBOL_GPL(mux_control_get);
>  
>  /**
> @@ -523,6 +640,26 @@ static void devm_mux_control_release(struct device *dev, void *res)
>  	mux_control_put(mux);
>  }
>  
> +/**
> + * mux_state_put() - Put away the mux-state for good.
> + * @mux: The mux-state to put away.

@mstate

Cheers,
Peter

> + *
> + * mux_control_put() reverses the effects of mux_control_get().
> + */
> +void mux_state_put(struct mux_state *mstate)
> +{
> +	mux_control_put(mstate->mux);
> +	kfree(mstate);
> +}
> +EXPORT_SYMBOL_GPL(mux_state_put);
> +
> +static void devm_mux_state_release(struct device *dev, void *res)
> +{
> +	struct mux_state *mstate = *(struct mux_state **)res;
> +
> +	mux_state_put(mstate);
> +}
> +
>  /**
>   * devm_mux_control_get() - Get the mux-control for a device, with resource
>   *			    management.
> @@ -553,6 +690,44 @@ struct mux_control *devm_mux_control_get(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_mux_control_get);
>  
> +/**
> + * devm_mux_state_get() - Get the mux-state for a device, with resource
> + *			  management.
> + * @dev: The device that needs a mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * Return: Pointer to the mux-state, or an ERR_PTR with a negative errno.
> + */
> +struct mux_state *devm_mux_state_get(struct device *dev,
> +				     const char *mux_name)
> +{
> +	struct mux_state **ptr, *mstate;
> +	struct mux_control *mux_ctrl;
> +	int state;
> +
> +	mstate = devm_kzalloc(dev, sizeof(struct mux_state), GFP_KERNEL);
> +	if (!mstate)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ptr = devres_alloc(devm_mux_state_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mux_ctrl = mux_get(dev, mux_name, &state);
> +	if (IS_ERR(mux_ctrl)) {
> +		devres_free(ptr);
> +		return (struct mux_state *)mux_ctrl;
> +	}
> +
> +	mstate->mux = mux_ctrl;
> +	mstate->state = state;
> +	*ptr = mstate;
> +	devres_add(dev, ptr);
> +
> +	return mstate;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_state_get);
> +
>  /*
>   * Using subsys_initcall instead of module_init here to try to ensure - for
>   * the non-modular case - that the subsystem is initialized when mux consumers
> diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
> index 7a09b040ac39..bf5abf062c21 100644
> --- a/include/linux/mux/consumer.h
> +++ b/include/linux/mux/consumer.h
> @@ -14,33 +14,50 @@
>  
>  struct device;
>  struct mux_control;
> +struct mux_state;
>  
>  unsigned int mux_control_states(struct mux_control *mux);
>  int __must_check mux_control_select_delay(struct mux_control *mux,
>  					  unsigned int state,
>  					  unsigned int delay_us);
> +int __must_check mux_state_select_delay(struct mux_state *mstate,
> +					unsigned int delay_us);
>  int __must_check mux_control_try_select_delay(struct mux_control *mux,
>  					      unsigned int state,
>  					      unsigned int delay_us);
> -
> +int __must_check mux_state_try_select_delay(struct mux_state *mstate,
> +					    unsigned int delay_us);
>  static inline int __must_check mux_control_select(struct mux_control *mux,
>  						  unsigned int state)
>  {
>  	return mux_control_select_delay(mux, state, 0);
>  }
>  
> +static inline int __must_check mux_state_select(struct mux_state *mstate)
> +{
> +	return mux_state_select_delay(mstate, 0);
> +}
>  static inline int __must_check mux_control_try_select(struct mux_control *mux,
>  						      unsigned int state)
>  {
>  	return mux_control_try_select_delay(mux, state, 0);
>  }
>  
> +static inline int __must_check mux_state_try_select(struct mux_state *mstate)
> +{
> +	return mux_state_try_select_delay(mstate, 0);
> +}
> +
>  int mux_control_deselect(struct mux_control *mux);
> +int mux_state_deselect(struct mux_state *mstate);
>  
>  struct mux_control *mux_control_get(struct device *dev, const char *mux_name);
>  void mux_control_put(struct mux_control *mux);
> +void mux_state_put(struct mux_state *mstate);
>  
>  struct mux_control *devm_mux_control_get(struct device *dev,
>  					 const char *mux_name);
> +struct mux_state *devm_mux_state_get(struct device *dev,
> +				     const char *mux_name);
>  
>  #endif /* _LINUX_MUX_CONSUMER_H */
> 

Re: [PATCH 1/2] dt-bindings: mux: Document mux-states property

[Rob Herring]

On Tue, 30 Nov 2021 17:48:46 +0530, Aswath Govindraju wrote:
> In some cases, it is required to provide the state to which the mux
> controller has be set to, from the consumer device tree node. Document the
> property mux-states that can be used for adding this support.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  .../devicetree/bindings/mux/gpio-mux.yaml     | 11 ++++++--
>  .../devicetree/bindings/mux/mux-consumer.yaml | 14 ++++++++++
>  .../bindings/mux/mux-controller.yaml          | 26 ++++++++++++++++++-
>  3 files changed, 48 insertions(+), 3 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mux/mux-controller.example.dt.yaml: can-phy4: 'mux-states' does not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1561624

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 1/2] dt-bindings: mux: Document mux-states property

[Rob Herring]

On Tue, Nov 30, 2021 at 05:48:46PM +0530, Aswath Govindraju wrote:
> In some cases, it is required to provide the state to which the mux
> controller has be set to, from the consumer device tree node. Document the
> property mux-states that can be used for adding this support.

I having a hard time understanding why you need this. One consumer 
configures a mux one way and another consumer another way? How do you 
arbitrate that? Please elaborate on what 'some cases' are and why it's 
required.

Can't you just add a cell for the 'state' allowing for 1-2 cells 
instead of 0-1?

> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  .../devicetree/bindings/mux/gpio-mux.yaml     | 11 ++++++--
>  .../devicetree/bindings/mux/mux-consumer.yaml | 14 ++++++++++
>  .../bindings/mux/mux-controller.yaml          | 26 ++++++++++++++++++-
>  3 files changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.yaml b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
> index 0a7c8d64981a..ee4de9fbaf4d 100644
> --- a/Documentation/devicetree/bindings/mux/gpio-mux.yaml
> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
> @@ -26,7 +26,10 @@ properties:
>        List of gpios used to control the multiplexer, least significant bit first.
>  
>    '#mux-control-cells':
> -    const: 0
> +    enum: [ 0, 1 ]
> +
> +  '#mux-state-cells':
> +    enum: [ 1, 2 ]
>  
>    idle-state:
>      default: -1
> @@ -34,7 +37,11 @@ properties:
>  required:
>    - compatible
>    - mux-gpios
> -  - "#mux-control-cells"
> +anyOf:
> +  - required:
> +      - "#mux-control-cells"
> +  - required:
> +      - "#mux-state-cells"
>  
>  additionalProperties: false
>  
> diff --git a/Documentation/devicetree/bindings/mux/mux-consumer.yaml b/Documentation/devicetree/bindings/mux/mux-consumer.yaml
> index 7af93298ab5c..64f353714227 100644
> --- a/Documentation/devicetree/bindings/mux/mux-consumer.yaml
> +++ b/Documentation/devicetree/bindings/mux/mux-consumer.yaml
> @@ -25,6 +25,11 @@ description: |
>    strings to label each of the mux controllers listed in the "mux-controls"
>    property.
>  
> +  If it is required to provide the state that the mux controller needs to
> +  be set to, the property "mux-states" must be used. An optional property
> +  "mux-state-names" can be used to provide a list of strings, to label
> +  each of the mux controllers listed in the "mux-states" property.
> +
>    mux-ctrl-specifier typically encodes the chip-relative mux controller number.
>    If the mux controller chip only provides a single mux controller, the
>    mux-ctrl-specifier can typically be left out.
> @@ -35,12 +40,21 @@ properties:
>    mux-controls:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>  
> +  mux-states:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +
>    mux-control-names:
>      description:
>        Devices that use more than a single mux controller can use the
>        "mux-control-names" property to map the name of the requested mux
>        controller to an index into the list given by the "mux-controls" property.
>  
> +  mux-state-names:
> +    description:
> +      Devices that use more than a single mux controller can use the
> +      "mux-state-names" property to map the name of the requested mux
> +      controller to an index into the list given by the "mux-states" property.
> +
>  additionalProperties: true
>  
>  ...
> diff --git a/Documentation/devicetree/bindings/mux/mux-controller.yaml b/Documentation/devicetree/bindings/mux/mux-controller.yaml
> index 736a84c3b6a5..b29dbf521f01 100644
> --- a/Documentation/devicetree/bindings/mux/mux-controller.yaml
> +++ b/Documentation/devicetree/bindings/mux/mux-controller.yaml
> @@ -25,7 +25,9 @@ description: |
>    --------------------
>  
>    Mux controller nodes must specify the number of cells used for the
> -  specifier using the '#mux-control-cells' property.
> +  specifier using the '#mux-control-cells' or 'mux-state-cells'
> +  property. Value of '#mux-state-cells' will always be one greater then
> +  the value of '#mux-control-cells'.
>  
>    Optionally, mux controller nodes can also specify the state the mux should
>    have when it is idle. The idle-state property is used for this. If the
> @@ -67,6 +69,8 @@ select:
>            pattern: '^mux-controller'
>      - required:
>          - '#mux-control-cells'
> +    - required:
> +        - '#mux-state-cells'
>  
>  properties:
>    $nodename:
> @@ -75,6 +79,9 @@ properties:
>    '#mux-control-cells':
>      enum: [ 0, 1 ]
>  
> +  '#mux-state-cells':
> +    enum: [ 1, 2 ]
> +
>    idle-state:
>      $ref: /schemas/types.yaml#/definitions/int32
>      minimum: -2
> @@ -179,4 +186,21 @@ examples:
>              };
>          };
>      };
> +
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    mux1: mux-controller {
> +        compatible = "gpio-mux";
> +        #mux-state-cells = <1>;
> +        mux-gpios = <&exp_som 2 GPIO_ACTIVE_HIGH>;
> +    };
> +
> +    transceiver4: can-phy4 {
> +        compatible = "ti,tcan1042";
> +        #phy-cells = <0>;
> +        max-bitrate = <5000000>;
> +        standby-gpios = <&exp_som 7 GPIO_ACTIVE_HIGH>;
> +        mux-states = <&mux1 1>;
> +    };
>  ...
> -- 
> 2.17.1
> 
> 

Re: [PATCH 1/2] dt-bindings: mux: Document mux-states property

[Peter Rosin]

On 2021-11-30 21:14, Rob Herring wrote:
> On Tue, Nov 30, 2021 at 05:48:46PM +0530, Aswath Govindraju wrote:
>> In some cases, it is required to provide the state to which the mux
>> controller has be set to, from the consumer device tree node. Document the
>> property mux-states that can be used for adding this support.
> 
> I having a hard time understanding why you need this. One consumer 
> configures a mux one way and another consumer another way? How do you 
> arbitrate that? Please elaborate on what 'some cases' are and why it's 
> required.
> 
> Can't you just add a cell for the 'state' allowing for 1-2 cells 
> instead of 0-1?

A mux controller can control several muxes. That happens e.g. when the
same gpio lines are connected to several mux chips in parallel. When
you operate one mux, the other parallel muxes just follow along. If
these muxes are then used orthogonally, coordination is needed. The real
world case I had was I2C and an analog signal connected to an ADC that
went through parallel/dependent muxes like this. It is simply not
possible to freely mux the I2C bus and the analog signal, they are tied
together and dependent and must coordinate their accesses.

The addition now is that Aswath wants a mux control client to "point
at" a single state instead of the whole mux control, and I see that as
a usable addition. It seems like a natural place to specify a single mux
state that some driver needs in some circumstance.

But, since a mux control is inherently a shared resource (see above),
one consumer might need a specific state and some other consumer might
need the whole mux control and manage the states as e.g. the existing
i2c-mux-gpmux binding is doing. So, you need to be able to specify both
ways to point at muxes; either to a single mux state, or to the whole mux
control.

While you could make the extra cell optional, that does not work for
the mux/adi,adg792a binding, since it is using the #mux-control-cells
property to determine which mode it should operate its three muxes in.
Either with one common/parallel mux control, or with three independent
mux controls.

So, that binding is already in the 0-1 territory, and adding an optional
extra cell makes it 0-1-2 with no way to determine what is intended when
the cell count is 1 (three independent mux controls OR one mux control
and a state). I see no way to add the extra state to that binding, short
of adding an extra property somewhere for that driver, but I simply did
not want to go that path because it would get inconsistent when trying
to add that in a backwards compatible way. Or rather, that was my
conclusion.

Suggestions welcome...

Cheers,
Peter

Re: [PATCH 1/2] dt-bindings: mux: Document mux-states property

[Aswath Govindraju]

Hi Rob,

On 01/12/21 2:18 am, Peter Rosin wrote:
> 
> 
> On 2021-11-30 21:14, Rob Herring wrote:
>> On Tue, Nov 30, 2021 at 05:48:46PM +0530, Aswath Govindraju wrote:
>>> In some cases, it is required to provide the state to which the mux
>>> controller has be set to, from the consumer device tree node. Document the
>>> property mux-states that can be used for adding this support.
>>
>> I having a hard time understanding why you need this. One consumer 
>> configures a mux one way and another consumer another way? How do you 
>> arbitrate that? Please elaborate on what 'some cases' are and why it's 
>> required.
>>
>> Can't you just add a cell for the 'state' allowing for 1-2 cells 
>> instead of 0-1?
> 
> A mux controller can control several muxes. That happens e.g. when the
> same gpio lines are connected to several mux chips in parallel. When
> you operate one mux, the other parallel muxes just follow along. If
> these muxes are then used orthogonally, coordination is needed. The real
> world case I had was I2C and an analog signal connected to an ADC that
> went through parallel/dependent muxes like this. It is simply not
> possible to freely mux the I2C bus and the analog signal, they are tied
> together and dependent and must coordinate their accesses.
> 
> The addition now is that Aswath wants a mux control client to "point
> at" a single state instead of the whole mux control, and I see that as
> a usable addition. It seems like a natural place to specify a single mux
> state that some driver needs in some circumstance.
> 
> But, since a mux control is inherently a shared resource (see above),
> one consumer might need a specific state and some other consumer might
> need the whole mux control and manage the states as e.g. the existing
> i2c-mux-gpmux binding is doing. So, you need to be able to specify both
> ways to point at muxes; either to a single mux state, or to the whole mux
> control.
> 
> While you could make the extra cell optional, that does not work for
> the mux/adi,adg792a binding, since it is using the #mux-control-cells
> property to determine which mode it should operate its three muxes in.
> Either with one common/parallel mux control, or with three independent
> mux controls.
> 
> So, that binding is already in the 0-1 territory, and adding an optional
> extra cell makes it 0-1-2 with no way to determine what is intended when
> the cell count is 1 (three independent mux controls OR one mux control
> and a state). I see no way to add the extra state to that binding, short
> of adding an extra property somewhere for that driver, but I simply did
> not want to go that path because it would get inconsistent when trying
> to add that in a backwards compatible way. Or rather, that was my
> conclusion.
> 
> Suggestions welcome...
> 


In addition to what Peter has mentioned, I would like to elaborate on my
use case for adding this feature. I am trying to implement this feature
in the TCAN104x transceiver driver, for selecting the mux state to route
the signals from CAN controller to transceivers on the board. The state
of the mux line to be set, can change based on the design and this is
needs to be provided from the device tree. Hence, I am trying to add
this support for providing the state to be set to the driver from the
device tree node.


Also, one more question on regarding DT check errors, may I know what
should be the order in which the patches need to be posted in order to
not get the error? This is because mux-states would be a new property to
be added in the TCAN104x bindings and I thought that it would need to be
posted after the patch for the changes in mux-controller are merged.

Thanks,
Aswath

> Cheers,
> Peter
> 

Re: [PATCH 1/2] dt-bindings: mux: Document mux-states property

[Rob Herring]

On Tue, Nov 30, 2021 at 10:32 PM Aswath Govindraju <a-govindraju@ti.com> wrote:
>
> Hi Rob,
>
> On 01/12/21 2:18 am, Peter Rosin wrote:
> >
> >
> > On 2021-11-30 21:14, Rob Herring wrote:
> >> On Tue, Nov 30, 2021 at 05:48:46PM +0530, Aswath Govindraju wrote:
> >>> In some cases, it is required to provide the state to which the mux
> >>> controller has be set to, from the consumer device tree node. Document the
> >>> property mux-states that can be used for adding this support.
> >>
> >> I having a hard time understanding why you need this. One consumer
> >> configures a mux one way and another consumer another way? How do you
> >> arbitrate that? Please elaborate on what 'some cases' are and why it's
> >> required.
> >>
> >> Can't you just add a cell for the 'state' allowing for 1-2 cells
> >> instead of 0-1?
> >
> > A mux controller can control several muxes. That happens e.g. when the
> > same gpio lines are connected to several mux chips in parallel. When
> > you operate one mux, the other parallel muxes just follow along. If
> > these muxes are then used orthogonally, coordination is needed. The real
> > world case I had was I2C and an analog signal connected to an ADC that
> > went through parallel/dependent muxes like this. It is simply not
> > possible to freely mux the I2C bus and the analog signal, they are tied
> > together and dependent and must coordinate their accesses.
> >
> > The addition now is that Aswath wants a mux control client to "point
> > at" a single state instead of the whole mux control, and I see that as
> > a usable addition. It seems like a natural place to specify a single mux
> > state that some driver needs in some circumstance.
> >
> > But, since a mux control is inherently a shared resource (see above),
> > one consumer might need a specific state and some other consumer might
> > need the whole mux control and manage the states as e.g. the existing
> > i2c-mux-gpmux binding is doing. So, you need to be able to specify both
> > ways to point at muxes; either to a single mux state, or to the whole mux
> > control.
> >
> > While you could make the extra cell optional, that does not work for
> > the mux/adi,adg792a binding, since it is using the #mux-control-cells
> > property to determine which mode it should operate its three muxes in.
> > Either with one common/parallel mux control, or with three independent
> > mux controls.
> >
> > So, that binding is already in the 0-1 territory, and adding an optional
> > extra cell makes it 0-1-2 with no way to determine what is intended when
> > the cell count is 1 (three independent mux controls OR one mux control
> > and a state). I see no way to add the extra state to that binding, short
> > of adding an extra property somewhere for that driver, but I simply did
> > not want to go that path because it would get inconsistent when trying
> > to add that in a backwards compatible way. Or rather, that was my
> > conclusion.
> >
> > Suggestions welcome...
> >
>
>
> In addition to what Peter has mentioned, I would like to elaborate on my
> use case for adding this feature. I am trying to implement this feature
> in the TCAN104x transceiver driver, for selecting the mux state to route
> the signals from CAN controller to transceivers on the board. The state
> of the mux line to be set, can change based on the design and this is
> needs to be provided from the device tree. Hence, I am trying to add
> this support for providing the state to be set to the driver from the
> device tree node.

Okay, please add something along the lines of what Peter said for when
you use which binding.

> Also, one more question on regarding DT check errors, may I know what
> should be the order in which the patches need to be posted in order to
> not get the error? This is because mux-states would be a new property to
> be added in the TCAN104x bindings and I thought that it would need to be
> posted after the patch for the changes in mux-controller are merged.

Looks like a circular dependency. Assuming you ran dt_binding_check on
the series, just add a note about the dependency and I won't send the
report.

Rob