Re: [PATCH v5 2/2] CAN: CAN driver to support multiple CAN bus on SPI interface

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

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

On 07/28/2014 06:38 PM, Stefano Babic wrote:
> This driver implements a simple protocol
> to transfer CAN messages to and from an external
> microcontroller via SPI interface.
> Multiple busses can be tranfered on the same SPI channel.
> 
> It was tested on a i.MX35 connected to a Freescale's
> HCS12 16-bit controller with 5 CAN busses.
> 
> Signed-off-by: Stefano Babic <sbabic@denx.de>

No line by line review, but some fundamental comments and questions:

Can you please use a consistent indention scheme for struct and enums, I
favour a single space against aligning with tab, as it ony fits 95% of
the time and breaks during upgrades.

Please only use u16, u32 and friends where the size of the register is
important, use plain int, unsinged int otherwise.

I'm not sure of fiddling with the GPIO which you requested as an
interrupt is a good idea. This is probably not available on all platforms.

Why do you have two functions calculating the checksum? Have you checked
if the kernel offers you a library for this?

Can you explain me the endianess, I don't get why you need
format_frame_output() and format_frame_input() and while your structs
that go over the wire have no specific endianess (i.e. no __be32 in the
struct msg_*).

Have you thought about using threaded interrupts? Or even better make
use of the asynchronous SPI framework?

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