Re: [linux-nfc] [PATCH 8/8] NFC: Add Intel FieldsPeak NFC solution driver
Hi Robert,
On Wed, Apr 01, 2015 at 06:35:31PM +0300, Robert Dolca wrote:
On Thu, Mar 26, 2015 at 2:30 AM, Samuel Ortiz sa...@linux.intel.com wrote:
+ /* If a patch was applied the new version is checked */
+ if (patched) {
+ r = nci_init(ndev);
+ if (r)
+ goto error;
+
+ r = fdp_nci_get_versions(ndev);
+ if (r)
+ goto error;
+
+ if (info-otp_version != info-otp_patch_version ||
+ info-ram_version != info-ram_patch_version) {
+ pr_err(FRP firmware update failed);
+ r = -EINVAL;
+ }
+ }
+
+ /* Check if the device has VSC */
+ if (fdp_post_fw_vsc_cfg[0]) {
+ /* Set the vendor specific configuration */
+ r = fdp_nci_set_production_data(ndev, fdp_post_fw_vsc_cfg[3],
+ fdp_post_fw_vsc_cfg[4]);
+ if (r)
+ goto error;
+ }
+
+ /* Set clock type and frequency */
+ r = fdp_nci_set_clock(ndev, 0, 26000);
+ if (r)
+ goto error;
The version checking, production data setting and clock setting should
be part of a post setup notification call. Please add an nci_dev
notify() ops that could get called on certain events, for example when
NCI is up. Bluetooth's HCI does something along those lines already.
From this notification hook you could implement this post setup stage.
The idea is for your setup routine to only do firmware update and
nothing else. It will make it shorter, and thus easier to read as well.
If the RAM patch wasn't applied successfully the device can't be used
so the setup function should fail.
If the production data (specifically the clock frequency) is not set
the device can not be used. If the user space tries to start polling
before the notification is sent the polling will fail. Having it
called later would mean introducing a race condition.
Sure. Then I'd rather have an additional NCI hook (e.g.
ndev-ops-open()) called synchronously after the setup stage that
could fail and make open fail as well. The idea here is to separate the
2 parts of your logic and make the code more readable.
Cheers,
Samuel.
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-nfc] [PATCH 8/8] NFC: Add Intel FieldsPeak NFC solution driver
On Thu, Mar 26, 2015 at 2:30 AM, Samuel Ortiz sa...@linux.intel.com wrote:
+ /* If a patch was applied the new version is checked */
+ if (patched) {
+ r = nci_init(ndev);
+ if (r)
+ goto error;
+
+ r = fdp_nci_get_versions(ndev);
+ if (r)
+ goto error;
+
+ if (info-otp_version != info-otp_patch_version ||
+ info-ram_version != info-ram_patch_version) {
+ pr_err(FRP firmware update failed);
+ r = -EINVAL;
+ }
+ }
+
+ /* Check if the device has VSC */
+ if (fdp_post_fw_vsc_cfg[0]) {
+ /* Set the vendor specific configuration */
+ r = fdp_nci_set_production_data(ndev, fdp_post_fw_vsc_cfg[3],
+ fdp_post_fw_vsc_cfg[4]);
+ if (r)
+ goto error;
+ }
+
+ /* Set clock type and frequency */
+ r = fdp_nci_set_clock(ndev, 0, 26000);
+ if (r)
+ goto error;
The version checking, production data setting and clock setting should
be part of a post setup notification call. Please add an nci_dev
notify() ops that could get called on certain events, for example when
NCI is up. Bluetooth's HCI does something along those lines already.
From this notification hook you could implement this post setup stage.
The idea is for your setup routine to only do firmware update and
nothing else. It will make it shorter, and thus easier to read as well.
Hi Samuel,
If the RAM patch wasn't applied successfully the device can't be used
so the setup function should fail.
If the production data (specifically the clock frequency) is not set
the device can not be used. If the user space tries to start polling
before the notification is sent the polling will fail. Having it
called later would mean introducing a race condition.
Cheers,
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] NFC: Add Intel FieldsPeak NFC solution driver
Robert,
Another comment:
On Tue, Feb 24, 2015 at 12:01:52PM +0200, Robert Dolca wrote:
+static struct i2c_device_id fdp_nci_i2c_id_table[] = {
+ {INT339A, 0},
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, fdp_nci_i2c_id_table);
+
+
+static const struct acpi_device_id fdp_nci_i2c_acpi_match[] = {
+ {INT339A, 0},
+ {}
+};
Why don't we have a MODULE_DEVICE_TABLE(acpi, fdp_nci_i2c_acpi_match);
here ?
Cheers,
Samuel.
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] NFC: Add Intel FieldsPeak NFC solution driver
On Tue, Feb 24, 2015 at 12:01:52PM +0200, Robert Dolca wrote:
+static struct i2c_device_id fdp_nci_i2c_id_table[] = {
+ {INT339A, 0},
If this is ACPI only device you can remove the above line.
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, fdp_nci_i2c_id_table);
And this as well.
+
+
+static const struct acpi_device_id fdp_nci_i2c_acpi_match[] = {
+ {INT339A, 0},
+ {}
+};
+
+static struct i2c_driver fdp_nci_i2c_driver = {
+ .driver = {
+.name = FDP_NCI_I2C_DRIVER_NAME,
+.owner = THIS_MODULE,
+.acpi_match_table = ACPI_PTR(fdp_nci_i2c_acpi_match),
+ },
+ .id_table = fdp_nci_i2c_id_table,
+ .probe = fdp_nci_i2c_probe,
+ .remove = fdp_nci_i2c_remove,
+};
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] NFC: Add Intel FieldsPeak NFC solution driver
Hi Robert,
On Tue, Feb 24, 2015 at 12:01:52PM +0200, Robert Dolca wrote:
The device can be enumerated using ACPI using the id INT339A.
Please give us some more details about the device. NCI ? HCI ?
Features ? What does the initial patchset support ?
+config NFC_FDP
+ tristate Intel FDP NFC driver
+ depends on NFC_NCI
+ select CRC_CCITT
+ default n
+ ---help---
+ Intel FDP core driver.
What's FDP ?
+ This is a driver based on the NCI NFC kernel layers.
What does it support ?
+
+ To compile this driver as a module, choose m here. The module will
+ be called pn547.
pn547, that's a typo.
+#define NCI_OP_PROP_PATCH_CMD
nci_opcode_pack(NCI_GID_PROP, 0x08)
+#define NCI_OP_PROP_SET_PRODUCTION_DATA_CMD nci_opcode_pack(NCI_GID_PROP,
0x23)
+#define NCI_OP_CORE_GET_CONFIG_CMD nci_opcode_pack(NCI_GID_CORE,
0x03)
This is an NFC Forum spcified command, it should be defined in nci.h.
+static void fdp_nci_get_version_req(struct nci_dev *ndev, unsigned long opt)
+{
+ fdp_nci_send_cmd(ndev, NCI_OP_CORE_GET_CONFIG_CMD,
+ sizeof(nci_core_get_config_otp_ram_version),
+ nci_core_get_config_otp_ram_version);
I don't think you need the intercept logic, so this should be a
simple wrapper around nci_send_cmd().
+int fdp_nci_create_conn(struct nci_dev *ndev)
+{
+ struct fdp_nci_info *info = nci_get_drvdata(ndev);
+ struct core_conn_create_dest_spec_params param;
+
+ param.type = 0xA0;
+ param.length = 0x00;
Would you please describe what those parameters are for ?
+
+ return nci_core_conn_create(info-ndev, 0xC2, 1, sizeof(param), param,
Please define 0xC2, #define FDP_FW_UPDATE_DEST 0xC2
+int fdp_nci_send_patch(struct nci_dev *ndev, u8 type)
+{
+ struct fdp_nci_info *info = nci_get_drvdata(ndev);
+ const struct firmware *fw;
+ struct sk_buff *skb;
+ unsigned long len;
+ u8 max_size, payload_size;
+ int rc = 0;
+
+ if ((type == NCI_PATCH_TYPE_OTP !info-otp_patch) ||
+ (type == NCI_PATCH_TYPE_RAM !info-ram_patch))
+ return -EINVAL;
+
+ if (type == NCI_PATCH_TYPE_OTP)
+ fw = info-otp_patch;
+ else
+ fw = info-ram_patch;
+
+ max_size = nci_conn_max_data_pkt_payload_size(ndev, info-conn_id);
+ if (!max_size)
+ return -EINVAL;
+
+ len = fw-size;
+
+ fdp_nci_set_data_pkt_counter(ndev, fdp_nci_send_patch_cb,
+ DIV_ROUND_UP(fw-size, max_size));
+
+ while (len) {
+
+ payload_size = min_t(unsigned long, (unsigned long) max_size,
+ len);
+
+ skb = nci_skb_alloc(ndev, (NCI_CTRL_HDR_SIZE + payload_size),
+ GFP_KERNEL);
+ skb_reserve(skb, NCI_CTRL_HDR_SIZE);
+
+ memcpy(skb_put(skb, payload_size), fw-data + (fw-size - len),
+payload_size);
Where is your control header set ? How does your boot ROM know how much
bytes are expected ?
+void fdp_nci_intercept_disable(struct nci_dev *ndev)
+{
+ struct fdp_nci_info *info = nci_get_drvdata(ndev);
+
+ pr_info(FDP NCI intercept disabled\n);
+ atomic_set(info-intercept, 0);
+}
Please remove all the intercept stuff here, go through the NCI core and
have a prop_rx ops called.
+int fdp_nci_recv_frame(struct nci_dev *ndev, struct sk_buff *skb)
+{
+ struct fdp_nci_info *info = nci_get_drvdata(ndev);
+
+ pr_debug(%s\n, __func__);
+
+ if (atomic_read(info-intercept)) {
+ skb_queue_tail(info-rx_q, skb);
+ schedule_work(info-rx_work);
+ return 0;
+ }
+
+ return nci_recv_frame(ndev, skb);
+}
+EXPORT_SYMBOL(fdp_nci_recv_frame);
Without the intercept stuff, this should only be nci_recv_frame().
+int fdp_nci_send_cmd(struct nci_dev *ndev, u16 opcode, u8 plen,
+ void *payload)
+{
+ fdp_nci_intercept_inc(ndev);
+ return nci_send_cmd(ndev, opcode, plen, payload);
+}
Ditto
+int fdp_nci_setup(struct nci_dev *ndev)
+{
+ /* Format: total length followed by an NCI packet */
+ struct fdp_nci_info *info = nci_get_drvdata(ndev);
+ char fdp_post_fw_vsc_cfg[] = { 0x1A, 0x2F, 0x23, 0x17,
+ 0x00, 0x00, 0x00,
+ 0x00, 0x04, 0x04, 0x2B, 0x20, 0xFF,
+ 0x09, 0x07, 0xFF, 0xFF, 0xFF, 0xFF,
+ 0x02, 0xFF, 0xFF,
+ 0x08, 0x03, 0x00, 0xFF, 0xFF };
+ int r;
+ u8 patched = 0;
+
+ pr_debug(%s\n, __func__);
+
+ r = nci_init(ndev);
I won't comment further on the fact that I'd prefer to not export
nci_init().
+ if (r)
+ goto error;
+
+ /* Get RAM and OTP version */
+ r =
[PATCH 8/8] NFC: Add Intel FieldsPeak NFC solution driver
The device can be enumerated using ACPI using the id INT339A.
The 1st GPIO is the IRQ and the 2nd is the RESET pin.
I can be also enumerated using platform init.
Signed-off-by: Robert Dolca robert.do...@intel.com
---
drivers/nfc/Kconfig | 1 +
drivers/nfc/fdp/Kconfig | 22 ++
drivers/nfc/fdp/Makefile | 10 +
drivers/nfc/fdp/cmd.c | 196 +++
drivers/nfc/fdp/core.c| 503 ++
drivers/nfc/fdp/fdp.h | 115 +
drivers/nfc/fdp/i2c.c | 454 ++
drivers/nfc/fdp/ntf.c | 68 ++
drivers/nfc/fdp/rsp.c | 117 +
include/linux/platform_data/fdp.h | 33 +++
10 files changed, 1519 insertions(+)
create mode 100644 drivers/nfc/fdp/Kconfig
create mode 100644 drivers/nfc/fdp/Makefile
create mode 100644 drivers/nfc/fdp/cmd.c
create mode 100644 drivers/nfc/fdp/core.c
create mode 100644 drivers/nfc/fdp/fdp.h
create mode 100644 drivers/nfc/fdp/i2c.c
create mode 100644 drivers/nfc/fdp/ntf.c
create mode 100644 drivers/nfc/fdp/rsp.c
create mode 100644 include/linux/platform_data/fdp.h
diff --git a/drivers/nfc/Kconfig b/drivers/nfc/Kconfig
index 6ee2e8d..302fe59 100644
--- a/drivers/nfc/Kconfig
+++ b/drivers/nfc/Kconfig
@@ -68,6 +68,7 @@ config NFC_PORT100
If unsure, say N.
+source drivers/nfc/fdp/Kconfig
source drivers/nfc/pn544/Kconfig
source drivers/nfc/microread/Kconfig
source drivers/nfc/nfcmrvl/Kconfig
diff --git a/drivers/nfc/fdp/Kconfig b/drivers/nfc/fdp/Kconfig
new file mode 100644
index 000..130a235
--- /dev/null
+++ b/drivers/nfc/fdp/Kconfig
@@ -0,0 +1,22 @@
+config NFC_FDP
+ tristate Intel FDP NFC driver
+ depends on NFC_NCI
+ select CRC_CCITT
+ default n
+ ---help---
+ Intel FDP core driver.
+ This is a driver based on the NCI NFC kernel layers.
+
+ To compile this driver as a module, choose m here. The module will
+ be called pn547.
+ Say N if unsure.
+
+config NFC_FDP_I2C
+ tristate NFC FDP i2c support
+ depends on NFC_FDP I2C
+ ---help---
+ This module adds support for the Intel FDP i2c interface.
+ Select this if your platform is using the i2c bus.
+
+ If you choose to build a module, it'll be called fdp_i2c.
+ Say N if unsure.
diff --git a/drivers/nfc/fdp/Makefile b/drivers/nfc/fdp/Makefile
new file mode 100644
index 000..caad636
--- /dev/null
+++ b/drivers/nfc/fdp/Makefile
@@ -0,0 +1,10 @@
+#
+# Makefile for FDP NCI based NFC driver
+#
+
+obj-$(CONFIG_NFC_FDP) += fdp.o
+obj-$(CONFIG_NFC_FDP_I2C) += fdp_i2c.o
+
+fdp-objs = core.o cmd.o ntf.o rsp.o
+fdp_i2c-objs = i2c.o
+
diff --git a/drivers/nfc/fdp/cmd.c b/drivers/nfc/fdp/cmd.c
new file mode 100644
index 000..1b78602
--- /dev/null
+++ b/drivers/nfc/fdp/cmd.c
@@ -0,0 +1,196 @@
+/* -
+ * Copyright (C) 2014-2016, Intel Corporation
+ *
+ * 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.
+ * - */
+
+#include linux/module.h
+#include linux/nfc.h
+#include linux/i2c.h
+#include linux/firmware.h
+#include net/nfc/nci_core.h
+
+#include fdp.h
+
+#define NCI_OP_PROP_PATCH_CMD nci_opcode_pack(NCI_GID_PROP, 0x08)
+#define NCI_OP_PROP_SET_PRODUCTION_DATA_CMD nci_opcode_pack(NCI_GID_PROP,
0x23)
+#define NCI_OP_CORE_GET_CONFIG_CMD nci_opcode_pack(NCI_GID_CORE,
0x03)
+static u8 nci_core_get_config_otp_ram_version[5] = {
+ 0x04,
+ NCI_PARAM_ID_FW_RAM_VERSION,
+ NCI_PARAM_ID_FW_OTP_VERSION,
+ NCI_PARAM_ID_OTP_LIMITED_VERSION,
+ NCI_PARAM_ID_KEY_INDEX_ID
+};
+
+#define NCI_GET_VERSION_TIMEOUT8000
+#define NCI_PATCH_REQUEST_TIMEOUT 8000
+
+struct production_data {
+ u8 len;
+ char *data;
+};
+
+static void fdp_nci_get_version_req(struct nci_dev *ndev, unsigned long opt)
+{
+ fdp_nci_send_cmd(ndev, NCI_OP_CORE_GET_CONFIG_CMD,
+sizeof(nci_core_get_config_otp_ram_version),
+nci_core_get_config_otp_ram_version);
+}
+
+static void fdp_nci_set_production_data_req(struct nci_dev *ndev,
+ unsigned long opt)
+{
+ struct production_data *pd = (struct production_data *)opt;
+
+ fdp_nci_send_cmd(ndev, NCI_OP_PROP_SET_PRODUCTION_DATA_CMD,
Re: [PATCH 8/8] NFC: Add Intel FieldsPeak NFC solution driver
I have no idea about NFC, but +config NFC_FDP + tristate Intel FDP NFC driver + depends on NFC_NCI + select CRC_CCITT + default n + ---help--- + Intel FDP core driver. + This is a driver based on the NCI NFC kernel layers. + + To compile this driver as a module, choose m here. The module will + be called pn547. this seems like a copy/paste error? Certainly it's called fdp? johannes -- To unsubscribe from this list: send the line unsubscribe linux-wireless in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] NFC: Add Intel FieldsPeak NFC solution driver
On Tue, Feb 24, 2015 at 11:33:10AM +0100, Johannes Berg wrote: +config NFC_FDP + tristate Intel FDP NFC driver + depends on NFC_NCI + select CRC_CCITT + default n + ---help--- + Intel FDP core driver. + This is a driver based on the NCI NFC kernel layers. + + To compile this driver as a module, choose m here. The module will + be called pn547. this seems like a copy/paste error? Certainly it's called fdp? You are right. It should be FDP instead of pn547. I will also rename the menu entry name from Intel FDP NFC driver to Intel FieldsPeak (FDP) NFC driver to be more clear what FDP means. Thanks, Robert -- To unsubscribe from this list: send the line unsubscribe linux-wireless in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
