Sebastian Haas wrote:
> 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?
I also do not see a need for an extra function here, especially not in a
header file. From my point of view, it does not improve readability.
Futhermore, the name is missleading and function names should be in
lower case.
>>> <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.
For me, the brackets are OK.
>>> <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)".
sizeof(*pointer) is less error prune, I believe.
> If the type of *priv changes, this sizeof(...) will automatically return
> the size of the new type.hjr5zhjhzdfyfg
>>>>>> +/*
>>>>>> + * 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;
Please use a proper type for I/O addresses:
void __iomem *membase;
And nowadays we should use ioread/write16, IIRC.
>>>>> 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?
Please consider using structures to describe the register layout. As I
see it, it really makes sense for that driver and you would get rid of
the deprecated macro definitions.
iowrite(®->mbtif1, 0xFFFF);
A full review will follow soon.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
