Re: [PATCH v3 2/3] NFC: trf7970a: Add device tree option of 1.8 Volt IO voltage

[Mark Greer]

On Wed, Dec 21, 2016 at 11:18:33PM -0500, Geoff Lansberry wrote:
> The TRF7970A has configuration options for supporting hardware designs
> with 1.8 Volt or 3.3 Volt IO.   This commit adds a device tree option,
> using a fixed regulator binding, for setting the io voltage to match
> the hardware configuration. If no option is supplied it defaults to
> 3.3 volt configuration.
> 
> Signed-off-by: Geoff Lansberry <ge...@kuvee.com>
> ---
>  .../devicetree/bindings/net/nfc/trf7970a.txt   |  2 ++
>  drivers/nfc/trf7970a.c | 26 
> +-

Acked-by: Mark Greer <mgr...@animalcreek.com>

Re: [PATCH v3 1/3] NFC: trf7970a: add device tree option for 27MHz clock

[Mark Greer]

On Wed, Dec 21, 2016 at 11:18:32PM -0500, Geoff Lansberry wrote:
> The TRF7970A has configuration options to support hardware designs
> which use a 27.12MHz clock. This commit adds a device tree option
> 'clock-frequency' to support configuring the this chip for default
> 13.56MHz clock or the optional 27.12MHz clock.
> 
> Signed-off-by: Geoff Lansberry <ge...@kuvee.com>
> ---

Acked-by: Mark Greer <mgr...@animalcreek.com>

Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel

[Mark Greer]

On Tue, Jan 03, 2017 at 01:35:18PM -0500, Geoff Lansberry wrote:
> On Tue, Jan 3, 2017 at 11:33 AM, Mark Greer <mgr...@animalcreek.com> wrote:
> > On Tue, Dec 27, 2016 at 09:18:32AM -0500, Geoff Lansberry wrote:

> >> In the meantime - here is some more info about how we use it.
> >>
> >> We do use NFC structures.I did find an interesting clue in that
> >> there are certain bottles that cause neard to segfault,  I'm not sure
> >> what is different about them.  We write a string, like
> >> "coppola_chardonnay_2015" to the bottles.
> >
> > Off the top of my head, it could be the length of the text.
> > It would be useful to compare the data that works to the data
> > that doesn't work.  Can you install NXP's 'TagInfo' app on a
> > smartphone and scan tags with working & non-working data?
> > You can email the data from the app to yourself, edit out
> > the cruft, and share here.
> 
> The data is always the same - and the tags are all the same.  Only
> difference is that the tag is physically different, and perhaps
> orientation; distance from antenna to tag is fixed.

Interesting...  They're all type 2 tags, right?

> I can't even
> write the tags at all, so reading them will show blank.   Also a minor
> but significant detail, is that the tags are embedded in such a way
> that the phone cannot get close enough to them to connect.

This section had me completely confused for a couple minutes until I realized
that you mean that you can read & write the tags using the trf7970a with
an attached antenna but not with your phone.  Is that correct?

If so, try a tag that isn't embedded in something else and move it around
the back of the phone.  Try to find where it works best.  The phone
manufacturers are notorius for paying little attention to the NFC antenna
they put on their products.  For example, I have a Samsung S5 next to me
and it seems to work best around the center of the phone.  I've used others
where I had to use the upper-left or upper-right corner of the phone.

Mark
--

Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel

[Mark Greer]

On Sat, Dec 24, 2016 at 11:17:18AM -0500, Geoff Lansberry wrote:
> Mark - I'm sorry, but I did not write this code, and therefore was not
> able to accurately describe it.   It is fixing a different issue, not
> the neard segfault that we are still chasing. Last week Jaret Cantu
> sent a separate email explaining the purpose of the code, which had
> you copied, did you see that?

Hm, no, I didn't.  I received an email from Justin Bronder but not from
Jaret Cantu.  Justin's email did help but is still pretty high-level.
We need a clear understanding as to what is happening in the digital
layer and the driver to know how execution is getting into a block of
error handling code that should never be executed.  Once we understand
that we can start thinking about what the best fix is.

> Does it explain why it was done to
> your satisfaction?   I've asked him to join in on the effort to push
> the change upstream, however he will not be available until the new
> year.

I expect that it would help if he joins.  After the holidays is fine -
I think many people are taking it easy for the next week or so, anyway.

> I know you did suggest that we split off that change from the others,
> and if now is the time to do that, let me know.   If you don't have
> the email from Jaret, also please let me know and I will forward it to
> you.

I think it would help you if you split it off because the first two patches
have a good chance of being accepted but this one doesn't (yet).  If you
separate the them, it will make it easier for Samuel to take the first two
(or he may take the first two anyway but its always good to make it as
easy maintainers as you can).

Mark
--

Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel

[Mark Greer]

On Wed, Dec 21, 2016 at 11:18:34PM -0500, Geoff Lansberry wrote:
> From: Jaret Cantu 
> 
> Repeated polling attempts cause a NULL dereference error to occur.
> This is because the state of the trf7970a is currently reading but
> another request has been made to send a command before it has finished.
> 
> The solution is to properly kill the waiting reading (workqueue)
> before failing on the send.
> 
> Signed-off-by: Geoff Lansberry 
> ---

You've still provided virtually no information on the actual problem(s)
nor justified why you think this is the best solution.  You're adding
code to a section of code that should _never_ be executed so the only
reasonable things I can infer is that there are, at least, two problems:

1) There is a bug causing execution to get into this block of code.

2) Once in this block of code, there is another bug.

You seem to be attempting to fix 2) and completely ignoring 1).
1) is the first bug that needs to be root-caused and fixed.

Also, what exactly is the "NULL dereference error" you mention?
Is this the neard crash you talked about in another thread or is
this a kernel crash?  If it is the kernel crash, please post the
relevant information.  If this is the neard crash - which seems
unlikely - then how can changing a section of kernel code that
shouldn't be executed in the first place fix that?

Mark
--

Re: [PATCH 2/3] NFC: trf7970a: Add device tree option of 1.8 Volt IO voltage

[Mark Greer]

On Wed, Dec 21, 2016 at 06:47:36AM -0500, Geoff Lansberry wrote:
> Thanks Mark.   Should I resubmit patches with the requested edits today, or 
> wait a bit for more comments?  What is the desired etiquette?

Its up to you.  I don't think there are any hard & fast rules on this.

If it were me, I would likely spin a new version today because there are
several responses already and it lets people review them at their leisure
over the holidays.

Just a thought - you may want to consider separating the third patch from
the other two.  The problems the first two solve are well understood and
have reasonable solutions (I believe).  The third one - for me, at least -
tries to fix a problem that is not well understood yet.

Mark
--

Re: [PATCH 2/3] NFC: trf7970a: Add device tree option of 1.8 Volt IO voltage

[Mark Greer]

On Tue, Dec 20, 2016 at 11:16:31AM -0500, Geoff Lansberry wrote:
> From: Geoff Lansberry 
> 
> The TRF7970A has configuration options for supporting hardware designs
> with 1.8 Volt or 3.3 Volt IO.   This commit adds a device tree option,
> using a fixed regulator binding, for setting the io voltage to match
> the hardware configuration. If no option is supplied it defaults to
> 3.3 volt configuration.

Sign-off ??  Same comment for you other patches.



Okay I see you have it at the end of the patch.  It should be here.
'git commit -s' is your friend.

> ---
>  .../devicetree/bindings/net/nfc/trf7970a.txt   |  4 ++--
>  drivers/nfc/trf7970a.c | 28 
> +-
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt 
> b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
> index e262ac1..b5777d8 100644
> --- a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
> +++ b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
> @@ -21,9 +21,9 @@ Optional SoC Specific Properties:
>  - t5t-rmb-extra-byte-quirk: Specify that the trf7970a has the erratum
>where an extra byte is returned by Read Multiple Block commands issued
>to Type 5 tags.
> +- vdd-io-supply: Regulator specifying voltage for vdd-io
>  - clock-frequency: Set to specify that the input frequency to the trf7970a 
> is 1356Hz or 2712Hz
>  
> -
>  Example (for ARM-based BeagleBone with TRF7970A on SPI1):
>  
>   {
> @@ -41,11 +41,11 @@ Example (for ARM-based BeagleBone with TRF7970A on SPI1):
> < 5 GPIO_ACTIVE_LOW>;
>   vin-supply = <_reg>;
>   vin-voltage-override = <500>;
> + vdd-io-supply = <_reg>;
>   autosuspend-delay = <3>;
>   irq-status-read-quirk;
>   en2-rf-quirk;
>   t5t-rmb-extra-byte-quirk;
> - vdd_io_1v8;

It was already mentioned but this shouldn't have been added in the
previous patch so it shouldn't be here now.

>   clock-frequency = <2712>;
>   status = "okay";
>   };
> diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
> index 4e051e9..8a88195 100644
> --- a/drivers/nfc/trf7970a.c
> +++ b/drivers/nfc/trf7970a.c

> @@ -2062,6 +2068,7 @@ static int trf7970a_probe(struct spi_device *spi)
>   return ret;
>   }
>  
> +

Please don't add an extra blank line.

>   of_property_read_u32(np, "clock-frequency", _freq);
>   if ((clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY) ||
>   (clk_freq != TRF7970A_27MHZ_CLOCK_FREQUENCY)) {
> @@ -2105,6 +2112,25 @@ static int trf7970a_probe(struct spi_device *spi)
>   if (uvolts > 400)
>   trf->chip_status_ctrl = TRF7970A_CHIP_STATUS_VRS5_3;
>  
> + trf->regulator = devm_regulator_get(>dev, "vdd-io");
> + if (IS_ERR(trf->regulator)) {
> + ret = PTR_ERR(trf->regulator);
> + dev_err(trf->dev, "Can't get VDD_IO regulator: %d\n", ret);
> + goto err_destroy_lock;
> + }
> +
> + ret = regulator_enable(trf->regulator);
> + if (ret) {
> + dev_err(trf->dev, "Can't enable VDD_IO: %d\n", ret);
> + goto err_destroy_lock;
> + }
> +
> +

Please don't add an extra blank line.

> + if (regulator_get_voltage(trf->regulator) == 180) {
> + trf->io_ctrl = TRF7970A_REG_IO_CTRL_IO_LOW;
> + dev_dbg(trf->dev, "trf7970a config vdd_io to 1.8V\n");
> + }
> +
>   trf->ddev = nfc_digital_allocate_device(_nfc_ops,
>   TRF7970A_SUPPORTED_PROTOCOLS,
>   NFC_DIGITAL_DRV_CAPS_IN_CRC |
> -- 
> Signed-off-by: Geoff Lansberry 

Your 'Signed-off-by:' goes at the end of the commit description not here.

Overall, I think you did the right thing (unless someone disagrees).
Just some minor issues.

Mark
--

Re: [PATCH 2/3] NFC: trf7970a: Add device tree option of 1.8 Volt IO voltage

[Mark Greer]

On Tue, Dec 20, 2016 at 11:13:23AM -0500, Geoff Lansberry wrote:
> On Mon, Dec 19, 2016 at 5:35 PM, Rob Herring  wrote:
> > On Thu, Dec 15, 2016 at 05:30:43PM -0500, Geoff Lansberry wrote:
> >> From: Geoff Lansberry 
> >>
> >> ---
> >>  Documentation/devicetree/bindings/net/nfc/trf7970a.txt |  2 ++
> >>  drivers/nfc/trf7970a.c | 13 -
> >>  2 files changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt 
> >> b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
> >> index 9dda879..208f045 100644
> >> --- a/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
> >> +++ b/Documentation/devicetree/bindings/net/nfc/trf7970a.txt
> >> @@ -21,6 +21,7 @@ Optional SoC Specific Properties:
> >>  - t5t-rmb-extra-byte-quirk: Specify that the trf7970a has the erratum
> >>where an extra byte is returned by Read Multiple Block commands issued
> >>to Type 5 tags.
> >> +- vdd_io_1v8: Set to specify that the trf7970a io voltage should be set 
> >> to 1.8V
> >
> > Use the regulator binding and provide a fixed 1.8V supply.
> >
> >>  - crystal_27mhz: Set to specify that the input frequency to the trf7970a 
> >> is 27.12MHz
> >>
> >>
> >> @@ -45,6 +46,7 @@ Example (for ARM-based BeagleBone with TRF7970A on SPI1):
> >>   irq-status-read-quirk;
> >>   en2-rf-quirk;
> >>   t5t-rmb-extra-byte-quirk;
> >> + vdd_io_1v8;
> >>   crystal_27mhz;
> >>   status = "okay";
> >>   };
> 
> Rob - using the regulator binding is new to me, but I've given it a
> shot and just sent you another set of patches for your inspection.
> Please let me know if this is what you had in mind.

This is my bad.  Geoff followed my example and did something similar to
'vin-voltage-override' which shouldn't have been there in the first place.
I have this fixed (I think) locally and will submit once it I'm back from
my holiday travels.

Mark
--

Re: nfc: trf7970a: Prevent repeated polling from crashing the kernel

[Mark Greer]

On Tue, Dec 20, 2016 at 02:13:52PM -0500, Justin Bronder wrote:
> On 20/12/16 11:59 -0700, Mark Greer wrote:
> > On Tue, Dec 20, 2016 at 11:16:32AM -0500, Geoff Lansberry wrote:
> > > From: Jaret Cantu <jaret.ca...@timesys.com>
> > > 
> > > Repeated polling attempts cause a NULL dereference error to occur.
> > > This is because the state of the trf7970a is currently reading but
> > > another request has been made to send a command before it has finished.
> > 
> > How is this happening?  Was trf7970a_abort_cmd() called and it didn't
> > work right?  Was it not called at all and there is a bug in the digital
> > layer?  More details please.
> > 
> > > The solution is to properly kill the waiting reading (workqueue)
> > > before failing on the send.
> > 
> > If the bug is in the calling code, then that is what should get fixed.
> > This seems to be a hack to work-around a digital layer bug.
> 
> One of our uses of NFC is to begin polling to read a tag and then stop polling
> (in order to save power) until we know via user interaction that we need to 
> poll
> again.  This is typically many minutes later so the power saving is pretty
> significant.  However, it's possible that a user will remove the tag before
> reading has completed.  We also detect this case and stop polling.  I can go
> more into this if necessary but that is what exposed a panic.
> 
> You can reproduce using neard and python, in our testing it was very likely to
> occur in 10-100 iterations of the following.:
> 
> #!/usr/bin/python
> import time
> 
> import dbus
> 
> bus = dbus.SystemBus()
> nfc0 = bus.get_object('org.neard', '/org/neard/nfc0')
> props = dbus.Interface(nfc0, 'org.freedesktop.DBus.Properties')
> 
> try:
> props.Set('org.neard.Adapter', 'Powered', dbus.Boolean(1))
> except:
> pass
> 
> adapter = dbus.Interface(nfc0, 'org.neard.Adapter')
> 
> for i in range(1000):
> adapter.StartPollLoop('Initiator')
> time.sleep(0.1)
> adapter.StopPollLoop()
> print(i)
> 
> I believe the last time we tested this was around the 4.1 release.

Thanks for the info, Justin, but I was also seeking more information
at the kernel NFC subsystem and trf7970a driver level.  This patch
adds code inside an 'if' in the driver whose condition should never
be evaluate to true but apparently it did.  How?

Thanks,

Mark
--

Re: [PATCH 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel

[Mark Greer]

On Tue, Dec 20, 2016 at 11:16:32AM -0500, Geoff Lansberry wrote:
> From: Jaret Cantu 
> 
> Repeated polling attempts cause a NULL dereference error to occur.
> This is because the state of the trf7970a is currently reading but
> another request has been made to send a command before it has finished.

How is this happening?  Was trf7970a_abort_cmd() called and it didn't
work right?  Was it not called at all and there is a bug in the digital
layer?  More details please.

> The solution is to properly kill the waiting reading (workqueue)
> before failing on the send.

If the bug is in the calling code, then that is what should get fixed.
This seems to be a hack to work-around a digital layer bug.

Mark
--

Re: [PATCH 1/3] NFC: trf7970a: add device tree option for 27MHz clock

[Mark Greer]

On Tue, Dec 20, 2016 at 01:29:13PM -0500, Geoff Lansberry wrote:
> On Tue, Dec 20, 2016 at 1:11 PM, Mark Greer <mgr...@animalcreek.com> wrote:
> > Hi Geoff.
> >
> > Please put the version in your subjects when submitting anything but the
> > initial version of a patch (e.g., [PATCH v2 1/3]).
> >
> > Which series do you want reviewed?
> >
> > Mark
> > --
> Sorry about the double posting, I had forgotten to erase the patches I
> generated while rebasing and checking, and I'll have to figure out how
> to add that v2 line to the automatically generated subject line if I
> end up submitting another round.

Hint: -v  option of 'git format-patch'

> Please review the three most recent patches, which have the send time
> of 17:16.

Okay, thank.

Mark
--

Re: [PATCH 1/3] NFC: trf7970a: add device tree option for 27MHz clock

[Mark Greer]

Hi Geoff.

Please put the version in your subjects when submitting anything but the
initial version of a patch (e.g., [PATCH v2 1/3]).

Which series do you want reviewed?

Mark
--

Re: [PATCH 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel

[Mark Greer]

On Thu, Dec 15, 2016 at 05:30:44PM -0500, Geoff Lansberry wrote:
> From: Jaret Cantu 
> 
> Repeated polling attempts cause a NULL dereference error to occur.
> This is because the curent state of the trf7970a is reading but
> a request has been made to send a command.
> 
> The solution is to properly kill the waiting reading (workqueue)
> before failing on the send.

Maybe its just me but I find this description a little hard to grok.
Mind reworking it?

The patch itself looks fine.

Thanks,

Mark
--

Re: [PATCH 2/3] NFC: trf7970a: Add device tree option of 1.8 Volt IO voltage

[Mark Greer]

On Thu, Dec 15, 2016 at 05:30:43PM -0500, Geoff Lansberry wrote:
> From: Geoff Lansberry 

Missing commit description.

> diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
> index 2d2a077..b4c37ab 100644
> --- a/drivers/nfc/trf7970a.c
> +++ b/drivers/nfc/trf7970a.c

> @@ -1048,6 +1049,11 @@ static int trf7970a_init(struct trf7970a *trf)
>   if (ret)
>   goto err_out;
>  
> + ret = trf7970a_write(trf, TRF7970A_REG_IO_CTRL,
> + trf->io_ctrl|TRF7970A_REG_IO_CTRL_VRS(0x1));

s/l|T/l | T/

Otherwise, looks good.

Mark
--

Re: [PATCH 1/3] NFC: trf7970a: add device tree option for 27MHz clock

[Mark Greer]

Hi Geoff.

On Thu, Dec 15, 2016 at 05:30:42PM -0500, Geoff Lansberry wrote:
> From: Geoff Lansberry 

Please add an informative commit description to all of your commits.
No matter how trivial this patch may seem to you now, it may not be
to others (or to you in a few years).

> diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
> index 26c9dbb..2d2a077 100644
> --- a/drivers/nfc/trf7970a.c
> +++ b/drivers/nfc/trf7970a.c

> @@ -1181,27 +1180,37 @@ static int trf7970a_in_config_rf_tech(struct trf7970a 
> *trf, int tech)
>   switch (tech) {
>   case NFC_DIGITAL_RF_TECH_106A:
>   trf->iso_ctrl_tech = TRF7970A_ISO_CTRL_14443A_106;
> - trf->modulator_sys_clk_ctrl = TRF7970A_MODULATOR_DEPTH_OOK;
> + trf->modulator_sys_clk_ctrl =
> + (trf->modulator_sys_clk_ctrl & 0xF8) |

nit: s/0xF8/0xf8/ please (for consistency with the rest of the file.).

Otherwise, it looks good.

Thanks,

Mark