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

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

[Alexander Shiyan]

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

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 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..26879c7 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)
+		return ret;
+
 	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.8.3.2

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

[Alexander Shiyan]

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>
---
 drivers/net/can/mcp251x.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c
index 26879c7..fea41b4 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) */
@@ -625,28 +627,20 @@ static int mcp251x_hw_reset(struct spi_device *spi)
 {
 	struct mcp251x_priv *priv = spi_get_drvdata(spi);
 	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 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;
+	/* Wait for oscillator startup timer after reset */
+	mdelay(MCP251X_OST_DELAY_MS);
+	
+	return ((mcp251x_read_reg(spi, CANSTAT) & CANCTRL_REQOP_MASK) ==
+		CANCTRL_REQOP_CONF) ? 0 : -ENODEV;
 }
 
 static int mcp251x_hw_probe(struct spi_device *spi)
@@ -776,7 +770,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.8.3.2

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

[Alexander Shiyan]

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>
---
 drivers/net/can/mcp251x.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c
index fea41b4..f4786c8 100644
--- a/drivers/net/can/mcp251x.c
+++ b/drivers/net/can/mcp251x.c
@@ -645,23 +645,19 @@ 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, "CANSTAT 0x%02x CANCTRL 0x%02x\n", st1, st2);
+	dev_dbg(&spi->dev, "CANCTRL 0x%02x\n", ctrl);
 
-	/* Check for power up default values */
-	return (st1 == 0x80 && st2 == 0x07) ? 1 : 0;
+	/* Check for power up default value */
+	return ((ctrl & 0x17) == 0x07) ? 0 : -ENODEV;
 }
 
 static int mcp251x_power_enable(struct regulator *reg, int enable)
@@ -1138,19 +1134,18 @@ 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;
-		goto error_probe;
-	}
-	mcp251x_hw_sleep(spi);
+	ret = mcp251x_hw_probe(spi);
+	if (!ret) {
+		mcp251x_hw_sleep(spi);
 
-	ret = register_candev(net);
-	if (ret)
-		goto error_probe;
+		ret = register_candev(net);
+		if (ret)
+			goto error_probe;
 
-	devm_can_led_init(net);
+		devm_can_led_init(net);
 
-	return ret;
+		return 0;
+	}
 
 error_probe:
 	if (mcp251x_enable_dma)
-- 
1.8.3.2

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

[Alexander Shiyan]

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>
---
 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 f4786c8..54a57b0 100644
--- a/drivers/net/can/mcp251x.c
+++ b/drivers/net/can/mcp251x.c
@@ -1151,7 +1151,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))
@@ -1175,8 +1174,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.8.3.2

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

[Marc Kleine-Budde]

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

On 03/28/2014 11:14 AM, Alexander Shiyan wrote:
> This patch moves setup of SPI bus a bit earlier and adds check
> for spi_setup() result to be sure SPI bus is communicated with the
> device properly.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  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..26879c7 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)
> +		return ret;

You're omitting the error handling here. I'll send a v2.

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

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 1/4] can: mcp251x: Check return value of spi_setup()

[Alexander Shiyan]

Tue, 01 Apr 2014 13:56:41 +0200 от Marc Kleine-Budde <mkl@pengutronix.de>:
> On 03/28/2014 11:14 AM, Alexander Shiyan wrote:
> > This patch moves setup of SPI bus a bit earlier and adds check
> > for spi_setup() result to be sure SPI bus is communicated with the
> > device properly.
> > 
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > ---
> >  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..26879c7 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)
> > +		return ret;
> 
> You're omitting the error handling here. I'll send a v2.

spi_setup() returns 0 or an error. What is wrong here?

---

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

[Marc Kleine-Budde]

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

On 04/01/2014 02:47 PM, Alexander Shiyan wrote:
> Tue, 01 Apr 2014 13:56:41 +0200 от Marc Kleine-Budde <mkl@pengutronix.de>:
>> On 03/28/2014 11:14 AM, Alexander Shiyan wrote:
>>> This patch moves setup of SPI bus a bit earlier and adds check
>>> for spi_setup() result to be sure SPI bus is communicated with the
>>> device properly.
>>>
>>> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
>>> ---
>>>  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..26879c7 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)
>>> +		return ret;
>>
>> You're omitting the error handling here. I'll send a v2.
> 
> spi_setup() returns 0 or an error. What is wrong here?

You're not cleaning up the allocated resources.

-               return ret;
+               return out_clk;

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 1/4] can: mcp251x: Check return value of spi_setup()

[Alexander Shiyan]

Tue, 01 Apr 2014 14:49:30 +0200 от Marc Kleine-Budde <mkl@pengutronix.de>:
> On 04/01/2014 02:47 PM, Alexander Shiyan wrote:
> > Tue, 01 Apr 2014 13:56:41 +0200 от Marc Kleine-Budde <mkl@pengutronix.de>:
> >> On 03/28/2014 11:14 AM, Alexander Shiyan wrote:
> >>> This patch moves setup of SPI bus a bit earlier and adds check
> >>> for spi_setup() result to be sure SPI bus is communicated with the
> >>> device properly.
> >>>
> >>> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> >>> ---
> >>>  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..26879c7 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)
> >>> +		return ret;
> >>
> >> You're omitting the error handling here. I'll send a v2.
> > 
> > spi_setup() returns 0 or an error. What is wrong here?
> 
> You're not cleaning up the allocated resources.
> 
> -               return ret;
> +               return out_clk;

Oh, yes, you are right.

---

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

[Marc Kleine-Budde]

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

On 03/28/2014 11:14 AM, Alexander Shiyan wrote:
> This patch remove unnecessary power disabling on error and on module
> exit since this is already done by regulator API.

Oh really? Can you point me to the commit in mainline which introduces
this change? As far as I understand devm just callss
devm_regulator_release(), which calls regulator_put().

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 1/4] can: mcp251x: Check return value of spi_setup()

[Marc Kleine-Budde]

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

On 04/01/2014 02:54 PM, Alexander Shiyan wrote:
> Tue, 01 Apr 2014 14:49:30 +0200 от Marc Kleine-Budde <mkl@pengutronix.de>:
>> On 04/01/2014 02:47 PM, Alexander Shiyan wrote:
>>> Tue, 01 Apr 2014 13:56:41 +0200 от Marc Kleine-Budde <mkl@pengutronix.de>:
>>>> On 03/28/2014 11:14 AM, Alexander Shiyan wrote:
>>>>> This patch moves setup of SPI bus a bit earlier and adds check
>>>>> for spi_setup() result to be sure SPI bus is communicated with the
>>>>> device properly.
>>>>>
>>>>> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
>>>>> ---
>>>>>  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..26879c7 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)
>>>>> +		return ret;
>>>>
>>>> You're omitting the error handling here. I'll send a v2.
>>>
>>> spi_setup() returns 0 or an error. What is wrong here?
>>
>> You're not cleaning up the allocated resources.
>>
>> -               return ret;
>> +               return out_clk;

Of course it's 'goto out_clk'.

I'll send a v2.

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 3/4] can: mcp251x: Improve mcp251x_hw_probe()

[Marc Kleine-Budde]

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

On 03/28/2014 11:14 AM, Alexander Shiyan wrote:
> 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>
> ---
>  drivers/net/can/mcp251x.c | 41 ++++++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c
> index fea41b4..f4786c8 100644
> --- a/drivers/net/can/mcp251x.c
> +++ b/drivers/net/can/mcp251x.c
> @@ -645,23 +645,19 @@ 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, "CANSTAT 0x%02x CANCTRL 0x%02x\n", st1, st2);
> +	dev_dbg(&spi->dev, "CANCTRL 0x%02x\n", ctrl);
>  
> -	/* Check for power up default values */
> -	return (st1 == 0x80 && st2 == 0x07) ? 1 : 0;
> +	/* Check for power up default value */
> +	return ((ctrl & 0x17) == 0x07) ? 0 : -ENODEV;

I've converted this into:

+       if ((ctrl & 0x17) != 0x07)
+               return -ENODEV;
+
+       return 0;

...which is more readable, IMHO.

>  }
>  
>  static int mcp251x_power_enable(struct regulator *reg, int enable)
> @@ -1138,19 +1134,18 @@ 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;
> -		goto error_probe;
> -	}
> -	mcp251x_hw_sleep(spi);
> +	ret = mcp251x_hw_probe(spi);
> +	if (!ret) {

I don't like the idea due to readability, that the non-error path is
inside the if () { }. I've fixed this, the diff is even smaller.

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 2/4] can: mcp251x: Improve mcp251x_hw_reset()

[Marc Kleine-Budde]

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

On 03/28/2014 11:14 AM, Alexander Shiyan wrote:
> 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>
> ---
>  drivers/net/can/mcp251x.c | 33 +++++++++++++--------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c
> index 26879c7..fea41b4 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) */
> @@ -625,28 +627,20 @@ static int mcp251x_hw_reset(struct spi_device *spi)
>  {
>  	struct mcp251x_priv *priv = spi_get_drvdata(spi);
>  	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 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;
> +	/* Wait for oscillator startup timer after reset */
> +	mdelay(MCP251X_OST_DELAY_MS);
> +	
> +	return ((mcp251x_read_reg(spi, CANSTAT) & CANCTRL_REQOP_MASK) ==
> +		CANCTRL_REQOP_CONF) ? 0 : -ENODEV;

I've converted this into a more standard/readable:

+       reg = mcp251x_read_reg(spi, CANSTAT);
+       if ((reg & CANCTRL_REQOP_MASK) != CANCTRL_REQOP_CONF)
+               return -ENODEV;

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 4/4] can: mcp251x: Remove unnecessary power disabling

[Alexander Shiyan]

Tue, 01 Apr 2014 22:36:27 +0200 от Marc Kleine-Budde <mkl@pengutronix.de>:
> On 03/28/2014 11:14 AM, Alexander Shiyan wrote:
> > This patch remove unnecessary power disabling on error and on module
> > exit since this is already done by regulator API.
> 
> Oh really? Can you point me to the commit in mainline which introduces
> this change? As far as I understand devm just callss
> devm_regulator_release(), which calls regulator_put().

It seems that you are right. I thought that the regulator is switched off when
the usage count reaches zero, but it is not.
In this case, please drop the [4/4] patch.

---

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

[Marc Kleine-Budde]

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

On 04/02/2014 08:17 AM, Alexander Shiyan wrote:
> Tue, 01 Apr 2014 22:36:27 +0200 от Marc Kleine-Budde <mkl@pengutronix.de>:
>> On 03/28/2014 11:14 AM, Alexander Shiyan wrote:
>>> This patch remove unnecessary power disabling on error and on module
>>> exit since this is already done by regulator API.
>>
>> Oh really? Can you point me to the commit in mainline which introduces
>> this change? As far as I understand devm just callss
>> devm_regulator_release(), which calls regulator_put().
> 
> It seems that you are right. I thought that the regulator is switched off when
> the usage count reaches zero, but it is not.
> In this case, please drop the [4/4] patch.

Will do, I'll push the remaining three patches to the can-next as soon
as you give me your okay for the v2 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 --]