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

[Marc Kleine-Budde <mkl@pengutronix.de>]

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