[PATCH RFC 0/2] CAN TRANSCEIVER: Add support for setting mux

[PATCH RFC 0/2] 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.

Aswath Govindraju (2):
  dt-bindings: phy: ti,tcan104x-can: Document mux-controls property
  phy: phy-can-transceiver: Add support for setting mux

 .../bindings/phy/ti,tcan104x-can.yaml         |  6 ++++++
 drivers/phy/phy-can-transceiver.c             | 21 +++++++++++++++++++
 2 files changed, 27 insertions(+)

-- 
2.17.1

[PATCH RFC 0/2] 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.

Aswath Govindraju (2):
  dt-bindings: phy: ti,tcan104x-can: Document mux-controls property
  phy: phy-can-transceiver: Add support for setting mux

 .../bindings/phy/ti,tcan104x-can.yaml         |  6 ++++++
 drivers/phy/phy-can-transceiver.c             | 21 +++++++++++++++++++
 2 files changed, 27 insertions(+)

-- 
2.17.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH RFC 1/2] 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>
---
 Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
index 6107880e5246..24cc3c04b85d 100644
--- a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
+++ b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
@@ -37,6 +37,11 @@ properties:
       max bit rate supported in bps
     minimum: 1
 
+  mux-controls:
+    description:
+      mux controller node to route the signals from controller to
+      transceiver.
+
 required:
   - compatible
   - '#phy-cells'
@@ -53,4 +58,5 @@ examples:
       max-bitrate = <5000000>;
       standby-gpios = <&wakeup_gpio1 16 GPIO_ACTIVE_LOW>;
       enable-gpios = <&main_gpio1 67 GPIO_ACTIVE_HIGH>;
+      mux-controls = <&mux0>;
     };
-- 
2.17.1

[PATCH RFC 1/2] 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>
---
 Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
index 6107880e5246..24cc3c04b85d 100644
--- a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
+++ b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
@@ -37,6 +37,11 @@ properties:
       max bit rate supported in bps
     minimum: 1
 
+  mux-controls:
+    description:
+      mux controller node to route the signals from controller to
+      transceiver.
+
 required:
   - compatible
   - '#phy-cells'
@@ -53,4 +58,5 @@ examples:
       max-bitrate = <5000000>;
       standby-gpios = <&wakeup_gpio1 16 GPIO_ACTIVE_LOW>;
       enable-gpios = <&main_gpio1 67 GPIO_ACTIVE_HIGH>;
+      mux-controls = <&mux0>;
     };
-- 
2.17.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH RFC 2/2] 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/phy-can-transceiver.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
index 6f3fe37dee0e..3d8da5226e27 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_control *mux_ctrl;
 };
 
 /* 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_ctrl) {
+		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);
+		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_ctrl)
+		mux_control_deselect(can_transceiver_phy->mux_ctrl);
 
 	return 0;
 }
@@ -95,6 +107,15 @@ 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_control *control;
+
+		control = devm_mux_control_get(dev, NULL);
+		if (IS_ERR(control))
+			return PTR_ERR(control);
+		can_transceiver_phy->mux_ctrl = control;
+	}
+
 	phy = devm_phy_create(dev, dev->of_node,
 			      &can_transceiver_phy_ops);
 	if (IS_ERR(phy)) {
-- 
2.17.1

[PATCH RFC 2/2] 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/phy-can-transceiver.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
index 6f3fe37dee0e..3d8da5226e27 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_control *mux_ctrl;
 };
 
 /* 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_ctrl) {
+		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);
+		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_ctrl)
+		mux_control_deselect(can_transceiver_phy->mux_ctrl);
 
 	return 0;
 }
@@ -95,6 +107,15 @@ 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_control *control;
+
+		control = devm_mux_control_get(dev, NULL);
+		if (IS_ERR(control))
+			return PTR_ERR(control);
+		can_transceiver_phy->mux_ctrl = control;
+	}
+
 	phy = devm_phy_create(dev, dev->of_node,
 			      &can_transceiver_phy_ops);
 	if (IS_ERR(phy)) {
-- 
2.17.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Marc Kleine-Budde]

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

On 11.11.2021 22:13: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/phy-can-transceiver.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
> index 6f3fe37dee0e..3d8da5226e27 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_control *mux_ctrl;
>  };
>  
>  /* 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_ctrl) {
> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);

Hard coding the "1" looks wrong here. I have seen some boards where you
can select between a CAN-2.0 and a single wire CAN transceiver with a
mux. So I think we cannot hard code the "1" here.

> +		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_ctrl)
> +		mux_control_deselect(can_transceiver_phy->mux_ctrl);
>  
>  	return 0;
>  }
> @@ -95,6 +107,15 @@ 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")) {

Is this the proper way of doing this? Looks like we need a
devm_mux_control_get_optional(), which doesn't return a -ENODEV if the
device doesn't exist.

Cc'ed Peter Rosin.

> +		struct mux_control *control;
> +
> +		control = devm_mux_control_get(dev, NULL);
> +		if (IS_ERR(control))
> +			return PTR_ERR(control);

What about making use of dev_err_probe()?

> +		can_transceiver_phy->mux_ctrl = control;
> +	}
> +
>  	phy = devm_phy_create(dev, dev->of_node,
>  			      &can_transceiver_phy_ops);
>  	if (IS_ERR(phy)) {
> -- 
> 2.17.1
> 
>

Regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3337 bytes --]

On 11.11.2021 22:13: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/phy-can-transceiver.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
> index 6f3fe37dee0e..3d8da5226e27 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_control *mux_ctrl;
>  };
>  
>  /* 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_ctrl) {
> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);

Hard coding the "1" looks wrong here. I have seen some boards where you
can select between a CAN-2.0 and a single wire CAN transceiver with a
mux. So I think we cannot hard code the "1" here.

> +		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_ctrl)
> +		mux_control_deselect(can_transceiver_phy->mux_ctrl);
>  
>  	return 0;
>  }
> @@ -95,6 +107,15 @@ 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")) {

Is this the proper way of doing this? Looks like we need a
devm_mux_control_get_optional(), which doesn't return a -ENODEV if the
device doesn't exist.

Cc'ed Peter Rosin.

> +		struct mux_control *control;
> +
> +		control = devm_mux_control_get(dev, NULL);
> +		if (IS_ERR(control))
> +			return PTR_ERR(control);

What about making use of dev_err_probe()?

> +		can_transceiver_phy->mux_ctrl = control;
> +	}
> +
>  	phy = devm_phy_create(dev, dev->of_node,
>  			      &can_transceiver_phy_ops);
>  	if (IS_ERR(phy)) {
> -- 
> 2.17.1
> 
>

Regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Aswath Govindraju]

Hi Marc,

On 12/11/21 2:10 pm, Marc Kleine-Budde wrote:
> On 11.11.2021 22:13: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/phy-can-transceiver.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
>> index 6f3fe37dee0e..3d8da5226e27 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_control *mux_ctrl;
>>  };
>>  
>>  /* 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_ctrl) {
>> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);
> 
> Hard coding the "1" looks wrong here. I have seen some boards where you
> can select between a CAN-2.0 and a single wire CAN transceiver with a
> mux. So I think we cannot hard code the "1" here.
> 

Yes, as you mentioned it is not ideal to hard code "1". I feel that, it
would be much better to read the state of the mux to be set from the
mux-controls property. The issue that I see with this approach is that
the current implementation in the mux framework only allows for one
argument, which is for indicating the line to be toggled in the mux. If
more arguments are added then an error is returned from the
"mux_control_get". I am not sure why this limitation was added.


>> +		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_ctrl)
>> +		mux_control_deselect(can_transceiver_phy->mux_ctrl);
>>  
>>  	return 0;
>>  }
>> @@ -95,6 +107,15 @@ 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")) {
> 
> Is this the proper way of doing this? Looks like we need a
> devm_mux_control_get_optional(), which doesn't return a -ENODEV if the
> device doesn't exist.
> 
> Cc'ed Peter Rosin.
> 
>> +		struct mux_control *control;
>> +
>> +		control = devm_mux_control_get(dev, NULL);
>> +		if (IS_ERR(control))
>> +			return PTR_ERR(control);
> 
> What about making use of dev_err_probe()?
> 

Sure, I will make this change.

Thank you for the comments.

Regards,
Aswath

>> +		can_transceiver_phy->mux_ctrl = control;
>> +	}
>> +
>>  	phy = devm_phy_create(dev, dev->of_node,
>>  			      &can_transceiver_phy_ops);
>>  	if (IS_ERR(phy)) {
>> -- 
>> 2.17.1
>>
>>
> 
> Regards,
> Marc
> 

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

[Aswath Govindraju]

Hi Marc,

On 12/11/21 2:10 pm, Marc Kleine-Budde wrote:
> On 11.11.2021 22:13: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/phy-can-transceiver.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
>> index 6f3fe37dee0e..3d8da5226e27 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_control *mux_ctrl;
>>  };
>>  
>>  /* 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_ctrl) {
>> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);
> 
> Hard coding the "1" looks wrong here. I have seen some boards where you
> can select between a CAN-2.0 and a single wire CAN transceiver with a
> mux. So I think we cannot hard code the "1" here.
> 

Yes, as you mentioned it is not ideal to hard code "1". I feel that, it
would be much better to read the state of the mux to be set from the
mux-controls property. The issue that I see with this approach is that
the current implementation in the mux framework only allows for one
argument, which is for indicating the line to be toggled in the mux. If
more arguments are added then an error is returned from the
"mux_control_get". I am not sure why this limitation was added.


>> +		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_ctrl)
>> +		mux_control_deselect(can_transceiver_phy->mux_ctrl);
>>  
>>  	return 0;
>>  }
>> @@ -95,6 +107,15 @@ 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")) {
> 
> Is this the proper way of doing this? Looks like we need a
> devm_mux_control_get_optional(), which doesn't return a -ENODEV if the
> device doesn't exist.
> 
> Cc'ed Peter Rosin.
> 
>> +		struct mux_control *control;
>> +
>> +		control = devm_mux_control_get(dev, NULL);
>> +		if (IS_ERR(control))
>> +			return PTR_ERR(control);
> 
> What about making use of dev_err_probe()?
> 

Sure, I will make this change.

Thank you for the comments.

Regards,
Aswath

>> +		can_transceiver_phy->mux_ctrl = control;
>> +	}
>> +
>>  	phy = devm_phy_create(dev, dev->of_node,
>>  			      &can_transceiver_phy_ops);
>>  	if (IS_ERR(phy)) {
>> -- 
>> 2.17.1
>>
>>
> 
> Regards,
> Marc
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Peter Rosin]

Hi!

On 2021-11-12 14:48, Aswath Govindraju wrote:
> Hi Marc,
> 
> On 12/11/21 2:10 pm, Marc Kleine-Budde wrote:
>> On 11.11.2021 22:13: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/phy-can-transceiver.c | 21 +++++++++++++++++++++
>>>  1 file changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
>>> index 6f3fe37dee0e..3d8da5226e27 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_control *mux_ctrl;
>>>  };
>>>  
>>>  /* 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_ctrl) {
>>> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);
>>
>> Hard coding the "1" looks wrong here. I have seen some boards where you
>> can select between a CAN-2.0 and a single wire CAN transceiver with a
>> mux. So I think we cannot hard code the "1" here.
>>
> 
> Yes, as you mentioned it is not ideal to hard code "1". I feel that, it
> would be much better to read the state of the mux to be set from the
> mux-controls property. The issue that I see with this approach is that
> the current implementation in the mux framework only allows for one
> argument, which is for indicating the line to be toggled in the mux. If
> more arguments are added then an error is returned from the
> "mux_control_get". I am not sure why this limitation was added.

The only current use of the first argument is for mux chips that contain
more than one mux control. The limit in the mux core is there since no
mux driver need more than this one argument. The number of mux-control
property arguments is fixed by the #mux-control-cells property in the
mux-control node. I don't see any way to and a new optional mux-control
property argument that specifies a specific state. How would that not
break all existing users?

The current mux interface is designed around the idea that you wrap a
mux control in a mux (lacking better name) application. There are
several such mux applications in the tree, those for I2C, IIO and SPI
pops into my head, and that you then tie the end user consumer to this
muxing application. The mux state comes as a part of how you have tied
the end user consumer to the mux application and is not really something
that the mux-control is involved in.

In other words, a mux-control is not really designed to be used directly
by a driver that needs only one of the states.

However, I'm not saying that doing so isn't also a useful model. It
cetainly sound like it could be. However, the reason it's not done that
way is that I did not want to add muxing code to *all* drivers. I.e. it
would not be flexible to have to add boilerplate mux code to each and
every IIO driver that happen to be connected in a way that a mux has to
be in a certain state for the signal to reach the ADC (or whatever).
Instead, new IIO channels are created for the appropriate mux states
and the IIO mux is connected to the parent IIO channel. When one of the
muxed channels is accessed the mux is selected as needed, and the ADC
driver needs to know nothing about it. If two muxes need to be in a
certain position, you again have no need to "pollute" drivers with
double builerplate mux code. Instead, you simply add two levels of
muxing to the muxed IIO channel.

I think the same is probably true in this case too, and that it would
perhaps be better to create a mux application for phys? But I don't know
what the phy structure looks like, so I'm not in a position to say for
sure if this model fits. But I imagine that phys have providers and
consumers and that a mux can be jammed in there in some way and
intercept some api such that the needed mux state can be selected when
needed.

Cheers,
Peter

> 
>>> +		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_ctrl)
>>> +		mux_control_deselect(can_transceiver_phy->mux_ctrl);
>>>  
>>>  	return 0;
>>>  }
>>> @@ -95,6 +107,15 @@ 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")) {
>>
>> Is this the proper way of doing this? Looks like we need a
>> devm_mux_control_get_optional(), which doesn't return a -ENODEV if the
>> device doesn't exist.
>>
>> Cc'ed Peter Rosin.
>>
>>> +		struct mux_control *control;
>>> +
>>> +		control = devm_mux_control_get(dev, NULL);
>>> +		if (IS_ERR(control))
>>> +			return PTR_ERR(control);
>>
>> What about making use of dev_err_probe()?
>>
> 
> Sure, I will make this change.
> 
> Thank you for the comments.
> 
> Regards,
> Aswath
> 
>>> +		can_transceiver_phy->mux_ctrl = control;
>>> +	}
>>> +
>>>  	phy = devm_phy_create(dev, dev->of_node,
>>>  			      &can_transceiver_phy_ops);
>>>  	if (IS_ERR(phy)) {
>>> -- 
>>> 2.17.1
>>>
>>>
>>
>> Regards,
>> Marc
>>
> 

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

[Peter Rosin]

Hi!

On 2021-11-12 14:48, Aswath Govindraju wrote:
> Hi Marc,
> 
> On 12/11/21 2:10 pm, Marc Kleine-Budde wrote:
>> On 11.11.2021 22:13: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/phy-can-transceiver.c | 21 +++++++++++++++++++++
>>>  1 file changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
>>> index 6f3fe37dee0e..3d8da5226e27 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_control *mux_ctrl;
>>>  };
>>>  
>>>  /* 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_ctrl) {
>>> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);
>>
>> Hard coding the "1" looks wrong here. I have seen some boards where you
>> can select between a CAN-2.0 and a single wire CAN transceiver with a
>> mux. So I think we cannot hard code the "1" here.
>>
> 
> Yes, as you mentioned it is not ideal to hard code "1". I feel that, it
> would be much better to read the state of the mux to be set from the
> mux-controls property. The issue that I see with this approach is that
> the current implementation in the mux framework only allows for one
> argument, which is for indicating the line to be toggled in the mux. If
> more arguments are added then an error is returned from the
> "mux_control_get". I am not sure why this limitation was added.

The only current use of the first argument is for mux chips that contain
more than one mux control. The limit in the mux core is there since no
mux driver need more than this one argument. The number of mux-control
property arguments is fixed by the #mux-control-cells property in the
mux-control node. I don't see any way to and a new optional mux-control
property argument that specifies a specific state. How would that not
break all existing users?

The current mux interface is designed around the idea that you wrap a
mux control in a mux (lacking better name) application. There are
several such mux applications in the tree, those for I2C, IIO and SPI
pops into my head, and that you then tie the end user consumer to this
muxing application. The mux state comes as a part of how you have tied
the end user consumer to the mux application and is not really something
that the mux-control is involved in.

In other words, a mux-control is not really designed to be used directly
by a driver that needs only one of the states.

However, I'm not saying that doing so isn't also a useful model. It
cetainly sound like it could be. However, the reason it's not done that
way is that I did not want to add muxing code to *all* drivers. I.e. it
would not be flexible to have to add boilerplate mux code to each and
every IIO driver that happen to be connected in a way that a mux has to
be in a certain state for the signal to reach the ADC (or whatever).
Instead, new IIO channels are created for the appropriate mux states
and the IIO mux is connected to the parent IIO channel. When one of the
muxed channels is accessed the mux is selected as needed, and the ADC
driver needs to know nothing about it. If two muxes need to be in a
certain position, you again have no need to "pollute" drivers with
double builerplate mux code. Instead, you simply add two levels of
muxing to the muxed IIO channel.

I think the same is probably true in this case too, and that it would
perhaps be better to create a mux application for phys? But I don't know
what the phy structure looks like, so I'm not in a position to say for
sure if this model fits. But I imagine that phys have providers and
consumers and that a mux can be jammed in there in some way and
intercept some api such that the needed mux state can be selected when
needed.

Cheers,
Peter

> 
>>> +		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_ctrl)
>>> +		mux_control_deselect(can_transceiver_phy->mux_ctrl);
>>>  
>>>  	return 0;
>>>  }
>>> @@ -95,6 +107,15 @@ 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")) {
>>
>> Is this the proper way of doing this? Looks like we need a
>> devm_mux_control_get_optional(), which doesn't return a -ENODEV if the
>> device doesn't exist.
>>
>> Cc'ed Peter Rosin.
>>
>>> +		struct mux_control *control;
>>> +
>>> +		control = devm_mux_control_get(dev, NULL);
>>> +		if (IS_ERR(control))
>>> +			return PTR_ERR(control);
>>
>> What about making use of dev_err_probe()?
>>
> 
> Sure, I will make this change.
> 
> Thank you for the comments.
> 
> Regards,
> Aswath
> 
>>> +		can_transceiver_phy->mux_ctrl = control;
>>> +	}
>>> +
>>>  	phy = devm_phy_create(dev, dev->of_node,
>>>  			      &can_transceiver_phy_ops);
>>>  	if (IS_ERR(phy)) {
>>> -- 
>>> 2.17.1
>>>
>>>
>>
>> Regards,
>> Marc
>>
> 

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Aswath Govindraju]

Hi Peter,

On 13/11/21 12:45 am, Peter Rosin wrote:
> Hi!
> 
> On 2021-11-12 14:48, Aswath Govindraju wrote:
>> Hi Marc,
>>
>> On 12/11/21 2:10 pm, Marc Kleine-Budde wrote:
>>> On 11.11.2021 22:13: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/phy-can-transceiver.c | 21 +++++++++++++++++++++
>>>>  1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
>>>> index 6f3fe37dee0e..3d8da5226e27 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_control *mux_ctrl;
>>>>  };
>>>>  
>>>>  /* 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_ctrl) {
>>>> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);
>>>
>>> Hard coding the "1" looks wrong here. I have seen some boards where you
>>> can select between a CAN-2.0 and a single wire CAN transceiver with a
>>> mux. So I think we cannot hard code the "1" here.
>>>
>>
>> Yes, as you mentioned it is not ideal to hard code "1". I feel that, it
>> would be much better to read the state of the mux to be set from the
>> mux-controls property. The issue that I see with this approach is that
>> the current implementation in the mux framework only allows for one
>> argument, which is for indicating the line to be toggled in the mux. If
>> more arguments are added then an error is returned from the
>> "mux_control_get". I am not sure why this limitation was added.
> 
> The only current use of the first argument is for mux chips that contain
> more than one mux control. The limit in the mux core is there since no
> mux driver need more than this one argument. The number of mux-control
> property arguments is fixed by the #mux-control-cells property in the
> mux-control node. I don't see any way to and a new optional mux-control
> property argument that specifies a specific state. How would that not
> break all existing users?
> 

My idea was to use the second argument for reading the state of mux to
be set after increasing the #mux-control-cells value to 2. I don't think
this will break the existing mux controller users as the second argument
was not used till now, would be equivalent to adding an additional feature.

One more question that I had is, if the number of arguments match the
#mux-control-cells and if the number of arguments are greater than 1 why
is an error being returned?

> The current mux interface is designed around the idea that you wrap a
> mux control in a mux (lacking better name) application. There are
> several such mux applications in the tree, those for I2C, IIO and SPI
> pops into my head, and that you then tie the end user consumer to this
> muxing application. The mux state comes as a part of how you have tied
> the end user consumer to the mux application and is not really something
> that the mux-control is involved in.
> 
> In other words, a mux-control is not really designed to be used directly
> by a driver that needs only one of the states.
> 
> However, I'm not saying that doing so isn't also a useful model. It
> cetainly sound like it could be. However, the reason it's not done that
> way is that I did not want to add muxing code to *all* drivers. I.e. it
> would not be flexible to have to add boilerplate mux code to each and
> every IIO driver that happen to be connected in a way that a mux has to
> be in a certain state for the signal to reach the ADC (or whatever).
> Instead, new IIO channels are created for the appropriate mux states
> and the IIO mux is connected to the parent IIO channel. When one of the
> muxed channels is accessed the mux is selected as needed, and the ADC
> driver needs to know nothing about it. If two muxes need to be in a
> certain position, you again have no need to "pollute" drivers with
> double builerplate mux code. Instead, you simply add two levels of
> muxing to the muxed IIO channel.
> 
> I think the same is probably true in this case too, and that it would
> perhaps be better to create a mux application for phys? But I don't know
> what the phy structure looks like, so I'm not in a position to say for
> sure if this model fits. But I imagine that phys have providers and
> consumers and that a mux can be jammed in there in some way and
> intercept some api such that the needed mux state can be selected when
> needed.
> 

Yes, I understand that reading the state of the mux in drivers would not
be efficient as it would adding the boiler plate code in each of the
drivers. However, for phys as each of them can be used for a different
interface, I am not sure if a common mux phy wrapper can be introduced.
This is reason why I felt that drivers should be allowed to read the
state of the mux directly, when no mux wrapper application is suitable
for it.

Thanks,
Aswath

> Cheers,
> Peter
> 
>>
>>>> +		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_ctrl)
>>>> +		mux_control_deselect(can_transceiver_phy->mux_ctrl);
>>>>  
>>>>  	return 0;
>>>>  }
>>>> @@ -95,6 +107,15 @@ 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")) {
>>>
>>> Is this the proper way of doing this? Looks like we need a
>>> devm_mux_control_get_optional(), which doesn't return a -ENODEV if the
>>> device doesn't exist.
>>>
>>> Cc'ed Peter Rosin.
>>>
>>>> +		struct mux_control *control;
>>>> +
>>>> +		control = devm_mux_control_get(dev, NULL);
>>>> +		if (IS_ERR(control))
>>>> +			return PTR_ERR(control);
>>>
>>> What about making use of dev_err_probe()?
>>>
>>
>> Sure, I will make this change.
>>
>> Thank you for the comments.
>>
>> Regards,
>> Aswath
>>
>>>> +		can_transceiver_phy->mux_ctrl = control;
>>>> +	}
>>>> +
>>>>  	phy = devm_phy_create(dev, dev->of_node,
>>>>  			      &can_transceiver_phy_ops);
>>>>  	if (IS_ERR(phy)) {
>>>> -- 
>>>> 2.17.1
>>>>
>>>>
>>>
>>> Regards,
>>> Marc
>>>
>>


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Aswath Govindraju]

Hi Peter,

On 13/11/21 12:45 am, Peter Rosin wrote:
> Hi!
> 
> On 2021-11-12 14:48, Aswath Govindraju wrote:
>> Hi Marc,
>>
>> On 12/11/21 2:10 pm, Marc Kleine-Budde wrote:
>>> On 11.11.2021 22:13: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/phy-can-transceiver.c | 21 +++++++++++++++++++++
>>>>  1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
>>>> index 6f3fe37dee0e..3d8da5226e27 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_control *mux_ctrl;
>>>>  };
>>>>  
>>>>  /* 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_ctrl) {
>>>> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);
>>>
>>> Hard coding the "1" looks wrong here. I have seen some boards where you
>>> can select between a CAN-2.0 and a single wire CAN transceiver with a
>>> mux. So I think we cannot hard code the "1" here.
>>>
>>
>> Yes, as you mentioned it is not ideal to hard code "1". I feel that, it
>> would be much better to read the state of the mux to be set from the
>> mux-controls property. The issue that I see with this approach is that
>> the current implementation in the mux framework only allows for one
>> argument, which is for indicating the line to be toggled in the mux. If
>> more arguments are added then an error is returned from the
>> "mux_control_get". I am not sure why this limitation was added.
> 
> The only current use of the first argument is for mux chips that contain
> more than one mux control. The limit in the mux core is there since no
> mux driver need more than this one argument. The number of mux-control
> property arguments is fixed by the #mux-control-cells property in the
> mux-control node. I don't see any way to and a new optional mux-control
> property argument that specifies a specific state. How would that not
> break all existing users?
> 

My idea was to use the second argument for reading the state of mux to
be set after increasing the #mux-control-cells value to 2. I don't think
this will break the existing mux controller users as the second argument
was not used till now, would be equivalent to adding an additional feature.

One more question that I had is, if the number of arguments match the
#mux-control-cells and if the number of arguments are greater than 1 why
is an error being returned?

> The current mux interface is designed around the idea that you wrap a
> mux control in a mux (lacking better name) application. There are
> several such mux applications in the tree, those for I2C, IIO and SPI
> pops into my head, and that you then tie the end user consumer to this
> muxing application. The mux state comes as a part of how you have tied
> the end user consumer to the mux application and is not really something
> that the mux-control is involved in.
> 
> In other words, a mux-control is not really designed to be used directly
> by a driver that needs only one of the states.
> 
> However, I'm not saying that doing so isn't also a useful model. It
> cetainly sound like it could be. However, the reason it's not done that
> way is that I did not want to add muxing code to *all* drivers. I.e. it
> would not be flexible to have to add boilerplate mux code to each and
> every IIO driver that happen to be connected in a way that a mux has to
> be in a certain state for the signal to reach the ADC (or whatever).
> Instead, new IIO channels are created for the appropriate mux states
> and the IIO mux is connected to the parent IIO channel. When one of the
> muxed channels is accessed the mux is selected as needed, and the ADC
> driver needs to know nothing about it. If two muxes need to be in a
> certain position, you again have no need to "pollute" drivers with
> double builerplate mux code. Instead, you simply add two levels of
> muxing to the muxed IIO channel.
> 
> I think the same is probably true in this case too, and that it would
> perhaps be better to create a mux application for phys? But I don't know
> what the phy structure looks like, so I'm not in a position to say for
> sure if this model fits. But I imagine that phys have providers and
> consumers and that a mux can be jammed in there in some way and
> intercept some api such that the needed mux state can be selected when
> needed.
> 

Yes, I understand that reading the state of the mux in drivers would not
be efficient as it would adding the boiler plate code in each of the
drivers. However, for phys as each of them can be used for a different
interface, I am not sure if a common mux phy wrapper can be introduced.
This is reason why I felt that drivers should be allowed to read the
state of the mux directly, when no mux wrapper application is suitable
for it.

Thanks,
Aswath

> Cheers,
> Peter
> 
>>
>>>> +		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_ctrl)
>>>> +		mux_control_deselect(can_transceiver_phy->mux_ctrl);
>>>>  
>>>>  	return 0;
>>>>  }
>>>> @@ -95,6 +107,15 @@ 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")) {
>>>
>>> Is this the proper way of doing this? Looks like we need a
>>> devm_mux_control_get_optional(), which doesn't return a -ENODEV if the
>>> device doesn't exist.
>>>
>>> Cc'ed Peter Rosin.
>>>
>>>> +		struct mux_control *control;
>>>> +
>>>> +		control = devm_mux_control_get(dev, NULL);
>>>> +		if (IS_ERR(control))
>>>> +			return PTR_ERR(control);
>>>
>>> What about making use of dev_err_probe()?
>>>
>>
>> Sure, I will make this change.
>>
>> Thank you for the comments.
>>
>> Regards,
>> Aswath
>>
>>>> +		can_transceiver_phy->mux_ctrl = control;
>>>> +	}
>>>> +
>>>>  	phy = devm_phy_create(dev, dev->of_node,
>>>>  			      &can_transceiver_phy_ops);
>>>>  	if (IS_ERR(phy)) {
>>>> -- 
>>>> 2.17.1
>>>>
>>>>
>>>
>>> Regards,
>>> Marc
>>>
>>

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

[Peter Rosin]

Hi!

On 2021-11-15 07:31, Aswath Govindraju wrote:
> Hi Peter,
> 
> On 13/11/21 12:45 am, Peter Rosin wrote:
>> Hi!
>>
>> On 2021-11-12 14:48, Aswath Govindraju wrote:
>>> Hi Marc,
>>>
>>> On 12/11/21 2:10 pm, Marc Kleine-Budde wrote:
>>>> On 11.11.2021 22:13: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/phy-can-transceiver.c | 21 +++++++++++++++++++++
>>>>>  1 file changed, 21 insertions(+)
>>>>>
>>>>> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
>>>>> index 6f3fe37dee0e..3d8da5226e27 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_control *mux_ctrl;
>>>>>  };
>>>>>  
>>>>>  /* 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_ctrl) {
>>>>> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);
>>>>
>>>> Hard coding the "1" looks wrong here. I have seen some boards where you
>>>> can select between a CAN-2.0 and a single wire CAN transceiver with a
>>>> mux. So I think we cannot hard code the "1" here.
>>>>
>>>
>>> Yes, as you mentioned it is not ideal to hard code "1". I feel that, it
>>> would be much better to read the state of the mux to be set from the
>>> mux-controls property. The issue that I see with this approach is that
>>> the current implementation in the mux framework only allows for one
>>> argument, which is for indicating the line to be toggled in the mux. If
>>> more arguments are added then an error is returned from the
>>> "mux_control_get". I am not sure why this limitation was added.
>>
>> The only current use of the first argument is for mux chips that contain
>> more than one mux control. The limit in the mux core is there since no
>> mux driver need more than this one argument. The number of mux-control
>> property arguments is fixed by the #mux-control-cells property in the
>> mux-control node. I don't see any way to and a new optional mux-control
>> property argument that specifies a specific state. How would that not
>> break all existing users?
>>
> 
> My idea was to use the second argument for reading the state of mux to
> be set after increasing the #mux-control-cells value to 2. I don't think
> this will break the existing mux controller users as the second argument
> was not used till now, would be equivalent to adding an additional feature.

Ok, I see what you mean now, sorry for being dense. If we allow this then
there is a need to add a special value that means all/many states (such as
-1 or something such) so that a mux-control can be used simultaneously by
drivers "pointing at" a specific state like you want to do, and by the
existing "application" style drivers that wraps the whole mux control.

I.e. something like this

	mux: mux {
		compatible = "mux-gpio";
		...

		#mux-control-cells = <1>; /* one more than previously */
	};

	phy {
		...

		mux-control = <&mux 3>; /* point to specific state */
	};

	i2c-mux {
		compatible = "i2c-mux-gpmux";
		parent = <&i2c0>
		mux-control = <&mux (-1)>; /* many states needed */

		...

		i2c@1 {
			eeprom@50 {
				...
			};
		};

		i2c@2 {
			...
		};
	};

Yes, I realize that accesses to the eeprom cannot happen if the mux is
constantly selected and locked in state 3 by the phy, and that a mux with
one channel being a phy and other channels being I2C might not be
realistic, but the same gpio lines might control several muxes that are
used for separate signals solving at least the latter "problem" with this
completely made up example. Anyway, the above is in principle, and HW
designs are sometimes too weird for words.

> One more question that I had is, if the number of arguments match the
> #mux-control-cells and if the number of arguments are greater than 1 why
> is an error being returned?

Changing that would require a bindings update anyway, so I simply
disallowed it as an error. Not much thought went into the decision,
as it couldn't be wrong to do what is being done with the bindings
that exist. That said, I have no problem lifting this restriction,
if there's a matching bindings update that makes it all fit.

>> The current mux interface is designed around the idea that you wrap a
>> mux control in a mux (lacking better name) application. There are
>> several such mux applications in the tree, those for I2C, IIO and SPI
>> pops into my head, and that you then tie the end user consumer to this
>> muxing application. The mux state comes as a part of how you have tied
>> the end user consumer to the mux application and is not really something
>> that the mux-control is involved in.
>>
>> In other words, a mux-control is not really designed to be used directly
>> by a driver that needs only one of the states.
>>
>> However, I'm not saying that doing so isn't also a useful model. It
>> cetainly sound like it could be. However, the reason it's not done that
>> way is that I did not want to add muxing code to *all* drivers. I.e. it
>> would not be flexible to have to add boilerplate mux code to each and
>> every IIO driver that happen to be connected in a way that a mux has to
>> be in a certain state for the signal to reach the ADC (or whatever).
>> Instead, new IIO channels are created for the appropriate mux states
>> and the IIO mux is connected to the parent IIO channel. When one of the
>> muxed channels is accessed the mux is selected as needed, and the ADC
>> driver needs to know nothing about it. If two muxes need to be in a
>> certain position, you again have no need to "pollute" drivers with
>> double builerplate mux code. Instead, you simply add two levels of
>> muxing to the muxed IIO channel.
>>
>> I think the same is probably true in this case too, and that it would
>> perhaps be better to create a mux application for phys? But I don't know
>> what the phy structure looks like, so I'm not in a position to say for
>> sure if this model fits. But I imagine that phys have providers and
>> consumers and that a mux can be jammed in there in some way and
>> intercept some api such that the needed mux state can be selected when
>> needed.
>>
> 
> Yes, I understand that reading the state of the mux in drivers would not
> be efficient as it would adding the boiler plate code in each of the
> drivers. However, for phys as each of them can be used for a different
> interface, I am not sure if a common mux phy wrapper can be introduced.
> This is reason why I felt that drivers should be allowed to read the
> state of the mux directly, when no mux wrapper application is suitable
> for it.

It need not be one grand unifying phy mux, it could be one for each
kind of phy interface. But again, I don't know much about how phys
work nor their interfaces, not event roughly how many drivers there
are etc etc. I have simply never needed to look.

Hmm, wild idea, maybe there could be a mux "application" for pinctrl?
I mean such that you could tie pinctrl states to mux states. It doesn't
sound like too bad of a match to me?

Cheers,
Peter

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

[Peter Rosin]

Hi!

On 2021-11-15 07:31, Aswath Govindraju wrote:
> Hi Peter,
> 
> On 13/11/21 12:45 am, Peter Rosin wrote:
>> Hi!
>>
>> On 2021-11-12 14:48, Aswath Govindraju wrote:
>>> Hi Marc,
>>>
>>> On 12/11/21 2:10 pm, Marc Kleine-Budde wrote:
>>>> On 11.11.2021 22:13: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/phy-can-transceiver.c | 21 +++++++++++++++++++++
>>>>>  1 file changed, 21 insertions(+)
>>>>>
>>>>> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
>>>>> index 6f3fe37dee0e..3d8da5226e27 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_control *mux_ctrl;
>>>>>  };
>>>>>  
>>>>>  /* 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_ctrl) {
>>>>> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);
>>>>
>>>> Hard coding the "1" looks wrong here. I have seen some boards where you
>>>> can select between a CAN-2.0 and a single wire CAN transceiver with a
>>>> mux. So I think we cannot hard code the "1" here.
>>>>
>>>
>>> Yes, as you mentioned it is not ideal to hard code "1". I feel that, it
>>> would be much better to read the state of the mux to be set from the
>>> mux-controls property. The issue that I see with this approach is that
>>> the current implementation in the mux framework only allows for one
>>> argument, which is for indicating the line to be toggled in the mux. If
>>> more arguments are added then an error is returned from the
>>> "mux_control_get". I am not sure why this limitation was added.
>>
>> The only current use of the first argument is for mux chips that contain
>> more than one mux control. The limit in the mux core is there since no
>> mux driver need more than this one argument. The number of mux-control
>> property arguments is fixed by the #mux-control-cells property in the
>> mux-control node. I don't see any way to and a new optional mux-control
>> property argument that specifies a specific state. How would that not
>> break all existing users?
>>
> 
> My idea was to use the second argument for reading the state of mux to
> be set after increasing the #mux-control-cells value to 2. I don't think
> this will break the existing mux controller users as the second argument
> was not used till now, would be equivalent to adding an additional feature.

Ok, I see what you mean now, sorry for being dense. If we allow this then
there is a need to add a special value that means all/many states (such as
-1 or something such) so that a mux-control can be used simultaneously by
drivers "pointing at" a specific state like you want to do, and by the
existing "application" style drivers that wraps the whole mux control.

I.e. something like this

	mux: mux {
		compatible = "mux-gpio";
		...

		#mux-control-cells = <1>; /* one more than previously */
	};

	phy {
		...

		mux-control = <&mux 3>; /* point to specific state */
	};

	i2c-mux {
		compatible = "i2c-mux-gpmux";
		parent = <&i2c0>
		mux-control = <&mux (-1)>; /* many states needed */

		...

		i2c@1 {
			eeprom@50 {
				...
			};
		};

		i2c@2 {
			...
		};
	};

Yes, I realize that accesses to the eeprom cannot happen if the mux is
constantly selected and locked in state 3 by the phy, and that a mux with
one channel being a phy and other channels being I2C might not be
realistic, but the same gpio lines might control several muxes that are
used for separate signals solving at least the latter "problem" with this
completely made up example. Anyway, the above is in principle, and HW
designs are sometimes too weird for words.

> One more question that I had is, if the number of arguments match the
> #mux-control-cells and if the number of arguments are greater than 1 why
> is an error being returned?

Changing that would require a bindings update anyway, so I simply
disallowed it as an error. Not much thought went into the decision,
as it couldn't be wrong to do what is being done with the bindings
that exist. That said, I have no problem lifting this restriction,
if there's a matching bindings update that makes it all fit.

>> The current mux interface is designed around the idea that you wrap a
>> mux control in a mux (lacking better name) application. There are
>> several such mux applications in the tree, those for I2C, IIO and SPI
>> pops into my head, and that you then tie the end user consumer to this
>> muxing application. The mux state comes as a part of how you have tied
>> the end user consumer to the mux application and is not really something
>> that the mux-control is involved in.
>>
>> In other words, a mux-control is not really designed to be used directly
>> by a driver that needs only one of the states.
>>
>> However, I'm not saying that doing so isn't also a useful model. It
>> cetainly sound like it could be. However, the reason it's not done that
>> way is that I did not want to add muxing code to *all* drivers. I.e. it
>> would not be flexible to have to add boilerplate mux code to each and
>> every IIO driver that happen to be connected in a way that a mux has to
>> be in a certain state for the signal to reach the ADC (or whatever).
>> Instead, new IIO channels are created for the appropriate mux states
>> and the IIO mux is connected to the parent IIO channel. When one of the
>> muxed channels is accessed the mux is selected as needed, and the ADC
>> driver needs to know nothing about it. If two muxes need to be in a
>> certain position, you again have no need to "pollute" drivers with
>> double builerplate mux code. Instead, you simply add two levels of
>> muxing to the muxed IIO channel.
>>
>> I think the same is probably true in this case too, and that it would
>> perhaps be better to create a mux application for phys? But I don't know
>> what the phy structure looks like, so I'm not in a position to say for
>> sure if this model fits. But I imagine that phys have providers and
>> consumers and that a mux can be jammed in there in some way and
>> intercept some api such that the needed mux state can be selected when
>> needed.
>>
> 
> Yes, I understand that reading the state of the mux in drivers would not
> be efficient as it would adding the boiler plate code in each of the
> drivers. However, for phys as each of them can be used for a different
> interface, I am not sure if a common mux phy wrapper can be introduced.
> This is reason why I felt that drivers should be allowed to read the
> state of the mux directly, when no mux wrapper application is suitable
> for it.

It need not be one grand unifying phy mux, it could be one for each
kind of phy interface. But again, I don't know much about how phys
work nor their interfaces, not event roughly how many drivers there
are etc etc. I have simply never needed to look.

Hmm, wild idea, maybe there could be a mux "application" for pinctrl?
I mean such that you could tie pinctrl states to mux states. It doesn't
sound like too bad of a match to me?

Cheers,
Peter

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Aswath Govindraju]

Hi Peter,

On 18/11/21 2:54 am, Peter Rosin wrote:
> Hi!
> 
> On 2021-11-15 07:31, Aswath Govindraju wrote:
>> Hi Peter,
>>
>> On 13/11/21 12:45 am, Peter Rosin wrote:
>>> Hi!
>>>
>>> On 2021-11-12 14:48, Aswath Govindraju wrote:
>>>> Hi Marc,
>>>>
>>>> On 12/11/21 2:10 pm, Marc Kleine-Budde wrote:
>>>>> On 11.11.2021 22:13: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/phy-can-transceiver.c | 21 +++++++++++++++++++++
>>>>>>  1 file changed, 21 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
>>>>>> index 6f3fe37dee0e..3d8da5226e27 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_control *mux_ctrl;
>>>>>>  };
>>>>>>  
>>>>>>  /* 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_ctrl) {
>>>>>> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);
>>>>>
>>>>> Hard coding the "1" looks wrong here. I have seen some boards where you
>>>>> can select between a CAN-2.0 and a single wire CAN transceiver with a
>>>>> mux. So I think we cannot hard code the "1" here.
>>>>>
>>>>
>>>> Yes, as you mentioned it is not ideal to hard code "1". I feel that, it
>>>> would be much better to read the state of the mux to be set from the
>>>> mux-controls property. The issue that I see with this approach is that
>>>> the current implementation in the mux framework only allows for one
>>>> argument, which is for indicating the line to be toggled in the mux. If
>>>> more arguments are added then an error is returned from the
>>>> "mux_control_get". I am not sure why this limitation was added.
>>>
>>> The only current use of the first argument is for mux chips that contain
>>> more than one mux control. The limit in the mux core is there since no
>>> mux driver need more than this one argument. The number of mux-control
>>> property arguments is fixed by the #mux-control-cells property in the
>>> mux-control node. I don't see any way to and a new optional mux-control
>>> property argument that specifies a specific state. How would that not
>>> break all existing users?
>>>
>>
>> My idea was to use the second argument for reading the state of mux to
>> be set after increasing the #mux-control-cells value to 2. I don't think
>> this will break the existing mux controller users as the second argument
>> was not used till now, would be equivalent to adding an additional feature.
> 
> Ok, I see what you mean now, sorry for being dense. If we allow this then
> there is a need to add a special value that means all/many states (such as
> -1 or something such) so that a mux-control can be used simultaneously by
> drivers "pointing at" a specific state like you want to do, and by the
> existing "application" style drivers that wraps the whole mux control.
> 
> I.e. something like this
> 
> 	mux: mux {
> 		compatible = "mux-gpio";
> 		...
> 
> 		#mux-control-cells = <1>; /* one more than previously */
> 	};
> 
> 	phy {
> 		...
> 
> 		mux-control = <&mux 3>; /* point to specific state */
> 	};
> 
> 	i2c-mux {
> 		compatible = "i2c-mux-gpmux";
> 		parent = <&i2c0>
> 		mux-control = <&mux (-1)>; /* many states needed */
> 
> 		...
> 
> 		i2c@1 {
> 			eeprom@50 {
> 				...
> 			};
> 		};
> 
> 		i2c@2 {
> 			...
> 		};
> 	};
> 
> Yes, I realize that accesses to the eeprom cannot happen if the mux is
> constantly selected and locked in state 3 by the phy, and that a mux with
> one channel being a phy and other channels being I2C might not be
> realistic, but the same gpio lines might control several muxes that are
> used for separate signals solving at least the latter "problem" with this
> completely made up example. Anyway, the above is in principle, and HW
> designs are sometimes too weird for words.
> 

This is almost exactly what I was intending to implement except for one
more change. The state of the mux will always be represented using the
second argument(i.e. #mux-control-cells = <2>).

For example,
mux-controls = <&mux 0 1>, <&mux 1 0>;


With this I think we wouldn't need a special value for all or many states.

>> One more question that I had is, if the number of arguments match the
>> #mux-control-cells and if the number of arguments are greater than 1 why
>> is an error being returned?
> 
> Changing that would require a bindings update anyway, so I simply
> disallowed it as an error. Not much thought went into the decision,
> as it couldn't be wrong to do what is being done with the bindings
> that exist. That said, I have no problem lifting this restriction,
> if there's a matching bindings update that makes it all fit.
> 

Sure, I think making a change in

Documentation/devicetree/bindings/mux/gpio-mux.yaml, should be good
enough I assume.


Thank you for the comments. I'll post a respin of this series, with the
above changes.

Thanks,
Aswath

>>> The current mux interface is designed around the idea that you wrap a
>>> mux control in a mux (lacking better name) application. There are
>>> several such mux applications in the tree, those for I2C, IIO and SPI
>>> pops into my head, and that you then tie the end user consumer to this
>>> muxing application. The mux state comes as a part of how you have tied
>>> the end user consumer to the mux application and is not really something
>>> that the mux-control is involved in.
>>>
>>> In other words, a mux-control is not really designed to be used directly
>>> by a driver that needs only one of the states.
>>>
>>> However, I'm not saying that doing so isn't also a useful model. It
>>> cetainly sound like it could be. However, the reason it's not done that
>>> way is that I did not want to add muxing code to *all* drivers. I.e. it
>>> would not be flexible to have to add boilerplate mux code to each and
>>> every IIO driver that happen to be connected in a way that a mux has to
>>> be in a certain state for the signal to reach the ADC (or whatever).
>>> Instead, new IIO channels are created for the appropriate mux states
>>> and the IIO mux is connected to the parent IIO channel. When one of the
>>> muxed channels is accessed the mux is selected as needed, and the ADC
>>> driver needs to know nothing about it. If two muxes need to be in a
>>> certain position, you again have no need to "pollute" drivers with
>>> double builerplate mux code. Instead, you simply add two levels of
>>> muxing to the muxed IIO channel.
>>>
>>> I think the same is probably true in this case too, and that it would
>>> perhaps be better to create a mux application for phys? But I don't know
>>> what the phy structure looks like, so I'm not in a position to say for
>>> sure if this model fits. But I imagine that phys have providers and
>>> consumers and that a mux can be jammed in there in some way and
>>> intercept some api such that the needed mux state can be selected when
>>> needed.
>>>
>>
>> Yes, I understand that reading the state of the mux in drivers would not
>> be efficient as it would adding the boiler plate code in each of the
>> drivers. However, for phys as each of them can be used for a different
>> interface, I am not sure if a common mux phy wrapper can be introduced.
>> This is reason why I felt that drivers should be allowed to read the
>> state of the mux directly, when no mux wrapper application is suitable
>> for it.
> 
> It need not be one grand unifying phy mux, it could be one for each
> kind of phy interface. But again, I don't know much about how phys
> work nor their interfaces, not event roughly how many drivers there
> are etc etc. I have simply never needed to look.
> 
> Hmm, wild idea, maybe there could be a mux "application" for pinctrl?
> I mean such that you could tie pinctrl states to mux states. It doesn't
> sound like too bad of a match to me?
> 
> Cheers,
> Peter
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Aswath Govindraju]

Hi Peter,

On 18/11/21 2:54 am, Peter Rosin wrote:
> Hi!
> 
> On 2021-11-15 07:31, Aswath Govindraju wrote:
>> Hi Peter,
>>
>> On 13/11/21 12:45 am, Peter Rosin wrote:
>>> Hi!
>>>
>>> On 2021-11-12 14:48, Aswath Govindraju wrote:
>>>> Hi Marc,
>>>>
>>>> On 12/11/21 2:10 pm, Marc Kleine-Budde wrote:
>>>>> On 11.11.2021 22:13: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/phy-can-transceiver.c | 21 +++++++++++++++++++++
>>>>>>  1 file changed, 21 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
>>>>>> index 6f3fe37dee0e..3d8da5226e27 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_control *mux_ctrl;
>>>>>>  };
>>>>>>  
>>>>>>  /* 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_ctrl) {
>>>>>> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);
>>>>>
>>>>> Hard coding the "1" looks wrong here. I have seen some boards where you
>>>>> can select between a CAN-2.0 and a single wire CAN transceiver with a
>>>>> mux. So I think we cannot hard code the "1" here.
>>>>>
>>>>
>>>> Yes, as you mentioned it is not ideal to hard code "1". I feel that, it
>>>> would be much better to read the state of the mux to be set from the
>>>> mux-controls property. The issue that I see with this approach is that
>>>> the current implementation in the mux framework only allows for one
>>>> argument, which is for indicating the line to be toggled in the mux. If
>>>> more arguments are added then an error is returned from the
>>>> "mux_control_get". I am not sure why this limitation was added.
>>>
>>> The only current use of the first argument is for mux chips that contain
>>> more than one mux control. The limit in the mux core is there since no
>>> mux driver need more than this one argument. The number of mux-control
>>> property arguments is fixed by the #mux-control-cells property in the
>>> mux-control node. I don't see any way to and a new optional mux-control
>>> property argument that specifies a specific state. How would that not
>>> break all existing users?
>>>
>>
>> My idea was to use the second argument for reading the state of mux to
>> be set after increasing the #mux-control-cells value to 2. I don't think
>> this will break the existing mux controller users as the second argument
>> was not used till now, would be equivalent to adding an additional feature.
> 
> Ok, I see what you mean now, sorry for being dense. If we allow this then
> there is a need to add a special value that means all/many states (such as
> -1 or something such) so that a mux-control can be used simultaneously by
> drivers "pointing at" a specific state like you want to do, and by the
> existing "application" style drivers that wraps the whole mux control.
> 
> I.e. something like this
> 
> 	mux: mux {
> 		compatible = "mux-gpio";
> 		...
> 
> 		#mux-control-cells = <1>; /* one more than previously */
> 	};
> 
> 	phy {
> 		...
> 
> 		mux-control = <&mux 3>; /* point to specific state */
> 	};
> 
> 	i2c-mux {
> 		compatible = "i2c-mux-gpmux";
> 		parent = <&i2c0>
> 		mux-control = <&mux (-1)>; /* many states needed */
> 
> 		...
> 
> 		i2c@1 {
> 			eeprom@50 {
> 				...
> 			};
> 		};
> 
> 		i2c@2 {
> 			...
> 		};
> 	};
> 
> Yes, I realize that accesses to the eeprom cannot happen if the mux is
> constantly selected and locked in state 3 by the phy, and that a mux with
> one channel being a phy and other channels being I2C might not be
> realistic, but the same gpio lines might control several muxes that are
> used for separate signals solving at least the latter "problem" with this
> completely made up example. Anyway, the above is in principle, and HW
> designs are sometimes too weird for words.
> 

This is almost exactly what I was intending to implement except for one
more change. The state of the mux will always be represented using the
second argument(i.e. #mux-control-cells = <2>).

For example,
mux-controls = <&mux 0 1>, <&mux 1 0>;


With this I think we wouldn't need a special value for all or many states.

>> One more question that I had is, if the number of arguments match the
>> #mux-control-cells and if the number of arguments are greater than 1 why
>> is an error being returned?
> 
> Changing that would require a bindings update anyway, so I simply
> disallowed it as an error. Not much thought went into the decision,
> as it couldn't be wrong to do what is being done with the bindings
> that exist. That said, I have no problem lifting this restriction,
> if there's a matching bindings update that makes it all fit.
> 

Sure, I think making a change in

Documentation/devicetree/bindings/mux/gpio-mux.yaml, should be good
enough I assume.


Thank you for the comments. I'll post a respin of this series, with the
above changes.

Thanks,
Aswath

>>> The current mux interface is designed around the idea that you wrap a
>>> mux control in a mux (lacking better name) application. There are
>>> several such mux applications in the tree, those for I2C, IIO and SPI
>>> pops into my head, and that you then tie the end user consumer to this
>>> muxing application. The mux state comes as a part of how you have tied
>>> the end user consumer to the mux application and is not really something
>>> that the mux-control is involved in.
>>>
>>> In other words, a mux-control is not really designed to be used directly
>>> by a driver that needs only one of the states.
>>>
>>> However, I'm not saying that doing so isn't also a useful model. It
>>> cetainly sound like it could be. However, the reason it's not done that
>>> way is that I did not want to add muxing code to *all* drivers. I.e. it
>>> would not be flexible to have to add boilerplate mux code to each and
>>> every IIO driver that happen to be connected in a way that a mux has to
>>> be in a certain state for the signal to reach the ADC (or whatever).
>>> Instead, new IIO channels are created for the appropriate mux states
>>> and the IIO mux is connected to the parent IIO channel. When one of the
>>> muxed channels is accessed the mux is selected as needed, and the ADC
>>> driver needs to know nothing about it. If two muxes need to be in a
>>> certain position, you again have no need to "pollute" drivers with
>>> double builerplate mux code. Instead, you simply add two levels of
>>> muxing to the muxed IIO channel.
>>>
>>> I think the same is probably true in this case too, and that it would
>>> perhaps be better to create a mux application for phys? But I don't know
>>> what the phy structure looks like, so I'm not in a position to say for
>>> sure if this model fits. But I imagine that phys have providers and
>>> consumers and that a mux can be jammed in there in some way and
>>> intercept some api such that the needed mux state can be selected when
>>> needed.
>>>
>>
>> Yes, I understand that reading the state of the mux in drivers would not
>> be efficient as it would adding the boiler plate code in each of the
>> drivers. However, for phys as each of them can be used for a different
>> interface, I am not sure if a common mux phy wrapper can be introduced.
>> This is reason why I felt that drivers should be allowed to read the
>> state of the mux directly, when no mux wrapper application is suitable
>> for it.
> 
> It need not be one grand unifying phy mux, it could be one for each
> kind of phy interface. But again, I don't know much about how phys
> work nor their interfaces, not event roughly how many drivers there
> are etc etc. I have simply never needed to look.
> 
> Hmm, wild idea, maybe there could be a mux "application" for pinctrl?
> I mean such that you could tie pinctrl states to mux states. It doesn't
> sound like too bad of a match to me?
> 
> Cheers,
> Peter
> 

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

[Peter Rosin]

>> Ok, I see what you mean now, sorry for being dense. If we allow this then
>> there is a need to add a special value that means all/many states (such as
>> -1 or something such) so that a mux-control can be used simultaneously by
>> drivers "pointing at" a specific state like you want to do, and by the
>> existing "application" style drivers that wraps the whole mux control.
>>
>> I.e. something like this
>>
>> 	mux: mux {
>> 		compatible = "mux-gpio";
>> 		...
>>
>> 		#mux-control-cells = <1>; /* one more than previously */
>> 	};
>>
>> 	phy {
>> 		...
>>
>> 		mux-control = <&mux 3>; /* point to specific state */
>> 	};
>>
>> 	i2c-mux {
>> 		compatible = "i2c-mux-gpmux";
>> 		parent = <&i2c0>
>> 		mux-control = <&mux (-1)>; /* many states needed */
>>
>> 		...
>>
>> 		i2c@1 {
>> 			eeprom@50 {
>> 				...
>> 			};
>> 		};
>>
>> 		i2c@2 {
>> 			...
>> 		};
>> 	};
>>
>> Yes, I realize that accesses to the eeprom cannot happen if the mux is
>> constantly selected and locked in state 3 by the phy, and that a mux with
>> one channel being a phy and other channels being I2C might not be
>> realistic, but the same gpio lines might control several muxes that are
>> used for separate signals solving at least the latter "problem" with this
>> completely made up example. Anyway, the above is in principle, and HW
>> designs are sometimes too weird for words.
>>
> 
> This is almost exactly what I was intending to implement except for one
> more change. The state of the mux will always be represented using the
> second argument(i.e. #mux-control-cells = <2>).
> 
> For example,
> mux-controls = <&mux 0 1>, <&mux 1 0>;
> 
> 
> With this I think we wouldn't need a special value for all or many states.

But you do. Several consumers need to be able to point to the same mux
control. If some of these consumers need one state, and some other need
all/many, the consumers needing many needs to be able to say that. Listing
many entries in mux-control = <>; is misleading since then the binding implies
that you could have different mux controls for each state, which is not
possible, at least not in the current implementations. It would also be
wasteful to needlessly establish links to the same mux control multiple
times, and the binding would cause bloated device trees even if you tried
to optimize this in the drivers. Therefore, I require a special value so
that consumers can continue to point at the mux control as a whole, even
if some other consumers of the same mux control wants to point at a specific
state.

>>> One more question that I had is, if the number of arguments match the
>>> #mux-control-cells and if the number of arguments are greater than 1 why
>>> is an error being returned?
>>
>> Changing that would require a bindings update anyway, so I simply
>> disallowed it as an error. Not much thought went into the decision,
>> as it couldn't be wrong to do what is being done with the bindings
>> that exist. That said, I have no problem lifting this restriction,
>> if there's a matching bindings update that makes it all fit.
>>
> 
> Sure, I think making a change in
> 
> Documentation/devicetree/bindings/mux/gpio-mux.yaml, should be good
> enough I assume.

Well, the new way to bind has very little to do with this being a gpio
mux. There is no reason not to allow this way to bind for any of the
other muxes. That said, the reg-mux binding has this:

  '#mux-control-cells':
    const: 1

Similarly, the adi,adg792a has explicit wording on how #mux-control-cells
works (but being a txt binding it is not checked, but that does not matter,
bindings should be correct). I now notice that this is missing from the
adi,adgs1408 binding, but that's an oversight.

The mux-controller binding has this:
  '#mux-control-cells':
    enum: [ 0, 1 ]

The mux-consumer binding should probably be updated with some words
on this subject too.

So, all mux bindings need updates when this door is opened. And, in order
to add this in a compatible way, the old way to bind with 0/1 cells needs
to continue to both work and be allowed.

I think it is easiest to add something common to the mux-controller
binding and then have the specific bindings simply inherit it from there
instead of adding (almost) the same words to all the driver bindings.

Cheers,
Peter

> Thank you for the comments. I'll post a respin of this series, with the
> above changes.

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

[Peter Rosin]

>> Ok, I see what you mean now, sorry for being dense. If we allow this then
>> there is a need to add a special value that means all/many states (such as
>> -1 or something such) so that a mux-control can be used simultaneously by
>> drivers "pointing at" a specific state like you want to do, and by the
>> existing "application" style drivers that wraps the whole mux control.
>>
>> I.e. something like this
>>
>> 	mux: mux {
>> 		compatible = "mux-gpio";
>> 		...
>>
>> 		#mux-control-cells = <1>; /* one more than previously */
>> 	};
>>
>> 	phy {
>> 		...
>>
>> 		mux-control = <&mux 3>; /* point to specific state */
>> 	};
>>
>> 	i2c-mux {
>> 		compatible = "i2c-mux-gpmux";
>> 		parent = <&i2c0>
>> 		mux-control = <&mux (-1)>; /* many states needed */
>>
>> 		...
>>
>> 		i2c@1 {
>> 			eeprom@50 {
>> 				...
>> 			};
>> 		};
>>
>> 		i2c@2 {
>> 			...
>> 		};
>> 	};
>>
>> Yes, I realize that accesses to the eeprom cannot happen if the mux is
>> constantly selected and locked in state 3 by the phy, and that a mux with
>> one channel being a phy and other channels being I2C might not be
>> realistic, but the same gpio lines might control several muxes that are
>> used for separate signals solving at least the latter "problem" with this
>> completely made up example. Anyway, the above is in principle, and HW
>> designs are sometimes too weird for words.
>>
> 
> This is almost exactly what I was intending to implement except for one
> more change. The state of the mux will always be represented using the
> second argument(i.e. #mux-control-cells = <2>).
> 
> For example,
> mux-controls = <&mux 0 1>, <&mux 1 0>;
> 
> 
> With this I think we wouldn't need a special value for all or many states.

But you do. Several consumers need to be able to point to the same mux
control. If some of these consumers need one state, and some other need
all/many, the consumers needing many needs to be able to say that. Listing
many entries in mux-control = <>; is misleading since then the binding implies
that you could have different mux controls for each state, which is not
possible, at least not in the current implementations. It would also be
wasteful to needlessly establish links to the same mux control multiple
times, and the binding would cause bloated device trees even if you tried
to optimize this in the drivers. Therefore, I require a special value so
that consumers can continue to point at the mux control as a whole, even
if some other consumers of the same mux control wants to point at a specific
state.

>>> One more question that I had is, if the number of arguments match the
>>> #mux-control-cells and if the number of arguments are greater than 1 why
>>> is an error being returned?
>>
>> Changing that would require a bindings update anyway, so I simply
>> disallowed it as an error. Not much thought went into the decision,
>> as it couldn't be wrong to do what is being done with the bindings
>> that exist. That said, I have no problem lifting this restriction,
>> if there's a matching bindings update that makes it all fit.
>>
> 
> Sure, I think making a change in
> 
> Documentation/devicetree/bindings/mux/gpio-mux.yaml, should be good
> enough I assume.

Well, the new way to bind has very little to do with this being a gpio
mux. There is no reason not to allow this way to bind for any of the
other muxes. That said, the reg-mux binding has this:

  '#mux-control-cells':
    const: 1

Similarly, the adi,adg792a has explicit wording on how #mux-control-cells
works (but being a txt binding it is not checked, but that does not matter,
bindings should be correct). I now notice that this is missing from the
adi,adgs1408 binding, but that's an oversight.

The mux-controller binding has this:
  '#mux-control-cells':
    enum: [ 0, 1 ]

The mux-consumer binding should probably be updated with some words
on this subject too.

So, all mux bindings need updates when this door is opened. And, in order
to add this in a compatible way, the old way to bind with 0/1 cells needs
to continue to both work and be allowed.

I think it is easiest to add something common to the mux-controller
binding and then have the specific bindings simply inherit it from there
instead of adding (almost) the same words to all the driver bindings.

Cheers,
Peter

> Thank you for the comments. I'll post a respin of this series, with the
> above changes.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Aswath Govindraju]

Hi Peter,

On 18/11/21 6:14 pm, Peter Rosin wrote:
>>> Ok, I see what you mean now, sorry for being dense. If we allow this then
>>> there is a need to add a special value that means all/many states (such as
>>> -1 or something such) so that a mux-control can be used simultaneously by
>>> drivers "pointing at" a specific state like you want to do, and by the
>>> existing "application" style drivers that wraps the whole mux control.
>>>
>>> I.e. something like this
>>>
>>> 	mux: mux {
>>> 		compatible = "mux-gpio";
>>> 		...
>>>
>>> 		#mux-control-cells = <1>; /* one more than previously */
>>> 	};
>>>
>>> 	phy {
>>> 		...
>>>
>>> 		mux-control = <&mux 3>; /* point to specific state */
>>> 	};
>>>
>>> 	i2c-mux {
>>> 		compatible = "i2c-mux-gpmux";
>>> 		parent = <&i2c0>
>>> 		mux-control = <&mux (-1)>; /* many states needed */
>>>
>>> 		...
>>>
>>> 		i2c@1 {
>>> 			eeprom@50 {
>>> 				...
>>> 			};
>>> 		};
>>>
>>> 		i2c@2 {
>>> 			...
>>> 		};
>>> 	};
>>>
>>> Yes, I realize that accesses to the eeprom cannot happen if the mux is
>>> constantly selected and locked in state 3 by the phy, and that a mux with
>>> one channel being a phy and other channels being I2C might not be
>>> realistic, but the same gpio lines might control several muxes that are
>>> used for separate signals solving at least the latter "problem" with this
>>> completely made up example. Anyway, the above is in principle, and HW
>>> designs are sometimes too weird for words.
>>>
>>
>> This is almost exactly what I was intending to implement except for one
>> more change. The state of the mux will always be represented using the
>> second argument(i.e. #mux-control-cells = <2>).
>>
>> For example,
>> mux-controls = <&mux 0 1>, <&mux 1 0>;
>>
>>
>> With this I think we wouldn't need a special value for all or many states.
> 
> But you do. Several consumers need to be able to point to the same mux
> control. If some of these consumers need one state, and some other need
> all/many, the consumers needing many needs to be able to say that. Listing
> many entries in mux-control = <>; is misleading since then the binding implies
> that you could have different mux controls for each state, which is not
> possible, at least not in the current implementations. It would also be
> wasteful to needlessly establish links to the same mux control multiple
> times, and the binding would cause bloated device trees even if you tried
> to optimize this in the drivers. Therefore, I require a special value so
> that consumers can continue to point at the mux control as a whole, even
> if some other consumers of the same mux control wants to point at a specific
> state.
> 


Understood. One issue that I see is that we certainly can not use the
first argument for representing state as it will result in errors for
current users.

I feel that the safest way to go would be by using a second argument to
represent the state or to represent multiple states can be used by the
driver. The issue that I see with this approach is that currently the
fist argument is used to select the line number from the mux and if the
we use two arguments like this,

mux-controls = <&mux 0 -1>

then this would mean that line nnumber 0 in the mux could use multiple
states and for a driver to use mutiple lines we would need to add an
entry for each line which would bloat the code a well increase the
complexity in the drivers while using devm_mux_get(). So, one solution
that I could think of is to use a "-1" for the first argument too. This
would indicate that the driver would need to toggle multiple lines in
the mux

For example,

1) mux-controls = <&mux -1 3> // the driver would need to set the mux
lines to 3 for enabling it

2) mux-controls = <&mux -1 -1> //the driver would need to set the mux
lines and multiple states in the mux

3) mux-controls = <&mux 0 1> // the driver would need to set the zeroth
mux line to 1

I do see that, going with this method would make <&mux ^\d*$ ^\d*$>(i.e.
any positive number in the first argument) redundant as it can be
represented with <&mux -1 *>. However, I think is the only way so that
existing users will not see issues.


>>>> One more question that I had is, if the number of arguments match the
>>>> #mux-control-cells and if the number of arguments are greater than 1 why
>>>> is an error being returned?
>>>
>>> Changing that would require a bindings update anyway, so I simply
>>> disallowed it as an error. Not much thought went into the decision,
>>> as it couldn't be wrong to do what is being done with the bindings
>>> that exist. That said, I have no problem lifting this restriction,
>>> if there's a matching bindings update that makes it all fit.
>>>
>>
>> Sure, I think making a change in
>>
>> Documentation/devicetree/bindings/mux/gpio-mux.yaml, should be good
>> enough I assume.
> 
> Well, the new way to bind has very little to do with this being a gpio
> mux. There is no reason not to allow this way to bind for any of the
> other muxes. That said, the reg-mux binding has this:
> 
>   '#mux-control-cells':
>     const: 1
> 
> Similarly, the adi,adg792a has explicit wording on how #mux-control-cells
> works (but being a txt binding it is not checked, but that does not matter,
> bindings should be correct). I now notice that this is missing from the
> adi,adgs1408 binding, but that's an oversight.
> 
> The mux-controller binding has this:
>   '#mux-control-cells':
>     enum: [ 0, 1 ]
> 
> The mux-consumer binding should probably be updated with some words
> on this subject too.
> 
> So, all mux bindings need updates when this door is opened. And, in order
> to add this in a compatible way, the old way to bind with 0/1 cells needs
> to continue to both work and be allowed.
> 
> I think it is easiest to add something common to the mux-controller
> binding and then have the specific bindings simply inherit it from there
> instead of adding (almost) the same words to all the driver bindings.
> 

Understood, I will try to add changes in the common mux-controller
bindings itself and then reference it in the gpio-mux bindings

Thanks,
Aswath

> Cheers,
> Peter
> 
>> Thank you for the comments. I'll post a respin of this series, with the
>> above changes.

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

[Aswath Govindraju]

Hi Peter,

On 18/11/21 6:14 pm, Peter Rosin wrote:
>>> Ok, I see what you mean now, sorry for being dense. If we allow this then
>>> there is a need to add a special value that means all/many states (such as
>>> -1 or something such) so that a mux-control can be used simultaneously by
>>> drivers "pointing at" a specific state like you want to do, and by the
>>> existing "application" style drivers that wraps the whole mux control.
>>>
>>> I.e. something like this
>>>
>>> 	mux: mux {
>>> 		compatible = "mux-gpio";
>>> 		...
>>>
>>> 		#mux-control-cells = <1>; /* one more than previously */
>>> 	};
>>>
>>> 	phy {
>>> 		...
>>>
>>> 		mux-control = <&mux 3>; /* point to specific state */
>>> 	};
>>>
>>> 	i2c-mux {
>>> 		compatible = "i2c-mux-gpmux";
>>> 		parent = <&i2c0>
>>> 		mux-control = <&mux (-1)>; /* many states needed */
>>>
>>> 		...
>>>
>>> 		i2c@1 {
>>> 			eeprom@50 {
>>> 				...
>>> 			};
>>> 		};
>>>
>>> 		i2c@2 {
>>> 			...
>>> 		};
>>> 	};
>>>
>>> Yes, I realize that accesses to the eeprom cannot happen if the mux is
>>> constantly selected and locked in state 3 by the phy, and that a mux with
>>> one channel being a phy and other channels being I2C might not be
>>> realistic, but the same gpio lines might control several muxes that are
>>> used for separate signals solving at least the latter "problem" with this
>>> completely made up example. Anyway, the above is in principle, and HW
>>> designs are sometimes too weird for words.
>>>
>>
>> This is almost exactly what I was intending to implement except for one
>> more change. The state of the mux will always be represented using the
>> second argument(i.e. #mux-control-cells = <2>).
>>
>> For example,
>> mux-controls = <&mux 0 1>, <&mux 1 0>;
>>
>>
>> With this I think we wouldn't need a special value for all or many states.
> 
> But you do. Several consumers need to be able to point to the same mux
> control. If some of these consumers need one state, and some other need
> all/many, the consumers needing many needs to be able to say that. Listing
> many entries in mux-control = <>; is misleading since then the binding implies
> that you could have different mux controls for each state, which is not
> possible, at least not in the current implementations. It would also be
> wasteful to needlessly establish links to the same mux control multiple
> times, and the binding would cause bloated device trees even if you tried
> to optimize this in the drivers. Therefore, I require a special value so
> that consumers can continue to point at the mux control as a whole, even
> if some other consumers of the same mux control wants to point at a specific
> state.
> 


Understood. One issue that I see is that we certainly can not use the
first argument for representing state as it will result in errors for
current users.

I feel that the safest way to go would be by using a second argument to
represent the state or to represent multiple states can be used by the
driver. The issue that I see with this approach is that currently the
fist argument is used to select the line number from the mux and if the
we use two arguments like this,

mux-controls = <&mux 0 -1>

then this would mean that line nnumber 0 in the mux could use multiple
states and for a driver to use mutiple lines we would need to add an
entry for each line which would bloat the code a well increase the
complexity in the drivers while using devm_mux_get(). So, one solution
that I could think of is to use a "-1" for the first argument too. This
would indicate that the driver would need to toggle multiple lines in
the mux

For example,

1) mux-controls = <&mux -1 3> // the driver would need to set the mux
lines to 3 for enabling it

2) mux-controls = <&mux -1 -1> //the driver would need to set the mux
lines and multiple states in the mux

3) mux-controls = <&mux 0 1> // the driver would need to set the zeroth
mux line to 1

I do see that, going with this method would make <&mux ^\d*$ ^\d*$>(i.e.
any positive number in the first argument) redundant as it can be
represented with <&mux -1 *>. However, I think is the only way so that
existing users will not see issues.


>>>> One more question that I had is, if the number of arguments match the
>>>> #mux-control-cells and if the number of arguments are greater than 1 why
>>>> is an error being returned?
>>>
>>> Changing that would require a bindings update anyway, so I simply
>>> disallowed it as an error. Not much thought went into the decision,
>>> as it couldn't be wrong to do what is being done with the bindings
>>> that exist. That said, I have no problem lifting this restriction,
>>> if there's a matching bindings update that makes it all fit.
>>>
>>
>> Sure, I think making a change in
>>
>> Documentation/devicetree/bindings/mux/gpio-mux.yaml, should be good
>> enough I assume.
> 
> Well, the new way to bind has very little to do with this being a gpio
> mux. There is no reason not to allow this way to bind for any of the
> other muxes. That said, the reg-mux binding has this:
> 
>   '#mux-control-cells':
>     const: 1
> 
> Similarly, the adi,adg792a has explicit wording on how #mux-control-cells
> works (but being a txt binding it is not checked, but that does not matter,
> bindings should be correct). I now notice that this is missing from the
> adi,adgs1408 binding, but that's an oversight.
> 
> The mux-controller binding has this:
>   '#mux-control-cells':
>     enum: [ 0, 1 ]
> 
> The mux-consumer binding should probably be updated with some words
> on this subject too.
> 
> So, all mux bindings need updates when this door is opened. And, in order
> to add this in a compatible way, the old way to bind with 0/1 cells needs
> to continue to both work and be allowed.
> 
> I think it is easiest to add something common to the mux-controller
> binding and then have the specific bindings simply inherit it from there
> instead of adding (almost) the same words to all the driver bindings.
> 

Understood, I will try to add changes in the common mux-controller
bindings itself and then reference it in the gpio-mux bindings

Thanks,
Aswath

> Cheers,
> Peter
> 
>> Thank you for the comments. I'll post a respin of this series, with the
>> above changes.


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 0/2] CAN TRANSCEIVER: Add support for setting mux

[Aswath Govindraju]

Hi all,

On 11/11/21 10:13 pm, 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.
> 
> Aswath Govindraju (2):
>   dt-bindings: phy: ti,tcan104x-can: Document mux-controls property
>   phy: phy-can-transceiver: Add support for setting mux
> 

Thank you for the comments. I have posted a v2 for this series after
making the changes that were requested.

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

Thanks,
Aswath

>  .../bindings/phy/ti,tcan104x-can.yaml         |  6 ++++++
>  drivers/phy/phy-can-transceiver.c             | 21 +++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 0/2] CAN TRANSCEIVER: Add support for setting mux

[Aswath Govindraju]

Hi all,

On 11/11/21 10:13 pm, 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.
> 
> Aswath Govindraju (2):
>   dt-bindings: phy: ti,tcan104x-can: Document mux-controls property
>   phy: phy-can-transceiver: Add support for setting mux
> 

Thank you for the comments. I have posted a v2 for this series after
making the changes that were requested.

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

Thanks,
Aswath

>  .../bindings/phy/ti,tcan104x-can.yaml         |  6 ++++++
>  drivers/phy/phy-can-transceiver.c             | 21 +++++++++++++++++++
>  2 files changed, 27 insertions(+)
>