Barry Song schrieb:
> 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:
>>>> Barry Song schrieb:
>>>>> + 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.
One or two lines can be a function of course, but if it used only at one
place and does a pretty trivial thing it's just waste of code lines.
But against your argumentation BFIN_CAN_WRITE_MSG does not finish a
standalone function. Reading or writing the _whole_ CAN message not just
the data part would be a complete function. Either you move
reading/writing identifier and dlc into BFIN_CAN_..._MSG too or simply
remove it and move the simple loop at the right place.
If I follow your argumentation I could put each single statement into a
function and everyone is happen with it. IMHO having the whole process
of reading/writing a CAN message in one code block like this:
id = read_reg ...
dlc = read_reg
for i < dlc
dat[i] = read_reg
is far more compact and still readable as:
id = read_reg
dlc = read_reg
read_data(dat, dlc) - Dont know what read_data actually does
Wolfgang or Oliver, what do you think?
>> <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.
As I wrote this special construct is a matter of personal.
>>>>> + 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.
Why? Right coding style is to avoid unnecessary brackets. Can't believe the
compiler complains about it.
>> <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)".
If the type of *priv changes, this sizeof(...) will automatically return
the size of the new type.
>>>>> +/*
>>>>> + * 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.
I don't think so. List, other opinions?
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
