Re: [RFC PATCH v2 0/4] Input: mpr121-polled: Add polled driver for MPR121
From: Michal VokÃÄ
Date: Fri Aug 02 2019 - 08:45:08 EST
On 02. 08. 19 1:49, Dmitry Torokhov wrote:
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()?
Nice, that solved the problem and I confirm the poller mechanism works
as expected. The sysfs poll/min/max interface also works just fine.
Please Cc me when you submit your patch. I think you can already add
my "Tested-by: Michal VokÃÄ <michal.vokac@xxxxxxxxx>".
I will send mine series when the poller is in your tree. I will include
the proposed DT binding change, adding the "linux,poll-interrupt"
property, though Rob did not respond to this yet.
What about the min/max poll interval limits? Was your idea those should
also be configurable from DT? Currently I defined some limits that are
reasonable for our use case but may not be suitable for someone else.
In the meantime I also need to improve reliability of the reading.
Sometimes the keys get stuck or an electrostatic discharge causes
reset of the chip. I will extract changes that deal with these problems
from the RFC series and from some downstream patches and submit those
later.
Thank you!
Michal