[PATCH v3 1/5] nfc: pn533: i2c: "pn532" as dt compatible string

[PATCH v3 1/5] 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.

Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
 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 4389eb4c8d0b..f4eae9dd5305 100644
--- a/drivers/nfc/pn533/i2c.c
+++ b/drivers/nfc/pn533/i2c.c
@@ -258,6 +258,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.19.1

[PATCH v3 2/5] nfc: pn532_uart: Add NXP PN532 to devicetree docs

[Lars Poeschel]

Add a simple binding doc for the pn532.

Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v3:
- seperate binding doc instead of entry in trivial-devices.txt

 .../devicetree/bindings/nfc/pn532.txt         | 31 +++++++++++++++++++
 1 file changed, 31 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..a2df72a5f504
--- /dev/null
+++ b/Documentation/devicetree/bindings/nfc/pn532.txt
@@ -0,0 +1,31 @@
+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.
+
+There are no additional properties provided by the driver at the moment.
+
+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";
+        };
+};
-- 
2.19.1

[PATCH v3 3/5] 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.

Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
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   | 283 +++++++++++++++++++++++++++++++++++++
 4 files changed, 304 insertions(+)
 create mode 100644 drivers/nfc/pn533/uart.c

diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig
index d94122dd30e4..29df7d97d209 100644
--- a/drivers/nfc/pn533/Kconfig
+++ b/drivers/nfc/pn533/Kconfig
@@ -25,3 +25,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 51d24c622fcb..bdfd6860d5e4 100644
--- a/drivers/nfc/pn533/Makefile
+++ b/drivers/nfc/pn533/Makefile
@@ -3,7 +3,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 88d569666c51..ca00508eefff 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -55,6 +55,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 */
@@ -96,6 +101,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..8fd71c8953ca
--- /dev/null
+++ b/drivers/nfc/pn533/uart.c
@@ -0,0 +1,283 @@
+// 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 VERSION "0.1"
+#define PN532_UART_DRIVER_NAME "pn532_uart"
+
+#define PN532_UART_SKB_BUFF_LEN	(PN533_CMD_DATAEXCH_DATA_MAXLEN * 2)
+
+struct pn532_uart_phy {
+	struct serdev_device *serdev;
+	struct sk_buff *recv_skb;
+	struct pn533 *priv;
+	int 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)
+{
+	static const u8 wakeup[] = {
+		0x55, 0x55, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+	/* wakeup sequence and dummy bytes for waiting time */
+	struct pn532_uart_phy *pn532 = dev->phy;
+	int count;
+
+	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)
+		count = serdev_device_write(pn532->serdev,
+				wakeup, sizeof(wakeup),
+				MAX_SCHEDULE_TIMEOUT);
+
+	count = serdev_device_write(pn532->serdev, out->data, out->len,
+			MAX_SCHEDULE_TIMEOUT);
+	if (PN533_FRAME_CMD(((struct pn533_std_frame *)out->data)) ==
+			PN533_CMD_SAM_CONFIGURATION)
+		pn532->send_wakeup = 0;
+
+	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;
+	static const u8 ack[PN533_STD_FRAME_ACK_SIZE] = {
+			0x00, 0x00, 0xff, 0x00, 0xff, 0x00};
+	/* spec 7.1.1.3:  Preamble, SoPC (2), ACK Code (2), Postamble */
+	int rc;
+
+	rc = serdev_device_write(pn532->serdev, ack, sizeof(ack),
+			MAX_SCHEDULE_TIMEOUT);
+
+	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 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,
+};
+
+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 == NULL)
+			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-uart", },
+	{},
+};
+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 == NULL)
+		goto err_exit;
+
+	pn532->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL);
+	if (pn532->recv_skb == NULL)
+		goto err_free;
+
+	pn532->serdev = serdev;
+	priv = pn533_register_device(PN533_DEVICE_PN532,
+				     PN533_NO_TYPE_B_PROTOCOLS,
+				     PN533_PROTO_REQ_ACK_RESP,
+				     pn532, &uart_phy_ops, NULL,
+				     &pn532->serdev->dev,
+				     &serdev->dev);
+
+	if (IS_ERR(priv)) {
+		err = PTR_ERR(priv);
+		goto err_skb;
+	}
+
+	pn532->priv = priv;
+	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 %s\n",
+				serdev->dev.init_name);
+		goto err_unregister;
+	}
+
+	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 = 1;
+	timer_setup(&pn532->cmd_timeout, pn532_cmd_timeout, 0);
+	err = pn533_finalize_setup(pn532->priv);
+	if (err)
+		goto err_serdev;
+
+	return 0;
+
+err_serdev:
+	serdev_device_close(serdev);
+err_unregister:
+	pn533_unregister_device(pn532->priv);
+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);
+
+	pn533_unregister_device(pn532->priv);
+	serdev_device_close(serdev);
+	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_DRIVER_NAME,
+		.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 ver " VERSION);
+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL");
-- 
2.19.1

[PATCH v3 4/5] 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.

Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
 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 a0cc1cc45292..c80ee79af86e 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -197,6 +197,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
@@ -1401,6 +1427,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)
 {
@@ -1546,6 +1667,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",
@@ -1569,9 +1691,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));
@@ -2468,7 +2654,8 @@ static int pn533_dev_up(struct nfc_dev *nfc_dev)
 {
 	struct pn533 *dev = nfc_get_drvdata(nfc_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 ca00508eefff..14a97b7d5ba7 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -18,10 +18,11 @@
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
-#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 |\
@@ -87,6 +88,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.19.1

[PATCH v3 5/5] 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.

Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
 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 8fd71c8953ca..a95ca01745ca 100644
--- a/drivers/nfc/pn533/uart.c
+++ b/drivers/nfc/pn533/uart.c
@@ -207,7 +207,7 @@ static int pn532_uart_probe(struct serdev_device *serdev)
 		goto err_free;
 
 	pn532->serdev = serdev;
-	priv = pn533_register_device(PN533_DEVICE_PN532,
+	priv = pn533_register_device(PN533_DEVICE_PN532_AUTOPOLL,
 				     PN533_NO_TYPE_B_PROTOCOLS,
 				     PN533_PROTO_REQ_ACK_RESP,
 				     pn532, &uart_phy_ops, NULL,
-- 
2.19.1

Re: [PATCH v3 2/5] nfc: pn532_uart: Add NXP PN532 to devicetree docs

[Rob Herring]

On Thu, Oct 25, 2018 at 03:29:33PM +0200, Lars Poeschel wrote:
> Add a simple binding doc for the pn532.
> 
> Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> ---
> Changes in v3:
> - seperate binding doc instead of entry in trivial-devices.txt
> 
>  .../devicetree/bindings/nfc/pn532.txt         | 31 +++++++++++++++++++
>  1 file changed, 31 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..a2df72a5f504
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nfc/pn532.txt
> @@ -0,0 +1,31 @@
> +NXP PN532 NFC Chip

What about the PN533?

> +
> +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.
> +
> +There are no additional properties provided by the driver at the moment.

I2C requires reg.

> +
> +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";
> +        };
> +};
> -- 
> 2.19.1
> 

Re: [PATCH v3 2/5] nfc: pn532_uart: Add NXP PN532 to devicetree docs

[Lars Poeschel]

On Thu, Oct 25, 2018 at 04:54:45PM -0500, Rob Herring wrote:
> On Thu, Oct 25, 2018 at 03:29:33PM +0200, Lars Poeschel wrote:
> > Add a simple binding doc for the pn532.
> > 
> > Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> > ---
> > Changes in v3:
> > - seperate binding doc instead of entry in trivial-devices.txt
> > 
> >  .../devicetree/bindings/nfc/pn532.txt         | 31 +++++++++++++++++++
> >  1 file changed, 31 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..a2df72a5f504
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/nfc/pn532.txt
> > @@ -0,0 +1,31 @@
> > +NXP PN532 NFC Chip
> 
> What about the PN533?

I read the publicy available docs from NXP a bit and I think this is
wrong. PN533 is the version of the chip with usb. It does not have a i2c
client interface. It only has a i2c master interface, but this serves us
no purpose here and is furthermore not supported by the driver.
PN532 is the version of the chip for "simple" serial protocols. It's
interfaces to the host are uart, i2c and spi.
So I left PN533 out here.

> > +
> > +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.
> > +
> > +There are no additional properties provided by the driver at the moment.
> 
> I2C requires reg.

I was so deeply focused at my uart part. Sorry, I missed that.
I will wait a bit if I get some more feedback and will address this in
the next version of the patchset.

> > +
> > +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";
> > +        };
> > +};
> > -- 
> > 2.19.1
> > 

Thank you for your review,

Lars

Re: [PATCH v3 3/5] nfc: pn533: add UART phy driver

[Johan Hovold]

On Thu, Oct 25, 2018 at 03:29:34PM +0200, 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.

Just a few drive-by comments below.

> +// SPDX-License-Identifier: GPL-2.0

This should match MODULE_LICENSE below which currently says v2 *or
later*.

> +/*
> + * 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.
> + */

> +#define VERSION "0.1"

We don't version kernel drivers individually, so please drop this here
and below.

> +static int pn532_uart_send_frame(struct pn533 *dev,
> +				struct sk_buff *out)
> +{
> +	static const u8 wakeup[] = {
> +		0x55, 0x55, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
> +	/* wakeup sequence and dummy bytes for waiting time */

Comments should go above the code they apply to.

> +	struct pn532_uart_phy *pn532 = dev->phy;
> +	int count;
> +
> +	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)
> +		count = serdev_device_write(pn532->serdev,
> +				wakeup, sizeof(wakeup),
> +				MAX_SCHEDULE_TIMEOUT);

No error handling?

> +
> +	count = serdev_device_write(pn532->serdev, out->data, out->len,
> +			MAX_SCHEDULE_TIMEOUT);

Same here.

> +	if (PN533_FRAME_CMD(((struct pn533_std_frame *)out->data)) ==
> +			PN533_CMD_SAM_CONFIGURATION)
> +		pn532->send_wakeup = 0;
> +
> +	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;
> +	static const u8 ack[PN533_STD_FRAME_ACK_SIZE] = {
> +			0x00, 0x00, 0xff, 0x00, 0xff, 0x00};
> +	/* spec 7.1.1.3:  Preamble, SoPC (2), ACK Code (2), Postamble */

Same as above.

> +	int rc;
> +
> +	rc = serdev_device_write(pn532->serdev, ack, sizeof(ack),
> +			MAX_SCHEDULE_TIMEOUT);

Error handling.

> +
> +	return 0;
> +}

> +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 == NULL)

I'd use !pn532 here (and elsewhere).

> +		goto err_exit;
> +
> +	pn532->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL);
> +	if (pn532->recv_skb == NULL)
> +		goto err_free;
> +
> +	pn532->serdev = serdev;
> +	priv = pn533_register_device(PN533_DEVICE_PN532,
> +				     PN533_NO_TYPE_B_PROTOCOLS,
> +				     PN533_PROTO_REQ_ACK_RESP,
> +				     pn532, &uart_phy_ops, NULL,
> +				     &pn532->serdev->dev,
> +				     &serdev->dev);
> +

Stray new line.

> +	if (IS_ERR(priv)) {
> +		err = PTR_ERR(priv);
> +		goto err_skb;
> +	}

Should you not set up your device fully before registering it? I'd
assume you could get callbacks from NFC core here.

> +
> +	pn532->priv = priv;
> +	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 %s\n",
> +				serdev->dev.init_name);

dev_err will include the device name so you can drop the init_name bit.

> +		goto err_unregister;
> +	}

Keeping the serial device open at all times will prevent low power
states on some platforms. Wouldn't it be possible to open the device
when the nfc interface is brought up (and during setup)?

> +
> +	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 = 1;
> +	timer_setup(&pn532->cmd_timeout, pn532_cmd_timeout, 0);
> +	err = pn533_finalize_setup(pn532->priv);
> +	if (err)
> +		goto err_serdev;
> +
> +	return 0;
> +
> +err_serdev:
> +	serdev_device_close(serdev);
> +err_unregister:
> +	pn533_unregister_device(pn532->priv);
> +err_skb:
> +	kfree_skb(pn532->recv_skb);
> +err_free:
> +	kfree(pn532);
> +err_exit:
> +	return err;
> +}

> +MODULE_AUTHOR("Lars Pöschel <poeschel@lemonage.de>");
> +MODULE_DESCRIPTION("PN532 UART driver ver " VERSION);
> +MODULE_VERSION(VERSION);

Drop version.

> +MODULE_LICENSE("GPL");

Doesn't match SPDX identifier above.

Johan

Re: [PATCH v3 3/5] nfc: pn533: add UART phy driver

[Marcel Holtmann]

Hi Johan,

>> 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.
> 
> Just a few drive-by comments below.
> 
>> +// SPDX-License-Identifier: GPL-2.0
> 
> This should match MODULE_LICENSE below which currently says v2 *or
> later*.
> 
>> +/*
>> + * 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.
>> + */
> 
>> +#define VERSION "0.1"
> 
> We don't version kernel drivers individually, so please drop this here
> and below.

if we don’t then maybe send patches to remove MODULE_VERSION first. Otherwise this is totally fine to do.

There are currently 670 usages of MODULE_VERSION and I have not heard anybody looking at removing this.

Regards

Marcel

Re: [PATCH v3 3/5] nfc: pn533: add UART phy driver

[Johan Hovold]

On Sun, Oct 28, 2018 at 02:46:24PM +0100, Marcel Holtmann wrote:

> >> +#define VERSION "0.1"
> > 
> > We don't version kernel drivers individually, so please drop this here
> > and below.
> 
> if we don’t then maybe send patches to remove MODULE_VERSION first.
> Otherwise this is totally fine to do.
> 
> There are currently 670 usages of MODULE_VERSION and I have not heard
> anybody looking at removing this.

Should have phrased that differently; versioning modules individually
does not make much sense when we already have a kernel version which is
tied to the driver code in question and which *does* get updated as new
kernels are released (unlike these module version defines, which tend to
stay unchanged).

For USB, we've dropped module versioning entirely and push back whenever
someone proposes to add it back. Other subsystems and particularly old
drivers may still have these macros of course.

In this case, the pn533 driver (and its interface drivers) is one of only
three NFC drivers which have MODULE_VERSION, but maybe it makes sense to
keep it in, if only for consistency with the other pn533 components.

Thanks,
Johan

Re: [PATCH v3 3/5] nfc: pn533: add UART phy driver

[Lars Poeschel]

Hi Johan,

thank you very much for the review!

On Sun, Oct 28, 2018 at 11:27:25AM +0100, Johan Hovold wrote:
> On Thu, Oct 25, 2018 at 03:29:34PM +0200, 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.
> 
> Just a few drive-by comments below.
> 
> > +// SPDX-License-Identifier: GPL-2.0
> 
> This should match MODULE_LICENSE below which currently says v2 *or
> later*.

Ok. Will change that to 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.
> > + */
> 
> > +#define VERSION "0.1"
> 
> We don't version kernel drivers individually, so please drop this here
> and below.

There was a comment from Marcel about this as well and I read it as: You
can do it, but it is not required and nobody really cares.
I included this, because the other pn532 phy driver (i2c) is doing it
this way, but I don't like it either, so I will drop this, as well as
the PN532_UART_DRIVER_NAME define in the next version.

> > +static int pn532_uart_send_frame(struct pn533 *dev,
> > +				struct sk_buff *out)
> > +{
> > +	static const u8 wakeup[] = {
> > +		0x55, 0x55, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +		0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
> > +	/* wakeup sequence and dummy bytes for waiting time */
> 
> Comments should go above the code they apply to.

Ok.

> > +	struct pn532_uart_phy *pn532 = dev->phy;
> > +	int count;
> > +
> > +	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)
> > +		count = serdev_device_write(pn532->serdev,
> > +				wakeup, sizeof(wakeup),
> > +				MAX_SCHEDULE_TIMEOUT);
> 
> No error handling?

Yes, good point. Also the variable name is misleading. I will change
that.

> > +
> > +	count = serdev_device_write(pn532->serdev, out->data, out->len,
> > +			MAX_SCHEDULE_TIMEOUT);
> 
> Same here.

Ok.

> > +	if (PN533_FRAME_CMD(((struct pn533_std_frame *)out->data)) ==
> > +			PN533_CMD_SAM_CONFIGURATION)
> > +		pn532->send_wakeup = 0;
> > +
> > +	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;
> > +	static const u8 ack[PN533_STD_FRAME_ACK_SIZE] = {
> > +			0x00, 0x00, 0xff, 0x00, 0xff, 0x00};
> > +	/* spec 7.1.1.3:  Preamble, SoPC (2), ACK Code (2), Postamble */
> 
> Same as above.

Ok.

> > +	int rc;
> > +
> > +	rc = serdev_device_write(pn532->serdev, ack, sizeof(ack),
> > +			MAX_SCHEDULE_TIMEOUT);
> 
> Error handling.

Ok.

> > +
> > +	return 0;
> > +}
> 
> > +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 == NULL)
> 
> I'd use !pn532 here (and elsewhere).

I will change it.

> > +		goto err_exit;
> > +
> > +	pn532->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL);
> > +	if (pn532->recv_skb == NULL)
> > +		goto err_free;
> > +
> > +	pn532->serdev = serdev;
> > +	priv = pn533_register_device(PN533_DEVICE_PN532,
> > +				     PN533_NO_TYPE_B_PROTOCOLS,
> > +				     PN533_PROTO_REQ_ACK_RESP,
> > +				     pn532, &uart_phy_ops, NULL,
> > +				     &pn532->serdev->dev,
> > +				     &serdev->dev);
> > +
> 
> Stray new line.

Ok.

> > +	if (IS_ERR(priv)) {
> > +		err = PTR_ERR(priv);
> > +		goto err_skb;
> > +	}
> 
> Should you not set up your device fully before registering it? I'd
> assume you could get callbacks from NFC core here.

I did not see any during my tests, but you are right: It feels a bit
odd.
The i2c driver is also requesting irqs after registering.
The pn533_finalize_setup() has to be last.
I could do the serdev_* stuff before, but ...

> > +
> > +	pn532->priv = priv;
> > +	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 %s\n",
> > +				serdev->dev.init_name);
> 
> dev_err will include the device name so you can drop the init_name bit.

Ok, i drop it.

> > +		goto err_unregister;
> > +	}
> 
> Keeping the serial device open at all times will prevent low power
> states on some platforms. Wouldn't it be possible to open the device
> when the nfc interface is brought up (and during setup)?

... that would then be contrary to this idea.
Also I don't see how to implement it with what is there today. i2c also
does not do something similar.
It can be done with adding some callbacks from the core (pn533.c) driver
to it's phy drivers.
Wouldn't that be the scope of another later patch then ?

Thanks again,
Lars

Re: [PATCH v3 3/5] nfc: pn533: add UART phy driver

[Johan Hovold]

On Mon, Oct 29, 2018 at 11:02:46AM +0100, Lars Poeschel wrote:
> Hi Johan,
> 
> thank you very much for the review!
> 
> On Sun, Oct 28, 2018 at 11:27:25AM +0100, Johan Hovold wrote:
> > On Thu, Oct 25, 2018 at 03:29:34PM +0200, 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.
> > 
> > Just a few drive-by comments below.

> > > +/*
> > > + * 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.
> > > + */
> > 
> > > +#define VERSION "0.1"
> > 
> > We don't version kernel drivers individually, so please drop this here
> > and below.
> 
> There was a comment from Marcel about this as well and I read it as: You
> can do it, but it is not required and nobody really cares.
> I included this, because the other pn532 phy driver (i2c) is doing it
> this way, but I don't like it either, so I will drop this, as well as
> the PN532_UART_DRIVER_NAME define in the next version.

Sounds good.

> > > +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 == NULL)
> > 
> > I'd use !pn532 here (and elsewhere).
> 
> I will change it.
> 
> > > +		goto err_exit;
> > > +
> > > +	pn532->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL);
> > > +	if (pn532->recv_skb == NULL)
> > > +		goto err_free;
> > > +
> > > +	pn532->serdev = serdev;
> > > +	priv = pn533_register_device(PN533_DEVICE_PN532,
> > > +				     PN533_NO_TYPE_B_PROTOCOLS,
> > > +				     PN533_PROTO_REQ_ACK_RESP,
> > > +				     pn532, &uart_phy_ops, NULL,
> > > +				     &pn532->serdev->dev,
> > > +				     &serdev->dev);
> > > +
> > 
> > Stray new line.
> 
> Ok.
> 
> > > +	if (IS_ERR(priv)) {
> > > +		err = PTR_ERR(priv);
> > > +		goto err_skb;
> > > +	}
> > 
> > Should you not set up your device fully before registering it? I'd
> > assume you could get callbacks from NFC core here.
> 
> I did not see any during my tests, but you are right: It feels a bit
> odd.
> The i2c driver is also requesting irqs after registering.
> The pn533_finalize_setup() has to be last.
> I could do the serdev_* stuff before, but ...
> 
> > > +
> > > +	pn532->priv = priv;
> > > +	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 %s\n",
> > > +				serdev->dev.init_name);
> > 
> > dev_err will include the device name so you can drop the init_name bit.
> 
> Ok, i drop it.
> 
> > > +		goto err_unregister;
> > > +	}
> > 
> > Keeping the serial device open at all times will prevent low power
> > states on some platforms. Wouldn't it be possible to open the device
> > when the nfc interface is brought up (and during setup)?
> 
> ... that would then be contrary to this idea.

Not necessarily, that depends on what callbacks you can expect and at
what time.

> Also I don't see how to implement it with what is there today. i2c also
> does not do something similar.

But i2c doesn't have the concept of an "open" port consuming power.

> It can be done with adding some callbacks from the core (pn533.c) driver
> to it's phy drivers.

Haven't looked at it in any detail, but in general serdev driver should
only keep the port open while the device is in use.

I only noticed that nfc core has dev_up/down callbacks which looks like
they could be used for something like this.

> Wouldn't that be the scope of another later patch then ?

Possibly. We have accepted some serdev drivers already taking the lazy
approach of opening the port in probe. Depending on the driver, it may
not be too bad (e.g. for some specific hardware which you know you'll
always use), but it not really nice to have everyone pay a price in
terms of power-consumption for a feature that is rarely used.

Johan

Re: [PATCH v3 3/5] nfc: pn533: add UART phy driver

[Lars Poeschel]

On Mon, Oct 29, 2018 at 12:07:04PM +0100, Johan Hovold wrote:
> > > > +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 == NULL)
> > > 
> > > I'd use !pn532 here (and elsewhere).
> > 
> > I will change it.
> > 
> > > > +		goto err_exit;
> > > > +
> > > > +	pn532->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL);
> > > > +	if (pn532->recv_skb == NULL)
> > > > +		goto err_free;
> > > > +
> > > > +	pn532->serdev = serdev;
> > > > +	priv = pn533_register_device(PN533_DEVICE_PN532,
> > > > +				     PN533_NO_TYPE_B_PROTOCOLS,
> > > > +				     PN533_PROTO_REQ_ACK_RESP,
> > > > +				     pn532, &uart_phy_ops, NULL,
> > > > +				     &pn532->serdev->dev,
> > > > +				     &serdev->dev);
> > > > +
> > > 
> > > Stray new line.
> > 
> > Ok.
> > 
> > > > +	if (IS_ERR(priv)) {
> > > > +		err = PTR_ERR(priv);
> > > > +		goto err_skb;
> > > > +	}
> > > 
> > > Should you not set up your device fully before registering it? I'd
> > > assume you could get callbacks from NFC core here.
> > 
> > I did not see any during my tests, but you are right: It feels a bit
> > odd.
> > The i2c driver is also requesting irqs after registering.
> > The pn533_finalize_setup() has to be last.
> > I could do the serdev_* stuff before, but ...
> > 
> > > > +
> > > > +	pn532->priv = priv;
> > > > +	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 %s\n",
> > > > +				serdev->dev.init_name);
> > > 
> > > dev_err will include the device name so you can drop the init_name bit.
> > 
> > Ok, i drop it.
> > 
> > > > +		goto err_unregister;
> > > > +	}
> > > 
> > > Keeping the serial device open at all times will prevent low power
> > > states on some platforms. Wouldn't it be possible to open the device
> > > when the nfc interface is brought up (and during setup)?
> > 
> > ... that would then be contrary to this idea.
> 
> Not necessarily, that depends on what callbacks you can expect and at
> what time.
> 
> > Also I don't see how to implement it with what is there today. i2c also
> > does not do something similar.
> 
> But i2c doesn't have the concept of an "open" port consuming power.
> 
> > It can be done with adding some callbacks from the core (pn533.c) driver
> > to it's phy drivers.
> 
> Haven't looked at it in any detail, but in general serdev driver should
> only keep the port open while the device is in use.
> 
> I only noticed that nfc core has dev_up/down callbacks which looks like
> they could be used for something like this.

Yes, this is to the nfc core. But my uart phy driver binds to the pn533
driver and this does then register to nfc core. So this needs some more
changes to pn533 driver to be able to be informed of dev_up/down.
I'll look into this tomorrow.

> > Wouldn't that be the scope of another later patch then ?
> 
> Possibly. We have accepted some serdev drivers already taking the lazy
> approach of opening the port in probe. Depending on the driver, it may
> not be too bad (e.g. for some specific hardware which you know you'll
> always use), but it not really nice to have everyone pay a price in
> terms of power-consumption for a feature that is rarely used.

Is there a way in serdev to close a port, but still occupy it ?
I'd like to do the basic chip initialisation in _probe and then close
the port for power-consuption reasons. I'd like to have the port still
occupied, so that it's not available to other possible users in the
meantime. I'd then do a serdev open again in dev_up and really use it
from there.
dev_down is then for serdev close and also still occupy it.
closing and releasing would then be done in _remove.

Regards,
Lars

Re: [PATCH v3 3/5] nfc: pn533: add UART phy driver

[Johan Hovold]

On Mon, Oct 29, 2018 at 04:51:10PM +0100, Lars Poeschel wrote:
> On Mon, Oct 29, 2018 at 12:07:04PM +0100, Johan Hovold wrote:

> > > Wouldn't that be the scope of another later patch then ?
> > 
> > Possibly. We have accepted some serdev drivers already taking the lazy
> > approach of opening the port in probe. Depending on the driver, it may
> > not be too bad (e.g. for some specific hardware which you know you'll
> > always use), but it not really nice to have everyone pay a price in
> > terms of power-consumption for a feature that is rarely used.
> 
> Is there a way in serdev to close a port, but still occupy it ?
> I'd like to do the basic chip initialisation in _probe and then close
> the port for power-consuption reasons. I'd like to have the port still
> occupied, so that it's not available to other possible users in the
> meantime. I'd then do a serdev open again in dev_up and really use it
> from there.
> dev_down is then for serdev close and also still occupy it.
> closing and releasing would then be done in _remove.

The serdev device is bound you driver regardless of whether you open the
port or not, so just use serdev_device_open() and serdev_device_close()
as necessary at probe() if you need to do some setup and then later at
dev_up() and dev_down(), respectively.

Johan