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