On Tue, Nov 10, 2009 at 5:12 PM, Sebastian Haas <[email protected]> wrote:
> 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.
I can't agree that. Whether being a function doesn't depend on simple
or not, just depends on whether it finishes a standalone function. One
or two lines can be a function too. Placing too many lines which are
not related closely into a function make the function not readable in
fact.
>
> <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.
I can't agree that. People don't want to know the detail of
CAN_READ_XOID can just understand the context which is called without
reading into the lines of functions/macros. That's just the advantages
of encapsulation.
>
>>>> + 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?
No. It is needed. That is right coding style.
>
> <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?
I don't know why. But some kernel people insist on using
"sizeof(*pointer)" but not "sizeof(data structure)".
>
> <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?
Good! Thanks!
>
> <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.
I think removing CAN_READ/WRITE_REG will destroy readability. Same
reason with CAN_READ_XOID.
>
> 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
