Re: [RFC PATCH v2 0/4] Input: mpr121-polled: Add polled driver for MPR121

[Dmitry Torokhov]

On Tue, Jul 30, 2019 at 11:25:49AM +0200, Michal VokÃÄ wrote:
> On 27. 07. 19 9:31, Dmitry Torokhov wrote:
> > On Fri, Jul 26, 2019 at 01:31:31PM +0200, Michal VokÃÄ wrote:
> > > On 25. 07. 19 16:40, Dmitry Torokhov wrote:
> > > > On Thu, Jul 25, 2019 at 02:58:02PM +0200, Michal VokÃÄ wrote:
> > > > > On 25. 07. 19 10:57, Dmitry Torokhov wrote:
> > > > > > Hi Michal,
> > > > > > 
> > > > > > On Tue, May 21, 2019 at 08:51:17AM +0200, Michal VokÃÄ wrote:
> > > > > > > On 21. 05. 19 7:37, Dmitry Torokhov wrote:
> > > > > > > > Hi Michal,
> > > > > > > > 
> > > > > > > > On Fri, May 17, 2019 at 03:12:49PM +0200, Michal VokÃÄ wrote:
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > I have to deal with a situation where we have a custom i.MX6 based
> > > > > > > > > platform in production that uses the MPR121 touchkey controller.
> > > > > > > > > Unfortunately the chip is connected using only the I2C interface.
> > > > > > > > > The interrupt line is not used. Back in 2015 (Linux v3.14), my
> > > > > > > > > colleague modded the existing mpr121_touchkey.c driver to use polling
> > > > > > > > > instead of interrupt.
> > > > > > > > > 
> > > > > > > > > For quite some time yet I am in a process of updating the product from
> > > > > > > > > the ancient Freescale v3.14 kernel to the latest mainline and pushing
> > > > > > > > > any needed changes upstream. The DT files for our imx6dl-yapp4 platform
> > > > > > > > > already made it into v5.1-rc.
> > > > > > > > > 
> > > > > > > > > I rebased and updated our mpr121 patch to the latest mainline.
> > > > > > > > > It is created as a separate driver, similarly to gpio_keys_polled.
> > > > > > > > > 
> > > > > > > > > The I2C device is quite susceptible to ESD. An ESD test quite often
> > > > > > > > > causes reset of the chip or some register randomly changes its value.
> > > > > > > > > The [PATCH 3/4] adds a write-through register cache. With the cache
> > > > > > > > > this state can be detected and the device can be re-initialied.
> > > > > > > > > 
> > > > > > > > > The main question is: Is there any chance that such a polled driver
> > > > > > > > > could be accepted? Is it correct to implement it as a separate driver
> > > > > > > > > or should it be done as an option in the existing driver? I can not
> > > > > > > > > really imagine how I would do that though..
> > > > > > > > > 
> > > > > > > > > There are also certain worries that the MPR121 chip may no longer be
> > > > > > > > > available in nonspecifically distant future. In case of EOL I will need
> > > > > > > > > to add a polled driver for an other touchkey chip. May it be already
> > > > > > > > > in mainline or a completely new one.
> > > > > > > > 
> > > > > > > > I think that my addition of input_polled_dev was ultimately a wrong
> > > > > > > > thing to do. I am looking into enabling polling mode for regular input
> > > > > > > > devices as we then can enable polling mode in existing drivers.
> > > > > > > 
> > > > > > > OK, that sounds good. Especially when one needs to switch from one chip
> > > > > > > to another that is already in tree, the need for a whole new polling
> > > > > > > driver is eliminated.
> > > > > > 
> > > > > > Could you please try the patch below and see if it works for your use
> > > > > > case? Note that I have not tried running it, but it compiles so it must
> > > > > > be good ;)
> > > > > 
> > > > > Hi Dmitry,
> > > > > Thank you very much for the patch!
> > > > > I gave it a shot and it seems you forgot to add the input-poller.h file
> > > > > to the patch.. it does not compile on my side :(
> > > > 
> > > > Oops ;) Please see the updated patch below.
> > > 
> > > Thank you, now it is (almost) good as you said :D
> > > 
> > > > > 
> > > > > > Input: add support for polling to input devices
> > > > > > 
> > > > > > From: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > > > > > 
> > > > > > Separating "normal" and "polled" input devices was a mistake, as often we want
> > > > > > to allow the very same device work on both interrupt-driven and polled mode,
> > > > > > depending on the board on which the device is used.
> > > > > > 
> > > > > > This introduces new APIs:
> > > > > > 
> > > > > > - input_setup_polling
> > > > > > - input_set_poll_interval
> > > > > > - input_set_min_poll_interval
> > > > > > - input_set_max_poll_interval
> > > > > > 
> > > > > > These new APIs allow switching an input device into polled mode with sysfs
> > > > > > attributes matching drivers using input_polled_dev APIs that will be eventually
> > > > > > removed.
> > > > > 
> > > > > After reading this I am not really sure what else needs to be done
> > > > > to test/use the poller. I suspect I need to modify the input device
> > > > > driver (mpr121_touchkey.c in my case) like this:
> > > > > 
> > > > > If the interrupt gpio is not provided in DT, the device driver probe
> > > > > function should:
> > > > >    - not request the threaded interrupt
> > > > >    - call input_setup_polling and provide it with poll_fn
> > > > >      Can the mpr_touchkey_interrupt function be used as is for this
> > > > >      purpose? The only problem I see is it returns IRQ_HANDLED.
> > > > 
> > > > I'd factor out code suitable for polling from mpr_touchkey_interrupt()
> > > > and then do
> > > > 
> > > > static irqreturn_t mpr_touchkey_interrupt(...)
> > > > {
> > > > 	mpr_touchkey_report(...);
> > > > 	return IRQ_HANDLED;
> > > > }
> > > > 
> > > 
> > > Probably a trivial problem for experienced kernel hacker but I can not
> > > wrap my head around this - the interrupt handler takes the mpr121
> > > device id as an argument while the poller poll_fn takes struct input_dev.
> > > 
> > > I fail to figure out how to get the device id from the input device.
> > > 
> Thanks for the hints Dmitry. I am trying my best but still have some
> issues with the input_set/get_drvdata.
> 
> The kernel Oopses on NULL pointer dereference in mpr_touchkey_report.
> Here is the backtrace:
> 
> [    2.916960] 8<--- cut here ---
> [    2.920022] Unable to handle kernel NULL pointer dereference at virtual address 000001d0
> [    2.928138] pgd = (ptrval)

Ah, that's my fault I believe. Can you please try sticking

	poller->input = dev;

into input_setup_polling()?

Thanks.

-- 
Dmitry