[PATCH v2 0/2] flexcan driver updates

[PATCH v2 0/2] flexcan driver updates

[Shawn Guo]

Changes since v1:
* Add phy-standby-gpios support
* Remove enable-active-low and use of_get_named_gpio_flags instead
* Use devm_gpio_request_one instead of gpio_request_one

Shawn Guo (2):
  net: flexcan: clock-frequency is optional for device tree probe
  net: flexcan: add transceiver switch gpios support

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

-- 
1.7.5.4

[PATCH v2 0/2] flexcan driver updates

[Shawn Guo]

Changes since v1:
* Add phy-standby-gpios support
* Remove enable-active-low and use of_get_named_gpio_flags instead
* Use devm_gpio_request_one instead of gpio_request_one

Shawn Guo (2):
  net: flexcan: clock-frequency is optional for device tree probe
  net: flexcan: add transceiver switch gpios support

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

-- 
1.7.5.4

[PATCH v2 1/2] net: flexcan: clock-frequency is optional for device tree probe

[Shawn Guo]

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 <shawn.guo@linaro.org>
---
 .../devicetree/bindings/net/can/fsl-flexcan.txt    |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
index f31b686..8ff324e 100644
--- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
+++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
@@ -11,6 +11,9 @@ Required properties:
 
 - reg : Offset and length of the register set for this device
 - interrupts : Interrupt tuple for this device
+
+Optional properties:
+
 - clock-frequency : The oscillator frequency driving the flexcan device
 
 Example:
-- 
1.7.5.4

[PATCH v2 1/2] net: flexcan: clock-frequency is optional for device tree probe

[Shawn Guo]

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 <shawn.guo@linaro.org>
---
 .../devicetree/bindings/net/can/fsl-flexcan.txt    |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
index f31b686..8ff324e 100644
--- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
+++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
@@ -11,6 +11,9 @@ Required properties:
 
 - reg : Offset and length of the register set for this device
 - interrupts : Interrupt tuple for this device
+
+Optional properties:
+
 - clock-frequency : The oscillator frequency driving the flexcan device
 
 Example:
-- 
1.7.5.4

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[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.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 .../devicetree/bindings/net/can/fsl-flexcan.txt    |    2 +
 drivers/net/can/flexcan.c                          |   62 ++++++++++++++++++++
 2 files changed, 64 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
index 8ff324e..e0dbac7 100644
--- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
+++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
@@ -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:
 
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 38c0690..1ce3f9e 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -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,
 						"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;
+			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;
 }
 
-- 
1.7.5.4

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[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.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 .../devicetree/bindings/net/can/fsl-flexcan.txt    |    2 +
 drivers/net/can/flexcan.c                          |   62 ++++++++++++++++++++
 2 files changed, 64 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
index 8ff324e..e0dbac7 100644
--- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
+++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
@@ -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:
 
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 38c0690..1ce3f9e 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -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,
 						"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;
+			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;
 }
 
-- 
1.7.5.4

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> 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 <shawn.guo@linaro.org>
> ---
>  .../devicetree/bindings/net/can/fsl-flexcan.txt    |    2 +
>  drivers/net/can/flexcan.c                          |   62 ++++++++++++++++++++
>  2 files changed, 64 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> index 8ff324e..e0dbac7 100644
> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> @@ -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
>  
Why the plural form 'gpios'? It's just one gpio.
s/gpios/gpio/ ?


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> 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 <shawn.guo@linaro.org>
> ---
>  .../devicetree/bindings/net/can/fsl-flexcan.txt    |    2 +
>  drivers/net/can/flexcan.c                          |   62 ++++++++++++++++++++
>  2 files changed, 64 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> index 8ff324e..e0dbac7 100644
> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> @@ -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
>  
Why the plural form 'gpios'? It's just one gpio.
s/gpios/gpio/ ?


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 07:22:14AM +0200, Lothar Waßmann wrote:
> > +- 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/ ?
> 
It's a common pattern suggested by common gpio bindings
Documentation/devicetree/bindings/gpio/gpio.txt.

-- 
Regards,
Shawn

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 07:22:14AM +0200, Lothar Wa?mann wrote:
> > +- 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/ ?
> 
It's a common pattern suggested by common gpio bindings
Documentation/devicetree/bindings/gpio/gpio.txt.

-- 
Regards,
Shawn

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Hui Wang]

Shawn Guo wrote:
> 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 <shawn.guo@linaro.org>
> ---
>  .../devicetree/bindings/net/can/fsl-flexcan.txt    |    2 +
>  drivers/net/can/flexcan.c                          |   62 ++++++++++++++++++++
>  2 files changed, 64 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> index 8ff324e..e0dbac7 100644
> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> @@ -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.

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

> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 38c0690..1ce3f9e 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -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.
>  #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>
>  
>   

Other looks fine to me.

Regards,
Hui.

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Hui Wang]

Shawn Guo wrote:
> 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 <shawn.guo@linaro.org>
> ---
>  .../devicetree/bindings/net/can/fsl-flexcan.txt    |    2 +
>  drivers/net/can/flexcan.c                          |   62 ++++++++++++++++++++
>  2 files changed, 64 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> index 8ff324e..e0dbac7 100644
> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> @@ -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.

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

> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 38c0690..1ce3f9e 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -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.
>  #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>
>  
>   

Other looks fine to me.

Regards,
Hui.

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 02:14:39PM +0800, Hui Wang wrote:
> >diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >index 8ff324e..e0dbac7 100644
> >--- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >+++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >@@ -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.
> 
> +        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.

> >diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> >index 38c0690..1ce3f9e 100644
> >--- a/drivers/net/can/flexcan.c
> >+++ b/drivers/net/can/flexcan.c
> >@@ -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.

-- 
Regards,
Shawn

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 02:14:39PM +0800, Hui Wang wrote:
> >diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >index 8ff324e..e0dbac7 100644
> >--- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >+++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >@@ -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.
> 
> +        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.

> >diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> >index 38c0690..1ce3f9e 100644
> >--- a/drivers/net/can/flexcan.c
> >+++ b/drivers/net/can/flexcan.c
> >@@ -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.

-- 
Regards,
Shawn

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Hui Wang]

Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 02:14:39PM +0800, Hui Wang wrote:
>   
>>> diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>> index 8ff324e..e0dbac7 100644
>>> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>> +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>> @@ -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.
>>
>> +        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.
>>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>>> index 38c0690..1ce3f9e 100644
>>> --- a/drivers/net/can/flexcan.c
>>> +++ b/drivers/net/can/flexcan.c
>>> @@ -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.
>
>   
Got it.

OK to me.

Regards,
Hui.

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Hui Wang]

Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 02:14:39PM +0800, Hui Wang wrote:
>   
>>> diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>> index 8ff324e..e0dbac7 100644
>>> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>> +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>> @@ -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.
>>
>> +        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.
>>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>>> index 38c0690..1ce3f9e 100644
>>> --- a/drivers/net/can/flexcan.c
>>> +++ b/drivers/net/can/flexcan.c
>>> @@ -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.
>
>   
Got it.

OK to me.

Regards,
Hui.

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Marc Kleine-Budde]

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

On 06/28/2012 05:21 AM, Shawn Guo wrote:
> 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.

> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  .../devicetree/bindings/net/can/fsl-flexcan.txt    |    2 +
>  drivers/net/can/flexcan.c                          |   62 ++++++++++++++++++++
>  2 files changed, 64 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> index 8ff324e..e0dbac7 100644
> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> @@ -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:
>  
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 38c0690..1ce3f9e 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -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,
>  						"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.

> +			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;
>  }
>  


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Marc Kleine-Budde]

On 06/28/2012 05:21 AM, Shawn Guo wrote:
> 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.

> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  .../devicetree/bindings/net/can/fsl-flexcan.txt    |    2 +
>  drivers/net/can/flexcan.c                          |   62 ++++++++++++++++++++
>  2 files changed, 64 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> index 8ff324e..e0dbac7 100644
> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> @@ -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:
>  
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 38c0690..1ce3f9e 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -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,
>  						"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.

> +			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;
>  }
>  


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120628/9b38782a/attachment.sig>

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Marc Kleine-Budde]

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

On 06/28/2012 09:01 AM, Hui Wang wrote:
> Shawn Guo wrote:
>> On Thu, Jun 28, 2012 at 02:14:39PM +0800, Hui Wang wrote:
>>  
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>> b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>> index 8ff324e..e0dbac7 100644
>>>> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>> +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>> @@ -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.
>>>
>>> +        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.
>>>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>>>> index 38c0690..1ce3f9e 100644
>>>> --- a/drivers/net/can/flexcan.c
>>>> +++ b/drivers/net/can/flexcan.c
>>>> @@ -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.
>>
>>   
> Got it.
> 
> OK to me.

Is this an Acked-by?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Marc Kleine-Budde]

On 06/28/2012 09:01 AM, Hui Wang wrote:
> Shawn Guo wrote:
>> On Thu, Jun 28, 2012 at 02:14:39PM +0800, Hui Wang wrote:
>>  
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>> b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>> index 8ff324e..e0dbac7 100644
>>>> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>> +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>> @@ -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.
>>>
>>> +        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.
>>>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>>>> index 38c0690..1ce3f9e 100644
>>>> --- a/drivers/net/can/flexcan.c
>>>> +++ b/drivers/net/can/flexcan.c
>>>> @@ -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.
>>
>>   
> Got it.
> 
> OK to me.

Is this an Acked-by?

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120628/b1b05659/attachment.sig>

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 12:31:56PM +0200, Marc Kleine-Budde wrote:
> 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.

> > +	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,
> >  						"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.
> 
Ok, we will have these two bool variables named phy_en_active_low and
phy_stby_active_low.  Will resend to change them.

-- 
Regards,
Shawn

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 12:31:56PM +0200, Marc Kleine-Budde wrote:
> 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.

> > +	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,
> >  						"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.
> 
Ok, we will have these two bool variables named phy_en_active_low and
phy_stby_active_low.  Will resend to change them.

-- 
Regards,
Shawn

Re: [PATCH v2 1/2] net: flexcan: clock-frequency is optional for device tree probe

[Dong Aisheng]

On Thu, Jun 28, 2012 at 11:21:40AM +0800, Shawn Guo wrote:
> 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 <shawn.guo@linaro.org>

Acked-by: Dong Aisheng <dong.aisheng@linaro.org>

Regards
Dong Aisheng

[PATCH v2 1/2] net: flexcan: clock-frequency is optional for device tree probe

[Dong Aisheng]

On Thu, Jun 28, 2012 at 11:21:40AM +0800, Shawn Guo wrote:
> 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 <shawn.guo@linaro.org>

Acked-by: Dong Aisheng <dong.aisheng@linaro.org>

Regards
Dong Aisheng

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Marc Kleine-Budde]

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

On 06/28/2012 01:21 PM, Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 12:31:56PM +0200, Marc Kleine-Budde wrote:
>> 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.

Marc

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Marc Kleine-Budde]

On 06/28/2012 01:21 PM, Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 12:31:56PM +0200, Marc Kleine-Budde wrote:
>> 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.

Marc

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120628/75c661b1/attachment.sig>

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Dong Aisheng]

On Thu, Jun 28, 2012 at 11:21:41AM +0800, Shawn Guo wrote:
> 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, ideally these things are not so suitable to be put in controller driver
since they're platform specific.
However i also did not have better idea now.

One rough thought is maybe create a supported can phy libs and hook to controller
dynamically?

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

> +			}
> +		}
>  	}
>  
>  	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;
>  }
>  

Regards
Dong Aisheng

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Dong Aisheng]

On Thu, Jun 28, 2012 at 11:21:41AM +0800, Shawn Guo wrote:
> 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, ideally these things are not so suitable to be put in controller driver
since they're platform specific.
However i also did not have better idea now.

One rough thought is maybe create a supported can phy libs and hook to controller
dynamically?

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

> +			}
> +		}
>  	}
>  
>  	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;
>  }
>  

Regards
Dong Aisheng

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Dong Aisheng]

On Thu, Jun 28, 2012 at 12:31:56PM +0200, Marc Kleine-Budde wrote:
> On 06/28/2012 05:21 AM, Shawn Guo wrote:
> > 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.
> 
Hmm, i also had this wondering.
I quote some info from the spec like:
"The MC33902 is a high speed CAN physical interface. The device 
includes an internal 5.0 V supply for the CAN bus transceiver, and 
requires only a connection to a battery line."

Maybe transceiver is more close to spec.

Regards
Dong Aisheng

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Dong Aisheng]

On Thu, Jun 28, 2012 at 12:31:56PM +0200, Marc Kleine-Budde wrote:
> On 06/28/2012 05:21 AM, Shawn Guo wrote:
> > 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.
> 
Hmm, i also had this wondering.
I quote some info from the spec like:
"The MC33902 is a high speed CAN physical interface. The device 
includes an internal 5.0 V supply for the CAN bus transceiver, and 
requires only a connection to a battery line."

Maybe transceiver is more close to spec.

Regards
Dong Aisheng

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
> 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.

-- 
Regards,
Shawn

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
> 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.

-- 
Regards,
Shawn

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
> > +		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;
> 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@80032000 {
		pinctrl-names = "default";
		pinctrl-0 = <&can0_pins_a>;
		phy-enable-gpios = <&gpio2 13 0>;
		status = "okay";
	};

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


-- 
Regards,
Shawn

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
> > +		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;
> 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";
	};


-- 
Regards,
Shawn

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Dong Aisheng]

On Thu, Jun 28, 2012 at 07:46:02PM +0800, Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
> > > +		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;
> > 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@80032000 {
> 		pinctrl-names = "default";
> 		pinctrl-0 = <&can0_pins_a>;
> 		phy-enable-gpios = <&gpio2 13 0>;
> 		status = "okay";
> 	};
> 
> 	can1: can@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.

Regards
Dong Aisheng

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Dong Aisheng]

On Thu, Jun 28, 2012 at 07:46:02PM +0800, Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
> > > +		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;
> > 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.

Regards
Dong Aisheng

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 07:41:10PM +0800, Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
> > 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 :)

-- 
Regards,
Shawn

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 07:41:10PM +0800, Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
> > 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 :)

-- 
Regards,
Shawn

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 07:48:16PM +0800, Dong Aisheng wrote:
> 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.
> 
I wouldn't agree that driver should take care of all such kind of board
weirdness.

-- 
Regards,
Shawn

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 07:48:16PM +0800, Dong Aisheng wrote:
> 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.
> 
I wouldn't agree that driver should take care of all such kind of board
weirdness.

-- 
Regards,
Shawn

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Dong Aisheng]

On Thu, Jun 28, 2012 at 07:48:16PM +0800, Dong Aisheng wrote:
> On Thu, Jun 28, 2012 at 07:46:02PM +0800, Shawn Guo wrote:
> > On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
> > > > +		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;
> > > 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@80032000 {
> > 		pinctrl-names = "default";
> > 		pinctrl-0 = <&can0_pins_a>;
> > 		phy-enable-gpios = <&gpio2 13 0>;
> > 		status = "okay";
> > 	};
> > 
> > 	can1: can@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?

Regards
Dong Aisheng

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Dong Aisheng]

On Thu, Jun 28, 2012 at 07:48:16PM +0800, Dong Aisheng wrote:
> On Thu, Jun 28, 2012 at 07:46:02PM +0800, Shawn Guo wrote:
> > On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
> > > > +		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;
> > > 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.
> 
One more thing, for such dt representation, if can0 phy is disabled,
can1 may be disabled wrongly too, right?

Regards
Dong Aisheng

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Marc Kleine-Budde]

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

On 06/28/2012 01:46 PM, Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
>>> +		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;
>> 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@80032000 {
> 		pinctrl-names = "default";
> 		pinctrl-0 = <&can0_pins_a>;
> 		phy-enable-gpios = <&gpio2 13 0>;
> 		status = "okay";
> 	};
> 
> 	can1: can@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

Marc

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Marc Kleine-Budde]

On 06/28/2012 01:46 PM, Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
>>> +		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;
>> 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";
> 	};

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

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

Marc

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120628/6de502a1/attachment.sig>

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 07:52:26PM +0800, Shawn Guo wrote:
> 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 :)
> 
Oh, even longer, since we have agreed "high" should be renamed to
"active_low":

	bool transceiver_en_active_low;
	bool transceiver_stby_active_low;

Can we keep using "phy" to have the names a little bit shorter?

-- 
Regards,
Shawn

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 07:52:26PM +0800, Shawn Guo wrote:
> 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 :)
> 
Oh, even longer, since we have agreed "high" should be renamed to
"active_low":

	bool transceiver_en_active_low;
	bool transceiver_stby_active_low;

Can we keep using "phy" to have the names a little bit shorter?

-- 
Regards,
Shawn

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
> > 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.
> 
A transceiver is just a dumb piece of hardware, while a PHY contains
some intelligence of its own.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
> > 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.
> 
A transceiver is just a dumb piece of hardware, while a PHY contains
some intelligence of its own.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Kurt Van Dijck]

On Thu, Jun 28, 2012 at 07:41:10PM +0800, Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
> > 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.

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

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

Kurt

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Kurt Van Dijck]

On Thu, Jun 28, 2012 at 07:41:10PM +0800, Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
> > 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.

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

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

Kurt

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Marc Kleine-Budde]

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

On 06/28/2012 02:08 PM, Kurt Van Dijck wrote:
> On Thu, Jun 28, 2012 at 07:41:10PM +0800, Shawn Guo wrote:
>> On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
>>> 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.
> 
> 'trx' seemed the right abbreviation
> http://www.acronymfinder.com/TRX.html
> 
> thanks to Oliver, in his email on May 9th.

I was about to propose CamelCase to keep the variable names shorter:

bool transceiverStbyActiveLow;

scnr, Marc

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Marc Kleine-Budde]

On 06/28/2012 02:08 PM, Kurt Van Dijck wrote:
> On Thu, Jun 28, 2012 at 07:41:10PM +0800, Shawn Guo wrote:
>> On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
>>> 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.
> 
> 'trx' seemed the right abbreviation
> http://www.acronymfinder.com/TRX.html
> 
> thanks to Oliver, in his email on May 9th.

I was about to propose CamelCase to keep the variable names shorter:

bool transceiverStbyActiveLow;

scnr, Marc

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120628/667b8137/attachment.sig>

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Dong Aisheng]

On Thu, Jun 28, 2012 at 08:05:24PM +0800, Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 07:52:26PM +0800, Shawn Guo wrote:
> > 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 :)
> > 
> Oh, even longer, since we have agreed "high" should be renamed to
> "active_low":
> 
> 	bool transceiver_en_active_low;
> 	bool transceiver_stby_active_low;
> 
How about just active_low since they're just temp variables?

And if you do not assign them together at last, you may only need:
int gpio;
bool active_high;

That saves you two variables.

Regards
Dong Aisheng

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Dong Aisheng]

On Thu, Jun 28, 2012 at 08:05:24PM +0800, Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 07:52:26PM +0800, Shawn Guo wrote:
> > 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 :)
> > 
> Oh, even longer, since we have agreed "high" should be renamed to
> "active_low":
> 
> 	bool transceiver_en_active_low;
> 	bool transceiver_stby_active_low;
> 
How about just active_low since they're just temp variables?

And if you do not assign them together at last, you may only need:
int gpio;
bool active_high;

That saves you two variables.

Regards
Dong Aisheng

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 02:07:57PM +0200, Lothar Waßmann wrote:
> Hi,
> 
> Shawn Guo writes:
> > On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
> > > 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.
> > 
> 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.

-- 
Regards,
Shawn

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 02:07:57PM +0200, Lothar Wa?mann wrote:
> Hi,
> 
> Shawn Guo writes:
> > On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
> > > 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.
> > 
> 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.

-- 
Regards,
Shawn

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 02:08:35PM +0200, Kurt Van Dijck wrote:
> 'trx' seemed the right abbreviation
> http://www.acronymfinder.com/TRX.html
> 
sounds good.

-- 
Regards,
Shawn

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 02:08:35PM +0200, Kurt Van Dijck wrote:
> 'trx' seemed the right abbreviation
> http://www.acronymfinder.com/TRX.html
> 
sounds good.

-- 
Regards,
Shawn

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Dong Aisheng]

On Thu, Jun 28, 2012 at 08:05:14PM +0800, Marc Kleine-Budde wrote:
> On 06/28/2012 01:46 PM, Shawn Guo wrote:
> > On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
> >>> +		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;
> >> 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@80032000 {
> > 		pinctrl-names = "default";
> > 		pinctrl-0 = <&can0_pins_a>;
> > 		phy-enable-gpios = <&gpio2 13 0>;
> > 		status = "okay";
> > 	};
> > 
> > 	can1: can@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
> 
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.

Regards
Dong Aisheng

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Dong Aisheng]

On Thu, Jun 28, 2012 at 08:05:14PM +0800, Marc Kleine-Budde wrote:
> On 06/28/2012 01:46 PM, Shawn Guo wrote:
> > On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
> >>> +		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;
> >> 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";
> > 	};
> 
> 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.

Regards
Dong Aisheng

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Lothar Waßmann]

Shawn Guo writes:
> On Thu, Jun 28, 2012 at 07:41:10PM +0800, Shawn Guo wrote:
> > On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
> > > 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 :)
> 
'xcvr' is a common acronym for 'transceiver'.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Lothar Waßmann]

Shawn Guo writes:
> On Thu, Jun 28, 2012 at 07:41:10PM +0800, Shawn Guo wrote:
> > On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
> > > 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 :)
> 
'xcvr' is a common acronym for 'transceiver'.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 08:02:46PM +0800, Dong Aisheng wrote:
> One more thing, for such dt representation, if can0 phy is disabled,
> can1 may be disabled wrongly too, right?
> 
Right.  Looks like we have to take care of the weirdness then.

-- 
Regards,
Shawn

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 08:02:46PM +0800, Dong Aisheng wrote:
> One more thing, for such dt representation, if can0 phy is disabled,
> can1 may be disabled wrongly too, right?
> 
Right.  Looks like we have to take care of the weirdness then.

-- 
Regards,
Shawn

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On Thu, Jun 28, 2012 at 02:07:57PM +0200, Lothar Waßmann wrote:
> > Hi,
> > 
> > Shawn Guo writes:
> > > On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
> > > > 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.
> > > 
> > 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
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On Thu, Jun 28, 2012 at 02:07:57PM +0200, Lothar Wa?mann wrote:
> > Hi,
> > 
> > Shawn Guo writes:
> > > On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
> > > > 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.
> > > 
> > 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
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Lothar Waßmann]

Hi,

Dong Aisheng writes:
> On Thu, Jun 28, 2012 at 08:05:14PM +0800, Marc Kleine-Budde wrote:
> > On 06/28/2012 01:46 PM, Shawn Guo wrote:
> > > On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
> > >>> +		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;
> > >> 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@80032000 {
> > > 		pinctrl-names = "default";
> > > 		pinctrl-0 = <&can0_pins_a>;
> > > 		phy-enable-gpios = <&gpio2 13 0>;
> > > 		status = "okay";
> > > 	};
> > > 
> > > 	can1: can@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
> > 
> 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.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Lothar Waßmann]

Hi,

Dong Aisheng writes:
> On Thu, Jun 28, 2012 at 08:05:14PM +0800, Marc Kleine-Budde wrote:
> > On 06/28/2012 01:46 PM, Shawn Guo wrote:
> > > On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
> > >>> +		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;
> > >> 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";
> > > 	};
> > 
> > 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.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 08:18:18PM +0800, Dong Aisheng wrote:
> > > 	can0: can@80032000 {
> > > 		pinctrl-names = "default";
> > > 		pinctrl-0 = <&can0_pins_a>;
> > > 		phy-enable-gpios = <&gpio2 13 0>;
> > > 		status = "okay";
> > > 	};
> > > 
> > > 	can1: can@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
> > 
> 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.
> 
Yes.  It also reminds an error in the dts above.  phy-standby-gpios
should be used instead.

-- 
Regards,
Shawn

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 08:18:18PM +0800, Dong Aisheng wrote:
> > > 	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
> > 
> 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.
> 
Yes.  It also reminds an error in the dts above.  phy-standby-gpios
should be used instead.

-- 
Regards,
Shawn

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 02:32:22PM +0200, Lothar Waßmann wrote:
> 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.
> 
That would be great.

-- 
Regards,
Shawn

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Shawn Guo]

On Thu, Jun 28, 2012 at 02:32:22PM +0200, Lothar Wa?mann wrote:
> 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.
> 
That would be great.

-- 
Regards,
Shawn

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Dong Aisheng]

On Thu, Jun 28, 2012 at 08:32:22PM +0800, Lothar Waßmann wrote:
> Hi,
> 
> Dong Aisheng writes:
> > On Thu, Jun 28, 2012 at 08:05:14PM +0800, Marc Kleine-Budde wrote:
> > > On 06/28/2012 01:46 PM, Shawn Guo wrote:
> > > > On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
> > > >>> +		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;
> > > >> 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@80032000 {
> > > > 		pinctrl-names = "default";
> > > > 		pinctrl-0 = <&can0_pins_a>;
> > > > 		phy-enable-gpios = <&gpio2 13 0>;
> > > > 		status = "okay";
> > > > 	};
> > > > 
> > > > 	can1: can@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
> > > 
> > 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.
> 
Would be glad to see your patches.
It may be useful, not only for this case,
we also meet some other issues, i'm not sure but may have similar requirement
like SDIO WiFi reset, usb phy reset and etc.

Regards
Dong Aisheng

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Dong Aisheng]

On Thu, Jun 28, 2012 at 08:32:22PM +0800, Lothar Wa?mann wrote:
> Hi,
> 
> Dong Aisheng writes:
> > On Thu, Jun 28, 2012 at 08:05:14PM +0800, Marc Kleine-Budde wrote:
> > > On 06/28/2012 01:46 PM, Shawn Guo wrote:
> > > > On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
> > > >>> +		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;
> > > >> 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";
> > > > 	};
> > > 
> > > 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.
> 
Would be glad to see your patches.
It may be useful, not only for this case,
we also meet some other issues, i'm not sure but may have similar requirement
like SDIO WiFi reset, usb phy reset and etc.

Regards
Dong Aisheng

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Marc Kleine-Budde]

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

On 06/28/2012 02:18 PM, Dong Aisheng wrote:
> On Thu, Jun 28, 2012 at 08:05:14PM +0800, Marc Kleine-Budde wrote:
>> On 06/28/2012 01:46 PM, Shawn Guo wrote:
>>> On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
>>>>> +		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;
>>>> 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@80032000 {
>>> 		pinctrl-names = "default";
>>> 		pinctrl-0 = <&can0_pins_a>;
>>> 		phy-enable-gpios = <&gpio2 13 0>;
>>> 		status = "okay";
>>> 	};
>>>
>>> 	can1: can@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
>>
> 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 just talked to Sascha, He pointed out, that there already is regulator
that toggles a gpio (only one, not several). I haven't looked at the
details, but a regulator should solve the "two CAN ports share the same
transceiver/phy" problem.

Marc

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Marc Kleine-Budde]

On 06/28/2012 02:18 PM, Dong Aisheng wrote:
> On Thu, Jun 28, 2012 at 08:05:14PM +0800, Marc Kleine-Budde wrote:
>> On 06/28/2012 01:46 PM, Shawn Guo wrote:
>>> On Thu, Jun 28, 2012 at 07:33:28PM +0800, Dong Aisheng wrote:
>>>>> +		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;
>>>> 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";
>>> 	};
>>
>> 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 just talked to Sascha, He pointed out, that there already is regulator
that toggles a gpio (only one, not several). I haven't looked at the
details, but a regulator should solve the "two CAN ports share the same
transceiver/phy" problem.

Marc

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120629/6732d95d/attachment.sig>

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Hui Wang]

Lothar Waßmann wrote:
> Hi,
>
> Shawn Guo writes:
>   
>> On Thu, Jun 28, 2012 at 02:07:57PM +0200, Lothar Waßmann wrote:
>>     
>>> Hi,
>>>
>>> Shawn Guo writes:
>>>       
>>>> On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
>>>>         
>>>>> 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.
>>>>
>>>>         
>>> 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.
>
>   
Sorry for reply late, in my first patch, i chose "phy" instead of "xcvr" 
because the MC33902 datasheet tell me it is a "high speed CAN physical 
interface", and it includes "an internal 5.0 V supply for the CAN bus 
transceiver".

And from the diagram in the page 1 of the MC33902 datasheet, the MC33902 
includes bus xcvr, i/o control logic, power supply and external 
regulator control logic. As a result i decided to use phy in the driver.

Regards,
Hui.
> Lothar Waßmann
>   

[PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Hui Wang]

Lothar Wa?mann wrote:
> Hi,
>
> Shawn Guo writes:
>   
>> On Thu, Jun 28, 2012 at 02:07:57PM +0200, Lothar Wa?mann wrote:
>>     
>>> Hi,
>>>
>>> Shawn Guo writes:
>>>       
>>>> On Thu, Jun 28, 2012 at 01:29:19PM +0200, Marc Kleine-Budde wrote:
>>>>         
>>>>> 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.
>>>>
>>>>         
>>> 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.
>
>   
Sorry for reply late, in my first patch, i chose "phy" instead of "xcvr" 
because the MC33902 datasheet tell me it is a "high speed CAN physical 
interface", and it includes "an internal 5.0 V supply for the CAN bus 
transceiver".

And from the diagram in the page 1 of the MC33902 datasheet, the MC33902 
includes bus xcvr, i/o control logic, power supply and external 
regulator control logic. As a result i decided to use phy in the driver.

Regards,
Hui.
> Lothar Wa?mann
>   

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Marc Kleine-Budde]

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

adding linux-can on Cc.

On 08/08/2012 10:43 AM, Hui Wang wrote:
> 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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH v2 2/2] net: flexcan: add transceiver switch gpios support

[Hui Wang]

Marc Kleine-Budde wrote:
> adding linux-can on Cc.
>
> On 08/08/2012 10:43 AM, Hui Wang wrote:
>   
>> 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/
>
>   
Glad to know it, thanks.

Hui.
> Marc
>
>