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:
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.
>
>
> 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.
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?
>
> Wolfgang.
>
>
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
