On Tue, Nov 10, 2009 at 8:14 PM, Wolfgang Grandegger <[email protected]>
wrote:
> 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.
Functions can be divided into less functions. That's right thinking
for software design. You can say a software finish a standalone
function, but there are many little standalone functions in it.
>>
>> 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.
Maybe the name is not too good. But I don't think using functions will
decrease readability. Those functions transfer blackfin data layout to
generic can message data layout. It is specific with blackfin
hardware. The function will hide the details for reading/writing
packet body. It's easier for people to understand the up level flows
without reading into the lines in bottom level functions.
>
>>>> <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.
Ok.
>
>> 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.
Using "base_address + offset" to access registers are more generic
than using structure, and that makes it easier and more clear to
support multi CAN instances.
>
> iowrite(®->mbtif1, 0xFFFF);
>
> A full review will follow soon.
>
> Wolfgang.
>
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
