* Re: [Question : drivers/input ] Fixing Event Filter Mechanism in input subsystem
From: Anshul Garg @ 2014-12-23 14:34 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, anshul.g@samsung.com
In-Reply-To: <20141222172622.GA18556@dtor-ws>
Dear Mr Dmitry ,
Thanks for the reply.
I understand that if some handler grabs the input device then all
events will sent to that handler only.
My Concern is If No handler has grabbed the input device then all events
should go to all handlers after application of all filter handlers on
input event
list ( As we have to check for each event whether that event can be filtered
or not).
For example : If 5 handlers registered for the input device and input device is
not grabbed.
Now among these 5 handlers 2 are filter handlers and remaining 3 are regular
input handlers.
So we have to filter the event list first after applying 2 filters
then send the remaining
events to all registered handler.
In this case as per current implementation we pass the events array to
each handler. Input Core does events filtering fr handler then send remaining
events to handler.
What i am proposing is first we have to pass input_value list to all
filter handlers
After filteration of events, we can send the remaining events (Some
events might be removed after applying filter) to all handlers.
In case device is grabbed we will just send the events to handler
which grabbed to device.
I hope to hear from you soon.
Thanks
On Mon, Dec 22, 2014 at 10:56 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Anshul,
>
> On Mon, Dec 22, 2014 at 10:36:09PM +0530, Anshul Garg wrote:
>> In function input_pass_values in input.c file , input core sends all
>> events to each handler associated with the input device ,
>>
>> rcu_read_lock();
>>
>> handle = rcu_dereference(dev->grab);
>> if (handle) {
>> count = input_to_handler(handle, vals, count);
>> } else {
>> list_for_each_entry_rcu(handle, &dev->h_list, d_node)
>> if (handle->open)
>> count = input_to_handler(handle, vals, count);
>> }
>>
>> after this in input_to_handler function events are filtered and sent
>> to the handler.
>>
>> for (v = vals; v != vals + count; v++) {
>> if (handler->filter &&
>> handler->filter(handle, v->type, v->code, v->value))
>> continue;
>> if (end != v)
>> *end = *v;
>> end++;
>> }
>>
>>
>>
>> But as per previous event filter mechanism all the events should be
>> parsed from all
>> handlers after that remaining events should be sent to handlers list.
>>
>> And in comments also its mentioned as
>>
>> /*
>> * Pass event first through all filters and then, if event has not been
>> * filtered out, through all open handles.*/
>>
>> So current approach to filter events seems to be incorrect.
>>
>> Please help to clarify my query.
>
> When you "grab" input device your handler gets exclusive access to all
> events coming form it. Neither filer handlers nor regular input handlers
> receive events from this device until you release it.
>
> So I believe it works as intended.
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* Re: [PATCHv3 2/5] ARM:sunxi:drivers:input Add support for A10/A20 PS2
From: Vishnu Patekar @ 2014-12-23 22:28 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input@vger.kernel.org, maxime.ripard@free-electrons.com,
Hans de Goede, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, robh+dt@kernel.org,
pawel.moll@arm.com, mark.rutland@arm.com,
ijc+devicetree@hellion.org.uk, Kumar Gala, linux@arm.linux.org.uk,
Grant Likely, benh@kernel.crashing.org, msalter@redhat.com,
ralf@linux-mips.org, jdelvare@suse.de
In-Reply-To: <20141214203759.GA36053@dtor-ws>
Hello Dmitry,
Sorry for delayed response. please find my comment in-lined.
Please let me know in case I am wrong.
On Mon, Dec 15, 2014 at 2:07 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Vishnu,
>
> On Fri, Dec 12, 2014 at 11:55:45PM +0530, VishnuPatekar wrote:
>>
>> Signed-off-by: VishnuPatekar <vishnupatekar0510@gmail.com>
>> ---
>> drivers/input/serio/Kconfig | 11 ++
>> drivers/input/serio/Makefile | 1 +
>> drivers/input/serio/sun4i-ps2.c | 362 +++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 374 insertions(+)
>> create mode 100644 drivers/input/serio/sun4i-ps2.c
>>
>> diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
>> index bc2d474..964afc5 100644
>> --- a/drivers/input/serio/Kconfig
>> +++ b/drivers/input/serio/Kconfig
>> @@ -281,4 +281,15 @@ config HYPERV_KEYBOARD
>> To compile this driver as a module, choose M here: the module will
>> be called hyperv_keyboard.
>>
>> +config SERIO_SUN4I_PS2
>> + tristate "Allwinner A10 PS/2 controller support"
>> + default n
>> + depends on ARCH_SUNXI || COMPILE_TEST
>> + help
>> + This selects support for the PS/2 Host Controller on
>> + Allwinner A10.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called sun4i-ps2.
>> +
>> endif
>> diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
>> index 815d874..c600089 100644
>> --- a/drivers/input/serio/Makefile
>> +++ b/drivers/input/serio/Makefile
>> @@ -29,3 +29,4 @@ obj-$(CONFIG_SERIO_ARC_PS2) += arc_ps2.o
>> obj-$(CONFIG_SERIO_APBPS2) += apbps2.o
>> obj-$(CONFIG_SERIO_OLPC_APSP) += olpc_apsp.o
>> obj-$(CONFIG_HYPERV_KEYBOARD) += hyperv-keyboard.o
>> +obj-$(CONFIG_SERIO_SUN4I_PS2) += sun4i-ps2.o
>> diff --git a/drivers/input/serio/sun4i-ps2.c b/drivers/input/serio/sun4i-ps2.c
>> new file mode 100644
>> index 0000000..e6d22ae
>> --- /dev/null
>> +++ b/drivers/input/serio/sun4i-ps2.c
>> @@ -0,0 +1,362 @@
>> +/*
>> + * Driver for Allwinner A10 PS2 host controller
>> + *
>> + * Author: Vishnu Patekar <vishnupatekar0510@gmail.com>
>> + * Aaron.maoye <leafy.myeh@newbietech.com>
>> + *
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/serio.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/errno.h>
>> +#include <linux/slab.h>
>> +#include <linux/list.h>
>> +#include <linux/io.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +
>> +#define DRIVER_NAME "sun4i-ps2"
>> +
>> +/* register offset definitions */
>> +#define PS2_REG_GCTL (0x00) /* PS2 Module Global Control Reg */
>> +#define PS2_REG_DATA (0x04) /* PS2 Module Data Reg */
>> +#define PS2_REG_LCTL (0x08) /* PS2 Module Line Control Reg */
>> +#define PS2_REG_LSTS (0x0C) /* PS2 Module Line Status Reg */
>> +#define PS2_REG_FCTL (0x10) /* PS2 Module FIFO Control Reg */
>> +#define PS2_REG_FSTS (0x14) /* PS2 Module FIFO Status Reg */
>> +#define PS2_REG_CLKDR (0x18) /* PS2 Module Clock Divider Reg*/
>
> You do not need parenthesis around simple constants.
Okie, I'll remove.
>
>> +
>> +/* PS2 GLOBAL CONTROL REGISTER PS2_GCTL */
>> +#define PS2_GCTL_INTFLAG BIT(4)
>> +#define PS2_GCTL_INTEN BIT(3)
>> +#define PS2_GCTL_RESET BIT(2)
>> +#define PS2_GCTL_MASTER BIT(1)
>> +#define PS2_GCTL_BUSEN BIT(0)
>> +
>> +/* PS2 LINE CONTROL REGISTER */
>> +#define PS2_LCTL_NOACK BIT(18)
>> +#define PS2_LCTL_TXDTOEN BIT(8)
>> +#define PS2_LCTL_STOPERREN BIT(3)
>> +#define PS2_LCTL_ACKERREN BIT(2)
>> +#define PS2_LCTL_PARERREN BIT(1)
>> +#define PS2_LCTL_RXDTOEN BIT(0)
>> +
>> +/* PS2 LINE STATUS REGISTER */
>> +#define PS2_LSTS_TXTDO BIT(8)
>> +#define PS2_LSTS_STOPERR BIT(3)
>> +#define PS2_LSTS_ACKERR BIT(2)
>> +#define PS2_LSTS_PARERR BIT(1)
>> +#define PS2_LSTS_RXTDO BIT(0)
>> +
>> +#define PS2_LINE_ERROR_BIT \
>> + (PS2_LSTS_TXTDO | PS2_LSTS_STOPERR | PS2_LSTS_ACKERR | \
>> + PS2_LSTS_PARERR | PS2_LSTS_RXTDO)
>> +
>> +/* PS2 FIFO CONTROL REGISTER */
>> +#define PS2_FCTL_TXRST BIT(17)
>> +#define PS2_FCTL_RXRST BIT(16)
>> +#define PS2_FCTL_TXUFIEN BIT(10)
>> +#define PS2_FCTL_TXOFIEN BIT(9)
>> +#define PS2_FCTL_TXRDYIEN BIT(8)
>> +#define PS2_FCTL_RXUFIEN BIT(2)
>> +#define PS2_FCTL_RXOFIEN BIT(1)
>> +#define PS2_FCTL_RXRDYIEN BIT(0)
>> +
>> +/* PS2 FIFO STATUS REGISTER */
>> +#define PS2_FSTS_TXUF BIT(10)
>> +#define PS2_FSTS_TXOF BIT(9)
>> +#define PS2_FSTS_TXRDY BIT(8)
>> +#define PS2_FSTS_RXUF BIT(2)
>> +#define PS2_FSTS_RXOF BIT(1)
>> +#define PS2_FSTS_RXRDY BIT(0)
>> +
>> +#define PS2_FIFO_ERROR_BIT \
>> + (PS2_FSTS_TXUF | PS2_FSTS_TXOF | PS2_FSTS_RXUF | PS2_FSTS_RXOF)
>> +
>> +#define PS2_SAMPLE_CLK (1000000)
>> +#define PS2_SCLK (125000)
I'll remove these parenthesis around simple constants.
>> +
>> +struct sun4i_ps2data {
>> + struct serio *serio;
>> + struct device *dev;
>> +
>> + /* IO mapping base */
>> + void __iomem *reg_base;
>> +
>> + /* clock management */
>> + struct clk *clk;
>> +
>> + /* irq */
>> + spinlock_t lock;
>> + int irq;
>> +};
>> +
>> +/*********************/
>> +/* Interrupt handler */
>> +/*********************/
>> +static irqreturn_t sun4i_ps2_interrupt(int irq, void *dev_id)
>> +{
>> + struct sun4i_ps2data *drvdata = dev_id;
>> + u32 intr_status;
>> + u32 fifo_status;
>> + unsigned char byte;
>> + u32 rval;
>> + u32 error = 0;
>> +
>> + spin_lock(&drvdata->lock);
>> +
>> + /* Get the PS/2 interrupts and clear them */
>> + intr_status = readl(drvdata->reg_base + PS2_REG_LSTS);
>> + fifo_status = readl(drvdata->reg_base + PS2_REG_FSTS);
>> +
>> + /*Check Line Status Register*/
>> + if (intr_status & PS2_LINE_ERROR_BIT) {
>> + if (intr_status & PS2_LSTS_STOPERR)
>> + dev_info(drvdata->dev, "PS/2 Stop Bit Error!");
>
> The stop bit error is I believe should be reported to consumers as
> SSERIO_FRAME, receive timeout as SERIO_TIMEOUT and parity as
> SERIO_PARITY.
Yes, this can be passed via flags in setio_interrpt. I'll add this.
>
>> + if (intr_status & PS2_LSTS_ACKERR)
>> + dev_info(drvdata->dev, "PS/2 Acknowledge Error!\n");
>
> When is this error signalled?
this is signaled when ACK is not received after data transmitted.
>
>> + if (intr_status & PS2_LSTS_PARERR)
>> + dev_info(drvdata->dev, "PS/2 Parity Error!\n");
>> + if (intr_status & PS2_LSTS_TXTDO)
>> + dev_info(drvdata->dev, "PS/2 Transmit Data Timeout!\n");
>
> Should not you check this status in serio_write() instead of here?
I never get transmit data timeout in serio_write(), even if PS2
keyboard is not connected.
However, it generates this interrupt.
Yes, in case of timeout/failure in serio_write SERIO_TIMEOUT can be returned.
>
>> + if (intr_status & PS2_LSTS_RXTDO)
>> + dev_info(drvdata->dev, "PS/2 Receive Data Timeout!\n");
>> +
>> + /*reset PS/2 controller*/
>> + rval = readl(drvdata->reg_base + PS2_REG_GCTL);
>> + writel(rval | PS2_GCTL_RESET, drvdata->reg_base + PS2_REG_GCTL);
>
> Why do we need to reset controller in case of, let's say, parity glitch?
As per the Reference Manual :
Setting SOFT RESET bit will reset transmitter and receiver of PS2
Module, and the status of transmitter and receiver will revert
to the default state, but not affect any control bits in register,
and data in TXFIFO/RXFIFO.
I think, there is no harm in soft resetting PS2 instead of writing to
multiple registers.
>
>> +
>> + rval = PS2_LSTS_TXTDO | PS2_LSTS_STOPERR | PS2_LSTS_ACKERR |
>> + PS2_LSTS_PARERR | PS2_LSTS_RXTDO;
>> + writel(rval, drvdata->reg_base + PS2_REG_LSTS);
>> + error = 1;
>> + }
>> +
>> + /*Check FIFO Status Register*/
>> + if (fifo_status & PS2_FIFO_ERROR_BIT) {
>> + if (fifo_status & PS2_FSTS_TXUF)
>> + dev_info(drvdata->dev, "PS/2 Tx FIFO Underflow!\n");
>> + if (fifo_status & PS2_FSTS_TXOF)
>> + dev_info(drvdata->dev, "PS/2 Tx FIFO Overflow!\n");
>> + if (fifo_status & PS2_FSTS_RXUF)
>> + dev_info(drvdata->dev, "PS/2 Rx FIFO Underflow!\n");
>> + if (fifo_status & PS2_FSTS_RXOF)
>> + dev_info(drvdata->dev, "PS/2 Rx FIFO Overflow!\n");
>> + /*reset PS/2 controller*/
>> + writel(readl(drvdata->reg_base + PS2_REG_GCTL) | PS2_GCTL_RESET,
>> + drvdata->reg_base + PS2_REG_GCTL);
>
> Again, we do we need to reset controller? Does it stop receiving data
> after RX overflow condition?
After RX overflow it just set the overflow flag in FSTS reg, then we
can reset FIFO to get more data.
>
>> +
>> + rval = PS2_FSTS_TXUF | PS2_FSTS_TXOF | PS2_FSTS_TXRDY |
>> + PS2_FSTS_RXUF | PS2_FSTS_RXOF | PS2_FSTS_RXRDY;
>> + writel(rval, drvdata->reg_base + PS2_REG_FSTS);
>> + error = 1;
>> + }
>> +
>> + rval = (fifo_status >> 16) & 0x3;
>> + while (!error && rval--) {
>> + byte = readl(drvdata->reg_base + PS2_REG_DATA) & 0xff;
>> + serio_interrupt(drvdata->serio, byte, 0);
>> + }
>> +
>> + writel(intr_status, drvdata->reg_base + PS2_REG_LSTS);
>> + writel(fifo_status, drvdata->reg_base + PS2_REG_FSTS);
>> +
>> + spin_unlock(&drvdata->lock);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +
>> +static int sun4i_ps2_open(struct serio *pserio)
>> +{
>> + struct sun4i_ps2data *drvdata = pserio->port_data;
>> + u32 src_clk = 0;
>> + u32 clk_scdf;
>> + u32 clk_pcdf;
>> + u32 rval;
>> + unsigned long flags;
>> +
>> + /*Set Line Control And Enable Interrupt*/
>> + rval = PS2_LCTL_TXDTOEN | PS2_LCTL_STOPERREN | PS2_LCTL_ACKERREN
>> + | PS2_LCTL_PARERREN | PS2_LCTL_RXDTOEN;
>> + writel(rval, drvdata->reg_base + PS2_REG_LCTL);
>> +
>> + /*Reset FIFO*/
>> + rval = PS2_FCTL_TXRST | PS2_FCTL_RXRST | PS2_FCTL_TXUFIEN
>> + | PS2_FCTL_TXOFIEN | PS2_FCTL_RXUFIEN
>> + | PS2_FCTL_RXOFIEN | PS2_FCTL_RXRDYIEN;
>> +
>> + writel(rval, drvdata->reg_base + PS2_REG_FCTL);
>> +
>> + src_clk = clk_get_rate(drvdata->clk);
>> + /*Set Clock Divider Register*/
>> + clk_scdf = DIV_ROUND_UP(src_clk, PS2_SAMPLE_CLK) - 1;
>> + clk_pcdf = DIV_ROUND_UP(PS2_SAMPLE_CLK, PS2_SCLK) - 1;
>> + rval = (clk_scdf<<8) | clk_pcdf;
>> + writel(rval, drvdata->reg_base + PS2_REG_CLKDR);
>> +
>> +
>> + /*Set Global Control Register*/
>> + rval = PS2_GCTL_RESET | PS2_GCTL_INTEN | PS2_GCTL_MASTER
>> + | PS2_GCTL_BUSEN;
>> +
>> + spin_lock_irqsave(&drvdata->lock, flags);
>> + writel(rval, drvdata->reg_base + PS2_REG_GCTL);
>> + spin_unlock_irqrestore(&drvdata->lock, flags);
>> +
>> + return 0;
>> +}
>> +
>> +static void sun4i_ps2_close(struct serio *pserio)
>> +{
>> + struct sun4i_ps2data *drvdata = pserio->port_data;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&drvdata->lock, flags);
>> + /* Disable the PS2 interrupts */
>> + writel(0, drvdata->reg_base + PS2_REG_GCTL);
>> + spin_unlock_irqrestore(&drvdata->lock, flags);
>
>
> I think you want to add synchronize_irq() here to make sure pending
> interrupt handler runs to completion before you return.
Yes, synchronize_irq()
>
>> +}
>> +
>> +static int sun4i_ps2_write(struct serio *pserio, unsigned char val)
>> +{
>> + unsigned long expire = jiffies + msecs_to_jiffies(10000);
>> + struct sun4i_ps2data *drvdata;
>> +
>> + drvdata = (struct sun4i_ps2data *)pserio->port_data;
>> +
>> + do {
>> + if (readl(drvdata->reg_base + PS2_REG_FSTS) & PS2_FSTS_TXRDY) {
>> + writel(val, drvdata->reg_base + PS2_REG_DATA);
>> + return 0;
>> + }
>> + } while (time_before(jiffies, expire));
>> +
>> + return 0;
>> +}
>> +
>> +static int sun4i_ps2_probe(struct platform_device *pdev)
>> +{
>> + struct resource *res; /* IO mem resources */
>> + struct sun4i_ps2data *drvdata;
>> + struct serio *serio;
>> + struct device *dev = &pdev->dev;
>> + unsigned int irq;
>> + int error;
>> +
>> + drvdata = devm_kzalloc(dev, sizeof(struct sun4i_ps2data), GFP_KERNEL);
>> + serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
>> + if (!drvdata || !serio) {
>> + error = -ENOMEM;
>> + goto err_free_mem;
>> + }
>> +
>> + spin_lock_init(&drvdata->lock);
>> +
>> + /* IO */
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + drvdata->reg_base = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(drvdata->reg_base)) {
>> + dev_err(dev, "failed to map registers\n");
>> + error = PTR_ERR(drvdata->reg_base);
>> + goto err_free_mem;
>> + }
>> +
>> + drvdata->clk = devm_clk_get(dev, NULL);
>> + if (IS_ERR(drvdata->clk)) {
>> + error = PTR_ERR(drvdata->clk);
>> + dev_err(dev, "couldn't get clock %d\n", error);
>> + goto err_free_mem;
>> + }
>> +
>> + error = clk_prepare_enable(drvdata->clk);
>> + if (error) {
>> + dev_err(dev, "failed to enable clock %d\n", error);
>> + goto err_free_mem;
>> + }
>> +
>
> I think you should explicitly prohibit the device generating interrupts
> by writing to PS2_ERG_CTRL here.
Okie.
>
>> + serio->id.type = SERIO_8042;
>> + serio->write = sun4i_ps2_write;
>> + serio->open = sun4i_ps2_open;
>> + serio->close = sun4i_ps2_close;
>> + serio->port_data = drvdata;
>> + serio->dev.parent = dev;
>> + strlcpy(serio->name, dev_name(dev), sizeof(serio->name));
>> + strlcpy(serio->phys, dev_name(dev), sizeof(serio->phys));
>> +
>> + /* Get IRQ for the device */
>> + irq = platform_get_irq(pdev, 0);
>> + if (!irq) {
>> + dev_err(dev, "no IRQ found\n");
>> + error = -ENXIO;
>> + goto error_disable_clk;
>> + }
>> +
>> + drvdata->irq = irq;
>> + drvdata->serio = serio;
>> + drvdata->dev = dev;
>> + error = devm_request_threaded_irq(dev, drvdata->irq,
>> + sun4i_ps2_interrupt, NULL, 0, DRIVER_NAME, drvdata);
>> +
>> + if (error) {
>> + dev_err(drvdata->dev, "Interrupt alloc failed %d:error:%d\n",
>> + drvdata->irq, error);
>> + goto error_disable_clk;
>> + }
>> +
>> + serio_register_port(serio);
>> + platform_set_drvdata(pdev, drvdata);
>> +
>> + return 0; /* success */
>> +
>> +error_disable_clk:
>> + clk_disable_unprepare(drvdata->clk);
>> +
>> +err_free_mem:
>> + kfree(serio);
>> + return error;
>> +}
>> +
>> +static int sun4i_ps2_remove(struct platform_device *pdev)
>> +{
>> + struct sun4i_ps2data *drvdata = platform_get_drvdata(pdev);
>> +
>> + serio_unregister_port(drvdata->serio);
>> + disable_irq(drvdata->irq);
>
> I do not think you need to disable irq here - serio_close will make sure
> that the device does not generate interrupts.
Okie
>
>> +
>> + if (!IS_ERR(drvdata->clk))
>> + clk_disable_unprepare(drvdata->clk);
>> + kfree(drvdata->serio);
>> +
>> + return 0;
>> +}
>> +
>> +/* Match table for of_platform binding */
>> +static const struct of_device_id sun4i_ps2_match[] = {
>> + { .compatible = "allwinner,sun4i-a10-ps2", },
>> + { },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, sun4i_ps2_match);
>> +
>> +/*platform driver structure*/
>
> I do not think we should keep the obvious comments like this one.
Okie, I'll remove it.
>
>> +static struct platform_driver sun4i_ps2_driver = {
>> + .probe = sun4i_ps2_probe,
>> + .remove = sun4i_ps2_remove,
>> + .driver = {
>> + .name = DRIVER_NAME,
>> + .of_match_table = sun4i_ps2_match,
>> + },
>> +};
>> +module_platform_driver(sun4i_ps2_driver);
>> +
>> +MODULE_AUTHOR("Vishnu Patekar <vishnupatekar0510@gmail.com>");
>> +MODULE_AUTHOR("Aaron.maoye <leafy.myeh@newbietech.com>");
>> +MODULE_DESCRIPTION("Allwinner A10/Sun4i PS/2 driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.7.9.5
>>
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* [PATCH 1/1] Elantech touchpad detection fix for laptops
From: Srihari Vijayaraghavan @ 2014-12-24 0:17 UTC (permalink / raw)
To: linux-input
Cc: Dmitry Torokhov, Mateusz Jończyk, Zakariya Dehlawi,
linux-kernel
Hello Folks,
[Thanks for Dmitry for pointing out my error in previous attempts at
sending this email. I've fixed it now. Sorry if you've received this
email multiple times by now.]
Months ago, I raised kernel bugzilla 81331
(https://bugzilla.kernel.org/show_bug.cgi?id=81331) for Linux not
detecting my Elantech touchpad from a cold boot. As can be seen in
that bug report, besides myself, it's indeed affecting a number of
other users.
Since then, based on the original patch fix given by Mateusz Jonczyk,
I've extended it to be this (which not only fixes my problem, but
others as well) & would appreciate your review (and inputs thereof) &
if you're happy with it, then request you to kindly merge it upstream
on mainline & stable.
On behalf of all affected users, much appreciate your help in this
matter. Thank you.
PS: If gmail somehow spoils this patch when it comes through, then
please refer to the above kernel bugzilla URL, where the same patch is
available as a file attachment.
diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
index 479f332..5ae99f3 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1270,6 +1270,7 @@ bytes respectively. Such letter suffixes can
also be entirely omitted.
i8042.notimeout [HW] Ignore timeout condition signalled by controller
i8042.reset [HW] Reset the controller during init and cleanup
i8042.unlock [HW] Unlock (ignore) the keylock
+ i8042.kbdreset [HW] Reset keyboard to detect Elantech touchpad
i810= [HW,DRM]
diff --git a/drivers/input/serio/i8042-x86ia64io.h
b/drivers/input/serio/i8042-x86ia64io.h
index faeeb13..40e85ed 100644
--- a/drivers/input/serio/i8042-x86ia64io.h
+++ b/drivers/input/serio/i8042-x86ia64io.h
@@ -735,6 +735,29 @@ static const struct dmi_system_id __initconst
i8042_dmi_dritek_table[] = {
{ }
};
+/*
+ * Just as per kernel bugzilla #81331, some Elantech touchpad based laptops
+ * -- e.g., Gigabyte P35 v2, Gigabyte X3 Plus -- need keyboard reset while
+ * probing for the touchpad to get detected, initialised & finally work.
+ */
+static const struct dmi_system_id __initconst
i8042_dmi_elantech_kbdreset_table[] = {
+ {
+ /* Gigabyte P35 v2 */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "GIGABYTE"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "P35V2"),
+ },
+ },
+ {
+ /* Aorus branded Gigabyte X3 Plus */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "GIGABYTE"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "X3"),
+ },
+ },
+ { }
+};
+
#endif /* CONFIG_X86 */
#ifdef CONFIG_PNP
@@ -1030,6 +1053,9 @@ static int __init i8042_platform_init(void)
if (dmi_check_system(i8042_dmi_dritek_table))
i8042_dritek = true;
+ if (dmi_check_system(i8042_dmi_elantech_kbdreset_table))
+ i8042_kbdreset = true;
+
/*
* A20 was already enabled during early kernel init. But some buggy
* BIOSes (in MSI Laptops) require A20 to be enabled using 8042 to
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index f5a98af..7cf42df 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -67,6 +67,10 @@ static bool i8042_notimeout;
module_param_named(notimeout, i8042_notimeout, bool, 0);
MODULE_PARM_DESC(notimeout, "Ignore timeouts signalled by i8042");
+static bool i8042_kbdreset;
+module_param_named(kbdreset, i8042_kbdreset, bool, 0);
+MODULE_PARM_DESC(kbdreset, "Reset keyboard to detect Elantech
touchpad on some laptops");
+
#ifdef CONFIG_X86
static bool i8042_dritek;
module_param_named(dritek, i8042_dritek, bool, 0);
@@ -790,6 +794,14 @@ static int __init i8042_check_aux(void)
return -1;
/*
+ * Reset keyboard to detect touchpad on some laptops having Elantech touchpad
+ */
+ if (i8042_kbdreset) {
+ pr_warn("Resetting keyboard to detect Elantech
touchpad on some laptops\n");
+ i8042_kbd_write(NULL, (unsigned char) 0xff);
+ }
+
+/*
* Test AUX IRQ delivery to make sure BIOS did not grab the IRQ and
* used it for a PCI card or somethig else.
*/
^ permalink raw reply related
* [PATCH v5 0/6] Touchscreen performance related fixes
From: Vignesh R @ 2014-12-24 7:03 UTC (permalink / raw)
To: Jonathan Cameron, Hartmut Knaack, Dmitry Torokhov, Lee Jones,
Sebastian Andrzej Siewior
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Benoit Cousson, Tony Lindgren, Russell King, Lars-Peter Clausen,
Peter Meerwald, Samuel Ortiz, Brad Griffis, Paul Gortmaker,
Sanjeev Sharma, Jan Kardell, Felipe Balbi, Wolfram Sang,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA, Vignesh R
This series is rebase of v4 onto v3.19-rc1. It also fixes concerns
expressed on v4 wrt simultaneous use of IIO and TSC.
I have tested this patch series on am335x-evm and Beaglebone black
with lcd7-cape.
Note that, these patches do not work as expected on Beaglebone Black
with BB-View 4.3 Cape from Farnell due to a hardware issue in the cape.
Change log:
v5:
- Rebased onto v3.19-rc1
- minor changes in patch 2,3 and 5.
v3:
- Replace delta filtering logic in TSC driver with median filtering
as suggested by Richard.
- Addressed Lee Jones comments.
v2:
- Addressed comments by Hartmut Knaack
- patch 2 was split into two as per Lee Jones comment
Brad Griffis (2):
input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC
steps
input: touchscreen: ti_am335x_tsc: Remove udelay in interrupt handler
Vignesh R (4):
mfd: ti_am335x_tscadc: Remove unwanted reg_se_cache save
ARM: dts: AM335x: Make charge delay a DT parameter for TSC
input: touchscreen: ti_am335x_tsc: Use charge delay DT parameter
input: touchscreen: ti_am335x_tsc: Replace delta filtering with median
filtering
.../bindings/input/touchscreen/ti-tsc-adc.txt | 15 ++
arch/arm/boot/dts/am335x-evm.dts | 1 +
drivers/iio/adc/ti_am335x_adc.c | 5 +-
drivers/input/touchscreen/ti_am335x_tsc.c | 196 ++++++++++++---------
drivers/mfd/ti_am335x_tscadc.c | 13 +-
include/linux/mfd/ti_am335x_tscadc.h | 4 +-
6 files changed, 136 insertions(+), 98 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH v5 1/6] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps
From: Vignesh R @ 2014-12-24 7:03 UTC (permalink / raw)
To: Jonathan Cameron, Hartmut Knaack, Dmitry Torokhov, Lee Jones,
Sebastian Andrzej Siewior
Cc: Mark Rutland, Vignesh R, Wolfram Sang, linux-iio, Paul Gortmaker,
Peter Meerwald, Lars-Peter Clausen, Russell King, Jan Kardell,
Tony Lindgren, linux-input, Sanjeev Sharma, devicetree,
Brad Griffis, Pawel Moll, Ian Campbell, Rob Herring, linux-omap,
linux-arm-kernel, Samuel Ortiz, linux-kernel, Felipe Balbi,
Benoit Cousson, Kumar Gala
In-Reply-To: <1419404619-10744-1-git-send-email-vigneshr@ti.com>
From: Brad Griffis <bgriffis@ti.com>
This patch makes the initial changes required to workaround TSC-false
pen-up interrupts. It is required to implement these changes in order to
remove udelay in the TSC interrupt handler and false pen-up events.
The charge step is to be executed immediately after sampling X+. Hence
TSC is made to use higher numbered steps (steps 5 to 16 for 5 co-ordinate
readouts, 4 wire TSC configuration) and ADC to use lower ones. Further
X co-ordinate readouts must be the last to be sampled, thus co-ordinates
are sampled in the order Y-Z-X.
Signed-off-by: Brad Griffis <bgriffis@ti.com>
[vigneshr@ti.com: Ported the patch from v3.12 to v3.19rc1]
Signed-off-by: Vignesh R <vigneshr@ti.com>
Acked-by: Jonathan Cameron <jic23@kernel.org>
---
drivers/iio/adc/ti_am335x_adc.c | 5 ++--
drivers/input/touchscreen/ti_am335x_tsc.c | 42 ++++++++++++++++++-------------
2 files changed, 26 insertions(+), 21 deletions(-)
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index b730864731e8..adba23246474 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -86,19 +86,18 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
{
struct tiadc_device *adc_dev = iio_priv(indio_dev);
unsigned int stepconfig;
- int i, steps;
+ int i, steps = 0;
/*
* There are 16 configurable steps and 8 analog input
* lines available which are shared between Touchscreen and ADC.
*
- * Steps backwards i.e. from 16 towards 0 are used by ADC
+ * Steps forwards i.e. from 0 towards 16 are used by ADC
* depending on number of input lines needed.
* Channel would represent which analog input
* needs to be given to ADC to digitalize data.
*/
- steps = TOTAL_STEPS - adc_dev->channels;
if (iio_buffer_enabled(indio_dev))
stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
| STEPCONFIG_MODE_SWCNT;
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 004f1346a957..dfbb9fe6a270 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -121,7 +121,7 @@ static void titsc_step_config(struct titsc *ts_dev)
{
unsigned int config;
int i;
- int end_step;
+ int end_step, first_step, tsc_steps;
u32 stepenable;
config = STEPCONFIG_MODE_HWSYNC |
@@ -140,9 +140,11 @@ static void titsc_step_config(struct titsc *ts_dev)
break;
}
- /* 1 … coordinate_readouts is for X */
- end_step = ts_dev->coordinate_readouts;
- for (i = 0; i < end_step; i++) {
+ tsc_steps = ts_dev->coordinate_readouts * 2 + 2;
+ first_step = TOTAL_STEPS - tsc_steps;
+ /* Steps 16 to 16-coordinate_readouts is for X */
+ end_step = first_step + tsc_steps;
+ for (i = end_step - ts_dev->coordinate_readouts; i < end_step; i++) {
titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
}
@@ -164,9 +166,9 @@ static void titsc_step_config(struct titsc *ts_dev)
break;
}
- /* coordinate_readouts … coordinate_readouts * 2 is for Y */
- end_step = ts_dev->coordinate_readouts * 2;
- for (i = ts_dev->coordinate_readouts; i < end_step; i++) {
+ /* 1 ... coordinate_readouts is for Y */
+ end_step = first_step + ts_dev->coordinate_readouts;
+ for (i = first_step; i < end_step; i++) {
titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
}
@@ -179,7 +181,7 @@ static void titsc_step_config(struct titsc *ts_dev)
titsc_writel(ts_dev, REG_CHARGECONFIG, config);
titsc_writel(ts_dev, REG_CHARGEDELAY, CHARGEDLY_OPENDLY);
- /* coordinate_readouts * 2 … coordinate_readouts * 2 + 2 is for Z */
+ /* coordinate_readouts + 1 ... coordinate_readouts + 2 is for Z */
config = STEPCONFIG_MODE_HWSYNC |
STEPCONFIG_AVG_16 | ts_dev->bit_yp |
ts_dev->bit_xn | STEPCONFIG_INM_ADCREFM |
@@ -194,8 +196,11 @@ static void titsc_step_config(struct titsc *ts_dev)
titsc_writel(ts_dev, REG_STEPDELAY(end_step),
STEPCONFIG_OPENDLY);
- /* The steps1 … end and bit 0 for TS_Charge */
- stepenable = (1 << (end_step + 2)) - 1;
+ /* The steps end ... end - readouts * 2 + 2 and bit 0 for TS_Charge */
+ stepenable = 1;
+ for (i = 0; i < tsc_steps; i++)
+ stepenable |= 1 << (first_step + i + 1);
+
ts_dev->step_mask = stepenable;
am335x_tsc_se_set_cache(ts_dev->mfd_tscadc, ts_dev->step_mask);
}
@@ -209,6 +214,7 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
unsigned int read, diff;
unsigned int i, channel;
unsigned int creads = ts_dev->coordinate_readouts;
+ unsigned int first_step = TOTAL_STEPS - (creads * 2 + 2);
*z1 = *z2 = 0;
if (fifocount % (creads * 2 + 2))
@@ -226,7 +232,7 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
channel = (read & 0xf0000) >> 16;
read &= 0xfff;
- if (channel < creads) {
+ if (channel > first_step + creads + 2) {
diff = abs(read - prev_val_x);
if (diff < prev_diff_x) {
prev_diff_x = diff;
@@ -234,19 +240,19 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
}
prev_val_x = read;
- } else if (channel < creads * 2) {
+ } else if (channel == first_step + creads + 1) {
+ *z1 = read;
+
+ } else if (channel == first_step + creads + 2) {
+ *z2 = read;
+
+ } else if (channel > first_step) {
diff = abs(read - prev_val_y);
if (diff < prev_diff_y) {
prev_diff_y = diff;
*y = read;
}
prev_val_y = read;
-
- } else if (channel < creads * 2 + 1) {
- *z1 = read;
-
- } else if (channel < creads * 2 + 2) {
- *z2 = read;
}
}
}
--
1.9.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v5 2/6] input: touchscreen: ti_am335x_tsc: Remove udelay in interrupt handler
From: Vignesh R @ 2014-12-24 7:03 UTC (permalink / raw)
To: Jonathan Cameron, Hartmut Knaack, Dmitry Torokhov, Lee Jones,
Sebastian Andrzej Siewior
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Benoit Cousson, Tony Lindgren, Russell King, Lars-Peter Clausen,
Peter Meerwald, Samuel Ortiz, Brad Griffis, Paul Gortmaker,
Sanjeev Sharma, Jan Kardell, Felipe Balbi, Wolfram Sang,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA, Vignesh R
In-Reply-To: <1419404619-10744-1-git-send-email-vigneshr-l0cyMroinI0@public.gmane.org>
From: Brad Griffis <bgriffis-l0cyMroinI0@public.gmane.org>
TSC interrupt handler had udelay to avoid reporting of false pen-up
interrupt to user space. This patch implements workaround suggesting in
Advisory 1.0.31 of silicon errata for am335x, thus eliminating udelay
and touchscreen lag. This also improves performance of touchscreen and
eliminates sudden jump of cursor at touch release.
IDLECONFIG and CHARGECONFIG registers are to be configured
with same values in order to eliminate false pen-up events. This
workaround may result in false pen-down to be detected, hence considerable
charge step delay needs to be added. The charge delay is set to 0xB000
(in terms of ADC clock cycles) by default.
TSC steps are disabled at the end of every sampling cycle and EOS bit is
set. Once the EOS bit is set, the TSC steps need to be re-enabled to begin
next sampling cycle.
Signed-off-by: Brad Griffis <bgriffis-l0cyMroinI0@public.gmane.org>
[vigneshr-l0cyMroinI0@public.gmane.org: Ported the patch from v3.12 to v3.19rc1]
Signed-off-by: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
---
v5:
- enable TSC steps only on EOS interrup
v4:
- check for PEN_IRQ bit in ADCFSM to avoid false-pen when ADC
and TSC are used together.
- Set charge delay to 0x400 as default. Most devices function
normally at this value.
drivers/input/touchscreen/ti_am335x_tsc.c | 67 ++++++++++++++-----------------
include/linux/mfd/ti_am335x_tscadc.h | 3 +-
2 files changed, 33 insertions(+), 37 deletions(-)
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index dfbb9fe6a270..8650b1dd24dc 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -173,11 +173,9 @@ static void titsc_step_config(struct titsc *ts_dev)
titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
}
- /* Charge step configuration */
- config = ts_dev->bit_xp | ts_dev->bit_yn |
- STEPCHARGE_RFP_XPUL | STEPCHARGE_RFM_XNUR |
- STEPCHARGE_INM_AN1 | STEPCHARGE_INP(ts_dev->inp_yp);
+ /* Make CHARGECONFIG same as IDLECONFIG */
+ config = titsc_readl(ts_dev, REG_IDLECONFIG);
titsc_writel(ts_dev, REG_CHARGECONFIG, config);
titsc_writel(ts_dev, REG_CHARGEDELAY, CHARGEDLY_OPENDLY);
@@ -261,12 +259,34 @@ static irqreturn_t titsc_irq(int irq, void *dev)
{
struct titsc *ts_dev = dev;
struct input_dev *input_dev = ts_dev->input;
- unsigned int status, irqclr = 0;
+ unsigned int fsm, status, irqclr = 0;
unsigned int x = 0, y = 0;
unsigned int z1, z2, z;
- unsigned int fsm;
- status = titsc_readl(ts_dev, REG_IRQSTATUS);
+ status = titsc_readl(ts_dev, REG_RAWIRQSTATUS);
+ if (status & IRQENB_HW_PEN) {
+ ts_dev->pen_down = true;
+ titsc_writel(ts_dev, REG_IRQWAKEUP, 0x00);
+ titsc_writel(ts_dev, REG_IRQCLR, IRQENB_HW_PEN);
+ irqclr |= IRQENB_HW_PEN;
+ }
+
+ if (status & IRQENB_PENUP) {
+ fsm = titsc_readl(ts_dev, REG_ADCFSM);
+ if (fsm == ADCFSM_STEPID) {
+ ts_dev->pen_down = false;
+ input_report_key(input_dev, BTN_TOUCH, 0);
+ input_report_abs(input_dev, ABS_PRESSURE, 0);
+ input_sync(input_dev);
+ } else {
+ ts_dev->pen_down = true;
+ }
+ irqclr |= IRQENB_PENUP;
+ }
+
+ if (status & IRQENB_EOS)
+ irqclr |= IRQENB_EOS;
+
/*
* ADC and touchscreen share the IRQ line.
* FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
@@ -297,37 +317,11 @@ static irqreturn_t titsc_irq(int irq, void *dev)
}
irqclr |= IRQENB_FIFO0THRES;
}
-
- /*
- * Time for sequencer to settle, to read
- * correct state of the sequencer.
- */
- udelay(SEQ_SETTLE);
-
- status = titsc_readl(ts_dev, REG_RAWIRQSTATUS);
- if (status & IRQENB_PENUP) {
- /* Pen up event */
- fsm = titsc_readl(ts_dev, REG_ADCFSM);
- if (fsm == ADCFSM_STEPID) {
- ts_dev->pen_down = false;
- input_report_key(input_dev, BTN_TOUCH, 0);
- input_report_abs(input_dev, ABS_PRESSURE, 0);
- input_sync(input_dev);
- } else {
- ts_dev->pen_down = true;
- }
- irqclr |= IRQENB_PENUP;
- }
-
- if (status & IRQENB_HW_PEN) {
-
- titsc_writel(ts_dev, REG_IRQWAKEUP, 0x00);
- titsc_writel(ts_dev, REG_IRQCLR, IRQENB_HW_PEN);
- }
-
if (irqclr) {
titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
- am335x_tsc_se_set_cache(ts_dev->mfd_tscadc, ts_dev->step_mask);
+ if (status & IRQENB_EOS)
+ am335x_tsc_se_set_cache(ts_dev->mfd_tscadc,
+ ts_dev->step_mask);
return IRQ_HANDLED;
}
return IRQ_NONE;
@@ -417,6 +411,7 @@ static int titsc_probe(struct platform_device *pdev)
}
titsc_writel(ts_dev, REG_IRQENABLE, IRQENB_FIFO0THRES);
+ titsc_writel(ts_dev, REG_IRQENABLE, IRQENB_EOS);
err = titsc_config_wires(ts_dev);
if (err) {
dev_err(&pdev->dev, "wrong i/p wire configuration\n");
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index e2e70053470e..3f4e994ace2b 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -52,6 +52,7 @@
/* IRQ enable */
#define IRQENB_HW_PEN BIT(0)
+#define IRQENB_EOS BIT(1)
#define IRQENB_FIFO0THRES BIT(2)
#define IRQENB_FIFO0OVRRUN BIT(3)
#define IRQENB_FIFO0UNDRFLW BIT(4)
@@ -107,7 +108,7 @@
/* Charge delay */
#define CHARGEDLY_OPEN_MASK (0x3FFFF << 0)
#define CHARGEDLY_OPEN(val) ((val) << 0)
-#define CHARGEDLY_OPENDLY CHARGEDLY_OPEN(1)
+#define CHARGEDLY_OPENDLY CHARGEDLY_OPEN(0x400)
/* Control register */
#define CNTRLREG_TSCSSENB BIT(0)
--
1.9.1
^ permalink raw reply related
* [PATCH v5 3/6] mfd: ti_am335x_tscadc: Remove unwanted reg_se_cache save
From: Vignesh R @ 2014-12-24 7:03 UTC (permalink / raw)
To: Jonathan Cameron, Hartmut Knaack, Dmitry Torokhov, Lee Jones,
Sebastian Andrzej Siewior
Cc: Mark Rutland, Vignesh R, Wolfram Sang, linux-iio, Paul Gortmaker,
Peter Meerwald, Lars-Peter Clausen, Russell King, Jan Kardell,
Tony Lindgren, linux-input, Sanjeev Sharma, devicetree,
Brad Griffis, Pawel Moll, Ian Campbell, Rob Herring, linux-omap,
linux-arm-kernel, Samuel Ortiz, linux-kernel, Felipe Balbi,
Benoit Cousson, Kumar Gala
In-Reply-To: <1419404619-10744-1-git-send-email-vigneshr@ti.com>
In one shot mode, sequencer automatically disables all enabled steps at
the end of each cycle. (both ADC steps and TSC steps) Hence these steps
need not be saved in reg_se_cache for clearing these steps at a later
stage.
Also, when ADC wakes up Sequencer should not be busy executing any of the
config steps except for the charge step. Previously charge step was 1 ADC
clock cycle and hence it was ignored.
TSC steps are always disabled at the end of each conversion cycle, hence
there is no need to explicitly disable TSC steps by writing 0 to REG_SE.
Signed-off-by: Vignesh R <vigneshr@ti.com>
---
v5:
- Remove unnecessary clearing of REG_SE
drivers/mfd/ti_am335x_tscadc.c | 13 +++++--------
include/linux/mfd/ti_am335x_tscadc.h | 1 +
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 467c80e1c4ae..e4e4b22eebc9 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -68,12 +68,6 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc)
DEFINE_WAIT(wait);
u32 reg;
- /*
- * disable TSC steps so it does not run while the ADC is using it. If
- * write 0 while it is running (it just started or was already running)
- * then it completes all steps that were enabled and stops then.
- */
- tscadc_writel(tsadc, REG_SE, 0);
reg = tscadc_readl(tsadc, REG_ADCFSM);
if (reg & SEQ_STATUS) {
tsadc->adc_waiting = true;
@@ -86,8 +80,12 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc)
spin_lock_irq(&tsadc->reg_lock);
finish_wait(&tsadc->reg_se_wait, &wait);
+ /*
+ * Sequencer should either be idle or
+ * busy applying the charge step.
+ */
reg = tscadc_readl(tsadc, REG_ADCFSM);
- WARN_ON(reg & SEQ_STATUS);
+ WARN_ON((reg & SEQ_STATUS) && !(reg & CHARGE_STEP));
tsadc->adc_waiting = false;
}
tsadc->adc_in_use = true;
@@ -96,7 +94,6 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc)
void am335x_tsc_se_set_once(struct ti_tscadc_dev *tsadc, u32 val)
{
spin_lock_irq(&tsadc->reg_lock);
- tsadc->reg_se_cache |= val;
am335x_tscadc_need_adc(tsadc);
tscadc_writel(tsadc, REG_SE, val);
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 3f4e994ace2b..1fd50dcfe47c 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -128,6 +128,7 @@
/* Sequencer Status */
#define SEQ_STATUS BIT(5)
+#define CHARGE_STEP 0x11
#define ADC_CLK 3000000
#define TOTAL_STEPS 16
--
1.9.1
^ permalink raw reply related
* [PATCH v5 4/6] ARM: dts: AM335x: Make charge delay a DT parameter for TSC
From: Vignesh R @ 2014-12-24 7:03 UTC (permalink / raw)
To: Jonathan Cameron, Hartmut Knaack, Dmitry Torokhov, Lee Jones,
Sebastian Andrzej Siewior
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Benoit Cousson, Tony Lindgren, Russell King, Lars-Peter Clausen,
Peter Meerwald, Samuel Ortiz, Brad Griffis, Paul Gortmaker,
Sanjeev Sharma, Jan Kardell, Felipe Balbi, Wolfram Sang,
devicetree, linux-kernel, linux-omap, linux-arm-kernel, linux-iio,
linux-input, Vignesh R
In-Reply-To: <1419404619-10744-1-git-send-email-vigneshr@ti.com>
The charge delay value is by default 0x400. But it can be set to lower
values on some boards, as long as false pen-ups are avoided. Lowering the
value increases the sampling rate (though current sampling rate is
sufficient for TSC operation). In some boards, the value has to be
increased to avoid false pen-up events. Hence, charge delay has been
made a DT parameter.
Signed-off-by: Vignesh R <vigneshr@ti.com>
---
v4:
- Set charge delay to 0x400 as default. Most devices function normally
at this value
.../devicetree/bindings/input/touchscreen/ti-tsc-adc.txt | 15 +++++++++++++++
arch/arm/boot/dts/am335x-evm.dts | 1 +
2 files changed, 16 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
index 878549ba814d..6df5028a4ad3 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
@@ -28,6 +28,20 @@ Required properties:
ti,adc-channels: List of analog inputs available for ADC.
AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
+Optional properties:
+- child "tsc"
+ ti,charge-delay: Length of touch screen charge delay step in terms of
+ ADC clock cycles. Charge delay value should be large
+ in order to avoid false pen-up events. This value
+ affects the overall sampling speed, hence need to be
+ kept as low as possible, while avoiding false pen-up
+ event. Start from a lower value, say 0x400, and
+ increase value until false pen-up events are avoided.
+ The pen-up detection happens immediately after the
+ charge step, so this does in fact function as a
+ hardware knob for adjusting the amount of "settling
+ time".
+
Example:
tscadc: tscadc@44e0d000 {
compatible = "ti,am3359-tscadc";
@@ -36,6 +50,7 @@ Example:
ti,x-plate-resistance = <200>;
ti,coordiante-readouts = <5>;
ti,wire-config = <0x00 0x11 0x22 0x33>;
+ ti,charge-delay = <0x400>;
};
adc {
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index 54f118c08db8..66342515df20 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -648,6 +648,7 @@
ti,x-plate-resistance = <200>;
ti,coordinate-readouts = <5>;
ti,wire-config = <0x00 0x11 0x22 0x33>;
+ ti,charge-delay = <0x400>;
};
adc {
--
1.9.1
^ permalink raw reply related
* [PATCH v5 5/6] input: touchscreen: ti_am335x_tsc: Use charge delay DT parameter
From: Vignesh R @ 2014-12-24 7:03 UTC (permalink / raw)
To: Jonathan Cameron, Hartmut Knaack, Dmitry Torokhov, Lee Jones,
Sebastian Andrzej Siewior
Cc: Mark Rutland, Vignesh R, Wolfram Sang, linux-iio, Paul Gortmaker,
Peter Meerwald, Lars-Peter Clausen, Russell King, Jan Kardell,
Tony Lindgren, linux-input, Sanjeev Sharma, devicetree,
Brad Griffis, Pawel Moll, Ian Campbell, Rob Herring, linux-omap,
linux-arm-kernel, Samuel Ortiz, linux-kernel, Felipe Balbi,
Benoit Cousson, Kumar Gala
In-Reply-To: <1419404619-10744-1-git-send-email-vigneshr@ti.com>
This patch reads charge delay from tsc DT node and writes to
REG_CHARGEDELAY register. If the charge delay is not specified in DT
then default value of 0x400(CHARGEDLY_OPENDLY) is used.
Signed-off-by: Vignesh R <vigneshr@ti.com>
---
v5:
- print out a warning when ti,charge-delay is not specified
drivers/input/touchscreen/ti_am335x_tsc.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 8650b1dd24dc..9cb8b4a01dca 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -52,6 +52,7 @@ struct titsc {
u32 bit_xp, bit_xn, bit_yp, bit_yn;
u32 inp_xp, inp_xn, inp_yp, inp_yn;
u32 step_mask;
+ u32 charge_delay;
};
static unsigned int titsc_readl(struct titsc *ts, unsigned int reg)
@@ -177,7 +178,7 @@ static void titsc_step_config(struct titsc *ts_dev)
config = titsc_readl(ts_dev, REG_IDLECONFIG);
titsc_writel(ts_dev, REG_CHARGECONFIG, config);
- titsc_writel(ts_dev, REG_CHARGEDELAY, CHARGEDLY_OPENDLY);
+ titsc_writel(ts_dev, REG_CHARGEDELAY, ts_dev->charge_delay);
/* coordinate_readouts + 1 ... coordinate_readouts + 2 is for Z */
config = STEPCONFIG_MODE_HWSYNC |
@@ -368,6 +369,17 @@ static int titsc_parse_dt(struct platform_device *pdev,
if (err < 0)
return err;
+ err = of_property_read_u32(node, "ti,charge-delay",
+ &ts_dev->charge_delay);
+ /*
+ * If ti,charge-delay value is not specified, then use CHARGEDLY_OPENDLY
+ * as the default value.
+ */
+ if (err < 0) {
+ ts_dev->charge_delay = CHARGEDLY_OPENDLY;
+ dev_warn(&pdev->dev, "ti,charge-delay not specified\n");
+ }
+
return of_property_read_u32_array(node, "ti,wire-config",
ts_dev->config_inp, ARRAY_SIZE(ts_dev->config_inp));
}
--
1.9.1
^ permalink raw reply related
* [PATCH v5 6/6] input: touchscreen: ti_am335x_tsc: Replace delta filtering with median filtering
From: Vignesh R @ 2014-12-24 7:03 UTC (permalink / raw)
To: Jonathan Cameron, Hartmut Knaack, Dmitry Torokhov, Lee Jones,
Sebastian Andrzej Siewior
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Benoit Cousson, Tony Lindgren, Russell King, Lars-Peter Clausen,
Peter Meerwald, Samuel Ortiz, Brad Griffis, Paul Gortmaker,
Sanjeev Sharma, Jan Kardell, Felipe Balbi, Wolfram Sang,
devicetree, linux-kernel, linux-omap, linux-arm-kernel, linux-iio,
linux-input, Vignesh R
In-Reply-To: <1419404619-10744-1-git-send-email-vigneshr@ti.com>
Previously, delta filtering was applied TSC co-ordinate readouts before
reporting a single value to user space. This patch replaces delta filtering
with median filtering. Median filtering sorts co-ordinate readouts, drops min
and max values, and reports the average of remaining values. This method is
more sensible than delta filering. Median filtering is applied only if number
of readouts is greater than 3 else just average of co-ordinate readouts is
reported.
Signed-off-by: Vignesh R <vigneshr@ti.com>
---
drivers/input/touchscreen/ti_am335x_tsc.c | 91 +++++++++++++++++--------------
1 file changed, 51 insertions(+), 40 deletions(-)
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 9cb8b4a01dca..504b85db6425 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -26,6 +26,7 @@
#include <linux/delay.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/sort.h>
#include <linux/mfd/ti_am335x_tscadc.h>
@@ -204,56 +205,61 @@ static void titsc_step_config(struct titsc *ts_dev)
am335x_tsc_se_set_cache(ts_dev->mfd_tscadc, ts_dev->step_mask);
}
+static int titsc_cmp_coord(const void *a, const void *b)
+{
+ return *(int *)a - *(int *)b;
+}
+
static void titsc_read_coordinates(struct titsc *ts_dev,
u32 *x, u32 *y, u32 *z1, u32 *z2)
{
- unsigned int fifocount = titsc_readl(ts_dev, REG_FIFO0CNT);
- unsigned int prev_val_x = ~0, prev_val_y = ~0;
- unsigned int prev_diff_x = ~0, prev_diff_y = ~0;
- unsigned int read, diff;
- unsigned int i, channel;
+ unsigned int yvals[7], xvals[7];
+ unsigned int i, xsum = 0, ysum = 0;
unsigned int creads = ts_dev->coordinate_readouts;
- unsigned int first_step = TOTAL_STEPS - (creads * 2 + 2);
- *z1 = *z2 = 0;
- if (fifocount % (creads * 2 + 2))
- fifocount -= fifocount % (creads * 2 + 2);
- /*
- * Delta filter is used to remove large variations in sampled
- * values from ADC. The filter tries to predict where the next
- * coordinate could be. This is done by taking a previous
- * coordinate and subtracting it form current one. Further the
- * algorithm compares the difference with that of a present value,
- * if true the value is reported to the sub system.
- */
- for (i = 0; i < fifocount; i++) {
- read = titsc_readl(ts_dev, REG_FIFO0);
-
- channel = (read & 0xf0000) >> 16;
- read &= 0xfff;
- if (channel > first_step + creads + 2) {
- diff = abs(read - prev_val_x);
- if (diff < prev_diff_x) {
- prev_diff_x = diff;
- *x = read;
- }
- prev_val_x = read;
+ for (i = 0; i < creads; i++) {
+ yvals[i] = titsc_readl(ts_dev, REG_FIFO0);
+ yvals[i] &= 0xfff;
+ }
- } else if (channel == first_step + creads + 1) {
- *z1 = read;
+ *z1 = titsc_readl(ts_dev, REG_FIFO0);
+ *z1 &= 0xfff;
+ *z2 = titsc_readl(ts_dev, REG_FIFO0);
+ *z2 &= 0xfff;
- } else if (channel == first_step + creads + 2) {
- *z2 = read;
+ for (i = 0; i < creads; i++) {
+ xvals[i] = titsc_readl(ts_dev, REG_FIFO0);
+ xvals[i] &= 0xfff;
+ }
- } else if (channel > first_step) {
- diff = abs(read - prev_val_y);
- if (diff < prev_diff_y) {
- prev_diff_y = diff;
- *y = read;
- }
- prev_val_y = read;
+ /*
+ * If co-ordinates readouts is less than 4 then
+ * report the average. In case of 4 or more
+ * readouts, sort the co-ordinate samples, drop
+ * min and max values and report the average of
+ * remaining values.
+ */
+ if (creads <= 3) {
+ for (i = 0; i < creads; i++) {
+ ysum += yvals[i];
+ xsum += xvals[i];
}
+ ysum /= creads;
+ xsum /= creads;
+ } else {
+ sort(yvals, creads, sizeof(unsigned int),
+ titsc_cmp_coord, NULL);
+ sort(xvals, creads, sizeof(unsigned int),
+ titsc_cmp_coord, NULL);
+ for (i = 1; i < creads - 1; i++) {
+ ysum += yvals[i];
+ xsum += xvals[i];
+ }
+ ysum /= creads - 2;
+ xsum /= creads - 2;
}
+ *y = ysum;
+ *x = xsum;
}
static irqreturn_t titsc_irq(int irq, void *dev)
@@ -369,6 +375,11 @@ static int titsc_parse_dt(struct platform_device *pdev,
if (err < 0)
return err;
+ if (ts_dev->coordinate_readouts <= 0) {
+ dev_warn(&pdev->dev, "invalid co-ordinate readouts, resetting it to 5\n");
+ ts_dev->coordinate_readouts = 5;
+ }
+
err = of_property_read_u32(node, "ti,charge-delay",
&ts_dev->charge_delay);
/*
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 1/7] input: alps: Set correct name of psmouse device in alps_init()
From: Pali Rohár @ 2014-12-24 7:48 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Hans de Goede, Vadim Klishko, linux-input, linux-kernel
In-Reply-To: <201412200953.38259@pali>
[-- Attachment #1: Type: Text/Plain, Size: 4764 bytes --]
On Saturday 20 December 2014 09:53:38 Pali Rohár wrote:
> On Tuesday 16 December 2014 12:58:20 Pali Rohár wrote:
> > On Tuesday 16 December 2014 06:02:34 Dmitry Torokhov wrote:
> > > Hi Pali,
> > >
> > > On Fri, Nov 14, 2014 at 08:38:20PM +0100, Pali Rohár wrote:
> > > > On some laptops after starting them from off state (not
> > > > after reboot), function alps_probe_trackstick_v3()
> > > > (called from function alps_identify()) does not detect
> > > > trackstick. To fix this problem we need to reset
> > > > device. But function alps_identify() is called also
> > > > from alps_detect() and we do not want to reset device
> > > > in detect function because it will slow down
> > > > initialization of all other non alps devices.
> > > >
> > > > This patch moves code for setting correct device name &
> > > > protocol from function alps_detect() to alps_init()
> > > > which already doing full device reset.
> > > >
> > > > So this patch removes need to do trackstick detection in
> > > > alps_detect() function.
> > > >
> > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > ---
> > > >
> > > > drivers/input/mouse/alps.c | 17 ++++++++++++++---
> > > > 1 file changed, 14 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/input/mouse/alps.c
> > > > b/drivers/input/mouse/alps.c index 8d85c79..9ffa98d
> > > > 100644 --- a/drivers/input/mouse/alps.c
> > > > +++ b/drivers/input/mouse/alps.c
> > > > @@ -2392,6 +2392,10 @@ int alps_init(struct psmouse
> > > > *psmouse)
> > > >
> > > > if (input_register_device(priv->dev2))
> > > >
> > > > goto init_fail;
> > > >
> > > > + if (!(priv->flags & ALPS_DUALPOINT))
> > > > + psmouse->name = "GlidePoint TouchPad";
> > > > + psmouse->model = priv->proto_version;
> > > > +
> > > >
> > > > psmouse->protocol_handler = alps_process_byte;
> > > > psmouse->poll = alps_poll;
> > > > psmouse->disconnect = alps_disconnect;
> > > >
> > > > @@ -2422,11 +2426,18 @@ int alps_detect(struct psmouse
> > > > *psmouse, bool set_properties)
> > > >
> > > > return -1;
> > > >
> > > > if (set_properties) {
> > > >
> > > > + /*
> > > > + * NOTE: To detect model and trackstick presence we
> >
> > need
> >
> > > > to do + * full device reset. To speed up
> >
> > detection
> >
> > > > and prevent + * calling duplicate
>
> initialization
>
> > > > sequence (both in + * alps_detect() and
> > > > alps_init()) we set model/protocol + * version
>
> and
>
> > > > correct name in alps_init() (which will + *
do
> >
> > full
> >
> > > > device reset). For now set name to DualPoint. + */
> > > >
> > > > psmouse->vendor = "ALPS";
> > > >
> > > > - psmouse->name = dummy.flags & ALPS_DUALPOINT ?
> > > > - "DualPoint TouchPad" : "GlidePoint";
> > > > - psmouse->model = dummy.proto_version << 8;
> > > > + psmouse->name = "DualPoint TouchPad";
> > > >
> > > > }
> > > >
> > > > +
> > >
> > > I do not quite like the way we change the device
> > > description back and forth. Do you think we could
> > > allocate the "real" priv structure in alps_detect() and
> > > have alps_init() expect to find it (and free it if
> > > set_properties is false). This way we'd go through
> > > initialization once in detect, it will be authoritative,
> > > and we would set the name of the device properly from the
> > > beginning.
> > >
> > > Thanks.
> >
> > No without introducing another psmouse_reset call. I want to
> > reduce time of loading driver, so I think this is better.
>
> Also psmouse-base.c call psmouse_reset between alps_detect()
> and alps_init(). We must do trackstick detection after
> psmouse_reset, but intorducing another psmouse_reset in
> alps_detect() will slow down initialization for ALPS devices.
> Also do not remember that psmouse_reset will lock both
> keyboard & mouse for one second (sometimes for two) on my
> laptop. I played with this and I think my patch is good
> approach how to do trackstick detection without slowdown or
> other side effects.
>
> Also we do not need to know if ALPS touchpad has trackstick
> device in alps_detect() phase. In alps_detect() we just need
> to know if PS/2 device is ALPS or not. From psmouse/serio
> point of view there is only one ALPS device on PS/2/i8042
> bus. Just alps.ko driver parse PS/2 packets and detect which
> packet has touchpad header and which trackpoint. So we really
> do not need to know if trackstick is there or not in
> alps_detect() function.
Dmitry ping, can you finally review this patch series? I it there
for more then month...
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH v8 0/6] mfd: AXP20x: Add support for AXP202 and AXP209
From: Carlo Caione @ 2014-12-24 11:00 UTC (permalink / raw)
To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
Cc: Lee Jones, Mark Brown, Maxime Ripard, Dmitry Torokhov,
Chen-Yu Tsai, linux-arm-kernel,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Carlo Caione
In-Reply-To: <1419303194-3075-1-git-send-email-wens-jdAy2FN1RRM@public.gmane.org>
On Tue, Dec 23, 2014 at 3:53 AM, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> Hi everyone,
Hi ChenYu,
Thank you very much for respinning this.
--
Carlo Caione
^ permalink raw reply
* Re: [Question : drivers/input ] Fixing Event Filter Mechanism in input subsystem
From: Dmitry Torokhov @ 2014-12-24 18:25 UTC (permalink / raw)
To: Anshul Garg; +Cc: linux-input, anshul.g@samsung.com
In-Reply-To: <CA+HOOsgDm-ZGycDwdzw3SvbscUefp7xcD-ikFSF2heouHMhS4g@mail.gmail.com>
Hi Anshul,
On Tue, Dec 23, 2014 at 08:04:45PM +0530, Anshul Garg wrote:
> Dear Mr Dmitry ,
>
> Thanks for the reply.
>
> I understand that if some handler grabs the input device then all
> events will sent to that handler only.
>
> My Concern is If No handler has grabbed the input device then all events
> should go to all handlers after application of all filter handlers on
> input event
> list ( As we have to check for each event whether that event can be filtered
> or not).
>
> For example : If 5 handlers registered for the input device and input device is
> not grabbed.
> Now among these 5 handlers 2 are filter handlers and remaining 3 are regular
> input handlers.
> So we have to filter the event list first after applying 2 filters
> then send the remaining
> events to all registered handler.
>
> In this case as per current implementation we pass the events array to
> each handler. Input Core does events filtering fr handler then send remaining
> events to handler.
>
> What i am proposing is first we have to pass input_value list to all
> filter handlers
> After filteration of events, we can send the remaining events (Some
> events might be removed after applying filter) to all handlers.
As far as I can see that is exactly what happens: we first pass the even list
to filters, which may cause the list to contract - see input_to_handler() - and
then to regular handlers.
I think the tricky part is the fact that we deliberately put filters in the
head of the dev->h_list, and normal handlers are put in the tail. And we also
expect the input_handler to either implement ->filter() or ->event[s]()
callback, but not both.
In case I still misunderstand what the issue you are trying to point out -
please do post your patch and we can discuss the code.
Thanks!
--
Dmitry
^ permalink raw reply
* Re: NULL pointer dereference in i2c-hid
From: Gabriele Mazzotta @ 2014-12-24 23:53 UTC (permalink / raw)
To: Andrew Duggan
Cc: Mika Westerberg, linux-input, linux-kernel, benjamin.tissoires,
jkosina
In-Reply-To: <548B3E32.2050503@synaptics.com>
On Friday 12 December 2014 11:12:50 Andrew Duggan wrote:
> On 12/12/2014 12:12 AM, Gabriele Mazzotta wrote:
> > On Thursday 11 December 2014 16:26:00 Andrew Duggan wrote:
> >> On 12/11/2014 01:57 PM, Gabriele Mazzotta wrote:
> >>> On Thursday 11 December 2014 13:34:02 Andrew Duggan wrote:
> >>>> On 12/11/2014 01:17 PM, Gabriele Mazzotta wrote:
> >>>>> On Thursday 11 December 2014 12:46:53 Andrew Duggan wrote:
> >>>>>> On 12/11/2014 11:40 AM, Gabriele Mazzotta wrote:
> >>>>>>> On Thursday 11 December 2014 11:21:43 Andrew Duggan wrote:
> >>>>>>>> On 12/11/2014 11:11 AM, Gabriele Mazzotta wrote:
> >>>>>>>>> On Thursday 11 December 2014 10:40:05 Andrew Duggan wrote:
> >>>>>>>>>> On 12/11/2014 10:16 AM, Gabriele Mazzotta wrote:
> >>>>>>>>>>> On Thursday 11 December 2014 16:03:07 Mika Westerberg wrote:
> >>>>>>>>>>>> On Thu, Dec 11, 2014 at 10:58:01AM +0200, Mika Westerberg wrote:
> >>>>>>>>>>>>> On Wed, Dec 10, 2014 at 06:04:51PM +0100, Gabriele Mazzotta wrote:
> >>>>>>>>>>>>>> my laptop uses a touchpad that needs hid-rmi along with i2c-hid to work.
> >>>>>>>>>>>>>> i2c-hid and hid-rmi can be loaded and unloaded independelty from each
> >>>>>>>>>>>>>> other, however since 34f439e4afcd ("HID: i2c-hid: add runtime PM support")
> >>>>>>>>>>>>>> if I unload hid-rmi and after it I also unload i2c-hid, I get a NULL
> >>>>>>>>>>>>>> pointer dereference.
> >>>>>>>>>>>>> I'll look into this.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I can reproduce this easily with i2c-hid + hid-multitouch following your
> >>>>>>>>>>>>> directions.
> >>>>>>>>>>>> Can you try the below patch?
> >>>>>>>>>>>>
> >>>>>>>>>>>> I think we shouldn't free buffers yet in ->stop() because we need the
> >>>>>>>>>>>> command buffer sending power commands to the device. Also it seems that
> >>>>>>>>>>>> ->start() re-allocates buffers anyway if maximum size increases.
> >>>>>>>>>>>>
> >>>>>>>>>>>> It shouldn't even leak memory as we release buffers at ->remove()
> >>>>>>>>>>>> anyway.
> >>>>>>>>>>>>
> >>>>>>>>>>>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> >>>>>>>>>>>> index 62cec01937ea..68a8c938feea 100644
> >>>>>>>>>>>> --- a/drivers/hid/i2c-hid/i2c-hid.c
> >>>>>>>>>>>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> >>>>>>>>>>>> @@ -705,12 +705,7 @@ static int i2c_hid_start(struct hid_device *hid)
> >>>>>>>>>>>>
> >>>>>>>>>>>> static void i2c_hid_stop(struct hid_device *hid)
> >>>>>>>>>>>> {
> >>>>>>>>>>>> - struct i2c_client *client = hid->driver_data;
> >>>>>>>>>>>> - struct i2c_hid *ihid = i2c_get_clientdata(client);
> >>>>>>>>>>>> -
> >>>>>>>>>>>> hid->claimed = 0;
> >>>>>>>>>>>> -
> >>>>>>>>>>>> - i2c_hid_free_buffers(ihid);
> >>>>>>>>>>>> }
> >>>>>>>>>>>>
> >>>>>>>>>>>> static int i2c_hid_open(struct hid_device *hid)
> >>>>>>>>>>> Yes, it works, thanks.
> >>>>>>>>>>>
> >>>>>>>>>>> This change seems to also prevent kernel ooops when I unload either
> >>>>>>>>>>> i2c-hid or i2c-designware-platform while the touchpad is in use,
> >>>>>>>>>>> thing that is likely to happen because of the other bug I reported.
> >>>>>>>>>>>
> >>>>>>>>>>> Speaking of it, does any of you have any suggestion on how to debug it?
> >>>>>>>>>> I was able to reproduce the initial issue by unloading hid-rmi and
> >>>>>>>>>> i2c-hid while holding my fingers on the touchpad. Mika's patch fixes it
> >>>>>>>>>> for me.
> >>>>>>>>>>
> >>>>>>>>>> For the original bug, you can modprobe i2c-hid debug=1 and we can see
> >>>>>>>>>> what data the touchpad is reporting. That might help narrowing down if
> >>>>>>>>>> it's noise which the touchpad thinks are fingers or if there is a
> >>>>>>>>>> problem with the I2C lines causing spurious interrupts.
> >>>>>>>>>>
> >>>>>>>>>> Andrew
> >>>>>>>>> I've already tried to do that and here what I got:
> >>>>>>>>>
> >>>>>>>>> When I release the finger, the last message is repeated 81 times.
> >>>>>>>>> If the byte containing informations about the width of the finger
> >>>>>>>>> becomes equal to either c0 or 0c at least once, the last message is
> >>>>>>>>> repeated indefinitely and changes as soon as I start using the touchpad.
> >>>>>>>>> The only way to stop it is to unload and reload i2c-hid.
> >>>>>>>> The reports before log throttling kicks in would still be useful. For
> >>>>>>>> instance c0 is outside of the range of finger width which we report so
> >>>>>>>> something is wrong there. But, the touchpad should stop interrupting
> >>>>>>>> once the finger is lifted. The fact that subsequent reads are reporting
> >>>>>>>> the same data does sound like a problem with I2C getting confused and
> >>>>>>>> continuously interrupting and reading the old finger data. I am also
> >>>>>>>> curious about the value of the byte after the report id.
> >>>>>>>>
> >>>>>>>> Andrew
> >>>>>>> If I'm not wrong c0 means that the width is 12 on y axis, while 0c means
> >>>>>>> that the width is 12 on the x axis.
> >>>>>> You are correct! I forgot width was 4 bits for X and Y in the same byte.
> >>>>>> That makes more sense.
> >>>>>>
> >>>>>>> I have to correct myself. The important thing is that the byte is either
> >>>>>>> cx or xc, where x is anything below c.
> >>>>>>>
> >>>>>>> Another correction. Sometimes unloading i2c-hid is not enough, I have to
> >>>>>>> first disable the touchpad with xinput and then unload i2c-hid. If I
> >>>>>>> don't do it, the messages starts reappearing as soon as I reload i2c-hid.
> >>>>>>>
> >>>>>>> I did several tests in the past months and I'm quite sure that the bug
> >>>>>>> happens only past xc/cy.
> >>>>>> This maybe coincidental. It is not obvious to me why a certain width
> >>>>>> value would cause the symptoms described.
> >>>>> Yes, I don't deny it. This is just a constant I found in my tests.
> >>>>>
> >>>>>>> Here few lines right before the bug. The last line is repeated indefinitely:
> >>>>>>>
> >>>>>>> [ 1983.527097] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 67 5b 61 0a 5f 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.537211] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 67 5b 63 0a 60 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.547329] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 67 5b 64 0a 60 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.557486] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 67 5b 66 0a 60 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.567663] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 67 5b 68 0a 60 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.577719] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 67 5b 6a 0a 61 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.587852] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 67 5b 6b 0a 61 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.598001] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 67 5b 6e 0a 62 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.608215] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 68 5b 61 0a 62 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.618288] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 68 5b 64 0b 63 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.628493] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 68 5b 67 0b 63 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.638552] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 68 5b 69 0b 64 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.648663] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 68 5b 6c 0b 64 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.658789] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 68 5b 6f 0b 64 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.668923] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 69 5b 61 0b 65 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.678819] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 69 5b 64 0b 65 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.689230] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 69 5b 66 0b 65 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.699435] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 69 5b 68 0b 65 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.709502] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 69 5b 6a 0b 66 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.719574] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 69 5b 6c 0b 66 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.729713] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 69 5b 6e 0b 66 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.739863] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6a 5b 60 0b 66 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.750001] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6a 5b 62 0b 66 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.760150] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 6a 5b 64 0b 67 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.770291] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6a 5b 66 0b 67 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.780445] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6a 5b 68 0b 67 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.790490] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6a 5b 5a 0b 68 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.800667] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 6a 5b 4e 0c 69 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.810691] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6b 5b 31 0c 69 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.820963] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6b 5b 24 0c 6a 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.831071] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6b 5b 17 0c 6a 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.841178] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6b 5b 09 0c 6a 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.851325] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6b 5b 0b 0c 6b 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.861435] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6b 5b 0d 0c 6b 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.871566] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 6b 5b 0f 0c 6b 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.881735] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6c 5b 01 0c 6b 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.891975] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6c 5b 03 0c 6b 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.902073] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6c 5b 05 0c 6c 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.912155] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6c 5b 07 0c 6b 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.922224] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6c 5b 09 0c 6c 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.932364] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6c 5b 0b 0c 6c 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.942480] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6c 5b 0d 0c 6c 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.952612] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 6c 5b 0f 0c 6c 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.962774] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6d 5a f1 0d 6d 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.972932] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6d 5a f3 0d 6e 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.982872] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6d 5a f6 0d 6f 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1983.993194] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 6d 5a f9 0d 6f 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.003295] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6d 5a fc 0d 6f 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.013511] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6d 5a ff 0d 70 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.023590] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6e 5a f2 0d 70 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.033747] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 6e 5a f5 0e 71 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.043850] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6e 5a f8 0e 71 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.053873] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6e 5a fb 0e 71 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.064077] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6e 5a fe 0e 72 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.074207] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6f 5a f1 0e 72 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.084425] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 6f 5a f3 0e 72 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.094533] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6f 5a f6 0e 73 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.104629] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6f 5a f8 0e 73 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.114742] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6f 5a fa 0e 73 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.124890] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6f 5a fc 0e 73 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.135006] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6f 5a fe 0e 73 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.145149] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 70 5a f0 0e 73 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.155317] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 70 5a e2 0e 73 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.165380] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 70 5a d4 0d 74 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.175532] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 70 5a d6 0d 74 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.185409] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 70 5a d8 0d 75 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.195761] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 70 5a db 0d 75 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.205909] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 70 5a dd 0d 75 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.216034] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 71 5a d0 0e 76 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.226198] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 71 5a d3 0e 77 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.236301] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 71 5a d6 0e 77 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.246520] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 71 5a cb 0f 79 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.256573] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 72 5a b1 0e 78 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.266697] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 72 5a a7 0e 77 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.276823] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 72 5a 6d 0d 72 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.286921] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 73 59 96 0a 5a 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.296888] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 00 00 73 59 96 00 00 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.306825] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 00 00 73 59 96 00 00 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>>> [ 1984.316980] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 00 00 73 59 96 00 00 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>>>> The last report which repeats indicates that no fingers are present.
> >>>>>> But, the click button is generating the interrupt and it is reporting
> >>>>>> that it is in the down position. Could the click button be getting stuck
> >>>>>> down? That would also explain why the data in the report is not
> >>>>>> changing. Could you be bumping the touchpad with your palm which is
> >>>>>> causing the click button to get stuck in the down position? That might
> >>>>>> also explain the large width value. Our max width is 15, so 12 would
> >>>>>> typically indicate a contact larger then a normal finger contact. That
> >>>>>> would explain the width of 12 showing up when this happens.
> >>>>> Yes, that's not a normal finger touch and the reported width seems
> >>>>> quite accurate to me. There are no clicks involved, laying the side of
> >>>>> my thumb gently on the top part of the clickpad (from where even voluntary
> >>>>> clicks are hard to obtain) is enough to trigger the bug. If a click is
> >>>>> reported, than something is really wrong.
> >>>> Do the reports look the same? With the repeated reports starting with
> >>>> "20 00 0c 04"?
> >>> I'm not sure I understood exactly what you want to know, so this might
> >>> not answer your questions.
> >>>
> >>> All the repeated messages look like this:
> >>>
> >>> 20 00 0c 04 00 00 27 57 fa 00 00 07 8a 0c 00 00 7e 87 eb 00 00 aa 8f 37 00 00 61 4d f1 00 00 00
> >>>
> >>> The values equal to zero are always equal to zero when there are no
> >>> fingers on the touchpad. The non-zero values are equal to the last
> >>> valid value recorded and don't change when the fingers are released.
> >>>
> >>> To get the above line I touched the touchpad with 5 fingers so that
> >>> all the bytes (except the last one which is different from zero on
> >>> click release) were used.
> >> Yes, that was the information I was looking for. But, this must not be
> >> my day since it turns out I misremembered what 0x4 stood for and this is
> >> unrelated to the click button. You can ignore everything I said about
> >> the click button =)
> >>
> >> The report you have above is actually a finger lift report which
> >> indicates that all of the fingers left the touchpad. I checked the
> >> firmware configuration for this touchpad and it will repeat the lift
> >> report probably about 40 times. This is the expected behavior. However,
> >> you reported earlier that the message had been repeated 81 times? That
> >> seems a bit high and it would not explain why it gets repeated indefinitely.
> >>
> >> You can try disabling the repeating finger lift reports and see if that
> >> helps. To do that you need to download a utility at:
> >> https://github.com/aduggan/rmi4utils
> >>
> >> After building it run:
> >> $ sudo ./rmihidtool -r 0x45 1 /dev/hidraw0
> >> This should print out 0x78 assuming the register address is what I
> >> expect. If not I can send further instructions on how to figure that out.
> >>
> >> To disable the repeating lift report run:
> >> $ sudo ./rmihidtool -w 0x45 0x38 /dev/hidraw0
> >>
> >> After you do that there should only be one finger lift report.
> > Yes, this disables the repeating lift report, but the bug is still there.
>
> Ok, I would have been surprised if this fixed it. But, at least it helps
> to narrow down the cause.
>
> >
> >> Also, if you can get the firmware id from your touchpad that would also
> >> be useful.
> >>
> >> $ sudo ./rmihidtool -f /dev/hidraw0
> > firmware id: 1522295
> Thanks, I will see if I can get any additional information on this.
>
> Andrew
Hi,
I think I found the source of the problem.
$ ./rmihidtool /dev/hidraw1 -r 0x50 1
0x01 #PalmDetect Interrupt Enable, right?
$ ./rmihidtool /dev/hidraw1 -w 0x50 0 #Disable PalmDetect Interrupt
It makes more sense now that widths greater than 12 trigger the bug.
Gabriele
^ permalink raw reply
* Re: [Question : drivers/input ] Fixing Event Filter Mechanism in input subsystem
From: Anshul Garg @ 2014-12-25 5:41 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, anshul.g@samsung.com
In-Reply-To: <20141224182511.GA21956@dtor-glaptop>
Dear Mr Dmitry ,
Thanks a lot for the clarification.
I was in assumption that one handler can support both ->filter() and
->event[s]()
Callback.So that's why i have prepared the patch to first do the
filter then pass
the events.
Can you please tell me why current implementation doesn't expect handler can
have both callbacks?
I think input core should be generic to allow any type of handlers which can
support both filter and events callbacks.
Please help to answer above query as my patch is based on this pre-assumption
that one handler can support both callbacks .
If we really need to have support of such handlers in input core then
only my patch
is good.
Hope to hear from you soon :)
Thanks
Anshul Garg
On Wed, Dec 24, 2014 at 11:55 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Anshul,
>
> On Tue, Dec 23, 2014 at 08:04:45PM +0530, Anshul Garg wrote:
>> Dear Mr Dmitry ,
>>
>> Thanks for the reply.
>>
>> I understand that if some handler grabs the input device then all
>> events will sent to that handler only.
>>
>> My Concern is If No handler has grabbed the input device then all events
>> should go to all handlers after application of all filter handlers on
>> input event
>> list ( As we have to check for each event whether that event can be filtered
>> or not).
>>
>> For example : If 5 handlers registered for the input device and input device is
>> not grabbed.
>> Now among these 5 handlers 2 are filter handlers and remaining 3 are regular
>> input handlers.
>> So we have to filter the event list first after applying 2 filters
>> then send the remaining
>> events to all registered handler.
>>
>> In this case as per current implementation we pass the events array to
>> each handler. Input Core does events filtering fr handler then send remaining
>> events to handler.
>>
>> What i am proposing is first we have to pass input_value list to all
>> filter handlers
>> After filteration of events, we can send the remaining events (Some
>> events might be removed after applying filter) to all handlers.
>
> As far as I can see that is exactly what happens: we first pass the even list
> to filters, which may cause the list to contract - see input_to_handler() - and
> then to regular handlers.
>
> I think the tricky part is the fact that we deliberately put filters in the
> head of the dev->h_list, and normal handlers are put in the tail. And we also
> expect the input_handler to either implement ->filter() or ->event[s]()
> callback, but not both.
>
> In case I still misunderstand what the issue you are trying to point out -
> please do post your patch and we can discuss the code.
>
> Thanks!
>
> --
> Dmitry
^ permalink raw reply
* [PATCH 4/13] incorrect use of init_completion fixup
From: Nicholas Mc Guire @ 2014-12-26 8:06 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Helge Deller, Paul Gortmaker, linux-input, linux-parisc,
linux-kernel, Nicholas Mc Guire
The successive init_completion calls should be reinit_completion calls.
patch against 3.19.0-rc1 linux-next
Acked-by: Helge Deller <deller@gmx.de>
Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
---
drivers/input/keyboard/hil_kbd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/input/keyboard/hil_kbd.c b/drivers/input/keyboard/hil_kbd.c
index 610a8af..5b152f2 100644
--- a/drivers/input/keyboard/hil_kbd.c
+++ b/drivers/input/keyboard/hil_kbd.c
@@ -473,7 +473,7 @@ static int hil_dev_connect(struct serio *serio, struct serio_driver *drv)
if (error)
goto bail1;
- init_completion(&dev->cmd_done);
+ reinit_completion(&dev->cmd_done);
serio_write(serio, 0);
serio_write(serio, 0);
serio_write(serio, HIL_PKT_CMD >> 8);
@@ -482,7 +482,7 @@ static int hil_dev_connect(struct serio *serio, struct serio_driver *drv)
if (error)
goto bail1;
- init_completion(&dev->cmd_done);
+ reinit_completion(&dev->cmd_done);
serio_write(serio, 0);
serio_write(serio, 0);
serio_write(serio, HIL_PKT_CMD >> 8);
@@ -491,7 +491,7 @@ static int hil_dev_connect(struct serio *serio, struct serio_driver *drv)
if (error)
goto bail1;
- init_completion(&dev->cmd_done);
+ reinit_completion(&dev->cmd_done);
serio_write(serio, 0);
serio_write(serio, 0);
serio_write(serio, HIL_PKT_CMD >> 8);
--
1.7.10.4
^ permalink raw reply related
* [PATCH 0/5] AM437x SK: Add power-button support
From: Felipe Balbi @ 2014-12-26 19:28 UTC (permalink / raw)
To: Tony Lindgren, Dmitry Torokhov, Samuel Ortiz, Lee Jones
Cc: Linux OMAP Mailing List, linux-input, devicetree,
Linux ARM Kernel Mailing List, Felipe Balbi
Hi,
the following patches add tps65218 power button support and
make it usable with AM437x SK.
Developed and tested on top of v3.19-rc1. Logs at [1]
[1] http://hastebin.com/ecopenuqot
Felipe Balbi (5):
mfd: tps65218: make INT[12] and STATUS registers volatile
mfd: tps65218: make INT1 our status_base register
input: misc: add tps65218 power button driver
arm: boot: dts: am437x-sk: add power button binding
arm: omap2plus_defconfig: enable TPS65218 power button
arch/arm/boot/dts/am437x-sk-evm.dts | 5 ++
arch/arm/configs/omap2plus_defconfig | 1 +
drivers/input/misc/Kconfig | 10 +++
drivers/input/misc/Makefile | 1 +
drivers/input/misc/tps65218-pwrbutton.c | 135 ++++++++++++++++++++++++++++++++
drivers/mfd/tps65218.c | 12 +++
6 files changed, 164 insertions(+)
create mode 100644 drivers/input/misc/tps65218-pwrbutton.c
--
2.2.0
^ permalink raw reply
* [PATCH 1/5] mfd: tps65218: make INT[12] and STATUS registers volatile
From: Felipe Balbi @ 2014-12-26 19:28 UTC (permalink / raw)
To: Tony Lindgren, Dmitry Torokhov, Samuel Ortiz, Lee Jones
Cc: Linux OMAP Mailing List, linux-input, devicetree,
Linux ARM Kernel Mailing List, Felipe Balbi, stable, Keerthy
In-Reply-To: <1419622104-25812-1-git-send-email-balbi@ti.com>
STATUS register can be modified by the HW, so we
should bypass cache because of that.
In the case of INT[12] registers, they are the ones
that actually clear the IRQ source at the time they
are read. If we rely on the cache for them, we will
never be able to clear the interrupt, which will cause
our IRQ line to be disabled due to IRQ throttling.
Fixes: 44b4dc6 mfd: tps65218: Add driver for the TPS65218 PMIC
Cc: <stable@vger.kernel.org> # v3.15+
Cc: Keerthy <j-keerthy@ti.com>
Cc: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
drivers/mfd/tps65218.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/mfd/tps65218.c b/drivers/mfd/tps65218.c
index 0d256cb..2243f75 100644
--- a/drivers/mfd/tps65218.c
+++ b/drivers/mfd/tps65218.c
@@ -125,10 +125,21 @@ int tps65218_clear_bits(struct tps65218 *tps, unsigned int reg,
}
EXPORT_SYMBOL_GPL(tps65218_clear_bits);
+static const struct regmap_range tps65218_yes_ranges[] = {
+ regmap_reg_range(TPS65218_REG_INT1, TPS65218_REG_INT2),
+ regmap_reg_range(TPS65218_REG_STATUS, TPS65218_REG_STATUS),
+};
+
+static const struct regmap_access_table tps65218_volatile_table = {
+ .yes_ranges = tps65218_yes_ranges,
+ .n_yes_ranges = ARRAY_SIZE(tps65218_yes_ranges),
+};
+
static struct regmap_config tps65218_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
.cache_type = REGCACHE_RBTREE,
+ .volatile_table = &tps65218_volatile_table,
};
static const struct regmap_irq tps65218_irqs[] = {
--
2.2.0
^ permalink raw reply related
* [PATCH 2/5] mfd: tps65218: make INT1 our status_base register
From: Felipe Balbi @ 2014-12-26 19:28 UTC (permalink / raw)
To: Tony Lindgren, Dmitry Torokhov, Samuel Ortiz, Lee Jones
Cc: Linux OMAP Mailing List, linux-input, devicetree,
Linux ARM Kernel Mailing List, Felipe Balbi, stable, Keerthy
In-Reply-To: <1419622104-25812-1-git-send-email-balbi@ti.com>
If we don't tell regmap-irq that our first status
register is at offset 1, it will try to read offset
zero, which is the chipid register.
Fixes: 44b4dc6 mfd: tps65218: Add driver for the TPS65218 PMIC
Cc: <stable@vger.kernel.org> # v3.15+
Cc: Keerthy <j-keerthy@ti.com>
Cc: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
drivers/mfd/tps65218.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mfd/tps65218.c b/drivers/mfd/tps65218.c
index 2243f75..d6b7643 100644
--- a/drivers/mfd/tps65218.c
+++ b/drivers/mfd/tps65218.c
@@ -204,6 +204,7 @@ static struct regmap_irq_chip tps65218_irq_chip = {
.num_regs = 2,
.mask_base = TPS65218_REG_INT_MASK1,
+ .status_base = TPS65218_REG_INT1,
};
static const struct of_device_id of_tps65218_match_table[] = {
--
2.2.0
^ permalink raw reply related
* [PATCH 3/5] input: misc: add tps65218 power button driver
From: Felipe Balbi @ 2014-12-26 19:28 UTC (permalink / raw)
To: Tony Lindgren, Dmitry Torokhov, Samuel Ortiz, Lee Jones
Cc: Linux OMAP Mailing List, linux-input, devicetree,
Linux ARM Kernel Mailing List, Felipe Balbi
In-Reply-To: <1419622104-25812-1-git-send-email-balbi@ti.com>
With this driver, we can report KEY_POWER on
AM437x SK. This patch has been tested with said
board.
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
drivers/input/misc/Kconfig | 10 +++
drivers/input/misc/Makefile | 1 +
drivers/input/misc/tps65218-pwrbutton.c | 135 ++++++++++++++++++++++++++++++++
3 files changed, 146 insertions(+)
create mode 100644 drivers/input/misc/tps65218-pwrbutton.c
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 23297ab..364cfb8 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -404,6 +404,16 @@ config INPUT_RETU_PWRBUTTON
To compile this driver as a module, choose M here. The module will
be called retu-pwrbutton.
+config INPUT_TPS65218_PWRBUTTON
+ tristate "TPS65218 Power button driver"
+ depends on MFD_TPS65218
+ help
+ Say Y here if you want to enable power buttong reporting for
+ the TPS65218 Power Management IC device.
+
+ To compile this driver as a module, choose M here. The module will
+ be called tps65218-pwrbutton.
+
config INPUT_TWL4030_PWRBUTTON
tristate "TWL4030 Power button Driver"
depends on TWL4030_CORE
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 19c7603..a923753 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_INPUT_SGI_BTNS) += sgi_btns.o
obj-$(CONFIG_INPUT_SIRFSOC_ONKEY) += sirfsoc-onkey.o
obj-$(CONFIG_INPUT_SOC_BUTTON_ARRAY) += soc_button_array.o
obj-$(CONFIG_INPUT_SPARCSPKR) += sparcspkr.o
+obj-$(CONFIG_INPUT_TPS65218_PWRBUTTON) += tps65218-pwrbutton.o
obj-$(CONFIG_INPUT_TWL4030_PWRBUTTON) += twl4030-pwrbutton.o
obj-$(CONFIG_INPUT_TWL4030_VIBRA) += twl4030-vibra.o
obj-$(CONFIG_INPUT_TWL6040_VIBRA) += twl6040-vibra.o
diff --git a/drivers/input/misc/tps65218-pwrbutton.c b/drivers/input/misc/tps65218-pwrbutton.c
new file mode 100644
index 0000000..a1f8a19
--- /dev/null
+++ b/drivers/input/misc/tps65218-pwrbutton.c
@@ -0,0 +1,135 @@
+/*
+ * Texas Instruments' TPS65218 Power Button Input Driver
+ *
+ * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com/
+ * Author: Felipe Balbi <balbi@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/tps65218.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct tps65218_pwrbutton {
+ struct device *dev;
+ struct tps65218 *tps;
+ struct input_dev *idev;
+ int irq;
+
+ unsigned int pressed:1;
+};
+
+static irqreturn_t tps65218_pwr_irq(int irq, void *_pwr)
+{
+ struct tps65218_pwrbutton *pwr = _pwr;
+ unsigned int reg;
+ int ret;
+
+ ret = tps65218_reg_read(pwr->tps, TPS65218_REG_STATUS, ®);
+ if (ret) {
+ dev_err(pwr->dev, "can't read register --> %d\n", ret);
+ goto out;
+ }
+
+ if (reg & TPS65218_STATUS_PB_STATE) {
+ input_report_key(pwr->idev, KEY_POWER, 1);
+ pm_wakeup_event(pwr->dev, 0);
+ pwr->pressed = true;
+ } else {
+ /*
+ * we don't want to report KEY_POWER releases on any of the
+ * other IRQ sources. Only when button was previously pressed
+ */
+ if (pwr->pressed) {
+ input_report_key(pwr->idev, KEY_POWER, 0);
+ pwr->pressed = false;
+ }
+ }
+
+ input_sync(pwr->idev);
+
+out:
+ return IRQ_HANDLED;
+}
+
+static int tps65218_pwron_probe(struct platform_device *pdev)
+{
+ struct tps65218 *tps = dev_get_drvdata(pdev->dev.parent);
+ struct device *dev = &pdev->dev;
+ struct tps65218_pwrbutton *pwr;
+ struct input_dev *idev;
+ int ret;
+
+ pwr = devm_kzalloc(dev, sizeof(*pwr), GFP_KERNEL);
+ if (!pwr)
+ return -ENOMEM;
+
+ idev = devm_input_allocate_device(dev);
+ if (!idev)
+ return -ENOMEM;
+
+ idev->name = "tps65218_pwrbutton";
+ idev->phys = "tps65218_pwrbutton/input0";
+ idev->dev.parent = dev;
+ idev->id.bustype = BUS_I2C;
+
+ input_set_capability(idev, EV_KEY, KEY_POWER);
+
+ pwr->tps = tps;
+ pwr->dev = dev;
+ pwr->idev = idev;
+ pwr->irq = platform_get_irq(pdev, 0);
+ platform_set_drvdata(pdev, pwr);
+ device_init_wakeup(dev, true);
+
+ ret = devm_request_threaded_irq(dev, pwr->irq, NULL, tps65218_pwr_irq,
+ IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+ dev_name(dev), pwr);
+ if (ret) {
+ dev_err(dev, "failed to request IRQ #%d --> %d\n",
+ pwr->irq, ret);
+ return ret;
+ }
+
+ ret = input_register_device(idev);
+ if (ret) {
+ dev_err(dev, "Can't register power button --> %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static struct of_device_id of_tps65218_pwr_match[] = {
+ { .compatible = "ti,tps65218-pwrbutton" },
+ { },
+};
+
+MODULE_DEVICE_TABLE(of, of_tps65218_pwr_match);
+
+static struct platform_driver tps65218_pwron_driver = {
+ .probe = tps65218_pwron_probe,
+ .driver = {
+ .name = "tps65218_pwrbutton",
+ .of_match_table = of_tps65218_pwr_match,
+ },
+};
+module_platform_driver(tps65218_pwron_driver);
+
+MODULE_DESCRIPTION("TPS65218 Power Button");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Felipe Balbi <balbi@ti.com>");
--
2.2.0
^ permalink raw reply related
* [PATCH 4/5] arm: boot: dts: am437x-sk: add power button binding
From: Felipe Balbi @ 2014-12-26 19:28 UTC (permalink / raw)
To: Tony Lindgren, Dmitry Torokhov, Samuel Ortiz, Lee Jones
Cc: Linux OMAP Mailing List, linux-input, devicetree,
Linux ARM Kernel Mailing List, Felipe Balbi
In-Reply-To: <1419622104-25812-1-git-send-email-balbi@ti.com>
Let this board report KEY_POWER so upper layers
can decide what to do when power button is pressed.
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
arch/arm/boot/dts/am437x-sk-evm.dts | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/am437x-sk-evm.dts b/arch/arm/boot/dts/am437x-sk-evm.dts
index 53bbfc9..c53840c 100644
--- a/arch/arm/boot/dts/am437x-sk-evm.dts
+++ b/arch/arm/boot/dts/am437x-sk-evm.dts
@@ -386,6 +386,11 @@
regulator-always-on;
};
+ power-button {
+ compatible = "ti,tps65218-pwrbutton";
+ status = "okay";
+ interrupts = <3 IRQ_TYPE_EDGE_BOTH>;
+ };
};
at24@50 {
--
2.2.0
^ permalink raw reply related
* [PATCH 5/5] arm: omap2plus_defconfig: enable TPS65218 power button
From: Felipe Balbi @ 2014-12-26 19:28 UTC (permalink / raw)
To: Tony Lindgren, Dmitry Torokhov, Samuel Ortiz, Lee Jones
Cc: Linux OMAP Mailing List, linux-input, devicetree,
Linux ARM Kernel Mailing List, Felipe Balbi
In-Reply-To: <1419622104-25812-1-git-send-email-balbi@ti.com>
Enable tps65218 power button driver by default as
a dynamically linked module so AM437x SK can report
power button presses.
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
arch/arm/configs/omap2plus_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/omap2plus_defconfig b/arch/arm/configs/omap2plus_defconfig
index c2c3a85..f4a4b2f 100644
--- a/arch/arm/configs/omap2plus_defconfig
+++ b/arch/arm/configs/omap2plus_defconfig
@@ -183,6 +183,7 @@ CONFIG_TOUCHSCREEN_EDT_FT5X06=m
CONFIG_TOUCHSCREEN_TSC2005=m
CONFIG_TOUCHSCREEN_TSC2007=m
CONFIG_INPUT_MISC=y
+CONFIG_INPUT_TPS65218_PWRBUTTON=m
CONFIG_INPUT_TWL4030_PWRBUTTON=y
# CONFIG_LEGACY_PTYS is not set
CONFIG_SERIAL_8250=y
--
2.2.0
^ permalink raw reply related
* Re: [PATCH 3/5] input: misc: add tps65218 power button driver
From: Felipe Balbi @ 2014-12-26 19:33 UTC (permalink / raw)
To: Felipe Balbi
Cc: Tony Lindgren, Dmitry Torokhov, Samuel Ortiz, Lee Jones,
Linux OMAP Mailing List, linux-input, devicetree,
Linux ARM Kernel Mailing List
In-Reply-To: <1419622104-25812-4-git-send-email-balbi@ti.com>
[-- Attachment #1: Type: text/plain, Size: 326 bytes --]
Hi,
On Fri, Dec 26, 2014 at 01:28:22PM -0600, Felipe Balbi wrote:
> +static struct of_device_id of_tps65218_pwr_match[] = {
> + { .compatible = "ti,tps65218-pwrbutton" },
forgot to document this compatible. I'll add it localy but still wait a
a few days before resending so people have time to review.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH] hid: Add a new id 0x501a for Genius MousePen i608X
From: Giedrius Statkevičius @ 2014-12-26 22:28 UTC (permalink / raw)
To: jkosina; +Cc: linux-input, linux-kernel, linux-usb, Giedrius Statkevičius
New Genius MousePen i608X devices have a new id 0x501a instead of the
old 0x5011 so add a new #define with "_2" appended and change required
places.
The remaining two checkpatch warnings about line length
being over 80 characters are present in the original files too and this
patch was made in the same style (no line break).
Just adding a new id and changing the required places should make the
new device work without any issues according to the bug report in the
following url.
This patch was made according to and fixes:
https://bugzilla.kernel.org/show_bug.cgi?id=67111
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
Only compile tested as I don't have this hardware to test it.
drivers/hid/hid-core.c | 1 +
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-kye.c | 4 ++++
drivers/hid/usbhid/hid-quirks.c | 1 +
4 files changed, 7 insertions(+)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index f2a4879..5f49a11 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1821,6 +1821,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_ERGO_525V) },
{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_EASYPEN_I405X) },
{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_MOUSEPEN_I608X) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_MOUSEPEN_I608X_2) },
{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_EASYPEN_M610X) },
{ HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
{ HID_USB_DEVICE(USB_VENDOR_ID_LCPOWER, USB_DEVICE_ID_LCPOWER_LC1000 ) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index ac20ba0..a96bb03 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -531,6 +531,7 @@
#define USB_DEVICE_ID_KYE_GPEN_560 0x5003
#define USB_DEVICE_ID_KYE_EASYPEN_I405X 0x5010
#define USB_DEVICE_ID_KYE_MOUSEPEN_I608X 0x5011
+#define USB_DEVICE_ID_KYE_MOUSEPEN_I608X_2 0x501a
#define USB_DEVICE_ID_KYE_EASYPEN_M610X 0x5013
#define USB_VENDOR_ID_LABTEC 0x1020
diff --git a/drivers/hid/hid-kye.c b/drivers/hid/hid-kye.c
index b92bf01..158fcf5 100644
--- a/drivers/hid/hid-kye.c
+++ b/drivers/hid/hid-kye.c
@@ -323,6 +323,7 @@ static __u8 *kye_report_fixup(struct hid_device *hdev, __u8 *rdesc,
}
break;
case USB_DEVICE_ID_KYE_MOUSEPEN_I608X:
+ case USB_DEVICE_ID_KYE_MOUSEPEN_I608X_2:
if (*rsize == MOUSEPEN_I608X_RDESC_ORIG_SIZE) {
rdesc = mousepen_i608x_rdesc_fixed;
*rsize = sizeof(mousepen_i608x_rdesc_fixed);
@@ -415,6 +416,7 @@ static int kye_probe(struct hid_device *hdev, const struct hid_device_id *id)
switch (id->product) {
case USB_DEVICE_ID_KYE_EASYPEN_I405X:
case USB_DEVICE_ID_KYE_MOUSEPEN_I608X:
+ case USB_DEVICE_ID_KYE_MOUSEPEN_I608X_2:
case USB_DEVICE_ID_KYE_EASYPEN_M610X:
ret = kye_tablet_enable(hdev);
if (ret) {
@@ -446,6 +448,8 @@ static const struct hid_device_id kye_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_KYE,
USB_DEVICE_ID_KYE_MOUSEPEN_I608X) },
{ HID_USB_DEVICE(USB_VENDOR_ID_KYE,
+ USB_DEVICE_ID_KYE_MOUSEPEN_I608X_2) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_KYE,
USB_DEVICE_ID_KYE_EASYPEN_M610X) },
{ HID_USB_DEVICE(USB_VENDOR_ID_KYE,
USB_DEVICE_ID_GENIUS_GILA_GAMING_MOUSE) },
diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
index dc89be9..b27b3d3 100644
--- a/drivers/hid/usbhid/hid-quirks.c
+++ b/drivers/hid/usbhid/hid-quirks.c
@@ -124,6 +124,7 @@ static const struct hid_blacklist {
{ USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_WIRELESS, HID_QUIRK_MULTI_INPUT },
{ USB_VENDOR_ID_SIGMA_MICRO, USB_DEVICE_ID_SIGMA_MICRO_KEYBOARD, HID_QUIRK_NO_INIT_REPORTS },
{ USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_MOUSEPEN_I608X, HID_QUIRK_MULTI_INPUT },
+ { USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_MOUSEPEN_I608X_2, HID_QUIRK_MULTI_INPUT },
{ USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_EASYPEN_M610X, HID_QUIRK_MULTI_INPUT },
{ USB_VENDOR_ID_NTRIG, USB_DEVICE_ID_NTRIG_DUOSENSE, HID_QUIRK_NO_INIT_REPORTS },
{ USB_VENDOR_ID_SEMICO, USB_DEVICE_ID_SEMICO_USB_KEYKOARD, HID_QUIRK_NO_INIT_REPORTS },
--
2.2.1
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH 3/5] input: misc: add tps65218 power button driver
From: Dmitry Torokhov @ 2014-12-26 23:41 UTC (permalink / raw)
To: Felipe Balbi
Cc: Tony Lindgren, Samuel Ortiz, Lee Jones, Linux OMAP Mailing List,
linux-input, devicetree, Linux ARM Kernel Mailing List
In-Reply-To: <1419622104-25812-4-git-send-email-balbi@ti.com>
Hi Felipe,
On Fri, Dec 26, 2014 at 01:28:22PM -0600, Felipe Balbi wrote:
> With this driver, we can report KEY_POWER on
> AM437x SK. This patch has been tested with said
> board.
>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
> drivers/input/misc/Kconfig | 10 +++
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/tps65218-pwrbutton.c | 135 ++++++++++++++++++++++++++++++++
> 3 files changed, 146 insertions(+)
> create mode 100644 drivers/input/misc/tps65218-pwrbutton.c
>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 23297ab..364cfb8 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -404,6 +404,16 @@ config INPUT_RETU_PWRBUTTON
> To compile this driver as a module, choose M here. The module will
> be called retu-pwrbutton.
>
> +config INPUT_TPS65218_PWRBUTTON
> + tristate "TPS65218 Power button driver"
> + depends on MFD_TPS65218
> + help
> + Say Y here if you want to enable power buttong reporting for
> + the TPS65218 Power Management IC device.
> +
> + To compile this driver as a module, choose M here. The module will
> + be called tps65218-pwrbutton.
> +
> config INPUT_TWL4030_PWRBUTTON
> tristate "TWL4030 Power button Driver"
> depends on TWL4030_CORE
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 19c7603..a923753 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -59,6 +59,7 @@ obj-$(CONFIG_INPUT_SGI_BTNS) += sgi_btns.o
> obj-$(CONFIG_INPUT_SIRFSOC_ONKEY) += sirfsoc-onkey.o
> obj-$(CONFIG_INPUT_SOC_BUTTON_ARRAY) += soc_button_array.o
> obj-$(CONFIG_INPUT_SPARCSPKR) += sparcspkr.o
> +obj-$(CONFIG_INPUT_TPS65218_PWRBUTTON) += tps65218-pwrbutton.o
> obj-$(CONFIG_INPUT_TWL4030_PWRBUTTON) += twl4030-pwrbutton.o
> obj-$(CONFIG_INPUT_TWL4030_VIBRA) += twl4030-vibra.o
> obj-$(CONFIG_INPUT_TWL6040_VIBRA) += twl6040-vibra.o
> diff --git a/drivers/input/misc/tps65218-pwrbutton.c b/drivers/input/misc/tps65218-pwrbutton.c
> new file mode 100644
> index 0000000..a1f8a19
> --- /dev/null
> +++ b/drivers/input/misc/tps65218-pwrbutton.c
> @@ -0,0 +1,135 @@
> +/*
> + * Texas Instruments' TPS65218 Power Button Input Driver
> + *
> + * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com/
> + * Author: Felipe Balbi <balbi@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/tps65218.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +struct tps65218_pwrbutton {
> + struct device *dev;
> + struct tps65218 *tps;
> + struct input_dev *idev;
> + int irq;
You do not need to store IRQ since you are not using it outside of
probe().
> +
> + unsigned int pressed:1;
You probably can remove 'pressed' as well and send events directly to
input core and rely on it to filter out duplicates so there won't be
release event without press event. If you prefer to keep it please use
bool instead of bit field.
> +};
> +
> +static irqreturn_t tps65218_pwr_irq(int irq, void *_pwr)
> +{
> + struct tps65218_pwrbutton *pwr = _pwr;
> + unsigned int reg;
> + int ret;
> +
> + ret = tps65218_reg_read(pwr->tps, TPS65218_REG_STATUS, ®);
> + if (ret) {
> + dev_err(pwr->dev, "can't read register --> %d\n", ret);
> + goto out;
> + }
> +
> + if (reg & TPS65218_STATUS_PB_STATE) {
> + input_report_key(pwr->idev, KEY_POWER, 1);
> + pm_wakeup_event(pwr->dev, 0);
> + pwr->pressed = true;
> + } else {
> + /*
> + * we don't want to report KEY_POWER releases on any of the
> + * other IRQ sources. Only when button was previously pressed
> + */
> + if (pwr->pressed) {
> + input_report_key(pwr->idev, KEY_POWER, 0);
> + pwr->pressed = false;
> + }
> + }
> +
> + input_sync(pwr->idev);
> +
> +out:
> + return IRQ_HANDLED;
> +}
> +
> +static int tps65218_pwron_probe(struct platform_device *pdev)
> +{
> + struct tps65218 *tps = dev_get_drvdata(pdev->dev.parent);
> + struct device *dev = &pdev->dev;
> + struct tps65218_pwrbutton *pwr;
> + struct input_dev *idev;
> + int ret;
> +
> + pwr = devm_kzalloc(dev, sizeof(*pwr), GFP_KERNEL);
> + if (!pwr)
> + return -ENOMEM;
> +
> + idev = devm_input_allocate_device(dev);
> + if (!idev)
> + return -ENOMEM;
> +
> + idev->name = "tps65218_pwrbutton";
> + idev->phys = "tps65218_pwrbutton/input0";
> + idev->dev.parent = dev;
> + idev->id.bustype = BUS_I2C;
> +
> + input_set_capability(idev, EV_KEY, KEY_POWER);
> +
> + pwr->tps = tps;
> + pwr->dev = dev;
> + pwr->idev = idev;
> + pwr->irq = platform_get_irq(pdev, 0);
> + platform_set_drvdata(pdev, pwr);
> + device_init_wakeup(dev, true);
> +
> + ret = devm_request_threaded_irq(dev, pwr->irq, NULL, tps65218_pwr_irq,
> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + dev_name(dev), pwr);
I am very fond of calling variables that hold error codes "error", Then
you have:
error = f();
if (error) {
/* handle error */
}
> + if (ret) {
> + dev_err(dev, "failed to request IRQ #%d --> %d\n",
> + pwr->irq, ret);
> + return ret;
> + }
> +
> + ret = input_register_device(idev);
> + if (ret) {
> + dev_err(dev, "Can't register power button --> %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static struct of_device_id of_tps65218_pwr_match[] = {
> + { .compatible = "ti,tps65218-pwrbutton" },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(of, of_tps65218_pwr_match);
> +
> +static struct platform_driver tps65218_pwron_driver = {
> + .probe = tps65218_pwron_probe,
> + .driver = {
> + .name = "tps65218_pwrbutton",
> + .of_match_table = of_tps65218_pwr_match,
> + },
> +};
> +module_platform_driver(tps65218_pwron_driver);
> +
> +MODULE_DESCRIPTION("TPS65218 Power Button");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Felipe Balbi <balbi@ti.com>");
> --
> 2.2.0
>
Thanks!
--
Dmitry
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox