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

[Marco Giorgi <giorgi.marco.96@disroot.org>]

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.