Add driver for mouse logitech M560

Commit Message

Goffredo Baroncelli May 1, 2015, 8:43 a.m. UTC

From: Goffredo Baroncelli <[e-mail]>

The Logitech M560 is a wireless mouse designed for windows 8 which uses
the unifying receiver.
Compared to a standard one, some buttons (the middle one and the
two ones placed on the side) are bound to a key combination
instead of a generating classic "mouse" button events.

The device shows up as a mouse and keyboard combination: when the middle
button is pressed it sends a key (as keyboard) combination, the same
happens for the two side button. The left/right/wheel work as expected
from a mouse. To complicate things further, the middle button sends
different keys combinations between odd and even presses.
In the "even" press it also sends a left click. But the worst thing
is that no event is generated when the middle button is released.

It is possible to re-configure the mouse sending a command (see function
m560_send_config_command()). After this command the mouse sends some
useful data when the buttons are pressed and/or released.

Signed-off-by: Goffredo Baroncelli <[e-mail]>
---
 drivers/hid/hid-logitech-hidpp.c | 242 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 239 insertions(+), 3 deletions(-)

Comments

Goffredo Baroncelli May 1, 2015, 8:56 a.m. UTC | #1

Hi All,

This is the 4th attempt for a Logitech mouse M560 hid driver.
In the previous attempt I didn't check carefully the arguments of the function
m560_raw_event() this leaded sometime to a system crash.
On the basis of the feedback received, I made some cleanup to the code.

The patch is against v4.1-rc1.

Special thanks to Antonio and Benjamin for their support and testing.

BR
G.Baroncelli


Changelog:
- v1 first issue
- v2 accepted Benjamin Tissoires suggestions
- v3 use input_report*() function instead of rewriting the hid-report
- v4 add check of the arguments in m560_raw_event()


--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5

Antonio Ospite May 5, 2015, 3:36 p.m. UTC | #2

On Fri,  1 May 2015 10:43:33 +0200
Goffredo Baroncelli <[e-mail]> wrote:

> From: Goffredo Baroncelli <[e-mail]>
> 
> The Logitech M560 is a wireless mouse designed for windows 8 which uses
> the unifying receiver.
> Compared to a standard one, some buttons (the middle one and the
> two ones placed on the side) are bound to a key combination
> instead of a generating classic "mouse" button events.
> 
> The device shows up as a mouse and keyboard combination: when the middle
> button is pressed it sends a key (as keyboard) combination, the same
> happens for the two side button. The left/right/wheel work as expected
> from a mouse. To complicate things further, the middle button sends
> different keys combinations between odd and even presses.
> In the "even" press it also sends a left click. But the worst thing
> is that no event is generated when the middle button is released.
> 
> It is possible to re-configure the mouse sending a command (see function
> m560_send_config_command()). After this command the mouse sends some
> useful data when the buttons are pressed and/or released.
> 
> Signed-off-by: Goffredo Baroncelli <[e-mail]>

Reviewed-by: Antonio Ospite <[e-mail]>

Thanks a lot Goffredo, there are some final comments inlined below, they
are not blockers tho, and IMHO they could be addressed in future patches
if you didn't feel like re-submitting.

BTW in your future patches add the version suffix to the patch itself,
you can use the "--subject-prefix" option of git format-patch.

Annotations and patch history can go after the '---' marker as usual.

Sorry for not catching everything in one pass.

> ---
>  drivers/hid/hid-logitech-hidpp.c | 242 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 239 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index b3cf6fd..e7dc23e 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -40,8 +40,9 @@ MODULE_PARM_DESC(disable_raw_mode,
>  #define HIDPP_REPORT_LONG_LENGTH		20
>  
>  #define HIDPP_QUIRK_CLASS_WTP			BIT(0)
> +#define HIDPP_QUIRK_CLASS_M560			BIT(1)
>  
> -/* bits 1..20 are reserved for classes */
> +/* bits 2..20 are reserved for classes */
>  #define HIDPP_QUIRK_DELAYED_INIT		BIT(21)
>  #define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS	BIT(22)
>  #define HIDPP_QUIRK_MULTI_INPUT			BIT(23)
> @@ -941,6 +942,220 @@ static int wtp_connect(struct hid_device *hdev, bool connected)
>  			true, true);
>  }
>  

As Benjamin suggested, a comment saying that this is a duplicate of
drivers/hid/hid-core.c::extract() could be useful here.

> +static u32 hidpp_extract(u8 *report, unsigned offset, unsigned n)
> +{
> +	u64 x;
> +
> +	report += offset >> 3;  /* adjust byte index */
> +	offset &= 7;            /* now only need bit offset into one byte */
> +	x = get_unaligned_le64(report);
> +	x = (x >> offset) & ((1ULL << n) - 1);  /* extract bit field */
> +	return (u32)x;
> +}
> +
> +/* ------------------------------------------------------------------------- */
> +/* Logitech M560 devices                                                     */
> +/* ------------------------------------------------------------------------- */
> +
> +/*
> + * Logitech M560 protocol overview
> + *
> + * The Logitech M560 mouse, is designed for windows 8. When the middle and/or
> + * the sides buttons are pressed, it sends some keyboard keys events
> + * instead of buttons ones.
> + * To complicate things further, the middle button keys sequence
> + * is different from the odd press and the even press.
> + *
> + * forward button -> Super_R
> + * backward button -> Super_L+'d' (press only)
> + * middle button -> 1st time: Alt_L+SuperL+XF86TouchpadOff (press only)
> + *                  2nd time: left-click (press only)
> + * NB: press-only means that when the button is pressed, the
> + * KeyPress/ButtonPress and KeyRelease/ButtonRelease events are generated
> + * together sequentially; instead when the button is released, no event is
> + * generated !
> + *
> + * With the command
> + *	10<xx>0a 3500af03 (where <xx> is the mouse id),
> + * the mouse reacts differently:
> + * - it never send a keyboard key event

it never sends ...
             ^
> + * - for the three mouse button it sends:
> + *	middle button               press   11<xx>0a 3500af00...
> + *	side 1 button (forward)     press   11<xx>0a 3500b000...
> + *	side 2 button (backward)    press   11<xx>0a 3500ae00...
> + *	middle/side1/side2 button   release 11<xx>0a 35000000...
> + */
> +
> +static const u8 m560_config_parameter[] = {0x00, 0xaf, 0x03};
> +
> +struct m560_private_data {
> +	struct input_dev *input;
> +};
> +
> +/* how the button are mapped in the report */

how buttons are mapped ...
          ^
> +#define M560_MOUSE_BTN_LEFT		0x01
> +#define M560_MOUSE_BTN_RIGHT		0x02
> +#define M560_MOUSE_BTN_MIDDLE		0x04
> +#define M560_MOUSE_BTN_WHEEL_LEFT	0x08
> +#define M560_MOUSE_BTN_WHEEL_RIGHT	0x10
> +#define M560_MOUSE_BTN_FORWARD		0x20
> +#define M560_MOUSE_BTN_BACKWARD		0x40

These definitions for _MIDDLE, _FORWARD and _BACKWARD are not used
anymore in the code below. 

> +#define M560_SUB_ID			0x0a
> +#define M560_BUTTON_MODE_REGISTER	0x35
> +
> +static int m560_send_config_command(struct hid_device *hdev, bool connected)
> +{
> +	struct hidpp_report response;
> +	struct hidpp_device *hidpp_dev = hid_get_drvdata(hdev);
> +

hidpp_dev could be declared first, just to follow the style of the other
functions.

> +	if (!connected)
> +		return -ENODEV;
> +
> +	return hidpp_send_rap_command_sync(
> +		hidpp_dev,
> +		REPORT_ID_HIDPP_SHORT,
> +		M560_SUB_ID,
> +		M560_BUTTON_MODE_REGISTER,
> +		(u8 *)m560_config_parameter,
> +		sizeof(m560_config_parameter),
> +		&response
> +	);
> +

The empty line above can be dropped.

> +}
> +
> +static int m560_allocate(struct hid_device *hdev)
> +{
> +	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> +	struct m560_private_data *d;
> +
> +	d = devm_kzalloc(&hdev->dev, sizeof(struct m560_private_data),
> +			GFP_KERNEL);
> +	if (!d)
> +		return -ENOMEM;
> +
> +	hidpp->private_data = d;
> +
> +	return 0;
> +};
> +
> +static int m560_raw_event(struct hid_device *hdev, u8 *data, int size)
> +{
> +	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> +	struct m560_private_data *mydata = hidpp->private_data;
> +
> +

The extra empty line above can be dropped.

> +	/* sanity check */
> +	if (!mydata || !mydata->input)
> +		return 1;

And one empty line might be added here.

> +	if (size < 7)
> +		return 1;
> +
> +	/* exit if the data is not a mouse related report */
> +	if (data[0] != 0x02 && data[2] != M560_SUB_ID)

What are the possible values of data[0] that you observed?
Can it only be 0x02 and 0x11?

If we wanted to be stricter we could anticipate the full validation and
then simplify the ifs below to just check the report type, something
like:

	bool valid_mouse_report;
	...

	valid_mouse_report = (data[0] == 0x02 ||
	                      (data[0] == REPORT_ID_HIDPP_LONG &&
	                       data[2] == M560_SUB_ID &&
	                       data[6] == 0x00))

	if (!valid_mouse_report)
		return 0;

	if (data[0] == REPORT_ID_HIDPP_LONG) {
		...
	} else /* if (data[0] == 0x02) */ {
		...
	}

but maybe this is overkill.

> +		return 0;
> +
> +	if (data[0] == REPORT_ID_HIDPP_LONG &&
> +	    data[2] == M560_SUB_ID && data[6] == 0x00) {
> +		/*
> +		 * m560 mouse report for middle, forward and backward button
> +		 *
> +		 * data[0] = 0x11
> +		 * data[1] = device-id
> +		 * data[2] = 0x0a
> +		 * data[5] = 0xaf -> middle
> +		 *	     0xb0 -> forward
> +		 *	     0xae -> backward
> +		 *	     0x00 -> release all
> +		 * data[6] = 0x00
> +		 */
> +
> +		switch (data[5]) {
> +		case 0xaf:
> +			input_report_key(mydata->input, BTN_MIDDLE, 1);
> +			break;
> +		case 0xb0:
> +			input_report_key(mydata->input, BTN_FORWARD, 1);
> +			break;
> +		case 0xae:
> +			input_report_key(mydata->input, BTN_BACK, 1);
> +			break;
> +		case 0x00:
> +			input_report_key(mydata->input, BTN_BACK, 0);
> +			input_report_key(mydata->input, BTN_FORWARD, 0);
> +			input_report_key(mydata->input, BTN_MIDDLE, 0);
> +			break;

I am curios about the values of data[5] (and data[6]?) when multiple
buttons are pressed, it's a pity the hardware does not allow to combine
them consistently.

> +		default:
> +			return 1;
> +		}
> +		input_sync(mydata->input);
> +
> +	} else if (data[0] == 0x02) {
> +		/*
> +		 * Logitech M560 mouse report
> +		 *
> +		 * data[0] = type (0x02)
> +		 * data[1..2] = buttons
> +		 * data[3..5] = xy
> +		 * data[6] = wheel
> +		 */
> +
> +		int v;
> +
> +		input_report_key(mydata->input, BTN_LEFT,
> +			!!(data[1] & M560_MOUSE_BTN_LEFT));
> +		input_report_key(mydata->input, BTN_RIGHT,
> +			!!(data[1] & M560_MOUSE_BTN_RIGHT));
> +
> +		if (data[1] & M560_MOUSE_BTN_WHEEL_LEFT)
> +			input_report_rel(mydata->input, REL_HWHEEL, -1);
> +		else if (data[1] & M560_MOUSE_BTN_WHEEL_RIGHT)
> +			input_report_rel(mydata->input, REL_HWHEEL, 1);
> +
> +		v = hid_snto32(hidpp_extract(data+3, 0, 12), 12);
> +		input_report_rel(mydata->input, REL_X, v);
> +
> +		v = hid_snto32(hidpp_extract(data+3, 12, 12), 12);
> +		input_report_rel(mydata->input, REL_Y, v);
> +
> +		v = hid_snto32(data[6], 8);
> +		input_report_rel(mydata->input, REL_WHEEL, v);
> +
> +		input_sync(mydata->input);
> +	}
> +
> +	return 0;
> +}
> +
> +static void m560_populate_input(struct hidpp_device *hidpp,
> +		struct input_dev *input_dev, bool origin_is_hid_core)
> +{
> +	struct m560_private_data *mydata = hidpp->private_data;
> +
> +	mydata->input = input_dev;
> +
> +	__set_bit(EV_KEY, mydata->input->evbit);
> +	__set_bit(BTN_MIDDLE, mydata->input->keybit);
> +	__set_bit(BTN_RIGHT, mydata->input->keybit);
> +	__set_bit(BTN_LEFT, mydata->input->keybit);
> +	__set_bit(BTN_BACK, mydata->input->keybit);
> +	__set_bit(BTN_FORWARD, mydata->input->keybit);
> +
> +	__set_bit(EV_REL, mydata->input->evbit);
> +	__set_bit(REL_X, mydata->input->relbit);
> +	__set_bit(REL_Y, mydata->input->relbit);
> +	__set_bit(REL_WHEEL, mydata->input->relbit);
> +	__set_bit(REL_HWHEEL, mydata->input->relbit);
> +

The empty line above can be dropped.

> +}
> +
> +static int m560_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> +		struct hid_field *field, struct hid_usage *usage,
> +		unsigned long **bit, int *max)
> +{
> +	return -1;
> +}
> +
>  /* -------------------------------------------------------------------------- */
>  /* Generic HID++ devices                                                      */
>  /* -------------------------------------------------------------------------- */
> @@ -953,6 +1168,10 @@ static int hidpp_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  
>  	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
>  		return wtp_input_mapping(hdev, hi, field, usage, bit, max);
> +	else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560 &&
> +		 field->application != HID_GD_MOUSE)
> +			return m560_input_mapping(hdev, hi, field, usage,
> +						  bit, max);

To be pedantic, the return above has one extra indentation level, it
should be at the same level as the one in the other branch.

>  
>  	return 0;
>  }
> @@ -962,6 +1181,8 @@ static void hidpp_populate_input(struct hidpp_device *hidpp,
>  {
>  	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
>  		wtp_populate_input(hidpp, input, origin_is_hid_core);
> +	else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560)
> +		m560_populate_input(hidpp, input, origin_is_hid_core);
>  }
>  
>  static void hidpp_input_configured(struct hid_device *hdev,
> @@ -1049,6 +1270,8 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
>  
>  	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
>  		return wtp_raw_event(hdev, data, size);
> +	else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560)
> +		return m560_raw_event(hdev, data, size);
>  
>  	return 0;
>  }
> @@ -1126,6 +1349,10 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>  		ret = wtp_connect(hdev, connected);
>  		if (ret)
>  			return;
> +	} else if ((hidpp->quirks & HIDPP_QUIRK_CLASS_M560) && connected) {

No need to check for connected here anymore, the check is now in
m560_send_config_command()

> +		ret = m560_send_config_command(hdev, connected);
> +		if (ret)
> +			return;
>  	}
>  
>  	if (!connected || hidpp->delayed_input)
> @@ -1201,7 +1428,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
>  		ret = wtp_allocate(hdev, id);
>  		if (ret)
> -			goto wtp_allocate_fail;
> +			goto allocate_fail;
> +	}
> +	if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {

else if here?

> +		ret = m560_allocate(hdev);
> +		if (ret)
> +			goto allocate_fail;
>  	}
>  
>  	INIT_WORK(&hidpp->work, delayed_work_cb);
> @@ -1268,7 +1500,7 @@ hid_hw_start_fail:
>  hid_parse_fail:
>  	cancel_work_sync(&hidpp->work);
>  	mutex_destroy(&hidpp->send_mutex);
> -wtp_allocate_fail:
> +allocate_fail:
>  	hid_set_drvdata(hdev, NULL);
>  	return ret;
>  }
> @@ -1301,6 +1533,10 @@ static const struct hid_device_id hidpp_devices[] = {
>  		USB_VENDOR_ID_LOGITECH, 0x4102),
>  	  .driver_data = HIDPP_QUIRK_DELAYED_INIT | HIDPP_QUIRK_MULTI_INPUT |
>  			 HIDPP_QUIRK_CLASS_WTP },
> +	{ /* Mouse logitech M560 */
> +	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
> +		USB_VENDOR_ID_LOGITECH, 0x402d),
> +	  .driver_data = HIDPP_QUIRK_DELAYED_INIT | HIDPP_QUIRK_CLASS_M560 },
>  
>  	{ HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
>  		USB_VENDOR_ID_LOGITECH, HID_ANY_ID)},
> -- 
> 2.1.4
>

Antonio Ospite May 6, 2015, 6:48 a.m. UTC | #3

On Tue, 5 May 2015 17:36:59 +0200
Antonio Ospite <[e-mail]> wrote:

> On Fri,  1 May 2015 10:43:33 +0200
> Goffredo Baroncelli <[e-mail]> wrote:
> 
[...]
> > +	/* exit if the data is not a mouse related report */
> > +	if (data[0] != 0x02 && data[2] != M560_SUB_ID)
> 
> What are the possible values of data[0] that you observed?
> Can it only be 0x02 and 0x11?
> 
> If we wanted to be stricter we could anticipate the full validation and
> then simplify the ifs below to just check the report type, something
> like:
> 
> 	bool valid_mouse_report;
> 	...
> 
> 	valid_mouse_report = (data[0] == 0x02 ||
> 	                      (data[0] == REPORT_ID_HIDPP_LONG &&
> 	                       data[2] == M560_SUB_ID &&
> 	                       data[6] == 0x00))
> 
> 	if (!valid_mouse_report)
> 		return 0;
> 
> 	if (data[0] == REPORT_ID_HIDPP_LONG) {
> 		...
> 	} else /* if (data[0] == 0x02) */ {
> 		...
> 	}
> 
> but maybe this is overkill.
> 

Or maybe just using an else, without the preventive validation, would
already improve readability, checking things only once:

	if (data[0] == REPORT_ID_HIDPP_LONG &&
	    data[2] == M560_SUB_ID && data[6] == 0x00) {
		...
	} else if (data[0] == 0x02) {
		...
	} else {
		/* unsupported report */
		return 0;
	}

But again, this is just a suggestion.

Ciao,
   Antonio

Antonio Ospite May 6, 2015, 7:35 a.m. UTC | #4

On Wed, 6 May 2015 08:48:14 +0200
Antonio Ospite <[e-mail]> wrote:

> On Tue, 5 May 2015 17:36:59 +0200
> Antonio Ospite <[e-mail]> wrote:
> 
> > On Fri,  1 May 2015 10:43:33 +0200
> > Goffredo Baroncelli <[e-mail]> wrote:
> > 
> [...]
> > > +	/* exit if the data is not a mouse related report */
> > > +	if (data[0] != 0x02 && data[2] != M560_SUB_ID)
> > 
> > What are the possible values of data[0] that you observed?
> > Can it only be 0x02 and 0x11?
> > 
> > If we wanted to be stricter we could anticipate the full validation and
> > then simplify the ifs below to just check the report type, something
> > like:
> > 
> > 	bool valid_mouse_report;
> > 	...
> > 
> > 	valid_mouse_report = (data[0] == 0x02 ||
> > 	                      (data[0] == REPORT_ID_HIDPP_LONG &&
> > 	                       data[2] == M560_SUB_ID &&
> > 	                       data[6] == 0x00))
> > 
> > 	if (!valid_mouse_report)
> > 		return 0;
> > 
> > 	if (data[0] == REPORT_ID_HIDPP_LONG) {
> > 		...
> > 	} else /* if (data[0] == 0x02) */ {
> > 		...
> > 	}
> > 
> > but maybe this is overkill.
> > 
> 
> Or maybe just using an else, without the preventive validation, would
> already improve readability, checking things only once:
> 
> 	if (data[0] == REPORT_ID_HIDPP_LONG &&
> 	    data[2] == M560_SUB_ID && data[6] == 0x00) {
> 		...
> 	} else if (data[0] == 0x02) {
> 		...
> 	} else {
> 		/* unsupported report */
> 		return 0;
> 	}
> 

If the function returns 0 at the end, then the else here is even
redundant.

We may return 1 at the end of the function to differentiate between "no
action performed (0)" and "no further processing (1)" like stated in
include/linux/hid.h line 661, even if I don't see the core handling the
difference between the two return values when calling the raw_event
callback.

Benjamin, can you comment about what the convention is here? I am
referring also to the checks at the begin of m560_raw_event().

Thanks,
   Antonio

Benjamin Tissoires May 6, 2015, 1:27 p.m. UTC | #5

On Wed, May 6, 2015 at 3:35 AM, Antonio Ospite <[e-mail]> wrote:
> On Wed, 6 May 2015 08:48:14 +0200
> Antonio Ospite <[e-mail]> wrote:
>
>> On Tue, 5 May 2015 17:36:59 +0200
>> Antonio Ospite <[e-mail]> wrote:
>>
>> > On Fri,  1 May 2015 10:43:33 +0200
>> > Goffredo Baroncelli <[e-mail]> wrote:
>> >
>> [...]
>> > > + /* exit if the data is not a mouse related report */
>> > > + if (data[0] != 0x02 && data[2] != M560_SUB_ID)
>> >
>> > What are the possible values of data[0] that you observed?
>> > Can it only be 0x02 and 0x11?
>> >
>> > If we wanted to be stricter we could anticipate the full validation and
>> > then simplify the ifs below to just check the report type, something
>> > like:
>> >
>> >     bool valid_mouse_report;
>> >     ...
>> >
>> >     valid_mouse_report = (data[0] == 0x02 ||
>> >                           (data[0] == REPORT_ID_HIDPP_LONG &&
>> >                            data[2] == M560_SUB_ID &&
>> >                            data[6] == 0x00))
>> >
>> >     if (!valid_mouse_report)
>> >             return 0;
>> >
>> >     if (data[0] == REPORT_ID_HIDPP_LONG) {
>> >             ...
>> >     } else /* if (data[0] == 0x02) */ {
>> >             ...
>> >     }
>> >
>> > but maybe this is overkill.
>> >
>>
>> Or maybe just using an else, without the preventive validation, would
>> already improve readability, checking things only once:
>>
>>       if (data[0] == REPORT_ID_HIDPP_LONG &&
>>           data[2] == M560_SUB_ID && data[6] == 0x00) {
>>               ...
>>       } else if (data[0] == 0x02) {
>>               ...
>>       } else {
>>               /* unsupported report */
>>               return 0;
>>       }
>>
>
> If the function returns 0 at the end, then the else here is even
> redundant.
>
> We may return 1 at the end of the function to differentiate between "no
> action performed (0)" and "no further processing (1)" like stated in
> include/linux/hid.h line 661, even if I don't see the core handling the
> difference between the two return values when calling the raw_event
> callback.
>
> Benjamin, can you comment about what the convention is here? I am

As you saw, we used to care, but we don't anymore. "no further
processing (1)" is now simply ignored and nobody took the time to fix
it properly. The last change which introduced that was b1a1442 ("HID:
core: fix reporting of raw events"), and it fixed a real problem. But
that means that we simply ignore the positive return value.

There may be a chance that we fix that in the future (I have too much
on my plate right now), so I would advice to follow the documentation
even if it is not entirely true anymore.

> referring also to the checks at the begin of m560_raw_event().

Again, there is no strict rule. The less operations you do, the better
it is (it will be called at each report, so potentially can add a lot
of head over).  Also the clearer it is, the better. That's why I am
often tempted to add extra functions for each type of processing with
a clear "return m560_process_hidpp_report()". Something like that :)

>
> Thanks,
>    Antonio

Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index b3cf6fd..e7dc23e 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -40,8 +40,9 @@  MODULE_PARM_DESC(disable_raw_mode,
 #define HIDPP_REPORT_LONG_LENGTH		20
 
 #define HIDPP_QUIRK_CLASS_WTP			BIT(0)
+#define HIDPP_QUIRK_CLASS_M560			BIT(1)
 
-/* bits 1..20 are reserved for classes */
+/* bits 2..20 are reserved for classes */
 #define HIDPP_QUIRK_DELAYED_INIT		BIT(21)
 #define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS	BIT(22)
 #define HIDPP_QUIRK_MULTI_INPUT			BIT(23)
@@ -941,6 +942,220 @@  static int wtp_connect(struct hid_device *hdev, bool connected)
 			true, true);
 }
 
+static u32 hidpp_extract(u8 *report, unsigned offset, unsigned n)
+{
+	u64 x;
+
+	report += offset >> 3;  /* adjust byte index */
+	offset &= 7;            /* now only need bit offset into one byte */
+	x = get_unaligned_le64(report);
+	x = (x >> offset) & ((1ULL << n) - 1);  /* extract bit field */
+	return (u32)x;
+}
+
+/* ------------------------------------------------------------------------- */
+/* Logitech M560 devices                                                     */
+/* ------------------------------------------------------------------------- */
+
+/*
+ * Logitech M560 protocol overview
+ *
+ * The Logitech M560 mouse, is designed for windows 8. When the middle and/or
+ * the sides buttons are pressed, it sends some keyboard keys events
+ * instead of buttons ones.
+ * To complicate things further, the middle button keys sequence
+ * is different from the odd press and the even press.
+ *
+ * forward button -> Super_R
+ * backward button -> Super_L+'d' (press only)
+ * middle button -> 1st time: Alt_L+SuperL+XF86TouchpadOff (press only)
+ *                  2nd time: left-click (press only)
+ * NB: press-only means that when the button is pressed, the
+ * KeyPress/ButtonPress and KeyRelease/ButtonRelease events are generated
+ * together sequentially; instead when the button is released, no event is
+ * generated !
+ *
+ * With the command
+ *	10<xx>0a 3500af03 (where <xx> is the mouse id),
+ * the mouse reacts differently:
+ * - it never send a keyboard key event
+ * - for the three mouse button it sends:
+ *	middle button               press   11<xx>0a 3500af00...
+ *	side 1 button (forward)     press   11<xx>0a 3500b000...
+ *	side 2 button (backward)    press   11<xx>0a 3500ae00...
+ *	middle/side1/side2 button   release 11<xx>0a 35000000...
+ */
+
+static const u8 m560_config_parameter[] = {0x00, 0xaf, 0x03};
+
+struct m560_private_data {
+	struct input_dev *input;
+};
+
+/* how the button are mapped in the report */
+#define M560_MOUSE_BTN_LEFT		0x01
+#define M560_MOUSE_BTN_RIGHT		0x02
+#define M560_MOUSE_BTN_MIDDLE		0x04
+#define M560_MOUSE_BTN_WHEEL_LEFT	0x08
+#define M560_MOUSE_BTN_WHEEL_RIGHT	0x10
+#define M560_MOUSE_BTN_FORWARD		0x20
+#define M560_MOUSE_BTN_BACKWARD		0x40
+
+#define M560_SUB_ID			0x0a
+#define M560_BUTTON_MODE_REGISTER	0x35
+
+static int m560_send_config_command(struct hid_device *hdev, bool connected)
+{
+	struct hidpp_report response;
+	struct hidpp_device *hidpp_dev = hid_get_drvdata(hdev);
+
+	if (!connected)
+		return -ENODEV;
+
+	return hidpp_send_rap_command_sync(
+		hidpp_dev,
+		REPORT_ID_HIDPP_SHORT,
+		M560_SUB_ID,
+		M560_BUTTON_MODE_REGISTER,
+		(u8 *)m560_config_parameter,
+		sizeof(m560_config_parameter),
+		&response
+	);
+
+}
+
+static int m560_allocate(struct hid_device *hdev)
+{
+	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+	struct m560_private_data *d;
+
+	d = devm_kzalloc(&hdev->dev, sizeof(struct m560_private_data),
+			GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+
+	hidpp->private_data = d;
+
+	return 0;
+};
+
+static int m560_raw_event(struct hid_device *hdev, u8 *data, int size)
+{
+	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+	struct m560_private_data *mydata = hidpp->private_data;
+
+
+	/* sanity check */
+	if (!mydata || !mydata->input)
+		return 1;
+	if (size < 7)
+		return 1;
+
+	/* exit if the data is not a mouse related report */
+	if (data[0] != 0x02 && data[2] != M560_SUB_ID)
+		return 0;
+
+	if (data[0] == REPORT_ID_HIDPP_LONG &&
+	    data[2] == M560_SUB_ID && data[6] == 0x00) {
+		/*
+		 * m560 mouse report for middle, forward and backward button
+		 *
+		 * data[0] = 0x11
+		 * data[1] = device-id
+		 * data[2] = 0x0a
+		 * data[5] = 0xaf -> middle
+		 *	     0xb0 -> forward
+		 *	     0xae -> backward
+		 *	     0x00 -> release all
+		 * data[6] = 0x00
+		 */
+
+		switch (data[5]) {
+		case 0xaf:
+			input_report_key(mydata->input, BTN_MIDDLE, 1);
+			break;
+		case 0xb0:
+			input_report_key(mydata->input, BTN_FORWARD, 1);
+			break;
+		case 0xae:
+			input_report_key(mydata->input, BTN_BACK, 1);
+			break;
+		case 0x00:
+			input_report_key(mydata->input, BTN_BACK, 0);
+			input_report_key(mydata->input, BTN_FORWARD, 0);
+			input_report_key(mydata->input, BTN_MIDDLE, 0);
+			break;
+		default:
+			return 1;
+		}
+		input_sync(mydata->input);
+
+	} else if (data[0] == 0x02) {
+		/*
+		 * Logitech M560 mouse report
+		 *
+		 * data[0] = type (0x02)
+		 * data[1..2] = buttons
+		 * data[3..5] = xy
+		 * data[6] = wheel
+		 */
+
+		int v;
+
+		input_report_key(mydata->input, BTN_LEFT,
+			!!(data[1] & M560_MOUSE_BTN_LEFT));
+		input_report_key(mydata->input, BTN_RIGHT,
+			!!(data[1] & M560_MOUSE_BTN_RIGHT));
+
+		if (data[1] & M560_MOUSE_BTN_WHEEL_LEFT)
+			input_report_rel(mydata->input, REL_HWHEEL, -1);
+		else if (data[1] & M560_MOUSE_BTN_WHEEL_RIGHT)
+			input_report_rel(mydata->input, REL_HWHEEL, 1);
+
+		v = hid_snto32(hidpp_extract(data+3, 0, 12), 12);
+		input_report_rel(mydata->input, REL_X, v);
+
+		v = hid_snto32(hidpp_extract(data+3, 12, 12), 12);
+		input_report_rel(mydata->input, REL_Y, v);
+
+		v = hid_snto32(data[6], 8);
+		input_report_rel(mydata->input, REL_WHEEL, v);
+
+		input_sync(mydata->input);
+	}
+
+	return 0;
+}
+
+static void m560_populate_input(struct hidpp_device *hidpp,
+		struct input_dev *input_dev, bool origin_is_hid_core)
+{
+	struct m560_private_data *mydata = hidpp->private_data;
+
+	mydata->input = input_dev;
+
+	__set_bit(EV_KEY, mydata->input->evbit);
+	__set_bit(BTN_MIDDLE, mydata->input->keybit);
+	__set_bit(BTN_RIGHT, mydata->input->keybit);
+	__set_bit(BTN_LEFT, mydata->input->keybit);
+	__set_bit(BTN_BACK, mydata->input->keybit);
+	__set_bit(BTN_FORWARD, mydata->input->keybit);
+
+	__set_bit(EV_REL, mydata->input->evbit);
+	__set_bit(REL_X, mydata->input->relbit);
+	__set_bit(REL_Y, mydata->input->relbit);
+	__set_bit(REL_WHEEL, mydata->input->relbit);
+	__set_bit(REL_HWHEEL, mydata->input->relbit);
+
+}
+
+static int m560_input_mapping(struct hid_device *hdev, struct hid_input *hi,
+		struct hid_field *field, struct hid_usage *usage,
+		unsigned long **bit, int *max)
+{
+	return -1;
+}
+
 /* -------------------------------------------------------------------------- */
 /* Generic HID++ devices                                                      */
 /* -------------------------------------------------------------------------- */
@@ -953,6 +1168,10 @@  static int hidpp_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
 		return wtp_input_mapping(hdev, hi, field, usage, bit, max);
+	else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560 &&
+		 field->application != HID_GD_MOUSE)
+			return m560_input_mapping(hdev, hi, field, usage,
+						  bit, max);
 
 	return 0;
 }
@@ -962,6 +1181,8 @@  static void hidpp_populate_input(struct hidpp_device *hidpp,
 {
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
 		wtp_populate_input(hidpp, input, origin_is_hid_core);
+	else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560)
+		m560_populate_input(hidpp, input, origin_is_hid_core);
 }
 
 static void hidpp_input_configured(struct hid_device *hdev,
@@ -1049,6 +1270,8 @@  static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
 		return wtp_raw_event(hdev, data, size);
+	else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560)
+		return m560_raw_event(hdev, data, size);
 
 	return 0;
 }
@@ -1126,6 +1349,10 @@  static void hidpp_connect_event(struct hidpp_device *hidpp)
 		ret = wtp_connect(hdev, connected);
 		if (ret)
 			return;
+	} else if ((hidpp->quirks & HIDPP_QUIRK_CLASS_M560) && connected) {
+		ret = m560_send_config_command(hdev, connected);
+		if (ret)
+			return;
 	}
 
 	if (!connected || hidpp->delayed_input)
@@ -1201,7 +1428,12 @@  static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
 		ret = wtp_allocate(hdev, id);
 		if (ret)
-			goto wtp_allocate_fail;
+			goto allocate_fail;
+	}
+	if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {
+		ret = m560_allocate(hdev);
+		if (ret)
+			goto allocate_fail;
 	}
 
 	INIT_WORK(&hidpp->work, delayed_work_cb);
@@ -1268,7 +1500,7 @@  hid_hw_start_fail:
 hid_parse_fail:
 	cancel_work_sync(&hidpp->work);
 	mutex_destroy(&hidpp->send_mutex);
-wtp_allocate_fail:
+allocate_fail:
 	hid_set_drvdata(hdev, NULL);
 	return ret;
 }
@@ -1301,6 +1533,10 @@  static const struct hid_device_id hidpp_devices[] = {
 		USB_VENDOR_ID_LOGITECH, 0x4102),
 	  .driver_data = HIDPP_QUIRK_DELAYED_INIT | HIDPP_QUIRK_MULTI_INPUT |
 			 HIDPP_QUIRK_CLASS_WTP },
+	{ /* Mouse logitech M560 */
+	  HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
+		USB_VENDOR_ID_LOGITECH, 0x402d),
+	  .driver_data = HIDPP_QUIRK_DELAYED_INIT | HIDPP_QUIRK_CLASS_M560 },
 
 	{ HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE,
 		USB_VENDOR_ID_LOGITECH, HID_ANY_ID)},