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
