can: c_can: Add suspend/resume and pinctrl support

Adopt pinctrl support to c_can driver based on c_can device pointer, pinctrl driver configure SoC pins to d_can mode according to definitions provided in .dts file.

In device specific device tree file 'pinctrl-names = "default";' and 'pinctrl-0 = <&d_can1_pins>;' needs to add to configure pins from c_can driver. d_can1_pins node contains the pinmux/config details of d_can L/H pins.

Signed-off-by AnilKumar Ch <[email protected]>

---
 drivers/net/can/c_can/c_can_platform.c |    7 +++++++
 1 file changed, 7 insertions(+)

drivers/net/can/c_can/c_can_platform.c +7 -0

index 90801c4..c351975 100644
@@ -33,6 +33,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/pinctrl/consumer.h>
 
 #include <linux/can/dev.h>
 

@@ -98,6 +99,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 	struct c_can_priv *priv;
 	const struct of_device_id *match;
 	const struct platform_device_id *id;
+	struct pinctrl *pinctrl;
 	struct resource *mem;
 	int irq;
 	struct clk *clk;

@@ -114,6 +116,11 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 		id = platform_get_device_id(pdev);
 	}
 
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		dev_warn(&pdev->dev,
+			"failed to configure pins from driver\n");
+
 	/* get the appropriate clk */
 	clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk)) {

Quoted 4 lines · AnilKumar Ch

Adopt pinctrl support to c_can driver based on c_can device pointer, pinctrl driver configure SoC pins to d_can mode according to definitions provided in .dts file.

Quoted 4 lines

In device specific device tree file 'pinctrl-names = "default";' and 'pinctrl-0 = <&d_can1_pins>;' needs to add to configure pins from c_can driver. d_can1_pins node contains the pinmux/config details of d_can L/H pins.

Looks good, is the pinctrl property documented in the dt bindings already?

Quoted 40 lines

Signed-off-by AnilKumar Ch <[email protected]>

---
 drivers/net/can/c_can/c_can_platform.c |    7 +++++++
 1 file changed, 7 insertions(+)

drivers/net/can/c_can/c_can_platform.c +7 -0

index 90801c4..c351975 100644
@@ -33,6 +33,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/pinctrl/consumer.h>
 
 #include <linux/can/dev.h>
 

@@ -98,6 +99,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 	struct c_can_priv *priv;
 	const struct of_device_id *match;
 	const struct platform_device_id *id;
+	struct pinctrl *pinctrl;
 	struct resource *mem;
 	int irq;
 	struct clk *clk;

@@ -114,6 +116,11 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 		id = platform_get_device_id(pdev);
 	}
 
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		dev_warn(&pdev->dev,
+			"failed to configure pins from driver\n");
+
 	/* get the appropriate clk */
 	clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk)) {

Signature · 4 lines

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   |

Hi Marc,

Quoted 10 lines · Marc Kleine-Budde

Quoted 8 lines · AnilKumar Ch

Adopt pinctrl support to c_can driver based on c_can device pointer, pinctrl driver configure SoC pins to d_can mode according to definitions provided in .dts file.

In device specific device tree file 'pinctrl-names = "default";' and 'pinctrl-0 = <&d_can1_pins>;' needs to add to configure pins from c_can driver. d_can1_pins node contains the pinmux/config details of d_can L/H pins.

Quoted 2 lines

Looks good, is the pinctrl property documented in the dt bindings already?

Yes, its here Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt

Quoted 13 lines · Chimata

Hi Marc,

Quoted 10 lines · Marc Kleine-Budde

Quoted 8 lines · AnilKumar Ch

Adopt pinctrl support to c_can driver based on c_can device pointer, pinctrl driver configure SoC pins to d_can mode according to definitions provided in .dts file.

In device specific device tree file 'pinctrl-names = "default";' and 'pinctrl-0 = <&d_can1_pins>;' needs to add to configure pins from c_can driver. d_can1_pins node contains the pinmux/config details of d_can L/H pins.

Quoted 4 lines

Quoted 2 lines

Looks good, is the pinctrl property documented in the dt bindings already?

Yes, its here Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt

Sorry, I meant not the generic pinctrl, but the c_can/d_can bindings documentation (Documentation/devicetree/bindings/net/can/).

Add D_CAN raminit support to C_CAN driver to enable D_CAN RAM. DCAN RAM holds all the message objects during transmission or receiving of data. This initialization/de-initialization should be done in synchronous with D_CAN clock.

Signed-off-by AnilKumar Ch <[email protected]>

---
 drivers/net/can/c_can/c_can.c          |   13 ++++++++++++
 drivers/net/can/c_can/c_can.h          |    2 ++
 drivers/net/can/c_can/c_can_platform.c |   10 +++++++++
 include/linux/can/platform/c_can.h     |   36 ++++++++++++++++++++++++++++++++
 4 files changed, 61 insertions(+)
 create mode 100644 include/linux/can/platform/c_can.h

drivers/net/can/c_can/c_can.c +13 -0

index aa6c5eb..c175410 100644
@@ -214,6 +214,12 @@ static inline void c_can_pm_runtime_put_sync(const struct c_can_priv *priv)
 		pm_runtime_put_sync(priv->device);
 }
 
+static inline void c_can_reset_ram(const struct c_can_priv *priv, bool enable)
+{
+	if (priv->ram_init)
+		priv->ram_init(priv->instance, enable);
+}
+
 static inline int get_tx_next_msg_obj(const struct c_can_priv *priv)
 {
 	return (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) +

@@ -1071,6 +1077,8 @@ static int c_can_open(struct net_device *dev)
 	struct c_can_priv *priv = netdev_priv(dev);
 
 	c_can_pm_runtime_get_sync(priv);
+	/* Initialize DCAN RAM */
+	c_can_reset_ram(priv, true);
 
 	/* open the can device */
 	err = open_candev(dev);

@@ -1099,6 +1107,8 @@ static int c_can_open(struct net_device *dev)
 exit_irq_fail:
 	close_candev(dev);
 exit_open_fail:
+	/* De-Initialize DCAN RAM */
+	c_can_reset_ram(priv, false);
 	c_can_pm_runtime_put_sync(priv);
 	return err;
 }

@@ -1112,6 +1122,9 @@ static int c_can_close(struct net_device *dev)
 	c_can_stop(dev);
 	free_irq(dev->irq, dev);
 	close_candev(dev);
+
+	/* De-Initialize DCAN RAM */
+	c_can_reset_ram(priv, false);
 	c_can_pm_runtime_put_sync(priv);
 
 	return 0;

drivers/net/can/c_can/c_can.h +2 -0

index 1437a6d..5f6339c 100644
@@ -166,6 +166,8 @@ struct c_can_priv {
 	unsigned int tx_echo;
 	void *priv;		/* for board-specific data */
 	u16 irqstatus;
+	unsigned int instance;
+	void (*ram_init) (unsigned int instance, bool enable);
 };
 
 struct net_device *alloc_c_can_dev(void);

drivers/net/can/c_can/c_can_platform.c +2 -0

index c351975..c6963b2 100644
@@ -34,6 +34,7 @@
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/can/platform/c_can.h>
 
 #include <linux/can/dev.h>
 

@@ -98,6 +99,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 	struct net_device *dev;
 	struct c_can_priv *priv;
 	const struct of_device_id *match;
+	struct c_can_platform_data *pdata = NULL;
 	const struct platform_device_id *id;
 	struct pinctrl *pinctrl;
 	struct resource *mem;

drivers/net/can/c_can/c_can_platform.c +4 -0

+			goto exit_free_device;
+		}
+		priv->ram_init = pdata->ram_init;
+		priv->instance = pdata->instance;
 		break;
 	default:
 		ret = -EINVAL;

include/linux/can/platform/c_can.h new file +36 -0

new file mode 100644
index 0000000..84b27d2
@@ -0,0 +1,36 @@
+/*
+ * C_CAN controller driver platform header
+ *
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Bosch C_CAN/D_CAN controller is compliant to CAN protocol version 2.0
+ * part A and B.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __CAN_PLATFORM_C_CAN_H__
+#define __CAN_PLATFORM_C_CAN_H__
+
+/**
+ * struct c_can_platform_data - C_CAN/D_CAN Platform Data
+ *
+ * @instance:		CAN instance, required for d_can raminit
+ * @ram_init:		CAN RAM initialization
+ *
+ * Platform data structure to get all platform specific settings.
+ * this structure also accounts the fact that the IP may have different
+ * RAM for different SOC's
+ */
+struct c_can_platform_data {
+	unsigned int instance;
+	void (*ram_init) (unsigned int instance, bool enable);
+};
+#endif

Quoted 94 lines · AnilKumar Ch

Add D_CAN raminit support to C_CAN driver to enable D_CAN RAM. DCAN RAM holds all the message objects during transmission or receiving of data. This initialization/de-initialization should be done in synchronous with D_CAN clock.

Signed-off-by AnilKumar Ch <[email protected]>

---
 drivers/net/can/c_can/c_can.c          |   13 ++++++++++++
 drivers/net/can/c_can/c_can.h          |    2 ++
 drivers/net/can/c_can/c_can_platform.c |   10 +++++++++
 include/linux/can/platform/c_can.h     |   36 ++++++++++++++++++++++++++++++++
 4 files changed, 61 insertions(+)
 create mode 100644 include/linux/can/platform/c_can.h

drivers/net/can/c_can/c_can.c +13 -0

index aa6c5eb..c175410 100644
@@ -214,6 +214,12 @@ static inline void c_can_pm_runtime_put_sync(const struct c_can_priv *priv)
 		pm_runtime_put_sync(priv->device);
 }
 
+static inline void c_can_reset_ram(const struct c_can_priv *priv, bool enable)
+{
+	if (priv->ram_init)
+		priv->ram_init(priv->instance, enable);
+}
+
 static inline int get_tx_next_msg_obj(const struct c_can_priv *priv)
 {
 	return (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) +

@@ -1071,6 +1077,8 @@ static int c_can_open(struct net_device *dev)
 	struct c_can_priv *priv = netdev_priv(dev);
 
 	c_can_pm_runtime_get_sync(priv);
+	/* Initialize DCAN RAM */
+	c_can_reset_ram(priv, true);
 
 	/* open the can device */
 	err = open_candev(dev);

@@ -1099,6 +1107,8 @@ static int c_can_open(struct net_device *dev)
 exit_irq_fail:
 	close_candev(dev);
 exit_open_fail:
+	/* De-Initialize DCAN RAM */
+	c_can_reset_ram(priv, false);
 	c_can_pm_runtime_put_sync(priv);
 	return err;
 }

@@ -1112,6 +1122,9 @@ static int c_can_close(struct net_device *dev)
 	c_can_stop(dev);
 	free_irq(dev->irq, dev);
 	close_candev(dev);
+
+	/* De-Initialize DCAN RAM */
+	c_can_reset_ram(priv, false);
 	c_can_pm_runtime_put_sync(priv);
 
 	return 0;

drivers/net/can/c_can/c_can.h +2 -0

index 1437a6d..5f6339c 100644
@@ -166,6 +166,8 @@ struct c_can_priv {
 	unsigned int tx_echo;
 	void *priv;		/* for board-specific data */
 	u16 irqstatus;
+	unsigned int instance;
+	void (*ram_init) (unsigned int instance, bool enable);
 };
 
 struct net_device *alloc_c_can_dev(void);

drivers/net/can/c_can/c_can_platform.c +2 -0

index c351975..c6963b2 100644
@@ -34,6 +34,7 @@
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/can/platform/c_can.h>
 
 #include <linux/can/dev.h>
 

@@ -98,6 +99,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 	struct net_device *dev;
 	struct c_can_priv *priv;
 	const struct of_device_id *match;
+	struct c_can_platform_data *pdata = NULL;
 	const struct platform_device_id *id;
 	struct pinctrl *pinctrl;
 	struct resource *mem;

@@ -179,6 +181,14 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 		priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES;

drivers/net/can/c_can/c_can_platform.c static int __devinit c_can_plat_probe(struct platform_device *pdev) · line 187

Quoted 6 lines

drivers/net/can/c_can/c_can_platform.c +4 -0

 		priv->read_reg = c_can_plat_read_reg_aligned_to_16bit;
 		priv->write_reg = c_can_plat_write_reg_aligned_to_16bit;
+		pdata = pdev->dev.platform_data;
+		if (!pdata) {
+			dev_err(&pdev->dev, "d_can platform data missing\n");
+			ret = -EINVAL;

Is the ram_init mandatory on all d_can? There might be non omap d_can users.

Marc

Quoted 50 lines

drivers/net/can/c_can/c_can_platform.c +4 -0

+			goto exit_free_device;
+		}
+		priv->ram_init = pdata->ram_init;
+		priv->instance = pdata->instance;
 		break;
 	default:
 		ret = -EINVAL;

include/linux/can/platform/c_can.h new file +36 -0

new file mode 100644
index 0000000..84b27d2
@@ -0,0 +1,36 @@
+/*
+ * C_CAN controller driver platform header
+ *
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Bosch C_CAN/D_CAN controller is compliant to CAN protocol version 2.0
+ * part A and B.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __CAN_PLATFORM_C_CAN_H__
+#define __CAN_PLATFORM_C_CAN_H__
+
+/**
+ * struct c_can_platform_data - C_CAN/D_CAN Platform Data
+ *
+ * @instance:		CAN instance, required for d_can raminit
+ * @ram_init:		CAN RAM initialization
+ *
+ * Platform data structure to get all platform specific settings.
+ * this structure also accounts the fact that the IP may have different
+ * RAM for different SOC's
+ */
+struct c_can_platform_data {
+	unsigned int instance;
+	void (*ram_init) (unsigned int instance, bool enable);
+};
+#endif

Signature · 4 lines

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   |

Hi Marc,

Quoted 95 lines · Marc Kleine-Budde

Quoted 94 lines · AnilKumar Ch

Add D_CAN raminit support to C_CAN driver to enable D_CAN RAM. DCAN RAM holds all the message objects during transmission or receiving of data. This initialization/de-initialization should be done in synchronous with D_CAN clock.

Signed-off-by AnilKumar Ch <[email protected]>

---
 drivers/net/can/c_can/c_can.c          |   13 ++++++++++++
 drivers/net/can/c_can/c_can.h          |    2 ++
 drivers/net/can/c_can/c_can_platform.c |   10 +++++++++
 include/linux/can/platform/c_can.h     |   36 ++++++++++++++++++++++++++++++++
 4 files changed, 61 insertions(+)
 create mode 100644 include/linux/can/platform/c_can.h

drivers/net/can/c_can/c_can.c +13 -0

index aa6c5eb..c175410 100644
@@ -214,6 +214,12 @@ static inline void c_can_pm_runtime_put_sync(const struct c_can_priv *priv)
 		pm_runtime_put_sync(priv->device);
 }
 
+static inline void c_can_reset_ram(const struct c_can_priv *priv, bool enable)
+{
+	if (priv->ram_init)
+		priv->ram_init(priv->instance, enable);
+}
+
 static inline int get_tx_next_msg_obj(const struct c_can_priv *priv)
 {
 	return (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) +

@@ -1071,6 +1077,8 @@ static int c_can_open(struct net_device *dev)
 	struct c_can_priv *priv = netdev_priv(dev);
 
 	c_can_pm_runtime_get_sync(priv);
+	/* Initialize DCAN RAM */
+	c_can_reset_ram(priv, true);
 
 	/* open the can device */
 	err = open_candev(dev);

@@ -1099,6 +1107,8 @@ static int c_can_open(struct net_device *dev)
 exit_irq_fail:
 	close_candev(dev);
 exit_open_fail:
+	/* De-Initialize DCAN RAM */
+	c_can_reset_ram(priv, false);
 	c_can_pm_runtime_put_sync(priv);
 	return err;
 }

@@ -1112,6 +1122,9 @@ static int c_can_close(struct net_device *dev)
 	c_can_stop(dev);
 	free_irq(dev->irq, dev);
 	close_candev(dev);
+
+	/* De-Initialize DCAN RAM */
+	c_can_reset_ram(priv, false);
 	c_can_pm_runtime_put_sync(priv);
 
 	return 0;

drivers/net/can/c_can/c_can.h +2 -0

index 1437a6d..5f6339c 100644
@@ -166,6 +166,8 @@ struct c_can_priv {
 	unsigned int tx_echo;
 	void *priv;		/* for board-specific data */
 	u16 irqstatus;
+	unsigned int instance;
+	void (*ram_init) (unsigned int instance, bool enable);
 };
 
 struct net_device *alloc_c_can_dev(void);

drivers/net/can/c_can/c_can_platform.c +2 -0

index c351975..c6963b2 100644
@@ -34,6 +34,7 @@
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/can/platform/c_can.h>
 
 #include <linux/can/dev.h>
 

@@ -98,6 +99,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 	struct net_device *dev;
 	struct c_can_priv *priv;
 	const struct of_device_id *match;
+	struct c_can_platform_data *pdata = NULL;
 	const struct platform_device_id *id;
 	struct pinctrl *pinctrl;
 	struct resource *mem;

@@ -179,6 +181,14 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 		priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES;

Quoted 8 lines

drivers/net/can/c_can/c_can_platform.c static int __devinit c_can_plat_probe(struct platform_device *pdev) · line 187

Quoted 6 lines

drivers/net/can/c_can/c_can_platform.c +4 -0

 		priv->read_reg = c_can_plat_read_reg_aligned_to_16bit;
 		priv->write_reg = c_can_plat_write_reg_aligned_to_16bit;
+		pdata = pdev->dev.platform_data;
+		if (!pdata) {
+			dev_err(&pdev->dev, "d_can platform data missing\n");
+			ret = -EINVAL;

Is the ram_init mandatory on all d_can? There might be non omap d_can users.

As per AM335x specifications d_can module should have ram_init. In that case it's better to print warning and break the switch.

Quoted 45 lines

Marc

Quoted 42 lines

drivers/net/can/c_can/c_can_platform.c +4 -0

+			goto exit_free_device;
+		}
+		priv->ram_init = pdata->ram_init;
+		priv->instance = pdata->instance;
 		break;
 	default:
 		ret = -EINVAL;

include/linux/can/platform/c_can.h new file +29 -0

new file mode 100644
index 0000000..84b27d2
@@ -0,0 +1,36 @@
+/*
+ * C_CAN controller driver platform header
+ *
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Bosch C_CAN/D_CAN controller is compliant to CAN protocol version 2.0
+ * part A and B.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __CAN_PLATFORM_C_CAN_H__
+#define __CAN_PLATFORM_C_CAN_H__
+
+/**
+ * struct c_can_platform_data - C_CAN/D_CAN Platform Data
+ *
+ * @instance:		CAN instance, required for d_can raminit
+ * @ram_init:		CAN RAM initialization
+ *
+ * Platform data structure to get all platform specific settings.
+ * this structure also accounts the fact that the IP may have different

Quoted 17 lines

Quoted 8 lines

include/linux/can/platform/c_can.h new file +7 -0

+ * RAM for different SOC's
+ */
+struct c_can_platform_data {
+	unsigned int instance;
+	void (*ram_init) (unsigned int instance, bool enable);
+};
+#endif

Signature · 4 lines

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   |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 107 lines · Chimata

Hi Marc,

Quoted 103 lines · Marc Kleine-Budde

Quoted 93 lines · AnilKumar Ch

Add D_CAN raminit support to C_CAN driver to enable D_CAN RAM. DCAN RAM holds all the message objects during transmission or receiving of data. This initialization/de-initialization should be done in synchronous with D_CAN clock.

Signed-off-by AnilKumar Ch <[email protected]>

---
 drivers/net/can/c_can/c_can.c          |   13 ++++++++++++
 drivers/net/can/c_can/c_can.h          |    2 ++
 drivers/net/can/c_can/c_can_platform.c |   10 +++++++++
 include/linux/can/platform/c_can.h     |   36 ++++++++++++++++++++++++++++++++
 4 files changed, 61 insertions(+)
 create mode 100644 include/linux/can/platform/c_can.h

drivers/net/can/c_can/c_can.c +13 -0

index aa6c5eb..c175410 100644
@@ -214,6 +214,12 @@ static inline void c_can_pm_runtime_put_sync(const struct c_can_priv *priv)
 		pm_runtime_put_sync(priv->device);
 }
 
+static inline void c_can_reset_ram(const struct c_can_priv *priv, bool enable)
+{
+	if (priv->ram_init)
+		priv->ram_init(priv->instance, enable);
+}
+
 static inline int get_tx_next_msg_obj(const struct c_can_priv *priv)
 {
 	return (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) +

@@ -1071,6 +1077,8 @@ static int c_can_open(struct net_device *dev)
 	struct c_can_priv *priv = netdev_priv(dev);
 
 	c_can_pm_runtime_get_sync(priv);
+	/* Initialize DCAN RAM */
+	c_can_reset_ram(priv, true);
 
 	/* open the can device */
 	err = open_candev(dev);

@@ -1099,6 +1107,8 @@ static int c_can_open(struct net_device *dev)
 exit_irq_fail:
 	close_candev(dev);
 exit_open_fail:
+	/* De-Initialize DCAN RAM */
+	c_can_reset_ram(priv, false);
 	c_can_pm_runtime_put_sync(priv);
 	return err;
 }

@@ -1112,6 +1122,9 @@ static int c_can_close(struct net_device *dev)
 	c_can_stop(dev);
 	free_irq(dev->irq, dev);
 	close_candev(dev);
+
+	/* De-Initialize DCAN RAM */
+	c_can_reset_ram(priv, false);
 	c_can_pm_runtime_put_sync(priv);
 
 	return 0;

drivers/net/can/c_can/c_can.h +2 -0

index 1437a6d..5f6339c 100644
@@ -166,6 +166,8 @@ struct c_can_priv {
 	unsigned int tx_echo;
 	void *priv;		/* for board-specific data */
 	u16 irqstatus;
+	unsigned int instance;
+	void (*ram_init) (unsigned int instance, bool enable);
 };
 
 struct net_device *alloc_c_can_dev(void);

drivers/net/can/c_can/c_can_platform.c +2 -0

index c351975..c6963b2 100644
@@ -34,6 +34,7 @@
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/can/platform/c_can.h>
 
 #include <linux/can/dev.h>
 

@@ -98,6 +99,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 	struct net_device *dev;
 	struct c_can_priv *priv;
 	const struct of_device_id *match;
+	struct c_can_platform_data *pdata = NULL;
 	const struct platform_device_id *id;
 	struct pinctrl *pinctrl;
 	struct resource *mem;

drivers/net/can/c_can/c_can_platform.c static int __devinit c_can_plat_probe(struct platform_device *pdev) · line 187

Quoted 7 lines

drivers/net/can/c_can/c_can_platform.c +4 -0

@@ -179,6 +181,14 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 		priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES;
 		priv->read_reg = c_can_plat_read_reg_aligned_to_16bit;
 		priv->write_reg = c_can_plat_write_reg_aligned_to_16bit;
+		pdata = pdev->dev.platform_data;
+		if (!pdata) {
+			dev_err(&pdev->dev, "d_can platform data missing\n");
+			ret = -EINVAL;

Is the ram_init mandatory on all d_can? There might be non omap d_can users.

Quoted 3 lines

As per AM335x specifications d_can module should have ram_init. In that case it's better to print warning and break the switch.

As far as I know, "ram_init" is part of IP spec, how it is controlled does varies based on SoC integration.

Add of_dev_auxdata to pass d_can raminit callback APIs to initialize d_can RAM. D_CAN RAM initialization bits are present in CONTROL module address space, which can be accessed by platform specific code. So callback functions are added to serve this purpose, this can done by using of_dev_auxdata.

Callback API is added to of_dev_auxdata with different instance numbers for two instances of D_CAN IP. These callback functions are used to enable/disable D_CAN RAM from CAN driver.

Signed-off-by AnilKumar Ch <[email protected]>

---
 arch/arm/mach-omap2/board-generic.c |   40 ++++++++++++++++++++++++++++++++++-
 arch/arm/mach-omap2/control.h       |    4 ++++
 2 files changed, 43 insertions(+), 1 deletion(-)

arch/arm/mach-omap2/board-generic.c +39 -1

index 6f93a20..b68e642 100644
@@ -15,6 +15,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/irqdomain.h>
+#include <linux/can/platform/c_can.h>
 
 #include <mach/hardware.h>
 #include <asm/hardware/gic.h>

@@ -22,6 +23,8 @@
 
 #include <plat/board.h>
 #include "common.h"
+#include "control.h"
+#include "iomap.h"
 #include "common-board-devices.h"
 
 #if !(defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3))

@@ -37,11 +40,46 @@ static struct of_device_id omap_dt_match_table[] __initdata = {
 	{ }
 };
 
+void d_can_hw_raminit(unsigned int instance, bool enable)
+{
+	u32 val;
+
+	val = readl(AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT));
+	if (enable) {
+		val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance);
+		val |= AM33XX_DCAN_RAMINIT_START_MASK(instance);
+		writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT));
+	} else {
+		val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance);
+		writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT));
+	}
+}
+
+static struct c_can_platform_data d_can0_pdata = {
+	.ram_init	= d_can_hw_raminit,
+	.instance	= 0,
+};
+
+static struct c_can_platform_data d_can1_pdata = {
+	.ram_init	= d_can_hw_raminit,
+	.instance	= 1,
+};
+
+static const struct of_dev_auxdata am33xx_auxdata_lookup[] __initconst = {
+	OF_DEV_AUXDATA("bosch,d_can", 0x481cc000, NULL, &d_can0_pdata),
+	OF_DEV_AUXDATA("bosch,d_can", 0x481d0000, NULL, &d_can1_pdata),
+	{ },
+};
+
 static void __init omap_generic_init(void)
 {
 	omap_sdrc_init(NULL, NULL);
 
-	of_platform_populate(NULL, omap_dt_match_table, NULL, NULL);
+	if (of_machine_is_compatible("ti,am33xx"))
+		of_platform_populate(NULL, omap_dt_match_table,
+				am33xx_auxdata_lookup, NULL);
+	else
+		of_platform_populate(NULL, omap_dt_match_table, NULL, NULL);
 }
 
 #ifdef CONFIG_SOC_OMAP2420

arch/arm/mach-omap2/control.h +4 -0

index b8cdc85..afd189b 100644
@@ -356,6 +356,10 @@
 #define AM33XX_CONTROL_STATUS_SYSBOOT1_SHIFT		22
 #define AM33XX_CONTROL_STATUS_SYSBOOT1_MASK		(0x3 << 22)
 
+/* AM33XX DCAN bitfields */
+#define AM33XX_CONTROL_DCAN_RAMINIT		0x644
+#define AM33XX_DCAN_RAMINIT_START_MASK(i)	(1 << (i))
+
 /* CONTROL OMAP STATUS register to identify OMAP3 features */
 #define OMAP3_CONTROL_OMAP_STATUS	0x044c
 

Quoted 6 lines · AnilKumar Ch

Add of_dev_auxdata to pass d_can raminit callback APIs to initialize d_can RAM. D_CAN RAM initialization bits are present in CONTROL module address space, which can be accessed by platform specific code. So callback functions are added to serve this purpose, this can done by using of_dev_auxdata.

Quoted 5 lines

Callback API is added to of_dev_auxdata with different instance numbers for two instances of D_CAN IP. These callback functions are used to enable/disable D_CAN RAM from CAN driver.

Signed-off-by AnilKumar Ch <[email protected]>

This will be a more complicated. This patch will go over the arm tree, but needs a header going via net.

Quoted 91 lines

---
 arch/arm/mach-omap2/board-generic.c |   40 ++++++++++++++++++++++++++++++++++-
 arch/arm/mach-omap2/control.h       |    4 ++++
 2 files changed, 43 insertions(+), 1 deletion(-)

arch/arm/mach-omap2/board-generic.c +39 -1

index 6f93a20..b68e642 100644
@@ -15,6 +15,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/irqdomain.h>
+#include <linux/can/platform/c_can.h>
 
 #include <mach/hardware.h>
 #include <asm/hardware/gic.h>

@@ -22,6 +23,8 @@
 
 #include <plat/board.h>
 #include "common.h"
+#include "control.h"
+#include "iomap.h"
 #include "common-board-devices.h"
 
 #if !(defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3))

@@ -37,11 +40,46 @@ static struct of_device_id omap_dt_match_table[] __initdata = {
 	{ }
 };
 
+void d_can_hw_raminit(unsigned int instance, bool enable)
+{
+	u32 val;
+
+	val = readl(AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT));
+	if (enable) {
+		val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance);
+		val |= AM33XX_DCAN_RAMINIT_START_MASK(instance);
+		writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT));
+	} else {
+		val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance);
+		writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT));
+	}
+}
+
+static struct c_can_platform_data d_can0_pdata = {
+	.ram_init	= d_can_hw_raminit,
+	.instance	= 0,
+};
+
+static struct c_can_platform_data d_can1_pdata = {
+	.ram_init	= d_can_hw_raminit,
+	.instance	= 1,
+};
+
+static const struct of_dev_auxdata am33xx_auxdata_lookup[] __initconst = {
+	OF_DEV_AUXDATA("bosch,d_can", 0x481cc000, NULL, &d_can0_pdata),
+	OF_DEV_AUXDATA("bosch,d_can", 0x481d0000, NULL, &d_can1_pdata),
+	{ },
+};
+
 static void __init omap_generic_init(void)
 {
 	omap_sdrc_init(NULL, NULL);
 
-	of_platform_populate(NULL, omap_dt_match_table, NULL, NULL);
+	if (of_machine_is_compatible("ti,am33xx"))
+		of_platform_populate(NULL, omap_dt_match_table,
+				am33xx_auxdata_lookup, NULL);
+	else
+		of_platform_populate(NULL, omap_dt_match_table, NULL, NULL);
 }
 
 #ifdef CONFIG_SOC_OMAP2420

arch/arm/mach-omap2/control.h +4 -0

index b8cdc85..afd189b 100644
@@ -356,6 +356,10 @@
 #define AM33XX_CONTROL_STATUS_SYSBOOT1_SHIFT		22
 #define AM33XX_CONTROL_STATUS_SYSBOOT1_MASK		(0x3 << 22)
 
+/* AM33XX DCAN bitfields */
+#define AM33XX_CONTROL_DCAN_RAMINIT		0x644
+#define AM33XX_DCAN_RAMINIT_START_MASK(i)	(1 << (i))
+
 /* CONTROL OMAP STATUS register to identify OMAP3 features */
 #define OMAP3_CONTROL_OMAP_STATUS	0x044c
 

Signature · 4 lines

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   |

Quoted 7 lines · Marc Kleine-Budde

Quoted 6 lines · AnilKumar Ch

Add of_dev_auxdata to pass d_can raminit callback APIs to initialize d_can RAM. D_CAN RAM initialization bits are present in CONTROL module address space, which can be accessed by platform specific code. So callback functions are added to serve this purpose, this can done by using of_dev_auxdata.

Quoted 9 lines

Quoted 5 lines

Callback API is added to of_dev_auxdata with different instance numbers for two instances of D_CAN IP. These callback functions are used to enable/disable D_CAN RAM from CAN driver.

Signed-off-by AnilKumar Ch <[email protected]>

This will be a more complicated. This patch will go over the arm tree, but needs a header going via net.

Marc,

I agree this is a bit complicated but this has to go along with this patch series otherwise build will break. If there are no changes required I will request Tony to ack it.

Tony,

If there are no changes required, could you please ack this patch so that this will go to linux-can tree.

Thanks
AnilKumar

Quoted 100 lines

Quoted 91 lines

---
 arch/arm/mach-omap2/board-generic.c |   40 ++++++++++++++++++++++++++++++++++-
 arch/arm/mach-omap2/control.h       |    4 ++++
 2 files changed, 43 insertions(+), 1 deletion(-)

arch/arm/mach-omap2/board-generic.c +39 -1

index 6f93a20..b68e642 100644
@@ -15,6 +15,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/irqdomain.h>
+#include <linux/can/platform/c_can.h>
 
 #include <mach/hardware.h>
 #include <asm/hardware/gic.h>

@@ -22,6 +23,8 @@
 
 #include <plat/board.h>
 #include "common.h"
+#include "control.h"
+#include "iomap.h"
 #include "common-board-devices.h"
 
 #if !(defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3))

@@ -37,11 +40,46 @@ static struct of_device_id omap_dt_match_table[] __initdata = {
 	{ }
 };
 
+void d_can_hw_raminit(unsigned int instance, bool enable)
+{
+	u32 val;
+
+	val = readl(AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT));
+	if (enable) {
+		val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance);
+		val |= AM33XX_DCAN_RAMINIT_START_MASK(instance);
+		writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT));
+	} else {
+		val &= ~AM33XX_DCAN_RAMINIT_START_MASK(instance);
+		writel(val, AM33XX_CTRL_REGADDR(AM33XX_CONTROL_DCAN_RAMINIT));
+	}
+}
+
+static struct c_can_platform_data d_can0_pdata = {
+	.ram_init	= d_can_hw_raminit,
+	.instance	= 0,
+};
+
+static struct c_can_platform_data d_can1_pdata = {
+	.ram_init	= d_can_hw_raminit,
+	.instance	= 1,
+};
+
+static const struct of_dev_auxdata am33xx_auxdata_lookup[] __initconst = {
+	OF_DEV_AUXDATA("bosch,d_can", 0x481cc000, NULL, &d_can0_pdata),
+	OF_DEV_AUXDATA("bosch,d_can", 0x481d0000, NULL, &d_can1_pdata),
+	{ },
+};
+
 static void __init omap_generic_init(void)
 {
 	omap_sdrc_init(NULL, NULL);
 
-	of_platform_populate(NULL, omap_dt_match_table, NULL, NULL);
+	if (of_machine_is_compatible("ti,am33xx"))
+		of_platform_populate(NULL, omap_dt_match_table,
+				am33xx_auxdata_lookup, NULL);
+	else
+		of_platform_populate(NULL, omap_dt_match_table, NULL, NULL);
 }
 
 #ifdef CONFIG_SOC_OMAP2420

arch/arm/mach-omap2/control.h +4 -0

index b8cdc85..afd189b 100644
@@ -356,6 +356,10 @@
 #define AM33XX_CONTROL_STATUS_SYSBOOT1_SHIFT		22
 #define AM33XX_CONTROL_STATUS_SYSBOOT1_MASK		(0x3 << 22)
 
+/* AM33XX DCAN bitfields */
+#define AM33XX_CONTROL_DCAN_RAMINIT		0x644
+#define AM33XX_DCAN_RAMINIT_START_MASK(i)	(1 << (i))
+
 /* CONTROL OMAP STATUS register to identify OMAP3 features */
 #define OMAP3_CONTROL_OMAP_STATUS	0x044c
 

Signature · 4 lines

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   |

Adds suspend resume support to DCAN driver which enables DCAN power down mode bit (PDR). Then DCAN will ack the local power-down mode by setting PDA bit in STATUS register.

Also adds a status flag to know the status of DCAN module, whether it is opened or not.

Signed-off-by AnilKumar Ch <[email protected]>

---
 drivers/net/can/c_can/c_can.c          |   78 ++++++++++++++++++++++++++++++++
 drivers/net/can/c_can/c_can.h          |    5 ++
 drivers/net/can/c_can/c_can_platform.c |   70 ++++++++++++++++++++++++++--
 3 files changed, 150 insertions(+), 3 deletions(-)

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

index c175410..36d5db4 100644
@@ -46,6 +46,9 @@
 #define IF_ENUM_REG_LEN		11
 #define C_CAN_IFACE(reg, iface)	(C_CAN_IF1_##reg + (iface) * IF_ENUM_REG_LEN)
 
+/* control extension register D_CAN specific */
+#define CONTROL_EX_PDR		BIT(8)
+
 /* control register */
 #define CONTROL_TEST		BIT(7)
 #define CONTROL_CCE		BIT(6)

@@ -65,6 +68,7 @@
 #define TEST_BASIC		BIT(2)
 
 /* status register */
+#define STATUS_PDA		BIT(10)
 #define STATUS_BOFF		BIT(7)
 #define STATUS_EWARN		BIT(6)
 #define STATUS_EPASS		BIT(5)

drivers/net/can/c_can/c_can.c +3 -0

+
 /* napi related */
 #define C_CAN_NAPI_WEIGHT	C_CAN_MSG_OBJ_RX_NUM
 

@@ -1102,6 +1109,7 @@ static int c_can_open(struct net_device *dev)
 
 	netif_start_queue(dev);
 
+	priv->is_opened = true;
 	return 0;
 
 exit_irq_fail:

@@ -1126,6 +1134,7 @@ static int c_can_close(struct net_device *dev)
 	/* De-Initialize DCAN RAM */
 	c_can_reset_ram(priv, false);
 	c_can_pm_runtime_put_sync(priv);
+	priv->is_opened = false;
 
 	return 0;
 }

@@ -1154,6 +1163,75 @@ struct net_device *alloc_c_can_dev(void)
 }

drivers/net/can/c_can/c_can.c +15 -0

+
+	/* Wait for the PDA bit to get set */
+	time_out = jiffies + msecs_to_jiffies(INIT_WAIT_COUNT);
+	while (!(priv->read_reg(priv, C_CAN_STS_REG) & STATUS_PDA) &&
+				time_after(time_out, jiffies))
+		cpu_relax();
+
+	if (time_after(jiffies, time_out))
+		return -ETIMEDOUT;
+
+	c_can_stop(dev);
+
+	/* De-initialize DCAN RAM */
+	c_can_reset_ram(priv, false);
+	c_can_pm_runtime_put_sync(priv);

drivers/net/can/c_can/c_can.c +20 -0

+	if (!priv->is_opened)
+		return 0;
+
+	c_can_pm_runtime_get_sync(priv);
+	/* Initialize DCAN RAM */
+	c_can_reset_ram(priv, true);
+
+	/* Clear PDR and INIT bits */
+	priv->write_reg(priv, C_CAN_CTRL_EX_REG,
+		priv->read_reg(priv, C_CAN_CTRL_EX_REG) & ~CONTROL_EX_PDR);
+	priv->write_reg(priv, C_CAN_CTRL_REG,
+		priv->read_reg(priv, C_CAN_CTRL_REG) & ~CONTROL_INIT);
+
+	/* Wait for the PDA bit to get clear */
+	time_out = jiffies + msecs_to_jiffies(INIT_WAIT_COUNT);
+	while ((priv->read_reg(priv, C_CAN_STS_REG) & STATUS_PDA) &&
+				time_after(time_out, jiffies))
+		cpu_relax();
+
+	if (time_after(jiffies, time_out))

drivers/net/can/c_can/c_can.c +2 -0

+#endif
+
 void free_c_can_dev(struct net_device *dev)
 {
 	free_candev(dev);

drivers/net/can/c_can/c_can.h +5 -0

index 5f6339c..e5dd7ef 100644
@@ -24,6 +24,7 @@
 
 enum reg {
 	C_CAN_CTRL_REG = 0,
+	C_CAN_CTRL_EX_REG,
 	C_CAN_STS_REG,
 	C_CAN_ERR_CNT_REG,
 	C_CAN_BTR_REG,

@@ -104,6 +105,7 @@ static const u16 reg_map_c_can[] = {
 
 static const u16 reg_map_d_can[] = {
 	[C_CAN_CTRL_REG]	= 0x00,
+	[C_CAN_CTRL_EX_REG]	= 0x02,
 	[C_CAN_STS_REG]		= 0x04,
 	[C_CAN_ERR_CNT_REG]	= 0x08,
 	[C_CAN_BTR_REG]		= 0x0C,

@@ -166,6 +168,7 @@ struct c_can_priv {
 	unsigned int tx_echo;
 	void *priv;		/* for board-specific data */
 	u16 irqstatus;
+	bool is_opened;
 	unsigned int instance;
 	void (*ram_init) (unsigned int instance, bool enable);
 };

@@ -174,5 +177,7 @@ struct net_device *alloc_c_can_dev(void);
 void free_c_can_dev(struct net_device *dev);
 int register_c_can_dev(struct net_device *dev);
 void unregister_c_can_dev(struct net_device *dev);
+int c_can_power_up(struct net_device *dev);
+int c_can_power_down(struct net_device *dev);
 
 #endif /* C_CAN_H */

drivers/net/can/c_can/c_can_platform.c +0 -0

index c6963b2..65ec232 100644

drivers/net/can/c_can/c_can_platform.c +4 -0

+
+	if (id->driver_data != BOSCH_D_CAN) {
+		dev_warn(&pdev->dev, "Not supported\n");
+		return 0;

drivers/net/can/c_can/c_can_platform.c +17 -0

+		return ret;
+	}
+
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+	if (netif_running(ndev)) {
+		netif_device_attach(ndev);
+		netif_start_queue(ndev);
+	}
+
+	return 0;
+}
+#else
+#define c_can_suspend NULL
+#define c_can_resume NULL
+#endif
+
 static struct platform_driver c_can_plat_driver = {
 	.driver = {
 		.name = KBUILD_MODNAME,
 		.owner = THIS_MODULE,

Quoted 6 lines · AnilKumar Ch

Adds suspend resume support to DCAN driver which enables DCAN power down mode bit (PDR). Then DCAN will ack the local power-down mode by setting PDA bit in STATUS register.

Also adds a status flag to know the status of DCAN module, whether it is opened or not.

Use "ndev->flags & IFF_UP" for that. Have a look at the at91_can driver [1]. I'm not sure if you need locking here.

[1] http://lxr.free-electrons.com/source/drivers/net/can/at91_can.c#L1198

Quoted 33 lines

Signed-off-by AnilKumar Ch <[email protected]>

---
 drivers/net/can/c_can/c_can.c          |   78 ++++++++++++++++++++++++++++++++
 drivers/net/can/c_can/c_can.h          |    5 ++
 drivers/net/can/c_can/c_can_platform.c |   70 ++++++++++++++++++++++++++--
 3 files changed, 150 insertions(+), 3 deletions(-)

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

index c175410..36d5db4 100644
@@ -46,6 +46,9 @@
 #define IF_ENUM_REG_LEN		11
 #define C_CAN_IFACE(reg, iface)	(C_CAN_IF1_##reg + (iface) * IF_ENUM_REG_LEN)
 
+/* control extension register D_CAN specific */
+#define CONTROL_EX_PDR		BIT(8)
+
 /* control register */
 #define CONTROL_TEST		BIT(7)
 #define CONTROL_CCE		BIT(6)

@@ -65,6 +68,7 @@
 #define TEST_BASIC		BIT(2)
 
 /* status register */
+#define STATUS_PDA		BIT(10)
 #define STATUS_BOFF		BIT(7)
 #define STATUS_EWARN		BIT(6)
 #define STATUS_EPASS		BIT(5)

@@ -164,6 +168,9 @@
 /* minimum timeout for checking BUSY status */
 #define MIN_TIMEOUT_VALUE	6
 

drivers/net/can/c_can/c_can.c line 172

Quoted 2 lines

drivers/net/can/c_can/c_can.c +2 -0

+/* Wait for ~1 sec for INIT bit */
+#define INIT_WAIT_COUNT		1000

What unit? INIT_WAIT_MS would be a better name.

Quoted 24 lines

drivers/net/can/c_can/c_can.c +3 -0

+
 /* napi related */
 #define C_CAN_NAPI_WEIGHT	C_CAN_MSG_OBJ_RX_NUM
 

@@ -1102,6 +1109,7 @@ static int c_can_open(struct net_device *dev)
 
 	netif_start_queue(dev);
 
+	priv->is_opened = true;
 	return 0;
 
 exit_irq_fail:

@@ -1126,6 +1134,7 @@ static int c_can_close(struct net_device *dev)
 	/* De-Initialize DCAN RAM */
 	c_can_reset_ram(priv, false);
 	c_can_pm_runtime_put_sync(priv);
+	priv->is_opened = false;
 
 	return 0;
 }

@@ -1154,6 +1163,75 @@ struct net_device *alloc_c_can_dev(void)
 }
 EXPORT_SYMBOL_GPL(alloc_c_can_dev);
 

drivers/net/can/c_can/c_can.c struct net_device *alloc_c_can_dev(void) · line 1173

Quoted 8 lines

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

+#ifdef CONFIG_PM
+int c_can_power_down(struct net_device *dev)
+{
+	unsigned long time_out;
+	struct c_can_priv *priv = netdev_priv(dev);
+
+	if (!priv->is_opened)
+		return 0;

Should we add a BUG_ON(id->driver_data != BOSCH_D_CAN)?

drivers/net/can/c_can/c_can.c struct net_device *alloc_c_can_dev(void) · line 1177

Quoted 5 lines

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

+
+	/* set `PDR` value so the device goes to power down mode */
+	priv->write_reg(priv, C_CAN_CTRL_EX_REG,
+		priv->read_reg(priv, C_CAN_CTRL_EX_REG) | CONTROL_EX_PDR);

Please use an intermediate variable:

u32 val;

val = priv->read_reg(priv, C_CAN_CTRL_EX_REG);
val |= CONTROL_EX_PDR;
priv->write_reg(priv, C_CAN_CTRL_EX_REG, val);

Quoted 16 lines

drivers/net/can/c_can/c_can.c +15 -0

+
+	/* Wait for the PDA bit to get set */
+	time_out = jiffies + msecs_to_jiffies(INIT_WAIT_COUNT);
+	while (!(priv->read_reg(priv, C_CAN_STS_REG) & STATUS_PDA) &&
+				time_after(time_out, jiffies))
+		cpu_relax();
+
+	if (time_after(jiffies, time_out))
+		return -ETIMEDOUT;
+
+	c_can_stop(dev);
+
+	/* De-initialize DCAN RAM */
+	c_can_reset_ram(priv, false);
+	c_can_pm_runtime_put_sync(priv);

drivers/net/can/c_can/c_can.c struct net_device *alloc_c_can_dev(void) · line 1202

Quoted 10 lines

drivers/net/can/c_can/c_can.c +10 -0

+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(c_can_power_down);
+
+int c_can_power_up(struct net_device *dev)
+{
+	unsigned long time_out;
+	struct c_can_priv *priv = netdev_priv(dev);
+

BUG_ON?

Quoted 24 lines

drivers/net/can/c_can/c_can.c +24 -0

+	if (!priv->is_opened)
+		return 0;
+
+	c_can_pm_runtime_get_sync(priv);
+	/* Initialize DCAN RAM */
+	c_can_reset_ram(priv, true);
+
+	/* Clear PDR and INIT bits */
+	priv->write_reg(priv, C_CAN_CTRL_EX_REG,
+		priv->read_reg(priv, C_CAN_CTRL_EX_REG) & ~CONTROL_EX_PDR);
+	priv->write_reg(priv, C_CAN_CTRL_REG,
+		priv->read_reg(priv, C_CAN_CTRL_REG) & ~CONTROL_INIT);
+
+	/* Wait for the PDA bit to get clear */
+	time_out = jiffies + msecs_to_jiffies(INIT_WAIT_COUNT);
+	while ((priv->read_reg(priv, C_CAN_STS_REG) & STATUS_PDA) &&
+				time_after(time_out, jiffies))
+		cpu_relax();
+
+	if (time_after(jiffies, time_out))
+		return -ETIMEDOUT;
+
+	c_can_start(dev);
+

drivers/net/can/c_can/c_can.c struct net_device *alloc_c_can_dev(void) · line 1232

Quoted 6 lines

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

+	return 0;
+}
+EXPORT_SYMBOL_GPL(c_can_power_up);
+#else
+#define c_can_power_down NULL
+#define c_can_power_up NULL

These are not used, are they?

Quoted 50 lines

drivers/net/can/c_can/c_can.c +2 -0

+#endif
+
 void free_c_can_dev(struct net_device *dev)
 {
 	free_candev(dev);

drivers/net/can/c_can/c_can.h +5 -0

index 5f6339c..e5dd7ef 100644
@@ -24,6 +24,7 @@
 
 enum reg {
 	C_CAN_CTRL_REG = 0,
+	C_CAN_CTRL_EX_REG,
 	C_CAN_STS_REG,
 	C_CAN_ERR_CNT_REG,
 	C_CAN_BTR_REG,

@@ -104,6 +105,7 @@ static const u16 reg_map_c_can[] = {
 
 static const u16 reg_map_d_can[] = {
 	[C_CAN_CTRL_REG]	= 0x00,
+	[C_CAN_CTRL_EX_REG]	= 0x02,
 	[C_CAN_STS_REG]		= 0x04,
 	[C_CAN_ERR_CNT_REG]	= 0x08,
 	[C_CAN_BTR_REG]		= 0x0C,

@@ -166,6 +168,7 @@ struct c_can_priv {
 	unsigned int tx_echo;
 	void *priv;		/* for board-specific data */
 	u16 irqstatus;
+	bool is_opened;
 	unsigned int instance;
 	void (*ram_init) (unsigned int instance, bool enable);
 };

@@ -174,5 +177,7 @@ struct net_device *alloc_c_can_dev(void);
 void free_c_can_dev(struct net_device *dev);
 int register_c_can_dev(struct net_device *dev);
 void unregister_c_can_dev(struct net_device *dev);
+int c_can_power_up(struct net_device *dev);
+int c_can_power_down(struct net_device *dev);
 
 #endif /* C_CAN_H */

drivers/net/can/c_can/c_can_platform.c +0 -0

index c6963b2..65ec232 100644
@@ -255,15 +255,79 @@ static int __devexit c_can_plat_remove(struct platform_device *pdev)
 	return 0;
 }
 

drivers/net/can/c_can/c_can_platform.c static int __devexit c_can_plat_remove(struct platform_device *pdev) · line 264

Quoted 7 lines

drivers/net/can/c_can/c_can_platform.c +7 -0

+#ifdef CONFIG_PM
+static int c_can_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	int ret;
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct c_can_priv *priv = netdev_priv(ndev);
+	const struct platform_device_id *id = platform_get_device_id(pdev);

Does that work on DT probed devices? You probably have to save the id->driver_data in struct c_can_priv.

Quoted 5 lines

drivers/net/can/c_can/c_can_platform.c +4 -0

+
+	if (id->driver_data != BOSCH_D_CAN) {
+		dev_warn(&pdev->dev, "Not supported\n");
+		return 0;

drivers/net/can/c_can/c_can_platform.c static int __devexit c_can_plat_remove(struct platform_device *pdev) · line 278

Quoted 10 lines

drivers/net/can/c_can/c_can_platform.c +10 -0

+	}
+
+	if (netif_running(ndev)) {
+		netif_stop_queue(ndev);
+		netif_device_detach(ndev);
+	}
+
+	ret = c_can_power_down(ndev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enter power down mode\n");

netdev_err

drivers/net/can/c_can/c_can_platform.c static int __devexit c_can_plat_remove(struct platform_device *pdev) · line 290

Quoted 12 lines

drivers/net/can/c_can/c_can_platform.c +12 -0

+		return ret;
+	}
+
+	priv->can.state = CAN_STATE_SLEEPING;
+
+	return 0;
+}
+
+static int c_can_resume(struct platform_device *pdev)
+{
+	int ret;
+

please remove the empty line ^^

Quoted 6 lines

drivers/net/can/c_can/c_can_platform.c +6 -0

+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct c_can_priv *priv = netdev_priv(ndev);
+	const struct platform_device_id *id = platform_get_device_id(pdev);
+
+	if (id->driver_data != BOSCH_D_CAN) {
+		dev_warn(&pdev->dev, "Not supported\n");

drivers/net/can/c_can/c_can_platform.c static int __devexit c_can_plat_remove(struct platform_device *pdev) · line 302

Quoted 6 lines

drivers/net/can/c_can/c_can_platform.c +6 -0

+		return 0;
+	}
+
+	ret = c_can_power_up(ndev);
+	if (ret) {
+		dev_err(&pdev->dev, "Still in power down mode\n");

netdev_err

Quoted 27 lines

drivers/net/can/c_can/c_can_platform.c +17 -3

+		return ret;
+	}
+
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+	if (netif_running(ndev)) {
+		netif_device_attach(ndev);
+		netif_start_queue(ndev);
+	}
+
+	return 0;
+}
+#else
+#define c_can_suspend NULL
+#define c_can_resume NULL
+#endif
+
 static struct platform_driver c_can_plat_driver = {
 	.driver = {
 		.name = KBUILD_MODNAME,
 		.owner = THIS_MODULE,
 		.of_match_table = of_match_ptr(c_can_of_table),
 	},
-	.probe = c_can_plat_probe,
-	.remove = __devexit_p(c_can_plat_remove),
-	.id_table = c_can_id_table,

drivers/net/can/c_can/c_can_platform.c static int __devexit c_can_plat_remove(struct platform_device *pdev) · line 330

Quoted 5 lines

drivers/net/can/c_can/c_can_platform.c +5 -0

+	.probe		= c_can_plat_probe,
+	.remove		= __devexit_p(c_can_plat_remove),
+	.suspend	= c_can_suspend,
+	.resume		= c_can_resume,
+	.id_table	= c_can_id_table,

Please don't indent here with tab. Just stay with one space on both sides of the "=".

Marc,

Thanks for the comments,

Quoted 11 lines · Marc Kleine-Budde

Quoted 6 lines · AnilKumar Ch

Adds suspend resume support to DCAN driver which enables DCAN power down mode bit (PDR). Then DCAN will ack the local power-down mode by setting PDA bit in STATUS register.

Also adds a status flag to know the status of DCAN module, whether it is opened or not.

Use "ndev->flags & IFF_UP" for that. Have a look at the at91_can driver [1]. I'm not sure if you need locking here.

Then I can use this to check the status, lock is not required.

Quoted 35 lines

[1] http://lxr.free-electrons.com/source/drivers/net/can/at91_can.c#L1198

Quoted 33 lines

Signed-off-by AnilKumar Ch <[email protected]>

---
 drivers/net/can/c_can/c_can.c          |   78 ++++++++++++++++++++++++++++++++
 drivers/net/can/c_can/c_can.h          |    5 ++
 drivers/net/can/c_can/c_can_platform.c |   70 ++++++++++++++++++++++++++--
 3 files changed, 150 insertions(+), 3 deletions(-)

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

index c175410..36d5db4 100644
@@ -46,6 +46,9 @@
 #define IF_ENUM_REG_LEN		11
 #define C_CAN_IFACE(reg, iface)	(C_CAN_IF1_##reg + (iface) * IF_ENUM_REG_LEN)
 
+/* control extension register D_CAN specific */
+#define CONTROL_EX_PDR		BIT(8)
+
 /* control register */
 #define CONTROL_TEST		BIT(7)
 #define CONTROL_CCE		BIT(6)

@@ -65,6 +68,7 @@
 #define TEST_BASIC		BIT(2)
 
 /* status register */
+#define STATUS_PDA		BIT(10)
 #define STATUS_BOFF		BIT(7)
 #define STATUS_EWARN		BIT(6)
 #define STATUS_EPASS		BIT(5)

@@ -164,6 +168,9 @@
 /* minimum timeout for checking BUSY status */
 #define MIN_TIMEOUT_VALUE	6
 

Quoted 5 lines

drivers/net/can/c_can/c_can.c line 172

Quoted 2 lines

drivers/net/can/c_can/c_can.c +2 -0

+/* Wait for ~1 sec for INIT bit */
+#define INIT_WAIT_COUNT		1000

What unit? INIT_WAIT_MS would be a better name.

Sure, will change

Quoted 24 lines

Quoted 24 lines

drivers/net/can/c_can/c_can.c +3 -0

+
 /* napi related */
 #define C_CAN_NAPI_WEIGHT	C_CAN_MSG_OBJ_RX_NUM
 

@@ -1102,6 +1109,7 @@ static int c_can_open(struct net_device *dev)
 
 	netif_start_queue(dev);
 
+	priv->is_opened = true;
 	return 0;
 
 exit_irq_fail:

@@ -1126,6 +1134,7 @@ static int c_can_close(struct net_device *dev)
 	/* De-Initialize DCAN RAM */
 	c_can_reset_ram(priv, false);
 	c_can_pm_runtime_put_sync(priv);
+	priv->is_opened = false;
 
 	return 0;
 }

@@ -1154,6 +1163,75 @@ struct net_device *alloc_c_can_dev(void)
 }
 EXPORT_SYMBOL_GPL(alloc_c_can_dev);
 

Quoted 10 lines

drivers/net/can/c_can/c_can.c struct net_device *alloc_c_can_dev(void) · line 1173

Quoted 8 lines

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

+#ifdef CONFIG_PM
+int c_can_power_down(struct net_device *dev)
+{
+	unsigned long time_out;
+	struct c_can_priv *priv = netdev_priv(dev);
+
+	if (!priv->is_opened)
+		return 0;

Should we add a BUG_ON(id->driver_data != BOSCH_D_CAN)?

These APIs are called from platform driver where device type device type is verified. I think we have to add BUG_ON() in platform driver.

Quoted 7 lines

drivers/net/can/c_can/c_can.c struct net_device *alloc_c_can_dev(void) · line 1177

Quoted 4 lines

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

+
+	/* set `PDR` value so the device goes to power down mode */
+	priv->write_reg(priv, C_CAN_CTRL_EX_REG,
+		priv->read_reg(priv, C_CAN_CTRL_EX_REG) | CONTROL_EX_PDR);

Please use an intermediate variable:

Quoted 5 lines

u32 val;

val = priv->read_reg(priv, C_CAN_CTRL_EX_REG);
val |= CONTROL_EX_PDR;
priv->write_reg(priv, C_CAN_CTRL_EX_REG, val);

I will change

Quoted 53 lines

Quoted 16 lines

drivers/net/can/c_can/c_can.c +15 -0

+
+	/* Wait for the PDA bit to get set */
+	time_out = jiffies + msecs_to_jiffies(INIT_WAIT_COUNT);
+	while (!(priv->read_reg(priv, C_CAN_STS_REG) & STATUS_PDA) &&
+				time_after(time_out, jiffies))
+		cpu_relax();
+
+	if (time_after(jiffies, time_out))
+		return -ETIMEDOUT;
+
+	c_can_stop(dev);
+
+	/* De-initialize DCAN RAM */
+	c_can_reset_ram(priv, false);
+	c_can_pm_runtime_put_sync(priv);

drivers/net/can/c_can/c_can.c struct net_device *alloc_c_can_dev(void) · line 1202

Quoted 10 lines

drivers/net/can/c_can/c_can.c +10 -0

+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(c_can_power_down);
+
+int c_can_power_up(struct net_device *dev)
+{
+	unsigned long time_out;
+	struct c_can_priv *priv = netdev_priv(dev);
+

BUG_ON?

Quoted 24 lines

drivers/net/can/c_can/c_can.c +24 -0

+	if (!priv->is_opened)
+		return 0;
+
+	c_can_pm_runtime_get_sync(priv);
+	/* Initialize DCAN RAM */
+	c_can_reset_ram(priv, true);
+
+	/* Clear PDR and INIT bits */
+	priv->write_reg(priv, C_CAN_CTRL_EX_REG,
+		priv->read_reg(priv, C_CAN_CTRL_EX_REG) & ~CONTROL_EX_PDR);
+	priv->write_reg(priv, C_CAN_CTRL_REG,
+		priv->read_reg(priv, C_CAN_CTRL_REG) & ~CONTROL_INIT);
+
+	/* Wait for the PDA bit to get clear */
+	time_out = jiffies + msecs_to_jiffies(INIT_WAIT_COUNT);
+	while ((priv->read_reg(priv, C_CAN_STS_REG) & STATUS_PDA) &&
+				time_after(time_out, jiffies))
+		cpu_relax();
+
+	if (time_after(jiffies, time_out))
+		return -ETIMEDOUT;
+
+	c_can_start(dev);
+

Quoted 8 lines

drivers/net/can/c_can/c_can.c struct net_device *alloc_c_can_dev(void) · line 1232

Quoted 6 lines

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

+	return 0;
+}
+EXPORT_SYMBOL_GPL(c_can_power_up);
+#else
+#define c_can_power_down NULL
+#define c_can_power_up NULL

These are not used, are they?

Not used, I will remove this else part and adding
#ifdef CONFIG_PM in header file as well.

Quoted 49 lines

Quoted 49 lines

drivers/net/can/c_can/c_can.c +2 -0

+#endif
+
 void free_c_can_dev(struct net_device *dev)
 {
 	free_candev(dev);

drivers/net/can/c_can/c_can.h +5 -0

index 5f6339c..e5dd7ef 100644
@@ -24,6 +24,7 @@
 
 enum reg {
 	C_CAN_CTRL_REG = 0,
+	C_CAN_CTRL_EX_REG,
 	C_CAN_STS_REG,
 	C_CAN_ERR_CNT_REG,
 	C_CAN_BTR_REG,

@@ -104,6 +105,7 @@ static const u16 reg_map_c_can[] = {
 
 static const u16 reg_map_d_can[] = {
 	[C_CAN_CTRL_REG]	= 0x00,
+	[C_CAN_CTRL_EX_REG]	= 0x02,
 	[C_CAN_STS_REG]		= 0x04,
 	[C_CAN_ERR_CNT_REG]	= 0x08,
 	[C_CAN_BTR_REG]		= 0x0C,

@@ -166,6 +168,7 @@ struct c_can_priv {
 	unsigned int tx_echo;
 	void *priv;		/* for board-specific data */
 	u16 irqstatus;
+	bool is_opened;
 	unsigned int instance;
 	void (*ram_init) (unsigned int instance, bool enable);
 };

@@ -174,5 +177,7 @@ struct net_device *alloc_c_can_dev(void);
 void free_c_can_dev(struct net_device *dev);
 int register_c_can_dev(struct net_device *dev);
 void unregister_c_can_dev(struct net_device *dev);
+int c_can_power_up(struct net_device *dev);
+int c_can_power_down(struct net_device *dev);
 
 #endif /* C_CAN_H */

drivers/net/can/c_can/c_can_platform.c +0 -0

index c6963b2..65ec232 100644
@@ -255,15 +255,79 @@ static int __devexit c_can_plat_remove(struct platform_device *pdev)
 	return 0;
 }
 

Quoted 10 lines

drivers/net/can/c_can/c_can_platform.c static int __devexit c_can_plat_remove(struct platform_device *pdev) · line 264

Quoted 7 lines

drivers/net/can/c_can/c_can_platform.c +7 -0

+#ifdef CONFIG_PM
+static int c_can_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	int ret;
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct c_can_priv *priv = netdev_priv(ndev);
+	const struct platform_device_id *id = platform_get_device_id(pdev);

Does that work on DT probed devices? You probably have to save the id->driver_data in struct c_can_priv.

I will add extra variable to c_can_priv to save the dev_id so that it will work for dt case as well.

Quoted 16 lines

Quoted 5 lines

drivers/net/can/c_can/c_can_platform.c +4 -0

+
+	if (id->driver_data != BOSCH_D_CAN) {
+		dev_warn(&pdev->dev, "Not supported\n");
+		return 0;

drivers/net/can/c_can/c_can_platform.c static int __devexit c_can_plat_remove(struct platform_device *pdev) · line 278

Quoted 10 lines

drivers/net/can/c_can/c_can_platform.c +10 -0

+	}
+
+	if (netif_running(ndev)) {
+		netif_stop_queue(ndev);
+		netif_device_detach(ndev);
+	}
+
+	ret = c_can_power_down(ndev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enter power down mode\n");

netdev_err

Quoted 13 lines

drivers/net/can/c_can/c_can_platform.c static int __devexit c_can_plat_remove(struct platform_device *pdev) · line 290

Quoted 12 lines

drivers/net/can/c_can/c_can_platform.c +12 -0

+		return ret;
+	}
+
+	priv->can.state = CAN_STATE_SLEEPING;
+
+	return 0;
+}
+
+static int c_can_resume(struct platform_device *pdev)
+{
+	int ret;
+

please remove the empty line ^^

Sure

Quoted 6 lines

Quoted 6 lines

drivers/net/can/c_can/c_can_platform.c +6 -0

+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct c_can_priv *priv = netdev_priv(ndev);
+	const struct platform_device_id *id = platform_get_device_id(pdev);
+
+	if (id->driver_data != BOSCH_D_CAN) {
+		dev_warn(&pdev->dev, "Not supported\n");

Quoted 8 lines

drivers/net/can/c_can/c_can_platform.c static int __devexit c_can_plat_remove(struct platform_device *pdev) · line 302

Quoted 6 lines

drivers/net/can/c_can/c_can_platform.c +6 -0

+		return 0;
+	}
+
+	ret = c_can_power_up(ndev);
+	if (ret) {
+		dev_err(&pdev->dev, "Still in power down mode\n");

netdev_err

I will change.

Quoted 27 lines

Quoted 27 lines

drivers/net/can/c_can/c_can_platform.c +17 -3

+		return ret;
+	}
+
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+	if (netif_running(ndev)) {
+		netif_device_attach(ndev);
+		netif_start_queue(ndev);
+	}
+
+	return 0;
+}
+#else
+#define c_can_suspend NULL
+#define c_can_resume NULL
+#endif
+
 static struct platform_driver c_can_plat_driver = {
 	.driver = {
 		.name = KBUILD_MODNAME,
 		.owner = THIS_MODULE,
 		.of_match_table = of_match_ptr(c_can_of_table),
 	},
-	.probe = c_can_plat_probe,
-	.remove = __devexit_p(c_can_plat_remove),
-	.id_table = c_can_id_table,

Quoted 9 lines

drivers/net/can/c_can/c_can_platform.c static int __devexit c_can_plat_remove(struct platform_device *pdev) · line 330

Quoted 5 lines

drivers/net/can/c_can/c_can_platform.c +5 -0

+	.probe		= c_can_plat_probe,
+	.remove		= __devexit_p(c_can_plat_remove),
+	.suspend	= c_can_suspend,
+	.resume		= c_can_resume,
+	.id_table	= c_can_id_table,

Please don't indent here with tab. Just stay with one space on both sides of the "=".

Point taken

Thanks
AnilKumar

Quoted 28 lines · Chimata

Marc,

Thanks for the comments,

Quoted 11 lines · Marc Kleine-Budde

Quoted 6 lines · AnilKumar Ch

Adds suspend resume support to DCAN driver which enables DCAN power down mode bit (PDR). Then DCAN will ack the local power-down mode by setting PDA bit in STATUS register.

Also adds a status flag to know the status of DCAN module, whether it is opened or not.

Use "ndev->flags & IFF_UP" for that. Have a look at the at91_can driver [1]. I'm not sure if you need locking here.

Then I can use this to check the status, lock is not required.

Quoted 8 lines

[1] http://lxr.free-electrons.com/source/drivers/net/can/at91_can.c#L1198

Quoted 6 lines

Signed-off-by AnilKumar Ch <[email protected]>

---
 drivers/net/can/c_can/c_can.c          |   78 ++++++++++++++++++++++++++++++++
 drivers/net/can/c_can/c_can.h          |    5 ++
 drivers/net/can/c_can/c_can_platform.c |   70 ++++++++++++++++++++++++++--
 3 files changed, 150 insertions(+), 3 deletions(-)

Quoted 11 lines

Quoted 10 lines

drivers/net/can/c_can/c_can.c struct net_device *alloc_c_can_dev(void) · line 1173

Quoted 8 lines

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

+#ifdef CONFIG_PM
+int c_can_power_down(struct net_device *dev)
+{
+	unsigned long time_out;
+	struct c_can_priv *priv = netdev_priv(dev);
+
+	if (!priv->is_opened)
+		return 0;

Should we add a BUG_ON(id->driver_data != BOSCH_D_CAN)?

Quoted 3 lines

These APIs are called from platform driver where device type device type is verified. I think we have to add BUG_ON() in platform driver.

The platform driver returns if not on D_CAN and will not call this function. But this functions are exported, so can be called by someone else. So if the caller is not D_CAN, it's a bug.

Hi Marc,

Quoted 29 lines · Marc Kleine-Budde

Quoted 28 lines · Chimata

Marc,

Thanks for the comments,

Quoted 11 lines · Marc Kleine-Budde

Quoted 6 lines · AnilKumar Ch

Adds suspend resume support to DCAN driver which enables DCAN power down mode bit (PDR). Then DCAN will ack the local power-down mode by setting PDA bit in STATUS register.

Also adds a status flag to know the status of DCAN module, whether it is opened or not.

Use "ndev->flags & IFF_UP" for that. Have a look at the at91_can driver [1]. I'm not sure if you need locking here.

Then I can use this to check the status, lock is not required.

Quoted 8 lines

[1] http://lxr.free-electrons.com/source/drivers/net/can/at91_can.c#L1198

Quoted 6 lines

Signed-off-by AnilKumar Ch <[email protected]>

---
 drivers/net/can/c_can/c_can.c          |   78 ++++++++++++++++++++++++++++++++
 drivers/net/can/c_can/c_can.h          |    5 ++
 drivers/net/can/c_can/c_can_platform.c |   70 ++++++++++++++++++++++++++--
 3 files changed, 150 insertions(+), 3 deletions(-)

[snip]

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

drivers/net/can/c_can/c_can.c struct net_device *alloc_c_can_dev(void) · line 1173

Quoted 8 lines

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

+#ifdef CONFIG_PM
+int c_can_power_down(struct net_device *dev)
+{
+	unsigned long time_out;
+	struct c_can_priv *priv = netdev_priv(dev);
+
+	if (!priv->is_opened)
+		return 0;

Should we add a BUG_ON(id->driver_data != BOSCH_D_CAN)?

Quoted 8 lines

Quoted 3 lines

These APIs are called from platform driver where device type device type is verified. I think we have to add BUG_ON() in platform driver.

The platform driver returns if not on D_CAN and will not call this function. But this functions are exported, so can be called by someone else. So if the caller is not D_CAN, it's a bug.

I agree with you, but I have some concern here.
I think we should do "return 0;" instead of BUG_ON(). With BUG_ON() system will hang and ultimately user lost his/her contents.

Quoted 55 lines · Chimata

Hi Marc,

Quoted 29 lines · Marc Kleine-Budde

Quoted 28 lines · Chimata

Marc,

Thanks for the comments,

Quoted 11 lines · Marc Kleine-Budde

Quoted 6 lines · AnilKumar Ch

Adds suspend resume support to DCAN driver which enables DCAN power down mode bit (PDR). Then DCAN will ack the local power-down mode by setting PDA bit in STATUS register.

Also adds a status flag to know the status of DCAN module, whether it is opened or not.

Use "ndev->flags & IFF_UP" for that. Have a look at the at91_can driver [1]. I'm not sure if you need locking here.

Then I can use this to check the status, lock is not required.

Quoted 8 lines

[1] http://lxr.free-electrons.com/source/drivers/net/can/at91_can.c#L1198

Quoted 6 lines

Signed-off-by AnilKumar Ch <[email protected]>

---
 drivers/net/can/c_can/c_can.c          |   78 ++++++++++++++++++++++++++++++++
 drivers/net/can/c_can/c_can.h          |    5 ++
 drivers/net/can/c_can/c_can_platform.c |   70 ++++++++++++++++++++++++++--
 3 files changed, 150 insertions(+), 3 deletions(-)

[snip]

Quoted 19 lines

Quoted 11 lines

Quoted 10 lines

drivers/net/can/c_can/c_can.c struct net_device *alloc_c_can_dev(void) · line 1173

Quoted 8 lines

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

+#ifdef CONFIG_PM
+int c_can_power_down(struct net_device *dev)
+{
+	unsigned long time_out;
+	struct c_can_priv *priv = netdev_priv(dev);
+
+	if (!priv->is_opened)
+		return 0;

Should we add a BUG_ON(id->driver_data != BOSCH_D_CAN)?

Quoted 3 lines

These APIs are called from platform driver where device type device type is verified. I think we have to add BUG_ON() in platform driver.

The platform driver returns if not on D_CAN and will not call this function. But this functions are exported, so can be called by someone else. So if the caller is not D_CAN, it's a bug.

Quoted 4 lines

I agree with you, but I have some concern here.
I think we should do "return 0;" instead of BUG_ON(). With BUG_ON() system will hang and ultimately user lost his/her contents.

Good point, better safe then sorry.
But this should not happen. What about WARN_ON()?

Marc,

Quoted 61 lines · Marc Kleine-Budde

Quoted 59 lines · Chimata

Hi Marc,

Quoted 29 lines · Marc Kleine-Budde

Quoted 28 lines · Chimata

Marc,

Thanks for the comments,

Quoted 11 lines · Marc Kleine-Budde

Quoted 6 lines · AnilKumar Ch

Adds suspend resume support to DCAN driver which enables DCAN power down mode bit (PDR). Then DCAN will ack the local power-down mode by setting PDA bit in STATUS register.

Also adds a status flag to know the status of DCAN module, whether it is opened or not.

Use "ndev->flags & IFF_UP" for that. Have a look at the at91_can driver [1]. I'm not sure if you need locking here.

Then I can use this to check the status, lock is not required.

Quoted 8 lines

[1] http://lxr.free-electrons.com/source/drivers/net/can/at91_can.c#L1198

Quoted 6 lines

Signed-off-by AnilKumar Ch <[email protected]>

---
 drivers/net/can/c_can/c_can.c          |   78 ++++++++++++++++++++++++++++++++
 drivers/net/can/c_can/c_can.h          |    5 ++
 drivers/net/can/c_can/c_can_platform.c |   70 ++++++++++++++++++++++++++--
 3 files changed, 150 insertions(+), 3 deletions(-)

[snip]

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+#ifdef CONFIG_PM
+int c_can_power_down(struct net_device *dev)
+{
+	unsigned long time_out;
+	struct c_can_priv *priv = netdev_priv(dev);
+
+	if (!priv->is_opened)
+		return 0;

Should we add a BUG_ON(id->driver_data != BOSCH_D_CAN)?

Quoted 8 lines

Quoted 3 lines

These APIs are called from platform driver where device type device type is verified. I think we have to add BUG_ON() in platform driver.

The platform driver returns if not on D_CAN and will not call this function. But this functions are exported, so can be called by someone else. So if the caller is not D_CAN, it's a bug.

I agree with you, but I have some concern here.
I think we should do "return 0;" instead of BUG_ON(). With BUG_ON() system will hang and ultimately user lost his/her contents.

Quoted 3 lines

Good point, better safe then sorry.
But this should not happen. What about WARN_ON()?

Either would be fine printing a warning message or WARN_ON()