Re: [RFC PATCH v1 02/20] gpio: Add GPIO polling interface to GPIO lib

From: Kent Gibson <warthog618@gmail.com>
To: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: lakshmi.sowjanya.d@intel.com,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-gpio <linux-gpio@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	mgross@linux.intel.com,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	tamal.saha@intel.com, bala.senthil@intel.com
Subject: Re: [RFC PATCH v1 02/20] gpio: Add GPIO polling interface to GPIO lib
Date: Thu, 7 Oct 2021 10:14:24 +0800	[thread overview]
Message-ID: <20211007021424.GA13920@sol> (raw)
In-Reply-To: <CAMpxmJWeZP-f-3BoWwX7PkWNZySn5RP=rc4cVyLEwYmSb6if+w@mail.gmail.com>

On Wed, Sep 22, 2021 at 12:03:53PM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 24, 2021 at 6:48 PM <lakshmi.sowjanya.d@intel.com> wrote:
> >
> > From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> >
> > Some Intel Timed I/O devices do not implement IRQ functionality. Augment
> > read() interface to allow polling.
> >
> > Add two GPIO device methods: setup_poll() and poll():
> > - setup_poll() configures the GPIO interface e.g. capture rising edges
> > - poll() checks for events on the interface
> >
> > To implement polling, the driver must implement the two functions above
> > and should either leave to_irq() method NULL or return irq 0.
> >
> > setup_poll() should configure the hardware to 'listen' for input events.
> >
> > poll() driver implementation must return the realtime timestamp
> > corresponding to the event and -EAGAIN if no data is available.
> >
> > Co-developed-by: Christopher Hall <christopher.s.hall@intel.com>
> > Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
> > Signed-off-by: Tamal Saha <tamal.saha@intel.com>
> > Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> > Reviewed-by: Mark Gross <mgross@linux.intel.com>
> > ---
> 
> Interesting. So the idea is to allow user-space to read line events as
> if they were generated by interrupts handled in the kernel. While this
> whole series has a long way to go and this patch looks wrong to me in
> several places at first glance, I find the idea interesting. Cc'ing
> Kent who's the author of most of this code - Kent: what do you think?
> 

It is interesting that we're seeing more hardware that provides more
detailed edge info than we get from irq.  The hte patch series can also
provide hardware timestamps, but the Timed I/O could even provide the
sequence numbers.
It might be worth abstracting the edge detection so edge events could be
more easily driven by subsystems other than irq, without festooning cdev
with special cases.

I'm not a fan of the polling here though, particularly from userspace.
If polling can't be avoided (why did they not provide an irq??) then I
would do the polling in kernel and place any resulting events in the
cdev kfifo for userspace to read as per the existing events.

Of course that is without knowing a whole lot about the hardware or use
cases.  The Intel datasheet doesn't provide much in the way of data :|.

Cheers,
Kent.