[PATCH] PPS: Restore lost capture-clear option to pps-gpio module.

[PATCH] PPS: Restore lost capture-clear option to pps-gpio module.

[Gary E. Miller]

In the conversion from platform to device tree the capture-clear
option was lost.

capture-clear is needed so that time_pps_fetch() will report both
edges of each PPS pulse.  Both edges are needed so that userland
programs, like gpsd, can autodetect the leading and trailing PPS
edges.

Signed-off-by: Gary E. Miller <gem@rellim.com>
Fixes: c5dbcf8b70b5 ("pps-gpio: add device-tree binding and support")
Cc: Ricardo Martins <rasm@fe.up.pt>
Cc: James Nuss <jamesnuss@nanometrics.ca>
Cc: Jan Luebbe <jlu@pengutronix.de>
Acked-by: Rodolfo Giometti <giometti@linux.it>
> ---`
---
 Documentation/devicetree/bindings/pps/pps-gpio.txt | 2 ++
 drivers/pps/clients/pps-gpio.c                     | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt b/Documentation/devicetree/bindings/pps/pps-gpio.txt
index 40bf9c3..98f8898 100644
--- a/Documentation/devicetree/bindings/pps/pps-gpio.txt
+++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt
@@ -10,6 +10,7 @@ Required properties:
 Optional properties:
 - assert-falling-edge: when present, assert is indicated by a falling edge
                        (instead of by a rising edge)
+- capture-clear: when present, also capture the clear edge
 
 Example:
 	pps {
@@ -17,4 +18,5 @@ Example:
 		gpios = <&gpio2 6 0>;
 
 		assert-falling-edge;
+		capture-clear;
 	};
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 333ad7d..dc4a71c 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -119,6 +119,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
 
 		if (of_get_property(np, "assert-falling-edge", NULL))
 			data->assert_falling_edge = true;
+		if (of_get_property(np, "capture-clear", NULL))
+			data->capture_clear = true;
 	}
 
 	/* GPIO setup */
-- 
2.3.5

Re: [PATCH] PPS: Restore lost capture-clear option to pps-gpio module.

[Jan Lübbe]

Hi Gary,

On Do, 2015-04-02 at 12:21 -0700, Gary E. Miller wrote:
> In the conversion from platform to device tree the capture-clear
> option was lost.
> 
> capture-clear is needed so that time_pps_fetch() will report both
> edges of each PPS pulse.  Both edges are needed so that userland
> programs, like gpsd, can autodetect the leading and trailing PPS
> edges.

This is intentional. The userspace program can configure which edges it
wants to capture at runtime. Only the polarity (assert-falling-edge) is
actually determined by the hardware and must be described in the device
tree.

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] PPS: Restore lost capture-clear option to pps-gpio module.

[Gary E. Miller]

Yo Jan!

On Thu, 09 Apr 2015 15:05:09 +0200
Jan Lübbe <jlu@pengutronix.de> wrote:

> Hi Gary,
> 
> On Do, 2015-04-02 at 12:21 -0700, Gary E. Miller wrote:
> > In the conversion from platform to device tree the capture-clear
> > option was lost.
> > 
> > capture-clear is needed so that time_pps_fetch() will report both
> > edges of each PPS pulse.  Both edges are needed so that userland
> > programs, like gpsd, can autodetect the leading and trailing PPS
> > edges.
> 
> This is intentional. The userspace program can configure which edges
> it wants to capture at runtime.

Yes, on pps-ldisc, but no on pps-gpio.

If you think I am wrong then please show me how I can do that in my
userspace program. I have talked to several experts that could not
figure out how to do so.  They all started with the same assertion as
yours.

The time_pps_setcap() function call will not work to do so because
the PPS_CAPTUREASSERT is locked out before time_pps_getcap().

You can easily verify this with the ppstest program on a Raspberry
Pi running Debian Wheezy using an Adafruit GPS.  

Notice that info->catpure_clear can ONLY be set one place in the entire
driver, and that place is during platform probing.  pps-gpio.c line 110.
No other place.

Without info->capture_clear it is not possible to capture the clear
edge.   Check pps-gpio.c lines 68 and 82.

I would be happy to provide you ssh access to the test platform
if you think you can prove me wrong.  I have test programs I
can show you that demonstrate this.

> Only the polarity
> (assert-falling-edge) is actually determined by the hardware and must
> be described in the device tree.

Now, but when it was a platform driver the capture assert could also be
selected in the platform hardware description.  I'm just trying to
restore lost functionality.

If instead you would be in favor of always allowing capture assert that
would be good as well.

RGDS
GARY
---------------------------------------------------------------------------
Gary E. Miller Rellim 109 NW Wilmington Ave., Suite E, Bend, OR 97701
	gem@rellim.com  Tel:+1(541)382-8588

Re: [PATCH] PPS: Restore lost capture-clear option to pps-gpio module.

[Jan Lübbe]

On Do, 2015-04-09 at 12:49 -0700, Gary E. Miller wrote:
> Yo Jan!
> 
> On Thu, 09 Apr 2015 15:05:09 +0200
> Jan Lübbe <jlu@pengutronix.de> wrote:
> 
> > Hi Gary,
> > 
> > On Do, 2015-04-02 at 12:21 -0700, Gary E. Miller wrote:
> > > In the conversion from platform to device tree the capture-clear
> > > option was lost.
> > > 
> > > capture-clear is needed so that time_pps_fetch() will report both
> > > edges of each PPS pulse.  Both edges are needed so that userland
> > > programs, like gpsd, can autodetect the leading and trailing PPS
> > > edges.
> > 
> > This is intentional. The userspace program can configure which edges
> > it wants to capture at runtime.
> 
> Yes, on pps-ldisc, but no on pps-gpio.
> 
> If you think I am wrong then please show me how I can do that in my
> userspace program. I have talked to several experts that could not
> figure out how to do so.  They all started with the same assertion as
> yours.
> 
> The time_pps_setcap() function call will not work to do so because
> the PPS_CAPTUREASSERT is locked out before time_pps_getcap().

Yes, you are correct. Looking at pps_cdev_ioctl(PPS_SETPARAMS), it will
check the requested mode against pps->info.mode and then store the
configured mode in pps->params.mode.

> Notice that info->catpure_clear can ONLY be set one place in the entire
> driver, and that place is during platform probing.  pps-gpio.c line 110.
> No other place.
> 
> Without info->capture_clear it is not possible to capture the clear
> edge.   Check pps-gpio.c lines 68 and 82.

Yes. The IRQ capabilities (as stored in info.mode) only depend on the
actual capabilities of the GPIO, so it shouldn't be necessary to declare
them again in the pps-gpio node.

> > Only the polarity
> > (assert-falling-edge) is actually determined by the hardware and must
> > be described in the device tree.
> 
> Now, but when it was a platform driver the capture assert could also be
> selected in the platform hardware description.  I'm just trying to
> restore lost functionality.
> 
> If instead you would be in favor of always allowing capture assert that
> would be good as well.

I'm strongly against adding "configuration" (as opposed to HW
description) to DT. To me, the correct way is to allow configuration of
capture clear from userspace if the HW supports it.

To do that, the pps-gpio driver would need to check (probably in
pps_gpio_probe) if the GPIO supports falling and/or rising edges. Then
it can set the info.mode field correctly. From
pps_cdev_ioctl(PPS_SETPARAMS) we'd then need to cause the driver to
reconfigure its IRQ. This way we won't cause unnecessary interrupts for
edges not needed by userspace.

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] PPS: Restore lost capture-clear option to pps-gpio module.

[Gary E. Miller]

Yo Jan!

On Fri, 10 Apr 2015 10:15:20 +0200
Jan Lübbe <jlu@pengutronix.de> wrote:

> > The time_pps_setcap() function call will not work to do so because
> > the PPS_CAPTUREASSERT is locked out before time_pps_getcap().
> 
> Yes, you are correct. Looking at pps_cdev_ioctl(PPS_SETPARAMS), it
> will check the requested mode against pps->info.mode and then store
> the configured mode in pps->params.mode.

Glad you now see it my way.

> > Without info->capture_clear it is not possible to capture the clear
> > edge.   Check pps-gpio.c lines 68 and 82.
> 
> Yes.

Glad you now see it my way.

> The IRQ capabilities (as stored in info.mode) only depend on the
> actual capabilities of the GPIO, so it shouldn't be necessary to
> declare them again in the pps-gpio node.

Nothing in my patch touches the IRQ.  Only what happens when the IRQ
happens.

At this time there is no way to declare the GPIO capabilites, so no
duplication.  I'm trying to restore the previously exitisting
functionality.

> > > Only the polarity
> > > (assert-falling-edge) is actually determined by the hardware and
> > > must be described in the device tree.
> > 
> > Now, but when it was a platform driver the capture assert could
> > also be selected in the platform hardware description.  I'm just
> > trying to restore lost functionality.
> > 
> > If instead you would be in favor of always allowing capture assert
> > that would be good as well.
> 
> I'm strongly against adding "configuration" (as opposed to HW
> description) to DT. To me, the correct way is to allow configuration
> of capture clear from userspace if the HW supports it.

I agree with you, but I was asked to fix a bug not change the (broken)
current behavior.  The current behavior is a flag.  

If you can help me get this in the kernel I would be happy to change the
patch to always allow capture_clear and nt require the configuration
variable.

> To do that, the pps-gpio driver would need to check (probably in
> pps_gpio_probe) if the GPIO supports falling and/or rising edges.

AFAIK, the GPIO always supports IRQ on either edge.  I did look at the
ARM doc.  I have reports that the assert_falling_edge always works, and
that also uses the other edge.

So, if you can get my patch in the kernel if I make capture_clear
always on I will do so.

OTOH, if there is any doubt that the clear edge can support IRQ then
my patch is still the way to go.

> Then
> it can set the info.mode field correctly. From
> pps_cdev_ioctl(PPS_SETPARAMS) we'd then need to cause the driver to
> reconfigure its IRQ. This way we won't cause unnecessary interrupts
> for edges not needed by userspace.

Looking the code as it is now I see no way to set IRQ for one edge and 
not the other.  Both edges now trigger the IRQ, but one is ignored.  I
just want to un-ignore the ignored one.

I do see the ARM hardware allows to trigger on one or both edges.  If I
can get this patch, or something similar, accepted I could look at IRQ
optimization as a follow on patch.

BUT, I am reluctant to go there as a lot of code uses gpio_to_irq() and
I am only focusing on this one narrow issue of PPS GPIO.  That should be
handled by someone that really understands all the gpio_to_irq() uses
and can get a large patch accepted

AND, worrying about one (possibly) unused IRQ a second is pretty much
down in the noise.

RGDS
GARY
---------------------------------------------------------------------------
Gary E. Miller Rellim 109 NW Wilmington Ave., Suite E, Bend, OR 97701
	gem@rellim.com  Tel:+1(541)382-8588