Hello Samuel,
This mail is in reference of your below feedback.
>Do you really need a specific IRQ handler for that ? Can't you do it from the
>threaded interrupt context ?
After some experiments at our end, we found that we are not able to remove small ISR (hardirq context) from driver code.
Reason : There are few commands of our NFC transceiver (the ones that do not interact or data exchange with the tag), we want to complete synchronously. This is also in accordance with the overlying digital framework expectations (for ex. implementation of in_configure_hw() op). For that we use the completion structure of Linux Kernel (as suggested by you) and waiting for the completion using wait_for_completion_timeout() in the requesting thread. The completion is done from the corresponding interrupt handler.
If we use threaded interrupt handler for the same, it is introducing un-stability in the system. Depending on when threaded ISR is get scheduled by the scheduler, sometimes completion take less time and some time it takes long time to complete. In this situation it is becoming difficult to provide a guaranteed (and hard coded) value of timeout in which completion can be guaranteed to the requesting thread. Other way could be to wait for completion without timeout but in that case we would not be able to catch hardware errors (real timeout happens due to faulty hardware), so there is risk of hang in the system.
Thus we will be using hardirq context to do the completion to have a stable and more deterministic behavior as code in hardirq context guaranteed to run sooner than in thread context. In this way we can set a fixed (upper bound) timeout value in wait_for_completion_timeout() in the requesting thread. In that way if timeout happens we are confident that it is due to hardware failure !!
Note that the rest of the commands response from NFC transceiver will be handled by the threaded ISR. This include all the exchanges of frames between the tag and the transceiver. So bulk processing (and hence no time critical or synchronous completion requirement) will be part of the threaded ISR. In such cases the hardirq context will merely wake-up the ISR thread and exit.
Rest of your feedbacks are accommodated and I am going to release new version(v2) of driver accordingly.
Thanks & Regards,
Shikha
>-----Original Message-----
>From: Shikha SINGH
>Sent: Monday, October 26, 2015 11:28 AM
>To: 'Samuel Ortiz'
>Cc: aloisio.almeida@xxxxxxxxxxxxx; lauro.venancio@xxxxxxxxxxxxx; linux-
>wireless@xxxxxxxxxxxxxxx; linux-nfc@xxxxxxxxxxxx; Raunaque Mujeeb QUAISER;
>Manoj KUMAR; Sylvain FIDELIS; Patrick SOHN
>Subject: RE: [[linux-nfc] PATCH v1.0 2/3] driver: nfc: st95hf: ST NFC Transceiver
>support
>
>
>Hi Samuel,
> Thanks for your review.
>
>
>On Sat, Sep 12, 2015 at 03:21:34AM -0400, Shikha Singh wrote:
>> diff --git a/drivers/nfc/st95hf/Makefile b/drivers/nfc/st95hf/Makefile
>> new file mode 100644 index 0000000..2d8f8f3
>> --- /dev/null
>> +++ b/drivers/nfc/st95hf/Makefile
>> @@ -0,0 +1,6 @@
>> +#
>> +# Makefile for STMicroelectronics NFC transceiver ST95HF # #
>> +
>> +obj-$(CONFIG_NFC_ST95HF) += st_transceiver.o
>We should be more specific and call it st_nfc_transceiver for example.
>
>Ok.
>
>> +/* Function to send user provided buffer to ST95HF through SPI */ int
>> +spi_send_to_st95hf(struct spi_context *spicontext,
>> + unsigned char *buffertx, int datalen,
>> + enum req_type reqtype)
>To be consistent with the rest of your API, this one should be called
>st95hf_spi_send(). It obviously implies that it is sending packets to the
>transceiver.
>
>Ok.
>
>> +{
>> + struct spi_message m;
>> + int result = 0;
>> + struct spi_device *spidev = spicontext->spidev;
>> + struct spi_transfer tx_transfer = {
>> + .rx_buf = NULL,
>> + .tx_buf = buffertx,
>> + .len = datalen,
>> + .cs_change = 0,
>> + .bits_per_word = 0,
>> + .delay_usecs = 0,
>> + .speed_hz = 0,
>> + };
>You don't need to explicitely set those fields to NULL or 0, this one could be
>simplified to:
>
>struct spi_transfer t = { .tx_buf = buffertx, .len = datalen, };
>
>Ok.
>
>> + spicontext->reply_from_st95 = 0;
>> +
>> + if (reqtype == SYNC)
>> + spicontext->req_issync = true;
>> + else
>> + spicontext->req_issync = false;
>> +
>> + spi_message_init(&m);
>> + spi_message_add_tail(&tx_transfer, &m);
>> +
>> + result = spi_sync(spidev, &m);
>> + if (result) {
>> + dev_err(&spidev->dev,
>> + "error: sending cmd to st95hf using SPI\n");
>> + return result;
>> + }
>> +
>> + if (reqtype == ASYNC) { /* return for asynchronous or no-wait case */
>> + return 0;
>> + }
>> +
>> + do {
>> + result = wait_event_interruptible_timeout(
>> + spicontext->st95wait_queue,
>> + spicontext->reply_from_st95 == 1,
>> + 1000);
>> + } while (result < 0);
>If result is < 0 it means your transfer got interrupted by a signal, there is no
>point in looping around result. You may just want to return result in that case.
>
>Agree. I will correct in next version.
>
>A couple more comments here:
>- A completion is probably enough for what you're trying to achieve.
>
>Yes. I will make changes accordingly.
>
>- This is racy: If you have 2 threads calling spi_send_to_st95hf() at
> the same time they may end up waiting on the waitqueue at the same
> time as well. The first interrupt that comes will wake both at the
> same time although it should not.
> In order to avoid that race, you need to synchronize the calls to
> spi_sync with e.g. a mutex so that you don't issue an spi_sync while
> someone is already on the waitqueue or sleeping for completion.
>
>Ok. Although at present there is no situation where spi_send_to_st95hf() can be
>called simultaneously but I will accommodate your suggestion to have a safe
>code.
>
>> +
>> + if (result == 0) {
>> + dev_err(&spidev->dev, "error: response not ready timeout\n");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + if (result > 0)
>> + result = 0;
>> +
>> + return result;
>> +}
>> +EXPORT_SYMBOL_GPL(spi_send_to_st95hf);
>> +
>> +/* Function to Receive command Response */ int
>> +spi_receive_response(struct spi_context *spicontext,
>> + unsigned char *receivebuff,
>> + int *len)
>2 things:
>
>- Let's call that one st95hf_spi_recv_response()
>
>Ok.
>
>- It typically should return the number of read bytes, instead of
> passing a len pointer to the argument list.
>
>Ok.
>
>
>> +{
>> + struct spi_transfer tx_takeresponse1;
>> + struct spi_transfer tx_takeresponse2;
>> + struct spi_transfer tx_takedata;
>> + struct spi_message m;
>> + unsigned char readdata_cmd = ST95HF_COMMAND_RECEIVE;
>> + int ret = 0;
>> +
>> + struct spi_device *spidev = spicontext->spidev;
>> +
>> + *len = 0;
>> +
>> + memset(&tx_takeresponse1, 0x0, sizeof(struct spi_transfer));
>> + memset(&tx_takeresponse2, 0x0, sizeof(struct spi_transfer));
>> + memset(&tx_takedata, 0x0, sizeof(struct spi_transfer));
>> +
>> + tx_takeresponse1.tx_buf = &readdata_cmd;
>> + tx_takeresponse1.len = 1;
>> +
>> + tx_takeresponse2.rx_buf = receivebuff;
>> + /* 1 byte Response code + 1 byte length of data */
>> + tx_takeresponse2.len = 2;
>> + /* Dont allow to make chipselect high */
>> + tx_takeresponse2.cs_change = 1;
>> +
>> + spi_message_init(&m);
>> + spi_message_add_tail(&tx_takeresponse1, &m);
>> + spi_message_add_tail(&tx_takeresponse2, &m);
>
>So this whole block of code could be simplified to:
>
>struct spi_transfer t[2] = { { .tx_buf = &readdata_cmd, .len = 1, },
> { .rx_buf = receive_buff, .len = 2 , .cs_change = 1, }, };
>
>spi_message_init(&m);
>spi_message_add_tail(&t[0], &m);
>spi_message_add_tail(&t[1], &m);
>
>Ok.
>
>> + ret = spi_sync(spidev, &m);
>> + if (ret)
>> + return ret;
>> +
>> + /* 2 bytes are already read */
>> + *len = 2;
>> +
>> + /*support of long frame*/
>Nitpick: Please add a space after /* and before */
>
>Ok.
>
>> + if (receivebuff[0] & 0x60)
>> + *len += (((receivebuff[0] & 0x60) >> 5) << 8) | receivebuff[1];
>> + else
>> + *len += receivebuff[1];
>> +
>> + /* Now make a transfer to take only relevant data */
>> + tx_takedata.rx_buf = &receivebuff[2];
>> + tx_takedata.len = (*len) - 2;
>> + tx_takedata.cs_change = 0;
>> +
>> + spi_message_init(&m);
>> + spi_message_add_tail(&tx_takedata, &m);
>> +
>> + return spi_sync(spidev, &m);
>> +}
>> +EXPORT_SYMBOL_GPL(spi_receive_response);
>> +
>> +int spi_receive_echo_response(struct spi_context *spicontext,
>> + unsigned char *receivebuff)
>Similar comment, please call it st95hf_recv_echo().
>
>Ok.
>
>> +{
>> + struct spi_transfer tx_takeresponse1;
>> + struct spi_transfer tx_takedata;
>> + struct spi_message m;
>> + unsigned char readdata_cmd = ST95HF_COMMAND_RECEIVE;
>> + struct spi_device *spidev = spicontext->spidev;
>> +
>> + memset(&tx_takeresponse1, 0x0, sizeof(struct spi_transfer));
>> + memset(&tx_takedata, 0x0, sizeof(struct spi_transfer));
>> +
>> + tx_takeresponse1.tx_buf = &readdata_cmd;
>> + tx_takeresponse1.len = 1;
>> + tx_takeresponse1.rx_buf = NULL;
>> +
>> + tx_takedata.rx_buf = receivebuff;
>> + tx_takedata.tx_buf = NULL;
>> + tx_takedata.len = 1;
>> +
>> + spi_message_init(&m);
>> + spi_message_add_tail(&tx_takeresponse1, &m);
>> + spi_message_add_tail(&tx_takedata, &m);
>Similar comment as well, this can be simplified as described above.
>
>Ok.
>
>> + return spi_sync(spidev, &m);
>> +}
>> +EXPORT_SYMBOL_GPL(spi_receive_echo_response);
>> diff --git a/drivers/nfc/st95hf/spi.h b/drivers/nfc/st95hf/spi.h new
>> file mode 100644 index 0000000..aa3eea0d
>> --- /dev/null
>> +++ b/drivers/nfc/st95hf/spi.h
>> @@ -0,0 +1,45 @@
>> + /*
>> + *
>> + --------------------------------------------------------------------
>> + ---------
>> + * drivers/nfc/st95hf/spi.h functions declarations for SPI
>> + communication
>> + *
>> + --------------------------------------------------------------------
>> + ---------
>> + *
>> + * Copyright (C) 2015 STMicroelectronics – All Rights Reserved
>> + * Author: Shikha Singh <shikha.singh@xxxxxx>
>> + *
>> + * May be copied or modified under the terms of the GNU General
>> + Public
>> + * License Version 2.0 only. See linux/COPYING for more information.
>> + *
>> + --------------------------------------------------------------------
>> + -------
>> + */
>> +
>> +#ifndef __LINUX_ST95HF_SPI_H
>> +#define __LINUX_ST95HF_SPI_H
>> +
>> +#include <linux/spi/spi.h>
>> +
>> +struct spi_context {
>struct st95hf_spi_context would be more apropriate.
>
>Ok.
>
>> + int reply_from_st95;
>> + wait_queue_head_t st95wait_queue;
>I think those 2 can easily be replaced by a simple completion structure.
>
>Agree. I will replace it with completion structure of kernel.
>
>> +static irqreturn_t irq_handler(int irq, void *st95hfcontext) {
>> + struct st95hf_context *stcontext =
>> + (struct st95hf_context *)st95hfcontext;
>> +
>> + if (stcontext->spicontext.req_issync) {
>> + stcontext->spicontext.reply_from_st95 = 1;
>> + wake_up_interruptible(&stcontext->spicontext.st95wait_queue);
>> + return IRQ_HANDLED;
>> + }
>> +
>> + return IRQ_WAKE_THREAD;
>> +}
>Do you really need a specific IRQ handler for that ? Can't you do it from the
>threaded interrupt context ?
>
>Currently we don't need to run any specific code in hard IRQ context so yes we
>can use single threaded interrupt context for all cases.
>I will modify.
>
>> +static irqreturn_t irq_thread_handler(int irq, void *st95hfcontext)
>> +{
>> + int result;
>> + int res_len;
>> + int skb_len;
>> + static bool wtx;
>> + struct param_list new_params[1];
>> + struct st95hf_context *stcontext =
>> + (struct st95hf_context *)st95hfcontext;
>> + unsigned char error_byte;
>> + struct device *dev = &stcontext->nfcdev->dev;
>> + struct nfc_digital_dev *nfcddev = stcontext->ddev;
>> + unsigned char val_mm;
>> +
>> + struct st95_digital_cmd_complete_arg *cb_arg =
>> + stcontext->complete_cb_arg;
>> +
>> + if (!cb_arg) {
>> + dev_err(dev, "cb_arg is NULL in threaded ISR\n");
>> + BUG();
>> + }
>> +
>> + result = spi_receive_response(&stcontext->spicontext,
>> + cb_arg->skb_resp->data,
>> + &res_len);
>> + if (result) {
>> + dev_err(dev, "res receive threaded ISR err = 0x%x\n", result);
>> + goto end;
>> + }
>> +
>> + if (*((cb_arg->skb_resp->data) + 2) == WTX_REQ_FROM_TAG) {
>> + /* Request for new FWT from tag */
>FWT ?
>
>FWT is Frame Waiting Time.
>According to ISO-DEP protocol, tag can send the request to reader for new
>waiting time using WTX request (Waiting Time eXtension) when tag need more
>time at its end for processing.
>In this situation, in response of WTX request reader is also required to be re-
>configured to increase its timeout.
>So that reader will wait for response from tag for the sufficient time before
>declaring the timeout error.
>
>
>> + result = iso14443_config_fdt(stcontext,
>> + (*((cb_arg->skb_resp->data) + 3)
>> + & 0x3f));
>> + if (result) {
>> + dev_err(dev, "Config. setting error on WTX req, err =
>0x%x\n",
>> + result);
>> + goto end;
>> + }
>> + wtx = true;
>> +
>> + /* ASYNC req as no response expected */
>> + new_params[0].param_offset = 1;
>> + new_params[0].new_param_val =
>> + *((cb_arg->skb_resp->data) + 3);
>> +
>> + result = st95hf_send_cmd(stcontext,
>> + WTX_RESPONSE,
>> + 1,
>> + new_params);
>> + if (result) {
>> + dev_err(dev, "WTX response send, err = 0x%x\n",
>result);
>> + goto end;
>> + }
>> + return IRQ_HANDLED;
>> + }
>A few comments:
>
>- You probably want to define a struct sk_buff *skb_resp =
> cb_arg->skb_resp
>
>Ok. I will simplify the code by defining a local structure as struct sk_buff
>*skb_resp =
> cb_arg->skb_resp.
>
>- *((cb_arg->skb_resp->data) + 2) becomes skb_resp->data[2]
>
>Ok. Will modify the whole code accordingly.
>
>- This handler is almost 200 lines of code long, it could be splitted in
> smaller routines:
> * One for handling FWT from tag
> * One for handling I/O errors
> * One for actually processing the response Ok.
>I will split the handler to make it more modular.
>
>> +static struct nfc_digital_ops st95hf_nfc_digital_ops = {
>> + .in_configure_hw = st95hf_in_configure_hw,
>> + .in_send_cmd = st95hf_in_send_cmd,
>> +
>> + .tg_listen = st95hf_tg_listen,
>> + .tg_configure_hw = st95hf_tg_configure_hw,
>> + .tg_send_cmd = st95hf_tg_send_cmd,
>> + .tg_get_rf_tech = st95hf_tg_get_rf_tech,
>> +
>> + .switch_rf = st95hf_switch_rf,
>> + .abort_cmd = st95hf_abort_cmd,
>All the above ops can be static.
>
>Ok.
>
>> +static int st95hf_probe(struct spi_device *nfc_spi_dev) {
>> + int ret;
>> +
>> + struct st95hf_context *st95context;
>> + struct spi_context *spicontext;
>> + struct device_node *np = nfc_spi_dev->dev.of_node;
>> +
>> + pr_info("ST SPI SLAVE DRIVER (ST95HF R/W 14443A/B) PROBE
>CALLED\n");
>> +
>> + st95context = devm_kzalloc(&nfc_spi_dev->dev,
>> + sizeof(struct st95hf_context),
>> + GFP_KERNEL);
>> + if (!st95context)
>> + return -ENOMEM;
>> +
>> + spicontext = &st95context->spicontext;
>> +
>> + spicontext->spidev = nfc_spi_dev;
>> +
>> + st95context->fwi =
>> +cmd_array[ISO14443A_PROTOCOL_SELECT].cmd_params[2];
>> +
>> + if (of_get_property(np, "st95hfvin-supply", NULL)) {
>Please use device_property_present to be firmware API agnostic.
>
>Ok.
>
>Thanks!!
>Best Regards,
>Shikha
��.n��������+%������w��{.n�����{���zW����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
