Hi, Thanks Jonathan for your suggestion. My laptop drinks some coffee. It will recover until next Tuesday. Please wait a moment.
regards xue liu 2010/7/19 Jonathan Cameron <[email protected]>: > > Hi, > > On 07/18/10 13:39, Xue Liu wrote: >>>From 7601e0cca43dbbd02d5d7e33d3bc858d8acaf384 Mon Sep 17 00:00:00 2001 >> From: liuxue <liu...@liuxue-laptop.(none)> > You are going to want to fix your address unless you want that getting > in the logs! >> Date: Sun, 18 Jul 2010 20:24:24 +0800 >> Subject: [PATCH] ieee802154/cc2420:Add new functions and etc >> >> * Add cc2420 ram access function > Why? read_ram isn't used. Please tell us if this is only > the first of a series of patches and these will be used later. > If not conventionally it should be here. > >> * Add cc2420_ed() > Not actually true. It was there before, you've merely > made it actually do something, please adjust comment. >> and cc2420_set_hw_addr_filt() > Excelent. (I haven't reviewed it as I don't have the data > sheet to hand right now). >> * Add sysfs support for debug > Fine for extremely temporary functions, but using sysfs for > debug functionality is never going into mainline and also, > you have way more than one value per file coming out. > Take a look at debugfs.. > > It would be better to never let anything like this that will > block an eventual mainline merge from ever getting into > the drivers. > >> * Delete useless comments >> * Update my profile > > Whole patch looks fundamentally sound to me. Please fix the formatting > errors below and make sure checkpatch is happy. > > Could you perhaps rip out the sysfs debug code? > > Also you have introduced a #if 0 block. If you really want > that it certainly needs some explanation! > > Thanks and keep up the good work, it is great > to have a decent driver for this device at last. > > Jonathan > >> --- >> drivers/ieee802154/cc2420.c | 248 >> +++++++++++++++++++++++++++++++++++++------ >> include/linux/spi/cc2420.h | 2 +- >> 2 files changed, 218 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/ieee802154/cc2420.c b/drivers/ieee802154/cc2420.c >> index 7839ddb..8a6d9fb 100644 >> --- a/drivers/ieee802154/cc2420.c >> +++ b/drivers/ieee802154/cc2420.c >> @@ -14,7 +14,7 @@ >> * >> * Author: Jonathan Cameron <[email protected]> >> * >> - * Modified 2010: liuxue <[email protected]> >> + * Modified 2010: xue liu <[email protected]> >> */ >> >> #include <linux/kernel.h> >> @@ -34,22 +34,45 @@ >> >> #define CC2420_WRITEREG(x) (x) >> #define CC2420_READREG(x) (0x40 | x) >> +#define CC2420_RAMADDR(x) ((x & 0x7F) | 0x80) >> +#define CC2420_RAMBANK(x) ((x >> 1) & 0xc0) >> +#define CC2420_WRITERAM(x) (x) >> +#define CC2420_READRAM(x) (0x20 | x) >> >> #define CC2420_FREQ_MASK 0x3FF >> #define CC2420_ADR_DECODE_MASK 0x0B00 >> #define CC2420_FIFOP_THR_MASK 0x003F >> #define CC2420_CRC_MASK 0x80 >> +#define CC2420_RSSI_MASK 0x7F >> +#define CC2420_FSMSTATE_MASK 0x2F >> >> #define CC2420_MANFIDLOW 0x233D >> #define CC2420_MANFIDHIGH 0x3000 /* my chip appears to version 3 - >> broaden this with testing */ >> >> +#define RSSI_OFFSET 45 >> + >> #define STATE_PDOWN 0 >> #define STATE_IDLE 1 >> -#define STATE_RX_CALIB 2 >> -#define STATE_RX_CALIB2 40 >> +#define STATE_RX_CALIBRATE 2 >> +#define STATE_RX_CALIBRATE2 40 >> >> #define STATE_RX_SFD_SEARCH_MIN 3 >> #define STATE_RX_SFD_SEARCH_MAX 6 >> +#define STATE_RX_FRAME 16 >> +#define STATE_RX_FRAME2 40 /* Same with >> STATE_RX_CALIB2 ? */ >> +#define STATE_RX_WAIT 14 >> +#define STATE_RX_OVERFLOW 17 >> +#define STATE_TX_ACK_CALIBRATE 48 >> +#define STATE_TX_ACK_PREAMBLE_MIN 49 >> +#define STATE_TX_ACK_PREAMBLE_MAX 51 >> +#define STATE_TX_ACK_MIN 52 >> +#define STATE_TX_ACK_MAX 54 >> +#define STATE_TX_CALIBRATE 32 >> +#define STATE_TX_PREAMBLE_MIN 34 >> +#define STATE_TX_PREAMBLE_MAX 36 >> +#define STATE_TX_FRAME_MIN 37 >> +#define STATE_TX_FRAME_MAX 39 >> +#define STATE_TX_UNDERFLOW 56 >> >> struct cc2420_local { >> struct cc2420_platform_data *pdata; >> @@ -168,10 +191,6 @@ static int cc2420_write_16_bit_reg_partial(struct >> cc2420_local *lp, >> >> lp->buf[0] = CC2420_WRITEREG(addr); >> >> - //dev_vdbg(&lp->spi->dev, "test: ~(mask >> 8) | (data >> 8) = %x\n", >> ~(mask >> 8) | (data >> 8)); >> - //dev_vdbg(&lp->spi->dev, "test: ~(mask & 0xFF) | (data & 0xFF) = >> %x\n", ~(mask & 0xFF) | (data & 0xFF)); >> - //lp->buf[1] &= ~(mask >> 8) | (data >> 8); >> - //lp->buf[2] &= ~(mask & 0xFF) | (data & 0xFF); >> lp->buf[1] &= ~(mask >> 8); >> lp->buf[2] &= ~(mask & 0xFF); >> lp->buf[1] |= (mask >> 8) & (data >> 8); >> @@ -191,8 +210,7 @@ err_ret: >> return ret; >> } >> >> -static int >> -cc2420_channel(struct ieee802154_dev *dev, int channel) >> +static int cc2420_channel(struct ieee802154_dev *dev, int channel) >> { >> struct cc2420_local *lp = dev->priv; >> int ret; >> @@ -209,8 +227,77 @@ cc2420_channel(struct ieee802154_dev *dev, int channel) >> return ret; >> } >> >> -static int >> -cc2420_write_txfifo(struct cc2420_local *lp, u8 *data, u8 len) >> +static int cc2420_read_ram(struct cc2420_local *lp, u16 addr, u8 len, u8 >> *data) >> +{ >> + int status; >> + struct spi_message msg; >> + struct spi_transfer xfer_head = { >> + .len = 2, >> + .tx_buf = lp->buf, >> + .rx_buf = lp->buf, >> + }; >> + struct spi_transfer xfer_buf = { >> + .len = len, >> + .rx_buf = data, >> + }; >> + >> + mutex_lock(&lp->bmux); >> + lp->buf[0] = CC2420_RAMADDR(addr); >> + lp->buf[1] = CC2420_READRAM(CC2420_RAMBANK(addr)); >> + dev_dbg(&lp->spi->dev, "read ram addr buf[0] = %02x\n", lp->buf[0]); >> + dev_dbg(&lp->spi->dev, "mem bank buf[1] = %02x\n", lp->buf[1]); >> + spi_message_init(&msg); >> + spi_message_add_tail(&xfer_head, &msg); >> + spi_message_add_tail(&xfer_buf, &msg); >> + >> + status = spi_sync(lp->spi, &msg); >> + dev_dbg(&lp->spi->dev, "spi status = %d\n", status); >> + if (msg.status) >> + status = msg.status; >> + dev_dbg(&lp->spi->dev, "return cc2420 status buf[0] = %02x\n", >> lp->buf[0]); >> + dev_dbg(&lp->spi->dev, "buf[1] = %02x\n", lp->buf[1]); >> + >> + mutex_unlock(&lp->bmux); >> + >> + return status; >> +} >> + >> +static int cc2420_write_ram(struct cc2420_local *lp, u16 addr, u8 >> len, u8 *data) >> +{ >> + int status; >> + struct spi_message msg; >> + struct spi_transfer xfer_head = { >> + .len = 2, >> + .tx_buf = lp->buf, >> + .rx_buf = lp->buf, >> + }; >> + struct spi_transfer xfer_buf = { >> + .len = len, >> + .tx_buf = data, >> + }; >> + >> + mutex_lock(&lp->bmux); >> + lp->buf[0] = CC2420_RAMADDR(addr); >> + lp->buf[1] = CC2420_WRITERAM(CC2420_RAMBANK(addr)); >> + dev_dbg(&lp->spi->dev, "write ram addr buf[0] = %02x\n", lp->buf[0]); >> + dev_dbg(&lp->spi->dev, "ram bank buf[1] = %02x\n", lp->buf[1]); >> + >> + spi_message_init(&msg); >> + spi_message_add_tail(&xfer_head, &msg); >> + spi_message_add_tail(&xfer_buf, &msg); >> + >> + status = spi_sync(lp->spi, &msg); >> + dev_dbg(&lp->spi->dev, "spi status = %d\n", status); >> + if (msg.status) >> + status = msg.status; >> + dev_dbg(&lp->spi->dev, "cc2420 status = %02x\n", lp->buf[0]); >> + dev_dbg(&lp->spi->dev, "buf[1] = %02x\n", lp->buf[1]); >> + >> + mutex_unlock(&lp->bmux); >> + return status; >> +} >> + >> +static int cc2420_write_txfifo(struct cc2420_local *lp, u8 *data, u8 len) >> { >> int status; >> /* Length byte must include FCS even if calculated in hardware */ >> @@ -250,8 +337,7 @@ cc2420_write_txfifo(struct cc2420_local *lp, u8 >> *data, u8 len) >> return status; >> } >> >> -static int >> -cc2420_read_rxfifo(struct cc2420_local *lp, u8 *data, u8 *len, u8 *lqi) >> +static int cc2420_read_rxfifo(struct cc2420_local *lp, u8 *data, u8 >> *len, u8 *lqi) >> { >> int status; >> struct spi_message msg; >> @@ -291,8 +377,7 @@ cc2420_read_rxfifo(struct cc2420_local *lp, u8 >> *data, u8 *len, u8 *lqi) >> } >> >> >> -static int >> -cc2420_tx(struct ieee802154_dev *dev, struct sk_buff *skb) >> +static int cc2420_tx(struct ieee802154_dev *dev, struct sk_buff *skb) >> { >> struct cc2420_local *lp = dev->priv; >> int rc; >> @@ -378,23 +463,72 @@ static int cc2420_rx(struct cc2420_local *lp) >> return 0; >> } >> >> -static int >> -cc2420_ed(struct ieee802154_dev *dev, u8 *level) >> +static int cc2420_set_hw_addr_filt(struct ieee802154_dev *dev, >> + struct ieee802154_hw_addr_filt *filt, >> + unsigned long changed) >> +{ >> + struct cc2420_local *lp = dev->priv; >> + u16 reg; >> + >> + might_sleep(); >> + > Something dubious going on with formatting. Please run > checkpatch over the patch. >> + dev_dbg(&lp->spi->dev, "cc2420_set_hw_addr_filt\n"); >> + > > checkpagch isn't going to like the unnecessary {} either. >> + if (changed & IEEE802515_IEEEADDR_CHANGED) { >> + cc2420_write_ram(lp, CC2420_RAM_IEEEADR, >> + IEEE802154_ADDR_LEN, >> filt->ieee_addr); >> + } >> + >> + if (changed & IEEE802515_SADDR_CHANGED) { >> + u8 short_addr[2]; >> + short_addr[0] = filt->short_addr & 0xff; /* LSB */ >> + short_addr[1] = filt->short_addr >> 8; /* MSB */ >> + cc2420_write_ram(lp, CC2420_RAM_SHORTADR, 2, short_addr); >> + } >> + >> + if (changed & IEEE802515_PANID_CHANGED) { >> + u8 panid[2]; >> + panid[0] = filt->pan_id & 0xff; /* LSB */ >> + panid[1] = filt->pan_id >> 8; /* MSB */ >> + cc2420_write_ram(lp, CC2420_RAM_PANID, 2, panid); >> + } >> + >> + if (changed & IEEE802515_PANC_CHANGED) { >> + cc2420_read_16_bit_reg(lp, CC2420_MDMCTRL0, ®); >> + if (filt->pan_coord) >> + reg |= 1 << CC2420_MDMCTRL0_PANCRD; >> + else >> + reg &= ~(1 << CC2420_MDMCTRL0_PANCRD); >> + cc2420_write_16_bit_reg_partial(lp, CC2420_MDMCTRL0, >> + >> reg, 1 << CC2420_MDMCTRL0_PANCRD); >> + } >> + >> + return 0; >> +} >> + >> +static int cc2420_ed(struct ieee802154_dev *dev, u8 *level) >> { >> struct cc2420_local *lp = dev->priv; >> + u16 rssi; >> + int ret; >> dev_dbg(&lp->spi->dev, "ed called\n"); >> - *level = 0xbe; >> - return 0; >> + >> + /* You should know *data present a signed number */ >> + ret = cc2420_read_16_bit_reg(lp, CC2420_RSSI, &rssi); >> + if (ret) >> + return ret; >> + >> + /* P = RSSI_VAL + RSSI_OFFSET[dBm] */ >> + *level = (rssi & CC2420_RSSI_MASK) + RSSI_OFFSET; >> + return ret; >> } >> >> -static int >> -cc2420_start(struct ieee802154_dev *dev) >> +static int cc2420_start(struct ieee802154_dev *dev) >> { >> return cc2420_cmd_strobe(dev->priv, CC2420_SRXON); >> } >> >> -static void >> -cc2420_stop(struct ieee802154_dev *dev) >> +static void cc2420_stop(struct ieee802154_dev *dev) >> { >> cc2420_cmd_strobe(dev->priv, CC2420_SRFOFF); >> } >> @@ -406,6 +540,45 @@ static struct ieee802154_ops cc2420_ops = { >> .start = cc2420_start, >> .stop = cc2420_stop, >> .set_channel = cc2420_channel, >> + .set_hw_addr_filt = cc2420_set_hw_addr_filt, >> +}; >> + >> +static ssize_t cc2420_show(struct device *dev, >> + struct device_attribute *devattr, >> + char *buf) >> +{ >> + struct cc2420_local *lp = dev_get_drvdata(dev); >> + u16 stat; >> + > spaces instead of tab. checkpatch again.... >> + cc2420_read_16_bit_reg(lp, CC2420_FSMSTATE, &stat); >> + stat &= CC2420_FSMSTATE_MASK; >> + > Gah! don't let Greg KH or anyone else see this abuse > of sysfs.. >> + return sprintf(buf, "Radio Control States = >> %X:\n%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n", stat, >> + stat == STATE_PDOWN ? "POWER DOWN" : "", >> + stat == STATE_IDLE ? "IDLE" : "", >> + stat == STATE_RX_CALIBRATE ? "RX_CALIBRATE" >> : "", >> + stat == STATE_RX_WAIT ? "RX_WAIT" : "", >> + stat == STATE_RX_OVERFLOW ? "RX_OVERFLOW" : >> "", >> + stat <= STATE_RX_SFD_SEARCH_MAX && stat >= >> STATE_RX_SFD_SEARCH_MIN ? "RX_SFD_SEARCH" : "", >> + stat <= STATE_TX_ACK_MAX && stat >= >> STATE_TX_ACK_MIN ? "TX_ACK" : "", >> + stat <= STATE_TX_ACK_PREAMBLE_MAX && stat >= >> STATE_TX_ACK_PREAMBLE_MIN ? "TX_ACK_PREAMBLE" : "", >> + stat <= STATE_TX_PREAMBLE_MAX && stat >= >> STATE_TX_PREAMBLE_MIN >> ? "TX_PREAMBLE" : "", >> + stat <= STATE_TX_FRAME_MAX && stat >= >> STATE_TX_FRAME_MIN ? >> "TX_FRAME" : "", >> + stat == STATE_RX_FRAME ? "RX_FRAME" : "", >> + stat == STATE_TX_ACK_CALIBRATE ? >> "TX_ACK_CALIBRATE" : "", >> + stat == STATE_TX_CALIBRATE ? "TX_CALIBRATE" >> : "", >> + stat == STATE_TX_UNDERFLOW ? "TX_UNDERFLOW" >> : ""); >> +} >> + >> +static DEVICE_ATTR(cc2420_fs, 0664, cc2420_show, NULL); >> + >> +static struct attribute *cc2420_attributes[] = { >> + &dev_attr_cc2420_fs.attr, >> + NULL, >> +}; >> + >> +static const struct attribute_group cc2420_attr_group = { >> + .attrs = cc2420_attributes, >> }; >> >> static int cc2420_register(struct cc2420_local *lp) >> @@ -431,6 +604,7 @@ static int cc2420_register(struct cc2420_local *lp) >> ret = ieee802154_register_device(lp->dev); >> if (ret) >> goto err_free_device; >> + >> return 0; >> err_free_device: >> ieee802154_free_device(lp->dev); >> @@ -456,7 +630,6 @@ static irqreturn_t cc2420_isr(int irq, void *data) >> } >> spin_unlock(&lp->lock); >> >> - /* pin or value? */ >> if (irq == lp->sfd_irq) >> schedule_work(&lp->sfd_irqwork); >> >> @@ -678,20 +851,31 @@ static int __devinit cc2420_probe(struct spi_device >> *spi) >> goto err_free_sfd_irq; >> } >> >> - dev_dbg(&lp->spi->dev, "Disable hardware address decoding\n"); >> - cc2420_write_16_bit_reg_partial(lp, CC2420_MDMCTRL0, >> - 0, 1 << CC2420_MDMCTRL0_ADRDECODE); >> + /* We have cc2420_set_hw_addr_filt. */ > > This needs some explanation if you are going to leave it here. >> +#if 0 >> + dev_dbg(&lp->spi->dev, "Disable hardware address decoding\n"); >> + cc2420_write_16_bit_reg_partial(lp, CC2420_MDMCTRL0, >> + 0, 1 << CC2420_MDMCTRL0_ADRDECODE); >> +#endif >> dev_info(&lp->spi->dev, "Set fifo threshold to 127\n"); >> cc2420_write_16_bit_reg_partial(lp, CC2420_IOCFG0, 127, >> CC2420_FIFOP_THR_MASK); >> ret = cc2420_register(lp); >> if (ret) >> goto err_free_sfd_irq; >> >> + dev_set_drvdata(&spi->dev, lp); >> + >> + ret = sysfs_create_group(&spi->dev.kobj, &cc2420_attr_group); >> + if (ret) { >> + dev_err(&spi->dev, "sysfs_create_group failed!\n"); >> + goto err_free_sfd_irq; >> + } >> + >> return 0; >> err_free_sfd_irq: >> - free_irq(gpio_to_irq(lp->pdata->sfd), lp); >> + free_irq(lp->sfd_irq, lp); >> err_free_fifop_irq: >> - free_irq(gpio_to_irq(lp->pdata->fifop), lp); >> + free_irq(lp->fifop_irq, lp); >> err_disable_vreg: >> gpio_set_value(lp->pdata->vreg, 0); >> err_free_gpio_vreg: >> @@ -719,8 +903,10 @@ static int __devexit cc2420_remove(struct spi_device >> *spi) >> struct cc2420_local *lp = spi_get_drvdata(spi); >> >> cc2420_unregister(lp); >> - free_irq(gpio_to_irq(lp->pdata->fifop), lp); >> - free_irq(gpio_to_irq(lp->pdata->sfd), lp); >> + free_irq(lp->fifop_irq, lp); >> + free_irq(lp->sfd_irq, lp); >> + flush_work(&lp->fifop_irqwork); >> + flush_work(&lp->sfd_irqwork); >> gpio_free(lp->pdata->vreg); >> gpio_free(lp->pdata->reset); >> gpio_free(lp->pdata->sfd); >> diff --git a/include/linux/spi/cc2420.h b/include/linux/spi/cc2420.h >> index 79b699d..5db3d58 100644 >> --- a/include/linux/spi/cc2420.h >> +++ b/include/linux/spi/cc2420.h >> @@ -14,7 +14,7 @@ >> * >> * Author: Jonathan Cameron <[email protected]> >> * >> - * Modified 2010: liuxue <[email protected]> >> + * Modified 2010: xue liu <[email protected]> >> */ >> >> #ifndef __CC2420_H >> >> >> >> ------------------------------------------------------------------------------ >> This SF.net email is sponsored by Sprint >> What will you do first with EVO, the first 4G phone? >> Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first >> >> >> >> _______________________________________________ >> Linux-zigbee-devel mailing list >> [email protected] >> https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel > > ------------------------------------------------------------------------------ This SF.net email is sponsored by Sprint What will you do first with EVO, the first 4G phone? Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first _______________________________________________ Linux-zigbee-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
