diff mbox

[3/4] input/alps: Allow touchsticks to report pressure

Message ID 1464436862-2649-4-git-send-email-ben@smart-cactus.org (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Gamari May 28, 2016, 12:01 p.m. UTC 
The SS5 hardware can report this.
---
 drivers/input/mouse/alps.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Hans de Goede May 28, 2016, 12:40 p.m. UTC | #1 
Hi,

On 28-05-16 14:01, Ben Gamari wrote:
> The SS5 hardware can report this.
> ---
>  drivers/input/mouse/alps.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 7874f4f..25d2cad 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -103,6 +103,7 @@ static const struct alps_nibble_commands alps_v6_nibble_commands[] = {
>  					   6-byte ALPS packet */
>  #define ALPS_STICK_BITS		0x100	/* separate stick button bits */
>  #define ALPS_BUTTONPAD		0x200	/* device is a clickpad */
> +#define ALPS_DUALPOINT_WITH_PRESSURE		0x400	/* device can report trackpoint pressure */
>
>  static const struct alps_model_info alps_model_data[] = {
>  	{ { 0x32, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },	/* Toshiba Salellite Pro M10 */
> @@ -1270,9 +1271,11 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
>  		} else {
>  			int x = (s8)(((p[0] & 1) << 7) | (p[1] & 0x7f));
>  			int y = (s8)(((p[3] & 1) << 7) | (p[2] & 0x7f));
> +			int pressure = (s8)(p[4] & 0x7f);
>
>  			input_report_rel(priv->dev2, REL_X, x);
>  			input_report_rel(priv->dev2, REL_Y, -y);
> +			input_report_abs(priv->dev2, ABS_PRESSURE, pressure);
>  		}
>  		break;
>
> @@ -2996,6 +2999,10 @@ int alps_init(struct psmouse *psmouse)
>
>  		input_set_capability(dev2, EV_REL, REL_X);
>  		input_set_capability(dev2, EV_REL, REL_Y);
> +		if (priv->flags & ALPS_DUALPOINT_WITH_PRESSURE) {
> +			input_set_capability(dev2, EV_ABS, ABS_PRESSURE);
> +			input_set_abs_params(dev2, ABS_PRESSURE, 0, 127, 0, 0);
> +		}
>  		input_set_capability(dev2, EV_KEY, BTN_LEFT);
>  		input_set_capability(dev2, EV_KEY, BTN_RIGHT);
>  		input_set_capability(dev2, EV_KEY, BTN_MIDDLE);

This seems wrong, reporting ABS_PRESSURE on a relative device. And yes, this
really is a relative device (it sends repeated delta events when you
keep pushing at the same force, rather then sending a single coordinate
value).

Maybe we need a REL_PRESSURE for cases like this ?

Regards,

Hans


--
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
Ben Gamari June 10, 2016, 10:21 a.m. UTC | #2 
Hans de Goede <hdegoede@redhat.com> writes:

> Hi,
>>
>> @@ -2996,6 +2999,10 @@ int alps_init(struct psmouse *psmouse)
>>
>>  		input_set_capability(dev2, EV_REL, REL_X);
>>  		input_set_capability(dev2, EV_REL, REL_Y);
>> +		if (priv->flags & ALPS_DUALPOINT_WITH_PRESSURE) {
>> +			input_set_capability(dev2, EV_ABS, ABS_PRESSURE);
>> +			input_set_abs_params(dev2, ABS_PRESSURE, 0, 127, 0, 0);
>> +		}
>>  		input_set_capability(dev2, EV_KEY, BTN_LEFT);
>>  		input_set_capability(dev2, EV_KEY, BTN_RIGHT);
>>  		input_set_capability(dev2, EV_KEY, BTN_MIDDLE);
>
> This seems wrong, reporting ABS_PRESSURE on a relative device. And yes, this
> really is a relative device (it sends repeated delta events when you
> keep pushing at the same force, rather then sending a single coordinate
> value).
>
> Maybe we need a REL_PRESSURE for cases like this ?
>
Fair enough. However, if such a thing were added how would one specify
the range of the value (which seems necessary for the pressure to have
any meaning whatsoever)? It seems odd that the distinctions of
absolute/relative and bounded/unbounded values are conflated in the
event interface. It's not clear to me what input_set_abs_params isn't
instead input_set_params.

Cheers

- Ben
Hans de Goede June 10, 2016, 10:57 a.m. UTC | #3 
Hi,

On 10-06-16 12:21, Ben Gamari wrote:
> Hans de Goede <hdegoede@redhat.com> writes:
>
>> Hi,
>>>
>>> @@ -2996,6 +2999,10 @@ int alps_init(struct psmouse *psmouse)
>>>
>>>  		input_set_capability(dev2, EV_REL, REL_X);
>>>  		input_set_capability(dev2, EV_REL, REL_Y);
>>> +		if (priv->flags & ALPS_DUALPOINT_WITH_PRESSURE) {
>>> +			input_set_capability(dev2, EV_ABS, ABS_PRESSURE);
>>> +			input_set_abs_params(dev2, ABS_PRESSURE, 0, 127, 0, 0);
>>> +		}
>>>  		input_set_capability(dev2, EV_KEY, BTN_LEFT);
>>>  		input_set_capability(dev2, EV_KEY, BTN_RIGHT);
>>>  		input_set_capability(dev2, EV_KEY, BTN_MIDDLE);
>>
>> This seems wrong, reporting ABS_PRESSURE on a relative device. And yes, this
>> really is a relative device (it sends repeated delta events when you
>> keep pushing at the same force, rather then sending a single coordinate
>> value).
>>
>> Maybe we need a REL_PRESSURE for cases like this ?
>>
> Fair enough. However, if such a thing were added how would one specify
> the range of the value (which seems necessary for the pressure to have
> any meaning whatsoever)?

Good question, adding Peter Hutterer to the Cc.

Peter, some of the newer alps pointing sticks can also report pressure,
which is somewhat of an ill fit for the relative events interface
otherwise used with pointing sticks, do you have any suggestions on
how to deal with this ?

Regards,

Hans
--
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
Peter Hutterer June 13, 2016, 12:21 a.m. UTC | #4 
On Fri, Jun 10, 2016 at 12:57:12PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 10-06-16 12:21, Ben Gamari wrote:
> > Hans de Goede <hdegoede@redhat.com> writes:
> > 
> > > Hi,
> > > > 
> > > > @@ -2996,6 +2999,10 @@ int alps_init(struct psmouse *psmouse)
> > > > 
> > > >  		input_set_capability(dev2, EV_REL, REL_X);
> > > >  		input_set_capability(dev2, EV_REL, REL_Y);
> > > > +		if (priv->flags & ALPS_DUALPOINT_WITH_PRESSURE) {
> > > > +			input_set_capability(dev2, EV_ABS, ABS_PRESSURE);
> > > > +			input_set_abs_params(dev2, ABS_PRESSURE, 0, 127, 0, 0);
> > > > +		}
> > > >  		input_set_capability(dev2, EV_KEY, BTN_LEFT);
> > > >  		input_set_capability(dev2, EV_KEY, BTN_RIGHT);
> > > >  		input_set_capability(dev2, EV_KEY, BTN_MIDDLE);
> > > 
> > > This seems wrong, reporting ABS_PRESSURE on a relative device. And yes, this
> > > really is a relative device (it sends repeated delta events when you
> > > keep pushing at the same force, rather then sending a single coordinate
> > > value).
> > > 
> > > Maybe we need a REL_PRESSURE for cases like this ?
> > > 
> > Fair enough. However, if such a thing were added how would one specify
> > the range of the value (which seems necessary for the pressure to have
> > any meaning whatsoever)?
> 
> Good question, adding Peter Hutterer to the Cc.
> 
> Peter, some of the newer alps pointing sticks can also report pressure,
> which is somewhat of an ill fit for the relative events interface
> otherwise used with pointing sticks, do you have any suggestions on
> how to deal with this ?

I think ABS_PRESSURE is fine here provided the ranges are set up correctly
and he device really sends an absolute value. It might confuse userspace at
first but there is no real requirement for a device to stick to one
interface only.

REL_PRESSURE would be a lot harder to handle because we don't have a
reference frame on what the permitted axis ranges could be and it would make
the actual value effectively meaningless (without a MOUSE_DPI-like database)

Cheers,
   Peter
--
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
diff mbox

Patch

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 7874f4f..25d2cad 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -103,6 +103,7 @@  static const struct alps_nibble_commands alps_v6_nibble_commands[] = {
 					   6-byte ALPS packet */
 #define ALPS_STICK_BITS		0x100	/* separate stick button bits */
 #define ALPS_BUTTONPAD		0x200	/* device is a clickpad */
+#define ALPS_DUALPOINT_WITH_PRESSURE		0x400	/* device can report trackpoint pressure */
 
 static const struct alps_model_info alps_model_data[] = {
 	{ { 0x32, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },	/* Toshiba Salellite Pro M10 */
@@ -1270,9 +1271,11 @@  static int alps_decode_ss4_v2(struct alps_fields *f,
 		} else {
 			int x = (s8)(((p[0] & 1) << 7) | (p[1] & 0x7f));
 			int y = (s8)(((p[3] & 1) << 7) | (p[2] & 0x7f));
+			int pressure = (s8)(p[4] & 0x7f);
 
 			input_report_rel(priv->dev2, REL_X, x);
 			input_report_rel(priv->dev2, REL_Y, -y);
+			input_report_abs(priv->dev2, ABS_PRESSURE, pressure);
 		}
 		break;
 
@@ -2996,6 +2999,10 @@  int alps_init(struct psmouse *psmouse)
 
 		input_set_capability(dev2, EV_REL, REL_X);
 		input_set_capability(dev2, EV_REL, REL_Y);
+		if (priv->flags & ALPS_DUALPOINT_WITH_PRESSURE) {
+			input_set_capability(dev2, EV_ABS, ABS_PRESSURE);
+			input_set_abs_params(dev2, ABS_PRESSURE, 0, 127, 0, 0);
+		}
 		input_set_capability(dev2, EV_KEY, BTN_LEFT);
 		input_set_capability(dev2, EV_KEY, BTN_RIGHT);
 		input_set_capability(dev2, EV_KEY, BTN_MIDDLE);