* Re: [PATCH v2 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
From: Nick Dyer @ 2014-08-08 14:07 UTC (permalink / raw)
To: Javier Martinez Canillas, Dmitry Torokhov
Cc: Stephen Warren, Yufeng Shen, Benson Leung, Doug Anderson,
Olof Johansson, Tomasz Figa, linux-input-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1407397492-17475-1-git-send-email-javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
On 07/08/14 08:44, Javier Martinez Canillas wrote:
> The Atmel maXTouch driver assumed that the IRQ type flags will
> always be passed using platform data but this is not true when
> booting using Device Trees. In these setups the interrupt type
> was ignored by the driver when requesting an IRQ.
>
> This means that it will fail if a machine specified other type
> than IRQ_TYPE_NONE. The right approach is to get the IRQ flags
> that was parsed by OF from the "interrupt" Device Tree propery.
>
> Signed-off-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
I'm happy for this to go in if Dmitry will accept it. It does seem to be a
quirk of some platforms that it is necessary, but it's only one line.
Thanks for spending so much time debugging this.
Signed-off-by: Nick Dyer <nick.dyer-8YpZQSjhsQH10XsdtD+oqA@public.gmane.org>
> ---
>
> This patch was first sent as a part of a two part series along
> with [PATCH 2/2] Input: atmel_mxt_ts - Add keycodes array example.
>
> But there are no dependencies between these two patches so there
> is no need to resend that one with no changes for v2.
>
> Changes since v1:
> - Assign flags to pdata->irqflags in mxt_parse_dt() instead of probe().
> Suggested by Tomasz Figa.
>
> drivers/input/touchscreen/atmel_mxt_ts.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 03b8571..5c8cbd3 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -22,6 +22,7 @@
> #include <linux/i2c.h>
> #include <linux/i2c/atmel_mxt_ts.h>
> #include <linux/input/mt.h>
> +#include <linux/irq.h>
> #include <linux/interrupt.h>
> #include <linux/of.h>
> #include <linux/slab.h>
> @@ -2093,6 +2094,8 @@ static struct mxt_platform_data *mxt_parse_dt(struct i2c_client *client)
> if (!pdata)
> return ERR_PTR(-ENOMEM);
>
> + pdata->irqflags = irq_get_trigger_type(client->irq);
> +
> if (of_find_property(client->dev.of_node, "linux,gpio-keymap",
> &proplen)) {
> pdata->t19_num_keys = proplen / sizeof(u32);
>
--
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
* Backslash repeat bug
From: Fredrik Hallenberg @ 2014-08-08 14:11 UTC (permalink / raw)
To: linux-input, nveber
Hi,
On my Corsair K70 keyboard, when pressing some key and then the
backslash key (KEY_BACKSLASH) in rapid succession the backslash
character will be repeated three times. This bug has been mentioned on
this list before (by Norbert Veber on the 24th of februrary) but there
was no responses, also Norbert consider it a problem with Corsair
keyboards but as I describe below I feel it is more of a general
problem in the HID input code.
This bug is discussed on the Corsair forums, but it seems the exact
same thing also happens with Gigabyte Osmium and QPAD MK85, possibly
it is a problem on all keyboards supporting n-key rollover.
I believe the root cause of this is that the keyboard reports all keys
including both HID keys 0x31 and 0x32 which are both mapped to
KEY_BACKSLASH in hid-input.c. I am using a nordic keyboard meaning
KEY_BACKSLASH is labeled "apostrophe" and that HID key 0x32 is
reported. A US keyboard has a backslash key which should report 0x31.
I have added some traces before the input_event call at the end of
hidinput_report_event. When pressing A key and then apostrophe,
something like this is seen (omitting all inactive keys):
VALUE 1 CODE 30 HID 0x4
VALUE 0 CODE 43 HID 0x31
VALUE 0 CODE 43 HID 0x32
Output: a
VALUE 1 CODE 30 HID 0x4
VALUE 0 CODE 43 HID 0x31
VALUE 1 CODE 43 HID 0x32
Output: '
VALUE 0 CODE 30 HID 0x4
VALUE 0 CODE 43 HID 0x31
VALUE 1 CODE 43 HID 0x32
Output: '
VALUE 0 CODE 30 HID 0x4
VALUE 0 CODE 43 HID 0x31
VALUE 1 CODE 43 HID 0x32
Output: '
VALUE 0 CODE 30 HID 0x4
VALUE 0 CODE 43 HID 0x31
VALUE 0 CODE 43 HID 0x32
So the fact that 0x31 is inactive will cause bad release events on key
43 to be sent.
I have made a simple patch that tracks which of the two variants is
active and ignores the other one. It works but I don't have much
knowledge on HID input so before posting it would like to know if
someone has input on how to do a cleaner fix. I am happy to help with
testing or implementation if I am able.
Regards,
Fredrik
^ permalink raw reply
* [Toshiba Satellite E55-A5114] ETPS/2 Elantech Touchpad not working
From: hugo @ 2014-08-08 14:26 UTC (permalink / raw)
To: linux-input
Hi,
I have reported a new bug related to Elantech v4 not working at all.
Please see below.
https://bugzilla.kernel.org/show_bug.cgi?id=81941
I have sent a mail also to Dmitry. Should I talk to someone else?
Thanks!
Hugo
^ permalink raw reply
* [RESEND PATCH] Support Xbox One controllers in xpad driver.
From: Ted Mielczarek @ 2014-08-08 14:46 UTC (permalink / raw)
To: linux-input
Cc: dmitry.torokhov, paul.gortmaker, hor27, petr, magissia,
linux-kernel, Ted Mielczarek
Xbox One controllers require an initialization message to start sending data,
so xpad_init_output becomes a required function. The Xbox One controller does
not have LEDs like the Xbox 360 controller, so that functionality is not
implemented. The format of messages controlling rumble is currently
undocumented, so rumble support is not yet implemented.
The only thing I'm not certain about in this patch is the check against bInterfaceNumber in xpad_probe. The Xbox One controller advertises three interfaces with the same interface class, subclass and protocol, and I couldn't see any way to match just the first one using usb_device_id.
Signed-off-by: Ted Mielczarek <ted@mielczarek.org>
---
drivers/input/joystick/xpad.c | 163 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 149 insertions(+), 14 deletions(-)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 603fe0d..800e2f4 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -95,7 +95,8 @@
#define XTYPE_XBOX 0
#define XTYPE_XBOX360 1
#define XTYPE_XBOX360W 2
-#define XTYPE_UNKNOWN 3
+#define XTYPE_XBOXONE 3
+#define XTYPE_UNKNOWN 4
static bool dpad_to_buttons;
module_param(dpad_to_buttons, bool, S_IRUGO);
@@ -121,6 +122,7 @@ static const struct xpad_device {
{ 0x045e, 0x0287, "Microsoft Xbox Controller S", 0, XTYPE_XBOX },
{ 0x045e, 0x0289, "Microsoft X-Box pad v2 (US)", 0, XTYPE_XBOX },
{ 0x045e, 0x028e, "Microsoft X-Box 360 pad", 0, XTYPE_XBOX360 },
+ { 0x045e, 0x02d1, "Microsoft X-Box One pad", 0, XTYPE_XBOXONE },
{ 0x045e, 0x0291, "Xbox 360 Wireless Receiver (XBOX)", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX360W },
{ 0x045e, 0x0719, "Xbox 360 Wireless Receiver", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX360W },
{ 0x044f, 0x0f07, "Thrustmaster, Inc. Controller", 0, XTYPE_XBOX },
@@ -245,9 +247,20 @@ static const signed short xpad_abs_triggers[] = {
{ XPAD_XBOX360_VENDOR_PROTOCOL(vend,1) }, \
{ XPAD_XBOX360_VENDOR_PROTOCOL(vend,129) }
+/* The Xbox One controller uses subclass 71 and protocol 208. */
+#define XPAD_XBOXONE_VENDOR_PROTOCOL(vend,pr) \
+ .match_flags = USB_DEVICE_ID_MATCH_VENDOR | USB_DEVICE_ID_MATCH_INT_INFO, \
+ .idVendor = (vend), \
+ .bInterfaceClass = USB_CLASS_VENDOR_SPEC, \
+ .bInterfaceSubClass = 71, \
+ .bInterfaceProtocol = (pr)
+#define XPAD_XBOXONE_VENDOR(vend) \
+ { XPAD_XBOXONE_VENDOR_PROTOCOL(vend,208) }
+
static struct usb_device_id xpad_table[] = {
{ USB_INTERFACE_INFO('X', 'B', 0) }, /* X-Box USB-IF not approved class */
XPAD_XBOX360_VENDOR(0x045e), /* Microsoft X-Box 360 controllers */
+ XPAD_XBOXONE_VENDOR(0x045e), /* Microsoft X-Box One controllers */
XPAD_XBOX360_VENDOR(0x046d), /* Logitech X-Box 360 style controllers */
XPAD_XBOX360_VENDOR(0x0738), /* Mad Catz X-Box 360 controllers */
{ USB_DEVICE(0x0738, 0x4540) }, /* Mad Catz Beat Pad */
@@ -470,6 +483,101 @@ static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned cha
xpad360_process_packet(xpad, cmd, &data[4]);
}
+/*
+ * xpadone_process_buttons
+ *
+ * Process a button update packet from an Xbox one controller.
+ */
+static void xpadone_process_buttons(struct usb_xpad *xpad,
+ struct input_dev *dev,
+ unsigned char *data)
+{
+ /* menu/view buttons */
+ input_report_key(dev, BTN_START, data[4] & 0x04);
+ input_report_key(dev, BTN_SELECT, data[4] & 0x08);
+
+ /* buttons A,B,X,Y */
+ input_report_key(dev, BTN_A, data[4] & 0x10);
+ input_report_key(dev, BTN_B, data[4] & 0x20);
+ input_report_key(dev, BTN_X, data[4] & 0x40);
+ input_report_key(dev, BTN_Y, data[4] & 0x80);
+
+ /* digital pad */
+ if (xpad->mapping & MAP_DPAD_TO_BUTTONS) {
+ /* dpad as buttons (left, right, up, down) */
+ input_report_key(dev, BTN_TRIGGER_HAPPY1, data[5] & 0x04);
+ input_report_key(dev, BTN_TRIGGER_HAPPY2, data[5] & 0x08);
+ input_report_key(dev, BTN_TRIGGER_HAPPY3, data[5] & 0x01);
+ input_report_key(dev, BTN_TRIGGER_HAPPY4, data[5] & 0x02);
+ } else {
+ input_report_abs(dev, ABS_HAT0X,
+ !!(data[5] & 0x08) - !!(data[5] & 0x04));
+ input_report_abs(dev, ABS_HAT0Y,
+ !!(data[5] & 0x02) - !!(data[5] & 0x01));
+ }
+
+ /* TL/TR */
+ input_report_key(dev, BTN_TL, data[5] & 0x10);
+ input_report_key(dev, BTN_TR, data[5] & 0x20);
+
+ /* stick press left/right */
+ input_report_key(dev, BTN_THUMBL, data[5] & 0x40);
+ input_report_key(dev, BTN_THUMBR, data[5] & 0x80);
+
+ if (!(xpad->mapping & MAP_STICKS_TO_NULL)) {
+ /* left stick */
+ input_report_abs(dev, ABS_X,
+ (__s16) le16_to_cpup((__le16 *)(data + 10)));
+ input_report_abs(dev, ABS_Y,
+ ~(__s16) le16_to_cpup((__le16 *)(data + 12)));
+
+ /* right stick */
+ input_report_abs(dev, ABS_RX,
+ (__s16) le16_to_cpup((__le16 *)(data + 14)));
+ input_report_abs(dev, ABS_RY,
+ ~(__s16) le16_to_cpup((__le16 *)(data + 16)));
+ }
+
+ /* triggers left/right */
+ if (xpad->mapping & MAP_TRIGGERS_TO_BUTTONS) {
+ input_report_key(dev, BTN_TL2,
+ (__u16) le16_to_cpup((__le16 *)(data + 6)));
+ input_report_key(dev, BTN_TR2,
+ (__u16) le16_to_cpup((__le16 *)(data + 8)));
+ } else {
+ input_report_abs(dev, ABS_Z,
+ (__u16) le16_to_cpup((__le16 *)(data + 6)));
+ input_report_abs(dev, ABS_RZ,
+ (__u16) le16_to_cpup((__le16 *)(data + 8)));
+ }
+}
+
+/*
+ * xpadone_process_packet
+ *
+ * Completes a request by converting the data into events for the
+ * input subsystem. This version is for the Xbox One controller.
+ *
+ * The report format was gleaned from
+ * https://github.com/kylelemons/xbox/blob/master/xbox.go
+ */
+
+static void xpadone_process_packet(struct usb_xpad *xpad,
+ u16 cmd, unsigned char *data)
+{
+ struct input_dev *dev = xpad->dev;
+
+ if (data[0] == 0x20) {
+ xpadone_process_buttons(xpad, dev, data);
+ } else if (data[0] == 0x07) {
+ /* the xbox button has its own special report */
+ input_report_key(dev, BTN_MODE, data[4] & 0x01);
+ }
+
+ if (data[0] == 0x20 || data[0] == 0x07)
+ input_sync(dev);
+}
+
static void xpad_irq_in(struct urb *urb)
{
struct usb_xpad *xpad = urb->context;
@@ -502,6 +610,9 @@ static void xpad_irq_in(struct urb *urb)
case XTYPE_XBOX360W:
xpad360w_process_packet(xpad, 0, xpad->idata);
break;
+ case XTYPE_XBOXONE:
+ xpadone_process_packet(xpad, 0, xpad->idata);
+ break;
default:
xpad_process_packet(xpad, 0, xpad->idata);
}
@@ -535,7 +646,6 @@ static void xpad_bulk_out(struct urb *urb)
}
}
-#if defined(CONFIG_JOYSTICK_XPAD_FF) || defined(CONFIG_JOYSTICK_XPAD_LEDS)
static void xpad_irq_out(struct urb *urb)
{
struct usb_xpad *xpad = urb->context;
@@ -593,7 +703,11 @@ static int xpad_init_output(struct usb_interface *intf, struct usb_xpad *xpad)
goto fail2;
}
- ep_irq_out = &intf->cur_altsetting->endpoint[1].desc;
+ if (xpad->xtype == XTYPE_XBOXONE)
+ /* Xbox One controller has in/out endpoints swapped. */
+ ep_irq_out = &intf->cur_altsetting->endpoint[0].desc;
+ else
+ ep_irq_out = &intf->cur_altsetting->endpoint[1].desc;
usb_fill_int_urb(xpad->irq_out, xpad->udev,
usb_sndintpipe(xpad->udev, ep_irq_out->bEndpointAddress),
xpad->odata, XPAD_PKT_LEN,
@@ -621,11 +735,6 @@ static void xpad_deinit_output(struct usb_xpad *xpad)
xpad->odata, xpad->odata_dma);
}
}
-#else
-static int xpad_init_output(struct usb_interface *intf, struct usb_xpad *xpad) { return 0; }
-static void xpad_deinit_output(struct usb_xpad *xpad) {}
-static void xpad_stop_output(struct usb_xpad *xpad) {}
-#endif
#ifdef CONFIG_JOYSTICK_XPAD_FF
static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect)
@@ -692,7 +801,7 @@ static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect
static int xpad_init_ff(struct usb_xpad *xpad)
{
- if (xpad->xtype == XTYPE_UNKNOWN)
+ if (xpad->xtype == XTYPE_UNKNOWN || xpad->xtype == XTYPE_XBOXONE)
return 0;
input_set_capability(xpad->dev, EV_FF, FF_RUMBLE);
@@ -801,6 +910,14 @@ static int xpad_open(struct input_dev *dev)
if (usb_submit_urb(xpad->irq_in, GFP_KERNEL))
return -EIO;
+ if (xpad->xtype == XTYPE_XBOXONE) {
+ /* Xbox one controller needs to be initialized. */
+ xpad->odata[0] = 0x05;
+ xpad->odata[1] = 0x20;
+ xpad->irq_out->transfer_buffer_length = 2;
+ return usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+ }
+
return 0;
}
@@ -816,6 +933,7 @@ static void xpad_close(struct input_dev *dev)
static void xpad_set_up_abs(struct input_dev *input_dev, signed short abs)
{
+ struct usb_xpad *xpad = input_get_drvdata(input_dev);
set_bit(abs, input_dev->absbit);
switch (abs) {
@@ -827,7 +945,10 @@ static void xpad_set_up_abs(struct input_dev *input_dev, signed short abs)
break;
case ABS_Z:
case ABS_RZ: /* the triggers (if mapped to axes) */
- input_set_abs_params(input_dev, abs, 0, 255, 0, 0);
+ if (xpad->xtype == XTYPE_XBOXONE)
+ input_set_abs_params(input_dev, abs, 0, 1023, 0, 0);
+ else
+ input_set_abs_params(input_dev, abs, 0, 255, 0, 0);
break;
case ABS_HAT0X:
case ABS_HAT0Y: /* the d-pad (only if dpad is mapped to axes */
@@ -850,6 +971,15 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
break;
}
+ if (xpad_device[i].xtype == XTYPE_XBOXONE
+ && intf->cur_altsetting->desc.bInterfaceNumber != 0) {
+ /* The Xbox One controller lists three interfaces all with the
+ * same interface class, subclass and protocol. Differentiate by
+ * interface number.
+ */
+ return -ENODEV;
+ }
+
xpad = kzalloc(sizeof(struct usb_xpad), GFP_KERNEL);
input_dev = input_allocate_device();
if (!xpad || !input_dev) {
@@ -920,7 +1050,8 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
__set_bit(xpad_common_btn[i], input_dev->keybit);
/* set up model-specific ones */
- if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype == XTYPE_XBOX360W) {
+ if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype == XTYPE_XBOX360W
+ || xpad->xtype == XTYPE_XBOXONE) {
for (i = 0; xpad360_btn[i] >= 0; i++)
__set_bit(xpad360_btn[i], input_dev->keybit);
} else {
@@ -933,7 +1064,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
__set_bit(xpad_btn_pad[i], input_dev->keybit);
} else {
for (i = 0; xpad_abs_pad[i] >= 0; i++)
- xpad_set_up_abs(input_dev, xpad_abs_pad[i]);
+ xpad_set_up_abs(input_dev, xpad_abs_pad[i]);
}
if (xpad->mapping & MAP_TRIGGERS_TO_BUTTONS) {
@@ -956,7 +1087,11 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
if (error)
goto fail5;
- ep_irq_in = &intf->cur_altsetting->endpoint[0].desc;
+ if (xpad->xtype == XTYPE_XBOXONE)
+ /* Xbox One controller has in/out endpoints swapped. */
+ ep_irq_in = &intf->cur_altsetting->endpoint[1].desc;
+ else
+ ep_irq_in = &intf->cur_altsetting->endpoint[0].desc;
usb_fill_int_urb(xpad->irq_in, udev,
usb_rcvintpipe(udev, ep_irq_in->bEndpointAddress),
xpad->idata, XPAD_PKT_LEN, xpad_irq_in,
@@ -1010,7 +1145,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
/*
* Submit the int URB immediately rather than waiting for open
* because we get status messages from the device whether
- * or not any controllers are attached. In fact, it's
+ * or not any controllers are attached. In fact, it's
* exactly the message that a controller has arrived that
* we're waiting for.
*/
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 2/2] Input: atmel_mxt_ts - Add keycodes array example
From: Javier Martinez Canillas @ 2014-08-08 14:52 UTC (permalink / raw)
To: Nick Dyer, Dmitry Torokhov
Cc: Stephen Warren, Yufeng Shen, Benson Leung, Doug Anderson,
Olof Johansson, linux-input, devicetree, linux-samsung-soc,
linux-kernel
In-Reply-To: <53E37329.4020002@itdev.co.uk>
Hello Nick,
On 08/07/2014 02:38 PM, Nick Dyer wrote:
>>
>> Also, the current documentation says that the array limit
>> is four entries but the driver dynamically allocates the
>> keymap array and does not limit the array size.
>
> There is a physical limit to the number of GPIOs on the device. The number
> 4 is wrong, the protocol does allow for up to 8 GPIOs. But it is a hard limit.
>
Thanks a lot for the explanation, then I guess s/4/8 is enough.
>>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> ---
>> Documentation/devicetree/bindings/input/atmel,maxtouch.txt | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
>> index baef432..be50476 100644
>> --- a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
>> +++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
>> @@ -11,7 +11,7 @@ Required properties:
>>
>> Optional properties for main touchpad device:
>>
>> -- linux,gpio-keymap: An array of up to 4 entries indicating the Linux
>> +- linux,gpio-keymap: An array of entries indicating the Linux
>> keycode generated by each GPIO. Linux keycodes are defined in
>> <dt-bindings/input/input.h>.
>>
>> @@ -22,4 +22,10 @@ Example:
>> reg = <0x4b>;
>> interrupt-parent = <&gpio>;
>> interrupts = <TEGRA_GPIO(W, 3) IRQ_TYPE_LEVEL_LOW>;
>> + linux,gpio-keymap = < BTN_LEFT
>> + BTN_TOOL_FINGER
>> + BTN_TOOL_DOUBLETAP
>> + BTN_TOOL_TRIPLETAP
>> + BTN_TOOL_QUADTAP
>> + BTN_TOOL_QUINTTAP >;
>
> I'm afraid you have misunderstood the impact of this change to the way that
> the GPIOs coming in to the touch controller are mapped to key codes. Look
Unfortunately there are no boards in mainline using this "linux,gpio-keymap"
property so I tried to figure out what the expected values were by reading the
driver. So is more than possible that I got them wrong.
By passing all these keycodes the touchpad worked as expected for me and the
driver did the same than the Chrome OS driver that has these keycodes hardcoded
when is_tp is true.
> at the protocol guide for T19.
>
I don't have access to proper documentation and I wouldn't expect people to have
access to non-public docs in order to use a Device Tree binding.
That's why I wanted to document an example, so using this property could be
easier for others and they shouldn't have to look at the driver in order to
figure it out (and getting it wrong as you said :) )
So it would be great if you could provide an example on how this is supposed to
be used.
> The DOUBLE/TRIPLE/QUAD/QUINTTAP stuff is filled in for us by the input core
> when we use INPUT_MT_POINTER, anyway.
>
Thanks for the hint, I didn't know that this was the case but I just looked at
input_mt_init_slots() [0] and indeed those are not needed.
Best regards,
Javier
[0]: http://lxr.free-electrons.com/source/drivers/input/input-mt.c#L69
^ permalink raw reply
* [PATCH 2/2] Input: atmel_mxt_ts - split config update a bit
From: nick.dyer @ 2014-08-08 15:58 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Yufeng Shen, Daniel Kurtz, Henrik Rydberg, Joonyoung Shim,
Alan Bowens, linux-input, linux-kernel, Peter Meerwald,
Benson Leung, Olof Johansson, Sekhar Nori, Stephen Warren,
Nick Dyer
In-Reply-To: <1407513483-30050-1-git-send-email-nick.dyer@itdev.co.uk>
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Let's split config update code a bit so it is hopefully a bit easier to
read. Also, the firmware update callback should be the entity releasing
firmware blob, not lower layers.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 270 +++++++++++++++++--------------
1 file changed, 145 insertions(+), 125 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index a5f943d..bbaf3ff 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1064,6 +1064,133 @@ static u32 mxt_calculate_crc(u8 *base, off_t start_off, off_t end_off)
return crc;
}
+static int mxt_prepare_cfg_mem(struct mxt_data *data,
+ const struct firmware *cfg,
+ unsigned int data_pos,
+ unsigned int cfg_start_ofs,
+ u8 *config_mem,
+ size_t config_mem_size)
+{
+ struct device *dev = &data->client->dev;
+ struct mxt_object *object;
+ unsigned int type, instance, size, byte_offset;
+ int offset;
+ int ret;
+ int i;
+ u16 reg;
+ u8 val;
+
+ while (data_pos < cfg->size) {
+ /* Read type, instance, length */
+ ret = sscanf(cfg->data + data_pos, "%x %x %x%n",
+ &type, &instance, &size, &offset);
+ if (ret == 0) {
+ /* EOF */
+ break;
+ } else if (ret != 3) {
+ dev_err(dev, "Bad format: failed to parse object\n");
+ return -EINVAL;
+ }
+ data_pos += offset;
+
+ object = mxt_get_object(data, type);
+ if (!object) {
+ /* Skip object */
+ for (i = 0; i < size; i++) {
+ ret = sscanf(cfg->data + data_pos, "%hhx%n",
+ &val,
+ &offset);
+ data_pos += offset;
+ }
+ continue;
+ }
+
+ if (size > mxt_obj_size(object)) {
+ /*
+ * Either we are in fallback mode due to wrong
+ * config or config from a later fw version,
+ * or the file is corrupt or hand-edited.
+ */
+ dev_warn(dev, "Discarding %zu byte(s) in T%u\n",
+ size - mxt_obj_size(object), type);
+ } else if (mxt_obj_size(object) > size) {
+ /*
+ * If firmware is upgraded, new bytes may be added to
+ * end of objects. It is generally forward compatible
+ * to zero these bytes - previous behaviour will be
+ * retained. However this does invalidate the CRC and
+ * will force fallback mode until the configuration is
+ * updated. We warn here but do nothing else - the
+ * malloc has zeroed the entire configuration.
+ */
+ dev_warn(dev, "Zeroing %zu byte(s) in T%d\n",
+ mxt_obj_size(object) - size, type);
+ }
+
+ if (instance >= mxt_obj_instances(object)) {
+ dev_err(dev, "Object instances exceeded!\n");
+ return -EINVAL;
+ }
+
+ reg = object->start_address + mxt_obj_size(object) * instance;
+
+ for (i = 0; i < size; i++) {
+ ret = sscanf(cfg->data + data_pos, "%hhx%n",
+ &val,
+ &offset);
+ if (ret != 1) {
+ dev_err(dev, "Bad format in T%d\n", type);
+ return -EINVAL;
+ }
+ data_pos += offset;
+
+ if (i > mxt_obj_size(object))
+ continue;
+
+ byte_offset = reg + i - cfg_start_ofs;
+
+ if (byte_offset >= 0 &&
+ byte_offset <= config_mem_size) {
+ *(config_mem + byte_offset) = val;
+ } else {
+ dev_err(dev, "Bad object: reg:%d, T%d, ofs=%d\n",
+ reg, object->type, byte_offset);
+ return -EINVAL;
+ }
+ }
+ }
+
+ return 0;
+}
+
+static int mxt_upload_cfg_mem(struct mxt_data *data, unsigned int cfg_start,
+ u8 *config_mem, size_t config_mem_size)
+{
+ unsigned int byte_offset = 0;
+ int error;
+
+ /* Write configuration as blocks */
+ while (byte_offset < config_mem_size) {
+ unsigned int size = config_mem_size - byte_offset;
+
+ if (size > MXT_MAX_BLOCK_WRITE)
+ size = MXT_MAX_BLOCK_WRITE;
+
+ error = __mxt_write_reg(data->client,
+ cfg_start + byte_offset,
+ size, config_mem + byte_offset);
+ if (error) {
+ dev_err(&data->client->dev,
+ "Config write error, ret=%d\n", error);
+ return error;
+ }
+
+ byte_offset += size;
+ }
+
+ return 0;
+}
+
/*
* mxt_update_cfg - download configuration to chip
*
@@ -1087,26 +1214,20 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
{
struct device *dev = &data->client->dev;
struct mxt_info cfg_info;
- struct mxt_object *object;
int ret;
int offset;
int data_pos;
- int byte_offset;
int i;
int cfg_start_ofs;
u32 info_crc, config_crc, calculated_crc;
u8 *config_mem;
size_t config_mem_size;
- unsigned int type, instance, size;
- u8 val;
- u16 reg;
mxt_update_crc(data, MXT_COMMAND_REPORTALL, 1);
if (strncmp(cfg->data, MXT_CFG_MAGIC, strlen(MXT_CFG_MAGIC))) {
dev_err(dev, "Unrecognised config file\n");
- ret = -EINVAL;
- goto release;
+ return -EINVAL;
}
data_pos = strlen(MXT_CFG_MAGIC);
@@ -1118,8 +1239,7 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
&offset);
if (ret != 1) {
dev_err(dev, "Bad format\n");
- ret = -EINVAL;
- goto release;
+ return -EINVAL;
}
data_pos += offset;
@@ -1127,30 +1247,26 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
if (cfg_info.family_id != data->info.family_id) {
dev_err(dev, "Family ID mismatch!\n");
- ret = -EINVAL;
- goto release;
+ return -EINVAL;
}
if (cfg_info.variant_id != data->info.variant_id) {
dev_err(dev, "Variant ID mismatch!\n");
- ret = -EINVAL;
- goto release;
+ return -EINVAL;
}
/* Read CRCs */
ret = sscanf(cfg->data + data_pos, "%x%n", &info_crc, &offset);
if (ret != 1) {
dev_err(dev, "Bad format: failed to parse Info CRC\n");
- ret = -EINVAL;
- goto release;
+ return -EINVAL;
}
data_pos += offset;
ret = sscanf(cfg->data + data_pos, "%x%n", &config_crc, &offset);
if (ret != 1) {
dev_err(dev, "Bad format: failed to parse Config CRC\n");
- ret = -EINVAL;
- goto release;
+ return -EINVAL;
}
data_pos += offset;
@@ -1166,8 +1282,7 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
} else if (config_crc == data->config_crc) {
dev_dbg(dev, "Config CRC 0x%06X: OK\n",
data->config_crc);
- ret = 0;
- goto release;
+ return 0;
} else {
dev_info(dev, "Config CRC 0x%06X: does not match file 0x%06X\n",
data->config_crc, config_crc);
@@ -1186,93 +1301,13 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
config_mem = kzalloc(config_mem_size, GFP_KERNEL);
if (!config_mem) {
dev_err(dev, "Failed to allocate memory\n");
- ret = -ENOMEM;
- goto release;
+ return -ENOMEM;
}
- while (data_pos < cfg->size) {
- /* Read type, instance, length */
- ret = sscanf(cfg->data + data_pos, "%x %x %x%n",
- &type, &instance, &size, &offset);
- if (ret == 0) {
- /* EOF */
- break;
- } else if (ret != 3) {
- dev_err(dev, "Bad format: failed to parse object\n");
- ret = -EINVAL;
- goto release_mem;
- }
- data_pos += offset;
-
- object = mxt_get_object(data, type);
- if (!object) {
- /* Skip object */
- for (i = 0; i < size; i++) {
- ret = sscanf(cfg->data + data_pos, "%hhx%n",
- &val,
- &offset);
- data_pos += offset;
- }
- continue;
- }
-
- if (size > mxt_obj_size(object)) {
- /*
- * Either we are in fallback mode due to wrong
- * config or config from a later fw version,
- * or the file is corrupt or hand-edited.
- */
- dev_warn(dev, "Discarding %zu byte(s) in T%u\n",
- size - mxt_obj_size(object), type);
- } else if (mxt_obj_size(object) > size) {
- /*
- * If firmware is upgraded, new bytes may be added to
- * end of objects. It is generally forward compatible
- * to zero these bytes - previous behaviour will be
- * retained. However this does invalidate the CRC and
- * will force fallback mode until the configuration is
- * updated. We warn here but do nothing else - the
- * malloc has zeroed the entire configuration.
- */
- dev_warn(dev, "Zeroing %zu byte(s) in T%d\n",
- mxt_obj_size(object) - size, type);
- }
-
- if (instance >= mxt_obj_instances(object)) {
- dev_err(dev, "Object instances exceeded!\n");
- ret = -EINVAL;
- goto release_mem;
- }
-
- reg = object->start_address + mxt_obj_size(object) * instance;
-
- for (i = 0; i < size; i++) {
- ret = sscanf(cfg->data + data_pos, "%hhx%n",
- &val,
- &offset);
- if (ret != 1) {
- dev_err(dev, "Bad format in T%d\n", type);
- ret = -EINVAL;
- goto release_mem;
- }
- data_pos += offset;
-
- if (i > mxt_obj_size(object))
- continue;
-
- byte_offset = reg + i - cfg_start_ofs;
-
- if ((byte_offset >= 0)
- && (byte_offset <= config_mem_size)) {
- *(config_mem + byte_offset) = val;
- } else {
- dev_err(dev, "Bad object: reg:%d, T%d, ofs=%d\n",
- reg, object->type, byte_offset);
- ret = -EINVAL;
- goto release_mem;
- }
- }
- }
+ ret = mxt_prepare_cfg_mem(data, cfg, data_pos, cfg_start_ofs,
+ config_mem, config_mem_size);
+ if (ret)
+ goto release_mem;
/* Calculate crc of the received configs (not the raw config file) */
if (data->T7_address < cfg_start_ofs) {
@@ -1286,28 +1321,14 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
data->T7_address - cfg_start_ofs,
config_mem_size);
- if (config_crc > 0 && (config_crc != calculated_crc))
+ if (config_crc > 0 && config_crc != calculated_crc)
dev_warn(dev, "Config CRC error, calculated=%06X, file=%06X\n",
calculated_crc, config_crc);
- /* Write configuration as blocks */
- byte_offset = 0;
- while (byte_offset < config_mem_size) {
- size = config_mem_size - byte_offset;
-
- if (size > MXT_MAX_BLOCK_WRITE)
- size = MXT_MAX_BLOCK_WRITE;
-
- ret = __mxt_write_reg(data->client,
- cfg_start_ofs + byte_offset,
- size, config_mem + byte_offset);
- if (ret != 0) {
- dev_err(dev, "Config write error, ret=%d\n", ret);
- goto release_mem;
- }
-
- byte_offset += size;
- }
+ ret = mxt_upload_cfg_mem(data, cfg_start_ofs,
+ config_mem, config_mem_size);
+ if (ret)
+ goto release_mem;
mxt_update_crc(data, MXT_COMMAND_BACKUPNV, MXT_BACKUP_VALUE);
@@ -1319,8 +1340,6 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
release_mem:
kfree(config_mem);
-release:
- release_firmware(cfg);
return ret;
}
@@ -1640,6 +1659,7 @@ static int mxt_configure_objects(struct mxt_data *data,
static void mxt_config_cb(const struct firmware *cfg, void *ctx)
{
mxt_configure_objects(ctx, cfg);
+ release_firmware(cfg);
}
static int mxt_initialize(struct mxt_data *data)
--
1.9.1
^ permalink raw reply related
* Re: Minor fixes for atmel_mxt_ts
From: nick.dyer @ 2014-08-08 15:58 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Yufeng Shen, Daniel Kurtz, Henrik Rydberg, Joonyoung Shim,
Alan Bowens, linux-input, linux-kernel, Peter Meerwald,
Benson Leung, Olof Johansson, Sekhar Nori, Stephen Warren
In-Reply-To: <20140807165932.GA23198@core.coreip.homeip.net>
Hi Dmitry-
I tested both your patches, here they are rebased on your current tree and
with my signoff.
I need to have a little more of a play with the change to the
input_unregister_device code before putting it it. I want to be sure that all
the possible probe failures do the sensible thing. Hopefully will get that
done early next week.
regards
Nick
^ permalink raw reply
* [PATCH 1/2] Input: atmel_mxt_ts - simplify mxt_initialize a bit
From: nick.dyer @ 2014-08-08 15:58 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Yufeng Shen, Daniel Kurtz, Henrik Rydberg, Joonyoung Shim,
Alan Bowens, linux-input, linux-kernel, Peter Meerwald,
Benson Leung, Olof Johansson, Sekhar Nori, Stephen Warren,
Nick Dyer
In-Reply-To: <1407513483-30050-1-git-send-email-nick.dyer@itdev.co.uk>
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
I think having control flow with 2 goto/labels/flags is quite hard to read,
this version is a bit more readable IMO.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 81 +++++++++++++++++---------------
1 file changed, 42 insertions(+), 39 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index d50c614..a5f943d 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -359,7 +359,6 @@ static int mxt_bootloader_read(struct mxt_data *data,
msg.buf = val;
ret = i2c_transfer(data->client->adapter, &msg, 1);
-
if (ret == 1) {
ret = 0;
} else {
@@ -414,6 +413,7 @@ static int mxt_lookup_bootloader_address(struct mxt_data *data, bool retry)
case 0x5b:
bootloader = appmode - 0x26;
break;
+
default:
dev_err(&data->client->dev,
"Appmode i2c address 0x%02x not found\n",
@@ -425,20 +425,20 @@ static int mxt_lookup_bootloader_address(struct mxt_data *data, bool retry)
return 0;
}
-static int mxt_probe_bootloader(struct mxt_data *data, bool retry)
+static int mxt_probe_bootloader(struct mxt_data *data, bool alt_address)
{
struct device *dev = &data->client->dev;
- int ret;
+ int error;
u8 val;
bool crc_failure;
- ret = mxt_lookup_bootloader_address(data, retry);
- if (ret)
- return ret;
+ error = mxt_lookup_bootloader_address(data, alt_address);
+ if (error)
+ return error;
- ret = mxt_bootloader_read(data, &val, 1);
- if (ret)
- return ret;
+ error = mxt_bootloader_read(data, &val, 1);
+ if (error)
+ return error;
/* Check app crc fail mode */
crc_failure = (val & ~MXT_BOOT_STATUS_MASK) == MXT_APP_CRC_FAIL;
@@ -1645,41 +1645,39 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)
static int mxt_initialize(struct mxt_data *data)
{
struct i2c_client *client = data->client;
+ int recovery_attempts = 0;
int error;
- bool alt_bootloader_addr = false;
- bool retry = false;
-retry_info:
- error = mxt_get_info(data);
- if (error) {
-retry_bootloader:
- error = mxt_probe_bootloader(data, alt_bootloader_addr);
+ while (1) {
+ error = mxt_get_info(data);
+ if (!error)
+ break;
+
+ /* Check bootloader state */
+ error = mxt_probe_bootloader(data, false);
if (error) {
- if (alt_bootloader_addr) {
+ dev_info(&client->dev, "Trying alternate bootloader address\n");
+ error = mxt_probe_bootloader(data, true);
+ if (error) {
/* Chip is not in appmode or bootloader mode */
return error;
}
+ }
- dev_info(&client->dev, "Trying alternate bootloader address\n");
- alt_bootloader_addr = true;
- goto retry_bootloader;
- } else {
- if (retry) {
- dev_err(&client->dev, "Could not recover from bootloader mode\n");
- /*
- * We can reflash from this state, so do not
- * abort init
- */
- data->in_bootloader = true;
- return 0;
- }
-
- /* Attempt to exit bootloader into app mode */
- mxt_send_bootloader_cmd(data, false);
- msleep(MXT_FW_RESET_TIME);
- retry = true;
- goto retry_info;
+ /* OK, we are in bootloader, see if we can recover */
+ if (++recovery_attempts > 1) {
+ dev_err(&client->dev, "Could not recover from bootloader mode\n");
+ /*
+ * We can reflash from this state, so do not
+ * abort initialization.
+ */
+ data->in_bootloader = true;
+ return 0;
}
+
+ /* Attempt to exit bootloader into app mode */
+ mxt_send_bootloader_cmd(data, false);
+ msleep(MXT_FW_RESET_TIME);
}
/* Get object table information */
@@ -1693,9 +1691,14 @@ retry_bootloader:
if (error)
goto err_free_object_table;
- request_firmware_nowait(THIS_MODULE, true, MXT_CFG_NAME,
- &data->client->dev, GFP_KERNEL, data,
- mxt_config_cb);
+ error = request_firmware_nowait(THIS_MODULE, true, MXT_CFG_NAME,
+ &client->dev, GFP_KERNEL, data,
+ mxt_config_cb);
+ if (error) {
+ dev_err(&client->dev, "Failed to invoke firmware loader: %d\n",
+ error);
+ goto err_free_object_table;
+ }
return 0;
--
1.9.1
^ permalink raw reply related
* Re: Backslash repeat bug
From: Fredrik Hallenberg @ 2014-08-08 16:08 UTC (permalink / raw)
To: linux-input
In-Reply-To: <CAMsZVf-Y5202EyYod+ZDfjEd_OS7N2oOFYEMniCc5Z12B14uqA@mail.gmail.com>
I just found bugzilla bug #70181 which is the same problem as I
describe but seen with a QPAD MK-85 keyboard. I will add my findings
to that bug.
On Fri, Aug 8, 2014 at 4:11 PM, Fredrik Hallenberg <megahallon@gmail.com> wrote:
> Hi,
>
> On my Corsair K70 keyboard, when pressing some key and then the
> backslash key (KEY_BACKSLASH) in rapid succession the backslash
> character will be repeated three times. This bug has been mentioned on
> this list before (by Norbert Veber on the 24th of februrary) but there
> was no responses, also Norbert consider it a problem with Corsair
> keyboards but as I describe below I feel it is more of a general
> problem in the HID input code.
>
> This bug is discussed on the Corsair forums, but it seems the exact
> same thing also happens with Gigabyte Osmium and QPAD MK85, possibly
> it is a problem on all keyboards supporting n-key rollover.
>
> I believe the root cause of this is that the keyboard reports all keys
> including both HID keys 0x31 and 0x32 which are both mapped to
> KEY_BACKSLASH in hid-input.c. I am using a nordic keyboard meaning
> KEY_BACKSLASH is labeled "apostrophe" and that HID key 0x32 is
> reported. A US keyboard has a backslash key which should report 0x31.
>
> I have added some traces before the input_event call at the end of
> hidinput_report_event. When pressing A key and then apostrophe,
> something like this is seen (omitting all inactive keys):
>
> VALUE 1 CODE 30 HID 0x4
> VALUE 0 CODE 43 HID 0x31
> VALUE 0 CODE 43 HID 0x32
> Output: a
>
> VALUE 1 CODE 30 HID 0x4
> VALUE 0 CODE 43 HID 0x31
> VALUE 1 CODE 43 HID 0x32
> Output: '
>
> VALUE 0 CODE 30 HID 0x4
> VALUE 0 CODE 43 HID 0x31
> VALUE 1 CODE 43 HID 0x32
> Output: '
>
> VALUE 0 CODE 30 HID 0x4
> VALUE 0 CODE 43 HID 0x31
> VALUE 1 CODE 43 HID 0x32
> Output: '
>
> VALUE 0 CODE 30 HID 0x4
> VALUE 0 CODE 43 HID 0x31
> VALUE 0 CODE 43 HID 0x32
>
> So the fact that 0x31 is inactive will cause bad release events on key
> 43 to be sent.
>
> I have made a simple patch that tracks which of the two variants is
> active and ignores the other one. It works but I don't have much
> knowledge on HID input so before posting it would like to know if
> someone has input on how to do a cleaner fix. I am happy to help with
> testing or implementation if I am able.
>
> Regards,
>
> Fredrik
^ permalink raw reply
* Re: [PATCH 1/2] Input: atmel_mxt_ts - simplify mxt_initialize a bit
From: Dmitry Torokhov @ 2014-08-08 16:19 UTC (permalink / raw)
To: nick.dyer
Cc: Yufeng Shen, Daniel Kurtz, Henrik Rydberg, Joonyoung Shim,
Alan Bowens, linux-input, linux-kernel, Peter Meerwald,
Benson Leung, Olof Johansson, Sekhar Nori, Stephen Warren
In-Reply-To: <1407513483-30050-2-git-send-email-nick.dyer@itdev.co.uk>
On Fri, Aug 08, 2014 at 04:58:02PM +0100, nick.dyer@itdev.co.uk wrote:
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> I think having control flow with 2 goto/labels/flags is quite hard to read,
> this version is a bit more readable IMO.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
Nick, are there any changes from the version I posted? (If not then I do
not need to drop my patch from the queue and re-import this one).
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] Input: atmel_mxt_ts - simplify mxt_initialize a bit
From: Dmitry Torokhov @ 2014-08-08 16:19 UTC (permalink / raw)
To: nick.dyer
Cc: Yufeng Shen, Daniel Kurtz, Henrik Rydberg, Joonyoung Shim,
Alan Bowens, linux-input, linux-kernel, Peter Meerwald,
Benson Leung, Olof Johansson, Sekhar Nori, Stephen Warren
In-Reply-To: <20140808161901.GA26820@core.coreip.homeip.net>
On Fri, Aug 08, 2014 at 09:19:01AM -0700, Dmitry Torokhov wrote:
> On Fri, Aug 08, 2014 at 04:58:02PM +0100, nick.dyer@itdev.co.uk wrote:
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >
> > I think having control flow with 2 goto/labels/flags is quite hard to read,
> > this version is a bit more readable IMO.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
>
> Nick, are there any changes from the version I posted? (If not then I do
> not need to drop my patch from the queue and re-import this one).
Ah, sorry, should have read all my email before replying...
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
From: Dmitry Torokhov @ 2014-08-08 16:21 UTC (permalink / raw)
To: Nick Dyer
Cc: Javier Martinez Canillas, Stephen Warren, Yufeng Shen,
Benson Leung, Doug Anderson, Olof Johansson, Tomasz Figa,
linux-input, devicetree, linux-samsung-soc, linux-kernel
In-Reply-To: <53E4D9A5.908@itdev.co.uk>
On Fri, Aug 08, 2014 at 03:07:33PM +0100, Nick Dyer wrote:
> On 07/08/14 08:44, Javier Martinez Canillas wrote:
> > The Atmel maXTouch driver assumed that the IRQ type flags will
> > always be passed using platform data but this is not true when
> > booting using Device Trees. In these setups the interrupt type
> > was ignored by the driver when requesting an IRQ.
> >
> > This means that it will fail if a machine specified other type
> > than IRQ_TYPE_NONE. The right approach is to get the IRQ flags
> > that was parsed by OF from the "interrupt" Device Tree propery.
> >
> > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>
> I'm happy for this to go in if Dmitry will accept it. It does seem to be a
> quirk of some platforms that it is necessary, but it's only one line.
I'd rather not as it masks the deeper platform issue. There might be
other drovers also expecting platform/OF code set up interrupt triggers
and working/not working by chance.
Can we figure out why the platform in question needs this change?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
From: Tomasz Figa @ 2014-08-08 16:25 UTC (permalink / raw)
To: Javier Martinez Canillas, Dmitry Torokhov
Cc: Nick Dyer, Stephen Warren, Yufeng Shen, Benson Leung,
Doug Anderson, Olof Johansson, linux-input, devicetree,
linux-samsung-soc, linux-kernel, Thomas Gleixner, Jason Cooper,
Benjamin Herrenschmidt
In-Reply-To: <53E4CF8D.7080302@collabora.co.uk>
+CC Thomas, Jason and Ben
On 08.08.2014 15:24, Javier Martinez Canillas wrote:
> Hello,
>
> On 08/07/2014 06:47 PM, Dmitry Torokhov wrote:
>>
>> Actually, I take this back. In mainline everything as it should: if
>> pdata does not specify particular trigger the flags requested end up
>> being IRQF_ONESHOT, which should preserve trigger bits previously set up
>> by the board or OF code. In Chrome kernel we have:
>>
>
> In theory it should work as Dmitry and Nick said since if no trigger flags are
> set then whatever was set before (by OF, platform code, ACPI) should be used.
>
> I also verified what Tomasz mentioned that the IRQ trigger type is parsed and
> set correctly by OF:
>
> irq_of_parse_and_map()
> irq_create_of_mapping()
> irq_set_irq_type()
> __irq_set_trigger()
> chip->irq_set_type()
> exynos_irq_set_type()
>
> But for some reason it doesn't work for me unless I set the trigger type in the
> flags passed to request_threaded_irq().
>
> I found that what makes it to work is the logic in __setup_irq() [0] that Nick
> pointed out on a previous email:
>
> /* Setup the type (level, edge polarity) if configured: */
> if (new->flags & IRQF_TRIGGER_MASK) {
> ret = __irq_set_trigger(desc, irq,
> new->flags & IRQF_TRIGGER_MASK);
>
> if (ret)
> goto out_mask;
> }
>
> So __irq_set_trigger() is only executed if the struct irqaction flags has a
> trigger flag which makes sense since this shouldn't be necessary with DT due
> __irq_set_trigger() being called from irq_create_of_mapping() before when the
> "interrupts" property is parsed.
>
> But for some reason it is necessary for me... I checked that struct irq_data
> state_use_accessors value is correct and even tried setting that value to
> new->flags after the mentioned code block but it makes no difference. Input
> events are not reported by evtest and AFAICT interrupts are not being generated.
>
> It works though if the trigger type is passed to request_threaded_irq() like
> $subject does or if new->flags are set before the new->flags & IRQF_TRIGGER_MASK
> conditional.
>
> For example, with the following changes interrupts are fired correctly:
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 3dc6a61..2d8adbb 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1169,6 +1169,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc,
> struct irqaction *new)
>
> init_waitqueue_head(&desc->wait_for_threads);
>
> + if (!(new->flags & IRQF_TRIGGER_MASK))
> + new->flags |= irqd_get_trigger_type(desc);
> +
> /* Setup the type (level, edge polarity) if configured: */
> if (new->flags & IRQF_TRIGGER_MASK) {
> ret = __irq_set_trigger(desc, irq,
>
> Any ideas what could be wrong here?
>
>> /* Default to falling edge if no platform data provided */
>> irqflags = data->pdata ? data->pdata->irqflags : IRQF_TRIGGER_FALLING;
>> error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
>> irqflags | IRQF_ONESHOT,
>> client->name, data);
>>
>
> Exactly, that's how I found the issue. When I compared both drivers I noticed
> that the Chrome OS driver did that and since all the supported platforms are DT
> based, the above is equivalent to just IRQF_TRIGGER_FALLING | IRQF_ONESHOT.
>
> So according to my explanation, new->flags & IRQF_TRIGGER_MASK is true so
> __irq_set_trigger() is executed and that's why it works with the Chrome OS driver.
>
> In fact the Chrome OS DTS does not set a trigger type in the "interrupts" property:
>
> trackpad@4b {
> reg=<0x4b>;
> compatible="atmel,atmel_mxt_tp";
> interrupts=<1 0>;
> interrupt-parent=<&gpx1>;
> wakeup-source;
> };
>
>
>> which I believe should go away. If it is needed on ACPI systems we need
>> to figure out how to do things we can do with OF there.
>>
>
> The above code should not be related to ACPI systems since whatever code that
> parses an ACPI table should just call irq_set_irq_type() like is made by OF, so
> request_threaded_irq() should just work with ACPI too.
>
> I agree it should go away but first I want to understand why is needed in the
> first place. Unfortunately commit:
>
> 031f136 ("Input: atmel_mxt_ts - Set default irqflags when there is no pdata")
>
> from the Chrome OS 3.8 does not explain why this is needed, instead of adding
> this information in the DTS (e.g: interrupts=<1 IRQ_TYPE_EDGE_FALLING>).
>
>> Thanks.
>>
>
> Best regards,
> Javier
>
> [0]: http://lxr.free-electrons.com/source/kernel/irq/manage.c#L1172
>
^ permalink raw reply
* Re: [PATCH] Input: da9052_onkey: convert to use devm_*
From: Dmitry Torokhov @ 2014-08-08 16:26 UTC (permalink / raw)
To: Kiran Padwal
Cc: support.opensource, anthony.olech.opensource, david.chen,
kiran.padwal21, linux-input, linux-kernel
In-Reply-To: <1407482324-17713-1-git-send-email-kiran.padwal@smartplayin.com>
Hi Kiran,
On Fri, Aug 08, 2014 at 12:48:44PM +0530, Kiran Padwal wrote:
> Converting to devm functions can make the code smaller and cleaner.
I'd rather not mix use of managed and non-managed resources in one
driver.
Thanks.
>
> Signed-off-by: Kiran Padwal <kiran.padwal@smartplayin.com>
> ---
> drivers/input/misc/da9052_onkey.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/input/misc/da9052_onkey.c b/drivers/input/misc/da9052_onkey.c
> index 184c8f2..909441e 100644
> --- a/drivers/input/misc/da9052_onkey.c
> +++ b/drivers/input/misc/da9052_onkey.c
> @@ -84,12 +84,14 @@ static int da9052_onkey_probe(struct platform_device *pdev)
> return -EINVAL;
> }
>
> - onkey = kzalloc(sizeof(*onkey), GFP_KERNEL);
> - input_dev = input_allocate_device();
> - if (!onkey || !input_dev) {
> - dev_err(&pdev->dev, "Failed to allocate memory\n");
> - error = -ENOMEM;
> - goto err_free_mem;
> + onkey = devm_kzalloc(&pdev->dev, sizeof(*onkey), GFP_KERNEL);
> + if (!onkey)
> + return -ENOMEM;
> +
> + input_dev = devm_input_allocate_device(&pdev->dev);
> + if (!input_dev) {
> + dev_err(&pdev->dev, "Failed to allocate input device\n");
> + return -ENOMEM;
> }
>
> onkey->input = input_dev;
> @@ -108,7 +110,7 @@ static int da9052_onkey_probe(struct platform_device *pdev)
> if (error < 0) {
> dev_err(onkey->da9052->dev,
> "Failed to register ONKEY IRQ: %d\n", error);
> - goto err_free_mem;
> + return error;
> }
>
> error = input_register_device(onkey->input);
> @@ -124,9 +126,6 @@ static int da9052_onkey_probe(struct platform_device *pdev)
> err_free_irq:
> da9052_free_irq(onkey->da9052, DA9052_IRQ_NONKEY, onkey);
> cancel_delayed_work_sync(&onkey->work);
> -err_free_mem:
> - input_free_device(input_dev);
> - kfree(onkey);
>
> return error;
> }
> @@ -139,7 +138,6 @@ static int da9052_onkey_remove(struct platform_device *pdev)
> cancel_delayed_work_sync(&onkey->work);
>
> input_unregister_device(onkey->input);
> - kfree(onkey);
>
> return 0;
> }
> --
> 1.7.9.5
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] Input: da9055_onkey: Remove unnecessary OOM meassage
From: Dmitry Torokhov @ 2014-08-08 16:26 UTC (permalink / raw)
To: Kiran Padwal
Cc: support.opensource, anthony.olech.opensource, david.chen,
kiran.padwal21, linux-input, linux-kernel
In-Reply-To: <1407482097-17624-1-git-send-email-kiran.padwal@smartplayin.com>
On Fri, Aug 08, 2014 at 12:44:57PM +0530, Kiran Padwal wrote:
> There is no need to print out-of-memory errors since this is already
> done by the memory management subsystem which even calls dump_stack().
This is implementation detail as far as I am concerned. We might switch
dumping stack off down the road.
Thanks.
>
> Signed-off-by: Kiran Padwal <kiran.padwal@smartplayin.com>
> ---
> drivers/input/misc/da9055_onkey.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/input/misc/da9055_onkey.c b/drivers/input/misc/da9055_onkey.c
> index 4765799..ed6eaa9 100644
> --- a/drivers/input/misc/da9055_onkey.c
> +++ b/drivers/input/misc/da9055_onkey.c
> @@ -87,10 +87,8 @@ static int da9055_onkey_probe(struct platform_device *pdev)
> }
>
> onkey = devm_kzalloc(&pdev->dev, sizeof(*onkey), GFP_KERNEL);
> - if (!onkey) {
> - dev_err(&pdev->dev, "Failed to allocate memory\n");
> + if (!onkey)
> return -ENOMEM;
> - }
>
> input_dev = input_allocate_device();
> if (!input_dev) {
> --
> 1.7.9.5
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
From: Javier Martinez Canillas @ 2014-08-08 16:38 UTC (permalink / raw)
To: Dmitry Torokhov, Nick Dyer
Cc: Stephen Warren, Yufeng Shen, Benson Leung, Doug Anderson,
Olof Johansson, Tomasz Figa, linux-input, devicetree,
linux-samsung-soc, linux-kernel, Thomas Gleixner, Jason Cooper,
Benjamin Herrenschmidt, Thomas Abraham
In-Reply-To: <20140808162146.GC26820@core.coreip.homeip.net>
+Thomas Gleixner, Jason Cooper, Benjamin Herrenschmidt and Thomas Abraham
Hello Dmitry,
On 08/08/2014 06:21 PM, Dmitry Torokhov wrote:
> On Fri, Aug 08, 2014 at 03:07:33PM +0100, Nick Dyer wrote:
>> On 07/08/14 08:44, Javier Martinez Canillas wrote:
>> > The Atmel maXTouch driver assumed that the IRQ type flags will
>> > always be passed using platform data but this is not true when
>> > booting using Device Trees. In these setups the interrupt type
>> > was ignored by the driver when requesting an IRQ.
>> >
>> > This means that it will fail if a machine specified other type
>> > than IRQ_TYPE_NONE. The right approach is to get the IRQ flags
>> > that was parsed by OF from the "interrupt" Device Tree propery.
>> >
>> > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>>
>> I'm happy for this to go in if Dmitry will accept it. It does seem to be a
>> quirk of some platforms that it is necessary, but it's only one line.
>
> I'd rather not as it masks the deeper platform issue. There might be
> other drovers also expecting platform/OF code set up interrupt triggers
> and working/not working by chance.
>
I totally agree. When posted the patch I thought that it was the right fix but
after your explanation and studying the IRQ core I see that as you said this i
just hiding a more fundamental issue. We should fix the root cause instead of
adding a workaround for every driver.
> Can we figure out why the platform in question needs this change?
>
I dig further on this. First I wanted to see if the problem was on IRQ core or
in the irqchip driver so I tried calling the chip's .irq_set_type function
handler directly using the flags set by __irq_set_trigger() the first time that
it's called from OF when the "interrupts" property is parsed.
Doing that makes the device to trigger interrupts so the problem seems to be
related to the pinctrl-exynos driver and is not in the IRQ core.
So, this is the change I did just for testing purposes to make it work again:
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 3dc6a61..ed76b25 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1176,6 +1176,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc,
struct irqaction *new)
if (ret)
goto out_mask;
+ } else if (irq == 283 /* mapped IRQ number for touchpad */) {
+ struct irq_chip *chip = desc->irq_data.chip;
+ chip->irq_set_type(&desc->irq_data,
+ irq_get_trigger_type(irq));
}
desc->istate &= ~(IRQS_AUTODETECT | IRQS_SPURIOUS_DISABLED | \
It seems as if the first call to exynos_irq_set_type() that is made by OF is a
no-op while the second call is the one that actually setups the hw correctly.
Does this make any sense? Maybe is related to the pin not being muxed in the
correct function when the "interrupts" property is parsed by OF?
Now I added some debug logs to see what could be different but all the variables
have the same values in both cases:
When called happens due irq_parse_and_map():
irq 283 type 2 pin 1 shift 4 base 4026679296 reg_con 3588 con 32 flags 0 mask 0
When called happens due request_threaded_irq():
irq 283 type 2 pin 1 shift 4 base 4026679296 reg_con 3588 con 32 flags 0 mask 0
I would really appreciate if someone that is more familiar with the driver or
this chip can provide some hints.
Thanks a lot and best regards,
Javier
^ permalink raw reply related
* Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl
From: Dmitry Torokhov @ 2014-08-08 17:47 UTC (permalink / raw)
To: David Herrmann
Cc: Peter Hutterer, Benjamin Tissoires, open list:HID CORE LAYER,
Dmitry Torokhov
In-Reply-To: <CANq1E4QB3wiHRTjb_4xmVvXwOZvdpOQR9nZtnaZt=k2GaLJAmw@mail.gmail.com>
On Fri, Aug 08, 2014 at 03:26:56PM +0200, David Herrmann wrote:
> Hi
>
> On Wed, Aug 6, 2014 at 3:35 AM, Peter Hutterer <peter.hutterer@who-t.net> wrote:
> >> +
> >> +/**
> >> + * EVIOCGABSRANGE - Fetch range of ABS values
> >> + *
> >> + * This fetches the current values of a range of ABS codes atomically. The range
> >> + * of codes to fetch and the buffer-types are passed as "struct input_absrange",
> >> + * which has the following fields:
> >> + * slots: Number of MT slots to fetch data for.
> >> + * code: First ABS axis to query.
> >> + * count: Number of ABS axes to query starting at @code.
> >> + * buffer: Pointer to a receive buffer where to store the fetched ABS
> >> + * values. This buffer must be an array of __s32 with at least
> >> + * (@slots * @code) elements. The buffer is interpreted as two
> >> + * dimensional __s32 array, declared as: __s32[slots][codes]
> >
> > tbh this seems more complicated than necessary. Have you thought about
> > just dumping the events into the client buffer as if they came fresh in from
> > the device? So to sync, the client calls the ioctl with a buffer and a
> > buffer size, and the kernel simply writes a series of struct input_events
> > into that buffer, with ABS_MT_SLOT as required for all slots, (optionally?)
> > followed by a SYN_DROPPED. So the buffer afterwards could look like this:
> > EV_ABS ABS_X 30
> > EV_ABS ABS_X 1202
> > EV_ABS ABS_MT_SLOT 0
> > EV_ABS ABS_MT_POSITION_X 30
> > EV_ABS ABS_MT_POSITION_Y 1202
> > EV_ABS ABS_MT_SLOT 1
> > EV_ABS ABS_MT_POSITION_X 80
> > EV_ABS ABS_MT_POSITION_Y 1800
> > EV_SYN SYN_REPORT 0
> >
> > the client can then go through and just process the events on-by-one as it
> > would otherwise with real events.
> >
> > This approach could be even extended to include EV_KEY, etc. providing a
> > single ioctl to sync the whole state of the device atomically.
> >
> > comments?
I like it.
>
> So you mean instead of passing a __32 array we pass a "struct
> input_event" array and write it there? So bypassing the receive-queue?
> That does sound quite nice, indeed. We could replace all the other
> "sync" ioctls and just provide a way to receive all the events
> directly.
>
> Something like:
>
> EVIOCQUERY(struct input_query)
>
> struct input_query {
> __u16 type;
> __u16 start_code;
> __u16 end_code;
> __u16 slots;
>
> struct input_event buffer[];
> };
No, it is more like EVIOCRESYNC(void) which makes input core to dump all
existing state into the client's standard event queue so that here is no
need to reconcile/reconstruct anything. We could give a new SYN marker
to indicate end-of-state boundary.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
From: Javier Martinez Canillas @ 2014-08-08 18:29 UTC (permalink / raw)
To: Dmitry Torokhov, Nick Dyer
Cc: Stephen Warren, Yufeng Shen, Benson Leung, Doug Anderson,
Olof Johansson, Tomasz Figa, linux-input, devicetree,
linux-samsung-soc, linux-kernel, Thomas Gleixner, Jason Cooper,
Benjamin Herrenschmidt, Thomas Abraham
In-Reply-To: <53E4FD1B.6060600@collabora.co.uk>
Hello,
On 08/08/2014 06:38 PM, Javier Martinez Canillas wrote:
>
>
> It seems as if the first call to exynos_irq_set_type() that is made by OF is a
> no-op while the second call is the one that actually setups the hw correctly.
> Does this make any sense? Maybe is related to the pin not being muxed in the
> correct function when the "interrupts" property is parsed by OF?
>
So after a conversation with Tomasz Figa over IRC the problem was after all that
the pin was reconfigured. The IRQ trigger type resulted to be just a red herring
and not a direct cause.
The pinctrl-eyxnos driver does the IRQ pinmux setup in the .irq_set_type
function handler. So what happened was that OF parsed the "interrupts" property
and called exynos_irq_set_type() which did the pinmux setup.
But after that, due a DTS pinctrl configuration the pin function was changed as
a GPIO input and that happened before the atmel driver was probed. So when the
driver called request_threaded_irq(), the correct flags were used but the pin
was not configured as an IRQ anymore so IRQ were not fired.
Setting a trigger type just had the side effect of calling exynos_irq_set_type()
which again setup the pin as an IRQ.
To fix the issue a variation of patch [0] will be posted that moves the IRQ
pinmux setup from .irq_set_type to the .irq_request_resources function handler.
That way the pin will be setup as IRQ regardless of the the trigger type [1]
when someone calls request_[threaded]_irq().
Only the mentioned patch fixes the issue but Tomasz said that even a call to
gpio_direction_{input,output} can change the pin configuration so he will post
another patch that will add a bit mask to samsung_pin_bank to prevent any pinmux
reconfiguration.
Thanks a lot and best regards,
Javier
[0]: http://thread.gmane.org/gmane.linux.kernel.samsung-soc/34259/focus=34261
[1]: http://lxr.free-electrons.com/source/kernel/irq/manage.c#L1162
^ permalink raw reply
* Re: [PATCH v2 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
From: Tomasz Figa @ 2014-08-08 18:41 UTC (permalink / raw)
To: Javier Martinez Canillas, Dmitry Torokhov, Nick Dyer
Cc: Stephen Warren, Yufeng Shen, Benson Leung, Doug Anderson,
Olof Johansson, linux-input-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Thomas Gleixner,
Jason Cooper, Benjamin Herrenschmidt, Thomas Abraham,
Linus Walleij
In-Reply-To: <53E5170C.10302-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
+CC Linus, as this became also pinctrl related.
On 08.08.2014 20:29, Javier Martinez Canillas wrote:
> Hello,
>
> On 08/08/2014 06:38 PM, Javier Martinez Canillas wrote:
>>
>>
>> It seems as if the first call to exynos_irq_set_type() that is made by OF is a
>> no-op while the second call is the one that actually setups the hw correctly.
>> Does this make any sense? Maybe is related to the pin not being muxed in the
>> correct function when the "interrupts" property is parsed by OF?
>>
>
> So after a conversation with Tomasz Figa over IRC the problem was after all that
> the pin was reconfigured. The IRQ trigger type resulted to be just a red herring
> and not a direct cause.
>
> The pinctrl-eyxnos driver does the IRQ pinmux setup in the .irq_set_type
> function handler. So what happened was that OF parsed the "interrupts" property
> and called exynos_irq_set_type() which did the pinmux setup.
>
> But after that, due a DTS pinctrl configuration the pin function was changed as
> a GPIO input and that happened before the atmel driver was probed. So when the
> driver called request_threaded_irq(), the correct flags were used but the pin
> was not configured as an IRQ anymore so IRQ were not fired.
>
> Setting a trigger type just had the side effect of calling exynos_irq_set_type()
> which again setup the pin as an IRQ.
>
> To fix the issue a variation of patch [0] will be posted that moves the IRQ
> pinmux setup from .irq_set_type to the .irq_request_resources function handler.
> That way the pin will be setup as IRQ regardless of the the trigger type [1]
> when someone calls request_[threaded]_irq().
>
> Only the mentioned patch fixes the issue but Tomasz said that even a call to
> gpio_direction_{input,output} can change the pin configuration so he will post
> another patch that will add a bit mask to samsung_pin_bank to prevent any pinmux
> reconfiguration.
To add a bit more information about the hardware, setting up a GPIO
interrupt on Samsung SoCs is a two-step operation - in addition to
trigger configuration in a dedicated register, the pinmux must be also
reconfigured to GPIO interrupt, which is a different function than
normal GPIO input, although I/O-wise they both behave in the same way
and gpio_get_value() can be used on a pin configured as IRQ as well.
I'm afraid that such design implies that handling of this in the driver
must be done on a very low level, because it involves three possible
interfaces changing the pinmux - pinctrl, GPIO and irqchip and such
subtletes like gpio_direction_input() that shouldn't neither fail if a
pin is already configured as an interrupt nor change the configuration
to normal input.
Best regards,
Tomasz
--
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
* Re: [PATCH v2 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
From: Doug Anderson @ 2014-08-08 20:54 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Dmitry Torokhov, Nick Dyer, Stephen Warren, Yufeng Shen,
Benson Leung, Olof Johansson, Tomasz Figa,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-samsung-soc, linux-kernel@vger.kernel.org, Thomas Gleixner,
Jason Cooper, Benjamin Herrenschmidt, Thomas Abraham
In-Reply-To: <53E5170C.10302@collabora.co.uk>
Hi,
On Fri, Aug 8, 2014 at 11:29 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> Hello,
>
> On 08/08/2014 06:38 PM, Javier Martinez Canillas wrote:
>>
>>
>> It seems as if the first call to exynos_irq_set_type() that is made by OF is a
>> no-op while the second call is the one that actually setups the hw correctly.
>> Does this make any sense? Maybe is related to the pin not being muxed in the
>> correct function when the "interrupts" property is parsed by OF?
>>
>
> So after a conversation with Tomasz Figa over IRC the problem was after all that
> the pin was reconfigured. The IRQ trigger type resulted to be just a red herring
> and not a direct cause.
>
> The pinctrl-eyxnos driver does the IRQ pinmux setup in the .irq_set_type
> function handler. So what happened was that OF parsed the "interrupts" property
> and called exynos_irq_set_type() which did the pinmux setup.
>
> But after that, due a DTS pinctrl configuration the pin function was changed as
> a GPIO input and that happened before the atmel driver was probed. So when the
> driver called request_threaded_irq(), the correct flags were used but the pin
> was not configured as an IRQ anymore so IRQ were not fired.
>
> Setting a trigger type just had the side effect of calling exynos_irq_set_type()
> which again setup the pin as an IRQ.
>
> To fix the issue a variation of patch [0] will be posted that moves the IRQ
> pinmux setup from .irq_set_type to the .irq_request_resources function handler.
> That way the pin will be setup as IRQ regardless of the the trigger type [1]
> when someone calls request_[threaded]_irq().
>
> Only the mentioned patch fixes the issue but Tomasz said that even a call to
> gpio_direction_{input,output} can change the pin configuration so he will post
> another patch that will add a bit mask to samsung_pin_bank to prevent any pinmux
> reconfiguration.
Would just making a device tree change fix this? AKA for the pin, do:
samsung,pin-function = <0xf>;
I have some vague recollection that I set interrupts to pin-function
"0" by default for some reason (assuming they would go to 0xf when
interrupts were enabled). ...but I can't for the life of me remember
if it was a good reason or just seemed like the right thing to do.
-Doug
^ permalink raw reply
* Re: [PATCH v2 05/06] input synaptics-rmi4: Add firmware update support
From: Christopher Heiny @ 2014-08-08 21:10 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina, Erol Eryilmaz, Bob Balcomb
In-Reply-To: <20140807064251.GE651@core.coreip.homeip.net>
On 08/06/2014 11:42 PM, Dmitry Torokhov wrote:
> On Wed, Aug 06, 2014 at 04:28:26PM -0700, Christopher Heiny wrote:
>> [I sent this last Thursday, but it never showed up on the input
>> list. I'm assuming nobody else saw it.]
>>
>>
>> On 07/31/2014 02:58 PM, Dmitry Torokhov wrote:
>>> On Thu, Jul 31, 2014 at 02:43:47PM -0700, Christopher Heiny wrote:
>>>> On 07/31/2014 02:19 PM, Dmitry Torokhov wrote:
>>>>> On Thu, Jul 31, 2014 at 02:00:14PM -0700, Christopher Heiny wrote:
>>>>>>> On 07/31/2014 10:53 AM, Dmitry Torokhov wrote:
>>>>>>>>> Hi Christopher,
>>>>>>>>>
>>>>>>>>> On Wed, Mar 12, 2014 at 06:53:56PM -0700, Christopher Heiny wrote:
>>>>>>>>>>> Add support for updating firmware on RMI4 devices with V5 bootloader.
>>>>>>>>>
>>>>>>>>> I am wondering why F34 is not following the staindard RMI function
>>>>>>>>> implementation. By that I mean that it does not declare itself as struct
>>>>>>>>> rmi_function_handler and does not rely on RMI core to bind itself to the device
>>>>>>>>> if device supports it.
>>>>>>>
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> We originally had an F34 implementation that followed the RMI4
>>>>>>> function standard and exposed most of the basic F34 operations via
>>>>>>> sysfs. However, we got feedback (both on LKML and offline) (a)
>>>>>>> recommending to use request_firmware, and (b) improve reflash times
>>>>>>> while (c) reducing impact on boot time, and (d) "get rid of all that
>>>>>>> sysfs crap" (paraphrased, but close to it).
>>>>>>>
>>>>>>> So after looking at how some other drivers use request_firmware, we
>>>>>>> came up with the current approach. Switching to request_firmware
>>>>>>> definitely sped up the reflash times! Including a check to see if
>>>>>>> firmware update is required before setting up the RMI4
>>>>>>> sensor/function structures also significantly reduced boot times.
>>>>>
>>>>> I am not suggesting you stop using request-firmware or introduce
>>>>> bazillion of new sysfs attributes. I just wondered why you have manual
>>>>> "binding" of F34 functionality instead of standrad RMI4 function
>>>>> binding, liek you do for F01, F11 and so forth.
>>>>
>>>> Sorry! My answer wasn't very clear on that part, was it?
>>>>
>>>> The manual binding gets the reflash (if required) done very early in
>>>> the boot/probe process. This eliminates the need to set up the
>>>> whole sensor + functions structure, tear it down in order to
>>>> reflash, and then build it all back up again. It is felt that the
>>>> time savings is significant, especially on highly featured products.
>>>
>>> I am sorry but I have hard time accepting this argument. How often do
>>> you reflash devices during normal operation and how long does it take to
>>> initialize the device compared to getting entire userspace up and
>>> running to be able to actually supply or serve flash data (even without
>>> using usermode helper to flash you need filesystem with the firmware to
>>> be mounted)?
>>
>> That was my argument exactly, but that was the direction we were
>> pushed. I'd much rather implement it as we discussed offline
>> earlier this week. If you were to say: "I'm sorry, but this simply
>> can't be merged as it stands." you wouldn't get any argument from me
>> on technical grounds. There might be people who will argue about the
>> additional calendar time it would take to restructure it, though.
>
> OK, then I will just say this: "I'm sorry, but this simply can't be merged as
> it stands."
>
> Now, I am talking about mainline here, I am fairly certain we can resolve
> scheduling issues between what you currently have and what is needed in the
> end.
OK. The question on our end becomes - can we accept the current
implementation as a baseline while we rework the implementation to use a
conformant F34 implementation? Our rough estimation is that this will
be ready around the end of this month or early next month.
Thanks,
Chris
^ permalink raw reply
* Re: [PATCH v2 05/06] input synaptics-rmi4: Add firmware update support
From: Dmitry Torokhov @ 2014-08-08 21:20 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina, Erol Eryilmaz, Bob Balcomb
In-Reply-To: <53E53CAF.6000802@synaptics.com>
On Friday, August 08, 2014 02:10:07 PM Christopher Heiny wrote:
> On 08/06/2014 11:42 PM, Dmitry Torokhov wrote:
> > On Wed, Aug 06, 2014 at 04:28:26PM -0700, Christopher Heiny wrote:
> >> [I sent this last Thursday, but it never showed up on the input
> >> list. I'm assuming nobody else saw it.]
> >>
> >> On 07/31/2014 02:58 PM, Dmitry Torokhov wrote:
> >>> On Thu, Jul 31, 2014 at 02:43:47PM -0700, Christopher Heiny wrote:
> >>>> On 07/31/2014 02:19 PM, Dmitry Torokhov wrote:
> >>>>> On Thu, Jul 31, 2014 at 02:00:14PM -0700, Christopher Heiny wrote:
> >>>>>>> On 07/31/2014 10:53 AM, Dmitry Torokhov wrote:
> >>>>>>>>> Hi Christopher,
> >>>>>>>>>
> >>>>>>>>> On Wed, Mar 12, 2014 at 06:53:56PM -0700, Christopher Heiny wrote:
> >>>>>>>>>>> Add support for updating firmware on RMI4 devices with V5
> >>>>>>>>>>> bootloader.
> >>>>>>>>>
> >>>>>>>>> I am wondering why F34 is not following the staindard RMI function
> >>>>>>>>> implementation. By that I mean that it does not declare itself as
> >>>>>>>>> struct
> >>>>>>>>> rmi_function_handler and does not rely on RMI core to bind itself
> >>>>>>>>> to the device if device supports it.
> >>>>>>>
> >>>>>>> Hi Dmitry,
> >>>>>>>
> >>>>>>> We originally had an F34 implementation that followed the RMI4
> >>>>>>> function standard and exposed most of the basic F34 operations via
> >>>>>>> sysfs. However, we got feedback (both on LKML and offline) (a)
> >>>>>>> recommending to use request_firmware, and (b) improve reflash times
> >>>>>>> while (c) reducing impact on boot time, and (d) "get rid of all that
> >>>>>>> sysfs crap" (paraphrased, but close to it).
> >>>>>>>
> >>>>>>> So after looking at how some other drivers use request_firmware, we
> >>>>>>> came up with the current approach. Switching to request_firmware
> >>>>>>> definitely sped up the reflash times! Including a check to see if
> >>>>>>> firmware update is required before setting up the RMI4
> >>>>>>> sensor/function structures also significantly reduced boot times.
> >>>>>
> >>>>> I am not suggesting you stop using request-firmware or introduce
> >>>>> bazillion of new sysfs attributes. I just wondered why you have manual
> >>>>> "binding" of F34 functionality instead of standrad RMI4 function
> >>>>> binding, liek you do for F01, F11 and so forth.
> >>>>
> >>>> Sorry! My answer wasn't very clear on that part, was it?
> >>>>
> >>>> The manual binding gets the reflash (if required) done very early in
> >>>> the boot/probe process. This eliminates the need to set up the
> >>>> whole sensor + functions structure, tear it down in order to
> >>>> reflash, and then build it all back up again. It is felt that the
> >>>> time savings is significant, especially on highly featured products.
> >>>
> >>> I am sorry but I have hard time accepting this argument. How often do
> >>> you reflash devices during normal operation and how long does it take to
> >>> initialize the device compared to getting entire userspace up and
> >>> running to be able to actually supply or serve flash data (even without
> >>> using usermode helper to flash you need filesystem with the firmware to
> >>> be mounted)?
> >>
> >> That was my argument exactly, but that was the direction we were
> >> pushed. I'd much rather implement it as we discussed offline
> >> earlier this week. If you were to say: "I'm sorry, but this simply
> >> can't be merged as it stands." you wouldn't get any argument from me
> >> on technical grounds. There might be people who will argue about the
> >> additional calendar time it would take to restructure it, though.
> >
> > OK, then I will just say this: "I'm sorry, but this simply can't be merged
> > as it stands."
> >
> > Now, I am talking about mainline here, I am fairly certain we can resolve
> > scheduling issues between what you currently have and what is needed in
> > the
> > end.
>
> OK. The question on our end becomes - can we accept the current
> implementation as a baseline while we rework the implementation to use a
> conformant F34 implementation? Our rough estimation is that this will
> be ready around the end of this month or early next month.
Chris,
We have a couple months till the next merge window.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
From: Javier Martinez Canillas @ 2014-08-08 22:26 UTC (permalink / raw)
To: Doug Anderson
Cc: Dmitry Torokhov, Nick Dyer, Stephen Warren, Yufeng Shen,
Benson Leung, Olof Johansson, Tomasz Figa,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-samsung-soc, linux-kernel@vger.kernel.org, Thomas Gleixner,
Jason Cooper, Benjamin Herrenschmidt, Thomas Abraham
In-Reply-To: <CAD=FV=WSNqjHLLbTh1LBF3RFrCnWnj498hivivcmx1SbuYp=Hw@mail.gmail.com>
Hello Doug,
On 08/08/2014 10:54 PM, Doug Anderson wrote:
> Hi,
>>
>> To fix the issue a variation of patch [0] will be posted that moves the IRQ
>> pinmux setup from .irq_set_type to the .irq_request_resources function handler.
>> That way the pin will be setup as IRQ regardless of the the trigger type [1]
>> when someone calls request_[threaded]_irq().
>>
>> Only the mentioned patch fixes the issue but Tomasz said that even a call to
>> gpio_direction_{input,output} can change the pin configuration so he will post
>> another patch that will add a bit mask to samsung_pin_bank to prevent any pinmux
>> reconfiguration.
>
> Would just making a device tree change fix this? AKA for the pin, do:
>
> samsung,pin-function = <0xf>;
>
Yes, when configuring the pin as 0xf (GPIO interrupt) instead of 0x0 (GPIO
input) the issue is not present indeed. So after Nick answer my question about
what I got wrong with the "linux,gpio-keymap" property, I will change the pin
function when re-posing the DTS changes for the atmel touchpad.
> I have some vague recollection that I set interrupts to pin-function
> "0" by default for some reason (assuming they would go to 0xf when
> interrupts were enabled). ...but I can't for the life of me remember
> if it was a good reason or just seemed like the right thing to do.
>
It would be great to know if there is a good reason. I see indeed that all
pinctrl setup in the Peach Pit/Pi and Snow DTS that involves interrupts are
using 0x0 as the pin function. Since as Tomasz explained Samsung SoC makes a
difference between GPIO-IRQ and GPIO input I guess that it's better to change
those to 0xf instead. What do you think?
Regardless of this though I think that both the patch to move the IRQ
pinmux setup from .irq_set_type to the .irq_request_resources and the patch to
to prevent any pinmux reconfiguration are good improvements to avoid future
issues like the one we found here.
> -Doug
>
Best regards,
Javier
^ permalink raw reply
* Re: [PATCH v2 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
From: Doug Anderson @ 2014-08-08 23:58 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Dmitry Torokhov, Nick Dyer, Stephen Warren, Yufeng Shen,
Benson Leung, Olof Johansson, Tomasz Figa,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-samsung-soc, linux-kernel@vger.kernel.org, Thomas Gleixner,
Jason Cooper, Benjamin Herrenschmidt, Thomas Abraham
In-Reply-To: <53E54E91.9060905@collabora.co.uk>
Javier,
On Fri, Aug 8, 2014 at 3:26 PM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
>> I have some vague recollection that I set interrupts to pin-function
>> "0" by default for some reason (assuming they would go to 0xf when
>> interrupts were enabled). ...but I can't for the life of me remember
>> if it was a good reason or just seemed like the right thing to do.
>>
>
> It would be great to know if there is a good reason. I see indeed that all
> pinctrl setup in the Peach Pit/Pi and Snow DTS that involves interrupts are
> using 0x0 as the pin function. Since as Tomasz explained Samsung SoC makes a
> difference between GPIO-IRQ and GPIO input I guess that it's better to change
> those to 0xf instead. What do you think?
I think it's worth trying out. If there are no problems with it then
let's do it.
My vague recollection is that I was worried that pinctrl would take
effect right at driver probe time (maybe this used to happen? or maybe
I imagined it?) and that configuring to 0xf at this point in time
would cause a spurious interrupt. I can't remember ever testing it so
it was probably just something I imagined. Even if it was configured
as 0xf I'd imagine that the interrupt would be masked anyway so there
should be no spurious interrupt, right?
> Regardless of this though I think that both the patch to move the IRQ
> pinmux setup from .irq_set_type to the .irq_request_resources and the patch to
> to prevent any pinmux reconfiguration are good improvements to avoid future
> issues like the one we found here.
OK. I'll let you, Tomasz, and Linus figure out what's best here since
I haven't done extensive thinking on it. ;)
-Doug
^ permalink raw reply
* [PATCH] Input: wacom - Fix compiler warning if !CONFIG_PM
From: Geert Uytterhoeven @ 2014-08-09 17:07 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, Geert Uytterhoeven
If CONFIG_PM is not set:
drivers/hid/wacom_sys.c:1436: warning: ‘wacom_reset_resume’ defined but
not used
Protect the unused functions by #ifdef CONFIG_PM to fix this.
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
drivers/hid/wacom_sys.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 3e388ec31da8..f0db7eca9023 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1416,6 +1416,7 @@ static void wacom_remove(struct hid_device *hdev)
kfree(wacom);
}
+#ifdef CONFIG_PM
static int wacom_resume(struct hid_device *hdev)
{
struct wacom *wacom = hid_get_drvdata(hdev);
@@ -1436,6 +1437,7 @@ static int wacom_reset_resume(struct hid_device *hdev)
{
return wacom_resume(hdev);
}
+#endif /* CONFIG_PM */
static struct hid_driver wacom_driver = {
.name = "wacom",
--
1.9.1
^ permalink raw reply related
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