Hi Wolfgang, > -----Original Message----- > From: Wolfgang Grandegger [mailto:[email protected]] > Sent: Wednesday, September 01, 2010 5:18 PM > To: Bhupesh SHARMA > Cc: [email protected] > Subject: Re: [RFC PATCH 1/4] SPEAr320 CCAN driver > > On 09/01/2010 11:46 AM, Bhupesh SHARMA wrote: > > Hi Wolfgang, > > > > Please ignore the earlier mail.. > > > >> -----Original Message----- > >> From: Wolfgang Grandegger [mailto:[email protected]] > >> Sent: Wednesday, September 01, 2010 2:44 PM > >> To: Bhupesh SHARMA > >> Cc: [email protected] > >> Subject: Re: [RFC PATCH 1/4] SPEAr320 CCAN driver > >> > >> Hi Bhupesh, > >> > >> On 09/01/2010 11:01 AM, Bhupesh SHARMA wrote: > >>> 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: > >> > >> ... > >> > >>>>> 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. > >> > >> Can you show us the new spear1310_can_read/write functions? I want > to > >> understand how generic they really are. > > > > spear1310 differs from spear320 in the way that 16-bit registers are > aligned at a 16-bit boundary > > whereas in spear320 these 16-bit registers are aligned at a 32-bit > boundary. So, > > while spear1310 routines should look like this: > > > > static u16 spear1310_can_read_reg(const struct bosch_ccan_priv *priv, > > enum ccan_regs reg) > > { > > u16 val; > > > > /* 16 bit registers are aligned at 16-bit boundary */ > > val = readw(priv->reg_base + reg); > > return val; > > } > > > > static void spear1310_can_write_reg(const struct bosch_ccan_priv > *priv, > > enum ccan_regs reg, u16 val) > > { > > /* 16 bit registers are aligned at 16-bit boundary */ > > writew(val, priv->reg_base + reg); > > } > > > > The one for SPEAr320 will look like: > > static u16 spear320_can_read_reg(const struct bosch_ccan_priv *priv, > > enum ccan_regs reg) > > { > > u16 val; > > > > /* shifting 1 place because 16 bit registers are word aligned */ > > val = readw(priv->reg_base + (reg << 1)); > > return val; > > } > > > > static void spear320_can_write_reg(const struct bosch_ccan_priv > *priv, > > enum ccan_regs reg, u16 val) > > { > > /* shifting 1 place because 16 bit registers are word aligned */ > > writew(val, priv->reg_base + (reg << 1)); > > } > > These functions are still pretty generic and could be handled by a > generic platform C_CAN driver via platform data field "reg_shift". > > >>>> 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 :) > >> > >> Sorry, I though my note above would help. > > > > I appreciate your efforts in reviewing this patch. > > > >>> 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 > >> > >> Yes, but it is a *generic* interface usable for various platforms > which > >> select the access method in their platform code. > >> > >>> 2. sja1000.c is pretty similar in implementation/purpose to > >> bosch_ccan.c > >> > >> Yes. > >> > >>> In case we design one CCAN platform driver then where should the > >> difference in spearxxx_can_read() > >>> and spearxxx_can_write be managed? > >> > >> I'm just hoping that the I/O functions are generic enough to support > >> other platforms as well. If not, a separate interface makes sense. > >> > > > > Please let me know on basis of the above register definitions. > > Let's go for the generic C_CAN platform driver. >
And where should the platform dependent differences be captured, at some place like 'include/linux/can/platform/spear320.h' ? Regards, Bhupesh _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
