Messages in this thread | | | From | "Masayuki Ohtake" <> | Subject | Re: [MeeGo-Dev][PATCH v3] Topcliff: Update PCH_CAN driver to 2.6.35 | Date | Wed, 13 Oct 2010 19:09:42 +0900 |
| |
On Thursday, September 30, 2010 6:10 PM, Wolfgang Grandegger wrote:
> > + iowrite32(num, &(priv->regs)->if2_creq); > + while (counter) { >> + 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.
Though it is possible to integrate to one function by adding parameter, I think, current two function method is more easily to read.
> > > >> + 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); > >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.
> > > + 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. Because this is our HW specification. Before setting bitrate, run-mode must be "STOP".
> > > +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.
> > > + /* 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, Ohtake(OKISemi)
| |