[PATCH net-next 2/2] net: nfc: s3fwrn5: Support a UART interface

[PATCH net-next 2/2] net: nfc: s3fwrn5: Support a UART interface

[Bongsu Jeon]

Since S3FWRN82 NFC Chip, The UART interface can be used.
S3FWRN82 uses NCI protocol and supports I2C and UART interface.

Signed-off-by: Bongsu Jeon <bongsu.jeon@samsung.com>
---
 drivers/nfc/s3fwrn5/Kconfig  |  12 ++
 drivers/nfc/s3fwrn5/Makefile |   2 +
 drivers/nfc/s3fwrn5/uart.c   | 250 +++++++++++++++++++++++++++++++++++
 3 files changed, 264 insertions(+)
 create mode 100644 drivers/nfc/s3fwrn5/uart.c

diff --git a/drivers/nfc/s3fwrn5/Kconfig b/drivers/nfc/s3fwrn5/Kconfig
index 3f8b6da58280..6f88737769e1 100644
--- a/drivers/nfc/s3fwrn5/Kconfig
+++ b/drivers/nfc/s3fwrn5/Kconfig
@@ -20,3 +20,15 @@ config NFC_S3FWRN5_I2C
 	  To compile this driver as a module, choose m here. The module will
 	  be called s3fwrn5_i2c.ko.
 	  Say N if unsure.
+
+config NFC_S3FWRN82_UART
+	tristate "Samsung S3FWRN82 UART support"
+	depends on NFC_NCI && SERIAL_DEV_BUS
+	select NFC_S3FWRN5
+	help
+	  This module adds support for a UART interface to the S3FWRN82 chip.
+	  Select this if your platform is using the UART bus.
+
+	  To compile this driver as a module, choose m here. The module will
+	  be called s3fwrn82_uart.ko.
+	  Say N if unsure.
diff --git a/drivers/nfc/s3fwrn5/Makefile b/drivers/nfc/s3fwrn5/Makefile
index d0ffa35f50e8..d1902102060b 100644
--- a/drivers/nfc/s3fwrn5/Makefile
+++ b/drivers/nfc/s3fwrn5/Makefile
@@ -5,6 +5,8 @@
 
 s3fwrn5-objs = core.o firmware.o nci.o
 s3fwrn5_i2c-objs = i2c.o
+s3fwrn82_uart-objs = uart.o
 
 obj-$(CONFIG_NFC_S3FWRN5) += s3fwrn5.o
 obj-$(CONFIG_NFC_S3FWRN5_I2C) += s3fwrn5_i2c.o
+obj-$(CONFIG_NFC_S3FWRN82_UART) += s3fwrn82_uart.o
diff --git a/drivers/nfc/s3fwrn5/uart.c b/drivers/nfc/s3fwrn5/uart.c
new file mode 100644
index 000000000000..b3c36a5b28d3
--- /dev/null
+++ b/drivers/nfc/s3fwrn5/uart.c
@@ -0,0 +1,250 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * UART Link Layer for S3FWRN82 NCI based Driver
+ *
+ * Copyright (C) 2020 Samsung Electronics
+ * Author: Bongsu Jeon <bongsu.jeon@samsung.com>
+ * 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 <linux/gpio.h>
+#include <linux/of_gpio.h>
+
+#include "s3fwrn5.h"
+
+#define S3FWRN82_UART_DRIVER_NAME "s3fwrn82_uart"
+#define S3FWRN82_NCI_HEADER 3
+#define S3FWRN82_NCI_IDX 2
+#define S3FWRN82_EN_WAIT_TIME 20
+#define NCI_SKB_BUFF_LEN 258
+
+struct s3fwrn82_uart_phy {
+	struct serdev_device *ser_dev;
+	struct nci_dev *ndev;
+	struct sk_buff *recv_skb;
+
+	unsigned int gpio_en;
+	unsigned int gpio_fw_wake;
+
+	/* mutex is used to synchronize */
+	struct mutex mutex;
+	enum s3fwrn5_mode mode;
+};
+
+static void s3fwrn82_uart_set_wake(void *phy_id, bool wake)
+{
+	struct s3fwrn82_uart_phy *phy = phy_id;
+
+	mutex_lock(&phy->mutex);
+	gpio_set_value(phy->gpio_fw_wake, wake);
+	msleep(S3FWRN82_EN_WAIT_TIME);
+	mutex_unlock(&phy->mutex);
+}
+
+static void s3fwrn82_uart_set_mode(void *phy_id, enum s3fwrn5_mode mode)
+{
+	struct s3fwrn82_uart_phy *phy = phy_id;
+
+	mutex_lock(&phy->mutex);
+	if (phy->mode == mode)
+		goto out;
+	phy->mode = mode;
+	gpio_set_value(phy->gpio_en, 1);
+	gpio_set_value(phy->gpio_fw_wake, 0);
+	if (mode == S3FWRN5_MODE_FW)
+		gpio_set_value(phy->gpio_fw_wake, 1);
+	if (mode != S3FWRN5_MODE_COLD) {
+		msleep(S3FWRN82_EN_WAIT_TIME);
+		gpio_set_value(phy->gpio_en, 0);
+		msleep(S3FWRN82_EN_WAIT_TIME);
+	}
+out:
+	mutex_unlock(&phy->mutex);
+}
+
+static enum s3fwrn5_mode s3fwrn82_uart_get_mode(void *phy_id)
+{
+	struct s3fwrn82_uart_phy *phy = phy_id;
+	enum s3fwrn5_mode mode;
+
+	mutex_lock(&phy->mutex);
+	mode = phy->mode;
+	mutex_unlock(&phy->mutex);
+	return mode;
+}
+
+static int s3fwrn82_uart_write(void *phy_id, struct sk_buff *out)
+{
+	struct s3fwrn82_uart_phy *phy = phy_id;
+	int err;
+
+	err = serdev_device_write(phy->ser_dev,
+				  out->data, out->len,
+				  MAX_SCHEDULE_TIMEOUT);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static const struct s3fwrn5_phy_ops uart_phy_ops = {
+	.set_wake = s3fwrn82_uart_set_wake,
+	.set_mode = s3fwrn82_uart_set_mode,
+	.get_mode = s3fwrn82_uart_get_mode,
+	.write = s3fwrn82_uart_write,
+};
+
+static int s3fwrn82_uart_read(struct serdev_device *serdev,
+			      const unsigned char *data,
+			      size_t count)
+{
+	struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
+	size_t i;
+
+	for (i = 0; i < count; i++) {
+		skb_put_u8(phy->recv_skb, *data++);
+
+		if (phy->recv_skb->len < S3FWRN82_NCI_HEADER)
+			continue;
+
+		if ((phy->recv_skb->len - S3FWRN82_NCI_HEADER)
+				< phy->recv_skb->data[S3FWRN82_NCI_IDX])
+			continue;
+
+		s3fwrn5_recv_frame(phy->ndev, phy->recv_skb, phy->mode);
+		phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL);
+		if (!phy->recv_skb)
+			return 0;
+	}
+
+	return i;
+}
+
+static struct serdev_device_ops s3fwrn82_serdev_ops = {
+	.receive_buf = s3fwrn82_uart_read,
+	.write_wakeup = serdev_device_write_wakeup,
+};
+
+static const struct of_device_id s3fwrn82_uart_of_match[] = {
+	{ .compatible = "samsung,s3fwrn82-uart", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, s3fwrn82_uart_of_match);
+
+static int s3fwrn82_uart_parse_dt(struct serdev_device *serdev)
+{
+	struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
+	struct device_node *np = serdev->dev.of_node;
+
+	if (!np)
+		return -ENODEV;
+
+	phy->gpio_en = of_get_named_gpio(np, "en-gpios", 0);
+	if (!gpio_is_valid(phy->gpio_en))
+		return -ENODEV;
+
+	phy->gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
+	if (!gpio_is_valid(phy->gpio_fw_wake))
+		return -ENODEV;
+
+	return 0;
+}
+
+static int s3fwrn82_uart_probe(struct serdev_device *serdev)
+{
+	struct s3fwrn82_uart_phy *phy;
+	int ret = -ENOMEM;
+
+	phy = devm_kzalloc(&serdev->dev, sizeof(*phy), GFP_KERNEL);
+	if (!phy)
+		goto err_exit;
+
+	phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL);
+	if (!phy->recv_skb)
+		goto err_free;
+
+	mutex_init(&phy->mutex);
+	phy->mode = S3FWRN5_MODE_COLD;
+
+	phy->ser_dev = serdev;
+	serdev_device_set_drvdata(serdev, phy);
+	serdev_device_set_client_ops(serdev, &s3fwrn82_serdev_ops);
+	ret = serdev_device_open(serdev);
+	if (ret) {
+		dev_err(&serdev->dev, "Unable to open device\n");
+		goto err_skb;
+	}
+
+	ret = serdev_device_set_baudrate(serdev, 115200);
+	if (ret != 115200) {
+		ret = -EINVAL;
+		goto err_serdev;
+	}
+
+	serdev_device_set_flow_control(serdev, false);
+
+	ret = s3fwrn82_uart_parse_dt(serdev);
+	if (ret < 0)
+		goto err_serdev;
+
+	ret = devm_gpio_request_one(&phy->ser_dev->dev,
+				    phy->gpio_en,
+				    GPIOF_OUT_INIT_HIGH,
+				    "s3fwrn82_en");
+	if (ret < 0)
+		goto err_serdev;
+
+	ret = devm_gpio_request_one(&phy->ser_dev->dev,
+				    phy->gpio_fw_wake,
+				    GPIOF_OUT_INIT_LOW,
+				    "s3fwrn82_fw_wake");
+	if (ret < 0)
+		goto err_serdev;
+
+	ret = s3fwrn5_probe(&phy->ndev, phy, &phy->ser_dev->dev, &uart_phy_ops);
+	if (ret < 0)
+		goto err_serdev;
+
+	return ret;
+
+err_serdev:
+	serdev_device_close(serdev);
+err_skb:
+	kfree_skb(phy->recv_skb);
+err_free:
+	kfree(phy);
+err_exit:
+	return ret;
+}
+
+static void s3fwrn82_uart_remove(struct serdev_device *serdev)
+{
+	struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
+
+	s3fwrn5_remove(phy->ndev);
+	serdev_device_close(serdev);
+	kfree_skb(phy->recv_skb);
+	kfree(phy);
+}
+
+static struct serdev_device_driver s3fwrn82_uart_driver = {
+	.probe = s3fwrn82_uart_probe,
+	.remove = s3fwrn82_uart_remove,
+	.driver = {
+		.name = S3FWRN82_UART_DRIVER_NAME,
+		.of_match_table = of_match_ptr(s3fwrn82_uart_of_match),
+	},
+};
+
+module_serdev_device_driver(s3fwrn82_uart_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("UART driver for Samsung NFC");
+MODULE_AUTHOR("Bongsu Jeon <bongsu.jeon@samsung.com>");
-- 
2.17.1

Re: [PATCH net-next 2/2] net: nfc: s3fwrn5: Support a UART interface

[krzk]

On Mon, Nov 23, 2020 at 04:56:58PM +0900, Bongsu Jeon wrote:
> Since S3FWRN82 NFC Chip, The UART interface can be used.
> S3FWRN82 uses NCI protocol and supports I2C and UART interface.
> 
> Signed-off-by: Bongsu Jeon <bongsu.jeon@samsung.com>

Please start sending emails properly, e.g. with git send-email, so all
your patches in the patchset are referencing the first patch.

> ---
>  drivers/nfc/s3fwrn5/Kconfig  |  12 ++
>  drivers/nfc/s3fwrn5/Makefile |   2 +
>  drivers/nfc/s3fwrn5/uart.c   | 250 +++++++++++++++++++++++++++++++++++
>  3 files changed, 264 insertions(+)
>  create mode 100644 drivers/nfc/s3fwrn5/uart.c
> 
> diff --git a/drivers/nfc/s3fwrn5/Kconfig b/drivers/nfc/s3fwrn5/Kconfig
> index 3f8b6da58280..6f88737769e1 100644
> --- a/drivers/nfc/s3fwrn5/Kconfig
> +++ b/drivers/nfc/s3fwrn5/Kconfig
> @@ -20,3 +20,15 @@ config NFC_S3FWRN5_I2C
>  	  To compile this driver as a module, choose m here. The module will
>  	  be called s3fwrn5_i2c.ko.
>  	  Say N if unsure.
> +
> +config NFC_S3FWRN82_UART
> +	tristate "Samsung S3FWRN82 UART support"
> +	depends on NFC_NCI && SERIAL_DEV_BUS

What about SERIAL_DEV_BUS as module? Shouldn't this be
SERIAL_DEV_BUS || !SERIAL_DEV_BUS?

> +	select NFC_S3FWRN5
> +	help
> +	  This module adds support for a UART interface to the S3FWRN82 chip.
> +	  Select this if your platform is using the UART bus.
> +
> +	  To compile this driver as a module, choose m here. The module will
> +	  be called s3fwrn82_uart.ko.
> +	  Say N if unsure.
> diff --git a/drivers/nfc/s3fwrn5/Makefile b/drivers/nfc/s3fwrn5/Makefile
> index d0ffa35f50e8..d1902102060b 100644
> --- a/drivers/nfc/s3fwrn5/Makefile
> +++ b/drivers/nfc/s3fwrn5/Makefile
> @@ -5,6 +5,8 @@
>  
>  s3fwrn5-objs = core.o firmware.o nci.o
>  s3fwrn5_i2c-objs = i2c.o
> +s3fwrn82_uart-objs = uart.o
>  
>  obj-$(CONFIG_NFC_S3FWRN5) += s3fwrn5.o
>  obj-$(CONFIG_NFC_S3FWRN5_I2C) += s3fwrn5_i2c.o
> +obj-$(CONFIG_NFC_S3FWRN82_UART) += s3fwrn82_uart.o
> diff --git a/drivers/nfc/s3fwrn5/uart.c b/drivers/nfc/s3fwrn5/uart.c
> new file mode 100644
> index 000000000000..b3c36a5b28d3
> --- /dev/null
> +++ b/drivers/nfc/s3fwrn5/uart.c
> @@ -0,0 +1,250 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * UART Link Layer for S3FWRN82 NCI based Driver
> + *
> + * Copyright (C) 2020 Samsung Electronics
> + * Author: Bongsu Jeon <bongsu.jeon@samsung.com>

You copied a lot from existing i2c.c. Please keep also the original
copyrights.

> + * 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 <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +
> +#include "s3fwrn5.h"
> +
> +#define S3FWRN82_UART_DRIVER_NAME "s3fwrn82_uart"

Remove the define, it is used only once.

> +#define S3FWRN82_NCI_HEADER 3
> +#define S3FWRN82_NCI_IDX 2
> +#define S3FWRN82_EN_WAIT_TIME 20
> +#define NCI_SKB_BUFF_LEN 258
> +
> +struct s3fwrn82_uart_phy {
> +	struct serdev_device *ser_dev;
> +	struct nci_dev *ndev;
> +	struct sk_buff *recv_skb;
> +
> +	unsigned int gpio_en;
> +	unsigned int gpio_fw_wake;
> +
> +	/* mutex is used to synchronize */

Please do not write obvious comments. Mutex is always used to
synchronize, what else is it for? Instead you must describe what exactly
is protected with mutex.

> +	struct mutex mutex;
> +	enum s3fwrn5_mode mode;
> +};
> +
> +static void s3fwrn82_uart_set_wake(void *phy_id, bool wake)
> +{
> +	struct s3fwrn82_uart_phy *phy = phy_id;
> +
> +	mutex_lock(&phy->mutex);
> +	gpio_set_value(phy->gpio_fw_wake, wake);
> +	msleep(S3FWRN82_EN_WAIT_TIME);
> +	mutex_unlock(&phy->mutex);
> +}
> +
> +static void s3fwrn82_uart_set_mode(void *phy_id, enum s3fwrn5_mode mode)
> +{
> +	struct s3fwrn82_uart_phy *phy = phy_id;
> +
> +	mutex_lock(&phy->mutex);
> +	if (phy->mode == mode)
> +		goto out;
> +	phy->mode = mode;
> +	gpio_set_value(phy->gpio_en, 1);
> +	gpio_set_value(phy->gpio_fw_wake, 0);
> +	if (mode == S3FWRN5_MODE_FW)
> +		gpio_set_value(phy->gpio_fw_wake, 1);
> +	if (mode != S3FWRN5_MODE_COLD) {
> +		msleep(S3FWRN82_EN_WAIT_TIME);
> +		gpio_set_value(phy->gpio_en, 0);
> +		msleep(S3FWRN82_EN_WAIT_TIME);
> +	}
> +out:
> +	mutex_unlock(&phy->mutex);
> +}
> +
> +static enum s3fwrn5_mode s3fwrn82_uart_get_mode(void *phy_id)
> +{
> +	struct s3fwrn82_uart_phy *phy = phy_id;
> +	enum s3fwrn5_mode mode;
> +
> +	mutex_lock(&phy->mutex);
> +	mode = phy->mode;
> +	mutex_unlock(&phy->mutex);
> +	return mode;
> +}

All this duplicates I2C version. You need to start either reusing common
blocks.

> +
> +static int s3fwrn82_uart_write(void *phy_id, struct sk_buff *out)
> +{
> +	struct s3fwrn82_uart_phy *phy = phy_id;
> +	int err;
> +
> +	err = serdev_device_write(phy->ser_dev,
> +				  out->data, out->len,
> +				  MAX_SCHEDULE_TIMEOUT);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static const struct s3fwrn5_phy_ops uart_phy_ops = {
> +	.set_wake = s3fwrn82_uart_set_wake,
> +	.set_mode = s3fwrn82_uart_set_mode,
> +	.get_mode = s3fwrn82_uart_get_mode,
> +	.write = s3fwrn82_uart_write,
> +};
> +
> +static int s3fwrn82_uart_read(struct serdev_device *serdev,
> +			      const unsigned char *data,
> +			      size_t count)
> +{
> +	struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
> +	size_t i;
> +
> +	for (i = 0; i < count; i++) {
> +		skb_put_u8(phy->recv_skb, *data++);
> +
> +		if (phy->recv_skb->len < S3FWRN82_NCI_HEADER)
> +			continue;
> +
> +		if ((phy->recv_skb->len - S3FWRN82_NCI_HEADER)
> +				< phy->recv_skb->data[S3FWRN82_NCI_IDX])
> +			continue;
> +
> +		s3fwrn5_recv_frame(phy->ndev, phy->recv_skb, phy->mode);
> +		phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL);
> +		if (!phy->recv_skb)
> +			return 0;
> +	}
> +
> +	return i;
> +}
> +
> +static struct serdev_device_ops s3fwrn82_serdev_ops = {

const

> +	.receive_buf = s3fwrn82_uart_read,
> +	.write_wakeup = serdev_device_write_wakeup,
> +};
> +
> +static const struct of_device_id s3fwrn82_uart_of_match[] = {
> +	{ .compatible = "samsung,s3fwrn82-uart", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, s3fwrn82_uart_of_match);
> +
> +static int s3fwrn82_uart_parse_dt(struct serdev_device *serdev)
> +{
> +	struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
> +	struct device_node *np = serdev->dev.of_node;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	phy->gpio_en = of_get_named_gpio(np, "en-gpios", 0);
> +	if (!gpio_is_valid(phy->gpio_en))
> +		return -ENODEV;
> +
> +	phy->gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);

You should not cast it it unsigned int. I'll fix the s3fwrn5 from which
you copied this apparently.

> +	if (!gpio_is_valid(phy->gpio_fw_wake))
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int s3fwrn82_uart_probe(struct serdev_device *serdev)
> +{
> +	struct s3fwrn82_uart_phy *phy;
> +	int ret = -ENOMEM;
> +
> +	phy = devm_kzalloc(&serdev->dev, sizeof(*phy), GFP_KERNEL);
> +	if (!phy)
> +		goto err_exit;
> +
> +	phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL);
> +	if (!phy->recv_skb)
> +		goto err_free;
> +
> +	mutex_init(&phy->mutex);
> +	phy->mode = S3FWRN5_MODE_COLD;
> +
> +	phy->ser_dev = serdev;
> +	serdev_device_set_drvdata(serdev, phy);
> +	serdev_device_set_client_ops(serdev, &s3fwrn82_serdev_ops);
> +	ret = serdev_device_open(serdev);
> +	if (ret) {
> +		dev_err(&serdev->dev, "Unable to open device\n");
> +		goto err_skb;
> +	}
> +
> +	ret = serdev_device_set_baudrate(serdev, 115200);

Why baudrate is fixed?

> +	if (ret != 115200) {
> +		ret = -EINVAL;
> +		goto err_serdev;
> +	}
> +
> +	serdev_device_set_flow_control(serdev, false);
> +
> +	ret = s3fwrn82_uart_parse_dt(serdev);
> +	if (ret < 0)
> +		goto err_serdev;
> +
> +	ret = devm_gpio_request_one(&phy->ser_dev->dev,
> +				    phy->gpio_en,
> +				    GPIOF_OUT_INIT_HIGH,
> +				    "s3fwrn82_en");

This is weirdly wrapped.

> +	if (ret < 0)
> +		goto err_serdev;
> +
> +	ret = devm_gpio_request_one(&phy->ser_dev->dev,
> +				    phy->gpio_fw_wake,
> +				    GPIOF_OUT_INIT_LOW,
> +				    "s3fwrn82_fw_wake");
> +	if (ret < 0)
> +		goto err_serdev;
> +
> +	ret = s3fwrn5_probe(&phy->ndev, phy, &phy->ser_dev->dev, &uart_phy_ops);
> +	if (ret < 0)
> +		goto err_serdev;
> +
> +	return ret;
> +
> +err_serdev:
> +	serdev_device_close(serdev);
> +err_skb:
> +	kfree_skb(phy->recv_skb);
> +err_free:
> +	kfree(phy);

Eee.... why? Did you test this code?

> +err_exit:
> +	return ret;
> +}
> +
> +static void s3fwrn82_uart_remove(struct serdev_device *serdev)
> +{
> +	struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
> +
> +	s3fwrn5_remove(phy->ndev);
> +	serdev_device_close(serdev);
> +	kfree_skb(phy->recv_skb);
> +	kfree(phy);

This does not look like tested...

Best regards,
Krzysztof

Re: [PATCH net-next 2/2] net: nfc: s3fwrn5: Support a UART interface

[Bongsu Jeon]

On Mon, Nov 23, 2020 at 5:55 PM krzk@kernel.org <krzk@kernel.org> wrote:
>
> On Mon, Nov 23, 2020 at 04:56:58PM +0900, Bongsu Jeon wrote:
> > Since S3FWRN82 NFC Chip, The UART interface can be used.
> > S3FWRN82 uses NCI protocol and supports I2C and UART interface.
> >
> > Signed-off-by: Bongsu Jeon <bongsu.jeon@samsung.com>
>
> Please start sending emails properly, e.g. with git send-email, so all
> your patches in the patchset are referencing the first patch.
>
Ok. I will do that.

> > ---
> >  drivers/nfc/s3fwrn5/Kconfig  |  12 ++
> >  drivers/nfc/s3fwrn5/Makefile |   2 +
> >  drivers/nfc/s3fwrn5/uart.c   | 250 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 264 insertions(+)
> >  create mode 100644 drivers/nfc/s3fwrn5/uart.c
> >
> > diff --git a/drivers/nfc/s3fwrn5/Kconfig b/drivers/nfc/s3fwrn5/Kconfig
> > index 3f8b6da58280..6f88737769e1 100644
> > --- a/drivers/nfc/s3fwrn5/Kconfig
> > +++ b/drivers/nfc/s3fwrn5/Kconfig
> > @@ -20,3 +20,15 @@ config NFC_S3FWRN5_I2C
> >         To compile this driver as a module, choose m here. The module will
> >         be called s3fwrn5_i2c.ko.
> >         Say N if unsure.
> > +
> > +config NFC_S3FWRN82_UART
> > +     tristate "Samsung S3FWRN82 UART support"
> > +     depends on NFC_NCI && SERIAL_DEV_BUS
>
> What about SERIAL_DEV_BUS as module? Shouldn't this be
> SERIAL_DEV_BUS || !SERIAL_DEV_BUS?
>

SERIAL_DEV_BUS is okay even if SERIAL_DEV_BUS is defined as module.

> > +     select NFC_S3FWRN5
> > +     help
> > +       This module adds support for a UART interface to the S3FWRN82 chip.
> > +       Select this if your platform is using the UART bus.
> > +
> > +       To compile this driver as a module, choose m here. The module will
> > +       be called s3fwrn82_uart.ko.
> > +       Say N if unsure.
> > diff --git a/drivers/nfc/s3fwrn5/Makefile b/drivers/nfc/s3fwrn5/Makefile
> > index d0ffa35f50e8..d1902102060b 100644
> > --- a/drivers/nfc/s3fwrn5/Makefile
> > +++ b/drivers/nfc/s3fwrn5/Makefile
> > @@ -5,6 +5,8 @@
> >
> >  s3fwrn5-objs = core.o firmware.o nci.o
> >  s3fwrn5_i2c-objs = i2c.o
> > +s3fwrn82_uart-objs = uart.o
> >
> >  obj-$(CONFIG_NFC_S3FWRN5) += s3fwrn5.o
> >  obj-$(CONFIG_NFC_S3FWRN5_I2C) += s3fwrn5_i2c.o
> > +obj-$(CONFIG_NFC_S3FWRN82_UART) += s3fwrn82_uart.o
> > diff --git a/drivers/nfc/s3fwrn5/uart.c b/drivers/nfc/s3fwrn5/uart.c
> > new file mode 100644
> > index 000000000000..b3c36a5b28d3
> > --- /dev/null
> > +++ b/drivers/nfc/s3fwrn5/uart.c
> > @@ -0,0 +1,250 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * UART Link Layer for S3FWRN82 NCI based Driver
> > + *
> > + * Copyright (C) 2020 Samsung Electronics
> > + * Author: Bongsu Jeon <bongsu.jeon@samsung.com>
>
> You copied a lot from existing i2c.c. Please keep also the original
> copyrights.
>

Okay. I will keep also the original copyrights.

> > + * 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 <linux/gpio.h>
> > +#include <linux/of_gpio.h>
> > +
> > +#include "s3fwrn5.h"
> > +
> > +#define S3FWRN82_UART_DRIVER_NAME "s3fwrn82_uart"
>
> Remove the define, it is used only once.
>
> > +#define S3FWRN82_NCI_HEADER 3
> > +#define S3FWRN82_NCI_IDX 2
> > +#define S3FWRN82_EN_WAIT_TIME 20
> > +#define NCI_SKB_BUFF_LEN 258
> > +
> > +struct s3fwrn82_uart_phy {
> > +     struct serdev_device *ser_dev;
> > +     struct nci_dev *ndev;
> > +     struct sk_buff *recv_skb;
> > +
> > +     unsigned int gpio_en;
> > +     unsigned int gpio_fw_wake;
> > +
> > +     /* mutex is used to synchronize */
>
> Please do not write obvious comments. Mutex is always used to
> synchronize, what else is it for? Instead you must describe what exactly
> is protected with mutex.
>

I understand it. I will fix it.

> > +     struct mutex mutex;
> > +     enum s3fwrn5_mode mode;
> > +};
> > +
> > +static void s3fwrn82_uart_set_wake(void *phy_id, bool wake)
> > +{
> > +     struct s3fwrn82_uart_phy *phy = phy_id;
> > +
> > +     mutex_lock(&phy->mutex);
> > +     gpio_set_value(phy->gpio_fw_wake, wake);
> > +     msleep(S3FWRN82_EN_WAIT_TIME);
> > +     mutex_unlock(&phy->mutex);
> > +}
> > +
> > +static void s3fwrn82_uart_set_mode(void *phy_id, enum s3fwrn5_mode mode)
> > +{
> > +     struct s3fwrn82_uart_phy *phy = phy_id;
> > +
> > +     mutex_lock(&phy->mutex);
> > +     if (phy->mode == mode)
> > +             goto out;
> > +     phy->mode = mode;
> > +     gpio_set_value(phy->gpio_en, 1);
> > +     gpio_set_value(phy->gpio_fw_wake, 0);
> > +     if (mode == S3FWRN5_MODE_FW)
> > +             gpio_set_value(phy->gpio_fw_wake, 1);
> > +     if (mode != S3FWRN5_MODE_COLD) {
> > +             msleep(S3FWRN82_EN_WAIT_TIME);
> > +             gpio_set_value(phy->gpio_en, 0);
> > +             msleep(S3FWRN82_EN_WAIT_TIME);
> > +     }
> > +out:
> > +     mutex_unlock(&phy->mutex);
> > +}
> > +
> > +static enum s3fwrn5_mode s3fwrn82_uart_get_mode(void *phy_id)
> > +{
> > +     struct s3fwrn82_uart_phy *phy = phy_id;
> > +     enum s3fwrn5_mode mode;
> > +
> > +     mutex_lock(&phy->mutex);
> > +     mode = phy->mode;
> > +     mutex_unlock(&phy->mutex);
> > +     return mode;
> > +}
>
> All this duplicates I2C version. You need to start either reusing common
> blocks.
>

Okay. I will do refactoring on i2c.c and uart.c to make common blocks.
 is it okay to separate a patch for it?

> > +
> > +static int s3fwrn82_uart_write(void *phy_id, struct sk_buff *out)
> > +{
> > +     struct s3fwrn82_uart_phy *phy = phy_id;
> > +     int err;
> > +
> > +     err = serdev_device_write(phy->ser_dev,
> > +                               out->data, out->len,
> > +                               MAX_SCHEDULE_TIMEOUT);
> > +     if (err < 0)
> > +             return err;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct s3fwrn5_phy_ops uart_phy_ops = {
> > +     .set_wake = s3fwrn82_uart_set_wake,
> > +     .set_mode = s3fwrn82_uart_set_mode,
> > +     .get_mode = s3fwrn82_uart_get_mode,
> > +     .write = s3fwrn82_uart_write,
> > +};
> > +
> > +static int s3fwrn82_uart_read(struct serdev_device *serdev,
> > +                           const unsigned char *data,
> > +                           size_t count)
> > +{
> > +     struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
> > +     size_t i;
> > +
> > +     for (i = 0; i < count; i++) {
> > +             skb_put_u8(phy->recv_skb, *data++);
> > +
> > +             if (phy->recv_skb->len < S3FWRN82_NCI_HEADER)
> > +                     continue;
> > +
> > +             if ((phy->recv_skb->len - S3FWRN82_NCI_HEADER)
> > +                             < phy->recv_skb->data[S3FWRN82_NCI_IDX])
> > +                     continue;
> > +
> > +             s3fwrn5_recv_frame(phy->ndev, phy->recv_skb, phy->mode);
> > +             phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL);
> > +             if (!phy->recv_skb)
> > +                     return 0;
> > +     }
> > +
> > +     return i;
> > +}
> > +
> > +static struct serdev_device_ops s3fwrn82_serdev_ops = {
>
> const
>
> > +     .receive_buf = s3fwrn82_uart_read,
> > +     .write_wakeup = serdev_device_write_wakeup,
> > +};
> > +
> > +static const struct of_device_id s3fwrn82_uart_of_match[] = {
> > +     { .compatible = "samsung,s3fwrn82-uart", },
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, s3fwrn82_uart_of_match);
> > +
> > +static int s3fwrn82_uart_parse_dt(struct serdev_device *serdev)
> > +{
> > +     struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
> > +     struct device_node *np = serdev->dev.of_node;
> > +
> > +     if (!np)
> > +             return -ENODEV;
> > +
> > +     phy->gpio_en = of_get_named_gpio(np, "en-gpios", 0);
> > +     if (!gpio_is_valid(phy->gpio_en))
> > +             return -ENODEV;
> > +
> > +     phy->gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
>
> You should not cast it it unsigned int. I'll fix the s3fwrn5 from which
> you copied this apparently.
>

Okay. I will fix it.

> > +     if (!gpio_is_valid(phy->gpio_fw_wake))
> > +             return -ENODEV;
> > +
> > +     return 0;
> > +}
> > +
> > +static int s3fwrn82_uart_probe(struct serdev_device *serdev)
> > +{
> > +     struct s3fwrn82_uart_phy *phy;
> > +     int ret = -ENOMEM;
> > +
> > +     phy = devm_kzalloc(&serdev->dev, sizeof(*phy), GFP_KERNEL);
> > +     if (!phy)
> > +             goto err_exit;
> > +
> > +     phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL);
> > +     if (!phy->recv_skb)
> > +             goto err_free;
> > +
> > +     mutex_init(&phy->mutex);
> > +     phy->mode = S3FWRN5_MODE_COLD;
> > +
> > +     phy->ser_dev = serdev;
> > +     serdev_device_set_drvdata(serdev, phy);
> > +     serdev_device_set_client_ops(serdev, &s3fwrn82_serdev_ops);
> > +     ret = serdev_device_open(serdev);
> > +     if (ret) {
> > +             dev_err(&serdev->dev, "Unable to open device\n");
> > +             goto err_skb;
> > +     }
> > +
> > +     ret = serdev_device_set_baudrate(serdev, 115200);
>
> Why baudrate is fixed?
>

RN82 NFC chip only supports 115200 baudrate for UART.

> > +     if (ret != 115200) {
> > +             ret = -EINVAL;
> > +             goto err_serdev;
> > +     }
> > +
> > +     serdev_device_set_flow_control(serdev, false);
> > +
> > +     ret = s3fwrn82_uart_parse_dt(serdev);
> > +     if (ret < 0)
> > +             goto err_serdev;
> > +
> > +     ret = devm_gpio_request_one(&phy->ser_dev->dev,
> > +                                 phy->gpio_en,
> > +                                 GPIOF_OUT_INIT_HIGH,
> > +                                 "s3fwrn82_en");
>
> This is weirdly wrapped.
>

Did you ask about devem_gpio_request_one function's parenthesis and parameters?
If it is right, I changed it after i ran the checkpatch.pl --strict and
i saw message like the alignment should match open parenthesis.

> > +     if (ret < 0)
> > +             goto err_serdev;
> > +
> > +     ret = devm_gpio_request_one(&phy->ser_dev->dev,
> > +                                 phy->gpio_fw_wake,
> > +                                 GPIOF_OUT_INIT_LOW,
> > +                                 "s3fwrn82_fw_wake");
> > +     if (ret < 0)
> > +             goto err_serdev;
> > +
> > +     ret = s3fwrn5_probe(&phy->ndev, phy, &phy->ser_dev->dev, &uart_phy_ops);
> > +     if (ret < 0)
> > +             goto err_serdev;
> > +
> > +     return ret;
> > +
> > +err_serdev:
> > +     serdev_device_close(serdev);
> > +err_skb:
> > +     kfree_skb(phy->recv_skb);
> > +err_free:
> > +     kfree(phy);
>
> Eee.... why? Did you test this code?
>

I didn't test this code. i just added this code as defense code.
If the error happens, then allocated memory and device will be free
according to the fail case.

> > +err_exit:
> > +     return ret;
> > +}
> > +
> > +static void s3fwrn82_uart_remove(struct serdev_device *serdev)
> > +{
> > +     struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
> > +
> > +     s3fwrn5_remove(phy->ndev);
> > +     serdev_device_close(serdev);
> > +     kfree_skb(phy->recv_skb);
> > +     kfree(phy);
>
> This does not look like tested...
>

I tested this code using unbind of the serial device.
It worked and I saw the debugging log that i added for checking the
code to be sure.

> Best regards,
> Krzysztof

Re: [PATCH net-next 2/2] net: nfc: s3fwrn5: Support a UART interface

[krzk]

On Tue, Nov 24, 2020 at 09:05:52PM +0900, Bongsu Jeon wrote:
> On Mon, Nov 23, 2020 at 5:55 PM krzk@kernel.org <krzk@kernel.org> wrote:
> > > +static enum s3fwrn5_mode s3fwrn82_uart_get_mode(void *phy_id)
> > > +{
> > > +     struct s3fwrn82_uart_phy *phy = phy_id;
> > > +     enum s3fwrn5_mode mode;
> > > +
> > > +     mutex_lock(&phy->mutex);
> > > +     mode = phy->mode;
> > > +     mutex_unlock(&phy->mutex);
> > > +     return mode;
> > > +}
> >
> > All this duplicates I2C version. You need to start either reusing common
> > blocks.
> >
> 
> Okay. I will do refactoring on i2c.c and uart.c to make common blocks.
>  is it okay to separate a patch for it?

Yes, that would be the best - refactor the driver to split some common
methods and then in next patch add new s3fwrn82 UART driver.

> > > +
> > > +static int s3fwrn82_uart_write(void *phy_id, struct sk_buff *out)
> > > +{
> > > +     struct s3fwrn82_uart_phy *phy = phy_id;
> > > +     int err;
> > > +
> > > +     err = serdev_device_write(phy->ser_dev,
> > > +                               out->data, out->len,
> > > +                               MAX_SCHEDULE_TIMEOUT);
> > > +     if (err < 0)
> > > +             return err;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static const struct s3fwrn5_phy_ops uart_phy_ops = {
> > > +     .set_wake = s3fwrn82_uart_set_wake,
> > > +     .set_mode = s3fwrn82_uart_set_mode,
> > > +     .get_mode = s3fwrn82_uart_get_mode,
> > > +     .write = s3fwrn82_uart_write,
> > > +};
> > > +
> > > +static int s3fwrn82_uart_read(struct serdev_device *serdev,
> > > +                           const unsigned char *data,
> > > +                           size_t count)
> > > +{
> > > +     struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
> > > +     size_t i;
> > > +
> > > +     for (i = 0; i < count; i++) {
> > > +             skb_put_u8(phy->recv_skb, *data++);
> > > +
> > > +             if (phy->recv_skb->len < S3FWRN82_NCI_HEADER)
> > > +                     continue;
> > > +
> > > +             if ((phy->recv_skb->len - S3FWRN82_NCI_HEADER)
> > > +                             < phy->recv_skb->data[S3FWRN82_NCI_IDX])
> > > +                     continue;
> > > +
> > > +             s3fwrn5_recv_frame(phy->ndev, phy->recv_skb, phy->mode);
> > > +             phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL);
> > > +             if (!phy->recv_skb)
> > > +                     return 0;
> > > +     }
> > > +
> > > +     return i;
> > > +}
> > > +
> > > +static struct serdev_device_ops s3fwrn82_serdev_ops = {
> >
> > const
> >
> > > +     .receive_buf = s3fwrn82_uart_read,
> > > +     .write_wakeup = serdev_device_write_wakeup,
> > > +};
> > > +
> > > +static const struct of_device_id s3fwrn82_uart_of_match[] = {
> > > +     { .compatible = "samsung,s3fwrn82-uart", },
> > > +     {},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, s3fwrn82_uart_of_match);
> > > +
> > > +static int s3fwrn82_uart_parse_dt(struct serdev_device *serdev)
> > > +{
> > > +     struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
> > > +     struct device_node *np = serdev->dev.of_node;
> > > +
> > > +     if (!np)
> > > +             return -ENODEV;
> > > +
> > > +     phy->gpio_en = of_get_named_gpio(np, "en-gpios", 0);
> > > +     if (!gpio_is_valid(phy->gpio_en))
> > > +             return -ENODEV;
> > > +
> > > +     phy->gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
> >
> > You should not cast it it unsigned int. I'll fix the s3fwrn5 from which
> > you copied this apparently.
> >
> 
> Okay. I will fix it.
> 
> > > +     if (!gpio_is_valid(phy->gpio_fw_wake))
> > > +             return -ENODEV;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int s3fwrn82_uart_probe(struct serdev_device *serdev)
> > > +{
> > > +     struct s3fwrn82_uart_phy *phy;
> > > +     int ret = -ENOMEM;
> > > +
> > > +     phy = devm_kzalloc(&serdev->dev, sizeof(*phy), GFP_KERNEL);
> > > +     if (!phy)
> > > +             goto err_exit;
> > > +
> > > +     phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL);
> > > +     if (!phy->recv_skb)
> > > +             goto err_free;
> > > +
> > > +     mutex_init(&phy->mutex);
> > > +     phy->mode = S3FWRN5_MODE_COLD;
> > > +
> > > +     phy->ser_dev = serdev;
> > > +     serdev_device_set_drvdata(serdev, phy);
> > > +     serdev_device_set_client_ops(serdev, &s3fwrn82_serdev_ops);
> > > +     ret = serdev_device_open(serdev);
> > > +     if (ret) {
> > > +             dev_err(&serdev->dev, "Unable to open device\n");
> > > +             goto err_skb;
> > > +     }
> > > +
> > > +     ret = serdev_device_set_baudrate(serdev, 115200);
> >
> > Why baudrate is fixed?
> >
> 
> RN82 NFC chip only supports 115200 baudrate for UART.

OK, I guess it could be extended in the future for other frequencies, if
needed.

> 
> > > +     if (ret != 115200) {
> > > +             ret = -EINVAL;
> > > +             goto err_serdev;
> > > +     }
> > > +
> > > +     serdev_device_set_flow_control(serdev, false);
> > > +
> > > +     ret = s3fwrn82_uart_parse_dt(serdev);
> > > +     if (ret < 0)
> > > +             goto err_serdev;
> > > +
> > > +     ret = devm_gpio_request_one(&phy->ser_dev->dev,
> > > +                                 phy->gpio_en,
> > > +                                 GPIOF_OUT_INIT_HIGH,
> > > +                                 "s3fwrn82_en");
> >
> > This is weirdly wrapped.
> >
> 
> Did you ask about devem_gpio_request_one function's parenthesis and parameters?
> If it is right, I changed it after i ran the checkpatch.pl --strict and
> i saw message like the alignment should match open parenthesis.

Yeah, but it does not mean to wrap after each argument. It should be
something like:

        ret = devm_gpio_request_one(&phy->ser_dev->dev, phy->gpio_en,
                                    GPIOF_OUT_INIT_HIGH, "s3fwrn82_en");

> 
> > > +     if (ret < 0)
> > > +             goto err_serdev;
> > > +
> > > +     ret = devm_gpio_request_one(&phy->ser_dev->dev,
> > > +                                 phy->gpio_fw_wake,
> > > +                                 GPIOF_OUT_INIT_LOW,
> > > +                                 "s3fwrn82_fw_wake");
> > > +     if (ret < 0)
> > > +             goto err_serdev;
> > > +
> > > +     ret = s3fwrn5_probe(&phy->ndev, phy, &phy->ser_dev->dev, &uart_phy_ops);
> > > +     if (ret < 0)
> > > +             goto err_serdev;
> > > +
> > > +     return ret;
> > > +
> > > +err_serdev:
> > > +     serdev_device_close(serdev);
> > > +err_skb:
> > > +     kfree_skb(phy->recv_skb);
> > > +err_free:
> > > +     kfree(phy);
> >
> > Eee.... why? Did you test this code?
> >
> 
> I didn't test this code. i just added this code as defense code.
> If the error happens, then allocated memory and device will be free
> according to the fail case.

Really, this won't work. It's kind of obvious why... You cannot use
kfree() on memory which is not allocated with kzalloc(). Or IOW, you
cannot use it if it is being freed by devm.

I doubt that you tested either this or the remove callback because if
you did test it, you would see easily:

[  145.018200] Unable to handle kernel paging request at virtual address ffffffed
[  145.025428] pgd = 67e71ef8
[  145.027774] [ffffffed] *pgd=6fffd861, *pte=00000000, *ppte=00000000
[  145.034052] Internal error: Oops: 837 [#1] PREEMPT SMP ARM
[  145.039278] Modules linked in: s5p_mfc exynos_gsc s5p_jpeg v4l2_mem2mem videobuf2_dma_contig videobuf2_memops videobuf2_v4l2 videobuf2_common videodev mc
[  145.052987] CPU: 2 PID: 325 Comm: bash Tainted: G        W         5.10.0-rc3-next-20201113-00008-g62dd0da04641-dirty #79
[  145.063883] Hardware name: Samsung Exynos (Flattened Device Tree)
[  145.069971] PC is at devres_remove+0x9c/0xb4
[  145.074180] LR is at devres_remove+0x78/0xb4
[  145.078418] pc : [<c06a6134>]    lr : [<c06a6110>]    psr: a0010093
[  145.084666] sp : c6af7de0  ip : 00000001  fp : c2dd16e4
[  145.089861] r10: c2dd16c0  r9 : 60010013  r8 : c05894a8
[  145.095060] r7 : c058b128  r6 : c4bbc100  r5 : c2dd1410  r4 : c440d5c0
[  145.101564] r3 : ffffffed  r2 : c4bbc100  r1 : 60010013  r0 : c2dd16c0
[  145.108068] Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
[  145.115260] Control: 10c5387d  Table: 46aa006a  DAC: 00000051
[  145.120979] Process bash (pid: 325, stack limit = 0x97d30bf6)
[  145.126696] Stack: (0xc6af7de0 to 0xc6af8000)
[  145.131039] 7de0: c4bbc100 c2dd1410 c058b128 c2dd1410 c440d5c0 c39a3400 c1305f08 c1348d28
[  145.139185] 7e00: 0000002b c06a6a54 c2dd1410 00000000 c440d5c0 c058ab94 c2dd1410 c06ccd04
[  145.147333] 7e20: c2dd1410 c19ab618 00000000 c19ab620 c39a3400 c06a1ff8 00000000 c2dd1524
[  145.155476] 7e40: c1c16800 c2dd1410 c1305f08 c1305f08 c1348d28 c39a3400 c6af7f78 c39a3410
[  145.163621] 7e60: c1c16800 c06a2624 c1305f08 c1305f08 0000000c c2dd1410 00000000 c1305f08
[  145.171769] 7e80: 0000000c c39a3400 c6af7f78 c39a3410 c1c16800 c06a29ec c2dd1410 c1305f08
[  145.179914] 7ea0: c12f0410 c06a0728 0000000c c4bbc140 00000000 00000000 c39a3400 c0381968
[  145.188058] 7ec0: 00000000 00000000 00000000 c5164640 0000000c 005e0da0 c6af7f78 c02d6120
[  145.196202] 7ee0: c038185c c02d5b70 00000001 00000000 c02d6120 00000000 00000000 c133c27a
[  145.204353] 7f00: c39f5690 c127b15c c116e588 c0b40ef0 c133c14f c3af4450 c39f5670 00000000
[  145.212497] 7f20: c3af4450 c018ea90 c120958c c39f5670 c3af4450 c116e588 c02f9988 c0197a00
[  145.220639] 7f40: c116e588 c0b40ef0 00000003 c1208ec8 00000000 c5164640 c5164640 00000000
[  145.228784] 7f60: 00000000 005e0da0 0000000c 00000004 00000000 c02d6120 00000000 00000000
[  145.236931] 7f80: 00000006 c1208ec8 c5164640 0000000c 005e0da0 b6fec680 00000004 c0100244
[  145.245077] 7fa0: c6af6000 c0100060 0000000c 005e0da0 00000001 005e0da0 0000000c 00000000
[  145.253222] 7fc0: 0000000c 005e0da0 b6fec680 00000004 b6f6310c b6f62c74 00000000 00000000
[  145.261370] 7fe0: 00000498 bef78290 b6e84ea4 b6ee2300 60010010 00000001 00000000 00000000
[  145.269555] [<c06a6134>] (devres_remove) from [<c06a6a54>] (devres_release+0x10/0x3c)
[  145.277340] [<c06a6a54>] (devres_release) from [<c058ab94>] (devm_pinctrl_put+0x20/0x44)
[  145.285388] [<c058ab94>] (devm_pinctrl_put) from [<c06ccd04>] (pinctrl_bind_pins+0xd0/0x27c)
[  145.293786] [<c06ccd04>] (pinctrl_bind_pins) from [<c06a1ff8>] (really_probe+0x90/0x4f0)
[  145.301842] [<c06a1ff8>] (really_probe) from [<c06a2624>] (driver_probe_device+0x78/0x1e0)
[  145.310069] [<c06a2624>] (driver_probe_device) from [<c06a29ec>] (device_driver_attach+0x58/0x60)
[  145.318936] [<c06a29ec>] (device_driver_attach) from [<c06a0728>] (bind_store+0x8c/0x100)
[  145.327074] [<c06a0728>] (bind_store) from [<c0381968>] (kernfs_fop_write+0x10c/0x230)
[  145.334960] [<c0381968>] (kernfs_fop_write) from [<c02d5b70>] (vfs_write+0xcc/0x524)
[  145.342658] [<c02d5b70>] (vfs_write) from [<c02d6120>] (ksys_write+0x64/0xf0)
[  145.349759] [<c02d6120>] (ksys_write) from [<c0100060>] (ret_fast_syscall+0x0/0x2c)
[  145.357377] Exception stack(0xc6af7fa8 to 0xc6af7ff0)
[  145.362395] 7fa0:                   0000000c 005e0da0 00000001 005e0da0 0000000c 00000000
[  145.370551] 7fc0: 0000000c 005e0da0 b6fec680 00000004 b6f6310c b6f62c74 00000000 00000000
[  145.378699] 7fe0: 00000498 bef78290 b6e84ea4 b6ee2300
[  145.383719] Code: e1a0000a e5942000 e1a01009 e5823004 (e5832000)
[  145.389795] ---[ end trace 769f050185612ec3 ]---

Please fix the double-free.

Best regards,
Krzysztof

Re: [PATCH net-next 2/2] net: nfc: s3fwrn5: Support a UART interface

[Bongsu Jeon]

On 11/24/20, krzk@kernel.org <krzk@kernel.org> wrote:
> On Tue, Nov 24, 2020 at 09:05:52PM +0900, Bongsu Jeon wrote:
>> On Mon, Nov 23, 2020 at 5:55 PM krzk@kernel.org <krzk@kernel.org> wrote:
>> > > +static enum s3fwrn5_mode s3fwrn82_uart_get_mode(void *phy_id)
>> > > +{
>> > > +     struct s3fwrn82_uart_phy *phy = phy_id;
>> > > +     enum s3fwrn5_mode mode;
>> > > +
>> > > +     mutex_lock(&phy->mutex);
>> > > +     mode = phy->mode;
>> > > +     mutex_unlock(&phy->mutex);
>> > > +     return mode;
>> > > +}
>> >
>> > All this duplicates I2C version. You need to start either reusing
>> > common
>> > blocks.
>> >
>>
>> Okay. I will do refactoring on i2c.c and uart.c to make common blocks.
>>  is it okay to separate a patch for it?
>
> Yes, that would be the best - refactor the driver to split some common
> methods and then in next patch add new s3fwrn82 UART driver.
>
>> > > +
>> > > +static int s3fwrn82_uart_write(void *phy_id, struct sk_buff *out)
>> > > +{
>> > > +     struct s3fwrn82_uart_phy *phy = phy_id;
>> > > +     int err;
>> > > +
>> > > +     err = serdev_device_write(phy->ser_dev,
>> > > +                               out->data, out->len,
>> > > +                               MAX_SCHEDULE_TIMEOUT);
>> > > +     if (err < 0)
>> > > +             return err;
>> > > +
>> > > +     return 0;
>> > > +}
>> > > +
>> > > +static const struct s3fwrn5_phy_ops uart_phy_ops = {
>> > > +     .set_wake = s3fwrn82_uart_set_wake,
>> > > +     .set_mode = s3fwrn82_uart_set_mode,
>> > > +     .get_mode = s3fwrn82_uart_get_mode,
>> > > +     .write = s3fwrn82_uart_write,
>> > > +};
>> > > +
>> > > +static int s3fwrn82_uart_read(struct serdev_device *serdev,
>> > > +                           const unsigned char *data,
>> > > +                           size_t count)
>> > > +{
>> > > +     struct s3fwrn82_uart_phy *phy =
>> > > serdev_device_get_drvdata(serdev);
>> > > +     size_t i;
>> > > +
>> > > +     for (i = 0; i < count; i++) {
>> > > +             skb_put_u8(phy->recv_skb, *data++);
>> > > +
>> > > +             if (phy->recv_skb->len < S3FWRN82_NCI_HEADER)
>> > > +                     continue;
>> > > +
>> > > +             if ((phy->recv_skb->len - S3FWRN82_NCI_HEADER)
>> > > +                             <
>> > > phy->recv_skb->data[S3FWRN82_NCI_IDX])
>> > > +                     continue;
>> > > +
>> > > +             s3fwrn5_recv_frame(phy->ndev, phy->recv_skb,
>> > > phy->mode);
>> > > +             phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN,
>> > > GFP_KERNEL);
>> > > +             if (!phy->recv_skb)
>> > > +                     return 0;
>> > > +     }
>> > > +
>> > > +     return i;
>> > > +}
>> > > +
>> > > +static struct serdev_device_ops s3fwrn82_serdev_ops = {
>> >
>> > const
>> >
>> > > +     .receive_buf = s3fwrn82_uart_read,
>> > > +     .write_wakeup = serdev_device_write_wakeup,
>> > > +};
>> > > +
>> > > +static const struct of_device_id s3fwrn82_uart_of_match[] = {
>> > > +     { .compatible = "samsung,s3fwrn82-uart", },
>> > > +     {},
>> > > +};
>> > > +MODULE_DEVICE_TABLE(of, s3fwrn82_uart_of_match);
>> > > +
>> > > +static int s3fwrn82_uart_parse_dt(struct serdev_device *serdev)
>> > > +{
>> > > +     struct s3fwrn82_uart_phy *phy =
>> > > serdev_device_get_drvdata(serdev);
>> > > +     struct device_node *np = serdev->dev.of_node;
>> > > +
>> > > +     if (!np)
>> > > +             return -ENODEV;
>> > > +
>> > > +     phy->gpio_en = of_get_named_gpio(np, "en-gpios", 0);
>> > > +     if (!gpio_is_valid(phy->gpio_en))
>> > > +             return -ENODEV;
>> > > +
>> > > +     phy->gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
>> >
>> > You should not cast it it unsigned int. I'll fix the s3fwrn5 from which
>> > you copied this apparently.
>> >
>>
>> Okay. I will fix it.
>>
>> > > +     if (!gpio_is_valid(phy->gpio_fw_wake))
>> > > +             return -ENODEV;
>> > > +
>> > > +     return 0;
>> > > +}
>> > > +
>> > > +static int s3fwrn82_uart_probe(struct serdev_device *serdev)
>> > > +{
>> > > +     struct s3fwrn82_uart_phy *phy;
>> > > +     int ret = -ENOMEM;
>> > > +
>> > > +     phy = devm_kzalloc(&serdev->dev, sizeof(*phy), GFP_KERNEL);
>> > > +     if (!phy)
>> > > +             goto err_exit;
>> > > +
>> > > +     phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL);
>> > > +     if (!phy->recv_skb)
>> > > +             goto err_free;
>> > > +
>> > > +     mutex_init(&phy->mutex);
>> > > +     phy->mode = S3FWRN5_MODE_COLD;
>> > > +
>> > > +     phy->ser_dev = serdev;
>> > > +     serdev_device_set_drvdata(serdev, phy);
>> > > +     serdev_device_set_client_ops(serdev, &s3fwrn82_serdev_ops);
>> > > +     ret = serdev_device_open(serdev);
>> > > +     if (ret) {
>> > > +             dev_err(&serdev->dev, "Unable to open device\n");
>> > > +             goto err_skb;
>> > > +     }
>> > > +
>> > > +     ret = serdev_device_set_baudrate(serdev, 115200);
>> >
>> > Why baudrate is fixed?
>> >
>>
>> RN82 NFC chip only supports 115200 baudrate for UART.
>
> OK, I guess it could be extended in the future for other frequencies, if
> needed.
>
>>
>> > > +     if (ret != 115200) {
>> > > +             ret = -EINVAL;
>> > > +             goto err_serdev;
>> > > +     }
>> > > +
>> > > +     serdev_device_set_flow_control(serdev, false);
>> > > +
>> > > +     ret = s3fwrn82_uart_parse_dt(serdev);
>> > > +     if (ret < 0)
>> > > +             goto err_serdev;
>> > > +
>> > > +     ret = devm_gpio_request_one(&phy->ser_dev->dev,
>> > > +                                 phy->gpio_en,
>> > > +                                 GPIOF_OUT_INIT_HIGH,
>> > > +                                 "s3fwrn82_en");
>> >
>> > This is weirdly wrapped.
>> >
>>
>> Did you ask about devem_gpio_request_one function's parenthesis and
>> parameters?
>> If it is right, I changed it after i ran the checkpatch.pl --strict and
>> i saw message like the alignment should match open parenthesis.
>
> Yeah, but it does not mean to wrap after each argument. It should be
> something like:
>
>         ret = devm_gpio_request_one(&phy->ser_dev->dev, phy->gpio_en,
>                                     GPIOF_OUT_INIT_HIGH, "s3fwrn82_en");
>
>>
>> > > +     if (ret < 0)
>> > > +             goto err_serdev;
>> > > +
>> > > +     ret = devm_gpio_request_one(&phy->ser_dev->dev,
>> > > +                                 phy->gpio_fw_wake,
>> > > +                                 GPIOF_OUT_INIT_LOW,
>> > > +                                 "s3fwrn82_fw_wake");
>> > > +     if (ret < 0)
>> > > +             goto err_serdev;
>> > > +
>> > > +     ret = s3fwrn5_probe(&phy->ndev, phy, &phy->ser_dev->dev,
>> > > &uart_phy_ops);
>> > > +     if (ret < 0)
>> > > +             goto err_serdev;
>> > > +
>> > > +     return ret;
>> > > +
>> > > +err_serdev:
>> > > +     serdev_device_close(serdev);
>> > > +err_skb:
>> > > +     kfree_skb(phy->recv_skb);
>> > > +err_free:
>> > > +     kfree(phy);
>> >
>> > Eee.... why? Did you test this code?
>> >
>>
>> I didn't test this code. i just added this code as defense code.
>> If the error happens, then allocated memory and device will be free
>> according to the fail case.
>
> Really, this won't work. It's kind of obvious why... You cannot use
> kfree() on memory which is not allocated with kzalloc(). Or IOW, you
> cannot use it if it is being freed by devm.
>
> I doubt that you tested either this or the remove callback because if
> you did test it, you would see easily:
>

Thanks to explain it in detail.

> Please fix the double-free.
>

I understand it and will remove the kfree(phy).
And i did the remove callback test using following echo command's
parameters on raspberry pi.
But i didn't see the error log like yours.

Echo serial0-0 > /sys/bus/serial/devices/serial0/serial0-0/driver/unbind

> Best regards,
> Krzysztof
>
>

Re: [PATCH net-next 2/2] net: nfc: s3fwrn5: Support a UART interface

[Bongsu Jeon]

On 11/25/20, Bongsu Jeon <bongsu.jeon2@gmail.com> wrote:
> On 11/24/20, krzk@kernel.org <krzk@kernel.org> wrote:
>> On Tue, Nov 24, 2020 at 09:05:52PM +0900, Bongsu Jeon wrote:
>>> On Mon, Nov 23, 2020 at 5:55 PM krzk@kernel.org <krzk@kernel.org> wrote:
>>> > > +static enum s3fwrn5_mode s3fwrn82_uart_get_mode(void *phy_id)
>>> > > +{
>>> > > +     struct s3fwrn82_uart_phy *phy = phy_id;
>>> > > +     enum s3fwrn5_mode mode;
>>> > > +
>>> > > +     mutex_lock(&phy->mutex);
>>> > > +     mode = phy->mode;
>>> > > +     mutex_unlock(&phy->mutex);
>>> > > +     return mode;
>>> > > +}
>>> >
>>> > All this duplicates I2C version. You need to start either reusing
>>> > common
>>> > blocks.
>>> >
>>>
>>> Okay. I will do refactoring on i2c.c and uart.c to make common blocks.
>>>  is it okay to separate a patch for it?
>>
>> Yes, that would be the best - refactor the driver to split some common
>> methods and then in next patch add new s3fwrn82 UART driver.
>>
>>> > > +
>>> > > +static int s3fwrn82_uart_write(void *phy_id, struct sk_buff *out)
>>> > > +{
>>> > > +     struct s3fwrn82_uart_phy *phy = phy_id;
>>> > > +     int err;
>>> > > +
>>> > > +     err = serdev_device_write(phy->ser_dev,
>>> > > +                               out->data, out->len,
>>> > > +                               MAX_SCHEDULE_TIMEOUT);
>>> > > +     if (err < 0)
>>> > > +             return err;
>>> > > +
>>> > > +     return 0;
>>> > > +}
>>> > > +
>>> > > +static const struct s3fwrn5_phy_ops uart_phy_ops = {
>>> > > +     .set_wake = s3fwrn82_uart_set_wake,
>>> > > +     .set_mode = s3fwrn82_uart_set_mode,
>>> > > +     .get_mode = s3fwrn82_uart_get_mode,
>>> > > +     .write = s3fwrn82_uart_write,
>>> > > +};
>>> > > +
>>> > > +static int s3fwrn82_uart_read(struct serdev_device *serdev,
>>> > > +                           const unsigned char *data,
>>> > > +                           size_t count)
>>> > > +{
>>> > > +     struct s3fwrn82_uart_phy *phy =
>>> > > serdev_device_get_drvdata(serdev);
>>> > > +     size_t i;
>>> > > +
>>> > > +     for (i = 0; i < count; i++) {
>>> > > +             skb_put_u8(phy->recv_skb, *data++);
>>> > > +
>>> > > +             if (phy->recv_skb->len < S3FWRN82_NCI_HEADER)
>>> > > +                     continue;
>>> > > +
>>> > > +             if ((phy->recv_skb->len - S3FWRN82_NCI_HEADER)
>>> > > +                             <
>>> > > phy->recv_skb->data[S3FWRN82_NCI_IDX])
>>> > > +                     continue;
>>> > > +
>>> > > +             s3fwrn5_recv_frame(phy->ndev, phy->recv_skb,
>>> > > phy->mode);
>>> > > +             phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN,
>>> > > GFP_KERNEL);
>>> > > +             if (!phy->recv_skb)
>>> > > +                     return 0;
>>> > > +     }
>>> > > +
>>> > > +     return i;
>>> > > +}
>>> > > +
>>> > > +static struct serdev_device_ops s3fwrn82_serdev_ops = {
>>> >
>>> > const
>>> >
>>> > > +     .receive_buf = s3fwrn82_uart_read,
>>> > > +     .write_wakeup = serdev_device_write_wakeup,
>>> > > +};
>>> > > +
>>> > > +static const struct of_device_id s3fwrn82_uart_of_match[] = {
>>> > > +     { .compatible = "samsung,s3fwrn82-uart", },
>>> > > +     {},
>>> > > +};
>>> > > +MODULE_DEVICE_TABLE(of, s3fwrn82_uart_of_match);
>>> > > +
>>> > > +static int s3fwrn82_uart_parse_dt(struct serdev_device *serdev)
>>> > > +{
>>> > > +     struct s3fwrn82_uart_phy *phy =
>>> > > serdev_device_get_drvdata(serdev);
>>> > > +     struct device_node *np = serdev->dev.of_node;
>>> > > +
>>> > > +     if (!np)
>>> > > +             return -ENODEV;
>>> > > +
>>> > > +     phy->gpio_en = of_get_named_gpio(np, "en-gpios", 0);
>>> > > +     if (!gpio_is_valid(phy->gpio_en))
>>> > > +             return -ENODEV;
>>> > > +
>>> > > +     phy->gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
>>> >
>>> > You should not cast it it unsigned int. I'll fix the s3fwrn5 from
>>> > which
>>> > you copied this apparently.
>>> >
>>>
>>> Okay. I will fix it.
>>>
>>> > > +     if (!gpio_is_valid(phy->gpio_fw_wake))
>>> > > +             return -ENODEV;
>>> > > +
>>> > > +     return 0;
>>> > > +}
>>> > > +
>>> > > +static int s3fwrn82_uart_probe(struct serdev_device *serdev)
>>> > > +{
>>> > > +     struct s3fwrn82_uart_phy *phy;
>>> > > +     int ret = -ENOMEM;
>>> > > +
>>> > > +     phy = devm_kzalloc(&serdev->dev, sizeof(*phy), GFP_KERNEL);
>>> > > +     if (!phy)
>>> > > +             goto err_exit;
>>> > > +
>>> > > +     phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL);
>>> > > +     if (!phy->recv_skb)
>>> > > +             goto err_free;
>>> > > +
>>> > > +     mutex_init(&phy->mutex);
>>> > > +     phy->mode = S3FWRN5_MODE_COLD;
>>> > > +
>>> > > +     phy->ser_dev = serdev;
>>> > > +     serdev_device_set_drvdata(serdev, phy);
>>> > > +     serdev_device_set_client_ops(serdev, &s3fwrn82_serdev_ops);
>>> > > +     ret = serdev_device_open(serdev);
>>> > > +     if (ret) {
>>> > > +             dev_err(&serdev->dev, "Unable to open device\n");
>>> > > +             goto err_skb;
>>> > > +     }
>>> > > +
>>> > > +     ret = serdev_device_set_baudrate(serdev, 115200);
>>> >
>>> > Why baudrate is fixed?
>>> >
>>>
>>> RN82 NFC chip only supports 115200 baudrate for UART.
>>
>> OK, I guess it could be extended in the future for other frequencies, if
>> needed.
>>
>>>
>>> > > +     if (ret != 115200) {
>>> > > +             ret = -EINVAL;
>>> > > +             goto err_serdev;
>>> > > +     }
>>> > > +
>>> > > +     serdev_device_set_flow_control(serdev, false);
>>> > > +
>>> > > +     ret = s3fwrn82_uart_parse_dt(serdev);
>>> > > +     if (ret < 0)
>>> > > +             goto err_serdev;
>>> > > +
>>> > > +     ret = devm_gpio_request_one(&phy->ser_dev->dev,
>>> > > +                                 phy->gpio_en,
>>> > > +                                 GPIOF_OUT_INIT_HIGH,
>>> > > +                                 "s3fwrn82_en");
>>> >
>>> > This is weirdly wrapped.
>>> >
>>>
>>> Did you ask about devem_gpio_request_one function's parenthesis and
>>> parameters?
>>> If it is right, I changed it after i ran the checkpatch.pl --strict and
>>> i saw message like the alignment should match open parenthesis.
>>
>> Yeah, but it does not mean to wrap after each argument. It should be
>> something like:
>>
>>         ret = devm_gpio_request_one(&phy->ser_dev->dev, phy->gpio_en,
>>                                     GPIOF_OUT_INIT_HIGH, "s3fwrn82_en");
>>
>>>
>>> > > +     if (ret < 0)
>>> > > +             goto err_serdev;
>>> > > +
>>> > > +     ret = devm_gpio_request_one(&phy->ser_dev->dev,
>>> > > +                                 phy->gpio_fw_wake,
>>> > > +                                 GPIOF_OUT_INIT_LOW,
>>> > > +                                 "s3fwrn82_fw_wake");
>>> > > +     if (ret < 0)
>>> > > +             goto err_serdev;
>>> > > +
>>> > > +     ret = s3fwrn5_probe(&phy->ndev, phy, &phy->ser_dev->dev,
>>> > > &uart_phy_ops);
>>> > > +     if (ret < 0)
>>> > > +             goto err_serdev;
>>> > > +
>>> > > +     return ret;
>>> > > +
>>> > > +err_serdev:
>>> > > +     serdev_device_close(serdev);
>>> > > +err_skb:
>>> > > +     kfree_skb(phy->recv_skb);
>>> > > +err_free:
>>> > > +     kfree(phy);
>>> >
>>> > Eee.... why? Did you test this code?
>>> >
>>>
>>> I didn't test this code. i just added this code as defense code.
>>> If the error happens, then allocated memory and device will be free
>>> according to the fail case.
>>
>> Really, this won't work. It's kind of obvious why... You cannot use
>> kfree() on memory which is not allocated with kzalloc(). Or IOW, you
>> cannot use it if it is being freed by devm.
>>
>> I doubt that you tested either this or the remove callback because if
>> you did test it, you would see easily:
>>
>
> Thanks to explain it in detail.
>
>> Please fix the double-free.
>>
>
> I understand it and will remove the kfree(phy).
> And i did the remove callback test using following echo command's
> parameters on raspberry pi.
> But i didn't see the error log like yours.
>
> Echo serial0-0 > /sys/bus/serial/devices/serial0/serial0-0/driver/unbind
>

Sorry to reply this email in a row.
I could see the log like yours when i changed the code at uart probe
functiom to make an error situation by force as below.
ret = -EINVAL;
     // s3fwrn5_probe(~~~

and i couldn't see the log when i removed the kfree(phy).
Thanks to mention it.


>> Best regards,
>> Krzysztof
>>
>>
>