[PATCH] can: mcp251x: add some messages to mcp251x_can_probe()

[PATCH] can: mcp251x: add some messages to mcp251x_can_probe()

[Ed Spiridonov]

Print error message if mcp251x_hw_probe() fails (e.g. MCP2515 isn't connected or other wiring issue).
Print information message if mcp251x_can_probe() is successful.
---
 drivers/net/can/spi/mcp251x.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index cf36d26..49453f7 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -1145,8 +1145,10 @@ static int mcp251x_can_probe(struct spi_device *spi)
 
 	/* Here is OK to not lock the MCP, no one knows about it yet */
 	ret = mcp251x_hw_probe(spi);
-	if (ret)
+	if (ret) {
+		dev_err(&spi->dev, "hw_probe failed, err=%d (wrong wiring?)\n", -ret);
 		goto error_probe;
+	}
 
 	mcp251x_hw_sleep(spi);
 
@@ -1156,6 +1158,8 @@ static int mcp251x_can_probe(struct spi_device *spi)
 
 	devm_can_led_init(net);
 
+	netdev_info(net, "successfully initialized.\n");
+
 	return 0;
 
 error_probe:
-- 
2.7.0

Re: [PATCH] can: mcp251x: add some messages to mcp251x_can_probe()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1536 bytes --]

On 05/29/2016 03:59 AM, Ed Spiridonov wrote:
> Print error message if mcp251x_hw_probe() fails (e.g. MCP2515 isn't connected or other wiring issue).
> Print information message if mcp251x_can_probe() is successful.

Can I add your S-o-b to the patch?

> ---
>  drivers/net/can/spi/mcp251x.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
> index cf36d26..49453f7 100644
> --- a/drivers/net/can/spi/mcp251x.c
> +++ b/drivers/net/can/spi/mcp251x.c
> @@ -1145,8 +1145,10 @@ static int mcp251x_can_probe(struct spi_device *spi)
>  
>  	/* Here is OK to not lock the MCP, no one knows about it yet */
>  	ret = mcp251x_hw_probe(spi);
> -	if (ret)
> +	if (ret) {
> +		dev_err(&spi->dev, "hw_probe failed, err=%d (wrong wiring?)\n", -ret);
>  		goto error_probe;
> +	}
>  
>  	mcp251x_hw_sleep(spi);
>  
> @@ -1156,6 +1158,8 @@ static int mcp251x_can_probe(struct spi_device *spi)
>  
>  	devm_can_led_init(net);
>  
> +	netdev_info(net, "successfully initialized.\n");

I'll remove this hunk, as we try to make the CAN devices as silent as
possible in case of no errors.

> +
>  	return 0;
>  
>  error_probe:
> 

regards,
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: 455 bytes --]

Re: [PATCH] can: mcp251x: add some messages to mcp251x_can_probe()

[Ed Spiridonov]

17.06.2016 12:22 пользователь "Marc Kleine-Budde" <mkl@pengutronix.de> написал:
>
> > @@ -1156,6 +1158,8 @@ static int mcp251x_can_probe(struct spi_device *spi)
> >
> >       devm_can_led_init(net);
> >
> > +     netdev_info(net, "successfully initialized.\n");
>
> I'll remove this hunk, as we try to make the CAN devices as silent as
> possible in case of no errors.


Why it is so important to be totally silent? IMHO one line per CAN bus
isn't too much.
But it could help to debug new configuration, e.g. to find wrong
device tree configuration, especially with several CAN buses.

I made this patch because a lot of complains on Raspberry PI forum.
Current module does not produce any message if module initializes
MCP2510/2515 chip successfully or not. It could be confusing.

Re: [PATCH] can: mcp251x: add some messages to mcp251x_can_probe()

[Oliver Hartkopp]

Hello Marc,

On 06/17/2016 10:41 PM, Ed Spiridonov wrote:
> 17.06.2016 12:22 пользователь "Marc Kleine-Budde" <mkl@pengutronix.de> написал:
>>
>>> @@ -1156,6 +1158,8 @@ static int mcp251x_can_probe(struct spi_device *spi)
>>>
>>>        devm_can_led_init(net);
>>>
>>> +     netdev_info(net, "successfully initialized.\n");
>>
>> I'll remove this hunk, as we try to make the CAN devices as silent as
>> possible in case of no errors.

I agree with Ed in this special case.

In opposite to CAN interfaces that reside on local busses or inside of 
SoCs the mcp251x is attached indirectly via SPI. This adds additional 
options to fail at initialization time.

As USB interfaces also tell about the USB plug-in and about their 
successful netdev initialization - the SPI based interfaces should be 
able to do it similarly.

Even the mcp251x stuff is not a very high performance CAN interface 
implementation many RasPi users do their first steps with SocketCAN 
using this tricky hardware setup.

That's why I vote for more information at initialization time too.

Regards,
Oliver

>
>
> Why it is so important to be totally silent? IMHO one line per CAN bus
> isn't too much.
> But it could help to debug new configuration, e.g. to find wrong
> device tree configuration, especially with several CAN buses.
>
> I made this patch because a lot of complains on Raspberry PI forum.
> Current module does not produce any message if module initializes
> MCP2510/2515 chip successfully or not. It could be confusing.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] can: mcp251x: add some messages to mcp251x_can_probe()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1358 bytes --]

On 06/17/2016 10:35 PM, Ed Spiridonov wrote:
> 17.06.2016 12:22 пользователь "Marc Kleine-Budde" <mkl@pengutronix.de
> <mailto:mkl@pengutronix.de>> написал:
> 
>     > @@ -1156,6 +1158,8 @@ static int mcp251x_can_probe(struct
>     spi_device *spi)
>     >
>     >       devm_can_led_init(net);
>     >
>     > +     netdev_info(net, "successfully initialized.\n");
> 
>     I'll remove this hunk, as we try to make the CAN devices as silent as
>     possible in case of no errors.
> 
> 
> Why it is so important to be totally silent? IMHO one line per CAN bus
> isn't too much.
> But it could help to debug new configuration, e.g. to find wrong device
> tree configuration, especially with several CAN buses.
> 
> I made this patch because a lot of complains on Raspberry PI forum.
> Current module does not produce any message if module initializes
> MCP2510/2515 chip successfully or not. It could be confusing.

ok, can I have your S-o-b [1]?

Marc

[1]
http://lxr.free-electrons.com/source/Documentation/SubmittingPatches#L409

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

Re: [PATCH] can: mcp251x: add some messages to mcp251x_can_probe()

[Ed Spiridonov]

2016-06-20 11:47 GMT+03:00 Marc Kleine-Budde <mkl@pengutronix.de>:
> ok, can I have your S-o-b [1]?

I submitted new version of this patch.
I'm unsure if my english is good enough, you are welcome to fix it if
you see fit..