Barry Song wrote:
> On Wed, Nov 11, 2009 at 4:22 AM, Wolfgang Grandegger <[email protected]>
> wrote:
>> 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);
>> +}
> Here I am just following sja1000:
But for SJA1000 the function is exported and used by various drivers.
> int register_sja1000dev(struct net_device *dev)
> {
> if (!sja1000_probe_chip(dev))
> return -ENODEV;
>
> dev->flags |= IFF_ECHO; /* we support local echo */
> dev->netdev_ops = &sja1000_netdev_ops;
>
> set_reset_mode(dev);
> chipset_init(dev);
>
> return register_candev(dev);
> }
>
> I think I can delete this function "register_bfin_candev" and call the
> related codes directly.
Yep, that would be my personal preference, at least.
>>
>> 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.
> I am sure I know structure can help to handle offset. But it seems it
> is not a common way in other driver sub-system. If CAN prefers to use
> structure to describe registers, I think I can follow the convention.
> But in fact, Blackfin has a lot of registers(0xFFC02A00~0xFFC02FFF)
> about CAN, only a little non-continuous part is used in this driver.
This could be overcome with padding fields, e.g. res[100].
> If I use structure, I will get a very fat one, but only several fields
> are accessed. So do you still think it is a good way in this driver?
To be pragmatic, if structures make the code more compact, transparent
and improve readability, they should be used. Please check if that's the
case for your driver.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
