flexcan driver updates

The property clock-frequency is optional for device tree probe. When it's absent, the flexcan driver will try to get the frequency from clk system by calling clk_get_rate.

Signed-off-by Shawn Guo <[email protected]>

The flexcan driver has function pointer transceiver_switch defined in flexcan_platform_data for platform codes to hook up their transceiver switch implementation. However this does not cope with device tree probe.

It's been observed that platforms mostly use gpios to control the switch of flexcan transceiver, like enable and standby. The patch adds transceiver switch gpios support into flexcan driver, so that platforms booting from device tree can just define properties phy-enable-gpios and phy-standby-gpios to have flexcan driver control the gpios.

Signed-off-by Shawn Guo <[email protected]>

---
 .../devicetree/bindings/net/can/fsl-flexcan.txt    |    2 +
 drivers/net/can/flexcan.c                          |   62 ++++++++++++++++++++
 2 files changed, 64 insertions(+), 0 deletions(-)

Documentation/devicetree/bindings/net/can/fsl-flexcan.txt +0 -0

index 8ff324e..e0dbac7 100644

Quoted 3 lines

+- phy-enable-gpios : Specify the gpio used to enable phy
+- phy-standby-gpios : Specify the gpio used to put phy into STANDBY mode

Why the plural form 'gpios'? It's just one gpio.
s/gpios/gpio/ ?

The flexcan driver has function pointer transceiver_switch defined in flexcan_platform_data for platform codes to hook up their transceiver switch implementation. However this does not cope with device tree probe.

It's been observed that platforms mostly use gpios to control the switch of flexcan transceiver, like enable and standby. The patch adds transceiver switch gpios support into flexcan driver, so that platforms booting from device tree can just define properties phy-enable-gpios and phy-standby-gpios to have flexcan driver control the gpios.

Signed-off-by Shawn Guo <[email protected]>

---
 .../devicetree/bindings/net/can/fsl-flexcan.txt    |    2 +
 drivers/net/can/flexcan.c                          |   62 ++++++++++++++++++++
 2 files changed, 64 insertions(+), 0 deletions(-)

Documentation/devicetree/bindings/net/can/fsl-flexcan.txt +0 -0

index 8ff324e..e0dbac7 100644

Quoted 10 lines

Documentation/devicetree/bindings/net/can/fsl-flexcan.txt +0 -0

index 8ff324e..e0dbac7 100644
@@ -15,6 +15,8 @@ Required properties:

Optional properties:
- clock-frequency : The oscillator frequency driving the flexcan device

+- phy-enable-gpios : Specify the gpio used to enable phy
+- phy-standby-gpios : Specify the gpio used to put phy into STANDBY mode

Example:

Do we need to add new added entries in the example section as well.

E.g.

drivers/net/can/flexcan.c line 29

Quoted 9 lines

drivers/net/can/flexcan.c +1 -0

index 38c0690..1ce3f9e 100644
@@ -26,6 +26,7 @@
 #include <linux/can/platform/flexcan.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/gpio.h>

It seems <linux/of_gpio.h> already unconditionally includes this header.

Quoted 13 lines · Hui Wang

Quoted 11 lines

Documentation/devicetree/bindings/net/can/fsl-flexcan.txt +0 -0

index 8ff324e..e0dbac7 100644
@@ -15,6 +15,8 @@ Required properties:

Optional properties:
- clock-frequency : The oscillator frequency driving the flexcan device

+- phy-enable-gpios : Specify the gpio used to enable phy
+- phy-standby-gpios : Specify the gpio used to put phy into STANDBY mode

Example:

Do we need to add new added entries in the example section as well.

Review

Quoted 6 lines

E.g.

(quoted diff) +2 -0

+        phy-enable-gpios = <&gpio1 4 0>; /* GPIO1_4, active high*/
+        phy-standby-gpios = <&gpio1 2 1>; /* GPIO1_2, active low */

I chose not to because after all these two are optional properties.

Quoted 12 lines

drivers/net/can/flexcan.c line 30

Quoted 10 lines

drivers/net/can/flexcan.c +1 -0

index 38c0690..1ce3f9e 100644
@@ -26,6 +26,7 @@
 #include <linux/can/platform/flexcan.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/gpio.h>
 

It seems <linux/of_gpio.h> already unconditionally includes this header.

Documentation/SubmitChecklist

1: If you use a facility then #include the file that defines/declares
that facility. Don't depend on other header files pulling in ones that you use.

Quoted 18 lines · Shawn Guo

Quoted 16 lines · Hui Wang

Quoted 9 lines

diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt index 8ff324e..e0dbac7 100644

Documentation/devicetree/bindings/net/can/fsl-flexcan.txt +0 -0

@@ -15,6 +15,8 @@ Required properties:

Optional properties:
- clock-frequency : The oscillator frequency driving the flexcan device

Quoted 5 lines

+- phy-enable-gpios : Specify the gpio used to enable phy
+- phy-standby-gpios : Specify the gpio used to put phy into STANDBY

mode
Example:

Do we need to add new added entries in the example section as well.

Quoted 9 lines

Review

Quoted 6 lines

E.g.

(quoted diff) +2 -0

+        phy-enable-gpios = <&gpio1 4 0>; /* GPIO1_4, active high*/
+        phy-standby-gpios = <&gpio1 2 1>; /* GPIO1_2, active low */

I chose not to because after all these two are optional properties.

OK.

Quoted 15 lines

Quoted 12 lines

drivers/net/can/flexcan.c line 30

Quoted 10 lines

drivers/net/can/flexcan.c +1 -0

index 38c0690..1ce3f9e 100644
@@ -26,6 +26,7 @@
 #include <linux/can/platform/flexcan.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/gpio.h>
 

It seems <linux/of_gpio.h> already unconditionally includes this header.

Documentation/SubmitChecklist

Quoted 5 lines

1: If you use a facility then #include the file that defines/declares
that facility. Don't depend on other header files pulling in ones that you use.

Got it.

OK to me.

The flexcan driver has function pointer transceiver_switch defined in flexcan_platform_data for platform codes to hook up their transceiver switch implementation. However this does not cope with device tree probe.

It's been observed that platforms mostly use gpios to control the switch of flexcan transceiver, like enable and standby. The patch adds transceiver switch gpios support into flexcan driver, so that platforms booting from device tree can just define properties phy-enable-gpios and phy-standby-gpios to have flexcan driver control the gpios.

Signed-off-by Shawn Guo <[email protected]>

---
 .../devicetree/bindings/net/can/fsl-flexcan.txt    |    2 +
 drivers/net/can/flexcan.c                          |   62 ++++++++++++++++++++
 2 files changed, 64 insertions(+), 0 deletions(-)

Documentation/devicetree/bindings/net/can/fsl-flexcan.txt +2 -0

index 8ff324e..e0dbac7 100644
@@ -15,6 +15,8 @@ Required properties:
 Optional properties:
 
 - clock-frequency : The oscillator frequency driving the flexcan device
+- phy-enable-gpios : Specify the gpio used to enable phy
+- phy-standby-gpios : Specify the gpio used to put phy into STANDBY mode
 
 Example:
 

drivers/net/can/flexcan.c +23 -0

index 38c0690..1ce3f9e 100644
@@ -26,6 +26,7 @@
 #include <linux/can/platform/flexcan.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/gpio.h>
 #include <linux/if_arp.h>
 #include <linux/if_ether.h>
 #include <linux/interrupt.h>

@@ -34,6 +35,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/pinctrl/consumer.h>
 

@@ -180,6 +182,11 @@ struct flexcan_priv {
 
 	struct clk *clk;
 	struct flexcan_platform_data *pdata;
+
+	int phy_en_gpio;
+	int phy_stby_gpio;
+	bool phy_en_high;
+	bool phy_stby_high;
 };
 
 static struct can_bittiming_const flexcan_bittiming_const = {

@@ -224,6 +231,17 @@ static inline void flexcan_write(u32 val, void __iomem *addr)
  */
 static void flexcan_transceiver_switch(const struct flexcan_priv *priv, int on)
 {
+	/*
+	 * on == 1: enable phy and exit standby
+	 * on == 0: disable phy and enter standby
+	 */
+	if (gpio_is_valid(priv->phy_en_gpio))
+		gpio_set_value(priv->phy_en_gpio,
+			       priv->phy_en_high ? on : !on);
+	if (gpio_is_valid(priv->phy_stby_gpio))
+		gpio_set_value(priv->phy_stby_gpio,
+			       priv->phy_stby_high ? !on : on);
+
 	if (priv->pdata && priv->pdata->transceiver_switch)
 		priv->pdata->transceiver_switch(on);
 }

@@ -933,6 +951,10 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 	resource_size_t mem_size;
 	int err, irq;
 	u32 clock_freq = 0;
+	int phy_en_gpio = -EINVAL;
+	int phy_stby_gpio = -EINVAL;
+	bool phy_en_high = true;
+	bool phy_stby_high = true;
 
 	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
 	if (IS_ERR(pinctrl))

@@ -940,11 +962,46 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 
 	if (pdev->dev.of_node) {
 		const u32 *clock_freq_p;
+		enum of_gpio_flags flags;
 
 		clock_freq_p = of_get_property(pdev->dev.of_node,

drivers/net/can/flexcan.c +32 -0

+			err = devm_gpio_request_one(&pdev->dev, phy_en_gpio,
+						    GPIOF_DIR_OUT,
+						    "phy-enable");
+			if (err) {
+				dev_err(&pdev->dev,
+					"failed to request gpio %d: %d\n",
+					phy_en_gpio, err);
+				goto failed_gpio;
+			}
+		}
+
+		phy_stby_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+							"phy-standby-gpios",
+							0, &flags);
+		if (gpio_is_valid(phy_stby_gpio)) {
+			if (flags == OF_GPIO_ACTIVE_LOW)
+				phy_stby_high = false;
+			err = devm_gpio_request_one(&pdev->dev, phy_stby_gpio,
+						    GPIOF_DIR_OUT,
+						    "phy-standby");
+			if (err) {
+				dev_err(&pdev->dev,
+					"failed to request gpio %d: %d\n",
+					phy_stby_gpio, err);
+				goto failed_gpio;
+			}
+		}
 	}
 
 	if (!clock_freq) {

@@ -997,6 +1054,10 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 	priv->base = base;
 	priv->dev = dev;
 	priv->clk = clk;
+	priv->phy_en_gpio = phy_en_gpio;
+	priv->phy_en_high = phy_en_high;
+	priv->phy_stby_gpio = phy_stby_gpio;
+	priv->phy_stby_high = phy_stby_high;
 	priv->pdata = pdev->dev.platform_data;
 
 	netif_napi_add(dev, &priv->napi, flexcan_poll, FLEXCAN_NAPI_WEIGHT);

@@ -1025,6 +1086,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 	if (clk)
 		clk_put(clk);
  failed_clock:
+ failed_gpio:
 	return err;
 }
 

Hmm I'm wondering if transceiver or phy is the correct name here. In platform_data it's called transceiver_switch.

Quoted 13 lines

drivers/net/can/flexcan.c +5 -0

+	int phy_en_gpio = -EINVAL;
+	int phy_stby_gpio = -EINVAL;
+	bool phy_en_high = true;
+	bool phy_stby_high = true;
 
 	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
 	if (IS_ERR(pinctrl))

@@ -940,11 +962,46 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 
 	if (pdev->dev.of_node) {
 		const u32 *clock_freq_p;
+		enum of_gpio_flags flags;
 

drivers/net/can/flexcan.c static int __devinit flexcan_probe(struct platform_device *pdev) · line 977

Quoted 11 lines

drivers/net/can/flexcan.c +7 -0

 		clock_freq_p = of_get_property(pdev->dev.of_node,
 						"clock-frequency", NULL);
 		if (clock_freq_p)
 			clock_freq = *clock_freq_p;
+
+		phy_en_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+						      "phy-enable-gpios",
+						       0, &flags);
+		if (gpio_is_valid(phy_en_gpio)) {
+			if (flags == OF_GPIO_ACTIVE_LOW)
+				phy_en_high = false;

I really like the "flags" solution, much better than a DT property. What about keeping the term active_low instead of en_high? From my limited knowledge about electronic I say, that active low is a standard term.

Quoted 2 lines · Marc Kleine-Budde

Hmm I'm wondering if transceiver or phy is the correct name here. In platform_data it's called transceiver_switch.

The transceiver_switch in platform_data names a function, while we are naming a couple gpios which happen to be access in that function. It looks all correct to me.

I mean which name is more precise, do these gpio enable/standy a "phy" or a "transceiver". For example:
http://www.nxp.com/documents/application_note/AN00094.pdf, this document
says: TJA1041/1041A high speed CAN transceiver.

Quoted 5 lines · Marc Kleine-Budde

I mean which name is more precise, do these gpio enable/standy a "phy" or a "transceiver". For example:
http://www.nxp.com/documents/application_note/AN00094.pdf, this document
says: TJA1041/1041A high speed CAN transceiver.

Isn't term "phy" (physical interface) generally meant to be the same thing as "transceiver"? I just happened to like the shorter one as what Hui did in his patch.

But it does not really matter to me, will change the name since you care about it.

Do you care the variable name also? If so, we will get:

        int transceiver_en_gpio;
        int transceiver_stby_gpio;
        bool transceiver_en_high;
        bool transceiver_stby_high;

So everything becomes long :)

Quoted 7 lines · Shawn Guo

Do you care the variable name also? If so, we will get:

        int transceiver_en_gpio;
        int transceiver_stby_gpio;
        bool transceiver_en_high;
        bool transceiver_stby_high;

Quoted 2 lines

So everything becomes long :)

Oh, even longer, since we have agreed "high" should be renamed to "active_low":

	bool transceiver_en_active_low;
	bool transceiver_stby_active_low;

Quoted 6 lines · Shawn Guo

Quoted 5 lines · Marc Kleine-Budde

I mean which name is more precise, do these gpio enable/standy a "phy" or a "transceiver". For example:
http://www.nxp.com/documents/application_note/AN00094.pdf, this document
says: TJA1041/1041A high speed CAN transceiver.

Quoted 7 lines

Isn't term "phy" (physical interface) generally meant to be the same thing as "transceiver"? I just happened to like the shorter one as what Hui did in his patch.

But it does not really matter to me, will change the name since you care about it.

Do you care the variable name also? If so, we will get:

        int transceiver_en_gpio;
        int transceiver_stby_gpio;
        bool transceiver_en_high;
        bool transceiver_stby_high;

So everything becomes long :)

Quoted 5 lines · Marc Kleine-Budde

I mean which name is more precise, do these gpio enable/standy a "phy" or a "transceiver". For example:
http://www.nxp.com/documents/application_note/AN00094.pdf, this document
says: TJA1041/1041A high speed CAN transceiver.

Isn't term "phy" (physical interface) generally meant to be the same thing as "transceiver"? I just happened to like the shorter one as what Hui did in his patch.

But it does not really matter to me, will change the name since you care about it.

Hi,

Quoted 10 lines · Shawn Guo

Quoted 5 lines · Marc Kleine-Budde

I mean which name is more precise, do these gpio enable/standy a "phy" or a "transceiver". For example:
http://www.nxp.com/documents/application_note/AN00094.pdf, this document
says: TJA1041/1041A high speed CAN transceiver.

Isn't term "phy" (physical interface) generally meant to be the same thing as "transceiver"? I just happened to like the shorter one as what Hui did in his patch.

Quoted 3 lines

But it does not really matter to me, will change the name since you care about it.

A transceiver is just a dumb piece of hardware, while a PHY contains some intelligence of its own.

Quoted 13 lines · Lothar Wa?mann

Hi,

Quoted 10 lines · Shawn Guo

Quoted 5 lines · Marc Kleine-Budde

I mean which name is more precise, do these gpio enable/standy a "phy" or a "transceiver". For example:
http://www.nxp.com/documents/application_note/AN00094.pdf, this document
says: TJA1041/1041A high speed CAN transceiver.

Isn't term "phy" (physical interface) generally meant to be the same thing as "transceiver"? I just happened to like the shorter one as what Hui did in his patch.

Quoted 6 lines

Quoted 3 lines

But it does not really matter to me, will change the name since you care about it.

A transceiver is just a dumb piece of hardware, while a PHY contains some intelligence of its own.

Then, it sounds more like a PHY than transceiver, since it's an IC chip with some control over it.

Hi,

Quoted 22 lines · Shawn Guo

Quoted 20 lines · Lothar Wa?mann

Hi,

Quoted 16 lines · Shawn Guo

Quoted 6 lines · Marc Kleine-Budde

I mean which name is more precise, do these gpio enable/standy a "phy" or a "transceiver". For example:
http://www.nxp.com/documents/application_note/AN00094.pdf, this document
says: TJA1041/1041A high speed CAN transceiver.

Isn't term "phy" (physical interface) generally meant to be the same thing as "transceiver"? I just happened to like the shorter one as what Hui did in his patch.

But it does not really matter to me, will change the name since you care about it.

Quoted 8 lines

Quoted 4 lines

A transceiver is just a dumb piece of hardware, while a PHY contains some intelligence of its own.

Then, it sounds more like a PHY than transceiver, since it's an IC chip with some control over it.

The 'I' in 'IC' does not stand for 'intelligent', but for 'integrated'. ;)
A can transceiver is usually merely a switchable buffer. There are no registers to configure it or an internal processor that does some magic.

Lothar Wa?mann

Quoted 5 lines · Marc Kleine-Budde

I mean which name is more precise, do these gpio enable/standy a "phy" or a "transceiver". For example:
http://www.nxp.com/documents/application_note/AN00094.pdf, this document
says: TJA1041/1041A high speed CAN transceiver.

Isn't term "phy" (physical interface) generally meant to be the same thing as "transceiver"? I just happened to like the shorter one as what Hui did in his patch.

Quoted 6 lines · Shawn Guo

Quoted 5 lines · Marc Kleine-Budde

I mean which name is more precise, do these gpio enable/standy a "phy" or a "transceiver". For example:
http://www.nxp.com/documents/application_note/AN00094.pdf, this document
says: TJA1041/1041A high speed CAN transceiver.

Quoted 3 lines

Isn't term "phy" (physical interface) generally meant to be the same thing as "transceiver"? I just happened to like the shorter one as what Hui did in his patch.

'trx' seemed the right abbreviation
http://www.acronymfinder.com/TRX.html

thanks to Oliver, in his email on May 9th.

'trx' seemed the right abbreviation
http://www.acronymfinder.com/TRX.html

Quoted 11 lines · Shawn Guo

The flexcan driver has function pointer transceiver_switch defined in flexcan_platform_data for platform codes to hook up their transceiver switch implementation. However this does not cope with device tree probe.

It's been observed that platforms mostly use gpios to control the switch of flexcan transceiver, like enable and standby. The patch adds transceiver switch gpios support into flexcan driver, so that platforms booting from device tree can just define properties phy-enable-gpios and phy-standby-gpios to have flexcan driver control the gpios.

Hmm I'm wondering if transceiver or phy is the correct name here. In platform_data it's called transceiver_switch.

The flexcan driver has function pointer transceiver_switch defined in flexcan_platform_data for platform codes to hook up their transceiver switch implementation. However this does not cope with device tree probe.

It's been observed that platforms mostly use gpios to control the switch of flexcan transceiver, like enable and standby. The patch adds transceiver switch gpios support into flexcan driver, so that platforms booting from device tree can just define properties phy-enable-gpios and phy-standby-gpios to have flexcan driver control the gpios.

drivers/net/can/flexcan.c +4 -0

+		phy_stby_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+							"phy-standby-gpios",
+							0, &flags);
+		if (gpio_is_valid(phy_stby_gpio)) {

drivers/net/can/flexcan.c +6 -0

+			}
+		}
 	}
 
 	if (!clock_freq) {

@@ -997,6 +1054,10 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 	priv->base = base;
 	priv->dev = dev;
 	priv->clk = clk;
+	priv->phy_en_gpio = phy_en_gpio;
+	priv->phy_en_high = phy_en_high;
+	priv->phy_stby_gpio = phy_stby_gpio;
+	priv->phy_stby_high = phy_stby_high;
 	priv->pdata = pdev->dev.platform_data;
 
 	netif_napi_add(dev, &priv->napi, flexcan_poll, FLEXCAN_NAPI_WEIGHT);

Quoted 8 lines

drivers/net/can/flexcan.c +8 -0

+		phy_stby_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+							"phy-standby-gpios",
+							0, &flags);
+		if (gpio_is_valid(phy_stby_gpio)) {
+			if (flags == OF_GPIO_ACTIVE_LOW)
+				phy_stby_high = false;
+			err = devm_gpio_request_one(&pdev->dev, phy_stby_gpio,
+						    GPIOF_DIR_OUT,

drivers/net/can/flexcan.c static int __devinit flexcan_probe(struct platform_device *pdev) · line 1002

Quoted 6 lines

drivers/net/can/flexcan.c +6 -0

+						    "phy-standby");
+			if (err) {
+				dev_err(&pdev->dev,
+					"failed to request gpio %d: %d\n",
+					phy_stby_gpio, err);
+				goto failed_gpio;

I checked mx28 evk, it seems the phy only has a STB gpio and shared by both CAN0&CAN1. I wonder the CAN1 probe may fail here.

Quoted 8 lines · Dong Aisheng

Quoted 8 lines

drivers/net/can/flexcan.c +8 -0

+		phy_stby_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+							"phy-standby-gpios",
+							0, &flags);
+		if (gpio_is_valid(phy_stby_gpio)) {
+			if (flags == OF_GPIO_ACTIVE_LOW)
+				phy_stby_high = false;
+			err = devm_gpio_request_one(&pdev->dev, phy_stby_gpio,
+						    GPIOF_DIR_OUT,

Quoted 9 lines

drivers/net/can/flexcan.c static int __devinit flexcan_probe(struct platform_device *pdev) · line 1002

Quoted 6 lines

drivers/net/can/flexcan.c +6 -0

+						    "phy-standby");
+			if (err) {
+				dev_err(&pdev->dev,
+					"failed to request gpio %d: %d\n",
+					phy_stby_gpio, err);
+				goto failed_gpio;

I checked mx28 evk, it seems the phy only has a STB gpio and shared by both CAN0&CAN1. I wonder the CAN1 probe may fail here.

It can be managed by dts. Here is what I have in imx28-evk.dts, where only can0 has phy-enable-gpios property.

	can0: can at 80032000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can0_pins_a>;
		phy-enable-gpios = <&gpio2 13 0>;
		status = "okay";
	};

	can1: can at 80034000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can1_pins_a>;
		status = "okay";
	};

Yes, that is a solution.
But it may not so reasonable since dts file should represent hw, but here we have to do some trick by we know how driver works.
Maybe we need find a better solution.

Quoted 22 lines · Shawn Guo

Quoted 8 lines · Dong Aisheng

Quoted 8 lines

drivers/net/can/flexcan.c +8 -0

+		phy_stby_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+							"phy-standby-gpios",
+							0, &flags);
+		if (gpio_is_valid(phy_stby_gpio)) {
+			if (flags == OF_GPIO_ACTIVE_LOW)
+				phy_stby_high = false;
+			err = devm_gpio_request_one(&pdev->dev, phy_stby_gpio,
+						    GPIOF_DIR_OUT,

Quoted 9 lines

drivers/net/can/flexcan.c static int __devinit flexcan_probe(struct platform_device *pdev) · line 1002

Quoted 6 lines

drivers/net/can/flexcan.c +6 -0

+						    "phy-standby");
+			if (err) {
+				dev_err(&pdev->dev,
+					"failed to request gpio %d: %d\n",
+					phy_stby_gpio, err);
+				goto failed_gpio;

I checked mx28 evk, it seems the phy only has a STB gpio and shared by both CAN0&CAN1. I wonder the CAN1 probe may fail here.

It can be managed by dts. Here is what I have in imx28-evk.dts, where only can0 has phy-enable-gpios property.

Quoted 12 lines

	can0: can at 80032000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can0_pins_a>;
		phy-enable-gpios = <&gpio2 13 0>;
		status = "okay";
	};

	can1: can at 80034000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can1_pins_a>;
		status = "okay";
	};

Yes, that is a solution.
But it may not so reasonable since dts file should represent hw, but here we have to do some trick by we know how driver works.
Maybe we need find a better solution.

One more thing, for such dt representation, if can0 phy is disabled, can1 may be disabled wrongly too, right?

Quoted 8 lines · Dong Aisheng

Quoted 8 lines

drivers/net/can/flexcan.c +8 -0

+		phy_stby_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+							"phy-standby-gpios",
+							0, &flags);
+		if (gpio_is_valid(phy_stby_gpio)) {
+			if (flags == OF_GPIO_ACTIVE_LOW)
+				phy_stby_high = false;
+			err = devm_gpio_request_one(&pdev->dev, phy_stby_gpio,
+						    GPIOF_DIR_OUT,

Quoted 9 lines

drivers/net/can/flexcan.c static int __devinit flexcan_probe(struct platform_device *pdev) · line 1002

Quoted 6 lines

drivers/net/can/flexcan.c +6 -0

+						    "phy-standby");
+			if (err) {
+				dev_err(&pdev->dev,
+					"failed to request gpio %d: %d\n",
+					phy_stby_gpio, err);
+				goto failed_gpio;

I checked mx28 evk, it seems the phy only has a STB gpio and shared by both CAN0&CAN1. I wonder the CAN1 probe may fail here.

It can be managed by dts. Here is what I have in imx28-evk.dts, where only can0 has phy-enable-gpios property.

	can0: can at 80032000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can0_pins_a>;
		phy-enable-gpios = <&gpio2 13 0>;
		status = "okay";
	};

	can1: can at 80034000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can1_pins_a>;
		status = "okay";
	};

Quoted 22 lines · Shawn Guo

Quoted 8 lines · Dong Aisheng

Quoted 8 lines

drivers/net/can/flexcan.c +8 -0

+		phy_stby_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+							"phy-standby-gpios",
+							0, &flags);
+		if (gpio_is_valid(phy_stby_gpio)) {
+			if (flags == OF_GPIO_ACTIVE_LOW)
+				phy_stby_high = false;
+			err = devm_gpio_request_one(&pdev->dev, phy_stby_gpio,
+						    GPIOF_DIR_OUT,

Quoted 9 lines

drivers/net/can/flexcan.c static int __devinit flexcan_probe(struct platform_device *pdev) · line 1002

Quoted 6 lines

drivers/net/can/flexcan.c +6 -0

+						    "phy-standby");
+			if (err) {
+				dev_err(&pdev->dev,
+					"failed to request gpio %d: %d\n",
+					phy_stby_gpio, err);
+				goto failed_gpio;

I checked mx28 evk, it seems the phy only has a STB gpio and shared by both CAN0&CAN1. I wonder the CAN1 probe may fail here.

It can be managed by dts. Here is what I have in imx28-evk.dts, where only can0 has phy-enable-gpios property.

Quoted 12 lines

	can0: can at 80032000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can0_pins_a>;
		phy-enable-gpios = <&gpio2 13 0>;
		status = "okay";
	};

	can1: can at 80034000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can1_pins_a>;
		status = "okay";
	};

Will this work if can0 is down and can1 is up?

Can we abstract the transceiver power as a regulator? Or a clock? :P

Quoted 36 lines · Marc Kleine-Budde

Quoted 34 lines · Shawn Guo

Quoted 8 lines · Dong Aisheng

Quoted 8 lines

drivers/net/can/flexcan.c +8 -0

+		phy_stby_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+							"phy-standby-gpios",
+							0, &flags);
+		if (gpio_is_valid(phy_stby_gpio)) {
+			if (flags == OF_GPIO_ACTIVE_LOW)
+				phy_stby_high = false;
+			err = devm_gpio_request_one(&pdev->dev, phy_stby_gpio,
+						    GPIOF_DIR_OUT,

Quoted 9 lines

drivers/net/can/flexcan.c static int __devinit flexcan_probe(struct platform_device *pdev) · line 1002

Quoted 6 lines

drivers/net/can/flexcan.c +6 -0

+						    "phy-standby");
+			if (err) {
+				dev_err(&pdev->dev,
+					"failed to request gpio %d: %d\n",
+					phy_stby_gpio, err);
+				goto failed_gpio;

I checked mx28 evk, it seems the phy only has a STB gpio and shared by both CAN0&CAN1. I wonder the CAN1 probe may fail here.

It can be managed by dts. Here is what I have in imx28-evk.dts, where only can0 has phy-enable-gpios property.

	can0: can at 80032000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can0_pins_a>;
		phy-enable-gpios = <&gpio2 13 0>;
		status = "okay";
	};

	can1: can at 80034000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can1_pins_a>;
		status = "okay";
	};

Quoted 4 lines

Will this work if can0 is down and can1 is up?

Can we abstract the transceiver power as a regulator? Or a clock? :P

Hmm, it may not be a power.
For mx28evk, it's a STBY pin.
So it may hard to abstract it as a regulator or clock.

I have created a 'gpio-switch' driver in my private tree that can be used for exactly this purpose. It takes care of the pin polarity and initial state of the pins and can handle shared pins like in this case. The client driver does not even need to care whether an actual GPIO exists for the current platform.

I could prepare a patch and post it here.

Hi,

Quoted 37 lines · Dong Aisheng

Quoted 36 lines · Marc Kleine-Budde

Quoted 34 lines · Shawn Guo

Quoted 8 lines · Dong Aisheng

Quoted 8 lines

drivers/net/can/flexcan.c +8 -0

+		phy_stby_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+							"phy-standby-gpios",
+							0, &flags);
+		if (gpio_is_valid(phy_stby_gpio)) {
+			if (flags == OF_GPIO_ACTIVE_LOW)
+				phy_stby_high = false;
+			err = devm_gpio_request_one(&pdev->dev, phy_stby_gpio,
+						    GPIOF_DIR_OUT,

Quoted 9 lines

drivers/net/can/flexcan.c static int __devinit flexcan_probe(struct platform_device *pdev) · line 1002

Quoted 6 lines

drivers/net/can/flexcan.c +6 -0

+						    "phy-standby");
+			if (err) {
+				dev_err(&pdev->dev,
+					"failed to request gpio %d: %d\n",
+					phy_stby_gpio, err);
+				goto failed_gpio;

I checked mx28 evk, it seems the phy only has a STB gpio and shared by both CAN0&CAN1. I wonder the CAN1 probe may fail here.

It can be managed by dts. Here is what I have in imx28-evk.dts, where only can0 has phy-enable-gpios property.

	can0: can at 80032000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can0_pins_a>;
		phy-enable-gpios = <&gpio2 13 0>;
		status = "okay";
	};

	can1: can at 80034000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can1_pins_a>;
		status = "okay";
	};

Quoted 8 lines

Quoted 4 lines

Will this work if can0 is down and can1 is up?

Can we abstract the transceiver power as a regulator? Or a clock? :P

Hmm, it may not be a power.
For mx28evk, it's a STBY pin.
So it may hard to abstract it as a regulator or clock.

I have created a 'gpio-switch' driver in my private tree that can be used for exactly this purpose. It takes care of the pin polarity and initial state of the pins and can handle shared pins like in this case. The client driver does not even need to care whether an actual GPIO exists for the current platform.

I could prepare a patch and post it here.

Quoted 13 lines

Quoted 12 lines

	can0: can at 80032000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can0_pins_a>;
		phy-enable-gpios = <&gpio2 13 0>;
		status = "okay";
	};

	can1: can at 80034000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can1_pins_a>;
		status = "okay";
	};

Quoted 4 lines

Will this work if can0 is down and can1 is up?

Can we abstract the transceiver power as a regulator? Or a clock? :P

Hmm, it may not be a power.
For mx28evk, it's a STBY pin.
So it may hard to abstract it as a regulator or clock.

Quoted 36 lines · Marc Kleine-Budde

Quoted 34 lines · Shawn Guo

Quoted 8 lines · Dong Aisheng

Quoted 8 lines

drivers/net/can/flexcan.c +8 -0

+		phy_stby_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+							"phy-standby-gpios",
+							0, &flags);
+		if (gpio_is_valid(phy_stby_gpio)) {
+			if (flags == OF_GPIO_ACTIVE_LOW)
+				phy_stby_high = false;
+			err = devm_gpio_request_one(&pdev->dev, phy_stby_gpio,
+						    GPIOF_DIR_OUT,

Quoted 9 lines

drivers/net/can/flexcan.c static int __devinit flexcan_probe(struct platform_device *pdev) · line 1002

Quoted 6 lines

drivers/net/can/flexcan.c +6 -0

+						    "phy-standby");
+			if (err) {
+				dev_err(&pdev->dev,
+					"failed to request gpio %d: %d\n",
+					phy_stby_gpio, err);
+				goto failed_gpio;

I checked mx28 evk, it seems the phy only has a STB gpio and shared by both CAN0&CAN1. I wonder the CAN1 probe may fail here.

It can be managed by dts. Here is what I have in imx28-evk.dts, where only can0 has phy-enable-gpios property.

	can0: can at 80032000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can0_pins_a>;
		phy-enable-gpios = <&gpio2 13 0>;
		status = "okay";
	};

	can1: can at 80034000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can1_pins_a>;
		status = "okay";
	};

Quoted 4 lines

Will this work if can0 is down and can1 is up?

Can we abstract the transceiver power as a regulator? Or a clock? :P

Hmm, it may not be a power.
For mx28evk, it's a STBY pin.
So it may hard to abstract it as a regulator or clock.

Hi Shawn,

Any progress on this patch, has it been merged?

adding linux-can on Cc.

Quoted 4 lines · Hui Wang

Hi Shawn,

Any progress on this patch, has it been merged?

No, for several reasons:
- I don't want to have a "toggle transceiver gpio" function open coded in every CAN driver. For now it's just flexcan, but on at91 we need the same functionality.
- The transceiver may be shared by several CAN cores, to sharing must be designed in.
- Lothar Waßmann posted a more generic solution:
http://patchwork.ozlabs.org/patch/168023/

Marc