Barry Song wrote: > 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.
If it comes out that structures are cumbersome to use, go ahead with your old approach. > After you finish full review, I can change this together with others as V2. I first have to review the PEAK PCI patch posted recently. Then your patch will follow a.s.a.p. Thanks, Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
