[PATCH v6 1/7] nfc: pn533: i2c: "pn532" as dt compatible string

[PATCH v6 1/7] nfc: pn533: i2c: "pn532" as dt compatible string

[Lars Poeschel]

It is favourable to have one unified compatible string for devices that
have multiple interfaces. So this adds simply "pn532" as the devicetree
binding compatible string and makes a note that the old ones are
deprecated.

Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v6:
- Rebased the patch series on v5.3-rc5

Changes in v3:
- This patch is new in v3

 drivers/nfc/pn533/i2c.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
index 1832cd921ea7..1abd40398a5a 100644
--- a/drivers/nfc/pn533/i2c.c
+++ b/drivers/nfc/pn533/i2c.c
@@ -245,6 +245,11 @@ static int pn533_i2c_remove(struct i2c_client *client)
 }
 
 static const struct of_device_id of_pn533_i2c_match[] = {
+	{ .compatible = "nxp,pn532", },
+	/*
+	 * NOTE: The use of the compatibles with the trailing "...-i2c" is
+	 * deprecated and will be removed.
+	 */
 	{ .compatible = "nxp,pn533-i2c", },
 	{ .compatible = "nxp,pn532-i2c", },
 	{},
-- 
2.23.0.rc1

[PATCH v6 2/7] nfc: pn532_uart: Add NXP PN532 to devicetree docs

[Lars Poeschel]

Add a simple binding doc for the pn532.

Reviewed-by: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v6:
- Rebased the patch series on v5.3-rc5
- Picked up Rob's Reviewed-By

Changes in v4:
- Add documentation about reg property in case of i2c

Changes in v3:
- seperate binding doc instead of entry in trivial-devices.txt

 .../devicetree/bindings/nfc/pn532.txt         | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nfc/pn532.txt

diff --git a/Documentation/devicetree/bindings/nfc/pn532.txt b/Documentation/devicetree/bindings/nfc/pn532.txt
new file mode 100644
index 000000000000..d5aaa588073d
--- /dev/null
+++ b/Documentation/devicetree/bindings/nfc/pn532.txt
@@ -0,0 +1,33 @@
+NXP PN532 NFC Chip
+
+Required properties:
+- compatible: Should be
+    - "nxp,pn532" Place a node with this inside the devicetree node of the bus
+                  where the NFC chip is connected to.
+                  Currently the kernel has phy bindings for uart and i2c.
+    - "nxp,pn532-i2c" (DEPRECATED) only works for the i2c binding.
+    - "nxp,pn533-i2c" (DEPRECATED) only works for the i2c binding.
+
+Required properties if connected on i2c:
+- reg: for the i2c bus address. This is fixed at 0x48 for the PN532.
+
+Example uart:
+
+uart4: serial@49042000 {
+        compatible = "ti,omap3-uart";
+
+        pn532: nfc {
+                compatible = "nxp,pn532";
+        };
+};
+
+Example i2c:
+
+i2c1: i2c@0 {
+        compatible = "ti,omap3-i2c";
+
+        pn532: nfc {
+                compatible = "nxp,pn532";
+                reg = <0x48>;
+        };
+};
-- 
2.23.0.rc1

[PATCH v6 2/7] nfc: pn532_uart: Add NXP PN532 to devicetree docs

[Lars Poeschel]

Add a simple binding doc for the pn532.

Reviewed-by: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v6:
- Rebased the patch series on v5.3-rc5
- Picked up Rob's Reviewed-By

Changes in v4:
- Add documentation about reg property in case of i2c

Changes in v3:
- seperate binding doc instead of entry in trivial-devices.txt

 .../devicetree/bindings/nfc/pn532.txt         | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nfc/pn532.txt

diff --git a/Documentation/devicetree/bindings/nfc/pn532.txt b/Documentation/devicetree/bindings/nfc/pn532.txt
new file mode 100644
index 000000000000..d5aaa588073d
--- /dev/null
+++ b/Documentation/devicetree/bindings/nfc/pn532.txt
@@ -0,0 +1,33 @@
+NXP PN532 NFC Chip
+
+Required properties:
+- compatible: Should be
+    - "nxp,pn532" Place a node with this inside the devicetree node of the bus
+                  where the NFC chip is connected to.
+                  Currently the kernel has phy bindings for uart and i2c.
+    - "nxp,pn532-i2c" (DEPRECATED) only works for the i2c binding.
+    - "nxp,pn533-i2c" (DEPRECATED) only works for the i2c binding.
+
+Required properties if connected on i2c:
+- reg: for the i2c bus address. This is fixed at 0x48 for the PN532.
+
+Example uart:
+
+uart4: serial@49042000 {
+        compatible = "ti,omap3-uart";
+
+        pn532: nfc {
+                compatible = "nxp,pn532";
+        };
+};
+
+Example i2c:
+
+i2c1: i2c@0 {
+        compatible = "ti,omap3-i2c";
+
+        pn532: nfc {
+                compatible = "nxp,pn532";
+                reg = <0x48>;
+        };
+};
-- 
2.23.0.rc1

[PATCH v6 3/7] nfc: pn533: Add dev_up/dev_down hooks to phy_ops

[Lars Poeschel]

This adds hooks for dev_up and dev_down to the phy_ops. They are
optional.
The idea is to inform the phy driver when the nfc chip is really going
to be used. When it is not used, the phy driver can suspend it's
interface to the nfc chip to save some power. The nfc chip is considered
not in use before dev_up and after dev_down.

Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v6:
- Rebased the patch series on v5.3-rc5

Changes in v5:
- (dev->phy_ops->dev_up) instead (dev->phy_ops)

Changes in v4:
- This patch is new in v4

 drivers/nfc/pn533/pn533.c | 12 +++++++++++-
 drivers/nfc/pn533/pn533.h |  9 +++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index a172a32aa9d9..64836c727aee 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -2458,6 +2458,9 @@ static int pn533_dev_up(struct nfc_dev *nfc_dev)
 {
 	struct pn533 *dev = nfc_get_drvdata(nfc_dev);
 
+	if (dev->phy_ops->dev_up)
+		dev->phy_ops->dev_up(dev);
+
 	if (dev->device_type == PN533_DEVICE_PN532) {
 		int rc = pn532_sam_configuration(nfc_dev);
 
@@ -2470,7 +2473,14 @@ static int pn533_dev_up(struct nfc_dev *nfc_dev)
 
 static int pn533_dev_down(struct nfc_dev *nfc_dev)
 {
-	return pn533_rf_field(nfc_dev, 0);
+	struct pn533 *dev = nfc_get_drvdata(nfc_dev);
+	int ret;
+
+	ret = pn533_rf_field(nfc_dev, 0);
+	if (dev->phy_ops->dev_down && !ret)
+		dev->phy_ops->dev_down(dev);
+
+	return ret;
 }
 
 static struct nfc_ops pn533_nfc_ops = {
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index 8bf9d6ece0f5..570ee0a3e832 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -207,6 +207,15 @@ struct pn533_phy_ops {
 			  struct sk_buff *out);
 	int (*send_ack)(struct pn533 *dev, gfp_t flags);
 	void (*abort_cmd)(struct pn533 *priv, gfp_t flags);
+	/*
+	 * dev_up and dev_down are optional.
+	 * They are used to inform the phy layer that the nfc chip
+	 * is going to be really used very soon. The phy layer can then
+	 * bring up it's interface to the chip and have it suspended for power
+	 * saving reasons otherwise.
+	 */
+	void (*dev_up)(struct pn533 *priv);
+	void (*dev_down)(struct pn533 *priv);
 };
 
 
-- 
2.23.0.rc1

[PATCH v6 4/7] nfc: pn533: Split pn533 init & nfc_register

[Lars Poeschel]

There is a problem in the initialisation and setup of the pn533: It
registers with nfc too early. It could happen, that it finished
registering with nfc and someone starts using it. But setup of the pn533
is not yet finished. Bad or at least unintended things could happen.
So I split out nfc registering (and unregistering) to seperate functions
that have to be called late in probe then.

Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v6:
- Rebased the patch series on v5.3-rc5

Changes in v5:
- This patch is new in v5

 drivers/nfc/pn533/i2c.c   | 17 +++++-----
 drivers/nfc/pn533/pn533.c | 66 ++++++++++++++++++++-------------------
 drivers/nfc/pn533/pn533.h | 11 ++++---
 drivers/nfc/pn533/usb.c   | 12 +++++--
 4 files changed, 59 insertions(+), 47 deletions(-)

diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
index 1abd40398a5a..e9e5a1ec8857 100644
--- a/drivers/nfc/pn533/i2c.c
+++ b/drivers/nfc/pn533/i2c.c
@@ -193,12 +193,10 @@ static int pn533_i2c_probe(struct i2c_client *client,
 	phy->i2c_dev = client;
 	i2c_set_clientdata(client, phy);
 
-	priv = pn533_register_device(PN533_DEVICE_PN532,
-				     PN533_NO_TYPE_B_PROTOCOLS,
+	priv = pn53x_common_init(PN533_DEVICE_PN532,
 				     PN533_PROTO_REQ_ACK_RESP,
 				     phy, &i2c_phy_ops, NULL,
-				     &phy->i2c_dev->dev,
-				     &client->dev);
+				     &phy->i2c_dev->dev);
 
 	if (IS_ERR(priv)) {
 		r = PTR_ERR(priv);
@@ -220,13 +218,17 @@ static int pn533_i2c_probe(struct i2c_client *client,
 	if (r)
 		goto fn_setup_err;
 
-	return 0;
+	r = pn53x_register_nfc(priv, PN533_NO_TYPE_B_PROTOCOLS, &client->dev);
+	if (r)
+		goto fn_setup_err;
+
+	return r;
 
 fn_setup_err:
 	free_irq(client->irq, phy);
 
 irq_rqst_err:
-	pn533_unregister_device(phy->priv);
+	pn53x_common_clean(phy->priv);
 
 	return r;
 }
@@ -239,7 +241,8 @@ static int pn533_i2c_remove(struct i2c_client *client)
 
 	free_irq(client->irq, phy);
 
-	pn533_unregister_device(phy->priv);
+	pn53x_unregister_nfc(phy->priv);
+	pn53x_common_clean(phy->priv);
 
 	return 0;
 }
diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index 64836c727aee..a8c756caa678 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -2590,14 +2590,12 @@ int pn533_finalize_setup(struct pn533 *dev)
 }
 EXPORT_SYMBOL_GPL(pn533_finalize_setup);
 
-struct pn533 *pn533_register_device(u32 device_type,
-				u32 protocols,
+struct pn533 *pn53x_common_init(u32 device_type,
 				enum pn533_protocol_type protocol_type,
 				void *phy,
 				struct pn533_phy_ops *phy_ops,
 				struct pn533_frame_ops *fops,
-				struct device *dev,
-				struct device *parent)
+				struct device *dev)
 {
 	struct pn533 *priv;
 	int rc = -ENOMEM;
@@ -2638,43 +2636,18 @@ struct pn533 *pn533_register_device(u32 device_type,
 	skb_queue_head_init(&priv->fragment_skb);
 
 	INIT_LIST_HEAD(&priv->cmd_queue);
-
-	priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols,
-					   priv->ops->tx_header_len +
-					   PN533_CMD_DATAEXCH_HEAD_LEN,
-					   priv->ops->tx_tail_len);
-	if (!priv->nfc_dev) {
-		rc = -ENOMEM;
-		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);
-	if (rc)
-		goto free_nfc_dev;
-
 	return priv;
 
-free_nfc_dev:
-	nfc_free_device(priv->nfc_dev);
-
-destroy_wq:
-	destroy_workqueue(priv->wq);
 error:
 	kfree(priv);
 	return ERR_PTR(rc);
 }
-EXPORT_SYMBOL_GPL(pn533_register_device);
+EXPORT_SYMBOL_GPL(pn53x_common_init);
 
-void pn533_unregister_device(struct pn533 *priv)
+void pn53x_common_clean(struct pn533 *priv)
 {
 	struct pn533_cmd *cmd, *n;
 
-	nfc_unregister_device(priv->nfc_dev);
-	nfc_free_device(priv->nfc_dev);
-
 	flush_delayed_work(&priv->poll_work);
 	destroy_workqueue(priv->wq);
 
@@ -2689,8 +2662,37 @@ void pn533_unregister_device(struct pn533 *priv)
 
 	kfree(priv);
 }
-EXPORT_SYMBOL_GPL(pn533_unregister_device);
+EXPORT_SYMBOL_GPL(pn53x_common_clean);
+
+int pn53x_register_nfc(struct pn533 *priv, u32 protocols,
+			struct device *parent)
+{
+	int rc = -ENOMEM;
+
+	priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols,
+					   priv->ops->tx_header_len +
+					   PN533_CMD_DATAEXCH_HEAD_LEN,
+					   priv->ops->tx_tail_len);
+	if (!priv->nfc_dev)
+		return -ENOMEM;
+
+	nfc_set_parent_dev(priv->nfc_dev, parent);
+	nfc_set_drvdata(priv->nfc_dev, priv);
+
+	rc = nfc_register_device(priv->nfc_dev);
+	if (rc)
+		nfc_free_device(priv->nfc_dev);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(pn53x_register_nfc);
 
+void pn53x_unregister_nfc(struct pn533 *priv)
+{
+	nfc_unregister_device(priv->nfc_dev);
+	nfc_free_device(priv->nfc_dev);
+}
+EXPORT_SYMBOL_GPL(pn53x_unregister_nfc);
 
 MODULE_AUTHOR("Lauro Ramos Venancio <lauro.venancio@openbossa.org>");
 MODULE_AUTHOR("Aloisio Almeida Jr <aloisio.almeida@openbossa.org>");
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index 570ee0a3e832..510ddebbd896 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -219,18 +219,19 @@ struct pn533_phy_ops {
 };
 
 
-struct pn533 *pn533_register_device(u32 device_type,
-				u32 protocols,
+struct pn533 *pn53x_common_init(u32 device_type,
 				enum pn533_protocol_type protocol_type,
 				void *phy,
 				struct pn533_phy_ops *phy_ops,
 				struct pn533_frame_ops *fops,
-				struct device *dev,
-				struct device *parent);
+				struct device *dev);
 
 int pn533_finalize_setup(struct pn533 *dev);
-void pn533_unregister_device(struct pn533 *priv);
+void pn53x_common_clean(struct pn533 *priv);
 void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status);
+int pn53x_register_nfc(struct pn533 *priv, u32 protocols,
+			struct device *parent);
+void pn53x_unregister_nfc(struct pn533 *priv);
 
 bool pn533_rx_frame_is_cmd_response(struct pn533 *dev, void *frame);
 bool pn533_rx_frame_is_ack(void *_frame);
diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
index c5289eaf17ee..a1c6a41944c6 100644
--- a/drivers/nfc/pn533/usb.c
+++ b/drivers/nfc/pn533/usb.c
@@ -534,9 +534,9 @@ static int pn533_usb_probe(struct usb_interface *interface,
 		goto error;
 	}
 
-	priv = pn533_register_device(id->driver_info, protocols, protocol_type,
+	priv = pn53x_common_init(id->driver_info, protocol_type,
 					phy, &usb_phy_ops, fops,
-					&phy->udev->dev, &interface->dev);
+					&phy->udev->dev);
 
 	if (IS_ERR(priv)) {
 		rc = PTR_ERR(priv);
@@ -550,9 +550,14 @@ static int pn533_usb_probe(struct usb_interface *interface,
 		goto error;
 
 	usb_set_intfdata(interface, phy);
+	rc = pn53x_register_nfc(priv, protocols, &interface->dev);
+	if (rc)
+		goto err_clean;
 
 	return 0;
 
+err_clean:
+	pn53x_common_clean(priv);
 error:
 	usb_free_urb(phy->in_urb);
 	usb_free_urb(phy->out_urb);
@@ -570,7 +575,8 @@ static void pn533_usb_disconnect(struct usb_interface *interface)
 	if (!phy)
 		return;
 
-	pn533_unregister_device(phy->priv);
+	pn53x_unregister_nfc(phy->priv);
+	pn53x_common_clean(phy->priv);
 
 	usb_set_intfdata(interface, NULL);
 
-- 
2.23.0.rc1

Re: [PATCH v6 4/7] nfc: pn533: Split pn533 init & nfc_register

[Claudiu.Beznea]

On 20.08.2019 15:03, Lars Poeschel wrote:
> There is a problem in the initialisation and setup of the pn533: It
> registers with nfc too early. It could happen, that it finished
> registering with nfc and someone starts using it. But setup of the pn533
> is not yet finished. Bad or at least unintended things could happen.
> So I split out nfc registering (and unregistering) to seperate functions
> that have to be called late in probe then.
> 
> Cc: Johan Hovold <johan@kernel.org>
> Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> ---
> Changes in v6:
> - Rebased the patch series on v5.3-rc5
> 
> Changes in v5:
> - This patch is new in v5
> 
>  drivers/nfc/pn533/i2c.c   | 17 +++++-----
>  drivers/nfc/pn533/pn533.c | 66 ++++++++++++++++++++-------------------
>  drivers/nfc/pn533/pn533.h | 11 ++++---
>  drivers/nfc/pn533/usb.c   | 12 +++++--
>  4 files changed, 59 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
> index 1abd40398a5a..e9e5a1ec8857 100644
> --- a/drivers/nfc/pn533/i2c.c
> +++ b/drivers/nfc/pn533/i2c.c
> @@ -193,12 +193,10 @@ static int pn533_i2c_probe(struct i2c_client *client,
>  	phy->i2c_dev = client;
>  	i2c_set_clientdata(client, phy);
>  
> -	priv = pn533_register_device(PN533_DEVICE_PN532,
> -				     PN533_NO_TYPE_B_PROTOCOLS,
> +	priv = pn53x_common_init(PN533_DEVICE_PN532,
>  				     PN533_PROTO_REQ_ACK_RESP,
>  				     phy, &i2c_phy_ops, NULL,
> -				     &phy->i2c_dev->dev,
> -				     &client->dev);
> +				     &phy->i2c_dev->dev);
>  
>  	if (IS_ERR(priv)) {
>  		r = PTR_ERR(priv);
> @@ -220,13 +218,17 @@ static int pn533_i2c_probe(struct i2c_client *client,
>  	if (r)
>  		goto fn_setup_err;
>  
> -	return 0;
> +	r = pn53x_register_nfc(priv, PN533_NO_TYPE_B_PROTOCOLS, &client->dev);
> +	if (r)
> +		goto fn_setup_err;
> +
> +	return r;
>  
>  fn_setup_err:
>  	free_irq(client->irq, phy);
>  
>  irq_rqst_err:
> -	pn533_unregister_device(phy->priv);
> +	pn53x_common_clean(phy->priv);
>  
>  	return r;
>  }
> @@ -239,7 +241,8 @@ static int pn533_i2c_remove(struct i2c_client *client)
>  
>  	free_irq(client->irq, phy);
>  
> -	pn533_unregister_device(phy->priv);
> +	pn53x_unregister_nfc(phy->priv);
> +	pn53x_common_clean(phy->priv);
>  
>  	return 0;
>  }
> diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
> index 64836c727aee..a8c756caa678 100644
> --- a/drivers/nfc/pn533/pn533.c
> +++ b/drivers/nfc/pn533/pn533.c
> @@ -2590,14 +2590,12 @@ int pn533_finalize_setup(struct pn533 *dev)
>  }
>  EXPORT_SYMBOL_GPL(pn533_finalize_setup);
>  
> -struct pn533 *pn533_register_device(u32 device_type,
> -				u32 protocols,
> +struct pn533 *pn53x_common_init(u32 device_type,
>  				enum pn533_protocol_type protocol_type,
>  				void *phy,
>  				struct pn533_phy_ops *phy_ops,
>  				struct pn533_frame_ops *fops,
> -				struct device *dev,
> -				struct device *parent)
> +				struct device *dev)
>  {
>  	struct pn533 *priv;
>  	int rc = -ENOMEM;
> @@ -2638,43 +2636,18 @@ struct pn533 *pn533_register_device(u32 device_type,
>  	skb_queue_head_init(&priv->fragment_skb);
>  
>  	INIT_LIST_HEAD(&priv->cmd_queue);
> -
> -	priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols,
> -					   priv->ops->tx_header_len +
> -					   PN533_CMD_DATAEXCH_HEAD_LEN,
> -					   priv->ops->tx_tail_len);
> -	if (!priv->nfc_dev) {
> -		rc = -ENOMEM;
> -		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);
> -	if (rc)
> -		goto free_nfc_dev;
> -
>  	return priv;
>  
> -free_nfc_dev:
> -	nfc_free_device(priv->nfc_dev);
> -
> -destroy_wq:
> -	destroy_workqueue(priv->wq);
>  error:
>  	kfree(priv);
>  	return ERR_PTR(rc);
>  }
> -EXPORT_SYMBOL_GPL(pn533_register_device);
> +EXPORT_SYMBOL_GPL(pn53x_common_init);
>  
> -void pn533_unregister_device(struct pn533 *priv)
> +void pn53x_common_clean(struct pn533 *priv)
>  {
>  	struct pn533_cmd *cmd, *n;
>  
> -	nfc_unregister_device(priv->nfc_dev);
> -	nfc_free_device(priv->nfc_dev);
> -
>  	flush_delayed_work(&priv->poll_work);
>  	destroy_workqueue(priv->wq);
>  
> @@ -2689,8 +2662,37 @@ void pn533_unregister_device(struct pn533 *priv)
>  
>  	kfree(priv);
>  }
> -EXPORT_SYMBOL_GPL(pn533_unregister_device);
> +EXPORT_SYMBOL_GPL(pn53x_common_clean);
> +
> +int pn53x_register_nfc(struct pn533 *priv, u32 protocols,
> +			struct device *parent)
> +{
> +	int rc = -ENOMEM;

No need to initialize rc here... or just return rc below.

> +
> +	priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols,
> +					   priv->ops->tx_header_len +
> +					   PN533_CMD_DATAEXCH_HEAD_LEN,
> +					   priv->ops->tx_tail_len);
> +	if (!priv->nfc_dev)
> +		return -ENOMEM;
> +
> +	nfc_set_parent_dev(priv->nfc_dev, parent);
> +	nfc_set_drvdata(priv->nfc_dev, priv);
> +
> +	rc = nfc_register_device(priv->nfc_dev);
> +	if (rc)
> +		nfc_free_device(priv->nfc_dev);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(pn53x_register_nfc);
>  
> +void pn53x_unregister_nfc(struct pn533 *priv)
> +{
> +	nfc_unregister_device(priv->nfc_dev);
> +	nfc_free_device(priv->nfc_dev);
> +}
> +EXPORT_SYMBOL_GPL(pn53x_unregister_nfc);
>  
>  MODULE_AUTHOR("Lauro Ramos Venancio <lauro.venancio@openbossa.org>");
>  MODULE_AUTHOR("Aloisio Almeida Jr <aloisio.almeida@openbossa.org>");
> diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
> index 570ee0a3e832..510ddebbd896 100644
> --- a/drivers/nfc/pn533/pn533.h
> +++ b/drivers/nfc/pn533/pn533.h
> @@ -219,18 +219,19 @@ struct pn533_phy_ops {
>  };
>  
>  
> -struct pn533 *pn533_register_device(u32 device_type,
> -				u32 protocols,
> +struct pn533 *pn53x_common_init(u32 device_type,
>  				enum pn533_protocol_type protocol_type,
>  				void *phy,
>  				struct pn533_phy_ops *phy_ops,
>  				struct pn533_frame_ops *fops,
> -				struct device *dev,
> -				struct device *parent);
> +				struct device *dev);
>  
>  int pn533_finalize_setup(struct pn533 *dev);
> -void pn533_unregister_device(struct pn533 *priv);
> +void pn53x_common_clean(struct pn533 *priv);
>  void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status);
> +int pn53x_register_nfc(struct pn533 *priv, u32 protocols,
> +			struct device *parent);
> +void pn53x_unregister_nfc(struct pn533 *priv);
>  
>  bool pn533_rx_frame_is_cmd_response(struct pn533 *dev, void *frame);
>  bool pn533_rx_frame_is_ack(void *_frame);
> diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
> index c5289eaf17ee..a1c6a41944c6 100644
> --- a/drivers/nfc/pn533/usb.c
> +++ b/drivers/nfc/pn533/usb.c
> @@ -534,9 +534,9 @@ static int pn533_usb_probe(struct usb_interface *interface,
>  		goto error;
>  	}
>  
> -	priv = pn533_register_device(id->driver_info, protocols, protocol_type,
> +	priv = pn53x_common_init(id->driver_info, protocol_type,
>  					phy, &usb_phy_ops, fops,
> -					&phy->udev->dev, &interface->dev);
> +					&phy->udev->dev);
>  
>  	if (IS_ERR(priv)) {
>  		rc = PTR_ERR(priv);
> @@ -550,9 +550,14 @@ static int pn533_usb_probe(struct usb_interface *interface,
>  		goto error;
>  
>  	usb_set_intfdata(interface, phy);

Above this instruction there is this code:

	rc = pn533_finalize_setup(priv);
	if (rc)
		goto error;

Instead of "goto error;" you should have "goto err_clean;"

> +	rc = pn53x_register_nfc(priv, protocols, &interface->dev);
> +	if (rc)
> +		goto err_clean;
>  
>  	return 0;
>  
> +err_clean:
> +	pn53x_common_clean(priv);
>  error:
>  	usb_free_urb(phy->in_urb);
>  	usb_free_urb(phy->out_urb);
> @@ -570,7 +575,8 @@ static void pn533_usb_disconnect(struct usb_interface *interface)
>  	if (!phy)
>  		return;
>  
> -	pn533_unregister_device(phy->priv);
> +	pn53x_unregister_nfc(phy->priv);
> +	pn53x_common_clean(phy->priv);
>  
>  	usb_set_intfdata(interface, NULL);
>  
> 

Re: [PATCH v6 4/7] nfc: pn533: Split pn533 init & nfc_register

[Lars Poeschel]

On Thu, Aug 22, 2019 at 10:08:40AM +0000, Claudiu.Beznea@microchip.com wrote:
> 
> 
> On 20.08.2019 15:03, Lars Poeschel wrote:
> > There is a problem in the initialisation and setup of the pn533: It
> > registers with nfc too early. It could happen, that it finished
> > registering with nfc and someone starts using it. But setup of the pn533
> > is not yet finished. Bad or at least unintended things could happen.
> > So I split out nfc registering (and unregistering) to seperate functions
> > that have to be called late in probe then.
> > 
> > Cc: Johan Hovold <johan@kernel.org>
> > Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> > ---
> > Changes in v6:
> > - Rebased the patch series on v5.3-rc5
> > 
> > Changes in v5:
> > - This patch is new in v5
> > 
> >  drivers/nfc/pn533/i2c.c   | 17 +++++-----
> >  drivers/nfc/pn533/pn533.c | 66 ++++++++++++++++++++-------------------
> >  drivers/nfc/pn533/pn533.h | 11 ++++---
> >  drivers/nfc/pn533/usb.c   | 12 +++++--
> >  4 files changed, 59 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
> > index 1abd40398a5a..e9e5a1ec8857 100644
> > --- a/drivers/nfc/pn533/i2c.c
> > +++ b/drivers/nfc/pn533/i2c.c
> > @@ -193,12 +193,10 @@ static int pn533_i2c_probe(struct i2c_client *client,
> >  	phy->i2c_dev = client;
> >  	i2c_set_clientdata(client, phy);
> >  
> > -	priv = pn533_register_device(PN533_DEVICE_PN532,
> > -				     PN533_NO_TYPE_B_PROTOCOLS,
> > +	priv = pn53x_common_init(PN533_DEVICE_PN532,
> >  				     PN533_PROTO_REQ_ACK_RESP,
> >  				     phy, &i2c_phy_ops, NULL,
> > -				     &phy->i2c_dev->dev,
> > -				     &client->dev);
> > +				     &phy->i2c_dev->dev);
> >  
> >  	if (IS_ERR(priv)) {
> >  		r = PTR_ERR(priv);
> > @@ -220,13 +218,17 @@ static int pn533_i2c_probe(struct i2c_client *client,
> >  	if (r)
> >  		goto fn_setup_err;
> >  
> > -	return 0;
> > +	r = pn53x_register_nfc(priv, PN533_NO_TYPE_B_PROTOCOLS, &client->dev);
> > +	if (r)
> > +		goto fn_setup_err;
> > +
> > +	return r;
> >  
> >  fn_setup_err:
> >  	free_irq(client->irq, phy);
> >  
> >  irq_rqst_err:
> > -	pn533_unregister_device(phy->priv);
> > +	pn53x_common_clean(phy->priv);
> >  
> >  	return r;
> >  }
> > @@ -239,7 +241,8 @@ static int pn533_i2c_remove(struct i2c_client *client)
> >  
> >  	free_irq(client->irq, phy);
> >  
> > -	pn533_unregister_device(phy->priv);
> > +	pn53x_unregister_nfc(phy->priv);
> > +	pn53x_common_clean(phy->priv);
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
> > index 64836c727aee..a8c756caa678 100644
> > --- a/drivers/nfc/pn533/pn533.c
> > +++ b/drivers/nfc/pn533/pn533.c
> > @@ -2590,14 +2590,12 @@ int pn533_finalize_setup(struct pn533 *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(pn533_finalize_setup);
> >  
> > -struct pn533 *pn533_register_device(u32 device_type,
> > -				u32 protocols,
> > +struct pn533 *pn53x_common_init(u32 device_type,
> >  				enum pn533_protocol_type protocol_type,
> >  				void *phy,
> >  				struct pn533_phy_ops *phy_ops,
> >  				struct pn533_frame_ops *fops,
> > -				struct device *dev,
> > -				struct device *parent)
> > +				struct device *dev)
> >  {
> >  	struct pn533 *priv;
> >  	int rc = -ENOMEM;
> > @@ -2638,43 +2636,18 @@ struct pn533 *pn533_register_device(u32 device_type,
> >  	skb_queue_head_init(&priv->fragment_skb);
> >  
> >  	INIT_LIST_HEAD(&priv->cmd_queue);
> > -
> > -	priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols,
> > -					   priv->ops->tx_header_len +
> > -					   PN533_CMD_DATAEXCH_HEAD_LEN,
> > -					   priv->ops->tx_tail_len);
> > -	if (!priv->nfc_dev) {
> > -		rc = -ENOMEM;
> > -		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);
> > -	if (rc)
> > -		goto free_nfc_dev;
> > -
> >  	return priv;
> >  
> > -free_nfc_dev:
> > -	nfc_free_device(priv->nfc_dev);
> > -
> > -destroy_wq:
> > -	destroy_workqueue(priv->wq);
> >  error:
> >  	kfree(priv);
> >  	return ERR_PTR(rc);
> >  }
> > -EXPORT_SYMBOL_GPL(pn533_register_device);
> > +EXPORT_SYMBOL_GPL(pn53x_common_init);
> >  
> > -void pn533_unregister_device(struct pn533 *priv)
> > +void pn53x_common_clean(struct pn533 *priv)
> >  {
> >  	struct pn533_cmd *cmd, *n;
> >  
> > -	nfc_unregister_device(priv->nfc_dev);
> > -	nfc_free_device(priv->nfc_dev);
> > -
> >  	flush_delayed_work(&priv->poll_work);
> >  	destroy_workqueue(priv->wq);
> >  
> > @@ -2689,8 +2662,37 @@ void pn533_unregister_device(struct pn533 *priv)
> >  
> >  	kfree(priv);
> >  }
> > -EXPORT_SYMBOL_GPL(pn533_unregister_device);
> > +EXPORT_SYMBOL_GPL(pn53x_common_clean);
> > +
> > +int pn53x_register_nfc(struct pn533 *priv, u32 protocols,
> > +			struct device *parent)
> > +{
> > +	int rc = -ENOMEM;
> 
> No need to initialize rc here... or just return rc below.

I will remove the initialization. Looks better to me.

> > +
> > +	priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols,
> > +					   priv->ops->tx_header_len +
> > +					   PN533_CMD_DATAEXCH_HEAD_LEN,
> > +					   priv->ops->tx_tail_len);
> > +	if (!priv->nfc_dev)
> > +		return -ENOMEM;
> > +
> > +	nfc_set_parent_dev(priv->nfc_dev, parent);
> > +	nfc_set_drvdata(priv->nfc_dev, priv);
> > +
> > +	rc = nfc_register_device(priv->nfc_dev);
> > +	if (rc)
> > +		nfc_free_device(priv->nfc_dev);
> > +
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(pn53x_register_nfc);
> >  
> > +void pn53x_unregister_nfc(struct pn533 *priv)
> > +{
> > +	nfc_unregister_device(priv->nfc_dev);
> > +	nfc_free_device(priv->nfc_dev);
> > +}
> > +EXPORT_SYMBOL_GPL(pn53x_unregister_nfc);
> >  
> >  MODULE_AUTHOR("Lauro Ramos Venancio <lauro.venancio@openbossa.org>");
> >  MODULE_AUTHOR("Aloisio Almeida Jr <aloisio.almeida@openbossa.org>");
> > diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
> > index 570ee0a3e832..510ddebbd896 100644
> > --- a/drivers/nfc/pn533/pn533.h
> > +++ b/drivers/nfc/pn533/pn533.h
> > @@ -219,18 +219,19 @@ struct pn533_phy_ops {
> >  };
> >  
> >  
> > -struct pn533 *pn533_register_device(u32 device_type,
> > -				u32 protocols,
> > +struct pn533 *pn53x_common_init(u32 device_type,
> >  				enum pn533_protocol_type protocol_type,
> >  				void *phy,
> >  				struct pn533_phy_ops *phy_ops,
> >  				struct pn533_frame_ops *fops,
> > -				struct device *dev,
> > -				struct device *parent);
> > +				struct device *dev);
> >  
> >  int pn533_finalize_setup(struct pn533 *dev);
> > -void pn533_unregister_device(struct pn533 *priv);
> > +void pn53x_common_clean(struct pn533 *priv);
> >  void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status);
> > +int pn53x_register_nfc(struct pn533 *priv, u32 protocols,
> > +			struct device *parent);
> > +void pn53x_unregister_nfc(struct pn533 *priv);
> >  
> >  bool pn533_rx_frame_is_cmd_response(struct pn533 *dev, void *frame);
> >  bool pn533_rx_frame_is_ack(void *_frame);
> > diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
> > index c5289eaf17ee..a1c6a41944c6 100644
> > --- a/drivers/nfc/pn533/usb.c
> > +++ b/drivers/nfc/pn533/usb.c
> > @@ -534,9 +534,9 @@ static int pn533_usb_probe(struct usb_interface *interface,
> >  		goto error;
> >  	}
> >  
> > -	priv = pn533_register_device(id->driver_info, protocols, protocol_type,
> > +	priv = pn53x_common_init(id->driver_info, protocol_type,
> >  					phy, &usb_phy_ops, fops,
> > -					&phy->udev->dev, &interface->dev);
> > +					&phy->udev->dev);
> >  
> >  	if (IS_ERR(priv)) {
> >  		rc = PTR_ERR(priv);
> > @@ -550,9 +550,14 @@ static int pn533_usb_probe(struct usb_interface *interface,
> >  		goto error;
> >  
> >  	usb_set_intfdata(interface, phy);
> 
> Above this instruction there is this code:
> 
> 	rc = pn533_finalize_setup(priv);
> 	if (rc)
> 		goto error;
> 
> Instead of "goto error;" you should have "goto err_clean;"

Thank you for catching this one. I will change it.

> > +	rc = pn53x_register_nfc(priv, protocols, &interface->dev);
> > +	if (rc)
> > +		goto err_clean;
> >  
> >  	return 0;
> >  
> > +err_clean:
> > +	pn53x_common_clean(priv);
> >  error:
> >  	usb_free_urb(phy->in_urb);
> >  	usb_free_urb(phy->out_urb);
> > @@ -570,7 +575,8 @@ static void pn533_usb_disconnect(struct usb_interface *interface)
> >  	if (!phy)
> >  		return;
> >  
> > -	pn533_unregister_device(phy->priv);
> > +	pn53x_unregister_nfc(phy->priv);
> > +	pn53x_common_clean(phy->priv);
> >  
> >  	usb_set_intfdata(interface, NULL);
> >  
> > 

[PATCH v6 5/7] nfc: pn533: add UART phy driver

[Lars Poeschel]

This adds the UART phy interface for the pn533 driver.
The pn533 driver can be used through UART interface this way.
It is implemented as a serdev device.

Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v6:
- Rebased the patch series on v5.3-rc5

Changes in v5:
- Use the splitted pn53x_common_init and pn53x_register_nfc
  and pn53x_common_clean and pn53x_unregister_nfc alike

Changes in v4:
- SPDX-License-Identifier: GPL-2.0+
- Source code comments above refering items
- Error check for serdev_device_write's
- Change if (xxx == NULL) to if (!xxx)
- Remove device name from a dev_err
- move pn533_register in _probe a bit towards the end of _probe
- make use of newly added dev_up / dev_down phy_ops
- control send_wakeup variable from dev_up / dev_down

Changes in v3:
- depend on SERIAL_DEV_BUS in Kconfig

Changes in v2:
- switched from tty line discipline to serdev, resulting in many
  simplifications
- SPDX License Identifier

 drivers/nfc/pn533/Kconfig  |  11 ++
 drivers/nfc/pn533/Makefile |   2 +
 drivers/nfc/pn533/pn533.h  |   8 +
 drivers/nfc/pn533/uart.c   | 316 +++++++++++++++++++++++++++++++++++++
 4 files changed, 337 insertions(+)
 create mode 100644 drivers/nfc/pn533/uart.c

diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig
index f6d6b345ba0d..7fe1bbe26568 100644
--- a/drivers/nfc/pn533/Kconfig
+++ b/drivers/nfc/pn533/Kconfig
@@ -26,3 +26,14 @@ config NFC_PN533_I2C
 
 	  If you choose to build a module, it'll be called pn533_i2c.
 	  Say N if unsure.
+
+config NFC_PN532_UART
+	tristate "NFC PN532 device support (UART)"
+	depends on SERIAL_DEV_BUS
+	select NFC_PN533
+	---help---
+	  This module adds support for the NXP pn532 UART interface.
+	  Select this if your platform is using the UART bus.
+
+	  If you choose to build a module, it'll be called pn532_uart.
+	  Say N if unsure.
diff --git a/drivers/nfc/pn533/Makefile b/drivers/nfc/pn533/Makefile
index 43c25b4f9466..b9648337576f 100644
--- a/drivers/nfc/pn533/Makefile
+++ b/drivers/nfc/pn533/Makefile
@@ -4,7 +4,9 @@
 #
 pn533_usb-objs  = usb.o
 pn533_i2c-objs  = i2c.o
+pn532_uart-objs  = uart.o
 
 obj-$(CONFIG_NFC_PN533)     += pn533.o
 obj-$(CONFIG_NFC_PN533_USB) += pn533_usb.o
 obj-$(CONFIG_NFC_PN533_I2C) += pn533_i2c.o
+obj-$(CONFIG_NFC_PN532_UART) += pn532_uart.o
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index 510ddebbd896..6541088fad73 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -43,6 +43,11 @@
 
 /* Preamble (1), SoPC (2), ACK Code (2), Postamble (1) */
 #define PN533_STD_FRAME_ACK_SIZE 6
+/*
+ * Preamble (1), SoPC (2), Packet Length (1), Packet Length Checksum (1),
+ * Specific Application Level Error Code (1) , Postamble (1)
+ */
+#define PN533_STD_ERROR_FRAME_SIZE 8
 #define PN533_STD_FRAME_CHECKSUM(f) (f->data[f->datalen])
 #define PN533_STD_FRAME_POSTAMBLE(f) (f->data[f->datalen + 1])
 /* Half start code (3), LEN (4) should be 0xffff for extended frame */
@@ -84,6 +89,9 @@
 #define PN533_CMD_MI_MASK 0x40
 #define PN533_CMD_RET_SUCCESS 0x00
 
+#define PN533_FRAME_DATALEN_ACK 0x00
+#define PN533_FRAME_DATALEN_ERROR 0x01
+#define PN533_FRAME_DATALEN_EXTENDED 0xFF
 
 enum  pn533_protocol_type {
 	PN533_PROTO_REQ_ACK_RESP = 0,
diff --git a/drivers/nfc/pn533/uart.c b/drivers/nfc/pn533/uart.c
new file mode 100644
index 000000000000..f1cc2354a4fd
--- /dev/null
+++ b/drivers/nfc/pn533/uart.c
@@ -0,0 +1,316 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for NXP PN532 NFC Chip - UART transport layer
+ *
+ * Copyright (C) 2018 Lemonage Software GmbH
+ * Author: Lars Pöschel <poeschel@lemonage.de>
+ * All rights reserved.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/nfc.h>
+#include <linux/netdevice.h>
+#include <linux/of.h>
+#include <linux/serdev.h>
+#include "pn533.h"
+
+#define PN532_UART_SKB_BUFF_LEN	(PN533_CMD_DATAEXCH_DATA_MAXLEN * 2)
+
+enum send_wakeup {
+	PN532_SEND_NO_WAKEUP = 0,
+	PN532_SEND_WAKEUP,
+	PN532_SEND_LAST_WAKEUP,
+};
+
+
+struct pn532_uart_phy {
+	struct serdev_device *serdev;
+	struct sk_buff *recv_skb;
+	struct pn533 *priv;
+	enum send_wakeup send_wakeup;
+	struct timer_list cmd_timeout;
+	struct sk_buff *cur_out_buf;
+};
+
+static int pn532_uart_send_frame(struct pn533 *dev,
+				struct sk_buff *out)
+{
+	/* wakeup sequence and dummy bytes for waiting time */
+	static const u8 wakeup[] = {
+		0x55, 0x55, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+	struct pn532_uart_phy *pn532 = dev->phy;
+	int err;
+
+	print_hex_dump_debug("PN532_uart TX: ", DUMP_PREFIX_NONE, 16, 1,
+			     out->data, out->len, false);
+
+	pn532->cur_out_buf = out;
+	if (pn532->send_wakeup) {
+		err= serdev_device_write(pn532->serdev,
+				wakeup, sizeof(wakeup),
+				MAX_SCHEDULE_TIMEOUT);
+		if (err < 0)
+			return err;
+	}
+
+	if (pn532->send_wakeup == PN532_SEND_LAST_WAKEUP) {
+		pn532->send_wakeup = PN532_SEND_NO_WAKEUP;
+	}
+
+	err = serdev_device_write(pn532->serdev, out->data, out->len,
+			MAX_SCHEDULE_TIMEOUT);
+	if (err < 0)
+		return err;
+
+	mod_timer(&pn532->cmd_timeout, HZ / 40 + jiffies);
+	return 0;
+}
+
+static int pn532_uart_send_ack(struct pn533 *dev, gfp_t flags)
+{
+	struct pn532_uart_phy *pn532 = dev->phy;
+	/* spec 7.1.1.3:  Preamble, SoPC (2), ACK Code (2), Postamble */
+	static const u8 ack[PN533_STD_FRAME_ACK_SIZE] = {
+			0x00, 0x00, 0xff, 0x00, 0xff, 0x00};
+	int err;
+
+	err = serdev_device_write(pn532->serdev, ack, sizeof(ack),
+			MAX_SCHEDULE_TIMEOUT);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static void pn532_uart_abort_cmd(struct pn533 *dev, gfp_t flags)
+{
+	/* An ack will cancel the last issued command */
+	pn532_uart_send_ack(dev, flags);
+	/* schedule cmd_complete_work to finish current command execution */
+	pn533_recv_frame(dev, NULL, -ENOENT);
+}
+
+static void pn532_dev_up(struct pn533 *dev)
+{
+	struct pn532_uart_phy *pn532 = dev->phy;
+
+	serdev_device_open(pn532->serdev);
+	pn532->send_wakeup = PN532_SEND_LAST_WAKEUP;
+}
+
+static void pn532_dev_down(struct pn533 *dev)
+{
+	struct pn532_uart_phy *pn532 = dev->phy;
+
+	serdev_device_close(pn532->serdev);
+	pn532->send_wakeup = PN532_SEND_WAKEUP;
+}
+
+static struct pn533_phy_ops uart_phy_ops = {
+	.send_frame = pn532_uart_send_frame,
+	.send_ack = pn532_uart_send_ack,
+	.abort_cmd = pn532_uart_abort_cmd,
+	.dev_up = pn532_dev_up,
+	.dev_down = pn532_dev_down,
+};
+
+static void pn532_cmd_timeout(struct timer_list *t)
+{
+	struct pn532_uart_phy *dev = from_timer(dev, t, cmd_timeout);
+
+	pn532_uart_send_frame(dev->priv, dev->cur_out_buf);
+}
+
+/*
+ * scans the buffer if it contains a pn532 frame. It is not checked if the
+ * frame is really valid. This is later done with pn533_rx_frame_is_valid.
+ * This is useful for malformed or errornous transmitted frames. Adjusts the
+ * bufferposition where the frame starts, since pn533_recv_frame expects a
+ * well formed frame.
+ */
+static int pn532_uart_rx_is_frame(struct sk_buff *skb)
+{
+	int i;
+	u16 frame_len;
+	struct pn533_std_frame *std;
+	struct pn533_ext_frame *ext;
+
+	for (i = 0; i + PN533_STD_FRAME_ACK_SIZE <= skb->len; i++) {
+		std = (struct pn533_std_frame *)&skb->data[i];
+		/* search start code */
+		if (std->start_frame != cpu_to_be16(PN533_STD_FRAME_SOF))
+			continue;
+
+		/* frame type */
+		switch (std->datalen) {
+		case PN533_FRAME_DATALEN_ACK:
+			if (std->datalen_checksum == 0xff) {
+				skb_pull(skb, i);
+				return 1;
+			}
+
+			break;
+		case PN533_FRAME_DATALEN_ERROR:
+			if ((std->datalen_checksum == 0xff) &&
+					(skb->len >=
+					 PN533_STD_ERROR_FRAME_SIZE)) {
+				skb_pull(skb, i);
+				return 1;
+			}
+
+			break;
+		case PN533_FRAME_DATALEN_EXTENDED:
+			ext = (struct pn533_ext_frame *)&skb->data[i];
+			frame_len = ext->datalen;
+			if (skb->len >= frame_len +
+					sizeof(struct pn533_ext_frame) +
+					2 /* CKS + Postamble */) {
+				skb_pull(skb, i);
+				return 1;
+			}
+
+			break;
+		default: /* normal information frame */
+			frame_len = std->datalen;
+			if (skb->len >= frame_len +
+					sizeof(struct pn533_std_frame) +
+					2 /* CKS + Postamble */) {
+				skb_pull(skb, i);
+				return 1;
+			}
+
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static int pn532_receive_buf(struct serdev_device *serdev,
+		const unsigned char *data, size_t count)
+{
+	struct pn532_uart_phy *dev = serdev_device_get_drvdata(serdev);
+	size_t i;
+
+	del_timer(&dev->cmd_timeout);
+	for (i = 0; i < count; i++) {
+		skb_put_u8(dev->recv_skb, *data++);
+		if (!pn532_uart_rx_is_frame(dev->recv_skb))
+			continue;
+
+		pn533_recv_frame(dev->priv, dev->recv_skb, 0);
+		dev->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL);
+		if (!dev->recv_skb)
+			return 0;
+	}
+
+	return i;
+}
+
+static struct serdev_device_ops pn532_serdev_ops = {
+	.receive_buf = pn532_receive_buf,
+	.write_wakeup = serdev_device_write_wakeup,
+};
+
+static const struct of_device_id pn532_uart_of_match[] = {
+	{ .compatible = "nxp,pn532", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, pn532_uart_of_match);
+
+static int pn532_uart_probe(struct serdev_device *serdev)
+{
+	struct pn532_uart_phy *pn532;
+	struct pn533 *priv;
+	int err;
+
+	err = -ENOMEM;
+	pn532 = kzalloc(sizeof(*pn532), GFP_KERNEL);
+	if (!pn532)
+		goto err_exit;
+
+	pn532->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL);
+	if (!pn532->recv_skb)
+		goto err_free;
+
+	pn532->serdev = serdev;
+	serdev_device_set_drvdata(serdev, pn532);
+	serdev_device_set_client_ops(serdev, &pn532_serdev_ops);
+	err = serdev_device_open(serdev);
+	if (err) {
+		dev_err(&serdev->dev, "Unable to open device\n");
+		goto err_skb;
+	}
+
+	err = serdev_device_set_baudrate(serdev, 115200);
+	if (err != 115200) {
+		err = -EINVAL;
+		goto err_serdev;
+	}
+
+	serdev_device_set_flow_control(serdev, false);
+	pn532->send_wakeup = PN532_SEND_WAKEUP;
+	timer_setup(&pn532->cmd_timeout, pn532_cmd_timeout, 0);
+	priv = pn53x_common_init(PN533_DEVICE_PN532,
+				     PN533_PROTO_REQ_ACK_RESP,
+				     pn532, &uart_phy_ops, NULL,
+				     &pn532->serdev->dev);
+	if (IS_ERR(priv)) {
+		err = PTR_ERR(priv);
+		goto err_serdev;
+	}
+
+	pn532->priv = priv;
+	err = pn533_finalize_setup(pn532->priv);
+	if (err)
+		goto err_clean;
+
+	serdev_device_close(serdev);
+	err = pn53x_register_nfc(priv, PN533_NO_TYPE_B_PROTOCOLS, &serdev->dev);
+	if (err) {
+		pn53x_common_clean(pn532->priv);
+		goto err_skb;
+	}
+
+	return err;
+
+err_clean:
+	pn53x_common_clean(pn532->priv);
+err_serdev:
+	serdev_device_close(serdev);
+err_skb:
+	kfree_skb(pn532->recv_skb);
+err_free:
+	kfree(pn532);
+err_exit:
+	return err;
+}
+
+static void pn532_uart_remove(struct serdev_device *serdev)
+{
+	struct pn532_uart_phy *pn532 = serdev_device_get_drvdata(serdev);
+
+	pn53x_unregister_nfc(pn532->priv);
+	serdev_device_close(serdev);
+	pn53x_common_clean(pn532->priv);
+	kfree_skb(pn532->recv_skb);
+	kfree(pn532);
+}
+
+static struct serdev_device_driver pn532_uart_driver = {
+	.probe = pn532_uart_probe,
+	.remove = pn532_uart_remove,
+	.driver = {
+		.name = "pn532_uart",
+		.of_match_table = of_match_ptr(pn532_uart_of_match),
+	},
+};
+
+module_serdev_device_driver(pn532_uart_driver);
+
+MODULE_AUTHOR("Lars Pöschel <poeschel@lemonage.de>");
+MODULE_DESCRIPTION("PN532 UART driver");
+MODULE_LICENSE("GPL");
-- 
2.23.0.rc1

Re: [PATCH v6 5/7] nfc: pn533: add UART phy driver

[Claudiu.Beznea]

Hi Lars,

On 20.08.2019 15:03, Lars Poeschel wrote:
> This adds the UART phy interface for the pn533 driver.
> The pn533 driver can be used through UART interface this way.
> It is implemented as a serdev device.
> 
> Cc: Johan Hovold <johan@kernel.org>
> Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> ---
> Changes in v6:
> - Rebased the patch series on v5.3-rc5
> 
> Changes in v5:
> - Use the splitted pn53x_common_init and pn53x_register_nfc
>   and pn53x_common_clean and pn53x_unregister_nfc alike
> 
> Changes in v4:
> - SPDX-License-Identifier: GPL-2.0+
> - Source code comments above refering items
> - Error check for serdev_device_write's
> - Change if (xxx == NULL) to if (!xxx)
> - Remove device name from a dev_err
> - move pn533_register in _probe a bit towards the end of _probe
> - make use of newly added dev_up / dev_down phy_ops
> - control send_wakeup variable from dev_up / dev_down
> 
> Changes in v3:
> - depend on SERIAL_DEV_BUS in Kconfig
> 
> Changes in v2:
> - switched from tty line discipline to serdev, resulting in many
>   simplifications
> - SPDX License Identifier
> 
>  drivers/nfc/pn533/Kconfig  |  11 ++
>  drivers/nfc/pn533/Makefile |   2 +
>  drivers/nfc/pn533/pn533.h  |   8 +
>  drivers/nfc/pn533/uart.c   | 316 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 337 insertions(+)
>  create mode 100644 drivers/nfc/pn533/uart.c
> 
> diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig
> index f6d6b345ba0d..7fe1bbe26568 100644
> --- a/drivers/nfc/pn533/Kconfig
> +++ b/drivers/nfc/pn533/Kconfig
> @@ -26,3 +26,14 @@ config NFC_PN533_I2C
>  
>  	  If you choose to build a module, it'll be called pn533_i2c.
>  	  Say N if unsure.
> +
> +config NFC_PN532_UART
> +	tristate "NFC PN532 device support (UART)"
> +	depends on SERIAL_DEV_BUS
> +	select NFC_PN533
> +	---help---
> +	  This module adds support for the NXP pn532 UART interface.
> +	  Select this if your platform is using the UART bus.
> +
> +	  If you choose to build a module, it'll be called pn532_uart.
> +	  Say N if unsure.
> diff --git a/drivers/nfc/pn533/Makefile b/drivers/nfc/pn533/Makefile
> index 43c25b4f9466..b9648337576f 100644
> --- a/drivers/nfc/pn533/Makefile
> +++ b/drivers/nfc/pn533/Makefile
> @@ -4,7 +4,9 @@
>  #
>  pn533_usb-objs  = usb.o
>  pn533_i2c-objs  = i2c.o
> +pn532_uart-objs  = uart.o
>  
>  obj-$(CONFIG_NFC_PN533)     += pn533.o
>  obj-$(CONFIG_NFC_PN533_USB) += pn533_usb.o
>  obj-$(CONFIG_NFC_PN533_I2C) += pn533_i2c.o
> +obj-$(CONFIG_NFC_PN532_UART) += pn532_uart.o
> diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
> index 510ddebbd896..6541088fad73 100644
> --- a/drivers/nfc/pn533/pn533.h
> +++ b/drivers/nfc/pn533/pn533.h
> @@ -43,6 +43,11 @@
>  
>  /* Preamble (1), SoPC (2), ACK Code (2), Postamble (1) */
>  #define PN533_STD_FRAME_ACK_SIZE 6
> +/*
> + * Preamble (1), SoPC (2), Packet Length (1), Packet Length Checksum (1),
> + * Specific Application Level Error Code (1) , Postamble (1)
> + */
> +#define PN533_STD_ERROR_FRAME_SIZE 8
>  #define PN533_STD_FRAME_CHECKSUM(f) (f->data[f->datalen])
>  #define PN533_STD_FRAME_POSTAMBLE(f) (f->data[f->datalen + 1])
>  /* Half start code (3), LEN (4) should be 0xffff for extended frame */
> @@ -84,6 +89,9 @@
>  #define PN533_CMD_MI_MASK 0x40
>  #define PN533_CMD_RET_SUCCESS 0x00
>  
> +#define PN533_FRAME_DATALEN_ACK 0x00
> +#define PN533_FRAME_DATALEN_ERROR 0x01
> +#define PN533_FRAME_DATALEN_EXTENDED 0xFF
>  
>  enum  pn533_protocol_type {
>  	PN533_PROTO_REQ_ACK_RESP = 0,
> diff --git a/drivers/nfc/pn533/uart.c b/drivers/nfc/pn533/uart.c
> new file mode 100644
> index 000000000000..f1cc2354a4fd
> --- /dev/null
> +++ b/drivers/nfc/pn533/uart.c
> @@ -0,0 +1,316 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for NXP PN532 NFC Chip - UART transport layer
> + *
> + * Copyright (C) 2018 Lemonage Software GmbH
> + * Author: Lars Pöschel <poeschel@lemonage.de>
> + * All rights reserved.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/nfc.h>
> +#include <linux/netdevice.h>
> +#include <linux/of.h>
> +#include <linux/serdev.h>
> +#include "pn533.h"
> +
> +#define PN532_UART_SKB_BUFF_LEN	(PN533_CMD_DATAEXCH_DATA_MAXLEN * 2)
> +
> +enum send_wakeup {
> +	PN532_SEND_NO_WAKEUP = 0,
> +	PN532_SEND_WAKEUP,
> +	PN532_SEND_LAST_WAKEUP,
> +};
> +
> +
> +struct pn532_uart_phy {
> +	struct serdev_device *serdev;
> +	struct sk_buff *recv_skb;
> +	struct pn533 *priv;
> +	enum send_wakeup send_wakeup;

Could there be any concurrency issues w/ regards to accessing this
variable? I see it is accessed in pn532_uart_send_frame(), pn532_dev_up(),
pn532_dev_down() which may be called from the following wq:

        INIT_WORK(&priv->mi_tm_rx_work, pn533_wq_tm_mi_recv);

        INIT_WORK(&priv->mi_tm_tx_work, pn533_wq_tm_mi_send);

        INIT_DELAYED_WORK(&priv->poll_work, pn533_wq_poll);


and from net/nfc/core.c via dev_up()/dev_down().

> +	struct timer_list cmd_timeout;
> +	struct sk_buff *cur_out_buf;
> +};
> +
> +static int pn532_uart_send_frame(struct pn533 *dev,
> +				struct sk_buff *out)
> +{
> +	/* wakeup sequence and dummy bytes for waiting time */
> +	static const u8 wakeup[] = {
> +		0x55, 0x55, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
> +	struct pn532_uart_phy *pn532 = dev->phy;
> +	int err;
> +
> +	print_hex_dump_debug("PN532_uart TX: ", DUMP_PREFIX_NONE, 16, 1,
> +			     out->data, out->len, false);
> +
> +	pn532->cur_out_buf = out;
> +	if (pn532->send_wakeup) {
> +		err= serdev_device_write(pn532->serdev,
> +				wakeup, sizeof(wakeup),
> +				MAX_SCHEDULE_TIMEOUT);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	if (pn532->send_wakeup == PN532_SEND_LAST_WAKEUP) {
> +		pn532->send_wakeup = PN532_SEND_NO_WAKEUP;
> +	}
> +
> +	err = serdev_device_write(pn532->serdev, out->data, out->len,
> +			MAX_SCHEDULE_TIMEOUT);
> +	if (err < 0)
> +		return err;
> +
> +	mod_timer(&pn532->cmd_timeout, HZ / 40 + jiffies);
> +	return 0;
> +}
> +
> +static int pn532_uart_send_ack(struct pn533 *dev, gfp_t flags)
> +{
> +	struct pn532_uart_phy *pn532 = dev->phy;
> +	/* spec 7.1.1.3:  Preamble, SoPC (2), ACK Code (2), Postamble */
> +	static const u8 ack[PN533_STD_FRAME_ACK_SIZE] = {
> +			0x00, 0x00, 0xff, 0x00, 0xff, 0x00};
> +	int err;
> +
> +	err = serdev_device_write(pn532->serdev, ack, sizeof(ack),
> +			MAX_SCHEDULE_TIMEOUT);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static void pn532_uart_abort_cmd(struct pn533 *dev, gfp_t flags)
> +{
> +	/* An ack will cancel the last issued command */
> +	pn532_uart_send_ack(dev, flags);
> +	/* schedule cmd_complete_work to finish current command execution */
> +	pn533_recv_frame(dev, NULL, -ENOENT);
> +}
> +
> +static void pn532_dev_up(struct pn533 *dev)
> +{
> +	struct pn532_uart_phy *pn532 = dev->phy;
> +
> +	serdev_device_open(pn532->serdev);
> +	pn532->send_wakeup = PN532_SEND_LAST_WAKEUP;
> +}
> +
> +static void pn532_dev_down(struct pn533 *dev)
> +{
> +	struct pn532_uart_phy *pn532 = dev->phy;
> +
> +	serdev_device_close(pn532->serdev);
> +	pn532->send_wakeup = PN532_SEND_WAKEUP;
> +}
> +
> +static struct pn533_phy_ops uart_phy_ops = {
> +	.send_frame = pn532_uart_send_frame,
> +	.send_ack = pn532_uart_send_ack,
> +	.abort_cmd = pn532_uart_abort_cmd,
> +	.dev_up = pn532_dev_up,
> +	.dev_down = pn532_dev_down,
> +};
> +
> +static void pn532_cmd_timeout(struct timer_list *t)
> +{
> +	struct pn532_uart_phy *dev = from_timer(dev, t, cmd_timeout);
> +
> +	pn532_uart_send_frame(dev->priv, dev->cur_out_buf);
> +}
> +
> +/*
> + * scans the buffer if it contains a pn532 frame. It is not checked if the
> + * frame is really valid. This is later done with pn533_rx_frame_is_valid.
> + * This is useful for malformed or errornous transmitted frames. Adjusts the
> + * bufferposition where the frame starts, since pn533_recv_frame expects a
> + * well formed frame.
> + */
> +static int pn532_uart_rx_is_frame(struct sk_buff *skb)
> +{
> +	int i;
> +	u16 frame_len;
> +	struct pn533_std_frame *std;
> +	struct pn533_ext_frame *ext;
> +
> +	for (i = 0; i + PN533_STD_FRAME_ACK_SIZE <= skb->len; i++) {
> +		std = (struct pn533_std_frame *)&skb->data[i];
> +		/* search start code */
> +		if (std->start_frame != cpu_to_be16(PN533_STD_FRAME_SOF))
> +			continue;
> +
> +		/* frame type */
> +		switch (std->datalen) {
> +		case PN533_FRAME_DATALEN_ACK:
> +			if (std->datalen_checksum == 0xff) {
> +				skb_pull(skb, i);
> +				return 1;
> +			}
> +
> +			break;
> +		case PN533_FRAME_DATALEN_ERROR:
> +			if ((std->datalen_checksum == 0xff) &&
> +					(skb->len >=
> +					 PN533_STD_ERROR_FRAME_SIZE)) {
> +				skb_pull(skb, i);
> +				return 1;
> +			}
> +
> +			break;
> +		case PN533_FRAME_DATALEN_EXTENDED:
> +			ext = (struct pn533_ext_frame *)&skb->data[i];
> +			frame_len = ext->datalen;
> +			if (skb->len >= frame_len +
> +					sizeof(struct pn533_ext_frame) +
> +					2 /* CKS + Postamble */) {
> +				skb_pull(skb, i);
> +				return 1;
> +			}
> +
> +			break;
> +		default: /* normal information frame */
> +			frame_len = std->datalen;
> +			if (skb->len >= frame_len +
> +					sizeof(struct pn533_std_frame) +
> +					2 /* CKS + Postamble */) {
> +				skb_pull(skb, i);
> +				return 1;
> +			}
> +
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int pn532_receive_buf(struct serdev_device *serdev,
> +		const unsigned char *data, size_t count)
> +{
> +	struct pn532_uart_phy *dev = serdev_device_get_drvdata(serdev);
> +	size_t i;
> +
> +	del_timer(&dev->cmd_timeout);
> +	for (i = 0; i < count; i++) {
> +		skb_put_u8(dev->recv_skb, *data++);
> +		if (!pn532_uart_rx_is_frame(dev->recv_skb))
> +			continue;
> +
> +		pn533_recv_frame(dev->priv, dev->recv_skb, 0);
> +		dev->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL);
> +		if (!dev->recv_skb)
> +			return 0;
> +	}
> +
> +	return i;
> +}
> +
> +static struct serdev_device_ops pn532_serdev_ops = {
> +	.receive_buf = pn532_receive_buf,
> +	.write_wakeup = serdev_device_write_wakeup,
> +};
> +
> +static const struct of_device_id pn532_uart_of_match[] = {
> +	{ .compatible = "nxp,pn532", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, pn532_uart_of_match);
> +
> +static int pn532_uart_probe(struct serdev_device *serdev)
> +{
> +	struct pn532_uart_phy *pn532;
> +	struct pn533 *priv;
> +	int err;
> +
> +	err = -ENOMEM;
> +	pn532 = kzalloc(sizeof(*pn532), GFP_KERNEL);
> +	if (!pn532)
> +		goto err_exit;
> +
> +	pn532->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL);
> +	if (!pn532->recv_skb)
> +		goto err_free;
> +
> +	pn532->serdev = serdev;
> +	serdev_device_set_drvdata(serdev, pn532);
> +	serdev_device_set_client_ops(serdev, &pn532_serdev_ops);
> +	err = serdev_device_open(serdev);
> +	if (err) {
> +		dev_err(&serdev->dev, "Unable to open device\n");
> +		goto err_skb;
> +	}
> +
> +	err = serdev_device_set_baudrate(serdev, 115200);
> +	if (err != 115200) {
> +		err = -EINVAL;
> +		goto err_serdev;
> +	}
> +
> +	serdev_device_set_flow_control(serdev, false);
> +	pn532->send_wakeup = PN532_SEND_WAKEUP;
> +	timer_setup(&pn532->cmd_timeout, pn532_cmd_timeout, 0);
> +	priv = pn53x_common_init(PN533_DEVICE_PN532,
> +				     PN533_PROTO_REQ_ACK_RESP,
> +				     pn532, &uart_phy_ops, NULL,
> +				     &pn532->serdev->dev);
> +	if (IS_ERR(priv)) {
> +		err = PTR_ERR(priv);
> +		goto err_serdev;
> +	}
> +
> +	pn532->priv = priv;
> +	err = pn533_finalize_setup(pn532->priv);
> +	if (err)
> +		goto err_clean;
> +
> +	serdev_device_close(serdev);
> +	err = pn53x_register_nfc(priv, PN533_NO_TYPE_B_PROTOCOLS, &serdev->dev);
> +	if (err) {
> +		pn53x_common_clean(pn532->priv);
> +		goto err_skb;
> +	}
> +
> +	return err;
> +
> +err_clean:
> +	pn53x_common_clean(pn532->priv);
> +err_serdev:
> +	serdev_device_close(serdev);
> +err_skb:
> +	kfree_skb(pn532->recv_skb);
> +err_free:
> +	kfree(pn532);
> +err_exit:
> +	return err;
> +}
> +
> +static void pn532_uart_remove(struct serdev_device *serdev)
> +{
> +	struct pn532_uart_phy *pn532 = serdev_device_get_drvdata(serdev);
> +
> +	pn53x_unregister_nfc(pn532->priv);
> +	serdev_device_close(serdev);
> +	pn53x_common_clean(pn532->priv);
> +	kfree_skb(pn532->recv_skb);
> +	kfree(pn532);
> +}
> +
> +static struct serdev_device_driver pn532_uart_driver = {
> +	.probe = pn532_uart_probe,
> +	.remove = pn532_uart_remove,
> +	.driver = {
> +		.name = "pn532_uart",
> +		.of_match_table = of_match_ptr(pn532_uart_of_match),
> +	},
> +};
> +
> +module_serdev_device_driver(pn532_uart_driver);
> +
> +MODULE_AUTHOR("Lars Pöschel <poeschel@lemonage.de>");
> +MODULE_DESCRIPTION("PN532 UART driver");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH v6 5/7] nfc: pn533: add UART phy driver

[Lars Poeschel]

On Thu, Aug 22, 2019 at 10:09:09AM +0000, Claudiu.Beznea@microchip.com wrote:
> Hi Lars,
> 
> On 20.08.2019 15:03, Lars Poeschel wrote:
> > This adds the UART phy interface for the pn533 driver.
> > The pn533 driver can be used through UART interface this way.
> > It is implemented as a serdev device.
> > 
> > Cc: Johan Hovold <johan@kernel.org>
> > Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> > ---
> > Changes in v6:
> > - Rebased the patch series on v5.3-rc5
> > 
> > Changes in v5:
> > - Use the splitted pn53x_common_init and pn53x_register_nfc
> >   and pn53x_common_clean and pn53x_unregister_nfc alike
> > 
> > Changes in v4:
> > - SPDX-License-Identifier: GPL-2.0+
> > - Source code comments above refering items
> > - Error check for serdev_device_write's
> > - Change if (xxx == NULL) to if (!xxx)
> > - Remove device name from a dev_err
> > - move pn533_register in _probe a bit towards the end of _probe
> > - make use of newly added dev_up / dev_down phy_ops
> > - control send_wakeup variable from dev_up / dev_down
> > 
> > Changes in v3:
> > - depend on SERIAL_DEV_BUS in Kconfig
> > 
> > Changes in v2:
> > - switched from tty line discipline to serdev, resulting in many
> >   simplifications
> > - SPDX License Identifier
> > 
> >  drivers/nfc/pn533/Kconfig  |  11 ++
> >  drivers/nfc/pn533/Makefile |   2 +
> >  drivers/nfc/pn533/pn533.h  |   8 +
> >  drivers/nfc/pn533/uart.c   | 316 +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 337 insertions(+)
> >  create mode 100644 drivers/nfc/pn533/uart.c
> > 
> > diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig
> > index f6d6b345ba0d..7fe1bbe26568 100644
> > --- a/drivers/nfc/pn533/Kconfig
> > +++ b/drivers/nfc/pn533/Kconfig
> > @@ -26,3 +26,14 @@ config NFC_PN533_I2C
> >  
> >  	  If you choose to build a module, it'll be called pn533_i2c.
> >  	  Say N if unsure.
> > +
> > +config NFC_PN532_UART
> > +	tristate "NFC PN532 device support (UART)"
> > +	depends on SERIAL_DEV_BUS
> > +	select NFC_PN533
> > +	---help---
> > +	  This module adds support for the NXP pn532 UART interface.
> > +	  Select this if your platform is using the UART bus.
> > +
> > +	  If you choose to build a module, it'll be called pn532_uart.
> > +	  Say N if unsure.
> > diff --git a/drivers/nfc/pn533/Makefile b/drivers/nfc/pn533/Makefile
> > index 43c25b4f9466..b9648337576f 100644
> > --- a/drivers/nfc/pn533/Makefile
> > +++ b/drivers/nfc/pn533/Makefile
> > @@ -4,7 +4,9 @@
> >  #
> >  pn533_usb-objs  = usb.o
> >  pn533_i2c-objs  = i2c.o
> > +pn532_uart-objs  = uart.o
> >  
> >  obj-$(CONFIG_NFC_PN533)     += pn533.o
> >  obj-$(CONFIG_NFC_PN533_USB) += pn533_usb.o
> >  obj-$(CONFIG_NFC_PN533_I2C) += pn533_i2c.o
> > +obj-$(CONFIG_NFC_PN532_UART) += pn532_uart.o
> > diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
> > index 510ddebbd896..6541088fad73 100644
> > --- a/drivers/nfc/pn533/pn533.h
> > +++ b/drivers/nfc/pn533/pn533.h
> > @@ -43,6 +43,11 @@
> >  
> >  /* Preamble (1), SoPC (2), ACK Code (2), Postamble (1) */
> >  #define PN533_STD_FRAME_ACK_SIZE 6
> > +/*
> > + * Preamble (1), SoPC (2), Packet Length (1), Packet Length Checksum (1),
> > + * Specific Application Level Error Code (1) , Postamble (1)
> > + */
> > +#define PN533_STD_ERROR_FRAME_SIZE 8
> >  #define PN533_STD_FRAME_CHECKSUM(f) (f->data[f->datalen])
> >  #define PN533_STD_FRAME_POSTAMBLE(f) (f->data[f->datalen + 1])
> >  /* Half start code (3), LEN (4) should be 0xffff for extended frame */
> > @@ -84,6 +89,9 @@
> >  #define PN533_CMD_MI_MASK 0x40
> >  #define PN533_CMD_RET_SUCCESS 0x00
> >  
> > +#define PN533_FRAME_DATALEN_ACK 0x00
> > +#define PN533_FRAME_DATALEN_ERROR 0x01
> > +#define PN533_FRAME_DATALEN_EXTENDED 0xFF
> >  
> >  enum  pn533_protocol_type {
> >  	PN533_PROTO_REQ_ACK_RESP = 0,
> > diff --git a/drivers/nfc/pn533/uart.c b/drivers/nfc/pn533/uart.c
> > new file mode 100644
> > index 000000000000..f1cc2354a4fd
> > --- /dev/null
> > +++ b/drivers/nfc/pn533/uart.c
> > @@ -0,0 +1,316 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Driver for NXP PN532 NFC Chip - UART transport layer
> > + *
> > + * Copyright (C) 2018 Lemonage Software GmbH
> > + * Author: Lars Pöschel <poeschel@lemonage.de>
> > + * All rights reserved.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/nfc.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/of.h>
> > +#include <linux/serdev.h>
> > +#include "pn533.h"
> > +
> > +#define PN532_UART_SKB_BUFF_LEN	(PN533_CMD_DATAEXCH_DATA_MAXLEN * 2)
> > +
> > +enum send_wakeup {
> > +	PN532_SEND_NO_WAKEUP = 0,
> > +	PN532_SEND_WAKEUP,
> > +	PN532_SEND_LAST_WAKEUP,
> > +};
> > +
> > +
> > +struct pn532_uart_phy {
> > +	struct serdev_device *serdev;
> > +	struct sk_buff *recv_skb;
> > +	struct pn533 *priv;
> > +	enum send_wakeup send_wakeup;
> 
> Could there be any concurrency issues w/ regards to accessing this
> variable? I see it is accessed in pn532_uart_send_frame(), pn532_dev_up(),
> pn532_dev_down() which may be called from the following wq:
> 
>         INIT_WORK(&priv->mi_tm_rx_work, pn533_wq_tm_mi_recv);
> 
>         INIT_WORK(&priv->mi_tm_tx_work, pn533_wq_tm_mi_send);
> 
>         INIT_DELAYED_WORK(&priv->poll_work, pn533_wq_poll);
> 
> 
> and from net/nfc/core.c via dev_up()/dev_down().

Well, I spend some minutes thinking about this. There should be no real
problem. The code in pn533.c ensures, that commands are transmitted
sequencially. And it always is command - response. So if a command is
send, the driver waits for a response from the chip.
So pn532_uart_send_frame should not be called multiple times without
reaching at least serdev_device_write, but at this point the race is
already over.
There is one exception, this is the abort command. This command can be
sent without receiving a previous response. So there is the possibility
of a successful race.
The send_wakeup variable is used to control if we need to send a
wakeup request to the pn532 chip prior to the actual command we would
like to send.
Worst thing that I see could happen - if the race succeeds - is that we
send a wakeup to the chip that is propably not needed as it is already
awake. But this does not hurt as a wakeup send to the pn532 is
essentially a no-op if the chip is awake already. I could have
implemented it so, that a wakeup is sent in front of every command
without thinking and the driver would work.
The same is with pn532_dev_up. It could be that there is one wakeup sent
to much, but it does not hurt.
pn532_dev_down is not problematic I think.

To sum it up: There is maybe a very little probability, but it does
nothing bad. Question is now: Is it worth mutex'ing the send_wakeup
variable or can we leave it as-is ?

Thank you for your review, Claudiu.
Regards,
Lars

Re: [PATCH v6 5/7] nfc: pn533: add UART phy driver

[Claudiu.Beznea]

Hi Lars,

On 23.08.2019 13:06, Lars Poeschel wrote:
> External E-Mail
> 
> 
> On Thu, Aug 22, 2019 at 10:09:09AM +0000, Claudiu.Beznea@microchip.com wrote:
>> Hi Lars,
>>
>> On 20.08.2019 15:03, Lars Poeschel wrote:
>>> This adds the UART phy interface for the pn533 driver.
>>> The pn533 driver can be used through UART interface this way.
>>> It is implemented as a serdev device.
>>>
>>> Cc: Johan Hovold <johan@kernel.org>
>>> Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
>>> ---
>>> Changes in v6:
>>> - Rebased the patch series on v5.3-rc5
>>>
>>> Changes in v5:
>>> - Use the splitted pn53x_common_init and pn53x_register_nfc
>>>   and pn53x_common_clean and pn53x_unregister_nfc alike
>>>
>>> Changes in v4:
>>> - SPDX-License-Identifier: GPL-2.0+
>>> - Source code comments above refering items
>>> - Error check for serdev_device_write's
>>> - Change if (xxx == NULL) to if (!xxx)
>>> - Remove device name from a dev_err
>>> - move pn533_register in _probe a bit towards the end of _probe
>>> - make use of newly added dev_up / dev_down phy_ops
>>> - control send_wakeup variable from dev_up / dev_down
>>>
>>> Changes in v3:
>>> - depend on SERIAL_DEV_BUS in Kconfig
>>>
>>> Changes in v2:
>>> - switched from tty line discipline to serdev, resulting in many
>>>   simplifications
>>> - SPDX License Identifier
>>>
>>>  drivers/nfc/pn533/Kconfig  |  11 ++
>>>  drivers/nfc/pn533/Makefile |   2 +
>>>  drivers/nfc/pn533/pn533.h  |   8 +
>>>  drivers/nfc/pn533/uart.c   | 316 +++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 337 insertions(+)
>>>  create mode 100644 drivers/nfc/pn533/uart.c
>>>
>>> diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig
>>> index f6d6b345ba0d..7fe1bbe26568 100644
>>> --- a/drivers/nfc/pn533/Kconfig
>>> +++ b/drivers/nfc/pn533/Kconfig
>>> @@ -26,3 +26,14 @@ config NFC_PN533_I2C
>>>  
>>>  	  If you choose to build a module, it'll be called pn533_i2c.
>>>  	  Say N if unsure.
>>> +
>>> +config NFC_PN532_UART
>>> +	tristate "NFC PN532 device support (UART)"
>>> +	depends on SERIAL_DEV_BUS
>>> +	select NFC_PN533
>>> +	---help---
>>> +	  This module adds support for the NXP pn532 UART interface.
>>> +	  Select this if your platform is using the UART bus.
>>> +
>>> +	  If you choose to build a module, it'll be called pn532_uart.
>>> +	  Say N if unsure.
>>> diff --git a/drivers/nfc/pn533/Makefile b/drivers/nfc/pn533/Makefile
>>> index 43c25b4f9466..b9648337576f 100644
>>> --- a/drivers/nfc/pn533/Makefile
>>> +++ b/drivers/nfc/pn533/Makefile
>>> @@ -4,7 +4,9 @@
>>>  #
>>>  pn533_usb-objs  = usb.o
>>>  pn533_i2c-objs  = i2c.o
>>> +pn532_uart-objs  = uart.o
>>>  
>>>  obj-$(CONFIG_NFC_PN533)     += pn533.o
>>>  obj-$(CONFIG_NFC_PN533_USB) += pn533_usb.o
>>>  obj-$(CONFIG_NFC_PN533_I2C) += pn533_i2c.o
>>> +obj-$(CONFIG_NFC_PN532_UART) += pn532_uart.o
>>> diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
>>> index 510ddebbd896..6541088fad73 100644
>>> --- a/drivers/nfc/pn533/pn533.h
>>> +++ b/drivers/nfc/pn533/pn533.h
>>> @@ -43,6 +43,11 @@
>>>  
>>>  /* Preamble (1), SoPC (2), ACK Code (2), Postamble (1) */
>>>  #define PN533_STD_FRAME_ACK_SIZE 6
>>> +/*
>>> + * Preamble (1), SoPC (2), Packet Length (1), Packet Length Checksum (1),
>>> + * Specific Application Level Error Code (1) , Postamble (1)
>>> + */
>>> +#define PN533_STD_ERROR_FRAME_SIZE 8
>>>  #define PN533_STD_FRAME_CHECKSUM(f) (f->data[f->datalen])
>>>  #define PN533_STD_FRAME_POSTAMBLE(f) (f->data[f->datalen + 1])
>>>  /* Half start code (3), LEN (4) should be 0xffff for extended frame */
>>> @@ -84,6 +89,9 @@
>>>  #define PN533_CMD_MI_MASK 0x40
>>>  #define PN533_CMD_RET_SUCCESS 0x00
>>>  
>>> +#define PN533_FRAME_DATALEN_ACK 0x00
>>> +#define PN533_FRAME_DATALEN_ERROR 0x01
>>> +#define PN533_FRAME_DATALEN_EXTENDED 0xFF
>>>  
>>>  enum  pn533_protocol_type {
>>>  	PN533_PROTO_REQ_ACK_RESP = 0,
>>> diff --git a/drivers/nfc/pn533/uart.c b/drivers/nfc/pn533/uart.c
>>> new file mode 100644
>>> index 000000000000..f1cc2354a4fd
>>> --- /dev/null
>>> +++ b/drivers/nfc/pn533/uart.c
>>> @@ -0,0 +1,316 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Driver for NXP PN532 NFC Chip - UART transport layer
>>> + *
>>> + * Copyright (C) 2018 Lemonage Software GmbH
>>> + * Author: Lars Pöschel <poeschel@lemonage.de>
>>> + * All rights reserved.
>>> + */
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/nfc.h>
>>> +#include <linux/netdevice.h>
>>> +#include <linux/of.h>
>>> +#include <linux/serdev.h>
>>> +#include "pn533.h"
>>> +
>>> +#define PN532_UART_SKB_BUFF_LEN	(PN533_CMD_DATAEXCH_DATA_MAXLEN * 2)
>>> +
>>> +enum send_wakeup {
>>> +	PN532_SEND_NO_WAKEUP = 0,
>>> +	PN532_SEND_WAKEUP,
>>> +	PN532_SEND_LAST_WAKEUP,
>>> +};
>>> +
>>> +
>>> +struct pn532_uart_phy {
>>> +	struct serdev_device *serdev;
>>> +	struct sk_buff *recv_skb;
>>> +	struct pn533 *priv;
>>> +	enum send_wakeup send_wakeup;
>>
>> Could there be any concurrency issues w/ regards to accessing this
>> variable? I see it is accessed in pn532_uart_send_frame(), pn532_dev_up(),
>> pn532_dev_down() which may be called from the following wq:
>>
>>         INIT_WORK(&priv->mi_tm_rx_work, pn533_wq_tm_mi_recv);
>>
>>         INIT_WORK(&priv->mi_tm_tx_work, pn533_wq_tm_mi_send);
>>
>>         INIT_DELAYED_WORK(&priv->poll_work, pn533_wq_poll);
>>
>>
>> and from net/nfc/core.c via dev_up()/dev_down().
> 
> Well, I spend some minutes thinking about this. There should be no real
> problem. The code in pn533.c ensures, that commands are transmitted
> sequencially. And it always is command - response. So if a command is
> send, the driver waits for a response from the chip.
> So pn532_uart_send_frame should not be called multiple times without
> reaching at least serdev_device_write, but at this point the race is
> already over.
> There is one exception, this is the abort command. This command can be
> sent without receiving a previous response. So there is the possibility
> of a successful race.
> The send_wakeup variable is used to control if we need to send a
> wakeup request to the pn532 chip prior to the actual command we would
> like to send.
> Worst thing that I see could happen - if the race succeeds - is that we
> send a wakeup to the chip that is propably not needed as it is already
> awake. But this does not hurt as a wakeup send to the pn532 is
> essentially a no-op if the chip is awake already. I could have
> implemented it so, that a wakeup is sent in front of every command
> without thinking and the driver would work.
> The same is with pn532_dev_up. It could be that there is one wakeup sent
> to much, but it does not hurt.
> pn532_dev_down is not problematic I think.
> 
> To sum it up: There is maybe a very little probability, but it does
> nothing bad. Question is now: Is it worth mutex'ing the send_wakeup
> variable or can we leave it as-is ?

Being so as you described above, I am for leaving it as is. Maybe, as you
wish, document this somewhere (e.g. a comment in the code), so that others
to be aware of this.

Thank you,
Claudiu Beznea

> 
> Thank you for your review, Claudiu.
> Regards,
> Lars
> 
> 

[PATCH v6 6/7] nfc: pn533: Add autopoll capability

[Lars Poeschel]

pn532 devices support an autopoll command, that lets the chip
automatically poll for selected nfc technologies instead of manually
looping through every single nfc technology the user is interested in.
This is faster and less cpu and bus intensive than manually polling.
This adds this autopoll capability to the pn533 driver.

Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v6:
- Rebased the patch series on v5.3-rc5

 drivers/nfc/pn533/pn533.c | 193 +++++++++++++++++++++++++++++++++++++-
 drivers/nfc/pn533/pn533.h |  10 +-
 2 files changed, 197 insertions(+), 6 deletions(-)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index a8c756caa678..7e915239222b 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -185,6 +185,32 @@ struct pn533_cmd_jump_dep_response {
 	u8 gt[];
 } __packed;
 
+struct pn532_autopoll_resp {
+	u8 type;
+	u8 ln;
+	u8 tg;
+	u8 tgdata[];
+} __packed;
+
+/* PN532_CMD_IN_AUTOPOLL */
+#define PN532_AUTOPOLL_POLLNR_INFINITE	0xff
+#define PN532_AUTOPOLL_PERIOD		0x03 /* in units of 150 ms */
+
+#define PN532_AUTOPOLL_TYPE_GENERIC_106		0x00
+#define PN532_AUTOPOLL_TYPE_GENERIC_212		0x01
+#define PN532_AUTOPOLL_TYPE_GENERIC_424		0x02
+#define PN532_AUTOPOLL_TYPE_JEWEL		0x04
+#define PN532_AUTOPOLL_TYPE_MIFARE		0x10
+#define PN532_AUTOPOLL_TYPE_FELICA212		0x11
+#define PN532_AUTOPOLL_TYPE_FELICA424		0x12
+#define PN532_AUTOPOLL_TYPE_ISOA		0x20
+#define PN532_AUTOPOLL_TYPE_ISOB		0x23
+#define PN532_AUTOPOLL_TYPE_DEP_PASSIVE_106	0x40
+#define PN532_AUTOPOLL_TYPE_DEP_PASSIVE_212	0x41
+#define PN532_AUTOPOLL_TYPE_DEP_PASSIVE_424	0x42
+#define PN532_AUTOPOLL_TYPE_DEP_ACTIVE_106	0x80
+#define PN532_AUTOPOLL_TYPE_DEP_ACTIVE_212	0x81
+#define PN532_AUTOPOLL_TYPE_DEP_ACTIVE_424	0x82
 
 /* PN533_TG_INIT_AS_TARGET */
 #define PN533_INIT_TARGET_PASSIVE 0x1
@@ -1389,6 +1415,101 @@ static int pn533_poll_dep(struct nfc_dev *nfc_dev)
 	return rc;
 }
 
+static int pn533_autopoll_complete(struct pn533 *dev, void *arg,
+			       struct sk_buff *resp)
+{
+	u8 nbtg;
+	int rc;
+	struct pn532_autopoll_resp *apr;
+	struct nfc_target nfc_tgt;
+
+	if (IS_ERR(resp)) {
+		rc = PTR_ERR(resp);
+
+		nfc_err(dev->dev, "%s  autopoll complete error %d\n",
+			__func__, rc);
+
+		if (rc == -ENOENT) {
+			if (dev->poll_mod_count != 0)
+				return rc;
+			goto stop_poll;
+		} else if (rc < 0) {
+			nfc_err(dev->dev,
+				"Error %d when running autopoll\n", rc);
+			goto stop_poll;
+		}
+	}
+
+	nbtg = resp->data[0];
+	if ((nbtg > 2) || (nbtg <= 0))
+		return -EAGAIN;
+
+	apr = (struct pn532_autopoll_resp *)&resp->data[1];
+	while (nbtg--) {
+		memset(&nfc_tgt, 0, sizeof(struct nfc_target));
+		switch (apr->type) {
+		case PN532_AUTOPOLL_TYPE_ISOA:
+			dev_dbg(dev->dev, "ISOA");
+			rc = pn533_target_found_type_a(&nfc_tgt, apr->tgdata,
+						       apr->ln - 1);
+			break;
+		case PN532_AUTOPOLL_TYPE_FELICA212:
+		case PN532_AUTOPOLL_TYPE_FELICA424:
+			dev_dbg(dev->dev, "FELICA");
+			rc = pn533_target_found_felica(&nfc_tgt, apr->tgdata,
+						       apr->ln - 1);
+			break;
+		case PN532_AUTOPOLL_TYPE_JEWEL:
+			dev_dbg(dev->dev, "JEWEL");
+			rc = pn533_target_found_jewel(&nfc_tgt, apr->tgdata,
+						      apr->ln - 1);
+			break;
+		case PN532_AUTOPOLL_TYPE_ISOB:
+			dev_dbg(dev->dev, "ISOB");
+			rc = pn533_target_found_type_b(&nfc_tgt, apr->tgdata,
+						       apr->ln - 1);
+			break;
+		case PN532_AUTOPOLL_TYPE_MIFARE:
+			dev_dbg(dev->dev, "Mifare");
+			rc = pn533_target_found_type_a(&nfc_tgt, apr->tgdata,
+						       apr->ln - 1);
+			break;
+		default:
+			nfc_err(dev->dev,
+				    "Unknown current poll modulation");
+			rc = -EPROTO;
+		}
+
+		if (rc)
+			goto done;
+
+		if (!(nfc_tgt.supported_protocols & dev->poll_protocols)) {
+			nfc_err(dev->dev,
+				    "The Tg found doesn't have the desired protocol");
+			rc = -EAGAIN;
+			goto done;
+		}
+
+		dev->tgt_available_prots = nfc_tgt.supported_protocols;
+		apr = (struct pn532_autopoll_resp *)
+			(apr->tgdata + (apr->ln - 1));
+	}
+
+	pn533_poll_reset_mod_list(dev);
+	nfc_targets_found(dev->nfc_dev, &nfc_tgt, 1);
+
+done:
+	dev_kfree_skb(resp);
+	return rc;
+
+stop_poll:
+	nfc_err(dev->dev, "autopoll operation has been stopped\n");
+
+	pn533_poll_reset_mod_list(dev);
+	dev->poll_protocols = 0;
+	return rc;
+}
+
 static int pn533_poll_complete(struct pn533 *dev, void *arg,
 			       struct sk_buff *resp)
 {
@@ -1534,6 +1655,7 @@ static int pn533_start_poll(struct nfc_dev *nfc_dev,
 	struct pn533_poll_modulations *cur_mod;
 	u8 rand_mod;
 	int rc;
+	struct sk_buff *skb;
 
 	dev_dbg(dev->dev,
 		"%s: im protocols 0x%x tm protocols 0x%x\n",
@@ -1557,9 +1679,73 @@ static int pn533_start_poll(struct nfc_dev *nfc_dev,
 			tm_protocols = 0;
 	}
 
-	pn533_poll_create_mod_list(dev, im_protocols, tm_protocols);
 	dev->poll_protocols = im_protocols;
 	dev->listen_protocols = tm_protocols;
+	if (dev->device_type == PN533_DEVICE_PN532_AUTOPOLL) {
+		skb = pn533_alloc_skb(dev, 4 + 6);
+		if (!skb)
+			return -ENOMEM;
+
+		*((u8 *)skb_put(skb, sizeof(u8))) =
+			PN532_AUTOPOLL_POLLNR_INFINITE;
+		*((u8 *)skb_put(skb, sizeof(u8))) = PN532_AUTOPOLL_PERIOD;
+
+		if ((im_protocols & NFC_PROTO_MIFARE_MASK) &&
+				(im_protocols & NFC_PROTO_ISO14443_MASK) &&
+				(im_protocols & NFC_PROTO_NFC_DEP_MASK))
+			*((u8 *)skb_put(skb, sizeof(u8))) =
+				PN532_AUTOPOLL_TYPE_GENERIC_106;
+		else {
+			if (im_protocols & NFC_PROTO_MIFARE_MASK)
+				*((u8 *)skb_put(skb, sizeof(u8))) =
+					PN532_AUTOPOLL_TYPE_MIFARE;
+
+			if (im_protocols & NFC_PROTO_ISO14443_MASK)
+				*((u8 *)skb_put(skb, sizeof(u8))) =
+					PN532_AUTOPOLL_TYPE_ISOA;
+
+			if (im_protocols & NFC_PROTO_NFC_DEP_MASK) {
+				*((u8 *)skb_put(skb, sizeof(u8))) =
+					PN532_AUTOPOLL_TYPE_DEP_PASSIVE_106;
+				*((u8 *)skb_put(skb, sizeof(u8))) =
+					PN532_AUTOPOLL_TYPE_DEP_PASSIVE_212;
+				*((u8 *)skb_put(skb, sizeof(u8))) =
+					PN532_AUTOPOLL_TYPE_DEP_PASSIVE_424;
+			}
+		}
+
+		if (im_protocols & NFC_PROTO_FELICA_MASK ||
+				im_protocols & NFC_PROTO_NFC_DEP_MASK) {
+			*((u8 *)skb_put(skb, sizeof(u8))) =
+				PN532_AUTOPOLL_TYPE_FELICA212;
+			*((u8 *)skb_put(skb, sizeof(u8))) =
+				PN532_AUTOPOLL_TYPE_FELICA424;
+		}
+
+		if (im_protocols & NFC_PROTO_JEWEL_MASK)
+			*((u8 *)skb_put(skb, sizeof(u8))) =
+				PN532_AUTOPOLL_TYPE_JEWEL;
+
+		if (im_protocols & NFC_PROTO_ISO14443_B_MASK)
+			*((u8 *)skb_put(skb, sizeof(u8))) =
+				PN532_AUTOPOLL_TYPE_ISOB;
+
+		if (tm_protocols)
+			*((u8 *)skb_put(skb, sizeof(u8))) =
+				PN532_AUTOPOLL_TYPE_DEP_ACTIVE_106;
+
+		rc = pn533_send_cmd_async(dev, PN533_CMD_IN_AUTOPOLL, skb,
+				pn533_autopoll_complete, NULL);
+
+		if (rc < 0)
+			dev_kfree_skb(skb);
+		else
+			dev->poll_mod_count++;
+
+		return rc;
+	}
+
+	pn533_poll_create_mod_list(dev, im_protocols, tm_protocols);
 
 	/* Do not always start polling from the same modulation */
 	get_random_bytes(&rand_mod, sizeof(rand_mod));
@@ -2461,7 +2647,8 @@ static int pn533_dev_up(struct nfc_dev *nfc_dev)
 	if (dev->phy_ops->dev_up)
 		dev->phy_ops->dev_up(dev);
 
-	if (dev->device_type == PN533_DEVICE_PN532) {
+	if ((dev->device_type == PN533_DEVICE_PN532) ||
+		(dev->device_type == PN533_DEVICE_PN532_AUTOPOLL)) {
 		int rc = pn532_sam_configuration(nfc_dev);
 
 		if (rc)
@@ -2508,6 +2695,7 @@ static int pn533_setup(struct pn533 *dev)
 	case PN533_DEVICE_PASORI:
 	case PN533_DEVICE_ACR122U:
 	case PN533_DEVICE_PN532:
+	case PN533_DEVICE_PN532_AUTOPOLL:
 		max_retries.mx_rty_atr = 0x2;
 		max_retries.mx_rty_psl = 0x1;
 		max_retries.mx_rty_passive_act =
@@ -2544,6 +2732,7 @@ static int pn533_setup(struct pn533 *dev)
 	switch (dev->device_type) {
 	case PN533_DEVICE_STD:
 	case PN533_DEVICE_PN532:
+	case PN533_DEVICE_PN532_AUTOPOLL:
 		break;
 
 	case PN533_DEVICE_PASORI:
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index 6541088fad73..388fc1b4fcc1 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -6,10 +6,11 @@
  * Copyright (C) 2012-2013 Tieto Poland
  */
 
-#define PN533_DEVICE_STD     0x1
-#define PN533_DEVICE_PASORI  0x2
-#define PN533_DEVICE_ACR122U 0x3
-#define PN533_DEVICE_PN532   0x4
+#define PN533_DEVICE_STD		0x1
+#define PN533_DEVICE_PASORI		0x2
+#define PN533_DEVICE_ACR122U		0x3
+#define PN533_DEVICE_PN532		0x4
+#define PN533_DEVICE_PN532_AUTOPOLL	0x5
 
 #define PN533_ALL_PROTOCOLS (NFC_PROTO_JEWEL_MASK | NFC_PROTO_MIFARE_MASK |\
 			     NFC_PROTO_FELICA_MASK | NFC_PROTO_ISO14443_MASK |\
@@ -75,6 +76,7 @@
 #define PN533_CMD_IN_ATR 0x50
 #define PN533_CMD_IN_RELEASE 0x52
 #define PN533_CMD_IN_JUMP_FOR_DEP 0x56
+#define PN533_CMD_IN_AUTOPOLL 0x60
 
 #define PN533_CMD_TG_INIT_AS_TARGET 0x8c
 #define PN533_CMD_TG_GET_DATA 0x86
-- 
2.23.0.rc1

Re: [PATCH v6 6/7] nfc: pn533: Add autopoll capability

[Johan Hovold]

On Tue, Aug 20, 2019 at 02:03:43PM +0200, Lars Poeschel wrote:
> pn532 devices support an autopoll command, that lets the chip
> automatically poll for selected nfc technologies instead of manually
> looping through every single nfc technology the user is interested in.
> This is faster and less cpu and bus intensive than manually polling.
> This adds this autopoll capability to the pn533 driver.
> 
> Cc: Johan Hovold <johan@kernel.org>
> Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> ---
> Changes in v6:
> - Rebased the patch series on v5.3-rc5

Just two drive-by comments below.

>  drivers/nfc/pn533/pn533.c | 193 +++++++++++++++++++++++++++++++++++++-
>  drivers/nfc/pn533/pn533.h |  10 +-
>  2 files changed, 197 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
> index a8c756caa678..7e915239222b 100644
> --- a/drivers/nfc/pn533/pn533.c
> +++ b/drivers/nfc/pn533/pn533.c
> @@ -185,6 +185,32 @@ struct pn533_cmd_jump_dep_response {
>  	u8 gt[];
>  } __packed;
>  
> +struct pn532_autopoll_resp {
> +	u8 type;
> +	u8 ln;
> +	u8 tg;
> +	u8 tgdata[];
> +} __packed;

No need for __packed.

> +static int pn533_autopoll_complete(struct pn533 *dev, void *arg,
> +			       struct sk_buff *resp)
> +{
> +	u8 nbtg;
> +	int rc;
> +	struct pn532_autopoll_resp *apr;
> +	struct nfc_target nfc_tgt;
> +
> +	if (IS_ERR(resp)) {
> +		rc = PTR_ERR(resp);
> +
> +		nfc_err(dev->dev, "%s  autopoll complete error %d\n",
> +			__func__, rc);
> +
> +		if (rc == -ENOENT) {
> +			if (dev->poll_mod_count != 0)
> +				return rc;
> +			goto stop_poll;
> +		} else if (rc < 0) {
> +			nfc_err(dev->dev,
> +				"Error %d when running autopoll\n", rc);
> +			goto stop_poll;
> +		}
> +	}
> +
> +	nbtg = resp->data[0];
> +	if ((nbtg > 2) || (nbtg <= 0))
> +		return -EAGAIN;
> +
> +	apr = (struct pn532_autopoll_resp *)&resp->data[1];
> +	while (nbtg--) {
> +		memset(&nfc_tgt, 0, sizeof(struct nfc_target));
> +		switch (apr->type) {
> +		case PN532_AUTOPOLL_TYPE_ISOA:
> +			dev_dbg(dev->dev, "ISOA");

You forgot the '\n' here and elsewhere (some nfc_err as well).

Johan

Re: [PATCH v6 6/7] nfc: pn533: Add autopoll capability

[Lars Poeschel]

On Tue, Aug 20, 2019 at 02:23:44PM +0200, Johan Hovold wrote:
> On Tue, Aug 20, 2019 at 02:03:43PM +0200, Lars Poeschel wrote:

> >  drivers/nfc/pn533/pn533.c | 193 +++++++++++++++++++++++++++++++++++++-
> >  drivers/nfc/pn533/pn533.h |  10 +-
> >  2 files changed, 197 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
> > index a8c756caa678..7e915239222b 100644
> > --- a/drivers/nfc/pn533/pn533.c
> > +++ b/drivers/nfc/pn533/pn533.c
> > @@ -185,6 +185,32 @@ struct pn533_cmd_jump_dep_response {
> >  	u8 gt[];
> >  } __packed;
> >  
> > +struct pn532_autopoll_resp {
> > +	u8 type;
> > +	u8 ln;
> > +	u8 tg;
> > +	u8 tgdata[];
> > +} __packed;
> 
> No need for __packed.

I did a quick test without the __packed and indeed: It worked. I'd
sworn that it is needed in this place, because this autopoll response is
data that the nfc chip puts on the serial wire without padding inbetween
and this struct is used to access this data and without the __packed the
compiler should be allowed to put padding bytes between the struct
fields. But it choose to not do it. I am still not shure, why this
happens, but ok. I can remove the __packed.

> > +static int pn533_autopoll_complete(struct pn533 *dev, void *arg,
> > +			       struct sk_buff *resp)
> > +{
> > +	u8 nbtg;
> > +	int rc;
> > +	struct pn532_autopoll_resp *apr;
> > +	struct nfc_target nfc_tgt;
> > +
> > +	if (IS_ERR(resp)) {
> > +		rc = PTR_ERR(resp);
> > +
> > +		nfc_err(dev->dev, "%s  autopoll complete error %d\n",
> > +			__func__, rc);
> > +
> > +		if (rc == -ENOENT) {
> > +			if (dev->poll_mod_count != 0)
> > +				return rc;
> > +			goto stop_poll;
> > +		} else if (rc < 0) {
> > +			nfc_err(dev->dev,
> > +				"Error %d when running autopoll\n", rc);
> > +			goto stop_poll;
> > +		}
> > +	}
> > +
> > +	nbtg = resp->data[0];
> > +	if ((nbtg > 2) || (nbtg <= 0))
> > +		return -EAGAIN;
> > +
> > +	apr = (struct pn532_autopoll_resp *)&resp->data[1];
> > +	while (nbtg--) {
> > +		memset(&nfc_tgt, 0, sizeof(struct nfc_target));
> > +		switch (apr->type) {
> > +		case PN532_AUTOPOLL_TYPE_ISOA:
> > +			dev_dbg(dev->dev, "ISOA");
> 
> You forgot the '\n' here and elsewhere (some nfc_err as well).

I can add them. I will wait a bit for more review comments before
posting a new patchset.

Thanks so far for your quick review,
Lars

[PATCH v6 7/7] nfc: pn532_uart: Make use of pn532 autopoll

[Lars Poeschel]

This switches the pn532 UART phy driver from manually polling to the new
autopoll mechanism.

Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v6:
- Rebased the patch series on v5.3-rc5

 drivers/nfc/pn533/uart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nfc/pn533/uart.c b/drivers/nfc/pn533/uart.c
index f1cc2354a4fd..e3085e5b2d4c 100644
--- a/drivers/nfc/pn533/uart.c
+++ b/drivers/nfc/pn533/uart.c
@@ -254,7 +254,7 @@ static int pn532_uart_probe(struct serdev_device *serdev)
 	serdev_device_set_flow_control(serdev, false);
 	pn532->send_wakeup = PN532_SEND_WAKEUP;
 	timer_setup(&pn532->cmd_timeout, pn532_cmd_timeout, 0);
-	priv = pn53x_common_init(PN533_DEVICE_PN532,
+	priv = pn53x_common_init(PN533_DEVICE_PN532_AUTOPOLL,
 				     PN533_PROTO_REQ_ACK_RESP,
 				     pn532, &uart_phy_ops, NULL,
 				     &pn532->serdev->dev);
-- 
2.23.0.rc1