Re: [PATCH net-next-2.6 v3 1/1] can: c_can: Added support for Bosch C_CAN controller

Hi Bhupesh,

On 01/11/2011 05:50 AM, Bhupesh SHARMA wrote:

Hi Wolfgang,

Thanks for your review.
Please see my comments inline.

quoted

-----Original Message-----
From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]

...

quoted

quoted

+ * Bosch C_CAN controller is compliant to CAN protocol version 2.0

part A and B.

quoted

+ * 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.

...

quoted

quoted

+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.

quoted

quoted

+   /*
+    * 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.

...

quoted

quoted

+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.