* Re: [Linux-zigbee-devel] [PATCH net-next v1 1/3] ieee802154: cc2520: adds driver for TI CC2520 radio
2014-06-17 7:14 ` [PATCH net-next v1 1/3] ieee802154: cc2520: adds driver " Varka Bhadram
@ 2014-06-17 8:41 ` Alexander Aring
0 siblings, 0 replies; 2+ messages in thread
From: Alexander Aring @ 2014-06-17 8:41 UTC (permalink / raw)
To: Varka Bhadram
Cc: netdev, Varka Bhadram, linux-kernel, linux-zigbee-devel, davem
Hi Varka,
you are on v2 not v1. :-)
On Tue, Jun 17, 2014 at 12:44:56PM +0530, Varka Bhadram wrote:
> This patch adds the driver support for TI cc2520 radio.
>
> Few points about CC2520:
> - The CC2520 is TI's second generation IEEE 802.15.4 RF transceiver
> for the 2.4 GHz unlicensed ISM band.
> - DSSS baseband modem with 250 kbps data rate
> - Programmable output power up to +5 dBm
> - Excellent receiver sensitivity (-98 dBm)
>
I meant here more what the driver supports... not what the Transceiver
hardware supports.
> Signed-off-by: Varka Bhadram <varkab@cdac.in>
> ---
> drivers/net/ieee802154/cc2520.c | 966 +++++++++++++++++++++++++++++++++++++++
> include/linux/spi/cc2520.h | 26 ++
> 2 files changed, 992 insertions(+)
> create mode 100644 drivers/net/ieee802154/cc2520.c
> create mode 100644 include/linux/spi/cc2520.h
>
> diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
> new file mode 100644
> index 0000000..7e6afe3
> --- /dev/null
> +++ b/drivers/net/ieee802154/cc2520.c
> @@ -0,0 +1,966 @@
> +/* Driver for TI CC2520 802.15.4 Wireless-PAN Networking controller
> + *
> + * Copyright (C) 2014 Varka Bhadram <varkab@cdac.in>
> + * Md.Jamal Mohiuddin <mjmohiuddin@cdac.in>
> + * P Sowjanya <sowjanyap@cdac.in>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/gpio.h>
> +#include <linux/delay.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/cc2520.h>
> +#include <linux/workqueue.h>
> +#include <linux/interrupt.h>
> +
> +#include <linux/skbuff.h>
> +
> +#include <linux/pinctrl/consumer.h>
> +
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_gpio.h>
> +
> +#include <net/mac802154.h>
> +#include <net/wpan-phy.h>
> +
> +#define SPI_COMMAND_BUFFER 3
> +#define HIGH 1
> +#define LOW 0
> +#define STATE_IDLE 0
> +#define RSSI_VALID 0
> +#define RSSI_OFFSET 78
> +
> +#define CC2520_RAM_SIZE 640
> +#define CC2520_FIFO_SIZE 128
> +
> +#define CC2520RAM_TXFIFO 0x100,
> +#define CC2520RAM_RXFIFO 0x180,
> +#define CC2520RAM_IEEEADDR 0x3EA,
> +#define CC2520RAM_PANID 0x3F2,
> +#define CC2520RAM_SHORTADDR 0x3F4,
> +
> +#define CC2520_FREG_MASK 0x3F
> +
> +/*status byte values */
> +#define CC2520_STATUS_XOSC32M_STABLE (1 << 7)
> +#define CC2520_STATUS_RSSI_VALID (1 << 6)
> +#define CC2520_STATUS_TX_UNDERFLOW (1 << 3)
> +
> +/*IEEE 802.15.4 defined constants (2.4 GHz logical channels) */
> +#define CC2520_MINCHANNEL 11
> +#define CC2520_MAXCHANNEL 26
> +#define CC2520_CHANNEL_SPACING 5
> +
> +/*command strobes*/
> +#define CC2520_CMD_SNOP 0x00
> +#define CC2520_CMD_IBUFLD 0x02
> +#define CC2520_CMD_SIBUFEX 0x03
> +#define CC2520_CMD_SSAMPLECCA 0x04
> +#define CC2520_CMD_SRES 0x0f
> +#define CC2520_CMD_MEMORY_MASK 0x0f
> +#define CC2520_CMD_MEMORY_READ 0x10
> +#define CC2520_CMD_MEMORY_WRITE 0x20
> +#define CC2520_CMD_RXBUF 0x30
> +#define CC2520_CMD_RXBUFCP 0x38
> +#define CC2520_CMD_RXBUFMOV 0x32
> +#define CC2520_CMD_TXBUF 0x3A
> +#define CC2520_CMD_TXBUFCP 0x3E
> +#define CC2520_CMD_RANDOM 0x3C
> +#define CC2520_CMD_SXOSCON 0x40
> +#define CC2520_CMD_STXCAL 0x41
> +#define CC2520_CMD_SRXON 0x42
> +#define CC2520_CMD_STXON 0x43
> +#define CC2520_CMD_STXONCCA 0x44
> +#define CC2520_CMD_SRFOFF 0x45
> +#define CC2520_CMD_SXOSCOFF 0x46
> +#define CC2520_CMD_SFLUSHRX 0x47
> +#define CC2520_CMD_SFLUSHTX 0x48
> +#define CC2520_CMD_SACK 0x49
> +#define CC2520_CMD_SACKPEND 0x4A
> +#define CC2520_CMD_SNACK 0x4B
> +#define CC2520_CMD_SRXMASKBITSET 0x4C
> +#define CC2520_CMD_SRXMASKBITCLR 0x4D
> +#define CC2520_CMD_RXMASKAND 0x4E
> +#define CC2520_CMD_RXMASKOR 0x4F
> +#define CC2520_CMD_MEMCP 0x50
> +#define CC2520_CMD_MEMCPR 0x52
> +#define CC2520_CMD_MEMXCP 0x54
> +#define CC2520_CMD_MEMXWR 0x56
> +#define CC2520_CMD_BCLR 0x58
> +#define CC2520_CMD_BSET 0x59
> +#define CC2520_CMD_CTR_UCTR 0x60
> +#define CC2520_CMD_CBCMAC 0x64
> +#define CC2520_CMD_UCBCMAC 0x66
> +#define CC2520_CMD_CCM 0x68
> +#define CC2520_CMD_UCCM 0x6A
> +#define CC2520_CMD_ECB 0x70
> +#define CC2520_CMD_ECBO 0x72
> +#define CC2520_CMD_ECBX 0x74
> +#define CC2520_CMD_INC 0x78
> +#define CC2520_CMD_ABORT 0x7F
> +#define CC2520_CMD_REGISTER_READ 0x80
> +#define CC2520_CMD_REGISTER_WRITE 0xC0
> +
> +/* status registers */
> +#define CC2520_CHIPID 0x40
> +#define CC2520_VERSION 0x42
> +#define CC2520_EXTCLOCK 0x44
> +#define CC2520_MDMCTRL0 0x46
> +#define CC2520_MDMCTRL1 0x47
> +#define CC2520_FREQEST 0x48
> +#define CC2520_RXCTRL 0x4A
> +#define CC2520_FSCTRL 0x4C
> +#define CC2520_FSCAL0 0x4E
> +#define CC2520_FSCAL1 0x4F
> +#define CC2520_FSCAL2 0x50
> +#define CC2520_FSCAL3 0x51
> +#define CC2520_AGCCTRL0 0x52
> +#define CC2520_AGCCTRL1 0x53
> +#define CC2520_AGCCTRL2 0x54
> +#define CC2520_AGCCTRL3 0x55
> +#define CC2520_ADCTEST0 0x56
> +#define CC2520_ADCTEST1 0x57
> +#define CC2520_ADCTEST2 0x58
> +#define CC2520_MDMTEST0 0x5A
> +#define CC2520_MDMTEST1 0x5B
> +#define CC2520_DACTEST0 0x5C
> +#define CC2520_DACTEST1 0x5D
> +#define CC2520_ATEST 0x5E
> +#define CC2520_DACTEST2 0x5F
> +#define CC2520_PTEST0 0x60
> +#define CC2520_PTEST1 0x61
> +#define CC2520_RESERVED 0x62
> +#define CC2520_DPUBIST 0x7A
> +#define CC2520_ACTBIST 0x7C
> +#define CC2520_RAMBIST 0x7E
> +
> +/*frame registers*/
> +#define CC2520_FRMFILT0 0x00
> +#define CC2520_FRMFILT1 0x01
> +#define CC2520_SRCMATCH 0x02
> +#define CC2520_SRCSHORTEN0 0x04
> +#define CC2520_SRCSHORTEN1 0x05
> +#define CC2520_SRCSHORTEN2 0x06
> +#define CC2520_SRCEXTEN0 0x08
> +#define CC2520_SRCEXTEN1 0x09
> +#define CC2520_SRCEXTEN2 0x0A
> +#define CC2520_FRMCTRL0 0x0C
> +#define CC2520_FRMCTRL1 0x0D
> +#define CC2520_RXENABLE0 0x0E
> +#define CC2520_RXENABLE1 0x0F
> +#define CC2520_EXCFLAG0 0x10
> +#define CC2520_EXCFLAG1 0x11
> +#define CC2520_EXCFLAG2 0x12
> +#define CC2520_EXCMASKA0 0x14
> +#define CC2520_EXCMASKA1 0x15
> +#define CC2520_EXCMASKA2 0x16
> +#define CC2520_EXCMASKB0 0x18
> +#define CC2520_EXCMASKB1 0x19
> +#define CC2520_EXCMASKB2 0x1A
> +#define CC2520_EXCBINDX0 0x1C
> +#define CC2520_EXCBINDX1 0x1D
> +#define CC2520_EXCBINDY0 0x1E
> +#define CC2520_EXCBINDY1 0x1F
> +#define CC2520_GPIOCTRL0 0x20
> +#define CC2520_GPIOCTRL1 0x21
> +#define CC2520_GPIOCTRL2 0x22
> +#define CC2520_GPIOCTRL3 0x23
> +#define CC2520_GPIOCTRL4 0x24
> +#define CC2520_GPIOCTRL5 0x25
> +#define CC2520_GPIOPOLARITY 0x26
> +#define CC2520_GPIOCTRL 0x28
> +#define CC2520_DPUCON 0x2A
> +#define CC2520_DPUSTAT 0x2C
> +#define CC2520_FREQCTRL 0x2E
> +#define CC2520_FREQTUNE 0x2F
> +#define CC2520_TXPOWER 0x30
> +#define CC2520_TXCTRL 0x31
> +#define CC2520_FSMSTAT0 0x32
> +#define CC2520_FSMSTAT1 0x33
> +#define CC2520_FIFOPCTRL 0x34
> +#define CC2520_FSMCTRL 0x35
> +#define CC2520_CCACTRL0 0x36
> +#define CC2520_CCACTRL1 0x37
> +#define CC2520_RSSI 0x38
> +#define CC2520_RSSISTAT 0x39
> +#define CC2520_RXFIRST 0x3C
> +#define CC2520_RXFIFOCNT 0x3E
> +#define CC2520_TXFIFOCNT 0x3F
> +
> +/* Driver private information */
> +struct cc2520_private {
> + struct spi_device *spi; /* spi device structure */
> + struct ieee802154_dev *dev; /* Ieee802.15.4 device */
> + u8 *buf; /* SPI TX/Rx data buffer */
> + struct mutex buffer_mutex; /* SPI buffer mutex */
> + unsigned is_tx:1; /* Flag for sync b/w Tx and Rx */
> + int fifo_pin; /* FIFO GPIO pin number */
> + struct work_struct fifop_irqwork;/* Workqueue for FIFOP */
> + spinlock_t lock; /* Spin lock */
> + struct completion tx_complete; /* Work completion for Tx */
> +};
> +
> +/* Generic Functions */
> +static int
> +cc2520_cmd_strobe(struct cc2520_private *priv, u8 cmd)
> +{
> + int ret;
> + u8 status = 0xff;
> + struct spi_message msg;
> + struct spi_transfer xfer = {
> + .len = 0,
> + .tx_buf = priv->buf,
> + .rx_buf = priv->buf,
> + };
> +
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfer, &msg);
> +
> + mutex_lock(&priv->buffer_mutex);
> + priv->buf[xfer.len++] = cmd;
> + dev_vdbg(&priv->spi->dev,
> + "command strobe buf[0] = %02x\n",
> + priv->buf[0]);
> +
> + ret = spi_sync(priv->spi, &msg);
> + if (!ret)
> + status = priv->buf[0];
> + dev_vdbg(&priv->spi->dev,
> + "buf[0] = %02x\n", priv->buf[0]);
> + mutex_unlock(&priv->buffer_mutex);
> +
> + return ret;
> +}
What's about to drop the lowlevel spi calls and use spi helper functions?
> +
> +static int
> +cc2520_get_status(struct cc2520_private *priv, u8 *status)
> +{
> + int ret;
> + struct spi_message msg;
> + struct spi_transfer xfer = {
> + .len = 0,
> + .tx_buf = priv->buf,
> + .rx_buf = priv->buf,
> + };
> +
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfer, &msg);
> +
> + mutex_lock(&priv->buffer_mutex);
> + priv->buf[xfer.len++] = CC2520_CMD_SNOP;
> + dev_vdbg(&priv->spi->dev,
> + "get status command buf[0] = %02x\n", priv->buf[0]);
> +
> + ret = spi_sync(priv->spi, &msg);
> + if (!ret)
> + *status = priv->buf[0];
> + dev_vdbg(&priv->spi->dev,
> + "buf[0] = %02x\n", priv->buf[0]);
> + mutex_unlock(&priv->buffer_mutex);
> +
> + return ret;
> +}
> +
> +static int
> +cc2520_write_register(struct cc2520_private *priv, u8 reg, u8 value)
> +{
> + int status;
> + struct spi_message msg;
> + struct spi_transfer xfer = {
> + .len = 0,
> + .tx_buf = priv->buf,
> + .rx_buf = priv->buf,
> + };
> +
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfer, &msg);
> +
> + mutex_lock(&priv->buffer_mutex);
> +
> + if (reg <= CC2520_FREG_MASK) {
> + priv->buf[xfer.len++] = CC2520_CMD_REGISTER_WRITE | reg;
> + priv->buf[xfer.len++] = value;
> + } else {
> + priv->buf[xfer.len++] = CC2520_CMD_MEMORY_WRITE;
> + priv->buf[xfer.len++] = reg;
> + priv->buf[xfer.len++] = value;
> + }
> + status = spi_sync(priv->spi, &msg);
> + if (msg.status)
> + status = msg.status;
> +
> + mutex_unlock(&priv->buffer_mutex);
> +
> + return status;
> +}
> +
> +static int
> +cc2520_read_register(struct cc2520_private *priv, u8 reg, u8 *data)
> +{
> + int status;
> + struct spi_message msg;
> + struct spi_transfer xfer1 = {
> + .len = 0,
> + .tx_buf = priv->buf,
> + .rx_buf = priv->buf,
> + };
> +
> + struct spi_transfer xfer2 = {
> + .len = 1,
> + .rx_buf = data,
> + };
> +
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfer1, &msg);
> + spi_message_add_tail(&xfer2, &msg);
> +
> + mutex_lock(&priv->buffer_mutex);
> + priv->buf[xfer1.len++] = CC2520_CMD_MEMORY_READ;
> + priv->buf[xfer1.len++] = reg;
> +
> + status = spi_sync(priv->spi, &msg);
> + dev_dbg(&priv->spi->dev,
> + "spi status = %d\n", status);
> + if (msg.status)
> + status = msg.status;
> +
> + mutex_unlock(&priv->buffer_mutex);
> +
> + return status;
> +}
> +
> +static int
> +cc2520_write_txfifo(struct cc2520_private *priv, u8 *data, u8 len)
> +{
> + int status;
> +
> + /*
> + *length byte must include FCS even
> + *if it is calculated in the hardware
> + */
> + int len_byte = len + 2;
> +
> + struct spi_message msg;
> +
> + struct spi_transfer xfer_head = {
> + .len = 0,
> + .tx_buf = priv->buf,
> + .rx_buf = priv->buf,
> + };
> + struct spi_transfer xfer_len = {
> + .len = 1,
> + .tx_buf = &len_byte,
> + };
> + struct spi_transfer xfer_buf = {
> + .len = len,
> + .tx_buf = data,
> + };
> +
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfer_head, &msg);
> + spi_message_add_tail(&xfer_len, &msg);
> + spi_message_add_tail(&xfer_buf, &msg);
> +
> + mutex_lock(&priv->buffer_mutex);
> + priv->buf[xfer_head.len++] = CC2520_CMD_TXBUF;
> + dev_vdbg(&priv->spi->dev,
> + "TX_FIFO cmd buf[0] = %02x\n", priv->buf[0]);
> +
> + status = spi_sync(priv->spi, &msg);
> + dev_vdbg(&priv->spi->dev, "status = %d\n", status);
> + if (msg.status)
> + status = msg.status;
> + dev_vdbg(&priv->spi->dev, "status = %d\n", status);
> + dev_vdbg(&priv->spi->dev, "buf[0] = %02x\n", priv->buf[0]);
> + mutex_unlock(&priv->buffer_mutex);
> +
> + return status;
> +}
> +
> +static int
> +cc2520_read_rxfifo(struct cc2520_private *priv, u8 *data, u8 *len, u8 *lqi)
why is len a pointer here? It's never changed in this function I would
prefer const u8 len.
> +{
> + int status;
> + struct spi_message msg;
> +
> + struct spi_transfer xfer_head = {
> + .len = 0,
> + .tx_buf = priv->buf,
> + .rx_buf = priv->buf,
> + };
> + struct spi_transfer xfer_buf = {
> + .len = *len,
> + .rx_buf = data,
> + };
> +
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfer_head, &msg);
> + spi_message_add_tail(&xfer_buf, &msg);
> +
> + mutex_lock(&priv->buffer_mutex);
> + priv->buf[xfer_head.len++] = CC2520_CMD_RXBUF;
> +
> + dev_vdbg(&priv->spi->dev, "read rxfifo buf[0] = %02x\n", priv->buf[0]);
> + dev_vdbg(&priv->spi->dev, "buf[1] = %02x\n", priv->buf[1]);
> +
> + status = spi_sync(priv->spi, &msg);
> + dev_vdbg(&priv->spi->dev, "status = %d\n", status);
> + if (msg.status)
> + status = msg.status;
> + dev_vdbg(&priv->spi->dev, "status = %d\n", status);
> + dev_vdbg(&priv->spi->dev,
> + "return status buf[0] = %02x\n", priv->buf[0]);
> + dev_vdbg(&priv->spi->dev, "length buf[1] = %02x\n", priv->buf[1]);
> +
> + *lqi = data[priv->buf[1] - 1] & 0x7f;
> +
> + mutex_unlock(&priv->buffer_mutex);
> +
> + return status;
> +}
> +
> +static int cc2520_start(struct ieee802154_dev *dev)
> +{
> + return cc2520_cmd_strobe(dev->priv, CC2520_CMD_SRXON);
> +}
> +
> +static void cc2520_stop(struct ieee802154_dev *dev)
> +{
> + cc2520_cmd_strobe(dev->priv, CC2520_CMD_SRFOFF);
> +}
> +
> +static int
> +cc2520_tx(struct ieee802154_dev *dev, struct sk_buff *skb)
> +{
> + struct cc2520_private *priv = dev->priv;
> + unsigned long flags;
> + int rc;
> + u8 status = 0;
> +
> + rc = cc2520_cmd_strobe(priv, CC2520_CMD_SFLUSHTX);
> + if (rc)
> + goto err_tx;
> +
> + rc = cc2520_write_txfifo(priv, skb->data, skb->len);
> + if (rc)
> + goto err_tx;
> +
> + rc = cc2520_get_status(priv, &status);
> + if (rc)
> + goto err_tx;
> +
> + if (status & CC2520_STATUS_TX_UNDERFLOW) {
> + dev_err(&priv->spi->dev, "cc2520 tx underflow exception\n");
> + goto err_tx;
> + }
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + BUG_ON(priv->is_tx);
> + priv->is_tx = 1;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + rc = cc2520_cmd_strobe(priv, CC2520_CMD_STXONCCA);
> + if (rc)
> + goto err;
> +
> + rc = wait_for_completion_interruptible(&priv->tx_complete);
> + if (rc < 0)
> + goto err;
mhhh, I never see a reinit_completion and ask myself how the driver can
ever work?
We should also use a wait_for_completion_timeout here, then you need to
handle the error handling with if (!rc).
> +
> + cc2520_cmd_strobe(priv, CC2520_CMD_SFLUSHTX);
> + cc2520_cmd_strobe(priv, CC2520_CMD_SRXON);
> +
> + return rc;
> +err:
> + spin_lock_irqsave(&priv->lock, flags);
> + priv->is_tx = 0;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +err_tx:
> + return rc;
> +}
> +
> +
> +static int cc2520_rx(struct cc2520_private *priv)
> +{
> + u8 len = 0, lqi = 0, bytes = 1;
> + struct sk_buff *skb;
> +
> + cc2520_read_rxfifo(priv, &len, &bytes, &lqi);
> +
> + skb = alloc_skb(len, GFP_KERNEL);
> + if (!skb)
> + return -ENOMEM;
> +
> + cc2520_read_rxfifo(priv, skb_put(skb, len), &len, &lqi);
> + if (len < 2) {
> + kfree_skb(skb);
> + return -EINVAL;
> + }
> +
> + skb_trim(skb, skb->len - 2);
> +
> + ieee802154_rx_irqsafe(priv->dev, skb, lqi);
> +
> + dev_vdbg(&priv->spi->dev, "RXFIFO: %x %x\n", len, lqi);
> +
> + return 0;
> +}
> +
> +static int
> +cc2520_ed(struct ieee802154_dev *dev, u8 *level)
> +{
> + struct cc2520_private *priv = dev->priv;
> + u8 status = 0xff;
> + u8 rssi;
> + int ret;
> +
> + ret = cc2520_read_register(priv , CC2520_RSSISTAT, &status);
> + if (ret)
> + return ret;
> +
> + if (status != RSSI_VALID) {
> + ret = -EINVAL;
> + return ret;
> + }
> +
> + ret = cc2520_read_register(priv , CC2520_RSSI, &rssi);
> + if (ret)
> + return ret;
> +
> + /* level = RSSI(rssi) - OFFSET [dBm] : offset is 76dBm*/
> + *level = rssi - RSSI_OFFSET;
> +
> + return ret;
> +}
> +
> +static int
> +cc2520_set_channel(struct ieee802154_dev *dev, int page, int channel)
> +{
> + struct cc2520_private *priv = dev->priv;
> + int ret;
> +
> + might_sleep();
> + dev_dbg(&priv->spi->dev, "trying to set channel\n");
> +
> + BUG_ON(page != 0);
> + BUG_ON(channel < CC2520_MINCHANNEL);
> + BUG_ON(channel > CC2520_MAXCHANNEL);
> +
> + ret = cc2520_write_register(priv, CC2520_FREQCTRL,
> + 11 + 5*(channel - 11));
> +
> + return ret;
> +}
> +
> +static struct ieee802154_ops cc2520_ops = {
> + .owner = THIS_MODULE,
> + .start = cc2520_start,
> + .stop = cc2520_stop,
> + .xmit = cc2520_tx,
> + .ed = cc2520_ed,
> + .set_channel = cc2520_set_channel,
> +};
> +
> +static int cc2520_register(struct cc2520_private *priv)
> +{
> + int ret = -ENOMEM;
> +
> + priv->dev = ieee802154_alloc_device(sizeof(*priv), &cc2520_ops);
> + if (!priv->dev)
> + goto err_ret;
> +
> + priv->dev->priv = priv;
> + priv->dev->parent = &priv->spi->dev;
> + priv->dev->extra_tx_headroom = 0;
> +
> + /* We do support only 2.4 Ghz */
> + priv->dev->phy->channels_supported[0] = 0x7FFF800;
> + priv->dev->flags = IEEE802154_HW_OMIT_CKSUM;
> +
> + dev_vdbg(&priv->spi->dev, "registered cc2520\n");
> + ret = ieee802154_register_device(priv->dev);
> + if (ret)
> + goto err_free_device;
> +
> + return 0;
> +
> +err_free_device:
> + ieee802154_free_device(priv->dev);
> +err_ret:
> + return ret;
> +}
> +
> +static void cc2520_fifop_irqwork(struct work_struct *work)
> +{
> + struct cc2520_private *priv
> + = container_of(work, struct cc2520_private, fifop_irqwork);
> +
> + dev_dbg(&priv->spi->dev, "fifop interrupt received\n");
> +
> + if (gpio_get_value(priv->fifo_pin))
> + cc2520_rx(priv);
> + else
> + dev_err(&priv->spi->dev, "rxfifo overflow\n");
> +
> + cc2520_cmd_strobe(priv, CC2520_CMD_SFLUSHRX);
> + cc2520_cmd_strobe(priv, CC2520_CMD_SFLUSHRX);
> +}
> +
> +static irqreturn_t cc2520_fifop_isr(int irq, void *data)
> +{
> + struct cc2520_private *priv = data;
> +
> + schedule_work(&priv->fifop_irqwork);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t cc2520_sfd_isr(int irq, void *data)
> +{
> + struct cc2520_private *priv = data;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + if (priv->is_tx) {
> + priv->is_tx = 0;
> + spin_unlock_irqrestore(&priv->lock, flags);
> + dev_dbg(&priv->spi->dev, "SFD for TX\n");
> + complete(&priv->tx_complete);
> + } else {
> + spin_unlock_irqrestore(&priv->lock, flags);
> + dev_dbg(&priv->spi->dev, "SFD for RX\n");
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int cc2520_hw_init(struct cc2520_private *priv)
> +{
> + u8 status = 0, state = 0xff;
> + int ret;
> + int timeout = 100;
> +
> + ret = cc2520_read_register(priv, CC2520_FSMSTAT1, &state);
> + if (ret)
> + goto err_ret;
> +
> + if (state != STATE_IDLE)
> + return -EINVAL;
> +
> + do {
> + ret = cc2520_get_status(priv, &status);
> + if (ret)
> + goto err_ret;
> +
> + if (timeout-- <= 0) {
> + dev_err(&priv->spi->dev, "oscillator start failed!\n");
> + return ret;
> + }
> + udelay(1);
> + } while (!(status & CC2520_STATUS_XOSC32M_STABLE));
> +
> + dev_vdbg(&priv->spi->dev, "oscillator successfully brought up\n");
> +
> + /*Registers default value: section 28.1 in Datasheet*/
> + ret = cc2520_write_register(priv, CC2520_TXPOWER, 0xF7);
> + if (ret)
> + goto err_ret;
> +
> + ret = cc2520_write_register(priv, CC2520_CCACTRL0, 0x1A);
> + if (ret)
> + goto err_ret;
> +
> + ret = cc2520_write_register(priv, CC2520_MDMCTRL0, 0x85);
> + if (ret)
> + goto err_ret;
> +
> + ret = cc2520_write_register(priv, CC2520_MDMCTRL1, 0x14);
> + if (ret)
> + goto err_ret;
> +
> + ret = cc2520_write_register(priv, CC2520_RXCTRL, 0x3f);
> + if (ret)
> + goto err_ret;
> +
> + ret = cc2520_write_register(priv, CC2520_FSCTRL, 0x5a);
> + if (ret)
> + goto err_ret;
> +
> + ret = cc2520_write_register(priv, CC2520_FSCAL1, 0x2b);
> + if (ret)
> + goto err_ret;
> +
> + ret = cc2520_write_register(priv, CC2520_AGCCTRL1, 0x11);
> + if (ret)
> + goto err_ret;
> +
> + ret = cc2520_write_register(priv, CC2520_ADCTEST0, 0x10);
> + if (ret)
> + goto err_ret;
> +
> + ret = cc2520_write_register(priv, CC2520_ADCTEST1, 0x0e);
> + if (ret)
> + goto err_ret;
> +
> + ret = cc2520_write_register(priv, CC2520_ADCTEST2, 0x03);
> + if (ret)
> + goto err_ret;
> +
> + ret = cc2520_write_register(priv, CC2520_FRMCTRL0, 0x60);
> + if (ret)
> + goto err_ret;
> +
> + ret = cc2520_write_register(priv, CC2520_FRMCTRL1, 0x03);
> + if (ret)
> + goto err_ret;
> +
> + ret = cc2520_write_register(priv, CC2520_FRMFILT0, 0x00);
> + if (ret)
> + goto err_ret;
> +
> + ret = cc2520_write_register(priv, CC2520_FIFOPCTRL, 127);
> + if (ret)
> + goto err_ret;
> +
> + return 0;
> +
> +err_ret:
> + return ret;
> +}
> +
> +static struct cc2520_platform_data *
> +cc2520_get_platform_data(struct spi_device *spi)
> +{
> + struct cc2520_platform_data *pdata;
> + struct device_node __maybe_unused *np = spi->dev.of_node;
> + struct cc2520_private *priv = spi_get_drvdata(spi);
> +
> + if (!IS_ENABLED(CONFIG_OF) || !np)
> + return spi->dev.platform_data;
> +
> + pdata = devm_kzalloc(&spi->dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + goto done;
> +
> + pdata->fifo = of_get_named_gpio(np, "fifo-gpio", 0);
> + priv->fifo_pin = pdata->fifo;
> +
> + pdata->fifop = of_get_named_gpio(np, "fifop-gpio", 0);
> +
> + pdata->sfd = of_get_named_gpio(np, "sfd-gpio", 0);
> + pdata->cca = of_get_named_gpio(np, "cca-gpio", 0);
> + pdata->vreg = of_get_named_gpio(np, "vreg-gpio", 0);
> + pdata->reset = of_get_named_gpio(np, "reset-gpio", 0);
> +
> + spi->dev.platform_data = pdata;
> +
> +done:
> + return pdata;
> +}
> +
> +static int cc2520_probe(struct spi_device *spi)
> +{
> + struct cc2520_private *priv;
> + struct pinctrl *pinctrl;
> + struct cc2520_platform_data *pdata;
> + struct device_node __maybe_unused *np = spi->dev.of_node;
> + int ret;
> +
> + priv = devm_kzalloc(&spi->dev,
> + sizeof(struct cc2520_private), GFP_KERNEL);
> + if (!priv) {
> + ret = -ENOMEM;
> + goto err_ret;
> + }
> +
> + spi_set_drvdata(spi, priv);
> +
> + pinctrl = devm_pinctrl_get_select_default(&spi->dev);
> + if (IS_ERR(pinctrl))
> + dev_warn(&spi->dev,
> + "pinctrl pins are not configured from the driver");
> +
> + pdata = cc2520_get_platform_data(spi);
> + if (!pdata) {
> + dev_err(&spi->dev, "no platform data\n");
> + return -EINVAL;
> + }
> +
> + priv->spi = spi;
> +
> + priv->buf = devm_kzalloc(&spi->dev,
> + SPI_COMMAND_BUFFER, GFP_KERNEL);
> + if (!priv->buf) {
> + ret = -ENOMEM;
> + goto err_ret;
> + }
> +
> + mutex_init(&priv->buffer_mutex);
> + INIT_WORK(&priv->fifop_irqwork, cc2520_fifop_irqwork);
> + spin_lock_init(&priv->lock);
> + init_completion(&priv->tx_complete);
> +
> + /* Request all the gpio's */
> + if (!gpio_is_valid(pdata->fifo)) {
> + dev_err(&spi->dev, "fifo gpio is not valid\n");
> + ret = -EINVAL;
> + goto err_hw_init;
> + } else {
> + ret = devm_gpio_request_one(&spi->dev, pdata->fifo,
> + GPIOF_IN, "fifo");
> + if (ret)
> + goto err_hw_init;
> + }
you can drop the else branch here...
> +
> + if (!gpio_is_valid(pdata->cca)) {
> + dev_err(&spi->dev, "cca gpio is not valid\n");
> + ret = -EINVAL;
> + goto err_hw_init;
> + } else {
> + ret = devm_gpio_request_one(&spi->dev, pdata->cca,
> + GPIOF_IN, "cca");
> + if (ret)
> + goto err_hw_init;
> + }
> +
Same here.
> + if (!gpio_is_valid(pdata->fifop)) {
> + dev_err(&spi->dev, "fifop gpio is not valid\n");
> + ret = -EINVAL;
> + goto err_hw_init;
> + } else {
> + ret = devm_gpio_request_one(&spi->dev, pdata->fifop,
> + GPIOF_IN, "fifop");
> + if (ret)
> + goto err_hw_init;
> + }
> +
Same here.
> + if (!gpio_is_valid(pdata->sfd)) {
> + dev_err(&spi->dev, "sfd gpio is not valid\n");
> + ret = -EINVAL;
> + goto err_hw_init;
> + } else {
> + ret = devm_gpio_request_one(&spi->dev, pdata->sfd,
> + GPIOF_IN, "sfd");
> + if (ret)
> + goto err_hw_init;
> + }
> +
Same here.
> + if (!gpio_is_valid(pdata->reset)) {
> + dev_err(&spi->dev, "reset gpio is not valid\n");
> + ret = -EINVAL;
> + goto err_hw_init;
> + } else {
> + ret = devm_gpio_request_one(&spi->dev, pdata->reset,
> + GPIOF_OUT_INIT_LOW, "reset");
> + if (ret)
> + goto err_hw_init;
> + }
> +
Same here.
> + if (!gpio_is_valid(pdata->vreg)) {
> + dev_err(&spi->dev, "vreg gpio is not valid\n");
> + ret = -EINVAL;
> + goto err_hw_init;
> + } else {
> + ret = devm_gpio_request_one(&spi->dev, pdata->vreg,
> + GPIOF_OUT_INIT_LOW, "vreg");
> + if (ret)
> + goto err_hw_init;
> + }
> +
Same here.
> + gpio_set_value(pdata->vreg, HIGH);
> + udelay(100);
> +
> + gpio_set_value(pdata->reset, HIGH);
> + udelay(200);
> +
> + ret = cc2520_hw_init(priv);
> + if (ret)
> + goto err_hw_init;
> +
> + /* Set up fifop interrupt */
> + ret = devm_request_irq(&spi->dev,
> + gpio_to_irq(pdata->fifop),
> + cc2520_fifop_isr,
> + IRQF_TRIGGER_RISING,
> + dev_name(&spi->dev),
> + priv);
> + if (ret) {
> + dev_err(&spi->dev, "could not get fifop irq\n");
> + goto err_hw_init;
> + }
> +
> + /* Set up sfd interrupt */
> + ret = devm_request_irq(&spi->dev,
> + gpio_to_irq(pdata->sfd),
> + cc2520_sfd_isr,
> + IRQF_TRIGGER_FALLING,
> + dev_name(&spi->dev),
> + priv);
> + if (ret) {
> + dev_err(&spi->dev, "could not get sfd irq\n");
> + goto err_hw_init;
> + }
> +
> + ret = cc2520_register(priv);
> + if (ret)
> + goto err_hw_init;
> +
> + return 0;
> +
> +err_hw_init:
> + mutex_destroy(&priv->buffer_mutex);
> + flush_work(&priv->fifop_irqwork);
> +
> +err_ret:
> + return ret;
> +}
> +
> +static int cc2520_remove(struct spi_device *spi)
> +{
> + struct cc2520_private *priv = spi_get_drvdata(spi);
> +
> + ieee802154_unregister_device(priv->dev);
> + ieee802154_free_device(priv->dev);
> +
> + mutex_destroy(&priv->buffer_mutex);
> + flush_work(&priv->fifop_irqwork);
> +
> + return 0;
> +}
> +
> +static const struct spi_device_id cc2520_ids[] = {
> + {"cc2520", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(spi, cc2520_ids);
> +
> +static const struct of_device_id cc2520_of_ids[] = {
> + {.compatible = "ti,cc2520", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, cc2520_of_ids);
> +
> +/*SPI driver structure*/
> +static struct spi_driver cc2520_driver = {
> + .driver = {
> + .name = "cc2520",
> + .bus = &spi_bus_type,
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(cc2520_of_ids),
> + },
> + .id_table = cc2520_ids,
> + .probe = cc2520_probe,
> + .remove = cc2520_remove,
> +};
> +module_spi_driver(cc2520_driver);
> +
> +MODULE_DESCRIPTION("CC2520 Transceiver Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/spi/cc2520.h b/include/linux/spi/cc2520.h
> new file mode 100644
> index 0000000..56ce81e
> --- /dev/null
> +++ b/include/linux/spi/cc2520.h
> @@ -0,0 +1,26 @@
> +/* Header file for cc2520 driver
> + *
> + * Copyright (C) 2014 Varka Bhadram <varkab@cdac.in>
> + * Md.Jamal Mohiuddin <mjmohiuddin@cdac.in>
> + * P Sowjanya <sowjanyap@cdac.in>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + */
> +
> +#ifndef __CC2520_H
> +#define __CC2520_H
> +
> +struct cc2520_platform_data {
> + int fifo;
> + int fifop;
> + int cca;
> + int sfd;
> + int reset;
> + int vreg;
> +};
> +
> +#endif
> --
> 1.7.9.5
>
>
> ------------------------------------------------------------------------------
> HPCC Systems Open Source Big Data Platform from LexisNexis Risk Solutions
> Find What Matters Most in Your Big Data with HPCC Systems
> Open Source. Fast. Scalable. Simple. Ideal for Dirty Data.
> Leverages Graph Analysis for Fast Processing & Easy Data Exploration
> http://p.sf.net/sfu/hpccsystems
> _______________________________________________
> Linux-zigbee-devel mailing list
> Linux-zigbee-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [Linux-zigbee-devel] [PATCH net-next v1 1/3] ieee802154: cc2520: adds driver for TI CC2520 radio
[not found] <1929819035.7546.1402996542810.JavaMail.open-xchange@webmail.cdac.in>
@ 2014-06-17 9:51 ` Alexander Aring
0 siblings, 0 replies; 2+ messages in thread
From: Alexander Aring @ 2014-06-17 9:51 UTC (permalink / raw)
To: Varka Bhadram
Cc: Varka Bhadram, netdev, linux-kernel, linux-zigbee-devel, davem
Hi,
On Tue, Jun 17, 2014 at 02:45:42PM +0530, Varka Bhadram wrote:
> On 06/17/2014 02:11 PM, Alexander Aring wrote:
>
> >
> > Hi Varka,
...
> > > +
> > > +/* Generic Functions */
> > > +static int
> > > +cc2520_cmd_strobe(struct cc2520_private *priv, u8 cmd)
> > > +{
> > > + int ret;
> > > + u8 status = 0xff;
> > > + struct spi_message msg;
> > > + struct spi_transfer xfer = {
> > > + .len = 0,
> > > + .tx_buf = priv->buf,
> > > + .rx_buf = priv->buf,
> > > + };
> > > +
> > > + spi_message_init(&msg);
> > > + spi_message_add_tail(&xfer, &msg);
> > > +
> > > + mutex_lock(&priv->buffer_mutex);
> > > + priv->buf[xfer.len++] = cmd;
> > > + dev_vdbg(&priv->spi->dev,
> > > + "command strobe buf[0] = %02x\n",
> > > + priv->buf[0]);
> > > +
> > > + ret = spi_sync(priv->spi, &msg);
> > > + if (!ret)
> > > + status = priv->buf[0];
> > > + dev_vdbg(&priv->spi->dev,
> > > + "buf[0] = %02x\n", priv->buf[0]);
> > > + mutex_unlock(&priv->buffer_mutex);
> > > +
> > > + return ret;
> > > +}
> > >
> > > >
> > What's about to drop the lowlevel spi calls and use spi helper functions?
> >
> >
> Many of the people are suggesting to use spi_sync() functions instead of spi
> helper functions.
>
The spi helper function do the same what do you there in several
functions in this driver. Look in 'drivers/spi/spi.c'. This would
decrease much the code count.
> >
> > > >
> > > +
> > > +static int
> > > +cc2520_get_status(struct cc2520_private *priv, u8 *status)
> > > +{
> > > + int ret;
> > > + struct spi_message msg;
> > > + struct spi_transfer xfer = {
> > > + .len = 0,
> > > + .tx_buf = priv->buf,
> > > + .rx_buf = priv->buf,
> > > + };
> > > +
> > > + spi_message_init(&msg);
> > > + spi_message_add_tail(&xfer, &msg);
> > > +
> > > + mutex_lock(&priv->buffer_mutex);
> > > + priv->buf[xfer.len++] = CC2520_CMD_SNOP;
> > > + dev_vdbg(&priv->spi->dev,
> > > + "get status command buf[0] = %02x\n", priv->buf[0]);
> > > +
> > > + ret = spi_sync(priv->spi, &msg);
> > > + if (!ret)
> > > + *status = priv->buf[0];
> > > + dev_vdbg(&priv->spi->dev,
> > > + "buf[0] = %02x\n", priv->buf[0]);
> > > + mutex_unlock(&priv->buffer_mutex);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int
> > > +cc2520_write_register(struct cc2520_private *priv, u8 reg, u8
> > > value)
> > > +{
> > > + int status;
> > > + struct spi_message msg;
> > > + struct spi_transfer xfer = {
> > > + .len = 0,
> > > + .tx_buf = priv->buf,
> > > + .rx_buf = priv->buf,
> > > + };
> > > +
> > > + spi_message_init(&msg);
> > > + spi_message_add_tail(&xfer, &msg);
> > > +
> > > + mutex_lock(&priv->buffer_mutex);
> > > +
> > > + if (reg <= CC2520_FREG_MASK) {
> > > + priv->buf[xfer.len++] = CC2520_CMD_REGISTER_WRITE | reg;
> > > + priv->buf[xfer.len++] = value;
> > > + } else {
> > > + priv->buf[xfer.len++] = CC2520_CMD_MEMORY_WRITE;
> > > + priv->buf[xfer.len++] = reg;
> > > + priv->buf[xfer.len++] = value;
> > > + }
> > > + status = spi_sync(priv->spi, &msg);
> > > + if (msg.status)
> > > + status = msg.status;
> > > +
> > > + mutex_unlock(&priv->buffer_mutex);
> > > +
> > > + return status;
> > > +}
> > > +
> > > +static int
> > > +cc2520_read_register(struct cc2520_private *priv, u8 reg, u8 *data)
> > > +{
> > > + int status;
> > > + struct spi_message msg;
> > > + struct spi_transfer xfer1 = {
> > > + .len = 0,
> > > + .tx_buf = priv->buf,
> > > + .rx_buf = priv->buf,
> > > + };
> > > +
> > > + struct spi_transfer xfer2 = {
> > > + .len = 1,
> > > + .rx_buf = data,
> > > + };
> > > +
> > > + spi_message_init(&msg);
> > > + spi_message_add_tail(&xfer1, &msg);
> > > + spi_message_add_tail(&xfer2, &msg);
> > > +
> > > + mutex_lock(&priv->buffer_mutex);
> > > + priv->buf[xfer1.len++] = CC2520_CMD_MEMORY_READ;
> > > + priv->buf[xfer1.len++] = reg;
> > > +
> > > + status = spi_sync(priv->spi, &msg);
> > > + dev_dbg(&priv->spi->dev,
> > > + "spi status = %d\n", status);
> > > + if (msg.status)
> > > + status = msg.status;
> > > +
> > > + mutex_unlock(&priv->buffer_mutex);
> > > +
> > > + return status;
> > > +}
> > > +
> > > +static int
> > > +cc2520_write_txfifo(struct cc2520_private *priv, u8 *data, u8 len)
> > > +{
> > > + int status;
> > > +
> > > + /*
> > > + *length byte must include FCS even
> > > + *if it is calculated in the hardware
> > > + */
> > > + int len_byte = len + 2;
> > > +
> > > + struct spi_message msg;
> > > +
> > > + struct spi_transfer xfer_head = {
> > > + .len = 0,
> > > + .tx_buf = priv->buf,
> > > + .rx_buf = priv->buf,
> > > + };
> > > + struct spi_transfer xfer_len = {
> > > + .len = 1,
> > > + .tx_buf = &len_byte,
> > > + };
> > > + struct spi_transfer xfer_buf = {
> > > + .len = len,
> > > + .tx_buf = data,
> > > + };
> > > +
> > > + spi_message_init(&msg);
> > > + spi_message_add_tail(&xfer_head, &msg);
> > > + spi_message_add_tail(&xfer_len, &msg);
> > > + spi_message_add_tail(&xfer_buf, &msg);
> > > +
> > > + mutex_lock(&priv->buffer_mutex);
> > > + priv->buf[xfer_head.len++] = CC2520_CMD_TXBUF;
> > > + dev_vdbg(&priv->spi->dev,
> > > + "TX_FIFO cmd buf[0] = %02x\n", priv->buf[0]);
> > > +
> > > + status = spi_sync(priv->spi, &msg);
> > > + dev_vdbg(&priv->spi->dev, "status = %d\n", status);
> > > + if (msg.status)
> > > + status = msg.status;
> > > + dev_vdbg(&priv->spi->dev, "status = %d\n", status);
> > > + dev_vdbg(&priv->spi->dev, "buf[0] = %02x\n", priv->buf[0]);
> > > + mutex_unlock(&priv->buffer_mutex);
> > > +
> > > + return status;
> > > +}
> > > +
> > > +static int
> > > +cc2520_read_rxfifo(struct cc2520_private *priv, u8 *data, u8 *len,
> > > u8 *lqi)
> > >
> > > >
> > why is len a pointer here? It's never changed in this function I would
> > prefer const u8 len.
> >
> Good catch. Its my mistake. Previously i updated the len pointer and i used in
> cc2520_rx().
> Thanx
>
> >
> >
> >
> > > >
> > > +{
> > > + int status;
> > > + struct spi_message msg;
> > > +
> > > + struct spi_transfer xfer_head = {
> > > + .len = 0,
> > > + .tx_buf = priv->buf,
> > > + .rx_buf = priv->buf,
> > > + };
> > > + struct spi_transfer xfer_buf = {
> > > + .len = *len,
> > > + .rx_buf = data,
> > > + };
> > > +
> > > + spi_message_init(&msg);
> > > + spi_message_add_tail(&xfer_head, &msg);
> > > + spi_message_add_tail(&xfer_buf, &msg);
> > > +
> > > + mutex_lock(&priv->buffer_mutex);
> > > + priv->buf[xfer_head.len++] = CC2520_CMD_RXBUF;
> > > +
> > > + dev_vdbg(&priv->spi->dev, "read rxfifo buf[0] = %02x\n",
> > > priv->buf[0]);
> > > + dev_vdbg(&priv->spi->dev, "buf[1] = %02x\n", priv->buf[1]);
> > > +
> > > + status = spi_sync(priv->spi, &msg);
> > > + dev_vdbg(&priv->spi->dev, "status = %d\n", status);
> > > + if (msg.status)
> > > + status = msg.status;
> > > + dev_vdbg(&priv->spi->dev, "status = %d\n", status);
> > > + dev_vdbg(&priv->spi->dev,
> > > + "return status buf[0] = %02x\n", priv->buf[0]);
> > > + dev_vdbg(&priv->spi->dev, "length buf[1] = %02x\n", priv->buf[1]);
> > > +
> > > + *lqi = data[priv->buf[1] - 1] & 0x7f;
> > > +
> > > + mutex_unlock(&priv->buffer_mutex);
> > > +
> > > + return status;
> > > +}
> > > +
> > > +static int cc2520_start(struct ieee802154_dev *dev)
> > > +{
> > > + return cc2520_cmd_strobe(dev->priv, CC2520_CMD_SRXON);
> > > +}
> > > +
> > > +static void cc2520_stop(struct ieee802154_dev *dev)
> > > +{
> > > + cc2520_cmd_strobe(dev->priv, CC2520_CMD_SRFOFF);
> > > +}
> > > +
> > > +static int
> > > +cc2520_tx(struct ieee802154_dev *dev, struct sk_buff *skb)
> > > +{
> > > + struct cc2520_private *priv = dev->priv;
> > > + unsigned long flags;
> > > + int rc;
> > > + u8 status = 0;
> > > +
> > > + rc = cc2520_cmd_strobe(priv, CC2520_CMD_SFLUSHTX);
> > > + if (rc)
> > > + goto err_tx;
> > > +
> > > + rc = cc2520_write_txfifo(priv, skb->data, skb->len);
> > > + if (rc)
> > > + goto err_tx;
> > > +
> > > + rc = cc2520_get_status(priv, &status);
> > > + if (rc)
> > > + goto err_tx;
> > > +
> > > + if (status & CC2520_STATUS_TX_UNDERFLOW) {
> > > + dev_err(&priv->spi->dev, "cc2520 tx underflow exception\n");
> > > + goto err_tx;
> > > + }
> > > +
> > > + spin_lock_irqsave(&priv->lock, flags);
> > > + BUG_ON(priv->is_tx);
> > > + priv->is_tx = 1;
> > > + spin_unlock_irqrestore(&priv->lock, flags);
> > > +
> > > + rc = cc2520_cmd_strobe(priv, CC2520_CMD_STXONCCA);
> > > + if (rc)
> > > + goto err;
> > > +
> > > + rc = wait_for_completion_interruptible(&priv->tx_complete);
> > > + if (rc < 0)
> > > + goto err;
> > >
> > > >
> > mhhh, I never see a reinit_completion and ask myself how the driver can
> > ever work?
> >
> > We should also use a wait_for_completion_timeout here, then you need to
> > handle the error handling with if (!rc).
> >
> I initialized the work completion in the cc2520_probe():
> init_completion(&priv->tx_complete);
>
Ah, we only need a reinit_completion after a complete_all. Ok, sorry :-)
> It will wait until the SFD interrupt to raise and get the completion signal from
> sfd_isr() by:
> complete(&priv->tx_complete)
>
> wait_for_completion_interruptible() API will put the thread in interruptible
> state.
>
Yes, you are right but there is also a
wait_for_completion_interruptible_timeout. I mean for the case if you
waiting forever and no irq hits.
> >
> >
> >
...
> > > +
> > > + mutex_init(&priv->buffer_mutex);
> > > + INIT_WORK(&priv->fifop_irqwork, cc2520_fifop_irqwork);
> > > + spin_lock_init(&priv->lock);
> > > + init_completion(&priv->tx_complete);
> > > +
> > > + /* Request all the gpio's */
> > > + if (!gpio_is_valid(pdata->fifo)) {
> > > + dev_err(&spi->dev, "fifo gpio is not valid\n");
> > > + ret = -EINVAL;
> > > + goto err_hw_init;
> > > + } else {
> > > + ret = devm_gpio_request_one(&spi->dev, pdata->fifo,
> > > + GPIOF_IN, "fifo");
> > > + if (ret)
> > > + goto err_hw_init;
> > > + }
> > >
> > > >
> > you can drop the else branch here...
> >
>
> gpio_is_valid() will only check whether the GPIO PIN number is within the range
> of GPIO numbers or not.
> devm_gpio_request_one() request for the GPIO number. If other driver try to
> requst for the same number
> GPIO/Pinctrl subsytems through an error saying that 'The GPIO number is already
> used by someone else'
>
>
I meant something like this:
if (!gpio_is_valid(pdata->fifo)) {
dev_err(&spi->dev, "fifo gpio is not valid\n");
ret = -EINVAL;
goto err_hw_init;
}
ret = devm_gpio_request_one(&spi->dev, pdata->fifo,
GPIOF_IN, "fifo");
if (ret)
goto err_hw_init;
...
- Alex
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-06-17 9:51 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1929819035.7546.1402996542810.JavaMail.open-xchange@webmail.cdac.in>
2014-06-17 9:51 ` [Linux-zigbee-devel] [PATCH net-next v1 1/3] ieee802154: cc2520: adds driver for TI CC2520 radio Alexander Aring
2014-06-17 7:14 [PATCH net-next v1 0/3] Driver " Varka Bhadram
2014-06-17 7:14 ` [PATCH net-next v1 1/3] ieee802154: cc2520: adds driver " Varka Bhadram
2014-06-17 8:41 ` [Linux-zigbee-devel] " Alexander Aring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox