Hi Varka,
On Mon, Jun 16, 2014 at 10:21:56AM +0530, Varka Bhadram wrote:
>
Maybe some more information about this chip in the commit msg?
> Signed-off-by: Varka Bhadram <var...@cdac.in>
> ---
> drivers/net/ieee802154/cc2520.c | 805
> +++++++++++++++++++++++++++++++++++++++
> include/linux/spi/cc2520.h | 176 +++++++++
> 2 files changed, 981 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..e8b5993
> --- /dev/null
> +++ b/drivers/net/ieee802154/cc2520.c
> @@ -0,0 +1,805 @@
> +/* Driver for TI CC2520 802.15.4 Wireless-PAN Networking controller
> + *
> + * Copyright (C) 2014 Varka Bhadram <var...@cdac.in>
> + * Md.Jamal Mohiuddin <mjmohiud...@cdac.in>
> + * P Sowjanya <sowjan...@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
> +
> +/*Driver private information */
> +struct cc2520_private {
> + struct spi_device *spi;
> +
> + struct ieee802154_dev *dev;
> +
> + u8 *buf;
> + struct mutex buffer_mutex;
> +
> + unsigned is_tx:1;
> + int fifo_irq;
> +
> + int fifo_pin;
> + int fifop_pin;
> +
> + struct work_struct fifop_irqwork;
> +
> + spinlock_t lock;
> +
> + struct completion tx_complete;
> +};
> +
> +/*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);
I see, you maybe looked at others drivers like at86rf230 and see what
they do there. I really don't like the lowlevel spi calls in the others
drivers. There are spi helper functions like 'spi_write_then_read' look
in drivers/spi/spi.c for the helper functions. Then you don't need the
buffer_mutex also.
> +
> + 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;
> +}
> +
....
> +
> +static void cc2520_unregister(struct cc2520_private *priv)
> +{
> + ieee802154_unregister_device(priv->dev);
> + ieee802154_free_device(priv->dev);
> +}
Only used in remove callback of module. It's small enough to do this in
the remove callback.
> +
> +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;
> +
> + spin_lock(&priv->lock);
You need to use here spin_lock_irqsave and spin_unlock_irqrestore here.
Please verify you locking with PROVE_LOCKING enabled in your kernel.
In your xmit callback you use a irqsave spinlocks there. You need always
save to the "strongest" context which is a interrupt in your case.
> + if (priv->is_tx) {
> + dev_dbg(&priv->spi->dev, "SFD for TX\n");
> + priv->is_tx = 0;
> + complete(&priv->tx_complete);
> + } else
> + dev_dbg(&priv->spi->dev, "SFD for RX\n");
make brackets in the else branch if you have brackets in the "if" branch.
You don't need to lock all the things here I think:
--snip
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");
}
--snap
> + spin_unlock(&priv->lock);
> +
> + 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) {
> + ret = -EINVAL;
> + return ret;
return -EINVAL? saves the brackets ;)
> + }
> +
> + 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);
> + priv->fifop_pin = pdata->fifop;
> +
> + 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;
> +}
> +
> +/*Driver probe function*/
> +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 = kzalloc(sizeof(struct cc2520_private), GFP_KERNEL);
> + if (!priv) {
> + ret = -ENOMEM;
> + goto err_free_local;
> + }
why not devm_ calls?
> +
> + 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;
memory leak of priv here.
> + }
> +
> + mutex_init(&priv->buffer_mutex);
> + INIT_WORK(&priv->fifop_irqwork, cc2520_fifop_irqwork);
I really don't like also the workqueue here. The at86rf230 use also a
workqueue but the mrf24j40 uses 'devm_request_threaded_irq'. A threaded
irq can also handle sync spi calls.
> + spin_lock_init(&priv->lock);
> + init_completion(&priv->tx_complete);
> +
> + priv->spi = spi;
> +
> + priv->buf = kzalloc(SPI_COMMAND_BUFFER, GFP_KERNEL);
> + if (!priv->buf) {
> + ret = -ENOMEM;
> + goto err_free_buf;
> + }
> +
> + /*Request all the gpio's*/
> + if (gpio_is_valid(pdata->fifo)) {
> + ret = devm_gpio_request_one(&spi->dev, pdata->fifo,
> + GPIOF_IN, "fifo");
> + if (ret)
> + goto err_hw_init;
> + }
> +
> + if (gpio_is_valid(pdata->cca)) {
> + ret = devm_gpio_request_one(&spi->dev, pdata->cca,
> + GPIOF_IN, "cca");
> + if (ret)
> + goto err_hw_init;
> + }
> +
> + if (gpio_is_valid(pdata->fifop)) {
> + ret = devm_gpio_request_one(&spi->dev, pdata->fifop,
> + GPIOF_IN, "fifop");
> + if (ret)
> + goto err_hw_init;
> + }
> +
> + if (gpio_is_valid(pdata->sfd)) {
> + ret = devm_gpio_request_one(&spi->dev, pdata->sfd,
> + GPIOF_IN, "sfd");
> + if (ret)
> + goto err_hw_init;
> + }
> +
> + if (gpio_is_valid(pdata->reset)) {
> + ret = devm_gpio_request_one(&spi->dev, pdata->reset,
> + GPIOF_OUT_INIT_LOW, "reset");
> + if (ret)
> + goto err_hw_init;
> + }
> +
> + if (gpio_is_valid(pdata->vreg)) {
> + ret = devm_gpio_request_one(&spi->dev, pdata->vreg,
> + GPIOF_OUT_INIT_LOW, "vreg");
> + if (ret)
> + goto err_hw_init;
> + }
You should check on the not optional pins if you can request them. You
use in the above code the pins vreg, reset, fifo_irq, sfd. And this
failed if the gpio's are not valid.
means:
if (!gpio_is_valid(pdata->vreg)) {
dev_err(....)
ret = -EINVAL;
goto ...;
}
on all pins which are needed to use your chip.
> +
> + 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 */
> + priv->fifo_irq = gpio_to_irq(pdata->fifop);
why is fifo_irq in your "private data"? This is only used in this function.
> + ret = devm_request_irq(&spi->dev,
> + priv->fifo_irq,
> + 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_free_buf:
> + kfree(priv->buf);
> +
> +err_free_local:
> + kfree(priv);
> +
> + return ret;
> +}
> +
> +/*Driver remove function*/
> +static int cc2520_remove(struct spi_device *spi)
> +{
> + struct cc2520_private *priv = spi_get_drvdata(spi);
> +
> + cc2520_unregister(priv);
> +
> + mutex_destroy(&priv->buffer_mutex);
> + flush_work(&priv->fifop_irqwork);
> +
> + kfree(priv->buf);
> + kfree(priv);
> +
> + return 0;
> +}
> +
> +static const struct spi_device_id cc2520_ids[] = {
> + {"cc2520", 0},
You don't need to set the driver_data to 0. Simple:
{ "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..68a2c88
> --- /dev/null
> +++ b/include/linux/spi/cc2520.h
In this location of header files are platform_data structs only. I think
you should leave the cc2520_platform_data in this file and move the rest
of declaration to you cc2520.c file.
> @@ -0,0 +1,176 @@
> +#ifndef __CC2520_H
> +#define __CC2520_H
> +
> +#define RSSI_VALID 0
> +#define RSSI_OFFSET 78
> +#define CC2520_FREG_MASK 0x3F
> +
> +struct cc2520_platform_data {
> + int fifo;
> + int fifop;
> + int cca;
> + int sfd;
> + int reset;
> + int vreg;
> +};
> +
- Alex
------------------------------------------------------------------------------
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
