On 08/31/2010 12:32 PM, Bhupesh SHARMA wrote: > Hi Wolfgang, > > Thanks for your inputs. Please see my comments below: > >>> Signed-off-by: Bhupesh Sharma <[email protected]> >> >> Please truncate to the usual < 80 lines for readability. > > I am myself not a big supporter of < 80 lines being mandatory, especially if > it is in a place like this. > Please see Linus' comments on the same here: > http://www.linux-archive.org/device-mapper-development/295901-drop-80-character-limit-checkpatch-pl.html
I'm not sure if checkpatch will complain as I'm referring to the commit message (not the code), where that long lines a really unusual. > >>> >>> Index: bosch_ccan.c >>> =================================================================== >>> --- bosch_ccan.c (revision 0) >>> +++ bosch_ccan.c (revision 0) >>> @@ -0,0 +1,858 @@ >>> +/* >>> + * drivers/net/can/bosch_ccan.c >>> + * >>> + * CAN bus driver for Bosch CCAN controller >>> + * >>> + * Copyright (C) 2010 ST Microelectronics >>> + * Bhupesh Sharma <[email protected]> >>> + * >>> + * Borrowed heavily from the CCAN driver originally written by: >>> + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix >> <[email protected]> >>> + * - Simon Kallweit, intefo AG <[email protected]> >>> + * which can be viewed here: >>> + * http://svn.berlios.de/svnroot/repos/socketcan/trunk/kernel/2.6/ >>> + * drivers/net/can/old/ccan/ccan.c >>> + * >>> + * Bosch CCAN controller is compliant to CAN protocol version 2.0 >> part A and B. >>> + * Bosch CCAN user manual can be obtained from: >>> + * http://www.semiconductors.bosch.de/pdf/Users_Manual_C_CAN.pdf >>> + * >>> + * TODO: >>> + * - Add support for IF2 in Basic mode. >>> + * >>> + * This file is licensed under the terms of the GNU General Public >>> + * License version 2. This program is licensed "as is" without any >>> + * warranty of any kind, whether express or implied. >>> + */ >> >> See may comments on patch 1/4. > > Ack to adding 'Original Copyright' message in V2 and removing the > 'drivers/net/can/..' representation. > However, I have seen URL links in a number of kernel drivers at this place > (for e.g., see 'ti_hecc.c' CAN driver). I was just complaining about the "http://svn.berlios.de/..."; link. Not URLs in general. ... >>> + priv->tx_object++; >> >> Is this variable really useful? > > In my view it is helpful for debugging. But it's never checked, just incremented and decremented. Or have I missed something? ... >> Of these only CCAN_NORMAL_MODE is used. Please use priv->crtlmode to >> handle more of them and remove the unused cases. > > Please elaborate this. I have use CCAN_LOOPBACK_MODE as well during debugging > the driver. > These modes are especially successful in debugging or testing the PCB's for > non-functioning CAN interfaces. I mainly want to note, that code which is *never* used should be removed. ... >> How is bus-off recovery supposed to work? > > Bus-Off recovery works by providing the command: > # ip link set canX type can restart > > This causes 'bosch_ccan_start' to be called which enables the interrupts and > allows a change of Bus Off bit in Status Register which is properly handled > by ISR. Already checked the bus-off recovery on SPEAr320 platform. OK, sounds good. ... >>> Index: bosch_ccan.h >>> =================================================================== >>> --- bosch_ccan.h (revision 0) >>> +++ bosch_ccan.h (revision 0) ... >>> +/* >>> + * CCAN operating modes: >>> + * Support is available for default as well as test operating modes. >>> + * Normal mode will generally be used as a default mode in most >> cases, >>> + * however, various test modes may be useful in specific use-cases. >>> + */ >>> +enum bosch_ccan_operating_mode { >>> + CCAN_NORMAL_MODE = 0, >>> + CCAN_BASIC_MODE, >>> + CCAN_LOOPBACK_MODE, >>> + CCAN_LOOPBACK_WITH_SILENT_MODE, >>> + CCAN_SILENT_MODE >>> +}; >> >> I do not see a need for another enumeration o the type. We already have >> the CAN_CTRLMODE_*. > > Yes. But I don't see CAN_CTRLMODE_* capturing all the operating mode types > supported by Bosch CCAN. But in your driver just CCAN_NORMAL_MODE is used, so far. Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
