[PATCH RFC v3 0/4] MUX: Add support for reading enable state from DT

[PATCH RFC v3 0/4] MUX: Add support for reading enable state from DT

[Aswath Govindraju]

- The following series of patches add support for reading the state of the
  mux to be set for enabling given device.
- As these are RFC patches I have combined them into a single series for
  better understanding of the reason behind making this change.

Changes since v2:
- Fixed changes in phy-can-transceiver based on comments
- added select MULTIPLEXER in drivers/phy/Kconfig
- Changed the implemetation of getting state by adding new apis

Changes since v1:
- Added support for reading the enable state from DT instead of hardcoding
  the state to be set to 1.
- Made relavent changes in the bindings

Link to v2:
- https://patchwork.kernel.org/project/linux-phy/list/?series=583917

Link to v1,
- https://patchwork.kernel.org/project/linux-phy/list/?series=578863&state=*

Aswath Govindraju (4):
  dt-bindings: mux: Increase the number of arguments in mux-controls
  dt-bindings: phy: ti,tcan104x-can: Document mux-controls property
  mux: Add support for reading mux enable state from DT
  phy: phy-can-transceiver: Add support for setting mux

 .../devicetree/bindings/mux/gpio-mux.yaml     |   2 +-
 .../bindings/mux/mux-controller.yaml          |   2 +-
 .../bindings/phy/ti,tcan104x-can.yaml         |   8 +
 drivers/mux/core.c                            | 146 +++++++++++++++++-
 drivers/phy/Kconfig                           |   1 +
 drivers/phy/phy-can-transceiver.c             |  22 +++
 include/linux/mux/consumer.h                  |  19 ++-
 include/linux/mux/driver.h                    |  13 ++
 8 files changed, 206 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH RFC v3 1/4] dt-bindings: mux: Increase the number of arguments in mux-controls

[Aswath Govindraju]

Increase the allowed number of arguments in mux-controls to add support for
passing information regarding the state of the mux to be set, for a given
device.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
 Documentation/devicetree/bindings/mux/gpio-mux.yaml       | 2 +-
 Documentation/devicetree/bindings/mux/mux-controller.yaml | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.yaml b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
index 0a7c8d64981a..c810b7df39de 100644
--- a/Documentation/devicetree/bindings/mux/gpio-mux.yaml
+++ b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
@@ -26,7 +26,7 @@ properties:
       List of gpios used to control the multiplexer, least significant bit first.
 
   '#mux-control-cells':
-    const: 0
+    enum: [ 0, 1, 2 ]
 
   idle-state:
     default: -1
diff --git a/Documentation/devicetree/bindings/mux/mux-controller.yaml b/Documentation/devicetree/bindings/mux/mux-controller.yaml
index 736a84c3b6a5..0b4b067a97bf 100644
--- a/Documentation/devicetree/bindings/mux/mux-controller.yaml
+++ b/Documentation/devicetree/bindings/mux/mux-controller.yaml
@@ -73,7 +73,7 @@ properties:
     pattern: '^mux-controller(@.*|-[0-9a-f]+)?$'
 
   '#mux-control-cells':
-    enum: [ 0, 1 ]
+    enum: [ 0, 1, 2 ]
 
   idle-state:
     $ref: /schemas/types.yaml#/definitions/int32
-- 
2.17.1

[PATCH RFC v3 2/4] dt-bindings: phy: ti,tcan104x-can: Document mux-controls property

[Aswath Govindraju]

On some boards, for routing CAN signals from controller to transceivers,
muxes might need to be set. Use mux-controls property can be used for model
this.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
 .../devicetree/bindings/phy/ti,tcan104x-can.yaml          | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
index 6107880e5246..7392088cf060 100644
--- a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
+++ b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
@@ -37,6 +37,13 @@ properties:
       max bit rate supported in bps
     minimum: 1
 
+  mux-controls:
+    description:
+      mux controller node to route the signals from controller to
+      transceiver. The first and second arguments can be used
+      representing the line in the mux-controller to control and
+      the state to be set in the given line respectively.
+
 required:
   - compatible
   - '#phy-cells'
@@ -53,4 +60,5 @@ examples:
       max-bitrate = <5000000>;
       standby-gpios = <&wakeup_gpio1 16 GPIO_ACTIVE_LOW>;
       enable-gpios = <&main_gpio1 67 GPIO_ACTIVE_HIGH>;
+      mux-controls = <&mux0 0 1>;
     };
-- 
2.17.1

[PATCH RFC v3 3/4] mux: Add support for reading mux enable state from DT

[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
the second argument of the mux-controls property.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
 drivers/mux/core.c           | 146 ++++++++++++++++++++++++++++++++++-
 include/linux/mux/consumer.h |  19 ++++-
 include/linux/mux/driver.h   |  13 ++++
 3 files changed, 173 insertions(+), 5 deletions(-)

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 22f4709768d1..9622b98f9818 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -370,6 +370,29 @@ int mux_control_select_delay(struct mux_control *mux, unsigned int state,
 }
 EXPORT_SYMBOL_GPL(mux_control_select_delay);
 
+/**
+ * mux_state_select_delay() - Select the enable state in mux-state
+ * @mux: The mux-state to request a change of state from.
+ * @delay_us: The time to delay (in microseconds) if the mux state is changed.
+ *
+ * On successfully selecting the enable 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 the enable state or a negative
+ * errno on error.
+ */
+int mux_state_select_delay(struct mux_state *mux, unsigned int delay_us)
+{
+	return mux_control_select_delay(mux->mux, mux->enable_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 +428,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 multiplexer enable state.
+ * @mux: The mux-control to request a change of state from.
+ * @delay_us: The time to delay (in microseconds) if the mux state is changed.
+ *
+ * On successfully selecting the enable 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 *mux, unsigned int delay_us)
+{
+	return mux_control_try_select_delay(mux->mux, mux->enable_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 +475,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 *mux)
+{
+	return mux_control_deselect(mux->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 +504,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.
+ * @enable_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 *enable_state)
 {
 	struct device_node *np = dev->of_node;
 	struct of_phandle_args args;
@@ -481,8 +545,7 @@ 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))) {
+	if (!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);
@@ -500,8 +563,25 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
 		return ERR_PTR(-EINVAL);
 	}
 
+	if (args.args_count == 2)
+		*enable_state = args.args[1];
+
 	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)
+{
+	int state;
+
+	return mux_get(dev, mux_name, &state);
+}
 EXPORT_SYMBOL_GPL(mux_control_get);
 
 /**
@@ -523,6 +603,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 *mux_state)
+{
+	mux_control_put(mux_state->mux);
+	kfree(mux_state);
+}
+EXPORT_SYMBOL_GPL(mux_state_put);
+
+static void devm_mux_state_release(struct device *dev, void *res)
+{
+	struct mux_state *mux = *(struct mux_state **)res;
+
+	mux_state_put(mux);
+}
+
 /**
  * devm_mux_control_get() - Get the mux-control for a device, with resource
  *			    management.
@@ -553,6 +653,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, *mux_state;
+	struct mux_control *mux_ctrl;
+	int enable_state;
+
+	mux_state = devm_kzalloc(dev, sizeof(struct mux_state), GFP_KERNEL);
+	if (!mux_state)
+		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, &enable_state);
+	if (IS_ERR(mux_ctrl)) {
+		devres_free(ptr);
+		return (struct mux_state *)mux_ctrl;
+	}
+
+	mux_state->mux = mux_ctrl;
+	mux_state->enable_state = enable_state;
+	*ptr = mux_state;
+	devres_add(dev, ptr);
+
+	return mux_state;
+}
+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..d0f3242e148d 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 *mux,
+					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 *mux,
+					    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 *mux)
+{
+	return mux_state_select_delay(mux, 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 *mux)
+{
+	return mux_state_try_select_delay(mux, 0);
+}
+
 int mux_control_deselect(struct mux_control *mux);
+int mux_state_deselect(struct mux_state *mux);
 
 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 *mux);
 
 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 */
diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
index 18824064f8c0..c7236f307fbd 100644
--- a/include/linux/mux/driver.h
+++ b/include/linux/mux/driver.h
@@ -53,6 +53,19 @@ struct mux_control {
 	ktime_t last_change;
 };
 
+/**
+ * struct mux_state -	Represents a mux controller specific to a given device
+ * @mux:		Pointer to a mux controller
+ * @enable_state	State of the mux to be set for enabling a device
+ *
+ * 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 enable_state;
+};
+
 /**
  * struct mux_chip -	Represents a chip holding mux controllers.
  * @controllers:	Number of mux controllers handled by the chip.
-- 
2.17.1

[PATCH RFC v3 4/4] phy: phy-can-transceiver: Add support for setting mux

[Aswath Govindraju]

On some boards, for routing CAN signals from controller to transceiver,
muxes might need to be set. Therefore, add support for setting the mux by
reading the mux-controls property from the device tree node.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
 drivers/phy/Kconfig               |  1 +
 drivers/phy/phy-can-transceiver.c | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 82b63e60c5a2..300b0f2b5f84 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -64,6 +64,7 @@ config USB_LGM_PHY
 config PHY_CAN_TRANSCEIVER
 	tristate "CAN transceiver PHY"
 	select GENERIC_PHY
+	select MULTIPLEXER
 	help
 	  This option enables support for CAN transceivers as a PHY. This
 	  driver provides function for putting the transceivers in various
diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
index 6f3fe37dee0e..6c94e3846410 100644
--- a/drivers/phy/phy-can-transceiver.c
+++ b/drivers/phy/phy-can-transceiver.c
@@ -10,6 +10,7 @@
 #include<linux/module.h>
 #include<linux/gpio.h>
 #include<linux/gpio/consumer.h>
+#include <linux/mux/consumer.h>
 
 struct can_transceiver_data {
 	u32 flags;
@@ -21,13 +22,22 @@ struct can_transceiver_phy {
 	struct phy *generic_phy;
 	struct gpio_desc *standby_gpio;
 	struct gpio_desc *enable_gpio;
+	struct mux_state *mux_state;
 };
 
 /* Power on function */
 static int can_transceiver_phy_power_on(struct phy *phy)
 {
+	int ret;
 	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
 
+	if (can_transceiver_phy->mux_state) {
+		ret = mux_state_select(can_transceiver_phy->mux_state);
+		if (ret) {
+			dev_err(&phy->dev, "Failed to select CAN mux: %d\n", ret);
+			return ret;
+		}
+	}
 	if (can_transceiver_phy->standby_gpio)
 		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 0);
 	if (can_transceiver_phy->enable_gpio)
@@ -45,6 +55,8 @@ static int can_transceiver_phy_power_off(struct phy *phy)
 		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 1);
 	if (can_transceiver_phy->enable_gpio)
 		gpiod_set_value_cansleep(can_transceiver_phy->enable_gpio, 0);
+	if (can_transceiver_phy->mux_state)
+		mux_state_deselect(can_transceiver_phy->mux_state);
 
 	return 0;
 }
@@ -95,6 +107,16 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
 	match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
 	drvdata = match->data;
 
+	if (of_property_read_bool(dev->of_node, "mux-controls")) {
+		struct mux_state *mux_state;
+
+		mux_state = devm_mux_state_get(dev, NULL);
+		if (IS_ERR(mux_state))
+			return dev_err_probe(&pdev->dev, PTR_ERR(mux_state),
+					     "failed to get mux\n");
+		can_transceiver_phy->mux_state = mux_state;
+	}
+
 	phy = devm_phy_create(dev, dev->of_node,
 			      &can_transceiver_phy_ops);
 	if (IS_ERR(phy)) {
-- 
2.17.1

Re: [PATCH RFC v3 1/4] dt-bindings: mux: Increase the number of arguments in mux-controls

[Peter Rosin]

Hi!

You need to have some description on how #mux-control-cells now work.
The previous description is in mux-consumer.yaml and an update there
is needed.

However, I have realized that the adg792a binding uses #mux-control-cells
to indicate if it should expose its three muxes with one mux-control
and operate the muxes in parallel, or if it should be expose three
independent mux-controls. So, the approach in this series to always
have the #mux-control-cells property fixed at <2> when indicating a
state will not work for that binding. And I see no fix for that binding
without adding a new property.

So, I would like a different approach. Since I dislike how mux-controls
-after this series- is not (always) specifying a mux-control like the name
says, but instead optionally a specific state, the new property I would
like to add is #mux-state-cells such that it would always be one more
than #mux-control-cells.

	mux: mux-controller {
		compatible = "gpio-mux";
		#mux-control-cells = <0>;
		#mux-state-cells = <1>;

		mux-gpios = <...>;
	};

	can-phy {
		compatible = "ti,tcan1043";
		...
		mux-states = <&mux 1>;
	};

That solves the naming issue, the unused argument for mux-conrtrollers
that previously had #mux-control-cells = <0>, and the binding for adg792a
need no longer be inconsistent.

Or, how should this be solved? I'm sure there are other options...

Cheers,
Peter

On 2021-11-23 09:12, Aswath Govindraju wrote:
> Increase the allowed number of arguments in mux-controls to add support for
> passing information regarding the state of the mux to be set, for a given
> device.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  Documentation/devicetree/bindings/mux/gpio-mux.yaml       | 2 +-
>  Documentation/devicetree/bindings/mux/mux-controller.yaml | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.yaml b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
> index 0a7c8d64981a..c810b7df39de 100644
> --- a/Documentation/devicetree/bindings/mux/gpio-mux.yaml
> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
> @@ -26,7 +26,7 @@ properties:
>        List of gpios used to control the multiplexer, least significant bit first.
>  
>    '#mux-control-cells':
> -    const: 0
> +    enum: [ 0, 1, 2 ]
>  
>    idle-state:
>      default: -1
> diff --git a/Documentation/devicetree/bindings/mux/mux-controller.yaml b/Documentation/devicetree/bindings/mux/mux-controller.yaml
> index 736a84c3b6a5..0b4b067a97bf 100644
> --- a/Documentation/devicetree/bindings/mux/mux-controller.yaml
> +++ b/Documentation/devicetree/bindings/mux/mux-controller.yaml
> @@ -73,7 +73,7 @@ properties:
>      pattern: '^mux-controller(@.*|-[0-9a-f]+)?$'
>  
>    '#mux-control-cells':
> -    enum: [ 0, 1 ]
> +    enum: [ 0, 1, 2 ]
>  
>    idle-state:
>      $ref: /schemas/types.yaml#/definitions/int32
> 

Re: [PATCH RFC v3 3/4] mux: Add support for reading mux enable state from DT

[Peter Rosin]

Hi!

On 2021-11-23 09:12, 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
> the second argument of the mux-controls property.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  drivers/mux/core.c           | 146 ++++++++++++++++++++++++++++++++++-
>  include/linux/mux/consumer.h |  19 ++++-
>  include/linux/mux/driver.h   |  13 ++++
>  3 files changed, 173 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
> index 22f4709768d1..9622b98f9818 100644
> --- a/drivers/mux/core.c
> +++ b/drivers/mux/core.c
> @@ -370,6 +370,29 @@ int mux_control_select_delay(struct mux_control *mux, unsigned int state,
>  }
>  EXPORT_SYMBOL_GPL(mux_control_select_delay);
>  
> +/**
> + * mux_state_select_delay() - Select the enable state in mux-state

The terminology is that you have a "mux" with different "states" that you
"select". What you are referring to as enabling a mux state, is elsewhere
referred to as selecting the mux state.

> + * @mux: The mux-state to request a change of state from.
> + * @delay_us: The time to delay (in microseconds) if the mux state is changed.
> + *
> + * On successfully selecting the enable 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 the enable state or a negative
> + * errno on error.
> + */
> +int mux_state_select_delay(struct mux_state *mux, unsigned int delay_us)

I dislike the name "mux" here, that name is consistently referring to
a struct mux-control in the mux subsystem. If mux_state is too long,
maybe mstate or something such, just not plain mux. That goes for all
the struct mux_state variables that follow too. I.e. pick a new name
for variables of this type and stick to it (unless you need more than
one such variable in a context, of course).

> +{
> +	return mux_control_select_delay(mux->mux, mux->enable_state, delay_us);
> +}
> +EXPORT_SYMBOL_GPL(mux_state_select_delay);

Taking the above into account, I would like to see:

/**
 * mux_state_select_delay() - Select the mux-state
 * @mstate: 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, its mux-control 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 the mux-control is deselected (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)

And then similar changes for the other new mux_state_ functions.

> +
>  /**
>   * 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 +428,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 multiplexer enable state.
> + * @mux: The mux-control to request a change of state from.
> + * @delay_us: The time to delay (in microseconds) if the mux state is changed.
> + *
> + * On successfully selecting the enable 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 *mux, unsigned int delay_us)
> +{
> +	return mux_control_try_select_delay(mux->mux, mux->enable_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 +475,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 *mux)
> +{
> +	return mux_control_deselect(mux->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 +504,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.
> + * @enable_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 *enable_state)

s/enable_state/state/ (goes for all of the patch).

>  {
>  	struct device_node *np = dev->of_node;
>  	struct of_phandle_args args;
> @@ -481,8 +545,7 @@ 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 ||

It is inconsistent to allow more than 2 args, but then only allow
digging out the state from the 2nd arg if the count is exactly 2.

> -	    (!args.args_count && (mux_chip->controllers > 1))) {
> +	if (!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);
> @@ -500,8 +563,25 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	if (args.args_count == 2)
> +		*enable_state = args.args[1];
> +

With the suggested binding in my comment for patch 1/4, you'd need to do
either

	ret = of_parse_phandle_with_args(np,
					 "mux-controls", "#mux-control-cells",
					 index, &args);

or

	ret = of_parse_phandle_with_args(np,
					 "mux-states", "#mux-state-cells",
					 index, &args);

depending on if the mux_get helper gets a NULL (enable_)state pointer or a "real"
address, and then...

>  	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)
> +{
> +	int state;
> +
> +	return mux_get(dev, mux_name, &state);

...pass NULL as the 3rd arg here.

> +}
>  EXPORT_SYMBOL_GPL(mux_control_get);
>  
>  /**
> @@ -523,6 +603,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 *mux_state)
> +{
> +	mux_control_put(mux_state->mux);
> +	kfree(mux_state);
> +}
> +EXPORT_SYMBOL_GPL(mux_state_put);
> +
> +static void devm_mux_state_release(struct device *dev, void *res)
> +{
> +	struct mux_state *mux = *(struct mux_state **)res;
> +
> +	mux_state_put(mux);
> +}
> +
>  /**
>   * devm_mux_control_get() - Get the mux-control for a device, with resource
>   *			    management.
> @@ -553,6 +653,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, *mux_state;
> +	struct mux_control *mux_ctrl;
> +	int enable_state;
> +
> +	mux_state = devm_kzalloc(dev, sizeof(struct mux_state), GFP_KERNEL);
> +	if (!mux_state)
> +		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, &enable_state);
> +	if (IS_ERR(mux_ctrl)) {
> +		devres_free(ptr);
> +		return (struct mux_state *)mux_ctrl;
> +	}
> +
> +	mux_state->mux = mux_ctrl;
> +	mux_state->enable_state = enable_state;
> +	*ptr = mux_state;
> +	devres_add(dev, ptr);
> +
> +	return mux_state;
> +}
> +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..d0f3242e148d 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 *mux,
> +					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 *mux,
> +					    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 *mux)
> +{
> +	return mux_state_select_delay(mux, 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 *mux)
> +{
> +	return mux_state_try_select_delay(mux, 0);
> +}
> +
>  int mux_control_deselect(struct mux_control *mux);
> +int mux_state_deselect(struct mux_state *mux);
>  
>  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 *mux);
>  
>  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 */
> diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
> index 18824064f8c0..c7236f307fbd 100644
> --- a/include/linux/mux/driver.h
> +++ b/include/linux/mux/driver.h
> @@ -53,6 +53,19 @@ struct mux_control {
>  	ktime_t last_change;
>  };
>  
> +/**
> + * struct mux_state -	Represents a mux controller specific to a given device
> + * @mux:		Pointer to a mux controller
> + * @enable_state	State of the mux to be set for enabling a device
> + *
> + * 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 enable_state;
> +};
> +
>  /**
>   * struct mux_chip -	Represents a chip holding mux controllers.
>   * @controllers:	Number of mux controllers handled by the chip.
> 

This struct does not belong in driver.h, as it's purely a consumer thing.
That said, it need not be in consumer.h either, as there is no need to
"expose" the struct guts in any header. Just add it directly in core.c
which is the only file that digs around in the struct.

Cheers,
Peter

Re: [PATCH RFC v3 4/4] phy: phy-can-transceiver: Add support for setting mux

[Peter Rosin]

Hi!

On 2021-11-23 09:12, Aswath Govindraju wrote:
> On some boards, for routing CAN signals from controller to transceiver,
> muxes might need to be set. Therefore, add support for setting the mux by
> reading the mux-controls property from the device tree node.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  drivers/phy/Kconfig               |  1 +
>  drivers/phy/phy-can-transceiver.c | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 82b63e60c5a2..300b0f2b5f84 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -64,6 +64,7 @@ config USB_LGM_PHY
>  config PHY_CAN_TRANSCEIVER
>  	tristate "CAN transceiver PHY"
>  	select GENERIC_PHY
> +	select MULTIPLEXER
>  	help
>  	  This option enables support for CAN transceivers as a PHY. This
>  	  driver provides function for putting the transceivers in various
> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
> index 6f3fe37dee0e..6c94e3846410 100644
> --- a/drivers/phy/phy-can-transceiver.c
> +++ b/drivers/phy/phy-can-transceiver.c
> @@ -10,6 +10,7 @@
>  #include<linux/module.h>
>  #include<linux/gpio.h>
>  #include<linux/gpio/consumer.h>
> +#include <linux/mux/consumer.h>
>  
>  struct can_transceiver_data {
>  	u32 flags;
> @@ -21,13 +22,22 @@ struct can_transceiver_phy {
>  	struct phy *generic_phy;
>  	struct gpio_desc *standby_gpio;
>  	struct gpio_desc *enable_gpio;
> +	struct mux_state *mux_state;
>  };
>  
>  /* Power on function */
>  static int can_transceiver_phy_power_on(struct phy *phy)
>  {
> +	int ret;
>  	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
>  
> +	if (can_transceiver_phy->mux_state) {
> +		ret = mux_state_select(can_transceiver_phy->mux_state);
> +		if (ret) {
> +			dev_err(&phy->dev, "Failed to select CAN mux: %d\n", ret);
> +			return ret;
> +		}
> +	}
>  	if (can_transceiver_phy->standby_gpio)
>  		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 0);
>  	if (can_transceiver_phy->enable_gpio)
> @@ -45,6 +55,8 @@ static int can_transceiver_phy_power_off(struct phy *phy)
>  		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 1);
>  	if (can_transceiver_phy->enable_gpio)
>  		gpiod_set_value_cansleep(can_transceiver_phy->enable_gpio, 0);
> +	if (can_transceiver_phy->mux_state)
> +		mux_state_deselect(can_transceiver_phy->mux_state);

Careful, it is not obvious that you are following the documentation you
added in patch 3/4

+ * 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.

Or, are you absolutely certain that can_transceiver_phy_power_off cannot,
in any curcumstance, be called without a successful previous call to can_transceiver_phy_power_on? Or that can_transceiver_phy_power_on will
never ever be called again without a can_transceiver_phy_power_off in
between the two on calls?

If there is any doubt, you need to remember if you have selected/deselected
the mux. Maybe this should be remebered inside struct mux_state so that it
is always safe to call mux_state_select/mux_state_deselect? That's one way
to solve this difficulty.

But then again, the phy layer might ensure that extra precaution is not
needed. But it might also be that it for sure is intended that this is solved
in the phy layer, but that callbacks (or whatever) has been added "after the
fact" and that an extra "on" or "off" has just been just shrugged at.

Cheers,
Peter

>  
>  	return 0;
>  }
> @@ -95,6 +107,16 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
>  	match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
>  	drvdata = match->data;
>  
> +	if (of_property_read_bool(dev->of_node, "mux-controls")) {
> +		struct mux_state *mux_state;
> +
> +		mux_state = devm_mux_state_get(dev, NULL);
> +		if (IS_ERR(mux_state))
> +			return dev_err_probe(&pdev->dev, PTR_ERR(mux_state),
> +					     "failed to get mux\n");
> +		can_transceiver_phy->mux_state = mux_state;
> +	}
> +
>  	phy = devm_phy_create(dev, dev->of_node,
>  			      &can_transceiver_phy_ops);
>  	if (IS_ERR(phy)) {
> 

Re: [PATCH RFC v3 1/4] dt-bindings: mux: Increase the number of arguments in mux-controls

[Aswath Govindraju]

Hi Peter,

On 25/11/21 7:05 pm, Peter Rosin wrote:
> Hi!
> 
> You need to have some description on how #mux-control-cells now work.
> The previous description is in mux-consumer.yaml and an update there
> is needed.
> 
> However, I have realized that the adg792a binding uses #mux-control-cells
> to indicate if it should expose its three muxes with one mux-control
> and operate the muxes in parallel, or if it should be expose three
> independent mux-controls. So, the approach in this series to always
> have the #mux-control-cells property fixed at <2> when indicating a
> state will not work for that binding. And I see no fix for that binding
> without adding a new property.
> 
> So, I would like a different approach. Since I dislike how mux-controls
> -after this series- is not (always) specifying a mux-control like the name
> says, but instead optionally a specific state, the new property I would
> like to add is #mux-state-cells such that it would always be one more
> than #mux-control-cells.
> 
> 	mux: mux-controller {
> 		compatible = "gpio-mux";
> 		#mux-control-cells = <0>;
> 		#mux-state-cells = <1>;
> 
> 		mux-gpios = <...>;
> 	};
> 
> 	can-phy {
> 		compatible = "ti,tcan1043";
> 		...
> 		mux-states = <&mux 1>;
> 	};
> 
> That solves the naming issue, the unused argument for mux-conrtrollers
> that previously had #mux-control-cells = <0>, and the binding for adg792a
> need no longer be inconsistent.
> 
> Or, how should this be solved? I'm sure there are other options...
> 


I feel that the new approach using mux-state-cells seems to be
overpopulating the device tree nodes, when the state can be represented
using the control cells. I understand that the definition for
mux-controls is to only specify the control line to be used in a given
mux. Can't it now be upgraded to also represent the state at which the
control line has to be set to?

With respect to adg792a, it is inline with the current implementation
and the only change I think would be required in the driver is,

diff --git a/drivers/mux/adg792a.c b/drivers/mux/adg792a.c
index e8fc2fc1ab09..2cd3bb8a40d4 100644
--- a/drivers/mux/adg792a.c
+++ b/drivers/mux/adg792a.c
@@ -73,8 +73,6 @@ static int adg792a_probe(struct i2c_client *i2c)
        ret = device_property_read_u32(dev, "#mux-control-cells", &cells);
        if (ret < 0)
                return ret;
-       if (cells >= 2)
-               return -EINVAL;

        mux_chip = devm_mux_chip_alloc(dev, cells ? 3 : 1, 0);
        if (IS_ERR(mux_chip))

And the following series should be compatible with it. If adg792a driver
is the only issue then would there be any issue with only changing it
and using this implementation?

Thanks,
Aswath




> Cheers,
> Peter
> 
> On 2021-11-23 09:12, Aswath Govindraju wrote:
>> Increase the allowed number of arguments in mux-controls to add support for
>> passing information regarding the state of the mux to be set, for a given
>> device.
>>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
>>  Documentation/devicetree/bindings/mux/gpio-mux.yaml       | 2 +-
>>  Documentation/devicetree/bindings/mux/mux-controller.yaml | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.yaml b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>> index 0a7c8d64981a..c810b7df39de 100644
>> --- a/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>> @@ -26,7 +26,7 @@ properties:
>>        List of gpios used to control the multiplexer, least significant bit first.
>>  
>>    '#mux-control-cells':
>> -    const: 0
>> +    enum: [ 0, 1, 2 ]
>>  
>>    idle-state:
>>      default: -1
>> diff --git a/Documentation/devicetree/bindings/mux/mux-controller.yaml b/Documentation/devicetree/bindings/mux/mux-controller.yaml
>> index 736a84c3b6a5..0b4b067a97bf 100644
>> --- a/Documentation/devicetree/bindings/mux/mux-controller.yaml
>> +++ b/Documentation/devicetree/bindings/mux/mux-controller.yaml
>> @@ -73,7 +73,7 @@ properties:
>>      pattern: '^mux-controller(@.*|-[0-9a-f]+)?$'
>>  
>>    '#mux-control-cells':
>> -    enum: [ 0, 1 ]
>> +    enum: [ 0, 1, 2 ]
>>  
>>    idle-state:
>>      $ref: /schemas/types.yaml#/definitions/int32
>>

Re: [PATCH RFC v3 3/4] mux: Add support for reading mux enable state from DT

[Aswath Govindraju]

Hi Peter,

On 25/11/21 7:22 pm, Peter Rosin wrote:
> Hi!
> 
> On 2021-11-23 09:12, 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
>> the second argument of the mux-controls property.
>>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
>>  drivers/mux/core.c           | 146 ++++++++++++++++++++++++++++++++++-
>>  include/linux/mux/consumer.h |  19 ++++-
>>  include/linux/mux/driver.h   |  13 ++++
>>  3 files changed, 173 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>> index 22f4709768d1..9622b98f9818 100644
>> --- a/drivers/mux/core.c
>> +++ b/drivers/mux/core.c
>> @@ -370,6 +370,29 @@ int mux_control_select_delay(struct mux_control *mux, unsigned int state,
>>  }
>>  EXPORT_SYMBOL_GPL(mux_control_select_delay);
>>  
>> +/**
>> + * mux_state_select_delay() - Select the enable state in mux-state
> 
> The terminology is that you have a "mux" with different "states" that you
> "select". What you are referring to as enabling a mux state, is elsewhere
> referred to as selecting the mux state.
> 

Sorry, for mentioning what I mean by enable state. So, the idea is the
the state that would be mentioned in the DT property would be the state
to which the mux to be set for enabling the given device and hence I am
referring to it as enable state. I feel that referring to it as state
would not convey the above.

>> + * @mux: The mux-state to request a change of state from.
>> + * @delay_us: The time to delay (in microseconds) if the mux state is changed.
>> + *
>> + * On successfully selecting the enable 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 the enable state or a negative
>> + * errno on error.
>> + */
>> +int mux_state_select_delay(struct mux_state *mux, unsigned int delay_us)
> 
> I dislike the name "mux" here, that name is consistently referring to
> a struct mux-control in the mux subsystem. If mux_state is too long,
> maybe mstate or something such, just not plain mux. That goes for all
> the struct mux_state variables that follow too. I.e. pick a new name
> for variables of this type and stick to it (unless you need more than
> one such variable in a context, of course).
> 

Yes, using mux for mux_state type does seem to be misleading. I'll
change it to the mstate.

Thanks,
Aswath

>> +{
>> +	return mux_control_select_delay(mux->mux, mux->enable_state, delay_us);
>> +}
>> +EXPORT_SYMBOL_GPL(mux_state_select_delay);
> 
> Taking the above into account, I would like to see:
> 
> /**
>  * mux_state_select_delay() - Select the mux-state
>  * @mstate: 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, its mux-control 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 the mux-control is deselected (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)
> 
> And then similar changes for the other new mux_state_ functions.
> 
>> +
>>  /**
>>   * 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 +428,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 multiplexer enable state.
>> + * @mux: The mux-control to request a change of state from.
>> + * @delay_us: The time to delay (in microseconds) if the mux state is changed.
>> + *
>> + * On successfully selecting the enable 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 *mux, unsigned int delay_us)
>> +{
>> +	return mux_control_try_select_delay(mux->mux, mux->enable_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 +475,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 *mux)
>> +{
>> +	return mux_control_deselect(mux->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 +504,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.
>> + * @enable_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 *enable_state)
> 
> s/enable_state/state/ (goes for all of the patch).
> 
>>  {
>>  	struct device_node *np = dev->of_node;
>>  	struct of_phandle_args args;
>> @@ -481,8 +545,7 @@ 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 ||
> 
> It is inconsistent to allow more than 2 args, but then only allow
> digging out the state from the 2nd arg if the count is exactly 2.
> 
>> -	    (!args.args_count && (mux_chip->controllers > 1))) {
>> +	if (!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);
>> @@ -500,8 +563,25 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>  		return ERR_PTR(-EINVAL);
>>  	}
>>  
>> +	if (args.args_count == 2)
>> +		*enable_state = args.args[1];
>> +
> 
> With the suggested binding in my comment for patch 1/4, you'd need to do
> either
> 
> 	ret = of_parse_phandle_with_args(np,
> 					 "mux-controls", "#mux-control-cells",
> 					 index, &args);
> 
> or
> 
> 	ret = of_parse_phandle_with_args(np,
> 					 "mux-states", "#mux-state-cells",
> 					 index, &args);
> 
> depending on if the mux_get helper gets a NULL (enable_)state pointer or a "real"
> address, and then...
> 
>>  	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)
>> +{
>> +	int state;
>> +
>> +	return mux_get(dev, mux_name, &state);
> 
> ...pass NULL as the 3rd arg here.
> 
>> +}
>>  EXPORT_SYMBOL_GPL(mux_control_get);
>>  
>>  /**
>> @@ -523,6 +603,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 *mux_state)
>> +{
>> +	mux_control_put(mux_state->mux);
>> +	kfree(mux_state);
>> +}
>> +EXPORT_SYMBOL_GPL(mux_state_put);
>> +
>> +static void devm_mux_state_release(struct device *dev, void *res)
>> +{
>> +	struct mux_state *mux = *(struct mux_state **)res;
>> +
>> +	mux_state_put(mux);
>> +}
>> +
>>  /**
>>   * devm_mux_control_get() - Get the mux-control for a device, with resource
>>   *			    management.
>> @@ -553,6 +653,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, *mux_state;
>> +	struct mux_control *mux_ctrl;
>> +	int enable_state;
>> +
>> +	mux_state = devm_kzalloc(dev, sizeof(struct mux_state), GFP_KERNEL);
>> +	if (!mux_state)
>> +		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, &enable_state);
>> +	if (IS_ERR(mux_ctrl)) {
>> +		devres_free(ptr);
>> +		return (struct mux_state *)mux_ctrl;
>> +	}
>> +
>> +	mux_state->mux = mux_ctrl;
>> +	mux_state->enable_state = enable_state;
>> +	*ptr = mux_state;
>> +	devres_add(dev, ptr);
>> +
>> +	return mux_state;
>> +}
>> +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..d0f3242e148d 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 *mux,
>> +					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 *mux,
>> +					    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 *mux)
>> +{
>> +	return mux_state_select_delay(mux, 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 *mux)
>> +{
>> +	return mux_state_try_select_delay(mux, 0);
>> +}
>> +
>>  int mux_control_deselect(struct mux_control *mux);
>> +int mux_state_deselect(struct mux_state *mux);
>>  
>>  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 *mux);
>>  
>>  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 */
>> diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
>> index 18824064f8c0..c7236f307fbd 100644
>> --- a/include/linux/mux/driver.h
>> +++ b/include/linux/mux/driver.h
>> @@ -53,6 +53,19 @@ struct mux_control {
>>  	ktime_t last_change;
>>  };
>>  
>> +/**
>> + * struct mux_state -	Represents a mux controller specific to a given device
>> + * @mux:		Pointer to a mux controller
>> + * @enable_state	State of the mux to be set for enabling a device
>> + *
>> + * 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 enable_state;
>> +};
>> +
>>  /**
>>   * struct mux_chip -	Represents a chip holding mux controllers.
>>   * @controllers:	Number of mux controllers handled by the chip.
>>
> 
> This struct does not belong in driver.h, as it's purely a consumer thing.
> That said, it need not be in consumer.h either, as there is no need to
> "expose" the struct guts in any header. Just add it directly in core.c
> which is the only file that digs around in the struct.
> 
> Cheers,
> Peter
> 

Re: [PATCH RFC v3 4/4] phy: phy-can-transceiver: Add support for setting mux

[Aswath Govindraju]

Hi Peter,

On 25/11/21 7:37 pm, Peter Rosin wrote:
> Hi!
> 
> On 2021-11-23 09:12, Aswath Govindraju wrote:
>> On some boards, for routing CAN signals from controller to transceiver,
>> muxes might need to be set. Therefore, add support for setting the mux by
>> reading the mux-controls property from the device tree node.
>>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
>>  drivers/phy/Kconfig               |  1 +
>>  drivers/phy/phy-can-transceiver.c | 22 ++++++++++++++++++++++
>>  2 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 82b63e60c5a2..300b0f2b5f84 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -64,6 +64,7 @@ config USB_LGM_PHY
>>  config PHY_CAN_TRANSCEIVER
>>  	tristate "CAN transceiver PHY"
>>  	select GENERIC_PHY
>> +	select MULTIPLEXER
>>  	help
>>  	  This option enables support for CAN transceivers as a PHY. This
>>  	  driver provides function for putting the transceivers in various
>> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
>> index 6f3fe37dee0e..6c94e3846410 100644
>> --- a/drivers/phy/phy-can-transceiver.c
>> +++ b/drivers/phy/phy-can-transceiver.c
>> @@ -10,6 +10,7 @@
>>  #include<linux/module.h>
>>  #include<linux/gpio.h>
>>  #include<linux/gpio/consumer.h>
>> +#include <linux/mux/consumer.h>
>>  
>>  struct can_transceiver_data {
>>  	u32 flags;
>> @@ -21,13 +22,22 @@ struct can_transceiver_phy {
>>  	struct phy *generic_phy;
>>  	struct gpio_desc *standby_gpio;
>>  	struct gpio_desc *enable_gpio;
>> +	struct mux_state *mux_state;
>>  };
>>  
>>  /* Power on function */
>>  static int can_transceiver_phy_power_on(struct phy *phy)
>>  {
>> +	int ret;
>>  	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
>>  
>> +	if (can_transceiver_phy->mux_state) {
>> +		ret = mux_state_select(can_transceiver_phy->mux_state);
>> +		if (ret) {
>> +			dev_err(&phy->dev, "Failed to select CAN mux: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>>  	if (can_transceiver_phy->standby_gpio)
>>  		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 0);
>>  	if (can_transceiver_phy->enable_gpio)
>> @@ -45,6 +55,8 @@ static int can_transceiver_phy_power_off(struct phy *phy)
>>  		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 1);
>>  	if (can_transceiver_phy->enable_gpio)
>>  		gpiod_set_value_cansleep(can_transceiver_phy->enable_gpio, 0);
>> +	if (can_transceiver_phy->mux_state)
>> +		mux_state_deselect(can_transceiver_phy->mux_state);
> 
> Careful, it is not obvious that you are following the documentation you
> added in patch 3/4
> 
> + * 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.
> 
> Or, are you absolutely certain that can_transceiver_phy_power_off cannot,
> in any curcumstance, be called without a successful previous call to can_transceiver_phy_power_on? Or that can_transceiver_phy_power_on will
> never ever be called again without a can_transceiver_phy_power_off in
> between the two on calls?
> 
> If there is any doubt, you need to remember if you have selected/deselected
> the mux. Maybe this should be remebered inside struct mux_state so that it
> is always safe to call mux_state_select/mux_state_deselect? That's one way
> to solve this difficulty.
> 
> But then again, the phy layer might ensure that extra precaution is not
> needed. But it might also be that it for sure is intended that this is solved
> in the phy layer, but that callbacks (or whatever) has been added "after the
> fact" and that an extra "on" or "off" has just been just shrugged at.
> 

Thank you for pointing this out. I did forget to think about this case.
However, as you mentioned the phy layer does indeed only call the
can_transceiver_phy_power_off only if can_transceiver_phy_power_on was
called earlier and this is being done using power_count,

int phy_power_off(struct phy *phy)
{
        int ret;

        if (!phy)
                return 0;

        mutex_lock(&phy->mutex);
        if (phy->power_count == 1 && phy->ops->power_off) {
                ret =  phy->ops->power_off(phy);
                if (ret < 0) {
                        dev_err(&phy->dev, "phy poweroff failed -->
%d\n", ret);
                        mutex_unlock(&phy->mutex);
                        return ret;
                }
        }
        --phy->power_count;
        mutex_unlock(&phy->mutex);
        phy_pm_runtime_put(phy);

        if (phy->pwr)
                regulator_disable(phy->pwr);

        return 0;
}

Thanks,
Aswath

> Cheers,
> Peter
> 
>>  
>>  	return 0;
>>  }
>> @@ -95,6 +107,16 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
>>  	match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
>>  	drvdata = match->data;
>>  
>> +	if (of_property_read_bool(dev->of_node, "mux-controls")) {
>> +		struct mux_state *mux_state;
>> +
>> +		mux_state = devm_mux_state_get(dev, NULL);
>> +		if (IS_ERR(mux_state))
>> +			return dev_err_probe(&pdev->dev, PTR_ERR(mux_state),
>> +					     "failed to get mux\n");
>> +		can_transceiver_phy->mux_state = mux_state;
>> +	}
>> +
>>  	phy = devm_phy_create(dev, dev->of_node,
>>  			      &can_transceiver_phy_ops);
>>  	if (IS_ERR(phy)) {
>>

Re: [PATCH RFC v3 1/4] dt-bindings: mux: Increase the number of arguments in mux-controls

[Peter Rosin]

On 2021-11-29 05:36, Aswath Govindraju wrote:
> Hi Peter,
> 
> On 25/11/21 7:05 pm, Peter Rosin wrote:
>> Hi!
>>
>> You need to have some description on how #mux-control-cells now work.
>> The previous description is in mux-consumer.yaml and an update there
>> is needed.
>>
>> However, I have realized that the adg792a binding uses #mux-control-cells
>> to indicate if it should expose its three muxes with one mux-control
>> and operate the muxes in parallel, or if it should be expose three
>> independent mux-controls. So, the approach in this series to always
>> have the #mux-control-cells property fixed at <2> when indicating a
>> state will not work for that binding. And I see no fix for that binding
>> without adding a new property.
>>
>> So, I would like a different approach. Since I dislike how mux-controls
>> -after this series- is not (always) specifying a mux-control like the name
>> says, but instead optionally a specific state, the new property I would
>> like to add is #mux-state-cells such that it would always be one more
>> than #mux-control-cells.
>>
>> 	mux: mux-controller {
>> 		compatible = "gpio-mux";
>> 		#mux-control-cells = <0>;
>> 		#mux-state-cells = <1>;
>>
>> 		mux-gpios = <...>;
>> 	};
>>
>> 	can-phy {
>> 		compatible = "ti,tcan1043";
>> 		...
>> 		mux-states = <&mux 1>;
>> 	};
>>
>> That solves the naming issue, the unused argument for mux-conrtrollers
>> that previously had #mux-control-cells = <0>, and the binding for adg792a
>> need no longer be inconsistent.
>>
>> Or, how should this be solved? I'm sure there are other options...
>>
> 
> 
> I feel that the new approach using mux-state-cells seems to be
> overpopulating the device tree nodes, when the state can be represented
> using the control cells. I understand that the definition for
> mux-controls is to only specify the control line to be used in a given
> mux. Can't it now be upgraded to also represent the state at which the
> control line has to be set to?
> 
> With respect to adg792a, it is inline with the current implementation
> and the only change I think would be required in the driver is,

No, that does not work. See below.

> 
> diff --git a/drivers/mux/adg792a.c b/drivers/mux/adg792a.c
> index e8fc2fc1ab09..2cd3bb8a40d4 100644
> --- a/drivers/mux/adg792a.c
> +++ b/drivers/mux/adg792a.c
> @@ -73,8 +73,6 @@ static int adg792a_probe(struct i2c_client *i2c)
>         ret = device_property_read_u32(dev, "#mux-control-cells", &cells);
>         if (ret < 0)
>                 return ret;
> -       if (cells >= 2)
> -               return -EINVAL;
> 
>         mux_chip = devm_mux_chip_alloc(dev, cells ? 3 : 1, 0);

When you add cell #2 with the state, the cells variable will end up
as 2 always. Which means that there is no way to alloc one mux
control since "cells ? 3 : 1" will always end up as "3", with no
easy fix.

So, your approach does not work for this driver.

Cheers,
Peter

>         if (IS_ERR(mux_chip))
> 
> And the following series should be compatible with it. If adg792a driver
> is the only issue then would there be any issue with only changing it
> and using this implementation?
> 
> Thanks,
> Aswath
> 
> 
> 
> 
>> Cheers,
>> Peter
>>
>> On 2021-11-23 09:12, Aswath Govindraju wrote:
>>> Increase the allowed number of arguments in mux-controls to add support for
>>> passing information regarding the state of the mux to be set, for a given
>>> device.
>>>
>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>>> ---
>>>  Documentation/devicetree/bindings/mux/gpio-mux.yaml       | 2 +-
>>>  Documentation/devicetree/bindings/mux/mux-controller.yaml | 2 +-
>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.yaml b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>>> index 0a7c8d64981a..c810b7df39de 100644
>>> --- a/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>>> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>>> @@ -26,7 +26,7 @@ properties:
>>>        List of gpios used to control the multiplexer, least significant bit first.
>>>  
>>>    '#mux-control-cells':
>>> -    const: 0
>>> +    enum: [ 0, 1, 2 ]
>>>  
>>>    idle-state:
>>>      default: -1
>>> diff --git a/Documentation/devicetree/bindings/mux/mux-controller.yaml b/Documentation/devicetree/bindings/mux/mux-controller.yaml
>>> index 736a84c3b6a5..0b4b067a97bf 100644
>>> --- a/Documentation/devicetree/bindings/mux/mux-controller.yaml
>>> +++ b/Documentation/devicetree/bindings/mux/mux-controller.yaml
>>> @@ -73,7 +73,7 @@ properties:
>>>      pattern: '^mux-controller(@.*|-[0-9a-f]+)?$'
>>>  
>>>    '#mux-control-cells':
>>> -    enum: [ 0, 1 ]
>>> +    enum: [ 0, 1, 2 ]
>>>  
>>>    idle-state:
>>>      $ref: /schemas/types.yaml#/definitions/int32
>>>
> 

Re: [PATCH RFC v3 3/4] mux: Add support for reading mux enable state from DT

[Peter Rosin]

On 2021-11-29 05:44, Aswath Govindraju wrote:
> Hi Peter,
> 
> On 25/11/21 7:22 pm, Peter Rosin wrote:
>> Hi!
>>
>> On 2021-11-23 09:12, 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
>>> the second argument of the mux-controls property.
>>>
>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>>> ---
>>>  drivers/mux/core.c           | 146 ++++++++++++++++++++++++++++++++++-
>>>  include/linux/mux/consumer.h |  19 ++++-
>>>  include/linux/mux/driver.h   |  13 ++++
>>>  3 files changed, 173 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>>> index 22f4709768d1..9622b98f9818 100644
>>> --- a/drivers/mux/core.c
>>> +++ b/drivers/mux/core.c
>>> @@ -370,6 +370,29 @@ int mux_control_select_delay(struct mux_control *mux, unsigned int state,
>>>  }
>>>  EXPORT_SYMBOL_GPL(mux_control_select_delay);
>>>  
>>> +/**
>>> + * mux_state_select_delay() - Select the enable state in mux-state
>>
>> The terminology is that you have a "mux" with different "states" that you
>> "select". What you are referring to as enabling a mux state, is elsewhere
>> referred to as selecting the mux state.
>>
> 
> Sorry, for mentioning what I mean by enable state. So, the idea is the
> the state that would be mentioned in the DT property would be the state
> to which the mux to be set for enabling the given device and hence I am
> referring to it as enable state. I feel that referring to it as state
> would not convey the above.

Ah, but that this state it is use to "enable" your device is a mux
consumer detail in the context of the phy-can driver. Some other
driver might need a specific mux state for something completely
unrelated. So, the "enable" naming should not spread into the mux
code.

The situation is similar to when a driver needs an enable-gpio, the
gpio consumer knows that it's a gpio used to enable the device (or
whatever), but the gpio subsystem does not bother at all with what
the gpio is used for.

Cheers,
Peter

Re: [PATCH RFC v3 1/4] dt-bindings: mux: Increase the number of arguments in mux-controls

[Aswath Govindraju]

Hi Peter,

On 29/11/21 1:45 pm, Peter Rosin wrote:
> 
> 
> On 2021-11-29 05:36, Aswath Govindraju wrote:
>> Hi Peter,
>>
>> On 25/11/21 7:05 pm, Peter Rosin wrote:
>>> Hi!
>>>
>>> You need to have some description on how #mux-control-cells now work.
>>> The previous description is in mux-consumer.yaml and an update there
>>> is needed.
>>>
>>> However, I have realized that the adg792a binding uses #mux-control-cells
>>> to indicate if it should expose its three muxes with one mux-control
>>> and operate the muxes in parallel, or if it should be expose three
>>> independent mux-controls. So, the approach in this series to always
>>> have the #mux-control-cells property fixed at <2> when indicating a
>>> state will not work for that binding. And I see no fix for that binding
>>> without adding a new property.
>>>
>>> So, I would like a different approach. Since I dislike how mux-controls
>>> -after this series- is not (always) specifying a mux-control like the name
>>> says, but instead optionally a specific state, the new property I would
>>> like to add is #mux-state-cells such that it would always be one more
>>> than #mux-control-cells.
>>>
>>> 	mux: mux-controller {
>>> 		compatible = "gpio-mux";
>>> 		#mux-control-cells = <0>;
>>> 		#mux-state-cells = <1>;
>>>
>>> 		mux-gpios = <...>;
>>> 	};
>>>
>>> 	can-phy {
>>> 		compatible = "ti,tcan1043";
>>> 		...
>>> 		mux-states = <&mux 1>;
>>> 	};
>>>
>>> That solves the naming issue, the unused argument for mux-conrtrollers
>>> that previously had #mux-control-cells = <0>, and the binding for adg792a
>>> need no longer be inconsistent.
>>>
>>> Or, how should this be solved? I'm sure there are other options...
>>>
>>
>>
>> I feel that the new approach using mux-state-cells seems to be
>> overpopulating the device tree nodes, when the state can be represented
>> using the control cells. I understand that the definition for
>> mux-controls is to only specify the control line to be used in a given
>> mux. Can't it now be upgraded to also represent the state at which the
>> control line has to be set to?
>>
>> With respect to adg792a, it is inline with the current implementation
>> and the only change I think would be required in the driver is,
> 
> No, that does not work. See below.
> 
>>
>> diff --git a/drivers/mux/adg792a.c b/drivers/mux/adg792a.c
>> index e8fc2fc1ab09..2cd3bb8a40d4 100644
>> --- a/drivers/mux/adg792a.c
>> +++ b/drivers/mux/adg792a.c
>> @@ -73,8 +73,6 @@ static int adg792a_probe(struct i2c_client *i2c)
>>         ret = device_property_read_u32(dev, "#mux-control-cells", &cells);
>>         if (ret < 0)
>>                 return ret;
>> -       if (cells >= 2)
>> -               return -EINVAL;
>>
>>         mux_chip = devm_mux_chip_alloc(dev, cells ? 3 : 1, 0);
> 
> When you add cell #2 with the state, the cells variable will end up
> as 2 always. Which means that there is no way to alloc one mux
> control since "cells ? 3 : 1" will always end up as "3", with no
> easy fix.
> 
> So, your approach does not work for this driver.
> 

Sorry, but how is this different from the case of

#mux-control-cells = 1

If #mux-control-cells is equal to 1 it means the consumer will use a
given control line from the mux chip. The same would be the case when we
will be using, #mux-control-cells is equal to 2, except we can also
provide the state.

If the consumer will use all the lines then #mux-control-cells will be
set to 0. In this condition the state can't be provided from the DT and
the consumer will be controlling the entire mux chip. If
#mux-control-cells is greater than 0 then we will not be able to provide
multiple lines of control using a single mux-controls entry(mux-controls
= <...>;) right? We would have the using multiple mux-controls
entries(mux-controls = <...>, <...>;).

Thanks,
Aswath

> Cheers,
> Peter
> 
>>         if (IS_ERR(mux_chip))
>>
>> And the following series should be compatible with it. If adg792a driver
>> is the only issue then would there be any issue with only changing it
>> and using this implementation?
>>
>> Thanks,
>> Aswath
>>
>>
>>
>>
>>> Cheers,
>>> Peter
>>>
>>> On 2021-11-23 09:12, Aswath Govindraju wrote:
>>>> Increase the allowed number of arguments in mux-controls to add support for
>>>> passing information regarding the state of the mux to be set, for a given
>>>> device.
>>>>
>>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/mux/gpio-mux.yaml       | 2 +-
>>>>  Documentation/devicetree/bindings/mux/mux-controller.yaml | 2 +-
>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.yaml b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>>>> index 0a7c8d64981a..c810b7df39de 100644
>>>> --- a/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>>>> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>>>> @@ -26,7 +26,7 @@ properties:
>>>>        List of gpios used to control the multiplexer, least significant bit first.
>>>>  
>>>>    '#mux-control-cells':
>>>> -    const: 0
>>>> +    enum: [ 0, 1, 2 ]
>>>>  
>>>>    idle-state:
>>>>      default: -1
>>>> diff --git a/Documentation/devicetree/bindings/mux/mux-controller.yaml b/Documentation/devicetree/bindings/mux/mux-controller.yaml
>>>> index 736a84c3b6a5..0b4b067a97bf 100644
>>>> --- a/Documentation/devicetree/bindings/mux/mux-controller.yaml
>>>> +++ b/Documentation/devicetree/bindings/mux/mux-controller.yaml
>>>> @@ -73,7 +73,7 @@ properties:
>>>>      pattern: '^mux-controller(@.*|-[0-9a-f]+)?$'
>>>>  
>>>>    '#mux-control-cells':
>>>> -    enum: [ 0, 1 ]
>>>> +    enum: [ 0, 1, 2 ]
>>>>  
>>>>    idle-state:
>>>>      $ref: /schemas/types.yaml#/definitions/int32
>>>>
>>
> 

Re: [PATCH RFC v3 1/4] dt-bindings: mux: Increase the number of arguments in mux-controls

[Peter Rosin]

Hi Aswath,

On 2021-11-29 10:31, Aswath Govindraju wrote:
> Hi Peter,
> 
> On 29/11/21 1:45 pm, Peter Rosin wrote:
>>
>>
>> On 2021-11-29 05:36, Aswath Govindraju wrote:
>>> Hi Peter,
>>>
>>> On 25/11/21 7:05 pm, Peter Rosin wrote:
>>>> Hi!
>>>>
>>>> You need to have some description on how #mux-control-cells now work.
>>>> The previous description is in mux-consumer.yaml and an update there
>>>> is needed.
>>>>
>>>> However, I have realized that the adg792a binding uses #mux-control-cells
>>>> to indicate if it should expose its three muxes with one mux-control
>>>> and operate the muxes in parallel, or if it should be expose three
>>>> independent mux-controls. So, the approach in this series to always
>>>> have the #mux-control-cells property fixed at <2> when indicating a
>>>> state will not work for that binding. And I see no fix for that binding
>>>> without adding a new property.
>>>>
>>>> So, I would like a different approach. Since I dislike how mux-controls
>>>> -after this series- is not (always) specifying a mux-control like the name
>>>> says, but instead optionally a specific state, the new property I would
>>>> like to add is #mux-state-cells such that it would always be one more
>>>> than #mux-control-cells.
>>>>
>>>> 	mux: mux-controller {
>>>> 		compatible = "gpio-mux";
>>>> 		#mux-control-cells = <0>;
>>>> 		#mux-state-cells = <1>;
>>>>
>>>> 		mux-gpios = <...>;
>>>> 	};
>>>>
>>>> 	can-phy {
>>>> 		compatible = "ti,tcan1043";
>>>> 		...
>>>> 		mux-states = <&mux 1>;
>>>> 	};
>>>>
>>>> That solves the naming issue, the unused argument for mux-conrtrollers
>>>> that previously had #mux-control-cells = <0>, and the binding for adg792a
>>>> need no longer be inconsistent.
>>>>
>>>> Or, how should this be solved? I'm sure there are other options...
>>>>
>>>
>>>
>>> I feel that the new approach using mux-state-cells seems to be
>>> overpopulating the device tree nodes, when the state can be represented
>>> using the control cells. I understand that the definition for
>>> mux-controls is to only specify the control line to be used in a given
>>> mux. Can't it now be upgraded to also represent the state at which the
>>> control line has to be set to?
>>>
>>> With respect to adg792a, it is inline with the current implementation
>>> and the only change I think would be required in the driver is,
>>
>> No, that does not work. See below.
>>
>>>
>>> diff --git a/drivers/mux/adg792a.c b/drivers/mux/adg792a.c
>>> index e8fc2fc1ab09..2cd3bb8a40d4 100644
>>> --- a/drivers/mux/adg792a.c
>>> +++ b/drivers/mux/adg792a.c
>>> @@ -73,8 +73,6 @@ static int adg792a_probe(struct i2c_client *i2c)
>>>         ret = device_property_read_u32(dev, "#mux-control-cells", &cells);
>>>         if (ret < 0)
>>>                 return ret;
>>> -       if (cells >= 2)
>>> -               return -EINVAL;
>>>
>>>         mux_chip = devm_mux_chip_alloc(dev, cells ? 3 : 1, 0);
>>
>> When you add cell #2 with the state, the cells variable will end up
>> as 2 always. Which means that there is no way to alloc one mux
>> control since "cells ? 3 : 1" will always end up as "3", with no
>> easy fix.
>>
>> So, your approach does not work for this driver.
>>
> 
> Sorry, but how is this different from the case of
> 
> #mux-control-cells = 1
> 
> If #mux-control-cells is equal to 1 it means the consumer will use a
> given control line from the mux chip. The same would be the case when we
> will be using, #mux-control-cells is equal to 2, except we can also
> provide the state.
> 
> If the consumer will use all the lines then #mux-control-cells will be
> set to 0. In this condition the state can't be provided from the DT and
> the consumer will be controlling the entire mux chip. If
> #mux-control-cells is greater than 0 then we will not be able to provide
> multiple lines of control using a single mux-controls entry(mux-controls
> = <...>;) right? We would have the using multiple mux-controls
> entries(mux-controls = <...>, <...>;).

I think you misunderstand. The adg792a driver operates the chip in
different modes depending on if you specify 0 or 1 cells. With 0,
it's not just that the consumer operates three muxes. It is also, and
more importantly, that the three muxes are operated in parallel without
the consumer doing anything different with the single mux control it
sees (even if there are three muxes operated by that single mux
control).

That said, yes, you can make it limp along like you describe above.
But why should it not be possible to specify a specific state when
the adg792a driver operates the muxes in parallel? And yes, you could
add some other flag to indicate this mode, but my point is that it
is silly to add special cases like this if you don't need to. Since
adding a specific state is the new thing, that is what should be
added in a way that fits with the old stuff without imposing new
flags on that old stuff.

An example: the three muxes in an adg792a chip could be used as
two muxes for some I2C bus (SCL and SDA signals) and the third mux
for something unrelated. Suppose that you want to operate the adg792a
as three parallel muxes so that you mux SCL and SDA simultaneously
(as is expected by the i2c-mux-gpmux binding, it only expects one
mux control), and that you want to use the third mux as the enable-
state for your phy. With your suggested binding you cannot, unless
you add a mechanism to make the adg792a driver operate its muxes in
parallel even if there are two cells instead of zero. I.e. without
that new flag the i2c-mux-gpmux binding needs to see

	#mux-control-cells = <0>;

while your new phy binding instead needs to see

	#mux-control-cells = <2>;

And you obviously can't have it both ways.

(Sure, it would not be possible to mux the I2C bus while the phy
is enabled in the above example, but there could be some other
limitation in place that makes that invalid anyway. And it's just
an example anyway...)

A mux-control is potentially a shared resource, and bindings have
to take this into account.

Cheers,
Peter

Re: [PATCH RFC v3 1/4] dt-bindings: mux: Increase the number of arguments in mux-controls

[Aswath Govindraju]

Hi Peter,

On 29/11/21 5:58 pm, Peter Rosin wrote:
> Hi Aswath,
> 
> On 2021-11-29 10:31, Aswath Govindraju wrote:
>> Hi Peter,
>>
>> On 29/11/21 1:45 pm, Peter Rosin wrote:
>>>
>>>
>>> On 2021-11-29 05:36, Aswath Govindraju wrote:
>>>> Hi Peter,
>>>>
>>>> On 25/11/21 7:05 pm, Peter Rosin wrote:
>>>>> Hi!
>>>>>
>>>>> You need to have some description on how #mux-control-cells now work.
>>>>> The previous description is in mux-consumer.yaml and an update there
>>>>> is needed.
>>>>>
>>>>> However, I have realized that the adg792a binding uses #mux-control-cells
>>>>> to indicate if it should expose its three muxes with one mux-control
>>>>> and operate the muxes in parallel, or if it should be expose three
>>>>> independent mux-controls. So, the approach in this series to always
>>>>> have the #mux-control-cells property fixed at <2> when indicating a
>>>>> state will not work for that binding. And I see no fix for that binding
>>>>> without adding a new property.
>>>>>
>>>>> So, I would like a different approach. Since I dislike how mux-controls
>>>>> -after this series- is not (always) specifying a mux-control like the name
>>>>> says, but instead optionally a specific state, the new property I would
>>>>> like to add is #mux-state-cells such that it would always be one more
>>>>> than #mux-control-cells.
>>>>>
>>>>> 	mux: mux-controller {
>>>>> 		compatible = "gpio-mux";
>>>>> 		#mux-control-cells = <0>;
>>>>> 		#mux-state-cells = <1>;
>>>>>
>>>>> 		mux-gpios = <...>;
>>>>> 	};
>>>>>
>>>>> 	can-phy {
>>>>> 		compatible = "ti,tcan1043";
>>>>> 		...
>>>>> 		mux-states = <&mux 1>;
>>>>> 	};
>>>>>
>>>>> That solves the naming issue, the unused argument for mux-conrtrollers
>>>>> that previously had #mux-control-cells = <0>, and the binding for adg792a
>>>>> need no longer be inconsistent.
>>>>>
>>>>> Or, how should this be solved? I'm sure there are other options...
>>>>>
>>>>
>>>>
>>>> I feel that the new approach using mux-state-cells seems to be
>>>> overpopulating the device tree nodes, when the state can be represented
>>>> using the control cells. I understand that the definition for
>>>> mux-controls is to only specify the control line to be used in a given
>>>> mux. Can't it now be upgraded to also represent the state at which the
>>>> control line has to be set to?
>>>>
>>>> With respect to adg792a, it is inline with the current implementation
>>>> and the only change I think would be required in the driver is,
>>>
>>> No, that does not work. See below.
>>>
>>>>
>>>> diff --git a/drivers/mux/adg792a.c b/drivers/mux/adg792a.c
>>>> index e8fc2fc1ab09..2cd3bb8a40d4 100644
>>>> --- a/drivers/mux/adg792a.c
>>>> +++ b/drivers/mux/adg792a.c
>>>> @@ -73,8 +73,6 @@ static int adg792a_probe(struct i2c_client *i2c)
>>>>         ret = device_property_read_u32(dev, "#mux-control-cells", &cells);
>>>>         if (ret < 0)
>>>>                 return ret;
>>>> -       if (cells >= 2)
>>>> -               return -EINVAL;
>>>>
>>>>         mux_chip = devm_mux_chip_alloc(dev, cells ? 3 : 1, 0);
>>>
>>> When you add cell #2 with the state, the cells variable will end up
>>> as 2 always. Which means that there is no way to alloc one mux
>>> control since "cells ? 3 : 1" will always end up as "3", with no
>>> easy fix.
>>>
>>> So, your approach does not work for this driver.
>>>
>>
>> Sorry, but how is this different from the case of
>>
>> #mux-control-cells = 1
>>
>> If #mux-control-cells is equal to 1 it means the consumer will use a
>> given control line from the mux chip. The same would be the case when we
>> will be using, #mux-control-cells is equal to 2, except we can also
>> provide the state.
>>
>> If the consumer will use all the lines then #mux-control-cells will be
>> set to 0. In this condition the state can't be provided from the DT and
>> the consumer will be controlling the entire mux chip. If
>> #mux-control-cells is greater than 0 then we will not be able to provide
>> multiple lines of control using a single mux-controls entry(mux-controls
>> = <...>;) right? We would have the using multiple mux-controls
>> entries(mux-controls = <...>, <...>;).
> 
> I think you misunderstand. The adg792a driver operates the chip in
> different modes depending on if you specify 0 or 1 cells. With 0,
> it's not just that the consumer operates three muxes. It is also, and
> more importantly, that the three muxes are operated in parallel without
> the consumer doing anything different with the single mux control it
> sees (even if there are three muxes operated by that single mux
> control).
> 
> That said, yes, you can make it limp along like you describe above.
> But why should it not be possible to specify a specific state when
> the adg792a driver operates the muxes in parallel? And yes, you could
> add some other flag to indicate this mode, but my point is that it
> is silly to add special cases like this if you don't need to. Since
> adding a specific state is the new thing, that is what should be
> added in a way that fits with the old stuff without imposing new
> flags on that old stuff.
> 
> An example: the three muxes in an adg792a chip could be used as
> two muxes for some I2C bus (SCL and SDA signals) and the third mux
> for something unrelated. Suppose that you want to operate the adg792a
> as three parallel muxes so that you mux SCL and SDA simultaneously
> (as is expected by the i2c-mux-gpmux binding, it only expects one
> mux control), and that you want to use the third mux as the enable-
> state for your phy. With your suggested binding you cannot, unless
> you add a mechanism to make the adg792a driver operate its muxes in
> parallel even if there are two cells instead of zero. I.e. without
> that new flag the i2c-mux-gpmux binding needs to see
> 
> 	#mux-control-cells = <0>;
> 
> while your new phy binding instead needs to see
> 
> 	#mux-control-cells = <2>;
> 
> And you obviously can't have it both ways.
> 
> (Sure, it would not be possible to mux the I2C bus while the phy
> is enabled in the above example, but there could be some other
> limitation in place that makes that invalid anyway. And it's just
> an example anyway...)
> 
> A mux-control is potentially a shared resource, and bindings have
> to take this into account.
> 

Understood. Thank you for the explanation. Will correct the
implementation and post a respin.

Regards,
Aswath

> Cheers,
> Peter
> 

Re: [PATCH RFC v3 3/4] mux: Add support for reading mux enable state from DT

[Aswath Govindraju]

Hi Peter,

On 25/11/21 7:22 pm, Peter Rosin wrote:
> Hi!
> 
> On 2021-11-23 09:12, 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
>> the second argument of the mux-controls property.
>>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
>>  drivers/mux/core.c           | 146 ++++++++++++++++++++++++++++++++++-
>>  include/linux/mux/consumer.h |  19 ++++-
>>  include/linux/mux/driver.h   |  13 ++++
>>  3 files changed, 173 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>> index 22f4709768d1..9622b98f9818 100644
>> --- a/drivers/mux/core.c
>> +++ b/drivers/mux/core.c
>> @@ -370,6 +370,29 @@ int mux_control_select_delay(struct mux_control *mux, unsigned int state,
>>  }
>>  EXPORT_SYMBOL_GPL(mux_control_select_delay);
>>  

[...]

>>  }
>>  
>>  /**
>> - * 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.
>> + * @enable_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 *enable_state)
> 
> s/enable_state/state/ (goes for all of the patch).
> 
>>  {
>>  	struct device_node *np = dev->of_node;
>>  	struct of_phandle_args args;
>> @@ -481,8 +545,7 @@ 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 ||
> 
> It is inconsistent to allow more than 2 args, but then only allow
> digging out the state from the 2nd arg if the count is exactly 2.
> 
>> -	    (!args.args_count && (mux_chip->controllers > 1))) {
>> +	if (!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);
>> @@ -500,8 +563,25 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>  		return ERR_PTR(-EINVAL);
>>  	}
>>  
>> +	if (args.args_count == 2)
>> +		*enable_state = args.args[1];
>> +
> 
> With the suggested binding in my comment for patch 1/4, you'd need to do
> either
> 
> 	ret = of_parse_phandle_with_args(np,
> 					 "mux-controls", "#mux-control-cells",
> 					 index, &args);
> 
> or
> 
> 	ret = of_parse_phandle_with_args(np,
> 					 "mux-states", "#mux-state-cells",
> 					 index, &args);
> 
> depending on if the mux_get helper gets a NULL (enable_)state pointer or a "real"
> address, and then...
> 


Sorry, while trying to implement the above method, I came across one
more question. So, in a given consumer DT node we will be either having
only mux-states or mux-controls right? How would we take care of the
condition when we would want to set the state of a given line in the
controller. Especially when a single mux chip is used by multiple
consumers each using a different line. Wouldn't we require both
mux-controls and mux-states in that case? So, shouldn't the
implementation be such that we need to first read mux-controls and then
based whether the enable_state is NULL, we read mux-states?

Just to add more clarity, if we go about this method then we would have
both mux-controls and mux-states in the consumer DT node when we want to
specify the state.

Thanks,
Aswath

>>  	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)
>> +{

[...]

Re: [PATCH RFC v3 3/4] mux: Add support for reading mux enable state from DT

[Aswath Govindraju]

Hi Peter,

On 30/11/21 11:14 am, Aswath Govindraju wrote:
> Hi Peter,
> 
> On 25/11/21 7:22 pm, Peter Rosin wrote:
>> Hi!
>>
>> On 2021-11-23 09:12, 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
>>> the second argument of the mux-controls property.
>>>
>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>>> ---
>>>  drivers/mux/core.c           | 146 ++++++++++++++++++++++++++++++++++-
>>>  include/linux/mux/consumer.h |  19 ++++-
>>>  include/linux/mux/driver.h   |  13 ++++
>>>  3 files changed, 173 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>>> index 22f4709768d1..9622b98f9818 100644
>>> --- a/drivers/mux/core.c
>>> +++ b/drivers/mux/core.c
>>> @@ -370,6 +370,29 @@ int mux_control_select_delay(struct mux_control *mux, unsigned int state,
>>>  }
>>>  EXPORT_SYMBOL_GPL(mux_control_select_delay);
>>>  
> 
> [...]
> 
>>>  }
>>>  
>>>  /**
>>> - * 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.
>>> + * @enable_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 *enable_state)
>>
>> s/enable_state/state/ (goes for all of the patch).
>>
>>>  {
>>>  	struct device_node *np = dev->of_node;
>>>  	struct of_phandle_args args;
>>> @@ -481,8 +545,7 @@ 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 ||
>>
>> It is inconsistent to allow more than 2 args, but then only allow
>> digging out the state from the 2nd arg if the count is exactly 2.
>>
>>> -	    (!args.args_count && (mux_chip->controllers > 1))) {
>>> +	if (!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);
>>> @@ -500,8 +563,25 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>>  		return ERR_PTR(-EINVAL);
>>>  	}
>>>  
>>> +	if (args.args_count == 2)
>>> +		*enable_state = args.args[1];
>>> +
>>
>> With the suggested binding in my comment for patch 1/4, you'd need to do
>> either
>>
>> 	ret = of_parse_phandle_with_args(np,
>> 					 "mux-controls", "#mux-control-cells",
>> 					 index, &args);
>>
>> or
>>
>> 	ret = of_parse_phandle_with_args(np,
>> 					 "mux-states", "#mux-state-cells",
>> 					 index, &args);
>>
>> depending on if the mux_get helper gets a NULL (enable_)state pointer or a "real"
>> address, and then...
>>
> 
> 
> Sorry, while trying to implement the above method, I came across one
> more question. So, in a given consumer DT node we will be either having
> only mux-states or mux-controls right? How would we take care of the
> condition when we would want to set the state of a given line in the
> controller. Especially when a single mux chip is used by multiple
> consumers each using a different line. Wouldn't we require both
> mux-controls and mux-states in that case? So, shouldn't the
> implementation be such that we need to first read mux-controls and then
> based whether the enable_state is NULL, we read mux-states?
> 
> Just to add more clarity, if we go about this method then we would have
> both mux-controls and mux-states in the consumer DT node when we want to
> specify the state.
> 

I now understood the implementation that you described. mux-states will
be similar to the mux-controls after this patch is applied. mux-states
can have 2 arguments(mux line and state) whereas mux-controls can have
only one argument(mux line).

Sorry, for the inconvenience.

Thanks,
Aswath

> Thanks,
> Aswath
> 
>>>  	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)
>>> +{
> 
> [...]
> 

Re: [PATCH RFC v3 3/4] mux: Add support for reading mux enable state from DT

[Peter Rosin]

On 2021-11-30 06:58, Aswath Govindraju wrote:
> On 30/11/21 11:14 am, Aswath Govindraju wrote:
>> On 25/11/21 7:22 pm, Peter Rosin wrote:
>>> With the suggested binding in my comment for patch 1/4, you'd need to do
>>> either
>>>
>>> 	ret = of_parse_phandle_with_args(np,
>>> 					 "mux-controls", "#mux-control-cells",
>>> 					 index, &args);
>>>
>>> or
>>>
>>> 	ret = of_parse_phandle_with_args(np,
>>> 					 "mux-states", "#mux-state-cells",
>>> 					 index, &args);
>>>
>>> depending on if the mux_get helper gets a NULL (enable_)state pointer or a "real"
>>> address, and then...
>>>
>>
>>
>> Sorry, while trying to implement the above method, I came across one
>> more question. So, in a given consumer DT node we will be either having
>> only mux-states or mux-controls right? How would we take care of the
>> condition when we would want to set the state of a given line in the
>> controller. Especially when a single mux chip is used by multiple
>> consumers each using a different line. Wouldn't we require both
>> mux-controls and mux-states in that case? So, shouldn't the
>> implementation be such that we need to first read mux-controls and then
>> based whether the enable_state is NULL, we read mux-states?
>>
>> Just to add more clarity, if we go about this method then we would have
>> both mux-controls and mux-states in the consumer DT node when we want to
>> specify the state.
>>
> 
> I now understood the implementation that you described. mux-states will
> be similar to the mux-controls after this patch is applied. mux-states
> can have 2 arguments(mux line and state) whereas mux-controls can have
> only one argument(mux line).
> 
> Sorry, for the inconvenience.

No trouble at all. And thanks for tackling this! I think it can open up
the mux subsystem to more uses.

I'm not sure if the devicetree folks like the concept though, there has
been no comment from that direction yet. Because it does feel a bit
unusual to potentially have both #mux-control-cells and #mux-state-cells
in a single mux provider node, especially when they are as related as
they are. But sharing a mux control between several consumer is perhaps
not the most common usage, so it will probably be the exception to
require both in actual usage. But I think all that will be clearer
when/if we see the actual binding patches.

Cheers,
Peter