[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4A6F0238.6050605@hartkopp.net>
Date: Tue, 28 Jul 2009 15:50:48 +0200
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Sascha Hauer <s.hauer@...gutronix.de>
CC: Socketcan-core@...ts.berlios.de,
Linux Netdev List <netdev@...r.kernel.org>
Subject: Re: [PATCH V2] CAN: Add Flexcan CAN controller driver
Sascha Hauer wrote:
> Hi Oliver,
>
> On Tue, Jul 28, 2009 at 03:21:40PM +0200, Oliver Hartkopp wrote:
>> Sascha Hauer wrote:
>>> Hi,
>>>
>>> Here is the second version of the flexcan driver.
>> Hi Sascha,
>>
>> some more points i forgot to mention, sorry ...
>>
>>
>>> +/* Structure of the message buffer */
>>> +struct flexcan_mb {
>>> + u32 can_dlc;
>>> + u32 can_id;
>>> + u32 data[2];
>>> +};
>> This looks really hackish and does not reflect the structure of a flexcan
>> message buffer! The data is 'u8' and the name of 'dlc' for the
>> description/flag register is bad.
>>
>
> see below..
Especially can_dlc, can_id and data[] are known from struct can_frame which
really can confuse here ...
>
>>> +
>>> +static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
>>> +{
>>> + struct can_frame *frame = (struct can_frame *)skb->data;
>>> + struct flexcan_priv *priv = netdev_priv(dev);
>>> + struct flexcan_regs __iomem *regs = priv->base;
>>> + u32 can_id;
>>> + u32 dlc = MB_CNT_CODE(0xc) | (frame->can_dlc << 16);
>> Naming this variable 'dlc' does not hit the point. See below.
>>
>>> + u32 *can_data;
>> Really this needs to be fixed up by defining a proper mailbox struct.
>>
>>
>>> +
>>> + netif_stop_queue(dev);
>>> +
>>> + if (frame->can_id & CAN_EFF_FLAG) {
>>> + can_id = frame->can_id & MB_ID_EXT;
>> Please use CAN_EFF_MASK here.
>
> I used MX_ID_EXT intentionally because it it flexcan specific and just
> happens to be the same as CAN_EFF_MASK. I can change it if you like.
Yes, i've seen that. I would tend to use CAN_EFF_MASK here as you apply it on
frame->can_id.
When you get it from the controller MB_ID_EXT_MASK would be the better one.
>
>>
>>> + dlc |= MB_CNT_IDE | MB_CNT_SRR;
>>> + } else {
>>> + can_id = (frame->can_id & CAN_SFF_MASK) << 18;
>>> + }
>> Just nitpicking for Kernel coding style:
>> remove the last '{' and '}' pair.
>
> No, Documentation/CondingStyle suggests that if one branch needs braces
> the other branch should use them, too.
Sorry. Didn't know that.
>
>>> +
>>> + if (frame->can_id & CAN_RTR_FLAG)
>>> + dlc |= MB_CNT_RTR;
>>> +
>>> + writel(dlc, ®s->cantxfg[TX_BUF_ID].can_dlc);
>>> + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id);
>>> +
>>> + can_data = (u32 *)frame->data;
>>> + writel(cpu_to_be32(*can_data), ®s->cantxfg[TX_BUF_ID].data[0]);
>>> + writel(cpu_to_be32(*(can_data + 1)), ®s->cantxfg[TX_BUF_ID].data[1]);
>> IMHO it is not really transparent, that this is a correct handling to copy the
>> can_frame.data[] on all architectures. I bet creating a for-statement
>> regarding the dlc is not slower and makes really clear, what's going on here.
>
> This is indeed a problem here. The original Coldfire code I used as a
> template used a loop around unsigned char * which did the wrong thing
> for me.
This could be a good starting point for an investigation ;-)
> So yes, this is not generic here, but I have no idea how the
> generic code looks like. As Coldfire is big endian this doesn't seem
> that wrong.
I would try to define a proper flexcan_mb struct like
struct flexcan_mb {
u8 code;
u8 ctrl;
u16 timestamp;
u32 id;
u8 data[8];
}
And then see how it looks like ;-)
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