Barry,
Barry Song schrieb:
> On Tue, Nov 10, 2009 at 2:18 PM, Sebastian Haas <[email protected]> wrote:
>> Hi Barry,
>>
>> I'm sure Wolfgang will do a review, but let me also put some notes to your
>> patch.
>>
>> 2 things right at the beginning. Please remove the debug outputs (e.g.
>> entering specific functions). I would also recommend to put the contents of
>> bfin-can.h into the bfin-can.c. You should rename bfin-can.c to bfin_can.c
>> to match your drivers' name and the naming scheme of the other SocketCAN
>> drivers.
>>
>> I found a lot of comments like:
>> /*
>> * ...
>> */
>> Please convert to /* ... */ where possible, these should save some lines.
>>
>> Please have a look at the other drivers how they handle register access. You
>> hide a lot of access logic within macros.
>>
>> Sebastian
>>
>> Barry Song schrieb:
>>> Signed-off-by: Barry Song <[email protected]>
>>> Signed-off-by: Heinz-JÃŒrgen Oertel <[email protected]>
>>> ---
>>> drivers/net/can/Kconfig | 10 +
>>> drivers/net/can/Makefile | 1 +
>>> drivers/net/can/bfin-can.c | 674
>>> ++++++++++++++++++++++++++++++++++++++++++++
<snip>
>>> + BFIN_CAN_WRITE_MSG(priv, TRANSMIT_CHL, cf->data, dlc);
>> That is the only place where you use this inline function, save some lines
>> by inlining it explicit and remove the inline function BFIN_...
> I can't agree that. Whether encapsulating some codes to a funtion
> doesn't depend on whether it is only called in one only place. People
> include some codes to a function just because it can compete a
> standalone work.
You are right, but the function is just a simple for-loop. I understand your
point when the function does something long or complicated, but in this
case I would prefer removing the function.
<snip>
>>> +
>>> + /* get id and data length code */
>>> + if (isrc & (1 << RECEIVE_EXT_CHL)) {
>>> + /* extended frame format (EFF) */
>>> + id = CAN_READ_XOID(priv, RECEIVE_EXT_CHL);
>> This is the only point where you use this macro, why dont you read directly
>> and remove the macro.
>
> same reason with BFIN_CAN_WRITE_MSG.
This may be a matter of personal taste. But removing it and inlining in
explicitly
will save some lines and improve readability as you don't have to look
what CAN_READ_XOID actually does.
>>> + id |= CAN_EFF_FLAG;
>>> + obj = RECEIVE_EXT_CHL;
>>> + } else {
>>> + /* standard frame format (SFF) */
>>> + id = CAN_READ_OID(priv, RECEIVE_STD_CHL);
>> Dito.
>>
>>> + obj = RECEIVE_STD_CHL;
>>> + }
>> Empty line.
>>
>>> + if (CAN_READ_ID1(priv, obj) & RTR)
>>> + id |= CAN_RTR_FLAG;
>>> + dlc = CAN_READ_DLC(priv, obj);
>>> +
>>> + cf = (struct can_frame *)skb_put(skb, sizeof(*cf));
>>> + memset(cf, 0, sizeof(struct can_frame));
>> Not needed as well.
> Ok.
>>> + cf->can_id = id;
>>> + cf->can_dlc = dlc;
>>> +
>>> + BFIN_CAN_READ_MSG(priv, obj, cf->data, dlc);
>> That is the only place where you use this inline function, save some lines
>> by inlining it explicit and remove the inline function BFIN_...
> same reason with BFIN_CAN_WRITE_MSG.
Dito.
<snip>
>>> + if ((irq == priv->tx_irq) && CAN_READ_CTRL(priv, OFFSET_MBTIF2)) {
>> Is the additional paranthesis really necessary?
> It's better to be added to detect possible errors in interrupt.
Sorry I mean, why do you put "irq == priv->tx_irq" in seperate brackets,
it's not needed?
<snip>
>>> +struct net_device *alloc_bfin_candev(void)
>>> +{
>>> + struct net_device *dev;
>>> + struct bfin_can_priv *priv;
>>> +
>>> + dev = alloc_candev(sizeof(*priv));
>> Why not sizeof(struct bfin_can_priv)? Reads better, right?
Is this okay for you?
<snip>
>>> +void free_bfin_candev(struct net_device *dev)
>>> +{
>>> + free_candev(dev);
>>> +}
>> If this is the only thing, this function will ever do remove it and call
>> free_candev directly in your code.
And that?
<snip>
>>> +/*
>>> + * bfin can private data
>>> + */
>>> +struct bfin_can_priv {
>>> + struct can_priv can; /* must be the first member */
>>> + struct sk_buff *echo_skb;
>>> + struct net_device *dev;
>>> + u32 membase;
>> I would convert it to "u16 *" and remove the whole CAN_READ/WRITE_REG stuff.
How about that? I think it would improve readability.
Sebastian
--
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HRA Neuburg a.d. Donau, HR-Nr. 70.106
Phone: +49-8441-490260
Fax : +49-8441-81860
http://www.ems-wuensche.com
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
