Hi Wolfgang, > -----Original Message----- > From: Wolfgang Grandegger [mailto:[email protected]] > Sent: Wednesday, September 01, 2010 12:56 PM > To: Bhupesh SHARMA > Cc: [email protected] > Subject: Re: [RFC PATCH 1/4] SPEAr320 CCAN driver > > 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.
In a way you are correct: mainly the spear320_can_read() and spear320_can_write() change from spear1310_can_read() and spear1310_can_write() implementations. > 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_plat > form.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 > ... I must admit that now I am more confused :) Let me write down what my understanding is and please correct me if you think otherwise: 1. sja1000_platform.c is pretty similar in implementation/purpose to spear320_can.c/spear1310_can.c 2. sja1000.c is pretty similar in implementation/purpose to bosch_ccan.c In case we design one CCAN platform driver then where should the difference in spearxxx_can_read() and spearxxx_can_write be managed? > 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? > Regards, Bhupesh _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
