Messages in this thread | | | Subject | Re: [RFC 4/4] NFC: pn533: add I2C phy driver | From | Olliver Schinagl <> | Date | Wed, 4 May 2016 17:22:21 +0200 |
| |
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.thalmeier@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.thalmeier@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 >
| |