Hi Wolfgang, > -----Original Message----- > From: Wolfgang Grandegger [mailto:[email protected]] > Sent: Tuesday, August 31, 2010 4:35 PM > To: Bhupesh SHARMA > Cc: [email protected] > Subject: Re: [RFC PATCH 2/4 RESEND] Bosch CCAN driver > > 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.
Ok. The 80-line limit in the commit message will be easier to maintain using GIT. V2 will ensure this. > > > >>> > >>> 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. Ok, V2 will remove this :) > ... > > >>> + 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? > > ... No. Actually I forgot that I checked this variable only using my ICD for useful Tx :) So, V2 will be removing this. > >> 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. > ... Will be done in V2. > >> 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. V2 will try to incorporate all possible modes using CAN_CTRLMODE_* Regards, Bhupesh _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
