Date:	Thu, 12 Aug 2010 08:25:52 +0200
From:	Oliver Hartkopp <socketcan@...tkopp.net>
To:	"Wang, Qi" <qi.wang@...el.com>
CC:	Greg KH <gregkh@...e.de>, Daniel Baluta <daniel.baluta@...il.com>,
	Masayuki Ohtak <masa-korg@....okisemi.com>,
	"meego-dev@...go.com" <meego-dev@...go.com>,
	Wolfgang Grandegger <wg@...ndegger.com>,
	"socketcan-core@...ts.berlios.de" <socketcan-core@...ts.berlios.de>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Khor, Andrew Chih Howe" <andrew.chih.howe.khor@...el.com>,
	"arjan@...ux.intel.com" <arjan@...ux.intel.com>,
	"Wang, Yong Y" <yong.y.wang@...el.com>
Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_CAN driver to 2.6.35

On 12.08.2010 04:04, Greg KH wrote:
> On Thu, Aug 12, 2010 at 09:42:27AM +0800, Wang, Qi wrote:

>>> 2. Why don't you use kernel existing kfifo infrastructure? ([2]).
>> Just take a look at kfifo.h. This structure has been changed. I
>> remembered there was a spin_lock from kfifo previously. Currently it's
>> been removed, good.
>> OKI-sans, would you please take a look at ./include/linux/kfifo.h, and try to use this structure and APIs?
>>
>> Daniel,
>>
>> We're anxious to integrate those codes now. Perhaps it'll take us
>> quite a long time to use kfifo. How about implementing it with the
>> next version?
> 
> What do you mean by this?  Code isn't merged into the tree unless it is
> correct.  Please fix this now, it's not that big of a deal.
> 

Hello Qi,

i generally wonder what this FIFO is used for??

For me it looks like a artifact from a former chardev implementation, as
several other code sniplets do (see comments from Wolfgang and Marc).

The network driver model is used for CAN drivers and therefore all the
infrastructure for queueing inbound and outbound network traffic should be
used from the Kernel like all other CAN drivers and all other ethernet drivers do.

Additionally there is a powerful infrastructure to support the special
functions of CAN netdevices (like setting of bittimings, listen-only modes, or
to produce CAN driver states etc.), that's part of the CAN drivers in
drivers/net/can/ since it has gone mainline.

CAN netdevices are intentionally dumb (like the original ethernet adapters).
This allows a simple driver interface between the kernel and the hardware
driver, e.g. for queueing CAN frames. CAN drivers don't use any hardware
rx-filtering(!) due to multiuser requirements and it's a vital requirement
that the frames are not re-ordered on sending (by the 'some magic' CAN
controller dealing with CAN-ID priorities, e.g. see tx-path in the MSCAN driver).

>From my perspective about 70% of your code is obsolete, as you implemented
tricky details (like FIFOs, bittiming tables or magic filter handlings),
that's not needed at all.

Please take a look at the SJA1000 driver (for a low complex CAN controller) or
at the TI driver (for a higher complex CAN controller) how these drivers
handle the specialties of each hardware. And read some documentation in
Documentation/networking/can.txt to get an impression about the concepts of
the CAN implementation in the Linux Kernel and the CAN netdevices.

I strongly assume that your driver would be about 30 kBytes of code fitting
into a single c-file like the ti_hecc.c does. And that's definitely easier for
us to review and easier for you to maintain in the future ...

Thanks & best regards,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists