* Re: [PATCH] Input: atmel_mxt_ts - update to devm_* API
From: Dmitry Torokhov @ 2013-09-16 15:06 UTC (permalink / raw)
To: Manish Badarkhe; +Cc: linux-input, linux-kernel, josh.wu
In-Reply-To: <1379343016-6354-1-git-send-email-badarkhe.manish@gmail.com>
Hi Manish,
On Mon, Sep 16, 2013 at 08:20:16PM +0530, Manish Badarkhe wrote:
> @@ -1264,10 +1261,8 @@ static int mxt_remove(struct i2c_client *client)
> struct mxt_data *data = i2c_get_clientdata(client);
>
> sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> - free_irq(data->irq, data);
> input_unregister_device(data->input_dev);
> kfree(data->object_table);
> - kfree(data);
YOu need to be _very_ careful when mixing devm_* and non-devm-managed
resources, especially when interrupt is involved. In this particular
case you are offloading freeing of the interrupt (and disabling it) to
devm, which will happen later, bu you are freeing object table
immediately. This may cause driver crash if interrupt comes while device
is being unbound.
That said, there is a large outstanding series to the driver from Nick
Dryer that I am trying to merge, you may want to coordinate this change
with him.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dan Carpenter @ 2013-09-16 15:05 UTC (permalink / raw)
To: KY Srinivasan
Cc: olaf@aepfle.de, gregkh@linuxfoundation.org, jasowang@redhat.com,
dmitry.torokhov@gmail.com, linux-kernel@vger.kernel.org,
vojtech@suse.cz, linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <3b5096d0190b4440a8e25afbd22ab72d@SN2PR03MB061.namprd03.prod.outlook.com>
On Mon, Sep 16, 2013 at 02:46:24PM +0000, KY Srinivasan wrote:
> > > + case VM_PKT_DATA_INBAND:
> > > + hv_kbd_on_receive(device, desc);
> >
> > This is the error handling I mentioned at the top. hv_kbd_on_receive()
> > doesn't take into consideration the amount of data we recieved, it
> > trusts the offset we recieved from the user. There is an out of bounds
> > read.
>
> What user are you referring to. The message is sent by the host - the user keystroke
> is normalized into a fixed size packet by the host and sent to the guest. We will parse this
> packet, based on the host specified layout here.
>
The user means the hypervisor, yes.
I don't want the hypervisor accessing outside of the buffer. It is
robustness issue. Just check the offset against "bytes_recvd". It's
not complicated.
If you have a different place where the guest does this then tell me
which function to look at.
regards,
dan carpenter
^ permalink raw reply
* [PATCH] Input: atmel_tscadcc - update to devm_* API
From: Manish Badarkhe @ 2013-09-16 14:52 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: josh.wu, dmitry.torokhov, badarkhe.manish
Update the code to use devm_* API so that driver core will manage
resources.
Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
---
This is just clean up of code to manage resources using "devm_"
funtions. Not tested on hardware.
:100644 100644 bddabc5... 4ddf97c... M drivers/input/touchscreen/atmel_tsadcc.c
drivers/input/touchscreen/atmel_tsadcc.c | 53 ++++++++----------------------
1 file changed, 13 insertions(+), 40 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_tsadcc.c b/drivers/input/touchscreen/atmel_tsadcc.c
index bddabc5..4ddf97c 100644
--- a/drivers/input/touchscreen/atmel_tsadcc.c
+++ b/drivers/input/touchscreen/atmel_tsadcc.c
@@ -197,53 +197,47 @@ static int atmel_tsadcc_probe(struct platform_device *pdev)
}
/* Allocate memory for device */
- ts_dev = kzalloc(sizeof(struct atmel_tsadcc), GFP_KERNEL);
+ ts_dev = devm_kzalloc(&pdev->dev, sizeof(struct atmel_tsadcc),
+ GFP_KERNEL);
if (!ts_dev) {
dev_err(&pdev->dev, "failed to allocate memory.\n");
return -ENOMEM;
}
platform_set_drvdata(pdev, ts_dev);
- input_dev = input_allocate_device();
+ input_dev = devm_input_allocate_device(&pdev->dev);
if (!input_dev) {
dev_err(&pdev->dev, "failed to allocate input device.\n");
err = -EBUSY;
- goto err_free_mem;
+ goto err_out;
}
ts_dev->irq = platform_get_irq(pdev, 0);
if (ts_dev->irq < 0) {
dev_err(&pdev->dev, "no irq ID is designated.\n");
err = -ENODEV;
- goto err_free_dev;
+ goto err_out;
}
- if (!request_mem_region(res->start, resource_size(res),
- "atmel tsadcc regs")) {
- dev_err(&pdev->dev, "resources is unavailable.\n");
- err = -EBUSY;
- goto err_free_dev;
- }
-
- tsc_base = ioremap(res->start, resource_size(res));
+ tsc_base = devm_ioremap_resource(&pdev->dev, res);
if (!tsc_base) {
dev_err(&pdev->dev, "failed to map registers.\n");
err = -ENOMEM;
- goto err_release_mem;
+ goto err_out;
}
- err = request_irq(ts_dev->irq, atmel_tsadcc_interrupt, 0,
- pdev->dev.driver->name, ts_dev);
+ err = devm_request_irq(&pdev->dev, ts_dev->irq, atmel_tsadcc_interrupt,
+ 0, pdev->dev.driver->name, ts_dev);
if (err) {
dev_err(&pdev->dev, "failed to allocate irq.\n");
- goto err_unmap_regs;
+ goto err_out;
}
- ts_dev->clk = clk_get(&pdev->dev, "tsc_clk");
+ ts_dev->clk = devm_clk_get(&pdev->dev, "tsc_clk");
if (IS_ERR(ts_dev->clk)) {
dev_err(&pdev->dev, "failed to get ts_clk\n");
err = PTR_ERR(ts_dev->clk);
- goto err_free_irq;
+ goto err_out;
}
ts_dev->input = input_dev;
@@ -309,37 +303,17 @@ static int atmel_tsadcc_probe(struct platform_device *pdev)
err_fail:
clk_disable(ts_dev->clk);
- clk_put(ts_dev->clk);
-err_free_irq:
- free_irq(ts_dev->irq, ts_dev);
-err_unmap_regs:
- iounmap(tsc_base);
-err_release_mem:
- release_mem_region(res->start, resource_size(res));
-err_free_dev:
- input_free_device(input_dev);
-err_free_mem:
- kfree(ts_dev);
+err_out:
return err;
}
static int atmel_tsadcc_remove(struct platform_device *pdev)
{
struct atmel_tsadcc *ts_dev = platform_get_drvdata(pdev);
- struct resource *res;
-
- free_irq(ts_dev->irq, ts_dev);
input_unregister_device(ts_dev->input);
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- iounmap(tsc_base);
- release_mem_region(res->start, resource_size(res));
-
clk_disable(ts_dev->clk);
- clk_put(ts_dev->clk);
-
- kfree(ts_dev);
return 0;
}
@@ -356,4 +330,3 @@ module_platform_driver(atmel_tsadcc_driver);
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("Atmel TouchScreen Driver");
MODULE_AUTHOR("Dan Liang <dan.liang@atmel.com>");
-
--
1.7.10.4
^ permalink raw reply related
* [PATCH] Input: atmel_mxt_ts - update to devm_* API
From: Manish Badarkhe @ 2013-09-16 14:50 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: josh.wu, dmitry.torokhov, badarkhe.manish
Update the code to use devm_* API so that driver core will manage
resources.
Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
---
This is just clean up of code to manage resources using "devm_"
funtions. Not tested on hardware.
:100644 100644 59aa240... 73c5ad0... M drivers/input/touchscreen/atmel_mxt_ts.c
drivers/input/touchscreen/atmel_mxt_ts.c | 27 +++++++++++----------------
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 59aa240..73c5ad0 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1139,12 +1139,12 @@ static int mxt_probe(struct i2c_client *client,
if (!pdata)
return -EINVAL;
- data = kzalloc(sizeof(struct mxt_data), GFP_KERNEL);
- input_dev = input_allocate_device();
+ data = devm_kzalloc(&client->dev, sizeof(struct mxt_data), GFP_KERNEL);
+ input_dev = devm_input_allocate_device(&client->dev);
if (!data || !input_dev) {
dev_err(&client->dev, "Failed to allocate memory\n");
error = -ENOMEM;
- goto err_free_mem;
+ goto err_out;
}
data->is_tp = pdata && pdata->is_tp;
@@ -1170,7 +1170,7 @@ static int mxt_probe(struct i2c_client *client,
error = mxt_initialize(data);
if (error)
- goto err_free_mem;
+ goto err_out;
__set_bit(EV_ABS, input_dev->evbit);
__set_bit(EV_KEY, input_dev->evbit);
@@ -1224,9 +1224,10 @@ static int mxt_probe(struct i2c_client *client,
input_set_drvdata(input_dev, data);
i2c_set_clientdata(client, data);
- error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
- pdata->irqflags | IRQF_ONESHOT,
- client->name, data);
+ error = devm_request_threaded_irq(&client->dev,
+ client->irq, NULL, mxt_interrupt,
+ pdata->irqflags | IRQF_ONESHOT,
+ client->name, data);
if (error) {
dev_err(&client->dev, "Failed to register interrupt\n");
goto err_free_object;
@@ -1234,11 +1235,11 @@ static int mxt_probe(struct i2c_client *client,
error = mxt_make_highchg(data);
if (error)
- goto err_free_irq;
+ goto err_free_object;
error = input_register_device(input_dev);
if (error)
- goto err_free_irq;
+ goto err_free_object;
error = sysfs_create_group(&client->dev.kobj, &mxt_attr_group);
if (error)
@@ -1249,13 +1250,9 @@ static int mxt_probe(struct i2c_client *client,
err_unregister_device:
input_unregister_device(input_dev);
input_dev = NULL;
-err_free_irq:
- free_irq(client->irq, data);
err_free_object:
kfree(data->object_table);
-err_free_mem:
- input_free_device(input_dev);
- kfree(data);
+err_out:
return error;
}
@@ -1264,10 +1261,8 @@ static int mxt_remove(struct i2c_client *client)
struct mxt_data *data = i2c_get_clientdata(client);
sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
- free_irq(data->irq, data);
input_unregister_device(data->input_dev);
kfree(data->object_table);
- kfree(data);
return 0;
}
--
1.7.10.4
^ permalink raw reply related
* RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: KY Srinivasan @ 2013-09-16 14:46 UTC (permalink / raw)
To: Dan Carpenter
Cc: olaf@aepfle.de, gregkh@linuxfoundation.org, jasowang@redhat.com,
dmitry.torokhov@gmail.com, linux-kernel@vger.kernel.org,
vojtech@suse.cz, linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <20130916082110.GN25896@mwanda>
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, September 16, 2013 1:21 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; linux-input@vger.kernel.org;
> dmitry.torokhov@gmail.com; vojtech@suse.cz; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> synthetic keyboard
>
> The main thing is that could you improve the error handling in
> hv_kbd_on_channel_callback() explained inline.
Thanks Dan. I will address all the relevant issues you have pointed out.
I have answered/responded to your comments in-line.
>
> On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> > Add a new driver to support synthetic keyboard. On the next generation
> > Hyper-V guest firmware, many legacy devices will not be emulated and this
> > driver will be required.
> >
> > I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> > details of the AT keyboard driver.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > ---
> > drivers/input/serio/Kconfig | 7 +
> > drivers/input/serio/Makefile | 1 +
> > drivers/input/serio/hyperv-keyboard.c | 379
> +++++++++++++++++++++++++++++++++
> > 3 files changed, 387 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/input/serio/hyperv-keyboard.c
> >
> > diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> > index 1e691a3..f3996e7 100644
> > --- a/drivers/input/serio/Kconfig
> > +++ b/drivers/input/serio/Kconfig
> > @@ -267,4 +267,11 @@ config SERIO_OLPC_APSP
> > To compile this driver as a module, choose M here: the module will
> > be called olpc_apsp.
> >
> > +config HYPERV_KEYBOARD
> > + tristate "Microsoft Synthetic Keyboard driver"
> > + depends on HYPERV
> > + default HYPERV
> > + help
> > + Select this option to enable the Hyper-V Keyboard driver.
> > +
> > endif
> > diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> > index 12298b1..815d874 100644
> > --- a/drivers/input/serio/Makefile
> > +++ b/drivers/input/serio/Makefile
> > @@ -28,3 +28,4 @@ obj-$(CONFIG_SERIO_ALTERA_PS2) += altera_ps2.o
> > 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
> > diff --git a/drivers/input/serio/hyperv-keyboard.c
> b/drivers/input/serio/hyperv-keyboard.c
> > new file mode 100644
> > index 0000000..0d4625f
> > --- /dev/null
> > +++ b/drivers/input/serio/hyperv-keyboard.c
> > @@ -0,0 +1,379 @@
> > +/*
> > + * Copyright (c) 2013, Microsoft Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; 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/module.h>
> > +#include <linux/device.h>
> > +#include <linux/completion.h>
> > +#include <linux/hyperv.h>
> > +#include <linux/serio.h>
> > +#include <linux/slab.h>
> > +
> > +
>
> Extra blank line.
>
> > +/*
> > + * Current version 1.0
> > + *
> > + */
> > +#define SYNTH_KBD_VERSION_MAJOR 1
> > +#define SYNTH_KBD_VERSION_MINOR 0
> > +#define SYNTH_KBD_VERSION (SYNTH_KBD_VERSION_MINOR |
> \
> > + (SYNTH_KBD_VERSION_MAJOR << 16))
> > +
> > +
> > +/*
> > + * Message types in the synthetic input protocol
> > + */
> > +enum synth_kbd_msg_type {
> > + SYNTH_KBD_PROTOCOL_REQUEST = 1,
> > + SYNTH_KBD_PROTOCOL_RESPONSE = 2,
> > + SYNTH_KBD_EVENT = 3,
> > + SYNTH_KBD_LED_INDICATORS = 4,
> > +};
> > +
> > +/*
> > + * Basic message structures.
> > + */
> > +struct synth_kbd_msg_hdr {
> > + enum synth_kbd_msg_type type;
> > +};
>
> Enum type is wrong here because sizeof(enum) is up to the compiler to
> decide.
>
> > +
> > +struct synth_kbd_msg {
> > + struct synth_kbd_msg_hdr header;
> > + char data[1]; /* Enclosed message */
> > +};
>
> You could use a zero size array instead.
>
> > +
> > +union synth_kbd_version {
> > + struct {
> > + u16 minor_version;
> > + u16 major_version;
> > + };
> > + u32 version;
> > +};
> > +
> > +/*
> > + * Protocol messages
> > + */
> > +struct synth_kbd_protocol_request {
> > + struct synth_kbd_msg_hdr header;
> > + union synth_kbd_version version_requested;
> > +};
> > +
> > +struct synth_kbd_protocol_response {
> > + struct synth_kbd_msg_hdr header;
> > + u32 accepted:1;
> > + u32 reserved:31;
> > +};
> > +
> > +struct synth_kbd_keystroke {
> > + struct synth_kbd_msg_hdr header;
> > + u16 make_code;
> > + u16 reserved0;
> > + u32 is_unicode:1;
> > + u32 is_break:1;
> > + u32 is_e0:1;
> > + u32 is_e1:1;
> > + u32 reserved:28;
> > +};
> > +
> > +
>
> Extra blank line.
>
> > +#define HK_MAXIMUM_MESSAGE_SIZE 256
> > +
> > +#define KBD_VSC_SEND_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> > +#define KBD_VSC_RECV_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> > +
> > +#define XTKBD_EMUL0 0xe0
> > +#define XTKBD_EMUL1 0xe1
> > +#define XTKBD_RELEASE 0x80
> > +
> > +
>
> Extra blank.
>
> > +/*
> > + * Represents a keyboard device
> > + */
> > +struct hv_kbd_dev {
> > + unsigned char keycode[256];
> > + struct hv_device *device;
> > + struct synth_kbd_protocol_request protocol_req;
> > + struct synth_kbd_protocol_response protocol_resp;
> > + /* Synchronize the request/response if needed */
> > + struct completion wait_event;
> > + struct serio *hv_serio;
> > +};
> > +
> > +
>
> Extra blank.
>
> > +static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
> > +{
> > + struct hv_kbd_dev *kbd_dev;
> > + struct serio *hv_serio;
> > +
> > + kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
> > +
>
> Spurious blank line.
>
> > + if (!kbd_dev)
> > + return NULL;
> > +
> > + hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
> > +
>
> Spurious blank.
>
> > + if (hv_serio == NULL) {
> > + kfree(kbd_dev);
> > + return NULL;
> > + }
> > +
> > + hv_serio->id.type = SERIO_8042_XL;
>
> Pointless tab before the '='.
>
> > + strlcpy(hv_serio->name, dev_name(&device->device),
> > + sizeof(hv_serio->name));
> > + strlcpy(hv_serio->phys, dev_name(&device->device),
> > + sizeof(hv_serio->phys));
> > + hv_serio->dev.parent = &device->device;
>
> Why are there two spaces before the '='?
>
> > +
> > +
>
> Extra blank line.
>
> > + kbd_dev->device = device;
> > + kbd_dev->hv_serio = hv_serio;
> > + hv_set_drvdata(device, kbd_dev);
> > + init_completion(&kbd_dev->wait_event);
> > +
> > + return kbd_dev;
> > +}
> > +
> > +static void hv_kbd_free_device(struct hv_kbd_dev *device)
> > +{
> > + serio_unregister_port(device->hv_serio);
> > + kfree(device->hv_serio);
> > + hv_set_drvdata(device->device, NULL);
> > + kfree(device);
> > +}
> > +
> > +static void hv_kbd_on_receive(struct hv_device *device,
> > + struct vmpacket_descriptor *packet)
> > +{
> > + struct synth_kbd_msg *msg;
> > + struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
> > + struct synth_kbd_keystroke *ks_msg;
> > + u16 scan_code;
> > +
> > + msg = (struct synth_kbd_msg *)((unsigned long)packet +
> > + (packet->offset8 << 3));
> > +
> > + switch (msg->header.type) {
> > + case SYNTH_KBD_PROTOCOL_RESPONSE:
> > + memcpy(&kbd_dev->protocol_resp, msg,
> > + sizeof(struct synth_kbd_protocol_response));
> > + complete(&kbd_dev->wait_event);
> > + break;
> > + case SYNTH_KBD_EVENT:
> > + ks_msg = (struct synth_kbd_keystroke *)msg;
> > + scan_code = ks_msg->make_code;
> > +
> > + /*
> > + * Inject the information through the serio interrupt.
> > + */
> > + if (ks_msg->is_e0)
> > + serio_interrupt(kbd_dev->hv_serio, XTKBD_EMUL0, 0);
> > + serio_interrupt(kbd_dev->hv_serio,
> > + scan_code | (ks_msg->is_break ?
> > + XTKBD_RELEASE : 0),
> > + 0);
> > +
>
> It would be more readable to do:
>
> if (ks_msg->is_break)
> scan_code |= XTKBD_RELEASE;
> serio_interrupt(kbd_dev->hv_serio, scan_code, 0);
>
>
> > + break;
> > +
> > + default:
> > + pr_err("unhandled message type %d\n", msg->header.type);
>
> Just a question. This can only be triggered by the hyperviser, right?
Yes.
>
> > + }
> > +}
> > +
> > +static void hv_kbd_on_channel_callback(void *context)
> > +{
> > + const int packet_size = 0x100;
> > + int ret;
> > + struct hv_device *device = context;
> > + u32 bytes_recvd;
> > + u64 req_id;
> > + struct vmpacket_descriptor *desc;
> > + unsigned char *buffer;
> > + int bufferlen = packet_size;
> > +
> > + buffer = kmalloc(bufferlen, GFP_ATOMIC);
> > + if (!buffer)
> > + return;
> > +
> > + do {
>
>
> Forever loops should be while (1) { instead of do { } while (1).
>
>
> > + ret = vmbus_recvpacket_raw(device->channel, buffer,
> > + bufferlen, &bytes_recvd, &req_id);
> > +
> > + switch (ret) {
> > + case 0:
> > + if (bytes_recvd <= 0) {
>
> There can never be a negative number of bytes_recvd.
>
> > + kfree(buffer);
> > + return;
> > + }
> > + desc = (struct vmpacket_descriptor *)buffer;
> > +
> > + switch (desc->type) {
> > + case VM_PKT_COMP:
> > + break;
> > +
> > + case VM_PKT_DATA_INBAND:
> > + hv_kbd_on_receive(device, desc);
>
> This is the error handling I mentioned at the top. hv_kbd_on_receive()
> doesn't take into consideration the amount of data we recieved, it
> trusts the offset we recieved from the user. There is an out of bounds
> read.
What user are you referring to. The message is sent by the host - the user keystroke
is normalized into a fixed size packet by the host and sent to the guest. We will parse this
packet, based on the host specified layout here.
>
> > + break;
> > +
> > + default:
> > + pr_err("unhandled packet type %d, tid %llx len
> %d\n",
> > + desc->type, req_id, bytes_recvd);
> > + break;
> > + }
> > +
> > + break;
> > +
> > + case -ENOBUFS:
> > + kfree(buffer);
> > + /* Handle large packet */
> > + bufferlen = bytes_recvd;
> > + buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
> > +
> > + if (!buffer)
> > + return;
> > +
> > + break;
> > + }
> > + } while (1);
> > +
> > +}
> > +
> > +static int hv_kbd_connect_to_vsp(struct hv_device *device)
> > +{
> > + int ret = 0;
>
> Don't initialize this.
>
> > + int t;
> > + struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
> > + struct synth_kbd_protocol_request *request;
> > + struct synth_kbd_protocol_response *response;
> > +
> > + request = &kbd_dev->protocol_req;
> > + memset(request, 0, sizeof(struct synth_kbd_protocol_request));
> > +
> > + request->header.type = SYNTH_KBD_PROTOCOL_REQUEST;
> > + request->version_requested.version = SYNTH_KBD_VERSION;
> > +
> > + ret = vmbus_sendpacket(device->channel, request,
> > + sizeof(struct synth_kbd_protocol_request),
> > + (unsigned long)request,
> > + VM_PKT_DATA_INBAND,
> > +
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > + if (ret)
> > + goto cleanup;
>
> There is no cleanup. Just return directly.
>
> > +
> > + t = wait_for_completion_timeout(&kbd_dev->wait_event, 10 * HZ);
> > + if (!t) {
> > + ret = -ETIMEDOUT;
> > + goto cleanup;
>
> return -ETIMEOUT;
>
> > + }
> > +
> > + response = &kbd_dev->protocol_resp;
> > +
> > + if (!response->accepted) {
> > + pr_err("synth_kbd protocol request failed (version %d)\n",
> > + SYNTH_KBD_VERSION);
> > + ret = -ENODEV;
> > + goto cleanup;
>
> Just return -ENODEV;
>
> > + }
> > +
>
> return 0;
>
> > +
> > +cleanup:
> > + return ret;
> > +}
> > +
> > +static int hv_kbd_probe(struct hv_device *device,
> > + const struct hv_vmbus_device_id *dev_id)
> > +{
> > + int ret;
> > + struct hv_kbd_dev *kbd_dev;
> > +
> > + kbd_dev = hv_kbd_alloc_device(device);
> > +
>
> Delete the blank line.
>
> > + if (!kbd_dev)
> > + return -ENOMEM;
> > +
> > + ret = vmbus_open(device->channel,
> > + KBD_VSC_SEND_RING_BUFFER_SIZE,
> > + KBD_VSC_RECV_RING_BUFFER_SIZE,
> > + NULL,
> > + 0,
> > + hv_kbd_on_channel_callback,
> > + device
> > + );
> > +
>
> Delete the blank line.
>
> > + if (ret)
> > + goto probe_err0;
> > +
> > + ret = hv_kbd_connect_to_vsp(device);
> > +
>
> Delete the blank line.
>
> > + if (ret)
> > + goto probe_err1;
> > +
> > + serio_register_port(kbd_dev->hv_serio);
> > +
> > + return ret;
>
> return 0;
>
> > +
> > +probe_err1:
> > + vmbus_close(device->channel);
>
> The label here should be "err_close:". The number is more GW-Basic
> style than C.
>
> > +
> > +probe_err0:
>
> The label should be "err_free_dev:".
>
> > + hv_kbd_free_device(kbd_dev);
> > +
> > + return ret;
> > +}
> > +
> > +
>
> Extra blank line.
>
> > +static int hv_kbd_remove(struct hv_device *dev)
>
> regards,
> dan carpenter
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Jürgen Beisert @ 2013-09-16 14:34 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Jonathan Cameron, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
fabio.estevam-KZfg59tc24xl57MIdRCFDg,
jic23-KWPb1pKIrIJaa/9Udqfwiw, Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Otavio Salvador
In-Reply-To: <201309161623.48247.marex-ynQEQJNshbs@public.gmane.org>
Hi Marek,
On Monday 16 September 2013 16:23:48 Marek Vasut wrote:
> > On Sunday 15 September 2013 12:56:25 Jonathan Cameron wrote:
> > > On 09/11/13 09:18, Juergen Beisert wrote:
> > > > For battery driven systems it is a very bad idea to collect the
> > > > touchscreen data within a kernel busy loop.
> > > >
> > > > This change uses the features of the hardware to delay and accumulate
> > > > samples in hardware to avoid a high interrupt and CPU load.
> > > >
> > > > Note: this is only tested on an i.MX23 SoC yet.
> > > >
> > > > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > > > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> > > > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > > > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > > > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> > >
> > > While this driver is placed in IIO within staging at the moment, these
> > > changes are definitely input related. Hence I have cc'd Dmitry and the
> > > input list.
> > >
> > > I am personaly a little uncomfortable that we have such a complex bit
> > > of input code sat within an IIO driver but such is life.
> >
> > Maybe an MFD for this ADC unit would be a better way to go? Currently I
> > have a different problem with this driver, because the ADC unit monitors
> > the battery as well. And the charging driver from the power subsystem
> > needs these values to charge the battery in a correct manner.
>
> Are you planning to post the power block patches too ?
When they will work: yes. Currently it crashes all the time. The PMIC is a
beast...
Regards,
Juergen
--
Pengutronix e.K. | Juergen Beisert |
Linux Solutions for Science and Industry | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Marek Vasut @ 2013-09-16 14:23 UTC (permalink / raw)
To: Jürgen Beisert
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Jonathan Cameron, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
fabio.estevam-KZfg59tc24xl57MIdRCFDg,
jic23-KWPb1pKIrIJaa/9Udqfwiw, Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Otavio Salvador
In-Reply-To: <201309161010.22151.jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Dear Jürgen Beisert,
> Hi Jonathan,
>
> On Sunday 15 September 2013 12:56:25 Jonathan Cameron wrote:
> > On 09/11/13 09:18, Juergen Beisert wrote:
> > > For battery driven systems it is a very bad idea to collect the
> > > touchscreen data within a kernel busy loop.
> > >
> > > This change uses the features of the hardware to delay and accumulate
> > > samples in hardware to avoid a high interrupt and CPU load.
> > >
> > > Note: this is only tested on an i.MX23 SoC yet.
> > >
> > > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> > > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> >
> > While this driver is placed in IIO within staging at the moment, these
> > changes are definitely input related. Hence I have cc'd Dmitry and the
> > input list.
> >
> > I am personaly a little uncomfortable that we have such a complex bit of
> > input code sat within an IIO driver but such is life.
>
> Maybe an MFD for this ADC unit would be a better way to go? Currently I
> have a different problem with this driver, because the ADC unit monitors
> the battery as well. And the charging driver from the power subsystem
> needs these values to charge the battery in a correct manner.
Are you planning to post the power block patches too ?
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [rtc-linux] Re: [PATCH 3/3] HID RTC: Open sensor hub open close
From: Alessandro Zummo @ 2013-09-16 13:07 UTC (permalink / raw)
To: rtc-linux-/JYPxA39Uh5TLH3MbocFFw
Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A, Srinivas Pandruvada,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, jc23-KWPb1pKIrIJaa/9Udqfwiw,
jkosina-AlSwsSmVLrQ, holler-SXC+2es9fhnfWeYVQQPykw
In-Reply-To: <52358980.4030000-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On Sun, 15 Sep 2013 11:18:40 +0100
Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> cc'd Alessandro and rtc list as this is within RTC. I'll happilly take
> it through IIO given the dependency, but I would like an Ack from the RTC
> side.
here you go!
Acked-by: Alessandro Zummo <a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org>
--
Best regards,
Alessandro Zummo,
Tower Technologies - Torino, Italy
http://www.towertech.it
^ permalink raw reply
* Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Jürgen Beisert @ 2013-09-16 9:37 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, marex-ynQEQJNshbs,
fabio.estevam-KZfg59tc24xl57MIdRCFDg,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Torokhov,
jic23-KWPb1pKIrIJaa/9Udqfwiw,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Jonathan Cameron
In-Reply-To: <201309161010.22151.jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Hi Jonathan,
On Monday 16 September 2013 10:10:22 Jürgen Beisert wrote:
> [...]
> > While this driver is placed in IIO within staging at the moment, these
> > changes are definitely input related. Hence I have cc'd Dmitry and the
> > input list.
> >
> > I am personaly a little uncomfortable that we have such a complex bit of
> > input code sat within an IIO driver but such is life.
>
> Maybe an MFD for this ADC unit would be a better way to go? Currently I
> have a different problem with this driver, because the ADC unit monitors
> the battery as well. And the charging driver from the power subsystem needs
> these values to charge the battery in a correct manner.
I would suggest:
diff --git a/drivers/staging/iio/TODO b/drivers/staging/iio/TODO
index 04c2326..c22a0ed 100644
--- a/drivers/staging/iio/TODO
+++ b/drivers/staging/iio/TODO
@@ -13,6 +13,17 @@ Would be nice
3) Expand device set. Lots of other maxim adc's have very
similar interfaces.
+MXS LRADC driver:
+This is a classic MFD device as it combines the following subdevices
+ - touchscreen controller (input subsystem related device)
+ - general purpose ADC channels
+ - battery voltage monitor (power subsystem related device)
+ - die temperature monitor (thermal management)
+
+At least the battery voltage and die temperature feature is required in-kernel
+by a driver of the SoC's battery charging unit to avoid any damage to the
+silicon and the battery.
+
TSL2561
Would be nice
1) Open question of userspace vs kernel space balance when
Regards,
Juergen
--
Pengutronix e.K. | Juergen Beisert |
Linux Solutions for Science and Industry | http://www.pengutronix.de/ |
^ permalink raw reply related
* Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Jürgen Beisert @ 2013-09-16 8:38 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Jonathan Cameron, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, marex-ynQEQJNshbs,
fabio.estevam-KZfg59tc24xl57MIdRCFDg, Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <5235DBEA.7030402-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Hi Jonathan,
On Sunday 15 September 2013 18:10:18 Jonathan Cameron wrote:
> On 09/15/13 11:56, Jonathan Cameron wrote:
> > On 09/11/13 09:18, Juergen Beisert wrote:
> >> For battery driven systems it is a very bad idea to collect the
> >> touchscreen data within a kernel busy loop.
> >>
> >> This change uses the features of the hardware to delay and accumulate
> >> samples in hardware to avoid a high interrupt and CPU load.
> >>
> >> Note: this is only tested on an i.MX23 SoC yet.
> >>
> >> Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> >> CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> >> CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> >> CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >> CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> >
> > While this driver is placed in IIO within staging at the moment, these
> > changes are definitely input related. Hence I have cc'd Dmitry and the
> > input list.
> >
> > I am personaly a little uncomfortable that we have such a complex bit of
> > input code sat within an IIO driver but such is life.
>
> The logic in here looks reasonable to me. I am far from a specialist in how
> these touch screens are normally handled though.
>
> One thing to note is that you really want to get a proposed device tree
> spec out asap as that can take longer to review than the driver. If you
> are proposing to do that as a future patch, then take into account that
> you'll need to ensure these are the defaults if it is not specified in the
> device tree for ever more (which is more painful than hammering out he
> device tree stuff now!)
> ...
Will do.
Regards,
Juergen
--
Pengutronix e.K. | Juergen Beisert |
Linux Solutions for Science and Industry | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dan Carpenter @ 2013-09-16 8:21 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: olaf, gregkh, jasowang, dmitry.torokhov, linux-kernel, vojtech,
linux-input, apw, devel
In-Reply-To: <1379309334-25042-1-git-send-email-kys@microsoft.com>
The main thing is that could you improve the error handling in
hv_kbd_on_channel_callback() explained inline.
On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> Add a new driver to support synthetic keyboard. On the next generation
> Hyper-V guest firmware, many legacy devices will not be emulated and this
> driver will be required.
>
> I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> details of the AT keyboard driver.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
> drivers/input/serio/Kconfig | 7 +
> drivers/input/serio/Makefile | 1 +
> drivers/input/serio/hyperv-keyboard.c | 379 +++++++++++++++++++++++++++++++++
> 3 files changed, 387 insertions(+), 0 deletions(-)
> create mode 100644 drivers/input/serio/hyperv-keyboard.c
>
> diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> index 1e691a3..f3996e7 100644
> --- a/drivers/input/serio/Kconfig
> +++ b/drivers/input/serio/Kconfig
> @@ -267,4 +267,11 @@ config SERIO_OLPC_APSP
> To compile this driver as a module, choose M here: the module will
> be called olpc_apsp.
>
> +config HYPERV_KEYBOARD
> + tristate "Microsoft Synthetic Keyboard driver"
> + depends on HYPERV
> + default HYPERV
> + help
> + Select this option to enable the Hyper-V Keyboard driver.
> +
> endif
> diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> index 12298b1..815d874 100644
> --- a/drivers/input/serio/Makefile
> +++ b/drivers/input/serio/Makefile
> @@ -28,3 +28,4 @@ obj-$(CONFIG_SERIO_ALTERA_PS2) += altera_ps2.o
> 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
> diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
> new file mode 100644
> index 0000000..0d4625f
> --- /dev/null
> +++ b/drivers/input/serio/hyperv-keyboard.c
> @@ -0,0 +1,379 @@
> +/*
> + * Copyright (c) 2013, Microsoft Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; 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/module.h>
> +#include <linux/device.h>
> +#include <linux/completion.h>
> +#include <linux/hyperv.h>
> +#include <linux/serio.h>
> +#include <linux/slab.h>
> +
> +
Extra blank line.
> +/*
> + * Current version 1.0
> + *
> + */
> +#define SYNTH_KBD_VERSION_MAJOR 1
> +#define SYNTH_KBD_VERSION_MINOR 0
> +#define SYNTH_KBD_VERSION (SYNTH_KBD_VERSION_MINOR | \
> + (SYNTH_KBD_VERSION_MAJOR << 16))
> +
> +
> +/*
> + * Message types in the synthetic input protocol
> + */
> +enum synth_kbd_msg_type {
> + SYNTH_KBD_PROTOCOL_REQUEST = 1,
> + SYNTH_KBD_PROTOCOL_RESPONSE = 2,
> + SYNTH_KBD_EVENT = 3,
> + SYNTH_KBD_LED_INDICATORS = 4,
> +};
> +
> +/*
> + * Basic message structures.
> + */
> +struct synth_kbd_msg_hdr {
> + enum synth_kbd_msg_type type;
> +};
Enum type is wrong here because sizeof(enum) is up to the compiler to
decide.
> +
> +struct synth_kbd_msg {
> + struct synth_kbd_msg_hdr header;
> + char data[1]; /* Enclosed message */
> +};
You could use a zero size array instead.
> +
> +union synth_kbd_version {
> + struct {
> + u16 minor_version;
> + u16 major_version;
> + };
> + u32 version;
> +};
> +
> +/*
> + * Protocol messages
> + */
> +struct synth_kbd_protocol_request {
> + struct synth_kbd_msg_hdr header;
> + union synth_kbd_version version_requested;
> +};
> +
> +struct synth_kbd_protocol_response {
> + struct synth_kbd_msg_hdr header;
> + u32 accepted:1;
> + u32 reserved:31;
> +};
> +
> +struct synth_kbd_keystroke {
> + struct synth_kbd_msg_hdr header;
> + u16 make_code;
> + u16 reserved0;
> + u32 is_unicode:1;
> + u32 is_break:1;
> + u32 is_e0:1;
> + u32 is_e1:1;
> + u32 reserved:28;
> +};
> +
> +
Extra blank line.
> +#define HK_MAXIMUM_MESSAGE_SIZE 256
> +
> +#define KBD_VSC_SEND_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> +#define KBD_VSC_RECV_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> +
> +#define XTKBD_EMUL0 0xe0
> +#define XTKBD_EMUL1 0xe1
> +#define XTKBD_RELEASE 0x80
> +
> +
Extra blank.
> +/*
> + * Represents a keyboard device
> + */
> +struct hv_kbd_dev {
> + unsigned char keycode[256];
> + struct hv_device *device;
> + struct synth_kbd_protocol_request protocol_req;
> + struct synth_kbd_protocol_response protocol_resp;
> + /* Synchronize the request/response if needed */
> + struct completion wait_event;
> + struct serio *hv_serio;
> +};
> +
> +
Extra blank.
> +static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
> +{
> + struct hv_kbd_dev *kbd_dev;
> + struct serio *hv_serio;
> +
> + kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
> +
Spurious blank line.
> + if (!kbd_dev)
> + return NULL;
> +
> + hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
> +
Spurious blank.
> + if (hv_serio == NULL) {
> + kfree(kbd_dev);
> + return NULL;
> + }
> +
> + hv_serio->id.type = SERIO_8042_XL;
Pointless tab before the '='.
> + strlcpy(hv_serio->name, dev_name(&device->device),
> + sizeof(hv_serio->name));
> + strlcpy(hv_serio->phys, dev_name(&device->device),
> + sizeof(hv_serio->phys));
> + hv_serio->dev.parent = &device->device;
Why are there two spaces before the '='?
> +
> +
Extra blank line.
> + kbd_dev->device = device;
> + kbd_dev->hv_serio = hv_serio;
> + hv_set_drvdata(device, kbd_dev);
> + init_completion(&kbd_dev->wait_event);
> +
> + return kbd_dev;
> +}
> +
> +static void hv_kbd_free_device(struct hv_kbd_dev *device)
> +{
> + serio_unregister_port(device->hv_serio);
> + kfree(device->hv_serio);
> + hv_set_drvdata(device->device, NULL);
> + kfree(device);
> +}
> +
> +static void hv_kbd_on_receive(struct hv_device *device,
> + struct vmpacket_descriptor *packet)
> +{
> + struct synth_kbd_msg *msg;
> + struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
> + struct synth_kbd_keystroke *ks_msg;
> + u16 scan_code;
> +
> + msg = (struct synth_kbd_msg *)((unsigned long)packet +
> + (packet->offset8 << 3));
> +
> + switch (msg->header.type) {
> + case SYNTH_KBD_PROTOCOL_RESPONSE:
> + memcpy(&kbd_dev->protocol_resp, msg,
> + sizeof(struct synth_kbd_protocol_response));
> + complete(&kbd_dev->wait_event);
> + break;
> + case SYNTH_KBD_EVENT:
> + ks_msg = (struct synth_kbd_keystroke *)msg;
> + scan_code = ks_msg->make_code;
> +
> + /*
> + * Inject the information through the serio interrupt.
> + */
> + if (ks_msg->is_e0)
> + serio_interrupt(kbd_dev->hv_serio, XTKBD_EMUL0, 0);
> + serio_interrupt(kbd_dev->hv_serio,
> + scan_code | (ks_msg->is_break ?
> + XTKBD_RELEASE : 0),
> + 0);
> +
It would be more readable to do:
if (ks_msg->is_break)
scan_code |= XTKBD_RELEASE;
serio_interrupt(kbd_dev->hv_serio, scan_code, 0);
> + break;
> +
> + default:
> + pr_err("unhandled message type %d\n", msg->header.type);
Just a question. This can only be triggered by the hyperviser, right?
> + }
> +}
> +
> +static void hv_kbd_on_channel_callback(void *context)
> +{
> + const int packet_size = 0x100;
> + int ret;
> + struct hv_device *device = context;
> + u32 bytes_recvd;
> + u64 req_id;
> + struct vmpacket_descriptor *desc;
> + unsigned char *buffer;
> + int bufferlen = packet_size;
> +
> + buffer = kmalloc(bufferlen, GFP_ATOMIC);
> + if (!buffer)
> + return;
> +
> + do {
Forever loops should be while (1) { instead of do { } while (1).
> + ret = vmbus_recvpacket_raw(device->channel, buffer,
> + bufferlen, &bytes_recvd, &req_id);
> +
> + switch (ret) {
> + case 0:
> + if (bytes_recvd <= 0) {
There can never be a negative number of bytes_recvd.
> + kfree(buffer);
> + return;
> + }
> + desc = (struct vmpacket_descriptor *)buffer;
> +
> + switch (desc->type) {
> + case VM_PKT_COMP:
> + break;
> +
> + case VM_PKT_DATA_INBAND:
> + hv_kbd_on_receive(device, desc);
This is the error handling I mentioned at the top. hv_kbd_on_receive()
doesn't take into consideration the amount of data we recieved, it
trusts the offset we recieved from the user. There is an out of bounds
read.
> + break;
> +
> + default:
> + pr_err("unhandled packet type %d, tid %llx len %d\n",
> + desc->type, req_id, bytes_recvd);
> + break;
> + }
> +
> + break;
> +
> + case -ENOBUFS:
> + kfree(buffer);
> + /* Handle large packet */
> + bufferlen = bytes_recvd;
> + buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
> +
> + if (!buffer)
> + return;
> +
> + break;
> + }
> + } while (1);
> +
> +}
> +
> +static int hv_kbd_connect_to_vsp(struct hv_device *device)
> +{
> + int ret = 0;
Don't initialize this.
> + int t;
> + struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
> + struct synth_kbd_protocol_request *request;
> + struct synth_kbd_protocol_response *response;
> +
> + request = &kbd_dev->protocol_req;
> + memset(request, 0, sizeof(struct synth_kbd_protocol_request));
> +
> + request->header.type = SYNTH_KBD_PROTOCOL_REQUEST;
> + request->version_requested.version = SYNTH_KBD_VERSION;
> +
> + ret = vmbus_sendpacket(device->channel, request,
> + sizeof(struct synth_kbd_protocol_request),
> + (unsigned long)request,
> + VM_PKT_DATA_INBAND,
> + VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> + if (ret)
> + goto cleanup;
There is no cleanup. Just return directly.
> +
> + t = wait_for_completion_timeout(&kbd_dev->wait_event, 10 * HZ);
> + if (!t) {
> + ret = -ETIMEDOUT;
> + goto cleanup;
return -ETIMEOUT;
> + }
> +
> + response = &kbd_dev->protocol_resp;
> +
> + if (!response->accepted) {
> + pr_err("synth_kbd protocol request failed (version %d)\n",
> + SYNTH_KBD_VERSION);
> + ret = -ENODEV;
> + goto cleanup;
Just return -ENODEV;
> + }
> +
return 0;
> +
> +cleanup:
> + return ret;
> +}
> +
> +static int hv_kbd_probe(struct hv_device *device,
> + const struct hv_vmbus_device_id *dev_id)
> +{
> + int ret;
> + struct hv_kbd_dev *kbd_dev;
> +
> + kbd_dev = hv_kbd_alloc_device(device);
> +
Delete the blank line.
> + if (!kbd_dev)
> + return -ENOMEM;
> +
> + ret = vmbus_open(device->channel,
> + KBD_VSC_SEND_RING_BUFFER_SIZE,
> + KBD_VSC_RECV_RING_BUFFER_SIZE,
> + NULL,
> + 0,
> + hv_kbd_on_channel_callback,
> + device
> + );
> +
Delete the blank line.
> + if (ret)
> + goto probe_err0;
> +
> + ret = hv_kbd_connect_to_vsp(device);
> +
Delete the blank line.
> + if (ret)
> + goto probe_err1;
> +
> + serio_register_port(kbd_dev->hv_serio);
> +
> + return ret;
return 0;
> +
> +probe_err1:
> + vmbus_close(device->channel);
The label here should be "err_close:". The number is more GW-Basic
style than C.
> +
> +probe_err0:
The label should be "err_free_dev:".
> + hv_kbd_free_device(kbd_dev);
> +
> + return ret;
> +}
> +
> +
Extra blank line.
> +static int hv_kbd_remove(struct hv_device *dev)
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Jürgen Beisert @ 2013-09-16 8:10 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Jonathan Cameron, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, marex-ynQEQJNshbs,
fabio.estevam-KZfg59tc24xl57MIdRCFDg,
jic23-KWPb1pKIrIJaa/9Udqfwiw, Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <52359259.8010202-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Hi Jonathan,
On Sunday 15 September 2013 12:56:25 Jonathan Cameron wrote:
> On 09/11/13 09:18, Juergen Beisert wrote:
> > For battery driven systems it is a very bad idea to collect the
> > touchscreen data within a kernel busy loop.
> >
> > This change uses the features of the hardware to delay and accumulate
> > samples in hardware to avoid a high interrupt and CPU load.
> >
> > Note: this is only tested on an i.MX23 SoC yet.
> >
> > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
>
> While this driver is placed in IIO within staging at the moment, these
> changes are definitely input related. Hence I have cc'd Dmitry and the
> input list.
>
> I am personaly a little uncomfortable that we have such a complex bit of
> input code sat within an IIO driver but such is life.
Maybe an MFD for this ADC unit would be a better way to go? Currently I have a
different problem with this driver, because the ADC unit monitors the battery
as well. And the charging driver from the power subsystem needs these values
to charge the battery in a correct manner.
> [...]
Regards,
Juergen
--
Pengutronix e.K. | Juergen Beisert |
Linux Solutions for Science and Industry | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 20/51] Input: atmel_mxt_ts - Set default irqflags when there is no pdata
From: Dmitry Torokhov @ 2013-09-16 2:25 UTC (permalink / raw)
To: rydberg
Cc: Nick Dyer, Daniel Kurtz, Joonyoung Shim, Alan Bowens, linux-input,
linux-kernel, Peter Meerwald, Benson Leung, Olof Johansson,
Yufeng Shen
In-Reply-To: <20130718171744.GC32381@polaris.bitmath.org>
On Thu, Jul 18, 2013 at 07:17:44PM +0200, rydberg@euromail.se wrote:
> Hi Nick,
>
> > From: Yufeng Shen <miletus@chromium.org>
> >
> > This is the preparation for supporting the code path when there is
> > platform data provided and still boot the device into a sane state
> > with backup NVRAM config.
> >
> > Make the irqflags default to be IRQF_TRIGGER_FALLING if no platform data is
> > provided.
I think if there is no platform data we should use 0 as IRQ falgs and
assume that IRQ line is properly configured by the board code or via
device tree.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: K. Y. Srinivasan @ 2013-09-16 5:28 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, linux-input, dmitry.torokhov,
vojtech, olaf, apw, jasowang
Cc: K. Y. Srinivasan
Add a new driver to support synthetic keyboard. On the next generation
Hyper-V guest firmware, many legacy devices will not be emulated and this
driver will be required.
I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
details of the AT keyboard driver.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/input/serio/Kconfig | 7 +
drivers/input/serio/Makefile | 1 +
drivers/input/serio/hyperv-keyboard.c | 379 +++++++++++++++++++++++++++++++++
3 files changed, 387 insertions(+), 0 deletions(-)
create mode 100644 drivers/input/serio/hyperv-keyboard.c
diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index 1e691a3..f3996e7 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig
@@ -267,4 +267,11 @@ config SERIO_OLPC_APSP
To compile this driver as a module, choose M here: the module will
be called olpc_apsp.
+config HYPERV_KEYBOARD
+ tristate "Microsoft Synthetic Keyboard driver"
+ depends on HYPERV
+ default HYPERV
+ help
+ Select this option to enable the Hyper-V Keyboard driver.
+
endif
diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
index 12298b1..815d874 100644
--- a/drivers/input/serio/Makefile
+++ b/drivers/input/serio/Makefile
@@ -28,3 +28,4 @@ obj-$(CONFIG_SERIO_ALTERA_PS2) += altera_ps2.o
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
diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
new file mode 100644
index 0000000..0d4625f
--- /dev/null
+++ b/drivers/input/serio/hyperv-keyboard.c
@@ -0,0 +1,379 @@
+/*
+ * Copyright (c) 2013, Microsoft Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; 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/module.h>
+#include <linux/device.h>
+#include <linux/completion.h>
+#include <linux/hyperv.h>
+#include <linux/serio.h>
+#include <linux/slab.h>
+
+
+/*
+ * Current version 1.0
+ *
+ */
+#define SYNTH_KBD_VERSION_MAJOR 1
+#define SYNTH_KBD_VERSION_MINOR 0
+#define SYNTH_KBD_VERSION (SYNTH_KBD_VERSION_MINOR | \
+ (SYNTH_KBD_VERSION_MAJOR << 16))
+
+
+/*
+ * Message types in the synthetic input protocol
+ */
+enum synth_kbd_msg_type {
+ SYNTH_KBD_PROTOCOL_REQUEST = 1,
+ SYNTH_KBD_PROTOCOL_RESPONSE = 2,
+ SYNTH_KBD_EVENT = 3,
+ SYNTH_KBD_LED_INDICATORS = 4,
+};
+
+/*
+ * Basic message structures.
+ */
+struct synth_kbd_msg_hdr {
+ enum synth_kbd_msg_type type;
+};
+
+struct synth_kbd_msg {
+ struct synth_kbd_msg_hdr header;
+ char data[1]; /* Enclosed message */
+};
+
+union synth_kbd_version {
+ struct {
+ u16 minor_version;
+ u16 major_version;
+ };
+ u32 version;
+};
+
+/*
+ * Protocol messages
+ */
+struct synth_kbd_protocol_request {
+ struct synth_kbd_msg_hdr header;
+ union synth_kbd_version version_requested;
+};
+
+struct synth_kbd_protocol_response {
+ struct synth_kbd_msg_hdr header;
+ u32 accepted:1;
+ u32 reserved:31;
+};
+
+struct synth_kbd_keystroke {
+ struct synth_kbd_msg_hdr header;
+ u16 make_code;
+ u16 reserved0;
+ u32 is_unicode:1;
+ u32 is_break:1;
+ u32 is_e0:1;
+ u32 is_e1:1;
+ u32 reserved:28;
+};
+
+
+#define HK_MAXIMUM_MESSAGE_SIZE 256
+
+#define KBD_VSC_SEND_RING_BUFFER_SIZE (10 * PAGE_SIZE)
+#define KBD_VSC_RECV_RING_BUFFER_SIZE (10 * PAGE_SIZE)
+
+#define XTKBD_EMUL0 0xe0
+#define XTKBD_EMUL1 0xe1
+#define XTKBD_RELEASE 0x80
+
+
+/*
+ * Represents a keyboard device
+ */
+struct hv_kbd_dev {
+ unsigned char keycode[256];
+ struct hv_device *device;
+ struct synth_kbd_protocol_request protocol_req;
+ struct synth_kbd_protocol_response protocol_resp;
+ /* Synchronize the request/response if needed */
+ struct completion wait_event;
+ struct serio *hv_serio;
+};
+
+
+static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
+{
+ struct hv_kbd_dev *kbd_dev;
+ struct serio *hv_serio;
+
+ kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
+
+ if (!kbd_dev)
+ return NULL;
+
+ hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
+
+ if (hv_serio == NULL) {
+ kfree(kbd_dev);
+ return NULL;
+ }
+
+ hv_serio->id.type = SERIO_8042_XL;
+ strlcpy(hv_serio->name, dev_name(&device->device),
+ sizeof(hv_serio->name));
+ strlcpy(hv_serio->phys, dev_name(&device->device),
+ sizeof(hv_serio->phys));
+ hv_serio->dev.parent = &device->device;
+
+
+ kbd_dev->device = device;
+ kbd_dev->hv_serio = hv_serio;
+ hv_set_drvdata(device, kbd_dev);
+ init_completion(&kbd_dev->wait_event);
+
+ return kbd_dev;
+}
+
+static void hv_kbd_free_device(struct hv_kbd_dev *device)
+{
+ serio_unregister_port(device->hv_serio);
+ kfree(device->hv_serio);
+ hv_set_drvdata(device->device, NULL);
+ kfree(device);
+}
+
+static void hv_kbd_on_receive(struct hv_device *device,
+ struct vmpacket_descriptor *packet)
+{
+ struct synth_kbd_msg *msg;
+ struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
+ struct synth_kbd_keystroke *ks_msg;
+ u16 scan_code;
+
+ msg = (struct synth_kbd_msg *)((unsigned long)packet +
+ (packet->offset8 << 3));
+
+ switch (msg->header.type) {
+ case SYNTH_KBD_PROTOCOL_RESPONSE:
+ memcpy(&kbd_dev->protocol_resp, msg,
+ sizeof(struct synth_kbd_protocol_response));
+ complete(&kbd_dev->wait_event);
+ break;
+ case SYNTH_KBD_EVENT:
+ ks_msg = (struct synth_kbd_keystroke *)msg;
+ scan_code = ks_msg->make_code;
+
+ /*
+ * Inject the information through the serio interrupt.
+ */
+ if (ks_msg->is_e0)
+ serio_interrupt(kbd_dev->hv_serio, XTKBD_EMUL0, 0);
+ serio_interrupt(kbd_dev->hv_serio,
+ scan_code | (ks_msg->is_break ?
+ XTKBD_RELEASE : 0),
+ 0);
+
+ break;
+
+ default:
+ pr_err("unhandled message type %d\n", msg->header.type);
+ }
+}
+
+static void hv_kbd_on_channel_callback(void *context)
+{
+ const int packet_size = 0x100;
+ int ret;
+ struct hv_device *device = context;
+ u32 bytes_recvd;
+ u64 req_id;
+ struct vmpacket_descriptor *desc;
+ unsigned char *buffer;
+ int bufferlen = packet_size;
+
+ buffer = kmalloc(bufferlen, GFP_ATOMIC);
+ if (!buffer)
+ return;
+
+ do {
+ ret = vmbus_recvpacket_raw(device->channel, buffer,
+ bufferlen, &bytes_recvd, &req_id);
+
+ switch (ret) {
+ case 0:
+ if (bytes_recvd <= 0) {
+ kfree(buffer);
+ return;
+ }
+ desc = (struct vmpacket_descriptor *)buffer;
+
+ switch (desc->type) {
+ case VM_PKT_COMP:
+ break;
+
+ case VM_PKT_DATA_INBAND:
+ hv_kbd_on_receive(device, desc);
+ break;
+
+ default:
+ pr_err("unhandled packet type %d, tid %llx len %d\n",
+ desc->type, req_id, bytes_recvd);
+ break;
+ }
+
+ break;
+
+ case -ENOBUFS:
+ kfree(buffer);
+ /* Handle large packet */
+ bufferlen = bytes_recvd;
+ buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
+
+ if (!buffer)
+ return;
+
+ break;
+ }
+ } while (1);
+
+}
+
+static int hv_kbd_connect_to_vsp(struct hv_device *device)
+{
+ int ret = 0;
+ int t;
+ struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
+ struct synth_kbd_protocol_request *request;
+ struct synth_kbd_protocol_response *response;
+
+ request = &kbd_dev->protocol_req;
+ memset(request, 0, sizeof(struct synth_kbd_protocol_request));
+
+ request->header.type = SYNTH_KBD_PROTOCOL_REQUEST;
+ request->version_requested.version = SYNTH_KBD_VERSION;
+
+ ret = vmbus_sendpacket(device->channel, request,
+ sizeof(struct synth_kbd_protocol_request),
+ (unsigned long)request,
+ VM_PKT_DATA_INBAND,
+ VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+ if (ret)
+ goto cleanup;
+
+ t = wait_for_completion_timeout(&kbd_dev->wait_event, 10 * HZ);
+ if (!t) {
+ ret = -ETIMEDOUT;
+ goto cleanup;
+ }
+
+ response = &kbd_dev->protocol_resp;
+
+ if (!response->accepted) {
+ pr_err("synth_kbd protocol request failed (version %d)\n",
+ SYNTH_KBD_VERSION);
+ ret = -ENODEV;
+ goto cleanup;
+ }
+
+
+cleanup:
+ return ret;
+}
+
+static int hv_kbd_probe(struct hv_device *device,
+ const struct hv_vmbus_device_id *dev_id)
+{
+ int ret;
+ struct hv_kbd_dev *kbd_dev;
+
+ kbd_dev = hv_kbd_alloc_device(device);
+
+ if (!kbd_dev)
+ return -ENOMEM;
+
+ ret = vmbus_open(device->channel,
+ KBD_VSC_SEND_RING_BUFFER_SIZE,
+ KBD_VSC_RECV_RING_BUFFER_SIZE,
+ NULL,
+ 0,
+ hv_kbd_on_channel_callback,
+ device
+ );
+
+ if (ret)
+ goto probe_err0;
+
+ ret = hv_kbd_connect_to_vsp(device);
+
+ if (ret)
+ goto probe_err1;
+
+ serio_register_port(kbd_dev->hv_serio);
+
+ return ret;
+
+probe_err1:
+ vmbus_close(device->channel);
+
+probe_err0:
+ hv_kbd_free_device(kbd_dev);
+
+ return ret;
+}
+
+
+static int hv_kbd_remove(struct hv_device *dev)
+{
+ struct hv_kbd_dev *kbd_dev = hv_get_drvdata(dev);
+
+ vmbus_close(dev->channel);
+ hv_kbd_free_device(kbd_dev);
+ return 0;
+}
+
+/*
+ * Keyboard GUID
+ * {f912ad6d-2b17-48ea-bd65-f927a61c7684}
+ */
+#define HV_KBD_GUID \
+ .guid = { \
+ 0x6d, 0xad, 0x12, 0xf9, 0x17, 0x2b, 0xea, 0x48, \
+ 0xbd, 0x65, 0xf9, 0x27, 0xa6, 0x1c, 0x76, 0x84 \
+ }
+
+static const struct hv_vmbus_device_id id_table[] = {
+ /* Keyboard guid */
+ { HV_KBD_GUID, },
+ { },
+};
+
+MODULE_DEVICE_TABLE(vmbus, id_table);
+
+static struct hv_driver hv_kbd_drv = {
+ .name = KBUILD_MODNAME,
+ .id_table = id_table,
+ .probe = hv_kbd_probe,
+ .remove = hv_kbd_remove,
+};
+
+static int __init hv_kbd_init(void)
+{
+ return vmbus_driver_register(&hv_kbd_drv);
+}
+
+static void __exit hv_kbd_exit(void)
+{
+ vmbus_driver_unregister(&hv_kbd_drv);
+}
+
+MODULE_LICENSE("GPL");
+module_init(hv_kbd_init);
+module_exit(hv_kbd_exit);
--
1.7.4.1
^ permalink raw reply related
* [PATCH 2/2] Input: synaptics_usb - Add stick_hid param
From: Andrew Deason @ 2013-09-15 21:02 UTC (permalink / raw)
To: linux-input
In-Reply-To: <20130915155755.946c17243cc33d621245138f@dson.org>
Add a module parameter (stick_hid) that makes the trackpoint device
present itself as a HID mouse. In this mode, the hardware translates
the force readings into relative mouse motion itself, instead of the
driver performing the translation.
This can be useful, since the code to translate trackpoint force
readings into cursor movement inside synaptics_usb itself still
involves some guesswork. Without detailed knowledge of the hardware
device, it may not ever be possible to get the same results from our
driver as what the hardware provides. This parameter allows the
behavior to be identical to behavior before the introduction of the
synaptics_usb driver, if the user prefers that behavior.
The stick_hid mode also requires fewer calculations to be done by the
synaptics_usb driver, if someone is bothered by the overhead of the
trackpoint movement calculations.
Signed-off-by: Andrew Deason <adeason@dson.org>
---
drivers/input/mouse/synaptics_usb.c | 54 ++++++++++++++++++++++++++++++++-----
1 file changed, 47 insertions(+), 7 deletions(-)
diff --git a/drivers/input/mouse/synaptics_usb.c b/drivers/input/mouse/synaptics_usb.c
index 8571005..652afae 100644
--- a/drivers/input/mouse/synaptics_usb.c
+++ b/drivers/input/mouse/synaptics_usb.c
@@ -35,7 +35,8 @@
* setting 1: one int endpoint for absolute finger position
* setting 2 (cPad only): one int endpoint for absolute finger position and
* two bulk endpoints for the display (in/out)
- * This driver uses setting 1.
+ * This driver uses setting 1. For touchsticks, you can use setting 0 instead
+ * by setting the stick_hid module parameter.
*/
#include <linux/kernel.h>
@@ -443,7 +444,9 @@ static void synusb_report_buttons(struct synusb *synusb)
input_report_key(input_dev, BTN_MIDDLE, synusb->data[1] & 0x02);
}
-static void synusb_report_stick(struct synusb *synusb)
+/* This uses the 'raw' force readings from the trackpoint and translates them
+ * into relative mouse movement. */
+static void synusb_report_stick_raw(struct synusb *synusb)
{
struct input_dev *input_dev = synusb->input;
int x, y;
@@ -470,6 +473,24 @@ static void synusb_report_stick(struct synusb *synusb)
input_sync(input_dev);
}
+/* This just reads the HID data from the stick device, and just passes it on
+ * to the input subsystem. */
+static int stick_hid;
+static void synusb_report_stick_hid(struct synusb *synusb)
+{
+ struct input_dev *dev = synusb->input;
+ signed char *data = synusb->data;
+
+ input_report_key(dev, BTN_LEFT, data[0] & 0x01);
+ input_report_key(dev, BTN_RIGHT, data[0] & 0x02);
+ input_report_key(dev, BTN_MIDDLE, data[0] & 0x04);
+
+ input_report_rel(dev, REL_X, data[1]);
+ input_report_rel(dev, REL_Y, data[2]);
+
+ input_sync(dev);
+}
+
static void synusb_report_touchpad(struct synusb *synusb)
{
struct input_dev *input_dev = synusb->input;
@@ -555,9 +576,13 @@ static void synusb_irq(struct urb *urb)
break;
}
- if (synusb->flags & SYNUSB_STICK)
- synusb_report_stick(synusb);
- else
+ if (synusb->flags & SYNUSB_STICK) {
+ if (stick_hid) {
+ synusb_report_stick_hid(synusb);
+ } else {
+ synusb_report_stick_raw(synusb);
+ }
+ } else
synusb_report_touchpad(synusb);
resubmit:
@@ -640,6 +665,15 @@ static int synusb_probe(struct usb_interface *intf,
unsigned int intf_num = intf->cur_altsetting->desc.bInterfaceNumber;
unsigned int altsetting = min(intf->num_altsetting, 1U);
int error;
+ int pipe, maxp;
+
+ if ((id->driver_info & SYNUSB_COMBO) && intf_num == 1) {
+ /* we are a pointing stick */
+ if (stick_hid) {
+ /* alt setting 0 is the HID non-advanced mode */
+ altsetting = 0;
+ }
+ }
error = usb_set_interface(udev, intf_num, altsetting);
if (error) {
@@ -687,9 +721,12 @@ static int synusb_probe(struct usb_interface *intf,
goto err_free_urb;
}
+ pipe = usb_rcvintpipe(udev, ep->bEndpointAddress);
+ maxp = usb_maxpacket(udev, pipe, usb_pipeout(pipe));
+
usb_fill_int_urb(synusb->urb, udev,
- usb_rcvintpipe(udev, ep->bEndpointAddress),
- synusb->data, SYNUSB_RECV_SIZE,
+ pipe,
+ synusb->data, (maxp > SYNUSB_RECV_SIZE ? SYNUSB_RECV_SIZE : maxp),
synusb_irq, synusb,
ep->bInterval);
synusb->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
@@ -920,3 +957,6 @@ MODULE_LICENSE("GPL");
module_param(stick_debug, int, 0644);
MODULE_PARM_DESC(stick_debug, "Activate pointing stick movement debugging output");
+
+module_param(stick_hid, int, 0644);
+MODULE_PARM_DESC(stick_hid, "Use HID data for pointing sticks instead of raw force readings");
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH 1/2] Input: synaptics_usb - Improve stick movement
From: Andrew Deason @ 2013-09-15 21:01 UTC (permalink / raw)
To: linux-input
In-Reply-To: <20130915155755.946c17243cc33d621245138f@dson.org>
Currently, for the pointing stick, we receive some (x, y) coordinates
from the usb device, and report those directly as relative movement to
the input subsystem. However, in order to behave similarly to other
trackpoint devices, these values need to be interpreted as readings
from the force sensor, and translated into cursor movement.
With this commit, we take the force readings and run them through
roughly the same translation as described by the "TrackPoint System
Version 4.0 Engineering Specification" (available via
<http://blogs.epfl.ch/icenet/documents/Ykt3Eext.pdf> and probably
other places). This adds the sysfs attributes 'inertia',
'sensitivity', and 'speed' to the input device, which behave similarly
to the sysfs attributes in other trackpoint devices (where the
translation occurs in hardware).
Note that the information provided by the trackpoint spec is sometimes
vague, and it is not certain if all of it also applies to synaptics
devices with trackpoints. Some of the specific values provided in this
commit are guesses, and partly based on experimentation for what
"feels" like other trackpoint devices. Specifically, this was tested
with an IBM model SK-8835 (FRU 02R0400, vendor/product id 06cb:0009).
The cursor movement was compared to the movement with the same device
under HID mode, as well as an IBM PS/2 model RT3200 (FRU 37L0888) and
a T60 trackpoint.
Signed-off-by: Andrew Deason <adeason@dson.org>
---
drivers/input/mouse/synaptics_usb.c | 377 +++++++++++++++++++++++++++++++++++-
1 file changed, 371 insertions(+), 6 deletions(-)
diff --git a/drivers/input/mouse/synaptics_usb.c b/drivers/input/mouse/synaptics_usb.c
index 64cf34e..8571005 100644
--- a/drivers/input/mouse/synaptics_usb.c
+++ b/drivers/input/mouse/synaptics_usb.c
@@ -27,8 +27,8 @@
/*
* There are three different types of Synaptics USB devices: Touchpads,
* touchsticks (or trackpoints), and touchscreens. Touchpads are well supported
- * by this driver, touchstick support has not been tested much yet, and
- * touchscreens have not been tested at all.
+ * by this driver, touchstick support is tested but still involves guesswork,
+ * and touchscreens have not been tested at all.
*
* Up to three alternate settings are possible:
* setting 0: one int endpoint for relative movement (used by usbhid.ko)
@@ -90,8 +90,350 @@ struct synusb {
/* characteristics of the device */
unsigned long flags;
+
+ /* pointing stick user-visible options */
+ unsigned int stick_inertia;
+ unsigned int stick_sensitivity;
+ unsigned int stick_speed;
+
+ /* pointing stick data; only used for SYNUSB_STICK */
+ int64_t stick_last_mag;
+ int64_t stick_accum_x;
+ int64_t stick_accum_y;
};
+/* Much of the following code deals with converting trackpoint force readings
+ * into cursor movements. Some of the comments reference the trackpoint
+ * engineering spec, which is the "TrackPoint System Version 4.0 Engineering
+ * Specification", and is where many of the constants in this code come from.
+ * This document be found, amongst other places, at
+ * <http://blogs.epfl.ch/icenet/documents/Ykt3Eext.pdf>. */
+
+static ssize_t synusb_attr_stick_inertia_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct usb_interface *intf = to_usb_interface(dev->parent);
+ struct synusb *synusb = usb_get_intfdata(intf);
+
+ return snprintf(buf, PAGE_SIZE, "%u\n",
+ synusb->stick_inertia);
+}
+
+static ssize_t synusb_attr_stick_inertia_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf,
+ size_t count)
+{
+ struct usb_interface *intf = to_usb_interface(dev->parent);
+ struct synusb *synusb = usb_get_intfdata(intf);
+ unsigned int value;
+
+ if (kstrtouint(buf, 10, &value) || value > 255)
+ return -EINVAL;
+
+ synusb->stick_inertia = value;
+
+ return count;
+}
+
+static ssize_t synusb_attr_stick_sensitivity_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct usb_interface *intf = to_usb_interface(dev->parent);
+ struct synusb *synusb = usb_get_intfdata(intf);
+
+ return snprintf(buf, PAGE_SIZE, "%u\n",
+ synusb->stick_sensitivity);
+}
+
+static ssize_t synusb_attr_stick_sensitivity_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf,
+ size_t count)
+{
+ struct usb_interface *intf = to_usb_interface(dev->parent);
+ struct synusb *synusb = usb_get_intfdata(intf);
+ unsigned int value;
+
+ if (kstrtouint(buf, 10, &value) || value > 255)
+ return -EINVAL;
+
+ synusb->stick_sensitivity = value;
+
+ return count;
+}
+
+static ssize_t synusb_attr_stick_speed_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct usb_interface *intf = to_usb_interface(dev->parent);
+ struct synusb *synusb = usb_get_intfdata(intf);
+
+ return snprintf(buf, PAGE_SIZE, "%u\n",
+ synusb->stick_speed);
+}
+
+static ssize_t synusb_attr_stick_speed_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf,
+ size_t count)
+{
+ struct usb_interface *intf = to_usb_interface(dev->parent);
+ struct synusb *synusb = usb_get_intfdata(intf);
+ unsigned int value;
+
+ if (kstrtouint(buf, 10, &value) || value > 255)
+ return -EINVAL;
+
+ synusb->stick_speed = value;
+
+ return count;
+}
+
+/* sysfs attribute "inertia": Negative inertia factor. Increasing this means
+ * negative inertia plays a bigger role. */
+static struct device_attribute synusb_attr_stick_inertia =
+ __ATTR(inertia, S_IWUSR | S_IRUGO,
+ synusb_attr_stick_inertia_show,
+ synusb_attr_stick_inertia_store);
+
+/* sysfs attribute "sensitivity": Pointing stick sensitivity. We take the force
+ * readings from the stick, and multiply by the sensitivity value and divide by
+ * 128. So, setting this to 255 means you need to push about half as hard to
+ * get the same result. */
+static struct device_attribute synusb_attr_stick_sensitivity =
+ __ATTR(sensitivity, S_IWUSR | S_IRUGO,
+ synusb_attr_stick_sensitivity_show,
+ synusb_attr_stick_sensitivity_store);
+
+/* sysfs attribute "speed": Speed factor, aka "value6" in the trackpoint spec.
+ * Increasing this means the cursor will move faster when the stick sees more
+ * force, but at milder forces the cursor speed is unaffected. */
+static struct device_attribute synusb_attr_stick_speed =
+ __ATTR(speed, S_IWUSR | S_IRUGO,
+ synusb_attr_stick_speed_show,
+ synusb_attr_stick_speed_store);
+
+static struct attribute *synusb_stick_attrs[] = {
+ &synusb_attr_stick_inertia.attr,
+ &synusb_attr_stick_sensitivity.attr,
+ &synusb_attr_stick_speed.attr,
+ NULL
+};
+
+static struct attribute_group synusb_stick_attr_group = {
+ .attrs = synusb_stick_attrs,
+};
+
+/* For various calculations of pointer stick movement, we need sub-integer
+ * precision. So, STICK_PRECISION is our fixed-point multiplier, in order to
+ * perform some calculations with such precision. */
+#define STICK_PRECISION 128
+#define STICK_FMT "%ld.%03d"
+#define STICK_FMT_VAL(x) (long)(x / STICK_PRECISION), ((int)(x % STICK_PRECISION)*1000/STICK_PRECISION)
+
+static int stick_debug;
+#define stick_dprintk(format, arg...) if (stick_debug) printk(KERN_DEBUG format, ##arg)
+
+/* This function approximates the "TrackPoint Transfer Function" on page 43 of
+ * the trackpoint spec, which appears to be a simple piecewise linear function.
+ * Many of these constants are eyeballed or guessed, and are partially based on
+ * experimentation to see what feels similar to a PS/2 trackpoint device. They
+ * may be wrong. */
+static int64_t
+stick_transfer_function(struct synusb *synusb, uint64_t magnitude)
+{
+ /* The transfer function has approximately 6 different segments. Some of
+ * these segments are constant, and others depend on the "speed" parameter.
+ * Here we provide some constants for the various segments, which are
+ * largely determined by looking at the trackpoint transfer function graph. */
+ static const int64_t STICK_XFER_0_X = 15 * STICK_PRECISION;
+ static const int64_t STICK_XFER_0_Y = 0;
+
+ static const int64_t STICK_XFER_1_X = 35 * STICK_PRECISION;
+ static const int64_t STICK_XFER_1_Y = 10 * STICK_PRECISION;
+
+ static const int64_t STICK_XFER_2_X = 55 * STICK_PRECISION;
+ static const int64_t STICK_XFER_2_Y = 22 * STICK_PRECISION;
+
+ static const int64_t STICK_XFER_3_X = 85 * STICK_PRECISION;
+
+ static const int64_t STICK_XFER_4_X = 115 * STICK_PRECISION;
+
+ int64_t slope;
+ int64_t yint;
+ int64_t plateau;
+ int64_t result;
+ int sect;
+
+ magnitude = magnitude * synusb->stick_sensitivity / 128;
+ stick_dprintk("post-sensitivity mag "STICK_FMT"\n", STICK_FMT_VAL(magnitude));
+
+ slope = 0;
+
+ if (magnitude < STICK_XFER_2_X) {
+ /* These lower ranges don't need to know the value of 'plateau'. */
+ if (magnitude < STICK_XFER_0_X) {
+ yint = STICK_XFER_0_Y;
+ sect = 0;
+
+ } else if (magnitude < STICK_XFER_1_X) {
+ yint = STICK_XFER_1_Y;
+ sect = 1;
+
+ } else {
+ yint = STICK_XFER_2_Y;
+ sect = 2;
+ }
+ } else {
+ /* Calculate the value of the plateau speed, from page 24 of the
+ * trackpoint spec.
+ * plateau = 22 + 4.3 * value6 */
+ plateau = 22*STICK_PRECISION + synusb->stick_speed * STICK_PRECISION * 43 / 10;
+
+ if (magnitude < STICK_XFER_3_X) {
+ /* Calculate the slope and y-intercept of a line going through the points
+ * (STICK_XFER_2_X, STICK_XFER_2_Y) and (STICK_XFER_3_X, plateau)
+ */
+ slope = STICK_PRECISION * (plateau - STICK_XFER_2_Y) / (STICK_XFER_3_X - STICK_XFER_2_X);
+ yint = STICK_XFER_2_Y - STICK_XFER_2_X / STICK_PRECISION * slope;
+ sect = 3;
+
+ } else if (magnitude < STICK_XFER_4_X) {
+ yint = plateau;
+ sect = 4;
+
+ } else {
+ /* Use the same slope as segment 3, but go through the point
+ * (STICK_XFER_4_X, plateau) */
+ slope = STICK_PRECISION * (plateau - STICK_XFER_2_Y) / (STICK_XFER_3_X - STICK_XFER_2_X);
+ yint = plateau - STICK_XFER_4_X / STICK_PRECISION * slope;
+ sect = 5;
+ }
+ }
+
+ result = slope * magnitude / STICK_PRECISION + yint;
+ stick_dprintk(" slope "STICK_FMT"\n", STICK_FMT_VAL(slope));
+ stick_dprintk(" xfer func sect %d\n", sect);
+ stick_dprintk(" magnitude "STICK_FMT" -> "STICK_FMT"\n",
+ STICK_FMT_VAL(magnitude),
+ STICK_FMT_VAL(result));
+
+ return result;
+}
+
+static void
+synusb_stick_calculate_xy(struct synusb *synusb, int x, int y, int *a_x, int *a_y)
+{
+ int64_t magnitude;
+ int64_t magnitude_raw;
+ int64_t scale;
+ int64_t x_rate, y_rate;
+ int64_t x_grams, y_grams;
+
+ stick_dprintk("======================\n");
+ stick_dprintk(" raw stick reading (%d, %d)\n", x, y);
+
+ /* According to the trackpoint spec on page 6, each unit from the force
+ * sensor is 0.8 grams. That seems to be roughly correct for here. */
+ x_grams = x * STICK_PRECISION * 4 / 5;
+ y_grams = y * STICK_PRECISION * 4 / 5;
+
+ stick_dprintk(" grams ("STICK_FMT", "STICK_FMT")\n", STICK_FMT_VAL(x_grams), STICK_FMT_VAL(y_grams));
+
+ /* First, get the magnitude of our input vector (x, y). */
+ magnitude_raw = int_sqrt(x_grams*x_grams + y_grams*y_grams);
+ magnitude = magnitude_raw;
+ stick_dprintk(" input mag "STICK_FMT"\n", STICK_FMT_VAL(magnitude_raw));
+
+
+ if (magnitude_raw == 0) {
+ /* If our input is 0, our output is definitely zero. (Even if applying
+ * negative inertia to the input of the transfer function yields a
+ * nonzero magnitude, applying negative inertia to the output of the
+ * transfer function would yield a resultant magnitude of 0 anyway; try
+ * calculating it yourself and see.) This needs to be addressed as a
+ * special case in order to avoid dividing by zero when applying negative
+ * inertia to the output of the transfer function. Skip some processing
+ * while we're at it, since any further calculations are useless, since
+ * we know the outcome. */
+ x = 0;
+ y = 0;
+
+ } else {
+ /* Apply negative inertia to the pre-transfer-function magnitude, from
+ * page 44 of the trackpoint spec.
+ * magnitude += I * (Z_1 - Z_0) */
+ magnitude += synusb->stick_inertia * (magnitude_raw - synusb->stick_last_mag);
+
+ stick_dprintk(" previous mag "STICK_FMT"\n", STICK_FMT_VAL(synusb->stick_last_mag));
+ stick_dprintk(" post-inertia in mag "STICK_FMT"\n", STICK_FMT_VAL(magnitude));
+
+ /* Apply the transfer function. It's possible to get a negative magnitude
+ * here from the negative inertia function above. Normally that would give
+ * us a result of 0 from the transfer function, since we're in the dead
+ * zone. However, this seems to mean that instead the cursor should be
+ * "pushed" back in the opposite direction. So take the absolute value of
+ * the magnitude, apply the transfer function, and negate the result. */
+ if (magnitude < 0) {
+ magnitude = -1 * stick_transfer_function(synusb, -1 * magnitude);
+ } else {
+ magnitude = stick_transfer_function(synusb, magnitude);
+ }
+
+ BUG_ON(magnitude_raw == 0);
+
+ /* Apply negative inertia to the output of the transfer function, from
+ * page 44 of the trackpoint spec.
+ * magnitude *= 1/(1 + I*(1 - Z_0/Z_1)) */
+ magnitude = STICK_PRECISION * magnitude /
+ (STICK_PRECISION + synusb->stick_inertia *
+ (STICK_PRECISION - STICK_PRECISION * synusb->stick_last_mag / magnitude_raw));
+
+ /* How much do we scale our x and y components by in order to to achieve
+ * a magnitude of 'magnitude' ? */
+ scale = STICK_PRECISION * magnitude / magnitude_raw;
+
+ stick_dprintk("post-inertia out mag "STICK_FMT"\n", STICK_FMT_VAL(magnitude));
+ stick_dprintk(" scale "STICK_FMT"\n", STICK_FMT_VAL(scale));
+
+ x_rate = x_grams * scale / STICK_PRECISION;
+ y_rate = y_grams * scale / STICK_PRECISION;
+ /* x_rate and y_rate are now in units of mickeys per second, in fixed-point. */
+
+ stick_dprintk(" scaled coords ("STICK_FMT", "STICK_FMT") mickeys per sec\n", STICK_FMT_VAL(x_rate), STICK_FMT_VAL(y_rate));
+
+ /* Convert from 'mickeys per second' to 'mickeys per 10ms' */
+ x_rate = x_rate / 100;
+ y_rate = y_rate / 100;
+
+ stick_dprintk(" scaled coords ("STICK_FMT", "STICK_FMT") mickeys per 10ms\n", STICK_FMT_VAL(x_rate), STICK_FMT_VAL(y_rate));
+
+ synusb->stick_accum_x += x_rate;
+ synusb->stick_accum_y += y_rate;
+
+ /* Pull out a whole number from the accumulator, if a whole number has
+ * built up. */
+ x = synusb->stick_accum_x / STICK_PRECISION;
+ y = synusb->stick_accum_y / STICK_PRECISION;
+
+ synusb->stick_accum_x -= x * STICK_PRECISION;
+ synusb->stick_accum_y -= y * STICK_PRECISION;
+ }
+
+ stick_dprintk(" reporting coords (%d, %d)\n", x, y);
+ stick_dprintk(" accum ("STICK_FMT", "STICK_FMT")\n",
+ STICK_FMT_VAL(synusb->stick_accum_x), STICK_FMT_VAL(synusb->stick_accum_y));
+
+ synusb->stick_last_mag = magnitude_raw;
+
+ *a_x = x;
+ *a_y = y;
+}
+
static void synusb_report_buttons(struct synusb *synusb)
{
struct input_dev *input_dev = synusb->input;
@@ -108,12 +450,17 @@ static void synusb_report_stick(struct synusb *synusb)
unsigned int pressure;
pressure = synusb->data[6];
- x = (s16)(be16_to_cpup((__be16 *)&synusb->data[2]) << 3) >> 7;
- y = (s16)(be16_to_cpup((__be16 *)&synusb->data[4]) << 3) >> 7;
+ x = (s16)(be16_to_cpup((__be16 *)&synusb->data[2]) << 3) >> 3;
+ y = (s16)(be16_to_cpup((__be16 *)&synusb->data[4]) << 3) >> 3;
- if (pressure > 0) {
+ /* Synaptics gives us the y axis such that positive is up, and negative
+ * is down. We want positive y axis to be down, and negative to be up. */
+ y *= -1;
+
+ synusb_stick_calculate_xy(synusb, x, y, &x, &y);
+ if (x || y) {
input_report_rel(input_dev, REL_X, x);
- input_report_rel(input_dev, REL_Y, -y);
+ input_report_rel(input_dev, REL_Y, y);
}
input_report_abs(input_dev, ABS_PRESSURE, pressure);
@@ -422,8 +769,20 @@ static int synusb_probe(struct usb_interface *intf,
goto err_stop_io;
}
+ if (synusb->flags & SYNUSB_STICK) {
+ synusb->stick_inertia = 6;
+ synusb->stick_sensitivity = 128;
+ synusb->stick_speed = 97;
+
+ error = sysfs_create_group(&synusb->input->dev.kobj, &synusb_stick_attr_group);
+ if (error)
+ goto err_unregister_device;
+ }
+
return 0;
+err_unregister_device:
+ input_unregister_device(input_dev);
err_stop_io:
if (synusb->flags & SYNUSB_IO_ALWAYS)
synusb_close(synusb->input);
@@ -445,6 +804,9 @@ static void synusb_disconnect(struct usb_interface *intf)
struct synusb *synusb = usb_get_intfdata(intf);
struct usb_device *udev = interface_to_usbdev(intf);
+ if (synusb->flags & SYNUSB_STICK)
+ sysfs_remove_group(&synusb->input->dev.kobj, &synusb_stick_attr_group);
+
if (synusb->flags & SYNUSB_IO_ALWAYS)
synusb_close(synusb->input);
@@ -555,3 +917,6 @@ MODULE_AUTHOR("Rob Miller <rob@inpharmatica.co.uk>, "
"Jan Steinhoff <cpad@jan-steinhoff.de>");
MODULE_DESCRIPTION("Synaptics USB device driver");
MODULE_LICENSE("GPL");
+
+module_param(stick_debug, int, 0644);
+MODULE_PARM_DESC(stick_debug, "Activate pointing stick movement debugging output");
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH 0/2] synaptics_usb trackpoint cursor movement improvements
From: Andrew Deason @ 2013-09-15 21:01 UTC (permalink / raw)
To: linux-input
Here are a couple of patches for improving trackpoint movement with
synaptics_usb. Currently, without these, the movement is much worse than
it was before synaptics_usb was introduced into the tree, since the
device was previously handled by usbhid. This is intended to be applied
instead of the "Input: synaptics_usb - Scale stick axes evenly" patch
submitted previously; there's no reason to apply that one if these
patches are applied.
I'm not familiar with code for input devices (in Linux or in general),
so I may be making silly mistakes or violating certain conventions. Feel
free to tear me apart :) Some particular highlights of things I was
unsure of, and so may have been dumb things to do:
- I'm not sure if there's a better way or if there are standard
conventions for doing fixed-point arithmetic. I just multiplied or
divided by STICK_PRECISION at the appropriate places, but since this
needs to be done pretty often in the calculations and with care, it
seems pretty easy to accidentally screw it up in future changes.
- The sysfs attributes getters/setters are mostly boilerplate. I don't
know if it's frowned upon to do it this way and I need to reduce the
duplication, or if it doesn't really matter since there are only 3
items.
- I'm not sure if there's a better way to transition between the HID
and non-HID modes. It seems like this should be switchable at runtime
instead of needing to reload the module, but I can't find an example
of another module doing this.
Also, I've been testing these changes with my own devices just by
myself; I would welcome feedback from others with similar devices if any
of them see this. It's difficult to objectively test how the trackpoint
"feel"s with different parameters.
Andrew Deason (2):
Input: synaptics_usb - Improve stick movement
Input: synaptics_usb - Add stick_hid param
drivers/input/mouse/synaptics_usb.c | 431 ++++++++++++++++++++++++++++++++++--
1 file changed, 418 insertions(+), 13 deletions(-)
--
Andrew Deason
adeason@dson.org
^ permalink raw reply
* Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Jonathan Cameron @ 2013-09-15 16:10 UTC (permalink / raw)
To: Juergen Beisert
Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, marex-ynQEQJNshbs,
fabio.estevam-KZfg59tc24xl57MIdRCFDg, Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <52359259.8010202-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On 09/15/13 11:56, Jonathan Cameron wrote:
> On 09/11/13 09:18, Juergen Beisert wrote:
>> For battery driven systems it is a very bad idea to collect the touchscreen
>> data within a kernel busy loop.
>>
>> This change uses the features of the hardware to delay and accumulate samples in
>> hardware to avoid a high interrupt and CPU load.
>>
>> Note: this is only tested on an i.MX23 SoC yet.
>>
>> Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
>> CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
>> CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> While this driver is placed in IIO within staging at the moment, these changes are definitely
> input related. Hence I have cc'd Dmitry and the input list.
>
> I am personaly a little uncomfortable that we have such a complex bit of input code sat
> within an IIO driver but such is life.
The logic in here looks reasonable to me. I am far from a specialist in how these touch
screens are normally handled though.
One thing to note is that you really want to get a proposed device tree spec out asap
as that can take longer to review than the driver. If you are proposing to do that as a future
patch, then take into account that you'll need to ensure these are the defaults if
it is not specified in the device tree for ever more (which is more painful than
hammering out he device tree stuff now!)
...
>> +static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
>> + struct device_node *lradc_node)
>> +{
>> + /* TODO retrieve from device tree */
>> + lradc->over_sample_cnt = 4;
>> + lradc->over_sample_delay = 2;
>> + lradc->settling_delay = 10;
>> +
>> + return 0;
>> +}
>> +
...
^ permalink raw reply
* input question: ambient light sensor button
From: Pali Rohár @ 2013-09-15 13:53 UTC (permalink / raw)
To: linux-kernel, linux-input
[-- Attachment #1: Type: Text/Plain, Size: 923 bytes --]
Hello,
I do not know where to ask this question, but I think that kernel
developers could help me.
I have notebook with one special button on keyboard which is
designed for turning ambient light sensor on and off. By default
pressing button do nothing (I can turn ambient light sensor on/off
via sysfs platform wmi module). Button press is reported by
kernel input device AT Translated Set 2 keyboard and reports it
as button "touchpad off".
Of course "touchpad off" is incorrect and I'd like to ask which
kernel key or button from /usr/include/linux/input.h should be
mapped for my ambient light sensor button? Is there already some?
And what is strategy for allocating KEY_* and BTN_* numbers?
I'd like to know this, so udev could have correct DMI keymap
hooks and other userspace programs can understand ambient light
sensor button correctly.
--
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 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Jonathan Cameron @ 2013-09-15 10:56 UTC (permalink / raw)
To: Juergen Beisert
Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, marex-ynQEQJNshbs,
fabio.estevam-KZfg59tc24xl57MIdRCFDg,
jic23-KWPb1pKIrIJaa/9Udqfwiw, Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1378887511-24530-6-git-send-email-jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
On 09/11/13 09:18, Juergen Beisert wrote:
> For battery driven systems it is a very bad idea to collect the touchscreen
> data within a kernel busy loop.
>
> This change uses the features of the hardware to delay and accumulate samples in
> hardware to avoid a high interrupt and CPU load.
>
> Note: this is only tested on an i.MX23 SoC yet.
>
> Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
While this driver is placed in IIO within staging at the moment, these changes are definitely
input related. Hence I have cc'd Dmitry and the input list.
I am personaly a little uncomfortable that we have such a complex bit of input code sat
within an IIO driver but such is life.
> ---
> drivers/staging/iio/adc/mxs-lradc.c | 530 ++++++++++++++++++++++++++++++++----
> 1 file changed, 476 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index c81b186..185f068 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -129,6 +129,17 @@ enum mxs_lradc_ts {
> MXS_LRADC_TOUCHSCREEN_5WIRE,
> };
>
> +/*
> + * Touchscreen handling
> + */
> +enum lradc_ts_plate {
> + LRADC_TOUCH = 0,
> + LRADC_SAMPLE_X,
> + LRADC_SAMPLE_Y,
> + LRADC_SAMPLE_PRESSURE,
> + LRADC_SAMPLE_VALID,
> +};
> +
> struct mxs_lradc {
> struct device *dev;
> void __iomem *base;
> @@ -169,13 +180,25 @@ struct mxs_lradc {
> #define CHAN_MASK_TOUCHSCREEN_4WIRE (0xf << 2)
> #define CHAN_MASK_TOUCHSCREEN_5WIRE (0x1f << 2)
> enum mxs_lradc_ts use_touchscreen;
> - bool stop_touchscreen;
> bool use_touchbutton;
>
> struct input_dev *ts_input;
> struct work_struct ts_work;
>
> enum mxs_lradc_id soc;
> + enum lradc_ts_plate cur_plate; /* statemachine */
> + bool ts_valid;
> + unsigned ts_x_pos;
> + unsigned ts_y_pos;
> + unsigned ts_pressure;
> +
> + /* handle touchscreen's physical behaviour */
> + /* samples per coordinate */
> + unsigned over_sample_cnt;
> + /* time clocks between samples */
> + unsigned over_sample_delay;
> + /* time in clocks to wait after the plates where switched */
> + unsigned settling_delay;
> };
>
> #define LRADC_CTRL0 0x00
> @@ -227,19 +250,33 @@ struct mxs_lradc {
> #define LRADC_CH_ACCUMULATE (1 << 29)
> #define LRADC_CH_NUM_SAMPLES_MASK (0x1f << 24)
> #define LRADC_CH_NUM_SAMPLES_OFFSET 24
> +#define LRADC_CH_NUM_SAMPLES(x) \
> + ((x) << LRADC_CH_NUM_SAMPLES_OFFSET)
> #define LRADC_CH_VALUE_MASK 0x3ffff
> #define LRADC_CH_VALUE_OFFSET 0
>
> #define LRADC_DELAY(n) (0xd0 + (0x10 * (n)))
> #define LRADC_DELAY_TRIGGER_LRADCS_MASK (0xff << 24)
> #define LRADC_DELAY_TRIGGER_LRADCS_OFFSET 24
> +#define LRADC_DELAY_TRIGGER(x) \
> + (((x) << LRADC_DELAY_TRIGGER_LRADCS_OFFSET) & \
> + LRADC_DELAY_TRIGGER_LRADCS_MASK)
> #define LRADC_DELAY_KICK (1 << 20)
> #define LRADC_DELAY_TRIGGER_DELAYS_MASK (0xf << 16)
> #define LRADC_DELAY_TRIGGER_DELAYS_OFFSET 16
> +#define LRADC_DELAY_TRIGGER_DELAYS(x) \
> + (((x) << LRADC_DELAY_TRIGGER_DELAYS_OFFSET) & \
> + LRADC_DELAY_TRIGGER_DELAYS_MASK)
> #define LRADC_DELAY_LOOP_COUNT_MASK (0x1f << 11)
> #define LRADC_DELAY_LOOP_COUNT_OFFSET 11
> +#define LRADC_DELAY_LOOP(x) \
> + (((x) << LRADC_DELAY_LOOP_COUNT_OFFSET) & \
> + LRADC_DELAY_LOOP_COUNT_MASK)
> #define LRADC_DELAY_DELAY_MASK 0x7ff
> #define LRADC_DELAY_DELAY_OFFSET 0
> +#define LRADC_DELAY_DELAY(x) \
> + (((x) << LRADC_DELAY_DELAY_OFFSET) & \
> + LRADC_DELAY_DELAY_MASK)
>
> #define LRADC_CTRL4 0x140
> #define LRADC_CTRL4_LRADCSELECT_MASK(n) (0xf << ((n) * 4))
> @@ -312,6 +349,404 @@ static u32 mxs_lradc_drive_pressure(struct mxs_lradc *lradc)
> return LRADC_CTRL0_MX28_YPPSW | LRADC_CTRL0_MX28_XNNSW;
> }
>
> +static bool mxs_lradc_check_touch_event(struct mxs_lradc *lradc)
> +{
> + return !!(readl(lradc->base + LRADC_STATUS) &
> + LRADC_STATUS_TOUCH_DETECT_RAW);
> +}
> +
> +static void mxs_lradc_setup_ts_channel(struct mxs_lradc *lradc, unsigned ch)
> +{
> + /*
> + * prepare for oversampling conversion
> + *
> + * from the datasheet:
> + * "The ACCUMULATE bit in the appropriate channel register
> + * HW_LRADC_CHn must be set to 1 if NUM_SAMPLES is greater then 0;
> + * otherwise, the IRQs will not fire."
> + */
> + mxs_lradc_reg(lradc, LRADC_CH_ACCUMULATE |
> + LRADC_CH_NUM_SAMPLES(lradc->over_sample_cnt - 1),
> + LRADC_CH(ch));
> +
> + /* from the datasheet:
> + * "Software must clear this register in preparation for a
> + * multi-cycle accumulation.
> + */
> + mxs_lradc_reg_clear(lradc, LRADC_CH_VALUE_MASK, LRADC_CH(ch));
> +
> + /* prepare the delay/loop unit according to the oversampling count */
> + mxs_lradc_reg(lradc, LRADC_DELAY_TRIGGER(1 << ch) |
> + LRADC_DELAY_TRIGGER_DELAYS(0) |
> + LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
> + LRADC_DELAY_DELAY(lradc->over_sample_delay - 1),
> + LRADC_DELAY(3));
> +
> + mxs_lradc_reg_clear(lradc, LRADC_CTRL1_LRADC_IRQ(2) |
> + LRADC_CTRL1_LRADC_IRQ(3) | LRADC_CTRL1_LRADC_IRQ(4) |
> + LRADC_CTRL1_LRADC_IRQ(5), LRADC_CTRL1);
> +
> + /* wake us again, when the complete conversion is done */
> + mxs_lradc_reg_set(lradc, LRADC_CTRL1_LRADC_IRQ_EN(ch), LRADC_CTRL1);
> + /*
> + * after changing the touchscreen plates setting
> + * the signals need some initial time to settle. Start the
> + * SoC's delay unit and start the conversion later
> + * and automatically.
> + */
> + mxs_lradc_reg(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
> + LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
> + LRADC_DELAY_KICK |
> + LRADC_DELAY_DELAY(lradc->settling_delay),
> + LRADC_DELAY(2));
> +}
> +
> +/*
> + * Pressure detection is special:
> + * We want to do both required measurements for the pressure detection in
> + * one turn. Use the hardware features to chain both conversions and let the
> + * hardware report one interrupt if both conversions are done
> + */
> +static void mxs_lradc_setup_ts_pressure(struct mxs_lradc *lradc, unsigned ch1,
> + unsigned ch2)
> +{
> + u32 reg;
> +
> + /*
> + * prepare for oversampling conversion
> + *
> + * from the datasheet:
> + * "The ACCUMULATE bit in the appropriate channel register
> + * HW_LRADC_CHn must be set to 1 if NUM_SAMPLES is greater then 0;
> + * otherwise, the IRQs will not fire."
> + */
> + reg = LRADC_CH_ACCUMULATE |
> + LRADC_CH_NUM_SAMPLES(lradc->over_sample_cnt - 1);
> + mxs_lradc_reg(lradc, reg, LRADC_CH(ch1));
> + mxs_lradc_reg(lradc, reg, LRADC_CH(ch2));
> +
> + /* from the datasheet:
> + * "Software must clear this register in preparation for a
> + * multi-cycle accumulation.
> + */
> + mxs_lradc_reg_clear(lradc, LRADC_CH_VALUE_MASK, LRADC_CH(ch1));
> + mxs_lradc_reg_clear(lradc, LRADC_CH_VALUE_MASK, LRADC_CH(ch2));
> +
> + /* prepare the delay/loop unit according to the oversampling count */
> + mxs_lradc_reg(lradc, LRADC_DELAY_TRIGGER(1 << ch1) |
> + LRADC_DELAY_TRIGGER(1 << ch2) | /* start both channels */
> + LRADC_DELAY_TRIGGER_DELAYS(0) |
> + LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
> + LRADC_DELAY_DELAY(lradc->over_sample_delay - 1),
> + LRADC_DELAY(3));
> +
> + mxs_lradc_reg_clear(lradc, LRADC_CTRL1_LRADC_IRQ(2) |
> + LRADC_CTRL1_LRADC_IRQ(3) | LRADC_CTRL1_LRADC_IRQ(4) |
> + LRADC_CTRL1_LRADC_IRQ(5), LRADC_CTRL1);
> +
> + /* wake us again, when the conversions are done */
> + mxs_lradc_reg_set(lradc, LRADC_CTRL1_LRADC_IRQ_EN(ch2), LRADC_CTRL1);
> + /*
> + * after changing the touchscreen plates setting
> + * the signals need some initial time to settle. Start the
> + * SoC's delay unit and start the conversion later
> + * and automatically.
> + */
> + mxs_lradc_reg(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
> + LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
> + LRADC_DELAY_KICK |
> + LRADC_DELAY_DELAY(lradc->settling_delay), LRADC_DELAY(2));
> +}
> +
> +static unsigned mxs_lradc_read_raw_channel(struct mxs_lradc *lradc,
> + unsigned channel)
> +{
> + u32 reg;
> + unsigned num_samples, val;
> +
> + reg = readl(lradc->base + LRADC_CH(channel));
> + if (reg & LRADC_CH_ACCUMULATE)
> + num_samples = lradc->over_sample_cnt;
> + else
> + num_samples = 1;
> +
> + val = (reg & LRADC_CH_VALUE_MASK) >> LRADC_CH_VALUE_OFFSET;
> + return val / num_samples;
> +}
> +
> +static unsigned mxs_lradc_read_ts_pressure(struct mxs_lradc *lradc,
> + unsigned ch1, unsigned ch2)
> +{
> + u32 reg, mask;
> + unsigned pressure, m1, m2;
> +
> + mask = LRADC_CTRL1_LRADC_IRQ(ch1) | LRADC_CTRL1_LRADC_IRQ(ch2);
> + reg = readl(lradc->base + LRADC_CTRL1) & mask;
> +
> + while (reg != mask) {
> + reg = readl(lradc->base + LRADC_CTRL1) & mask;
> + dev_dbg(lradc->dev, "One channel is still busy: %X\n", reg);
> + }
> +
> + m1 = mxs_lradc_read_raw_channel(lradc, ch1);
> + m2 = mxs_lradc_read_raw_channel(lradc, ch2);
> +
> + if (m2 == 0) {
> + dev_warn(lradc->dev, "Cannot calculate pressure\n");
> + return 1 << (LRADC_RESOLUTION - 1);
> + }
> +
> + /* simply scale the value from 0 ... max ADC resolution */
> + pressure = m1;
> + pressure *= (1 << LRADC_RESOLUTION);
> + pressure /= m2;
> +
> + dev_dbg(lradc->dev, "Pressure = %u\n", pressure);
> + return pressure;
> +}
> +
> +#define TS_CH_XP 2
> +#define TS_CH_YP 3
> +#define TS_CH_XM 4
> +#define TS_CH_YM 5
> +
> +static int mxs_lradc_read_ts_channel(struct mxs_lradc *lradc)
> +{
> + u32 reg;
> + int val;
> +
> + reg = readl(lradc->base + LRADC_CTRL1);
> +
> + /* only channels 3 to 5 are of interest here */
> + if (reg & LRADC_CTRL1_LRADC_IRQ(TS_CH_YP)) {
> + mxs_lradc_reg_clear(lradc, LRADC_CTRL1_LRADC_IRQ_EN(TS_CH_YP) |
> + LRADC_CTRL1_LRADC_IRQ(TS_CH_YP), LRADC_CTRL1);
> + val = mxs_lradc_read_raw_channel(lradc, TS_CH_YP);
> + } else if (reg & LRADC_CTRL1_LRADC_IRQ(TS_CH_XM)) {
> + mxs_lradc_reg_clear(lradc, LRADC_CTRL1_LRADC_IRQ_EN(TS_CH_XM) |
> + LRADC_CTRL1_LRADC_IRQ(TS_CH_XM), LRADC_CTRL1);
> + val = mxs_lradc_read_raw_channel(lradc, TS_CH_XM);
> + } else if (reg & LRADC_CTRL1_LRADC_IRQ(TS_CH_YM)) {
> + mxs_lradc_reg_clear(lradc, LRADC_CTRL1_LRADC_IRQ_EN(TS_CH_YM) |
> + LRADC_CTRL1_LRADC_IRQ(TS_CH_YM), LRADC_CTRL1);
> + val = mxs_lradc_read_raw_channel(lradc, TS_CH_YM);
> + } else {
> + return -EIO;
> + }
> +
> + mxs_lradc_reg(lradc, 0, LRADC_DELAY(2));
> + mxs_lradc_reg(lradc, 0, LRADC_DELAY(3));
> +
> + return val;
> +}
> +
> +/*
> + * YP(open)--+-------------+
> + * | |--+
> + * | | |
> + * YM(-)--+-------------+ |
> + * +--------------+
> + * | |
> + * XP(weak+) XM(open)
> + *
> + * "weak+" means 200k Ohm VDDIO
> + * (-) means GND
> + */
> +static void mxs_lradc_setup_touch_detection(struct mxs_lradc *lradc)
> +{
> + /*
> + * In order to detect a touch event the 'touch detect enable' bit
> + * enables:
> + * - a weak pullup to the X+ connector
> + * - a strong ground at the Y- connector
> + */
> + mxs_lradc_reg_clear(lradc, mxs_lradc_plate_mask(lradc), LRADC_CTRL0);
> + mxs_lradc_reg_set(lradc, mxs_lradc_touch_detect_bit(lradc),
> + LRADC_CTRL0);
> +}
> +
> +/*
> + * YP(meas)--+-------------+
> + * | |--+
> + * | | |
> + * YM(open)--+-------------+ |
> + * +--------------+
> + * | |
> + * XP(+) XM(-)
> + *
> + * (+) means here 1.85 V
> + * (-) means here GND
> + */
> +static void mxs_lradc_prepare_x_pos(struct mxs_lradc *lradc)
> +{
> + mxs_lradc_reg_clear(lradc, mxs_lradc_plate_mask(lradc), LRADC_CTRL0);
> + mxs_lradc_reg_set(lradc, mxs_lradc_drive_x_plate(lradc), LRADC_CTRL0);
> +
> + lradc->cur_plate = LRADC_SAMPLE_X;
> + mxs_lradc_setup_ts_channel(lradc, TS_CH_YP);
> +}
> +
> +/*
> + * YP(+)--+-------------+
> + * | |--+
> + * | | |
> + * YM(-)--+-------------+ |
> + * +--------------+
> + * | |
> + * XP(open) XM(meas)
> + *
> + * (+) means here 1.85 V
> + * (-) means here GND
> + */
> +static void mxs_lradc_prepare_y_pos(struct mxs_lradc *lradc)
> +{
> + mxs_lradc_reg_clear(lradc, mxs_lradc_plate_mask(lradc), LRADC_CTRL0);
> + mxs_lradc_reg_set(lradc, mxs_lradc_drive_y_plate(lradc), LRADC_CTRL0);
> +
> + lradc->cur_plate = LRADC_SAMPLE_Y;
> + mxs_lradc_setup_ts_channel(lradc, TS_CH_XM);
> +}
> +
> +/*
> + * YP(+)--+-------------+
> + * | |--+
> + * | | |
> + * YM(meas)--+-------------+ |
> + * +--------------+
> + * | |
> + * XP(meas) XM(-)
> + *
> + * (+) means here 1.85 V
> + * (-) means here GND
> + */
> +static void mxs_lradc_prepare_pressure(struct mxs_lradc *lradc)
> +{
> + mxs_lradc_reg_clear(lradc, mxs_lradc_plate_mask(lradc), LRADC_CTRL0);
> + mxs_lradc_reg_set(lradc, mxs_lradc_drive_pressure(lradc), LRADC_CTRL0);
> +
> + lradc->cur_plate = LRADC_SAMPLE_PRESSURE;
> + mxs_lradc_setup_ts_pressure(lradc, TS_CH_XP, TS_CH_YM);
> +}
> +
> +static void mxs_lradc_enable_touch_detection(struct mxs_lradc *lradc)
> +{
> + mxs_lradc_setup_touch_detection(lradc);
> +
> + lradc->cur_plate = LRADC_TOUCH;
> + mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ |
> + LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
> + mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
> +}
> +
> +static void mxs_lradc_report_ts_event(struct mxs_lradc *lradc)
> +{
> + input_report_abs(lradc->ts_input, ABS_X, lradc->ts_x_pos);
> + input_report_abs(lradc->ts_input, ABS_Y, lradc->ts_y_pos);
> + input_report_abs(lradc->ts_input, ABS_PRESSURE, lradc->ts_pressure);
> + input_report_key(lradc->ts_input, BTN_TOUCH, 1);
> + input_sync(lradc->ts_input);
> +}
> +
> +static void mxs_lradc_complete_touch_event(struct mxs_lradc *lradc)
> +{
> + mxs_lradc_setup_touch_detection(lradc);
> + lradc->cur_plate = LRADC_SAMPLE_VALID;
> + /*
> + * start a dummy conversion to burn time to settle the signals
> + * note: we are not interested in the conversion's value
> + */
> + mxs_lradc_reg(lradc, 0, LRADC_CH(5));
> + mxs_lradc_reg_clear(lradc, LRADC_CTRL1_LRADC_IRQ(5), LRADC_CTRL1);
> + mxs_lradc_reg_set(lradc, LRADC_CTRL1_LRADC_IRQ_EN(5), LRADC_CTRL1);
> + mxs_lradc_reg(lradc, LRADC_DELAY_TRIGGER(1 << 5) |
> + LRADC_DELAY_KICK | LRADC_DELAY_DELAY(10), /* waste 5 ms */
> + LRADC_DELAY(2));
> +}
> +
> +/*
> + * in order to avoid false measurements, report only samples where
> + * the surface is still touched after the position measurement
> + */
> +static void mxs_lradc_finish_touch_event(struct mxs_lradc *lradc, bool valid)
> +{
> + /* if it is still touched, report the sample */
> + if (valid && mxs_lradc_check_touch_event(lradc)) {
> + lradc->ts_valid = true;
> + mxs_lradc_report_ts_event(lradc);
> + }
> +
> + /* if it is even still touched, continue with the next measurement */
> + if (mxs_lradc_check_touch_event(lradc)) {
> + mxs_lradc_prepare_y_pos(lradc);
> + return;
> + }
> +
> + if (lradc->ts_valid) {
> + /* signal the release */
> + lradc->ts_valid = false;
> + input_report_key(lradc->ts_input, BTN_TOUCH, 0);
> + input_sync(lradc->ts_input);
> + }
> +
> + /* if it is released, wait for the next touch via IRQ */
> + mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ, LRADC_CTRL1);
> + mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
> +}
> +
> +/* touchscreen's state machine */
> +static void mxs_lradc_handle_touch(struct mxs_lradc *lradc)
> +{
> + int val;
> +
> + switch (lradc->cur_plate) {
> + case LRADC_TOUCH:
> + /*
> + * start with the Y-pos, because it uses nearly the same plate
> + * settings like the touch detection
> + */
> + if (mxs_lradc_check_touch_event(lradc)) {
> + mxs_lradc_reg_clear(lradc,
> + LRADC_CTRL1_TOUCH_DETECT_IRQ_EN,
> + LRADC_CTRL1);
> + mxs_lradc_prepare_y_pos(lradc);
> + }
> + mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ,
> + LRADC_CTRL1);
> + return;
> +
> + case LRADC_SAMPLE_Y:
> + val = mxs_lradc_read_ts_channel(lradc);
> + if (val < 0) {
> + mxs_lradc_enable_touch_detection(lradc); /* re-start */
> + return;
> + }
> + lradc->ts_y_pos = val;
> + mxs_lradc_prepare_x_pos(lradc);
> + return;
> +
> + case LRADC_SAMPLE_X:
> + val = mxs_lradc_read_ts_channel(lradc);
> + if (val < 0) {
> + mxs_lradc_enable_touch_detection(lradc); /* re-start */
> + return;
> + }
> + lradc->ts_x_pos = val;
> + mxs_lradc_prepare_pressure(lradc);
> + return;
> +
> + case LRADC_SAMPLE_PRESSURE:
> + lradc->ts_pressure =
> + mxs_lradc_read_ts_pressure(lradc, TS_CH_XP, TS_CH_YM);
> + mxs_lradc_complete_touch_event(lradc);
> + return;
> +
> + case LRADC_SAMPLE_VALID:
> + val = mxs_lradc_read_ts_channel(lradc); /* ignore the value */
> + mxs_lradc_finish_touch_event(lradc, 1);
> + break;
> + }
> +}
> +
> /*
> * Raw I/O operations
> */
> @@ -385,15 +820,6 @@ static const struct iio_info mxs_lradc_iio_info = {
> .read_raw = mxs_lradc_read_raw,
> };
>
> -/*
> - * Touchscreen handling
> - */
> -enum lradc_ts_plate {
> - LRADC_SAMPLE_X,
> - LRADC_SAMPLE_Y,
> - LRADC_SAMPLE_PRESSURE,
> -};
> -
> static int mxs_lradc_ts_touched(struct mxs_lradc *lradc)
> {
> uint32_t reg;
> @@ -551,10 +977,6 @@ static void mxs_lradc_ts_work(struct work_struct *ts_work)
> input_report_key(lradc->ts_input, BTN_TOUCH, 0);
> input_sync(lradc->ts_input);
>
> - /* Do not restart the TS IRQ if the driver is shutting down. */
> - if (lradc->stop_touchscreen)
> - return;
> -
> /* Restart the touchscreen interrupts. */
> mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ, LRADC_CTRL1);
> mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
> @@ -564,36 +986,29 @@ static int mxs_lradc_ts_open(struct input_dev *dev)
> {
> struct mxs_lradc *lradc = input_get_drvdata(dev);
>
> - /* The touchscreen is starting. */
> - lradc->stop_touchscreen = false;
> -
> /* Enable the touch-detect circuitry. */
> - mxs_lradc_reg_set(lradc, mxs_lradc_touch_detect_bit(lradc),
> - LRADC_CTRL0);
> -
> - /* Enable the touch-detect IRQ. */
> - mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
> + mxs_lradc_enable_touch_detection(lradc);
>
> return 0;
> }
>
> -static void mxs_lradc_ts_close(struct input_dev *dev)
> +static void mxs_lradc_disable_ts(struct mxs_lradc *lradc)
> {
> - struct mxs_lradc *lradc = input_get_drvdata(dev);
> -
> - /* Indicate the touchscreen is stopping. */
> - lradc->stop_touchscreen = true;
> - mb();
> + /* stop all interrupts from firing */
> + mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN |
> + LRADC_CTRL1_LRADC_IRQ_EN(2) | LRADC_CTRL1_LRADC_IRQ_EN(3) |
> + LRADC_CTRL1_LRADC_IRQ_EN(4) | LRADC_CTRL1_LRADC_IRQ_EN(5),
> + LRADC_CTRL1);
>
> - /* Wait until touchscreen thread finishes any possible remnants. */
> - cancel_work_sync(&lradc->ts_work);
> + /* Power-down touchscreen touch-detect circuitry. */
> + mxs_lradc_reg_clear(lradc, mxs_lradc_plate_mask(lradc), LRADC_CTRL0);
> +}
>
> - /* Disable touchscreen touch-detect IRQ. */
> - mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN,
> - LRADC_CTRL1);
> +static void mxs_lradc_ts_close(struct input_dev *dev)
> +{
> + struct mxs_lradc *lradc = input_get_drvdata(dev);
>
> - /* Power-down touchscreen touch-detect circuitry. */
> - mxs_lradc_reg_clear(lradc, mxs_lradc_touch_detect_bit(lradc), LRADC_CTRL0);
> + mxs_lradc_disable_ts(lradc);
> }
>
> static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
> @@ -641,6 +1056,7 @@ static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
>
> cancel_work_sync(&lradc->ts_work);
>
> + mxs_lradc_disable_ts(lradc);
> input_unregister_device(lradc->ts_input);
> }
>
> @@ -653,30 +1069,23 @@ static irqreturn_t mxs_lradc_handle_irq(int irq, void *data)
> struct mxs_lradc *lradc = iio_priv(iio);
> unsigned long reg = readl(lradc->base + LRADC_CTRL1);
> const uint32_t ts_irq_mask =
> - LRADC_CTRL1_TOUCH_DETECT_IRQ_EN |
> - LRADC_CTRL1_TOUCH_DETECT_IRQ;
> + LRADC_CTRL1_TOUCH_DETECT_IRQ |
> + LRADC_CTRL1_LRADC_IRQ(2) |
> + LRADC_CTRL1_LRADC_IRQ(3) |
> + LRADC_CTRL1_LRADC_IRQ(4) |
> + LRADC_CTRL1_LRADC_IRQ(5);
>
> if (!(reg & mxs_lradc_irq_mask(lradc)))
> return IRQ_NONE;
>
> - /*
> - * Touchscreen IRQ handling code has priority and therefore
> - * is placed here. In case touchscreen IRQ arrives, disable
> - * it ASAP
> - */
> - if (reg & LRADC_CTRL1_TOUCH_DETECT_IRQ) {
> - mxs_lradc_reg_clear(lradc, ts_irq_mask, LRADC_CTRL1);
> - if (!lradc->stop_touchscreen)
> - schedule_work(&lradc->ts_work);
> - }
> + if (lradc->use_touchscreen && (reg & ts_irq_mask))
> + mxs_lradc_handle_touch(lradc);
>
> if (iio_buffer_enabled(iio))
> iio_trigger_poll(iio->trig, iio_get_time_ns());
> else if (reg & LRADC_CTRL1_LRADC_IRQ(0))
> complete(&lradc->completion);
>
> - mxs_lradc_reg_clear(lradc, reg & mxs_lradc_irq_mask(lradc), LRADC_CTRL1);
> -
> return IRQ_HANDLED;
> }
>
> @@ -971,6 +1380,17 @@ static const struct of_device_id mxs_lradc_dt_ids[] = {
> };
> MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
>
> +static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
> + struct device_node *lradc_node)
> +{
> + /* TODO retrieve from device tree */
> + lradc->over_sample_cnt = 4;
> + lradc->over_sample_delay = 2;
> + lradc->settling_delay = 10;
> +
> + return 0;
> +}
> +
> static int mxs_lradc_probe(struct platform_device *pdev)
> {
> const struct of_device_id *of_id =
> @@ -983,7 +1403,7 @@ static int mxs_lradc_probe(struct platform_device *pdev)
> struct iio_dev *iio;
> struct resource *iores;
> uint32_t ts_wires = 0;
> - int ret = 0;
> + int ret = 0, touch_ret;
> int i;
>
> /* Allocate the IIO device. */
> @@ -1003,7 +1423,7 @@ static int mxs_lradc_probe(struct platform_device *pdev)
> if (IS_ERR(lradc->base))
> return PTR_ERR(lradc->base);
>
> - INIT_WORK(&lradc->ts_work, mxs_lradc_ts_work);
> + touch_ret = mxs_lradc_probe_touchscreen(lradc, node);
>
> /* Check if touchscreen is enabled in DT. */
> ret = of_property_read_u32(node, "fsl,lradc-touchscreen-wires",
> @@ -1066,9 +1486,11 @@ static int mxs_lradc_probe(struct platform_device *pdev)
> goto err_dev;
>
> /* Register the touchscreen input device. */
> - ret = mxs_lradc_ts_register(lradc);
> - if (ret)
> - goto err_dev;
> + if (touch_ret == 0) {
> + ret = mxs_lradc_ts_register(lradc);
> + if (ret)
> + goto err_dev;
> + }
>
> /* Register IIO device. */
> ret = iio_device_register(iio);
>
^ permalink raw reply
* Re: [PATCH 3/3] HID RTC: Open sensor hub open close
From: Jonathan Cameron @ 2013-09-15 10:18 UTC (permalink / raw)
To: Srinivas Pandruvada
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, jc23-KWPb1pKIrIJaa/9Udqfwiw,
jkosina-AlSwsSmVLrQ, holler-SXC+2es9fhnfWeYVQQPykw,
Alessandro Zummo, rtc-linux-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <1378843432-5113-3-git-send-email-srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
cc'd Alessandro and rtc list as this is within RTC. I'll happilly take
it through IIO given the dependency, but I would like an Ack from the RTC
side.
On 09/10/13 21:03, Srinivas Pandruvada wrote:
> Open sensor hub when module is loaded and close when module is removed.
> This helps saving power by opening HID transport only when there is an
> user.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
> drivers/rtc/rtc-hid-sensor-time.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
> index 7273b01..1fe170c 100644
> --- a/drivers/rtc/rtc-hid-sensor-time.c
> +++ b/drivers/rtc/rtc-hid-sensor-time.c
> @@ -279,15 +279,28 @@ static int hid_time_probe(struct platform_device *pdev)
> return ret;
> }
>
> + ret = sensor_hub_device_open(hsdev);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to open sensor hub device!\n");
> + goto err_open;
> + }
> +
> time_state->rtc = devm_rtc_device_register(&pdev->dev,
> "hid-sensor-time", &hid_time_rtc_ops,
> THIS_MODULE);
>
> if (IS_ERR(time_state->rtc)) {
> dev_err(&pdev->dev, "rtc device register failed!\n");
> - return PTR_ERR(time_state->rtc);
> + ret = PTR_ERR(time_state->rtc);
> + goto err_rtc;
> }
>
> + return 0;
> +
> +err_rtc:
> + sensor_hub_device_close(hsdev);
> +err_open:
> + sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
> return ret;
> }
>
> @@ -295,6 +308,7 @@ static int hid_time_remove(struct platform_device *pdev)
> {
> struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
>
> + sensor_hub_device_close(hsdev);
> sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
>
> return 0;
>
^ permalink raw reply
* Re: [PATCH 1/3] HID: Delay opening HID device
From: Jonathan Cameron @ 2013-09-15 10:15 UTC (permalink / raw)
To: Srinivas Pandruvada
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, jkosina-AlSwsSmVLrQ,
holler-SXC+2es9fhnfWeYVQQPykw
In-Reply-To: <1378843432-5113-1-git-send-email-srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On 09/10/13 21:03, Srinivas Pandruvada wrote:
> Don't call hid_open_device till there is actually an user. This saves
> power by not opening underlying transport for HID. Also close device
> if there are no active mfd client using HID sensor hub.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Buglet inline.
> ---
> drivers/hid/hid-sensor-hub.c | 45 ++++++++++++++++++++++++++++++++----------
> include/linux/hid-sensor-hub.h | 18 +++++++++++++++++
> 2 files changed, 53 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 10e1581..8effdee 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -465,6 +465,39 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
> return 1;
> }
>
> +int sensor_hub_device_open(struct hid_sensor_hub_device *hsdev)
> +{
> + int ret;
> + struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
> +
> + mutex_lock(&data->mutex);
> + if (!hsdev->ref_cnt) {
> + ret = hid_hw_open(hsdev->hdev);
> + if (ret) {
> + hid_err(hsdev->hdev, "failed to open hid device\n");
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> + }
> + hsdev->ref_cnt++;
> + mutex_unlock(&data->mutex);
> +
> + return ret;
Not initialized.
> +}
> +EXPORT_SYMBOL_GPL(sensor_hub_device_open);
> +
> +void sensor_hub_device_close(struct hid_sensor_hub_device *hsdev)
> +{
> + struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
> +
> + mutex_lock(&data->mutex);
> + hsdev->ref_cnt--;
> + if (!hsdev->ref_cnt)
> + hid_hw_close(hsdev->hdev);
> + mutex_unlock(&data->mutex);
> +}
> +EXPORT_SYMBOL_GPL(sensor_hub_device_close);
> +
> static int sensor_hub_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
> {
> @@ -506,12 +539,6 @@ static int sensor_hub_probe(struct hid_device *hdev,
> hid_err(hdev, "hw start failed\n");
> return ret;
> }
> - ret = hid_hw_open(hdev);
> - if (ret) {
> - hid_err(hdev, "failed to open input interrupt pipe\n");
> - goto err_stop_hw;
> - }
> -
> INIT_LIST_HEAD(&sd->dyn_callback_list);
> sd->hid_sensor_client_cnt = 0;
> report_enum = &hdev->report_enum[HID_INPUT_REPORT];
> @@ -520,7 +547,7 @@ static int sensor_hub_probe(struct hid_device *hdev,
> if (dev_cnt > HID_MAX_PHY_DEVICES) {
> hid_err(hdev, "Invalid Physical device count\n");
> ret = -EINVAL;
> - goto err_close;
> + goto err_stop_hw;
> }
> sd->hid_sensor_hub_client_devs = kzalloc(dev_cnt *
> sizeof(struct mfd_cell),
> @@ -528,7 +555,7 @@ static int sensor_hub_probe(struct hid_device *hdev,
> if (sd->hid_sensor_hub_client_devs == NULL) {
> hid_err(hdev, "Failed to allocate memory for mfd cells\n");
> ret = -ENOMEM;
> - goto err_close;
> + goto err_stop_hw;
> }
> list_for_each_entry(report, &report_enum->report_list, list) {
> hid_dbg(hdev, "Report id:%x\n", report->id);
> @@ -565,8 +592,6 @@ err_free_names:
> for (i = 0; i < sd->hid_sensor_client_cnt ; ++i)
> kfree(sd->hid_sensor_hub_client_devs[i].name);
> kfree(sd->hid_sensor_hub_client_devs);
> -err_close:
> - hid_hw_close(hdev);
> err_stop_hw:
> hid_hw_stop(hdev);
>
> diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
> index 32ba451..a265af2 100644
> --- a/include/linux/hid-sensor-hub.h
> +++ b/include/linux/hid-sensor-hub.h
> @@ -47,11 +47,13 @@ struct hid_sensor_hub_attribute_info {
> * @hdev: Stores the hid instance.
> * @vendor_id: Vendor id of hub device.
> * @product_id: Product id of hub device.
> + * @ref_cnt: Number of MFD clients have opened this device
> */
> struct hid_sensor_hub_device {
> struct hid_device *hdev;
> u32 vendor_id;
> u32 product_id;
> + int ref_cnt;
> };
>
> /**
> @@ -74,6 +76,22 @@ struct hid_sensor_hub_callbacks {
> void *priv);
> };
>
> +/**
> +* sensor_hub_device_open() - Open hub device
> +* @hsdev: Hub device instance.
> +*
> +* Used to open hid device for sensor hub.
> +*/
> +int sensor_hub_device_open(struct hid_sensor_hub_device *hsdev);
> +
> +/**
> +* sensor_hub_device_clode() - Close hub device
> +* @hsdev: Hub device instance.
> +*
> +* Used to clode hid device for sensor hub.
> +*/
> +void sensor_hub_device_close(struct hid_sensor_hub_device *hsdev);
> +
> /* Registration functions */
>
> /**
>
^ permalink raw reply
* [git pull] Input updates for 3.12-rc0
From: Dmitry Torokhov @ 2013-09-15 5:34 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 830 bytes --]
Hi Linus,
Please pull from:
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus
or
master.kernel.org:/pub/scm/linux/kernel/git/dtor/input.git for-linus
to receive updates for the input subsystem. The only change is David
Hermann's new EVIOCREVOKE evdev ioctl that allows safely passing file
descriptors to input devices to session processes and later being able
to stop delivery of events through these fds so that inactive sessions
will no longer receive user input that does not belong to them.
Changelog:
---------
David Herrmann (1):
Input: evdev - add EVIOCREVOKE ioctl
Diffstat:
--------
drivers/input/evdev.c | 37 +++++++++++++++++++++++++++++++------
include/uapi/linux/input.h | 1 +
2 files changed, 32 insertions(+), 6 deletions(-)
--
Dmitry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] Input: gpio_keys - wakeup_trigger
From: Tomasz Figa @ 2013-09-14 12:16 UTC (permalink / raw)
To: Benson Leung
Cc: dmitry.torokhov, linux-kernel, linux-input, linux-gpio,
devicetree, david, dianders
In-Reply-To: <1379109160-32437-1-git-send-email-bleung@chromium.org>
Hi Benson,
On Friday 13 of September 2013 14:52:40 Benson Leung wrote:
> Allow wakeup_trigger to be defined per gpio button. Currently, all
> gpio buttons are set up as IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.
> It may be more appropriate to only wake the system on one edge, for
> example if the gpio is for a Lid Switch.
>
> Signed-off-by: Benson Leung <bleung@chromium.org>
> ---
> .../devicetree/bindings/gpio/gpio_keys.txt | 7 +++++++
> drivers/input/keyboard/gpio_keys.c | 23
> ++++++++++++++++++++-- include/linux/gpio_keys.h
> | 3 +++
> 3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio_keys.txt
> b/Documentation/devicetree/bindings/gpio/gpio_keys.txt index
> 5c2c021..243f569 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio_keys.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio_keys.txt
> @@ -20,6 +20,13 @@ Optional subnode-properties:
> - debounce-interval: Debouncing interval time in milliseconds.
> If not specified defaults to 5.
> - gpio-key,wakeup: Boolean, button can wake-up the system.
> + - gpio-key,wakeup-trigger : Specifies the type of wakeup behavior.
> + <1> == Rising Edge Trigger
> + <2> == Falling Edge Trigger
> + <3> == Both Rising and Falling Edge Trigger
> + <4> == Level High Trigger
> + <8> == Level Low Trigger
> + If not specified, defaults to <3> == Both Rising and Falling.
I don't like two things in this patch.
First is that this looks completely like a configuration option, not
hardware description, so it's not something that should be put into DT.
Especially that users might want to use another wake-up trigger depending
on their use cases. I'd rather see this as a sysfs entry.
Another thing is that this driver assumes that key events are indicated by
edges on the GPIO line, so I don't think level trigger setting make any
sense here. I'd rather allow three settings here (through a sysfs knob) -
key down, key up, key down or up.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH] Input: gpio_keys - wakeup_trigger
From: Doug Anderson @ 2013-09-13 23:56 UTC (permalink / raw)
To: Benson Leung
Cc: Dmitry Torokhov,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, david-/Q/L1SwJa3aEVqv0pETR8A
In-Reply-To: <1379109160-32437-1-git-send-email-bleung-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Benson,
On Fri, Sep 13, 2013 at 2:52 PM, Benson Leung <bleung-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> Allow wakeup_trigger to be defined per gpio button. Currently, all
> gpio buttons are set up as IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.
> It may be more appropriate to only wake the system on one edge, for example
> if the gpio is for a Lid Switch.
>
> Signed-off-by: Benson Leung <bleung-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
As discussed out of band, I'd tend to put the masking/error checking
in gpio_keys_get_devtree_pdata() then just rely on non-DT users not to
pass in nonsense, but I don't feel that strongly about it, so:
Reviewed-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ 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