On Wed, Nov 11, 2009 at 10:30 AM, Barry Song <[email protected]> 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: > > 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?
Mike, do you agree to move to using structure for registers too? > > >> >> Wolfgang. >> >> > _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
