Re: [PATCH net-next 1/2] dt-bindings: net: nfc: s3fwrn5: Support a UART interface
On Mon, Nov 23, 2020 at 04:55:26PM +0900, Bongsu Jeon wrote:
> Since S3FWRN82 NFC Chip, The UART interface can be used.
> S3FWRN82 supports I2C and UART interface.
>
> Signed-off-by: Bongsu Jeon
> ---
> .../bindings/net/nfc/samsung,s3fwrn5.yaml | 28 +--
> 1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/nfc/samsung,s3fwrn5.yaml
> b/Documentation/devicetree/bindings/net/nfc/samsung,s3fwrn5.yaml
> index cb0b8a560282..37b3e5ae5681 100644
> --- a/Documentation/devicetree/bindings/net/nfc/samsung,s3fwrn5.yaml
> +++ b/Documentation/devicetree/bindings/net/nfc/samsung,s3fwrn5.yaml
> @@ -13,6 +13,7 @@ maintainers:
> properties:
>compatible:
> const: samsung,s3fwrn5-i2c
> +const: samsung,s3fwrn82-uart
This does not work, you need to use enum. Did you run at least
dt_bindings_check?
The compatible should be just "samsung,s3fwrn82". I think it was a
mistake in the first s3fwrn5 submission to add a interface to
compatible.
>
>en-gpios:
> maxItems: 1
> @@ -47,10 +48,19 @@ additionalProperties: false
> required:
>- compatible
>- en-gpios
> - - interrupts
> - - reg
>- wake-gpios
>
> +allOf:
> + - if:
> + properties:
> +compatible:
> + contains:
> +const: samsung,s3fwrn5-i2c
> +then:
> + required:
> +- interrupts
> +- reg
> +
> examples:
>- |
> #include
> @@ -71,3 +81,17 @@ examples:
> wake-gpios = <&gpj0 2 GPIO_ACTIVE_HIGH>;
> };
> };
> + # UART example on Raspberry Pi
> + - |
> +&uart0 {
> +status = "okay";
> +
> +s3fwrn82_uart {
Just "bluetooth" to follow Devicetree specification.
Best regards,
Krzysztof
Re: [PATCH net-next 2/2] net: nfc: s3fwrn5: Support a UART interface
On Mon, Nov 23, 2020 at 04:56:58PM +0900, Bongsu Jeon wrote:
> Since S3FWRN82 NFC Chip, The UART interface can be used.
> S3FWRN82 uses NCI protocol and supports I2C and UART interface.
>
> Signed-off-by: Bongsu Jeon
Please start sending emails properly, e.g. with git send-email, so all
your patches in the patchset are referencing the first patch.
> ---
> drivers/nfc/s3fwrn5/Kconfig | 12 ++
> drivers/nfc/s3fwrn5/Makefile | 2 +
> drivers/nfc/s3fwrn5/uart.c | 250 +++
> 3 files changed, 264 insertions(+)
> create mode 100644 drivers/nfc/s3fwrn5/uart.c
>
> diff --git a/drivers/nfc/s3fwrn5/Kconfig b/drivers/nfc/s3fwrn5/Kconfig
> index 3f8b6da58280..6f88737769e1 100644
> --- a/drivers/nfc/s3fwrn5/Kconfig
> +++ b/drivers/nfc/s3fwrn5/Kconfig
> @@ -20,3 +20,15 @@ config NFC_S3FWRN5_I2C
> To compile this driver as a module, choose m here. The module will
> be called s3fwrn5_i2c.ko.
> Say N if unsure.
> +
> +config NFC_S3FWRN82_UART
> + tristate "Samsung S3FWRN82 UART support"
> + depends on NFC_NCI && SERIAL_DEV_BUS
What about SERIAL_DEV_BUS as module? Shouldn't this be
SERIAL_DEV_BUS || !SERIAL_DEV_BUS?
> + select NFC_S3FWRN5
> + help
> + This module adds support for a UART interface to the S3FWRN82 chip.
> + Select this if your platform is using the UART bus.
> +
> + To compile this driver as a module, choose m here. The module will
> + be called s3fwrn82_uart.ko.
> + Say N if unsure.
> diff --git a/drivers/nfc/s3fwrn5/Makefile b/drivers/nfc/s3fwrn5/Makefile
> index d0ffa35f50e8..d1902102060b 100644
> --- a/drivers/nfc/s3fwrn5/Makefile
> +++ b/drivers/nfc/s3fwrn5/Makefile
> @@ -5,6 +5,8 @@
>
> s3fwrn5-objs = core.o firmware.o nci.o
> s3fwrn5_i2c-objs = i2c.o
> +s3fwrn82_uart-objs = uart.o
>
> obj-$(CONFIG_NFC_S3FWRN5) += s3fwrn5.o
> obj-$(CONFIG_NFC_S3FWRN5_I2C) += s3fwrn5_i2c.o
> +obj-$(CONFIG_NFC_S3FWRN82_UART) += s3fwrn82_uart.o
> diff --git a/drivers/nfc/s3fwrn5/uart.c b/drivers/nfc/s3fwrn5/uart.c
> new file mode 100644
> index ..b3c36a5b28d3
> --- /dev/null
> +++ b/drivers/nfc/s3fwrn5/uart.c
> @@ -0,0 +1,250 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * UART Link Layer for S3FWRN82 NCI based Driver
> + *
> + * Copyright (C) 2020 Samsung Electronics
> + * Author: Bongsu Jeon
You copied a lot from existing i2c.c. Please keep also the original
copyrights.
> + * All rights reserved.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "s3fwrn5.h"
> +
> +#define S3FWRN82_UART_DRIVER_NAME "s3fwrn82_uart"
Remove the define, it is used only once.
> +#define S3FWRN82_NCI_HEADER 3
> +#define S3FWRN82_NCI_IDX 2
> +#define S3FWRN82_EN_WAIT_TIME 20
> +#define NCI_SKB_BUFF_LEN 258
> +
> +struct s3fwrn82_uart_phy {
> + struct serdev_device *ser_dev;
> + struct nci_dev *ndev;
> + struct sk_buff *recv_skb;
> +
> + unsigned int gpio_en;
> + unsigned int gpio_fw_wake;
> +
> + /* mutex is used to synchronize */
Please do not write obvious comments. Mutex is always used to
synchronize, what else is it for? Instead you must describe what exactly
is protected with mutex.
> + struct mutex mutex;
> + enum s3fwrn5_mode mode;
> +};
> +
> +static void s3fwrn82_uart_set_wake(void *phy_id, bool wake)
> +{
> + struct s3fwrn82_uart_phy *phy = phy_id;
> +
> + mutex_lock(&phy->mutex);
> + gpio_set_value(phy->gpio_fw_wake, wake);
> + msleep(S3FWRN82_EN_WAIT_TIME);
> + mutex_unlock(&phy->mutex);
> +}
> +
> +static void s3fwrn82_uart_set_mode(void *phy_id, enum s3fwrn5_mode mode)
> +{
> + struct s3fwrn82_uart_phy *phy = phy_id;
> +
> + mutex_lock(&phy->mutex);
> + if (phy->mode == mode)
> + 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(S3FWRN82_EN_WAIT_TIME);
> + gpio_set_value(phy->gpio_en, 0);
> + msleep(S3FWRN82_EN_WAIT_TIME);
> + }
> +out:
> + mutex_unlock(&phy->mutex);
> +}
> +
> +static enum s3fwrn5_mode s3fwrn82_uart_get_mode(void *phy_id)
> +{
> + struct s3fwrn82_uart_phy *phy = phy_id;
> + enum s3fwrn5_mode mode;
> +
> + mutex_lock(&phy->mutex);
> + mode = phy->mode;
> + mutex_unlock(&phy->mutex);
> + return mode;
> +}
All this duplicates I2C version. You need to start either reusing common
blocks.
> +
> +static int s3fwrn82_uart_write(void *phy_id, struct sk_buff *out)
> +{
> + struct s3fwrn82_uart_phy *phy = phy_id;
> + int err;
> +
> + err = serdev_device_write(phy->ser_dev,
> + out->data, out->len,
> + MAX_SCHEDULE_TIMEOUT);
> + if (err < 0)
> +
Re: [PATCH net-next 1/2] dt-bindings: net: nfc: s3fwrn5: Support a UART interface
On Tue, Nov 24, 2020 at 08:39:40PM +0900, Bongsu Jeon wrote:
> On Mon, Nov 23, 2020 at 5:02 PM k...@kernel.org wrote:
> >
> > On Mon, Nov 23, 2020 at 04:55:26PM +0900, Bongsu Jeon wrote:
> > examples:
> > >- |
> > > #include
> > > @@ -71,3 +81,17 @@ examples:
> > > wake-gpios = <&gpj0 2 GPIO_ACTIVE_HIGH>;
> > > };
> > > };
> > > + # UART example on Raspberry Pi
> > > + - |
> > > +&uart0 {
> > > +status = "okay";
> > > +
> > > +s3fwrn82_uart {
> >
> > Just "bluetooth" to follow Devicetree specification.
> Sorry. I don't understand this comment.
> Could you explain it?
> Does it mean i need to refer to the net/broadcom-bluetooth.txt?
The node name should be "bluetooth", not "s3fwrn82_uart", because of
Devicetree naming convention - node names should represent generic class
of a device.
Best regards,
Krzysztof
Re: [PATCH net-next 2/2] net: nfc: s3fwrn5: Support a UART interface
On Tue, Nov 24, 2020 at 09:05:52PM +0900, Bongsu Jeon wrote:
> On Mon, Nov 23, 2020 at 5:55 PM k...@kernel.org wrote:
> > > +static enum s3fwrn5_mode s3fwrn82_uart_get_mode(void *phy_id)
> > > +{
> > > + struct s3fwrn82_uart_phy *phy = phy_id;
> > > + enum s3fwrn5_mode mode;
> > > +
> > > + mutex_lock(&phy->mutex);
> > > + mode = phy->mode;
> > > + mutex_unlock(&phy->mutex);
> > > + return mode;
> > > +}
> >
> > All this duplicates I2C version. You need to start either reusing common
> > blocks.
> >
>
> Okay. I will do refactoring on i2c.c and uart.c to make common blocks.
> is it okay to separate a patch for it?
Yes, that would be the best - refactor the driver to split some common
methods and then in next patch add new s3fwrn82 UART driver.
> > > +
> > > +static int s3fwrn82_uart_write(void *phy_id, struct sk_buff *out)
> > > +{
> > > + struct s3fwrn82_uart_phy *phy = phy_id;
> > > + int err;
> > > +
> > > + err = serdev_device_write(phy->ser_dev,
> > > + out->data, out->len,
> > > + MAX_SCHEDULE_TIMEOUT);
> > > + if (err < 0)
> > > + return err;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct s3fwrn5_phy_ops uart_phy_ops = {
> > > + .set_wake = s3fwrn82_uart_set_wake,
> > > + .set_mode = s3fwrn82_uart_set_mode,
> > > + .get_mode = s3fwrn82_uart_get_mode,
> > > + .write = s3fwrn82_uart_write,
> > > +};
> > > +
> > > +static int s3fwrn82_uart_read(struct serdev_device *serdev,
> > > + const unsigned char *data,
> > > + size_t count)
> > > +{
> > > + struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
> > > + size_t i;
> > > +
> > > + for (i = 0; i < count; i++) {
> > > + skb_put_u8(phy->recv_skb, *data++);
> > > +
> > > + if (phy->recv_skb->len < S3FWRN82_NCI_HEADER)
> > > + continue;
> > > +
> > > + if ((phy->recv_skb->len - S3FWRN82_NCI_HEADER)
> > > + < phy->recv_skb->data[S3FWRN82_NCI_IDX])
> > > + continue;
> > > +
> > > + s3fwrn5_recv_frame(phy->ndev, phy->recv_skb, phy->mode);
> > > + phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL);
> > > + if (!phy->recv_skb)
> > > + return 0;
> > > + }
> > > +
> > > + return i;
> > > +}
> > > +
> > > +static struct serdev_device_ops s3fwrn82_serdev_ops = {
> >
> > const
> >
> > > + .receive_buf = s3fwrn82_uart_read,
> > > + .write_wakeup = serdev_device_write_wakeup,
> > > +};
> > > +
> > > +static const struct of_device_id s3fwrn82_uart_of_match[] = {
> > > + { .compatible = "samsung,s3fwrn82-uart", },
> > > + {},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, s3fwrn82_uart_of_match);
> > > +
> > > +static int s3fwrn82_uart_parse_dt(struct serdev_device *serdev)
> > > +{
> > > + struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev);
> > > + struct device_node *np = serdev->dev.of_node;
> > > +
> > > + if (!np)
> > > + return -ENODEV;
> > > +
> > > + phy->gpio_en = of_get_named_gpio(np, "en-gpios", 0);
> > > + if (!gpio_is_valid(phy->gpio_en))
> > > + return -ENODEV;
> > > +
> > > + phy->gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
> >
> > You should not cast it it unsigned int. I'll fix the s3fwrn5 from which
> > you copied this apparently.
> >
>
> Okay. I will fix it.
>
> > > + if (!gpio_is_valid(phy->gpio_fw_wake))
> > > + return -ENODEV;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int s3fwrn82_uart_probe(struct serdev_device *serdev)
> > > +{
> > > + struct s3fwrn82_uart_phy *phy;
> > > + int ret = -ENOMEM;
> > > +
> > > + phy = devm_kzalloc(&serdev->dev, sizeof(*phy), GFP_KERNEL)
Re: [PATCH net-next 1/3] nfc: s3fwrn5: Remove the max_payload.
On Sat, Nov 14, 2020 at 09:17:36AM +0900, Bongsu Jeon wrote: > > max_payload is unused. Thanks for the patch. You have an empty line at beginning of commit msg - please format the commit description correctly. Remove alo the trailing dot from subject. Best regards, Krzysztof > > Signed-off-by: Bongsu Jeon > --- > drivers/nfc/s3fwrn5/core.c| 3 +-- > drivers/nfc/s3fwrn5/i2c.c | 4 +--- > drivers/nfc/s3fwrn5/s3fwrn5.h | 3 +-- > 3 files changed, 3 insertions(+), 7 deletions(-)
Re: [PATCH net-next 2/3] nfc: s3fwrn5: Fix the misspelling in a comment
On Sat, Nov 14, 2020 at 09:19:20AM +0900, Bongsu Jeon wrote: > Empty commit messsage cannot be accepted. Best regards, Krzysztof
Re: [PATCH net-next 3/3] nfc: s3fwrn5: Change the error code.
On Sat, Nov 14, 2020 at 09:21:34AM +0900, Bongsu Jeon wrote: > > ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP. The same as in patch #1 - trailing dot a subject end empty line at beginning of commit. Please CC all required maintainers and lists when submitting patches. You skipped here LKML and the most important - NFC mailing list. Use the scripts/get_maintainer.pl to get the necessary addresses. Best regards, Krzysztof
Re: [PATCH net-next 1/3] nfc: s3fwrn5: Remove the max_payload
On Mon, Nov 16, 2020 at 10:12:05AM +0900, Bongsu Jeon wrote: > max_payload is unused. > > Signed-off-by: Bongsu Jeon Please version your patches (this should be a v2) and describe changes between versions in changelog, either in cover letter or after --- separator. Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH net-next 2/3] nfc: s3fwrn5: Fix the misspelling in a comment
On Mon, Nov 16, 2020 at 10:17:55AM +0900, Bongsu Jeon wrote: > stucture should be replaced by structure. > > Signed-off-by: Bongsu Jeon > --- > drivers/nfc/s3fwrn5/firmware.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH net-next 3/3] nfc: s3fwrn5: Change the error code
On Mon, Nov 16, 2020 at 10:19:50AM +0900, Bongsu Jeon wrote: > ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP. > > Signed-off-by: Bongsu Jeon > --- > drivers/nfc/s3fwrn5/s3fwrn5.h | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH net-next v2 2/3] nfc: s3fwrn5: Fix the misspelling in a comment
On Tue, Nov 17, 2020 at 10:17:42AM +0900, Bongsu Jeon wrote: > stucture should be replaced by structure. > > Signed-off-by: Bongsu Jeon I already reviewed it. Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH net-next v2 3/3] nfc: s3fwrn5: Change the error code
On Tue, Nov 17, 2020 at 10:18:50AM +0900, Bongsu Jeon wrote: > ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP. > > Signed-off-by: Bongsu Jeon > --- > drivers/nfc/s3fwrn5/s3fwrn5.h | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) I already reviewed it. Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH net-next v2 1/3] nfc: s3fwrn5: Remove the max_payload
On Tue, Nov 17, 2020 at 10:16:11AM +0900, Bongsu Jeon wrote: > max_payload is unused. Why did you resend the patch ignoring my review? I already provided you with a tag, so you should include it. https://www.kernel.org/doc/html/latest/process/submitting-patches.html Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
