* Re: [PATCH] Input: synaptics - add support for ForcePads
From: Dmitry Torokhov @ 2014-10-30 17:35 UTC (permalink / raw)
To: Peter Hutterer
Cc: Andrew Duggan, Hans de Goede, linux-input, Benjamin Tissoires,
Christopher Heiny, linux-kernel
In-Reply-To: <20141030003855.GA15677@jelly.redhat.com>
On Thu, Oct 30, 2014 at 10:38:55AM +1000, Peter Hutterer wrote:
> On Tue, Sep 09, 2014 at 03:57:25PM -0700, Dmitry Torokhov wrote:
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >
> > ForcePads are found on HP EliteBook 1040 laptops. They lack any kind of
> > physical buttons, instead they generate primary button click when user
> > presses somewhat hard on the surface of the touchpad. Unfortunately they
> > also report primary button click whenever there are 2 or more contacts
> > on the pad, messing up all multi-finger gestures (2-finger scrolling,
> > multi-finger tapping, etc). To cope with this behavior we introduce a
> > delay (currently 50 msecs) in reporting primary press in case more
> > contacts appear.
>
> can we assign INPUT_PROP_FORCEPAD to this? or PRESSUREPAD or something if
> we're worried about trademarks. This is something I'd like to know in
> userspace, we can then ignore clicks completely and do the rest ourselves.
Hmm, I think we can, but I'd like to wait to see if there are more
models with such touchpads (I sure hope there isn't; lack of tactile
feedback on presses is not the best user experience so I hope this
initiative dies a quick death).
Thanks.
--
Dmitry
^ permalink raw reply
* Re: Problems with Elantech touchpad in 3.18-rc2
From: Dmitry Torokhov @ 2014-10-30 18:03 UTC (permalink / raw)
To: Roel Aaij; +Cc: linux-input, Linus Torvalds
In-Reply-To: <545271E1.30306@gmail.com>
Hi Roel,
On Thu, Oct 30, 2014 at 06:14:09PM +0100, Roel Aaij wrote:
> Dear kernel input developers,
>
> While trying out 3.18-rc2, I noticed that the Elantech touchpad on my
> laptop didn't work as intended anymore. Mouse movement and tapping one
> finger worked, but scrolling and two finger tapping did not. X.org also
> didn't recognise the touchpad as a synaptics touchpad anymore.
>
> After googling around I found that i8042.nomux=0 brought it back to normal.
>
> My laptop is a Clevo model W650SH with a keyboard and a touchpad attached,
> and no touchschreen. Some output of dmesg is listed at the bottom, and the
> result of dmidecode is attached.
Argh, so quick search shows that there might be quite a few clones of
W650SH floating around, and I wonder if any of W650SF/W650SJ/W670SJQ/
W650SR/W650SZ exhibit the same behavior.
We might need to revert the nomux back to the original setting, although
I really do not want to do this: in my opinion it is better to have
touchpad revert to basic mode on affected laptops and work on
re-enabling MUX on them instead of having touchpad/keyboard completely
hosed from the get-go.
Linus, any guidance here? Can we live with such regression?
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH 1/2] Input: synaptics: Use in-kernel tracking for reporting mt data
From: Benjamin Tissoires @ 2014-10-30 18:33 UTC (permalink / raw)
To: Dmitry Torokhov, Henrik Rydberg, Hans de Goede
Cc: Peter Hutterer, linux-input, linux-kernel
The current code tries to consider all states and transitions to properly
detect which finger is attached to which slot. The code is quite huge
and difficult to read.
If the sensor manages to group the touch points but is not reliable in
giving tracking ids, we can simply use the kernel tracking method. Note
that it is already used by Cr-48 Chromebooks.
Incidentaly, this fixes a bug reported by Peter Hutterer:
"""
on the Lenovo T440, run:
evemu-record /dev/input/event4 | grep BTN_
then put one, two, three, two fingers down
when you go from 3 to 2 fingers the driver sends a spurious BTN_TOUCH 0
event:
E: 0.000000 0001 014a 0001 # EV_KEY / BTN_TOUCH 1
E: 0.000000 0001 0145 0001 # EV_KEY / BTN_TOOL_FINGER 1
E: 0.770008 0001 0145 0000 # EV_KEY / BTN_TOOL_FINGER 0
E: 0.770008 0001 014d 0001 # EV_KEY / BTN_TOOL_DOUBLETAP 1
E: 1.924716 0001 014d 0000 # EV_KEY / BTN_TOOL_DOUBLETAP 0
E: 1.924716 0001 014e 0001 # EV_KEY / BTN_TOOL_TRIPLETAP 1
.. changing from 3 to 2 fingers now
E: 3.152641 0001 014a 0000 # EV_KEY / BTN_TOUCH 0
E: 3.152641 0001 014d 0001 # EV_KEY / BTN_TOOL_DOUBLETAP 1
E: 3.152641 0001 014e 0000 # EV_KEY / BTN_TOOL_TRIPLETAP 0
E: 3.176948 0001 014a 0001 # EV_KEY / BTN_TOUCH 1
quick look in the kernel shows it's caused by hw.z going to 0 for a packet,
so probably a firmware bug. either way, it makes it hard to track BTN_TOUCH
as signal that at least one finger is down.
"""
The in-kernel tracking is enough to remove this spurious BTN_TOUCH 0.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
Hi Dmitry,
I started working on that for 2 other bug reports
https://bugs.freedesktop.org/show_bug.cgi?id=81278
and
https://bugs.freedesktop.org/show_bug.cgi?id=76722
I thought the cursor jumps could be fixed by the in-kernel tracking, but the
tracking needs a little bit more work to filter them out (patches to follow soon).
>From a user perspective, this patch does not change anything to what the user
previously had. It also fixes Peter's bug that's why I decide to send this out
by itself (removing ~350 lines of code and fixing bugs is always nice).
I think the cursor jump fixes will need more bikeshedding in input-mt.c (I am
*really* bad at designing APIs), so I'll send it later as an RFC.
Cheers,
Benjamin
drivers/input/mouse/synaptics.c | 397 ++++------------------------------------
drivers/input/mouse/synaptics.h | 18 +-
2 files changed, 34 insertions(+), 381 deletions(-)
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 9031a0a..fd89249 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -569,14 +569,6 @@ static void synaptics_pt_create(struct psmouse *psmouse)
* Functions to interpret the absolute mode packets
****************************************************************************/
-static void synaptics_mt_state_set(struct synaptics_mt_state *state, int count,
- int sgm, int agm)
-{
- state->count = count;
- state->sgm = sgm;
- state->agm = agm;
-}
-
static void synaptics_parse_agm(const unsigned char buf[],
struct synaptics_data *priv,
struct synaptics_hw_state *hw)
@@ -595,16 +587,13 @@ static void synaptics_parse_agm(const unsigned char buf[],
break;
case 2:
- /* AGM-CONTACT packet: (count, sgm, agm) */
- synaptics_mt_state_set(&agm->mt_state, buf[1], buf[2], buf[4]);
+ /* AGM-CONTACT packet: we are only interested in the count */
+ priv->agm_count = buf[1];
break;
default:
break;
}
-
- /* Record that at least one AGM has been received since last SGM */
- priv->agm_pending = true;
}
static bool is_forcepad;
@@ -798,388 +787,68 @@ static void synaptics_report_buttons(struct psmouse *psmouse,
input_report_key(dev, BTN_0 + i, hw->ext_buttons & (1 << i));
}
-static void synaptics_report_slot(struct input_dev *dev, int slot,
- const struct synaptics_hw_state *hw)
-{
- input_mt_slot(dev, slot);
- input_mt_report_slot_state(dev, MT_TOOL_FINGER, (hw != NULL));
- if (!hw)
- return;
-
- input_report_abs(dev, ABS_MT_POSITION_X, hw->x);
- input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(hw->y));
- input_report_abs(dev, ABS_MT_PRESSURE, hw->z);
-}
-
static void synaptics_report_mt_data(struct psmouse *psmouse,
- struct synaptics_mt_state *mt_state,
- const struct synaptics_hw_state *sgm)
+ const struct synaptics_hw_state *sgm,
+ int num_fingers)
{
struct input_dev *dev = psmouse->dev;
struct synaptics_data *priv = psmouse->private;
- struct synaptics_hw_state *agm = &priv->agm;
- struct synaptics_mt_state *old = &priv->mt_state;
+ const struct synaptics_hw_state *hw[2] = { sgm, &priv->agm };
+ struct input_mt_pos pos[2];
+ int slot[2], nsemi, i;
- switch (mt_state->count) {
- case 0:
- synaptics_report_slot(dev, 0, NULL);
- synaptics_report_slot(dev, 1, NULL);
- break;
- case 1:
- if (mt_state->sgm == -1) {
- synaptics_report_slot(dev, 0, NULL);
- synaptics_report_slot(dev, 1, NULL);
- } else if (mt_state->sgm == 0) {
- synaptics_report_slot(dev, 0, sgm);
- synaptics_report_slot(dev, 1, NULL);
- } else {
- synaptics_report_slot(dev, 0, NULL);
- synaptics_report_slot(dev, 1, sgm);
- }
- break;
- default:
- /*
- * If the finger slot contained in SGM is valid, and either
- * hasn't changed, or is new, or the old SGM has now moved to
- * AGM, then report SGM in MTB slot 0.
- * Otherwise, empty MTB slot 0.
- */
- if (mt_state->sgm != -1 &&
- (mt_state->sgm == old->sgm ||
- old->sgm == -1 || mt_state->agm == old->sgm))
- synaptics_report_slot(dev, 0, sgm);
- else
- synaptics_report_slot(dev, 0, NULL);
+ nsemi = clamp_val(num_fingers, 0, 2);
- /*
- * If the finger slot contained in AGM is valid, and either
- * hasn't changed, or is new, then report AGM in MTB slot 1.
- * Otherwise, empty MTB slot 1.
- *
- * However, in the case where the AGM is new, make sure that
- * that it is either the same as the old SGM, or there was no
- * SGM.
- *
- * Otherwise, if the SGM was just 1, and the new AGM is 2, then
- * the new AGM will keep the old SGM's tracking ID, which can
- * cause apparent drumroll. This happens if in the following
- * valid finger sequence:
- *
- * Action SGM AGM (MTB slot:Contact)
- * 1. Touch contact 0 (0:0)
- * 2. Touch contact 1 (0:0, 1:1)
- * 3. Lift contact 0 (1:1)
- * 4. Touch contacts 2,3 (0:2, 1:3)
- *
- * In step 4, contact 3, in AGM must not be given the same
- * tracking ID as contact 1 had in step 3. To avoid this,
- * the first agm with contact 3 is dropped and slot 1 is
- * invalidated (tracking ID = -1).
- */
- if (mt_state->agm != -1 &&
- (mt_state->agm == old->agm ||
- (old->agm == -1 &&
- (old->sgm == -1 || mt_state->agm == old->sgm))))
- synaptics_report_slot(dev, 1, agm);
- else
- synaptics_report_slot(dev, 1, NULL);
- break;
+ for (i = 0; i < nsemi; i++) {
+ pos[i].x = hw[i]->x;
+ pos[i].y = synaptics_invert_y(hw[i]->y);
}
+ input_mt_assign_slots(dev, slot, pos, nsemi);
+
+ for (i = 0; i < nsemi; i++) {
+ input_mt_slot(dev, slot[i]);
+ input_mt_report_slot_state(dev, MT_TOOL_FINGER, true);
+ input_report_abs(dev, ABS_MT_POSITION_X, pos[i].x);
+ input_report_abs(dev, ABS_MT_POSITION_Y, pos[i].y);
+ input_report_abs(dev, ABS_MT_PRESSURE, hw[i]->z);
+ }
+
+ input_mt_drop_unused(dev);
+
/* Don't use active slot count to generate BTN_TOOL events. */
input_mt_report_pointer_emulation(dev, false);
/* Send the number of fingers reported by touchpad itself. */
- input_mt_report_finger_count(dev, mt_state->count);
+ input_mt_report_finger_count(dev, num_fingers);
synaptics_report_buttons(psmouse, sgm);
input_sync(dev);
}
-/* Handle case where mt_state->count = 0 */
-static void synaptics_image_sensor_0f(struct synaptics_data *priv,
- struct synaptics_mt_state *mt_state)
-{
- synaptics_mt_state_set(mt_state, 0, -1, -1);
- priv->mt_state_lost = false;
-}
-
-/* Handle case where mt_state->count = 1 */
-static void synaptics_image_sensor_1f(struct synaptics_data *priv,
- struct synaptics_mt_state *mt_state)
-{
- struct synaptics_hw_state *agm = &priv->agm;
- struct synaptics_mt_state *old = &priv->mt_state;
-
- /*
- * If the last AGM was (0,0,0), and there is only one finger left,
- * then we absolutely know that SGM contains slot 0, and all other
- * fingers have been removed.
- */
- if (priv->agm_pending && agm->z == 0) {
- synaptics_mt_state_set(mt_state, 1, 0, -1);
- priv->mt_state_lost = false;
- return;
- }
-
- switch (old->count) {
- case 0:
- synaptics_mt_state_set(mt_state, 1, 0, -1);
- break;
- case 1:
- /*
- * If mt_state_lost, then the previous transition was 3->1,
- * and SGM now contains either slot 0 or 1, but we don't know
- * which. So, we just assume that the SGM now contains slot 1.
- *
- * If pending AGM and either:
- * (a) the previous SGM slot contains slot 0, or
- * (b) there was no SGM slot
- * then, the SGM now contains slot 1
- *
- * Case (a) happens with very rapid "drum roll" gestures, where
- * slot 0 finger is lifted and a new slot 1 finger touches
- * within one reporting interval.
- *
- * Case (b) happens if initially two or more fingers tap
- * briefly, and all but one lift before the end of the first
- * reporting interval.
- *
- * (In both these cases, slot 0 will becomes empty, so SGM
- * contains slot 1 with the new finger)
- *
- * Else, if there was no previous SGM, it now contains slot 0.
- *
- * Otherwise, SGM still contains the same slot.
- */
- if (priv->mt_state_lost ||
- (priv->agm_pending && old->sgm <= 0))
- synaptics_mt_state_set(mt_state, 1, 1, -1);
- else if (old->sgm == -1)
- synaptics_mt_state_set(mt_state, 1, 0, -1);
- break;
- case 2:
- /*
- * If mt_state_lost, we don't know which finger SGM contains.
- *
- * So, report 1 finger, but with both slots empty.
- * We will use slot 1 on subsequent 1->1
- */
- if (priv->mt_state_lost) {
- synaptics_mt_state_set(mt_state, 1, -1, -1);
- break;
- }
- /*
- * Since the last AGM was NOT (0,0,0), it was the finger in
- * slot 0 that has been removed.
- * So, SGM now contains previous AGM's slot, and AGM is now
- * empty.
- */
- synaptics_mt_state_set(mt_state, 1, old->agm, -1);
- break;
- case 3:
- /*
- * Since last AGM was not (0,0,0), we don't know which finger
- * is left.
- *
- * So, report 1 finger, but with both slots empty.
- * We will use slot 1 on subsequent 1->1
- */
- synaptics_mt_state_set(mt_state, 1, -1, -1);
- priv->mt_state_lost = true;
- break;
- case 4:
- case 5:
- /* mt_state was updated by AGM-CONTACT packet */
- break;
- }
-}
-
-/* Handle case where mt_state->count = 2 */
-static void synaptics_image_sensor_2f(struct synaptics_data *priv,
- struct synaptics_mt_state *mt_state)
-{
- struct synaptics_mt_state *old = &priv->mt_state;
-
- switch (old->count) {
- case 0:
- synaptics_mt_state_set(mt_state, 2, 0, 1);
- break;
- case 1:
- /*
- * If previous SGM contained slot 1 or higher, SGM now contains
- * slot 0 (the newly touching finger) and AGM contains SGM's
- * previous slot.
- *
- * Otherwise, SGM still contains slot 0 and AGM now contains
- * slot 1.
- */
- if (old->sgm >= 1)
- synaptics_mt_state_set(mt_state, 2, 0, old->sgm);
- else
- synaptics_mt_state_set(mt_state, 2, 0, 1);
- break;
- case 2:
- /*
- * If mt_state_lost, SGM now contains either finger 1 or 2, but
- * we don't know which.
- * So, we just assume that the SGM contains slot 0 and AGM 1.
- */
- if (priv->mt_state_lost)
- synaptics_mt_state_set(mt_state, 2, 0, 1);
- /*
- * Otherwise, use the same mt_state, since it either hasn't
- * changed, or was updated by a recently received AGM-CONTACT
- * packet.
- */
- break;
- case 3:
- /*
- * 3->2 transitions have two unsolvable problems:
- * 1) no indication is given which finger was removed
- * 2) no way to tell if agm packet was for finger 3
- * before 3->2, or finger 2 after 3->2.
- *
- * So, report 2 fingers, but empty all slots.
- * We will guess slots [0,1] on subsequent 2->2.
- */
- synaptics_mt_state_set(mt_state, 2, -1, -1);
- priv->mt_state_lost = true;
- break;
- case 4:
- case 5:
- /* mt_state was updated by AGM-CONTACT packet */
- break;
- }
-}
-
-/* Handle case where mt_state->count = 3 */
-static void synaptics_image_sensor_3f(struct synaptics_data *priv,
- struct synaptics_mt_state *mt_state)
-{
- struct synaptics_mt_state *old = &priv->mt_state;
-
- switch (old->count) {
- case 0:
- synaptics_mt_state_set(mt_state, 3, 0, 2);
- break;
- case 1:
- /*
- * If previous SGM contained slot 2 or higher, SGM now contains
- * slot 0 (one of the newly touching fingers) and AGM contains
- * SGM's previous slot.
- *
- * Otherwise, SGM now contains slot 0 and AGM contains slot 2.
- */
- if (old->sgm >= 2)
- synaptics_mt_state_set(mt_state, 3, 0, old->sgm);
- else
- synaptics_mt_state_set(mt_state, 3, 0, 2);
- break;
- case 2:
- /*
- * If the AGM previously contained slot 3 or higher, then the
- * newly touching finger is in the lowest available slot.
- *
- * If SGM was previously 1 or higher, then the new SGM is
- * now slot 0 (with a new finger), otherwise, the new finger
- * is now in a hidden slot between 0 and AGM's slot.
- *
- * In all such cases, the SGM now contains slot 0, and the AGM
- * continues to contain the same slot as before.
- */
- if (old->agm >= 3) {
- synaptics_mt_state_set(mt_state, 3, 0, old->agm);
- break;
- }
-
- /*
- * After some 3->1 and all 3->2 transitions, we lose track
- * of which slot is reported by SGM and AGM.
- *
- * For 2->3 in this state, report 3 fingers, but empty all
- * slots, and we will guess (0,2) on a subsequent 0->3.
- *
- * To userspace, the resulting transition will look like:
- * 2:[0,1] -> 3:[-1,-1] -> 3:[0,2]
- */
- if (priv->mt_state_lost) {
- synaptics_mt_state_set(mt_state, 3, -1, -1);
- break;
- }
-
- /*
- * If the (SGM,AGM) really previously contained slots (0, 1),
- * then we cannot know what slot was just reported by the AGM,
- * because the 2->3 transition can occur either before or after
- * the AGM packet. Thus, this most recent AGM could contain
- * either the same old slot 1 or the new slot 2.
- * Subsequent AGMs will be reporting slot 2.
- *
- * To userspace, the resulting transition will look like:
- * 2:[0,1] -> 3:[0,-1] -> 3:[0,2]
- */
- synaptics_mt_state_set(mt_state, 3, 0, -1);
- break;
- case 3:
- /*
- * If, for whatever reason, the previous agm was invalid,
- * Assume SGM now contains slot 0, AGM now contains slot 2.
- */
- if (old->agm <= 2)
- synaptics_mt_state_set(mt_state, 3, 0, 2);
- /*
- * mt_state either hasn't changed, or was updated by a recently
- * received AGM-CONTACT packet.
- */
- break;
-
- case 4:
- case 5:
- /* mt_state was updated by AGM-CONTACT packet */
- break;
- }
-}
-
-/* Handle case where mt_state->count = 4, or = 5 */
-static void synaptics_image_sensor_45f(struct synaptics_data *priv,
- struct synaptics_mt_state *mt_state)
-{
- /* mt_state was updated correctly by AGM-CONTACT packet */
- priv->mt_state_lost = false;
-}
-
static void synaptics_image_sensor_process(struct psmouse *psmouse,
struct synaptics_hw_state *sgm)
{
struct synaptics_data *priv = psmouse->private;
- struct synaptics_hw_state *agm = &priv->agm;
- struct synaptics_mt_state mt_state;
-
- /* Initialize using current mt_state (as updated by last agm) */
- mt_state = agm->mt_state;
+ int num_fingers;
/*
* Update mt_state using the new finger count and current mt_state.
*/
if (sgm->z == 0)
- synaptics_image_sensor_0f(priv, &mt_state);
+ num_fingers = 0;
else if (sgm->w >= 4)
- synaptics_image_sensor_1f(priv, &mt_state);
+ num_fingers = 1;
else if (sgm->w == 0)
- synaptics_image_sensor_2f(priv, &mt_state);
- else if (sgm->w == 1 && mt_state.count <= 3)
- synaptics_image_sensor_3f(priv, &mt_state);
+ num_fingers = 2;
+ else if (sgm->w == 1)
+ num_fingers = priv->agm_count ? priv->agm_count : 3;
else
- synaptics_image_sensor_45f(priv, &mt_state);
+ num_fingers = 4;
/* Send resulting input events to user space */
- synaptics_report_mt_data(psmouse, &mt_state, sgm);
-
- /* Store updated mt_state */
- priv->mt_state = agm->mt_state = mt_state;
- priv->agm_pending = false;
+ synaptics_report_mt_data(psmouse, sgm, num_fingers);
}
static void synaptics_profile_sensor_process(struct psmouse *psmouse,
@@ -1439,7 +1108,7 @@ static void set_input_params(struct psmouse *psmouse,
ABS_MT_POSITION_Y);
/* Image sensors can report per-contact pressure */
input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
- input_mt_init_slots(dev, 2, INPUT_MT_POINTER);
+ input_mt_init_slots(dev, 2, INPUT_MT_POINTER | INPUT_MT_TRACK);
/* Image sensors can signal 4 and 5 finger clicks */
__set_bit(BTN_TOOL_QUADTAP, dev->keybit);
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 1bd01f2..6faf9bb 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -119,16 +119,6 @@
#define SYN_REDUCED_FILTER_FUZZ 8
/*
- * A structure to describe which internal touchpad finger slots are being
- * reported in raw packets.
- */
-struct synaptics_mt_state {
- int count; /* num fingers being tracked */
- int sgm; /* which slot is reported by sgm pkt */
- int agm; /* which slot is reported by agm pkt*/
-};
-
-/*
* A structure to describe the state of the touchpad hardware (buttons and pad)
*/
struct synaptics_hw_state {
@@ -143,9 +133,6 @@ struct synaptics_hw_state {
unsigned int down:1;
unsigned char ext_buttons;
signed char scroll;
-
- /* As reported in last AGM-CONTACT packets */
- struct synaptics_mt_state mt_state;
};
struct synaptics_data {
@@ -170,15 +157,12 @@ struct synaptics_data {
struct serio *pt_port; /* Pass-through serio port */
- struct synaptics_mt_state mt_state; /* Current mt finger state */
- bool mt_state_lost; /* mt_state may be incorrect */
-
/*
* Last received Advanced Gesture Mode (AGM) packet. An AGM packet
* contains position data for a second contact, at half resolution.
*/
struct synaptics_hw_state agm;
- bool agm_pending; /* new AGM packet received */
+ unsigned int agm_count; /* finger count reported by agm */
/* ForcePad handling */
unsigned long press_start;
--
2.1.0
^ permalink raw reply related
* [PATCH 2/2] Input: synaptics: remove duplicated code
From: Benjamin Tissoires @ 2014-10-30 18:33 UTC (permalink / raw)
To: Dmitry Torokhov, Henrik Rydberg, Hans de Goede
Cc: Peter Hutterer, linux-input, linux-kernel
In-Reply-To: <1414693987-6059-1-git-send-email-benjamin.tissoires@redhat.com>
synaptics_profile_sensor_process() and synaptics_report_mt_data() now
share the exact same code. Remove one implementation and rely on the
other where it was used.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/input/mouse/synaptics.c | 38 +-------------------------------------
1 file changed, 1 insertion(+), 37 deletions(-)
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index fd89249..8ae1841 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -851,42 +851,6 @@ static void synaptics_image_sensor_process(struct psmouse *psmouse,
synaptics_report_mt_data(psmouse, sgm, num_fingers);
}
-static void synaptics_profile_sensor_process(struct psmouse *psmouse,
- struct synaptics_hw_state *sgm,
- int num_fingers)
-{
- struct input_dev *dev = psmouse->dev;
- struct synaptics_data *priv = psmouse->private;
- struct synaptics_hw_state *hw[2] = { sgm, &priv->agm };
- struct input_mt_pos pos[2];
- int slot[2], nsemi, i;
-
- nsemi = clamp_val(num_fingers, 0, 2);
-
- for (i = 0; i < nsemi; i++) {
- pos[i].x = hw[i]->x;
- pos[i].y = synaptics_invert_y(hw[i]->y);
- }
-
- input_mt_assign_slots(dev, slot, pos, nsemi);
-
- for (i = 0; i < nsemi; i++) {
- input_mt_slot(dev, slot[i]);
- input_mt_report_slot_state(dev, MT_TOOL_FINGER, true);
- input_report_abs(dev, ABS_MT_POSITION_X, pos[i].x);
- input_report_abs(dev, ABS_MT_POSITION_Y, pos[i].y);
- input_report_abs(dev, ABS_MT_PRESSURE, hw[i]->z);
- }
-
- input_mt_drop_unused(dev);
- input_mt_report_pointer_emulation(dev, false);
- input_mt_report_finger_count(dev, num_fingers);
-
- synaptics_report_buttons(psmouse, sgm);
-
- input_sync(dev);
-}
-
/*
* called for each full received packet from the touchpad
*/
@@ -951,7 +915,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
}
if (cr48_profile_sensor) {
- synaptics_profile_sensor_process(psmouse, &hw, num_fingers);
+ synaptics_report_mt_data(psmouse, &hw, num_fingers);
return;
}
--
2.1.0
^ permalink raw reply related
* [PATCH v3] psmouse: Add some support for the FocalTech PS/2 protocol extensions.
From: Mathias Gottschlag @ 2014-10-30 18:33 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Hans de Goede, Benjamin Tissoires
In-Reply-To: <54523224.8090905@gmail.com>
Most of the protocol for these touchpads has been reverse engineered. This
commit adds a basic multitouch-capable driver.
A lot of the protocol is still unknown. Especially, we don't know how to
identify the device yet apart from the PNP ID.
The previous workaround for these devices has been left in place in case
the driver is not compiled into the kernel or in case some other device
with the same PNP ID is not recognized by the driver yet still has the
same
problems with the device probing code.
---
(Sorry, some serious incompetence caused me to always test an old
version of the module, so I overlooked an embarrasing pagefault right
during initialization, where memory allocation did not happen early
enough)
Thanks for the first round of review, I hope I addressed all comments.
Changes compared to the last version:
- The detection code does not compare all registers anymore.
- The driver now reads the size of the touchpad during initialization.
This should add support for Asus X450JN, where the touchpad is a bit
wider.
- set_input_params has been simplified.
- fingers are now valid=0 when the touchpad reports a large object.
drivers/input/mouse/Kconfig | 10 ++
drivers/input/mouse/focaltech.c | 300
++++++++++++++++++++++++++++++++++++-
drivers/input/mouse/focaltech.h | 60 ++++++++
drivers/input/mouse/psmouse-base.c | 32 ++--
drivers/input/mouse/psmouse.h | 1 +
5 files changed, 386 insertions(+), 17 deletions(-)
diff --git a/drivers/input/mouse/Kconfig b/drivers/input/mouse/Kconfig
index 366fc7a..db973e5 100644
--- a/drivers/input/mouse/Kconfig
+++ b/drivers/input/mouse/Kconfig
@@ -146,6 +146,16 @@ config MOUSE_PS2_OLPC
If unsure, say N.
+config MOUSE_PS2_FOCALTECH
+ bool "FocalTech PS/2 mouse protocol extension" if EXPERT
+ default y
+ depends on MOUSE_PS2
+ help
+ Say Y here if you have a FocalTech PS/2 TouchPad connected to
+ your system.
+
+ If unsure, say Y.
+
config MOUSE_SERIAL
tristate "Serial mouse"
select SERIO
diff --git a/drivers/input/mouse/focaltech.c
b/drivers/input/mouse/focaltech.c
index f4d657e..26bc5b7 100644
--- a/drivers/input/mouse/focaltech.c
+++ b/drivers/input/mouse/focaltech.c
@@ -2,6 +2,7 @@
* Focaltech TouchPad PS/2 mouse driver
*
* Copyright (c) 2014 Red Hat Inc.
+ * Copyright (c) 2014 Mathias Gottschlag <mgottschlag@gmail.com>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -13,15 +14,14 @@
* Hans de Goede <hdegoede@redhat.com>
*/
-/*
- * The Focaltech PS/2 touchpad protocol is unknown. This drivers
deals with
- * detection only, to avoid further detection attempts confusing the
touchpad
- * this way it at least works in PS/2 mouse compatibility mode.
- */
#include <linux/device.h>
#include <linux/libps2.h>
+#include <linux/input/mt.h>
+#include <linux/serio.h>
+#include <linux/slab.h>
#include "psmouse.h"
+#include "focaltech.h"
static const char * const focaltech_pnp_ids[] = {
"FLT0101",
@@ -30,6 +30,12 @@ static const char * const focaltech_pnp_ids[] = {
NULL
};
+/*
+ * Even if the kernel is built without support for Focaltech PS/2
touchpads (or
+ * when the real driver fails to recognize the device), we still have
to detect
+ * them in order to avoid further detection attempts confusing the
touchpad.
+ * This way it at least works in PS/2 mouse compatibility mode.
+ */
int focaltech_detect(struct psmouse *psmouse, bool set_properties)
{
if (!psmouse_matches_pnp_id(psmouse, focaltech_pnp_ids))
@@ -37,16 +43,296 @@ int focaltech_detect(struct psmouse *psmouse,
bool set_properties)
if (set_properties) {
psmouse->vendor = "FocalTech";
- psmouse->name = "FocalTech Touchpad in mouse emulation mode";
+ psmouse->name = "FocalTech Touchpad";
}
return 0;
}
-int focaltech_init(struct psmouse *psmouse)
+static void focaltech_reset(struct psmouse *psmouse)
{
ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_RESET_DIS);
psmouse_reset(psmouse);
+}
+
+#ifdef CONFIG_MOUSE_PS2_FOCALTECH
+
+static void focaltech_report_state(struct psmouse *psmouse)
+{
+ int i;
+ struct focaltech_data *priv = psmouse->private;
+ struct focaltech_hw_state *state = &priv->state;
+ struct input_dev *dev = psmouse->dev;
+ int finger_count = 0;
+
+ for (i = 0; i < FOC_MAX_FINGERS; i++) {
+ struct focaltech_finger_state *finger = &state->fingers[i];
+ int active = finger->active && finger->valid;
+ input_mt_slot(dev, i);
+ input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
+ if (active) {
+ finger_count++;
+ input_report_abs(dev, ABS_MT_POSITION_X, finger->x);
+ input_report_abs(dev, ABS_MT_POSITION_Y,
+ focaltech_invert_y(finger->y));
+ }
+ }
+ input_mt_report_pointer_emulation(dev, finger_count);
+
+ input_report_key(psmouse->dev, BTN_LEFT, state->pressed);
+ input_sync(psmouse->dev);
+}
+
+static void process_touch_packet(struct focaltech_hw_state *state,
+ unsigned char *packet)
+{
+ int i;
+ unsigned char fingers = packet[1];
+
+ state->pressed = (packet[0] >> 4) & 1;
+ /* the second byte contains a bitmap of all fingers touching the pad */
+ for (i = 0; i < FOC_MAX_FINGERS; i++) {
+ if ((fingers & 0x1) && !state->fingers[i].active) {
+ /* we do not have a valid position for the finger yet */
+ state->fingers[i].valid = 0;
+ }
+ state->fingers[i].active = fingers & 0x1;
+ fingers >>= 1;
+ }
+}
+
+static void process_abs_packet(struct focaltech_hw_state *state,
+ unsigned char *packet)
+{
+ unsigned int finger = (packet[1] >> 4) - 1;
+
+ state->pressed = (packet[0] >> 4) & 1;
+ if (finger >= FOC_MAX_FINGERS)
+ return;
+ /*
+ * packet[5] contains some kind of tool size in the most significant
+ * nibble. 0xff is a special value (latching) that signals a large
+ * contact area.
+ */
+ if (packet[5] == 0xff) {
+ state->fingers[finger].valid = 0;
+ return;
+ }
+ state->fingers[finger].x = ((packet[1] & 0xf) << 8) | packet[2];
+ state->fingers[finger].y = (packet[3] << 8) | packet[4];
+ state->fingers[finger].valid = 1;
+}
+static void process_rel_packet(struct focaltech_hw_state *state,
+ unsigned char *packet)
+{
+ int finger1 = ((packet[0] >> 4) & 0x7) - 1;
+ int finger2 = ((packet[3] >> 4) & 0x7) - 1;
+
+ state->pressed = packet[0] >> 7;
+ if (finger1 < FOC_MAX_FINGERS) {
+ state->fingers[finger1].x += (char)packet[1];
+ state->fingers[finger1].y += (char)packet[2];
+ }
+ /*
+ * If there is an odd number of fingers, the last relative packet only
+ * contains one finger. In this case, the second finger index in the
+ * packet is 0 (we subtract 1 in the lines above to create array
+ * indices).
+ */
+ if (finger2 != -1 && finger2 < FOC_MAX_FINGERS) {
+ state->fingers[finger2].x += (char)packet[4];
+ state->fingers[finger2].y += (char)packet[5];
+ }
+}
+
+static void focaltech_process_packet(struct psmouse *psmouse)
+{
+ struct focaltech_data *priv = psmouse->private;
+ unsigned char *packet = psmouse->packet;
+
+ switch (packet[0] & 0xf) {
+ case FOC_TOUCH:
+ process_touch_packet(&priv->state, packet);
+ break;
+ case FOC_ABS:
+ process_abs_packet(&priv->state, packet);
+ break;
+ case FOC_REL:
+ process_rel_packet(&priv->state, packet);
+ break;
+ default:
+ psmouse_err(psmouse, "Unknown packet type: %02x", packet[0]);
+ break;
+ }
+
+ focaltech_report_state(psmouse);
+}
+
+static psmouse_ret_t focaltech_process_byte(struct psmouse *psmouse)
+{
+ if (psmouse->pktcnt >= 6) { /* Full packet received */
+ focaltech_process_packet(psmouse);
+ return PSMOUSE_FULL_PACKET;
+ }
+ /*
+ * we might want to do some validation of the data here, but we do not
+ * know the protocoll well enough
+ */
+ return PSMOUSE_GOOD_DATA;
+}
+
+static int focaltech_switch_protocol(struct psmouse *psmouse)
+{
+ struct ps2dev *ps2dev = &psmouse->ps2dev;
+ unsigned char param[3];
+
+ param[0] = 0;
+ if (ps2_command(ps2dev, param, 0x10f8))
+ return -EIO;
+ if (ps2_command(ps2dev, param, 0x10f8))
+ return -EIO;
+ if (ps2_command(ps2dev, param, 0x10f8))
+ return -EIO;
+ param[0] = 1;
+ if (ps2_command(ps2dev, param, 0x10f8))
+ return -EIO;
+ if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETSCALE11))
+ return -EIO;
+
+ if (ps2_command(ps2dev, param, PSMOUSE_CMD_ENABLE))
+ return -EIO;
return 0;
}
+
+static void focaltech_disconnect(struct psmouse *psmouse)
+{
+ focaltech_reset(psmouse);
+ kfree(psmouse->private);
+ psmouse->private = NULL;
+}
+
+static int focaltech_reconnect(struct psmouse *psmouse)
+{
+ focaltech_reset(psmouse);
+ if (focaltech_switch_protocol(psmouse)) {
+ psmouse_err(psmouse,
+ "Unable to initialize the device.");
+ return -1;
+ }
+ return 0;
+}
+
+static void set_input_params(struct psmouse *psmouse)
+{
+ struct input_dev *dev = psmouse->dev;
+ struct focaltech_data *priv = psmouse->private;
+
+ __set_bit(EV_ABS, dev->evbit);
+ input_set_abs_params(dev, ABS_MT_POSITION_X, 0, priv->x_max, 0, 0);
+ input_set_abs_params(dev, ABS_MT_POSITION_Y, 0, priv->y_max, 0, 0);
+ input_mt_init_slots(dev, 5, INPUT_MT_POINTER);
+ __clear_bit(EV_REL, dev->evbit);
+ __clear_bit(REL_X, dev->relbit);
+ __clear_bit(REL_Y, dev->relbit);
+ __clear_bit(BTN_RIGHT, dev->keybit);
+ __clear_bit(BTN_MIDDLE, dev->keybit);
+ __set_bit(INPUT_PROP_BUTTONPAD, dev->propbit);
+}
+
+static int focaltech_read_register(struct ps2dev *ps2dev, int reg,
+ unsigned char *param)
+{
+ if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETSCALE11))
+ return -1;
+ param[0] = 0;
+ if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES))
+ return -1;
+ if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES))
+ return -1;
+ if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES))
+ return -1;
+ param[0] = reg;
+ if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES))
+ return -1;
+ if (ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO))
+ return -1;
+ return 0;
+}
+
+static int focaltech_read_size(struct psmouse *psmouse)
+{
+ struct ps2dev *ps2dev = &psmouse->ps2dev;
+ struct focaltech_data *priv = psmouse->private;
+ char param[3];
+
+ if (focaltech_read_register(ps2dev, 2, param))
+ return -EIO;
+ /* not sure whether this is 100% correct */
+ priv->x_max = (unsigned char)param[1] * 128;
+ priv->y_max = (unsigned char)param[2] * 128;
+
+ return 0;
+}
+int focaltech_init(struct psmouse *psmouse)
+{
+ struct focaltech_data *priv;
+ int err;
+
+ psmouse->private = priv = kzalloc(sizeof(struct focaltech_data),
GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ focaltech_reset(psmouse);
+ if (focaltech_read_size(psmouse)) {
+ focaltech_reset(psmouse);
+ psmouse_err(psmouse,
+ "Unable to read the size of the touchpad.");
+ err = -ENOSYS;
+ goto fail;
+ }
+ if (focaltech_switch_protocol(psmouse)) {
+ focaltech_reset(psmouse);
+ psmouse_err(psmouse,
+ "Unable to initialize the device.");
+ err = -ENOSYS;
+ goto fail;
+ }
+
+ set_input_params(psmouse);
+
+ psmouse->protocol_handler = focaltech_process_byte;
+ psmouse->pktsize = 6;
+ psmouse->disconnect = focaltech_disconnect;
+ psmouse->reconnect = focaltech_reconnect;
+ psmouse->cleanup = focaltech_reset;
+ /* resync is not supported yet */
+ psmouse->resync_time = 0;
+
+ return 0;
+fail:
+ focaltech_reset(psmouse);
+ kfree(priv);
+ return err;
+}
+
+bool focaltech_supported(void)
+{
+ return true;
+}
+
+#else /* CONFIG_MOUSE_PS2_FOCALTECH */
+
+int focaltech_init(struct psmouse *psmouse)
+{
+ focaltech_reset(psmouse);
+
+ return 0;
+}
+
+bool focaltech_supported(void)
+{
+ return false;
+}
+
+#endif /* CONFIG_MOUSE_PS2_FOCALTECH */
diff --git a/drivers/input/mouse/focaltech.h
b/drivers/input/mouse/focaltech.h
index 498650c..68c5cfc 100644
--- a/drivers/input/mouse/focaltech.h
+++ b/drivers/input/mouse/focaltech.h
@@ -2,6 +2,7 @@
* Focaltech TouchPad PS/2 mouse driver
*
* Copyright (c) 2014 Red Hat Inc.
+ * Copyright (c) 2014 Mathias Gottschlag <mgottschlag@gmail.com>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -16,7 +17,66 @@
#ifndef _FOCALTECH_H
#define _FOCALTECH_H
+/*
+ * Packet types - the numbers are not consecutive, so we might be missing
+ * something here.
+ */
+#define FOC_TOUCH 0x3 /* bitmap of active fingers */
+#define FOC_ABS 0x6 /* absolute position of one finger */
+#define FOC_REL 0x9 /* relative position of 1-2 fingers */
+
+#define FOC_MAX_FINGERS 5
+
+#define FOC_MAX_X 2431
+#define FOC_MAX_Y 1663
+
+static inline int focaltech_invert_y(int y)
+{
+ return FOC_MAX_Y - y;
+}
+
+/*
+ * Current state of a single finger on the touchpad.
+ */
+struct focaltech_finger_state {
+ /* the touchpad has generated a touch event for the finger */
+ bool active;
+ /*
+ * The touchpad has sent position data for the finger. Touch event
+ * packages reset this flag for new fingers, and there is a time
+ * between the first touch event and the following absolute position
+ * packet for the finger where the touchpad has declared the finger to
+ * be valid, but we do not have any valid position yet.
+ */
+ bool valid;
+ /* absolute position (from the bottom left corner) of the finger */
+ unsigned int x;
+ unsigned int y;
+};
+
+/*
+ * Description of the current state of the touchpad hardware.
+ */
+struct focaltech_hw_state {
+ /*
+ * The touchpad tracks the positions of the fingers for us, the array
+ * indices correspond to the finger indices returned in the report
+ * packages.
+ */
+ struct focaltech_finger_state fingers[FOC_MAX_FINGERS];
+ /*
+ * True if the clickpad has been pressed.
+ */
+ bool pressed;
+};
+
+struct focaltech_data {
+ unsigned int x_max, y_max;
+ struct focaltech_hw_state state;
+};
+
int focaltech_detect(struct psmouse *psmouse, bool set_properties);
int focaltech_init(struct psmouse *psmouse);
+bool focaltech_supported(void);
#endif
diff --git a/drivers/input/mouse/psmouse-base.c
b/drivers/input/mouse/psmouse-base.c
index 26994f6..4a9de33 100644
--- a/drivers/input/mouse/psmouse-base.c
+++ b/drivers/input/mouse/psmouse-base.c
@@ -725,16 +725,19 @@ static int psmouse_extensions(struct psmouse
*psmouse,
/* Always check for focaltech, this is safe as it uses pnp-id
matching */
if (psmouse_do_detect(focaltech_detect, psmouse, set_properties) == 0) {
- if (!set_properties || focaltech_init(psmouse) == 0) {
- /*
- * Not supported yet, use bare protocol.
- * Note that we need to also restrict
- * psmouse_max_proto so that psmouse_initialize()
- * does not try to reset rate and resolution,
- * because even that upsets the device.
- */
- psmouse_max_proto = PSMOUSE_PS2;
- return PSMOUSE_PS2;
+ if (max_proto > PSMOUSE_IMEX) {
+ if (!set_properties || focaltech_init(psmouse) == 0) {
+ if (focaltech_supported())
+ return PSMOUSE_FOCALTECH;
+ /*
+ * Note that we need to also restrict
+ * psmouse_max_proto so that psmouse_initialize()
+ * does not try to reset rate and resolution,
+ * because even that upsets the device.
+ */
+ psmouse_max_proto = PSMOUSE_PS2;
+ return PSMOUSE_PS2;
+ }
}
}
@@ -1063,6 +1066,15 @@ static const struct psmouse_protocol
psmouse_protocols[] = {
.alias = "cortps",
.detect = cortron_detect,
},
+#ifdef CONFIG_MOUSE_PS2_FOCALTECH
+ {
+ .type = PSMOUSE_FOCALTECH,
+ .name = "FocalTechPS/2",
+ .alias = "focaltech",
+ .detect = focaltech_detect,
+ .init = focaltech_init,
+ },
+#endif
{
.type = PSMOUSE_AUTO,
.name = "auto",
diff --git a/drivers/input/mouse/psmouse.h b/drivers/input/mouse/psmouse.h
index f4cf664..c2ff137 100644
--- a/drivers/input/mouse/psmouse.h
+++ b/drivers/input/mouse/psmouse.h
@@ -96,6 +96,7 @@ enum psmouse_type {
PSMOUSE_FSP,
PSMOUSE_SYNAPTICS_RELATIVE,
PSMOUSE_CYPRESS,
+ PSMOUSE_FOCALTECH,
PSMOUSE_AUTO /* This one should always be last */
};
-- 1.9.1
^ permalink raw reply related
* Re: [PATCH] i8042: Add noloop quirk for Asus X750LN
From: Dmitry Torokhov @ 2014-10-30 18:34 UTC (permalink / raw)
To: Hans de Goede; +Cc: linux-input
In-Reply-To: <20141011183523.GA20881@dtor-ws>
On Sat, Oct 11, 2014 at 11:35:23AM -0700, Dmitry Torokhov wrote:
> On Sat, Oct 11, 2014 at 03:09:06PM +0200, Hans de Goede wrote:
> > Without this the aux port does not get detected, and consequently the touchpad
> > will not work.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1110011
> >
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Applied, thank you.
Hans,
Do you still have dmidecode for this laptop around? Can I see it? I am
just wondering what part of AUX_LOOP was causing issues. Was it aux
delivery test or loop test itself?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: Problems with Elantech touchpad in 3.18-rc2
From: Linus Torvalds @ 2014-10-30 19:06 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Roel Aaij, linux-input@vger.kernel.org
In-Reply-To: <20141030180331.GF36444@dtor-ws>
On Thu, Oct 30, 2014 at 11:03 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Linus, any guidance here? Can we live with such regression?
No. If people are already finding machines with problems, that means
that there will be a lot of them once distributions move to that
kernel.
And I'd much rather maintain an old blacklist of broken machines, than
start from scratch and have a whitelist for machines that want muxing.
So I guess we'll need to revert and go back to the bad old days.
That said, before we do that, maybe we can find a middle ground for
the default behavior. The old behavior is to always try to mux if the
hw seems to support it. The new behavior is to never try to mux by
default. How about "try to mux if the controller seems to support it,
but if you don't actually find any muxed devices behind the
controller, go back to no-mux mode"?
Is there any way to do something like that? I don't actually know how
the heck people set up those touchpads, so maybe I'm just whistling in
the dark, and you can't even tell.
IOW, can we just enumerate the muxed devices, and see if they actually
use anything else than just index zero? Make the default a bit more
dynamic than just "all on" or "all off"?
Put another way: right now we do a lot of checking in
i8042_check_aux(), but we don't do *any* checking of the individual
muxed ports. Could we perhaps do some check per mux port? Do some
PSMOUSE_CMD_ENABLE thing and see if you get anything back?
Am I being crazy again?
Linus
^ permalink raw reply
* Re: Problems with Elantech touchpad in 3.18-rc2
From: Dmitry Torokhov @ 2014-10-30 19:25 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Roel Aaij, linux-input@vger.kernel.org
In-Reply-To: <CA+55aFzkh8NMSPSe7D=sYHPqU3_v3PBUEQrjH7PvFPwkp=ytuA@mail.gmail.com>
On Thu, Oct 30, 2014 at 12:06:03PM -0700, Linus Torvalds wrote:
> On Thu, Oct 30, 2014 at 11:03 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Linus, any guidance here? Can we live with such regression?
>
> No. If people are already finding machines with problems, that means
> that there will be a lot of them once distributions move to that
> kernel.
My only argument here is that failure is much less severe than with MUX
case: when in legacy mode the worst that is happening is that advanced
features (multi-touch, two-finger scroilling, etc) do not work so the
experience is similar to Windows box with no venrod drivers installed.
Whereas when active MUX does not work but we are tying to use it your
device is completely hosed.
>
> And I'd much rather maintain an old blacklist of broken machines, than
> start from scratch and have a whitelist for machines that want muxing.
>
> So I guess we'll need to revert and go back to the bad old days.
>
> That said, before we do that, maybe we can find a middle ground for
> the default behavior. The old behavior is to always try to mux if the
> hw seems to support it. The new behavior is to never try to mux by
> default. How about "try to mux if the controller seems to support it,
> but if you don't actually find any muxed devices behind the
> controller, go back to no-mux mode"?
>
> Is there any way to do something like that? I don't actually know how
> the heck people set up those touchpads, so maybe I'm just whistling in
> the dark, and you can't even tell.
>
> IOW, can we just enumerate the muxed devices, and see if they actually
> use anything else than just index zero? Make the default a bit more
> dynamic than just "all on" or "all off"?
>
> Put another way: right now we do a lot of checking in
> i8042_check_aux(), but we don't do *any* checking of the individual
> muxed ports. Could we perhaps do some check per mux port? Do some
> PSMOUSE_CMD_ENABLE thing and see if you get anything back?
Very often (most often) with broken MUX we do see the device, it simply
does not react properly (refuses to get enabled, fails to activate in
advanced mode, spews garbage into data stream, etc), so I don't think
that putting entire psmouse initialization sequence with all various
protocols into i8042_check_aux() is good idea.
And I have no idea whatsoever how they will behave if you try activating
MUX mode, try using it, and then deactivate. I think that code path is
even less travelled than active MUX one.
--
Dmitry
^ permalink raw reply
* RE: [PATCH] input: add gpio based irq support to Elan touchpad
From: Krishnamoorthy, Jagadish @ 2014-10-30 22:03 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: bleung@chromium.org, dusonlin@emc.com.tw,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20141029005031.GE19675@dtor-ws>
Completely agree to the point that platform data can be modified to put the irq number instead of gpio number.
But with this change I am trying to make the driver more robust and functional irrespective of what is provided on the platform.
-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
Sent: Tuesday, October 28, 2014 5:51 PM
To: Krishnamoorthy, Jagadish
Cc: bleung@chromium.org; dusonlin@emc.com.tw; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] input: add gpio based irq support to Elan touchpad
On Tue, Oct 28, 2014 at 05:27:24PM -0700, jagadish.krishnamoorthy@intel.com wrote:
> From: Jagadish Krishnamoorthy <jagadish.krishnamoorthy@intel.com>
>
> Adding new member 'irq' in main device structure.
> On some of the platforms, gpio is passed in platform data instead of
> irq. Convert the gpio to irq and store it in the driver data.
No, please adjust your platform to set up i2c client data properly.
There is absolutely no reason for the driver to know if it delas with gpio or some other interrupt source.
Thanks.
>
> Signed-off-by: Jagadish Krishnamoorthy
> <jagadish.krishnamoorthy@intel.com>
> ---
> drivers/input/mouse/elan_i2c.h | 3 ++-
> drivers/input/mouse/elan_i2c_core.c | 44 ++++++++++++++++++++++++----------
> drivers/input/mouse/elan_i2c_i2c.c | 7 +++---
> drivers/input/mouse/elan_i2c_smbus.c | 3 ++-
> 4 files changed, 39 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/input/mouse/elan_i2c.h
> b/drivers/input/mouse/elan_i2c.h index 2e83862..bb94a96 100644
> --- a/drivers/input/mouse/elan_i2c.h
> +++ b/drivers/input/mouse/elan_i2c.h
> @@ -76,7 +76,8 @@ struct elan_transport_ops {
> int (*write_fw_block)(struct i2c_client *client,
> const u8 *page, u16 checksum, int idx);
> int (*finish_fw_update)(struct i2c_client *client,
> - struct completion *reset_done);
> + struct completion *reset_done,
> + unsigned int irq);
>
> int (*get_report)(struct i2c_client *client, u8 *report); }; diff
> --git a/drivers/input/mouse/elan_i2c_core.c
> b/drivers/input/mouse/elan_i2c_core.c
> index 0cb2be4..9c5c5d3 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -35,6 +35,7 @@
> #include <linux/completion.h>
> #include <linux/of.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/gpio.h>
> #include <asm/unaligned.h>
>
> #include "elan_i2c.h"
> @@ -75,6 +76,7 @@ struct elan_tp_data {
> unsigned int width_y;
> unsigned int x_res;
> unsigned int y_res;
> + unsigned int irq;
>
> u8 product_id;
> u8 fw_version;
> @@ -331,7 +333,8 @@ static int __elan_update_firmware(struct elan_tp_data *data,
> /* Wait WDT reset and power on reset */
> msleep(600);
>
> - error = data->ops->finish_fw_update(client, &data->fw_completion);
> + error = data->ops->finish_fw_update(client, &data->fw_completion,
> + data->irq);
> if (error)
> return error;
>
> @@ -356,7 +359,7 @@ static int elan_update_firmware(struct
> elan_tp_data *data,
>
> dev_dbg(&client->dev, "Starting firmware update....\n");
>
> - disable_irq(client->irq);
> + disable_irq(data->irq);
> data->in_fw_update = true;
>
> retval = __elan_update_firmware(data, fw); @@ -370,7 +373,7 @@
> static int elan_update_firmware(struct elan_tp_data *data,
> }
>
> data->in_fw_update = false;
> - enable_irq(client->irq);
> + enable_irq(data->irq);
>
> return retval;
> }
> @@ -482,7 +485,7 @@ static ssize_t calibrate_store(struct device *dev,
> if (retval)
> return retval;
>
> - disable_irq(client->irq);
> + disable_irq(data->irq);
>
> data->mode |= ETP_ENABLE_CALIBRATE;
> retval = data->ops->set_mode(client, data->mode); @@ -528,7 +531,7
> @@ out_disable_calibrate:
> retval = error;
> }
> out:
> - enable_irq(client->irq);
> + enable_irq(data->irq);
> mutex_unlock(&data->sysfs_mutex);
> return retval ?: count;
> }
> @@ -594,7 +597,7 @@ static ssize_t acquire_store(struct device *dev, struct device_attribute *attr,
> if (retval)
> return retval;
>
> - disable_irq(client->irq);
> + disable_irq(data->irq);
>
> data->baseline_ready = false;
>
> @@ -636,7 +639,7 @@ out_disable_calibrate:
> retval = error;
> }
> out:
> - enable_irq(client->irq);
> + enable_irq(data->irq);
> mutex_unlock(&data->sysfs_mutex);
> return retval ?: count;
> }
> @@ -900,6 +903,7 @@ static int elan_probe(struct i2c_client *client,
> const struct elan_transport_ops *transport_ops;
> struct device *dev = &client->dev;
> struct elan_tp_data *data;
> + struct gpio_desc *desc;
> unsigned long irqflags;
> int error;
>
> @@ -992,17 +996,31 @@ static int elan_probe(struct i2c_client *client,
> return error;
>
> /*
> + * Check for the irq number in platform data.
> + * If that fails check for gpio based irq.
> + */
> + if (client->irq >= 0) {
> + data->irq = client->irq;
> + } else {
> + desc = devm_gpiod_get_index(&client->dev, "ELAN_GPIO_IRQ", 0);
> + if (IS_ERR(desc) || gpiod_direction_input(desc)) {
> + dev_err(&client->dev, "failed to initialize gpio\n");
> + return error;
> + }
> + data->irq = gpiod_to_irq(desc);
> + }
> + /*
> * Systems using device tree should set up interrupt via DTS,
> * the rest will use the default falling edge interrupts.
> */
> irqflags = client->dev.of_node ? 0 : IRQF_TRIGGER_FALLING;
>
> - error = devm_request_threaded_irq(&client->dev, client->irq,
> + error = devm_request_threaded_irq(&client->dev, data->irq,
> NULL, elan_isr,
> irqflags | IRQF_ONESHOT,
> client->name, data);
> if (error) {
> - dev_err(&client->dev, "cannot register irq=%d\n", client->irq);
> + dev_err(&client->dev, "cannot register irq=%d\n", data->irq);
> return error;
> }
>
> @@ -1055,12 +1073,12 @@ static int __maybe_unused elan_suspend(struct device *dev)
> if (ret)
> return ret;
>
> - disable_irq(client->irq);
> + disable_irq(data->irq);
>
> if (device_may_wakeup(dev)) {
> ret = elan_sleep(data);
> /* Enable wake from IRQ */
> - data->irq_wake = (enable_irq_wake(client->irq) == 0);
> + data->irq_wake = (enable_irq_wake(data->irq) == 0);
> } else {
> ret = elan_disable_power(data);
> }
> @@ -1076,7 +1094,7 @@ static int __maybe_unused elan_resume(struct device *dev)
> int error;
>
> if (device_may_wakeup(dev) && data->irq_wake) {
> - disable_irq_wake(client->irq);
> + disable_irq_wake(data->irq);
> data->irq_wake = false;
> }
>
> @@ -1088,7 +1106,7 @@ static int __maybe_unused elan_resume(struct device *dev)
> if (error)
> dev_err(dev, "initialize when resuming failed: %d\n", error);
>
> - enable_irq(data->client->irq);
> + enable_irq(data->irq);
>
> return 0;
> }
> diff --git a/drivers/input/mouse/elan_i2c_i2c.c
> b/drivers/input/mouse/elan_i2c_i2c.c
> index 97d4937..5e897cd 100644
> --- a/drivers/input/mouse/elan_i2c_i2c.c
> +++ b/drivers/input/mouse/elan_i2c_i2c.c
> @@ -520,7 +520,8 @@ static int elan_i2c_write_fw_block(struct
> i2c_client *client, }
>
> static int elan_i2c_finish_fw_update(struct i2c_client *client,
> - struct completion *completion)
> + struct completion *completion,
> + unsigned int irq)
> {
> struct device *dev = &client->dev;
> long ret;
> @@ -529,13 +530,13 @@ static int elan_i2c_finish_fw_update(struct i2c_client *client,
> u8 buffer[ETP_I2C_INF_LENGTH];
>
> reinit_completion(completion);
> - enable_irq(client->irq);
> + enable_irq(irq);
>
> error = elan_i2c_write_cmd(client, ETP_I2C_STAND_CMD, ETP_I2C_RESET);
> if (!error)
> ret = wait_for_completion_interruptible_timeout(completion,
> msecs_to_jiffies(300));
> - disable_irq(client->irq);
> + disable_irq(irq);
>
> if (error) {
> dev_err(dev, "device reset failed: %d\n", error); diff --git
> a/drivers/input/mouse/elan_i2c_smbus.c
> b/drivers/input/mouse/elan_i2c_smbus.c
> index 359bf85..a81b8fb 100644
> --- a/drivers/input/mouse/elan_i2c_smbus.c
> +++ b/drivers/input/mouse/elan_i2c_smbus.c
> @@ -477,7 +477,8 @@ static int elan_smbus_get_report(struct i2c_client
> *client, u8 *report) }
>
> static int elan_smbus_finish_fw_update(struct i2c_client *client,
> - struct completion *fw_completion)
> + struct completion *fw_completion,
> + unsigned int irq)
> {
> /* No special handling unlike I2C transport */
> return 0;
> --
> 1.7.9.5
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: add gpio based irq support to Elan touchpad
From: Dmitry Torokhov @ 2014-10-30 22:11 UTC (permalink / raw)
To: Krishnamoorthy, Jagadish
Cc: bleung@chromium.org, dusonlin@emc.com.tw,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <26D9772FB34B1342A924594C590213A88001FB@ORSMSX107.amr.corp.intel.com>
On Thu, Oct 30, 2014 at 10:03:18PM +0000, Krishnamoorthy, Jagadish wrote:
> Completely agree to the point that platform data can be modified to put the irq number instead of gpio number.
> But with this change I am trying to make the driver more robust and functional irrespective of what is provided on the platform.
And then you are going to add the similar code to the other 1000s or
i2c, spi, etc driver in the kernel? No, you need to change your platform
to describe the hardware in a way that the core code can understand and
perform the setup as needed?
Thanks.
>
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Tuesday, October 28, 2014 5:51 PM
> To: Krishnamoorthy, Jagadish
> Cc: bleung@chromium.org; dusonlin@emc.com.tw; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] input: add gpio based irq support to Elan touchpad
>
> On Tue, Oct 28, 2014 at 05:27:24PM -0700, jagadish.krishnamoorthy@intel.com wrote:
> > From: Jagadish Krishnamoorthy <jagadish.krishnamoorthy@intel.com>
> >
> > Adding new member 'irq' in main device structure.
> > On some of the platforms, gpio is passed in platform data instead of
> > irq. Convert the gpio to irq and store it in the driver data.
>
> No, please adjust your platform to set up i2c client data properly.
> There is absolutely no reason for the driver to know if it delas with gpio or some other interrupt source.
>
> Thanks.
>
> >
> > Signed-off-by: Jagadish Krishnamoorthy
> > <jagadish.krishnamoorthy@intel.com>
> > ---
> > drivers/input/mouse/elan_i2c.h | 3 ++-
> > drivers/input/mouse/elan_i2c_core.c | 44 ++++++++++++++++++++++++----------
> > drivers/input/mouse/elan_i2c_i2c.c | 7 +++---
> > drivers/input/mouse/elan_i2c_smbus.c | 3 ++-
> > 4 files changed, 39 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/input/mouse/elan_i2c.h
> > b/drivers/input/mouse/elan_i2c.h index 2e83862..bb94a96 100644
> > --- a/drivers/input/mouse/elan_i2c.h
> > +++ b/drivers/input/mouse/elan_i2c.h
> > @@ -76,7 +76,8 @@ struct elan_transport_ops {
> > int (*write_fw_block)(struct i2c_client *client,
> > const u8 *page, u16 checksum, int idx);
> > int (*finish_fw_update)(struct i2c_client *client,
> > - struct completion *reset_done);
> > + struct completion *reset_done,
> > + unsigned int irq);
> >
> > int (*get_report)(struct i2c_client *client, u8 *report); }; diff
> > --git a/drivers/input/mouse/elan_i2c_core.c
> > b/drivers/input/mouse/elan_i2c_core.c
> > index 0cb2be4..9c5c5d3 100644
> > --- a/drivers/input/mouse/elan_i2c_core.c
> > +++ b/drivers/input/mouse/elan_i2c_core.c
> > @@ -35,6 +35,7 @@
> > #include <linux/completion.h>
> > #include <linux/of.h>
> > #include <linux/regulator/consumer.h>
> > +#include <linux/gpio.h>
> > #include <asm/unaligned.h>
> >
> > #include "elan_i2c.h"
> > @@ -75,6 +76,7 @@ struct elan_tp_data {
> > unsigned int width_y;
> > unsigned int x_res;
> > unsigned int y_res;
> > + unsigned int irq;
> >
> > u8 product_id;
> > u8 fw_version;
> > @@ -331,7 +333,8 @@ static int __elan_update_firmware(struct elan_tp_data *data,
> > /* Wait WDT reset and power on reset */
> > msleep(600);
> >
> > - error = data->ops->finish_fw_update(client, &data->fw_completion);
> > + error = data->ops->finish_fw_update(client, &data->fw_completion,
> > + data->irq);
> > if (error)
> > return error;
> >
> > @@ -356,7 +359,7 @@ static int elan_update_firmware(struct
> > elan_tp_data *data,
> >
> > dev_dbg(&client->dev, "Starting firmware update....\n");
> >
> > - disable_irq(client->irq);
> > + disable_irq(data->irq);
> > data->in_fw_update = true;
> >
> > retval = __elan_update_firmware(data, fw); @@ -370,7 +373,7 @@
> > static int elan_update_firmware(struct elan_tp_data *data,
> > }
> >
> > data->in_fw_update = false;
> > - enable_irq(client->irq);
> > + enable_irq(data->irq);
> >
> > return retval;
> > }
> > @@ -482,7 +485,7 @@ static ssize_t calibrate_store(struct device *dev,
> > if (retval)
> > return retval;
> >
> > - disable_irq(client->irq);
> > + disable_irq(data->irq);
> >
> > data->mode |= ETP_ENABLE_CALIBRATE;
> > retval = data->ops->set_mode(client, data->mode); @@ -528,7 +531,7
> > @@ out_disable_calibrate:
> > retval = error;
> > }
> > out:
> > - enable_irq(client->irq);
> > + enable_irq(data->irq);
> > mutex_unlock(&data->sysfs_mutex);
> > return retval ?: count;
> > }
> > @@ -594,7 +597,7 @@ static ssize_t acquire_store(struct device *dev, struct device_attribute *attr,
> > if (retval)
> > return retval;
> >
> > - disable_irq(client->irq);
> > + disable_irq(data->irq);
> >
> > data->baseline_ready = false;
> >
> > @@ -636,7 +639,7 @@ out_disable_calibrate:
> > retval = error;
> > }
> > out:
> > - enable_irq(client->irq);
> > + enable_irq(data->irq);
> > mutex_unlock(&data->sysfs_mutex);
> > return retval ?: count;
> > }
> > @@ -900,6 +903,7 @@ static int elan_probe(struct i2c_client *client,
> > const struct elan_transport_ops *transport_ops;
> > struct device *dev = &client->dev;
> > struct elan_tp_data *data;
> > + struct gpio_desc *desc;
> > unsigned long irqflags;
> > int error;
> >
> > @@ -992,17 +996,31 @@ static int elan_probe(struct i2c_client *client,
> > return error;
> >
> > /*
> > + * Check for the irq number in platform data.
> > + * If that fails check for gpio based irq.
> > + */
> > + if (client->irq >= 0) {
> > + data->irq = client->irq;
> > + } else {
> > + desc = devm_gpiod_get_index(&client->dev, "ELAN_GPIO_IRQ", 0);
> > + if (IS_ERR(desc) || gpiod_direction_input(desc)) {
> > + dev_err(&client->dev, "failed to initialize gpio\n");
> > + return error;
> > + }
> > + data->irq = gpiod_to_irq(desc);
> > + }
> > + /*
> > * Systems using device tree should set up interrupt via DTS,
> > * the rest will use the default falling edge interrupts.
> > */
> > irqflags = client->dev.of_node ? 0 : IRQF_TRIGGER_FALLING;
> >
> > - error = devm_request_threaded_irq(&client->dev, client->irq,
> > + error = devm_request_threaded_irq(&client->dev, data->irq,
> > NULL, elan_isr,
> > irqflags | IRQF_ONESHOT,
> > client->name, data);
> > if (error) {
> > - dev_err(&client->dev, "cannot register irq=%d\n", client->irq);
> > + dev_err(&client->dev, "cannot register irq=%d\n", data->irq);
> > return error;
> > }
> >
> > @@ -1055,12 +1073,12 @@ static int __maybe_unused elan_suspend(struct device *dev)
> > if (ret)
> > return ret;
> >
> > - disable_irq(client->irq);
> > + disable_irq(data->irq);
> >
> > if (device_may_wakeup(dev)) {
> > ret = elan_sleep(data);
> > /* Enable wake from IRQ */
> > - data->irq_wake = (enable_irq_wake(client->irq) == 0);
> > + data->irq_wake = (enable_irq_wake(data->irq) == 0);
> > } else {
> > ret = elan_disable_power(data);
> > }
> > @@ -1076,7 +1094,7 @@ static int __maybe_unused elan_resume(struct device *dev)
> > int error;
> >
> > if (device_may_wakeup(dev) && data->irq_wake) {
> > - disable_irq_wake(client->irq);
> > + disable_irq_wake(data->irq);
> > data->irq_wake = false;
> > }
> >
> > @@ -1088,7 +1106,7 @@ static int __maybe_unused elan_resume(struct device *dev)
> > if (error)
> > dev_err(dev, "initialize when resuming failed: %d\n", error);
> >
> > - enable_irq(data->client->irq);
> > + enable_irq(data->irq);
> >
> > return 0;
> > }
> > diff --git a/drivers/input/mouse/elan_i2c_i2c.c
> > b/drivers/input/mouse/elan_i2c_i2c.c
> > index 97d4937..5e897cd 100644
> > --- a/drivers/input/mouse/elan_i2c_i2c.c
> > +++ b/drivers/input/mouse/elan_i2c_i2c.c
> > @@ -520,7 +520,8 @@ static int elan_i2c_write_fw_block(struct
> > i2c_client *client, }
> >
> > static int elan_i2c_finish_fw_update(struct i2c_client *client,
> > - struct completion *completion)
> > + struct completion *completion,
> > + unsigned int irq)
> > {
> > struct device *dev = &client->dev;
> > long ret;
> > @@ -529,13 +530,13 @@ static int elan_i2c_finish_fw_update(struct i2c_client *client,
> > u8 buffer[ETP_I2C_INF_LENGTH];
> >
> > reinit_completion(completion);
> > - enable_irq(client->irq);
> > + enable_irq(irq);
> >
> > error = elan_i2c_write_cmd(client, ETP_I2C_STAND_CMD, ETP_I2C_RESET);
> > if (!error)
> > ret = wait_for_completion_interruptible_timeout(completion,
> > msecs_to_jiffies(300));
> > - disable_irq(client->irq);
> > + disable_irq(irq);
> >
> > if (error) {
> > dev_err(dev, "device reset failed: %d\n", error); diff --git
> > a/drivers/input/mouse/elan_i2c_smbus.c
> > b/drivers/input/mouse/elan_i2c_smbus.c
> > index 359bf85..a81b8fb 100644
> > --- a/drivers/input/mouse/elan_i2c_smbus.c
> > +++ b/drivers/input/mouse/elan_i2c_smbus.c
> > @@ -477,7 +477,8 @@ static int elan_smbus_get_report(struct i2c_client
> > *client, u8 *report) }
> >
> > static int elan_smbus_finish_fw_update(struct i2c_client *client,
> > - struct completion *fw_completion)
> > + struct completion *fw_completion,
> > + unsigned int irq)
> > {
> > /* No special handling unlike I2C transport */
> > return 0;
> > --
> > 1.7.9.5
> >
>
> --
> Dmitry
--
Dmitry
^ permalink raw reply
* Re: Synaptics, CAP_FORCEPAD, bad behavior
From: Ross Vandegrift @ 2014-10-30 21:58 UTC (permalink / raw)
To: linux-input
In-Reply-To: <543656A2.4080009@kernelconcepts.de>
Nicole Faerber <nicole.faerber <at> kernelconcepts.de> writes:
> psmouse serio1: synaptics: Touchpad model: 1, fw: 8.1, id: 0x1e2b1,
> caps: 0xd002a3/0x940300/0x12f800, board id: 2911, fw id: 2560
>
> The issue is that now a button release is only issued after the finger
> has completely left the touchpad and not when releasing the physical
> button. Is this physical button now called FORCEPAD? Anyway, this is
> pretty annoying. Double clicking become a real pain.
I'm also experiencing this, but wanted to add that it happens on only one of
two T440s laptops I've used recently.
Affected:
psmouse serio1: synaptics: Touchpad model: 1, fw: 8.1, id: 0x1e2b1, caps:
0xd001a3/0x940300/0x12e800, board id: 2962, fw id: 2560
Unaffected:
psmouse serio1: synaptics: Touchpad model: 1, fw: 8.1, id: 0x1e2b1, caps:
0xd002a3/0x940300/0x127c00, board id: 2668, fw id: 1545510
Ross
^ permalink raw reply
* Re: [PATCH 01/15] mfd: add new driver for Sharp LoCoMo
From: Linus Walleij @ 2014-10-31 7:42 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux Input,
linux-leds-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org, Andrea Adami,
Russell King, Daniel Mack, Haojian Zhuang, Robert Jarzmik,
Alexandre Courbot, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo
In-Reply-To: <1414454528-24240-2-git-send-email-dbaryshkov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Tue, Oct 28, 2014 at 1:01 AM, Dmitry Eremin-Solenikov
<dbaryshkov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> LoCoMo is a GA used on Sharp Zaurus SL-5x00. Current driver does has
> several design issues (special bus instead of platform bus, doesn't use
> mfd-core, etc).
>
> Implement 'core' parts of locomo support as an mfd driver.
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
(...)
> +/* DAC send data */
> +#define M62332_SLAVE_ADDR 0x4e /* Slave address */
> +#define M62332_W_BIT 0x00 /* W bit (0 only) */
> +#define M62332_SUB_ADDR 0x00 /* Sub address */
> +#define M62332_A_BIT 0x00 /* A bit (0 only) */
> +
> +/* DAC setup and hold times (expressed in us) */
> +#define DAC_BUS_FREE_TIME 5 /* 4.7 us */
> +#define DAC_START_SETUP_TIME 5 /* 4.7 us */
> +#define DAC_STOP_SETUP_TIME 4 /* 4.0 us */
> +#define DAC_START_HOLD_TIME 5 /* 4.7 us */
> +#define DAC_SCL_LOW_HOLD_TIME 5 /* 4.7 us */
> +#define DAC_SCL_HIGH_HOLD_TIME 4 /* 4.0 us */
> +#define DAC_DATA_SETUP_TIME 1 /* 250 ns */
> +#define DAC_DATA_HOLD_TIME 1 /* 300 ns */
> +#define DAC_LOW_SETUP_TIME 1 /* 300 ns */
> +#define DAC_HIGH_SETUP_TIME 1 /* 1000 ns */
(...)
It seems some DAC handling is part of the MFD driver, and we recently
discussed that MFD should not be doing misc stuff but mainly act as
arbiter and switching station.
Can you please move the DAC parts of the driver to
drivers/iio/dac?
The IIO DAC subsystem will likely add other goodies to
the driver for free and give a nice API to consumers.
> +/* IRQ support */
> +static void locomo_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + struct locomo *lchip = irq_get_handler_data(irq);
> + int req, i;
> +
> + /* Acknowledge the parent IRQ */
> + desc->irq_data.chip->irq_ack(&desc->irq_data);
> +
> + /* check why this interrupt was generated */
> + req = readw(lchip->base + LOCOMO_ICR) & 0x0f00;
> +
> + if (req) {
> + /* generate the next interrupt(s) */
> + irq = lchip->irq_base;
This use if a static IRQ base is oldschool. Use irqdomain,
see for example tc3589x.c.
irq_domain_add_simple() should suffice.
IIRC you have used irqdomain before so you know the drill.
> + for (i = 0; i <= 3; i++, irq++) {
> + if (req & (0x0100 << i))
> + generic_handle_irq(irq);
> +
> + }
Reading the status register once and then check the IRQs
can be dangerous on non-threaded interrupt handlers as it could
miss transient IRQs appearing duing the IRQ handling.
The best example code is in drivers/irqchip.
For example in irq-vic.c there is this nice loop:
while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
irq = ffs(stat) - 1;
handle_domain_irq(vic->domain, irq, regs);
handled = 1;
}
Note how stat is re-read on each iteration.
> +static void locomo_setup_irq(struct locomo *lchip)
> +{
> + int irq;
> +
> + lchip->irq_base = irq_alloc_descs(-1, 0, LOCOMO_NR_IRQS, -1);
irqdomain will allocate descriptors for you when calling
irq_create_mapping() which should be done for all lines.
> + /* Install handlers for IRQ_LOCOMO_* */
> + for (irq = lchip->irq_base;
> + irq < lchip->irq_base + LOCOMO_NR_IRQS;
> + irq++) {
> + irq_set_chip_and_handler(irq, &locomo_chip, handle_level_irq);
> + irq_set_chip_data(irq, lchip);
> + set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
> + }
> +
> + /*
> + * Install handler for IRQ_LOCOMO_HW.
> + */
> + irq_set_irq_type(lchip->irq, IRQ_TYPE_EDGE_FALLING);
> + irq_set_handler_data(lchip->irq, lchip);
> + irq_set_chained_handler(lchip->irq, locomo_handler);
> +}
(...)
> + /* Longtime timer */
> + writew(0, lchip->base + LOCOMO_LTINT);
> + /* SPI */
> + writew(0, lchip->base + LOCOMO_SPI + LOCOMO_SPIIE);
> +
> + writew(6 + 8 + 320 + 30 - 10, lchip->base + LOCOMO_ASD);
That's a few magic numbers and calculation don't you think?
A comment stating what's going on would be helpful.
> + r = readw(lchip->base + LOCOMO_ASD);
> + r |= 0x8000;
> + writew(r, lchip->base + LOCOMO_ASD);
> +
> + writew(6 + 8 + 320 + 30 - 10 - 128 + 4, lchip->base + LOCOMO_HSD);
Dito.
> + r = readw(lchip->base + LOCOMO_HSD);
> + r |= 0x8000;
> + writew(r, lchip->base + LOCOMO_HSD);
> +
> + writew(128 / 8, lchip->base + LOCOMO_HSC);
Dito.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" 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 02/15] GPIO: port LoCoMo gpio support from old driver
From: Linus Walleij @ 2014-10-31 7:48 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
Linux Input, linux-leds@vger.kernel.org,
linux-spi@vger.kernel.org, linux-fbdev@vger.kernel.org,
alsa-devel@alsa-project.org, Andrea Adami, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Alexandre Courbot,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Mark Brown, Jingoo
In-Reply-To: <1414454528-24240-3-git-send-email-dbaryshkov@gmail.com>
On Tue, Oct 28, 2014 at 1:01 AM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> Add gpiolib driver for gpio pins placed on the LoCoMo GA.
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
(...)
> +static int locomo_gpio_get(struct gpio_chip *chip,
> + unsigned offset)
> +{
> + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio);
> +
> + return readw(lg->regs + LOCOMO_GPL) & (1 << offset);
Do this:
#include <linux/bitops.h>
return !!(readw(lg->regs + LOCOMO_GPL) & BIT(offset));
So you clamp the returned value to a bool.
> +static void __locomo_gpio_set(struct gpio_chip *chip,
> + unsigned offset, int value)
> +{
> + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio);
> + u16 r;
> +
> + r = readw(lg->regs + LOCOMO_GPO);
> + if (value)
> + r |= 1 << offset;
r |= BIT(offset);
> + else
> + r &= ~(1 << offset);
r &= BIT(offset);
(etc, everywhere this pattern occurs).
> +static void locomo_gpio_set(struct gpio_chip *chip,
> + unsigned offset, int value)
> +{
> + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&lg->lock, flags);
> +
> + __locomo_gpio_set(chip, offset, value);
> +
> + spin_unlock_irqrestore(&lg->lock, flags);
If you actually always have to be getting and releasing a spin lock around
the register writes, contemplate using regmap-mmio because that
is part of what it does.
But is this locking really necessary?
> +static int locomo_gpio_remove(struct platform_device *pdev)
> +{
> + struct locomo_gpio *lg = platform_get_drvdata(pdev);
> + int ret;
> +
> + ret = gpiochip_remove(&lg->gpio);
> + if (ret) {
> + dev_err(&pdev->dev, "Can't remove gpio chip: %d\n", ret);
> + return ret;
> + }
The return value from gpiochip_remove() has been removed in v3.18-rc1
so this will not compile.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 09/15] ARM: sa1100: don't preallocate IRQ space for locomo
From: Linus Walleij @ 2014-10-31 7:50 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
Linux Input, linux-leds@vger.kernel.org,
linux-spi@vger.kernel.org, linux-fbdev@vger.kernel.org,
alsa-devel@alsa-project.org, Andrea Adami, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Alexandre Courbot,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Mark Brown, Jingoo
In-Reply-To: <1414454528-24240-10-git-send-email-dbaryshkov@gmail.com>
On Tue, Oct 28, 2014 at 1:02 AM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> As new locomo driver properly supports SPARSE_IRQ, stop playing with
> NR_IRQS on sa1100 (locomo was the last chip requiring NR_IRQ tricks).
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
That's right! By moving to irqdomain things get even better :)
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 14/15] gpio: locomo: implement per-pin irq handling
From: Linus Walleij @ 2014-10-31 8:00 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
Linux Input, linux-leds@vger.kernel.org,
linux-spi@vger.kernel.org, linux-fbdev@vger.kernel.org,
alsa-devel@alsa-project.org, Andrea Adami, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Alexandre Courbot,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Mark Brown, Jingoo
In-Reply-To: <1414454528-24240-15-git-send-email-dbaryshkov@gmail.com>
On Tue, Oct 28, 2014 at 1:02 AM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> LoCoMo has a possibility to generate per-GPIO edge irqs. Support for
> that was there in old locomo driver, got 'cleaned up' during old driver
> IRQ cascading cleanup and is now reimplemented. It is expected that
> SL-5500 (collie) will use locomo gpio irqs for mmc detection irq.
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Please don't use open-coded IRQ handling like this, we are moving
away from that.
In Kconfig,
select GPIOLIB_IRQCHIP
and look at the other drivers selecting this for inspiration. There
is even some documentation in Documentation/gpio/driver.txt
You will find that it cuts down a lot of overhead from your driver
and does everything in the right way in a central place.
> struct locomo_gpio {
> void __iomem *regs;
> + int irq;
>
> spinlock_t lock;
> struct gpio_chip gpio;
> + int irq_base;
gpiolib irqchip helpers uses irqdomain to do all this debasing
and rebasing for you. Go with that.
> +static int locomo_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio);
> +
> + return lg->irq_base + offset;
> +}
And it implements .to_irq() in the gpiolib core.
> +static void
> +locomo_gpio_handler(unsigned int irq, struct irq_desc *desc)
It's locomo_gpio_irq_handler() right?
> +{
> + u16 req;
> + struct locomo_gpio *lg = irq_get_handler_data(irq);
> + int i = lg->irq_base;
> +
> + req = readw(lg->regs + LOCOMO_GIR) &
> + readw(lg->regs + LOCOMO_GPD);
> +
> + while (req) {
> + if (req & 1)
> + generic_handle_irq(i);
> + req >>= 1;
> + i++;
> + }
Same thing as the MFD device, look closer at how you construct
the IRQ handling loop, so the register gets re-read each iteration.
> +static void locomo_gpio_ack_irq(struct irq_data *d)
> +{
> + struct locomo_gpio *lg = irq_data_get_irq_chip_data(d);
> + unsigned long flags;
> + u16 r;
> +
> + spin_lock_irqsave(&lg->lock, flags);
> +
> + r = readw(lg->regs + LOCOMO_GWE);
> + r |= (0x0001 << (d->irq - lg->irq_base));
> + writew(r, lg->regs + LOCOMO_GWE);
> +
> + r = readw(lg->regs + LOCOMO_GIS);
> + r &= ~(0x0001 << (d->irq - lg->irq_base));
> + writew(r, lg->regs + LOCOMO_GIS);
> +
> + r = readw(lg->regs + LOCOMO_GWE);
> + r &= ~(0x0001 << (d->irq - lg->irq_base));
> + writew(r, lg->regs + LOCOMO_GWE);
> +
> + spin_unlock_irqrestore(&lg->lock, flags);
> +}
I really wonder if this locking is needed around these
regioster accesses. It seems more like a habit than
like something that is actually needed. Think it over.
*irqsave* versions of spinlocks are definately wrong
in the irqchip callbacks, if you give it a minute I think
you quickly realize why.
> +static int locomo_gpio_type(struct irq_data *d, unsigned int type)
> +{
> + unsigned int mask;
> + struct locomo_gpio *lg = irq_data_get_irq_chip_data(d);
> + unsigned long flags;
> +
> + mask = 1 << (d->irq - lg->irq_base);
This should just use d->hwirq with irqdomain implemented
correctly.
(...)
> +static void locomo_gpio_setup_irq(struct locomo_gpio *lg)
> +{
> + int irq;
> +
> + lg->irq_base = irq_alloc_descs(-1, 0, LOCOMO_GPIO_NR_IRQS, -1);
> +
> + /* Install handlers for IRQ_LOCOMO_* */
> + for (irq = lg->irq_base;
> + irq < lg->irq_base + LOCOMO_GPIO_NR_IRQS;
> + irq++) {
> + irq_set_chip_and_handler(irq, &locomo_gpio_chip,
> + handle_edge_irq);
> + irq_set_chip_data(irq, lg);
> + set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
> + }
> +
> + /*
> + * Install handler for IRQ_LOCOMO_HW.
> + */
> + irq_set_handler_data(lg->irq, lg);
> + irq_set_chained_handler(lg->irq, locomo_gpio_handler);
> +}
All this gets redundant with gpiochip_irqchip_add()
and gpiochip_set_chained_irqchip().
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH] i8042: Add noloop quirk for Asus X750LN
From: Hans de Goede @ 2014-10-31 8:21 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
In-Reply-To: <20141030183410.GG36444@dtor-ws>
Hi,
On 10/30/2014 07:34 PM, Dmitry Torokhov wrote:
> On Sat, Oct 11, 2014 at 11:35:23AM -0700, Dmitry Torokhov wrote:
>> On Sat, Oct 11, 2014 at 03:09:06PM +0200, Hans de Goede wrote:
>>> Without this the aux port does not get detected, and consequently the touchpad
>>> will not work.
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1110011
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Applied, thank you.
>
> Hans,
>
> Do you still have dmidecode for this laptop around?
No I took the necessary identification strings from the dmesg, which is here:
https://bugzilla.redhat.com/attachment.cgi?id=944389
I'll drop you a private mail with the users email address, then you
can get into contact with him directly, I expect him to be willing to
help debug this further.
> Can I see it? I am
> just wondering what part of AUX_LOOP was causing issues. Was it aux
> delivery test or loop test itself?
I was wondering myself, which is why the above dmesg is with:
"dyndbg=file drivers/input/serio/* +p"
But that does not seem to print anything extra ..., is the i8042 code
properly using one of the foo_dbg macro's ?
Regards,
Hans
^ permalink raw reply
* Re: [PATCH] i8042: Add noloop quirk for Asus X750LN
From: Hans de Goede @ 2014-10-31 8:23 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
In-Reply-To: <545346A1.3070708@redhat.com>
Hi,
On 10/31/2014 09:21 AM, Hans de Goede wrote:
> Hi,
>
> On 10/30/2014 07:34 PM, Dmitry Torokhov wrote:
>> On Sat, Oct 11, 2014 at 11:35:23AM -0700, Dmitry Torokhov wrote:
>>> On Sat, Oct 11, 2014 at 03:09:06PM +0200, Hans de Goede wrote:
>>>> Without this the aux port does not get detected, and consequently the touchpad
>>>> will not work.
>>>>
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1110011
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> Applied, thank you.
>>
>> Hans,
>>
>> Do you still have dmidecode for this laptop around?
>
> No I took the necessary identification strings from the dmesg, which is here:
>
> https://bugzilla.redhat.com/attachment.cgi?id=944389
>
> I'll drop you a private mail with the users email address, then you
> can get into contact with him directly, I expect him to be willing to
> help debug this further.
>
>> Can I see it? I am
>> just wondering what part of AUX_LOOP was causing issues. Was it aux
>> delivery test or loop test itself?
>
> I was wondering myself, which is why the above dmesg is with:
>
> "dyndbg=file drivers/input/serio/* +p"
>
> But that does not seem to print anything extra ..., is the i8042 code
> properly using one of the foo_dbg macro's ?
Ah, wait I just realized that even though the user was asking for help
in Red Hat bugzilla he is actually a ubuntu user, I guess their kernels
just don't have dyndbg turned on.
Regards,
Hans
^ permalink raw reply
* re: HID: Introduce hidpp, a module to handle Logitech hid++ devices
From: Dan Carpenter @ 2014-10-31 9:15 UTC (permalink / raw)
To: benjamin.tissoires; +Cc: linux-input
Hello Benjamin Tissoires,
The patch 2f31c5252910: "HID: Introduce hidpp, a module to handle
Logitech hid++ devices" from Sep 30, 2014, leads to the following
static checker warning:
drivers/hid/hid-logitech-hidpp.c:359 hidpp_root_get_protocol_version()
warn: should this return really be negated?
drivers/hid/hid-logitech-hidpp.c
342 static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp)
343 {
344 struct hidpp_report response;
345 int ret;
346
347 ret = hidpp_send_fap_command_sync(hidpp,
348 HIDPP_PAGE_ROOT_IDX,
349 CMD_ROOT_GET_PROTOCOL_VERSION,
350 NULL, 0, &response);
351
352 if (ret == 1) {
^^^^^^^^
What does the "1" mean? Magic numbers are bad, yada yada yada.
353 hidpp->protocol_major = 1;
354 hidpp->protocol_minor = 0;
355 return 0;
356 }
357
358 if (ret)
359 return -ret;
^^^^^^^^^^^
This is wrong. The real problem is that hidpp_send_fap_command_sync()
mixes normal and custom error codes. The callers are inconsistent in
how they deal with it.
360
361 hidpp->protocol_major = response.fap.params[0];
362 hidpp->protocol_minor = response.fap.params[1];
363
364 return ret;
365 }
See also:
drivers/hid/hid-logitech-hidpp.c:398 hidpp_devicenametype_get_count() warn: should this return really be negated?
drivers/hid/hid-logitech-hidpp.c:417 hidpp_devicenametype_get_device_name() warn: should this return really be negated?
drivers/hid/hid-logitech-hidpp.c:524 hidpp_touchpad_get_raw_info() warn: should this return really be negated?
regards,
dan carpenter
^ permalink raw reply
* [patch] HID: logitech-hidpp: leaks and NULL dereferences
From: Dan Carpenter @ 2014-10-31 9:14 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Henrik Rydberg, linux-input, kernel-janitors
Shift the allocation down a few lines to avoid a memory leak and also
add a check for allocation failure.
Fixes: 2f31c5252910 ('HID: Introduce hidpp, a module to handle Logitech hid++ devices')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 361e97d..3cce995 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -200,13 +200,15 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp,
u8 feat_index, u8 funcindex_clientid, u8 *params, int param_count,
struct hidpp_report *response)
{
- struct hidpp_report *message = kzalloc(sizeof(struct hidpp_report),
- GFP_KERNEL);
+ struct hidpp_report *message;
int ret;
if (param_count > sizeof(message->fap.params))
return -EINVAL;
+ message = kzalloc(sizeof(struct hidpp_report), GFP_KERNEL);
+ if (!message)
+ return -ENOMEM;
message->report_id = REPORT_ID_HIDPP_LONG;
message->fap.feature_index = feat_index;
message->fap.funcindex_clientid = funcindex_clientid;
@@ -221,8 +223,7 @@ static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,
u8 report_id, u8 sub_id, u8 reg_address, u8 *params, int param_count,
struct hidpp_report *response)
{
- struct hidpp_report *message = kzalloc(sizeof(struct hidpp_report),
- GFP_KERNEL);
+ struct hidpp_report *message;
int ret;
if ((report_id != REPORT_ID_HIDPP_SHORT) &&
@@ -232,6 +233,9 @@ static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,
if (param_count > sizeof(message->rap.params))
return -EINVAL;
+ message = kzalloc(sizeof(struct hidpp_report), GFP_KERNEL);
+ if (!message)
+ return -ENOMEM;
message->report_id = report_id;
message->rap.sub_id = sub_id;
message->rap.reg_address = reg_address;
^ permalink raw reply related
* Re: [PATCH 09/15] ARM: sa1100: don't preallocate IRQ space for locomo
From: Dmitry Eremin-Solenikov @ 2014-10-31 9:33 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
Linux Input, linux-leds@vger.kernel.org,
linux-spi@vger.kernel.org, linux-fbdev@vger.kernel.org,
alsa-devel@alsa-project.org, Andrea Adami, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Alexandre Courbot,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Mark Brown, Jingoo
In-Reply-To: <CACRpkdZnAtXtdtKbuqj8a_6a=2jS_Jf6yrq=iz-GJV+4RifRhw@mail.gmail.com>
2014-10-31 10:50 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
> On Tue, Oct 28, 2014 at 1:02 AM, Dmitry Eremin-Solenikov
> <dbaryshkov@gmail.com> wrote:
>
>> As new locomo driver properly supports SPARSE_IRQ, stop playing with
>> NR_IRQS on sa1100 (locomo was the last chip requiring NR_IRQ tricks).
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>
> That's right! By moving to irqdomain things get even better :)
That is a separate topic in my repo. I'll push that after finishing with LoCoMo.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 14/15] gpio: locomo: implement per-pin irq handling
From: Dmitry Eremin-Solenikov @ 2014-10-31 9:35 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
Linux Input, linux-leds@vger.kernel.org,
linux-spi@vger.kernel.org, linux-fbdev@vger.kernel.org,
alsa-devel@alsa-project.org, Andrea Adami, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Alexandre Courbot,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Mark Brown, Jingoo
In-Reply-To: <CACRpkdamVriR9kJSeY=LVAx7ADukT8+GpqmNH9YMnL3OKUk8iw@mail.gmail.com>
2014-10-31 11:00 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
> On Tue, Oct 28, 2014 at 1:02 AM, Dmitry Eremin-Solenikov
> <dbaryshkov@gmail.com> wrote:
>
>> LoCoMo has a possibility to generate per-GPIO edge irqs. Support for
>> that was there in old locomo driver, got 'cleaned up' during old driver
>> IRQ cascading cleanup and is now reimplemented. It is expected that
>> SL-5500 (collie) will use locomo gpio irqs for mmc detection irq.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>
> Please don't use open-coded IRQ handling like this, we are moving
> away from that.
>
> In Kconfig,
>
> select GPIOLIB_IRQCHIP
>
> and look at the other drivers selecting this for inspiration. There
> is even some documentation in Documentation/gpio/driver.txt
>
> You will find that it cuts down a lot of overhead from your driver
> and does everything in the right way in a central place.
Thanks, I will take a look.
[Skipped the rest of the comments mostly dedicated to gpiolib_irqchip].
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 02/15] GPIO: port LoCoMo gpio support from old driver
From: Dmitry Eremin-Solenikov @ 2014-10-31 9:39 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux Input,
linux-leds-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org, Andrea Adami,
Russell King, Daniel Mack, Haojian Zhuang, Robert Jarzmik,
Alexandre Courbot, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo
In-Reply-To: <CACRpkdY7tRadod2vQfEytmw-ubaMAvr_=XTczD5bUMkqie0xkg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-10-31 10:48 GMT+03:00 Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
> On Tue, Oct 28, 2014 at 1:01 AM, Dmitry Eremin-Solenikov
> <dbaryshkov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> Add gpiolib driver for gpio pins placed on the LoCoMo GA.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
[skipped]
> (etc, everywhere this pattern occurs).
>> +static void locomo_gpio_set(struct gpio_chip *chip,
>> + unsigned offset, int value)
>> +{
>> + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio);
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&lg->lock, flags);
>> +
>> + __locomo_gpio_set(chip, offset, value);
>> +
>> + spin_unlock_irqrestore(&lg->lock, flags);
>
> If you actually always have to be getting and releasing a spin lock around
> the register writes, contemplate using regmap-mmio because that
> is part of what it does.
>
> But is this locking really necessary?
I have a custom of doing such locking and never having to think about
somebody breaking into RMW cycles.
Also isn't regmap an overkill here? Wouldn't regmap also do a lock/unlock
around each register read/write/RMW?
>> +static int locomo_gpio_remove(struct platform_device *pdev)
>> +{
>> + struct locomo_gpio *lg = platform_get_drvdata(pdev);
>> + int ret;
>> +
>> + ret = gpiochip_remove(&lg->gpio);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Can't remove gpio chip: %d\n", ret);
>> + return ret;
>> + }
>
> The return value from gpiochip_remove() has been removed in v3.18-rc1
> so this will not compile.
Yes, the fix will be in the next iteration. This patchset was based on 3.17
--
With best wishes
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" 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 11/15] sound: soc: poodle: make use of new locomo GPIO interface
From: Linus Walleij @ 2014-10-31 9:52 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Mark Brown, Dmitry Eremin-Solenikov,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux Input,
linux-leds-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org, Andrea Adami,
Russell King, Daniel Mack, Haojian Zhuang, Robert Jarzmik,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones
In-Reply-To: <CAAVeFuKgARoMFzf+663iP6cULs93d4WSQS8ESjUb9VcxguWurA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Oct 29, 2014 at 4:03 AM, Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Oct 28, 2014 at 11:58 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote:
>>> Since LoCoMo driver has been converted to provide proper gpiolib
>>> interface, make poodle ASoC platform driver use gpiolib API.
>>
>> Please use subject lines matching the style for the subsystem.
>>
>>> + ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
>>> + ret);
>>> + return ret;
>>> + }
>>
>> I sense a need for devm_gpio_request_array() here. Otherwise this looks
>> fine - ideally it'd move to gpiod but moving to gpiolib is a clear win
>> so no need to block on this.
>
> I wish Dmitry took the opportunity to move this driver to the gpiod
> API, especially since doing so would be trivial for this driver.
+1 on this.
However this platform is not device tree, so this implies setting up
a descriptor table for the affected driver(s) to work properly.
See Documentation/gpio/board.txt
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" 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 01/15] mfd: add new driver for Sharp LoCoMo
From: Dmitry Eremin-Solenikov @ 2014-10-31 9:54 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
Linux Input, linux-leds@vger.kernel.org,
linux-spi@vger.kernel.org, linux-fbdev@vger.kernel.org,
alsa-devel@alsa-project.org, Andrea Adami, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Alexandre Courbot,
Dmitry Torokhov, Bryan Wu, Richard Purdie, Samuel Ortiz,
Lee Jones, Mark Brown, Jingoo
In-Reply-To: <CACRpkdbW3NA00zMZr216ZeZpgVkXwKUo30biTVd5kcHKGGtkHg@mail.gmail.com>
Hello,
Thank you for the review of patches.
2014-10-31 10:42 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
> On Tue, Oct 28, 2014 at 1:01 AM, Dmitry Eremin-Solenikov
> <dbaryshkov@gmail.com> wrote:
>
>> LoCoMo is a GA used on Sharp Zaurus SL-5x00. Current driver does has
>> several design issues (special bus instead of platform bus, doesn't use
>> mfd-core, etc).
>>
>> Implement 'core' parts of locomo support as an mfd driver.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> (...)
>
>> +/* DAC send data */
>> +#define M62332_SLAVE_ADDR 0x4e /* Slave address */
>> +#define M62332_W_BIT 0x00 /* W bit (0 only) */
>> +#define M62332_SUB_ADDR 0x00 /* Sub address */
>> +#define M62332_A_BIT 0x00 /* A bit (0 only) */
>> +
>> +/* DAC setup and hold times (expressed in us) */
>> +#define DAC_BUS_FREE_TIME 5 /* 4.7 us */
>> +#define DAC_START_SETUP_TIME 5 /* 4.7 us */
>> +#define DAC_STOP_SETUP_TIME 4 /* 4.0 us */
>> +#define DAC_START_HOLD_TIME 5 /* 4.7 us */
>> +#define DAC_SCL_LOW_HOLD_TIME 5 /* 4.7 us */
>> +#define DAC_SCL_HIGH_HOLD_TIME 4 /* 4.0 us */
>> +#define DAC_DATA_SETUP_TIME 1 /* 250 ns */
>> +#define DAC_DATA_HOLD_TIME 1 /* 300 ns */
>> +#define DAC_LOW_SETUP_TIME 1 /* 300 ns */
>> +#define DAC_HIGH_SETUP_TIME 1 /* 1000 ns */
> (...)
>
> It seems some DAC handling is part of the MFD driver, and we recently
> discussed that MFD should not be doing misc stuff but mainly act as
> arbiter and switching station.
>
> Can you please move the DAC parts of the driver to
> drivers/iio/dac?
>
> The IIO DAC subsystem will likely add other goodies to
> the driver for free and give a nice API to consumers.
I wanted this part to be as simple as possible. I will look into IIO
DAC subsystem.
The DAC is as simple 2 channel 8-bit i2c device connected to a separate i2c bus
controlled through a register in LoCoMo device. One channel is used
for backlight,
other will be used for volume control. So (in theory) I can add the
following device
chain: locomo -> i2c-locomo -> m62332 -> IIO DAC client. However isn't that
quite an overkill for just backlight & volume control? Please advice me on this.
[skipped the irqdomain part - I will use them, thanks for the suggestion.]
>> + /* Longtime timer */
>> + writew(0, lchip->base + LOCOMO_LTINT);
>> + /* SPI */
>> + writew(0, lchip->base + LOCOMO_SPI + LOCOMO_SPIIE);
>> +
>> + writew(6 + 8 + 320 + 30 - 10, lchip->base + LOCOMO_ASD);
>
> That's a few magic numbers and calculation don't you think?
>
> A comment stating what's going on would be helpful.
Unfortunately little is known here - these values are c&p from old 2.4
Lineo code.
This part is related to generating synchronization pulses for accessing
touchscreen.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 11/15] sound: soc: poodle: make use of new locomo GPIO interface
From: Dmitry Eremin-Solenikov @ 2014-10-31 9:58 UTC (permalink / raw)
To: Linus Walleij
Cc: Alexandre Courbot, Mark Brown,
linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
Linux Input, linux-leds@vger.kernel.org,
linux-spi@vger.kernel.org, linux-fbdev@vger.kernel.org,
alsa-devel@alsa-project.org, Andrea Adami, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Dmitry Torokhov,
Bryan Wu, Richard Purdie, Samuel Ortiz, Lee Jones, Jingoo
In-Reply-To: <CACRpkdY-RDuxGMWe_SD613ovsEoHhxjP1+3T-m_zKQSx=mkFjA@mail.gmail.com>
2014-10-31 12:52 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
> On Wed, Oct 29, 2014 at 4:03 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> On Tue, Oct 28, 2014 at 11:58 PM, Mark Brown <broonie@kernel.org> wrote:
>>> On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote:
>>>> Since LoCoMo driver has been converted to provide proper gpiolib
>>>> interface, make poodle ASoC platform driver use gpiolib API.
>>>
>>> Please use subject lines matching the style for the subsystem.
>>>
>>>> + ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
>>>> + if (ret) {
>>>> + dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
>>>> + ret);
>>>> + return ret;
>>>> + }
>>>
>>> I sense a need for devm_gpio_request_array() here. Otherwise this looks
>>> fine - ideally it'd move to gpiod but moving to gpiolib is a clear win
>>> so no need to block on this.
>>
>> I wish Dmitry took the opportunity to move this driver to the gpiod
>> API, especially since doing so would be trivial for this driver.
>
> +1 on this.
>
> However this platform is not device tree, so this implies setting up
> a descriptor table for the affected driver(s) to work properly.
> See Documentation/gpio/board.txt
I checked the gpiod interfaces after original suggestion by Alexandre.
Introducing those mapping tables (much like pinctrl tables) look like
a duplicate effort if Russell will permit adding a DT support. So
I thought that I will reconsider gpiod/pinctrl/etc after fixing
LoCoMo, reiterating IRQ patches, possibly switching to COMMON_CLK
and (finally) thinking about device tree support.
--
With best wishes
Dmitry
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox