[PATCH v2 0/8] HID: sony: More Sony controller fixes and improvements.

[PATCH v2 0/8] HID: sony: More Sony controller fixes and improvements.

[Frank Praznik]

v2 rebases the previous set of patches against Benjamin Tissoires' latest patch
set and adds some additional changes:

- Adds a patch setting the new HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP for the
  Sixaxis and DualShock 4 when they are connected via Bluetooth.  This adds
  support for sending output reports via hidraw.
  
- A patch with a new union/struct is introduced for the Sixaxis output report.
  This improves readability, particularly in the later blink support patch.
  
- Adds an IDA id allocator and uses it to assign the number at the end of the
  battery string and set the default LED values relative to other Sony
  controllers.

- Drops the LED trigger patch as it probably doesn't belong in kernel space.
  
- Addresses various issues raised during code review of the previous patch set.

[PATCH v2 1/8] HID: sony: Fix Sixaxis cable state detection

[Frank Praznik]

Byte 31 of the Sixaxis report can change depending on whether or not the
controller is rumbling.  Using bit 3 is the only reliable way to detect the
state of the cable regardless of rumble activity.

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
 drivers/hid/hid-sony.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 4884bb5..4c445a0 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -863,7 +863,7 @@ static void sixaxis_parse_report(struct sony_sc *sc, __u8 *rd, int size)
 		battery_capacity = sixaxis_battery_capacity[index];
 		battery_charging = 0;
 	}
-	cable_state = !((rd[31] >> 4) & 0x01);
+	cable_state = !(rd[31] & 0x04);
 
 	spin_lock_irqsave(&sc->lock, flags);
 	sc->cable_state = cable_state;
-- 
1.8.5.3

[PATCH v2 2/8] HID: sony: Set the HID quriks flag for Bluetooth controllers

[Frank Praznik]

The Sixaxis and DualShock 4 want HID output reports sent on the control
endpoint when connected via Bluetooth.  Set the
HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP flag for these devices so hidraw write()
works properly.

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

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 4c445a0..908de27 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1632,11 +1632,21 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		sc->worker_initialized = 1;
 		INIT_WORK(&sc->state_worker, sixaxis_state_worker);
 	} else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
+		/*
+		 * The Sixaxis wants output reports sent on the ctrl endpoint
+		 * when connected via Bluetooth.
+		 */
+		hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP;
 		ret = sixaxis_set_operational_bt(hdev);
 		sc->worker_initialized = 1;
 		INIT_WORK(&sc->state_worker, sixaxis_state_worker);
 	} else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
 		if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
+			/*
+			 * The DualShock 4 wants output reports sent on the ctrl
+			 * endpoint when connected via Bluetooth.
+			 */
+			hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP;
 			ret = dualshock4_set_operational_bt(hdev);
 			if (ret < 0) {
 				hid_err(hdev, "failed to set the Dualshock 4 operational mode\n");
-- 
1.8.5.3

[PATCH v2 3/8] HID: sony: Use inliners for work queue initialization and cancellation

[Frank Praznik]

Use inliners to make sure that the work queue initialization flag is always
checked and set correctly when initializing or cancelling the work queue.

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---

 v2 doesn't set worker_initialized to 0 when cancelling work sync since
 cancel_work_sync doesn't deinitialize the work queue.

 drivers/hid/hid-sony.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 908de27..3df3306 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1578,6 +1578,20 @@ static int sony_check_add(struct sony_sc *sc)
 	return sony_check_add_dev_list(sc);
 }
 
+static inline void sony_init_work(struct sony_sc *sc,
+					void(*worker)(struct work_struct *))
+{
+	if (!sc->worker_initialized)
+		INIT_WORK(&sc->state_worker, worker);
+
+	sc->worker_initialized = 1;
+}
+
+static inline void sony_cancel_work_sync(struct sony_sc *sc)
+{
+	if (sc->worker_initialized)
+		cancel_work_sync(&sc->state_worker);
+}
 
 static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
@@ -1629,8 +1643,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);
-		sc->worker_initialized = 1;
-		INIT_WORK(&sc->state_worker, sixaxis_state_worker);
+		sony_init_work(sc, sixaxis_state_worker);
 	} else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
 		/*
 		 * The Sixaxis wants output reports sent on the ctrl endpoint
@@ -1638,8 +1651,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);
-		sc->worker_initialized = 1;
-		INIT_WORK(&sc->state_worker, sixaxis_state_worker);
+		sony_init_work(sc, sixaxis_state_worker);
 	} else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
 		if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
 			/*
@@ -1661,8 +1673,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		if (ret < 0)
 			goto err_stop;
 
-		sc->worker_initialized = 1;
-		INIT_WORK(&sc->state_worker, dualshock4_state_worker);
+		sony_init_work(sc, dualshock4_state_worker);
 	} else {
 		ret = 0;
 	}
@@ -1707,8 +1718,7 @@ err_stop:
 		sony_leds_remove(hdev);
 	if (sc->quirks & SONY_BATTERY_SUPPORT)
 		sony_battery_remove(sc);
-	if (sc->worker_initialized)
-		cancel_work_sync(&sc->state_worker);
+	sony_cancel_work_sync(sc);
 	sony_remove_dev_list(sc);
 	hid_hw_stop(hdev);
 	return ret;
@@ -1726,8 +1736,7 @@ static void sony_remove(struct hid_device *hdev)
 		sony_battery_remove(sc);
 	}
 
-	if (sc->worker_initialized)
-		cancel_work_sync(&sc->state_worker);
+	sony_cancel_work_sync(sc);
 
 	sony_remove_dev_list(sc);
 
-- 
1.8.5.3

[PATCH v2 4/8] HID: sony: Use a struct for the Sixaxis output report.

[Frank Praznik]

Use a struct for the Sixaxis output report that uses named members to set the
report fields.

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
 drivers/hid/hid-sony.c | 66 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 17 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 3df3306..5e9bb57 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -717,6 +717,36 @@ static enum power_supply_property sony_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 };
 
+struct sixaxis_led {
+	__u8 time_enabled; /* the total time the led is active (0xff means forever) */
+	__u8 duty_length;  /* how long a cycle is in deciseconds (0 means "really fast") */
+	__u8 enabled;
+	__u8 duty_off; /* % of duty_length the led is off (0xff means 100%) */
+	__u8 duty_on;  /* % of duty_length the led is on (0xff mean 100%) */
+} __packed;
+
+struct sixaxis_rumble {
+	__u8 padding;
+	__u8 right_duration; /* Right motor duration (0xff means forever) */
+	__u8 right_motor_on; /* Right (small) motor on/off, only supports values of 0 or 1 (off/on) */
+	__u8 left_duration; /* Left motor duration (0xff means forever) */
+	__u8 left_motor_force; /* left (large) motor, supports force values from 0 to 255 */
+} __packed;
+
+struct sixaxis_output_report {
+	__u8 report_id;
+	struct sixaxis_rumble rumble;
+	__u8 padding[4];
+	__u8 leds_bitmap; /* bitmap of enabled LEDs: LED_1 = 0x02, LED_2 = 0x04, ... */
+	struct sixaxis_led led[4]; /* LEDx at (4 - x), add a macro? */
+	struct sixaxis_led _reserved; /* LED5, not actually soldered */
+} __packed;
+
+union sixaxis_output_report_01 {
+	struct sixaxis_output_report data;
+	__u8 buf[36];
+};
+
 static spinlock_t sony_dev_list_lock;
 static LIST_HEAD(sony_device_list);
 
@@ -1244,29 +1274,31 @@ error_leds:
 static void sixaxis_state_worker(struct work_struct *work)
 {
 	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
-	unsigned char buf[] = {
-		0x01,
-		0x00, 0xff, 0x00, 0xff, 0x00,
-		0x00, 0x00, 0x00, 0x00, 0x00,
-		0xff, 0x27, 0x10, 0x00, 0x32,
-		0xff, 0x27, 0x10, 0x00, 0x32,
-		0xff, 0x27, 0x10, 0x00, 0x32,
-		0xff, 0x27, 0x10, 0x00, 0x32,
-		0x00, 0x00, 0x00, 0x00, 0x00
+	union sixaxis_output_report_01 report = {
+		.buf = {
+			0x01,
+			0x00, 0xff, 0x00, 0xff, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x00,
+			0xff, 0x27, 0x10, 0x00, 0x32,
+			0xff, 0x27, 0x10, 0x00, 0x32,
+			0xff, 0x27, 0x10, 0x00, 0x32,
+			0xff, 0x27, 0x10, 0x00, 0x32,
+			0x00, 0x00, 0x00, 0x00, 0x00
+		}
 	};
 
 #ifdef CONFIG_SONY_FF
-	buf[3] = sc->right ? 1 : 0;
-	buf[5] = sc->left;
+	report.data.rumble.right_motor_on = sc->right ? 1 : 0;
+	report.data.rumble.left_motor_force = sc->left;
 #endif
 
-	buf[10] |= sc->led_state[0] << 1;
-	buf[10] |= sc->led_state[1] << 2;
-	buf[10] |= sc->led_state[2] << 3;
-	buf[10] |= sc->led_state[3] << 4;
+	report.data.leds_bitmap |= sc->led_state[0] << 1;
+	report.data.leds_bitmap |= sc->led_state[1] << 2;
+	report.data.leds_bitmap |= sc->led_state[2] << 3;
+	report.data.leds_bitmap |= sc->led_state[3] << 4;
 
-	hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf), HID_OUTPUT_REPORT,
-			HID_REQ_SET_REPORT);
+	hid_hw_raw_request(sc->hdev, report.data.report_id, report.buf,
+			sizeof(report), HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
 }
 
 static void dualshock4_state_worker(struct work_struct *work)
-- 
1.8.5.3

[PATCH v2 5/8] HID: sony: Convert startup and shutdown functions to use a uniform parameter type

[Frank Praznik]

Convert all of the local initialization and shutdown functions to take a
parameter type of struct sony_sc* instead of using a mix of struct sony_sc* and
struct hid_device*.

Allows for the removal of some calls to hid_get_drvdata().

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
 drivers/hid/hid-sony.c | 67 ++++++++++++++++++++++++--------------------------
 1 file changed, 32 insertions(+), 35 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 5e9bb57..a9bcfbe 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1096,19 +1096,18 @@ static void buzz_set_leds(struct hid_device *hdev, const __u8 *leds)
 	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
 }
 
-static void sony_set_leds(struct hid_device *hdev, const __u8 *leds, int count)
+static void sony_set_leds(struct sony_sc *sc, const __u8 *leds, int count)
 {
-	struct sony_sc *drv_data = hid_get_drvdata(hdev);
 	int n;
 
 	BUG_ON(count > MAX_LEDS);
 
-	if (drv_data->quirks & BUZZ_CONTROLLER && count == 4) {
-		buzz_set_leds(hdev, leds);
+	if (sc->quirks & BUZZ_CONTROLLER && count == 4) {
+		buzz_set_leds(sc->hdev, leds);
 	} else {
 		for (n = 0; n < count; n++)
-			drv_data->led_state[n] = leds[n];
-		schedule_work(&drv_data->state_worker);
+			sc->led_state[n] = leds[n];
+		schedule_work(&sc->state_worker);
 	}
 }
 
@@ -1131,7 +1130,8 @@ static void sony_led_set_brightness(struct led_classdev *led,
 		if (led == drv_data->leds[n]) {
 			if (value != drv_data->led_state[n]) {
 				drv_data->led_state[n] = value;
-				sony_set_leds(hdev, drv_data->led_state, drv_data->led_count);
+				sony_set_leds(drv_data, drv_data->led_state,
+						drv_data->led_count);
 			}
 			break;
 		}
@@ -1160,30 +1160,28 @@ static enum led_brightness sony_led_get_brightness(struct led_classdev *led)
 	return LED_OFF;
 }
 
-static void sony_leds_remove(struct hid_device *hdev)
+static void sony_leds_remove(struct sony_sc *sc)
 {
-	struct sony_sc *drv_data;
 	struct led_classdev *led;
 	int n;
 
-	drv_data = hid_get_drvdata(hdev);
-	BUG_ON(!(drv_data->quirks & SONY_LED_SUPPORT));
+	BUG_ON(!(sc->quirks & SONY_LED_SUPPORT));
 
-	for (n = 0; n < drv_data->led_count; n++) {
-		led = drv_data->leds[n];
-		drv_data->leds[n] = NULL;
+	for (n = 0; n < sc->led_count; n++) {
+		led = sc->leds[n];
+		sc->leds[n] = NULL;
 		if (!led)
 			continue;
 		led_classdev_unregister(led);
 		kfree(led);
 	}
 
-	drv_data->led_count = 0;
+	sc->led_count = 0;
 }
 
-static int sony_leds_init(struct hid_device *hdev)
+static int sony_leds_init(struct sony_sc *sc)
 {
-	struct sony_sc *drv_data;
+	struct hid_device *hdev = sc->hdev;
 	int n, ret = 0;
 	int max_brightness;
 	int use_colors;
@@ -1195,11 +1193,10 @@ static int sony_leds_init(struct hid_device *hdev)
 	static const char * const color_str[] = { "red", "green", "blue" };
 	static const __u8 initial_values[MAX_LEDS] = { 0x00, 0x00, 0x00, 0x00 };
 
-	drv_data = hid_get_drvdata(hdev);
-	BUG_ON(!(drv_data->quirks & SONY_LED_SUPPORT));
+	BUG_ON(!(sc->quirks & SONY_LED_SUPPORT));
 
-	if (drv_data->quirks & BUZZ_CONTROLLER) {
-		drv_data->led_count = 4;
+	if (sc->quirks & BUZZ_CONTROLLER) {
+		sc->led_count = 4;
 		max_brightness = 1;
 		use_colors = 0;
 		name_len = strlen("::buzz#");
@@ -1207,14 +1204,14 @@ static int sony_leds_init(struct hid_device *hdev)
 		/* Validate expected report characteristics. */
 		if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 0, 0, 7))
 			return -ENODEV;
-	} else if (drv_data->quirks & DUALSHOCK4_CONTROLLER) {
-		drv_data->led_count = 3;
+	} else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
+		sc->led_count = 3;
 		max_brightness = 255;
 		use_colors = 1;
 		name_len = 0;
 		name_fmt = "%s:%s";
 	} else {
-		drv_data->led_count = 4;
+		sc->led_count = 4;
 		max_brightness = 1;
 		use_colors = 0;
 		name_len = strlen("::sony#");
@@ -1226,11 +1223,11 @@ static int sony_leds_init(struct hid_device *hdev)
 	 * only relevant if the driver is loaded after somebody actively set the
 	 * LEDs to on
 	 */
-	sony_set_leds(hdev, initial_values, drv_data->led_count);
+	sony_set_leds(sc, initial_values, sc->led_count);
 
 	name_sz = strlen(dev_name(&hdev->dev)) + name_len + 1;
 
-	for (n = 0; n < drv_data->led_count; n++) {
+	for (n = 0; n < sc->led_count; n++) {
 
 		if (use_colors)
 			name_sz = strlen(dev_name(&hdev->dev)) + strlen(color_str[n]) + 2;
@@ -1260,13 +1257,13 @@ static int sony_leds_init(struct hid_device *hdev)
 			goto error_leds;
 		}
 
-		drv_data->leds[n] = led;
+		sc->leds[n] = led;
 	}
 
 	return ret;
 
 error_leds:
-	sony_leds_remove(hdev);
+	sony_leds_remove(sc);
 
 	return ret;
 }
@@ -1355,9 +1352,9 @@ static int sony_play_effect(struct input_dev *dev, void *data,
 	return 0;
 }
 
-static int sony_init_ff(struct hid_device *hdev)
+static int sony_init_ff(struct sony_sc *sc)
 {
-	struct hid_input *hidinput = list_entry(hdev->inputs.next,
+	struct hid_input *hidinput = list_entry(sc->hdev->inputs.next,
 						struct hid_input, list);
 	struct input_dev *input_dev = hidinput->input;
 
@@ -1366,7 +1363,7 @@ static int sony_init_ff(struct hid_device *hdev)
 }
 
 #else
-static int sony_init_ff(struct hid_device *hdev)
+static int sony_init_ff(struct sony_sc *sc)
 {
 	return 0;
 }
@@ -1718,7 +1715,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto err_stop;
 
 	if (sc->quirks & SONY_LED_SUPPORT) {
-		ret = sony_leds_init(hdev);
+		ret = sony_leds_init(sc);
 		if (ret < 0)
 			goto err_stop;
 	}
@@ -1737,7 +1734,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	}
 
 	if (sc->quirks & SONY_FF_SUPPORT) {
-		ret = sony_init_ff(hdev);
+		ret = sony_init_ff(sc);
 		if (ret < 0)
 			goto err_close;
 	}
@@ -1747,7 +1744,7 @@ err_close:
 	hid_hw_close(hdev);
 err_stop:
 	if (sc->quirks & SONY_LED_SUPPORT)
-		sony_leds_remove(hdev);
+		sony_leds_remove(sc);
 	if (sc->quirks & SONY_BATTERY_SUPPORT)
 		sony_battery_remove(sc);
 	sony_cancel_work_sync(sc);
@@ -1761,7 +1758,7 @@ static void sony_remove(struct hid_device *hdev)
 	struct sony_sc *sc = hid_get_drvdata(hdev);
 
 	if (sc->quirks & SONY_LED_SUPPORT)
-		sony_leds_remove(hdev);
+		sony_leds_remove(sc);
 
 	if (sc->quirks & SONY_BATTERY_SUPPORT) {
 		hid_hw_close(hdev);
-- 
1.8.5.3

[PATCH v2 6/8] HID: sony: Add an IDA allocator to assign unique device ids

[Frank Praznik]

Add an IDA id allocator to assign unique, sequential device ids to Sixaxis and
DualShock 4 controllers.

Use explicit module init and exit functions since the IDA allocator must be
manually destroyed when the module is unloaded.

Use the device id as the unique number for the battery identification string.

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
 drivers/hid/hid-sony.c | 70 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 63 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index a9bcfbe..13af58c 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -33,6 +33,7 @@
 #include <linux/power_supply.h>
 #include <linux/spinlock.h>
 #include <linux/list.h>
+#include <linux/idr.h>
 #include <linux/input/mt.h>
 
 #include "hid-ids.h"
@@ -749,6 +750,7 @@ union sixaxis_output_report_01 {
 
 static spinlock_t sony_dev_list_lock;
 static LIST_HEAD(sony_device_list);
+static DEFINE_IDA(sony_device_id_allocator);
 
 struct sony_sc {
 	spinlock_t lock;
@@ -758,6 +760,7 @@ struct sony_sc {
 	unsigned long quirks;
 	struct work_struct state_worker;
 	struct power_supply battery;
+	int device_id;
 
 #ifdef CONFIG_SONY_FF
 	__u8 left;
@@ -1413,8 +1416,6 @@ static int sony_battery_get_property(struct power_supply *psy,
 
 static int sony_battery_probe(struct sony_sc *sc)
 {
-	static atomic_t power_id_seq = ATOMIC_INIT(0);
-	unsigned long power_id;
 	struct hid_device *hdev = sc->hdev;
 	int ret;
 
@@ -1424,15 +1425,13 @@ static int sony_battery_probe(struct sony_sc *sc)
 	 */
 	sc->battery_capacity = 100;
 
-	power_id = (unsigned long)atomic_inc_return(&power_id_seq);
-
 	sc->battery.properties = sony_battery_props;
 	sc->battery.num_properties = ARRAY_SIZE(sony_battery_props);
 	sc->battery.get_property = sony_battery_get_property;
 	sc->battery.type = POWER_SUPPLY_TYPE_BATTERY;
 	sc->battery.use_for_apm = 0;
-	sc->battery.name = kasprintf(GFP_KERNEL, "sony_controller_battery_%lu",
-				     power_id);
+	sc->battery.name = kasprintf(GFP_KERNEL, "sony_controller_battery_%i",
+				     sc->device_id);
 	if (!sc->battery.name)
 		return -ENOMEM;
 
@@ -1607,6 +1606,38 @@ static int sony_check_add(struct sony_sc *sc)
 	return sony_check_add_dev_list(sc);
 }
 
+static int sony_set_device_id(struct sony_sc *sc)
+{
+	int ret;
+
+	/*
+	 * Only DualShock 4 or Sixaxis controller get an id.
+	 * All others are set to -1.
+	 */
+	if ((sc->quirks & SIXAXIS_CONTROLLER) ||
+	    (sc->quirks & DUALSHOCK4_CONTROLLER)) {
+		ret = ida_simple_get(&sony_device_id_allocator, 0, 0,
+					GFP_KERNEL);
+		if (ret < 0) {
+			sc->device_id = -1;
+			return ret;
+		}
+		sc->device_id = ret;
+	} else {
+		sc->device_id = -1;
+	}
+
+	return 0;
+}
+
+static void sony_release_device_id(struct sony_sc *sc)
+{
+	if (sc->device_id >= 0) {
+		ida_simple_remove(&sony_device_id_allocator, sc->device_id);
+		sc->device_id = -1;
+	}
+}
+
 static inline void sony_init_work(struct sony_sc *sc,
 					void(*worker)(struct work_struct *))
 {
@@ -1658,6 +1689,12 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
+	ret = sony_set_device_id(sc);
+	if (ret < 0) {
+		hid_err(hdev, "failed to allocate the device id\n");
+		goto err_stop;
+	}
+
 	if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
 		/*
 		 * The Sony Sixaxis does not handle HID Output Reports on the
@@ -1749,6 +1786,7 @@ err_stop:
 		sony_battery_remove(sc);
 	sony_cancel_work_sync(sc);
 	sony_remove_dev_list(sc);
+	sony_release_device_id(sc);
 	hid_hw_stop(hdev);
 	return ret;
 }
@@ -1769,6 +1807,8 @@ static void sony_remove(struct hid_device *hdev)
 
 	sony_remove_dev_list(sc);
 
+	sony_release_device_id(sc);
+
 	hid_hw_stop(hdev);
 }
 
@@ -1813,6 +1853,22 @@ static struct hid_driver sony_driver = {
 	.report_fixup  = sony_report_fixup,
 	.raw_event     = sony_raw_event
 };
-module_hid_driver(sony_driver);
+
+static int __init sony_init(void)
+{
+	dbg_hid("Sony:%s\n", __func__);
+
+	return hid_register_driver(&sony_driver);
+}
+
+static void __exit sony_exit(void)
+{
+	dbg_hid("Sony:%s\n", __func__);
+
+	ida_destroy(&sony_device_id_allocator);
+	hid_unregister_driver(&sony_driver);
+}
+module_init(sony_init);
+module_exit(sony_exit);
 
 MODULE_LICENSE("GPL");
-- 
1.8.5.3

Re: [PATCH v2 6/8] HID: sony: Add an IDA allocator to assign unique device ids

[Antonio Ospite]

Hi Frank,

On Thu,  6 Mar 2014 17:32:54 -0500
Frank Praznik <frank.praznik@oh.rr.com> wrote:

> Add an IDA id allocator to assign unique, sequential device ids to Sixaxis and
> DualShock 4 controllers.
> 
> Use explicit module init and exit functions since the IDA allocator must be
> manually destroyed when the module is unloaded.
>
> Use the device id as the unique number for the battery identification string.
>

Have you thought about using the bdaddr as the battery id?

I think that decoupling led numbers (from the following patch) and
battery ids would be saner. For instance in a scenario when userspace
decided that the _second_ sixaxis has LEDs saying "controller
3" (because of different kind of joypads, remember?) we would have
battery still saying "2" because the battery id is assigned at probe
time while LEDs can change at any time. This mismatch may become
confusing.

> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
>  drivers/hid/hid-sony.c | 70 +++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 63 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index a9bcfbe..13af58c 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -33,6 +33,7 @@
>  #include <linux/power_supply.h>
>  #include <linux/spinlock.h>
>  #include <linux/list.h>
> +#include <linux/idr.h>
>  #include <linux/input/mt.h>
>  
>  #include "hid-ids.h"
> @@ -749,6 +750,7 @@ union sixaxis_output_report_01 {
>  
>  static spinlock_t sony_dev_list_lock;
>  static LIST_HEAD(sony_device_list);
> +static DEFINE_IDA(sony_device_id_allocator);
>  
>  struct sony_sc {
>  	spinlock_t lock;
> @@ -758,6 +760,7 @@ struct sony_sc {
>  	unsigned long quirks;
>  	struct work_struct state_worker;
>  	struct power_supply battery;
> +	int device_id;
>  
>  #ifdef CONFIG_SONY_FF
>  	__u8 left;
> @@ -1413,8 +1416,6 @@ static int sony_battery_get_property(struct power_supply *psy,
>  
>  static int sony_battery_probe(struct sony_sc *sc)
>  {
> -	static atomic_t power_id_seq = ATOMIC_INIT(0);
> -	unsigned long power_id;
>  	struct hid_device *hdev = sc->hdev;
>  	int ret;
>  
> @@ -1424,15 +1425,13 @@ static int sony_battery_probe(struct sony_sc *sc)
>  	 */
>  	sc->battery_capacity = 100;
>  
> -	power_id = (unsigned long)atomic_inc_return(&power_id_seq);
> -
>  	sc->battery.properties = sony_battery_props;
>  	sc->battery.num_properties = ARRAY_SIZE(sony_battery_props);
>  	sc->battery.get_property = sony_battery_get_property;
>  	sc->battery.type = POWER_SUPPLY_TYPE_BATTERY;
>  	sc->battery.use_for_apm = 0;
> -	sc->battery.name = kasprintf(GFP_KERNEL, "sony_controller_battery_%lu",
> -				     power_id);
> +	sc->battery.name = kasprintf(GFP_KERNEL, "sony_controller_battery_%i",
> +				     sc->device_id);
>  	if (!sc->battery.name)
>  		return -ENOMEM;
>  
> @@ -1607,6 +1606,38 @@ static int sony_check_add(struct sony_sc *sc)
>  	return sony_check_add_dev_list(sc);
>  }
>  
> +static int sony_set_device_id(struct sony_sc *sc)
> +{
> +	int ret;
> +
> +	/*
> +	 * Only DualShock 4 or Sixaxis controller get an id.
> +	 * All others are set to -1.
> +	 */
> +	if ((sc->quirks & SIXAXIS_CONTROLLER) ||
> +	    (sc->quirks & DUALSHOCK4_CONTROLLER)) {
> +		ret = ida_simple_get(&sony_device_id_allocator, 0, 0,
> +					GFP_KERNEL);
> +		if (ret < 0) {
> +			sc->device_id = -1;
> +			return ret;
> +		}
> +		sc->device_id = ret;
> +	} else {
> +		sc->device_id = -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static void sony_release_device_id(struct sony_sc *sc)
> +{
> +	if (sc->device_id >= 0) {
> +		ida_simple_remove(&sony_device_id_allocator, sc->device_id);
> +		sc->device_id = -1;
> +	}
> +}
> +
>  static inline void sony_init_work(struct sony_sc *sc,
>  					void(*worker)(struct work_struct *))
>  {
> @@ -1658,6 +1689,12 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		return ret;
>  	}
>  
> +	ret = sony_set_device_id(sc);
> +	if (ret < 0) {
> +		hid_err(hdev, "failed to allocate the device id\n");
> +		goto err_stop;
> +	}
> +
>  	if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
>  		/*
>  		 * The Sony Sixaxis does not handle HID Output Reports on the
> @@ -1749,6 +1786,7 @@ err_stop:
>  		sony_battery_remove(sc);
>  	sony_cancel_work_sync(sc);
>  	sony_remove_dev_list(sc);
> +	sony_release_device_id(sc);
>  	hid_hw_stop(hdev);
>  	return ret;
>  }
> @@ -1769,6 +1807,8 @@ static void sony_remove(struct hid_device *hdev)
>  
>  	sony_remove_dev_list(sc);
>  
> +	sony_release_device_id(sc);
> +
>  	hid_hw_stop(hdev);
>  }
>  
> @@ -1813,6 +1853,22 @@ static struct hid_driver sony_driver = {
>  	.report_fixup  = sony_report_fixup,
>  	.raw_event     = sony_raw_event
>  };
> -module_hid_driver(sony_driver);
> +
> +static int __init sony_init(void)
> +{
> +	dbg_hid("Sony:%s\n", __func__);
> +
> +	return hid_register_driver(&sony_driver);
> +}
> +
> +static void __exit sony_exit(void)
> +{
> +	dbg_hid("Sony:%s\n", __func__);
> +
> +	ida_destroy(&sony_device_id_allocator);
> +	hid_unregister_driver(&sony_driver);
> +}
> +module_init(sony_init);
> +module_exit(sony_exit);
>  
>  MODULE_LICENSE("GPL");
> -- 
> 1.8.5.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 v2 6/8] HID: sony: Add an IDA allocator to assign unique device ids

[Frank Praznik]

On 3/10/2014 18:25, Antonio Ospite wrote:
> Hi Frank,
>
> On Thu,  6 Mar 2014 17:32:54 -0500
> Frank Praznik <frank.praznik@oh.rr.com> wrote:
>
>> Add an IDA id allocator to assign unique, sequential device ids to Sixaxis and
>> DualShock 4 controllers.
>>
>> Use explicit module init and exit functions since the IDA allocator must be
>> manually destroyed when the module is unloaded.
>>
>> Use the device id as the unique number for the battery identification string.
>>
> Have you thought about using the bdaddr as the battery id?
>
> I think that decoupling led numbers (from the following patch) and
> battery ids would be saner. For instance in a scenario when userspace
> decided that the _second_ sixaxis has LEDs saying "controller
> 3" (because of different kind of joypads, remember?) we would have
> battery still saying "2" because the battery id is assigned at probe
> time while LEDs can change at any time. This mismatch may become
> confusing.
>

That's a good idea and it will match the naming scheme of the wiimote 
battery device, which I think is the only other game controller that 
reports battery status.  I'll make the change for v3.

[PATCH v2 7/8] HID: sony: Initialize the controller LEDs with the device ID value

[Frank Praznik]

Use the device ID to initialize the Sixaxis and DualShock 4 controller LEDs to
default values.  The number or color of the controller is set relative to other
connected Sony controllers.

Set the LED class brightness values to the initial values and add the new led to
the array before calling led_classdev_register so that the correct brightness value
shows up in the LED sysfs entry.

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---

 v2 uses the id assigned by the IDA allocator to set the default values.

 drivers/hid/hid-sony.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 53 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 13af58c..6de42b4 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1081,6 +1081,52 @@ static int dualshock4_set_operational_bt(struct hid_device *hdev)
 				HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
 }
 
+static void sixaxis_set_leds_from_id(int id, __u8 values[MAX_LEDS])
+{
+	static const __u8 sixaxis_leds[10][4] = {
+				{ 0x01, 0x00, 0x00, 0x00 },
+				{ 0x00, 0x01, 0x00, 0x00 },
+				{ 0x00, 0x00, 0x01, 0x00 },
+				{ 0x00, 0x00, 0x00, 0x01 },
+				{ 0x01, 0x00, 0x00, 0x01 },
+				{ 0x00, 0x01, 0x00, 0x01 },
+				{ 0x00, 0x00, 0x01, 0x01 },
+				{ 0x01, 0x00, 0x01, 0x01 },
+				{ 0x00, 0x01, 0x01, 0x01 },
+				{ 0x01, 0x01, 0x01, 0x01 }
+	};
+
+	BUG_ON(MAX_LEDS < ARRAY_SIZE(sixaxis_leds[0]));
+
+	if (id < 0)
+		return;
+
+	id %= 10;
+	memcpy(values, sixaxis_leds[id], sizeof(sixaxis_leds[id]));
+}
+
+static void dualshock4_set_leds_from_id(int id, __u8 values[MAX_LEDS])
+{
+	/* The first 4 color/index entries match what the PS4 assigns */
+	static const __u8 color_code[7][3] = {
+			/* Blue   */	{ 0x00, 0x00, 0x01 },
+			/* Red	  */	{ 0x01, 0x00, 0x00 },
+			/* Green  */	{ 0x00, 0x01, 0x00 },
+			/* Pink   */	{ 0x02, 0x00, 0x01 },
+			/* Orange */	{ 0x02, 0x01, 0x00 },
+			/* Teal   */	{ 0x00, 0x01, 0x01 },
+			/* White  */	{ 0x01, 0x01, 0x01 }
+	};
+
+	BUG_ON(MAX_LEDS < ARRAY_SIZE(color_code[0]));
+
+	if (id < 0)
+		return;
+
+	id %= 7;
+	memcpy(values, color_code[id], sizeof(color_code[id]));
+}
+
 static void buzz_set_leds(struct hid_device *hdev, const __u8 *leds)
 {
 	struct list_head *report_list =
@@ -1194,7 +1240,7 @@ static int sony_leds_init(struct sony_sc *sc)
 	size_t name_len;
 	const char *name_fmt;
 	static const char * const color_str[] = { "red", "green", "blue" };
-	static const __u8 initial_values[MAX_LEDS] = { 0x00, 0x00, 0x00, 0x00 };
+	__u8 initial_values[MAX_LEDS] = { 0 };
 
 	BUG_ON(!(sc->quirks & SONY_LED_SUPPORT));
 
@@ -1208,12 +1254,14 @@ static int sony_leds_init(struct sony_sc *sc)
 		if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 0, 0, 7))
 			return -ENODEV;
 	} else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
+		dualshock4_set_leds_from_id(sc->device_id, initial_values);
 		sc->led_count = 3;
 		max_brightness = 255;
 		use_colors = 1;
 		name_len = 0;
 		name_fmt = "%s:%s";
 	} else {
+		sixaxis_set_leds_from_id(sc->device_id, initial_values);
 		sc->led_count = 4;
 		max_brightness = 1;
 		use_colors = 0;
@@ -1248,19 +1296,20 @@ static int sony_leds_init(struct sony_sc *sc)
 		else
 			snprintf(name, name_sz, name_fmt, dev_name(&hdev->dev), n + 1);
 		led->name = name;
-		led->brightness = 0;
+		led->brightness = initial_values[n];
 		led->max_brightness = max_brightness;
 		led->brightness_get = sony_led_get_brightness;
 		led->brightness_set = sony_led_set_brightness;
 
+		sc->leds[n] = led;
+
 		ret = led_classdev_register(&hdev->dev, led);
 		if (ret) {
 			hid_err(hdev, "Failed to register LED %d\n", n);
+			sc->leds[n] = NULL;
 			kfree(led);
 			goto error_leds;
 		}
-
-		sc->leds[n] = led;
 	}
 
 	return ret;
-- 
1.8.5.3

Re: [PATCH v2 7/8] HID: sony: Initialize the controller LEDs with the device ID value

[Antonio Ospite]

On Thu,  6 Mar 2014 17:32:55 -0500
Frank Praznik <frank.praznik@oh.rr.com> wrote:

> Use the device ID to initialize the Sixaxis and DualShock 4 controller LEDs to
> default values.  The number or color of the controller is set relative to other
> connected Sony controllers.
>

You already know I am not a huge fan of this idea for the sixaxis and I
found another reason why: the Sixaxis requires the user to press the PS
button before it starts to actually send events and the all-blink
pattern is there to tell the user:
  
  "Look, even if the device is connected, it isn't fully functional yet,
   some action is required".

That's also why the BlueZ sixaxis plugin waits for events before
actually setting the LEDs via USB.

Furthermore I still seem to get the all-blink pattern even with the
patch applied, it seems to start _after_ the kernel driver set the
default as per your patch; do you also experience this?
And I still need a recent BLueZ with the sixaxis plugin to use the
controller via BT so I don't see the benefits of defaults over BT
either, but I am obviously biased.

That said, the approach used looks clean enough so I am not going to
oppose any further :)

Just please, if you can, test your changes in conjunction with the
BlueZ sixaxis plugin in order to make sure the two don't step on each
other toes too much.

Thanks,
   Antonio

> Set the LED class brightness values to the initial values and add the new led to
> the array before calling led_classdev_register so that the correct brightness value
> shows up in the LED sysfs entry.

Also thanks for thinking about that last part, it's a nice thing to do.

> 
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
> 
>  v2 uses the id assigned by the IDA allocator to set the default values.
> 
>  drivers/hid/hid-sony.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 53 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 13af58c..6de42b4 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -1081,6 +1081,52 @@ static int dualshock4_set_operational_bt(struct hid_device *hdev)
>  				HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>  }
>  
> +static void sixaxis_set_leds_from_id(int id, __u8 values[MAX_LEDS])
> +{
> +	static const __u8 sixaxis_leds[10][4] = {
> +				{ 0x01, 0x00, 0x00, 0x00 },
> +				{ 0x00, 0x01, 0x00, 0x00 },
> +				{ 0x00, 0x00, 0x01, 0x00 },
> +				{ 0x00, 0x00, 0x00, 0x01 },
> +				{ 0x01, 0x00, 0x00, 0x01 },
> +				{ 0x00, 0x01, 0x00, 0x01 },
> +				{ 0x00, 0x00, 0x01, 0x01 },
> +				{ 0x01, 0x00, 0x01, 0x01 },
> +				{ 0x00, 0x01, 0x01, 0x01 },
> +				{ 0x01, 0x01, 0x01, 0x01 }
> +	};
> +
> +	BUG_ON(MAX_LEDS < ARRAY_SIZE(sixaxis_leds[0]));
> +
> +	if (id < 0)
> +		return;
> +
> +	id %= 10;
> +	memcpy(values, sixaxis_leds[id], sizeof(sixaxis_leds[id]));
> +}
> +
> +static void dualshock4_set_leds_from_id(int id, __u8 values[MAX_LEDS])
> +{
> +	/* The first 4 color/index entries match what the PS4 assigns */
> +	static const __u8 color_code[7][3] = {
> +			/* Blue   */	{ 0x00, 0x00, 0x01 },
> +			/* Red	  */	{ 0x01, 0x00, 0x00 },
> +			/* Green  */	{ 0x00, 0x01, 0x00 },
> +			/* Pink   */	{ 0x02, 0x00, 0x01 },
> +			/* Orange */	{ 0x02, 0x01, 0x00 },
> +			/* Teal   */	{ 0x00, 0x01, 0x01 },
> +			/* White  */	{ 0x01, 0x01, 0x01 }
> +	};
> +
> +	BUG_ON(MAX_LEDS < ARRAY_SIZE(color_code[0]));
> +
> +	if (id < 0)
> +		return;
> +
> +	id %= 7;
> +	memcpy(values, color_code[id], sizeof(color_code[id]));
> +}
> +
>  static void buzz_set_leds(struct hid_device *hdev, const __u8 *leds)
>  {
>  	struct list_head *report_list =
> @@ -1194,7 +1240,7 @@ static int sony_leds_init(struct sony_sc *sc)
>  	size_t name_len;
>  	const char *name_fmt;
>  	static const char * const color_str[] = { "red", "green", "blue" };
> -	static const __u8 initial_values[MAX_LEDS] = { 0x00, 0x00, 0x00, 0x00 };
> +	__u8 initial_values[MAX_LEDS] = { 0 };
>  
>  	BUG_ON(!(sc->quirks & SONY_LED_SUPPORT));
>  
> @@ -1208,12 +1254,14 @@ static int sony_leds_init(struct sony_sc *sc)
>  		if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 0, 0, 7))
>  			return -ENODEV;
>  	} else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
> +		dualshock4_set_leds_from_id(sc->device_id, initial_values);
>  		sc->led_count = 3;
>  		max_brightness = 255;
>  		use_colors = 1;
>  		name_len = 0;
>  		name_fmt = "%s:%s";
>  	} else {
> +		sixaxis_set_leds_from_id(sc->device_id, initial_values);
>  		sc->led_count = 4;
>  		max_brightness = 1;
>  		use_colors = 0;
> @@ -1248,19 +1296,20 @@ static int sony_leds_init(struct sony_sc *sc)
>  		else
>  			snprintf(name, name_sz, name_fmt, dev_name(&hdev->dev), n + 1);
>  		led->name = name;
> -		led->brightness = 0;
> +		led->brightness = initial_values[n];
>  		led->max_brightness = max_brightness;
>  		led->brightness_get = sony_led_get_brightness;
>  		led->brightness_set = sony_led_set_brightness;
>  
> +		sc->leds[n] = led;
> +
>  		ret = led_classdev_register(&hdev->dev, led);
>  		if (ret) {
>  			hid_err(hdev, "Failed to register LED %d\n", n);
> +			sc->leds[n] = NULL;
>  			kfree(led);
>  			goto error_leds;
>  		}
> -
> -		sc->leds[n] = led;
>  	}
>  
>  	return ret;
> -- 
> 1.8.5.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 v2 7/8] HID: sony: Initialize the controller LEDs with the device ID value

[Frank Praznik]

On 3/10/2014 18:59, Antonio Ospite wrote:
> On Thu,  6 Mar 2014 17:32:55 -0500
> Frank Praznik<frank.praznik@oh.rr.com>  wrote:
>
>> Use the device ID to initialize the Sixaxis and DualShock 4 controller LEDs to
>> default values.  The number or color of the controller is set relative to other
>> connected Sony controllers.
>>
> You already know I am not a huge fan of this idea for the sixaxis and I
> found another reason why: the Sixaxis requires the user to press the PS
> button before it starts to actually send events and the all-blink
> pattern is there to tell the user:
>    
>    "Look, even if the device is connected, it isn't fully functional yet,
>     some action is required".
>
> That's also why the BlueZ sixaxis plugin waits for events before
> actually setting the LEDs via USB.
>
> Furthermore I still seem to get the all-blink pattern even with the
> patch applied, it seems to start _after_ the kernel driver set the
> default as per your patch; do you also experience this?
> And I still need a recent BLueZ with the sixaxis plugin to use the
> controller via BT so I don't see the benefits of defaults over BT
> either, but I am obviously biased.

Yeah, without the BlueZ plugin it keeps blinking on USB unless the PS 
button is pressed almost immediately since the controller overrides the 
initial settings and a new output report isn't sent until there is a 
reason to do so (a rumble event or the LEDs changing in sysfs).  I'm not 
really sure how to fix that in the driver without some hack-ish 
workarounds that may interfere with settings made in userland.  It's 
just an unavoidable quirk of the controller and no different from the 
existing behavior in that specific case.

I can still see setting the defaults in the driver being of some use on 
the Bluetooth side since there are a lot of distros that are still using 
BlueZ 4.x which doesn't have the plugin (everything Debian based for 
instance).  You need a third party program like sixpair to do the 
initial pairing, but it's still usable.

> Just please, if you can, test your changes in conjunction with the
> BlueZ sixaxis plugin in order to make sure the two don't step on each
> other toes too much.

I've tested it against the BlueZ plugin and there are no conflicts that 
I've experienced.  Like I've said before, the LEDs are only set to 
defaults during initialization and then never touched by the driver once 
the device is exposed to the system so the driver will never "step on 
the toes" of userland settings.  The driver has actually been setting 
the LEDs to default values since someone else added the LED controls for 
the Sixaxis in 3.14 and it doesn't seem to cause any problems.  The only 
difference this patch makes is that the LEDs are set to some meaningful 
default instead of "all-off".

[PATCH v2 8/8] HID: sony: Add blink support to the Sixaxis and DualShock 4 LEDs

[Frank Praznik]

Add support for setting the blink rate of the LEDs.  The Sixaxis allows control
over each individual LED, but the Dualshock 4 only has one global control for
the light bar so changing any individual color changes the global blink rate.

Setting the brightness cancels the blinking as per the LED class specifications.

The Sixaxis and Dualshock 4 controllers accept delays in decisecond increments
from 0 to 255 (2550 milliseconds).

The value at index 1 of the DualShock 4 USB output report must be 0xFF or the
light bar won't blink.

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---

 v2 addresses various code review issues and fixes a bug where the DualShock 4
 wouldn't blink when connected via USB unless byte 1 of the report is 0xFF.

 drivers/hid/hid-sony.c | 113 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 104 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 6de42b4..585d9c7 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -773,6 +773,8 @@ struct sony_sc {
 	__u8 battery_charging;
 	__u8 battery_capacity;
 	__u8 led_state[MAX_LEDS];
+	__u8 led_delay_on[MAX_LEDS];
+	__u8 led_delay_off[MAX_LEDS];
 	__u8 led_count;
 };
 
@@ -1167,7 +1169,7 @@ static void sony_led_set_brightness(struct led_classdev *led,
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
 	struct sony_sc *drv_data;
 
-	int n;
+	int n, blink_index;
 
 	drv_data = hid_get_drvdata(hdev);
 	if (!drv_data) {
@@ -1176,14 +1178,31 @@ static void sony_led_set_brightness(struct led_classdev *led,
 	}
 
 	for (n = 0; n < drv_data->led_count; n++) {
-		if (led == drv_data->leds[n]) {
-			if (value != drv_data->led_state[n]) {
-				drv_data->led_state[n] = value;
-				sony_set_leds(drv_data, drv_data->led_state,
-						drv_data->led_count);
-			}
+		if (led == drv_data->leds[n])
 			break;
-		}
+	}
+
+	/* This LED is not registered on this device */
+	if (n >= drv_data->led_count)
+		return;
+
+	/* The DualShock 4 has a global blink setting and always uses index 0 */
+	if (drv_data->quirks & DUALSHOCK4_CONTROLLER)
+		blink_index = 0;
+	else
+		blink_index = n;
+
+	if ((value != drv_data->led_state[n]) ||
+		drv_data->led_delay_on[blink_index] ||
+		drv_data->led_delay_off[blink_index]) {
+		drv_data->led_state[n] = value;
+
+		/* Setting the brightness stops the blinking */
+		drv_data->led_delay_on[blink_index] = 0;
+		drv_data->led_delay_off[blink_index] = 0;
+
+		sony_set_leds(drv_data, drv_data->led_state,
+				drv_data->led_count);
 	}
 }
 
@@ -1209,6 +1228,58 @@ static enum led_brightness sony_led_get_brightness(struct led_classdev *led)
 	return LED_OFF;
 }
 
+static int sony_led_blink_set(struct led_classdev *led, unsigned long *delay_on,
+				unsigned long *delay_off)
+{
+	struct device *dev = led->dev->parent;
+	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
+	struct sony_sc *drv_data = hid_get_drvdata(hdev);
+	int n;
+	__u8 new_on, new_off;
+
+	if (!drv_data) {
+		hid_err(hdev, "No device data\n");
+		return -EINVAL;
+	}
+
+	/* Max delay is 255 deciseconds or 2550 milliseconds */
+	if (*delay_on > 2550)
+		*delay_on = 2550;
+	if (*delay_off > 2550)
+		*delay_off = 2550;
+
+	/* Blink at 1 Hz if both values are zero */
+	if (!*delay_on && !*delay_off)
+		*delay_on = *delay_off = 1000;
+
+	new_on = *delay_on / 10;
+	new_off = *delay_off / 10;
+
+	/* The DualShock 4 has a global blink setting and always uses index 0 */
+	if (drv_data->quirks & DUALSHOCK4_CONTROLLER) {
+		n = 0;
+	} else {
+		for (n = 0; n < drv_data->led_count; n++) {
+			if (led == drv_data->leds[n])
+				break;
+		}
+	}
+
+	/* This LED is not registered on this device */
+	if (n >= drv_data->led_count)
+		return -EINVAL;
+
+	/* Don't schedule work if the values didn't change */
+	if (new_on != drv_data->led_delay_on[n] ||
+		new_off != drv_data->led_delay_off[n]) {
+		drv_data->led_delay_on[n] = new_on;
+		drv_data->led_delay_off[n] = new_off;
+		schedule_work(&drv_data->state_worker);
+	}
+
+	return 0;
+}
+
 static void sony_leds_remove(struct sony_sc *sc)
 {
 	struct led_classdev *led;
@@ -1301,6 +1372,9 @@ static int sony_leds_init(struct sony_sc *sc)
 		led->brightness_get = sony_led_get_brightness;
 		led->brightness_set = sony_led_set_brightness;
 
+		if (!(sc->quirks & BUZZ_CONTROLLER))
+			led->blink_set = sony_led_blink_set;
+
 		sc->leds[n] = led;
 
 		ret = led_classdev_register(&hdev->dev, led);
@@ -1323,6 +1397,7 @@ error_leds:
 static void sixaxis_state_worker(struct work_struct *work)
 {
 	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
+	int n;
 	union sixaxis_output_report_01 report = {
 		.buf = {
 			0x01,
@@ -1346,6 +1421,22 @@ static void sixaxis_state_worker(struct work_struct *work)
 	report.data.leds_bitmap |= sc->led_state[2] << 3;
 	report.data.leds_bitmap |= sc->led_state[3] << 4;
 
+	/*
+	 * The LEDs in the report are indexed in reverse order to their
+	 * corresponding light on the controller.
+	 * Index 0 = LED 4, index 1 = LED 3, etc...
+	 *
+	 * In the case of both delay values being zero (blinking disabled) the
+	 * default report values should be used or the controller LED will be
+	 * always off.
+	 */
+	for (n = 0; n < 4; n++) {
+		if (sc->led_delay_on[n] || sc->led_delay_off[n]) {
+			report.data.led[3 - n].duty_off = sc->led_delay_off[n];
+			report.data.led[3 - n].duty_on = sc->led_delay_on[n];
+		}
+	}
+
 	hid_hw_raw_request(sc->hdev, report.data.report_id, report.buf,
 			sizeof(report), HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
 }
@@ -1360,7 +1451,7 @@ static void dualshock4_state_worker(struct work_struct *work)
 
 	if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
 		buf[0] = 0x05;
-		buf[1] = 0x03;
+		buf[1] = 0xFF;
 		offset = 4;
 	} else {
 		buf[0] = 0x11;
@@ -1380,6 +1471,10 @@ static void dualshock4_state_worker(struct work_struct *work)
 	buf[offset++] = sc->led_state[1];
 	buf[offset++] = sc->led_state[2];
 
+	/* If both delay values are zero the DualShock 4 disables blinking. */
+	buf[offset++] = sc->led_delay_on[0];
+	buf[offset++] = sc->led_delay_off[0];
+
 	if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
 		hid_hw_output_report(hdev, buf, 32);
 	else
-- 
1.8.5.3

Re: [PATCH v2 0/8] HID: sony: More Sony controller fixes and improvements.

[Jiri Kosina]

On Thu, 6 Mar 2014, Frank Praznik wrote:

> v2 rebases the previous set of patches against Benjamin Tissoires' latest patch
> set and adds some additional changes:
> 
> - Adds a patch setting the new HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP for the
>   Sixaxis and DualShock 4 when they are connected via Bluetooth.  This adds
>   support for sending output reports via hidraw.
>   
> - A patch with a new union/struct is introduced for the Sixaxis output report.
>   This improves readability, particularly in the later blink support patch.
>   
> - Adds an IDA id allocator and uses it to assign the number at the end of the
>   battery string and set the default LED values relative to other Sony
>   controllers.
> 
> - Drops the LED trigger patch as it probably doesn't belong in kernel space.
>   
> - Addresses various issues raised during code review of the previous patch set.

Frank,

just to make things clear -- I am waiting for v3 due to baddr changes 
planned for 6/8.

Thanks,

-- 
Jiri Kosina
SUSE Labs