On 10/13/2010 12:09 PM, Masayuki Ohtake wrote:

On Thursday, September 30, 2010 6:10 PM, Wolfgang Grandegger wrote:

quoted

+ iowrite32(num, &(priv->regs)->if2_creq);
+ while (counter) {

quoted

+ if2_creq = (ioread32(&(priv->regs)->if2_creq)) &
+      CAN_IF_CREQ_BUSY;
+ if (!if2_creq)
+ break;
+ counter--;
+ }
+ if (!counter)
+ dev_err(&priv->ndev->dev, "IF2 BUSY Flag is set forever.\n");
+}

Duplicated code!

No.
These are not the same.

Of course they are not the same. The only difference is the register
offset (of if1 or if2). A common function with a pointer to the if
register as argument makes sense.

Though it is possible to integrate to one function by adding parameter,
I think, current two function method is more easily to read.

I disagree.

quoted

quoted

+ if (status & PCH_STUF_ERR)
+ cf->data[2] |= CAN_ERR_PROT_STUFF;
+
+ if (status & PCH_FORM_ERR)
+ cf->data[2] |= CAN_ERR_PROT_FORM;

+
+ if (status & PCH_ACK_ERR)
+ cf->data[2] |= CAN_ERR_PROT_LOC_ACK | CAN_ERR_PROT_LOC_ACK_DEL;
+
+ if ((status & PCH_BIT1_ERR) || (status & PCH_BIT0_ERR))
+ cf->data[2] |= CAN_ERR_PROT_BIT;
+
+ if (status & PCH_CRC_ERR)
+ cf->data[2] |= CAN_ERR_PROT_LOC_CRC_SEQ |
+ CAN_ERR_PROT_LOC_CRC_DEL;
+
+ if (status & PCH_LEC_ALL)
+ iowrite32(status | PCH_LEC_ALL,
+   &(priv->regs)->stat);

Well, if status==7 (PCH_LEC_ALL), all of the above conditions are true
as well... convinced now?

quoted

A bit-wise test of the above values is wrong, I believe. Please use the
switch statement instead.

The above conditions are not only one time.
I think "switch" is not suitable for the above.
Thus, current "if" processing is better.

I don't understand! The Last Error Code (LEC) can have values from 0 to
7. A "switch" statement is therefore the right choice. Or have I missed
something.

quoted

+ u32 brp;
+
+ pch_can_get_run_mode(priv, &curr_mode);
+ if (curr_mode == PCH_CAN_RUN)
+ pch_can_set_run_mode(priv, PCH_CAN_STOP);

The device is stopped when this function is called. Please remove.

No.
The above is necessary.

Yes, because you started the device *too early* in pch_can_open() called
by pch_open(). See my other related comments of my previous mail.

Because this is our HW specification.
Before setting bitrate, run-mode must be "STOP".

I think it can be avoided easily.

quoted

+static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+ canid_t id;
+ u32 id1 = 0;
+ u32 id2 = 0;

Need these values to be preset?

These values are not essential.
But these help a engineer to read this code.

I disagree.

quoted

+ /* Enable CAN Interrupts */
+ pch_can_set_int_custom(priv);
+
+ /* Restore Run Mode */
+ pch_can_set_run_mode(priv, PCH_CAN_RUN);
+
+ return retval;
+}

Are the suspend and resume functions tested?

Yes, we tested before.

=========================================

Except the above, we are modifying for your indications.

I will resubmit soon.

Thanks,

Wolfgang.