Hi Mark,
Code looks quite nice, especially since this looks like a fairly complex
driver. And the initial coments are quite useful, I appreciate that.
I have a few comments/questions though:
> + * been received and there isn't an error). The delay is 3 ms since delays
> + * over 2 ms have been observed during testing.
Would you say this timeout depend on the SPI bus bandwidth ?
> +#define TRF7970A_QUIRK_IRQ_STATUS_READ_ERRATA BIT(0)
Are there actually any TRF970 devices that do not need this quirk ?
> +static void trf7970a_drain_fifo(struct trf7970a *trf, u8 status)
> +{
> + struct sk_buff *skb = trf->rx_skb;
> + int ret;
> + u8 fifo_bytes;
> +
> + if (status & TRF7970A_IRQ_STATUS_ERROR) {
> + trf7970a_abort_and_send_err(trf, -EIO);
> + return;
> + }
> +
> + ret = trf7970a_read(trf, TRF7970A_FIFO_STATUS, &fifo_bytes);
> + if (ret) {
> + trf7970a_abort_and_send_err(trf, ret);
> + return;
> + }
> +
> + dev_dbg(trf->dev, "fifo_bytes: 0x%x\n", fifo_bytes);
> +
> + if (!fifo_bytes)
> + goto no_rx_data;
> +
> + if (fifo_bytes & TRF7970A_FIFO_STATUS_OVERFLOW) {
> + dev_err(trf->dev, "%s - fifo overflow: 0x%x\n", __func__,
> + fifo_bytes);
> + trf7970a_abort_and_send_err(trf, -EIO);
> + return;
> + }
> +
> + if (fifo_bytes > skb_tailroom(skb)) {
> + skb = skb_copy_expand(skb, skb_headroom(skb),
> + max_t(int, fifo_bytes,
> + TRF7970A_RX_SKB_ALLOC_SIZE),
> + GFP_KERNEL);
So there could be more pending bytes in the FIFO than you can accomodate
in your rx_skb ? Could we avoid that by allocating rx_skb to match the
FIFO size ?
> +static void trf7970a_timeout_work_handler(struct work_struct *work)
> +{
> + struct trf7970a *trf = container_of(work, struct trf7970a,
> + timeout_work.work);
> +
> + dev_dbg(trf->dev, "TIMEOUT - state: %d, ignore_timeout: %d\n",
> + trf->state, trf->ignore_timeout);
> +
> + mutex_lock(&trf->lock);
> +
> + if (trf->ignore_timeout)
> + trf->ignore_timeout = false;
> + else if (trf->state == TRF7970A_ST_WAIT_FOR_RX_DATA_CONT)
> + trf7970a_send_upstream(trf); /* No more rx data so send up */
> + else
> + trf7970a_abort_and_send_err(trf, -ETIMEDOUT);
> +
> + mutex_unlock(&trf->lock);
> +}
> +
> +/* ----------------------------------------------------------------- */
Nitpick: I suppose you're separating the internal logic from the digital
ops here ? Please add one comment line for that.
> +static int trf7970a_in_send_cmd(struct nfc_digital_dev *ddev,
> + struct sk_buff *skb, u16 timeout,
> + nfc_digital_cmd_complete_t cb, void *arg)
> +{
> + struct trf7970a *trf = nfc_digital_get_drvdata(ddev);
> + char *prefix;
> + unsigned int len;
> + int ret;
> +
> + dev_dbg(trf->dev, "New request - state: %d, timeout: %d ms, len: %d\n",
> + trf->state, timeout, skb->len);
> +
> + if (skb->len > TRF7970A_TX_MAX)
> + return -EINVAL;
> +
> + mutex_lock(&trf->lock);
> +
> + if ((trf->state != TRF7970A_ST_IDLE) &&
> + (trf->state != TRF7970A_ST_IDLE_RX_BLOCKED)) {
> + dev_err(trf->dev, "%s - Bogus state: %d\n", __func__,
> + trf->state);
> + ret = -EIO;
> + goto out_err;
> + }
> +
> + trf->rx_skb = nfc_alloc_recv_skb(TRF7970A_RX_SKB_ALLOC_SIZE,
> + GFP_KERNEL);
> + if (!trf->rx_skb) {
> + dev_dbg(trf->dev, "Can't alloc rx_skb\n");
> + ret = -ENOMEM;
> + goto out_err;
> + }
> +
> + if (trf->state == TRF7970A_ST_IDLE_RX_BLOCKED) {
> + ret = trf7970a_cmd(trf, TRF7970A_CMD_ENABLE_RX);
> + if (ret)
> + goto out_err;
> +
> + trf->state = TRF7970A_ST_IDLE;
> + }
> +
> + ret = trf7970a_per_cmd_config(trf, skb);
> + if (ret)
> + goto out_err;
> +
> + trf->ddev = ddev;
> + trf->tx_skb = skb;
As you're going to carry this guy around and may need it from e.g. your
threaded interrupt handler, shouldn't you take a reference (skb_get) on it ?
I'm concerned by the fact that you could see your tx_skb disappear from
abort_cmd and get an IRQ before your state is set to IDLE.
Hmm, I guess that's protected by the mutex and so when you get an abort
from the digital stack you reset the state to IDLE and no one should try
to touch tx_skb after you release the mutex. Is that what you had in
mind ?
> +static int trf7970a_probe(struct spi_device *spi)
> +{
> + struct device_node *np = spi->dev.of_node;
> + const struct spi_device_id *id = spi_get_device_id(spi);
> + struct trf7970a *trf;
> + int ret;
> +
> + if (!np) {
> + dev_err(&spi->dev, "No Device Tree entry\n");
> + return -EINVAL;
> + }
> +
> + trf = devm_kzalloc(&spi->dev, sizeof(*trf), GFP_KERNEL);
> + if (!trf)
> + return -ENOMEM;
> +
> + trf->state = TRF7970A_ST_OFF;
> + trf->dev = &spi->dev;
> + trf->spi = spi;
> + trf->quirks = id->driver_data;
> + trf->initialized = false;
> +
> + spi->mode = SPI_MODE_1;
> + spi->bits_per_word = 8;
> +
> + /* Get the optional Slave Select GPIO used for SPI with SS mode */
> + trf->ss_gpio = of_get_named_gpio(np, "ti,ss-gpio", 0);
> + if (trf->ss_gpio >= 0) {
> + ret = devm_gpio_request_one(trf->dev, trf->ss_gpio,
> + GPIOF_DIR_OUT | GPIOF_INIT_LOW, "SS");
> + if (ret) {
> + dev_err(trf->dev, "Can't request SS GPIO: %d\n", ret);
> + return ret;
> + }
> + } else {
> + dev_info(trf->dev, "Using SPI without SS mode\n");
> + }
> +
> + /* There are two enable pins - both must be present */
> + trf->en_gpio = of_get_named_gpio(np, "ti,enable-gpios", 0);
> + if (trf->en_gpio < 0) {
> + dev_err(trf->dev, "No EN GPIO property\n");
> + return ret;
You probably want to return trf->en_gpio or otherwise the devm code
won't release your managed resources.
> + }
> +
> + ret = devm_gpio_request_one(trf->dev, trf->en_gpio,
> + GPIOF_DIR_OUT | GPIOF_INIT_LOW, "EN");
> + if (ret) {
> + dev_err(trf->dev, "Can't request EN GPIO: %d\n", ret);
> + return ret;
> + }
> +
> + trf->en2_gpio = of_get_named_gpio(np, "ti,enable-gpios", 1);
> + if (trf->en2_gpio < 0) {
> + dev_err(trf->dev, "No EN2 GPIO property\n");
> + return ret;
Ditto.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
