Ira W. Snyder wrote:
> On Wed, Feb 10, 2010 at 07:45:30PM +0100, Wolfgang Grandegger wrote:
>> Ira W. Snyder wrote:
[snip]
>>> Ok. All of the existing code I have that uses Janz's char driver enables
>>> termination. I think just enabling termination (as I did in my driver)
>> The copyright made me think that you are the author of this code.
>>
>
> I am the only author of the driver posted in the first email of the
> thread. I have written it using only the datasheets provided by Janz on
> their website.
OK, perfect.
> For many years at my organization, we have been using Linux char drivers
> provided by Janz. We would like to move to a driver which is present in
> mainline Linux, to make switching kernels easier.
OK, I know.
>>> will be fine. If I find a need for switching off termination later
>>> (unlikely) then I'll add a sysfs attribute.
>> That is a bad idea. You must use termination if the controller is at one
>> end of the bus, which might not always be the case. But that's not that
>> important in the first place.
>>
>
> I'll get the rest of the driver working before I worry about this.
This could also be disabled via module parameters.
[snip]
>>> I used NAPI to process CAN frames only.
>> OK.
>>
>>> I could remove NAPI support and roll the functionality into the
>>> workqueue, if you think that would be better.
>> NAPI support is a good thing in general but having it in the work queue
>> seems more straight forward and you could get rid of a lot of
>> synchronization, I assume. But that's just a wild guess for the moment.
>>
>
> It is easy enough to implement. I'll do that for the next posting of the
> driver.
But only if it does simplify the code significantly.
[snip]
>>> Do you have ideas of how to do this without any indication of "fullness"
>>> from the hardware?
>> You could at least restrict the number of messages queued. Does the
>> firmware provide a TX done notification?
>>
>
> Nope. If there is mention of it in the manual, I cannot find it. :(
>
> I *do* have a notification mechanism if I don't support EFF CAN frames,
> but I'd really like to have support for that.
>
> I could read the entire ring off the device after each xmit() and see if
> it is full. But then I have no way of knowing when to start the queue
> again, unless I set a kernel timer and keep reading the ring. That is a
> terribly slow (and ugly) approach, though.
Yep. It will not be possible to support all the functionality with an
intelligent card. There are some restrictions.
[snip]
>>> I thought the can_(get|put)_echo_skb() functionality was the Linux CAN
>>> layer software fallback. Am I wrong?
>> No.
Hm, is my answer not clear. You implementation is *OK*.
>
> Ok, I'll try and figure out what these do. The comments in the code
> /really/ make it seem like these are for local loopback. The
> descriptions even talk about local loopback! A better description might
> help authors such as myself.
>
>>From drivers/net/can/dev.c:
>
> /*
> * Get the skb from the stack and loop it back locally
> *
> * The function is typically called when the TX done interrupt
> * is handled in the device driver. The driver must protect
> * access to priv->echo_skb, if necessary.
> */
> void can_get_echo_skb(struct net_device *dev, unsigned int idx)
>
> /*
> * Put the skb on the stack to be looped backed locally lateron
> *
> * The function is typically called in the start_xmit function
> * of the device driver. The driver must protect access to
> * priv->echo_skb, if necessary.
> */
> void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
> unsigned int idx)
>
As I said. There is *no* need to do the loopback in software (by using
these functions) if the hardware already does it. But then you should
use also "0" for the echo skb size in the following call:
ndev = alloc_candev(sizeof(*mod), 0);
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
