* [PATCH] HID: quirks: Fix keyboard + touchpad on Toshiba Click Mini not working
From: Hans de Goede @ 2018-01-17 20:05 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: Hans de Goede, linux-input, stable
The Toshiba Click Mini uses an i2c attached keyboard/touchpad combo
(single i2c_hid device for both) which has a vid:pid of 04F3:0401,
which is also used by a bunch of Elan touchpads which are handled by the
drivers/input/mouse/elan_i2c driver, but that driver deals with pure
touchpads and does not work for a combo device such as the one on the
Toshiba Click Mini.
The combo on the Mini has an ACPI id of ELAN0800, which is not claimed
by the elan_i2c driver, so check for that and if it is found do not ignore
the device. This fixes the keyboard/touchpad combo on the Mini not working
(although with the touchpad in mouse emulation mode).
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-quirks.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 1cf1e9a0d699..ed4b30bbdde3 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -729,7 +729,6 @@ static const struct hid_device_id hid_ignore_list[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EARTHMATE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EM_LT20) },
{ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, 0x0400) },
- { HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, 0x0401) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ESSENTIAL_REALITY, USB_DEVICE_ID_ESSENTIAL_REALITY_P5) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ETT, USB_DEVICE_ID_TC5UH) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ETT, USB_DEVICE_ID_TC4UM) },
@@ -997,6 +996,17 @@ bool hid_ignore(struct hid_device *hdev)
strncmp(hdev->name, "www.masterkit.ru MA901", 22) == 0)
return true;
break;
+ case USB_VENDOR_ID_ELAN:
+ /*
+ * Many Elan devices have a product id of 0x0401 and are handled
+ * by the elan_i2c input driver. But the ACPI HID ELAN0800 dev
+ * is not (and cannot be) handled by that driver ->
+ * Ignore all 0x0401 devs except for the ELAN0800 dev.
+ */
+ if (hdev->product == 0x0401 &&
+ strncmp(hdev->name, "ELAN0800", 8) != 0)
+ return true;
+ break;
}
if (hdev->type == HID_TYPE_USBMOUSE &&
--
2.14.3
^ permalink raw reply related
* Re: [PATCH v4] input: pxrc: new driver for PhoenixRC Flight Controller Adapter
From: Marcus Folkesson @ 2018-01-17 19:45 UTC (permalink / raw)
To: Randy Dunlap
Cc: Dmitry Torokhov, Jonathan Corbet, Tomohiro Yoshidomi,
David Herrmann, Philippe Ombredanne, Kate Stewart,
Greg Kroah-Hartman, linux-input, linux-doc, linux-kernel
In-Reply-To: <9607d51a-4382-396c-a4bd-46bbb24ba46e@infradead.org>
[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]
Hi Randy!
On Wed, Jan 17, 2018 at 11:13:43AM -0800, Randy Dunlap wrote:
> On 01/17/2018 11:07 AM, Marcus Folkesson wrote:
> > This driver let you plug in your RC controller to the adapter and
> > use it as input device in various RC simulators.
> >
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > ---
>
> > diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> > index f3c2f6ea8b44..18ab6dafff41 100644
> > --- a/drivers/input/joystick/Kconfig
> > +++ b/drivers/input/joystick/Kconfig
> > @@ -351,4 +351,13 @@ config JOYSTICK_PSXPAD_SPI_FF
> >
> > To drive rumble motor a dedicated power supply is required.
> >
> > +config JOYSTICK_PXRC
> > + tristate "PhoenixRC Flight Controller Adapter"
> > + depends on USB_ARCH_HAS_HCD
> > + select USB
>
> Drop the (first) depends and change "select USB" to "depends on USB".
>
> Argh, there are lots of input/ drivers that do that. That's bad/wrong.
> A driver should not select (enable) an entire subsystem. If the user had
> the subsystem disabled, it should stay that way.
>
> Dmitry, you can expect some patches...
>
Hah, of course it should be "depends on" :-)
Other drivers fooled me...
Should I fix the other drivers too when I'm on it, or do you?
> > + help
> > + Say Y here if you want to use the PhoenixRC Flight Controller Adapter.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called pxrc.
> > endif
>
> --
> ~Randy
Best regards
Marcus Folkesson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4] input: pxrc: new driver for PhoenixRC Flight Controller Adapter
From: Randy Dunlap @ 2018-01-17 19:13 UTC (permalink / raw)
To: Marcus Folkesson, Dmitry Torokhov, Jonathan Corbet,
Tomohiro Yoshidomi, David Herrmann, Philippe Ombredanne,
Kate Stewart, Greg Kroah-Hartman
Cc: linux-input, linux-doc, linux-kernel
In-Reply-To: <20180117190716.22380-1-marcus.folkesson@gmail.com>
On 01/17/2018 11:07 AM, Marcus Folkesson wrote:
> This driver let you plug in your RC controller to the adapter and
> use it as input device in various RC simulators.
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index f3c2f6ea8b44..18ab6dafff41 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -351,4 +351,13 @@ config JOYSTICK_PSXPAD_SPI_FF
>
> To drive rumble motor a dedicated power supply is required.
>
> +config JOYSTICK_PXRC
> + tristate "PhoenixRC Flight Controller Adapter"
> + depends on USB_ARCH_HAS_HCD
> + select USB
Drop the (first) depends and change "select USB" to "depends on USB".
Argh, there are lots of input/ drivers that do that. That's bad/wrong.
A driver should not select (enable) an entire subsystem. If the user had
the subsystem disabled, it should stay that way.
Dmitry, you can expect some patches...
> + help
> + Say Y here if you want to use the PhoenixRC Flight Controller Adapter.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called pxrc.
> endif
--
~Randy
^ permalink raw reply
* [PATCH v4] input: pxrc: new driver for PhoenixRC Flight Controller Adapter
From: Marcus Folkesson @ 2018-01-17 19:07 UTC (permalink / raw)
To: Dmitry Torokhov, Jonathan Corbet, Marcus Folkesson,
Tomohiro Yoshidomi, David Herrmann, Philippe Ombredanne,
Kate Stewart, Greg Kroah-Hartman
Cc: linux-input, linux-doc, linux-kernel
This driver let you plug in your RC controller to the adapter and
use it as input device in various RC simulators.
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
v4:
- Add call to usb_mark_last_busy() in irq
- Move code from pxrc_resume() to pxrc_reset_resume()
v3:
- Use RUDDER and MISC instead of TILT_X and TILT_Y
- Drop kref and anchor
- Rework URB handling
- Add PM support
v2:
- Change module license to GPLv2 to match SPDX tag
Documentation/input/devices/pxrc.rst | 57 +++++++
drivers/input/joystick/Kconfig | 9 +
drivers/input/joystick/Makefile | 1 +
drivers/input/joystick/pxrc.c | 318 +++++++++++++++++++++++++++++++++++
4 files changed, 385 insertions(+)
create mode 100644 Documentation/input/devices/pxrc.rst
create mode 100644 drivers/input/joystick/pxrc.c
diff --git a/Documentation/input/devices/pxrc.rst b/Documentation/input/devices/pxrc.rst
new file mode 100644
index 000000000000..ca11f646bae8
--- /dev/null
+++ b/Documentation/input/devices/pxrc.rst
@@ -0,0 +1,57 @@
+=======================================================
+pxrc - PhoenixRC Flight Controller Adapter
+=======================================================
+
+:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
+
+This driver let you use your own RC controller plugged into the
+adapter that comes with PhoenixRC [1]_ or other compatible adapters.
+
+The adapter supports 7 analog channels and 1 digital input switch.
+
+Notes
+=====
+
+Many RC controllers is able to configure which stick goes to which channel.
+This is also configurable in most simulators, so a matching is not necessary.
+
+The driver is generating the following input event for analog channels:
+
++---------+----------------+
+| Channel | Event |
++=========+================+
+| 1 | ABS_X |
++---------+----------------+
+| 2 | ABS_Y |
++---------+----------------+
+| 3 | ABS_RX |
++---------+----------------+
+| 4 | ABS_RY |
++---------+----------------+
+| 5 | ABS_RUDDER |
++---------+----------------+
+| 6 | ABS_THROTTLE |
++---------+----------------+
+| 7 | ABS_MISC |
++---------+----------------+
+
+The digital input switch is generated as an `BTN_A` event.
+
+Manual Testing
+==============
+
+To test this driver's functionality you may use `input-event` which is part of
+the `input layer utilities` suite [2]_.
+
+For example::
+
+ > modprobe pxrc
+ > input-events <devnr>
+
+To print all input events from input `devnr`.
+
+References
+==========
+
+.. [1] http://www.phoenix-sim.com/
+.. [2] https://www.kraxel.org/cgit/input/
diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index f3c2f6ea8b44..18ab6dafff41 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -351,4 +351,13 @@ config JOYSTICK_PSXPAD_SPI_FF
To drive rumble motor a dedicated power supply is required.
+config JOYSTICK_PXRC
+ tristate "PhoenixRC Flight Controller Adapter"
+ depends on USB_ARCH_HAS_HCD
+ select USB
+ help
+ Say Y here if you want to use the PhoenixRC Flight Controller Adapter.
+
+ To compile this driver as a module, choose M here: the
+ module will be called pxrc.
endif
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index 67651efda2e1..dd0492ebbed7 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
obj-$(CONFIG_JOYSTICK_MAGELLAN) += magellan.o
obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o
obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
+obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o
diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
new file mode 100644
index 000000000000..216df4bf4484
--- /dev/null
+++ b/drivers/input/joystick/pxrc.c
@@ -0,0 +1,318 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Phoenix RC Flight Controller Adapter
+ *
+ * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/usb.h>
+#include <linux/usb/input.h>
+#include <linux/mutex.h>
+#include <linux/input.h>
+
+#define PXRC_VENDOR_ID (0x1781)
+#define PXRC_PRODUCT_ID (0x0898)
+
+static const struct usb_device_id pxrc_table[] = {
+ { USB_DEVICE(PXRC_VENDOR_ID, PXRC_PRODUCT_ID) },
+ { }
+};
+MODULE_DEVICE_TABLE(usb, pxrc_table);
+
+struct pxrc {
+ struct input_dev *input;
+ struct usb_device *udev;
+ struct usb_interface *intf;
+ struct urb *urb;
+ __u8 epaddr;
+ char phys[64];
+ unsigned char *data;
+ size_t bsize;
+};
+
+static void pxrc_usb_irq(struct urb *urb)
+{
+ struct pxrc *pxrc = urb->context;
+ int error;
+
+ switch (urb->status) {
+ case 0:
+ /* success */
+ break;
+ case -ETIME:
+ /* this urb is timing out */
+ dev_dbg(&pxrc->intf->dev,
+ "%s - urb timed out - was the device unplugged?\n",
+ __func__);
+ return;
+ case -ECONNRESET:
+ case -ENOENT:
+ case -ESHUTDOWN:
+ case -EPIPE:
+ /* this urb is terminated, clean up */
+ dev_dbg(&pxrc->intf->dev, "%s - urb shutting down with status: %d\n",
+ __func__, urb->status);
+ return;
+ default:
+ dev_dbg(&pxrc->intf->dev, "%s - nonzero urb status received: %d\n",
+ __func__, urb->status);
+ goto exit;
+ }
+
+ if (urb->actual_length == 8) {
+ input_report_abs(pxrc->input, ABS_X, pxrc->data[0]);
+ input_report_abs(pxrc->input, ABS_Y, pxrc->data[2]);
+ input_report_abs(pxrc->input, ABS_RX, pxrc->data[3]);
+ input_report_abs(pxrc->input, ABS_RY, pxrc->data[4]);
+ input_report_abs(pxrc->input, ABS_RUDDER, pxrc->data[5]);
+ input_report_abs(pxrc->input, ABS_THROTTLE, pxrc->data[6]);
+ input_report_abs(pxrc->input, ABS_MISC, pxrc->data[7]);
+
+ input_report_key(pxrc->input, BTN_A, pxrc->data[1]);
+ }
+
+exit:
+ usb_mark_last_busy(interface_to_usbdev(pxrc->intf));
+
+ /* Resubmit to fetch new fresh URBs */
+ error = usb_submit_urb(urb, GFP_ATOMIC);
+ if (error && error != -EPERM)
+ dev_err(&pxrc->intf->dev,
+ "%s - usb_submit_urb failed with result: %d",
+ __func__, error);
+}
+
+static int pxrc_open(struct input_dev *input)
+{
+ struct pxrc *pxrc = input_get_drvdata(input);
+ int retval;
+
+ retval = usb_autopm_get_interface(pxrc->intf);
+ if (retval) {
+ dev_err(&pxrc->intf->dev,
+ "%s - usb_autopm_get_interface failed, error: %d\n",
+ __func__, retval);
+ return retval;
+ }
+
+ retval = usb_submit_urb(pxrc->urb, GFP_KERNEL);
+ if (retval) {
+ dev_err(&pxrc->intf->dev,
+ "%s - usb_submit_urb failed, error: %d\n",
+ __func__, retval);
+ retval = -EIO;
+ goto out;
+ }
+
+ pxrc->intf->needs_remote_wakeup = 1;
+
+out:
+ usb_autopm_put_interface(pxrc->intf);
+ return retval;
+}
+
+static void pxrc_close(struct input_dev *input)
+{
+ struct pxrc *pxrc = input_get_drvdata(input);
+ int autopm_error;
+
+ autopm_error = usb_autopm_get_interface(pxrc->intf);
+
+ usb_kill_urb(pxrc->urb);
+ pxrc->intf->needs_remote_wakeup = 0;
+
+ if (!autopm_error)
+ usb_autopm_put_interface(pxrc->intf);
+}
+
+static int pxrc_usb_init(struct pxrc *pxrc)
+{
+ struct usb_endpoint_descriptor *epirq;
+ unsigned int pipe;
+ int retval;
+
+ /* Set up the endpoint information */
+ /* This device only has an interrupt endpoint */
+ retval = usb_find_common_endpoints(pxrc->intf->cur_altsetting,
+ NULL, NULL, &epirq, NULL);
+ if (retval) {
+ dev_err(&pxrc->intf->dev,
+ "Could not find endpoint\n");
+ goto error;
+ }
+
+ pxrc->bsize = usb_endpoint_maxp(epirq);
+ pxrc->epaddr = epirq->bEndpointAddress;
+ pxrc->data = devm_kmalloc(&pxrc->intf->dev, pxrc->bsize, GFP_KERNEL);
+ if (!pxrc->data) {
+ retval = -ENOMEM;
+ goto error;
+ }
+
+ usb_set_intfdata(pxrc->intf, pxrc);
+ usb_make_path(pxrc->udev, pxrc->phys, sizeof(pxrc->phys));
+ strlcat(pxrc->phys, "/input0", sizeof(pxrc->phys));
+
+ pxrc->urb = usb_alloc_urb(0, GFP_KERNEL);
+ if (!pxrc->urb) {
+ retval = -ENOMEM;
+ goto error;
+ }
+
+ pipe = usb_rcvintpipe(pxrc->udev, pxrc->epaddr),
+ usb_fill_int_urb(pxrc->urb, pxrc->udev, pipe, pxrc->data, pxrc->bsize,
+ pxrc_usb_irq, pxrc, 1);
+
+error:
+ return retval;
+
+
+}
+
+static int pxrc_input_init(struct pxrc *pxrc)
+{
+ pxrc->input = devm_input_allocate_device(&pxrc->intf->dev);
+ if (pxrc->input == NULL) {
+ dev_err(&pxrc->intf->dev, "couldn't allocate input device\n");
+ return -ENOMEM;
+ }
+
+ pxrc->input->name = "PXRC Flight Controller Adapter";
+ pxrc->input->phys = pxrc->phys;
+ usb_to_input_id(pxrc->udev, &pxrc->input->id);
+
+ pxrc->input->open = pxrc_open;
+ pxrc->input->close = pxrc_close;
+
+ input_set_capability(pxrc->input, EV_KEY, BTN_A);
+ input_set_abs_params(pxrc->input, ABS_X, 0, 255, 0, 0);
+ input_set_abs_params(pxrc->input, ABS_Y, 0, 255, 0, 0);
+ input_set_abs_params(pxrc->input, ABS_RX, 0, 255, 0, 0);
+ input_set_abs_params(pxrc->input, ABS_RY, 0, 255, 0, 0);
+ input_set_abs_params(pxrc->input, ABS_RUDDER, 0, 255, 0, 0);
+ input_set_abs_params(pxrc->input, ABS_THROTTLE, 0, 255, 0, 0);
+ input_set_abs_params(pxrc->input, ABS_MISC, 0, 255, 0, 0);
+
+ input_set_drvdata(pxrc->input, pxrc);
+
+ return input_register_device(pxrc->input);
+}
+
+static int pxrc_probe(struct usb_interface *intf,
+ const struct usb_device_id *id)
+{
+ struct pxrc *pxrc;
+ int retval;
+
+ pxrc = devm_kzalloc(&intf->dev, sizeof(*pxrc), GFP_KERNEL);
+
+ if (!pxrc)
+ return -ENOMEM;
+
+ pxrc->udev = usb_get_dev(interface_to_usbdev(intf));
+ pxrc->intf = intf;
+
+ retval = pxrc_usb_init(pxrc);
+ if (retval)
+ goto error;
+
+ retval = pxrc_input_init(pxrc);
+ if (retval)
+ goto err_free_urb;
+
+ return 0;
+
+err_free_urb:
+ usb_free_urb(pxrc->urb);
+
+error:
+ return retval;
+}
+
+static void pxrc_disconnect(struct usb_interface *intf)
+{
+ struct pxrc *pxrc = usb_get_intfdata(intf);
+
+ usb_free_urb(pxrc->urb);
+ usb_set_intfdata(intf, NULL);
+}
+
+static int pxrc_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ struct pxrc *pxrc = usb_get_intfdata(intf);
+ struct input_dev *input_dev = pxrc->input;
+
+ mutex_lock(&input_dev->mutex);
+ usb_kill_urb(pxrc->urb);
+ mutex_unlock(&input_dev->mutex);
+
+ return 0;
+}
+
+static int pxrc_resume(struct usb_interface *intf)
+{
+ return 0;
+}
+
+static int pxrc_pre_reset(struct usb_interface *intf)
+{
+ struct pxrc *pxrc = usb_get_intfdata(intf);
+ struct input_dev *input_dev = pxrc->input;
+
+ mutex_lock(&input_dev->mutex);
+ usb_kill_urb(pxrc->urb);
+ return 0;
+}
+
+static int pxrc_post_reset(struct usb_interface *intf)
+{
+ struct pxrc *pxrc = usb_get_intfdata(intf);
+ struct input_dev *input_dev = pxrc->input;
+ int retval = 0;
+
+ if (input_dev->users && usb_submit_urb(pxrc->urb, GFP_KERNEL) < 0)
+ retval = -EIO;
+
+ mutex_unlock(&input_dev->mutex);
+
+ return retval;
+}
+
+static int pxrc_reset_resume(struct usb_interface *intf)
+{
+ struct pxrc *pxrc = usb_get_intfdata(intf);
+ struct input_dev *input_dev = pxrc->input;
+ int retval = 0;
+
+ mutex_lock(&input_dev->mutex);
+ if (input_dev->users && usb_submit_urb(pxrc->urb, GFP_KERNEL) < 0)
+ retval = -EIO;
+
+ mutex_unlock(&input_dev->mutex);
+ return retval;
+}
+
+static struct usb_driver pxrc_driver = {
+ .name = "pxrc",
+ .probe = pxrc_probe,
+ .disconnect = pxrc_disconnect,
+ .id_table = pxrc_table,
+ .suspend = pxrc_suspend,
+ .resume = pxrc_resume,
+ .pre_reset = pxrc_pre_reset,
+ .post_reset = pxrc_post_reset,
+ .reset_resume = pxrc_reset_resume,
+ .supports_autosuspend = 1,
+};
+
+module_usb_driver(pxrc_driver);
+
+MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
+MODULE_DESCRIPTION("PhoenixRC Flight Controller Adapter");
+MODULE_LICENSE("GPL v2");
--
2.15.1
^ permalink raw reply related
* Re: [PATCH v3] input: pxrc: new driver for PhoenixRC Flight Controller Adapter
From: Marcus Folkesson @ 2018-01-17 13:58 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jonathan Corbet, Tomohiro Yoshidomi, David Herrmann,
Philippe Ombredanne, Kate Stewart, Greg Kroah-Hartman,
linux-input, linux-doc, linux-kernel
In-Reply-To: <20180116231625.e3sgum667xfuumx4@dtor-ws>
Hello Dmitry,
On Tue, Jan 16, 2018 at 03:16:25PM -0800, Dmitry Torokhov wrote:
> Hi Marcus,
>
> On Sat, Jan 13, 2018 at 09:15:32PM +0100, Marcus Folkesson wrote:
> > This driver let you plug in your RC controller to the adapter and
> > use it as input device in various RC simulators.
> >
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > ---
> > v3:
> > - Use RUDDER and MISC instead of TILT_X and TILT_Y
> > - Drop kref and anchor
> > - Rework URB handling
> > - Add PM support
>
> How did you test the PM support? By default the autopm is disabled on
> USB devices; you need to enable it by writing to sysfs (I believe you
> need to 'echo "auto" > /sys/bus/usb/<device>/power/control) and see if
> it gets autosuspended when not in use and resumed after you start
> interacting with it.
The test I've done is simply reading from the input device and then call
`pm-suspend`.
It works, suspend is called and reset_resume() will submit the URB
again. Without the PM code, the application did not read any events upon
resume.
However, I found another tricky part.
If I enable autosuspend (as you suggest) it will suspend when noone is
using the device. Good.
But when someone is opening the device, input_dev->users is counted up
to 1 before resume() is called.
Is this intended?
This code (from resume()) will therefor allways submit the URB:
if (input_dev->users && usb_submit_urb(pxrc->urb, GFP_NOIO) < 0)
Then open() is called and fails because the urb is allready submitted.
input_dev->users is only incremented in input.c:input_open_device() what
I can tell?
I will move the submitting code to reset_resume() instead.
>
> > v2:
> > - Change module license to GPLv2 to match SPDX tag
> >
> > Documentation/input/devices/pxrc.rst | 57 +++++++
> > drivers/input/joystick/Kconfig | 9 +
> > drivers/input/joystick/Makefile | 1 +
> > drivers/input/joystick/pxrc.c | 320 +++++++++++++++++++++++++++++++++++
> > 4 files changed, 387 insertions(+)
> > create mode 100644 Documentation/input/devices/pxrc.rst
> > create mode 100644 drivers/input/joystick/pxrc.c
> >
> > diff --git a/Documentation/input/devices/pxrc.rst b/Documentation/input/devices/pxrc.rst
> > new file mode 100644
> > index 000000000000..ca11f646bae8
> > --- /dev/null
> > +++ b/Documentation/input/devices/pxrc.rst
> > @@ -0,0 +1,57 @@
> > +=======================================================
> > +pxrc - PhoenixRC Flight Controller Adapter
> > +=======================================================
> > +
> > +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> > +
> > +This driver let you use your own RC controller plugged into the
> > +adapter that comes with PhoenixRC [1]_ or other compatible adapters.
> > +
> > +The adapter supports 7 analog channels and 1 digital input switch.
> > +
> > +Notes
> > +=====
> > +
> > +Many RC controllers is able to configure which stick goes to which channel.
> > +This is also configurable in most simulators, so a matching is not necessary.
> > +
> > +The driver is generating the following input event for analog channels:
> > +
> > ++---------+----------------+
> > +| Channel | Event |
> > ++=========+================+
> > +| 1 | ABS_X |
> > ++---------+----------------+
> > +| 2 | ABS_Y |
> > ++---------+----------------+
> > +| 3 | ABS_RX |
> > ++---------+----------------+
> > +| 4 | ABS_RY |
> > ++---------+----------------+
> > +| 5 | ABS_RUDDER |
> > ++---------+----------------+
> > +| 6 | ABS_THROTTLE |
> > ++---------+----------------+
> > +| 7 | ABS_MISC |
> > ++---------+----------------+
> > +
> > +The digital input switch is generated as an `BTN_A` event.
> > +
> > +Manual Testing
> > +==============
> > +
> > +To test this driver's functionality you may use `input-event` which is part of
> > +the `input layer utilities` suite [2]_.
> > +
> > +For example::
> > +
> > + > modprobe pxrc
> > + > input-events <devnr>
> > +
> > +To print all input events from input `devnr`.
> > +
> > +References
> > +==========
> > +
> > +.. [1] http://www.phoenix-sim.com/
> > +.. [2] https://www.kraxel.org/cgit/input/
> > diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> > index f3c2f6ea8b44..18ab6dafff41 100644
> > --- a/drivers/input/joystick/Kconfig
> > +++ b/drivers/input/joystick/Kconfig
> > @@ -351,4 +351,13 @@ config JOYSTICK_PSXPAD_SPI_FF
> >
> > To drive rumble motor a dedicated power supply is required.
> >
> > +config JOYSTICK_PXRC
> > + tristate "PhoenixRC Flight Controller Adapter"
> > + depends on USB_ARCH_HAS_HCD
> > + select USB
> > + help
> > + Say Y here if you want to use the PhoenixRC Flight Controller Adapter.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called pxrc.
> > endif
> > diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> > index 67651efda2e1..dd0492ebbed7 100644
> > --- a/drivers/input/joystick/Makefile
> > +++ b/drivers/input/joystick/Makefile
> > @@ -23,6 +23,7 @@ obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
> > obj-$(CONFIG_JOYSTICK_MAGELLAN) += magellan.o
> > obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o
> > obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> > +obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> > obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> > obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> > obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o
> > diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
> > new file mode 100644
> > index 000000000000..98d9b8184c46
> > --- /dev/null
> > +++ b/drivers/input/joystick/pxrc.c
> > @@ -0,0 +1,320 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for Phoenix RC Flight Controller Adapter
> > + *
> > + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> > + *
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/errno.h>
> > +#include <linux/slab.h>
> > +#include <linux/module.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/usb.h>
> > +#include <linux/usb/input.h>
> > +#include <linux/mutex.h>
> > +#include <linux/input.h>
> > +
> > +#define PXRC_VENDOR_ID (0x1781)
> > +#define PXRC_PRODUCT_ID (0x0898)
> > +
> > +static const struct usb_device_id pxrc_table[] = {
> > + { USB_DEVICE(PXRC_VENDOR_ID, PXRC_PRODUCT_ID) },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(usb, pxrc_table);
> > +
> > +struct pxrc {
> > + struct input_dev *input;
> > + struct usb_device *udev;
> > + struct usb_interface *intf;
> > + struct urb *urb;
> > + __u8 epaddr;
> > + char phys[64];
> > + unsigned char *data;
> > + size_t bsize;
> > +};
> > +
> > +static void pxrc_usb_irq(struct urb *urb)
> > +{
> > + struct pxrc *pxrc = urb->context;
> > + int error;
> > +
> > + switch (urb->status) {
> > + case 0:
> > + /* success */
> > + break;
> > + case -ETIME:
> > + /* this urb is timing out */
> > + dev_dbg(&pxrc->intf->dev,
> > + "%s - urb timed out - was the device unplugged?\n",
> > + __func__);
> > + return;
> > + case -ECONNRESET:
> > + case -ENOENT:
> > + case -ESHUTDOWN:
> > + case -EPIPE:
> > + /* this urb is terminated, clean up */
> > + dev_dbg(&pxrc->intf->dev, "%s - urb shutting down with status: %d\n",
> > + __func__, urb->status);
> > + return;
> > + default:
> > + dev_dbg(&pxrc->intf->dev, "%s - nonzero urb status received: %d\n",
> > + __func__, urb->status);
> > + goto exit;
> > + }
> > +
> > + if (urb->actual_length == 8) {
> > + input_report_abs(pxrc->input, ABS_X, pxrc->data[0]);
> > + input_report_abs(pxrc->input, ABS_Y, pxrc->data[2]);
> > + input_report_abs(pxrc->input, ABS_RX, pxrc->data[3]);
> > + input_report_abs(pxrc->input, ABS_RY, pxrc->data[4]);
> > + input_report_abs(pxrc->input, ABS_RUDDER, pxrc->data[5]);
> > + input_report_abs(pxrc->input, ABS_THROTTLE, pxrc->data[6]);
> > + input_report_abs(pxrc->input, ABS_MISC, pxrc->data[7]);
> > +
> > + input_report_key(pxrc->input, BTN_A, pxrc->data[1]);
> > + }
> > +
> > +exit:
>
> I think you need
>
> usb_mark_last_busy(interface_to_usbdev(pxrc->intf));
>
> here.
>
Yes. Thank you.
> > + /* Resubmit to fetch new fresh URBs */
> > + error = usb_submit_urb(urb, GFP_ATOMIC);
> > + if (error && error != -EPERM)
> > + dev_err(&pxrc->intf->dev,
> > + "%s - usb_submit_urb failed with result: %d",
> > + __func__, error);
> > +}
> > +
> > +static int pxrc_open(struct input_dev *input)
> > +{
> > + struct pxrc *pxrc = input_get_drvdata(input);
> > + int retval;
> > +
> > + retval = usb_autopm_get_interface(pxrc->intf);
> > + if (retval) {
> > + dev_err(&pxrc->intf->dev,
> > + "%s - usb_autopm_get_interface failed, error: %d\n",
> > + __func__, retval);
> > + return retval;
> > + }
> > +
> > + retval = usb_submit_urb(pxrc->urb, GFP_KERNEL);
> > + if (retval) {
> > + dev_err(&pxrc->intf->dev,
> > + "%s - usb_submit_urb failed, error: %d\n",
> > + __func__, retval);
> > + retval = -EIO;
> > + goto out;
> > + }
> > +
> > + pxrc->intf->needs_remote_wakeup = 1;
> > +
> > +out:
> > + usb_autopm_put_interface(pxrc->intf);
> > + return retval;
> > +}
> > +
> > +static void pxrc_close(struct input_dev *input)
> > +{
> > + struct pxrc *pxrc = input_get_drvdata(input);
> > + int autopm_error;
> > +
> > + autopm_error = usb_autopm_get_interface(pxrc->intf);
> > +
> > + usb_kill_urb(pxrc->urb);
> > + pxrc->intf->needs_remote_wakeup = 0;
> > +
> > + if (!autopm_error)
> > + usb_autopm_put_interface(pxrc->intf);
> > +}
> > +
> > +static int pxrc_usb_init(struct pxrc *pxrc)
> > +{
> > + struct usb_endpoint_descriptor *epirq;
> > + unsigned int pipe;
> > + int retval;
> > +
> > + /* Set up the endpoint information */
> > + /* This device only has an interrupt endpoint */
> > + retval = usb_find_common_endpoints(pxrc->intf->cur_altsetting,
> > + NULL, NULL, &epirq, NULL);
> > + if (retval) {
> > + dev_err(&pxrc->intf->dev,
> > + "Could not find endpoint\n");
> > + goto error;
> > + }
> > +
> > + pxrc->bsize = usb_endpoint_maxp(epirq);
> > + pxrc->epaddr = epirq->bEndpointAddress;
> > + pxrc->data = devm_kmalloc(&pxrc->intf->dev, pxrc->bsize, GFP_KERNEL);
> > + if (!pxrc->data) {
> > + retval = -ENOMEM;
> > + goto error;
> > + }
> > +
> > + usb_set_intfdata(pxrc->intf, pxrc);
> > + usb_make_path(pxrc->udev, pxrc->phys, sizeof(pxrc->phys));
> > + strlcat(pxrc->phys, "/input0", sizeof(pxrc->phys));
> > +
> > + pxrc->urb = usb_alloc_urb(0, GFP_KERNEL);
> > + if (!pxrc->urb) {
> > + retval = -ENOMEM;
> > + goto error;
> > + }
> > +
> > + pipe = usb_rcvintpipe(pxrc->udev, pxrc->epaddr),
> > + usb_fill_int_urb(pxrc->urb, pxrc->udev, pipe, pxrc->data, pxrc->bsize,
> > + pxrc_usb_irq, pxrc, 1);
> > +
> > +error:
> > + return retval;
> > +
> > +
> > +}
> > +
> > +
> > +static int pxrc_input_init(struct pxrc *pxrc)
> > +{
> > + pxrc->input = devm_input_allocate_device(&pxrc->intf->dev);
> > + if (pxrc->input == NULL) {
> > + dev_err(&pxrc->intf->dev, "couldn't allocate input device\n");
> > + return -ENOMEM;
> > + }
> > +
> > + pxrc->input->name = "PXRC Flight Controller Adapter";
> > + pxrc->input->phys = pxrc->phys;
> > + usb_to_input_id(pxrc->udev, &pxrc->input->id);
> > +
> > + pxrc->input->open = pxrc_open;
> > + pxrc->input->close = pxrc_close;
> > +
> > + input_set_capability(pxrc->input, EV_KEY, BTN_A);
> > + input_set_abs_params(pxrc->input, ABS_X, 0, 255, 0, 0);
> > + input_set_abs_params(pxrc->input, ABS_Y, 0, 255, 0, 0);
> > + input_set_abs_params(pxrc->input, ABS_RX, 0, 255, 0, 0);
> > + input_set_abs_params(pxrc->input, ABS_RY, 0, 255, 0, 0);
> > + input_set_abs_params(pxrc->input, ABS_RUDDER, 0, 255, 0, 0);
> > + input_set_abs_params(pxrc->input, ABS_THROTTLE, 0, 255, 0, 0);
> > + input_set_abs_params(pxrc->input, ABS_MISC, 0, 255, 0, 0);
> > +
> > + input_set_drvdata(pxrc->input, pxrc);
> > +
> > + return input_register_device(pxrc->input);
> > +}
> > +
> > +static int pxrc_probe(struct usb_interface *intf,
> > + const struct usb_device_id *id)
> > +{
> > + struct pxrc *pxrc;
> > + int retval;
> > +
> > + pxrc = devm_kzalloc(&intf->dev, sizeof(*pxrc), GFP_KERNEL);
> > +
> > + if (!pxrc)
> > + return -ENOMEM;
> > +
> > + pxrc->udev = usb_get_dev(interface_to_usbdev(intf));
> > + pxrc->intf = intf;
> > +
> > + retval = pxrc_usb_init(pxrc);
> > + if (retval)
> > + goto error;
> > +
> > + retval = pxrc_input_init(pxrc);
> > + if (retval)
> > + goto err_free_urb;
> > +
> > + return 0;
> > +
> > +err_free_urb:
> > + usb_free_urb(pxrc->urb);
> > +
> > +error:
> > + return retval;
> > +}
> > +
> > +static void pxrc_disconnect(struct usb_interface *intf)
> > +{
> > + struct pxrc *pxrc = usb_get_intfdata(intf);
> > +
> > + usb_free_urb(pxrc->urb);
> > + usb_set_intfdata(intf, NULL);
> > +}
> > +
> > +static int pxrc_suspend(struct usb_interface *intf, pm_message_t message)
> > +{
> > + struct pxrc *pxrc = usb_get_intfdata(intf);
> > + struct input_dev *input_dev = pxrc->input;
> > +
> > + mutex_lock(&input_dev->mutex);
> > + usb_kill_urb(pxrc->urb);
> > + mutex_unlock(&input_dev->mutex);
> > +
> > + return 0;
> > +}
> > +
> > +static int pxrc_resume(struct usb_interface *intf)
> > +{
> > + struct pxrc *pxrc = usb_get_intfdata(intf);
> > + struct input_dev *input_dev = pxrc->input;
> > + int retval = 0;
> > +
> > + mutex_lock(&input_dev->mutex);
> > +
> > + if (input_dev->users && usb_submit_urb(pxrc->urb, GFP_NOIO) < 0)
> > + retval = -EIO;
> > +
> > + mutex_unlock(&input_dev->mutex);
> > +
> > + return retval;
> > +}
> > +
> > +static int pxrc_pre_reset(struct usb_interface *intf)
> > +{
> > + struct pxrc *pxrc = usb_get_intfdata(intf);
> > + struct input_dev *input_dev = pxrc->input;
> > +
> > + mutex_lock(&input_dev->mutex);
> > + usb_kill_urb(pxrc->urb);
> > +
> > + return 0;
> > +}
> > +
> > +static int pxrc_post_reset(struct usb_interface *intf)
> > +{
> > + struct pxrc *pxrc = usb_get_intfdata(intf);
> > + struct input_dev *input_dev = pxrc->input;
> > + int retval = 0;
> > +
> > + if (input_dev->users && usb_submit_urb(pxrc->urb, GFP_NOIO) < 0)
> > + retval = -EIO;
> > +
> > + mutex_unlock(&input_dev->mutex);
> > +
> > + return retval;
> > +}
> > +
> > +static int pxrc_reset_resume(struct usb_interface *intf)
> > +{
> > + return pxrc_resume(intf);
> > +}
> > +
> > +static struct usb_driver pxrc_driver = {
> > + .name = "pxrc",
> > + .probe = pxrc_probe,
> > + .disconnect = pxrc_disconnect,
> > + .id_table = pxrc_table,
> > + .suspend = pxrc_suspend,
> > + .resume = pxrc_resume,
> > + .pre_reset = pxrc_pre_reset,
> > + .post_reset = pxrc_post_reset,
> > + .reset_resume = pxrc_reset_resume,
> > + .supports_autosuspend = 1,
> > +};
> > +
> > +module_usb_driver(pxrc_driver);
> > +
> > +MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
> > +MODULE_DESCRIPTION("PhoenixRC Flight Controller Adapter");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.15.1
> >
>
> Thank you.
>
> --
> Dmitry
Best regards
Marcus Folkesson
^ permalink raw reply
* [PATCH] input: cyapa: remove redundant assignment to 'pwr_cmd'
From: Colin King @ 2018-01-17 10:45 UTC (permalink / raw)
To: Dmitry Torokhov, Guenter Roeck, linux-input; +Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The variable pwr_cmd is being assigned to cyapa->suspend_power_mode
twice, once during the declaration and once after taking an
interruptible mutex lock. Remove the redundant first assignment
since the value is never read and it is outside the mutex lock.
Cleans up clang warning:
drivers/input/mouse/cyapa.c:743:5: warning: Value stored to 'pwr_cmd'
during its initialization is never read
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/input/mouse/cyapa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
index fd8865c65caf..dfd3873513e4 100644
--- a/drivers/input/mouse/cyapa.c
+++ b/drivers/input/mouse/cyapa.c
@@ -740,7 +740,7 @@ static ssize_t cyapa_show_suspend_scanrate(struct device *dev,
char *buf)
{
struct cyapa *cyapa = dev_get_drvdata(dev);
- u8 pwr_cmd = cyapa->suspend_power_mode;
+ u8 pwr_cmd;
u16 sleep_time;
int len;
int error;
--
2.15.1
^ permalink raw reply related
* [PATCH] Input: synaptics-rmi4 - prevent UAF reported by KASAN
From: Nick Desaulniers @ 2018-01-17 6:25 UTC (permalink / raw)
Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, kasan-dev,
usacrc, Nick Desaulniers, stable, Dmitry Torokhov,
Benjamin Tissoires, Nick Dyer, Andrew Duggan, Christophe JAILLET,
Guenter Roeck, linux-input, linux-kernel
KASAN found a UAF due to dangling pointer. As the report below says,
rmi_f11_attention() accesses drvdata->attn_data.data, which was freed in
rmi_irq_fn.
[ 311.424062] BUG: KASAN: use-after-free in
rmi_f11_attention+0x526/0x5e0 [rmi_core]
[ 311.424067] Read of size 27 at addr ffff88041fd610db by task
irq/131-i2c_hid/1162
[ 311.424075] CPU: 0 PID: 1162 Comm: irq/131-i2c_hid Not tainted
4.15.0-rc8+ #2
[ 311.424076] Hardware name: Razer Blade Stealth/Razer, BIOS 6.05
01/26/2017
[ 311.424078] Call Trace:
[ 311.424086] dump_stack+0xae/0x12d
[ 311.424090] ? _atomic_dec_and_lock+0x103/0x103
[ 311.424094] ? show_regs_print_info+0xa/0xa
[ 311.424099] ? input_handle_event+0x10b/0x810
[ 311.424104] print_address_description+0x65/0x229
[ 311.424108] kasan_report.cold.5+0xa7/0x281
[ 311.424117] rmi_f11_attention+0x526/0x5e0 [rmi_core]
[ 311.424123] ? memcpy+0x1f/0x50
[ 311.424132] ? rmi_f11_attention+0x526/0x5e0 [rmi_core]
[ 311.424143] ? rmi_f11_probe+0x1e20/0x1e20 [rmi_core]
[ 311.424153] ? rmi_process_interrupt_requests+0x220/0x2a0 [rmi_core]
[ 311.424163] ? rmi_irq_fn+0x22c/0x270 [rmi_core]
[ 311.424173] ? rmi_process_interrupt_requests+0x2a0/0x2a0 [rmi_core]
[ 311.424177] ? free_irq+0xa0/0xa0
[ 311.424180] ? irq_finalize_oneshot.part.39+0xeb/0x180
[ 311.424190] ? rmi_process_interrupt_requests+0x2a0/0x2a0 [rmi_core]
[ 311.424193] ? irq_thread_fn+0x3d/0x80
[ 311.424197] ? irq_finalize_oneshot.part.39+0x180/0x180
[ 311.424200] ? irq_thread+0x21d/0x290
[ 311.424203] ? irq_thread_check_affinity+0x170/0x170
[ 311.424207] ? remove_wait_queue+0x150/0x150
[ 311.424212] ? kasan_unpoison_shadow+0x30/0x40
[ 311.424214] ? __init_waitqueue_head+0xa0/0xd0
[ 311.424218] ? task_non_contending.cold.55+0x18/0x18
[ 311.424221] ? irq_forced_thread_fn+0xa0/0xa0
[ 311.424226] ? irq_thread_check_affinity+0x170/0x170
[ 311.424230] ? kthread+0x19e/0x1c0
[ 311.424233] ? kthread_create_worker_on_cpu+0xc0/0xc0
[ 311.424237] ? ret_from_fork+0x32/0x40
[ 311.424244] Allocated by task 899:
[ 311.424249] kasan_kmalloc+0xbf/0xe0
[ 311.424252] __kmalloc_track_caller+0xd9/0x1f0
[ 311.424255] kmemdup+0x17/0x40
[ 311.424264] rmi_set_attn_data+0xa4/0x1b0 [rmi_core]
[ 311.424269] rmi_raw_event+0x10b/0x1f0 [hid_rmi]
[ 311.424278] hid_input_report+0x1a8/0x2c0 [hid]
[ 311.424283] i2c_hid_irq+0x146/0x1d0 [i2c_hid]
[ 311.424286] irq_thread_fn+0x3d/0x80
[ 311.424288] irq_thread+0x21d/0x290
[ 311.424291] kthread+0x19e/0x1c0
[ 311.424293] ret_from_fork+0x32/0x40
[ 311.424296] Freed by task 1162:
[ 311.424300] kasan_slab_free+0x71/0xc0
[ 311.424303] kfree+0x90/0x190
[ 311.424311] rmi_irq_fn+0x1b2/0x270 [rmi_core]
[ 311.424319] rmi_irq_fn+0x257/0x270 [rmi_core]
[ 311.424322] irq_thread_fn+0x3d/0x80
[ 311.424324] irq_thread+0x21d/0x290
[ 311.424327] kthread+0x19e/0x1c0
[ 311.424330] ret_from_fork+0x32/0x40
[ 311.424334] The buggy address belongs to the object at
ffff88041fd610c0
which belongs to the cache kmalloc-64 of size 64
[ 311.424340] The buggy address is located 27 bytes inside of
64-byte region [ffff88041fd610c0, ffff88041fd61100)
[ 311.424344] The buggy address belongs to the page:
[ 311.424348] page:ffffea00107f5840 count:1 mapcount:0 mapping:
(null) index:0x0
[ 311.424353] flags: 0x17ffffc0000100(slab)
[ 311.424358] raw: 0017ffffc0000100 0000000000000000 0000000000000000
00000001802a002a
[ 311.424363] raw: dead000000000100 dead000000000200 ffff8804228036c0
0000000000000000
[ 311.424366] page dumped because: kasan: bad access detected
[ 311.424369] Memory state around the buggy address:
[ 311.424373] ffff88041fd60f80: 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[ 311.424377] ffff88041fd61000: fb fb fb fb fb fb fb fb fc fc fc fc
fb fb fb fb
[ 311.424381] >ffff88041fd61080: fb fb fb fb fc fc fc fc fb fb fb fb
fb fb fb fb
[ 311.424384] ^
[ 311.424387] ffff88041fd61100: fc fc fc fc fb fb fb fb fb fb fb fb
fc fc fc fc
[ 311.424391] ffff88041fd61180: fb fb fb fb fb fb fb fb fc fc fc fc
fb fb fb fb
Cc: stable@vger.kernel.org
Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
drivers/input/rmi4/rmi_driver.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 4f2bb59..141ea22 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -230,8 +230,10 @@ static irqreturn_t rmi_irq_fn(int irq, void *dev_id)
rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev,
"Failed to process interrupt request: %d\n", ret);
- if (count)
+ if (count) {
kfree(attn_data.data);
+ attn_data.data = NULL;
+ }
if (!kfifo_is_empty(&drvdata->attn_fifo))
return rmi_irq_fn(irq, dev_id);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 0/6] Input: remove unneeded MODULE_VERSION usage
From: Dmitry Torokhov @ 2018-01-17 0:45 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-input, linux-kernel
In-Reply-To: <20171122174921.20192-1-gregkh@linuxfoundation.org>
On Wed, Nov 22, 2017 at 06:49:15PM +0100, Greg Kroah-Hartman wrote:
> Here's a short patch series against linux-next at the moment, that
> removes all of the MODULE_VERSION() usage in drivers/input/ as it's not
> needed at all.
>
> Greg Kroah-Hartman (6):
> Input: rmi4: remove unneeded MODULE_VERSION() usage
> Input: keyboard: pmic8xxx: remove unneeded MODULE_VERSION() usage
> Input: mouse: elan_i2c: remove unneeded MODULE_VERSION() usage
> Input: touchscreen: remove unneeded MODULE_VERSION() usage
> Input: misc: remove unneeded MODULE_VERSION() usage
> Input: remove remaining unneeded MODULE_VERSION() usage
Applied the lot, thank you Greg.
>
> drivers/input/input-polldev.c | 1 -
> drivers/input/keyboard/pmic8xxx-keypad.c | 1 -
> drivers/input/misc/apanel.c | 2 --
> drivers/input/misc/ati_remote2.c | 2 --
> drivers/input/misc/uinput.c | 1 -
> drivers/input/mouse/elan_i2c_core.c | 2 --
> drivers/input/rmi4/rmi_bus.c | 1 -
> drivers/input/rmi4/rmi_driver.h | 2 --
> drivers/input/rmi4/rmi_i2c.c | 1 -
> drivers/input/rmi4/rmi_spi.c | 1 -
> drivers/input/sparse-keymap.c | 1 -
> drivers/input/touchscreen/colibri-vf50-ts.c | 2 --
> drivers/input/touchscreen/elants_i2c.c | 2 --
> drivers/input/touchscreen/melfas_mip4.c | 1 -
> drivers/input/touchscreen/wdt87xx_i2c.c | 2 --
> 15 files changed, 22 deletions(-)
>
> --
> 2.15.0
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: remove unneeded DRIVER_LICENSE #defines
From: Dmitry Torokhov @ 2018-01-17 0:41 UTC (permalink / raw)
To: Philippe Ombredanne; +Cc: Greg Kroah-Hartman, Arvind Yadav, LKML, linux-input
In-Reply-To: <CAOFm3uG9QVCTEvV-_pjBGgKoHOWi6ps2tp=NABuHinOQi+QeTw@mail.gmail.com>
On Fri, Nov 17, 2017 at 03:51:31PM +0100, Philippe Ombredanne wrote:
> On Fri, Nov 17, 2017 at 3:17 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > There is no need to #define the license of the driver, just put it in
> > the MODULE_LICENSE() line directly as a text string.
> >
> > This allows tools that check that the module license matches the source
> > code license to work properly, as there is no need to unwind the
> > unneeded dereference. For some of these drivers, the #define is just a
> > few lines above the MODULE_LICENSE() line, which is extra pointless.
> >
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Arvind Yadav <arvind.yadav.cs@gmail.com>
> > Reported-by: Philippe Ombredanne <pombredanne@nexb.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
>
> Reviewed-by: Philippe Ombredanne <pombredanne@nexb.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Revert "Input: trackpoint - add new trackpoint firmware ID"
From: Dmitry Torokhov @ 2018-01-16 23:49 UTC (permalink / raw)
To: ulrik.debie-os; +Cc: Aaron Ma, Greg KH, Sebastian Schmidt, linux-input
In-Reply-To: <20180114203958.GA3686@beacon.debie>
Hi Ulrik,
On Sun, Jan 14, 2018 at 09:39:59PM +0100, ulrik.debie-os@e2big.org wrote:
> Hi Dmitry, Sebastian,
> Nack for the patch..
NAK because of the issue pointed by Sebastian, or bigger disagreement?
> As Sebastian pointed out, the trackpoint detection will always report an error. (see below)
>
> I builded your original patch and it resulted in detection as generic mouse for the trackpoint:
>
> [ 0.838143] serio: i8042 KBD port at 0x60,0x64 irq 1
> [ 0.838146] serio: i8042 AUX port at 0x60,0x64 irq 12
> [ 0.838267] mousedev: PS/2 mouse device common for all mice
> [ 9.321140] psmouse serio1: synaptics: queried max coordinates: x [..5676], y [..4758]
> [ 9.367095] psmouse serio1: synaptics: queried min coordinates: x [1266..], y [1096..]
> [ 9.367103] psmouse serio1: synaptics: The touchpad can support a better bus than the too old PS/2 protocol. Make sure MOUSE_PS2_SYNAPTICS_SMBUS and RMI4_SMB are enabled to get a better touchpad experience.
> [ 9.455953] psmouse serio1: synaptics: Touchpad model: 1, fw: 8.2, id: 0x1e2b1, caps: 0xf007a3/0x943300/0x12e800/0x410000, board id: 3157, fw id: 2405942
> [ 9.455970] psmouse serio1: synaptics: serio: Synaptics pass-through port at isa0060/serio1/input0
> [ 9.514117] input: SynPS/2 Synaptics TouchPad as /devices/platform/i8042/serio1/input/input7
> [ 11.458443] input: PS/2 Generic Mouse as /devices/platform/i8042/serio1/serio2/input/input18
>
> When I fixed your patch, it resulted in: (timing difference is because a dvd was inserted before)
> [ 0.843661] serio: i8042 KBD port at 0x60,0x64 irq 1
> [ 0.843664] serio: i8042 AUX port at 0x60,0x64 irq 12
> [ 0.843793] mousedev: PS/2 mouse device common for all mice
> [ 3.822411] psmouse serio1: synaptics: queried max coordinates: x [..5676], y [..4758]
> [ 3.868196] psmouse serio1: synaptics: queried min coordinates: x [1266..], y [1096..]
> [ 3.868203] psmouse serio1: synaptics: The touchpad can support a better bus than the too old PS/2 protocol. Make sure MOUSE_PS2_SYNAPTICS_SMBUS and RMI4_SMB are enabled to get a better touchpad experience.
> [ 3.961024] psmouse serio1: synaptics: Touchpad model: 1, fw: 8.2, id: 0x1e2b1, caps: 0xf007a3/0x943300/0x12e800/0x410000, board id: 3157, fw id: 2405942
> [ 3.961038] psmouse serio1: synaptics: serio: Synaptics pass-through port at isa0060/serio1/input0
> [ 4.017290] input: SynPS/2 Synaptics TouchPad as /devices/platform/i8042/serio1/input/input7
> [ 4.407930] psmouse serio2: trackpoint: failed to get extended button data, assuming 3 buttons
> [ 8.460022] psmouse serio2: trackpoint: IBM TrackPoint firmware: 0x0e, buttons: 3/3
> [ 8.740439] input: TPPS/2 IBM TrackPoint as /devices/platform/i8042/serio1/serio2/input/input18
>
> functionally both versions (with/without bug) resulted in a functional trackpoint with 3 buttons.
>
> Sebastian, with regards to your middle button question, as you can see in the output I've such hardware. (lenovo e570).
>
> Dimitrios, wasn't it your intention that this hardware would be detected as NON IBM ? Apparently it
> is IBM and still fails "Read Extended Button Status".
Only devices reporting 0x02, 0x03 or 0x04 as the first byte of the
xetended ID command should be identified as non-IBM. If device responds
with 0x01 then we have to assume it is proper IBM. Your appears to be
reporting 0x01 0x0e, like the real trackpoint would. Unfortunately it
seems Lenovo managed to mess up the firmware in those.
Thanks.
>
> Kind regards,
> Ulrik
>
> On Sat, Jan 06, 2018 at 10:52:01PM -0800, Dmitry Torokhov wrote:
> > Date: Sat, 6 Jan 2018 22:52:01 -0800
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > To: Aaron Ma <aaron.ma@canonical.com>
> > Cc: Greg KH <gregkh@linuxfoundation.org>, Sebastian Schmidt <yath@yath.de>,
> > linux-input@vger.kernel.org
> > Subject: Re: [PATCH] Revert "Input: trackpoint - add new trackpoint
> > firmware ID"
> > X-Mailing-List: linux-input@vger.kernel.org
> >
> > On Fri, Jan 05, 2018 at 08:23:13AM -0800, Dmitry Torokhov wrote:
> > > Hi Aaron,
> > >
> > > On Fri, Jan 05, 2018 at 09:29:26PM +0800, Aaron Ma wrote:
> > > > Hi Dmitry:
> > > >
> > > > Got the official info from Lenovo:
> > > > Lenovo introduced new TrackPoint compatible sticks ( ELAN/Alps/NXP
> > > > sticks) from 2016.
> > > > These new devices only support the minimum commands described in the
> > > > spec, which has been used in the current Windows driver.
> > >
> > > What is the exact list of the commands supported by each variant?
> > >
> > > >
> > > > Legacy TrackPoint: 0101 – 0E01
> > > > ALPS: 0102 – FF02
> > > > ELAN:0103 – FF03
> > > > NXP: 0104 – FF04
> > > >
> > > > 2.4.18 READ SECONDARY ID (x"E1")
> > > > This command will read the secondary device ID of the pointing device (2
> > > > bytes). The least significant byte is sent first. For the first byte,
> > > > the legacy TrackPoint controller from IBM will always return x"01", the
> > > > pointing stick from ALPS will always return x"02", the pointing stick
> > > > from Elan will always return x"03”, and the pointing stick from NXP will
> > > > always return 0x”04". And a second byte which denotes a specific set of
> > > > functional specifications. Differing ROM versions are used to denote
> > > > changes within a given functional set.
> > >
> > > Can you/Lenovo share the updated spec?
> > >
> > > >
> > > > The new devices (include Legacy ID:01) will not support the sysfs like
> > > > speed.
> > > >
> > > > So it is not right to revert the commit, it is about to add another 0x04
> > > > ID in it.
> > > >
> > > > Old sysfs could be stayed for old legacy device ID:01 or removed.
> > >
> > > No, because there are devices that have trackpoints properly
> > > implementing the protocol, before Lenovo started their "innovation".
> > >
> > > Do we have any way to distinguish between properly implemented
> > > trackpoints and Lenovo "improved" ones? I played with gen3 Carbon, and
> > > while it does not error out on "speed" attribute, unlike gen5, it still
> > > has no visible effects. Additionally, the "press to select"
> > > functionality seems to be disabled, and trying to enable it via sysfs
> > > results in register content being reverted to the original "disabled"
> > > setting in a second or two. Setting to swap X and Y axes does not work
> > > either, not sure about other bits of that control register.
> > > "sensitivity" does work though, again unlike my gen5.
> > >
> > > Thanks.
> > >
> > > --
> > > Dmitry
> >
> > Guys, could you please try the patch below? It will not solve the
> > "speed" issue on 0x01 devices, but hopefully we won't be exposing
> > non-existing controls on others.
> >
> > Thanks!
> >
> > --
> > Dmitry
> >
> >
> > Input: trackpoint - only expose supported controls for Elan, ALPS and NXP
> >
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >
> > The newer trackpoints from ALPS, Elan and NXP implement a very limited
> > subset of extended commands and controls that the original trackpoints
> > implemented, so we should not be exposing not working controls in sysfs.
> > The newer trackpoints also do not implement "Power On Reset" or "Read
> > Extended Button Status", so we should not be using these commands during
> > initialization.
> >
> > While we are at it, let's change "unsigned char" to u8 for byte data or
> > bool for booleans and use better suited error codes instead of -1.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > drivers/input/mouse/trackpoint.c | 241 +++++++++++++++++++++++---------------
> > drivers/input/mouse/trackpoint.h | 34 +++--
> > 2 files changed, 168 insertions(+), 107 deletions(-)
> >
> > diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
> > index 0871010f18d5..0ca80f465359 100644
> > --- a/drivers/input/mouse/trackpoint.c
> > +++ b/drivers/input/mouse/trackpoint.c
> > @@ -19,6 +19,13 @@
> > #include "psmouse.h"
> > #include "trackpoint.h"
> >
> > +static const char * const trackpoint_variants[] = {
> > + [TP_VARIANT_IBM] = "IBM",
> > + [TP_VARIANT_ALPS] = "ALPS",
> > + [TP_VARIANT_ELAN] = "Elan",
> > + [TP_VARIANT_NXP] = "NXP",
> > +};
> > +
> > /*
> > * Power-on Reset: Resets all trackpoint parameters, including RAM values,
> > * to defaults.
> > @@ -26,7 +33,7 @@
> > */
> > static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
> > {
> > - unsigned char results[2];
> > + u8 results[2];
> > int tries = 0;
> >
> > /* Issue POR command, and repeat up to once if 0xFC00 received */
> > @@ -38,7 +45,7 @@ static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
> >
> > /* Check for success response -- 0xAA00 */
> > if (results[0] != 0xAA || results[1] != 0x00)
> > - return -1;
> > + return -ENODEV;
> >
> > return 0;
> > }
> > @@ -46,8 +53,7 @@ static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
> > /*
> > * Device IO: read, write and toggle bit
> > */
> > -static int trackpoint_read(struct ps2dev *ps2dev,
> > - unsigned char loc, unsigned char *results)
> > +static int trackpoint_read(struct ps2dev *ps2dev, u8 loc, u8 *results)
> > {
> > if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
> > ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 1, loc))) {
> > @@ -57,8 +63,7 @@ static int trackpoint_read(struct ps2dev *ps2dev,
> > return 0;
> > }
> >
> > -static int trackpoint_write(struct ps2dev *ps2dev,
> > - unsigned char loc, unsigned char val)
> > +static int trackpoint_write(struct ps2dev *ps2dev, u8 loc, u8 val)
> > {
> > if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
> > ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_WRITE_MEM)) ||
> > @@ -70,8 +75,7 @@ static int trackpoint_write(struct ps2dev *ps2dev,
> > return 0;
> > }
> >
> > -static int trackpoint_toggle_bit(struct ps2dev *ps2dev,
> > - unsigned char loc, unsigned char mask)
> > +static int trackpoint_toggle_bit(struct ps2dev *ps2dev, u8 loc, u8 mask)
> > {
> > /* Bad things will happen if the loc param isn't in this range */
> > if (loc < 0x20 || loc >= 0x2F)
> > @@ -87,11 +91,11 @@ static int trackpoint_toggle_bit(struct ps2dev *ps2dev,
> > return 0;
> > }
> >
> > -static int trackpoint_update_bit(struct ps2dev *ps2dev, unsigned char loc,
> > - unsigned char mask, unsigned char value)
> > +static int trackpoint_update_bit(struct ps2dev *ps2dev,
> > + u8 loc, u8 mask, u8 value)
> > {
> > int retval = 0;
> > - unsigned char data;
> > + u8 data;
> >
> > trackpoint_read(ps2dev, loc, &data);
> > if (((data & mask) == mask) != !!value)
> > @@ -105,17 +109,18 @@ static int trackpoint_update_bit(struct ps2dev *ps2dev, unsigned char loc,
> > */
> > struct trackpoint_attr_data {
> > size_t field_offset;
> > - unsigned char command;
> > - unsigned char mask;
> > - unsigned char inverted;
> > - unsigned char power_on_default;
> > + u8 command;
> > + u8 mask;
> > + bool inverted;
> > + u8 power_on_default;
> > };
> >
> > -static ssize_t trackpoint_show_int_attr(struct psmouse *psmouse, void *data, char *buf)
> > +static ssize_t trackpoint_show_int_attr(struct psmouse *psmouse,
> > + void *data, char *buf)
> > {
> > struct trackpoint_data *tp = psmouse->private;
> > struct trackpoint_attr_data *attr = data;
> > - unsigned char value = *(unsigned char *)((char *)tp + attr->field_offset);
> > + u8 value = *(u8 *)((void *)tp + attr->field_offset);
> >
> > if (attr->inverted)
> > value = !value;
> > @@ -128,8 +133,8 @@ static ssize_t trackpoint_set_int_attr(struct psmouse *psmouse, void *data,
> > {
> > struct trackpoint_data *tp = psmouse->private;
> > struct trackpoint_attr_data *attr = data;
> > - unsigned char *field = (unsigned char *)((char *)tp + attr->field_offset);
> > - unsigned char value;
> > + u8 *field = (void *)tp + attr->field_offset;
> > + u8 value;
> > int err;
> >
> > err = kstrtou8(buf, 10, &value);
> > @@ -157,17 +162,14 @@ static ssize_t trackpoint_set_bit_attr(struct psmouse *psmouse, void *data,
> > {
> > struct trackpoint_data *tp = psmouse->private;
> > struct trackpoint_attr_data *attr = data;
> > - unsigned char *field = (unsigned char *)((char *)tp + attr->field_offset);
> > - unsigned int value;
> > + bool *field = (void *)tp + attr->field_offset;
> > + bool value;
> > int err;
> >
> > - err = kstrtouint(buf, 10, &value);
> > + err = kstrtobool(buf, &value);
> > if (err)
> > return err;
> >
> > - if (value > 1)
> > - return -EINVAL;
> > -
> > if (attr->inverted)
> > value = !value;
> >
> > @@ -193,30 +195,6 @@ PSMOUSE_DEFINE_ATTR(_name, S_IWUSR | S_IRUGO, \
> > &trackpoint_attr_##_name, \
> > trackpoint_show_int_attr, trackpoint_set_bit_attr)
> >
> > -#define TRACKPOINT_UPDATE_BIT(_psmouse, _tp, _name) \
> > -do { \
> > - struct trackpoint_attr_data *_attr = &trackpoint_attr_##_name; \
> > - \
> > - trackpoint_update_bit(&_psmouse->ps2dev, \
> > - _attr->command, _attr->mask, _tp->_name); \
> > -} while (0)
> > -
> > -#define TRACKPOINT_UPDATE(_power_on, _psmouse, _tp, _name) \
> > -do { \
> > - if (!_power_on || \
> > - _tp->_name != trackpoint_attr_##_name.power_on_default) { \
> > - if (!trackpoint_attr_##_name.mask) \
> > - trackpoint_write(&_psmouse->ps2dev, \
> > - trackpoint_attr_##_name.command, \
> > - _tp->_name); \
> > - else \
> > - TRACKPOINT_UPDATE_BIT(_psmouse, _tp, _name); \
> > - } \
> > -} while (0)
> > -
> > -#define TRACKPOINT_SET_POWER_ON_DEFAULT(_tp, _name) \
> > - (_tp->_name = trackpoint_attr_##_name.power_on_default)
> > -
> > TRACKPOINT_INT_ATTR(sensitivity, TP_SENS, TP_DEF_SENS);
> > TRACKPOINT_INT_ATTR(speed, TP_SPEED, TP_DEF_SPEED);
> > TRACKPOINT_INT_ATTR(inertia, TP_INERTIA, TP_DEF_INERTIA);
> > @@ -229,13 +207,33 @@ TRACKPOINT_INT_ATTR(ztime, TP_Z_TIME, TP_DEF_Z_TIME);
> > TRACKPOINT_INT_ATTR(jenks, TP_JENKS_CURV, TP_DEF_JENKS_CURV);
> > TRACKPOINT_INT_ATTR(drift_time, TP_DRIFT_TIME, TP_DEF_DRIFT_TIME);
> >
> > -TRACKPOINT_BIT_ATTR(press_to_select, TP_TOGGLE_PTSON, TP_MASK_PTSON, 0,
> > +TRACKPOINT_BIT_ATTR(press_to_select, TP_TOGGLE_PTSON, TP_MASK_PTSON, false,
> > TP_DEF_PTSON);
> > -TRACKPOINT_BIT_ATTR(skipback, TP_TOGGLE_SKIPBACK, TP_MASK_SKIPBACK, 0,
> > +TRACKPOINT_BIT_ATTR(skipback, TP_TOGGLE_SKIPBACK, TP_MASK_SKIPBACK, false,
> > TP_DEF_SKIPBACK);
> > -TRACKPOINT_BIT_ATTR(ext_dev, TP_TOGGLE_EXT_DEV, TP_MASK_EXT_DEV, 1,
> > +TRACKPOINT_BIT_ATTR(ext_dev, TP_TOGGLE_EXT_DEV, TP_MASK_EXT_DEV, true,
> > TP_DEF_EXT_DEV);
> >
> > +static bool trackpoint_is_attr_available(struct psmouse *psmouse,
> > + struct attribute *attr)
> > +{
> > + struct trackpoint_data *tp = psmouse->private;
> > +
> > + return tp->variant_id == TP_VARIANT_IBM ||
> > + attr == &psmouse_attr_sensitivity.dattr.attr ||
> > + attr == &psmouse_attr_press_to_select.dattr.attr;
> > +}
> > +
> > +static umode_t trackpoint_is_attr_visible(struct kobject *kobj,
> > + struct attribute *attr, int n)
> > +{
> > + struct device *dev = container_of(kobj, struct device, kobj);
> > + struct serio *serio = to_serio_port(dev);
> > + struct psmouse *psmouse = serio_get_drvdata(serio);
> > +
> > + return trackpoint_is_attr_available(psmouse, attr) ? attr->mode : 0;
> > +}
> > +
> > static struct attribute *trackpoint_attrs[] = {
> > &psmouse_attr_sensitivity.dattr.attr,
> > &psmouse_attr_speed.dattr.attr,
> > @@ -255,24 +253,56 @@ static struct attribute *trackpoint_attrs[] = {
> > };
> >
> > static struct attribute_group trackpoint_attr_group = {
> > - .attrs = trackpoint_attrs,
> > + .is_visible = trackpoint_is_attr_visible,
> > + .attrs = trackpoint_attrs,
> > };
> >
> > -static int trackpoint_start_protocol(struct psmouse *psmouse, unsigned char *firmware_id)
> > -{
> > - unsigned char param[2] = { 0 };
> > +#define TRACKPOINT_UPDATE(_power_on, _psmouse, _tp, _name) \
> > +do { \
> > + struct trackpoint_attr_data *_attr = &trackpoint_attr_##_name; \
> > + \
> > + if ((!_power_on || _tp->_name != _attr->power_on_default) && \
> > + trackpoint_is_attr_available(_psmouse, \
> > + &psmouse_attr_##_name.dattr.attr)) { \
> > + if (!_attr->mask) \
> > + trackpoint_write(&_psmouse->ps2dev, \
> > + _attr->command, _tp->_name); \
> > + else \
> > + trackpoint_update_bit(&_psmouse->ps2dev, \
> > + _attr->command, _attr->mask, \
> > + _tp->_name); \
> > + } \
> > +} while (0)
> >
> > - if (ps2_command(&psmouse->ps2dev, param, MAKE_PS2_CMD(0, 2, TP_READ_ID)))
> > - return -1;
> > +#define TRACKPOINT_SET_POWER_ON_DEFAULT(_tp, _name) \
> > +do { \
> > + _tp->_name = trackpoint_attr_##_name.power_on_default; \
> > +} while (0)
> >
> > - /* add new TP ID. */
> > - if (!(param[0] & TP_MAGIC_IDENT))
> > - return -1;
> > +static int trackpoint_start_protocol(struct psmouse *psmouse,
> > + u8 *variant_id, u8 *firmware_id)
> > +{
> > + u8 param[2] = { 0 };
> > + int error;
> >
> > - if (firmware_id)
> > - *firmware_id = param[1];
> > + error = ps2_command(&psmouse->ps2dev,
> > + param, MAKE_PS2_CMD(0, 2, TP_READ_ID));
> > + if (error)
> > + return error;
> > +
> > + switch (param[0]) {
> > + case TP_VARIANT_IBM:
> > + case TP_VARIANT_ALPS:
> > + case TP_VARIANT_ELAN:
> > + case TP_VARIANT_NXP:
> > + if (variant_id)
> > + *variant_id = param[0];
> > + if (firmware_id)
> > + *firmware_id = param[1];
> > + break;
> > + }
> >
> > - return 0;
> > + return -ENODEV;
>
> always report error .. not good.
>
> > }
> >
> > /*
> > @@ -285,7 +315,7 @@ static int trackpoint_sync(struct psmouse *psmouse, bool in_power_on_state)
> > {
> > struct trackpoint_data *tp = psmouse->private;
> >
> > - if (!in_power_on_state) {
> > + if (!in_power_on_state && tp->variant_id == TP_VARIANT_IBM) {
> > /*
> > * Disable features that may make device unusable
> > * with this driver.
> > @@ -347,7 +377,8 @@ static void trackpoint_defaults(struct trackpoint_data *tp)
> >
> > static void trackpoint_disconnect(struct psmouse *psmouse)
> > {
> > - sysfs_remove_group(&psmouse->ps2dev.serio->dev.kobj, &trackpoint_attr_group);
> > + device_remove_group(&psmouse->ps2dev.serio->dev,
> > + &trackpoint_attr_group);
> >
> > kfree(psmouse->private);
> > psmouse->private = NULL;
> > @@ -355,14 +386,20 @@ static void trackpoint_disconnect(struct psmouse *psmouse)
> >
> > static int trackpoint_reconnect(struct psmouse *psmouse)
> > {
> > - int reset_fail;
> > + struct trackpoint_data *tp = psmouse->private;
> > + int error;
> > + bool was_reset;
> >
> > - if (trackpoint_start_protocol(psmouse, NULL))
> > - return -1;
> > + error = trackpoint_start_protocol(psmouse, NULL, NULL);
> > + if (error)
> > + return error;
> >
> > - reset_fail = trackpoint_power_on_reset(&psmouse->ps2dev);
> > - if (trackpoint_sync(psmouse, !reset_fail))
> > - return -1;
> > + was_reset = tp->variant_id == TP_VARIANT_IBM &&
> > + trackpoint_power_on_reset(&psmouse->ps2dev) == 0;
> > +
> > + error = trackpoint_sync(psmouse, was_reset);
> > + if (error)
> > + return error;
> >
> > return 0;
> > }
> > @@ -370,46 +407,62 @@ static int trackpoint_reconnect(struct psmouse *psmouse)
> > int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
> > {
> > struct ps2dev *ps2dev = &psmouse->ps2dev;
> > - unsigned char firmware_id;
> > - unsigned char button_info;
> > + struct trackpoint_data *tp;
> > + u8 variant_id;
> > + u8 firmware_id;
> > + u8 button_info;
> > int error;
> >
> > - if (trackpoint_start_protocol(psmouse, &firmware_id))
> > - return -1;
> > + error = trackpoint_start_protocol(psmouse, &variant_id, &firmware_id);
> > + if (error)
> > + return error;
> >
> > if (!set_properties)
> > return 0;
> >
> > - if (trackpoint_read(ps2dev, TP_EXT_BTN, &button_info)) {
> > - psmouse_warn(psmouse, "failed to get extended button data, assuming 3 buttons\n");
> > - button_info = 0x33;
> > - }
> > -
> > - psmouse->private = kzalloc(sizeof(struct trackpoint_data), GFP_KERNEL);
> > - if (!psmouse->private)
> > + tp = kzalloc(sizeof(*tp), GFP_KERNEL);
> > + if (!tp)
> > return -ENOMEM;
> >
> > - psmouse->vendor = "IBM";
> > + trackpoint_defaults(tp);
> > + tp->variant_id = variant_id;
> > + tp->firmware_id = firmware_id;
> > +
> > + psmouse->private = tp;
> > +
> > + psmouse->vendor = trackpoint_variants[variant_id];
> > psmouse->name = "TrackPoint";
> >
> > psmouse->reconnect = trackpoint_reconnect;
> > psmouse->disconnect = trackpoint_disconnect;
> >
> > + if (variant_id != TP_VARIANT_IBM) {
> > + /* Newer variants do not support extended button query. */
> > + button_info = 0x33;
> > + } else {
> > + error = trackpoint_read(ps2dev, TP_EXT_BTN, &button_info);
> > + if (error) {
> > + psmouse_warn(psmouse,
> > + "failed to get extended button data, assuming 3 buttons\n");
> > + button_info = 0x33;
> > + }
> > + }
> > +
> > if ((button_info & 0x0f) >= 3)
> > - __set_bit(BTN_MIDDLE, psmouse->dev->keybit);
> > + input_set_capability(psmouse->dev, EV_KEY, BTN_MIDDLE);
> >
> > __set_bit(INPUT_PROP_POINTER, psmouse->dev->propbit);
> > __set_bit(INPUT_PROP_POINTING_STICK, psmouse->dev->propbit);
> >
> > - trackpoint_defaults(psmouse->private);
> > -
> > - error = trackpoint_power_on_reset(ps2dev);
> > -
> > - /* Write defaults to TP only if reset fails. */
> > - if (error)
> > + if (variant_id != TP_VARIANT_IBM ||
> > + trackpoint_power_on_reset(ps2dev) != 0) {
> > + /*
> > + * Write defaults to TP if we did not reset the trackpoint.
> > + */
> > trackpoint_sync(psmouse, false);
> > + }
> >
> > - error = sysfs_create_group(&ps2dev->serio->dev.kobj, &trackpoint_attr_group);
> > + error = device_add_group(&ps2dev->serio->dev, &trackpoint_attr_group);
> > if (error) {
> > psmouse_err(psmouse,
> > "failed to create sysfs attributes, error: %d\n",
> > @@ -420,8 +473,8 @@ int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
> > }
> >
> > psmouse_info(psmouse,
> > - "IBM TrackPoint firmware: 0x%02x, buttons: %d/%d\n",
> > - firmware_id,
> > + "%s TrackPoint firmware: 0x%02x, buttons: %d/%d\n",
> > + psmouse->vendor, firmware_id,
> > (button_info & 0xf0) >> 4, button_info & 0x0f);
> >
> > return 0;
> > diff --git a/drivers/input/mouse/trackpoint.h b/drivers/input/mouse/trackpoint.h
> > index 88055755f82e..10a039148234 100644
> > --- a/drivers/input/mouse/trackpoint.h
> > +++ b/drivers/input/mouse/trackpoint.h
> > @@ -21,10 +21,16 @@
> > #define TP_COMMAND 0xE2 /* Commands start with this */
> >
> > #define TP_READ_ID 0xE1 /* Sent for device identification */
> > -#define TP_MAGIC_IDENT 0x03 /* Sent after a TP_READ_ID followed */
> > - /* by the firmware ID */
> > - /* Firmware ID includes 0x1, 0x2, 0x3 */
> >
> > +/*
> > + * Valid first byte responses to the "Read Secondary ID" (0xE1) command.
> > + * 0x01 was the original IBM trackpoint, others implement very limited
> > + * subset of trackpoint features.
> > + */
> > +#define TP_VARIANT_IBM 0x01
> > +#define TP_VARIANT_ALPS 0x02
> > +#define TP_VARIANT_ELAN 0x03
> > +#define TP_VARIANT_NXP 0x04
> >
> > /*
> > * Commands
> > @@ -136,18 +142,20 @@
> >
> > #define MAKE_PS2_CMD(params, results, cmd) ((params<<12) | (results<<8) | (cmd))
> >
> > -struct trackpoint_data
> > -{
> > - unsigned char sensitivity, speed, inertia, reach;
> > - unsigned char draghys, mindrag;
> > - unsigned char thresh, upthresh;
> > - unsigned char ztime, jenks;
> > - unsigned char drift_time;
> > +struct trackpoint_data {
> > + u8 variant_id;
> > + u8 firmware_id;
> > +
> > + u8 sensitivity, speed, inertia, reach;
> > + u8 draghys, mindrag;
> > + u8 thresh, upthresh;
> > + u8 ztime, jenks;
> > + u8 drift_time;
> >
> > /* toggles */
> > - unsigned char press_to_select;
> > - unsigned char skipback;
> > - unsigned char ext_dev;
> > + bool press_to_select;
> > + bool skipback;
> > + bool ext_dev;
> > };
> >
> > #ifdef CONFIG_MOUSE_PS2_TRACKPOINT
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-input" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Dmitry
^ permalink raw reply
* Re: [PATCH] synaptics: fix rmi4 bus dependencies
From: Dmitry Torokhov @ 2018-01-16 23:44 UTC (permalink / raw)
To: Matteo Croce; +Cc: Andrew Duggan, linux-input
In-Reply-To: <CAGnkfhwF2O51ouEiX1KAMYNN+OqtpiZSHx62BqomO8vG0Wow8g@mail.gmail.com>
On Tue, Jan 16, 2018 at 07:08:04PM +0100, Matteo Croce wrote:
> On Tue, Jan 9, 2018 at 11:19 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Tue, Jan 02, 2018 at 11:37:07AM -0800, Andrew Duggan wrote:
> >> On 01/02/2018 11:21 AM, Dmitry Torokhov wrote:
> >> > On Tue, Jan 02, 2018 at 08:16:09PM +0100, Matteo Croce wrote:
> >> > > Make MOUSE_PS2_SYNAPTICS_SMBUS select MOUSE_PS2_SYNAPTICS because if
> >> > > the latter is disabled the detection and switching code isn't being run.
> >> > Hmm, it should. In synaptics.c:
> >> >
> >> > #if defined(CONFIG_MOUSE_PS2_SYNAPTICS) || \
> >> > defined(CONFIG_MOUSE_PS2_SYNAPTICS_SMBUS)
> >> >
> >> > int synaptics_init(struct psmouse *psmouse)
> >> > {
> >> > struct synaptics_device_info info;
> >> > int error;
> >> > int retval;
> >> >
> >> > psmouse_reset(psmouse);
> >> > ...
> >> > }
> >> >
> >> > The intent was to allow disabling the PS/2 portion of Synaptics driver
> >> > while retaining the switching to SMbus capability.
> >>
> >> Oh, I suggested to Matteo that this patch might be needed off list. I
> >> clearly didn't look closely enough to see that MOUSE_PS2_SYNAPTICS_SMBUS is
> >> intended to be able to be used independently of MOUSE_PS2_SYNAPTICS.
> >>
> >> However, he did seem to see behavior where the mode switch did not occur
> >> when MOUSE_PS2_SYNAPTICS was disabled, but MOUSE_PS2_SYNAPTICS_SMBUS was
> >> enabled. That will need to be investigated further.
> >
> > OK, I see what the problem is. Could you please try the version below?
> >
> > Thanks.
> >
> > --
> > Dmitry
> >
> >
> > Input: psmouse - fix Synaptics detection when protocol is disabled
> >
> > When Synaptics protocol is disabled, we still need to ytry and detect the
> > hardware, so we can switch to SMBus device if SMbus is detected, or we know
> > that it is Synaptics device and reset it properly for the bare PS/2
> > protocol.
> >
> > Fixes: c378b5119eb0 ("Input: psmouse - factor out common protocol probing code")
> > Reported-by: Matteo Croce <mcroce@redhat.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > drivers/input/mouse/psmouse-base.c | 34 +++++++++++++++++++++-------------
> > 1 file changed, 21 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
> > index f3eb407a3b7d..cbe12671f784 100644
> > --- a/drivers/input/mouse/psmouse-base.c
> > +++ b/drivers/input/mouse/psmouse-base.c
> > @@ -984,6 +984,21 @@ static void psmouse_apply_defaults(struct psmouse *psmouse)
> > psmouse->pt_deactivate = NULL;
> > }
> >
> > +static bool psmouse_do_detect(int (*detect)(struct psmouse *, bool),
> > + struct psmouse *psmouse, bool allow_passthrough,
> > + bool set_properties)
> > +{
> > + if (psmouse->ps2dev.serio->id.type == SERIO_PS_PSTHRU &&
> > + !allow_passthrough) {
> > + return false;
> > + }
> > +
> > + if (set_properties)
> > + psmouse_apply_defaults(psmouse);
> > +
> > + return detect(psmouse, set_properties) == 0;
> > +}
> > +
> > static bool psmouse_try_protocol(struct psmouse *psmouse,
> > enum psmouse_type type,
> > unsigned int *max_proto,
> > @@ -995,15 +1010,8 @@ static bool psmouse_try_protocol(struct psmouse *psmouse,
> > if (!proto)
> > return false;
> >
> > - if (psmouse->ps2dev.serio->id.type == SERIO_PS_PSTHRU &&
> > - !proto->try_passthru) {
> > - return false;
> > - }
> > -
> > - if (set_properties)
> > - psmouse_apply_defaults(psmouse);
> > -
> > - if (proto->detect(psmouse, set_properties) != 0)
> > + if (!psmouse_do_detect(proto->detect, psmouse, proto->try_passthru,
> > + set_properties))
> > return false;
> >
> > if (set_properties && proto->init && init_allowed) {
> > @@ -1036,8 +1044,8 @@ static int psmouse_extensions(struct psmouse *psmouse,
> > * Always check for focaltech, this is safe as it uses pnp-id
> > * matching.
> > */
> > - if (psmouse_try_protocol(psmouse, PSMOUSE_FOCALTECH,
> > - &max_proto, set_properties, false)) {
> > + if (psmouse_do_detect(focaltech_detect,
> > + psmouse, false, set_properties)) {
> > if (max_proto > PSMOUSE_IMEX &&
> > IS_ENABLED(CONFIG_MOUSE_PS2_FOCALTECH) &&
> > (!set_properties || focaltech_init(psmouse) == 0)) {
> > @@ -1083,8 +1091,8 @@ static int psmouse_extensions(struct psmouse *psmouse,
> > * probing for IntelliMouse.
> > */
> > if (max_proto > PSMOUSE_PS2 &&
> > - psmouse_try_protocol(psmouse, PSMOUSE_SYNAPTICS, &max_proto,
> > - set_properties, false)) {
> > + psmouse_do_detect(synaptics_detect,
> > + psmouse, false, set_properties)) {
> > synaptics_hardware = true;
> >
> > if (max_proto > PSMOUSE_IMEX) {
>
> Hi Dmitry,
>
> I've compiled the kernel with this config:
>
> $ zgrep CONFIG_MOUSE_PS2 /proc/config.gz
> CONFIG_MOUSE_PS2=m
> # CONFIG_MOUSE_PS2_ALPS is not set
> # CONFIG_MOUSE_PS2_BYD is not set
> # CONFIG_MOUSE_PS2_LOGIPS2PP is not set
> # CONFIG_MOUSE_PS2_SYNAPTICS is not set
> CONFIG_MOUSE_PS2_SYNAPTICS_SMBUS=y
> # CONFIG_MOUSE_PS2_CYPRESS is not set
> # CONFIG_MOUSE_PS2_LIFEBOOK is not set
> CONFIG_MOUSE_PS2_TRACKPOINT=y
> # CONFIG_MOUSE_PS2_ELANTECH is not set
> # CONFIG_MOUSE_PS2_SENTELIC is not set
> # CONFIG_MOUSE_PS2_TOUCHKIT is not set
> # CONFIG_MOUSE_PS2_FOCALTECH is not set
> CONFIG_MOUSE_PS2_SMBUS=y
>
> and the mouse is correctly detected as rmi4:
>
> $ dmesg |grep rmi4
> [ 20.750242] rmi4_smbus 7-002c: registering SMbus-connected sensor
> [ 20.808625] rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer:
> Synaptics, product: TM3145-003, fw id: 2073050
> [ 20.905347] input: Synaptics TM3145-003 as /devices/rmi4-00/input/input16
> [ 21.299073] input: TPPS/2 IBM TrackPoint as
> /devices/rmi4-00/rmi4-00.fn03/serio2/input/input17
Great, putting you as tested-by then.
>
> Anyway, is CONFIG_MOUSE_PS2_TRACKPOINT still needed for the trackpoint
> or CONFIG_MOUSE_PS2_SYNAPTICS_SMBUS will handle both?
No, the trackpoint is still a PS/2 device, it is simply attached as
a "guest" or pass-through port of an RMI device. You still need
CONFIG_MOUSE_PS2_TRACKPOINT if you want its advanced features.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: synaptics - reset the ABS_X/Y fuzz after initializing MT axes
From: Dmitry Torokhov @ 2018-01-16 23:41 UTC (permalink / raw)
To: Peter Hutterer; +Cc: linux-input, Benjamin Tissoires, Andrew Duggan
In-Reply-To: <20180116053459.GA14660@jelly>
On Tue, Jan 16, 2018 at 03:34:59PM +1000, Peter Hutterer wrote:
> input_mt_init_slots() resets the ABS_X/Y fuzz to 0 and expects the driver to
> call input_mt_report_pointer_emulation(). That is based on the MT position
> bits which are already defuzzed - hence a fuzz of 0.
>
> In the case of synaptics semi-mt devices, we report the ABS_X/Y axes manually.
> This results in the MT position being defuzzed but the single-touch emulation
> missing that defuzzing.
>
> Work around this by re-initializing the ABS_X/Y axes after the MT axis to
> get the same fuzz value back.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=104533
>
> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Applied, thank you.
> ---
> Semi-mt devices are Dodos, so I'd rather go for the easy fix here than a
> big rewrite of that code.
>
> drivers/input/mouse/synaptics.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index ee5466a374bf..0e04275e0d74 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -1280,6 +1280,14 @@ static void set_input_params(struct psmouse *psmouse,
> INPUT_MT_POINTER |
> (cr48_profile_sensor ?
> INPUT_MT_TRACK : INPUT_MT_SEMI_MT));
> +
> + /* For semi-mt devices we send ABS_X/Y ourselves instead of
> + * input_mt_report_pointer_emulation. But
> + * input_mt_init_slots() resets the fuzz to 0, leading to a
> + * filtered ABS_MT_POSITION_X but an unfiltered ABS_X
> + * position. Let's re-initialize ABS_X/Y here. */
> + if (!cr48_profile_sensor)
> + set_abs_position_params(dev, &priv->info, ABS_X, ABS_Y);
> }
>
> if (SYN_CAP_PALMDETECT(info->capabilities))
> --
> 2.14.3
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v3] input: pxrc: new driver for PhoenixRC Flight Controller Adapter
From: Dmitry Torokhov @ 2018-01-16 23:16 UTC (permalink / raw)
To: Marcus Folkesson
Cc: Jonathan Corbet, Tomohiro Yoshidomi, David Herrmann,
Philippe Ombredanne, Kate Stewart, Greg Kroah-Hartman,
linux-input, linux-doc, linux-kernel
In-Reply-To: <20180113201532.26016-1-marcus.folkesson@gmail.com>
Hi Marcus,
On Sat, Jan 13, 2018 at 09:15:32PM +0100, Marcus Folkesson wrote:
> This driver let you plug in your RC controller to the adapter and
> use it as input device in various RC simulators.
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
> v3:
> - Use RUDDER and MISC instead of TILT_X and TILT_Y
> - Drop kref and anchor
> - Rework URB handling
> - Add PM support
How did you test the PM support? By default the autopm is disabled on
USB devices; you need to enable it by writing to sysfs (I believe you
need to 'echo "auto" > /sys/bus/usb/<device>/power/control) and see if
it gets autosuspended when not in use and resumed after you start
interacting with it.
> v2:
> - Change module license to GPLv2 to match SPDX tag
>
> Documentation/input/devices/pxrc.rst | 57 +++++++
> drivers/input/joystick/Kconfig | 9 +
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/pxrc.c | 320 +++++++++++++++++++++++++++++++++++
> 4 files changed, 387 insertions(+)
> create mode 100644 Documentation/input/devices/pxrc.rst
> create mode 100644 drivers/input/joystick/pxrc.c
>
> diff --git a/Documentation/input/devices/pxrc.rst b/Documentation/input/devices/pxrc.rst
> new file mode 100644
> index 000000000000..ca11f646bae8
> --- /dev/null
> +++ b/Documentation/input/devices/pxrc.rst
> @@ -0,0 +1,57 @@
> +=======================================================
> +pxrc - PhoenixRC Flight Controller Adapter
> +=======================================================
> +
> +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> +
> +This driver let you use your own RC controller plugged into the
> +adapter that comes with PhoenixRC [1]_ or other compatible adapters.
> +
> +The adapter supports 7 analog channels and 1 digital input switch.
> +
> +Notes
> +=====
> +
> +Many RC controllers is able to configure which stick goes to which channel.
> +This is also configurable in most simulators, so a matching is not necessary.
> +
> +The driver is generating the following input event for analog channels:
> +
> ++---------+----------------+
> +| Channel | Event |
> ++=========+================+
> +| 1 | ABS_X |
> ++---------+----------------+
> +| 2 | ABS_Y |
> ++---------+----------------+
> +| 3 | ABS_RX |
> ++---------+----------------+
> +| 4 | ABS_RY |
> ++---------+----------------+
> +| 5 | ABS_RUDDER |
> ++---------+----------------+
> +| 6 | ABS_THROTTLE |
> ++---------+----------------+
> +| 7 | ABS_MISC |
> ++---------+----------------+
> +
> +The digital input switch is generated as an `BTN_A` event.
> +
> +Manual Testing
> +==============
> +
> +To test this driver's functionality you may use `input-event` which is part of
> +the `input layer utilities` suite [2]_.
> +
> +For example::
> +
> + > modprobe pxrc
> + > input-events <devnr>
> +
> +To print all input events from input `devnr`.
> +
> +References
> +==========
> +
> +.. [1] http://www.phoenix-sim.com/
> +.. [2] https://www.kraxel.org/cgit/input/
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index f3c2f6ea8b44..18ab6dafff41 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -351,4 +351,13 @@ config JOYSTICK_PSXPAD_SPI_FF
>
> To drive rumble motor a dedicated power supply is required.
>
> +config JOYSTICK_PXRC
> + tristate "PhoenixRC Flight Controller Adapter"
> + depends on USB_ARCH_HAS_HCD
> + select USB
> + help
> + Say Y here if you want to use the PhoenixRC Flight Controller Adapter.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called pxrc.
> endif
> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index 67651efda2e1..dd0492ebbed7 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
> obj-$(CONFIG_JOYSTICK_MAGELLAN) += magellan.o
> obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o
> obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> +obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o
> diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
> new file mode 100644
> index 000000000000..98d9b8184c46
> --- /dev/null
> +++ b/drivers/input/joystick/pxrc.c
> @@ -0,0 +1,320 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Phoenix RC Flight Controller Adapter
> + *
> + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +#include <linux/usb.h>
> +#include <linux/usb/input.h>
> +#include <linux/mutex.h>
> +#include <linux/input.h>
> +
> +#define PXRC_VENDOR_ID (0x1781)
> +#define PXRC_PRODUCT_ID (0x0898)
> +
> +static const struct usb_device_id pxrc_table[] = {
> + { USB_DEVICE(PXRC_VENDOR_ID, PXRC_PRODUCT_ID) },
> + { }
> +};
> +MODULE_DEVICE_TABLE(usb, pxrc_table);
> +
> +struct pxrc {
> + struct input_dev *input;
> + struct usb_device *udev;
> + struct usb_interface *intf;
> + struct urb *urb;
> + __u8 epaddr;
> + char phys[64];
> + unsigned char *data;
> + size_t bsize;
> +};
> +
> +static void pxrc_usb_irq(struct urb *urb)
> +{
> + struct pxrc *pxrc = urb->context;
> + int error;
> +
> + switch (urb->status) {
> + case 0:
> + /* success */
> + break;
> + case -ETIME:
> + /* this urb is timing out */
> + dev_dbg(&pxrc->intf->dev,
> + "%s - urb timed out - was the device unplugged?\n",
> + __func__);
> + return;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + case -EPIPE:
> + /* this urb is terminated, clean up */
> + dev_dbg(&pxrc->intf->dev, "%s - urb shutting down with status: %d\n",
> + __func__, urb->status);
> + return;
> + default:
> + dev_dbg(&pxrc->intf->dev, "%s - nonzero urb status received: %d\n",
> + __func__, urb->status);
> + goto exit;
> + }
> +
> + if (urb->actual_length == 8) {
> + input_report_abs(pxrc->input, ABS_X, pxrc->data[0]);
> + input_report_abs(pxrc->input, ABS_Y, pxrc->data[2]);
> + input_report_abs(pxrc->input, ABS_RX, pxrc->data[3]);
> + input_report_abs(pxrc->input, ABS_RY, pxrc->data[4]);
> + input_report_abs(pxrc->input, ABS_RUDDER, pxrc->data[5]);
> + input_report_abs(pxrc->input, ABS_THROTTLE, pxrc->data[6]);
> + input_report_abs(pxrc->input, ABS_MISC, pxrc->data[7]);
> +
> + input_report_key(pxrc->input, BTN_A, pxrc->data[1]);
> + }
> +
> +exit:
I think you need
usb_mark_last_busy(interface_to_usbdev(pxrc->intf));
here.
> + /* Resubmit to fetch new fresh URBs */
> + error = usb_submit_urb(urb, GFP_ATOMIC);
> + if (error && error != -EPERM)
> + dev_err(&pxrc->intf->dev,
> + "%s - usb_submit_urb failed with result: %d",
> + __func__, error);
> +}
> +
> +static int pxrc_open(struct input_dev *input)
> +{
> + struct pxrc *pxrc = input_get_drvdata(input);
> + int retval;
> +
> + retval = usb_autopm_get_interface(pxrc->intf);
> + if (retval) {
> + dev_err(&pxrc->intf->dev,
> + "%s - usb_autopm_get_interface failed, error: %d\n",
> + __func__, retval);
> + return retval;
> + }
> +
> + retval = usb_submit_urb(pxrc->urb, GFP_KERNEL);
> + if (retval) {
> + dev_err(&pxrc->intf->dev,
> + "%s - usb_submit_urb failed, error: %d\n",
> + __func__, retval);
> + retval = -EIO;
> + goto out;
> + }
> +
> + pxrc->intf->needs_remote_wakeup = 1;
> +
> +out:
> + usb_autopm_put_interface(pxrc->intf);
> + return retval;
> +}
> +
> +static void pxrc_close(struct input_dev *input)
> +{
> + struct pxrc *pxrc = input_get_drvdata(input);
> + int autopm_error;
> +
> + autopm_error = usb_autopm_get_interface(pxrc->intf);
> +
> + usb_kill_urb(pxrc->urb);
> + pxrc->intf->needs_remote_wakeup = 0;
> +
> + if (!autopm_error)
> + usb_autopm_put_interface(pxrc->intf);
> +}
> +
> +static int pxrc_usb_init(struct pxrc *pxrc)
> +{
> + struct usb_endpoint_descriptor *epirq;
> + unsigned int pipe;
> + int retval;
> +
> + /* Set up the endpoint information */
> + /* This device only has an interrupt endpoint */
> + retval = usb_find_common_endpoints(pxrc->intf->cur_altsetting,
> + NULL, NULL, &epirq, NULL);
> + if (retval) {
> + dev_err(&pxrc->intf->dev,
> + "Could not find endpoint\n");
> + goto error;
> + }
> +
> + pxrc->bsize = usb_endpoint_maxp(epirq);
> + pxrc->epaddr = epirq->bEndpointAddress;
> + pxrc->data = devm_kmalloc(&pxrc->intf->dev, pxrc->bsize, GFP_KERNEL);
> + if (!pxrc->data) {
> + retval = -ENOMEM;
> + goto error;
> + }
> +
> + usb_set_intfdata(pxrc->intf, pxrc);
> + usb_make_path(pxrc->udev, pxrc->phys, sizeof(pxrc->phys));
> + strlcat(pxrc->phys, "/input0", sizeof(pxrc->phys));
> +
> + pxrc->urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!pxrc->urb) {
> + retval = -ENOMEM;
> + goto error;
> + }
> +
> + pipe = usb_rcvintpipe(pxrc->udev, pxrc->epaddr),
> + usb_fill_int_urb(pxrc->urb, pxrc->udev, pipe, pxrc->data, pxrc->bsize,
> + pxrc_usb_irq, pxrc, 1);
> +
> +error:
> + return retval;
> +
> +
> +}
> +
> +
> +static int pxrc_input_init(struct pxrc *pxrc)
> +{
> + pxrc->input = devm_input_allocate_device(&pxrc->intf->dev);
> + if (pxrc->input == NULL) {
> + dev_err(&pxrc->intf->dev, "couldn't allocate input device\n");
> + return -ENOMEM;
> + }
> +
> + pxrc->input->name = "PXRC Flight Controller Adapter";
> + pxrc->input->phys = pxrc->phys;
> + usb_to_input_id(pxrc->udev, &pxrc->input->id);
> +
> + pxrc->input->open = pxrc_open;
> + pxrc->input->close = pxrc_close;
> +
> + input_set_capability(pxrc->input, EV_KEY, BTN_A);
> + input_set_abs_params(pxrc->input, ABS_X, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_Y, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_RX, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_RY, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_RUDDER, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_THROTTLE, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_MISC, 0, 255, 0, 0);
> +
> + input_set_drvdata(pxrc->input, pxrc);
> +
> + return input_register_device(pxrc->input);
> +}
> +
> +static int pxrc_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + struct pxrc *pxrc;
> + int retval;
> +
> + pxrc = devm_kzalloc(&intf->dev, sizeof(*pxrc), GFP_KERNEL);
> +
> + if (!pxrc)
> + return -ENOMEM;
> +
> + pxrc->udev = usb_get_dev(interface_to_usbdev(intf));
> + pxrc->intf = intf;
> +
> + retval = pxrc_usb_init(pxrc);
> + if (retval)
> + goto error;
> +
> + retval = pxrc_input_init(pxrc);
> + if (retval)
> + goto err_free_urb;
> +
> + return 0;
> +
> +err_free_urb:
> + usb_free_urb(pxrc->urb);
> +
> +error:
> + return retval;
> +}
> +
> +static void pxrc_disconnect(struct usb_interface *intf)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> +
> + usb_free_urb(pxrc->urb);
> + usb_set_intfdata(intf, NULL);
> +}
> +
> +static int pxrc_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> + struct input_dev *input_dev = pxrc->input;
> +
> + mutex_lock(&input_dev->mutex);
> + usb_kill_urb(pxrc->urb);
> + mutex_unlock(&input_dev->mutex);
> +
> + return 0;
> +}
> +
> +static int pxrc_resume(struct usb_interface *intf)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> + struct input_dev *input_dev = pxrc->input;
> + int retval = 0;
> +
> + mutex_lock(&input_dev->mutex);
> +
> + if (input_dev->users && usb_submit_urb(pxrc->urb, GFP_NOIO) < 0)
> + retval = -EIO;
> +
> + mutex_unlock(&input_dev->mutex);
> +
> + return retval;
> +}
> +
> +static int pxrc_pre_reset(struct usb_interface *intf)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> + struct input_dev *input_dev = pxrc->input;
> +
> + mutex_lock(&input_dev->mutex);
> + usb_kill_urb(pxrc->urb);
> +
> + return 0;
> +}
> +
> +static int pxrc_post_reset(struct usb_interface *intf)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> + struct input_dev *input_dev = pxrc->input;
> + int retval = 0;
> +
> + if (input_dev->users && usb_submit_urb(pxrc->urb, GFP_NOIO) < 0)
> + retval = -EIO;
> +
> + mutex_unlock(&input_dev->mutex);
> +
> + return retval;
> +}
> +
> +static int pxrc_reset_resume(struct usb_interface *intf)
> +{
> + return pxrc_resume(intf);
> +}
> +
> +static struct usb_driver pxrc_driver = {
> + .name = "pxrc",
> + .probe = pxrc_probe,
> + .disconnect = pxrc_disconnect,
> + .id_table = pxrc_table,
> + .suspend = pxrc_suspend,
> + .resume = pxrc_resume,
> + .pre_reset = pxrc_pre_reset,
> + .post_reset = pxrc_post_reset,
> + .reset_resume = pxrc_reset_resume,
> + .supports_autosuspend = 1,
> +};
> +
> +module_usb_driver(pxrc_driver);
> +
> +MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
> +MODULE_DESCRIPTION("PhoenixRC Flight Controller Adapter");
> +MODULE_LICENSE("GPL v2");
> --
> 2.15.1
>
Thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: trackpoint - force 3 buttons if 0 button is reported
From: Dmitry Torokhov @ 2018-01-16 23:07 UTC (permalink / raw)
To: Aaron Ma; +Cc: linux-input, linux-kernel
In-Reply-To: <6a96ce54-9f8a-2424-1e48-34ec6445cb8f@canonical.com>
On Sat, Jan 13, 2018 at 10:34:19AM +0800, Aaron Ma wrote:
> Will your patch go to stable kernel?
> If yes, that's fine.
Hmm, it looks like we need this patch after all as we do have screwy
Lenovos with apparently 0x01 devices reporting 0 buttons. From
https://bugzilla.kernel.org/show_bug.cgi?id=196253
thinkpad_acpi: ThinkPad BIOS R0DET87W (1.87 ), EC unknown
thinkpad_acpi: Lenovo ThinkPad E470, model 20H1004SGE
psmouse serio2: trackpoint: IBM TrackPoint firmware: 0x0e, buttons: 0/0
I'll ask them to try my patch and if it is indeed 0x01 device I'll apply
your patch.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] synaptics: fix rmi4 bus dependencies
From: Matteo Croce @ 2018-01-16 18:08 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Andrew Duggan, linux-input
In-Reply-To: <20180109221933.nexohjl75g4uu2nl@dtor-ws>
On Tue, Jan 9, 2018 at 11:19 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, Jan 02, 2018 at 11:37:07AM -0800, Andrew Duggan wrote:
>> On 01/02/2018 11:21 AM, Dmitry Torokhov wrote:
>> > On Tue, Jan 02, 2018 at 08:16:09PM +0100, Matteo Croce wrote:
>> > > Make MOUSE_PS2_SYNAPTICS_SMBUS select MOUSE_PS2_SYNAPTICS because if
>> > > the latter is disabled the detection and switching code isn't being run.
>> > Hmm, it should. In synaptics.c:
>> >
>> > #if defined(CONFIG_MOUSE_PS2_SYNAPTICS) || \
>> > defined(CONFIG_MOUSE_PS2_SYNAPTICS_SMBUS)
>> >
>> > int synaptics_init(struct psmouse *psmouse)
>> > {
>> > struct synaptics_device_info info;
>> > int error;
>> > int retval;
>> >
>> > psmouse_reset(psmouse);
>> > ...
>> > }
>> >
>> > The intent was to allow disabling the PS/2 portion of Synaptics driver
>> > while retaining the switching to SMbus capability.
>>
>> Oh, I suggested to Matteo that this patch might be needed off list. I
>> clearly didn't look closely enough to see that MOUSE_PS2_SYNAPTICS_SMBUS is
>> intended to be able to be used independently of MOUSE_PS2_SYNAPTICS.
>>
>> However, he did seem to see behavior where the mode switch did not occur
>> when MOUSE_PS2_SYNAPTICS was disabled, but MOUSE_PS2_SYNAPTICS_SMBUS was
>> enabled. That will need to be investigated further.
>
> OK, I see what the problem is. Could you please try the version below?
>
> Thanks.
>
> --
> Dmitry
>
>
> Input: psmouse - fix Synaptics detection when protocol is disabled
>
> When Synaptics protocol is disabled, we still need to ytry and detect the
> hardware, so we can switch to SMBus device if SMbus is detected, or we know
> that it is Synaptics device and reset it properly for the bare PS/2
> protocol.
>
> Fixes: c378b5119eb0 ("Input: psmouse - factor out common protocol probing code")
> Reported-by: Matteo Croce <mcroce@redhat.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/mouse/psmouse-base.c | 34 +++++++++++++++++++++-------------
> 1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
> index f3eb407a3b7d..cbe12671f784 100644
> --- a/drivers/input/mouse/psmouse-base.c
> +++ b/drivers/input/mouse/psmouse-base.c
> @@ -984,6 +984,21 @@ static void psmouse_apply_defaults(struct psmouse *psmouse)
> psmouse->pt_deactivate = NULL;
> }
>
> +static bool psmouse_do_detect(int (*detect)(struct psmouse *, bool),
> + struct psmouse *psmouse, bool allow_passthrough,
> + bool set_properties)
> +{
> + if (psmouse->ps2dev.serio->id.type == SERIO_PS_PSTHRU &&
> + !allow_passthrough) {
> + return false;
> + }
> +
> + if (set_properties)
> + psmouse_apply_defaults(psmouse);
> +
> + return detect(psmouse, set_properties) == 0;
> +}
> +
> static bool psmouse_try_protocol(struct psmouse *psmouse,
> enum psmouse_type type,
> unsigned int *max_proto,
> @@ -995,15 +1010,8 @@ static bool psmouse_try_protocol(struct psmouse *psmouse,
> if (!proto)
> return false;
>
> - if (psmouse->ps2dev.serio->id.type == SERIO_PS_PSTHRU &&
> - !proto->try_passthru) {
> - return false;
> - }
> -
> - if (set_properties)
> - psmouse_apply_defaults(psmouse);
> -
> - if (proto->detect(psmouse, set_properties) != 0)
> + if (!psmouse_do_detect(proto->detect, psmouse, proto->try_passthru,
> + set_properties))
> return false;
>
> if (set_properties && proto->init && init_allowed) {
> @@ -1036,8 +1044,8 @@ static int psmouse_extensions(struct psmouse *psmouse,
> * Always check for focaltech, this is safe as it uses pnp-id
> * matching.
> */
> - if (psmouse_try_protocol(psmouse, PSMOUSE_FOCALTECH,
> - &max_proto, set_properties, false)) {
> + if (psmouse_do_detect(focaltech_detect,
> + psmouse, false, set_properties)) {
> if (max_proto > PSMOUSE_IMEX &&
> IS_ENABLED(CONFIG_MOUSE_PS2_FOCALTECH) &&
> (!set_properties || focaltech_init(psmouse) == 0)) {
> @@ -1083,8 +1091,8 @@ static int psmouse_extensions(struct psmouse *psmouse,
> * probing for IntelliMouse.
> */
> if (max_proto > PSMOUSE_PS2 &&
> - psmouse_try_protocol(psmouse, PSMOUSE_SYNAPTICS, &max_proto,
> - set_properties, false)) {
> + psmouse_do_detect(synaptics_detect,
> + psmouse, false, set_properties)) {
> synaptics_hardware = true;
>
> if (max_proto > PSMOUSE_IMEX) {
Hi Dmitry,
I've compiled the kernel with this config:
$ zgrep CONFIG_MOUSE_PS2 /proc/config.gz
CONFIG_MOUSE_PS2=m
# CONFIG_MOUSE_PS2_ALPS is not set
# CONFIG_MOUSE_PS2_BYD is not set
# CONFIG_MOUSE_PS2_LOGIPS2PP is not set
# CONFIG_MOUSE_PS2_SYNAPTICS is not set
CONFIG_MOUSE_PS2_SYNAPTICS_SMBUS=y
# CONFIG_MOUSE_PS2_CYPRESS is not set
# CONFIG_MOUSE_PS2_LIFEBOOK is not set
CONFIG_MOUSE_PS2_TRACKPOINT=y
# CONFIG_MOUSE_PS2_ELANTECH is not set
# CONFIG_MOUSE_PS2_SENTELIC is not set
# CONFIG_MOUSE_PS2_TOUCHKIT is not set
# CONFIG_MOUSE_PS2_FOCALTECH is not set
CONFIG_MOUSE_PS2_SMBUS=y
and the mouse is correctly detected as rmi4:
$ dmesg |grep rmi4
[ 20.750242] rmi4_smbus 7-002c: registering SMbus-connected sensor
[ 20.808625] rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer:
Synaptics, product: TM3145-003, fw id: 2073050
[ 20.905347] input: Synaptics TM3145-003 as /devices/rmi4-00/input/input16
[ 21.299073] input: TPPS/2 IBM TrackPoint as
/devices/rmi4-00/rmi4-00.fn03/serio2/input/input17
Anyway, is CONFIG_MOUSE_PS2_TRACKPOINT still needed for the trackpoint
or CONFIG_MOUSE_PS2_SYNAPTICS_SMBUS will handle both?
Regards,
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply
* [PATCH] HID: logitech-hidpp: remove duplicated assignment to 'hidpp'
From: Colin King @ 2018-01-16 17:38 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Pointer hidpp is being assigned twice with the same value, remove
the second occurrance as it is redundant.
Cleans up clang warning:
drivers/hid/hid-logitech-hidpp.c:177:23: warning: Value stored to
'hidpp' during its initialization is never read
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/hid/hid-logitech-hidpp.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 19cc980eebce..b91e416a677d 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -177,8 +177,6 @@ static int __hidpp_send_report(struct hid_device *hdev,
struct hidpp_device *hidpp = hid_get_drvdata(hdev);
int fields_count, ret;
- hidpp = hid_get_drvdata(hdev);
-
switch (hidpp_report->report_id) {
case REPORT_ID_HIDPP_SHORT:
fields_count = HIDPP_REPORT_SHORT_LENGTH;
--
2.15.1
^ permalink raw reply related
* Re: [PATCH v2] Input: mms114 - drop platform data and use generic APIs
From: Simon Shields @ 2018-01-16 8:52 UTC (permalink / raw)
To: Andi Shyti; +Cc: Dmitry Torokhov, linux-input, devicetree, Rob Herring
In-Reply-To: <20180116075611.GC30945@gangnam.samsung>
Hi Andi,
Thanks for the review!
On Tue, Jan 16, 2018 at 04:56:11PM +0900, Andi Shyti wrote:
> Hi Simon,
>
> On Sat, Jan 13, 2018 at 01:04:56PM +1100, Simon Shields wrote:
> > The MMS114 platform data has no in-tree users, so drop it,
> > and make the driver depend on CONFIG_OF.
> >
> > Switch to using the standard touchscreen properties via
> > touchscreen_parse_properties(), and move the old DT parsing code
> > to use device_property_*() APIs.
> >
> > Finally, use touchscreen_report_pos to report x/y coordinates
> > and drop the custom x/y inversion code.
> >
> > Signed-off-by: Simon Shields <simon@lineageos.org>
> > ---
> > .../bindings/input/touchscreen/mms114.txt | 29 ++--
> > drivers/input/touchscreen/Kconfig | 1 +
> > drivers/input/touchscreen/mms114.c | 152 +++++++++------------
> > include/linux/platform_data/mms114.h | 24 ----
> > 4 files changed, 83 insertions(+), 123 deletions(-)
> > delete mode 100644 include/linux/platform_data/mms114.h
> >
>
> The patch looks good, but you would also need to update the dtsi
> files in this same patch:
>
> ./arch/arm/boot/dts/exynos4412-trats2.dts
> ./arch/arm/boot/dts/exynos4210-trats.dts
>
> and Cc the Samsung-soc mailing list.
>
> For now it's a nack because the touchscreen would not work
> anymore with the trats boards.
This patch keeps support for the old bindings. I've verified that both
the old and new bindings work on a GT-I9300 (trats2 with a different
bootloader/partition layout).
>
> One more thing, you forgot Rob's ACK.
I wasn't sure whether or not to keep the Reviewed-By tag, since
this is a new version of the patch. In the future, I'll keep it.
>
> Andi
Cheers,
Simon
^ permalink raw reply
* Re: [PATCH v2] Input: mms114 - drop platform data and use generic APIs
From: Andi Shyti @ 2018-01-16 7:56 UTC (permalink / raw)
To: Simon Shields
Cc: Dmitry Torokhov, linux-input-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring
In-Reply-To: <20180113020456.12391-1-simon-WP75azK+jQYgsBAKwltoeQ@public.gmane.org>
Hi Simon,
On Sat, Jan 13, 2018 at 01:04:56PM +1100, Simon Shields wrote:
> The MMS114 platform data has no in-tree users, so drop it,
> and make the driver depend on CONFIG_OF.
>
> Switch to using the standard touchscreen properties via
> touchscreen_parse_properties(), and move the old DT parsing code
> to use device_property_*() APIs.
>
> Finally, use touchscreen_report_pos to report x/y coordinates
> and drop the custom x/y inversion code.
>
> Signed-off-by: Simon Shields <simon-WP75azK+jQYgsBAKwltoeQ@public.gmane.org>
> ---
> .../bindings/input/touchscreen/mms114.txt | 29 ++--
> drivers/input/touchscreen/Kconfig | 1 +
> drivers/input/touchscreen/mms114.c | 152 +++++++++------------
> include/linux/platform_data/mms114.h | 24 ----
> 4 files changed, 83 insertions(+), 123 deletions(-)
> delete mode 100644 include/linux/platform_data/mms114.h
>
The patch looks good, but you would also need to update the dtsi
files in this same patch:
./arch/arm/boot/dts/exynos4412-trats2.dts
./arch/arm/boot/dts/exynos4210-trats.dts
and Cc the Samsung-soc mailing list.
For now it's a nack because the touchscreen would not work
anymore with the trats boards.
One more thing, you forgot Rob's ACK.
Andi
--
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
* [PATCH] Input: synaptics - reset the ABS_X/Y fuzz after initializing MT axes
From: Peter Hutterer @ 2018-01-16 5:34 UTC (permalink / raw)
To: linux-input; +Cc: Benjamin Tissoires, Dmitry Torokhov, Andrew Duggan
input_mt_init_slots() resets the ABS_X/Y fuzz to 0 and expects the driver to
call input_mt_report_pointer_emulation(). That is based on the MT position
bits which are already defuzzed - hence a fuzz of 0.
In the case of synaptics semi-mt devices, we report the ABS_X/Y axes manually.
This results in the MT position being defuzzed but the single-touch emulation
missing that defuzzing.
Work around this by re-initializing the ABS_X/Y axes after the MT axis to
get the same fuzz value back.
https://bugs.freedesktop.org/show_bug.cgi?id=104533
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
Semi-mt devices are Dodos, so I'd rather go for the easy fix here than a
big rewrite of that code.
drivers/input/mouse/synaptics.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index ee5466a374bf..0e04275e0d74 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -1280,6 +1280,14 @@ static void set_input_params(struct psmouse *psmouse,
INPUT_MT_POINTER |
(cr48_profile_sensor ?
INPUT_MT_TRACK : INPUT_MT_SEMI_MT));
+
+ /* For semi-mt devices we send ABS_X/Y ourselves instead of
+ * input_mt_report_pointer_emulation. But
+ * input_mt_init_slots() resets the fuzz to 0, leading to a
+ * filtered ABS_MT_POSITION_X but an unfiltered ABS_X
+ * position. Let's re-initialize ABS_X/Y here. */
+ if (!cr48_profile_sensor)
+ set_abs_position_params(dev, &priv->info, ABS_X, ABS_Y);
}
if (SYN_CAP_PALMDETECT(info->capabilities))
--
2.14.3
^ permalink raw reply related
* Neujahrsspende von 4.800.000 €
From: Tom Crist @ 2018-01-15 23:45 UTC (permalink / raw)
--
Hallo, Sie haben eine Spende von 4.800.000,00 Euro, ich habe die America Lottery in Amerika im Wert von 40 Millionen Dollar gewonnen und ich gebe einen Teil davon an fünf glückliche Menschen und Wohltätigkeits-Häuser in Erinnerung an meine verstorbene Frau, die an Krebs gestorben ist. Kontaktieren Sie mich für weitere Details:(tomcrist2009@gmail.com)
^ permalink raw reply
* Re: [PATCH] HID: asus: only support backlight when it's not driven by WMI
From: Daniel Drake @ 2018-01-15 20:33 UTC (permalink / raw)
To: Mustafa Kuscu
Cc: Andy Shevchenko, Jiri Kosina, Benjamin Tissoires, linux-input,
Linux Upstreaming Team, Carlo Caione, Corentin Chary,
acpi4asus-user, Platform Driver
In-Reply-To: <CAMz_NCr-3taeHeZ3NF2LYXrMGf25gWgL+dzdO=Ca=Bxug2EbCw@mail.gmail.com>
Hi Mustafa,
On Fri, Jan 12, 2018 at 1:33 PM, Mustafa Kuscu <mustafakuscu@gmail.com> wrote:
> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> - USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1) },
> + USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1), QUIRK_USE_KBD_BACKLIGHT },
This change is already included in the latest version of Linux so I
assume you are just running an old version.
Andy, still waiting for any more feedback on the issue detailed in
this thread once you have a chance.
Thanks
Daniel
^ permalink raw reply
* Re: [PATCH] HID: roccat: prevent an out of bounds read in kovaplus_profile_activated()
From: Silvan Jegen @ 2018-01-15 20:21 UTC (permalink / raw)
To: Dan Carpenter
Cc: Stefan Achatz, Jiri Kosina, Benjamin Tissoires, linux-input,
kernel-janitors
In-Reply-To: <20180110093903.yyoknoi7oc2cd5iy@mwanda>
Hi
On Wed, Jan 10, 2018 at 12:39:03PM +0300, Dan Carpenter wrote:
> We get the "new_profile_index" value from the mouse device when we're
> handling raw events. Smatch taints it as untrusted data and complains
> that we need a bounds check. This seems like a reasonable warning
> otherwise there is a small read beyond the end of the array.
>
> Fixes: 0e70f97f257e ("HID: roccat: Add support for Kova[+] mouse")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Looks good to me.
Acked-by: Silvan Jegen <s.jegen@gmail.com>
> diff --git a/drivers/hid/hid-roccat-kovaplus.c b/drivers/hid/hid-roccat-kovaplus.c
> index 43617fb28b87..317c9c2c0a7c 100644
> --- a/drivers/hid/hid-roccat-kovaplus.c
> +++ b/drivers/hid/hid-roccat-kovaplus.c
> @@ -37,6 +37,8 @@ static uint kovaplus_convert_event_cpi(uint value)
> static void kovaplus_profile_activated(struct kovaplus_device *kovaplus,
> uint new_profile_index)
> {
> + if (new_profile_index >= ARRAY_SIZE(kovaplus->profile_settings))
> + return;
> kovaplus->actual_profile = new_profile_index;
> kovaplus->actual_cpi = kovaplus->profile_settings[new_profile_index].cpi_startup_level;
> kovaplus->actual_x_sensitivity = kovaplus->profile_settings[new_profile_index].sensitivity_x;
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] Revert "Input: trackpoint - add new trackpoint firmware ID"
From: ulrik.debie-os @ 2018-01-14 20:39 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Aaron Ma, Greg KH, Sebastian Schmidt, linux-input
In-Reply-To: <20180107065201.q2xck62unijt3avr@dtor-ws>
Hi Dmitry, Sebastian,
Nack for the patch..
As Sebastian pointed out, the trackpoint detection will always report an error. (see below)
I builded your original patch and it resulted in detection as generic mouse for the trackpoint:
[ 0.838143] serio: i8042 KBD port at 0x60,0x64 irq 1
[ 0.838146] serio: i8042 AUX port at 0x60,0x64 irq 12
[ 0.838267] mousedev: PS/2 mouse device common for all mice
[ 9.321140] psmouse serio1: synaptics: queried max coordinates: x [..5676], y [..4758]
[ 9.367095] psmouse serio1: synaptics: queried min coordinates: x [1266..], y [1096..]
[ 9.367103] psmouse serio1: synaptics: The touchpad can support a better bus than the too old PS/2 protocol. Make sure MOUSE_PS2_SYNAPTICS_SMBUS and RMI4_SMB are enabled to get a better touchpad experience.
[ 9.455953] psmouse serio1: synaptics: Touchpad model: 1, fw: 8.2, id: 0x1e2b1, caps: 0xf007a3/0x943300/0x12e800/0x410000, board id: 3157, fw id: 2405942
[ 9.455970] psmouse serio1: synaptics: serio: Synaptics pass-through port at isa0060/serio1/input0
[ 9.514117] input: SynPS/2 Synaptics TouchPad as /devices/platform/i8042/serio1/input/input7
[ 11.458443] input: PS/2 Generic Mouse as /devices/platform/i8042/serio1/serio2/input/input18
When I fixed your patch, it resulted in: (timing difference is because a dvd was inserted before)
[ 0.843661] serio: i8042 KBD port at 0x60,0x64 irq 1
[ 0.843664] serio: i8042 AUX port at 0x60,0x64 irq 12
[ 0.843793] mousedev: PS/2 mouse device common for all mice
[ 3.822411] psmouse serio1: synaptics: queried max coordinates: x [..5676], y [..4758]
[ 3.868196] psmouse serio1: synaptics: queried min coordinates: x [1266..], y [1096..]
[ 3.868203] psmouse serio1: synaptics: The touchpad can support a better bus than the too old PS/2 protocol. Make sure MOUSE_PS2_SYNAPTICS_SMBUS and RMI4_SMB are enabled to get a better touchpad experience.
[ 3.961024] psmouse serio1: synaptics: Touchpad model: 1, fw: 8.2, id: 0x1e2b1, caps: 0xf007a3/0x943300/0x12e800/0x410000, board id: 3157, fw id: 2405942
[ 3.961038] psmouse serio1: synaptics: serio: Synaptics pass-through port at isa0060/serio1/input0
[ 4.017290] input: SynPS/2 Synaptics TouchPad as /devices/platform/i8042/serio1/input/input7
[ 4.407930] psmouse serio2: trackpoint: failed to get extended button data, assuming 3 buttons
[ 8.460022] psmouse serio2: trackpoint: IBM TrackPoint firmware: 0x0e, buttons: 3/3
[ 8.740439] input: TPPS/2 IBM TrackPoint as /devices/platform/i8042/serio1/serio2/input/input18
functionally both versions (with/without bug) resulted in a functional trackpoint with 3 buttons.
Sebastian, with regards to your middle button question, as you can see in the output I've such hardware. (lenovo e570).
Dimitrios, wasn't it your intention that this hardware would be detected as NON IBM ? Apparently it
is IBM and still fails "Read Extended Button Status".
Kind regards,
Ulrik
On Sat, Jan 06, 2018 at 10:52:01PM -0800, Dmitry Torokhov wrote:
> Date: Sat, 6 Jan 2018 22:52:01 -0800
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> To: Aaron Ma <aaron.ma@canonical.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>, Sebastian Schmidt <yath@yath.de>,
> linux-input@vger.kernel.org
> Subject: Re: [PATCH] Revert "Input: trackpoint - add new trackpoint
> firmware ID"
> X-Mailing-List: linux-input@vger.kernel.org
>
> On Fri, Jan 05, 2018 at 08:23:13AM -0800, Dmitry Torokhov wrote:
> > Hi Aaron,
> >
> > On Fri, Jan 05, 2018 at 09:29:26PM +0800, Aaron Ma wrote:
> > > Hi Dmitry:
> > >
> > > Got the official info from Lenovo:
> > > Lenovo introduced new TrackPoint compatible sticks ( ELAN/Alps/NXP
> > > sticks) from 2016.
> > > These new devices only support the minimum commands described in the
> > > spec, which has been used in the current Windows driver.
> >
> > What is the exact list of the commands supported by each variant?
> >
> > >
> > > Legacy TrackPoint: 0101 – 0E01
> > > ALPS: 0102 – FF02
> > > ELAN:0103 – FF03
> > > NXP: 0104 – FF04
> > >
> > > 2.4.18 READ SECONDARY ID (x"E1")
> > > This command will read the secondary device ID of the pointing device (2
> > > bytes). The least significant byte is sent first. For the first byte,
> > > the legacy TrackPoint controller from IBM will always return x"01", the
> > > pointing stick from ALPS will always return x"02", the pointing stick
> > > from Elan will always return x"03”, and the pointing stick from NXP will
> > > always return 0x”04". And a second byte which denotes a specific set of
> > > functional specifications. Differing ROM versions are used to denote
> > > changes within a given functional set.
> >
> > Can you/Lenovo share the updated spec?
> >
> > >
> > > The new devices (include Legacy ID:01) will not support the sysfs like
> > > speed.
> > >
> > > So it is not right to revert the commit, it is about to add another 0x04
> > > ID in it.
> > >
> > > Old sysfs could be stayed for old legacy device ID:01 or removed.
> >
> > No, because there are devices that have trackpoints properly
> > implementing the protocol, before Lenovo started their "innovation".
> >
> > Do we have any way to distinguish between properly implemented
> > trackpoints and Lenovo "improved" ones? I played with gen3 Carbon, and
> > while it does not error out on "speed" attribute, unlike gen5, it still
> > has no visible effects. Additionally, the "press to select"
> > functionality seems to be disabled, and trying to enable it via sysfs
> > results in register content being reverted to the original "disabled"
> > setting in a second or two. Setting to swap X and Y axes does not work
> > either, not sure about other bits of that control register.
> > "sensitivity" does work though, again unlike my gen5.
> >
> > Thanks.
> >
> > --
> > Dmitry
>
> Guys, could you please try the patch below? It will not solve the
> "speed" issue on 0x01 devices, but hopefully we won't be exposing
> non-existing controls on others.
>
> Thanks!
>
> --
> Dmitry
>
>
> Input: trackpoint - only expose supported controls for Elan, ALPS and NXP
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> The newer trackpoints from ALPS, Elan and NXP implement a very limited
> subset of extended commands and controls that the original trackpoints
> implemented, so we should not be exposing not working controls in sysfs.
> The newer trackpoints also do not implement "Power On Reset" or "Read
> Extended Button Status", so we should not be using these commands during
> initialization.
>
> While we are at it, let's change "unsigned char" to u8 for byte data or
> bool for booleans and use better suited error codes instead of -1.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/mouse/trackpoint.c | 241 +++++++++++++++++++++++---------------
> drivers/input/mouse/trackpoint.h | 34 +++--
> 2 files changed, 168 insertions(+), 107 deletions(-)
>
> diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
> index 0871010f18d5..0ca80f465359 100644
> --- a/drivers/input/mouse/trackpoint.c
> +++ b/drivers/input/mouse/trackpoint.c
> @@ -19,6 +19,13 @@
> #include "psmouse.h"
> #include "trackpoint.h"
>
> +static const char * const trackpoint_variants[] = {
> + [TP_VARIANT_IBM] = "IBM",
> + [TP_VARIANT_ALPS] = "ALPS",
> + [TP_VARIANT_ELAN] = "Elan",
> + [TP_VARIANT_NXP] = "NXP",
> +};
> +
> /*
> * Power-on Reset: Resets all trackpoint parameters, including RAM values,
> * to defaults.
> @@ -26,7 +33,7 @@
> */
> static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
> {
> - unsigned char results[2];
> + u8 results[2];
> int tries = 0;
>
> /* Issue POR command, and repeat up to once if 0xFC00 received */
> @@ -38,7 +45,7 @@ static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
>
> /* Check for success response -- 0xAA00 */
> if (results[0] != 0xAA || results[1] != 0x00)
> - return -1;
> + return -ENODEV;
>
> return 0;
> }
> @@ -46,8 +53,7 @@ static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
> /*
> * Device IO: read, write and toggle bit
> */
> -static int trackpoint_read(struct ps2dev *ps2dev,
> - unsigned char loc, unsigned char *results)
> +static int trackpoint_read(struct ps2dev *ps2dev, u8 loc, u8 *results)
> {
> if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
> ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 1, loc))) {
> @@ -57,8 +63,7 @@ static int trackpoint_read(struct ps2dev *ps2dev,
> return 0;
> }
>
> -static int trackpoint_write(struct ps2dev *ps2dev,
> - unsigned char loc, unsigned char val)
> +static int trackpoint_write(struct ps2dev *ps2dev, u8 loc, u8 val)
> {
> if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
> ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_WRITE_MEM)) ||
> @@ -70,8 +75,7 @@ static int trackpoint_write(struct ps2dev *ps2dev,
> return 0;
> }
>
> -static int trackpoint_toggle_bit(struct ps2dev *ps2dev,
> - unsigned char loc, unsigned char mask)
> +static int trackpoint_toggle_bit(struct ps2dev *ps2dev, u8 loc, u8 mask)
> {
> /* Bad things will happen if the loc param isn't in this range */
> if (loc < 0x20 || loc >= 0x2F)
> @@ -87,11 +91,11 @@ static int trackpoint_toggle_bit(struct ps2dev *ps2dev,
> return 0;
> }
>
> -static int trackpoint_update_bit(struct ps2dev *ps2dev, unsigned char loc,
> - unsigned char mask, unsigned char value)
> +static int trackpoint_update_bit(struct ps2dev *ps2dev,
> + u8 loc, u8 mask, u8 value)
> {
> int retval = 0;
> - unsigned char data;
> + u8 data;
>
> trackpoint_read(ps2dev, loc, &data);
> if (((data & mask) == mask) != !!value)
> @@ -105,17 +109,18 @@ static int trackpoint_update_bit(struct ps2dev *ps2dev, unsigned char loc,
> */
> struct trackpoint_attr_data {
> size_t field_offset;
> - unsigned char command;
> - unsigned char mask;
> - unsigned char inverted;
> - unsigned char power_on_default;
> + u8 command;
> + u8 mask;
> + bool inverted;
> + u8 power_on_default;
> };
>
> -static ssize_t trackpoint_show_int_attr(struct psmouse *psmouse, void *data, char *buf)
> +static ssize_t trackpoint_show_int_attr(struct psmouse *psmouse,
> + void *data, char *buf)
> {
> struct trackpoint_data *tp = psmouse->private;
> struct trackpoint_attr_data *attr = data;
> - unsigned char value = *(unsigned char *)((char *)tp + attr->field_offset);
> + u8 value = *(u8 *)((void *)tp + attr->field_offset);
>
> if (attr->inverted)
> value = !value;
> @@ -128,8 +133,8 @@ static ssize_t trackpoint_set_int_attr(struct psmouse *psmouse, void *data,
> {
> struct trackpoint_data *tp = psmouse->private;
> struct trackpoint_attr_data *attr = data;
> - unsigned char *field = (unsigned char *)((char *)tp + attr->field_offset);
> - unsigned char value;
> + u8 *field = (void *)tp + attr->field_offset;
> + u8 value;
> int err;
>
> err = kstrtou8(buf, 10, &value);
> @@ -157,17 +162,14 @@ static ssize_t trackpoint_set_bit_attr(struct psmouse *psmouse, void *data,
> {
> struct trackpoint_data *tp = psmouse->private;
> struct trackpoint_attr_data *attr = data;
> - unsigned char *field = (unsigned char *)((char *)tp + attr->field_offset);
> - unsigned int value;
> + bool *field = (void *)tp + attr->field_offset;
> + bool value;
> int err;
>
> - err = kstrtouint(buf, 10, &value);
> + err = kstrtobool(buf, &value);
> if (err)
> return err;
>
> - if (value > 1)
> - return -EINVAL;
> -
> if (attr->inverted)
> value = !value;
>
> @@ -193,30 +195,6 @@ PSMOUSE_DEFINE_ATTR(_name, S_IWUSR | S_IRUGO, \
> &trackpoint_attr_##_name, \
> trackpoint_show_int_attr, trackpoint_set_bit_attr)
>
> -#define TRACKPOINT_UPDATE_BIT(_psmouse, _tp, _name) \
> -do { \
> - struct trackpoint_attr_data *_attr = &trackpoint_attr_##_name; \
> - \
> - trackpoint_update_bit(&_psmouse->ps2dev, \
> - _attr->command, _attr->mask, _tp->_name); \
> -} while (0)
> -
> -#define TRACKPOINT_UPDATE(_power_on, _psmouse, _tp, _name) \
> -do { \
> - if (!_power_on || \
> - _tp->_name != trackpoint_attr_##_name.power_on_default) { \
> - if (!trackpoint_attr_##_name.mask) \
> - trackpoint_write(&_psmouse->ps2dev, \
> - trackpoint_attr_##_name.command, \
> - _tp->_name); \
> - else \
> - TRACKPOINT_UPDATE_BIT(_psmouse, _tp, _name); \
> - } \
> -} while (0)
> -
> -#define TRACKPOINT_SET_POWER_ON_DEFAULT(_tp, _name) \
> - (_tp->_name = trackpoint_attr_##_name.power_on_default)
> -
> TRACKPOINT_INT_ATTR(sensitivity, TP_SENS, TP_DEF_SENS);
> TRACKPOINT_INT_ATTR(speed, TP_SPEED, TP_DEF_SPEED);
> TRACKPOINT_INT_ATTR(inertia, TP_INERTIA, TP_DEF_INERTIA);
> @@ -229,13 +207,33 @@ TRACKPOINT_INT_ATTR(ztime, TP_Z_TIME, TP_DEF_Z_TIME);
> TRACKPOINT_INT_ATTR(jenks, TP_JENKS_CURV, TP_DEF_JENKS_CURV);
> TRACKPOINT_INT_ATTR(drift_time, TP_DRIFT_TIME, TP_DEF_DRIFT_TIME);
>
> -TRACKPOINT_BIT_ATTR(press_to_select, TP_TOGGLE_PTSON, TP_MASK_PTSON, 0,
> +TRACKPOINT_BIT_ATTR(press_to_select, TP_TOGGLE_PTSON, TP_MASK_PTSON, false,
> TP_DEF_PTSON);
> -TRACKPOINT_BIT_ATTR(skipback, TP_TOGGLE_SKIPBACK, TP_MASK_SKIPBACK, 0,
> +TRACKPOINT_BIT_ATTR(skipback, TP_TOGGLE_SKIPBACK, TP_MASK_SKIPBACK, false,
> TP_DEF_SKIPBACK);
> -TRACKPOINT_BIT_ATTR(ext_dev, TP_TOGGLE_EXT_DEV, TP_MASK_EXT_DEV, 1,
> +TRACKPOINT_BIT_ATTR(ext_dev, TP_TOGGLE_EXT_DEV, TP_MASK_EXT_DEV, true,
> TP_DEF_EXT_DEV);
>
> +static bool trackpoint_is_attr_available(struct psmouse *psmouse,
> + struct attribute *attr)
> +{
> + struct trackpoint_data *tp = psmouse->private;
> +
> + return tp->variant_id == TP_VARIANT_IBM ||
> + attr == &psmouse_attr_sensitivity.dattr.attr ||
> + attr == &psmouse_attr_press_to_select.dattr.attr;
> +}
> +
> +static umode_t trackpoint_is_attr_visible(struct kobject *kobj,
> + struct attribute *attr, int n)
> +{
> + struct device *dev = container_of(kobj, struct device, kobj);
> + struct serio *serio = to_serio_port(dev);
> + struct psmouse *psmouse = serio_get_drvdata(serio);
> +
> + return trackpoint_is_attr_available(psmouse, attr) ? attr->mode : 0;
> +}
> +
> static struct attribute *trackpoint_attrs[] = {
> &psmouse_attr_sensitivity.dattr.attr,
> &psmouse_attr_speed.dattr.attr,
> @@ -255,24 +253,56 @@ static struct attribute *trackpoint_attrs[] = {
> };
>
> static struct attribute_group trackpoint_attr_group = {
> - .attrs = trackpoint_attrs,
> + .is_visible = trackpoint_is_attr_visible,
> + .attrs = trackpoint_attrs,
> };
>
> -static int trackpoint_start_protocol(struct psmouse *psmouse, unsigned char *firmware_id)
> -{
> - unsigned char param[2] = { 0 };
> +#define TRACKPOINT_UPDATE(_power_on, _psmouse, _tp, _name) \
> +do { \
> + struct trackpoint_attr_data *_attr = &trackpoint_attr_##_name; \
> + \
> + if ((!_power_on || _tp->_name != _attr->power_on_default) && \
> + trackpoint_is_attr_available(_psmouse, \
> + &psmouse_attr_##_name.dattr.attr)) { \
> + if (!_attr->mask) \
> + trackpoint_write(&_psmouse->ps2dev, \
> + _attr->command, _tp->_name); \
> + else \
> + trackpoint_update_bit(&_psmouse->ps2dev, \
> + _attr->command, _attr->mask, \
> + _tp->_name); \
> + } \
> +} while (0)
>
> - if (ps2_command(&psmouse->ps2dev, param, MAKE_PS2_CMD(0, 2, TP_READ_ID)))
> - return -1;
> +#define TRACKPOINT_SET_POWER_ON_DEFAULT(_tp, _name) \
> +do { \
> + _tp->_name = trackpoint_attr_##_name.power_on_default; \
> +} while (0)
>
> - /* add new TP ID. */
> - if (!(param[0] & TP_MAGIC_IDENT))
> - return -1;
> +static int trackpoint_start_protocol(struct psmouse *psmouse,
> + u8 *variant_id, u8 *firmware_id)
> +{
> + u8 param[2] = { 0 };
> + int error;
>
> - if (firmware_id)
> - *firmware_id = param[1];
> + error = ps2_command(&psmouse->ps2dev,
> + param, MAKE_PS2_CMD(0, 2, TP_READ_ID));
> + if (error)
> + return error;
> +
> + switch (param[0]) {
> + case TP_VARIANT_IBM:
> + case TP_VARIANT_ALPS:
> + case TP_VARIANT_ELAN:
> + case TP_VARIANT_NXP:
> + if (variant_id)
> + *variant_id = param[0];
> + if (firmware_id)
> + *firmware_id = param[1];
> + break;
> + }
>
> - return 0;
> + return -ENODEV;
always report error .. not good.
> }
>
> /*
> @@ -285,7 +315,7 @@ static int trackpoint_sync(struct psmouse *psmouse, bool in_power_on_state)
> {
> struct trackpoint_data *tp = psmouse->private;
>
> - if (!in_power_on_state) {
> + if (!in_power_on_state && tp->variant_id == TP_VARIANT_IBM) {
> /*
> * Disable features that may make device unusable
> * with this driver.
> @@ -347,7 +377,8 @@ static void trackpoint_defaults(struct trackpoint_data *tp)
>
> static void trackpoint_disconnect(struct psmouse *psmouse)
> {
> - sysfs_remove_group(&psmouse->ps2dev.serio->dev.kobj, &trackpoint_attr_group);
> + device_remove_group(&psmouse->ps2dev.serio->dev,
> + &trackpoint_attr_group);
>
> kfree(psmouse->private);
> psmouse->private = NULL;
> @@ -355,14 +386,20 @@ static void trackpoint_disconnect(struct psmouse *psmouse)
>
> static int trackpoint_reconnect(struct psmouse *psmouse)
> {
> - int reset_fail;
> + struct trackpoint_data *tp = psmouse->private;
> + int error;
> + bool was_reset;
>
> - if (trackpoint_start_protocol(psmouse, NULL))
> - return -1;
> + error = trackpoint_start_protocol(psmouse, NULL, NULL);
> + if (error)
> + return error;
>
> - reset_fail = trackpoint_power_on_reset(&psmouse->ps2dev);
> - if (trackpoint_sync(psmouse, !reset_fail))
> - return -1;
> + was_reset = tp->variant_id == TP_VARIANT_IBM &&
> + trackpoint_power_on_reset(&psmouse->ps2dev) == 0;
> +
> + error = trackpoint_sync(psmouse, was_reset);
> + if (error)
> + return error;
>
> return 0;
> }
> @@ -370,46 +407,62 @@ static int trackpoint_reconnect(struct psmouse *psmouse)
> int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
> {
> struct ps2dev *ps2dev = &psmouse->ps2dev;
> - unsigned char firmware_id;
> - unsigned char button_info;
> + struct trackpoint_data *tp;
> + u8 variant_id;
> + u8 firmware_id;
> + u8 button_info;
> int error;
>
> - if (trackpoint_start_protocol(psmouse, &firmware_id))
> - return -1;
> + error = trackpoint_start_protocol(psmouse, &variant_id, &firmware_id);
> + if (error)
> + return error;
>
> if (!set_properties)
> return 0;
>
> - if (trackpoint_read(ps2dev, TP_EXT_BTN, &button_info)) {
> - psmouse_warn(psmouse, "failed to get extended button data, assuming 3 buttons\n");
> - button_info = 0x33;
> - }
> -
> - psmouse->private = kzalloc(sizeof(struct trackpoint_data), GFP_KERNEL);
> - if (!psmouse->private)
> + tp = kzalloc(sizeof(*tp), GFP_KERNEL);
> + if (!tp)
> return -ENOMEM;
>
> - psmouse->vendor = "IBM";
> + trackpoint_defaults(tp);
> + tp->variant_id = variant_id;
> + tp->firmware_id = firmware_id;
> +
> + psmouse->private = tp;
> +
> + psmouse->vendor = trackpoint_variants[variant_id];
> psmouse->name = "TrackPoint";
>
> psmouse->reconnect = trackpoint_reconnect;
> psmouse->disconnect = trackpoint_disconnect;
>
> + if (variant_id != TP_VARIANT_IBM) {
> + /* Newer variants do not support extended button query. */
> + button_info = 0x33;
> + } else {
> + error = trackpoint_read(ps2dev, TP_EXT_BTN, &button_info);
> + if (error) {
> + psmouse_warn(psmouse,
> + "failed to get extended button data, assuming 3 buttons\n");
> + button_info = 0x33;
> + }
> + }
> +
> if ((button_info & 0x0f) >= 3)
> - __set_bit(BTN_MIDDLE, psmouse->dev->keybit);
> + input_set_capability(psmouse->dev, EV_KEY, BTN_MIDDLE);
>
> __set_bit(INPUT_PROP_POINTER, psmouse->dev->propbit);
> __set_bit(INPUT_PROP_POINTING_STICK, psmouse->dev->propbit);
>
> - trackpoint_defaults(psmouse->private);
> -
> - error = trackpoint_power_on_reset(ps2dev);
> -
> - /* Write defaults to TP only if reset fails. */
> - if (error)
> + if (variant_id != TP_VARIANT_IBM ||
> + trackpoint_power_on_reset(ps2dev) != 0) {
> + /*
> + * Write defaults to TP if we did not reset the trackpoint.
> + */
> trackpoint_sync(psmouse, false);
> + }
>
> - error = sysfs_create_group(&ps2dev->serio->dev.kobj, &trackpoint_attr_group);
> + error = device_add_group(&ps2dev->serio->dev, &trackpoint_attr_group);
> if (error) {
> psmouse_err(psmouse,
> "failed to create sysfs attributes, error: %d\n",
> @@ -420,8 +473,8 @@ int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
> }
>
> psmouse_info(psmouse,
> - "IBM TrackPoint firmware: 0x%02x, buttons: %d/%d\n",
> - firmware_id,
> + "%s TrackPoint firmware: 0x%02x, buttons: %d/%d\n",
> + psmouse->vendor, firmware_id,
> (button_info & 0xf0) >> 4, button_info & 0x0f);
>
> return 0;
> diff --git a/drivers/input/mouse/trackpoint.h b/drivers/input/mouse/trackpoint.h
> index 88055755f82e..10a039148234 100644
> --- a/drivers/input/mouse/trackpoint.h
> +++ b/drivers/input/mouse/trackpoint.h
> @@ -21,10 +21,16 @@
> #define TP_COMMAND 0xE2 /* Commands start with this */
>
> #define TP_READ_ID 0xE1 /* Sent for device identification */
> -#define TP_MAGIC_IDENT 0x03 /* Sent after a TP_READ_ID followed */
> - /* by the firmware ID */
> - /* Firmware ID includes 0x1, 0x2, 0x3 */
>
> +/*
> + * Valid first byte responses to the "Read Secondary ID" (0xE1) command.
> + * 0x01 was the original IBM trackpoint, others implement very limited
> + * subset of trackpoint features.
> + */
> +#define TP_VARIANT_IBM 0x01
> +#define TP_VARIANT_ALPS 0x02
> +#define TP_VARIANT_ELAN 0x03
> +#define TP_VARIANT_NXP 0x04
>
> /*
> * Commands
> @@ -136,18 +142,20 @@
>
> #define MAKE_PS2_CMD(params, results, cmd) ((params<<12) | (results<<8) | (cmd))
>
> -struct trackpoint_data
> -{
> - unsigned char sensitivity, speed, inertia, reach;
> - unsigned char draghys, mindrag;
> - unsigned char thresh, upthresh;
> - unsigned char ztime, jenks;
> - unsigned char drift_time;
> +struct trackpoint_data {
> + u8 variant_id;
> + u8 firmware_id;
> +
> + u8 sensitivity, speed, inertia, reach;
> + u8 draghys, mindrag;
> + u8 thresh, upthresh;
> + u8 ztime, jenks;
> + u8 drift_time;
>
> /* toggles */
> - unsigned char press_to_select;
> - unsigned char skipback;
> - unsigned char ext_dev;
> + bool press_to_select;
> + bool skipback;
> + bool ext_dev;
> };
>
> #ifdef CONFIG_MOUSE_PS2_TRACKPOINT
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] Revert "Input: trackpoint - add new trackpoint firmware ID"
From: ulrik.debie-os @ 2018-01-14 20:57 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Aaron Ma, Greg KH, Sebastian Schmidt, linux-input
In-Reply-To: <20180114203958.GA3686@beacon.debie>
Hi,
On Sun, Jan 14, 2018 at 09:39:58PM +0100, ulrik.debie-os@e2big.org wrote:
>
> Hi Dmitry, Sebastian,
> Nack for the patch..
> As Sebastian pointed out, the trackpoint detection will always report an error. (see below)
>
> I builded your original patch and it resulted in detection as generic mouse for the trackpoint:
>
> [ 0.838143] serio: i8042 KBD port at 0x60,0x64 irq 1
> [ 0.838146] serio: i8042 AUX port at 0x60,0x64 irq 12
> [ 0.838267] mousedev: PS/2 mouse device common for all mice
> [ 9.321140] psmouse serio1: synaptics: queried max coordinates: x [..5676], y [..4758]
> [ 9.367095] psmouse serio1: synaptics: queried min coordinates: x [1266..], y [1096..]
> [ 9.367103] psmouse serio1: synaptics: The touchpad can support a better bus than the too old PS/2 protocol. Make sure MOUSE_PS2_SYNAPTICS_SMBUS and RMI4_SMB are enabled to get a better touchpad experience.
> [ 9.455953] psmouse serio1: synaptics: Touchpad model: 1, fw: 8.2, id: 0x1e2b1, caps: 0xf007a3/0x943300/0x12e800/0x410000, board id: 3157, fw id: 2405942
> [ 9.455970] psmouse serio1: synaptics: serio: Synaptics pass-through port at isa0060/serio1/input0
> [ 9.514117] input: SynPS/2 Synaptics TouchPad as /devices/platform/i8042/serio1/input/input7
> [ 11.458443] input: PS/2 Generic Mouse as /devices/platform/i8042/serio1/serio2/input/input18
>
> When I fixed your patch, it resulted in: (timing difference is because a dvd was inserted before)
> [ 0.843661] serio: i8042 KBD port at 0x60,0x64 irq 1
> [ 0.843664] serio: i8042 AUX port at 0x60,0x64 irq 12
> [ 0.843793] mousedev: PS/2 mouse device common for all mice
> [ 3.822411] psmouse serio1: synaptics: queried max coordinates: x [..5676], y [..4758]
> [ 3.868196] psmouse serio1: synaptics: queried min coordinates: x [1266..], y [1096..]
> [ 3.868203] psmouse serio1: synaptics: The touchpad can support a better bus than the too old PS/2 protocol. Make sure MOUSE_PS2_SYNAPTICS_SMBUS and RMI4_SMB are enabled to get a better touchpad experience.
> [ 3.961024] psmouse serio1: synaptics: Touchpad model: 1, fw: 8.2, id: 0x1e2b1, caps: 0xf007a3/0x943300/0x12e800/0x410000, board id: 3157, fw id: 2405942
> [ 3.961038] psmouse serio1: synaptics: serio: Synaptics pass-through port at isa0060/serio1/input0
> [ 4.017290] input: SynPS/2 Synaptics TouchPad as /devices/platform/i8042/serio1/input/input7
> [ 4.407930] psmouse serio2: trackpoint: failed to get extended button data, assuming 3 buttons
> [ 8.460022] psmouse serio2: trackpoint: IBM TrackPoint firmware: 0x0e, buttons: 3/3
> [ 8.740439] input: TPPS/2 IBM TrackPoint as /devices/platform/i8042/serio1/serio2/input/input18
>
> functionally both versions (with/without bug) resulted in a functional trackpoint with 3 buttons.
>
> Sebastian, with regards to your middle button question, as you can see in the output I've such hardware. (lenovo e570).
Sorry Sebastian, I was thinking Aaron had commited
"Input: trackpoint - assume 3 buttons when buttons detection fails"
commit 293b915fd9bebf33cdc906516fb28d54649a25ac but actually Oscar
Campos did. I just realised when I reread his mails that you were
actually not pointing to the patch where Aaron suggested an
amendment to this 3 button commit but actually to his commit
that resulted in regression for you.
>
> Dimitrios, wasn't it your intention that this hardware would be detected as NON IBM ? Apparently it
> is IBM and still fails "Read Extended Button Status".
>
> Kind regards,
> Ulrik
Kind regards,
Ulrik
>
> On Sat, Jan 06, 2018 at 10:52:01PM -0800, Dmitry Torokhov wrote:
> > Date: Sat, 6 Jan 2018 22:52:01 -0800
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > To: Aaron Ma <aaron.ma@canonical.com>
> > Cc: Greg KH <gregkh@linuxfoundation.org>, Sebastian Schmidt <yath@yath.de>,
> > linux-input@vger.kernel.org
> > Subject: Re: [PATCH] Revert "Input: trackpoint - add new trackpoint
> > firmware ID"
> > X-Mailing-List: linux-input@vger.kernel.org
> >
> > On Fri, Jan 05, 2018 at 08:23:13AM -0800, Dmitry Torokhov wrote:
> > > Hi Aaron,
> > >
> > > On Fri, Jan 05, 2018 at 09:29:26PM +0800, Aaron Ma wrote:
> > > > Hi Dmitry:
> > > >
> > > > Got the official info from Lenovo:
> > > > Lenovo introduced new TrackPoint compatible sticks ( ELAN/Alps/NXP
> > > > sticks) from 2016.
> > > > These new devices only support the minimum commands described in the
> > > > spec, which has been used in the current Windows driver.
> > >
> > > What is the exact list of the commands supported by each variant?
> > >
> > > >
> > > > Legacy TrackPoint: 0101 – 0E01
> > > > ALPS: 0102 – FF02
> > > > ELAN:0103 – FF03
> > > > NXP: 0104 – FF04
> > > >
> > > > 2.4.18 READ SECONDARY ID (x"E1")
> > > > This command will read the secondary device ID of the pointing device (2
> > > > bytes). The least significant byte is sent first. For the first byte,
> > > > the legacy TrackPoint controller from IBM will always return x"01", the
> > > > pointing stick from ALPS will always return x"02", the pointing stick
> > > > from Elan will always return x"03”, and the pointing stick from NXP will
> > > > always return 0x”04". And a second byte which denotes a specific set of
> > > > functional specifications. Differing ROM versions are used to denote
> > > > changes within a given functional set.
> > >
> > > Can you/Lenovo share the updated spec?
> > >
> > > >
> > > > The new devices (include Legacy ID:01) will not support the sysfs like
> > > > speed.
> > > >
> > > > So it is not right to revert the commit, it is about to add another 0x04
> > > > ID in it.
> > > >
> > > > Old sysfs could be stayed for old legacy device ID:01 or removed.
> > >
> > > No, because there are devices that have trackpoints properly
> > > implementing the protocol, before Lenovo started their "innovation".
> > >
> > > Do we have any way to distinguish between properly implemented
> > > trackpoints and Lenovo "improved" ones? I played with gen3 Carbon, and
> > > while it does not error out on "speed" attribute, unlike gen5, it still
> > > has no visible effects. Additionally, the "press to select"
> > > functionality seems to be disabled, and trying to enable it via sysfs
> > > results in register content being reverted to the original "disabled"
> > > setting in a second or two. Setting to swap X and Y axes does not work
> > > either, not sure about other bits of that control register.
> > > "sensitivity" does work though, again unlike my gen5.
> > >
> > > Thanks.
> > >
> > > --
> > > Dmitry
> >
> > Guys, could you please try the patch below? It will not solve the
> > "speed" issue on 0x01 devices, but hopefully we won't be exposing
> > non-existing controls on others.
> >
> > Thanks!
> >
> > --
> > Dmitry
> >
> >
> > Input: trackpoint - only expose supported controls for Elan, ALPS and NXP
> >
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >
> > The newer trackpoints from ALPS, Elan and NXP implement a very limited
> > subset of extended commands and controls that the original trackpoints
> > implemented, so we should not be exposing not working controls in sysfs.
> > The newer trackpoints also do not implement "Power On Reset" or "Read
> > Extended Button Status", so we should not be using these commands during
> > initialization.
> >
> > While we are at it, let's change "unsigned char" to u8 for byte data or
> > bool for booleans and use better suited error codes instead of -1.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > drivers/input/mouse/trackpoint.c | 241 +++++++++++++++++++++++---------------
> > drivers/input/mouse/trackpoint.h | 34 +++--
> > 2 files changed, 168 insertions(+), 107 deletions(-)
> >
> > diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
> > index 0871010f18d5..0ca80f465359 100644
> > --- a/drivers/input/mouse/trackpoint.c
> > +++ b/drivers/input/mouse/trackpoint.c
> > @@ -19,6 +19,13 @@
> > #include "psmouse.h"
> > #include "trackpoint.h"
> >
> > +static const char * const trackpoint_variants[] = {
> > + [TP_VARIANT_IBM] = "IBM",
> > + [TP_VARIANT_ALPS] = "ALPS",
> > + [TP_VARIANT_ELAN] = "Elan",
> > + [TP_VARIANT_NXP] = "NXP",
> > +};
> > +
> > /*
> > * Power-on Reset: Resets all trackpoint parameters, including RAM values,
> > * to defaults.
> > @@ -26,7 +33,7 @@
> > */
> > static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
> > {
> > - unsigned char results[2];
> > + u8 results[2];
> > int tries = 0;
> >
> > /* Issue POR command, and repeat up to once if 0xFC00 received */
> > @@ -38,7 +45,7 @@ static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
> >
> > /* Check for success response -- 0xAA00 */
> > if (results[0] != 0xAA || results[1] != 0x00)
> > - return -1;
> > + return -ENODEV;
> >
> > return 0;
> > }
> > @@ -46,8 +53,7 @@ static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
> > /*
> > * Device IO: read, write and toggle bit
> > */
> > -static int trackpoint_read(struct ps2dev *ps2dev,
> > - unsigned char loc, unsigned char *results)
> > +static int trackpoint_read(struct ps2dev *ps2dev, u8 loc, u8 *results)
> > {
> > if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
> > ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 1, loc))) {
> > @@ -57,8 +63,7 @@ static int trackpoint_read(struct ps2dev *ps2dev,
> > return 0;
> > }
> >
> > -static int trackpoint_write(struct ps2dev *ps2dev,
> > - unsigned char loc, unsigned char val)
> > +static int trackpoint_write(struct ps2dev *ps2dev, u8 loc, u8 val)
> > {
> > if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
> > ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_WRITE_MEM)) ||
> > @@ -70,8 +75,7 @@ static int trackpoint_write(struct ps2dev *ps2dev,
> > return 0;
> > }
> >
> > -static int trackpoint_toggle_bit(struct ps2dev *ps2dev,
> > - unsigned char loc, unsigned char mask)
> > +static int trackpoint_toggle_bit(struct ps2dev *ps2dev, u8 loc, u8 mask)
> > {
> > /* Bad things will happen if the loc param isn't in this range */
> > if (loc < 0x20 || loc >= 0x2F)
> > @@ -87,11 +91,11 @@ static int trackpoint_toggle_bit(struct ps2dev *ps2dev,
> > return 0;
> > }
> >
> > -static int trackpoint_update_bit(struct ps2dev *ps2dev, unsigned char loc,
> > - unsigned char mask, unsigned char value)
> > +static int trackpoint_update_bit(struct ps2dev *ps2dev,
> > + u8 loc, u8 mask, u8 value)
> > {
> > int retval = 0;
> > - unsigned char data;
> > + u8 data;
> >
> > trackpoint_read(ps2dev, loc, &data);
> > if (((data & mask) == mask) != !!value)
> > @@ -105,17 +109,18 @@ static int trackpoint_update_bit(struct ps2dev *ps2dev, unsigned char loc,
> > */
> > struct trackpoint_attr_data {
> > size_t field_offset;
> > - unsigned char command;
> > - unsigned char mask;
> > - unsigned char inverted;
> > - unsigned char power_on_default;
> > + u8 command;
> > + u8 mask;
> > + bool inverted;
> > + u8 power_on_default;
> > };
> >
> > -static ssize_t trackpoint_show_int_attr(struct psmouse *psmouse, void *data, char *buf)
> > +static ssize_t trackpoint_show_int_attr(struct psmouse *psmouse,
> > + void *data, char *buf)
> > {
> > struct trackpoint_data *tp = psmouse->private;
> > struct trackpoint_attr_data *attr = data;
> > - unsigned char value = *(unsigned char *)((char *)tp + attr->field_offset);
> > + u8 value = *(u8 *)((void *)tp + attr->field_offset);
> >
> > if (attr->inverted)
> > value = !value;
> > @@ -128,8 +133,8 @@ static ssize_t trackpoint_set_int_attr(struct psmouse *psmouse, void *data,
> > {
> > struct trackpoint_data *tp = psmouse->private;
> > struct trackpoint_attr_data *attr = data;
> > - unsigned char *field = (unsigned char *)((char *)tp + attr->field_offset);
> > - unsigned char value;
> > + u8 *field = (void *)tp + attr->field_offset;
> > + u8 value;
> > int err;
> >
> > err = kstrtou8(buf, 10, &value);
> > @@ -157,17 +162,14 @@ static ssize_t trackpoint_set_bit_attr(struct psmouse *psmouse, void *data,
> > {
> > struct trackpoint_data *tp = psmouse->private;
> > struct trackpoint_attr_data *attr = data;
> > - unsigned char *field = (unsigned char *)((char *)tp + attr->field_offset);
> > - unsigned int value;
> > + bool *field = (void *)tp + attr->field_offset;
> > + bool value;
> > int err;
> >
> > - err = kstrtouint(buf, 10, &value);
> > + err = kstrtobool(buf, &value);
> > if (err)
> > return err;
> >
> > - if (value > 1)
> > - return -EINVAL;
> > -
> > if (attr->inverted)
> > value = !value;
> >
> > @@ -193,30 +195,6 @@ PSMOUSE_DEFINE_ATTR(_name, S_IWUSR | S_IRUGO, \
> > &trackpoint_attr_##_name, \
> > trackpoint_show_int_attr, trackpoint_set_bit_attr)
> >
> > -#define TRACKPOINT_UPDATE_BIT(_psmouse, _tp, _name) \
> > -do { \
> > - struct trackpoint_attr_data *_attr = &trackpoint_attr_##_name; \
> > - \
> > - trackpoint_update_bit(&_psmouse->ps2dev, \
> > - _attr->command, _attr->mask, _tp->_name); \
> > -} while (0)
> > -
> > -#define TRACKPOINT_UPDATE(_power_on, _psmouse, _tp, _name) \
> > -do { \
> > - if (!_power_on || \
> > - _tp->_name != trackpoint_attr_##_name.power_on_default) { \
> > - if (!trackpoint_attr_##_name.mask) \
> > - trackpoint_write(&_psmouse->ps2dev, \
> > - trackpoint_attr_##_name.command, \
> > - _tp->_name); \
> > - else \
> > - TRACKPOINT_UPDATE_BIT(_psmouse, _tp, _name); \
> > - } \
> > -} while (0)
> > -
> > -#define TRACKPOINT_SET_POWER_ON_DEFAULT(_tp, _name) \
> > - (_tp->_name = trackpoint_attr_##_name.power_on_default)
> > -
> > TRACKPOINT_INT_ATTR(sensitivity, TP_SENS, TP_DEF_SENS);
> > TRACKPOINT_INT_ATTR(speed, TP_SPEED, TP_DEF_SPEED);
> > TRACKPOINT_INT_ATTR(inertia, TP_INERTIA, TP_DEF_INERTIA);
> > @@ -229,13 +207,33 @@ TRACKPOINT_INT_ATTR(ztime, TP_Z_TIME, TP_DEF_Z_TIME);
> > TRACKPOINT_INT_ATTR(jenks, TP_JENKS_CURV, TP_DEF_JENKS_CURV);
> > TRACKPOINT_INT_ATTR(drift_time, TP_DRIFT_TIME, TP_DEF_DRIFT_TIME);
> >
> > -TRACKPOINT_BIT_ATTR(press_to_select, TP_TOGGLE_PTSON, TP_MASK_PTSON, 0,
> > +TRACKPOINT_BIT_ATTR(press_to_select, TP_TOGGLE_PTSON, TP_MASK_PTSON, false,
> > TP_DEF_PTSON);
> > -TRACKPOINT_BIT_ATTR(skipback, TP_TOGGLE_SKIPBACK, TP_MASK_SKIPBACK, 0,
> > +TRACKPOINT_BIT_ATTR(skipback, TP_TOGGLE_SKIPBACK, TP_MASK_SKIPBACK, false,
> > TP_DEF_SKIPBACK);
> > -TRACKPOINT_BIT_ATTR(ext_dev, TP_TOGGLE_EXT_DEV, TP_MASK_EXT_DEV, 1,
> > +TRACKPOINT_BIT_ATTR(ext_dev, TP_TOGGLE_EXT_DEV, TP_MASK_EXT_DEV, true,
> > TP_DEF_EXT_DEV);
> >
> > +static bool trackpoint_is_attr_available(struct psmouse *psmouse,
> > + struct attribute *attr)
> > +{
> > + struct trackpoint_data *tp = psmouse->private;
> > +
> > + return tp->variant_id == TP_VARIANT_IBM ||
> > + attr == &psmouse_attr_sensitivity.dattr.attr ||
> > + attr == &psmouse_attr_press_to_select.dattr.attr;
> > +}
> > +
> > +static umode_t trackpoint_is_attr_visible(struct kobject *kobj,
> > + struct attribute *attr, int n)
> > +{
> > + struct device *dev = container_of(kobj, struct device, kobj);
> > + struct serio *serio = to_serio_port(dev);
> > + struct psmouse *psmouse = serio_get_drvdata(serio);
> > +
> > + return trackpoint_is_attr_available(psmouse, attr) ? attr->mode : 0;
> > +}
> > +
> > static struct attribute *trackpoint_attrs[] = {
> > &psmouse_attr_sensitivity.dattr.attr,
> > &psmouse_attr_speed.dattr.attr,
> > @@ -255,24 +253,56 @@ static struct attribute *trackpoint_attrs[] = {
> > };
> >
> > static struct attribute_group trackpoint_attr_group = {
> > - .attrs = trackpoint_attrs,
> > + .is_visible = trackpoint_is_attr_visible,
> > + .attrs = trackpoint_attrs,
> > };
> >
> > -static int trackpoint_start_protocol(struct psmouse *psmouse, unsigned char *firmware_id)
> > -{
> > - unsigned char param[2] = { 0 };
> > +#define TRACKPOINT_UPDATE(_power_on, _psmouse, _tp, _name) \
> > +do { \
> > + struct trackpoint_attr_data *_attr = &trackpoint_attr_##_name; \
> > + \
> > + if ((!_power_on || _tp->_name != _attr->power_on_default) && \
> > + trackpoint_is_attr_available(_psmouse, \
> > + &psmouse_attr_##_name.dattr.attr)) { \
> > + if (!_attr->mask) \
> > + trackpoint_write(&_psmouse->ps2dev, \
> > + _attr->command, _tp->_name); \
> > + else \
> > + trackpoint_update_bit(&_psmouse->ps2dev, \
> > + _attr->command, _attr->mask, \
> > + _tp->_name); \
> > + } \
> > +} while (0)
> >
> > - if (ps2_command(&psmouse->ps2dev, param, MAKE_PS2_CMD(0, 2, TP_READ_ID)))
> > - return -1;
> > +#define TRACKPOINT_SET_POWER_ON_DEFAULT(_tp, _name) \
> > +do { \
> > + _tp->_name = trackpoint_attr_##_name.power_on_default; \
> > +} while (0)
> >
> > - /* add new TP ID. */
> > - if (!(param[0] & TP_MAGIC_IDENT))
> > - return -1;
> > +static int trackpoint_start_protocol(struct psmouse *psmouse,
> > + u8 *variant_id, u8 *firmware_id)
> > +{
> > + u8 param[2] = { 0 };
> > + int error;
> >
> > - if (firmware_id)
> > - *firmware_id = param[1];
> > + error = ps2_command(&psmouse->ps2dev,
> > + param, MAKE_PS2_CMD(0, 2, TP_READ_ID));
> > + if (error)
> > + return error;
> > +
> > + switch (param[0]) {
> > + case TP_VARIANT_IBM:
> > + case TP_VARIANT_ALPS:
> > + case TP_VARIANT_ELAN:
> > + case TP_VARIANT_NXP:
> > + if (variant_id)
> > + *variant_id = param[0];
> > + if (firmware_id)
> > + *firmware_id = param[1];
> > + break;
> > + }
> >
> > - return 0;
> > + return -ENODEV;
>
> always report error .. not good.
>
> > }
> >
> > /*
> > @@ -285,7 +315,7 @@ static int trackpoint_sync(struct psmouse *psmouse, bool in_power_on_state)
> > {
> > struct trackpoint_data *tp = psmouse->private;
> >
> > - if (!in_power_on_state) {
> > + if (!in_power_on_state && tp->variant_id == TP_VARIANT_IBM) {
> > /*
> > * Disable features that may make device unusable
> > * with this driver.
> > @@ -347,7 +377,8 @@ static void trackpoint_defaults(struct trackpoint_data *tp)
> >
> > static void trackpoint_disconnect(struct psmouse *psmouse)
> > {
> > - sysfs_remove_group(&psmouse->ps2dev.serio->dev.kobj, &trackpoint_attr_group);
> > + device_remove_group(&psmouse->ps2dev.serio->dev,
> > + &trackpoint_attr_group);
> >
> > kfree(psmouse->private);
> > psmouse->private = NULL;
> > @@ -355,14 +386,20 @@ static void trackpoint_disconnect(struct psmouse *psmouse)
> >
> > static int trackpoint_reconnect(struct psmouse *psmouse)
> > {
> > - int reset_fail;
> > + struct trackpoint_data *tp = psmouse->private;
> > + int error;
> > + bool was_reset;
> >
> > - if (trackpoint_start_protocol(psmouse, NULL))
> > - return -1;
> > + error = trackpoint_start_protocol(psmouse, NULL, NULL);
> > + if (error)
> > + return error;
> >
> > - reset_fail = trackpoint_power_on_reset(&psmouse->ps2dev);
> > - if (trackpoint_sync(psmouse, !reset_fail))
> > - return -1;
> > + was_reset = tp->variant_id == TP_VARIANT_IBM &&
> > + trackpoint_power_on_reset(&psmouse->ps2dev) == 0;
> > +
> > + error = trackpoint_sync(psmouse, was_reset);
> > + if (error)
> > + return error;
> >
> > return 0;
> > }
> > @@ -370,46 +407,62 @@ static int trackpoint_reconnect(struct psmouse *psmouse)
> > int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
> > {
> > struct ps2dev *ps2dev = &psmouse->ps2dev;
> > - unsigned char firmware_id;
> > - unsigned char button_info;
> > + struct trackpoint_data *tp;
> > + u8 variant_id;
> > + u8 firmware_id;
> > + u8 button_info;
> > int error;
> >
> > - if (trackpoint_start_protocol(psmouse, &firmware_id))
> > - return -1;
> > + error = trackpoint_start_protocol(psmouse, &variant_id, &firmware_id);
> > + if (error)
> > + return error;
> >
> > if (!set_properties)
> > return 0;
> >
> > - if (trackpoint_read(ps2dev, TP_EXT_BTN, &button_info)) {
> > - psmouse_warn(psmouse, "failed to get extended button data, assuming 3 buttons\n");
> > - button_info = 0x33;
> > - }
> > -
> > - psmouse->private = kzalloc(sizeof(struct trackpoint_data), GFP_KERNEL);
> > - if (!psmouse->private)
> > + tp = kzalloc(sizeof(*tp), GFP_KERNEL);
> > + if (!tp)
> > return -ENOMEM;
> >
> > - psmouse->vendor = "IBM";
> > + trackpoint_defaults(tp);
> > + tp->variant_id = variant_id;
> > + tp->firmware_id = firmware_id;
> > +
> > + psmouse->private = tp;
> > +
> > + psmouse->vendor = trackpoint_variants[variant_id];
> > psmouse->name = "TrackPoint";
> >
> > psmouse->reconnect = trackpoint_reconnect;
> > psmouse->disconnect = trackpoint_disconnect;
> >
> > + if (variant_id != TP_VARIANT_IBM) {
> > + /* Newer variants do not support extended button query. */
> > + button_info = 0x33;
> > + } else {
> > + error = trackpoint_read(ps2dev, TP_EXT_BTN, &button_info);
> > + if (error) {
> > + psmouse_warn(psmouse,
> > + "failed to get extended button data, assuming 3 buttons\n");
> > + button_info = 0x33;
> > + }
> > + }
> > +
> > if ((button_info & 0x0f) >= 3)
> > - __set_bit(BTN_MIDDLE, psmouse->dev->keybit);
> > + input_set_capability(psmouse->dev, EV_KEY, BTN_MIDDLE);
> >
> > __set_bit(INPUT_PROP_POINTER, psmouse->dev->propbit);
> > __set_bit(INPUT_PROP_POINTING_STICK, psmouse->dev->propbit);
> >
> > - trackpoint_defaults(psmouse->private);
> > -
> > - error = trackpoint_power_on_reset(ps2dev);
> > -
> > - /* Write defaults to TP only if reset fails. */
> > - if (error)
> > + if (variant_id != TP_VARIANT_IBM ||
> > + trackpoint_power_on_reset(ps2dev) != 0) {
> > + /*
> > + * Write defaults to TP if we did not reset the trackpoint.
> > + */
> > trackpoint_sync(psmouse, false);
> > + }
> >
> > - error = sysfs_create_group(&ps2dev->serio->dev.kobj, &trackpoint_attr_group);
> > + error = device_add_group(&ps2dev->serio->dev, &trackpoint_attr_group);
> > if (error) {
> > psmouse_err(psmouse,
> > "failed to create sysfs attributes, error: %d\n",
> > @@ -420,8 +473,8 @@ int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
> > }
> >
> > psmouse_info(psmouse,
> > - "IBM TrackPoint firmware: 0x%02x, buttons: %d/%d\n",
> > - firmware_id,
> > + "%s TrackPoint firmware: 0x%02x, buttons: %d/%d\n",
> > + psmouse->vendor, firmware_id,
> > (button_info & 0xf0) >> 4, button_info & 0x0f);
> >
> > return 0;
> > diff --git a/drivers/input/mouse/trackpoint.h b/drivers/input/mouse/trackpoint.h
> > index 88055755f82e..10a039148234 100644
> > --- a/drivers/input/mouse/trackpoint.h
> > +++ b/drivers/input/mouse/trackpoint.h
> > @@ -21,10 +21,16 @@
> > #define TP_COMMAND 0xE2 /* Commands start with this */
> >
> > #define TP_READ_ID 0xE1 /* Sent for device identification */
> > -#define TP_MAGIC_IDENT 0x03 /* Sent after a TP_READ_ID followed */
> > - /* by the firmware ID */
> > - /* Firmware ID includes 0x1, 0x2, 0x3 */
> >
> > +/*
> > + * Valid first byte responses to the "Read Secondary ID" (0xE1) command.
> > + * 0x01 was the original IBM trackpoint, others implement very limited
> > + * subset of trackpoint features.
> > + */
> > +#define TP_VARIANT_IBM 0x01
> > +#define TP_VARIANT_ALPS 0x02
> > +#define TP_VARIANT_ELAN 0x03
> > +#define TP_VARIANT_NXP 0x04
> >
> > /*
> > * Commands
> > @@ -136,18 +142,20 @@
> >
> > #define MAKE_PS2_CMD(params, results, cmd) ((params<<12) | (results<<8) | (cmd))
> >
> > -struct trackpoint_data
> > -{
> > - unsigned char sensitivity, speed, inertia, reach;
> > - unsigned char draghys, mindrag;
> > - unsigned char thresh, upthresh;
> > - unsigned char ztime, jenks;
> > - unsigned char drift_time;
> > +struct trackpoint_data {
> > + u8 variant_id;
> > + u8 firmware_id;
> > +
> > + u8 sensitivity, speed, inertia, reach;
> > + u8 draghys, mindrag;
> > + u8 thresh, upthresh;
> > + u8 ztime, jenks;
> > + u8 drift_time;
> >
> > /* toggles */
> > - unsigned char press_to_select;
> > - unsigned char skipback;
> > - unsigned char ext_dev;
> > + bool press_to_select;
> > + bool skipback;
> > + bool ext_dev;
> > };
> >
> > #ifdef CONFIG_MOUSE_PS2_TRACKPOINT
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-input" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 12/14] iio: adc: at91-sama5d2_adc: support for position and pressure channels
From: Jonathan Cameron @ 2018-01-14 10:47 UTC (permalink / raw)
To: Ludovic Desroches
Cc: Eugen Hristev, nicolas.ferre, alexandre.belloni, linux-iio,
linux-arm-kernel, devicetree, linux-kernel, linux-input,
dmitry.torokhov
In-Reply-To: <20180108141252.GC2425@rfolt0960.corp.atmel.com>
On Mon, 8 Jan 2018 15:12:52 +0100
Ludovic Desroches <ludovic.desroches@microchip.com> wrote:
> Hi Jonathan, Eugen,
>
> On Sat, Jan 06, 2018 at 03:05:37PM +0000, Jonathan Cameron wrote:
> > On Thu, 4 Jan 2018 17:17:54 +0200
> > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> >
> > > On 29.12.2017 19:02, Jonathan Cameron wrote:
> > > > On Fri, 22 Dec 2017 17:07:19 +0200
> > > > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> > > >
> > > >> The ADC IP supports position and pressure measurements for a touchpad
> > > >> connected on channels 0,1,2,3 for a 4-wire touchscreen with pressure
> > > >> measurement support.
> > > >> Using the inkern API, a driver can request a trigger and read the
> > > >> channel values from the ADC.
> > > >> The implementation provides a trigger named "touch" which can be
> > > >> connected to a consumer driver.
> > > >> Once a driver connects and attaches a pollfunc to this trigger, the
> > > >> configure trigger callback is called, and then the ADC driver will
> > > >> initialize pad measurement.
> > > >> First step is to enable touchscreen 4wire support and enable
> > > >> pen detect IRQ.
> > > >> Once a pen is detected, a periodic trigger is setup to trigger every
> > > >> 2 ms (e.g.) and sample the resistive touchscreen values. The trigger poll
> > > >> is called, and the consumer driver is then woke up, and it can read the
> > > >> respective channels for the values : X, and Y for position and pressure
> > > >> channel.
> > > >> Because only one trigger can be active in hardware in the same time,
> > > >> while touching the pad, the ADC will block any attempt to use the
> > > >> triggered buffer. Same, conversions using the software trigger are also
> > > >> impossible (since the periodic trigger is setup).
> > > >> If some driver wants to attach while the trigger is in use, it will
> > > >> also fail.
> > > >> Once the pen is not detected anymore, the trigger is free for use (hardware
> > > >> or software trigger, with or without DMA).
> > > >> Channels 0,1,2 and 3 are unavailable if a touchscreen is enabled.
> > > >>
> > > >> Some parts of this patch are based on initial original work by
> > > >> Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy
> > > >>
> > > > OK, so comments inline.
> > > >
> > > > What I'm missing currently though is an explanation of why the slightly
> > > > more standard arrangement of using a callback buffer doesn't work here.
> > > > The only addition I think you need to do that is to allow a consumer to
> > > > request a particular trigger. I also think some of the other provisions
> > > > could be handled using standard features and slightly reducing the flexibility.
> > > > I don't know for example if it's useful to allow other channels to be
> > > > read when touch is not in progress or not.
> > > >
> > > > So restrictions:
> > > >
> > > > 1. Touch screen channels can only be read when touch is enabled.
> > > > - use the available_scan_masks to control this. Or the callback that lets
> > > > you do the same dynamically.
> > > > 2. You need to push these channels to your consumer driver.
> > > > - register a callback buffer rather than jumping through the hoops to
> > > > insert your own pollfunc. That will call a function in your
> > > > consumer, providing the data from the 3 channels directly.
> > > > 3. You need to make sure it is using the right driver. For that you
> > > > will I think need a new interface.
> > > >
> > > > Various other comments inline. I may well be missing something as this is
> > > > a fair bit of complex code to read - if so then next version should have
> > > > a clear cover letter describing why this more standard approach can't be
> > > > used.
> > >
> > > Hello Jonathan and thanks for the review of my patch series,
> > >
> > > before starting and working over the required modifications and
> > > suggestions that you sent me, I want to be a little more explicit about
> > > the design of my implementation.
> > > Hope this will clarify some things, and maybe I can as well understand
> > > better what you have in mind to support this feature set.
> > >
> > > Why have I picked a pollfunction: We discussed a while back on the
> > > mailing list that you do not have an inkern mechanism to expose the
> > > triggers to other drivers, and that it may be a good idea to have it for
> > > such kind of actually multi function device, instead of having a MFD
> > > driver, an ADC driver, and an Input driver, all sharing the same
> > > register map, the same IRQ , etc, with some kind of synchronization to
> > > avoid stepping on each other for the hardware resource.
> >
> > No disagreement with that principle.
> >
> > > So I considered to expose the trigger by attaching and detaching
> > > pollfunctions to it. Which is the main thing what we use a trigger for.
> >
> > Hmm. It's definitely one approach. But we do already have other drivers
> > where the trigger is controlled by a consumer and to my mind that
> > is a cleaner approach as it doesn't short cut the equivalent of
> > doing it from userspace.
> >
> > drivers/iio/potentiostat/lmp91000.c does something similar though
> > for a rather different use. You need your consumer interface
> > to get the handle to the trigger in this case
> > (the lmp91000 is actually providing the trigger rather than
> > consuming it).
> >
> >
> > >
> > > So, what I had in mind, was to create a consumer driver that will
> > > request triggers from the IIO device just like other drivers request
> > > channels (part which is already done in IIO).
> > > In order to do this I had to somehow wake up the consumer driver when
> > > new data was available from the touchscreen. So, having the IRQ only in
> > > the ADC device, and then on Pen detect and No pen detect just start or
> > > stop the periodic trigger, which needs to be polled. The magic part is
> > > that the consumer driver has a poll function already attached to this
> > > trigger, so the poll function is just called every time we have new
> > > data. The poll function is attached as an irq handler, and then we can
> > > reuse all the read_raw data by using a scheduled work from the consumer
> > > driver, to read the channels.
> >
> > If you had done this via a callback buffer the only difference is that
> > the pollfunc would have been a standard one pulling the relevant channels
> > and passing them on down to the buffer interface which could then decide
> > what to do with them.
> >
> > > To do this, the ADC registers a special trigger named "touch trigger"
> > > which is never enabled by the ADC driver. Instead, when a pollfunc is
> > > attached to it, the attach function will also configure it with enabled
> > > state.
> >
> > Whilst it might not make sense to enable it in the touch screen driver
> > I'm not sure there is strictly any reason to prevent it being so used.
> >
> > > In the ADC, this means to start the touchscreen functionality. If
> > > the touch is requested, it will standby and wait for pen detect IRQ.
> > > Once we have pen detect, we can use a periodic trigger to sample the
> > > touch data, and poll the "touch" trigger. The consumer driver will wake
> > > up and schedule a work , that will use the standard read raw interface
> > > (inkern) that will read three virtual channels (position + pressure).
> > > They are not actual hardware channels, as the touch information is being
> > > received on channels 0,1,2,3, but reading these virtual channels will
> > > read from different registers inside the ADC IP ( x position, y
> > > position, pressure), do some computations on the data, and feed the
> > > consumer with the values , hiding the behind the scenes hardware
> > > specific calculations.
> >
> > I wouldn't worry about whether they are real channels or not. This
> > is really similar to a differential ADC (some of those do the differential
> > digitally). Light sensors often have a number of 'real' channels used
> > to derive (via hideous non linear calculations) the illuminance as
> > it's hard to build a light sensor with the same sensitivity as the human
> > eye. We have worse 'non real' channels as well such as activity channels
> > on some the accelerometers that report if it thinks you are walking /
> > running etc.
> >
> > > After trigger is polled , the ADC will resume normal functionality, and
> > > the consumer driver will continue to sleep.
> >
> > So this is where I'm unsure. Do you actually have a usecase where it
> > makes the sense to read from the ADC only when there is no touch? Any
> > system doing that has an obvious denial of service attack - touch the
> > screen.
> >
>
> You're right. We have an issue in this case due to the hardware. Using
> touchscreen has side effects on other channels. We can use only one
> trigger for all the channels. The situation would have been better with
> a trigger dedicated to the touchscreen.
>
> At the moment, we have not really stated about the exclusive use or not
> of the touchscreen. We suppose we can get some customers wanting to use
> both touchscreen and ADC. Eugen tried to deal with this case but, as you
> noticed, it can lead to DoS.
It's a restriction people aren't going to expect unfortunately...
>
> > > We need to have a periodic trigger to sample the data because the actual
> > > analog to digital conversion inside the IP block needs to be triggered.
> > > The touchscreen data measurements cannot happen in hardware without
> > > being triggered. If I try with a hrtimer to get a periodic IRQ to just
> > > read the data, it will never be ready. The datasheet states that the
> > > touchscreen measurements "will be attached to the conversion sequence".
> > > So the periodic trigger is forcing a conversion sequence. This could be
> > > done with a software trigger as well, but why the hassle to start it
> > > every 2 milliseconds (or other time interval), if we can do it by
> > > periodic trigger ?
> >
> > Ah, one reason here would be to allow separate consumers to use the
> > device. In that case you'd run with a periodic trigger all the time
> > and have two buffers attached, the buffer_cb that is feeding your
> > touchscreen and another buffer to deal with the other channels
> > (presumably the standard one an IIO device has when using buffered
> > interfaces).
>
> The issue is that we are sharing the periodic trigger so we have to use
> the same period for both usage.
Whilst a somewhat irritating restriction, it's probably not disastrous for
most ADC uses.
Jonathan
>
> Regards
>
> Ludovic
>
> >
> > The buffer demux would ensure the data from the right channels
> > ends up in the right place. It makes it look to the buffer
> > consumer like it is the only thing using / controlling the data
> > flow.
> >
> > > Once we get the No pen IRQ, we stop the periodic trigger and it can be
> > > used in another purpose (software or external as of now in the driver,
> > > in the future we can add PWM trigger and Timer trigger)
> >
> > This case isn't really useful though as any other use is denied
> > access when touch occurs.
> >
> > I'll summarise what I think would work for this below.
> >
> > >
> > > In short, the ADC in Sama5D2 also supports touchscreen, and in
> > > touchscreen mode , 4 of the channels are being used for this purpose.
> > > This however, doesn't stop the ADC to use the other channels . The
> > > hardware has 12 total single channels and they can be paired to have 6
> > > more differential channels. The only thing that is blocked is the
> > > trigger, but only if the pen is touching (when we start the periodic
> > > trigger to sample the touchscreen). If the pen is not touching, an
> > > external trigger or software trigger can be used without any issues (so
> > > why limit the functionality, if this is available from hardware ?).
> > > Because of the reason I discussed above (touchscreen sequence must be
> > > triggered), we cannot use another trigger in the same time.
> > >
> > >
> > > I see your idea with the callback buffer and it's worth exploring.
> > > Mainly this series was to actually show you what I had in mind about
> > > supporting the resistive touchscreen, and to give you some actually
> > > working code/patch, so we can discuss based on real implementation, not
> > > just suppositions.
> >
> > That side of things is fine.
> >
> > >
> > > You are right in many of the other comments that you said, and I will
> > > come up with a v2 to this series. For now, I need to know if this is a
> > > good or right direction in which I am going, or I should try to change
> > > all the mechanism to callback buffer ? Or maybe I am totally in a bad
> > > direction ?
> > > The requirements are that the consumer driver needs to be somehow woke
> > > up for every new touch data available, and report to the input
> > > subsystem. As it was done before, the at91 old driver, just creates and
> > > registers an input device by itself, and then reports the position and
> > > touches. I was thinking that with this trigger consumer implementation,
> > > things can be better in terms of subsystem separation and support.
> > >
> > > Thanks again and let me know of your thoughts,
> > >
> > > Eugen
> >
> > So a couple of things come to mind on how I'd structure this.
> > So what we have (very briefly)
> >
> > No touch screen case:
> > * Generic ADC using all sorts of different triggers
> >
> > Touch screen only case:
> > * Interrupt to indicate pen on / off
> > * A need to do a periodic trigger of the device but only
> > useful when touch is in progress.
> >
> > Touch screen and other users:
> > * Interrupt to indicate pen on / off
> > * Periodic trigger needed for touchscreen when touch in progress.
> > * Do not have denial of service on other channels.
> >
> > First two cases are easy enough by having a magic trigger, third
> > case is harder.
> > If we have the touchscreen then I would drop support for direct access to
> > to ADC channels whilst it's in use (so no sysfs - or emulate it if you
> > really want it by stashing results from scans done when touch is in
> > progress).
> >
> > Have your touch screen channels just as normal additional channels,
> > but only via the buffered interface (no _RAW attribute).
> > If someone sets up to read them via buffered interface with
> > a different trigger I think they'll get values - whether they
> > are right is dependent (if I understand correctly) on whether
> > there is a touch in progress. So no harm done and it'll make
> > the logic simpler.
> >
> > The moment touch is opened and acquires the IIO channels
> > fix the trigger (may need new interface) to the periodic one
> > that you were enabling and disabling on touch.
> > Things get dicey if there is an existing user so you may
> > have to do it on driver probe rather than open of the input
> > device if we effectively want touch to have the highest
> > priority use of the ADC.
> >
> > If other channels are enabled for buffered mode then note
> > this in the driver and have the periodic trigger on all the
> > time (to ensure they keep getting read) This will pass
> > garbage to your touch screen driver, but it'll remove it due
> > to the pressure value being too low so no harm there.
> >
> > Normal path will work for non touch channels (and in theory
> > the touch ones if they are turned on) via IIO buffer
> > interface. It'll be restricted in form due to the needs of
> > the touch driver, but better than nothing and should cover
> > most usecases.
> >
> > Now the interrupt on / off on touch bit becomes an optimization
> > in the case of only the buffer_cb being attached.
> >
> > I think that fits cleanly in the current IIO framework and
> > looks more similar to our existing provider consumer approaches.
> >
> > Still needs the hooks to get hold of the trigger though so
> > as to be able to tell the ADC which one to use. So rather
> > than being a trigger consumer interface, it's more of a trigger
> > configuration interface.. Exact term doesn't matter though.
> >
> > Jonathan
> >
> > >
> > >
> > >
> > > [...]
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
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