Re: [PATCH] CAN controller support for Allwinner A10/A20 SoCs

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

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

On 09/02/2015 08:44 PM, Gerhard Bertelsmann wrote:
> This driver add support for SUNXI CAN controllers found on Allwinner A10/A20 SoCs
> V1

Thanks for the patch. I suggest to use git send-email to post the patch.
Please test your patch with scripts/checkpatch.pl (included in the
kernel tree) before submitting - you can even use the "--file" option to
check a file, before creating the patch.

Here some general remarks:

- please remove the EXPORT_SYMBOL_GPL, as this is a stand alone driver.
- why are there udelay()s in the hot path of the code?
- please move the device tree bindings into a separate file in
  Documentation/devicetree/bindings/net/can/
  please provide that binding doc as a separate patch
- please provide a proper Signed-off-by line, with a working
  non obfuscated email address

Your priv struct seems a bit bloated:

> struct sunxican_priv {
> 	struct can_priv can;
> 	struct net_device *dev;

write only variable

> 	struct napi_struct napi;

not used

> 
> 	int open_time;

read only variable

> 	struct sk_buff *echo_skb;

not used

> 	void __iomem *base;
> 	struct clk *clk;
> 
> 	void *priv;		/* for board-specific data */

I think this is never used

> 
> 	unsigned long irq_flags;	/* for request_irq() */

This is a read only variable

> 	spinlock_t cmdreg_lock;	/* lock for concurrent cmd register writes */
> 
> 	u16 flags;		/* custom mode flags */

not used

> };

Please fix these sparse warning:

> drivers/net/can/sunxi_can.c:596:13: warning: symbol 'sunxi_can_interrupt' was not declared. Should it be static?
> drivers/net/can/sunxi_can.c:690:6: warning: symbol 'free_sunxicandev' was not declared. Should it be static?

Compile with "make C=1" to test yourself. Install the "sparse" package
of your distribution first.

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