On June 17, 2014 at 3:21 PM Alexander Aring <alex.ar...@gmail.com> wrote:
> 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.
>
In this case SFD irq should come while Tx and Rx.
After completion of the Frame Tx the cc2520 will make the SFD signal is low.
please see: 20.3.1 & 20.4.1 sections in
http://www.ti.com/lit/ds/symlink/cc2520.pdf
<http://www.ti.com/lit/ds/symlink/cc2520.pdf>
> > >
> > >
> > >
> ...
> > > > +
> > > > + 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;
I didn't find any difference but it looks good.
>
> ...
>
> - Alex
>
-Varka Bhadram
-------------------------------------------------------------------------------------------------------------------------------
[ C-DAC is on Social-Media too. Kindly follow us at:
Facebook: https://www.facebook.com/CDACINDIA & Twitter: @cdacindia ]
This e-mail is for the sole use of the intended recipient(s) and may
contain confidential and privileged information. If you are not the
intended recipient, please contact the sender by reply e-mail and destroy
all copies and the original message. Any unauthorized review, use,
disclosure, dissemination, forwarding, printing or copying of this email
is strictly prohibited and appropriate legal action will be taken.
-------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------
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
