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

["Gary E. Miller" <gem@rellim.com>]

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