On Wed, Nov 11, 2009 at 4:42 PM, Wolfgang Grandegger <[email protected]> wrote: > 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.
Personally, I think "structure" and "base_address + offset" are both ok to me as two different coding styles. If Mike has no other concerns, I think I can follow it. After you finish full review, I can change this together with others as V2. > > Wolfgang. > _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
