Re: [PATCH net-next-2.6 v2 1/1] can: c_can: Added support for Bosch C_CAN controller

[Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>]

On 12/22/2010 09:03 AM, Bhupesh SHARMA wrote:
> Hi Marc and Wolfgang,
> 
>> -----Original Message-----
>> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
>> Sent: Wednesday, December 22, 2010 1:21 PM
>> To: Marc Kleine-Budde
>> Cc: Bhupesh SHARMA; Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [PATCH net-next-2.6 v2 1/1] can: c_can: Added support for
>> Bosch C_CAN controller
>>
>> On 12/22/2010 07:52 AM, Marc Kleine-Budde wrote:
>>> On 12/22/2010 04:36 AM, Bhupesh SHARMA wrote:
>>>> Hi Wolfgang,
>>>>
>>>>> Hi Bhupesh,
>>>>>
>>>>> On 12/21/2010 05:48 AM, Bhupesh SHARMA wrote:
>>>>>> Hi Wolfgang,
>>>>> ...
>>>>>>> In the meantime I compared the CAN chapter of the PCH manual with
>>>>> the
>>>>>>> C_CAN manual. The paragraphs I checked are *identical*. This
>> makes
>>>>>>> clear, that the "pch_can" is a clone of the  C_CAN CAN
>> controller,
>>>>> with
>>>>>>> a few extensions, though. Therefore it would make sense, to
>>>>> implement a
>>>>>>> bus sensitive interface like for the SJA1000 allowing to handle
>> both
>>>>>>> CAN
>>>>>>> controllers with one driver sooner than later. Therefore, could
>> you
>>>>>>> please implement:
>>>>>>>
>>>>>>>   drivers/net/can/c_can/c_can.c
>>>>>>>                        /c_can_platform.c
>>>>>>>
>>>>>>> Then an interface to the PCI based PCH CAN controller could be
>> added
>>>>>>> easily, e.g. as "pch_pci.c". You already had something similar in
>>>>> your
>>>>>>> RFC version of the patch, IIRC.
>>>>>>
>>>>>> This was the approach I initially proposed in my RFC V1 patch :)
>>>>>> But unfortunately we could not agree to it.
>>>>>
>>>>> I know. But at that time I was not aware of any other bus used for
>> the
>>>>> C_CAN controller.
>>>>>
>>>>>> So, please let me reiterate what I understood and what was present
>>>>>> in RFC version of the patch. Please add your comments/views:
>>>>>>
>>>>>>         - drivers/net/can/c_can/c_can.c (similar on lines of
>>>>> sja1000.c)
>>>>>>         i.e. a)no *probe* / *remove* functions here,
>>>>>>              b)register read/write implemented here.
>>>>>>
>>>>>>         - drivers/net/can/c_can/c_can_platform.c (similar on lines
>> of
>>>>> sja1000_platform.c)
>>>>>>         i.e. *probe* / *remove* implemented here,
>>>>>
>>>>> Yes, that's what I'm thinking about.
>>>>>
>>>>>> Marc and Tomoya can also add their suggestions so that I can
>> finalize
>>>>> V3 a.s.a.p.
>>>>>
>>>>> That would be nice, indeed. Also have a look to Tomoya's PCH
>> driver,
>>>>> which also looks very good in the meantime.
>>>>
>>>> I am having a look at Tomoya's PCH driver, but as I mentioned in
>>>> RFC V1 patch, I would rather like to have a bus sensitive `c_can`
>> driver
>>>
>>> What do you mean by a "bus sensitive" driver?
>>
>> I was thinking about a "bus independent interface" like for the
>> SJA1000.
>> A bus sensitive driver would the be in c_can_platform.c.
>>
>>>> on top of which we can have the platform driver `c_can_platform`
>> which
>>>> essentially caters to the details of registers mapping/arch
>> differences.
>>>> Any other functionality like USB/PCI should be present in a separate
>> file
>>>> like `usb_c_can.c` or `pci_c_can.c`
>>>
>>> Sounds like the sja1000 approach, which is a good choice.
>>
>> I fully agree.
>>
>>>> If you agree I will try to circulate V3 a.s.ap.
>>>
>>> go ahead.
>>>
>>
>> Yes, please.
> 
> Ok, so I would try to circulate V3 by tomorrow with a sja1000 *like* approach that ensures 
> in-order packet reception as well.

That would be nice. But well, no hurry. It's Xmas time :-),

Wolfgang.