Hi Bhupesh, On 09/01/2010 06:40 AM, Bhupesh SHARMA wrote: > Hi Wolfgang, > >> -----Original Message----- >> From: Wolfgang Grandegger [mailto:[email protected]] >> Sent: Tuesday, August 31, 2010 2:05 PM >> To: Bhupesh SHARMA >> Cc: [email protected] >> Subject: Re: [RFC PATCH 1/4] SPEAr320 CCAN driver >> >> Hello, >> >> Thanks for your contribution. This is quick review of your driver. >> First >> some general comments: >> >> - Do we really need a separate driver interface for the CCAN? Is >> a platform driver with appropriate platform declarations in >> "include/linx/can/platform/ccan.h" not sufficient? > > Yes, I will explain the case below. > Initial thoughts were to develop a CAN driver for SPEAr320 SoC. > Now, this SOC uses two Bosch CCAN controllers glued together with some glue > logic > to work with the APB bus and SPEAr. > > The ways to do it could have been two: > - Develop a SPEAr320 CAN driver which includes everything (on the lines > of ti_hecc.c CAN driver..) > But, this would have limited the Bosch CCAN implementation as per > SPEAr glue logic and hence anyone > using Bosch CCAN controller inside their CAN implementations would > have to write separate code for the same. > - Develop two separate drivers: SPEAr320 CAN driver and Bosch CCAN > driver which > demarcate the platform details of SPEAr and the glue logic specific > to a SPEAr SoC and > the Bosch CCAN driver functions will be called by SPEAr CAN driver as > desired. > > The second way seems more flexible as it allows any design with underlying > Bosch CCAN controllers to use > the Bosch CCAN driver. > > It is also particularly useful as we now have a new SPEAr1310 SoC which also > uses the Bosch CCAN controller > , but the glue logic has changed from SPEAr320 and hence we can handle most > of the platform/machine specific details > in SPEAr1310 driver and keep Bosch CCAN driver as it is.
What has changed with the new Soc and what platform specific details and initializations do you think about? I personally do not see a lot of platform specific code in your spear320_can.c. Just spear320_can_read() and spear320_can_write() and also these functions look pretty generic. Also note that platform specific initialization is usually not done in the CAN driver but the platform code. I was thinking about a generic C_CAN platform driver similar to the sja1000+platform driver: http://lxr.linux.no/#linux+v2.6.35/drivers/net/can/sja1000/sja1000_platform.c Don't be confused. For the SJA1000 it consists of sja1000 plus sja1000_platform. For the C_CAN it would be just one driver (as we do not need to support other interfaces). As you can see, also this driver supports various access methods. What we should avoid is useless code duplication, e.g.: c_can/spear320_can.c c_can/spear1320_can.c c_can/other_can.c ... If feasible, we should go for the generic driver. Other opinions? >> - If such a CCAN driver interface makes sense, the code should go into >> a subdirectory named "ccan". > > I would not prefer that, as then the SPEAr driver files that invoke the Bosch > CCAN > driver functionalities will also be have to be kept here which does not seem > logical. As for the SJA1000, all C_CAN related files should go into a sub-directory, e.g.: c_can/Kconfig c_can/Makefile c_can/c_can.c c_can_c_can.h c_can/spear320_can.c c_can/other_can.c ... Does that sound more logical? Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
