Barry Song wrote:
> 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.
Well, functions are useful, of course. But various of your helper
functions just contain a view lines or are not even doing what the
function name makes think, e.g.:
+int register_bfin_candev(struct net_device *dev)
+{
+ dev->flags |= IFF_ECHO; /* we support local echo */
+ dev->netdev_ops = &bfin_can_netdev_ops;
+
+ bfin_can_set_reset_mode(dev);
+
+ return register_candev(dev);
+}
E.g., the bfin_can_set_reset_mode(dev) does not belong there. I really
do not see a strong reason how these 4 lines could justify an extra
function.
>>> 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.
Well, I do not agree with you here as well, but it might be just my
personal preference. I would not reject the patch because of this 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.
>>>> 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.
"reg->mbtif1" is nothing else than "base_address + offset", but the
compiler takes care of the offsets, etc. Furthermore, you benefit from
type checking and you can make less mistakes. For example, your
"CAN_WRITE_AMH(priv, channel, amh)" would translate into
"bfin_write32(®->chan[channel].amh, amh)". Using structures might be
cumbersome for some use-cases, like a SPI CAN driver, but for your
hardware it fits perfectly, I believe. As example, have a look to the
MSCAN driver.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
