[PATCH 0/3] HID: magicmouse: More scroll improvements

[PATCH 0/3] HID: magicmouse: More scroll improvements

[Chase Douglas]

The following three patches improve scrolling for Apple Magicmouse
devices. The first two were sent as a set of four patches. Two were
applied while two were reviewed later. They are now ready for
application. The last patch is new, but is rather simple.

I have been running with these patches for a few weeks and have been
very happy with the results :).

HID: magicmouse: properly account for scroll movement in state
HID: magicmouse: add param for scroll speed
HID: magicmouse: enable horizontal scrolling
--
 drivers/hid/hid-magicmouse.c |   63 ++++++++++++++++++++++++++++++------------
 1 files changed, 45 insertions(+), 18 deletions(-)

[PATCH 1/3] HID: magicmouse: properly account for scroll movement in state

[Chase Douglas]

Before this change, sequential scroll events would take a variable
amount of movement due to incorrect accounting. This change ensures all
scroll movements require a deterministic touch movement for an action to
occur.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
Acked-by: Michael Poole <mdpoole@troilus.org>
---
 drivers/hid/hid-magicmouse.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 4c4a79c..f44aaf2 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -189,7 +189,8 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 		case TOUCH_STATE_DRAG:
 			step = step / accel_profile[msc->scroll_accel];
 			if (step != 0) {
-				msc->touches[id].scroll_y = y;
+				msc->touches[id].scroll_y -=
+					step * accel_profile[msc->scroll_accel];
 				msc->scroll_jiffies = now;
 				input_report_rel(input, REL_WHEEL, step);
 			}
-- 
1.7.0.4

Re: [PATCH 1/3] HID: magicmouse: properly account for scroll movement in state

[Jiri Kosina]

On Sun, 20 Jun 2010, Chase Douglas wrote:

> Before this change, sequential scroll events would take a variable
> amount of movement due to incorrect accounting. This change ensures all
> scroll movements require a deterministic touch movement for an action to
> occur.
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> Acked-by: Michael Poole <mdpoole@troilus.org>
> ---
>  drivers/hid/hid-magicmouse.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index 4c4a79c..f44aaf2 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -189,7 +189,8 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
>  		case TOUCH_STATE_DRAG:
>  			step = step / accel_profile[msc->scroll_accel];
>  			if (step != 0) {
> -				msc->touches[id].scroll_y = y;
> +				msc->touches[id].scroll_y -=
> +					step * accel_profile[msc->scroll_accel];
>  				msc->scroll_jiffies = now;
>  				input_report_rel(input, REL_WHEEL, step);
>  			}

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

[PATCH 2/3] HID: magicmouse: add param for scroll speed

[Chase Douglas]

The new scroll_speed param takes an integer value from 0 to 63, where 0
is slowest and 63 is fastest. The default of 32 remains the same. This
parameter also affects scroll acceleration linearly.

A second part of this change is a tightly coupled modification to the
scroll acceleration. Previously, scroll acceleration could be reset
without lifting the scroll finger. This is rather unintuitive and hard
to control in the case where a user wants faster scrolling, but wants to
hold the scroll touch for longer than a moment.

Note that scroll acceleration levels are now 1-7, where 7 is slowest. In
the previous implementation, there were 8 levels defined, but it was
impossible to start at the slowest level. In order to keep the default
scroll speed unchanged, only 7 levels are used now.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
Acked-by: Michael Poole <mdpoole@troilus.org>
---
 drivers/hid/hid-magicmouse.c |   43 +++++++++++++++++++++++++++--------------
 1 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index f44aaf2..fe0c760 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -30,6 +30,17 @@ static bool emulate_scroll_wheel = true;
 module_param(emulate_scroll_wheel, bool, 0644);
 MODULE_PARM_DESC(emulate_scroll_wheel, "Emulate a scroll wheel");
 
+static unsigned int scroll_speed = 32;
+static int param_set_scroll_speed(const char *val, struct kernel_param *kp) {
+	unsigned long speed;
+	if (!val || strict_strtoul(val, 0, &speed) || speed > 63)
+		return -EINVAL;
+	scroll_speed = speed;
+	return 0;
+}
+module_param_call(scroll_speed, param_set_scroll_speed, param_get_uint, &scroll_speed, 0644);
+MODULE_PARM_DESC(scroll_speed, "Scroll speed, value from 0 (slow) to 63 (fast)");
+
 static bool scroll_acceleration = false;
 module_param(scroll_acceleration, bool, 0644);
 MODULE_PARM_DESC(scroll_acceleration, "Accelerate sequential scroll events");
@@ -54,6 +65,8 @@ MODULE_PARM_DESC(report_undeciphered, "Report undeciphered multi-touch state fie
 #define TOUCH_STATE_START 0x30
 #define TOUCH_STATE_DRAG  0x40
 
+#define SCROLL_ACCEL_DEFAULT 7
+
 /**
  * struct magicmouse_sc - Tracks Magic Mouse-specific data.
  * @input: Input device through which we report events.
@@ -145,7 +158,7 @@ static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state)
 	input_report_key(msc->input, BTN_RIGHT, state & 2);
 
 	if (state != last_state)
-		msc->scroll_accel = 0;
+		msc->scroll_accel = SCROLL_ACCEL_DEFAULT;
 }
 
 static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tdata)
@@ -167,30 +180,28 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 	 * vertical touch motions.
 	 */
 	if (emulate_scroll_wheel) {
-		static const int accel_profile[] = {
-			256, 228, 192, 160, 128, 96, 64, 32,
-		};
 		unsigned long now = jiffies;
 		int step = msc->touches[id].scroll_y - y;
 
-		/* Reset acceleration after half a second. */
-		if (time_after(now, msc->scroll_jiffies + HZ / 2))
-			msc->scroll_accel = 0;
-
 		/* Calculate and apply the scroll motion. */
 		switch (tdata[7] & TOUCH_STATE_MASK) {
 		case TOUCH_STATE_START:
 			msc->touches[id].scroll_y = y;
-			if (scroll_acceleration)
-				msc->scroll_accel = min_t(int,
-						msc->scroll_accel + 1,
-						ARRAY_SIZE(accel_profile) - 1);
+
+			/* Reset acceleration after half a second. */
+			if (scroll_acceleration && time_before(now,
+						msc->scroll_jiffies + HZ / 2))
+				msc->scroll_accel = max_t(int,
+						msc->scroll_accel - 1, 1);
+			else
+				msc->scroll_accel = SCROLL_ACCEL_DEFAULT;
+
 			break;
 		case TOUCH_STATE_DRAG:
-			step = step / accel_profile[msc->scroll_accel];
+			step /= (64 - (int)scroll_speed) * msc->scroll_accel;
 			if (step != 0) {
-				msc->touches[id].scroll_y -=
-					step * accel_profile[msc->scroll_accel];
+				msc->touches[id].scroll_y -= step *
+					(64 - scroll_speed) * msc->scroll_accel;
 				msc->scroll_jiffies = now;
 				input_report_rel(input, REL_WHEEL, step);
 			}
@@ -351,6 +362,8 @@ static int magicmouse_probe(struct hid_device *hdev,
 		return -ENOMEM;
 	}
 
+	msc->scroll_accel = SCROLL_ACCEL_DEFAULT;
+
 	msc->quirks = id->driver_data;
 	hid_set_drvdata(hdev, msc);
 
-- 
1.7.0.4

[PATCH 3/3] HID: magicmouse: enable horizontal scrolling

[Chase Douglas]

Mimicks OS X behavior.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 drivers/hid/hid-magicmouse.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index fe0c760..0b89c1c 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -95,6 +95,7 @@ struct magicmouse_sc {
 	struct {
 		short x;
 		short y;
+		short scroll_x;
 		short scroll_y;
 		u8 size;
 	} touches[16];
@@ -181,11 +182,13 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 	 */
 	if (emulate_scroll_wheel) {
 		unsigned long now = jiffies;
-		int step = msc->touches[id].scroll_y - y;
+		int step_x = msc->touches[id].scroll_x - x;
+		int step_y = msc->touches[id].scroll_y - y;
 
 		/* Calculate and apply the scroll motion. */
 		switch (tdata[7] & TOUCH_STATE_MASK) {
 		case TOUCH_STATE_START:
+			msc->touches[id].scroll_x = x;
 			msc->touches[id].scroll_y = y;
 
 			/* Reset acceleration after half a second. */
@@ -198,12 +201,20 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 
 			break;
 		case TOUCH_STATE_DRAG:
-			step /= (64 - (int)scroll_speed) * msc->scroll_accel;
-			if (step != 0) {
-				msc->touches[id].scroll_y -= step *
+			step_x /= (64 - (int)scroll_speed) * msc->scroll_accel;
+			if (step_x != 0) {
+				msc->touches[id].scroll_x -= step_x *
 					(64 - scroll_speed) * msc->scroll_accel;
 				msc->scroll_jiffies = now;
-				input_report_rel(input, REL_WHEEL, step);
+				input_report_rel(input, REL_HWHEEL, -step_x);
+			}
+
+			step_y /= (64 - (int)scroll_speed) * msc->scroll_accel;
+			if (step_y != 0) {
+				msc->touches[id].scroll_y -= step_y *
+					(64 - scroll_speed) * msc->scroll_accel;
+				msc->scroll_jiffies = now;
+				input_report_rel(input, REL_WHEEL, step_y);
 			}
 			break;
 		}
@@ -318,8 +329,10 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
 	__set_bit(EV_REL, input->evbit);
 	__set_bit(REL_X, input->relbit);
 	__set_bit(REL_Y, input->relbit);
-	if (emulate_scroll_wheel)
+	if (emulate_scroll_wheel) {
 		__set_bit(REL_WHEEL, input->relbit);
+		__set_bit(REL_HWHEEL, input->relbit);
+	}
 
 	if (report_touches) {
 		__set_bit(EV_ABS, input->evbit);
-- 
1.7.0.4

Re: [PATCH 3/3] HID: magicmouse: enable horizontal scrolling

[Michael Poole]

Chase Douglas writes:

> Mimicks OS X behavior.
>
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> ---
>  drivers/hid/hid-magicmouse.c |   25 +++++++++++++++++++------
>  1 files changed, 19 insertions(+), 6 deletions(-)

I like this general idea, but Dmitry Torokhov didn't like my previous
patch to achieve a similar effect:
https://patchwork.kernel.org/patch/84201/

The only behavioral question I would have is whether a single
touch/swipe should be locked in one direction or if it should be able to
generate both horizontal and vertical scroll events.  That seems like a
user preference thing to me.

(My real wish is for X and the various widget libraries to get decent
multitouch support soon, so that the kernel doesn't have to address
those policy questions.)

Michael

> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index fe0c760..0b89c1c 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -95,6 +95,7 @@ struct magicmouse_sc {
>  	struct {
>  		short x;
>  		short y;
> +		short scroll_x;
>  		short scroll_y;
>  		u8 size;
>  	} touches[16];
> @@ -181,11 +182,13 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
>  	 */
>  	if (emulate_scroll_wheel) {
>  		unsigned long now = jiffies;
> -		int step = msc->touches[id].scroll_y - y;
> +		int step_x = msc->touches[id].scroll_x - x;
> +		int step_y = msc->touches[id].scroll_y - y;
>  
>  		/* Calculate and apply the scroll motion. */
>  		switch (tdata[7] & TOUCH_STATE_MASK) {
>  		case TOUCH_STATE_START:
> +			msc->touches[id].scroll_x = x;
>  			msc->touches[id].scroll_y = y;
>  
>  			/* Reset acceleration after half a second. */
> @@ -198,12 +201,20 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
>  
>  			break;
>  		case TOUCH_STATE_DRAG:
> -			step /= (64 - (int)scroll_speed) * msc->scroll_accel;
> -			if (step != 0) {
> -				msc->touches[id].scroll_y -= step *
> +			step_x /= (64 - (int)scroll_speed) * msc->scroll_accel;
> +			if (step_x != 0) {
> +				msc->touches[id].scroll_x -= step_x *
>  					(64 - scroll_speed) * msc->scroll_accel;
>  				msc->scroll_jiffies = now;
> -				input_report_rel(input, REL_WHEEL, step);
> +				input_report_rel(input, REL_HWHEEL, -step_x);
> +			}
> +
> +			step_y /= (64 - (int)scroll_speed) * msc->scroll_accel;
> +			if (step_y != 0) {
> +				msc->touches[id].scroll_y -= step_y *
> +					(64 - scroll_speed) * msc->scroll_accel;
> +				msc->scroll_jiffies = now;
> +				input_report_rel(input, REL_WHEEL, step_y);
>  			}
>  			break;
>  		}
> @@ -318,8 +329,10 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
>  	__set_bit(EV_REL, input->evbit);
>  	__set_bit(REL_X, input->relbit);
>  	__set_bit(REL_Y, input->relbit);
> -	if (emulate_scroll_wheel)
> +	if (emulate_scroll_wheel) {
>  		__set_bit(REL_WHEEL, input->relbit);
> +		__set_bit(REL_HWHEEL, input->relbit);
> +	}
>  
>  	if (report_touches) {
>  		__set_bit(EV_ABS, input->evbit);

Re: [PATCH 3/3] HID: magicmouse: enable horizontal scrolling

[Chase Douglas]

On Mon, 2010-06-21 at 21:29 -0400, Michael Poole wrote:
> Chase Douglas writes:
> 
> > Mimicks OS X behavior.
> >
> > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > ---
> >  drivers/hid/hid-magicmouse.c |   25 +++++++++++++++++++------
> >  1 files changed, 19 insertions(+), 6 deletions(-)
> 
> I like this general idea, but Dmitry Torokhov didn't like my previous
> patch to achieve a similar effect:
> https://patchwork.kernel.org/patch/84201/

To be fair, that patch had a lot more in it. I'm not sure that appending
horizontal scroll to what's already in the driver is too much to ask.

> The only behavioral question I would have is whether a single
> touch/swipe should be locked in one direction or if it should be able to
> generate both horizontal and vertical scroll events.  That seems like a
> user preference thing to me.

Not that we should necessarily mimick OS X, but they scroll in both axes
simultaneously. I have no problem with it, and I've not heard of anyone
else gripe either.

Locking it in to one direction may not seem logical to an end user, and
would add more complexity to the code.

> (My real wish is for X and the various widget libraries to get decent
> multitouch support soon, so that the kernel doesn't have to address
> those policy questions.)

I'm trying to help out with that, and I realize it's the real end-game
here. However, like you said, there's no support quite yet. So while end
users twiddle their thumbs waiting for support above the kernel, we
should throw them a bone.

-- Chase

Re: [PATCH 3/3] HID: magicmouse: enable horizontal scrolling

[Jiri Kosina]

On Mon, 21 Jun 2010, Chase Douglas wrote:

> > > Mimicks OS X behavior.
> > >
> > > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > > ---
> > >  drivers/hid/hid-magicmouse.c |   25 +++++++++++++++++++------
> > >  1 files changed, 19 insertions(+), 6 deletions(-)
> > 
> > I like this general idea, but Dmitry Torokhov didn't like my previous
> > patch to achieve a similar effect:
> > https://patchwork.kernel.org/patch/84201/
> 
> To be fair, that patch had a lot more in it. I'm not sure that appending
> horizontal scroll to what's already in the driver is too much to ask.

I agree here. Horizontal scrolling support itself is fine.

I have the patches in my to-review queue, will process them shortly.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 0/3] HID: magicmouse: More scroll improvements

[Jiri Kosina]

On Sun, 20 Jun 2010, Chase Douglas wrote:

> The following three patches improve scrolling for Apple Magicmouse
> devices. The first two were sent as a set of four patches. Two were
> applied while two were reviewed later. They are now ready for
> application. The last patch is new, but is rather simple.
> 
> I have been running with these patches for a few weeks and have been
> very happy with the results :).
> 
> HID: magicmouse: properly account for scroll movement in state
> HID: magicmouse: add param for scroll speed
> HID: magicmouse: enable horizontal scrolling
> --
>  drivers/hid/hid-magicmouse.c |   63 ++++++++++++++++++++++++++++++------------
>  1 files changed, 45 insertions(+), 18 deletions(-)

I have applied all three patches into my 'magicmouse' branch. Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.