Hi Bhupesh, On 01/11/2011 05:50 AM, Bhupesh SHARMA wrote: > Hi Wolfgang, > > Thanks for your review. > Please see my comments inline. > >> -----Original Message----- >> From: Wolfgang Grandegger [mailto:[email protected]] ...
>>> + * Bosch C_CAN controller is compliant to CAN protocol version 2.0 >> part A and B. >>> + * Bosch C_CAN user manual can be obtained from: >>> + * http://www.semiconductors.bosch.de/pdf/Users_Manual_C_CAN.pdf >> >> Unfortunately, this link is not valid any more. > > Oops.. > It seems they have shifted to: > http://www.semiconductors.bosch.de/media/en/pdf/ipmodules_1/c_can/users_manual_c_can.pdf Ah, nice. I was not aware of that new link. ... >>> +int c_can_write_msg_object(struct net_device *dev, >>> + int iface, struct can_frame *frame, int objno) >>> +{ >>> + int i; >>> + u16 flags = 0; >>> + unsigned int id; >>> + struct c_can_priv *priv = netdev_priv(dev); >>> + >>> + if (frame->can_id & CAN_EFF_FLAG) { >>> + id = frame->can_id & CAN_EFF_MASK; >>> + flags |= IF_ARB_MSGXTD; >>> + } else >>> + id = ((frame->can_id & CAN_SFF_MASK) << 18); I just realize that the {} for the else block is missing. >>> + /* >>> + * check for 'last error code' which tells us the >>> + * type of the last error to occur on the CAN bus >>> + */ >>> + switch (lec_type) { >>> + /* common for all type of bus errors */ >>> + priv->can.can_stats.bus_error++; >>> + stats->rx_errors++; >>> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; >>> + cf->data[2] |= CAN_ERR_PROT_UNSPEC; >> >> Are you sure that this part is ever executed? I wonder why the compile >> does >> not complain. > > I did not get any compilation error. I know. > But I will check if the program flow enters this part or not. Good. It was *not* executed in my little user space test program. ... >>> +static int __devexit c_can_plat_remove(struct platform_device *pdev) >>> +{ >>> + struct net_device *dev = platform_get_drvdata(pdev); >>> + struct c_can_priv *priv = netdev_priv(dev); >>> + struct resource *mem; >>> + >>> + /* disable all interrupts */ >>> + c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS); >> >> To avoid exportign that function, couldn't it be done at the beginning >> of >> unregister_c_can_dev()? > > Yes this can be done. > But, IMHO *disabling* interrupts seems more logical as part of __devexit > Code. I think the interrupts are already disabled when you unload the module because the device must be closed (c_can_stop has been called). Anyway, I think the above c_can_enable_all_interrupts() call is well placed in the unregister function. I would avoid the export the function. Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
