Re: [PATCH] Input: Add support for Fujitsu S762 laptops scroll wheel

["Eisenberger Tamás" <ufooka@gmail.com>]

Hi Dimitry,

Thanks for your reply!

On 2013-05-21 20:58, Dmitry Torokhov wrote:
> Hi Tamás,
>
> On Tue, May 21, 2013 at 06:28:26PM +0200, Eisenberger Tamás wrote:
>> Hello,
>>
>> This were my first contribution, and I received no response at all :(
>> Am I done something wrong, no one else is interested in supporting
>> this particular piece of hardware or what?
>>
>> If anyone has a minute please review it.
>>
>
> Ah, yes, sorry...
>
>> Thanks:
>> Tamás
>>
>> On 2013-05-06 19:35, Eisenberger Tamás wrote:
>>> Detects and makes the Touch Scroll Wheel found on some Fujitsu laptops
>>> working.
>>>
>>> The detection is based on the (hopefully) unique E7 report of the wheel
>>> device.
>>> Up and down scrolling is detected by the only byte that has different
>>> values in the devices packets, but it should be fine since it only able
>>> to report that two events.
>>>
>>> Signed-off-by: Tama's Eisenberger <tamas@eisenberger.hu>
>>> Index: linux/drivers/input/mouse/alps.c
>>> ===================================================================
>>> --- linux.orig/drivers/input/mouse/alps.c
>>> +++ linux/drivers/input/mouse/alps.c
>>> @@ -410,6 +410,17 @@ static void alps_process_trackstick_pack
>>>   	if (packet[1] == 0x7f && packet[2] == 0x7f && packet[4] == 0x7f)
>>>   		return;
>>>
>>> +	if (priv->quirks & ALPS_QUIRK_SCROLL_WHEEL) {
>>> +		if (packet[3] == 0x4c)
>>> +			input_report_rel(dev, REL_WHEEL, 1);
>>> +
>>> +		if (packet[3] == 0x58)
>>> +			input_report_rel(dev, REL_WHEEL, -1);
>
> Are these really the only values that emitted by the wheel? Have you
> tried spinning it vigorously to see if it produces different results?

I've tried to scroll really slow, and really fast with it, also tryed to 
move my finger diagonally and randomly but only that two were sent. Also 
events are really rare when not moving the finger around the edge.

>
>>> +
>>> +		input_sync(dev);
>>> +		return;
>
> Why are we ignoring the rest of the packet in this case? Does it contain
> invalid data?

Without my modifications the original driver reports middle button 
events when I scroll upwards, so I have to return here to prevent this. 
I don't think that this scroll well can co-exists with a trackstick.

>
>>> +	}
>>> +
>>>   	x = (s8)(((packet[0] & 0x20) << 2) | (packet[1] & 0x7f));
>>>   	y = (s8)(((packet[0] & 0x10) << 3) | (packet[2] & 0x7f));
>>>   	z = (packet[4] & 0x7c) >> 2;
>>> @@ -1255,6 +1266,7 @@ error:
>>>   static int alps_setup_trackstick_v3(struct psmouse *psmouse, int
>>> reg_base)
>>>   {
>>>   	struct ps2dev *ps2dev = &psmouse->ps2dev;
>>> +	struct alps_data *priv = psmouse->private;
>>>   	int ret = 0;
>>>   	unsigned char param[4];
>>>
>>> @@ -1277,6 +1289,12 @@ static int alps_setup_trackstick_v3(stru
>>>   		psmouse_dbg(psmouse, "trackstick E7 report: %3ph\n", param);
>>>
>>>   		/*
>>> +		 * Detect fujitsu scroll wheel device
>>> +		 */
>>> +		if (param[0] == 0x34 && param[1] == 0x01 && param[2] == 0x14)
>>> +			priv->quirks |= ALPS_QUIRK_SCROLL_WHEEL;
>>> +
>
> I believe this quirk should go into alps_model_data[] table.

I've tried to do that but it seems that the exact same (as my primary 
touchpad) signature is already in the alps_model_data[] table (E7=73 02 
64, EC=88 07 9d) and the secondary device seems to be independent. This 
is why I'm trying to use the EC report of the secondary device but it 
seemed to be overkill to create a new  model_data array for attached 
devices or I don't know what could be the consequences to use the same 
array also for secondary devices.

>
>>> +		/*
>>>   		 * Not sure what this does, but it is absolutely
>>>   		 * essential. Without it, the touchpad does not
>>>   		 * work at all and the trackstick just emits normal
>>> @@ -1798,12 +1816,16 @@ int alps_init(struct psmouse *psmouse)
>>>   	dev2->id.product = PSMOUSE_ALPS;
>>>   	dev2->id.version = 0x0000;
>>>   	dev2->dev.parent = &psmouse->ps2dev.serio->dev;
>>> -
>>>   	dev2->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REL);
>>>   	dev2->relbit[BIT_WORD(REL_X)] = BIT_MASK(REL_X) | BIT_MASK(REL_Y);
>>>   	dev2->keybit[BIT_WORD(BTN_LEFT)] =
>>>   		BIT_MASK(BTN_LEFT) | BIT_MASK(BTN_MIDDLE) | BIT_MASK(BTN_RIGHT);
>>>
>>> +	if (priv->quirks & ALPS_QUIRK_SCROLL_WHEEL) {
>>> +		dev2->name = "DualPoint Scroll Wheel";
>
> I'd keep the name as is.

No prob. I'l remove the name change.

>
>>> +		dev2->relbit[BIT_WORD(REL_X)] |= BIT_MASK(REL_WHEEL);
>>> +	}
>>> +
>>>   	if (input_register_device(priv->dev2))
>>>   		goto init_fail;
>>>
>>> Index: linux/drivers/input/mouse/alps.h
>>> ===================================================================
>>> --- linux.orig/drivers/input/mouse/alps.h
>>> +++ linux/drivers/input/mouse/alps.h
>>> @@ -158,6 +158,7 @@ struct alps_data {
>>>   };
>>>
>>>   #define ALPS_QUIRK_TRACKSTICK_BUTTONS	1 /* trakcstick buttons in
>>> trackstick packet */
>>> +#define ALPS_QUIRK_SCROLL_WHEEL		2 /* secondary device is scroll wheel
>>> */
>>>
>>>   #ifdef CONFIG_MOUSE_PS2_ALPS
>>>   int alps_detect(struct psmouse *psmouse, bool set_properties);
>>>
>
> BTW, your mailer line-wrapped the patch so it is not possible to apply,
> when resending please make sure the long lines stay intact.

You're right, I’ll try to use an other one.

>
> Thanks.
>

So basically as I see the main concern is the introduction of a new 
quirk, but I don't know how to do this using the alps_model_data[] if 
you have any suggestions please tell me!

Thanks:
Tamás
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html