Hello, On 01/11/2011 09:29 AM, Wolfgang Grandegger wrote: > 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.
I was told that Bosch's page for the C_CAN is here: http://www.semiconductors.bosch.de/en/ipmodules/can/canipmodules/c_can/c_can.asp There it's also written for what bus interfaces the C_CAN is available for, e.g.: "The ASIC version is delivered with the AMBA APB bus interface from ARM." which is obviously used in the "Platform Controller Hub" eg20t. Wolfgang. > > ... >>>> +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 > > _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
