[PATCH RFC net 0/2] nfc: nxp-nci: Fix i2c read on ThinkPad hardware

[PATCH RFC net 0/2] nfc: nxp-nci: Fix i2c read on ThinkPad hardware

[Marco Giorgi]

This patch addresses issues with "I2C" read on ThinkPad hardware.

My machine (ThinkPad T590) is equipped with an NXP1001 NFC reader, although
working flawlessly with Lenovo's Windows drivers, on Linux the driver
implementation doesn't work.

My speculation on the error is that the IRQ which is associated with the device
doesn't take into account the device's IRQ GPIO value. This patch addresses
that by exiting from the IRQ if the GPIO is low.

With this, I've been able to read a tag with neard's nfctool.

This is the behavior of the mainline `nxp_nci_i2c` driver:

# nfctool -d nfc0 -1 
Connection timed out
# dmesg -Wtd 
[<   0.000000>] nxp-nci_i2c i2c-NXP1001:00: NFC: Read failed with error -121 
[<   5.142581>] nci: __nci_request: wait_for_completion_interruptible_timeout failed 0 
[<   0.013474>] nxp-nci_i2c i2c-NXP1001:00: NFC: Read failed with error -121 

This is the patched `nxp_nci_i2c` driver:

# nfctool -d nfc0 -1 
nfc0: 
         Tags: [ ] 
         Devices: [ ] 
         Protocols: [ Felica MIFARE Jewel ISO-DEP NFC-DEP ] 
         Powered: Yes 
         RF Mode: None 
         lto: 0 
         rw: 0 
         miux: 0 
 
# nfctool -d nfc0 -p 
Start polling on nfc0 as initiator 
 
Targets found for nfc0 
 Tags: [ tag0 ] 
 Devices: [ ] 


No output from `dmesg`

Marco Giorgi (2):
  nfc: nxp-nci: Fix i2c read on ThinkPad hardware
  nfc: nxp-nci: Fix i2c read on ThinkPad hardware

 drivers/nfc/nxp-nci/i2c.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)


base-commit: 44f8baaf230c655c249467ca415b570deca8df77
-- 
2.41.0

[PATCH RFC net 1/2] nfc: nxp-nci: Fix i2c read on ThinkPad hardware

[Marco Giorgi]

Add the IRQ GPIO configuration.

Signed-off-by: Marco Giorgi <giorgi.marco.96@disroot.org>
---
 drivers/nfc/nxp-nci/i2c.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/nfc/nxp-nci/i2c.c b/drivers/nfc/nxp-nci/i2c.c
index d4c299be7949..4ba26a958258 100644
--- a/drivers/nfc/nxp-nci/i2c.c
+++ b/drivers/nfc/nxp-nci/i2c.c
@@ -35,6 +35,7 @@ struct nxp_nci_i2c_phy {
 
 	struct gpio_desc *gpiod_en;
 	struct gpio_desc *gpiod_fw;
+	struct gpio_desc *gpiod_irq;
 
 	int hard_fault; /*
 			 * < 0 if hardware error occurred (e.g. i2c err)
@@ -254,10 +255,12 @@ static irqreturn_t nxp_nci_i2c_irq_thread_fn(int irq, void *phy_id)
 	return IRQ_NONE;
 }
 
+static const struct acpi_gpio_params irq_gpios = { 0, 0, false };
 static const struct acpi_gpio_params firmware_gpios = { 1, 0, false };
 static const struct acpi_gpio_params enable_gpios = { 2, 0, false };
 
 static const struct acpi_gpio_mapping acpi_nxp_nci_gpios[] = {
+	{ "irq-gpios", &irq_gpios, 1 },
 	{ "enable-gpios", &enable_gpios, 1 },
 	{ "firmware-gpios", &firmware_gpios, 1 },
 	{ }
@@ -286,6 +289,12 @@ static int nxp_nci_i2c_probe(struct i2c_client *client)
 	if (r)
 		dev_dbg(dev, "Unable to add GPIO mapping table\n");
 
+	phy->gpiod_irq = devm_gpiod_get(dev, "irq", GPIOD_IN);
+	if (IS_ERR(phy->gpiod_irq)) {
+		nfc_err(dev, "Failed to get IRQ gpio\n");
+		return PTR_ERR(phy->gpiod_irq);
+	}
+
 	phy->gpiod_en = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
 	if (IS_ERR(phy->gpiod_en)) {
 		nfc_err(dev, "Failed to get EN gpio\n");
-- 
2.41.0

[PATCH RFC net 2/2] nfc: nxp-nci: Fix i2c read on ThinkPad hardware

[Marco Giorgi]

Read IRQ GPIO value and exit from IRQ if the device is not ready.

Signed-off-by: Marco Giorgi <giorgi.marco.96@disroot.org>
---
 drivers/nfc/nxp-nci/i2c.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/nfc/nxp-nci/i2c.c b/drivers/nfc/nxp-nci/i2c.c
index 4ba26a958258..d40a640c64d6 100644
--- a/drivers/nfc/nxp-nci/i2c.c
+++ b/drivers/nfc/nxp-nci/i2c.c
@@ -148,6 +148,16 @@ static int nxp_nci_i2c_nci_read(struct nxp_nci_i2c_phy *phy,
 	struct i2c_client *client = phy->i2c_dev;
 	int r;
 
+	r = gpiod_get_value(phy->gpiod_irq);
+	if (r < 0) {
+		nfc_err(&client->dev, "Error reading IRQ GPIO\n");
+		goto nci_read_exit;
+	}
+	if (r != 1) { /* Device is busy, go out */
+		r = -EBUSY;
+		goto nci_read_exit;
+	}
+
 	r = i2c_master_recv(client, (u8 *) &header, NCI_CTRL_HDR_SIZE);
 	if (r < 0) {
 		goto nci_read_exit;
@@ -226,6 +236,9 @@ static irqreturn_t nxp_nci_i2c_irq_thread_fn(int irq, void *phy_id)
 		break;
 	}
 
+	if (r == -EBUSY) {
+		goto exit_irq_handled;
+	}
 	if (r == -EREMOTEIO) {
 		phy->hard_fault = r;
 		if (info->mode == NXP_NCI_MODE_FW)
-- 
2.41.0

Re: [PATCH RFC net 1/2] nfc: nxp-nci: Fix i2c read on ThinkPad hardware

[Krzysztof Kozlowski]

On 07/06/2023 19:00, Marco Giorgi wrote:
> Add the IRQ GPIO configuration.

Why? Please include reasons in commit msg. What you are doing is quite
easy to see.

> 
> Signed-off-by: Marco Giorgi <giorgi.marco.96@disroot.org>
> ---
>  drivers/nfc/nxp-nci/i2c.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/nfc/nxp-nci/i2c.c b/drivers/nfc/nxp-nci/i2c.c
> index d4c299be7949..4ba26a958258 100644
> --- a/drivers/nfc/nxp-nci/i2c.c
> +++ b/drivers/nfc/nxp-nci/i2c.c
> @@ -35,6 +35,7 @@ struct nxp_nci_i2c_phy {
>  
>  	struct gpio_desc *gpiod_en;
>  	struct gpio_desc *gpiod_fw;
> +	struct gpio_desc *gpiod_irq;
>  
>  	int hard_fault; /*
>  			 * < 0 if hardware error occurred (e.g. i2c err)
> @@ -254,10 +255,12 @@ static irqreturn_t nxp_nci_i2c_irq_thread_fn(int irq, void *phy_id)
>  	return IRQ_NONE;
>  }
>  
> +static const struct acpi_gpio_params irq_gpios = { 0, 0, false };
>  static const struct acpi_gpio_params firmware_gpios = { 1, 0, false };
>  static const struct acpi_gpio_params enable_gpios = { 2, 0, false };
>  
>  static const struct acpi_gpio_mapping acpi_nxp_nci_gpios[] = {
> +	{ "irq-gpios", &irq_gpios, 1 },
>  	{ "enable-gpios", &enable_gpios, 1 },
>  	{ "firmware-gpios", &firmware_gpios, 1 },
>  	{ }
> @@ -286,6 +289,12 @@ static int nxp_nci_i2c_probe(struct i2c_client *client)
>  	if (r)
>  		dev_dbg(dev, "Unable to add GPIO mapping table\n");
>  
> +	phy->gpiod_irq = devm_gpiod_get(dev, "irq", GPIOD_IN);

Bindings do not allow it. Please update bindings... or not, because they
clearly state that interrupts are already there.

You need to explain what is this.



Best regards,
Krzysztof

Re: [PATCH RFC net 2/2] nfc: nxp-nci: Fix i2c read on ThinkPad hardware

[Krzysztof Kozlowski]

On 07/06/2023 19:00, Marco Giorgi wrote:
> Read IRQ GPIO value and exit from IRQ if the device is not ready.

Why? What problem are you solving?

Why IRQ GPIO - whatever it is - means device is not ready for I2C transfer?

> 
> Signed-off-by: Marco Giorgi <giorgi.marco.96@disroot.org>
> ---
>  drivers/nfc/nxp-nci/i2c.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)

Best regards,
Krzysztof

Re: [PATCH RFC net 1/2] nfc: nxp-nci: Fix i2c read on ThinkPad hardware

[Marco Giorgi]

Hi Krzysztof,

On Wed, 7 Jun 2023 19:45:25 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 07/06/2023 19:00, Marco Giorgi wrote:
> > Add the IRQ GPIO configuration.  
> 
> Why? Please include reasons in commit msg. What you are doing is quite
> easy to see.

This is my fault, I only put the patch reason in patch [0/2].

Basically, I found out that the mainline driver is not working on my
machine (Lenovo ThinkPad T590).

I suspect that the I2C read IRQ is somehow misconfigured, and it
triggers even when the NFC chip is not ready to be read, resulting in
an error.

In this patch [1/2], I'm adding the "IRQ" GPIO to the driver so its
value can be directly read from the IRQ thread.

In patch [2/2], I'm safely returning from the IRQ thread when the IRQ
GPIO is not active.

> 
> > 
> > Signed-off-by: Marco Giorgi <giorgi.marco.96@disroot.org>
> > ---
> >  drivers/nfc/nxp-nci/i2c.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/nfc/nxp-nci/i2c.c b/drivers/nfc/nxp-nci/i2c.c
> > index d4c299be7949..4ba26a958258 100644
> > --- a/drivers/nfc/nxp-nci/i2c.c
> > +++ b/drivers/nfc/nxp-nci/i2c.c
> > @@ -35,6 +35,7 @@ struct nxp_nci_i2c_phy {
> >  
> >  	struct gpio_desc *gpiod_en;
> >  	struct gpio_desc *gpiod_fw;
> > +	struct gpio_desc *gpiod_irq;
> >  
> >  	int hard_fault; /*
> >  			 * < 0 if hardware error occurred (e.g.
> > i2c err) @@ -254,10 +255,12 @@ static irqreturn_t
> > nxp_nci_i2c_irq_thread_fn(int irq, void *phy_id) return IRQ_NONE;
> >  }
> >  
> > +static const struct acpi_gpio_params irq_gpios = { 0, 0, false };
> >  static const struct acpi_gpio_params firmware_gpios = { 1, 0,
> > false }; static const struct acpi_gpio_params enable_gpios = { 2,
> > 0, false }; 
> >  static const struct acpi_gpio_mapping acpi_nxp_nci_gpios[] = {
> > +	{ "irq-gpios", &irq_gpios, 1 },
> >  	{ "enable-gpios", &enable_gpios, 1 },
> >  	{ "firmware-gpios", &firmware_gpios, 1 },
> >  	{ }
> > @@ -286,6 +289,12 @@ static int nxp_nci_i2c_probe(struct i2c_client
> > *client) if (r)
> >  		dev_dbg(dev, "Unable to add GPIO mapping table\n");
> >  
> > +	phy->gpiod_irq = devm_gpiod_get(dev, "irq", GPIOD_IN);  
> 
> Bindings do not allow it. Please update bindings... or not, because
> they clearly state that interrupts are already there.
> 
> You need to explain what is this.

Thanks, I will address bindings in future patch versions.

> 
> 
> 
> Best regards,
> Krzysztof
> 

Thanks for your feedback,
Marco.

Re: [PATCH RFC net 2/2] nfc: nxp-nci: Fix i2c read on ThinkPad hardware

[Marco Giorgi]

Hi Krzysztof,

On Wed, 7 Jun 2023 19:46:35 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 07/06/2023 19:00, Marco Giorgi wrote:
> > Read IRQ GPIO value and exit from IRQ if the device is not ready.
> 
> Why? What problem are you solving?
> 
> Why IRQ GPIO - whatever it is - means device is not ready for I2C
> transfer?

It seems that the I2C IRQ gets triggered also when the IRQ GPIO is not
active (low value).

This patch queries the IRQ GPIO before issuing the I2C read.
If the GPIO is low, the IRQ returns with IRQ_HANDLED.

I don't know why the IRQ is triggered even when the GPIO is low, I'm
still looking into it.

I've submitted this patch in the hope to get feedback from other people
to understand what's going on.

> 
> > 
> > Signed-off-by: Marco Giorgi <giorgi.marco.96@disroot.org>
> > ---
> >  drivers/nfc/nxp-nci/i2c.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> 
> Best regards,
> Krzysztof
> 

Thanks,
Marco.

Re: [PATCH RFC net 1/2] nfc: nxp-nci: Fix i2c read on ThinkPad hardware

[Krzysztof Kozlowski]

On 11/06/2023 18:25, Marco Giorgi wrote:
> Hi Krzysztof,
> 
> On Wed, 7 Jun 2023 19:45:25 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 07/06/2023 19:00, Marco Giorgi wrote:
>>> Add the IRQ GPIO configuration.  
>>
>> Why? Please include reasons in commit msg. What you are doing is quite
>> easy to see.
> 
> This is my fault, I only put the patch reason in patch [0/2].
> 
> Basically, I found out that the mainline driver is not working on my
> machine (Lenovo ThinkPad T590).
> 
> I suspect that the I2C read IRQ is somehow misconfigured, and it
> triggers even when the NFC chip is not ready to be read, resulting in
> an error.

Isn't this then a problem of your I2C controller?

> 
> In this patch [1/2], I'm adding the "IRQ" GPIO to the driver so its
> value can be directly read from the IRQ thread.

What is IRQ GPIO? If this is interrupt line, you are not handling it
correctly. This is quite surprising code.

> 
> In patch [2/2], I'm safely returning from the IRQ thread when the IRQ
> GPIO is not active.
> 


Best regards,
Krzysztof