[PATCH 0/2] hid: sony: Clear and restore controller state on suspend and resume

[PATCH 0/2] hid: sony: Clear and restore controller state on suspend and resume

[Frank Praznik]

On systems with standby power for charging devices the LEDs and rumble on the
controller can continue to function even when the system is in standby since
the state is not cleared when the system goes to sleep.

The state on wakeup can also differ from the state when the system entered
standby as the LEDs can be cleared when the device is reset on wakeup.

The first patch refactors the output report functions to allow sending an
output report without going through the work queue.  This is necessary when
clearing the state of the controller before the system is goes into standby
since the work might not be executed before the system goes to sleep.

The second patch implements the suspend and resume callbacks which serializes
and clears the LEDs on suspend and restores them on resume.

Force-feedback is cleared on suspend but the state is not restored on resume
since it can potentially result in hardware damage if the device is unattended
when the system wakes.  A new event will be required to resume force-feedback.

[PATCH 1/2] hid: sony: Refactor output report sending functions

[Frank Praznik]

Refactor output report sending functions to allow for the sending of
output reports without enqueing a work item.  Output reports for any device
can now be sent by calling the sony_send_output_report function which will
automatically dispatch the request to the appropriate output function.  The
individual state worker functions have been replaced with a universal
sony_state_worker function which calls sony_send_output_report.

Signed-off-by: Frank Praznik <frank.praznik@gmail.com>
---
 drivers/hid/hid-sony.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 661f94f..b84b2ce 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1782,7 +1782,7 @@ error_leds:
 	return ret;
 }
 
-static void sixaxis_state_worker(struct work_struct *work)
+static void sixaxis_send_output_report(struct sony_sc *sc)
 {
 	static const union sixaxis_output_report_01 default_report = {
 		.buf = {
@@ -1796,7 +1796,6 @@ static void sixaxis_state_worker(struct work_struct *work)
 			0x00, 0x00, 0x00, 0x00, 0x00
 		}
 	};
-	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
 	struct sixaxis_output_report *report =
 		(struct sixaxis_output_report *)sc->output_report_dmabuf;
 	int n;
@@ -1839,9 +1838,8 @@ static void sixaxis_state_worker(struct work_struct *work)
 			HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
 }
 
-static void dualshock4_state_worker(struct work_struct *work)
+static void dualshock4_send_output_report(struct sony_sc *sc)
 {
-	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
 	struct hid_device *hdev = sc->hdev;
 	__u8 *buf = sc->output_report_dmabuf;
 	int offset;
@@ -1886,9 +1884,8 @@ static void dualshock4_state_worker(struct work_struct *work)
 				HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
 }
 
-static void motion_state_worker(struct work_struct *work)
+static void motion_send_output_report(struct sony_sc *sc)
 {
-	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
 	struct hid_device *hdev = sc->hdev;
 	struct motion_output_report_02 *report =
 		(struct motion_output_report_02 *)sc->output_report_dmabuf;
@@ -1907,6 +1904,23 @@ static void motion_state_worker(struct work_struct *work)
 	hid_hw_output_report(hdev, (__u8 *)report, MOTION_REPORT_0x02_SIZE);
 }
 
+static void sony_send_output_report(struct sony_sc *sc)
+{
+	if (sc->quirks & DUALSHOCK4_CONTROLLER)
+		dualshock4_send_output_report(sc);
+	else if ((sc->quirks & SIXAXIS_CONTROLLER) ||
+			(sc->quirks & NAVIGATION_CONTROLLER))
+		sixaxis_send_output_report(sc);
+	else if (sc->quirks & MOTION_CONTROLLER)
+		motion_send_output_report(sc);
+}
+
+static void sony_state_worker(struct work_struct *work)
+{
+	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
+	sony_send_output_report(sc);
+}
+
 static int sony_allocate_output_report(struct sony_sc *sc)
 {
 	if ((sc->quirks & SIXAXIS_CONTROLLER) ||
@@ -2234,11 +2248,10 @@ static void sony_release_device_id(struct sony_sc *sc)
 	}
 }
 
-static inline void sony_init_work(struct sony_sc *sc,
-					void (*worker)(struct work_struct *))
+static inline void sony_init_work(struct sony_sc *sc)
 {
 	if (!sc->worker_initialized)
-		INIT_WORK(&sc->state_worker, worker);
+		INIT_WORK(&sc->state_worker, sony_state_worker);
 
 	sc->worker_initialized = 1;
 }
@@ -2312,7 +2325,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP;
 		hdev->quirks |= HID_QUIRK_SKIP_OUTPUT_REPORT_ID;
 		ret = sixaxis_set_operational_usb(hdev);
-		sony_init_work(sc, sixaxis_state_worker);
+		sony_init_work(sc);
 	} else if ((sc->quirks & SIXAXIS_CONTROLLER_BT) ||
 			(sc->quirks & NAVIGATION_CONTROLLER_BT)) {
 		/*
@@ -2321,7 +2334,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		 */
 		hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP;
 		ret = sixaxis_set_operational_bt(hdev);
-		sony_init_work(sc, sixaxis_state_worker);
+		sony_init_work(sc);
 	} else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
 		if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
 			/*
@@ -2336,9 +2349,9 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 			}
 		}
 
-		sony_init_work(sc, dualshock4_state_worker);
+		sony_init_work(sc);
 	} else if (sc->quirks & MOTION_CONTROLLER) {
-		sony_init_work(sc, motion_state_worker);
+		sony_init_work(sc);
 	} else {
 		ret = 0;
 	}
-- 
2.4.3

[PATCH 2/2] hid: sony: Save and restore controller state on suspend and resume

[Frank Praznik]

On hardware which provides standby power for charging devices the state
of the LEDs and force-feedback on controllers can persist even when the
system is in standby.  Additionally, the state of the controllers on resume
may be different from the state they were in at the time when they were
suspended (ie. LEDs remain off on resume).  This implements the suspend and
resume callbacks which saves and clears the state of the LEDs on suspend
and restores it on resume.  Force-feedback is stopped on suspend but
automatically restored on resume until a new event is received to avoid
potentially damaging hardware.

Signed-off-by: Frank Praznik <frank.praznik@gmail.com>
---
 drivers/hid/hid-sony.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index b84b2ce..4eff8f7 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1044,6 +1044,7 @@ struct sony_sc {
 	__u8 battery_charging;
 	__u8 battery_capacity;
 	__u8 led_state[MAX_LEDS];
+	__u8 resume_led_state[MAX_LEDS];
 	__u8 led_delay_on[MAX_LEDS];
 	__u8 led_delay_off[MAX_LEDS];
 	__u8 led_count;
@@ -2427,6 +2428,48 @@ static void sony_remove(struct hid_device *hdev)
 	hid_hw_stop(hdev);
 }
 
+#ifdef CONFIG_PM
+
+static int sony_suspend(struct hid_device *hdev, pm_message_t message)
+{
+	/*
+	 * On suspend save the current LED state,
+	 * stop running force-feedback and blank the LEDS.
+         */
+	if (SONY_LED_SUPPORT || SONY_FF_SUPPORT) {
+		struct sony_sc *sc = hid_get_drvdata(hdev);
+
+#ifdef CONFIG_SONY_FF
+		sc->left = sc->right = 0;
+#endif
+
+		memcpy(sc->resume_led_state, sc->led_state,
+			sizeof(sc->resume_led_state));
+		memset(sc->led_state, 0, sizeof(sc->led_state));
+
+		sony_send_output_report(sc);
+	}
+
+	return 0;
+}
+
+static int sony_resume(struct hid_device *hdev)
+{
+	/* Restore the state of controller LEDs on resume */
+	if (SONY_LED_SUPPORT) {
+		struct sony_sc *sc = hid_get_drvdata(hdev);
+
+		memcpy(sc->led_state, sc->resume_led_state,
+			sizeof(sc->led_state));
+
+		sony_set_leds(sc);
+	}
+
+	return 0;
+}
+
+#endif
+
 static const struct hid_device_id sony_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER),
 		.driver_data = SIXAXIS_CONTROLLER_USB },
@@ -2476,7 +2519,13 @@ static struct hid_driver sony_driver = {
 	.probe            = sony_probe,
 	.remove           = sony_remove,
 	.report_fixup     = sony_report_fixup,
-	.raw_event        = sony_raw_event
+	.raw_event        = sony_raw_event,
+
+#ifdef CONFIG_PM
+	.suspend          = sony_suspend,
+	.resume	          = sony_resume,
+	.reset_resume     = sony_resume,
+#endif
 };
 
 static int __init sony_init(void)
-- 
2.4.3

Re: [PATCH 1/2] hid: sony: Refactor output report sending functions

[Antonio Ospite]

On Sat,  7 Nov 2015 10:12:09 -0500
Frank Praznik <frank.praznik@gmail.com> wrote:

> Refactor output report sending functions to allow for the sending of
> output reports without enqueing a work item.  Output reports for any device
                         ^
                         enqueuing or enqueueing :)

> can now be sent by calling the sony_send_output_report function which will
> automatically dispatch the request to the appropriate output function.  The
> individual state worker functions have been replaced with a universal
> sony_state_worker function which calls sony_send_output_report.
> 
> Signed-off-by: Frank Praznik <frank.praznik@gmail.com>

Looks good to me, just one comment below.

> ---
>  drivers/hid/hid-sony.c | 39 ++++++++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 661f94f..b84b2ce 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -1782,7 +1782,7 @@ error_leds:
>  	return ret;
>  }
>  
> -static void sixaxis_state_worker(struct work_struct *work)
> +static void sixaxis_send_output_report(struct sony_sc *sc)
>  {
>  	static const union sixaxis_output_report_01 default_report = {
>  		.buf = {
> @@ -1796,7 +1796,6 @@ static void sixaxis_state_worker(struct work_struct *work)
>  			0x00, 0x00, 0x00, 0x00, 0x00
>  		}
>  	};
> -	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
>  	struct sixaxis_output_report *report =
>  		(struct sixaxis_output_report *)sc->output_report_dmabuf;
>  	int n;
> @@ -1839,9 +1838,8 @@ static void sixaxis_state_worker(struct work_struct *work)
>  			HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
>  }
>  
> -static void dualshock4_state_worker(struct work_struct *work)
> +static void dualshock4_send_output_report(struct sony_sc *sc)
>  {
> -	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
>  	struct hid_device *hdev = sc->hdev;
>  	__u8 *buf = sc->output_report_dmabuf;
>  	int offset;
> @@ -1886,9 +1884,8 @@ static void dualshock4_state_worker(struct work_struct *work)
>  				HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
>  }
>  
> -static void motion_state_worker(struct work_struct *work)
> +static void motion_send_output_report(struct sony_sc *sc)
>  {
> -	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
>  	struct hid_device *hdev = sc->hdev;
>  	struct motion_output_report_02 *report =
>  		(struct motion_output_report_02 *)sc->output_report_dmabuf;
> @@ -1907,6 +1904,23 @@ static void motion_state_worker(struct work_struct *work)
>  	hid_hw_output_report(hdev, (__u8 *)report, MOTION_REPORT_0x02_SIZE);
>  }
>  
> +static void sony_send_output_report(struct sony_sc *sc)
> +{
> +	if (sc->quirks & DUALSHOCK4_CONTROLLER)
> +		dualshock4_send_output_report(sc);
> +	else if ((sc->quirks & SIXAXIS_CONTROLLER) ||
> +			(sc->quirks & NAVIGATION_CONTROLLER))
> +		sixaxis_send_output_report(sc);
> +	else if (sc->quirks & MOTION_CONTROLLER)
> +		motion_send_output_report(sc);
> +}

We could have have a function pointer to a send_output_report callback
in struct sony_sc, set the appropriate call back in sony_probe() once
and for all and drop sony_send_output_report() which is identifying
again the device, something we already did in sony_probe().
Just an idea for a more declarative approach, but this way is OK too.

> +
> +static void sony_state_worker(struct work_struct *work)
> +{
> +	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
> +	sony_send_output_report(sc);

this would become:

	sc->send_output_report(sc);

same as in patch 2/2.

> +}
> +
>  static int sony_allocate_output_report(struct sony_sc *sc)
>  {
>  	if ((sc->quirks & SIXAXIS_CONTROLLER) ||
> @@ -2234,11 +2248,10 @@ static void sony_release_device_id(struct sony_sc *sc)
>  	}
>  }
>  
> -static inline void sony_init_work(struct sony_sc *sc,
> -					void (*worker)(struct work_struct *))
> +static inline void sony_init_work(struct sony_sc *sc)
>  {
>  	if (!sc->worker_initialized)
> -		INIT_WORK(&sc->state_worker, worker);
> +		INIT_WORK(&sc->state_worker, sony_state_worker);
>  
>  	sc->worker_initialized = 1;
>  }
> @@ -2312,7 +2325,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP;
>  		hdev->quirks |= HID_QUIRK_SKIP_OUTPUT_REPORT_ID;
>  		ret = sixaxis_set_operational_usb(hdev);
> -		sony_init_work(sc, sixaxis_state_worker);
> +		sony_init_work(sc);
>  	} else if ((sc->quirks & SIXAXIS_CONTROLLER_BT) ||
>  			(sc->quirks & NAVIGATION_CONTROLLER_BT)) {
>  		/*
> @@ -2321,7 +2334,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		 */
>  		hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP;
>  		ret = sixaxis_set_operational_bt(hdev);
> -		sony_init_work(sc, sixaxis_state_worker);
> +		sony_init_work(sc);
>  	} else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
>  		if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
>  			/*
> @@ -2336,9 +2349,9 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  			}
>  		}
>  
> -		sony_init_work(sc, dualshock4_state_worker);
> +		sony_init_work(sc);
>  	} else if (sc->quirks & MOTION_CONTROLLER) {
> -		sony_init_work(sc, motion_state_worker);
> +		sony_init_work(sc);
>  	} else {
>  		ret = 0;
>  	}
> -- 

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH 2/2] hid: sony: Save and restore controller state on suspend and resume

[Antonio Ospite]

On Sat,  7 Nov 2015 10:12:10 -0500
Frank Praznik <frank.praznik@gmail.com> wrote:

> On hardware which provides standby power for charging devices the state
> of the LEDs and force-feedback on controllers can persist even when the
> system is in standby.  Additionally, the state of the controllers on resume
> may be different from the state they were in at the time when they were
> suspended (ie. LEDs remain off on resume).  This implements the suspend and
> resume callbacks which saves and clears the state of the LEDs on suspend
> and restores it on resume.  Force-feedback is stopped on suspend but
> automatically restored on resume until a new event is received to avoid
> potentially damaging hardware.
> 
> Signed-off-by: Frank Praznik <frank.praznik@gmail.com>

Acked-by: Antonio Ospite <ao2@ao2.it>

> ---
>  drivers/hid/hid-sony.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index b84b2ce..4eff8f7 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -1044,6 +1044,7 @@ struct sony_sc {
>  	__u8 battery_charging;
>  	__u8 battery_capacity;
>  	__u8 led_state[MAX_LEDS];
> +	__u8 resume_led_state[MAX_LEDS];
>  	__u8 led_delay_on[MAX_LEDS];
>  	__u8 led_delay_off[MAX_LEDS];
>  	__u8 led_count;
> @@ -2427,6 +2428,48 @@ static void sony_remove(struct hid_device *hdev)
>  	hid_hw_stop(hdev);
>  }
>  
> +#ifdef CONFIG_PM
> +
> +static int sony_suspend(struct hid_device *hdev, pm_message_t message)
> +{
> +	/*
> +	 * On suspend save the current LED state,
> +	 * stop running force-feedback and blank the LEDS.
> +         */
> +	if (SONY_LED_SUPPORT || SONY_FF_SUPPORT) {
> +		struct sony_sc *sc = hid_get_drvdata(hdev);
> +
> +#ifdef CONFIG_SONY_FF
> +		sc->left = sc->right = 0;
> +#endif
> +
> +		memcpy(sc->resume_led_state, sc->led_state,
> +			sizeof(sc->resume_led_state));
> +		memset(sc->led_state, 0, sizeof(sc->led_state));
> +
> +		sony_send_output_report(sc);
> +	}
> +
> +	return 0;
> +}
> +
> +static int sony_resume(struct hid_device *hdev)
> +{
> +	/* Restore the state of controller LEDs on resume */
> +	if (SONY_LED_SUPPORT) {
> +		struct sony_sc *sc = hid_get_drvdata(hdev);
> +
> +		memcpy(sc->led_state, sc->resume_led_state,
> +			sizeof(sc->led_state));
> +
> +		sony_set_leds(sc);
> +	}
> +
> +	return 0;
> +}
> +
> +#endif
> +
>  static const struct hid_device_id sony_devices[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER),
>  		.driver_data = SIXAXIS_CONTROLLER_USB },
> @@ -2476,7 +2519,13 @@ static struct hid_driver sony_driver = {
>  	.probe            = sony_probe,
>  	.remove           = sony_remove,
>  	.report_fixup     = sony_report_fixup,
> -	.raw_event        = sony_raw_event
> +	.raw_event        = sony_raw_event,
> +
> +#ifdef CONFIG_PM
> +	.suspend          = sony_suspend,
> +	.resume	          = sony_resume,
> +	.reset_resume     = sony_resume,
> +#endif
>  };
>  
>  static int __init sony_init(void)
> -- 
> 2.4.3
> 
> --
> 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


-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH 1/2] hid: sony: Refactor output report sending functions

[Frank Praznik]

On 11/9/2015 09:02, Antonio Ospite wrote:
> On Sat,  7 Nov 2015 10:12:09 -0500
> Frank Praznik <frank.praznik@gmail.com> wrote:
>
>> Refactor output report sending functions to allow for the sending of
>> output reports without enqueing a work item.  Output reports for any device
>                           ^
>                           enqueuing or enqueueing :)
>
>> can now be sent by calling the sony_send_output_report function which will
>> automatically dispatch the request to the appropriate output function.  The
>> individual state worker functions have been replaced with a universal
>> sony_state_worker function which calls sony_send_output_report.
>>
>> Signed-off-by: Frank Praznik <frank.praznik@gmail.com>
> Looks good to me, just one comment below.
>
>> ---
>>   drivers/hid/hid-sony.c | 39 ++++++++++++++++++++++++++-------------
>>   1 file changed, 26 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>> index 661f94f..b84b2ce 100644
>> --- a/drivers/hid/hid-sony.c
>> +++ b/drivers/hid/hid-sony.c
>> @@ -1782,7 +1782,7 @@ error_leds:
>>   	return ret;
>>   }
>>   
>> -static void sixaxis_state_worker(struct work_struct *work)
>> +static void sixaxis_send_output_report(struct sony_sc *sc)
>>   {
>>   	static const union sixaxis_output_report_01 default_report = {
>>   		.buf = {
>> @@ -1796,7 +1796,6 @@ static void sixaxis_state_worker(struct work_struct *work)
>>   			0x00, 0x00, 0x00, 0x00, 0x00
>>   		}
>>   	};
>> -	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
>>   	struct sixaxis_output_report *report =
>>   		(struct sixaxis_output_report *)sc->output_report_dmabuf;
>>   	int n;
>> @@ -1839,9 +1838,8 @@ static void sixaxis_state_worker(struct work_struct *work)
>>   			HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
>>   }
>>   
>> -static void dualshock4_state_worker(struct work_struct *work)
>> +static void dualshock4_send_output_report(struct sony_sc *sc)
>>   {
>> -	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
>>   	struct hid_device *hdev = sc->hdev;
>>   	__u8 *buf = sc->output_report_dmabuf;
>>   	int offset;
>> @@ -1886,9 +1884,8 @@ static void dualshock4_state_worker(struct work_struct *work)
>>   				HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
>>   }
>>   
>> -static void motion_state_worker(struct work_struct *work)
>> +static void motion_send_output_report(struct sony_sc *sc)
>>   {
>> -	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
>>   	struct hid_device *hdev = sc->hdev;
>>   	struct motion_output_report_02 *report =
>>   		(struct motion_output_report_02 *)sc->output_report_dmabuf;
>> @@ -1907,6 +1904,23 @@ static void motion_state_worker(struct work_struct *work)
>>   	hid_hw_output_report(hdev, (__u8 *)report, MOTION_REPORT_0x02_SIZE);
>>   }
>>   
>> +static void sony_send_output_report(struct sony_sc *sc)
>> +{
>> +	if (sc->quirks & DUALSHOCK4_CONTROLLER)
>> +		dualshock4_send_output_report(sc);
>> +	else if ((sc->quirks & SIXAXIS_CONTROLLER) ||
>> +			(sc->quirks & NAVIGATION_CONTROLLER))
>> +		sixaxis_send_output_report(sc);
>> +	else if (sc->quirks & MOTION_CONTROLLER)
>> +		motion_send_output_report(sc);
>> +}
> We could have have a function pointer to a send_output_report callback
> in struct sony_sc, set the appropriate call back in sony_probe() once
> and for all and drop sony_send_output_report() which is identifying
> again the device, something we already did in sony_probe().
> Just an idea for a more declarative approach, but this way is OK too.
>
>> +
>> +static void sony_state_worker(struct work_struct *work)
>> +{
>> +	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
>> +	sony_send_output_report(sc);
> this would become:
>
> 	sc->send_output_report(sc);
>
> same as in patch 2/2.
>
>
> Thanks,
>     Antonio
>

Hi Antonio,

I like your suggestion.  One pointer dereference vs several branches 
should be faster and cleaner.  I'll do a v2 which implements this and 
fixes the commit typos when I have some time in the next day or two.

Regards,
     Frank