[PATCH v2 0/4] can: mcp251x: various improvements

[PATCH v2 0/4] can: mcp251x: various improvements

[Marc Kleine-Budde]

Hello,

I've picked up Alexander Shiyan's work.

Changes since v1:
- 1/4: mcp251x_can_probe(): do cleanup in case of spi_setup() fails
- 2/4: mcp251x_hw_reset(): convert ternary-? to if (err) return
- 3/4: mcp251x_hw_probe(): convert ternary-? to if (err) return

[PATCH v2 1/4] can: mcp251x: Check return value of spi_setup()

[Marc Kleine-Budde]

From: Alexander Shiyan <shc_work@mail.ru>

This patch moves setup of SPI bus a bit earlier and adds check for spi_setup()
result to be sure SPI bus is communicating with the device properly.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/mcp251x.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c
index 28c11f8..356d3da 100644
--- a/drivers/net/can/mcp251x.c
+++ b/drivers/net/can/mcp251x.c
@@ -1032,8 +1032,8 @@ static int mcp251x_can_probe(struct spi_device *spi)
 	struct mcp251x_platform_data *pdata = dev_get_platdata(&spi->dev);
 	struct net_device *net;
 	struct mcp251x_priv *priv;
-	int freq, ret = -ENODEV;
 	struct clk *clk;
+	int freq, ret;
 
 	clk = devm_clk_get(&spi->dev, NULL);
 	if (IS_ERR(clk)) {
@@ -1076,6 +1076,18 @@ static int mcp251x_can_probe(struct spi_device *spi)
 	priv->net = net;
 	priv->clk = clk;
 
+	spi_set_drvdata(spi, priv);
+
+	/* Configure the SPI bus */
+	spi->bits_per_word = 8;
+	if (mcp251x_is_2510(spi))
+		spi->max_speed_hz = spi->max_speed_hz ? : 5 * 1000 * 1000;
+	else
+		spi->max_speed_hz = spi->max_speed_hz ? : 10 * 1000 * 1000;
+	ret = spi_setup(spi);
+	if (ret)
+		goto out_clk;
+
 	priv->power = devm_regulator_get(&spi->dev, "vdd");
 	priv->transceiver = devm_regulator_get(&spi->dev, "xceiver");
 	if ((PTR_ERR(priv->power) == -EPROBE_DEFER) ||
@@ -1088,8 +1100,6 @@ static int mcp251x_can_probe(struct spi_device *spi)
 	if (ret)
 		goto out_clk;
 
-	spi_set_drvdata(spi, priv);
-
 	priv->spi = spi;
 	mutex_init(&priv->mcp_lock);
 
@@ -1134,15 +1144,6 @@ static int mcp251x_can_probe(struct spi_device *spi)
 
 	SET_NETDEV_DEV(net, &spi->dev);
 
-	/* Configure the SPI bus */
-	spi->mode = spi->mode ? : SPI_MODE_0;
-	if (mcp251x_is_2510(spi))
-		spi->max_speed_hz = spi->max_speed_hz ? : 5 * 1000 * 1000;
-	else
-		spi->max_speed_hz = spi->max_speed_hz ? : 10 * 1000 * 1000;
-	spi->bits_per_word = 8;
-	spi_setup(spi);
-
 	/* Here is OK to not lock the MCP, no one knows about it yet */
 	if (!mcp251x_hw_probe(spi)) {
 		ret = -ENODEV;
-- 
1.9.0.279.gdc9e3eb

[PATCH v2 2/4] can: mcp251x: Improve mcp251x_hw_reset()

[Marc Kleine-Budde]

From: Alexander Shiyan <shc_work@mail.ru>

The MCP251x utilizes an oscillator startup timer (OST), which holds the
chip in reset, to insure that the oscillator has stabilized before the
internal state machine begins to operate. The OST maintains reset for
the first 128 OSC clock cycles after power up or reset.
So, this patch removes unnecessary loops and reduce delay for power on
and reset to the safe value.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/mcp251x.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c
index 356d3da..9a8fd94 100644
--- a/drivers/net/can/mcp251x.c
+++ b/drivers/net/can/mcp251x.c
@@ -214,6 +214,8 @@
 
 #define TX_ECHO_SKB_MAX	1
 
+#define MCP251X_OST_DELAY_MS	(5)
+
 #define DEVICE_NAME "mcp251x"
 
 static int mcp251x_enable_dma; /* Enable SPI DMA. Default: 0 (Off) */
@@ -624,28 +626,24 @@ static int mcp251x_setup(struct net_device *net, struct mcp251x_priv *priv,
 static int mcp251x_hw_reset(struct spi_device *spi)
 {
 	struct mcp251x_priv *priv = spi_get_drvdata(spi);
+	u8 reg;
 	int ret;
-	unsigned long timeout;
+
+	/* Wait for oscillator startup timer after power up */
+	mdelay(MCP251X_OST_DELAY_MS);
 
 	priv->spi_tx_buf[0] = INSTRUCTION_RESET;
-	ret = spi_write(spi, priv->spi_tx_buf, 1);
-	if (ret) {
-		dev_err(&spi->dev, "reset failed: ret = %d\n", ret);
-		return -EIO;
-	}
+	ret = mcp251x_spi_trans(spi, 1);
+	if (ret)
+		return ret;
+
+	/* Wait for oscillator startup timer after reset */
+	mdelay(MCP251X_OST_DELAY_MS);
+	
+	reg = mcp251x_read_reg(spi, CANSTAT);
+	if ((reg & CANCTRL_REQOP_MASK) != CANCTRL_REQOP_CONF)
+		return -ENODEV;
 
-	/* Wait for reset to finish */
-	timeout = jiffies + HZ;
-	mdelay(10);
-	while ((mcp251x_read_reg(spi, CANSTAT) & CANCTRL_REQOP_MASK)
-	       != CANCTRL_REQOP_CONF) {
-		schedule();
-		if (time_after(jiffies, timeout)) {
-			dev_err(&spi->dev, "MCP251x didn't"
-				" enter in conf mode after reset\n");
-			return -EBUSY;
-		}
-	}
 	return 0;
 }
 
@@ -776,7 +774,6 @@ static void mcp251x_restart_work_handler(struct work_struct *ws)
 
 	mutex_lock(&priv->mcp_lock);
 	if (priv->after_suspend) {
-		mdelay(10);
 		mcp251x_hw_reset(spi);
 		mcp251x_setup(net, priv, spi);
 		if (priv->after_suspend & AFTER_SUSPEND_RESTART) {
-- 
1.9.0.279.gdc9e3eb

[PATCH v2 3/4] can: mcp251x: Improve mcp251x_hw_probe()

[Marc Kleine-Budde]

From: Alexander Shiyan <shc_work@mail.ru>

This patch adds check for mcp251x_hw_reset() result on startup and
removes unnecessary checking for CANSTAT register since this value
is being checked in mcp251x_hw_reset().

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/mcp251x.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c
index 9a8fd94..bc235f9 100644
--- a/drivers/net/can/mcp251x.c
+++ b/drivers/net/can/mcp251x.c
@@ -649,23 +649,22 @@ static int mcp251x_hw_reset(struct spi_device *spi)
 
 static int mcp251x_hw_probe(struct spi_device *spi)
 {
-	int st1, st2;
+	u8 ctrl;
+	int ret;
 
-	mcp251x_hw_reset(spi);
+	ret = mcp251x_hw_reset(spi);
+	if (ret)
+		return ret;
 
-	/*
-	 * Please note that these are "magic values" based on after
-	 * reset defaults taken from data sheet which allows us to see
-	 * if we really have a chip on the bus (we avoid common all
-	 * zeroes or all ones situations)
-	 */
-	st1 = mcp251x_read_reg(spi, CANSTAT) & 0xEE;
-	st2 = mcp251x_read_reg(spi, CANCTRL) & 0x17;
+	ctrl = mcp251x_read_reg(spi, CANCTRL);
+
+	dev_dbg(&spi->dev, "CANCTRL 0x%02x\n", ctrl);
 
-	dev_dbg(&spi->dev, "CANSTAT 0x%02x CANCTRL 0x%02x\n", st1, st2);
+	/* Check for power up default value */
+	if ((ctrl & 0x17) != 0x07)
+		return -ENODEV;
 
-	/* Check for power up default values */
-	return (st1 == 0x80 && st2 == 0x07) ? 1 : 0;
+	return 0;
 }
 
 static int mcp251x_power_enable(struct regulator *reg, int enable)
@@ -1142,10 +1141,10 @@ static int mcp251x_can_probe(struct spi_device *spi)
 	SET_NETDEV_DEV(net, &spi->dev);
 
 	/* Here is OK to not lock the MCP, no one knows about it yet */
-	if (!mcp251x_hw_probe(spi)) {
-		ret = -ENODEV;
+	ret = mcp251x_hw_probe(spi);
+	if (ret)
 		goto error_probe;
-	}
+
 	mcp251x_hw_sleep(spi);
 
 	ret = register_candev(net);
@@ -1154,7 +1153,7 @@ static int mcp251x_can_probe(struct spi_device *spi)
 
 	devm_can_led_init(net);
 
-	return ret;
+	return 0;
 
 error_probe:
 	if (mcp251x_enable_dma)
-- 
1.9.0.279.gdc9e3eb

[PATCH v2 4/4] can: mcp251x: Remove unnecessary power disabling

[Marc Kleine-Budde]

From: Alexander Shiyan <shc_work@mail.ru>

This patch remove unnecessary power disabling on error and on module
exit since this is already done by regulator API.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Hello,

I'm not sure if this actually works.

Marc

 drivers/net/can/mcp251x.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c
index bc235f9..1e294977 100644
--- a/drivers/net/can/mcp251x.c
+++ b/drivers/net/can/mcp251x.c
@@ -1159,7 +1159,6 @@ error_probe:
 	if (mcp251x_enable_dma)
 		dma_free_coherent(&spi->dev, PAGE_SIZE,
 				  priv->spi_tx_buf, priv->spi_tx_dma);
-	mcp251x_power_enable(priv->power, 0);
 
 out_clk:
 	if (!IS_ERR(clk))
@@ -1183,8 +1182,6 @@ static int mcp251x_can_remove(struct spi_device *spi)
 				  priv->spi_tx_buf, priv->spi_tx_dma);
 	}
 
-	mcp251x_power_enable(priv->power, 0);
-
 	if (!IS_ERR(priv->clk))
 		clk_disable_unprepare(priv->clk);
 
-- 
1.9.0.279.gdc9e3eb

Re: [PATCH v2 4/4] can: mcp251x: Remove unnecessary power disabling

[Marc Kleine-Budde]

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

On 04/01/2014 11:02 PM, Marc Kleine-Budde wrote:
> From: Alexander Shiyan <shc_work@mail.ru>
> 
> This patch remove unnecessary power disabling on error and on module
> exit since this is already done by regulator API.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
> Hello,
> 
> I'm not sure if this actually works.

Dropped as discussed.

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: 242 bytes --]

Re: [PATCH v2 0/4] can: mcp251x: various improvements

[Marc Kleine-Budde]

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

On 04/01/2014 11:02 PM, Marc Kleine-Budde wrote:
> I've picked up Alexander Shiyan's work.
> 
> Changes since v1:
> - 1/4: mcp251x_can_probe(): do cleanup in case of spi_setup() fails
> - 2/4: mcp251x_hw_reset(): convert ternary-? to if (err) return
> - 3/4: mcp251x_hw_probe(): convert ternary-? to if (err) return

Alexander, can you please test this series?

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: 242 bytes --]

Re: [PATCH v2 0/4] can: mcp251x: various improvements

[Alexander Shiyan]

Tue, 01 Apr 2014 23:03:27 +0200 от Marc Kleine-Budde <mkl@pengutronix.de>:
> On 04/01/2014 11:02 PM, Marc Kleine-Budde wrote:
> > I've picked up Alexander Shiyan's work.
> > 
> > Changes since v1:
> > - 1/4: mcp251x_can_probe(): do cleanup in case of spi_setup() fails
> > - 2/4: mcp251x_hw_reset(): convert ternary-? to if (err) return
> > - 3/4: mcp251x_hw_probe(): convert ternary-? to if (err) return
> 
> Alexander, can you please test this series?

Tested-by: Alexander Shiyan <shc_work@mail.ru>

---

Re: [PATCH v2 0/4] can: mcp251x: various improvements

[Marc Kleine-Budde]

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

On 04/02/2014 09:11 AM, Alexander Shiyan wrote:
> Tue, 01 Apr 2014 23:03:27 +0200 от Marc Kleine-Budde <mkl@pengutronix.de>:
>> On 04/01/2014 11:02 PM, Marc Kleine-Budde wrote:
>>> I've picked up Alexander Shiyan's work.
>>>
>>> Changes since v1:
>>> - 1/4: mcp251x_can_probe(): do cleanup in case of spi_setup() fails
>>> - 2/4: mcp251x_hw_reset(): convert ternary-? to if (err) return
>>> - 3/4: mcp251x_hw_probe(): convert ternary-? to if (err) return
>>
>> Alexander, can you please test this series?
> 
> Tested-by: Alexander Shiyan <shc_work@mail.ru>

Thanks

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: 242 bytes --]