From: Krzysztof Kozlowski <[email protected]>
GPIOs - as returned by of_get_named_gpio() and used by the gpiolib - are
signed integers, where negative number indicates error. The return
value of of_get_named_gpio() should not be assigned to an unsigned int
because in case of !CONFIG_GPIOLIB such number would be a valid GPIO.
Fixes: c04c674fadeb ("nfc: s3fwrn5: Add driver for Samsung S3FWRN5 NFC Chip")
Cc: <[email protected]>
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
drivers/nfc/s3fwrn5/i2c.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
index 0ffa389..ae26594 100644
--- a/drivers/nfc/s3fwrn5/i2c.c
+++ b/drivers/nfc/s3fwrn5/i2c.c
@@ -25,8 +25,8 @@ struct s3fwrn5_i2c_phy {
struct i2c_client *i2c_dev;
struct nci_dev *ndev;
- unsigned int gpio_en;
- unsigned int gpio_fw_wake;
+ int gpio_en;
+ int gpio_fw_wake;
struct mutex mutex;
--
1.9.1
From: Bongsu Jeon <[email protected]>
Extract the common phy blocks to reuse it.
The UART module will use the common blocks.
Signed-off-by: Bongsu Jeon <[email protected]>
---
drivers/nfc/s3fwrn5/i2c.c | 111 ++++++++++++---------------------------
drivers/nfc/s3fwrn5/phy_common.h | 86 ++++++++++++++++++++++++++++++
2 files changed, 119 insertions(+), 78 deletions(-)
create mode 100644 drivers/nfc/s3fwrn5/phy_common.h
diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
index 9a64eea..cd1b2a7 100644
--- a/drivers/nfc/s3fwrn5/i2c.c
+++ b/drivers/nfc/s3fwrn5/i2c.c
@@ -15,75 +15,30 @@
#include <net/nfc/nfc.h>
-#include "s3fwrn5.h"
+#include "phy_common.h"
#define S3FWRN5_I2C_DRIVER_NAME "s3fwrn5_i2c"
-#define S3FWRN5_EN_WAIT_TIME 20
-
struct s3fwrn5_i2c_phy {
+ struct phy_common common;
struct i2c_client *i2c_dev;
- struct nci_dev *ndev;
-
- int gpio_en;
- int gpio_fw_wake;
-
- struct mutex mutex;
- enum s3fwrn5_mode mode;
unsigned int irq_skip:1;
};
-static void s3fwrn5_i2c_set_wake(void *phy_id, bool wake)
-{
- struct s3fwrn5_i2c_phy *phy = phy_id;
-
- mutex_lock(&phy->mutex);
- gpio_set_value(phy->gpio_fw_wake, wake);
- msleep(S3FWRN5_EN_WAIT_TIME);
- mutex_unlock(&phy->mutex);
-}
-
static void s3fwrn5_i2c_set_mode(void *phy_id, enum s3fwrn5_mode mode)
{
struct s3fwrn5_i2c_phy *phy = phy_id;
- mutex_lock(&phy->mutex);
+ mutex_lock(&phy->common.mutex);
- if (phy->mode == mode)
+ if (s3fwrn5_phy_power_ctrl(&phy->common, mode) == false)
goto out;
- phy->mode = mode;
-
- gpio_set_value(phy->gpio_en, 1);
- gpio_set_value(phy->gpio_fw_wake, 0);
- if (mode == S3FWRN5_MODE_FW)
- gpio_set_value(phy->gpio_fw_wake, 1);
-
- if (mode != S3FWRN5_MODE_COLD) {
- msleep(S3FWRN5_EN_WAIT_TIME);
- gpio_set_value(phy->gpio_en, 0);
- msleep(S3FWRN5_EN_WAIT_TIME);
- }
-
phy->irq_skip = true;
out:
- mutex_unlock(&phy->mutex);
-}
-
-static enum s3fwrn5_mode s3fwrn5_i2c_get_mode(void *phy_id)
-{
- struct s3fwrn5_i2c_phy *phy = phy_id;
- enum s3fwrn5_mode mode;
-
- mutex_lock(&phy->mutex);
-
- mode = phy->mode;
-
- mutex_unlock(&phy->mutex);
-
- return mode;
+ mutex_unlock(&phy->common.mutex);
}
static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
@@ -91,7 +46,7 @@ static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
struct s3fwrn5_i2c_phy *phy = phy_id;
int ret;
- mutex_lock(&phy->mutex);
+ mutex_lock(&phy->common.mutex);
phy->irq_skip = false;
@@ -102,7 +57,7 @@ static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
ret = i2c_master_send(phy->i2c_dev, skb->data, skb->len);
}
- mutex_unlock(&phy->mutex);
+ mutex_unlock(&phy->common.mutex);
if (ret < 0)
return ret;
@@ -114,9 +69,9 @@ static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
}
static const struct s3fwrn5_phy_ops i2c_phy_ops = {
- .set_wake = s3fwrn5_i2c_set_wake,
+ .set_wake = s3fwrn5_phy_set_wake,
.set_mode = s3fwrn5_i2c_set_mode,
- .get_mode = s3fwrn5_i2c_get_mode,
+ .get_mode = s3fwrn5_phy_get_mode,
.write = s3fwrn5_i2c_write,
};
@@ -128,7 +83,7 @@ static int s3fwrn5_i2c_read(struct s3fwrn5_i2c_phy *phy)
char hdr[4];
int ret;
- hdr_size = (phy->mode == S3FWRN5_MODE_NCI) ?
+ hdr_size = (phy->common.mode == S3FWRN5_MODE_NCI) ?
NCI_CTRL_HDR_SIZE : S3FWRN5_FW_HDR_SIZE;
ret = i2c_master_recv(phy->i2c_dev, hdr, hdr_size);
if (ret < 0)
@@ -137,7 +92,7 @@ static int s3fwrn5_i2c_read(struct s3fwrn5_i2c_phy *phy)
if (ret < hdr_size)
return -EBADMSG;
- data_len = (phy->mode == S3FWRN5_MODE_NCI) ?
+ data_len = (phy->common.mode == S3FWRN5_MODE_NCI) ?
((struct nci_ctrl_hdr *)hdr)->plen :
((struct s3fwrn5_fw_header *)hdr)->len;
@@ -157,24 +112,24 @@ static int s3fwrn5_i2c_read(struct s3fwrn5_i2c_phy *phy)
}
out:
- return s3fwrn5_recv_frame(phy->ndev, skb, phy->mode);
+ return s3fwrn5_recv_frame(phy->common.ndev, skb, phy->common.mode);
}
static irqreturn_t s3fwrn5_i2c_irq_thread_fn(int irq, void *phy_id)
{
struct s3fwrn5_i2c_phy *phy = phy_id;
- if (!phy || !phy->ndev) {
+ if (!phy || !phy->common.ndev) {
WARN_ON_ONCE(1);
return IRQ_NONE;
}
- mutex_lock(&phy->mutex);
+ mutex_lock(&phy->common.mutex);
if (phy->irq_skip)
goto out;
- switch (phy->mode) {
+ switch (phy->common.mode) {
case S3FWRN5_MODE_NCI:
case S3FWRN5_MODE_FW:
s3fwrn5_i2c_read(phy);
@@ -184,7 +139,7 @@ static irqreturn_t s3fwrn5_i2c_irq_thread_fn(int irq, void *phy_id)
}
out:
- mutex_unlock(&phy->mutex);
+ mutex_unlock(&phy->common.mutex);
return IRQ_HANDLED;
}
@@ -197,19 +152,19 @@ static int s3fwrn5_i2c_parse_dt(struct i2c_client *client)
if (!np)
return -ENODEV;
- phy->gpio_en = of_get_named_gpio(np, "en-gpios", 0);
- if (!gpio_is_valid(phy->gpio_en)) {
+ phy->common.gpio_en = of_get_named_gpio(np, "en-gpios", 0);
+ if (!gpio_is_valid(phy->common.gpio_en)) {
/* Support also deprecated property */
- phy->gpio_en = of_get_named_gpio(np, "s3fwrn5,en-gpios", 0);
- if (!gpio_is_valid(phy->gpio_en))
+ phy->common.gpio_en = of_get_named_gpio(np, "s3fwrn5,en-gpios", 0);
+ if (!gpio_is_valid(phy->common.gpio_en))
return -ENODEV;
}
- phy->gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
- if (!gpio_is_valid(phy->gpio_fw_wake)) {
+ phy->common.gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
+ if (!gpio_is_valid(phy->common.gpio_fw_wake)) {
/* Support also deprecated property */
- phy->gpio_fw_wake = of_get_named_gpio(np, "s3fwrn5,fw-gpios", 0);
- if (!gpio_is_valid(phy->gpio_fw_wake))
+ phy->common.gpio_fw_wake = of_get_named_gpio(np, "s3fwrn5,fw-gpios", 0);
+ if (!gpio_is_valid(phy->common.gpio_fw_wake))
return -ENODEV;
}
@@ -226,8 +181,8 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
if (!phy)
return -ENOMEM;
- mutex_init(&phy->mutex);
- phy->mode = S3FWRN5_MODE_COLD;
+ mutex_init(&phy->common.mutex);
+ phy->common.mode = S3FWRN5_MODE_COLD;
phy->irq_skip = true;
phy->i2c_dev = client;
@@ -237,17 +192,17 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
if (ret < 0)
return ret;
- ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->gpio_en,
- GPIOF_OUT_INIT_HIGH, "s3fwrn5_en");
+ ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->common.gpio_en,
+ GPIOF_OUT_INIT_HIGH, "s3fwrn5_en");
if (ret < 0)
return ret;
- ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->gpio_fw_wake,
- GPIOF_OUT_INIT_LOW, "s3fwrn5_fw_wake");
+ ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->common.gpio_fw_wake,
+ GPIOF_OUT_INIT_LOW, "s3fwrn5_fw_wake");
if (ret < 0)
return ret;
- ret = s3fwrn5_probe(&phy->ndev, phy, &phy->i2c_dev->dev, &i2c_phy_ops);
+ ret = s3fwrn5_probe(&phy->common.ndev, phy, &phy->i2c_dev->dev, &i2c_phy_ops);
if (ret < 0)
return ret;
@@ -255,7 +210,7 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
s3fwrn5_i2c_irq_thread_fn, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
S3FWRN5_I2C_DRIVER_NAME, phy);
if (ret)
- s3fwrn5_remove(phy->ndev);
+ s3fwrn5_remove(phy->common.ndev);
return ret;
}
@@ -264,7 +219,7 @@ static int s3fwrn5_i2c_remove(struct i2c_client *client)
{
struct s3fwrn5_i2c_phy *phy = i2c_get_clientdata(client);
- s3fwrn5_remove(phy->ndev);
+ s3fwrn5_remove(phy->common.ndev);
return 0;
}
diff --git a/drivers/nfc/s3fwrn5/phy_common.h b/drivers/nfc/s3fwrn5/phy_common.h
new file mode 100644
index 0000000..14f7690
--- /dev/null
+++ b/drivers/nfc/s3fwrn5/phy_common.h
@@ -0,0 +1,86 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Link Layer for Samsung S3FWRN5 NCI based Driver
+ *
+ * Copyright (C) 2015 Samsung Electrnoics
+ * Robert Baldyga <[email protected]>
+ * Copyright (C) 2020 Samsung Electrnoics
+ * Bongsu Jeon <[email protected]>
+ */
+
+#ifndef __LOCAL_PHY_COMMON_H
+#define __LOCAL_PHY_COMMON_H
+
+#include "s3fwrn5.h"
+
+#define S3FWRN5_EN_WAIT_TIME 20
+
+struct phy_common {
+ struct nci_dev *ndev;
+
+ int gpio_en;
+ int gpio_fw_wake;
+
+ struct mutex mutex;
+
+ enum s3fwrn5_mode mode;
+};
+
+static inline void s3fwrn5_phy_set_wake(void *phy_id, bool wake)
+{
+ struct phy_common *phy = phy_id;
+
+ mutex_lock(&phy->mutex);
+ gpio_set_value(phy->gpio_fw_wake, wake);
+ msleep(S3FWRN5_EN_WAIT_TIME);
+ mutex_unlock(&phy->mutex);
+}
+
+static inline bool s3fwrn5_phy_power_ctrl(struct phy_common *phy,
+ enum s3fwrn5_mode mode)
+{
+ if (phy->mode == mode)
+ return false;
+
+ phy->mode = mode;
+
+ gpio_set_value(phy->gpio_en, 1);
+ gpio_set_value(phy->gpio_fw_wake, 0);
+ if (mode == S3FWRN5_MODE_FW)
+ gpio_set_value(phy->gpio_fw_wake, 1);
+
+ if (mode != S3FWRN5_MODE_COLD) {
+ msleep(S3FWRN5_EN_WAIT_TIME);
+ gpio_set_value(phy->gpio_en, 0);
+ msleep(S3FWRN5_EN_WAIT_TIME);
+ }
+
+ return true;
+}
+
+static inline void s3fwrn5_phy_set_mode(void *phy_id, enum s3fwrn5_mode mode)
+{
+ struct phy_common *phy = phy_id;
+
+ mutex_lock(&phy->mutex);
+
+ s3fwrn5_phy_power_ctrl(phy, mode);
+
+ mutex_unlock(&phy->mutex);
+}
+
+static inline enum s3fwrn5_mode s3fwrn5_phy_get_mode(void *phy_id)
+{
+ struct phy_common *phy = phy_id;
+ enum s3fwrn5_mode mode;
+
+ mutex_lock(&phy->mutex);
+
+ mode = phy->mode;
+
+ mutex_unlock(&phy->mutex);
+
+ return mode;
+}
+
+#endif /* __LOCAL_PHY_COMMON_H_ */
--
1.9.1
On Fri, Nov 27, 2020 at 12:33:37AM +0900, [email protected] wrote:
> From: Krzysztof Kozlowski <[email protected]>
>
> GPIOs - as returned by of_get_named_gpio() and used by the gpiolib - are
> signed integers, where negative number indicates error. The return
> value of of_get_named_gpio() should not be assigned to an unsigned int
> because in case of !CONFIG_GPIOLIB such number would be a valid GPIO.
>
> Fixes: c04c674fadeb ("nfc: s3fwrn5: Add driver for Samsung S3FWRN5 NFC Chip")
> Cc: <[email protected]>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
Why do you send my patch?
Best regards,
Krzysztof
From: Bongsu Jeon <[email protected]>
The delay of 20ms is enough to enable and
wake up the Samsung's nfc chip.
Signed-off-by: Bongsu Jeon <[email protected]>
---
drivers/nfc/s3fwrn5/i2c.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
index ae26594..9a64eea 100644
--- a/drivers/nfc/s3fwrn5/i2c.c
+++ b/drivers/nfc/s3fwrn5/i2c.c
@@ -19,7 +19,7 @@
#define S3FWRN5_I2C_DRIVER_NAME "s3fwrn5_i2c"
-#define S3FWRN5_EN_WAIT_TIME 150
+#define S3FWRN5_EN_WAIT_TIME 20
struct s3fwrn5_i2c_phy {
struct i2c_client *i2c_dev;
@@ -40,7 +40,7 @@ static void s3fwrn5_i2c_set_wake(void *phy_id, bool wake)
mutex_lock(&phy->mutex);
gpio_set_value(phy->gpio_fw_wake, wake);
- msleep(S3FWRN5_EN_WAIT_TIME/2);
+ msleep(S3FWRN5_EN_WAIT_TIME);
mutex_unlock(&phy->mutex);
}
@@ -63,7 +63,7 @@ static void s3fwrn5_i2c_set_mode(void *phy_id, enum s3fwrn5_mode mode)
if (mode != S3FWRN5_MODE_COLD) {
msleep(S3FWRN5_EN_WAIT_TIME);
gpio_set_value(phy->gpio_en, 0);
- msleep(S3FWRN5_EN_WAIT_TIME/2);
+ msleep(S3FWRN5_EN_WAIT_TIME);
}
phy->irq_skip = true;
--
1.9.1
On Fri, Nov 27, 2020 at 12:33:39AM +0900, [email protected] wrote:
> From: Bongsu Jeon <[email protected]>
>
> Extract the common phy blocks to reuse it.
> The UART module will use the common blocks.
Hi,
Thanks for the patch. Few comments below.
> Signed-off-by: Bongsu Jeon <[email protected]>
> ---
> drivers/nfc/s3fwrn5/i2c.c | 111 ++++++++++++---------------------------
> drivers/nfc/s3fwrn5/phy_common.h | 86 ++++++++++++++++++++++++++++++
> 2 files changed, 119 insertions(+), 78 deletions(-)
> create mode 100644 drivers/nfc/s3fwrn5/phy_common.h
>
> diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
> index 9a64eea..cd1b2a7 100644
> --- a/drivers/nfc/s3fwrn5/i2c.c
> +++ b/drivers/nfc/s3fwrn5/i2c.c
> @@ -15,75 +15,30 @@
>
> #include <net/nfc/nfc.h>
>
> -#include "s3fwrn5.h"
> +#include "phy_common.h"
>
> #define S3FWRN5_I2C_DRIVER_NAME "s3fwrn5_i2c"
>
> -#define S3FWRN5_EN_WAIT_TIME 20
> -
> struct s3fwrn5_i2c_phy {
> + struct phy_common common;
> struct i2c_client *i2c_dev;
> - struct nci_dev *ndev;
> -
> - int gpio_en;
> - int gpio_fw_wake;
> -
> - struct mutex mutex;
>
> - enum s3fwrn5_mode mode;
> unsigned int irq_skip:1;
> };
>
> -static void s3fwrn5_i2c_set_wake(void *phy_id, bool wake)
> -{
> - struct s3fwrn5_i2c_phy *phy = phy_id;
> -
> - mutex_lock(&phy->mutex);
> - gpio_set_value(phy->gpio_fw_wake, wake);
> - msleep(S3FWRN5_EN_WAIT_TIME);
> - mutex_unlock(&phy->mutex);
> -}
> -
> static void s3fwrn5_i2c_set_mode(void *phy_id, enum s3fwrn5_mode mode)
> {
> struct s3fwrn5_i2c_phy *phy = phy_id;
>
> - mutex_lock(&phy->mutex);
> + mutex_lock(&phy->common.mutex);
>
> - if (phy->mode == mode)
> + if (s3fwrn5_phy_power_ctrl(&phy->common, mode) == false)
> goto out;
>
> - phy->mode = mode;
> -
> - gpio_set_value(phy->gpio_en, 1);
> - gpio_set_value(phy->gpio_fw_wake, 0);
> - if (mode == S3FWRN5_MODE_FW)
> - gpio_set_value(phy->gpio_fw_wake, 1);
> -
> - if (mode != S3FWRN5_MODE_COLD) {
> - msleep(S3FWRN5_EN_WAIT_TIME);
> - gpio_set_value(phy->gpio_en, 0);
> - msleep(S3FWRN5_EN_WAIT_TIME);
> - }
> -
> phy->irq_skip = true;
>
> out:
> - mutex_unlock(&phy->mutex);
> -}
> -
> -static enum s3fwrn5_mode s3fwrn5_i2c_get_mode(void *phy_id)
> -{
> - struct s3fwrn5_i2c_phy *phy = phy_id;
> - enum s3fwrn5_mode mode;
> -
> - mutex_lock(&phy->mutex);
> -
> - mode = phy->mode;
> -
> - mutex_unlock(&phy->mutex);
> -
> - return mode;
> + mutex_unlock(&phy->common.mutex);
> }
>
> static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
> @@ -91,7 +46,7 @@ static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
> struct s3fwrn5_i2c_phy *phy = phy_id;
> int ret;
>
> - mutex_lock(&phy->mutex);
> + mutex_lock(&phy->common.mutex);
>
> phy->irq_skip = false;
>
> @@ -102,7 +57,7 @@ static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
> ret = i2c_master_send(phy->i2c_dev, skb->data, skb->len);
> }
>
> - mutex_unlock(&phy->mutex);
> + mutex_unlock(&phy->common.mutex);
>
> if (ret < 0)
> return ret;
> @@ -114,9 +69,9 @@ static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
> }
>
> static const struct s3fwrn5_phy_ops i2c_phy_ops = {
> - .set_wake = s3fwrn5_i2c_set_wake,
> + .set_wake = s3fwrn5_phy_set_wake,
> .set_mode = s3fwrn5_i2c_set_mode,
> - .get_mode = s3fwrn5_i2c_get_mode,
> + .get_mode = s3fwrn5_phy_get_mode,
> .write = s3fwrn5_i2c_write,
> };
>
> @@ -128,7 +83,7 @@ static int s3fwrn5_i2c_read(struct s3fwrn5_i2c_phy *phy)
> char hdr[4];
> int ret;
>
> - hdr_size = (phy->mode == S3FWRN5_MODE_NCI) ?
> + hdr_size = (phy->common.mode == S3FWRN5_MODE_NCI) ?
> NCI_CTRL_HDR_SIZE : S3FWRN5_FW_HDR_SIZE;
> ret = i2c_master_recv(phy->i2c_dev, hdr, hdr_size);
> if (ret < 0)
> @@ -137,7 +92,7 @@ static int s3fwrn5_i2c_read(struct s3fwrn5_i2c_phy *phy)
> if (ret < hdr_size)
> return -EBADMSG;
>
> - data_len = (phy->mode == S3FWRN5_MODE_NCI) ?
> + data_len = (phy->common.mode == S3FWRN5_MODE_NCI) ?
> ((struct nci_ctrl_hdr *)hdr)->plen :
> ((struct s3fwrn5_fw_header *)hdr)->len;
>
> @@ -157,24 +112,24 @@ static int s3fwrn5_i2c_read(struct s3fwrn5_i2c_phy *phy)
> }
>
> out:
> - return s3fwrn5_recv_frame(phy->ndev, skb, phy->mode);
> + return s3fwrn5_recv_frame(phy->common.ndev, skb, phy->common.mode);
> }
>
> static irqreturn_t s3fwrn5_i2c_irq_thread_fn(int irq, void *phy_id)
> {
> struct s3fwrn5_i2c_phy *phy = phy_id;
>
> - if (!phy || !phy->ndev) {
> + if (!phy || !phy->common.ndev) {
> WARN_ON_ONCE(1);
> return IRQ_NONE;
> }
>
> - mutex_lock(&phy->mutex);
> + mutex_lock(&phy->common.mutex);
>
> if (phy->irq_skip)
> goto out;
>
> - switch (phy->mode) {
> + switch (phy->common.mode) {
> case S3FWRN5_MODE_NCI:
> case S3FWRN5_MODE_FW:
> s3fwrn5_i2c_read(phy);
> @@ -184,7 +139,7 @@ static irqreturn_t s3fwrn5_i2c_irq_thread_fn(int irq, void *phy_id)
> }
>
> out:
> - mutex_unlock(&phy->mutex);
> + mutex_unlock(&phy->common.mutex);
>
> return IRQ_HANDLED;
> }
> @@ -197,19 +152,19 @@ static int s3fwrn5_i2c_parse_dt(struct i2c_client *client)
> if (!np)
> return -ENODEV;
>
> - phy->gpio_en = of_get_named_gpio(np, "en-gpios", 0);
> - if (!gpio_is_valid(phy->gpio_en)) {
> + phy->common.gpio_en = of_get_named_gpio(np, "en-gpios", 0);
> + if (!gpio_is_valid(phy->common.gpio_en)) {
> /* Support also deprecated property */
> - phy->gpio_en = of_get_named_gpio(np, "s3fwrn5,en-gpios", 0);
> - if (!gpio_is_valid(phy->gpio_en))
> + phy->common.gpio_en = of_get_named_gpio(np, "s3fwrn5,en-gpios", 0);
> + if (!gpio_is_valid(phy->common.gpio_en))
> return -ENODEV;
> }
>
> - phy->gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
> - if (!gpio_is_valid(phy->gpio_fw_wake)) {
> + phy->common.gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
> + if (!gpio_is_valid(phy->common.gpio_fw_wake)) {
> /* Support also deprecated property */
> - phy->gpio_fw_wake = of_get_named_gpio(np, "s3fwrn5,fw-gpios", 0);
> - if (!gpio_is_valid(phy->gpio_fw_wake))
> + phy->common.gpio_fw_wake = of_get_named_gpio(np, "s3fwrn5,fw-gpios", 0);
The lines here should wrap at 80 character.
> + if (!gpio_is_valid(phy->common.gpio_fw_wake))
> return -ENODEV;
> }
>
> @@ -226,8 +181,8 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
> if (!phy)
> return -ENOMEM;
>
> - mutex_init(&phy->mutex);
> - phy->mode = S3FWRN5_MODE_COLD;
> + mutex_init(&phy->common.mutex);
> + phy->common.mode = S3FWRN5_MODE_COLD;
> phy->irq_skip = true;
>
> phy->i2c_dev = client;
> @@ -237,17 +192,17 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
> if (ret < 0)
> return ret;
>
> - ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->gpio_en,
> - GPIOF_OUT_INIT_HIGH, "s3fwrn5_en");
> + ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->common.gpio_en,
> + GPIOF_OUT_INIT_HIGH, "s3fwrn5_en");
> if (ret < 0)
> return ret;
>
> - ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->gpio_fw_wake,
> - GPIOF_OUT_INIT_LOW, "s3fwrn5_fw_wake");
> + ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->common.gpio_fw_wake,
> + GPIOF_OUT_INIT_LOW, "s3fwrn5_fw_wake");
> if (ret < 0)
> return ret;
>
> - ret = s3fwrn5_probe(&phy->ndev, phy, &phy->i2c_dev->dev, &i2c_phy_ops);
> + ret = s3fwrn5_probe(&phy->common.ndev, phy, &phy->i2c_dev->dev, &i2c_phy_ops);
Please wrap the lines.
> if (ret < 0)
> return ret;
>
> @@ -255,7 +210,7 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
> s3fwrn5_i2c_irq_thread_fn, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> S3FWRN5_I2C_DRIVER_NAME, phy);
> if (ret)
> - s3fwrn5_remove(phy->ndev);
> + s3fwrn5_remove(phy->common.ndev);
>
> return ret;
> }
> @@ -264,7 +219,7 @@ static int s3fwrn5_i2c_remove(struct i2c_client *client)
> {
> struct s3fwrn5_i2c_phy *phy = i2c_get_clientdata(client);
>
> - s3fwrn5_remove(phy->ndev);
> + s3fwrn5_remove(phy->common.ndev);
>
> return 0;
> }
> diff --git a/drivers/nfc/s3fwrn5/phy_common.h b/drivers/nfc/s3fwrn5/phy_common.h
> new file mode 100644
> index 0000000..14f7690
> --- /dev/null
> +++ b/drivers/nfc/s3fwrn5/phy_common.h
> @@ -0,0 +1,86 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Link Layer for Samsung S3FWRN5 NCI based Driver
> + *
> + * Copyright (C) 2015 Samsung Electrnoics
> + * Robert Baldyga <[email protected]>
> + * Copyright (C) 2020 Samsung Electrnoics
> + * Bongsu Jeon <[email protected]>
> + */
> +
> +#ifndef __LOCAL_PHY_COMMON_H
> +#define __LOCAL_PHY_COMMON_H
Header guard: __NFC_S3FWRN5_PHY_COMMON_H
> +
> +#include "s3fwrn5.h"
This include should not be needed.
> +
> +#define S3FWRN5_EN_WAIT_TIME 20
> +
> +struct phy_common {
> + struct nci_dev *ndev;
> +
> + int gpio_en;
> + int gpio_fw_wake;
> +
> + struct mutex mutex;
> +
> + enum s3fwrn5_mode mode;
> +};
> +
> +static inline void s3fwrn5_phy_set_wake(void *phy_id, bool wake)
> +{
All these should go to a C source file. If needed - GPL exported.
> + struct phy_common *phy = phy_id;
> +
> + mutex_lock(&phy->mutex);
> + gpio_set_value(phy->gpio_fw_wake, wake);
> + msleep(S3FWRN5_EN_WAIT_TIME);
> + mutex_unlock(&phy->mutex);
> +}
> +
> +static inline bool s3fwrn5_phy_power_ctrl(struct phy_common *phy,
> + enum s3fwrn5_mode mode)
> +{
> + if (phy->mode == mode)
> + return false;
> +
> + phy->mode = mode;
> +
> + gpio_set_value(phy->gpio_en, 1);
> + gpio_set_value(phy->gpio_fw_wake, 0);
> + if (mode == S3FWRN5_MODE_FW)
> + gpio_set_value(phy->gpio_fw_wake, 1);
> +
> + if (mode != S3FWRN5_MODE_COLD) {
> + msleep(S3FWRN5_EN_WAIT_TIME);
> + gpio_set_value(phy->gpio_en, 0);
> + msleep(S3FWRN5_EN_WAIT_TIME);
> + }
> +
> + return true;
> +}
> +
> +static inline void s3fwrn5_phy_set_mode(void *phy_id, enum s3fwrn5_mode mode)
> +{
This looks unused. If you need it only for your next chip, add it with
next patch (the patch adding the user).
Best regards,
Krzysztof
On Fri, Nov 27, 2020 at 2:06 AM Krzysztof Kozlowski <[email protected]> wrote:
>
> On Fri, Nov 27, 2020 at 12:33:37AM +0900, [email protected] wrote:
> > From: Krzysztof Kozlowski <[email protected]>
> >
> > GPIOs - as returned by of_get_named_gpio() and used by the gpiolib - are
> > signed integers, where negative number indicates error. The return
> > value of of_get_named_gpio() should not be assigned to an unsigned int
> > because in case of !CONFIG_GPIOLIB such number would be a valid GPIO.
> >
> > Fixes: c04c674fadeb ("nfc: s3fwrn5: Add driver for Samsung S3FWRN5 NFC Chip")
> > Cc: <[email protected]>
> > Signed-off-by: Krzysztof Kozlowski <[email protected]>
>
> Why do you send my patch?
>
I think that your patch should be applied before refactoring for this driver.
So, I applied your patch to net-next branch and included your patch at
my patch list.
Is this the wrong process?
> Best regards,
> Krzysztof
On Fri, Nov 27, 2020 at 2:13 AM Krzysztof Kozlowski <[email protected]> wrote:
>
> On Fri, Nov 27, 2020 at 12:33:39AM +0900, [email protected] wrote:
> > From: Bongsu Jeon <[email protected]>
> >
> > Extract the common phy blocks to reuse it.
> > The UART module will use the common blocks.
>
>
> Hi,
>
> Thanks for the patch. Few comments below.
>
> > Signed-off-by: Bongsu Jeon <[email protected]>
> > ---
> > drivers/nfc/s3fwrn5/i2c.c | 111 ++++++++++++---------------------------
> > drivers/nfc/s3fwrn5/phy_common.h | 86 ++++++++++++++++++++++++++++++
> > 2 files changed, 119 insertions(+), 78 deletions(-)
> > create mode 100644 drivers/nfc/s3fwrn5/phy_common.h
> >
> > diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
> > index 9a64eea..cd1b2a7 100644
> > --- a/drivers/nfc/s3fwrn5/i2c.c
> > +++ b/drivers/nfc/s3fwrn5/i2c.c
> > @@ -15,75 +15,30 @@
> >
> > #include <net/nfc/nfc.h>
> >
> > -#include "s3fwrn5.h"
> > +#include "phy_common.h"
> >
> > #define S3FWRN5_I2C_DRIVER_NAME "s3fwrn5_i2c"
> >
> > -#define S3FWRN5_EN_WAIT_TIME 20
> > -
> > struct s3fwrn5_i2c_phy {
> > + struct phy_common common;
> > struct i2c_client *i2c_dev;
> > - struct nci_dev *ndev;
> > -
> > - int gpio_en;
> > - int gpio_fw_wake;
> > -
> > - struct mutex mutex;
> >
> > - enum s3fwrn5_mode mode;
> > unsigned int irq_skip:1;
> > };
> >
> > -static void s3fwrn5_i2c_set_wake(void *phy_id, bool wake)
> > -{
> > - struct s3fwrn5_i2c_phy *phy = phy_id;
> > -
> > - mutex_lock(&phy->mutex);
> > - gpio_set_value(phy->gpio_fw_wake, wake);
> > - msleep(S3FWRN5_EN_WAIT_TIME);
> > - mutex_unlock(&phy->mutex);
> > -}
> > -
> > static void s3fwrn5_i2c_set_mode(void *phy_id, enum s3fwrn5_mode mode)
> > {
> > struct s3fwrn5_i2c_phy *phy = phy_id;
> >
> > - mutex_lock(&phy->mutex);
> > + mutex_lock(&phy->common.mutex);
> >
> > - if (phy->mode == mode)
> > + if (s3fwrn5_phy_power_ctrl(&phy->common, mode) == false)
> > goto out;
> >
> > - phy->mode = mode;
> > -
> > - gpio_set_value(phy->gpio_en, 1);
> > - gpio_set_value(phy->gpio_fw_wake, 0);
> > - if (mode == S3FWRN5_MODE_FW)
> > - gpio_set_value(phy->gpio_fw_wake, 1);
> > -
> > - if (mode != S3FWRN5_MODE_COLD) {
> > - msleep(S3FWRN5_EN_WAIT_TIME);
> > - gpio_set_value(phy->gpio_en, 0);
> > - msleep(S3FWRN5_EN_WAIT_TIME);
> > - }
> > -
> > phy->irq_skip = true;
> >
> > out:
> > - mutex_unlock(&phy->mutex);
> > -}
> > -
> > -static enum s3fwrn5_mode s3fwrn5_i2c_get_mode(void *phy_id)
> > -{
> > - struct s3fwrn5_i2c_phy *phy = phy_id;
> > - enum s3fwrn5_mode mode;
> > -
> > - mutex_lock(&phy->mutex);
> > -
> > - mode = phy->mode;
> > -
> > - mutex_unlock(&phy->mutex);
> > -
> > - return mode;
> > + mutex_unlock(&phy->common.mutex);
> > }
> >
> > static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
> > @@ -91,7 +46,7 @@ static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
> > struct s3fwrn5_i2c_phy *phy = phy_id;
> > int ret;
> >
> > - mutex_lock(&phy->mutex);
> > + mutex_lock(&phy->common.mutex);
> >
> > phy->irq_skip = false;
> >
> > @@ -102,7 +57,7 @@ static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
> > ret = i2c_master_send(phy->i2c_dev, skb->data, skb->len);
> > }
> >
> > - mutex_unlock(&phy->mutex);
> > + mutex_unlock(&phy->common.mutex);
> >
> > if (ret < 0)
> > return ret;
> > @@ -114,9 +69,9 @@ static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
> > }
> >
> > static const struct s3fwrn5_phy_ops i2c_phy_ops = {
> > - .set_wake = s3fwrn5_i2c_set_wake,
> > + .set_wake = s3fwrn5_phy_set_wake,
> > .set_mode = s3fwrn5_i2c_set_mode,
> > - .get_mode = s3fwrn5_i2c_get_mode,
> > + .get_mode = s3fwrn5_phy_get_mode,
> > .write = s3fwrn5_i2c_write,
> > };
> >
> > @@ -128,7 +83,7 @@ static int s3fwrn5_i2c_read(struct s3fwrn5_i2c_phy *phy)
> > char hdr[4];
> > int ret;
> >
> > - hdr_size = (phy->mode == S3FWRN5_MODE_NCI) ?
> > + hdr_size = (phy->common.mode == S3FWRN5_MODE_NCI) ?
> > NCI_CTRL_HDR_SIZE : S3FWRN5_FW_HDR_SIZE;
> > ret = i2c_master_recv(phy->i2c_dev, hdr, hdr_size);
> > if (ret < 0)
> > @@ -137,7 +92,7 @@ static int s3fwrn5_i2c_read(struct s3fwrn5_i2c_phy *phy)
> > if (ret < hdr_size)
> > return -EBADMSG;
> >
> > - data_len = (phy->mode == S3FWRN5_MODE_NCI) ?
> > + data_len = (phy->common.mode == S3FWRN5_MODE_NCI) ?
> > ((struct nci_ctrl_hdr *)hdr)->plen :
> > ((struct s3fwrn5_fw_header *)hdr)->len;
> >
> > @@ -157,24 +112,24 @@ static int s3fwrn5_i2c_read(struct s3fwrn5_i2c_phy *phy)
> > }
> >
> > out:
> > - return s3fwrn5_recv_frame(phy->ndev, skb, phy->mode);
> > + return s3fwrn5_recv_frame(phy->common.ndev, skb, phy->common.mode);
> > }
> >
> > static irqreturn_t s3fwrn5_i2c_irq_thread_fn(int irq, void *phy_id)
> > {
> > struct s3fwrn5_i2c_phy *phy = phy_id;
> >
> > - if (!phy || !phy->ndev) {
> > + if (!phy || !phy->common.ndev) {
> > WARN_ON_ONCE(1);
> > return IRQ_NONE;
> > }
> >
> > - mutex_lock(&phy->mutex);
> > + mutex_lock(&phy->common.mutex);
> >
> > if (phy->irq_skip)
> > goto out;
> >
> > - switch (phy->mode) {
> > + switch (phy->common.mode) {
> > case S3FWRN5_MODE_NCI:
> > case S3FWRN5_MODE_FW:
> > s3fwrn5_i2c_read(phy);
> > @@ -184,7 +139,7 @@ static irqreturn_t s3fwrn5_i2c_irq_thread_fn(int irq, void *phy_id)
> > }
> >
> > out:
> > - mutex_unlock(&phy->mutex);
> > + mutex_unlock(&phy->common.mutex);
> >
> > return IRQ_HANDLED;
> > }
> > @@ -197,19 +152,19 @@ static int s3fwrn5_i2c_parse_dt(struct i2c_client *client)
> > if (!np)
> > return -ENODEV;
> >
> > - phy->gpio_en = of_get_named_gpio(np, "en-gpios", 0);
> > - if (!gpio_is_valid(phy->gpio_en)) {
> > + phy->common.gpio_en = of_get_named_gpio(np, "en-gpios", 0);
> > + if (!gpio_is_valid(phy->common.gpio_en)) {
> > /* Support also deprecated property */
> > - phy->gpio_en = of_get_named_gpio(np, "s3fwrn5,en-gpios", 0);
> > - if (!gpio_is_valid(phy->gpio_en))
> > + phy->common.gpio_en = of_get_named_gpio(np, "s3fwrn5,en-gpios", 0);
> > + if (!gpio_is_valid(phy->common.gpio_en))
> > return -ENODEV;
> > }
> >
> > - phy->gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
> > - if (!gpio_is_valid(phy->gpio_fw_wake)) {
> > + phy->common.gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
> > + if (!gpio_is_valid(phy->common.gpio_fw_wake)) {
> > /* Support also deprecated property */
> > - phy->gpio_fw_wake = of_get_named_gpio(np, "s3fwrn5,fw-gpios", 0);
> > - if (!gpio_is_valid(phy->gpio_fw_wake))
> > + phy->common.gpio_fw_wake = of_get_named_gpio(np, "s3fwrn5,fw-gpios", 0);
>
> The lines here should wrap at 80 character.
>
> > + if (!gpio_is_valid(phy->common.gpio_fw_wake))
> > return -ENODEV;
> > }
> >
> > @@ -226,8 +181,8 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
> > if (!phy)
> > return -ENOMEM;
> >
> > - mutex_init(&phy->mutex);
> > - phy->mode = S3FWRN5_MODE_COLD;
> > + mutex_init(&phy->common.mutex);
> > + phy->common.mode = S3FWRN5_MODE_COLD;
> > phy->irq_skip = true;
> >
> > phy->i2c_dev = client;
> > @@ -237,17 +192,17 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
> > if (ret < 0)
> > return ret;
> >
> > - ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->gpio_en,
> > - GPIOF_OUT_INIT_HIGH, "s3fwrn5_en");
> > + ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->common.gpio_en,
> > + GPIOF_OUT_INIT_HIGH, "s3fwrn5_en");
> > if (ret < 0)
> > return ret;
> >
> > - ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->gpio_fw_wake,
> > - GPIOF_OUT_INIT_LOW, "s3fwrn5_fw_wake");
> > + ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->common.gpio_fw_wake,
> > + GPIOF_OUT_INIT_LOW, "s3fwrn5_fw_wake");
> > if (ret < 0)
> > return ret;
> >
> > - ret = s3fwrn5_probe(&phy->ndev, phy, &phy->i2c_dev->dev, &i2c_phy_ops);
> > + ret = s3fwrn5_probe(&phy->common.ndev, phy, &phy->i2c_dev->dev, &i2c_phy_ops);
>
> Please wrap the lines.
>
> > if (ret < 0)
> > return ret;
> >
> > @@ -255,7 +210,7 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
> > s3fwrn5_i2c_irq_thread_fn, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > S3FWRN5_I2C_DRIVER_NAME, phy);
> > if (ret)
> > - s3fwrn5_remove(phy->ndev);
> > + s3fwrn5_remove(phy->common.ndev);
> >
> > return ret;
> > }
> > @@ -264,7 +219,7 @@ static int s3fwrn5_i2c_remove(struct i2c_client *client)
> > {
> > struct s3fwrn5_i2c_phy *phy = i2c_get_clientdata(client);
> >
> > - s3fwrn5_remove(phy->ndev);
> > + s3fwrn5_remove(phy->common.ndev);
> >
> > return 0;
> > }
> > diff --git a/drivers/nfc/s3fwrn5/phy_common.h b/drivers/nfc/s3fwrn5/phy_common.h
> > new file mode 100644
> > index 0000000..14f7690
> > --- /dev/null
> > +++ b/drivers/nfc/s3fwrn5/phy_common.h
> > @@ -0,0 +1,86 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later
> > + *
> > + * Link Layer for Samsung S3FWRN5 NCI based Driver
> > + *
> > + * Copyright (C) 2015 Samsung Electrnoics
> > + * Robert Baldyga <[email protected]>
> > + * Copyright (C) 2020 Samsung Electrnoics
> > + * Bongsu Jeon <[email protected]>
> > + */
> > +
> > +#ifndef __LOCAL_PHY_COMMON_H
> > +#define __LOCAL_PHY_COMMON_H
>
> Header guard: __NFC_S3FWRN5_PHY_COMMON_H
>
> > +
> > +#include "s3fwrn5.h"
>
> This include should not be needed.
>
Actually, I included this because of enum s3fwrn5_mode.
Do you think the following structure is good?
0. remove the '#include "s3fwrn5.h" and the common function's
definition in phy_common.h.
1. make phy_common.c that includes the common function's definition
and "s3fwrn5.h , phy_common.h".
2. i2c.c includes "s3fwrn5.h , phy_common.h".
> > +
> > +#define S3FWRN5_EN_WAIT_TIME 20
> > +
> > +struct phy_common {
> > + struct nci_dev *ndev;
> > +
> > + int gpio_en;
> > + int gpio_fw_wake;
> > +
> > + struct mutex mutex;
> > +
> > + enum s3fwrn5_mode mode;
> > +};
> > +
> > +static inline void s3fwrn5_phy_set_wake(void *phy_id, bool wake)
> > +{
>
> All these should go to a C source file. If needed - GPL exported.
>
> > + struct phy_common *phy = phy_id;
> > +
> > + mutex_lock(&phy->mutex);
> > + gpio_set_value(phy->gpio_fw_wake, wake);
> > + msleep(S3FWRN5_EN_WAIT_TIME);
> > + mutex_unlock(&phy->mutex);
> > +}
> > +
> > +static inline bool s3fwrn5_phy_power_ctrl(struct phy_common *phy,
> > + enum s3fwrn5_mode mode)
> > +{
> > + if (phy->mode == mode)
> > + return false;
> > +
> > + phy->mode = mode;
> > +
> > + gpio_set_value(phy->gpio_en, 1);
> > + gpio_set_value(phy->gpio_fw_wake, 0);
> > + if (mode == S3FWRN5_MODE_FW)
> > + gpio_set_value(phy->gpio_fw_wake, 1);
> > +
> > + if (mode != S3FWRN5_MODE_COLD) {
> > + msleep(S3FWRN5_EN_WAIT_TIME);
> > + gpio_set_value(phy->gpio_en, 0);
> > + msleep(S3FWRN5_EN_WAIT_TIME);
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static inline void s3fwrn5_phy_set_mode(void *phy_id, enum s3fwrn5_mode mode)
> > +{
>
> This looks unused. If you need it only for your next chip, add it with
> next patch (the patch adding the user).
>
Okay I understand it.
> Best regards,
> Krzysztof
On 11/27/20, Bongsu Jeon <[email protected]> wrote:
> On Fri, Nov 27, 2020 at 2:06 AM Krzysztof Kozlowski <[email protected]>
> wrote:
>>
>> On Fri, Nov 27, 2020 at 12:33:37AM +0900, [email protected] wrote:
>> > From: Krzysztof Kozlowski <[email protected]>
>> >
>> > GPIOs - as returned by of_get_named_gpio() and used by the gpiolib -
>> > are
>> > signed integers, where negative number indicates error. The return
>> > value of of_get_named_gpio() should not be assigned to an unsigned int
>> > because in case of !CONFIG_GPIOLIB such number would be a valid GPIO.
>> >
>> > Fixes: c04c674fadeb ("nfc: s3fwrn5: Add driver for Samsung S3FWRN5 NFC
>> > Chip")
>> > Cc: <[email protected]>
>> > Signed-off-by: Krzysztof Kozlowski <[email protected]>
>>
>> Why do you send my patch?
>>
>
> I think that your patch should be applied before refactoring for this
> driver.
> So, I applied your patch to net-next branch and included your patch at
> my patch list.
> Is this the wrong process?
>
Sorry to confuse you.
I found your patch when i updated my workspace using git pull.
>> Best regards,
>> Krzysztof
>
On Fri, Nov 27, 2020 at 08:09:24AM +0900, Bongsu Jeon wrote:
> On Fri, Nov 27, 2020 at 2:13 AM Krzysztof Kozlowski <[email protected]> wrote:
> >
> > On Fri, Nov 27, 2020 at 12:33:39AM +0900, [email protected] wrote:
> > > From: Bongsu Jeon <[email protected]>
> > >
> > > Extract the common phy blocks to reuse it.
> > > The UART module will use the common blocks.
> >
> >
> > Hi,
> >
> > Thanks for the patch. Few comments below.
> >
> > > Signed-off-by: Bongsu Jeon <[email protected]>
> > > ---
> > > drivers/nfc/s3fwrn5/i2c.c | 111 ++++++++++++---------------------------
> > > drivers/nfc/s3fwrn5/phy_common.h | 86 ++++++++++++++++++++++++++++++
> > > 2 files changed, 119 insertions(+), 78 deletions(-)
> > > create mode 100644 drivers/nfc/s3fwrn5/phy_common.h
> > >
> > > diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
> > > index 9a64eea..cd1b2a7 100644
> > > --- a/drivers/nfc/s3fwrn5/i2c.c
> > > +++ b/drivers/nfc/s3fwrn5/i2c.c
> > > @@ -15,75 +15,30 @@
> > >
> > > #include <net/nfc/nfc.h>
> > >
> > > -#include "s3fwrn5.h"
> > > +#include "phy_common.h"
> > >
> > > #define S3FWRN5_I2C_DRIVER_NAME "s3fwrn5_i2c"
> > >
> > > -#define S3FWRN5_EN_WAIT_TIME 20
> > > -
> > > struct s3fwrn5_i2c_phy {
> > > + struct phy_common common;
> > > struct i2c_client *i2c_dev;
> > > - struct nci_dev *ndev;
> > > -
> > > - int gpio_en;
> > > - int gpio_fw_wake;
> > > -
> > > - struct mutex mutex;
> > >
> > > - enum s3fwrn5_mode mode;
> > > unsigned int irq_skip:1;
> > > };
> > >
> > > -static void s3fwrn5_i2c_set_wake(void *phy_id, bool wake)
> > > -{
> > > - struct s3fwrn5_i2c_phy *phy = phy_id;
> > > -
> > > - mutex_lock(&phy->mutex);
> > > - gpio_set_value(phy->gpio_fw_wake, wake);
> > > - msleep(S3FWRN5_EN_WAIT_TIME);
> > > - mutex_unlock(&phy->mutex);
> > > -}
> > > -
> > > static void s3fwrn5_i2c_set_mode(void *phy_id, enum s3fwrn5_mode mode)
> > > {
> > > struct s3fwrn5_i2c_phy *phy = phy_id;
> > >
> > > - mutex_lock(&phy->mutex);
> > > + mutex_lock(&phy->common.mutex);
> > >
> > > - if (phy->mode == mode)
> > > + if (s3fwrn5_phy_power_ctrl(&phy->common, mode) == false)
> > > goto out;
> > >
> > > - phy->mode = mode;
> > > -
> > > - gpio_set_value(phy->gpio_en, 1);
> > > - gpio_set_value(phy->gpio_fw_wake, 0);
> > > - if (mode == S3FWRN5_MODE_FW)
> > > - gpio_set_value(phy->gpio_fw_wake, 1);
> > > -
> > > - if (mode != S3FWRN5_MODE_COLD) {
> > > - msleep(S3FWRN5_EN_WAIT_TIME);
> > > - gpio_set_value(phy->gpio_en, 0);
> > > - msleep(S3FWRN5_EN_WAIT_TIME);
> > > - }
> > > -
> > > phy->irq_skip = true;
> > >
> > > out:
> > > - mutex_unlock(&phy->mutex);
> > > -}
> > > -
> > > -static enum s3fwrn5_mode s3fwrn5_i2c_get_mode(void *phy_id)
> > > -{
> > > - struct s3fwrn5_i2c_phy *phy = phy_id;
> > > - enum s3fwrn5_mode mode;
> > > -
> > > - mutex_lock(&phy->mutex);
> > > -
> > > - mode = phy->mode;
> > > -
> > > - mutex_unlock(&phy->mutex);
> > > -
> > > - return mode;
> > > + mutex_unlock(&phy->common.mutex);
> > > }
> > >
> > > static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
> > > @@ -91,7 +46,7 @@ static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
> > > struct s3fwrn5_i2c_phy *phy = phy_id;
> > > int ret;
> > >
> > > - mutex_lock(&phy->mutex);
> > > + mutex_lock(&phy->common.mutex);
> > >
> > > phy->irq_skip = false;
> > >
> > > @@ -102,7 +57,7 @@ static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
> > > ret = i2c_master_send(phy->i2c_dev, skb->data, skb->len);
> > > }
> > >
> > > - mutex_unlock(&phy->mutex);
> > > + mutex_unlock(&phy->common.mutex);
> > >
> > > if (ret < 0)
> > > return ret;
> > > @@ -114,9 +69,9 @@ static int s3fwrn5_i2c_write(void *phy_id, struct sk_buff *skb)
> > > }
> > >
> > > static const struct s3fwrn5_phy_ops i2c_phy_ops = {
> > > - .set_wake = s3fwrn5_i2c_set_wake,
> > > + .set_wake = s3fwrn5_phy_set_wake,
> > > .set_mode = s3fwrn5_i2c_set_mode,
> > > - .get_mode = s3fwrn5_i2c_get_mode,
> > > + .get_mode = s3fwrn5_phy_get_mode,
> > > .write = s3fwrn5_i2c_write,
> > > };
> > >
> > > @@ -128,7 +83,7 @@ static int s3fwrn5_i2c_read(struct s3fwrn5_i2c_phy *phy)
> > > char hdr[4];
> > > int ret;
> > >
> > > - hdr_size = (phy->mode == S3FWRN5_MODE_NCI) ?
> > > + hdr_size = (phy->common.mode == S3FWRN5_MODE_NCI) ?
> > > NCI_CTRL_HDR_SIZE : S3FWRN5_FW_HDR_SIZE;
> > > ret = i2c_master_recv(phy->i2c_dev, hdr, hdr_size);
> > > if (ret < 0)
> > > @@ -137,7 +92,7 @@ static int s3fwrn5_i2c_read(struct s3fwrn5_i2c_phy *phy)
> > > if (ret < hdr_size)
> > > return -EBADMSG;
> > >
> > > - data_len = (phy->mode == S3FWRN5_MODE_NCI) ?
> > > + data_len = (phy->common.mode == S3FWRN5_MODE_NCI) ?
> > > ((struct nci_ctrl_hdr *)hdr)->plen :
> > > ((struct s3fwrn5_fw_header *)hdr)->len;
> > >
> > > @@ -157,24 +112,24 @@ static int s3fwrn5_i2c_read(struct s3fwrn5_i2c_phy *phy)
> > > }
> > >
> > > out:
> > > - return s3fwrn5_recv_frame(phy->ndev, skb, phy->mode);
> > > + return s3fwrn5_recv_frame(phy->common.ndev, skb, phy->common.mode);
> > > }
> > >
> > > static irqreturn_t s3fwrn5_i2c_irq_thread_fn(int irq, void *phy_id)
> > > {
> > > struct s3fwrn5_i2c_phy *phy = phy_id;
> > >
> > > - if (!phy || !phy->ndev) {
> > > + if (!phy || !phy->common.ndev) {
> > > WARN_ON_ONCE(1);
> > > return IRQ_NONE;
> > > }
> > >
> > > - mutex_lock(&phy->mutex);
> > > + mutex_lock(&phy->common.mutex);
> > >
> > > if (phy->irq_skip)
> > > goto out;
> > >
> > > - switch (phy->mode) {
> > > + switch (phy->common.mode) {
> > > case S3FWRN5_MODE_NCI:
> > > case S3FWRN5_MODE_FW:
> > > s3fwrn5_i2c_read(phy);
> > > @@ -184,7 +139,7 @@ static irqreturn_t s3fwrn5_i2c_irq_thread_fn(int irq, void *phy_id)
> > > }
> > >
> > > out:
> > > - mutex_unlock(&phy->mutex);
> > > + mutex_unlock(&phy->common.mutex);
> > >
> > > return IRQ_HANDLED;
> > > }
> > > @@ -197,19 +152,19 @@ static int s3fwrn5_i2c_parse_dt(struct i2c_client *client)
> > > if (!np)
> > > return -ENODEV;
> > >
> > > - phy->gpio_en = of_get_named_gpio(np, "en-gpios", 0);
> > > - if (!gpio_is_valid(phy->gpio_en)) {
> > > + phy->common.gpio_en = of_get_named_gpio(np, "en-gpios", 0);
> > > + if (!gpio_is_valid(phy->common.gpio_en)) {
> > > /* Support also deprecated property */
> > > - phy->gpio_en = of_get_named_gpio(np, "s3fwrn5,en-gpios", 0);
> > > - if (!gpio_is_valid(phy->gpio_en))
> > > + phy->common.gpio_en = of_get_named_gpio(np, "s3fwrn5,en-gpios", 0);
> > > + if (!gpio_is_valid(phy->common.gpio_en))
> > > return -ENODEV;
> > > }
> > >
> > > - phy->gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
> > > - if (!gpio_is_valid(phy->gpio_fw_wake)) {
> > > + phy->common.gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
> > > + if (!gpio_is_valid(phy->common.gpio_fw_wake)) {
> > > /* Support also deprecated property */
> > > - phy->gpio_fw_wake = of_get_named_gpio(np, "s3fwrn5,fw-gpios", 0);
> > > - if (!gpio_is_valid(phy->gpio_fw_wake))
> > > + phy->common.gpio_fw_wake = of_get_named_gpio(np, "s3fwrn5,fw-gpios", 0);
> >
> > The lines here should wrap at 80 character.
> >
> > > + if (!gpio_is_valid(phy->common.gpio_fw_wake))
> > > return -ENODEV;
> > > }
> > >
> > > @@ -226,8 +181,8 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
> > > if (!phy)
> > > return -ENOMEM;
> > >
> > > - mutex_init(&phy->mutex);
> > > - phy->mode = S3FWRN5_MODE_COLD;
> > > + mutex_init(&phy->common.mutex);
> > > + phy->common.mode = S3FWRN5_MODE_COLD;
> > > phy->irq_skip = true;
> > >
> > > phy->i2c_dev = client;
> > > @@ -237,17 +192,17 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
> > > if (ret < 0)
> > > return ret;
> > >
> > > - ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->gpio_en,
> > > - GPIOF_OUT_INIT_HIGH, "s3fwrn5_en");
> > > + ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->common.gpio_en,
> > > + GPIOF_OUT_INIT_HIGH, "s3fwrn5_en");
> > > if (ret < 0)
> > > return ret;
> > >
> > > - ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->gpio_fw_wake,
> > > - GPIOF_OUT_INIT_LOW, "s3fwrn5_fw_wake");
> > > + ret = devm_gpio_request_one(&phy->i2c_dev->dev, phy->common.gpio_fw_wake,
> > > + GPIOF_OUT_INIT_LOW, "s3fwrn5_fw_wake");
> > > if (ret < 0)
> > > return ret;
> > >
> > > - ret = s3fwrn5_probe(&phy->ndev, phy, &phy->i2c_dev->dev, &i2c_phy_ops);
> > > + ret = s3fwrn5_probe(&phy->common.ndev, phy, &phy->i2c_dev->dev, &i2c_phy_ops);
> >
> > Please wrap the lines.
> >
> > > if (ret < 0)
> > > return ret;
> > >
> > > @@ -255,7 +210,7 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
> > > s3fwrn5_i2c_irq_thread_fn, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > > S3FWRN5_I2C_DRIVER_NAME, phy);
> > > if (ret)
> > > - s3fwrn5_remove(phy->ndev);
> > > + s3fwrn5_remove(phy->common.ndev);
> > >
> > > return ret;
> > > }
> > > @@ -264,7 +219,7 @@ static int s3fwrn5_i2c_remove(struct i2c_client *client)
> > > {
> > > struct s3fwrn5_i2c_phy *phy = i2c_get_clientdata(client);
> > >
> > > - s3fwrn5_remove(phy->ndev);
> > > + s3fwrn5_remove(phy->common.ndev);
> > >
> > > return 0;
> > > }
> > > diff --git a/drivers/nfc/s3fwrn5/phy_common.h b/drivers/nfc/s3fwrn5/phy_common.h
> > > new file mode 100644
> > > index 0000000..14f7690
> > > --- /dev/null
> > > +++ b/drivers/nfc/s3fwrn5/phy_common.h
> > > @@ -0,0 +1,86 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later
> > > + *
> > > + * Link Layer for Samsung S3FWRN5 NCI based Driver
> > > + *
> > > + * Copyright (C) 2015 Samsung Electrnoics
> > > + * Robert Baldyga <[email protected]>
> > > + * Copyright (C) 2020 Samsung Electrnoics
> > > + * Bongsu Jeon <[email protected]>
> > > + */
> > > +
> > > +#ifndef __LOCAL_PHY_COMMON_H
> > > +#define __LOCAL_PHY_COMMON_H
> >
> > Header guard: __NFC_S3FWRN5_PHY_COMMON_H
> >
> > > +
> > > +#include "s3fwrn5.h"
> >
> > This include should not be needed.
> >
>
> Actually, I included this because of enum s3fwrn5_mode.
> Do you think the following structure is good?
>
> 0. remove the '#include "s3fwrn5.h" and the common function's
> definition in phy_common.h.
> 1. make phy_common.c that includes the common function's definition
> and "s3fwrn5.h , phy_common.h".
> 2. i2c.c includes "s3fwrn5.h , phy_common.h".
It looks like you already sent v2... I'll skip answering here then.
Best regards,
Krzysztof