netdev - Re: [Socketcan-users] [PATCH net-next v2 1/4] can: cc770: add driver core for the Bosch CC770 and Intel AN82527

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4ED4A3CD.4060908@grandegger.com>
Date:	Tue, 29 Nov 2011 10:20:13 +0100
From:	Wolfgang Grandegger <wg@...ndegger.com>
To:	Marc Kleine-Budde <mkl@...gutronix.de>
CC:	netdev@...r.kernel.org, socketcan-users@...ts.berlios.de,
	linux-can@...r.kernel.org
Subject: Re: [Socketcan-users] [PATCH net-next v2 1/4] can: cc770: add driver
 core for the Bosch CC770 and Intel AN82527

Hi Marc,

On 11/28/2011 12:28 PM, Marc Kleine-Budde wrote:
> On 11/25/2011 10:43 AM, Wolfgang Grandegger wrote:
>> Signed-off-by: Wolfgang Grandegger <wg@...ndegger.com>
>> ---
>>  drivers/net/can/Kconfig            |    2 +
>>  drivers/net/can/Makefile           |    1 +
>>  drivers/net/can/cc770/Kconfig      |    3 +
>>  drivers/net/can/cc770/Makefile     |    7 +
>>  drivers/net/can/cc770/cc770.c      |  895 ++++++++++++++++++++++++++++++++++++
>>  drivers/net/can/cc770/cc770.h      |  234 ++++++++++
>>  include/linux/can/platform/cc770.h |   33 ++
>>  7 files changed, 1175 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/net/can/cc770/Kconfig
>>  create mode 100644 drivers/net/can/cc770/Makefile
>>  create mode 100644 drivers/net/can/cc770/cc770.c
>>  create mode 100644 drivers/net/can/cc770/cc770.h
>>  create mode 100644 include/linux/can/platform/cc770.h
> 
> I don't know the hardware, but the code looks good to me, some comments:
> - The driver doesn't use NAPI, can this be added
> - The rx-handlers have a while(1) loop
>   For NAPI you have to add accounting, for the non NAPI case it would
>   be good, too.
> - I think you can move a large number of lines from the .h file into
>   the driver. Code that's not used in the different binding drivers.
> 
> More comments inline (mostly nitpicking)

I just sent out v4. For the sake of readability, I finally did not move
most of the head definitions in cc770.h into cc770.c and used the
variable name "o" consistently for the message object index (instead of
using "i").

Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html