Re: [PATCH] nfc: s3fwrn5: Add driver for Samsung S3FWRN5 NFC Chip

2015-07-30 Thread Robert Baldyga 
On 07/30/2015 12:22 PM, Paul Bolle wrote:
 On wo, 2015-07-29 at 14:15 +0200, Robert Baldyga wrote:
 --- /dev/null
 +++ b/drivers/nfc/s3fwrn5/Kconfig
 @@ -0,0 +1,22 @@
 +config NFC_S3FWRN5
 +tristate Samsung S3FWRN5 NFC driver
 +depends on NFC_NCI
 +default n
 +---help---
 +  Core driver for Samsung S3FWRN5 NFC chip.
 +
 +  To compile this driver as a module, choose m here. The module will
 +  be called s3fwrn5.ko.
 +  Say N if unsure.
 
 If I scanned the code correctly then all this module does is exporting
 three functions to s3fwrn5_i2c.ko. Note that there's also no
 module_unit() in sight. So it's a library, of sorts. And there's no
 reason to load this module without also loading s3fwrn5_i2c.ko. Likewise
 there's no reason to build it without also building s3fwrn5_i2c.ko.
 
 So perhaps you could merge the two Kconfig symbols you add in this
 patch.
 
 Or, if you'd like to keep both Kconfig symbols, for whatever reason, you
 might want to make NFC_S3FWRN5 a silent symbol that will be selected
 by NFC_S3FWRN5_I2C. (That probably requires NFC_S3FWRN5_I2C to depend on
 both NFC_NCI and I2C.)
 
 Would either of the above two options work here?

I would like to keep NFC_S3FWRN5 symbol, because in future another PHYs
can be supported, so this would allow to select core whether PHY will be
selected. However silent symbol seems to be a good solution.

 
 +config NFC_S3FWRN5_I2C
 +tristate Samsung S3FWRN5 I2C support
 +depends on NFC_S3FWRN5  I2C
 +default y
 
 You only added default y to make setting this symbol in make *config
 a one step process, right?

Thats right. Moreover for now i2c is the only available PHY, so if
someone decides to select S3FWRN5 core driver he also probably wants to
have the PHY selected ;)

 +---help---
 +  This module adds support for an I2C interface to the S3FWRN5 chip.
 +  Select this if your platform is using the I2C bus.
 +
 +  To compile this driver as a module, choose m here. The module will
 +  be called s3fwrn5_i2c.ko.
 +  Say N if unsure.
 
 (This advice is at odds with default y above, by the way.)
 
 --- /dev/null
 +++ b/drivers/nfc/s3fwrn5/Makefile
 
 +s3fwrn5-objs = core.o firmware.o nci.o
 +s3fwrn5_i2c-objs = i2c.o
 +
 +obj-$(CONFIG_NFC_S3FWRN5) += s3fwrn5.o
 +obj-$(CONFIG_NFC_S3FWRN5_I2C) += s3fwrn5_i2c.o
 
 --- /dev/null
 +++ b/drivers/nfc/s3fwrn5/core.c
 
 + * This program is free software; you can redistribute it and/or modify it
 + * under the terms and conditions of the GNU General Public License,
 + * version 2, as published by the Free Software Foundation.
 
 +int s3fwrn5_probe(struct nci_dev **ndev, void *phy_id, struct device *pdev,
 +struct s3fwrn5_phy_ops *phy_ops, unsigned int max_payload)
 +{
 +[...]
 +}
 +EXPORT_SYMBOL(s3fwrn5_probe);
 +
 +void s3fwrn5_remove(struct nci_dev *ndev)
 +{
 +[...]
 +}
 +EXPORT_SYMBOL(s3fwrn5_remove);
 +
 +int s3fwrn5_recv_frame(struct nci_dev *ndev, struct sk_buff *skb,
 +enum s3fwrn5_mode mode)
 +{
 +[...]
 +}
 +EXPORT_SYMBOL(s3fwrn5_recv_frame);
 
 +MODULE_LICENSE(GPL);
 
 --- /dev/null
 +++ b/drivers/nfc/s3fwrn5/i2c.c
 
 + * This program is free software; you can redistribute it and/or modify it
 + * under the terms and conditions of the GNU General Public License,
 + * version 2, as published by the Free Software Foundation.
 
 +static int s3fwrn5_i2c_fw_read(struct s3fwrn5_i2c_phy *phy)
 +{
 +[...]
 +
 +return s3fwrn5_recv_frame(phy-ndev, skb, S3FWRN5_MODE_FW);
 +}
 +
 +static int s3fwrn5_i2c_nci_read(struct s3fwrn5_i2c_phy *phy)
 +{
 +[...]
 +
 +return s3fwrn5_recv_frame(phy-ndev, skb, S3FWRN5_MODE_NCI);
 +}
 
 +static int s3fwrn5_i2c_probe(struct i2c_client *client,
 +  const struct i2c_device_id *id)
 +{
 +[...]
 +
 +ret = s3fwrn5_probe(phy-ndev, phy, phy-i2c_dev-dev, i2c_phy_ops,
 +S3FWRN5_I2C_MAX_PAYLOAD);
 +[...]
 +s3fwrn5_remove(phy-ndev);
 +
 +[...]
 +}
 +
 +static int s3fwrn5_i2c_remove(struct i2c_client *client)
 +{
 +[...]
 +
 +s3fwrn5_remove(phy-ndev);
 +
 +[...]
 +}
 
 +MODULE_LICENSE(GPL);
 
 Nit: both modules' code contain a comment referring to the GNU General
 Public License, version 2. They also both use the GPL ident in their
 MODULE_LICENSE() macro. And, according to include/linux/module.h, that
 ident states the license is GPL v2 or later. So I think either the
 comments or the idents need to change.
 

Will be fixed.

Thanks,
Robert Baldyga
--
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] nfc: s3fwrn5: Add driver for Samsung S3FWRN5 NFC Chip

2015-07-30 Thread Paul Bolle 
On wo, 2015-07-29 at 14:15 +0200, Robert Baldyga wrote:
 --- /dev/null
 +++ b/drivers/nfc/s3fwrn5/Kconfig
 @@ -0,0 +1,22 @@
 +config NFC_S3FWRN5
 + tristate Samsung S3FWRN5 NFC driver
 + depends on NFC_NCI
 + default n
 + ---help---
 +   Core driver for Samsung S3FWRN5 NFC chip.
 +
 +   To compile this driver as a module, choose m here. The module will
 +   be called s3fwrn5.ko.
 +   Say N if unsure.

If I scanned the code correctly then all this module does is exporting
three functions to s3fwrn5_i2c.ko. Note that there's also no
module_unit() in sight. So it's a library, of sorts. And there's no
reason to load this module without also loading s3fwrn5_i2c.ko. Likewise
there's no reason to build it without also building s3fwrn5_i2c.ko.

So perhaps you could merge the two Kconfig symbols you add in this
patch.

Or, if you'd like to keep both Kconfig symbols, for whatever reason, you
might want to make NFC_S3FWRN5 a silent symbol that will be selected
by NFC_S3FWRN5_I2C. (That probably requires NFC_S3FWRN5_I2C to depend on
both NFC_NCI and I2C.)

Would either of the above two options work here?

 +config NFC_S3FWRN5_I2C
 + tristate Samsung S3FWRN5 I2C support
 + depends on NFC_S3FWRN5  I2C
 + default y

You only added default y to make setting this symbol in make *config
a one step process, right?

 + ---help---
 +   This module adds support for an I2C interface to the S3FWRN5 chip.
 +   Select this if your platform is using the I2C bus.
 +
 +   To compile this driver as a module, choose m here. The module will
 +   be called s3fwrn5_i2c.ko.
 +   Say N if unsure.

(This advice is at odds with default y above, by the way.)

 --- /dev/null
 +++ b/drivers/nfc/s3fwrn5/Makefile

 +s3fwrn5-objs = core.o firmware.o nci.o
 +s3fwrn5_i2c-objs = i2c.o
 +
 +obj-$(CONFIG_NFC_S3FWRN5) += s3fwrn5.o
 +obj-$(CONFIG_NFC_S3FWRN5_I2C) += s3fwrn5_i2c.o

 --- /dev/null
 +++ b/drivers/nfc/s3fwrn5/core.c

 + * This program is free software; you can redistribute it and/or modify it
 + * under the terms and conditions of the GNU General Public License,
 + * version 2, as published by the Free Software Foundation.

 +int s3fwrn5_probe(struct nci_dev **ndev, void *phy_id, struct device *pdev,
 + struct s3fwrn5_phy_ops *phy_ops, unsigned int max_payload)
 +{
 + [...]
 +}
 +EXPORT_SYMBOL(s3fwrn5_probe);
 +
 +void s3fwrn5_remove(struct nci_dev *ndev)
 +{
 + [...]
 +}
 +EXPORT_SYMBOL(s3fwrn5_remove);
 +
 +int s3fwrn5_recv_frame(struct nci_dev *ndev, struct sk_buff *skb,
 + enum s3fwrn5_mode mode)
 +{
 + [...]
 +}
 +EXPORT_SYMBOL(s3fwrn5_recv_frame);

 +MODULE_LICENSE(GPL);

 --- /dev/null
 +++ b/drivers/nfc/s3fwrn5/i2c.c

 + * This program is free software; you can redistribute it and/or modify it
 + * under the terms and conditions of the GNU General Public License,
 + * version 2, as published by the Free Software Foundation.

 +static int s3fwrn5_i2c_fw_read(struct s3fwrn5_i2c_phy *phy)
 +{
 + [...]
 +
 + return s3fwrn5_recv_frame(phy-ndev, skb, S3FWRN5_MODE_FW);
 +}
 +
 +static int s3fwrn5_i2c_nci_read(struct s3fwrn5_i2c_phy *phy)
 +{
 + [...]
 +
 + return s3fwrn5_recv_frame(phy-ndev, skb, S3FWRN5_MODE_NCI);
 +}

 +static int s3fwrn5_i2c_probe(struct i2c_client *client,
 +   const struct i2c_device_id *id)
 +{
 + [...]
 +
 + ret = s3fwrn5_probe(phy-ndev, phy, phy-i2c_dev-dev, i2c_phy_ops,
 + S3FWRN5_I2C_MAX_PAYLOAD);
 + [...]
 + s3fwrn5_remove(phy-ndev);
 +
 + [...]
 +}
 +
 +static int s3fwrn5_i2c_remove(struct i2c_client *client)
 +{
 + [...]
 +
 + s3fwrn5_remove(phy-ndev);
 +
 + [...]
 +}

 +MODULE_LICENSE(GPL);

Nit: both modules' code contain a comment referring to the GNU General
Public License, version 2. They also both use the GPL ident in their
MODULE_LICENSE() macro. And, according to include/linux/module.h, that
ident states the license is GPL v2 or later. So I think either the
comments or the idents need to change.

Thanks,


Paul Bolle
--
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

[PATCH] nfc: s3fwrn5: Add driver for Samsung S3FWRN5 NFC Chip

2015-07-29 Thread Robert Baldyga 
Add driver for Samsung S3FWRN5 NFC controller.
S3FWRN5 is using NCI protocol and I2C communication interface.

Signed-off-by: Robert Baldyga r.bald...@samsung.com
---
 .../devicetree/bindings/net/nfc/s3fwrn5.txt|  27 ++
 MAINTAINERS|   6 +
 drivers/nfc/Kconfig|   1 +
 drivers/nfc/Makefile   |   1 +
 drivers/nfc/s3fwrn5/Kconfig|  22 +
 drivers/nfc/s3fwrn5/Makefile   |  11 +
 drivers/nfc/s3fwrn5/core.c | 232 +
 drivers/nfc/s3fwrn5/firmware.c | 522 +
 drivers/nfc/s3fwrn5/firmware.h | 111 +
 drivers/nfc/s3fwrn5/i2c.c  | 367 +++
 drivers/nfc/s3fwrn5/nci.c  | 364 ++
 drivers/nfc/s3fwrn5/nci.h  | 101 
 drivers/nfc/s3fwrn5/s3fwrn5.h  | 104 
 13 files changed, 1869 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/nfc/s3fwrn5.txt
 create mode 100644 drivers/nfc/s3fwrn5/Kconfig
 create mode 100644 drivers/nfc/s3fwrn5/Makefile
 create mode 100644 drivers/nfc/s3fwrn5/core.c
 create mode 100644 drivers/nfc/s3fwrn5/firmware.c
 create mode 100644 drivers/nfc/s3fwrn5/firmware.h
 create mode 100644 drivers/nfc/s3fwrn5/i2c.c
 create mode 100644 drivers/nfc/s3fwrn5/nci.c
 create mode 100644 drivers/nfc/s3fwrn5/nci.h
 create mode 100644 drivers/nfc/s3fwrn5/s3fwrn5.h

diff --git a/Documentation/devicetree/bindings/net/nfc/s3fwrn5.txt 
b/Documentation/devicetree/bindings/net/nfc/s3fwrn5.txt
new file mode 100644
index 000..fb1e75f
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/nfc/s3fwrn5.txt
@@ -0,0 +1,27 @@
+* Samsung S3FWRN5 NCI NFC Controller
+
+Required properties:
+- compatible: Should be samsung,s3fwrn5-i2c.
+- reg: address on the bus
+- interrupt-parent: phandle for the interrupt gpio controller
+- interrupts: GPIO interrupt to which the chip is connected
+- s3fwrn5,en-gpios: Output GPIO pin used for enabling/disabling the chip
+- s3fwrn5,fw-gpios: Output GPIO pin used to enter firmware mode and
+  sleep/wakeup control
+
+Example:
+
+hsi2c_4 {
+   status = okay;
+   s3fwrn5@27 {
+   compatible = samsung,s3fwrn5-i2c;
+
+   reg = 0x27;
+
+   interrupt-parent = gpa1;
+   interrupts = 3 0 0;
+
+   s3fwrn5,en-gpios = gpf1 4 0;
+   s3fwrn5,fw-gpios = gpj0 2 0;
+   };
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 3a4b7cb..72efe7c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8586,6 +8586,12 @@ L:   linux-me...@vger.kernel.org
 S: Supported
 F: drivers/media/i2c/s5k5baf.c
 
+SAMSUNG S3FWRN5 NFC DRIVER
+M: Robert Baldyga r.bald...@samsung.com
+L: linux-...@lists.01.org (moderated for non-subscribers)
+S: Supported
+F: drivers/nfc/s3fwrn5
+
 SAMSUNG SOC CLOCK DRIVERS
 M: Sylwester Nawrocki s.nawro...@samsung.com
 M: Tomasz Figa tomasz.f...@gmail.com
diff --git a/drivers/nfc/Kconfig b/drivers/nfc/Kconfig
index 722673c..6639cd1 100644
--- a/drivers/nfc/Kconfig
+++ b/drivers/nfc/Kconfig
@@ -74,4 +74,5 @@ source drivers/nfc/nfcmrvl/Kconfig
 source drivers/nfc/st21nfca/Kconfig
 source drivers/nfc/st-nci/Kconfig
 source drivers/nfc/nxp-nci/Kconfig
+source drivers/nfc/s3fwrn5/Kconfig
 endmenu
diff --git a/drivers/nfc/Makefile b/drivers/nfc/Makefile
index 368b6df..2757fe1 100644
--- a/drivers/nfc/Makefile
+++ b/drivers/nfc/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_NFC_TRF7970A)+= trf7970a.o
 obj-$(CONFIG_NFC_ST21NFCA) += st21nfca/
 obj-$(CONFIG_NFC_ST_NCI)   += st-nci/
 obj-$(CONFIG_NFC_NXP_NCI)  += nxp-nci/
+obj-$(CONFIG_NFC_S3FWRN5)  += s3fwrn5/
diff --git a/drivers/nfc/s3fwrn5/Kconfig b/drivers/nfc/s3fwrn5/Kconfig
new file mode 100644
index 000..d1f62e2
--- /dev/null
+++ b/drivers/nfc/s3fwrn5/Kconfig
@@ -0,0 +1,22 @@
+config NFC_S3FWRN5
+   tristate Samsung S3FWRN5 NFC driver
+   depends on NFC_NCI
+   default n
+   ---help---
+ Core driver for Samsung S3FWRN5 NFC chip.
+
+ To compile this driver as a module, choose m here. The module will
+ be called s3fwrn5.ko.
+ Say N if unsure.
+
+config NFC_S3FWRN5_I2C
+   tristate Samsung S3FWRN5 I2C support
+   depends on NFC_S3FWRN5  I2C
+   default y
+   ---help---
+ This module adds support for an I2C interface to the S3FWRN5 chip.
+ Select this if your platform is using the I2C bus.
+
+ To compile this driver as a module, choose m here. The module will
+ be called s3fwrn5_i2c.ko.
+ Say N if unsure.
diff --git a/drivers/nfc/s3fwrn5/Makefile b/drivers/nfc/s3fwrn5/Makefile
new file mode 100644
index 000..3381c34
--- /dev/null
+++ b/drivers/nfc/s3fwrn5/Makefile
@@ -0,0 +1,11 @@
+#
+# Makefile for Samsung S3FWRN5 NFC driver
+#
+
+s3fwrn5-objs =