Message-ID: <57307BB7.8080902@schinagl.nl>
Date:	Mon, 9 May 2016 13:59:51 +0200
From:	Olliver Schinagl <oliver@...inagl.nl>
To:	Michael Thalmeier <michael.thalmeier@...e.at>,
	Samuel Ortiz <sameo@...ux.intel.com>
Cc:	Lauro Ramos Venancio <lauro.venancio@...nbossa.org>,
	Aloisio Almeida Jr <aloisio.almeida@...nbossa.org>,
	linux-kernel@...r.kernel.org, linux-nfc@...ts.01.org,
	michael@...lmeier.at
Subject: Re: [PATCH 00/11] NFC: pn533: bug fixes and improvements

Michael,

On 04-05-16 17:18, Olliver Schinagl wrote:
> Hi Michael,
>
> On 28-04-16 20:18, Olliver Schinagl wrote:
>> Hi Michael,
>>
>> I know i'm a little late to the party, but why did you not name the
>> directory/driver pn53x? I don't think we should debate wether a 533 is a
>> 532 with a USB added, or wether a 532 is a 533 with the USB stripped, I
>> was just curious. As for the pn533-i2c, in the KConfig, I'd probably
>> recommend naming it pn532 at the least though, since there is no variant
>> other then USB for the 533?
>>
>> Further more, I think it would be wise to add some documentation and nfc
>> bindings examples.
>>
>> I'm using it as a subnode of an i2c node as such:
>>
>> &i2c2 {
>>      pinctrl-names = "default";
>>      pinctrl-0 = <&i2c2_pins_a>;
>>      status = "okay";
>>
>>      pn532: pn53x@48 {
>>          compatible = "nxp,pn532-i2c";
>>          reg = <0x48>;
>>      };
>> };
>>
>> But not sure if this is correct at all (I haven't gotten it to work yet,
>> partly due to a burnt level shifter). For example, I'm not sure what
>> other properties are needed.
> I think that this is all fine, it should be 0x24 instead of 0x48 if it
> matters at all (hardcoded in the driver?) but my initial problem was
> that i was loading pn533 and not pn533_i2c.
>
> The driver seems to like to use an interrupt (one of the optional pins
> on the chip (nice for the documentation :)) other then the i2c interrupt
> I assume?
>
> After the warning that we do not have an interrupt, we call
> pn533_register, which then just sits there waiting forever. I'll add
> more printk's into the pn533 driver to find out why, but if you have an
> idea and can save me some work, that'd be great!
I've added some printk's and have found out where it is waiting, but 
don't quite understand the why.

The i2c probe stuff gets properly executed until the register, the register
pn533_get_firmware_version calls pn533_send_cmd_sync to retrieve the 
firmware version of the 532.

It does that by calling waiting for pn533_send_cmd_async via 
wait_for_completion(&arg.done), Unfortunatly however, 
wait_for_completion, well just waits forever.

Looking into the wait_for_completion code, I only see:

          might_sleep();

          spin_lock_irq(&x->wait.lock);
          timeout = do_wait_for_common(x, action, timeout, state);
          spin_unlock_irq(&x->wait.lock);
          return timeout;

but i'm I suppose those irq's have nothing to do with the hardware 
interrupt the i2c driver wants/needs?

dingging into 'ops->send_frame', I do see data going over the line on 
the scope, and while I did not analyze the exact bytes on the scope, I 
think it's quite certain to say it's the firmware request bytes/answers.

So the request does go over the line happily.

But this is where I get stuck now.

Find below my printk mess, I did not use __FUNCTION__ and __LINE__ 
concistently at all.

[   22.087495] Probing pn533!
[   22.090360] pn533_i2c 2-0024: pn533_i2c_probe
[   22.094728] pn533_i2c 2-0024: IRQ: 0
[   22.098309] requested irq 0
[   22.101145] registering device
[   22.104208] pn reg start
[   22.106747] pre mutex
[   22.109021] post mutex
[   22.111422] INIT_DONE
[   22.113736] Timer init
[   22.116101] timer init done
[   22.118895] skb_queue
[   22.121205] skb_queue done
[   22.123919] INIT_LIST
[   22.126193] pn533_get_firmware_version, start
[   22.130588] DEBUG: pn533_get_firmware_version pn533_alloc_skb (2393)
[   22.137069] DEBUG: pn533_get_firmware_version pn533_alloc_skb done 
(2397)
[   22.143994] DEBUG: pn533_send_cmd_sync start (607)
[   22.148875] DEBUG: pn533_send_cmd_sync done init_completion (609)
[   22.155099] __pn533_send_async DEBUG, sending command 0x2 (422)
[   22.161059] __pn533_send_async DEBUG, build frame (433)
[   22.166402] __pn533_send_async DEBUG, build frame done (435)
[   22.172132] __pn533_send_async DEBUG, cmd not pending (440)
[   22.177721] pn533_i2c_send_frame DEBUG, no fault yet 74
[   22.183024] pn533_i2c_send_frame DEBUG, sending master 78
[   22.189430] pn533_i2c_send_frame DEBUG, master sent 80
[   22.194631] pn533_i2c_send_frame DEBUG, done 94
[   22.199172] __pn533_send_async DEBUG, frame sent (442)
[   22.204347] DEBUG: pn533_send_cmd_sync pn533_send_cmd_async done (613)
[   22.210985] DEBUG: pn533_send_cmd_sync going to wait for completion 
(619)

What else is needed to get this going?

Olliver
>
> Olliver
>
>>
>> Olliver
>>
>> On 21-04-16 16:43, Michael Thalmeier wrote:
>>> Hello Samuel,
>>>
>>> This patchset fixes some major bugs in the pn533 drivers (usb and i2c)
>>> and
>>> improves performance of the PN532 chip by increasing its clock speed.
>>>
>>> Best Regards
>>> Michael
>>>
>>> Michael Thalmeier (11):
>>>    NFC: pn533: i2c: free irq on driver remove
>>>    NFC: pn533: fix order of initialization
>>>    NFC: pn533: i2c: do not call pn533_recv_frame with aborted commands
>>>    NFC: pn533: reset poll modulation list before calling
>>> nfc_targets_found
>>>    NFC: pn533: handle interrupted commands in pn533_recv_frame
>>>    NFC: pn533: usb: fix errors when poll is stopped
>>>    NFC: pn533: improve cmd queue handling
>>>    NFC: pn533: reduce output when stopping poll
>>>    NFC: pn533: use nfc_alloc_recv_skb for skb allocation
>>>    NFC: pn533: set cmd status when not set
>>>    nfc: pn533: increase clock frequency for PN532
>>>
>>>   drivers/nfc/pn533/i2c.c   |  20 ++++--
>>>   drivers/nfc/pn533/pn533.c | 154
>>> +++++++++++++++++++++++++++++++++++++---------
>>>   drivers/nfc/pn533/pn533.h |   5 +-
>>>   drivers/nfc/pn533/usb.c   |  15 +++--
>>>   4 files changed, 153 insertions(+), 41 deletions(-)
>>>
>>
>>
>
>

Powered by blists - more mailing lists