[PATCH 00/11] NFC: pn533: bug fixes and improvements

[PATCH 00/11] NFC: pn533: bug fixes and improvements

[Michael Thalmeier]

Hello Samuel,

This patchset fixes some major bugs in the pn533 drivers (usb and i2c) and
improves performance of the PN532 chip by increasing its clock speed.

Best Regards
Michael

Michael Thalmeier (11):
  NFC: pn533: i2c: free irq on driver remove
  NFC: pn533: fix order of initialization
  NFC: pn533: i2c: do not call pn533_recv_frame with aborted commands
  NFC: pn533: reset poll modulation list before calling nfc_targets_found
  NFC: pn533: handle interrupted commands in pn533_recv_frame
  NFC: pn533: usb: fix errors when poll is stopped
  NFC: pn533: improve cmd queue handling
  NFC: pn533: reduce output when stopping poll
  NFC: pn533: use nfc_alloc_recv_skb for skb allocation
  NFC: pn533: set cmd status when not set
  nfc: pn533: increase clock frequency for PN532

 drivers/nfc/pn533/i2c.c   |  20 ++++--
 drivers/nfc/pn533/pn533.c | 154 +++++++++++++++++++++++++++++++++++++---------
 drivers/nfc/pn533/pn533.h |   5 +-
 drivers/nfc/pn533/usb.c   |  15 +++--
 4 files changed, 153 insertions(+), 41 deletions(-)

-- 
2.5.5

[PATCH 01/11] NFC: pn533: i2c: free irq on driver remove

[Michael Thalmeier]

The requested irq needs to be freed when removing the driver, otherwise a
following driver load fails to request the irq.

Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
---
 drivers/nfc/pn533/i2c.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
index 9679aa5..1a622e1 100644
--- a/drivers/nfc/pn533/i2c.c
+++ b/drivers/nfc/pn533/i2c.c
@@ -236,6 +236,8 @@ static int pn533_i2c_remove(struct i2c_client *client)
 
 	pn533_unregister_device(phy->priv);
 
+	free_irq(client->irq, phy);
+
 	return 0;
 }
 
-- 
2.5.5

[PATCH 02/11] NFC: pn533: fix order of initialization

[Michael Thalmeier]

Correctly call nfc_set_parent_dev before nfc_register_device. Otherwise the
driver will oops when being removed.

Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
---
 drivers/nfc/pn533/i2c.c   | 3 ++-
 drivers/nfc/pn533/pn533.c | 4 +++-
 drivers/nfc/pn533/pn533.h | 3 ++-
 drivers/nfc/pn533/usb.c   | 3 +--
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
index 1a622e1..0141f19 100644
--- a/drivers/nfc/pn533/i2c.c
+++ b/drivers/nfc/pn533/i2c.c
@@ -211,7 +211,8 @@ static int pn533_i2c_probe(struct i2c_client *client,
 				     PN533_NO_TYPE_B_PROTOCOLS,
 				     PN533_PROTO_REQ_ACK_RESP,
 				     phy, &i2c_phy_ops, NULL,
-				     &phy->i2c_dev->dev);
+				     &phy->i2c_dev->dev,
+				     &client->dev);
 
 	if (IS_ERR(priv)) {
 		r = PTR_ERR(priv);
diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index ee9e8f1..d82eecd 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -2554,7 +2554,8 @@ struct pn533 *pn533_register_device(u32 device_type,
 				void *phy,
 				struct pn533_phy_ops *phy_ops,
 				struct pn533_frame_ops *fops,
-				struct device *dev)
+				struct device *dev,
+				struct device *parent)
 {
 	struct pn533_fw_version fw_ver;
 	struct pn533 *priv;
@@ -2617,6 +2618,7 @@ struct pn533 *pn533_register_device(u32 device_type,
 		goto destroy_wq;
 	}
 
+	nfc_set_parent_dev(priv->nfc_dev, parent);
 	nfc_set_drvdata(priv->nfc_dev, priv);
 
 	rc = nfc_register_device(priv->nfc_dev);
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index ba604f6..553c7d1 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -228,7 +228,8 @@ struct pn533 *pn533_register_device(u32 device_type,
 				void *phy,
 				struct pn533_phy_ops *phy_ops,
 				struct pn533_frame_ops *fops,
-				struct device *dev);
+				struct device *dev,
+				struct device *parent);
 
 void pn533_unregister_device(struct pn533 *priv);
 void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status);
diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
index 4f73cbf..8ca0603 100644
--- a/drivers/nfc/pn533/usb.c
+++ b/drivers/nfc/pn533/usb.c
@@ -536,7 +536,7 @@ static int pn533_usb_probe(struct usb_interface *interface,
 
 	priv = pn533_register_device(id->driver_info, protocols, protocol_type,
 					phy, &usb_phy_ops, fops,
-					&phy->udev->dev);
+					&phy->udev->dev, &interface->dev);
 
 	if (IS_ERR(priv)) {
 		rc = PTR_ERR(priv);
@@ -544,7 +544,6 @@ static int pn533_usb_probe(struct usb_interface *interface,
 	}
 
 	phy->priv = priv;
-	nfc_set_parent_dev(priv->nfc_dev, &interface->dev);
 
 	usb_set_intfdata(interface, phy);
 
-- 
2.5.5

[PATCH 03/11] NFC: pn533: i2c: do not call pn533_recv_frame with aborted commands

[Michael Thalmeier]

When a command gets aborted the pn533 core does not need any RX frames that
may be received until a new frame is sent.

Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
---
 drivers/nfc/pn533/i2c.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
index 0141f19..1dc89248 100644
--- a/drivers/nfc/pn533/i2c.c
+++ b/drivers/nfc/pn533/i2c.c
@@ -39,6 +39,8 @@ struct pn533_i2c_phy {
 	struct i2c_client *i2c_dev;
 	struct pn533 *priv;
 
+	bool aborted;
+
 	int hard_fault;		/*
 				 * < 0 if hardware error occurred (e.g. i2c err)
 				 * and prevents normal operation.
@@ -71,6 +73,8 @@ static int pn533_i2c_send_frame(struct pn533 *dev,
 	if (phy->priv == NULL)
 		phy->priv = dev;
 
+	phy->aborted = false;
+
 	print_hex_dump_debug("PN533_i2c TX: ", DUMP_PREFIX_NONE, 16, 1,
 			     out->data, out->len, false);
 
@@ -93,13 +97,15 @@ static int pn533_i2c_send_frame(struct pn533 *dev,
 
 static void pn533_i2c_abort_cmd(struct pn533 *dev, gfp_t flags)
 {
+	struct pn533_i2c_phy *phy = dev->phy;
+
+	phy->aborted = true;
+
 	/* An ack will cancel the last issued command */
 	pn533_i2c_send_ack(dev, flags);
 
 	/* schedule cmd_complete_work to finish current command execution */
-	if (dev->cmd != NULL)
-		dev->cmd->status = -ENOENT;
-	queue_work(dev->wq, &dev->cmd_complete_work);
+	pn533_recv_frame(phy->priv, NULL, -ENOENT);
 }
 
 static int pn533_i2c_read(struct pn533_i2c_phy *phy, struct sk_buff **skb)
@@ -164,7 +170,8 @@ static irqreturn_t pn533_i2c_irq_thread_fn(int irq, void *data)
 		return IRQ_HANDLED;
 	}
 
-	pn533_recv_frame(phy->priv, skb, 0);
+	if (!phy->aborted)
+		pn533_recv_frame(phy->priv, skb, 0);
 
 	return IRQ_HANDLED;
 }
-- 
2.5.5

[PATCH 04/11] NFC: pn533: reset poll modulation list before calling nfc_targets_found

[Michael Thalmeier]

We need to reset the poll modulation list before calling nfc_targets_found
because otherwise it is possible that the application is scheduled to run
before the modulation list is cleared and gets an error "Cannot activate
target while polling" upon calling activate_target.

Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
---
 drivers/nfc/pn533/pn533.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index d82eecd..745181e 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -865,6 +865,7 @@ static int pn533_target_found_type_b(struct nfc_target *nfc_tgt, u8 *tgt_data,
 	return 0;
 }
 
+static void pn533_poll_reset_mod_list(struct pn533 *dev);
 static int pn533_target_found(struct pn533 *dev, u8 tg, u8 *tgdata,
 			      int tgdata_len)
 {
@@ -914,6 +915,7 @@ static int pn533_target_found(struct pn533 *dev, u8 tg, u8 *tgdata,
 
 	dev->tgt_available_prots = nfc_tgt.supported_protocols;
 
+	pn533_poll_reset_mod_list(dev);
 	nfc_targets_found(dev->nfc_dev, &nfc_tgt, 1);
 
 	return 0;
@@ -980,10 +982,8 @@ static int pn533_start_poll_complete(struct pn533 *dev, struct sk_buff *resp)
 		rc = pn533_target_found(dev, tg, tgdata, tgdata_len);
 
 		/* We must stop the poll after a valid target found */
-		if (rc == 0) {
-			pn533_poll_reset_mod_list(dev);
+		if (rc == 0)
 			return 0;
-		}
 	}
 
 	return -EAGAIN;
-- 
2.5.5

[PATCH 05/11] NFC: pn533: handle interrupted commands in pn533_recv_frame

[Michael Thalmeier]

When pn533_recv_frame is called from within abort_command context the current
dev->cmd is not guaranteed to be set.

Additionally on receiving an error status we can omit frame checking and
simply schedule the workquueue.

Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
---
 drivers/nfc/pn533/pn533.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index 745181e..d9c5583 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -2016,8 +2016,16 @@ _error:
  */
 void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status)
 {
+	if (!dev->cmd)
+		goto sched_wq;
+
 	dev->cmd->status = status;
 
+	if (status != 0) {
+		dev_dbg(dev->dev, "%s: Error received: %d\n", __func__, status);
+		goto sched_wq;
+	}
+
 	if (skb == NULL) {
 		pr_err("NULL Frame -> link is dead\n");
 		goto sched_wq;
-- 
2.5.5

[PATCH 06/11] NFC: pn533: usb: fix errors when poll is stopped

[Michael Thalmeier]

When a poll ist stopped we need to kill the out_urb request too before
starting a new request.

Additionally check if cmd is set in pn533_recv_ack befor accessing its struct
members.

Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
---
 drivers/nfc/pn533/usb.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
index 8ca0603..5500175 100644
--- a/drivers/nfc/pn533/usb.c
+++ b/drivers/nfc/pn533/usb.c
@@ -98,7 +98,8 @@ static void pn533_recv_ack(struct urb *urb)
 	struct pn533_std_frame *in_frame;
 	int rc;
 
-	cmd->status = urb->status;
+	if (cmd)
+		cmd->status = urb->status;
 
 	switch (urb->status) {
 	case 0:
@@ -120,7 +121,8 @@ static void pn533_recv_ack(struct urb *urb)
 
 	if (!pn533_rx_frame_is_ack(in_frame)) {
 		nfc_err(&phy->udev->dev, "Received an invalid ack\n");
-		cmd->status = -EIO;
+		if (cmd)
+			cmd->status = -EIO;
 		goto sched_wq;
 	}
 
@@ -128,7 +130,8 @@ static void pn533_recv_ack(struct urb *urb)
 	if (rc) {
 		nfc_err(&phy->udev->dev,
 			"usb_submit_urb failed with result %d\n", rc);
-		cmd->status = rc;
+		if (cmd)
+			cmd->status = rc;
 		goto sched_wq;
 	}
 
@@ -209,6 +212,9 @@ static void pn533_usb_abort_cmd(struct pn533 *dev, gfp_t flags)
 	if (dev->device_type == PN533_DEVICE_ACR122U)
 		return;
 
+	/* cancel the out urb request */
+	usb_kill_urb(phy->out_urb);
+
 	/* An ack will cancel the last issued command */
 	pn533_usb_send_ack(dev, flags);
 
-- 
2.5.5

Re: [PATCH 06/11] NFC: pn533: usb: fix errors when poll is stopped

[Samuel Ortiz]

Hi Michael,

On Thu, Apr 21, 2016 at 04:43:54PM +0200, Michael Thalmeier wrote:
> When a poll ist stopped we need to kill the out_urb request too before
> starting a new request.
> 
> Additionally check if cmd is set in pn533_recv_ack befor accessing its struct
> members.
I understand those 2 (stopping a poll and cmd being NULL) are not
completely unrelated, but I'd prefer if we could have separate patches
here.

As a matter of fact it seems you're trying to handle a potential NULL
cmd pointer in several of the subsequent patches and I'd like to see all
of those grouped in one patch with an explanation as to why we need to
do this (i.e. how come we can have such pointer being NULL).

Cheers,
Samuel.

[PATCH 07/11] NFC: pn533: improve cmd queue handling

[Michael Thalmeier]

Make sure cmd is set before a frame is passed to the transport layer for
sending. In addition pn533_send_async_complete checks if cmd is set before
accessing its members.

Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
---
 drivers/nfc/pn533/pn533.c | 54 +++++++++++++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 20 deletions(-)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index d9c5583..d1cc70a 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -383,26 +383,36 @@ static void pn533_build_cmd_frame(struct pn533 *dev, u8 cmd_code,
 static int pn533_send_async_complete(struct pn533 *dev)
 {
 	struct pn533_cmd *cmd = dev->cmd;
-	int status = cmd->status;
+	int rc = 0;
 
-	struct sk_buff *req = cmd->req;
-	struct sk_buff *resp = cmd->resp;
+	if (cmd) {
+		int status = cmd->status;
 
-	int rc;
+		struct sk_buff *req = cmd->req;
+		struct sk_buff *resp = cmd->resp;
 
-	dev_kfree_skb(req);
+		dev_kfree_skb(req);
 
-	if (status < 0) {
-		rc = cmd->complete_cb(dev, cmd->complete_cb_context,
-				      ERR_PTR(status));
-		dev_kfree_skb(resp);
-		goto done;
-	}
+		if (status < 0) {
+			rc = cmd->complete_cb(dev, cmd->complete_cb_context,
+					      ERR_PTR(status));
+			dev_kfree_skb(resp);
+			goto done;
+		}
+
+		/* when no response is set we got interrupted */
+		if (!resp)
+			resp = ERR_PTR(-EINTR);
 
-	skb_pull(resp, dev->ops->rx_header_len);
-	skb_trim(resp, resp->len - dev->ops->rx_tail_len);
+		if (!IS_ERR(resp)) {
+			skb_pull(resp, dev->ops->rx_header_len);
+			skb_trim(resp, resp->len - dev->ops->rx_tail_len);
+		}
 
-	rc = cmd->complete_cb(dev, cmd->complete_cb_context, resp);
+		rc = cmd->complete_cb(dev, cmd->complete_cb_context, resp);
+	} else {
+		dev_dbg(dev->dev, "%s: cmd not set\n", __func__);
+	}
 
 done:
 	kfree(cmd);
@@ -434,12 +444,14 @@ static int __pn533_send_async(struct pn533 *dev, u8 cmd_code,
 	mutex_lock(&dev->cmd_lock);
 
 	if (!dev->cmd_pending) {
+		dev->cmd = cmd;
 		rc = dev->phy_ops->send_frame(dev, req);
-		if (rc)
+		if (rc) {
+			dev->cmd = NULL;
 			goto error;
+		}
 
 		dev->cmd_pending = 1;
-		dev->cmd = cmd;
 		goto unlock;
 	}
 
@@ -511,11 +523,12 @@ static int pn533_send_cmd_direct_async(struct pn533 *dev, u8 cmd_code,
 
 	pn533_build_cmd_frame(dev, cmd_code, req);
 
+	dev->cmd = cmd;
 	rc = dev->phy_ops->send_frame(dev, req);
-	if (rc < 0)
+	if (rc < 0) {
+		dev->cmd = NULL;
 		kfree(cmd);
-	else
-		dev->cmd = cmd;
+	}
 
 	return rc;
 }
@@ -550,14 +563,15 @@ static void pn533_wq_cmd(struct work_struct *work)
 
 	mutex_unlock(&dev->cmd_lock);
 
+	dev->cmd = cmd;
 	rc = dev->phy_ops->send_frame(dev, cmd->req);
 	if (rc < 0) {
+		dev->cmd = NULL;
 		dev_kfree_skb(cmd->req);
 		kfree(cmd);
 		return;
 	}
 
-	dev->cmd = cmd;
 }
 
 struct pn533_sync_cmd_response {
-- 
2.5.5

Re: [PATCH 07/11] NFC: pn533: improve cmd queue handling

[Samuel Ortiz]

Hi Michael,

On Thu, Apr 21, 2016 at 04:43:55PM +0200, Michael Thalmeier wrote:
> Make sure cmd is set before a frame is passed to the transport layer for
> sending. In addition pn533_send_async_complete checks if cmd is set before
> accessing its members.
> 
> Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
> ---
>  drivers/nfc/pn533/pn533.c | 54 +++++++++++++++++++++++++++++------------------
>  1 file changed, 34 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
> index d9c5583..d1cc70a 100644
> --- a/drivers/nfc/pn533/pn533.c
> +++ b/drivers/nfc/pn533/pn533.c
> @@ -383,26 +383,36 @@ static void pn533_build_cmd_frame(struct pn533 *dev, u8 cmd_code,
>  static int pn533_send_async_complete(struct pn533 *dev)
>  {
>  	struct pn533_cmd *cmd = dev->cmd;
> -	int status = cmd->status;
> +	int rc = 0;
>  
> -	struct sk_buff *req = cmd->req;
> -	struct sk_buff *resp = cmd->resp;
> +	if (cmd) {
if (!cmd) {
	dev_dbg(dev->dev, "%s: cmd not set\n", __func__);
	return rc;
}

would make the rest of the code more readable.

Cheers,
Samuel.

[PATCH 08/11] NFC: pn533: reduce output when stopping poll

[Michael Thalmeier]

Handle return codes for stopped polling operations better to reduce logging
activity.

Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
---
 drivers/nfc/pn533/pn533.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index d1cc70a..c06d22f 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -1259,7 +1259,8 @@ static int pn533_rf_complete(struct pn533 *dev, void *arg,
 	if (IS_ERR(resp)) {
 		rc = PTR_ERR(resp);
 
-		nfc_err(dev->dev, "RF setting error %d\n", rc);
+		if (rc != -ENOENT)
+			nfc_err(dev->dev, "RF setting error %d\n", rc);
 
 		return rc;
 	}
@@ -1416,9 +1417,6 @@ static int pn533_poll_complete(struct pn533 *dev, void *arg,
 	if (IS_ERR(resp)) {
 		rc = PTR_ERR(resp);
 
-		nfc_err(dev->dev, "%s  Poll complete error %d\n",
-			__func__, rc);
-
 		if (rc == -ENOENT) {
 			if (dev->poll_mod_count != 0)
 				return rc;
@@ -1457,7 +1455,7 @@ done:
 	return rc;
 
 stop_poll:
-	nfc_err(dev->dev, "Polling operation has been stopped\n");
+	dev_dbg(dev->dev, "Polling operation has been stopped\n");
 
 	pn533_poll_reset_mod_list(dev);
 	dev->poll_protocols = 0;
-- 
2.5.5

Re: [PATCH 08/11] NFC: pn533: reduce output when stopping poll

[Samuel Ortiz]

Hi Michael,

On Thu, Apr 21, 2016 at 04:43:56PM +0200, Michael Thalmeier wrote:
> @@ -1259,7 +1259,8 @@ static int pn533_rf_complete(struct pn533 *dev, void *arg,
>  	if (IS_ERR(resp)) {
>  		rc = PTR_ERR(resp);
>  
> -		nfc_err(dev->dev, "RF setting error %d\n", rc);
> +		if (rc != -ENOENT)
> +			nfc_err(dev->dev, "RF setting error %d\n", rc);
Why ?


>  
>  		return rc;
>  	}
> @@ -1416,9 +1417,6 @@ static int pn533_poll_complete(struct pn533 *dev, void *arg,
>  	if (IS_ERR(resp)) {
>  		rc = PTR_ERR(resp);
>  
> -		nfc_err(dev->dev, "%s  Poll complete error %d\n",
> -			__func__, rc);
> -
Why are you removing that one ?

Cheers,
Samuel.

[PATCH 09/11] NFC: pn533: use nfc_alloc_recv_skb for skb allocation

[Michael Thalmeier]

When multiple receive frames need to be put together in pn533_build_response
we need to use nfc_alloc_recv_skb instead of the normal alloc_skb. Otherwise
the nfc core causes an skb error when it tries to push the status byte in
front of the data.

Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
---
 drivers/nfc/pn533/pn533.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index c06d22f..ae13277 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -1942,7 +1942,7 @@ static struct sk_buff *pn533_build_response(struct pn533 *dev)
 	dev_dbg(dev->dev, "%s total length %d\n",
 		__func__, skb_len);
 
-	skb = alloc_skb(skb_len, GFP_KERNEL);
+	skb = nfc_alloc_recv_skb(skb_len, GFP_KERNEL);
 	if (skb == NULL)
 		goto out;
 
-- 
2.5.5

Re: [PATCH 09/11] NFC: pn533: use nfc_alloc_recv_skb for skb allocation

[Samuel Ortiz]

Hi Michael,

On Thu, Apr 21, 2016 at 04:43:57PM +0200, Michael Thalmeier wrote:
> When multiple receive frames need to be put together in pn533_build_response
> we need to use nfc_alloc_recv_skb instead of the normal alloc_skb. Otherwise
> the nfc core causes an skb error when it tries to push the status byte in
> front of the data.
> 
> Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
> ---
>  drivers/nfc/pn533/pn533.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
> index c06d22f..ae13277 100644
> --- a/drivers/nfc/pn533/pn533.c
> +++ b/drivers/nfc/pn533/pn533.c
> @@ -1942,7 +1942,7 @@ static struct sk_buff *pn533_build_response(struct pn533 *dev)
>  	dev_dbg(dev->dev, "%s total length %d\n",
>  		__func__, skb_len);
>  
> -	skb = alloc_skb(skb_len, GFP_KERNEL);
> +	skb = nfc_alloc_recv_skb(skb_len, GFP_KERNEL);
That looks reasonable. What surprises me is that I'm pretty sure this
got tested (MI bit set on the Rx path)...
Do you get the error from rawsock_add_header() ?

Cheers,
Samuel.

[PATCH 10/11] NFC: pn533: set cmd status when not set

[Michael Thalmeier]

When pn533_recv_frame is called with skb = NULL and cmd->status = 0, set
cmd->status to an error code.

Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
---
 drivers/nfc/pn533/pn533.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index ae13277..44bc5e0 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -2040,6 +2040,8 @@ void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status)
 
 	if (skb == NULL) {
 		pr_err("NULL Frame -> link is dead\n");
+		if (!dev->cmd->status)
+			dev->cmd->status = -ENOENT;
 		goto sched_wq;
 	}
 
-- 
2.5.5

[PATCH 11/11] nfc: pn533: increase clock frequency for PN532

[Michael Thalmeier]

Default clock frequency of PN532 is 6.78 MHz. Increase the frequency to 27.12
MHz to increase throughput.

Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>
---
 drivers/nfc/pn533/pn533.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nfc/pn533/pn533.h |  2 ++
 2 files changed, 72 insertions(+)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index 44bc5e0..19ee4ba 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -2469,6 +2469,63 @@ static int pn532_sam_configuration(struct nfc_dev *nfc_dev)
 	return 0;
 }
 
+#define PN533_CFR_27	0x00
+#define PN533_CFR_13	0x01
+#define PN533_CFR_6	0x02
+
+static int pn533_get_cfr(struct nfc_dev *nfc_dev)
+{
+	struct pn533 *dev = nfc_get_drvdata(nfc_dev);
+	struct sk_buff *skb;
+	struct sk_buff *resp;
+	int clk;
+
+	dev_dbg(dev->dev, "%s\n", __func__);
+
+	skb = pn533_alloc_skb(dev, 2);
+	if (!skb)
+		return -ENOMEM;
+
+	*skb_put(skb, 1) = 0x02;
+	*skb_put(skb, 1) = 0xFF;
+
+	resp = pn533_send_cmd_sync(dev, PN533_CMD_READ_REGISTER, skb);
+	if (IS_ERR(resp))
+		return PTR_ERR(resp);
+
+	if (resp->len != 1)
+		return -EINVAL;
+
+	clk = resp->data[0];
+
+	dev_kfree_skb(resp);
+	return clk;
+}
+
+static int pn533_set_cfr(struct nfc_dev *nfc_dev, int clk)
+{
+	struct pn533 *dev = nfc_get_drvdata(nfc_dev);
+	struct sk_buff *skb;
+	struct sk_buff *resp;
+
+	dev_dbg(dev->dev, "%s\n", __func__);
+
+	skb = pn533_alloc_skb(dev, 3);
+	if (!skb)
+		return -ENOMEM;
+
+	*skb_put(skb, 1) = 0x02;
+	*skb_put(skb, 1) = 0xFF;
+	*skb_put(skb, 1) = clk & 0x03;
+
+	resp = pn533_send_cmd_sync(dev, PN533_CMD_WRITE_REGISTER, skb);
+	if (IS_ERR(resp))
+		return PTR_ERR(resp);
+
+	dev_kfree_skb(resp);
+	return 0;
+}
+
 static int pn533_dev_up(struct nfc_dev *nfc_dev)
 {
 	struct pn533 *dev = nfc_get_drvdata(nfc_dev);
@@ -2478,6 +2535,10 @@ static int pn533_dev_up(struct nfc_dev *nfc_dev)
 
 		if (rc)
 			return rc;
+
+		rc = pn533_set_cfr(nfc_dev, PN533_CFR_27);
+		if (rc)
+			return rc;
 	}
 
 	return pn533_rf_field(nfc_dev, 1);
@@ -2485,6 +2546,15 @@ static int pn533_dev_up(struct nfc_dev *nfc_dev)
 
 static int pn533_dev_down(struct nfc_dev *nfc_dev)
 {
+	struct pn533 *dev = nfc_get_drvdata(nfc_dev);
+
+	if (dev->device_type == PN533_DEVICE_PN532) {
+		int rc = pn533_set_cfr(nfc_dev, PN533_CFR_6);
+
+		if (rc)
+			return rc;
+	}
+
 	return pn533_rf_field(nfc_dev, 0);
 }
 
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index 553c7d1..2bd3816 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -74,6 +74,8 @@
 #define PN533_FRAME_CMD(f) (f->data[1])
 
 #define PN533_CMD_GET_FIRMWARE_VERSION 0x02
+#define PN533_CMD_READ_REGISTER 0x06
+#define PN533_CMD_WRITE_REGISTER 0x08
 #define PN533_CMD_SAM_CONFIGURATION 0x14
 #define PN533_CMD_RF_CONFIGURATION 0x32
 #define PN533_CMD_IN_DATA_EXCHANGE 0x40
-- 
2.5.5

Re: [PATCH 00/11] NFC: pn533: bug fixes and improvements

[Olliver Schinagl]

Hi Michael,

I know i'm a little late to the party, but why did you not name the 
directory/driver pn53x? I don't think we should debate wether a 533 is a 
532 with a USB added, or wether a 532 is a 533 with the USB stripped, I 
was just curious. As for the pn533-i2c, in the KConfig, I'd probably 
recommend naming it pn532 at the least though, since there is no variant 
other then USB for the 533?

Further more, I think it would be wise to add some documentation and nfc 
bindings examples.

I'm using it as a subnode of an i2c node as such:

&i2c2 {
	pinctrl-names = "default";
	pinctrl-0 = <&i2c2_pins_a>;
	status = "okay";

	pn532: pn53x@48 {
		compatible = "nxp,pn532-i2c";
		reg = <0x48>;
	};
};

But not sure if this is correct at all (I haven't gotten it to work yet, 
partly due to a burnt level shifter). For example, I'm not sure what 
other properties are needed.

Olliver

On 21-04-16 16:43, Michael Thalmeier wrote:
> Hello Samuel,
>
> This patchset fixes some major bugs in the pn533 drivers (usb and i2c) and
> improves performance of the PN532 chip by increasing its clock speed.
>
> Best Regards
> Michael
>
> Michael Thalmeier (11):
>    NFC: pn533: i2c: free irq on driver remove
>    NFC: pn533: fix order of initialization
>    NFC: pn533: i2c: do not call pn533_recv_frame with aborted commands
>    NFC: pn533: reset poll modulation list before calling nfc_targets_found
>    NFC: pn533: handle interrupted commands in pn533_recv_frame
>    NFC: pn533: usb: fix errors when poll is stopped
>    NFC: pn533: improve cmd queue handling
>    NFC: pn533: reduce output when stopping poll
>    NFC: pn533: use nfc_alloc_recv_skb for skb allocation
>    NFC: pn533: set cmd status when not set
>    nfc: pn533: increase clock frequency for PN532
>
>   drivers/nfc/pn533/i2c.c   |  20 ++++--
>   drivers/nfc/pn533/pn533.c | 154 +++++++++++++++++++++++++++++++++++++---------
>   drivers/nfc/pn533/pn533.h |   5 +-
>   drivers/nfc/pn533/usb.c   |  15 +++--
>   4 files changed, 153 insertions(+), 41 deletions(-)
>

Re: [PATCH 00/11] NFC: pn533: bug fixes and improvements

[Olliver Schinagl]

Hi Michael,

On 28-04-16 20:18, Olliver Schinagl wrote:
> Hi Michael,
>
> I know i'm a little late to the party, but why did you not name the
> directory/driver pn53x? I don't think we should debate wether a 533 is a
> 532 with a USB added, or wether a 532 is a 533 with the USB stripped, I
> was just curious. As for the pn533-i2c, in the KConfig, I'd probably
> recommend naming it pn532 at the least though, since there is no variant
> other then USB for the 533?
>
> Further more, I think it would be wise to add some documentation and nfc
> bindings examples.
>
> I'm using it as a subnode of an i2c node as such:
>
> &i2c2 {
>      pinctrl-names = "default";
>      pinctrl-0 = <&i2c2_pins_a>;
>      status = "okay";
>
>      pn532: pn53x@48 {
>          compatible = "nxp,pn532-i2c";
>          reg = <0x48>;
>      };
> };
>
> But not sure if this is correct at all (I haven't gotten it to work yet,
> partly due to a burnt level shifter). For example, I'm not sure what
> other properties are needed.
I think that this is all fine, it should be 0x24 instead of 0x48 if it 
matters at all (hardcoded in the driver?) but my initial problem was 
that i was loading pn533 and not pn533_i2c.

The driver seems to like to use an interrupt (one of the optional pins 
on the chip (nice for the documentation :)) other then the i2c interrupt 
I assume?

After the warning that we do not have an interrupt, we call 
pn533_register, which then just sits there waiting forever. I'll add 
more printk's into the pn533 driver to find out why, but if you have an 
idea and can save me some work, that'd be great!

Olliver

>
> Olliver
>
> On 21-04-16 16:43, Michael Thalmeier wrote:
>> Hello Samuel,
>>
>> This patchset fixes some major bugs in the pn533 drivers (usb and i2c)
>> and
>> improves performance of the PN532 chip by increasing its clock speed.
>>
>> Best Regards
>> Michael
>>
>> Michael Thalmeier (11):
>>    NFC: pn533: i2c: free irq on driver remove
>>    NFC: pn533: fix order of initialization
>>    NFC: pn533: i2c: do not call pn533_recv_frame with aborted commands
>>    NFC: pn533: reset poll modulation list before calling
>> nfc_targets_found
>>    NFC: pn533: handle interrupted commands in pn533_recv_frame
>>    NFC: pn533: usb: fix errors when poll is stopped
>>    NFC: pn533: improve cmd queue handling
>>    NFC: pn533: reduce output when stopping poll
>>    NFC: pn533: use nfc_alloc_recv_skb for skb allocation
>>    NFC: pn533: set cmd status when not set
>>    nfc: pn533: increase clock frequency for PN532
>>
>>   drivers/nfc/pn533/i2c.c   |  20 ++++--
>>   drivers/nfc/pn533/pn533.c | 154
>> +++++++++++++++++++++++++++++++++++++---------
>>   drivers/nfc/pn533/pn533.h |   5 +-
>>   drivers/nfc/pn533/usb.c   |  15 +++--
>>   4 files changed, 153 insertions(+), 41 deletions(-)
>>
>
>

Re: [PATCH 00/11] NFC: pn533: bug fixes and improvements

[Olliver Schinagl]

Michael,

On 04-05-16 17:18, Olliver Schinagl wrote:
> Hi Michael,
>
> On 28-04-16 20:18, Olliver Schinagl wrote:
>> Hi Michael,
>>
>> I know i'm a little late to the party, but why did you not name the
>> directory/driver pn53x? I don't think we should debate wether a 533 is a
>> 532 with a USB added, or wether a 532 is a 533 with the USB stripped, I
>> was just curious. As for the pn533-i2c, in the KConfig, I'd probably
>> recommend naming it pn532 at the least though, since there is no variant
>> other then USB for the 533?
>>
>> Further more, I think it would be wise to add some documentation and nfc
>> bindings examples.
>>
>> I'm using it as a subnode of an i2c node as such:
>>
>> &i2c2 {
>>      pinctrl-names = "default";
>>      pinctrl-0 = <&i2c2_pins_a>;
>>      status = "okay";
>>
>>      pn532: pn53x@48 {
>>          compatible = "nxp,pn532-i2c";
>>          reg = <0x48>;
>>      };
>> };
>>
>> But not sure if this is correct at all (I haven't gotten it to work yet,
>> partly due to a burnt level shifter). For example, I'm not sure what
>> other properties are needed.
> I think that this is all fine, it should be 0x24 instead of 0x48 if it
> matters at all (hardcoded in the driver?) but my initial problem was
> that i was loading pn533 and not pn533_i2c.
>
> The driver seems to like to use an interrupt (one of the optional pins
> on the chip (nice for the documentation :)) other then the i2c interrupt
> I assume?
>
> After the warning that we do not have an interrupt, we call
> pn533_register, which then just sits there waiting forever. I'll add
> more printk's into the pn533 driver to find out why, but if you have an
> idea and can save me some work, that'd be great!
I've added some printk's and have found out where it is waiting, but 
don't quite understand the why.

The i2c probe stuff gets properly executed until the register, the register
pn533_get_firmware_version calls pn533_send_cmd_sync to retrieve the 
firmware version of the 532.

It does that by calling waiting for pn533_send_cmd_async via 
wait_for_completion(&arg.done), Unfortunatly however, 
wait_for_completion, well just waits forever.

Looking into the wait_for_completion code, I only see:

          might_sleep();

          spin_lock_irq(&x->wait.lock);
          timeout = do_wait_for_common(x, action, timeout, state);
          spin_unlock_irq(&x->wait.lock);
          return timeout;

but i'm I suppose those irq's have nothing to do with the hardware 
interrupt the i2c driver wants/needs?

dingging into 'ops->send_frame', I do see data going over the line on 
the scope, and while I did not analyze the exact bytes on the scope, I 
think it's quite certain to say it's the firmware request bytes/answers.

So the request does go over the line happily.

But this is where I get stuck now.

Find below my printk mess, I did not use __FUNCTION__ and __LINE__ 
concistently at all.

[   22.087495] Probing pn533!
[   22.090360] pn533_i2c 2-0024: pn533_i2c_probe
[   22.094728] pn533_i2c 2-0024: IRQ: 0
[   22.098309] requested irq 0
[   22.101145] registering device
[   22.104208] pn reg start
[   22.106747] pre mutex
[   22.109021] post mutex
[   22.111422] INIT_DONE
[   22.113736] Timer init
[   22.116101] timer init done
[   22.118895] skb_queue
[   22.121205] skb_queue done
[   22.123919] INIT_LIST
[   22.126193] pn533_get_firmware_version, start
[   22.130588] DEBUG: pn533_get_firmware_version pn533_alloc_skb (2393)
[   22.137069] DEBUG: pn533_get_firmware_version pn533_alloc_skb done 
(2397)
[   22.143994] DEBUG: pn533_send_cmd_sync start (607)
[   22.148875] DEBUG: pn533_send_cmd_sync done init_completion (609)
[   22.155099] __pn533_send_async DEBUG, sending command 0x2 (422)
[   22.161059] __pn533_send_async DEBUG, build frame (433)
[   22.166402] __pn533_send_async DEBUG, build frame done (435)
[   22.172132] __pn533_send_async DEBUG, cmd not pending (440)
[   22.177721] pn533_i2c_send_frame DEBUG, no fault yet 74
[   22.183024] pn533_i2c_send_frame DEBUG, sending master 78
[   22.189430] pn533_i2c_send_frame DEBUG, master sent 80
[   22.194631] pn533_i2c_send_frame DEBUG, done 94
[   22.199172] __pn533_send_async DEBUG, frame sent (442)
[   22.204347] DEBUG: pn533_send_cmd_sync pn533_send_cmd_async done (613)
[   22.210985] DEBUG: pn533_send_cmd_sync going to wait for completion 
(619)

What else is needed to get this going?

Olliver
>
> Olliver
>
>>
>> Olliver
>>
>> On 21-04-16 16:43, Michael Thalmeier wrote:
>>> Hello Samuel,
>>>
>>> This patchset fixes some major bugs in the pn533 drivers (usb and i2c)
>>> and
>>> improves performance of the PN532 chip by increasing its clock speed.
>>>
>>> Best Regards
>>> Michael
>>>
>>> Michael Thalmeier (11):
>>>    NFC: pn533: i2c: free irq on driver remove
>>>    NFC: pn533: fix order of initialization
>>>    NFC: pn533: i2c: do not call pn533_recv_frame with aborted commands
>>>    NFC: pn533: reset poll modulation list before calling
>>> nfc_targets_found
>>>    NFC: pn533: handle interrupted commands in pn533_recv_frame
>>>    NFC: pn533: usb: fix errors when poll is stopped
>>>    NFC: pn533: improve cmd queue handling
>>>    NFC: pn533: reduce output when stopping poll
>>>    NFC: pn533: use nfc_alloc_recv_skb for skb allocation
>>>    NFC: pn533: set cmd status when not set
>>>    nfc: pn533: increase clock frequency for PN532
>>>
>>>   drivers/nfc/pn533/i2c.c   |  20 ++++--
>>>   drivers/nfc/pn533/pn533.c | 154
>>> +++++++++++++++++++++++++++++++++++++---------
>>>   drivers/nfc/pn533/pn533.h |   5 +-
>>>   drivers/nfc/pn533/usb.c   |  15 +++--
>>>   4 files changed, 153 insertions(+), 41 deletions(-)
>>>
>>
>>
>
>