Hi Michael,

I know this is an old patch already applied, but I noticed the following:

On 25-03-16 15:46, Michael Thalmeier wrote:
This adds the I2C phy interface for the pn533 driver. This way the driver can
be used to interact with I2C connected pn532.

Signed-off-by: Michael Thalmeier <michael.thalme...@hale.at>
---
  drivers/nfc/pn533/Kconfig  |  11 ++
  drivers/nfc/pn533/Makefile |   2 +
  drivers/nfc/pn533/i2c.c    | 277 +++++++++++++++++++++++++++++++++++++++++++++
  drivers/nfc/pn533/pn533.c  |  29 +++++
  drivers/nfc/pn533/pn533.h  |   2 +
  5 files changed, 321 insertions(+)
  create mode 100644 drivers/nfc/pn533/i2c.c

diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig
index b5a926e..d94122d 100644
--- a/drivers/nfc/pn533/Kconfig
+++ b/drivers/nfc/pn533/Kconfig
@@ -14,3 +14,14 @@ config NFC_PN533_USB

          If you choose to build a module, it'll be called pn533_usb.
          Say N if unsure.
+
+config NFC_PN533_I2C
+       tristate "NFC PN533 device support (I2C)"
+       depends on I2C
+       select NFC_PN533
+       ---help---
+         This module adds support for the NXP pn533 I2C interface.
+         Select this if your platform is using the I2C bus.
+
+         If you choose to build a module, it'll be called pn533_i2c.
+         Say N if unsure.
diff --git a/drivers/nfc/pn533/Makefile b/drivers/nfc/pn533/Makefile
index 12c6be4..51d24c6 100644
--- a/drivers/nfc/pn533/Makefile
+++ b/drivers/nfc/pn533/Makefile
@@ -2,6 +2,8 @@
  # Makefile for PN533 NFC driver
  #
  pn533_usb-objs  = usb.o
+pn533_i2c-objs  = i2c.o

  obj-$(CONFIG_NFC_PN533)     += pn533.o
  obj-$(CONFIG_NFC_PN533_USB) += pn533_usb.o
+obj-$(CONFIG_NFC_PN533_I2C) += pn533_i2c.o
diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
new file mode 100644
index 0000000..35066b30
--- /dev/null
+++ b/drivers/nfc/pn533/i2c.c
@@ -0,0 +1,277 @@
+/*
+ * Driver for NXP PN533 NFC Chip - I2C transport layer
+ *
+ * Copyright (C) 2011 Instituto Nokia de Tecnologia
+ * Copyright (C) 2012-2013 Tieto Poland
+ * Copyright (C) 2016 HALE electronic
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/nfc.h>
+#include <linux/netdevice.h>
+#include <linux/interrupt.h>
+#include <net/nfc/nfc.h>
+#include "pn533.h"
+
+#define VERSION "0.1"
+
+#define PN533_I2C_DRIVER_NAME "pn533_i2c"
+
+struct pn533_i2c_phy {
+       struct i2c_client *i2c_dev;
+       struct pn533 *priv;
+
+       int hard_fault;         /*
+                                * < 0 if hardware error occured (e.g. i2c err)
+                                * and prevents normal operation.
+                                */
+};
+
+static int pn533_i2c_send_ack(struct pn533 *dev, gfp_t flags)
+{
+       struct pn533_i2c_phy *phy = dev->phy;
+       struct i2c_client *client = phy->i2c_dev;
+       u8 ack[6] = {0x00, 0x00, 0xff, 0x00, 0xff, 0x00};
+       /* spec 6.2.1.3:  Preamble, SoPC (2), ACK Code (2), Postamble */
+       int rc;
+
+       rc = i2c_master_send(client, ack, 6);
+
+       return rc;
+}
+
+static int pn533_i2c_send_frame(struct pn533 *dev,
+                               struct sk_buff *out)
+{
+       struct pn533_i2c_phy *phy = dev->phy;
+       struct i2c_client *client = phy->i2c_dev;
+       int rc;
+
+       if (phy->hard_fault != 0)
+               return phy->hard_fault;
+
+       if (phy->priv == NULL)
+               phy->priv = dev;
+
+       print_hex_dump_debug("PN533_i2c TX: ", DUMP_PREFIX_NONE, 16, 1,
+                            out->data, out->len, false);
+
+       rc = i2c_master_send(client, out->data, out->len);
+
+       if (rc == -EREMOTEIO) { /* Retry, chip was in power down */
+               usleep_range(6000, 10000);
+               rc = i2c_master_send(client, out->data, out->len);
+       }
+
+       if (rc >= 0) {
+               if (rc != out->len)
+                       rc = -EREMOTEIO;
+               else
+                       rc = 0;
+       }
+
+       return rc;
+}
+
+static void pn533_i2c_abort_cmd(struct pn533 *dev, gfp_t flags)
+{
+       /* An ack will cancel the last issued command */
+       pn533_i2c_send_ack(dev, flags);
+
+       /* schedule cmd_complete_work to finish current command execution */
+       if (dev->cmd != NULL)
+               dev->cmd->status = -ENOENT;
+       queue_work(dev->wq, &dev->cmd_complete_work);
+}
+
+static int pn533_i2c_read(struct pn533_i2c_phy *phy, struct sk_buff **skb)
+{
+       struct i2c_client *client = phy->i2c_dev;
+       int len = PN533_EXT_FRAME_HEADER_LEN +
+                 PN533_STD_FRAME_MAX_PAYLOAD_LEN +
+                 PN533_STD_FRAME_TAIL_LEN + 1;
+       int r;
+
+       *skb = alloc_skb(len, GFP_KERNEL);
+       if (*skb == NULL)
+               return -ENOMEM;
+
+       r = i2c_master_recv(client, skb_put(*skb, len), len);
+       if (r != len) {
+               nfc_err(&client->dev, "cannot read. r=%d len=%d\n", r, len);
+               kfree_skb(*skb);
+               return -EREMOTEIO;
+       }
+
+       if (!((*skb)->data[0] & 0x01)) {
+               nfc_err(&client->dev, "READY flag not set");
+               kfree_skb(*skb);
+               return -EBUSY;
+       }
+
+       /* remove READY byte */
+       skb_pull(*skb, 1);
+       /* trim to frame size */
+       skb_trim(*skb, phy->priv->ops->rx_frame_size((*skb)->data));
+
+       return 0;
+}
+
+static irqreturn_t pn533_i2c_irq_thread_fn(int irq, void *data)
+{
+       struct pn533_i2c_phy *phy = data;
+       struct i2c_client *client;
+       struct sk_buff *skb = NULL;
+       int r;
+
+       if (!phy || irq != phy->i2c_dev->irq) {
+               WARN_ON_ONCE(1);
+               return IRQ_NONE;
+       }
+
+       client = phy->i2c_dev;
+       dev_dbg(&client->dev, "IRQ\n");
+
+       if (phy->hard_fault != 0)
+               return IRQ_HANDLED;
+
+       r = pn533_i2c_read(phy, &skb);
+       if (r == -EREMOTEIO) {
+               phy->hard_fault = r;
+
+               pn533_recv_frame(phy->priv, NULL, -EREMOTEIO);
+
+               return IRQ_HANDLED;
+       } else if ((r == -ENOMEM) || (r == -EBADMSG) || (r == -EBUSY)) {
+               return IRQ_HANDLED;
+       }
This logic seems a bit odd and could be written cleaner no?

+
+       pn533_recv_frame(phy->priv, skb, 0);
+
+       return IRQ_HANDLED;
+
+#if 0
+#endif
But mostly, this shouldn't be here :)

+}
+
+static struct pn533_phy_ops i2c_phy_ops = {
+       .send_frame = pn533_i2c_send_frame,
+       .send_ack = pn533_i2c_send_ack,
+       .abort_cmd = pn533_i2c_abort_cmd,
+};
+
+
+static int pn533_i2c_probe(struct i2c_client *client,
+                              const struct i2c_device_id *id)
+{
+       struct pn533_i2c_phy *phy;
+       struct pn533 *priv;
+       int r = 0;
+
+       dev_dbg(&client->dev, "%s\n", __func__);
+       dev_dbg(&client->dev, "IRQ: %d\n", client->irq);
+
+       if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+               nfc_err(&client->dev, "Need I2C_FUNC_I2C\n");
+               return -ENODEV;
+       }
+
+       phy = devm_kzalloc(&client->dev, sizeof(struct pn533_i2c_phy),
+                          GFP_KERNEL);
+       if (!phy) {
+               nfc_err(&client->dev,
+                       "Cannot allocate memory for pn533 i2c phy.\n");
+               return -ENOMEM;
+       }
+
+       phy->i2c_dev = client;
+       i2c_set_clientdata(client, phy);
+
+       r = request_threaded_irq(client->irq, NULL, pn533_i2c_irq_thread_fn,
+                                IRQF_TRIGGER_FALLING | IRQF_SHARED | 
IRQF_ONESHOT,
+                                PN533_I2C_DRIVER_NAME, phy);
Are we allowed to call this if client->irq is 0?
+
+       if (r < 0) {
+               nfc_err(&client->dev, "Unable to register IRQ handler\n");
+       }
Because then this won't cause an error. The IRQ is fully optional on the hardware for i2c is it not?
+
+       priv = pn533_register_device(PN533_DEVICE_PN532,
+                                    PN533_NO_TYPE_B_PROTOCOLS,
+                                    PN533_PROTO_REQ_ACK_RESP,
+                                    phy, &i2c_phy_ops, NULL,
+                                    &phy->i2c_dev->dev);
Currently, this hangs, and i'll let you know why in a follow up.

+
+       if (IS_ERR(priv)) {
+               r = PTR_ERR(priv);
+               goto err_register;
+       }
+
+       phy->priv = priv;
+
+       return 0;
+
+err_register:
+       free_irq(client->irq, phy);
+
+       return r;
+}
+
+static int pn533_i2c_remove(struct i2c_client *client)
+{
+       struct pn533_i2c_phy *phy = i2c_get_clientdata(client);
+
+       dev_dbg(&client->dev, "%s\n", __func__);
+
+       pn533_unregister_device(phy->priv);
+
+       return 0;
+}
+
+static const struct of_device_id of_pn533_i2c_match[] = {
+       { .compatible = "nxp,pn533-i2c", },
+       { .compatible = "nxp,pn532-i2c", },
+       {},
+};
+MODULE_DEVICE_TABLE(of, of_pn533_i2c_match);
+
+static struct i2c_device_id pn533_i2c_id_table[] = {
+       { PN533_I2C_DRIVER_NAME, 0 },
+       {}
+};
+MODULE_DEVICE_TABLE(i2c, pn533_i2c_id_table);
+
+static struct i2c_driver pn533_i2c_driver = {
+       .driver = {
+                  .name = PN533_I2C_DRIVER_NAME,
+                  .owner = THIS_MODULE,
+                  .of_match_table = of_match_ptr(of_pn533_i2c_match),
+                 },
+       .probe = pn533_i2c_probe,
+       .id_table = pn533_i2c_id_table,
+       .remove = pn533_i2c_remove,
+};
+
+module_i2c_driver(pn533_i2c_driver);
+
+MODULE_AUTHOR("Michael Thalmeier <michael.thalme...@hale.at>");
+MODULE_DESCRIPTION("PN533 I2C driver ver " VERSION);
+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL");
diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index 4468284..3d286a7 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -2435,8 +2435,35 @@ static int pn533_rf_field(struct nfc_dev *nfc_dev, u8 rf)
        return rc;
  }

+static int pn532_sam_configuration(struct nfc_dev *nfc_dev)
+{
+       struct pn533 *dev = nfc_get_drvdata(nfc_dev);
+       struct sk_buff *skb;
+       struct sk_buff *resp;
+
+       skb = pn533_alloc_skb(dev, 1);
+       if (!skb)
+               return -ENOMEM;
+
+       *skb_put(skb, 1) = 0x01;
+
+       resp = pn533_send_cmd_sync(dev, PN533_CMD_SAM_CONFIGURATION, skb);
+       if (IS_ERR(resp))
+               return PTR_ERR(resp);
+
+       dev_kfree_skb(resp);
+       return 0;
+}
+
  static int pn533_dev_up(struct nfc_dev *nfc_dev)
  {
+       struct pn533 *dev = nfc_get_drvdata(nfc_dev);
+       if (dev->device_type == PN533_DEVICE_PN532) {
+               int rc = pn532_sam_configuration(nfc_dev);
+               if (rc)
+                       return rc;
+       }
+
        return pn533_rf_field(nfc_dev, 1);
  }

@@ -2469,6 +2496,7 @@ static int pn533_setup(struct pn533 *dev)
        case PN533_DEVICE_STD:
        case PN533_DEVICE_PASORI:
        case PN533_DEVICE_ACR122U:
+       case PN533_DEVICE_PN532:
                max_retries.mx_rty_atr = 0x2;
                max_retries.mx_rty_psl = 0x1;
                max_retries.mx_rty_passive_act =
@@ -2504,6 +2532,7 @@ static int pn533_setup(struct pn533 *dev)

        switch (dev->device_type) {
        case PN533_DEVICE_STD:
+       case PN533_DEVICE_PN532:
                break;

        case PN533_DEVICE_PASORI:
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index 0958c653..61c5d6a 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -21,6 +21,7 @@
  #define PN533_DEVICE_STD     0x1
  #define PN533_DEVICE_PASORI  0x2
  #define PN533_DEVICE_ACR122U 0x3
+#define PN533_DEVICE_PN532   0x4

  #define PN533_ALL_PROTOCOLS (NFC_PROTO_JEWEL_MASK | NFC_PROTO_MIFARE_MASK |\
                             NFC_PROTO_FELICA_MASK | NFC_PROTO_ISO14443_MASK |\
@@ -72,6 +73,7 @@
  #define PN533_FRAME_CMD(f) (f->data[1])

  #define PN533_CMD_GET_FIRMWARE_VERSION 0x02
+#define PN533_CMD_SAM_CONFIGURATION 0x14
  #define PN533_CMD_RF_CONFIGURATION 0x32
  #define PN533_CMD_IN_DATA_EXCHANGE 0x40
  #define PN533_CMD_IN_COMM_THRU     0x42


Reply via email to