| Message ID | 20190820120345.22593-5-poeschel@lemonage.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v6,1/7] nfc: pn533: i2c: "pn532" as dt compatible string | expand |
Hi Lars, On 20.08.2019 15:03, Lars Poeschel wrote: > This adds the UART phy interface for the pn533 driver. > The pn533 driver can be used through UART interface this way. > It is implemented as a serdev device. > > Cc: Johan Hovold <johan@kernel.org> > Signed-off-by: Lars Poeschel <poeschel@lemonage.de> > --- > Changes in v6: > - Rebased the patch series on v5.3-rc5 > > Changes in v5: > - Use the splitted pn53x_common_init and pn53x_register_nfc > and pn53x_common_clean and pn53x_unregister_nfc alike > > Changes in v4: > - SPDX-License-Identifier: GPL-2.0+ > - Source code comments above refering items > - Error check for serdev_device_write's > - Change if (xxx == NULL) to if (!xxx) > - Remove device name from a dev_err > - move pn533_register in _probe a bit towards the end of _probe > - make use of newly added dev_up / dev_down phy_ops > - control send_wakeup variable from dev_up / dev_down > > Changes in v3: > - depend on SERIAL_DEV_BUS in Kconfig > > Changes in v2: > - switched from tty line discipline to serdev, resulting in many > simplifications > - SPDX License Identifier > > drivers/nfc/pn533/Kconfig | 11 ++ > drivers/nfc/pn533/Makefile | 2 + > drivers/nfc/pn533/pn533.h | 8 + > drivers/nfc/pn533/uart.c | 316 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 337 insertions(+) > create mode 100644 drivers/nfc/pn533/uart.c > > diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig > index f6d6b345ba0d..7fe1bbe26568 100644 > --- a/drivers/nfc/pn533/Kconfig > +++ b/drivers/nfc/pn533/Kconfig > @@ -26,3 +26,14 @@ config NFC_PN533_I2C > > If you choose to build a module, it'll be called pn533_i2c. > Say N if unsure. > + > +config NFC_PN532_UART > + tristate "NFC PN532 device support (UART)" > + depends on SERIAL_DEV_BUS > + select NFC_PN533 > + ---help--- > + This module adds support for the NXP pn532 UART interface. > + Select this if your platform is using the UART bus. > + > + If you choose to build a module, it'll be called pn532_uart. > + Say N if unsure. > diff --git a/drivers/nfc/pn533/Makefile b/drivers/nfc/pn533/Makefile > index 43c25b4f9466..b9648337576f 100644 > --- a/drivers/nfc/pn533/Makefile > +++ b/drivers/nfc/pn533/Makefile > @@ -4,7 +4,9 @@ > # > pn533_usb-objs = usb.o > pn533_i2c-objs = i2c.o > +pn532_uart-objs = uart.o > > obj-$(CONFIG_NFC_PN533) += pn533.o > obj-$(CONFIG_NFC_PN533_USB) += pn533_usb.o > obj-$(CONFIG_NFC_PN533_I2C) += pn533_i2c.o > +obj-$(CONFIG_NFC_PN532_UART) += pn532_uart.o > diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h > index 510ddebbd896..6541088fad73 100644 > --- a/drivers/nfc/pn533/pn533.h > +++ b/drivers/nfc/pn533/pn533.h > @@ -43,6 +43,11 @@ > > /* Preamble (1), SoPC (2), ACK Code (2), Postamble (1) */ > #define PN533_STD_FRAME_ACK_SIZE 6 > +/* > + * Preamble (1), SoPC (2), Packet Length (1), Packet Length Checksum (1), > + * Specific Application Level Error Code (1) , Postamble (1) > + */ > +#define PN533_STD_ERROR_FRAME_SIZE 8 > #define PN533_STD_FRAME_CHECKSUM(f) (f->data[f->datalen]) > #define PN533_STD_FRAME_POSTAMBLE(f) (f->data[f->datalen + 1]) > /* Half start code (3), LEN (4) should be 0xffff for extended frame */ > @@ -84,6 +89,9 @@ > #define PN533_CMD_MI_MASK 0x40 > #define PN533_CMD_RET_SUCCESS 0x00 > > +#define PN533_FRAME_DATALEN_ACK 0x00 > +#define PN533_FRAME_DATALEN_ERROR 0x01 > +#define PN533_FRAME_DATALEN_EXTENDED 0xFF > > enum pn533_protocol_type { > PN533_PROTO_REQ_ACK_RESP = 0, > diff --git a/drivers/nfc/pn533/uart.c b/drivers/nfc/pn533/uart.c > new file mode 100644 > index 000000000000..f1cc2354a4fd > --- /dev/null > +++ b/drivers/nfc/pn533/uart.c > @@ -0,0 +1,316 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Driver for NXP PN532 NFC Chip - UART transport layer > + * > + * Copyright (C) 2018 Lemonage Software GmbH > + * Author: Lars Pöschel <poeschel@lemonage.de> > + * All rights reserved. > + */ > + > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/nfc.h> > +#include <linux/netdevice.h> > +#include <linux/of.h> > +#include <linux/serdev.h> > +#include "pn533.h" > + > +#define PN532_UART_SKB_BUFF_LEN (PN533_CMD_DATAEXCH_DATA_MAXLEN * 2) > + > +enum send_wakeup { > + PN532_SEND_NO_WAKEUP = 0, > + PN532_SEND_WAKEUP, > + PN532_SEND_LAST_WAKEUP, > +}; > + > + > +struct pn532_uart_phy { > + struct serdev_device *serdev; > + struct sk_buff *recv_skb; > + struct pn533 *priv; > + enum send_wakeup send_wakeup; Could there be any concurrency issues w/ regards to accessing this variable? I see it is accessed in pn532_uart_send_frame(), pn532_dev_up(), pn532_dev_down() which may be called from the following wq: INIT_WORK(&priv->mi_tm_rx_work, pn533_wq_tm_mi_recv); INIT_WORK(&priv->mi_tm_tx_work, pn533_wq_tm_mi_send); INIT_DELAYED_WORK(&priv->poll_work, pn533_wq_poll); and from net/nfc/core.c via dev_up()/dev_down(). > + struct timer_list cmd_timeout; > + struct sk_buff *cur_out_buf; > +}; > + > +static int pn532_uart_send_frame(struct pn533 *dev, > + struct sk_buff *out) > +{ > + /* wakeup sequence and dummy bytes for waiting time */ > + static const u8 wakeup[] = { > + 0x55, 0x55, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; > + struct pn532_uart_phy *pn532 = dev->phy; > + int err; > + > + print_hex_dump_debug("PN532_uart TX: ", DUMP_PREFIX_NONE, 16, 1, > + out->data, out->len, false); > + > + pn532->cur_out_buf = out; > + if (pn532->send_wakeup) { > + err= serdev_device_write(pn532->serdev, > + wakeup, sizeof(wakeup), > + MAX_SCHEDULE_TIMEOUT); > + if (err < 0) > + return err; > + } > + > + if (pn532->send_wakeup == PN532_SEND_LAST_WAKEUP) { > + pn532->send_wakeup = PN532_SEND_NO_WAKEUP; > + } > + > + err = serdev_device_write(pn532->serdev, out->data, out->len, > + MAX_SCHEDULE_TIMEOUT); > + if (err < 0) > + return err; > + > + mod_timer(&pn532->cmd_timeout, HZ / 40 + jiffies); > + return 0; > +} > + > +static int pn532_uart_send_ack(struct pn533 *dev, gfp_t flags) > +{ > + struct pn532_uart_phy *pn532 = dev->phy; > + /* spec 7.1.1.3: Preamble, SoPC (2), ACK Code (2), Postamble */ > + static const u8 ack[PN533_STD_FRAME_ACK_SIZE] = { > + 0x00, 0x00, 0xff, 0x00, 0xff, 0x00}; > + int err; > + > + err = serdev_device_write(pn532->serdev, ack, sizeof(ack), > + MAX_SCHEDULE_TIMEOUT); > + if (err < 0) > + return err; > + > + return 0; > +} > + > +static void pn532_uart_abort_cmd(struct pn533 *dev, gfp_t flags) > +{ > + /* An ack will cancel the last issued command */ > + pn532_uart_send_ack(dev, flags); > + /* schedule cmd_complete_work to finish current command execution */ > + pn533_recv_frame(dev, NULL, -ENOENT); > +} > + > +static void pn532_dev_up(struct pn533 *dev) > +{ > + struct pn532_uart_phy *pn532 = dev->phy; > + > + serdev_device_open(pn532->serdev); > + pn532->send_wakeup = PN532_SEND_LAST_WAKEUP; > +} > + > +static void pn532_dev_down(struct pn533 *dev) > +{ > + struct pn532_uart_phy *pn532 = dev->phy; > + > + serdev_device_close(pn532->serdev); > + pn532->send_wakeup = PN532_SEND_WAKEUP; > +} > + > +static struct pn533_phy_ops uart_phy_ops = { > + .send_frame = pn532_uart_send_frame, > + .send_ack = pn532_uart_send_ack, > + .abort_cmd = pn532_uart_abort_cmd, > + .dev_up = pn532_dev_up, > + .dev_down = pn532_dev_down, > +}; > + > +static void pn532_cmd_timeout(struct timer_list *t) > +{ > + struct pn532_uart_phy *dev = from_timer(dev, t, cmd_timeout); > + > + pn532_uart_send_frame(dev->priv, dev->cur_out_buf); > +} > + > +/* > + * scans the buffer if it contains a pn532 frame. It is not checked if the > + * frame is really valid. This is later done with pn533_rx_frame_is_valid. > + * This is useful for malformed or errornous transmitted frames. Adjusts the > + * bufferposition where the frame starts, since pn533_recv_frame expects a > + * well formed frame. > + */ > +static int pn532_uart_rx_is_frame(struct sk_buff *skb) > +{ > + int i; > + u16 frame_len; > + struct pn533_std_frame *std; > + struct pn533_ext_frame *ext; > + > + for (i = 0; i + PN533_STD_FRAME_ACK_SIZE <= skb->len; i++) { > + std = (struct pn533_std_frame *)&skb->data[i]; > + /* search start code */ > + if (std->start_frame != cpu_to_be16(PN533_STD_FRAME_SOF)) > + continue; > + > + /* frame type */ > + switch (std->datalen) { > + case PN533_FRAME_DATALEN_ACK: > + if (std->datalen_checksum == 0xff) { > + skb_pull(skb, i); > + return 1; > + } > + > + break; > + case PN533_FRAME_DATALEN_ERROR: > + if ((std->datalen_checksum == 0xff) && > + (skb->len >= > + PN533_STD_ERROR_FRAME_SIZE)) { > + skb_pull(skb, i); > + return 1; > + } > + > + break; > + case PN533_FRAME_DATALEN_EXTENDED: > + ext = (struct pn533_ext_frame *)&skb->data[i]; > + frame_len = ext->datalen; > + if (skb->len >= frame_len + > + sizeof(struct pn533_ext_frame) + > + 2 /* CKS + Postamble */) { > + skb_pull(skb, i); > + return 1; > + } > + > + break; > + default: /* normal information frame */ > + frame_len = std->datalen; > + if (skb->len >= frame_len + > + sizeof(struct pn533_std_frame) + > + 2 /* CKS + Postamble */) { > + skb_pull(skb, i); > + return 1; > + } > + > + break; > + } > + } > + > + return 0; > +} > + > +static int pn532_receive_buf(struct serdev_device *serdev, > + const unsigned char *data, size_t count) > +{ > + struct pn532_uart_phy *dev = serdev_device_get_drvdata(serdev); > + size_t i; > + > + del_timer(&dev->cmd_timeout); > + for (i = 0; i < count; i++) { > + skb_put_u8(dev->recv_skb, *data++); > + if (!pn532_uart_rx_is_frame(dev->recv_skb)) > + continue; > + > + pn533_recv_frame(dev->priv, dev->recv_skb, 0); > + dev->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL); > + if (!dev->recv_skb) > + return 0; > + } > + > + return i; > +} > + > +static struct serdev_device_ops pn532_serdev_ops = { > + .receive_buf = pn532_receive_buf, > + .write_wakeup = serdev_device_write_wakeup, > +}; > + > +static const struct of_device_id pn532_uart_of_match[] = { > + { .compatible = "nxp,pn532", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, pn532_uart_of_match); > + > +static int pn532_uart_probe(struct serdev_device *serdev) > +{ > + struct pn532_uart_phy *pn532; > + struct pn533 *priv; > + int err; > + > + err = -ENOMEM; > + pn532 = kzalloc(sizeof(*pn532), GFP_KERNEL); > + if (!pn532) > + goto err_exit; > + > + pn532->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL); > + if (!pn532->recv_skb) > + goto err_free; > + > + pn532->serdev = serdev; > + serdev_device_set_drvdata(serdev, pn532); > + serdev_device_set_client_ops(serdev, &pn532_serdev_ops); > + err = serdev_device_open(serdev); > + if (err) { > + dev_err(&serdev->dev, "Unable to open device\n"); > + goto err_skb; > + } > + > + err = serdev_device_set_baudrate(serdev, 115200); > + if (err != 115200) { > + err = -EINVAL; > + goto err_serdev; > + } > + > + serdev_device_set_flow_control(serdev, false); > + pn532->send_wakeup = PN532_SEND_WAKEUP; > + timer_setup(&pn532->cmd_timeout, pn532_cmd_timeout, 0); > + priv = pn53x_common_init(PN533_DEVICE_PN532, > + PN533_PROTO_REQ_ACK_RESP, > + pn532, &uart_phy_ops, NULL, > + &pn532->serdev->dev); > + if (IS_ERR(priv)) { > + err = PTR_ERR(priv); > + goto err_serdev; > + } > + > + pn532->priv = priv; > + err = pn533_finalize_setup(pn532->priv); > + if (err) > + goto err_clean; > + > + serdev_device_close(serdev); > + err = pn53x_register_nfc(priv, PN533_NO_TYPE_B_PROTOCOLS, &serdev->dev); > + if (err) { > + pn53x_common_clean(pn532->priv); > + goto err_skb; > + } > + > + return err; > + > +err_clean: > + pn53x_common_clean(pn532->priv); > +err_serdev: > + serdev_device_close(serdev); > +err_skb: > + kfree_skb(pn532->recv_skb); > +err_free: > + kfree(pn532); > +err_exit: > + return err; > +} > + > +static void pn532_uart_remove(struct serdev_device *serdev) > +{ > + struct pn532_uart_phy *pn532 = serdev_device_get_drvdata(serdev); > + > + pn53x_unregister_nfc(pn532->priv); > + serdev_device_close(serdev); > + pn53x_common_clean(pn532->priv); > + kfree_skb(pn532->recv_skb); > + kfree(pn532); > +} > + > +static struct serdev_device_driver pn532_uart_driver = { > + .probe = pn532_uart_probe, > + .remove = pn532_uart_remove, > + .driver = { > + .name = "pn532_uart", > + .of_match_table = of_match_ptr(pn532_uart_of_match), > + }, > +}; > + > +module_serdev_device_driver(pn532_uart_driver); > + > +MODULE_AUTHOR("Lars Pöschel <poeschel@lemonage.de>"); > +MODULE_DESCRIPTION("PN532 UART driver"); > +MODULE_LICENSE("GPL"); >
On Thu, Aug 22, 2019 at 10:09:09AM +0000, Claudiu.Beznea@microchip.com wrote: > Hi Lars, > > On 20.08.2019 15:03, Lars Poeschel wrote: > > This adds the UART phy interface for the pn533 driver. > > The pn533 driver can be used through UART interface this way. > > It is implemented as a serdev device. > > > > Cc: Johan Hovold <johan@kernel.org> > > Signed-off-by: Lars Poeschel <poeschel@lemonage.de> > > --- > > Changes in v6: > > - Rebased the patch series on v5.3-rc5 > > > > Changes in v5: > > - Use the splitted pn53x_common_init and pn53x_register_nfc > > and pn53x_common_clean and pn53x_unregister_nfc alike > > > > Changes in v4: > > - SPDX-License-Identifier: GPL-2.0+ > > - Source code comments above refering items > > - Error check for serdev_device_write's > > - Change if (xxx == NULL) to if (!xxx) > > - Remove device name from a dev_err > > - move pn533_register in _probe a bit towards the end of _probe > > - make use of newly added dev_up / dev_down phy_ops > > - control send_wakeup variable from dev_up / dev_down > > > > Changes in v3: > > - depend on SERIAL_DEV_BUS in Kconfig > > > > Changes in v2: > > - switched from tty line discipline to serdev, resulting in many > > simplifications > > - SPDX License Identifier > > > > drivers/nfc/pn533/Kconfig | 11 ++ > > drivers/nfc/pn533/Makefile | 2 + > > drivers/nfc/pn533/pn533.h | 8 + > > drivers/nfc/pn533/uart.c | 316 +++++++++++++++++++++++++++++++++++++ > > 4 files changed, 337 insertions(+) > > create mode 100644 drivers/nfc/pn533/uart.c > > > > diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig > > index f6d6b345ba0d..7fe1bbe26568 100644 > > --- a/drivers/nfc/pn533/Kconfig > > +++ b/drivers/nfc/pn533/Kconfig > > @@ -26,3 +26,14 @@ config NFC_PN533_I2C > > > > If you choose to build a module, it'll be called pn533_i2c. > > Say N if unsure. > > + > > +config NFC_PN532_UART > > + tristate "NFC PN532 device support (UART)" > > + depends on SERIAL_DEV_BUS > > + select NFC_PN533 > > + ---help--- > > + This module adds support for the NXP pn532 UART interface. > > + Select this if your platform is using the UART bus. > > + > > + If you choose to build a module, it'll be called pn532_uart. > > + Say N if unsure. > > diff --git a/drivers/nfc/pn533/Makefile b/drivers/nfc/pn533/Makefile > > index 43c25b4f9466..b9648337576f 100644 > > --- a/drivers/nfc/pn533/Makefile > > +++ b/drivers/nfc/pn533/Makefile > > @@ -4,7 +4,9 @@ > > # > > pn533_usb-objs = usb.o > > pn533_i2c-objs = i2c.o > > +pn532_uart-objs = uart.o > > > > obj-$(CONFIG_NFC_PN533) += pn533.o > > obj-$(CONFIG_NFC_PN533_USB) += pn533_usb.o > > obj-$(CONFIG_NFC_PN533_I2C) += pn533_i2c.o > > +obj-$(CONFIG_NFC_PN532_UART) += pn532_uart.o > > diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h > > index 510ddebbd896..6541088fad73 100644 > > --- a/drivers/nfc/pn533/pn533.h > > +++ b/drivers/nfc/pn533/pn533.h > > @@ -43,6 +43,11 @@ > > > > /* Preamble (1), SoPC (2), ACK Code (2), Postamble (1) */ > > #define PN533_STD_FRAME_ACK_SIZE 6 > > +/* > > + * Preamble (1), SoPC (2), Packet Length (1), Packet Length Checksum (1), > > + * Specific Application Level Error Code (1) , Postamble (1) > > + */ > > +#define PN533_STD_ERROR_FRAME_SIZE 8 > > #define PN533_STD_FRAME_CHECKSUM(f) (f->data[f->datalen]) > > #define PN533_STD_FRAME_POSTAMBLE(f) (f->data[f->datalen + 1]) > > /* Half start code (3), LEN (4) should be 0xffff for extended frame */ > > @@ -84,6 +89,9 @@ > > #define PN533_CMD_MI_MASK 0x40 > > #define PN533_CMD_RET_SUCCESS 0x00 > > > > +#define PN533_FRAME_DATALEN_ACK 0x00 > > +#define PN533_FRAME_DATALEN_ERROR 0x01 > > +#define PN533_FRAME_DATALEN_EXTENDED 0xFF > > > > enum pn533_protocol_type { > > PN533_PROTO_REQ_ACK_RESP = 0, > > diff --git a/drivers/nfc/pn533/uart.c b/drivers/nfc/pn533/uart.c > > new file mode 100644 > > index 000000000000..f1cc2354a4fd > > --- /dev/null > > +++ b/drivers/nfc/pn533/uart.c > > @@ -0,0 +1,316 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Driver for NXP PN532 NFC Chip - UART transport layer > > + * > > + * Copyright (C) 2018 Lemonage Software GmbH > > + * Author: Lars Pöschel <poeschel@lemonage.de> > > + * All rights reserved. > > + */ > > + > > +#include <linux/device.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/nfc.h> > > +#include <linux/netdevice.h> > > +#include <linux/of.h> > > +#include <linux/serdev.h> > > +#include "pn533.h" > > + > > +#define PN532_UART_SKB_BUFF_LEN (PN533_CMD_DATAEXCH_DATA_MAXLEN * 2) > > + > > +enum send_wakeup { > > + PN532_SEND_NO_WAKEUP = 0, > > + PN532_SEND_WAKEUP, > > + PN532_SEND_LAST_WAKEUP, > > +}; > > + > > + > > +struct pn532_uart_phy { > > + struct serdev_device *serdev; > > + struct sk_buff *recv_skb; > > + struct pn533 *priv; > > + enum send_wakeup send_wakeup; > > Could there be any concurrency issues w/ regards to accessing this > variable? I see it is accessed in pn532_uart_send_frame(), pn532_dev_up(), > pn532_dev_down() which may be called from the following wq: > > INIT_WORK(&priv->mi_tm_rx_work, pn533_wq_tm_mi_recv); > > INIT_WORK(&priv->mi_tm_tx_work, pn533_wq_tm_mi_send); > > INIT_DELAYED_WORK(&priv->poll_work, pn533_wq_poll); > > > and from net/nfc/core.c via dev_up()/dev_down(). Well, I spend some minutes thinking about this. There should be no real problem. The code in pn533.c ensures, that commands are transmitted sequencially. And it always is command - response. So if a command is send, the driver waits for a response from the chip. So pn532_uart_send_frame should not be called multiple times without reaching at least serdev_device_write, but at this point the race is already over. There is one exception, this is the abort command. This command can be sent without receiving a previous response. So there is the possibility of a successful race. The send_wakeup variable is used to control if we need to send a wakeup request to the pn532 chip prior to the actual command we would like to send. Worst thing that I see could happen - if the race succeeds - is that we send a wakeup to the chip that is propably not needed as it is already awake. But this does not hurt as a wakeup send to the pn532 is essentially a no-op if the chip is awake already. I could have implemented it so, that a wakeup is sent in front of every command without thinking and the driver would work. The same is with pn532_dev_up. It could be that there is one wakeup sent to much, but it does not hurt. pn532_dev_down is not problematic I think. To sum it up: There is maybe a very little probability, but it does nothing bad. Question is now: Is it worth mutex'ing the send_wakeup variable or can we leave it as-is ? Thank you for your review, Claudiu. Regards, Lars
Hi Lars, On 23.08.2019 13:06, Lars Poeschel wrote: > External E-Mail > > > On Thu, Aug 22, 2019 at 10:09:09AM +0000, Claudiu.Beznea@microchip.com wrote: >> Hi Lars, >> >> On 20.08.2019 15:03, Lars Poeschel wrote: >>> This adds the UART phy interface for the pn533 driver. >>> The pn533 driver can be used through UART interface this way. >>> It is implemented as a serdev device. >>> >>> Cc: Johan Hovold <johan@kernel.org> >>> Signed-off-by: Lars Poeschel <poeschel@lemonage.de> >>> --- >>> Changes in v6: >>> - Rebased the patch series on v5.3-rc5 >>> >>> Changes in v5: >>> - Use the splitted pn53x_common_init and pn53x_register_nfc >>> and pn53x_common_clean and pn53x_unregister_nfc alike >>> >>> Changes in v4: >>> - SPDX-License-Identifier: GPL-2.0+ >>> - Source code comments above refering items >>> - Error check for serdev_device_write's >>> - Change if (xxx == NULL) to if (!xxx) >>> - Remove device name from a dev_err >>> - move pn533_register in _probe a bit towards the end of _probe >>> - make use of newly added dev_up / dev_down phy_ops >>> - control send_wakeup variable from dev_up / dev_down >>> >>> Changes in v3: >>> - depend on SERIAL_DEV_BUS in Kconfig >>> >>> Changes in v2: >>> - switched from tty line discipline to serdev, resulting in many >>> simplifications >>> - SPDX License Identifier >>> >>> drivers/nfc/pn533/Kconfig | 11 ++ >>> drivers/nfc/pn533/Makefile | 2 + >>> drivers/nfc/pn533/pn533.h | 8 + >>> drivers/nfc/pn533/uart.c | 316 +++++++++++++++++++++++++++++++++++++ >>> 4 files changed, 337 insertions(+) >>> create mode 100644 drivers/nfc/pn533/uart.c >>> >>> diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig >>> index f6d6b345ba0d..7fe1bbe26568 100644 >>> --- a/drivers/nfc/pn533/Kconfig >>> +++ b/drivers/nfc/pn533/Kconfig >>> @@ -26,3 +26,14 @@ config NFC_PN533_I2C >>> >>> If you choose to build a module, it'll be called pn533_i2c. >>> Say N if unsure. >>> + >>> +config NFC_PN532_UART >>> + tristate "NFC PN532 device support (UART)" >>> + depends on SERIAL_DEV_BUS >>> + select NFC_PN533 >>> + ---help--- >>> + This module adds support for the NXP pn532 UART interface. >>> + Select this if your platform is using the UART bus. >>> + >>> + If you choose to build a module, it'll be called pn532_uart. >>> + Say N if unsure. >>> diff --git a/drivers/nfc/pn533/Makefile b/drivers/nfc/pn533/Makefile >>> index 43c25b4f9466..b9648337576f 100644 >>> --- a/drivers/nfc/pn533/Makefile >>> +++ b/drivers/nfc/pn533/Makefile >>> @@ -4,7 +4,9 @@ >>> # >>> pn533_usb-objs = usb.o >>> pn533_i2c-objs = i2c.o >>> +pn532_uart-objs = uart.o >>> >>> obj-$(CONFIG_NFC_PN533) += pn533.o >>> obj-$(CONFIG_NFC_PN533_USB) += pn533_usb.o >>> obj-$(CONFIG_NFC_PN533_I2C) += pn533_i2c.o >>> +obj-$(CONFIG_NFC_PN532_UART) += pn532_uart.o >>> diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h >>> index 510ddebbd896..6541088fad73 100644 >>> --- a/drivers/nfc/pn533/pn533.h >>> +++ b/drivers/nfc/pn533/pn533.h >>> @@ -43,6 +43,11 @@ >>> >>> /* Preamble (1), SoPC (2), ACK Code (2), Postamble (1) */ >>> #define PN533_STD_FRAME_ACK_SIZE 6 >>> +/* >>> + * Preamble (1), SoPC (2), Packet Length (1), Packet Length Checksum (1), >>> + * Specific Application Level Error Code (1) , Postamble (1) >>> + */ >>> +#define PN533_STD_ERROR_FRAME_SIZE 8 >>> #define PN533_STD_FRAME_CHECKSUM(f) (f->data[f->datalen]) >>> #define PN533_STD_FRAME_POSTAMBLE(f) (f->data[f->datalen + 1]) >>> /* Half start code (3), LEN (4) should be 0xffff for extended frame */ >>> @@ -84,6 +89,9 @@ >>> #define PN533_CMD_MI_MASK 0x40 >>> #define PN533_CMD_RET_SUCCESS 0x00 >>> >>> +#define PN533_FRAME_DATALEN_ACK 0x00 >>> +#define PN533_FRAME_DATALEN_ERROR 0x01 >>> +#define PN533_FRAME_DATALEN_EXTENDED 0xFF >>> >>> enum pn533_protocol_type { >>> PN533_PROTO_REQ_ACK_RESP = 0, >>> diff --git a/drivers/nfc/pn533/uart.c b/drivers/nfc/pn533/uart.c >>> new file mode 100644 >>> index 000000000000..f1cc2354a4fd >>> --- /dev/null >>> +++ b/drivers/nfc/pn533/uart.c >>> @@ -0,0 +1,316 @@ >>> +// SPDX-License-Identifier: GPL-2.0+ >>> +/* >>> + * Driver for NXP PN532 NFC Chip - UART transport layer >>> + * >>> + * Copyright (C) 2018 Lemonage Software GmbH >>> + * Author: Lars Pöschel <poeschel@lemonage.de> >>> + * All rights reserved. >>> + */ >>> + >>> +#include <linux/device.h> >>> +#include <linux/kernel.h> >>> +#include <linux/module.h> >>> +#include <linux/nfc.h> >>> +#include <linux/netdevice.h> >>> +#include <linux/of.h> >>> +#include <linux/serdev.h> >>> +#include "pn533.h" >>> + >>> +#define PN532_UART_SKB_BUFF_LEN (PN533_CMD_DATAEXCH_DATA_MAXLEN * 2) >>> + >>> +enum send_wakeup { >>> + PN532_SEND_NO_WAKEUP = 0, >>> + PN532_SEND_WAKEUP, >>> + PN532_SEND_LAST_WAKEUP, >>> +}; >>> + >>> + >>> +struct pn532_uart_phy { >>> + struct serdev_device *serdev; >>> + struct sk_buff *recv_skb; >>> + struct pn533 *priv; >>> + enum send_wakeup send_wakeup; >> >> Could there be any concurrency issues w/ regards to accessing this >> variable? I see it is accessed in pn532_uart_send_frame(), pn532_dev_up(), >> pn532_dev_down() which may be called from the following wq: >> >> INIT_WORK(&priv->mi_tm_rx_work, pn533_wq_tm_mi_recv); >> >> INIT_WORK(&priv->mi_tm_tx_work, pn533_wq_tm_mi_send); >> >> INIT_DELAYED_WORK(&priv->poll_work, pn533_wq_poll); >> >> >> and from net/nfc/core.c via dev_up()/dev_down(). > > Well, I spend some minutes thinking about this. There should be no real > problem. The code in pn533.c ensures, that commands are transmitted > sequencially. And it always is command - response. So if a command is > send, the driver waits for a response from the chip. > So pn532_uart_send_frame should not be called multiple times without > reaching at least serdev_device_write, but at this point the race is > already over. > There is one exception, this is the abort command. This command can be > sent without receiving a previous response. So there is the possibility > of a successful race. > The send_wakeup variable is used to control if we need to send a > wakeup request to the pn532 chip prior to the actual command we would > like to send. > Worst thing that I see could happen - if the race succeeds - is that we > send a wakeup to the chip that is propably not needed as it is already > awake. But this does not hurt as a wakeup send to the pn532 is > essentially a no-op if the chip is awake already. I could have > implemented it so, that a wakeup is sent in front of every command > without thinking and the driver would work. > The same is with pn532_dev_up. It could be that there is one wakeup sent > to much, but it does not hurt. > pn532_dev_down is not problematic I think. > > To sum it up: There is maybe a very little probability, but it does > nothing bad. Question is now: Is it worth mutex'ing the send_wakeup > variable or can we leave it as-is ? Being so as you described above, I am for leaving it as is. Maybe, as you wish, document this somewhere (e.g. a comment in the code), so that others to be aware of this. Thank you, Claudiu Beznea > > Thank you for your review, Claudiu. > Regards, > Lars > >
diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig index f6d6b345ba0d..7fe1bbe26568 100644 --- a/drivers/nfc/pn533/Kconfig +++ b/drivers/nfc/pn533/Kconfig @@ -26,3 +26,14 @@ config NFC_PN533_I2C If you choose to build a module, it'll be called pn533_i2c. Say N if unsure. + +config NFC_PN532_UART + tristate "NFC PN532 device support (UART)" + depends on SERIAL_DEV_BUS + select NFC_PN533 + ---help--- + This module adds support for the NXP pn532 UART interface. + Select this if your platform is using the UART bus. + + If you choose to build a module, it'll be called pn532_uart. + Say N if unsure. diff --git a/drivers/nfc/pn533/Makefile b/drivers/nfc/pn533/Makefile index 43c25b4f9466..b9648337576f 100644 --- a/drivers/nfc/pn533/Makefile +++ b/drivers/nfc/pn533/Makefile @@ -4,7 +4,9 @@ # pn533_usb-objs = usb.o pn533_i2c-objs = i2c.o +pn532_uart-objs = uart.o obj-$(CONFIG_NFC_PN533) += pn533.o obj-$(CONFIG_NFC_PN533_USB) += pn533_usb.o obj-$(CONFIG_NFC_PN533_I2C) += pn533_i2c.o +obj-$(CONFIG_NFC_PN532_UART) += pn532_uart.o diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h index 510ddebbd896..6541088fad73 100644 --- a/drivers/nfc/pn533/pn533.h +++ b/drivers/nfc/pn533/pn533.h @@ -43,6 +43,11 @@ /* Preamble (1), SoPC (2), ACK Code (2), Postamble (1) */ #define PN533_STD_FRAME_ACK_SIZE 6 +/* + * Preamble (1), SoPC (2), Packet Length (1), Packet Length Checksum (1), + * Specific Application Level Error Code (1) , Postamble (1) + */ +#define PN533_STD_ERROR_FRAME_SIZE 8 #define PN533_STD_FRAME_CHECKSUM(f) (f->data[f->datalen]) #define PN533_STD_FRAME_POSTAMBLE(f) (f->data[f->datalen + 1]) /* Half start code (3), LEN (4) should be 0xffff for extended frame */ @@ -84,6 +89,9 @@ #define PN533_CMD_MI_MASK 0x40 #define PN533_CMD_RET_SUCCESS 0x00 +#define PN533_FRAME_DATALEN_ACK 0x00 +#define PN533_FRAME_DATALEN_ERROR 0x01 +#define PN533_FRAME_DATALEN_EXTENDED 0xFF enum pn533_protocol_type { PN533_PROTO_REQ_ACK_RESP = 0, diff --git a/drivers/nfc/pn533/uart.c b/drivers/nfc/pn533/uart.c new file mode 100644 index 000000000000..f1cc2354a4fd --- /dev/null +++ b/drivers/nfc/pn533/uart.c @@ -0,0 +1,316 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Driver for NXP PN532 NFC Chip - UART transport layer + * + * Copyright (C) 2018 Lemonage Software GmbH + * Author: Lars Pöschel <poeschel@lemonage.de> + * All rights reserved. + */ + +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/nfc.h> +#include <linux/netdevice.h> +#include <linux/of.h> +#include <linux/serdev.h> +#include "pn533.h" + +#define PN532_UART_SKB_BUFF_LEN (PN533_CMD_DATAEXCH_DATA_MAXLEN * 2) + +enum send_wakeup { + PN532_SEND_NO_WAKEUP = 0, + PN532_SEND_WAKEUP, + PN532_SEND_LAST_WAKEUP, +}; + + +struct pn532_uart_phy { + struct serdev_device *serdev; + struct sk_buff *recv_skb; + struct pn533 *priv; + enum send_wakeup send_wakeup; + struct timer_list cmd_timeout; + struct sk_buff *cur_out_buf; +}; + +static int pn532_uart_send_frame(struct pn533 *dev, + struct sk_buff *out) +{ + /* wakeup sequence and dummy bytes for waiting time */ + static const u8 wakeup[] = { + 0x55, 0x55, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; + struct pn532_uart_phy *pn532 = dev->phy; + int err; + + print_hex_dump_debug("PN532_uart TX: ", DUMP_PREFIX_NONE, 16, 1, + out->data, out->len, false); + + pn532->cur_out_buf = out; + if (pn532->send_wakeup) { + err= serdev_device_write(pn532->serdev, + wakeup, sizeof(wakeup), + MAX_SCHEDULE_TIMEOUT); + if (err < 0) + return err; + } + + if (pn532->send_wakeup == PN532_SEND_LAST_WAKEUP) { + pn532->send_wakeup = PN532_SEND_NO_WAKEUP; + } + + err = serdev_device_write(pn532->serdev, out->data, out->len, + MAX_SCHEDULE_TIMEOUT); + if (err < 0) + return err; + + mod_timer(&pn532->cmd_timeout, HZ / 40 + jiffies); + return 0; +} + +static int pn532_uart_send_ack(struct pn533 *dev, gfp_t flags) +{ + struct pn532_uart_phy *pn532 = dev->phy; + /* spec 7.1.1.3: Preamble, SoPC (2), ACK Code (2), Postamble */ + static const u8 ack[PN533_STD_FRAME_ACK_SIZE] = { + 0x00, 0x00, 0xff, 0x00, 0xff, 0x00}; + int err; + + err = serdev_device_write(pn532->serdev, ack, sizeof(ack), + MAX_SCHEDULE_TIMEOUT); + if (err < 0) + return err; + + return 0; +} + +static void pn532_uart_abort_cmd(struct pn533 *dev, gfp_t flags) +{ + /* An ack will cancel the last issued command */ + pn532_uart_send_ack(dev, flags); + /* schedule cmd_complete_work to finish current command execution */ + pn533_recv_frame(dev, NULL, -ENOENT); +} + +static void pn532_dev_up(struct pn533 *dev) +{ + struct pn532_uart_phy *pn532 = dev->phy; + + serdev_device_open(pn532->serdev); + pn532->send_wakeup = PN532_SEND_LAST_WAKEUP; +} + +static void pn532_dev_down(struct pn533 *dev) +{ + struct pn532_uart_phy *pn532 = dev->phy; + + serdev_device_close(pn532->serdev); + pn532->send_wakeup = PN532_SEND_WAKEUP; +} + +static struct pn533_phy_ops uart_phy_ops = { + .send_frame = pn532_uart_send_frame, + .send_ack = pn532_uart_send_ack, + .abort_cmd = pn532_uart_abort_cmd, + .dev_up = pn532_dev_up, + .dev_down = pn532_dev_down, +}; + +static void pn532_cmd_timeout(struct timer_list *t) +{ + struct pn532_uart_phy *dev = from_timer(dev, t, cmd_timeout); + + pn532_uart_send_frame(dev->priv, dev->cur_out_buf); +} + +/* + * scans the buffer if it contains a pn532 frame. It is not checked if the + * frame is really valid. This is later done with pn533_rx_frame_is_valid. + * This is useful for malformed or errornous transmitted frames. Adjusts the + * bufferposition where the frame starts, since pn533_recv_frame expects a + * well formed frame. + */ +static int pn532_uart_rx_is_frame(struct sk_buff *skb) +{ + int i; + u16 frame_len; + struct pn533_std_frame *std; + struct pn533_ext_frame *ext; + + for (i = 0; i + PN533_STD_FRAME_ACK_SIZE <= skb->len; i++) { + std = (struct pn533_std_frame *)&skb->data[i]; + /* search start code */ + if (std->start_frame != cpu_to_be16(PN533_STD_FRAME_SOF)) + continue; + + /* frame type */ + switch (std->datalen) { + case PN533_FRAME_DATALEN_ACK: + if (std->datalen_checksum == 0xff) { + skb_pull(skb, i); + return 1; + } + + break; + case PN533_FRAME_DATALEN_ERROR: + if ((std->datalen_checksum == 0xff) && + (skb->len >= + PN533_STD_ERROR_FRAME_SIZE)) { + skb_pull(skb, i); + return 1; + } + + break; + case PN533_FRAME_DATALEN_EXTENDED: + ext = (struct pn533_ext_frame *)&skb->data[i]; + frame_len = ext->datalen; + if (skb->len >= frame_len + + sizeof(struct pn533_ext_frame) + + 2 /* CKS + Postamble */) { + skb_pull(skb, i); + return 1; + } + + break; + default: /* normal information frame */ + frame_len = std->datalen; + if (skb->len >= frame_len + + sizeof(struct pn533_std_frame) + + 2 /* CKS + Postamble */) { + skb_pull(skb, i); + return 1; + } + + break; + } + } + + return 0; +} + +static int pn532_receive_buf(struct serdev_device *serdev, + const unsigned char *data, size_t count) +{ + struct pn532_uart_phy *dev = serdev_device_get_drvdata(serdev); + size_t i; + + del_timer(&dev->cmd_timeout); + for (i = 0; i < count; i++) { + skb_put_u8(dev->recv_skb, *data++); + if (!pn532_uart_rx_is_frame(dev->recv_skb)) + continue; + + pn533_recv_frame(dev->priv, dev->recv_skb, 0); + dev->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL); + if (!dev->recv_skb) + return 0; + } + + return i; +} + +static struct serdev_device_ops pn532_serdev_ops = { + .receive_buf = pn532_receive_buf, + .write_wakeup = serdev_device_write_wakeup, +}; + +static const struct of_device_id pn532_uart_of_match[] = { + { .compatible = "nxp,pn532", }, + {}, +}; +MODULE_DEVICE_TABLE(of, pn532_uart_of_match); + +static int pn532_uart_probe(struct serdev_device *serdev) +{ + struct pn532_uart_phy *pn532; + struct pn533 *priv; + int err; + + err = -ENOMEM; + pn532 = kzalloc(sizeof(*pn532), GFP_KERNEL); + if (!pn532) + goto err_exit; + + pn532->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL); + if (!pn532->recv_skb) + goto err_free; + + pn532->serdev = serdev; + serdev_device_set_drvdata(serdev, pn532); + serdev_device_set_client_ops(serdev, &pn532_serdev_ops); + err = serdev_device_open(serdev); + if (err) { + dev_err(&serdev->dev, "Unable to open device\n"); + goto err_skb; + } + + err = serdev_device_set_baudrate(serdev, 115200); + if (err != 115200) { + err = -EINVAL; + goto err_serdev; + } + + serdev_device_set_flow_control(serdev, false); + pn532->send_wakeup = PN532_SEND_WAKEUP; + timer_setup(&pn532->cmd_timeout, pn532_cmd_timeout, 0); + priv = pn53x_common_init(PN533_DEVICE_PN532, + PN533_PROTO_REQ_ACK_RESP, + pn532, &uart_phy_ops, NULL, + &pn532->serdev->dev); + if (IS_ERR(priv)) { + err = PTR_ERR(priv); + goto err_serdev; + } + + pn532->priv = priv; + err = pn533_finalize_setup(pn532->priv); + if (err) + goto err_clean; + + serdev_device_close(serdev); + err = pn53x_register_nfc(priv, PN533_NO_TYPE_B_PROTOCOLS, &serdev->dev); + if (err) { + pn53x_common_clean(pn532->priv); + goto err_skb; + } + + return err; + +err_clean: + pn53x_common_clean(pn532->priv); +err_serdev: + serdev_device_close(serdev); +err_skb: + kfree_skb(pn532->recv_skb); +err_free: + kfree(pn532); +err_exit: + return err; +} + +static void pn532_uart_remove(struct serdev_device *serdev) +{ + struct pn532_uart_phy *pn532 = serdev_device_get_drvdata(serdev); + + pn53x_unregister_nfc(pn532->priv); + serdev_device_close(serdev); + pn53x_common_clean(pn532->priv); + kfree_skb(pn532->recv_skb); + kfree(pn532); +} + +static struct serdev_device_driver pn532_uart_driver = { + .probe = pn532_uart_probe, + .remove = pn532_uart_remove, + .driver = { + .name = "pn532_uart", + .of_match_table = of_match_ptr(pn532_uart_of_match), + }, +}; + +module_serdev_device_driver(pn532_uart_driver); + +MODULE_AUTHOR("Lars Pöschel <poeschel@lemonage.de>"); +MODULE_DESCRIPTION("PN532 UART driver"); +MODULE_LICENSE("GPL");
This adds the UART phy interface for the pn533 driver. The pn533 driver can be used through UART interface this way. It is implemented as a serdev device. Cc: Johan Hovold <johan@kernel.org> Signed-off-by: Lars Poeschel <poeschel@lemonage.de> --- Changes in v6: - Rebased the patch series on v5.3-rc5 Changes in v5: - Use the splitted pn53x_common_init and pn53x_register_nfc and pn53x_common_clean and pn53x_unregister_nfc alike Changes in v4: - SPDX-License-Identifier: GPL-2.0+ - Source code comments above refering items - Error check for serdev_device_write's - Change if (xxx == NULL) to if (!xxx) - Remove device name from a dev_err - move pn533_register in _probe a bit towards the end of _probe - make use of newly added dev_up / dev_down phy_ops - control send_wakeup variable from dev_up / dev_down Changes in v3: - depend on SERIAL_DEV_BUS in Kconfig Changes in v2: - switched from tty line discipline to serdev, resulting in many simplifications - SPDX License Identifier drivers/nfc/pn533/Kconfig | 11 ++ drivers/nfc/pn533/Makefile | 2 + drivers/nfc/pn533/pn533.h | 8 + drivers/nfc/pn533/uart.c | 316 +++++++++++++++++++++++++++++++++++++ 4 files changed, 337 insertions(+) create mode 100644 drivers/nfc/pn533/uart.c