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)); } > >> 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. Regards, Bhupesh _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
