Hi Bhupesh,
On 09/01/2010 01:52 PM, Bhupesh SHARMA wrote:
> 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' ?
In "include/linux/can/platform/c_can.h" similar as for the sja1000.h:
struct c_can_platform_data {
u32 osc_freq; /* CAN bus oscillator frequency in Hz */
int reg_shift; /* offset to address shift */
... /* whatever is needed */
};
The platform data will then be filled in the platform code of the spear320.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
