Hello.
On Tue, 2010-04-13 at 09:20, Hennerich, Michael wrote:
> Stefan Schmidt wrote on 2010-04-12:
> > > +
> > > +/*
> > > + * DEBUG LEVEL
> > > + * 0 OFF
> > > + * 1 INFO
> > > + * 2 INFO + TRACE
> > > + */
> > > +
> > > +#define ADF_DEBUG 0
> > > +#define DBG(n, args...) do { if (ADF_DEBUG >= (n)) pr_debug(args); }
> > while (0)
> >
> > Why do you need an extra layer over the standard debug macros?
>
> It allows me to set different types of verbosity.
> In addition and depending on the Debug Level - these statements get
> completely optimized away by gcc.
I still wonder why you need all this debugging is needed when the driver is
working and ready for mainline submission.
> > Wrong indentation.
>
> Well - looking at the source I made this patch from, the indention looks
> right.
> This applies to all your highlighted indention problems.
> I think the problem is caused by my outlook email client. Shame on me :-)
The you should consider fixing this setup: Documentation/email-clients.txt
> > > +static int adf7242_wait_ready(struct adf7242_local *lp)
> > > +{
> > > + u8 stat;
> > > + int cnt = 0;
> > > +
> > > + DBG(2, "%s :Enter\n", __func__);
> > > +
> > > + do {
> > > + adf7242_status(lp, &stat);
> > > + cnt++;
> > > + } while (!(stat & STAT_RC_READY) && (cnt < MAX_POLL_LOOPS));
> > > +
> > > + DBG(2, "%s :Exit loops=%d\n", __func__, cnt);
> > > +
> > > + return 0;
> > > +}
Does not seem to be a MAC function and again int return value that is always 0.
> > > +static int adf7242_wait_status(struct adf7242_local *lp, int status)
> > > +{
> > > + u8 stat;
> > > + int cnt = 0;
> > > +
> > > + DBG(2, "%s :Enter\n", __func__);
> > > +
> > > + do {
> > > + adf7242_status(lp, &stat);
> > > + stat &= RC_STATUS_MASK;
> > > + cnt++;
> > > + } while ((stat != status) && (cnt < MAX_POLL_LOOPS));
> > > +
> > > + DBG(2, "%s :Exit loops=%d\n", __func__, cnt);
> > > +
> > > + return 0;
> > > +}
Same here.
> > > +static int adf7242_read_fbuf(struct adf7242_local *lp,
> > > + u8 *data, u8 *len, u8 *lqi)
> > > +{
> > > + u8 *buf = lp->buf;
> > > + int status;
> > > + struct spi_message msg;
> > > + struct spi_transfer xfer_head = {
> > > + .len = 3,
> > > + .tx_buf = buf,
> > > + .rx_buf = buf,
> > > +
> > > + };
> > > + struct spi_transfer xfer_buf = {
> > > + .len = *len,
> > > + .rx_buf = data,
> > > + };
> > > +
> > > + DBG(2, "%s :Enter\n", __func__);
> > > + adf7242_wait_ready(lp);
> > > +
> > > + mutex_lock(&lp->bmux);
> >
> > Move this mutex around spi_sync as in the other functions?
>
> This mutex is supposed to also protect lp->buf.
Hmm, I don't see the need for this. Care to elaborate?
> > > +static int adf7242_ed(struct ieee802154_dev *dev, u8 *level)
> > > +{
> > > + struct adf7242_local *lp = dev->priv;
> > > + int ret;
> > > +
> > > + DBG(2, "%s :Enter\n", __func__);
> > > +#if 0
> > > + adf7242_cmd(lp, CMD_RC_PHY_RDY);
> > > + adf7242_cmd(lp, CMD_RC_CCA);
> > > + adf7242_wait_status(lp, RC_STATUS_PHY_RDY);
> > > +#else
> >
> > Dead code.
>
> I'm not yet 100% decided which one to use.
> I'll clean that up some day.
A penny for everytime I heard this. Such code stays forever.
> > > +static int adf7242_channel(struct ieee802154_dev *dev, int channel)
> > > +{
> > > + struct adf7242_local *lp = dev->priv;
> > > + unsigned long freq;
> > > +
> > > + DBG(2, "%s :Enter\n", __func__);
> > > + DBG(1, "%s :Channel=%d\n", __func__, channel);
> > > +
> > > + might_sleep();
> > > +
> > > + BUG_ON(channel < 11);
> > > + BUG_ON(channel > 26);
> > > +
> > > + freq = (2405 + 5 * (channel - 11)) * 100;
> > > +
> > > + adf7242_cmd(lp, CMD_RC_PHY_RDY);
> > > +
> > > + adf7242_write_reg(lp, REG_CH_FREQ0, freq);
> > > + adf7242_write_reg(lp, REG_CH_FREQ1, freq >> 8);
> > > + adf7242_write_reg(lp, REG_CH_FREQ2, freq >> 16);
> > > +
> > > + adf7242_cmd(lp, CMD_RC_RX);
> > > +
> > > + dev->phy->current_channel = channel;
> > > + DBG(2, "%s :Exit\n", __func__);
> > > +
> > > + return 0;
> >
> > Could be made a void return value as we never return anything useful.
>
> Well - this is a common mac8021511 function!
> If you think this function should be better return void - feel free to
> propose an API change.
Right. I did not realize this. Sorry. Other driver might have something useful
to return here so an int makes sense.
> > > +static int adf7242_suspend(struct spi_device *spi, pm_message_t
> > message)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static int adf7242_resume(struct spi_device *spi)
> > > +{
> > > + return 0;
> > > +}
> >
> > If you don't implement PM you could just leave it out. At least be
> > prepared to
> > compile without CONFIG_PM set.
>
> I consider this as reminder to implement suspend/resume.
> These functions will be implemented soon.
Add them when you actually implement it.
> > > +static ssize_t adf7242_show(struct device *dev,
> > > + struct device_attribute *devattr,
> > > + char *buf)
> > > +{
> > > + struct adf7242_local *lp = dev_get_drvdata(dev);
> > > + u8 stat;
> > > +
> > > + adf7242_status(lp, &stat);
> > > +
> > > + return sprintf(buf, "STATUS = %X:\n%s\n%s%s%s%s%s\n", stat,
> > > + stat & STAT_RC_READY ? "RC_READY" : "RC_BUSY",
> > > + (stat & 0xf) == RC_STATUS_IDLE ? "RC_STATUS_IDLE" :
> > "",
> > > + (stat & 0xf) == RC_STATUS_MEAS ? "RC_STATUS_MEAS" :
> > "",
> > > + (stat & 0xf) == RC_STATUS_PHY_RDY ?
> > "RC_STATUS_PHY_RDY" : "",
> > > + (stat & 0xf) == RC_STATUS_RX ? "RC_STATUS_RX" : "",
> > > + (stat & 0xf) == RC_STATUS_TX ? "RC_STATUS_TX" : "");
> > > +
> > > +}
> > > +static DEVICE_ATTR(status, 0664, adf7242_show, NULL);
> >
> > This needs to be dosumented as sysfs ABI. Hard to remove later. Are you
> > sure
> > that you actually need it?
>
> No - this was just for debug purposes.
What is the plan with it then? Once it hits Linus tree you can't remove it
anymore easily. You have to go through feature-removal-schedule.txt
Also you still would need tro document it.
regards
Stefan Schmidt
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Linux-zigbee-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
