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

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

[Frank Praznik]

This set consists of one bugfix, two mostly cosmetic changes and three larger
patches for the LED subsystem.

Patch #4 adds hardware blink support to the controller LEDs.  Values from 0 to
2.5 seconds are supported by the hardware.  The Sixaxis can set all of the LEDs
individually, but the DualShock 4 only has one global setting for the entire
light bar so only the value from the most recently set LED is used.

Patch #5 adds an LED trigger that reports the controller battery status via the
registered LEDs.  The LEDs will flash if the controller is charging or if the
battery is low, and remain solid otherwise.

Patch #6 initializes the LEDs to a default value of LED 1 on the Sixaxis and
blue on the DualShock 4 so there is some indication that the controller is
powered on and connected in the case of Bluetooth.  The code can be used to set
the LEDs based on the device number, but I'm not sure how to actually retrieve
the controller number from the system.  I saw the xpad patches posted a few
weeks ago where the minor number of the joydev device was used, but I'm under
the impression that doing that is not ideal.  Any suggestions?

[PATCH 1/6] 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 b1aa6f0..31ba01a 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -864,7 +864,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 2/6] HID: sony: Convert startup and shutdown functions to use a uniform parameter type

[Frank Praznik]

Convert all of the 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.

sony_set_leds() was converted as well as it was just pulling the sony_sc
struct out of the hid_device.

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

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 31ba01a..233c094 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1106,19 +1106,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);
 	}
 }
 
@@ -1141,7 +1140,7 @@ 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;
 		}
@@ -1170,30 +1169,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;
@@ -1205,11 +1202,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#");
@@ -1217,14 +1213,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#");
@@ -1236,11 +1232,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;
@@ -1270,13 +1266,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;
 }
@@ -1366,9 +1362,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;
 
@@ -1377,7 +1373,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;
 }
@@ -1697,7 +1693,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;
 	}
@@ -1716,7 +1712,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;
 	}
@@ -1726,7 +1722,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);
 	if (sc->worker_initialized)
@@ -1741,7 +1737,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 3/6] HID: sony: Use inliners for work queue initialization and cancellation

[Frank Praznik]

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

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

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 233c094..dc6e6fa 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1617,6 +1617,22 @@ 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);
+
+	sc->worker_initialized = 0;
+}
 
 static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
@@ -1657,12 +1673,10 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
 		hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
 		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) {
 		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) {
 			ret = dualshock4_set_operational_bt(hdev);
@@ -1679,8 +1693,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;
 	}
@@ -1725,8 +1738,7 @@ err_stop:
 		sony_leds_remove(sc);
 	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;
@@ -1744,8 +1756,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 4/6] HID: sony: Add blink support to the 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 them all.

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).

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

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index dc6e6fa..914a6cc 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -741,6 +741,8 @@ struct sony_sc {
 	__u8 battery_charging;
 	__u8 battery_capacity;
 	__u8 led_state[MAX_LEDS];
+	__u8 led_blink_on[MAX_LEDS];
+	__u8 led_blink_off[MAX_LEDS];
 	__u8 led_count;
 };
 
@@ -1127,8 +1129,7 @@ static void sony_led_set_brightness(struct led_classdev *led,
 	struct device *dev = led->dev->parent;
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
 	struct sony_sc *drv_data;
-
-	int n;
+	int n, blink_index = 0;
 
 	drv_data = hid_get_drvdata(hdev);
 	if (!drv_data) {
@@ -1136,14 +1137,30 @@ static void sony_led_set_brightness(struct led_classdev *led,
 		return;
 	}
 
+	/* Get the index of the 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 LED and always uses index 0 */
+	if (!(drv_data->quirks & DUALSHOCK4_CONTROLLER))
+		blink_index = n;
+
+	if ((value != drv_data->led_state[n])   ||
+		drv_data->led_blink_on[blink_index] ||
+		drv_data->led_blink_off[blink_index]) {
+		drv_data->led_state[n] = value;
+
+		/* Setting the brightness stops the blinking */
+		drv_data->led_blink_on[blink_index] = 0;
+		drv_data->led_blink_off[blink_index] = 0;
+
+		sony_set_leds(drv_data, drv_data->led_state, drv_data->led_count);
 	}
 }
 
@@ -1169,6 +1186,56 @@ static enum led_brightness sony_led_get_brightness(struct led_classdev *led)
 	return LED_OFF;
 }
 
+static int sony_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 = 0;
+	__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 blink controls are global on the DualShock 4 */
+	if (!(drv_data->quirks & DUALSHOCK4_CONTROLLER)) {
+		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_blink_on[n] ||
+		new_off != drv_data->led_blink_off[n]) {
+		drv_data->led_blink_on[n] = new_on;
+		drv_data->led_blink_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;
@@ -1259,6 +1326,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_blink_set;
+
 		ret = led_classdev_register(&hdev->dev, led);
 		if (ret) {
 			hid_err(hdev, "Failed to register LED %d\n", n);
@@ -1280,14 +1350,15 @@ 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;
 	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,
+		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 4 */
+		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 3 */
+		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 2 */
+		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 1 */
 		0x00, 0x00, 0x00, 0x00, 0x00
 	};
 
@@ -1301,6 +1372,13 @@ static void sixaxis_state_worker(struct work_struct *work)
 	buf[10] |= sc->led_state[2] << 3;
 	buf[10] |= sc->led_state[3] << 4;
 
+	for (n = 0; n < 4; n++) {
+		if (sc->led_blink_on[n] || sc->led_blink_off[n]) {
+			buf[29-(n*5)] = sc->led_blink_off[n];
+			buf[30-(n*5)] = sc->led_blink_on[n];
+		}
+	}
+
 	if (sc->quirks & SIXAXIS_CONTROLLER_USB)
 		hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
 	else
@@ -1338,6 +1416,9 @@ static void dualshock4_state_worker(struct work_struct *work)
 	buf[offset++] = sc->led_state[1];
 	buf[offset++] = sc->led_state[2];
 
+	buf[offset++] = sc->led_blink_on[0];
+	buf[offset++] = sc->led_blink_off[0];
+
 	if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
 		hid_hw_output_report(hdev, buf, 32);
 	else
-- 
1.8.5.3

Re: [PATCH 4/6] HID: sony: Add blink support to the LEDs

[Antonio Ospite]

Hi Frank,

On Fri, 28 Feb 2014 22:58:59 -0500
Frank Praznik <frank.praznik@oh.rr.com> wrote:

> 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 them all.
> 
> 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).
> 
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
>  drivers/hid/hid-sony.c | 105 +++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 93 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index dc6e6fa..914a6cc 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -741,6 +741,8 @@ struct sony_sc {
>  	__u8 battery_charging;
>  	__u8 battery_capacity;
>  	__u8 led_state[MAX_LEDS];
> +	__u8 led_blink_on[MAX_LEDS];
> +	__u8 led_blink_off[MAX_LEDS];

Are those values meant to be for the duty cycle in deciseconds?
What about using a more explicative name? leds_blink_on makes me think
to something boolean (it could be just me), maybe leds_delay_on and
leds_delay_off?

Also grouping spare arrays into a single array of structs may be worth
considering:

struct sony_sc {
	...
	struct {
		struct led_classdev *ldev;
		__u8 state;
		__u8 delay_on;
		__u8 delay_off;
	} leds[MAX_LEDS];
	...
};

Defining the struct for leds separately if you prefer so.

>  	__u8 led_count;
>  };
>  
> @@ -1127,8 +1129,7 @@ static void sony_led_set_brightness(struct led_classdev *led,
>  	struct device *dev = led->dev->parent;
>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>  	struct sony_sc *drv_data;
> -
> -	int n;
> +	int n, blink_index = 0;
>  
>  	drv_data = hid_get_drvdata(hdev);
>  	if (!drv_data) {
> @@ -1136,14 +1137,30 @@ static void sony_led_set_brightness(struct led_classdev *led,
>  		return;
>  	}
>  
> +	/* Get the index of the 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 LED and always uses index 0 */
> +	if (!(drv_data->quirks & DUALSHOCK4_CONTROLLER))
> +		blink_index = n;
> +

If you feel the need for a comment here, what about not initializing
blink_index to 0 before and add an else block here, this way the code
itself is more explicit, and more symmetric too.

> +	if ((value != drv_data->led_state[n])   ||
> +		drv_data->led_blink_on[blink_index] ||
> +		drv_data->led_blink_off[blink_index]) {
> +		drv_data->led_state[n] = value;
> +
> +		/* Setting the brightness stops the blinking */
> +		drv_data->led_blink_on[blink_index] = 0;
> +		drv_data->led_blink_off[blink_index] = 0;
> +
> +		sony_set_leds(drv_data, drv_data->led_state, drv_data->led_count);
>  	}
>  }
>  
> @@ -1169,6 +1186,56 @@ static enum led_brightness sony_led_get_brightness(struct led_classdev *led)
>  	return LED_OFF;
>  }
>  
> +static int sony_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 = 0;
> +	__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 blink controls are global on the DualShock 4 */
> +	if (!(drv_data->quirks & DUALSHOCK4_CONTROLLER)) {
> +		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_blink_on[n] ||
> +		new_off != drv_data->led_blink_off[n]) {
> +		drv_data->led_blink_on[n] = new_on;
> +		drv_data->led_blink_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;
> @@ -1259,6 +1326,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_blink_set;
> +
>  		ret = led_classdev_register(&hdev->dev, led);
>  		if (ret) {
>  			hid_err(hdev, "Failed to register LED %d\n", n);
> @@ -1280,14 +1350,15 @@ 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;
>  	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,
> +		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 4 */
> +		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 3 */
> +		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 2 */
> +		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 1 */
>  		0x00, 0x00, 0x00, 0x00, 0x00
>  	};
>  
> @@ -1301,6 +1372,13 @@ static void sixaxis_state_worker(struct work_struct *work)
>  	buf[10] |= sc->led_state[2] << 3;
>  	buf[10] |= sc->led_state[3] << 4;
>  
> +	for (n = 0; n < 4; n++) {
> +		if (sc->led_blink_on[n] || sc->led_blink_off[n]) {
> +			buf[29-(n*5)] = sc->led_blink_off[n];
> +			buf[30-(n*5)] = sc->led_blink_on[n];
                            ^^^^^^^^
Kernel coding style prefers spaces around operators.
I see that scripts/checkpatch.pl does not warn about this, but it's in
Documentation/CodingStyle.

However the calculations here made me wonder if it's the case to go
semantic and represent the output report with a struct instead of an
array (maybe even using a union), so you can access the individual
fields in a more meaningful, and less bug prone, way.

For example (untested):

struct sixaxis_led {
	uint8_t time_enabled; /* the total time the led is active (0xff means forever) */
	uint8_t duty_length;   /* how long a cycle is in deciseconds (0 means "really fast") */
	uint8_t enabled;
	uint8_t duty_off; /* % of duty_length the led is off (0xff means 100%) */
	uint8_t duty_on; /* % of duty_length the led is on (0xff mean 100%) */

} __attribute__ ((packed));

struct sixaxis_output_report {
	uint8_t report_id;
	uint8_t rumble[5]; /* TODO: add the rumble bits here... */
	uint8_t padding[4];
	uint8_t 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 */
}; __attribute__ ((packed));

union output_report_01 {
	struct sixaxis_output_report data;
	uint8_t buf[36];
};

I had the snippet above buried somewhere and I don't remember where
all the info came from.

> +		}
> +	}
> +
>  	if (sc->quirks & SIXAXIS_CONTROLLER_USB)
>  		hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
>  	else
> @@ -1338,6 +1416,9 @@ static void dualshock4_state_worker(struct work_struct *work)
>  	buf[offset++] = sc->led_state[1];
>  	buf[offset++] = sc->led_state[2];
>  
> +	buf[offset++] = sc->led_blink_on[0];
> +	buf[offset++] = sc->led_blink_off[0];
> +
>  	if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
>  		hid_hw_output_report(hdev, buf, 32);
>  	else
> -- 
> 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 4/6] HID: sony: Add blink support to the LEDs

[Frank Praznik]

On 3/1/2014 09:20, Antonio Ospite wrote:
> Hi Frank,
>
> On Fri, 28 Feb 2014 22:58:59 -0500
> Frank Praznik <frank.praznik@oh.rr.com> wrote:
>
>> 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 them all.
>>
>> 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).
>>
>> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
>> ---
>>   drivers/hid/hid-sony.c | 105 +++++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 93 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>> index dc6e6fa..914a6cc 100644
>> --- a/drivers/hid/hid-sony.c
>> +++ b/drivers/hid/hid-sony.c
>> @@ -741,6 +741,8 @@ struct sony_sc {
>>   	__u8 battery_charging;
>>   	__u8 battery_capacity;
>>   	__u8 led_state[MAX_LEDS];
>> +	__u8 led_blink_on[MAX_LEDS];
>> +	__u8 led_blink_off[MAX_LEDS];
> Are those values meant to be for the duty cycle in deciseconds?
> What about using a more explicative name? leds_blink_on makes me think
> to something boolean (it could be just me), maybe leds_delay_on and
> leds_delay_off?
>
> Also grouping spare arrays into a single array of structs may be worth
> considering:
>
> struct sony_sc {
> 	...
> 	struct {
> 		struct led_classdev *ldev;
> 		__u8 state;
> 		__u8 delay_on;
> 		__u8 delay_off;
> 	} leds[MAX_LEDS];
> 	...
> };
>
> Defining the struct for leds separately if you prefer so.
It would be a little more organized, but it would also add extra padding 
to the struct.  I'll think about this one.
>
>>   	__u8 led_count;
>>   };
>>   
>> @@ -1127,8 +1129,7 @@ static void sony_led_set_brightness(struct led_classdev *led,
>>   	struct device *dev = led->dev->parent;
>>   	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>>   	struct sony_sc *drv_data;
>> -
>> -	int n;
>> +	int n, blink_index = 0;
>>   
>>   	drv_data = hid_get_drvdata(hdev);
>>   	if (!drv_data) {
>> @@ -1136,14 +1137,30 @@ static void sony_led_set_brightness(struct led_classdev *led,
>>   		return;
>>   	}
>>   
>> +	/* Get the index of the 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 LED and always uses index 0 */
>> +	if (!(drv_data->quirks & DUALSHOCK4_CONTROLLER))
>> +		blink_index = n;
>> +
> If you feel the need for a comment here, what about not initializing
> blink_index to 0 before and add an else block here, this way the code
> itself is more explicit, and more symmetric too.
Good idea, I'll fix it in v2.
>
>> +	if ((value != drv_data->led_state[n])   ||
>> +		drv_data->led_blink_on[blink_index] ||
>> +		drv_data->led_blink_off[blink_index]) {
>> +		drv_data->led_state[n] = value;
>> +
>> +		/* Setting the brightness stops the blinking */
>> +		drv_data->led_blink_on[blink_index] = 0;
>> +		drv_data->led_blink_off[blink_index] = 0;
>> +
>> +		sony_set_leds(drv_data, drv_data->led_state, drv_data->led_count);
>>   	}
>>   }
>>   
>> @@ -1169,6 +1186,56 @@ static enum led_brightness sony_led_get_brightness(struct led_classdev *led)
>>   	return LED_OFF;
>>   }
>>   
>> +static int sony_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 = 0;
>> +	__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 blink controls are global on the DualShock 4 */
>> +	if (!(drv_data->quirks & DUALSHOCK4_CONTROLLER)) {
>> +		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_blink_on[n] ||
>> +		new_off != drv_data->led_blink_off[n]) {
>> +		drv_data->led_blink_on[n] = new_on;
>> +		drv_data->led_blink_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;
>> @@ -1259,6 +1326,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_blink_set;
>> +
>>   		ret = led_classdev_register(&hdev->dev, led);
>>   		if (ret) {
>>   			hid_err(hdev, "Failed to register LED %d\n", n);
>> @@ -1280,14 +1350,15 @@ 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;
>>   	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,
>> +		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 4 */
>> +		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 3 */
>> +		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 2 */
>> +		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 1 */
>>   		0x00, 0x00, 0x00, 0x00, 0x00
>>   	};
>>   
>> @@ -1301,6 +1372,13 @@ static void sixaxis_state_worker(struct work_struct *work)
>>   	buf[10] |= sc->led_state[2] << 3;
>>   	buf[10] |= sc->led_state[3] << 4;
>>   
>> +	for (n = 0; n < 4; n++) {
>> +		if (sc->led_blink_on[n] || sc->led_blink_off[n]) {
>> +			buf[29-(n*5)] = sc->led_blink_off[n];
>> +			buf[30-(n*5)] = sc->led_blink_on[n];
>                              ^^^^^^^^
> Kernel coding style prefers spaces around operators.
> I see that scripts/checkpatch.pl does not warn about this, but it's in
> Documentation/CodingStyle.
>
> However the calculations here made me wonder if it's the case to go
> semantic and represent the output report with a struct instead of an
> array (maybe even using a union), so you can access the individual
> fields in a more meaningful, and less bug prone, way.
>
> For example (untested):
>
> struct sixaxis_led {
> 	uint8_t time_enabled; /* the total time the led is active (0xff means forever) */
> 	uint8_t duty_length;   /* how long a cycle is in deciseconds (0 means "really fast") */
> 	uint8_t enabled;
> 	uint8_t duty_off; /* % of duty_length the led is off (0xff means 100%) */
> 	uint8_t duty_on; /* % of duty_length the led is on (0xff mean 100%) */
>
> } __attribute__ ((packed));
>
> struct sixaxis_output_report {
> 	uint8_t report_id;
> 	uint8_t rumble[5]; /* TODO: add the rumble bits here... */
> 	uint8_t padding[4];
> 	uint8_t 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 */
> }; __attribute__ ((packed));
>
> union output_report_01 {
> 	struct sixaxis_output_report data;
> 	uint8_t buf[36];
> };
>
> I had the snippet above buried somewhere and I don't remember where
> all the info came from.
Nice, this will be much clearer.  I'll tidy it up and add this as a 
separate refactor patch in v2.  Thanks for the snippet.
>> +		}
>> +	}
>> +
>>   	if (sc->quirks & SIXAXIS_CONTROLLER_USB)
>>   		hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
>>   	else
>> @@ -1338,6 +1416,9 @@ static void dualshock4_state_worker(struct work_struct *work)
>>   	buf[offset++] = sc->led_state[1];
>>   	buf[offset++] = sc->led_state[2];
>>   
>> +	buf[offset++] = sc->led_blink_on[0];
>> +	buf[offset++] = sc->led_blink_off[0];
>> +
>>   	if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
>>   		hid_hw_output_report(hdev, buf, 32);
>>   	else
>> -- 
>> 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
>

[PATCH 5/6] HID: sony: Add an led trigger to report controller battery status.

[Frank Praznik]

Creates an LED trigger that changes LED behavior depending on the state of the
controller battery.

The trigger function runs on a 500 millisecond timer and only updates the LEDs
if the controller power state has changed or a new device has been added to the
trigger.

The trigger sets the LEDs to solid if the controller is in wireless mode and
the battery is above 20% power or if the controller is plugged in and the
battery has completed charging.  If the controller is not charging and the
battery drops to or below 20% it blinks the LEDs in 500ms intervals. If the
controller is plugged in and charging it blinks the LEDs in 1 second intervals.

The order of subsystem initialization had to be changed in sony_probe() so that
the trigger is created before the LEDs are initialized.

By default the controller LEDs are set to the trigger local to that controller.

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

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 914a6cc..d7889ac 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -730,6 +730,17 @@ struct sony_sc {
 	struct work_struct state_worker;
 	struct power_supply battery;
 
+#ifdef CONFIG_LEDS_TRIGGERS
+	spinlock_t trigger_lock;
+	struct led_trigger battery_trigger;
+	struct timer_list battery_trigger_timer;
+	atomic_t trigger_device_added;
+	__u8 trigger_initialized;
+	__u8 trigger_timer_initialized;
+	__u8 trigger_capacity;
+	__u8 trigger_charging;
+#endif
+
 #ifdef CONFIG_SONY_FF
 	__u8 left;
 	__u8 right;
@@ -1329,14 +1340,19 @@ static int sony_leds_init(struct sony_sc *sc)
 		if (!(sc->quirks & BUZZ_CONTROLLER))
 			led->blink_set = sony_blink_set;
 
+#ifdef CONFIG_LEDS_TRIGGERS
+		led->default_trigger = sc->battery_trigger.name;
+#endif
+
+		sc->leds[n] = led;
+
 		ret = led_classdev_register(&hdev->dev, led);
 		if (ret) {
 			hid_err(hdev, "Failed to register LED %d\n", n);
 			kfree(led);
+			sc->leds[n] = NULL;
 			goto error_leds;
 		}
-
-		sc->leds[n] = led;
 	}
 
 	return ret;
@@ -1552,6 +1568,137 @@ static void sony_battery_remove(struct sony_sc *sc)
 	sc->battery.name = NULL;
 }
 
+#ifdef CONFIG_LEDS_TRIGGERS
+static void sony_battery_trigger_callback(unsigned long data)
+{
+	struct sony_sc *drv_data = (struct sony_sc *)data;
+	struct led_classdev *led;
+	unsigned long flags;
+	unsigned long delay_on, delay_off;
+	int dev_added, ret;
+	__u8 charging, capacity;
+
+	/* Check if new LEDs were added since the last time */
+	dev_added = atomic_cmpxchg(&drv_data->trigger_device_added, 1, 0);
+
+	/* Get the battery info */
+	spin_lock_irqsave(&drv_data->lock, flags);
+	charging = drv_data->battery_charging;
+	capacity = drv_data->battery_capacity;
+	spin_unlock_irqrestore(&drv_data->lock, flags);
+
+	/* Don't set the LEDs if nothing has changed */
+	if (!dev_added && drv_data->trigger_capacity == capacity &&
+		drv_data->trigger_charging == charging)
+		goto reset_timer;
+
+	if (charging) {
+		/* Charging: blink at 1 sec intervals */
+		delay_on = delay_off = 1000;
+		led_trigger_blink(&drv_data->battery_trigger, &delay_on,
+				&delay_off);
+	} else if (capacity <= 20) {
+		/* Low battery: blink at 500ms intervals */
+		delay_on = delay_off = 500;
+		led_trigger_blink(&drv_data->battery_trigger, &delay_on,
+				&delay_off);
+	} else {
+		/*
+		 * Normal: set the brightness to stop blinking
+		 *
+		 * This just walks the list of LEDs on the trigger and sets the
+		 * brightness to the existing value. This leaves the brightness
+		 * the same but the blinking is stopped.
+		 */
+		read_lock(&drv_data->battery_trigger.leddev_list_lock);
+		list_for_each_entry(led,
+			&drv_data->battery_trigger.led_cdevs, trig_list) {
+			led_set_brightness(led, led->brightness);
+		}
+		read_unlock(&drv_data->battery_trigger.leddev_list_lock);
+	}
+
+	/* Cache the power state for next time */
+	drv_data->trigger_charging = charging;
+	drv_data->trigger_capacity = capacity;
+
+reset_timer:
+	ret = mod_timer(&drv_data->battery_trigger_timer,
+			jiffies + msecs_to_jiffies(500));
+
+	if (ret < 0)
+		hid_err(drv_data->hdev,
+			"Failed to set battery trigger timer\n");
+}
+
+static void sony_battery_trigger_activate(struct led_classdev *led)
+{
+	struct sony_sc *sc;
+
+	sc = container_of(led->trigger, struct sony_sc, battery_trigger);
+
+	/*
+	 * Set the device_added flag to tell the timer function that it
+	 * should send an update even if the power state hasn't changed.
+	 */
+	atomic_set(&sc->trigger_device_added, 1);
+}
+
+static int sony_battery_trigger_init(struct sony_sc *sc)
+{
+	int ret;
+
+	sc->battery_trigger.name = kasprintf(GFP_KERNEL,
+				"%s-blink-low-or-charging", sc->battery.name);
+	if (!sc->battery.name)
+		return -ENOMEM;
+
+	sc->battery_trigger.activate = sony_battery_trigger_activate;
+
+	ret = led_trigger_register(&sc->battery_trigger);
+	if (ret < 0)
+		goto trigger_failure;
+
+	setup_timer(&sc->battery_trigger_timer,
+			sony_battery_trigger_callback, (unsigned long)sc);
+
+	ret = mod_timer(&sc->battery_trigger_timer,
+			jiffies + msecs_to_jiffies(500));
+	if (ret < 0)
+		goto timer_failure;
+
+	sc->trigger_initialized = 1;
+
+	return 0;
+
+timer_failure:
+	led_trigger_unregister(&sc->battery_trigger);
+trigger_failure:
+	kfree(sc->battery_trigger.name);
+	return ret;
+}
+
+static void sony_battery_trigger_remove(struct sony_sc *sc)
+{
+	if (sc->trigger_initialized) {
+		del_timer_sync(&sc->battery_trigger_timer);
+		led_trigger_unregister(&sc->battery_trigger);
+		kfree(sc->battery_trigger.name);
+	}
+}
+#else
+static int sony_battery_trigger_init(struct sony_sc *sc)
+{
+	/* Nothing to do */
+	return 0;
+}
+
+static void sony_battery_trigger_remove(struct sony_sc *sc)
+{
+	/* Nothing to do */
+}
+#endif
+
 static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
 					int w, int h)
 {
@@ -1786,14 +1933,12 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (ret < 0)
 		goto err_stop;
 
-	if (sc->quirks & SONY_LED_SUPPORT) {
-		ret = sony_leds_init(sc);
+	if (sc->quirks & SONY_BATTERY_SUPPORT) {
+		ret = sony_battery_probe(sc);
 		if (ret < 0)
 			goto err_stop;
-	}
 
-	if (sc->quirks & SONY_BATTERY_SUPPORT) {
-		ret = sony_battery_probe(sc);
+		ret = sony_battery_trigger_init(sc);
 		if (ret < 0)
 			goto err_stop;
 
@@ -1805,6 +1950,12 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		}
 	}
 
+	if (sc->quirks & SONY_LED_SUPPORT) {
+		ret = sony_leds_init(sc);
+		if (ret < 0)
+			goto err_stop;
+	}
+
 	if (sc->quirks & SONY_FF_SUPPORT) {
 		ret = sony_init_ff(sc);
 		if (ret < 0)
@@ -1817,8 +1968,10 @@ err_close:
 err_stop:
 	if (sc->quirks & SONY_LED_SUPPORT)
 		sony_leds_remove(sc);
-	if (sc->quirks & SONY_BATTERY_SUPPORT)
+	if (sc->quirks & SONY_BATTERY_SUPPORT) {
+		sony_battery_trigger_remove(sc);
 		sony_battery_remove(sc);
+	}
 	sony_cancel_work_sync(sc);
 	sony_remove_dev_list(sc);
 	hid_hw_stop(hdev);
@@ -1834,6 +1987,7 @@ static void sony_remove(struct hid_device *hdev)
 
 	if (sc->quirks & SONY_BATTERY_SUPPORT) {
 		hid_hw_close(hdev);
+		sony_battery_trigger_remove(sc);
 		sony_battery_remove(sc);
 	}
 
-- 
1.8.5.3

Re: [PATCH 5/6] HID: sony: Add an led trigger to report controller battery status.

[Antonio Ospite]

On Fri, 28 Feb 2014 22:59:00 -0500
Frank Praznik <frank.praznik@oh.rr.com> wrote:

> Creates an LED trigger that changes LED behavior depending on the state of the
> controller battery.
> 

Frank, have you thought about adding the logic which activates the the
blinking patterns to the bluez plugin? I mean, from userspace you can
access the battery class and led class and decide what LED pattern to
show to indicate the battery status.

I'd try to have as less code a possible in the kernel if things can be
done in userspace. As a rule of thumb: don't make kernel drivers "too
intelligent".

Some more comments below.

> The trigger function runs on a 500 millisecond timer and only updates the LEDs
> if the controller power state has changed or a new device has been added to the
> trigger.
> 
> The trigger sets the LEDs to solid if the controller is in wireless mode and
> the battery is above 20% power or if the controller is plugged in and the
> battery has completed charging.  If the controller is not charging and the
> battery drops to or below 20% it blinks the LEDs in 500ms intervals. If the
> controller is plugged in and charging it blinks the LEDs in 1 second intervals.
> 
> The order of subsystem initialization had to be changed in sony_probe() so that
> the trigger is created before the LEDs are initialized.
> 
> By default the controller LEDs are set to the trigger local to that controller.
> 
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
>  drivers/hid/hid-sony.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 162 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 914a6cc..d7889ac 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -730,6 +730,17 @@ struct sony_sc {
>  	struct work_struct state_worker;
>  	struct power_supply battery;
>  
> +#ifdef CONFIG_LEDS_TRIGGERS
> +	spinlock_t trigger_lock;
> +	struct led_trigger battery_trigger;
> +	struct timer_list battery_trigger_timer;
> +	atomic_t trigger_device_added;
> +	__u8 trigger_initialized;
> +	__u8 trigger_timer_initialized;
> +	__u8 trigger_capacity;
> +	__u8 trigger_charging;
> +#endif
> +
>  #ifdef CONFIG_SONY_FF
>  	__u8 left;
>  	__u8 right;
> @@ -1329,14 +1340,19 @@ static int sony_leds_init(struct sony_sc *sc)
>  		if (!(sc->quirks & BUZZ_CONTROLLER))
>  			led->blink_set = sony_blink_set;
>  
> +#ifdef CONFIG_LEDS_TRIGGERS
> +		led->default_trigger = sc->battery_trigger.name;
> +#endif
> +
> +		sc->leds[n] = led;
> +
>  		ret = led_classdev_register(&hdev->dev, led);
>  		if (ret) {
>  			hid_err(hdev, "Failed to register LED %d\n", n);
>  			kfree(led);
> +			sc->leds[n] = NULL;
>  			goto error_leds;
>  		}
> -
> -		sc->leds[n] = led;
>  	}
>  
>  	return ret;
> @@ -1552,6 +1568,137 @@ static void sony_battery_remove(struct sony_sc *sc)
>  	sc->battery.name = NULL;
>  }
>  
> +#ifdef CONFIG_LEDS_TRIGGERS
> +static void sony_battery_trigger_callback(unsigned long data)
> +{
> +	struct sony_sc *drv_data = (struct sony_sc *)data;
> +	struct led_classdev *led;
> +	unsigned long flags;
> +	unsigned long delay_on, delay_off;
> +	int dev_added, ret;
> +	__u8 charging, capacity;
> +
> +	/* Check if new LEDs were added since the last time */
> +	dev_added = atomic_cmpxchg(&drv_data->trigger_device_added, 1, 0);
> +
> +	/* Get the battery info */

That's what I meant, is it right for a HID driver to check battery
status and _decide_ how to represent that info to users?

Anyhow, let's see also what other people think.

> +	spin_lock_irqsave(&drv_data->lock, flags);
> +	charging = drv_data->battery_charging;
> +	capacity = drv_data->battery_capacity;
> +	spin_unlock_irqrestore(&drv_data->lock, flags);
> +
> +	/* Don't set the LEDs if nothing has changed */
> +	if (!dev_added && drv_data->trigger_capacity == capacity &&
> +		drv_data->trigger_charging == charging)
> +		goto reset_timer;
> +
> +	if (charging) {
> +		/* Charging: blink at 1 sec intervals */
> +		delay_on = delay_off = 1000;
> +		led_trigger_blink(&drv_data->battery_trigger, &delay_on,
> +				&delay_off);
> +	} else if (capacity <= 20) {
> +		/* Low battery: blink at 500ms intervals */
> +		delay_on = delay_off = 500;
> +		led_trigger_blink(&drv_data->battery_trigger, &delay_on,
> +				&delay_off);
> +	} else {
> +		/*
> +		 * Normal: set the brightness to stop blinking
> +		 *
> +		 * This just walks the list of LEDs on the trigger and sets the
> +		 * brightness to the existing value. This leaves the brightness
> +		 * the same but the blinking is stopped.
> +		 */
> +		read_lock(&drv_data->battery_trigger.leddev_list_lock);
> +		list_for_each_entry(led,
> +			&drv_data->battery_trigger.led_cdevs, trig_list) {
> +			led_set_brightness(led, led->brightness);
> +		}
> +		read_unlock(&drv_data->battery_trigger.leddev_list_lock);
> +	}
> +
> +	/* Cache the power state for next time */
> +	drv_data->trigger_charging = charging;
> +	drv_data->trigger_capacity = capacity;
> +
> +reset_timer:
> +	ret = mod_timer(&drv_data->battery_trigger_timer,
> +			jiffies + msecs_to_jiffies(500));
> +
> +	if (ret < 0)
> +		hid_err(drv_data->hdev,
> +			"Failed to set battery trigger timer\n");
> +}
> +
> +static void sony_battery_trigger_activate(struct led_classdev *led)
> +{
> +	struct sony_sc *sc;
> +
> +	sc = container_of(led->trigger, struct sony_sc, battery_trigger);
> +
> +	/*
> +	 * Set the device_added flag to tell the timer function that it
> +	 * should send an update even if the power state hasn't changed.
> +	 */
> +	atomic_set(&sc->trigger_device_added, 1);
> +}
> +
> +static int sony_battery_trigger_init(struct sony_sc *sc)
> +{
> +	int ret;
> +
> +	sc->battery_trigger.name = kasprintf(GFP_KERNEL,
> +				"%s-blink-low-or-charging", sc->battery.name);
> +	if (!sc->battery.name)
> +		return -ENOMEM;
> +
> +	sc->battery_trigger.activate = sony_battery_trigger_activate;
> +
> +	ret = led_trigger_register(&sc->battery_trigger);
> +	if (ret < 0)
> +		goto trigger_failure;
> +
> +	setup_timer(&sc->battery_trigger_timer,
> +			sony_battery_trigger_callback, (unsigned long)sc);
> +
> +	ret = mod_timer(&sc->battery_trigger_timer,
> +			jiffies + msecs_to_jiffies(500));
> +	if (ret < 0)
> +		goto timer_failure;
> +
> +	sc->trigger_initialized = 1;
> +
> +	return 0;
> +
> +timer_failure:
> +	led_trigger_unregister(&sc->battery_trigger);
> +trigger_failure:
> +	kfree(sc->battery_trigger.name);
> +	return ret;
> +}
> +
> +static void sony_battery_trigger_remove(struct sony_sc *sc)
> +{
> +	if (sc->trigger_initialized) {
> +		del_timer_sync(&sc->battery_trigger_timer);
> +		led_trigger_unregister(&sc->battery_trigger);
> +		kfree(sc->battery_trigger.name);
> +	}
> +}
> +#else
> +static int sony_battery_trigger_init(struct sony_sc *sc)
> +{
> +	/* Nothing to do */
> +	return 0;
> +}
> +
> +static void sony_battery_trigger_remove(struct sony_sc *sc)
> +{
> +	/* Nothing to do */
> +}
> +#endif
> +
>  static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
>  					int w, int h)
>  {
> @@ -1786,14 +1933,12 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	if (ret < 0)
>  		goto err_stop;
>  
> -	if (sc->quirks & SONY_LED_SUPPORT) {
> -		ret = sony_leds_init(sc);
> +	if (sc->quirks & SONY_BATTERY_SUPPORT) {
> +		ret = sony_battery_probe(sc);
>  		if (ret < 0)
>  			goto err_stop;
> -	}
>  
> -	if (sc->quirks & SONY_BATTERY_SUPPORT) {
> -		ret = sony_battery_probe(sc);
> +		ret = sony_battery_trigger_init(sc);
>  		if (ret < 0)
>  			goto err_stop;
>  
> @@ -1805,6 +1950,12 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		}
>  	}
>  
> +	if (sc->quirks & SONY_LED_SUPPORT) {
> +		ret = sony_leds_init(sc);
> +		if (ret < 0)
> +			goto err_stop;
> +	}
> +
>  	if (sc->quirks & SONY_FF_SUPPORT) {
>  		ret = sony_init_ff(sc);
>  		if (ret < 0)
> @@ -1817,8 +1968,10 @@ err_close:
>  err_stop:
>  	if (sc->quirks & SONY_LED_SUPPORT)
>  		sony_leds_remove(sc);
> -	if (sc->quirks & SONY_BATTERY_SUPPORT)
> +	if (sc->quirks & SONY_BATTERY_SUPPORT) {
> +		sony_battery_trigger_remove(sc);
>  		sony_battery_remove(sc);
> +	}
>  	sony_cancel_work_sync(sc);
>  	sony_remove_dev_list(sc);
>  	hid_hw_stop(hdev);
> @@ -1834,6 +1987,7 @@ static void sony_remove(struct hid_device *hdev)
>  
>  	if (sc->quirks & SONY_BATTERY_SUPPORT) {
>  		hid_hw_close(hdev);
> +		sony_battery_trigger_remove(sc);
>  		sony_battery_remove(sc);
>  	}
>  
> -- 
> 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 5/6] HID: sony: Add an led trigger to report controller battery status.

[Frank Praznik]

On 2/28/2014 22:59, Frank Praznik wrote:
> Creates an LED trigger that changes LED behavior depending on the state of the
> controller battery.
>
> The trigger function runs on a 500 millisecond timer and only updates the LEDs
> if the controller power state has changed or a new device has been added to the
> trigger.
>
> The trigger sets the LEDs to solid if the controller is in wireless mode and
> the battery is above 20% power or if the controller is plugged in and the
> battery has completed charging.  If the controller is not charging and the
> battery drops to or below 20% it blinks the LEDs in 500ms intervals. If the
> controller is plugged in and charging it blinks the LEDs in 1 second intervals.
>
> The order of subsystem initialization had to be changed in sony_probe() so that
> the trigger is created before the LEDs are initialized.
>
> By default the controller LEDs are set to the trigger local to that controller.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
>   drivers/hid/hid-sony.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 162 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 914a6cc..d7889ac 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -730,6 +730,17 @@ struct sony_sc {
>   	struct work_struct state_worker;
>   	struct power_supply battery;
>   
> +#ifdef CONFIG_LEDS_TRIGGERS
> +	spinlock_t trigger_lock;
> +	struct led_trigger battery_trigger;
> +	struct timer_list battery_trigger_timer;
> +	atomic_t trigger_device_added;
> +	__u8 trigger_initialized;
> +	__u8 trigger_timer_initialized;
> +	__u8 trigger_capacity;
> +	__u8 trigger_charging;
> +#endif
> +
>   #ifdef CONFIG_SONY_FF
>   	__u8 left;
>   	__u8 right;
> @@ -1329,14 +1340,19 @@ static int sony_leds_init(struct sony_sc *sc)
>   		if (!(sc->quirks & BUZZ_CONTROLLER))
>   			led->blink_set = sony_blink_set;
>   
> +#ifdef CONFIG_LEDS_TRIGGERS
> +		led->default_trigger = sc->battery_trigger.name;
> +#endif
> +
> +		sc->leds[n] = led;
> +
>   		ret = led_classdev_register(&hdev->dev, led);
>   		if (ret) {
>   			hid_err(hdev, "Failed to register LED %d\n", n);
>   			kfree(led);
> +			sc->leds[n] = NULL;
>   			goto error_leds;
>   		}
> -
> -		sc->leds[n] = led;
>   	}
>   
>   	return ret;
> @@ -1552,6 +1568,137 @@ static void sony_battery_remove(struct sony_sc *sc)
>   	sc->battery.name = NULL;
>   }
>   
> +#ifdef CONFIG_LEDS_TRIGGERS
> +static void sony_battery_trigger_callback(unsigned long data)
> +{
> +	struct sony_sc *drv_data = (struct sony_sc *)data;
> +	struct led_classdev *led;
> +	unsigned long flags;
> +	unsigned long delay_on, delay_off;
> +	int dev_added, ret;
> +	__u8 charging, capacity;
> +
> +	/* Check if new LEDs were added since the last time */
> +	dev_added = atomic_cmpxchg(&drv_data->trigger_device_added, 1, 0);
> +
> +	/* Get the battery info */
> +	spin_lock_irqsave(&drv_data->lock, flags);
> +	charging = drv_data->battery_charging;
> +	capacity = drv_data->battery_capacity;
> +	spin_unlock_irqrestore(&drv_data->lock, flags);
> +
> +	/* Don't set the LEDs if nothing has changed */
> +	if (!dev_added && drv_data->trigger_capacity == capacity &&
> +		drv_data->trigger_charging == charging)
> +		goto reset_timer;
> +
> +	if (charging) {
> +		/* Charging: blink at 1 sec intervals */
> +		delay_on = delay_off = 1000;
> +		led_trigger_blink(&drv_data->battery_trigger, &delay_on,
> +				&delay_off);
> +	} else if (capacity <= 20) {
> +		/* Low battery: blink at 500ms intervals */
> +		delay_on = delay_off = 500;
> +		led_trigger_blink(&drv_data->battery_trigger, &delay_on,
> +				&delay_off);
> +	} else {
> +		/*
> +		 * Normal: set the brightness to stop blinking
> +		 *
> +		 * This just walks the list of LEDs on the trigger and sets the
> +		 * brightness to the existing value. This leaves the brightness
> +		 * the same but the blinking is stopped.
> +		 */
> +		read_lock(&drv_data->battery_trigger.leddev_list_lock);
> +		list_for_each_entry(led,
> +			&drv_data->battery_trigger.led_cdevs, trig_list) {
> +			led_set_brightness(led, led->brightness);
> +		}
> +		read_unlock(&drv_data->battery_trigger.leddev_list_lock);
> +	}
> +
> +	/* Cache the power state for next time */
> +	drv_data->trigger_charging = charging;
> +	drv_data->trigger_capacity = capacity;
> +
> +reset_timer:
> +	ret = mod_timer(&drv_data->battery_trigger_timer,
> +			jiffies + msecs_to_jiffies(500));
> +
> +	if (ret < 0)
> +		hid_err(drv_data->hdev,
> +			"Failed to set battery trigger timer\n");
> +}
> +
> +static void sony_battery_trigger_activate(struct led_classdev *led)
> +{
> +	struct sony_sc *sc;
> +
> +	sc = container_of(led->trigger, struct sony_sc, battery_trigger);
> +
> +	/*
> +	 * Set the device_added flag to tell the timer function that it
> +	 * should send an update even if the power state hasn't changed.
> +	 */
> +	atomic_set(&sc->trigger_device_added, 1);
> +}
> +
> +static int sony_battery_trigger_init(struct sony_sc *sc)
> +{
> +	int ret;
> +
> +	sc->battery_trigger.name = kasprintf(GFP_KERNEL,
> +				"%s-blink-low-or-charging", sc->battery.name);
> +	if (!sc->battery.name)
> +		return -ENOMEM;
> +
> +	sc->battery_trigger.activate = sony_battery_trigger_activate;
> +
> +	ret = led_trigger_register(&sc->battery_trigger);
> +	if (ret < 0)
> +		goto trigger_failure;
> +
> +	setup_timer(&sc->battery_trigger_timer,
> +			sony_battery_trigger_callback, (unsigned long)sc);
> +
> +	ret = mod_timer(&sc->battery_trigger_timer,
> +			jiffies + msecs_to_jiffies(500));
> +	if (ret < 0)
> +		goto timer_failure;
> +
> +	sc->trigger_initialized = 1;
> +
> +	return 0;
> +
> +timer_failure:
> +	led_trigger_unregister(&sc->battery_trigger);
> +trigger_failure:
> +	kfree(sc->battery_trigger.name);
> +	return ret;
> +}
> +
> +static void sony_battery_trigger_remove(struct sony_sc *sc)
> +{
> +	if (sc->trigger_initialized) {
> +		del_timer_sync(&sc->battery_trigger_timer);
> +		led_trigger_unregister(&sc->battery_trigger);
> +		kfree(sc->battery_trigger.name);
> +	}
> +}
> +#else
> +static int sony_battery_trigger_init(struct sony_sc *sc)
> +{
> +	/* Nothing to do */
> +	return 0;
> +}
> +
> +static void sony_battery_trigger_remove(struct sony_sc *sc)
> +{
> +	/* Nothing to do */
> +}
> +#endif
> +
>   static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
>   					int w, int h)
>   {
> @@ -1786,14 +1933,12 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   	if (ret < 0)
>   		goto err_stop;
>   
> -	if (sc->quirks & SONY_LED_SUPPORT) {
> -		ret = sony_leds_init(sc);
> +	if (sc->quirks & SONY_BATTERY_SUPPORT) {
> +		ret = sony_battery_probe(sc);
>   		if (ret < 0)
>   			goto err_stop;
> -	}
>   
> -	if (sc->quirks & SONY_BATTERY_SUPPORT) {
> -		ret = sony_battery_probe(sc);
> +		ret = sony_battery_trigger_init(sc);
>   		if (ret < 0)
>   			goto err_stop;
>   
> @@ -1805,6 +1950,12 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   		}
>   	}
>   
> +	if (sc->quirks & SONY_LED_SUPPORT) {
> +		ret = sony_leds_init(sc);
> +		if (ret < 0)
> +			goto err_stop;
> +	}
> +
>   	if (sc->quirks & SONY_FF_SUPPORT) {
>   		ret = sony_init_ff(sc);
>   		if (ret < 0)
> @@ -1817,8 +1968,10 @@ err_close:
>   err_stop:
>   	if (sc->quirks & SONY_LED_SUPPORT)
>   		sony_leds_remove(sc);
> -	if (sc->quirks & SONY_BATTERY_SUPPORT)
> +	if (sc->quirks & SONY_BATTERY_SUPPORT) {
> +		sony_battery_trigger_remove(sc);
>   		sony_battery_remove(sc);
> +	}
>   	sony_cancel_work_sync(sc);
>   	sony_remove_dev_list(sc);
>   	hid_hw_stop(hdev);
> @@ -1834,6 +1987,7 @@ static void sony_remove(struct hid_device *hdev)
>   
>   	if (sc->quirks & SONY_BATTERY_SUPPORT) {
>   		hid_hw_close(hdev);
> +		sony_battery_trigger_remove(sc);
>   		sony_battery_remove(sc);
>   	}
>   
I'm going to self-NAK this patch as I've found a problem scenario with 
it where LEDs might not blink when they should.  I would still 
appreciate feedback on whether or not this concept is worth persuing 
before spending more time on it though.

[PATCH 6/6] HID: sony: Turn on the LEDs by default.

[Frank Praznik]

Initialize the controller LEDs to a default value that isn't all-off so that
there is some visible indicator that the controller is powered on and
connected.

On the Sixaxis LED number 1 is turned on.
One the DualShock 4 the light bar is set to blue at the lowest brightness.

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

 Right now it just sets all of the controllers to the same value (LED 1 on the
 sixaxis and blue on the DS4) to indicate that the controller is connected and
 show the battery status set by the trigger in the previous patch. I'd like to
 be able to set the LEDs to the actual numerical controller value, but I'm not
 sure how to do that, other than the solution proposed in an xpad patch a few
 weeks ago where the minor number of the joydev device was retrieved.

 drivers/hid/hid-sony.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index d7889ac..7912f0a 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1101,6 +1101,44 @@ static int dualshock4_set_operational_bt(struct hid_device *hdev)
 				HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
 }
 
+static void sixaxis_set_leds_from_devnum(int devnum, __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]));
+	devnum %= 10;
+	memcpy(values, sixaxis_leds[devnum], sizeof(sixaxis_leds[devnum]));
+}
+
+static void dualshock4_set_leds_from_devnum(int devnum, __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]));
+	devnum %= 7;
+	memcpy(values, color_code[devnum], sizeof(color_code[devnum]));
+}
+
 static void buzz_set_leds(struct hid_device *hdev, const __u8 *leds)
 {
 	struct list_head *report_list =
@@ -1278,7 +1316,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));
 
@@ -1292,12 +1330,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_devnum(0, initial_values);
 		sc->led_count = 3;
 		max_brightness = 255;
 		use_colors = 1;
 		name_len = 0;
 		name_fmt = "%s:%s";
 	} else {
+		sixaxis_set_leds_from_devnum(0, initial_values);
 		sc->led_count = 4;
 		max_brightness = 1;
 		use_colors = 0;
@@ -1332,7 +1372,7 @@ 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;
-- 
1.8.5.3

Re: [PATCH 6/6] HID: sony: Turn on the LEDs by default.

[Antonio Ospite]

On Fri, 28 Feb 2014 22:59:01 -0500
Frank Praznik <frank.praznik@oh.rr.com> wrote:

> Initialize the controller LEDs to a default value that isn't all-off so that
> there is some visible indicator that the controller is powered on and
> connected.
> 
> On the Sixaxis LED number 1 is turned on.

I'd just NAK this.

Please start with all LEDs blinking just as the PS3 does, and let
userpsace decide what the controller number is.

Ciao,
   Antonio

> One the DualShock 4 the light bar is set to blue at the lowest brightness.
> 
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
> 
>  Right now it just sets all of the controllers to the same value (LED 1 on the
>  sixaxis and blue on the DS4) to indicate that the controller is connected and
>  show the battery status set by the trigger in the previous patch. I'd like to
>  be able to set the LEDs to the actual numerical controller value, but I'm not
>  sure how to do that, other than the solution proposed in an xpad patch a few
>  weeks ago where the minor number of the joydev device was retrieved.
>

As I said, I would just avoid doing that in the kernel.

>  drivers/hid/hid-sony.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index d7889ac..7912f0a 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -1101,6 +1101,44 @@ static int dualshock4_set_operational_bt(struct hid_device *hdev)
>  				HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>  }
>  
> +static void sixaxis_set_leds_from_devnum(int devnum, __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]));
> +	devnum %= 10;
> +	memcpy(values, sixaxis_leds[devnum], sizeof(sixaxis_leds[devnum]));
> +}
> +
> +static void dualshock4_set_leds_from_devnum(int devnum, __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]));
> +	devnum %= 7;
> +	memcpy(values, color_code[devnum], sizeof(color_code[devnum]));
> +}
> +
>  static void buzz_set_leds(struct hid_device *hdev, const __u8 *leds)
>  {
>  	struct list_head *report_list =
> @@ -1278,7 +1316,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));
>  
> @@ -1292,12 +1330,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_devnum(0, initial_values);
>  		sc->led_count = 3;
>  		max_brightness = 255;
>  		use_colors = 1;
>  		name_len = 0;
>  		name_fmt = "%s:%s";
>  	} else {
> +		sixaxis_set_leds_from_devnum(0, initial_values);
>  		sc->led_count = 4;
>  		max_brightness = 1;
>  		use_colors = 0;
> @@ -1332,7 +1372,7 @@ 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;
> -- 
> 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 0/6] HID: sony: More Sony controller fixes and improvements.

[Antonio Ospite]

Hi Frank,

On Fri, 28 Feb 2014 22:58:55 -0500
Frank Praznik <frank.praznik@oh.rr.com> wrote:

> This set consists of one bugfix, two mostly cosmetic changes and three larger
> patches for the LED subsystem.
> 
> Patch #4 adds hardware blink support to the controller LEDs.  Values from 0 to
> 2.5 seconds are supported by the hardware.  The Sixaxis can set all of the LEDs
> individually, but the DualShock 4 only has one global setting for the entire
> light bar so only the value from the most recently set LED is used.
>

Adding this is OK, as it adds access to something supported by the
hardware.

> Patch #5 adds an LED trigger that reports the controller battery status via the
> registered LEDs.  The LEDs will flash if the controller is charging or if the
> battery is low, and remain solid otherwise.
>

This kind of logic _may_ belong to userspace. More comments in the
actual patch.

> Patch #6 initializes the LEDs to a default value of LED 1 on the Sixaxis and
> blue on the DualShock 4 so there is some indication that the controller is
> powered on and connected in the case of Bluetooth.  The code can be used to set
> the LEDs based on the device number, but I'm not sure how to actually retrieve
> the controller number from the system.  I saw the xpad patches posted a few
> weeks ago where the minor number of the joydev device was used, but I'm under
> the impression that doing that is not ideal.  Any suggestions?

Setting the controller number is done by the bluez sixaxis plugin[1]
(in bluez 5.x) following the X in /dev/input/jsX, this covers the
case of a mixed-joypad scenario, IMHO it makes sense that the
controller number matches the joystick device number.
Imagine js0->Sixaxis1, js1->wiimote, js2->Sixaxis2, I think it make
sense to have the LEDs on Sixaxis2 say "controller 3", not 2.

This has been done in userspace with libudev for 2 reasons:
  1. the hid drivers should not have knowledge of the joystick layer;
  2. kernel drivers should be as simple as possible, and try to just
     exposing hardware functionalities but with as less "business logic"
     as possible in them.

The current implementation in the bluez plugin uses hidraw, but support
for the sysfs led class could be added in order to avoid conflicts with
the rumble; IIRC, currently, setting rumble values could override the
LED settings done via hidraw, because the LEDs state is not tracked in
the latter case.

Ciao,
   Antonio

[1]
http://git.kernel.org/cgit/bluetooth/bluez.git/tree/plugins/sixaxis.c

-- 
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 0/6] HID: sony: More Sony controller fixes and improvements.

[Frank Praznik]

On 3/1/2014 08:53, Antonio Ospite wrote:
> Hi Frank,
>
> On Fri, 28 Feb 2014 22:58:55 -0500
> Frank Praznik <frank.praznik@oh.rr.com> wrote:
>
>> This set consists of one bugfix, two mostly cosmetic changes and three larger
>> patches for the LED subsystem.
>>
>> Patch #4 adds hardware blink support to the controller LEDs.  Values from 0 to
>> 2.5 seconds are supported by the hardware.  The Sixaxis can set all of the LEDs
>> individually, but the DualShock 4 only has one global setting for the entire
>> light bar so only the value from the most recently set LED is used.
>>
> Adding this is OK, as it adds access to something supported by the
> hardware.
>
>> Patch #5 adds an LED trigger that reports the controller battery status via the
>> registered LEDs.  The LEDs will flash if the controller is charging or if the
>> battery is low, and remain solid otherwise.
>>
> This kind of logic _may_ belong to userspace. More comments in the
> actual patch.
Functionally this trigger is no different from the ones registered by 
the power supply system when a battery is registered, aside from the 
specific conditions under which the LED blinks.  I can understand the 
reservations about setting it as the default, but at the same time it's 
a trigger which can be easily disabled on the controller LEDs or be used 
to control other LED devices if the user desires it.

If this is something best kept out of kernel code though, that's fine.
>
>> Patch #6 initializes the LEDs to a default value of LED 1 on the Sixaxis and
>> blue on the DualShock 4 so there is some indication that the controller is
>> powered on and connected in the case of Bluetooth.  The code can be used to set
>> the LEDs based on the device number, but I'm not sure how to actually retrieve
>> the controller number from the system.  I saw the xpad patches posted a few
>> weeks ago where the minor number of the joydev device was used, but I'm under
>> the impression that doing that is not ideal.  Any suggestions?
> Setting the controller number is done by the bluez sixaxis plugin[1]
> (in bluez 5.x) following the X in /dev/input/jsX, this covers the
> case of a mixed-joypad scenario, IMHO it makes sense that the
> controller number matches the joystick device number.
> Imagine js0->Sixaxis1, js1->wiimote, js2->Sixaxis2, I think it make
> sense to have the LEDs on Sixaxis2 say "controller 3", not 2.
>
> This has been done in userspace with libudev for 2 reasons:
>    1. the hid drivers should not have knowledge of the joystick layer;
>    2. kernel drivers should be as simple as possible, and try to just
>       exposing hardware functionalities but with as less "business logic"
>       as possible in them.
>
> The current implementation in the bluez plugin uses hidraw, but support
> for the sysfs led class could be added in order to avoid conflicts with
> the rumble; IIRC, currently, setting rumble values could override the
> LED settings done via hidraw, because the LEDs state is not tracked in
> the latter case.
>
> Ciao,
>     Antonio
>
> [1]
> http://git.kernel.org/cgit/bluetooth/bluez.git/tree/plugins/sixaxis.c
>
This can be done in the driver. See 
https://www.mail-archive.com/linux-input@vger.kernel.org/msg08103.html

It seems that the main problem with that patch is that modern systems 
shouldn't be relying on joydev for this functionality.  I'd like to know 
what David Herrmann and Greg Kroah-Hartman came up with regarding the 
solution mentioned in the reply as it would be nice to be able to set 
the LEDs to the proper default values in the driver without needing to 
rely on an external daemon. Setting the defaults in the driver doesn't 
interfere with setting custom values after the device is initialized, so 
there are no issues if the user wants to use a custom LED daemon.

As far as the behavior of patch #6 (setting the LEDs to the same number 
or color on every connected device just to indicate that the controller 
is turned on), the xpad and wiimote drivers both initialize the LEDs to 
some default value where at least one is on to indicate that the 
controller is powered on and connected to the system.  The xpad driver 
increments an atomic counter for assigning values as controllers are 
connected and the wiimote always sets LED #1 to on.  Not ideal, but it 
serves it's purpose.

Personally I don't like the idea of relying on a BlueZ plugin to set the 
controller LED values as it seems to bring a lot of issues with it: 
users may not have BlueZ installed or enabled, some distros still use an 
old version, the plugin relies on joydev to get the device number which 
is why the patch I linked was NAKed, the current plugin implementation 
doesn't set them via sysfs so the setting will be lost if force-feedback 
is used and the plugin could conflict with other user-installed daemons 
that set the LEDs (unless udev guarantees a notification order?).  In 
the latter scenario, the user could disable the plugin, but then you 
lose the Sixaxis pairing functionality that it provides.  I also have to 
question as to why BlueZ is considered an appropriate place to set 
controller LEDs, particularly on controllers that aren't connected via 
Bluetooth.

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

[David Herrmann]

Hi

On Sun, Mar 2, 2014 at 5:26 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> On 3/1/2014 08:53, Antonio Ospite wrote:
>>
>> Hi Frank,
>>
>> On Fri, 28 Feb 2014 22:58:55 -0500
>> Frank Praznik <frank.praznik@oh.rr.com> wrote:
>>
>>> This set consists of one bugfix, two mostly cosmetic changes and three
>>> larger
>>> patches for the LED subsystem.
>>>
>>> Patch #4 adds hardware blink support to the controller LEDs.  Values from
>>> 0 to
>>> 2.5 seconds are supported by the hardware.  The Sixaxis can set all of
>>> the LEDs
>>> individually, but the DualShock 4 only has one global setting for the
>>> entire
>>> light bar so only the value from the most recently set LED is used.
>>>
>> Adding this is OK, as it adds access to something supported by the
>> hardware.
>>
>>> Patch #5 adds an LED trigger that reports the controller battery status
>>> via the
>>> registered LEDs.  The LEDs will flash if the controller is charging or if
>>> the
>>> battery is low, and remain solid otherwise.
>>>
>> This kind of logic _may_ belong to userspace. More comments in the
>> actual patch.
>
> Functionally this trigger is no different from the ones registered by the
> power supply system when a battery is registered, aside from the specific
> conditions under which the LED blinks.  I can understand the reservations
> about setting it as the default, but at the same time it's a trigger which
> can be easily disabled on the controller LEDs or be used to control other
> LED devices if the user desires it.
>
> If this is something best kept out of kernel code though, that's fine.
>
>>
>>> Patch #6 initializes the LEDs to a default value of LED 1 on the Sixaxis
>>> and
>>> blue on the DualShock 4 so there is some indication that the controller
>>> is
>>> powered on and connected in the case of Bluetooth.  The code can be used
>>> to set
>>> the LEDs based on the device number, but I'm not sure how to actually
>>> retrieve
>>> the controller number from the system.  I saw the xpad patches posted a
>>> few
>>> weeks ago where the minor number of the joydev device was used, but I'm
>>> under
>>> the impression that doing that is not ideal.  Any suggestions?
>>
>> Setting the controller number is done by the bluez sixaxis plugin[1]
>> (in bluez 5.x) following the X in /dev/input/jsX, this covers the
>> case of a mixed-joypad scenario, IMHO it makes sense that the
>> controller number matches the joystick device number.
>> Imagine js0->Sixaxis1, js1->wiimote, js2->Sixaxis2, I think it make
>> sense to have the LEDs on Sixaxis2 say "controller 3", not 2.
>>
>> This has been done in userspace with libudev for 2 reasons:
>>    1. the hid drivers should not have knowledge of the joystick layer;
>>    2. kernel drivers should be as simple as possible, and try to just
>>       exposing hardware functionalities but with as less "business logic"
>>       as possible in them.
>>
>> The current implementation in the bluez plugin uses hidraw, but support
>> for the sysfs led class could be added in order to avoid conflicts with
>> the rumble; IIRC, currently, setting rumble values could override the
>> LED settings done via hidraw, because the LEDs state is not tracked in
>> the latter case.
>>
>> Ciao,
>>     Antonio
>>
>> [1]
>> http://git.kernel.org/cgit/bluetooth/bluez.git/tree/plugins/sixaxis.c
>>
> This can be done in the driver. See
> https://www.mail-archive.com/linux-input@vger.kernel.org/msg08103.html
>
> It seems that the main problem with that patch is that modern systems
> shouldn't be relying on joydev for this functionality.  I'd like to know
> what David Herrmann and Greg Kroah-Hartman came up with regarding the
> solution mentioned in the reply as it would be nice to be able to set the
> LEDs to the proper default values in the driver without needing to rely on
> an external daemon. Setting the defaults in the driver doesn't interfere
> with setting custom values after the device is initialized, so there are no
> issues if the user wants to use a custom LED daemon.

The application using the controllers should do this. It allows to set
coherent values across different devices and buses. So you can have
one BT device, one USB device and one whatever device, but assign
sequential numbers to them from the application. We cannot do that in
the kernel, as we don't know which policy to follow. However, we can
provide a sane default. joydev is the *wrong* way to do that, though.
Instead, you should just use an ida/idr in your driver and assign
numbers properly. You can also use usb-interface-ids or similar if
they make sense. But please don't try to access input-handlers like
evdev or joydev. They're one layer atop, not below your driver!

Thanks
David

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

[Antonio Ospite]

On Sun, 02 Mar 2014 11:26:14 -0500
Frank Praznik <frank.praznik@oh.rr.com> wrote:

> On 3/1/2014 08:53, Antonio Ospite wrote:
[...]
> >
> >> Patch #6 initializes the LEDs to a default value of LED 1 on the Sixaxis and
> >> blue on the DualShock 4 so there is some indication that the controller is
> >> powered on and connected in the case of Bluetooth.  The code can be used to set
> >> the LEDs based on the device number, but I'm not sure how to actually retrieve
> >> the controller number from the system.  I saw the xpad patches posted a few
> >> weeks ago where the minor number of the joydev device was used, but I'm under
> >> the impression that doing that is not ideal.  Any suggestions?
> > Setting the controller number is done by the bluez sixaxis plugin[1]
> > (in bluez 5.x) following the X in /dev/input/jsX, this covers the
> > case of a mixed-joypad scenario, IMHO it makes sense that the
> > controller number matches the joystick device number.
> > Imagine js0->Sixaxis1, js1->wiimote, js2->Sixaxis2, I think it make
> > sense to have the LEDs on Sixaxis2 say "controller 3", not 2.
> >
> > This has been done in userspace with libudev for 2 reasons:
> >    1. the hid drivers should not have knowledge of the joystick layer;
> >    2. kernel drivers should be as simple as possible, and try to just
> >       exposing hardware functionalities but with as less "business logic"
> >       as possible in them.
> >
> > The current implementation in the bluez plugin uses hidraw, but support
> > for the sysfs led class could be added in order to avoid conflicts with
> > the rumble; IIRC, currently, setting rumble values could override the
> > LED settings done via hidraw, because the LEDs state is not tracked in
> > the latter case.
> >
> > Ciao,
> >     Antonio
> >
> > [1]
> > http://git.kernel.org/cgit/bluetooth/bluez.git/tree/plugins/sixaxis.c
> >
> This can be done in the driver. See 
> https://www.mail-archive.com/linux-input@vger.kernel.org/msg08103.html
>

xpad is a joystick driver, while hid-sony is a HID driver.

> It seems that the main problem with that patch is that modern systems 
> shouldn't be relying on joydev for this functionality.  I'd like to know 
> what David Herrmann and Greg Kroah-Hartman came up with regarding the 
> solution mentioned in the reply as it would be nice to be able to set 
> the LEDs to the proper default values in the driver without needing to 
> rely on an external daemon. Setting the defaults in the driver doesn't 
> interfere with setting custom values after the device is initialized, so 
> there are no issues if the user wants to use a custom LED daemon.

The way I see it, the problem here is not about joydev itself it's
about:
  1. the layering relationship between HID and joydev
  2. whether or not we consider assigning joypad numbers as a kernel
     job.

The NAK came because of 1.; as 2. is more debatable I guess.

> As far as the behavior of patch #6 (setting the LEDs to the same number 
> or color on every connected device just to indicate that the controller 
> is turned on), the xpad and wiimote drivers both initialize the LEDs to 
> some default value where at least one is on to indicate that the 
> controller is powered on and connected to the system.  The xpad driver 
> increments an atomic counter for assigning values as controllers are 
> connected and the wiimote always sets LED #1 to on.  Not ideal, but it 
> serves it's purpose.
>

For the sixaxis the all-blink pattern is used, it's a really dumb
indicator, but it's still an indicator.

> Personally I don't like the idea of relying on a BlueZ plugin to set the 
> controller LED values as it seems to bring a lot of issues with it: 
> users may not have BlueZ installed or enabled, some distros still use an 
> old version, the plugin relies on joydev to get the device number which 
> is why the patch I linked was NAKed, the current plugin implementation 
> doesn't set them via sysfs so the setting will be lost if force-feedback 
> is used and the plugin could conflict with other user-installed daemons 
> that set the LEDs (unless udev guarantees a notification order?).  In 
> the latter scenario, the user could disable the plugin, but then you 
> lose the Sixaxis pairing functionality that it provides.  I also have to 
> question as to why BlueZ is considered an appropriate place to set 
> controller LEDs, particularly on controllers that aren't connected via 
> Bluetooth.

Let me answer the last question first, the rationale was:

 1. the common use case for the sixaxis is considered to be its use
    via Bluetooth, can we agree on that?
 2. under this assumption BlueZ was chosen as a convenient place to do
    the pairing and association.
 3. Paring requires to access the device via USB in order to retrieve
    its bdaddr and set the master bdaddr.
 4. Once we are accessing the device via USB and BT in the BlueZ plugin
    anyway, just let's set the LEDs here.

A more general "excuse" is that clutter in userspace is slightly more
acceptable than clutter in kernel code.

And to reply to the other questions, yes, the bluez plugin is not
perfect by any means: it enforces dependencies, it's not using the leds
class yet, but it can be improved and IMHO it's still the most
convenient way to have responsibilities separated: the kernel driver
provides access to the hardware functionality and the userspace
software decides what to do with them.

However, since you are the one currently working on things you are
entitled with a stronger voice here, I am just whispering my opinions :)

Ciao,
   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 0/6] HID: sony: More Sony controller fixes and improvements.

[Frank Praznik]

On 3/4/2014 07:34, Antonio Ospite wrote:
> This can be done in the driver. See
> https://www.mail-archive.com/linux-input@vger.kernel.org/msg08103.html
>
>
> xpad is a joystick driver, while hid-sony is a HID driver.
I tried it and the hack works from HID space with some tweaking, but, 
yeah, it's a hack and doesn't belong in *any* driver.
> Let me answer the last question first, the rationale was:
>
>   1. the common use case for the sixaxis is considered to be its use
>      via Bluetooth, can we agree on that?
>   2. under this assumption BlueZ was chosen as a convenient place to do
>      the pairing and association.
>   3. Paring requires to access the device via USB in order to retrieve
>      its bdaddr and set the master bdaddr.
>   4. Once we are accessing the device via USB and BT in the BlueZ plugin
>      anyway, just let's set the LEDs here.
>
> A more general "excuse" is that clutter in userspace is slightly more
> acceptable than clutter in kernel code.
>
> And to reply to the other questions, yes, the bluez plugin is not
> perfect by any means: it enforces dependencies, it's not using the leds
> class yet, but it can be improved and IMHO it's still the most
> convenient way to have responsibilities separated: the kernel driver
> provides access to the hardware functionality and the userspace
> software decides what to do with them.
>
> However, since you are the one currently working on things you are
> entitled with a stronger voice here, I am just whispering my opinions :)
>
> Ciao,
>     Antonio
>
I understand your rationale, but I still think that the driver should 
assign sane defaults for cases where there isn't userspace software to 
set the LEDs.  Leaving the Sixaxis LEDs blinking doesn't differentiate 
between whether or not the controller is connected or trying to connect 
and on the Dualshock 4 the default is a bright-white battery draining 
light.  On the other hand, the current default of 'all-off', which has 
been the default since the initial LED patch, doesn't indicate whether 
the controller connected successfully or timed-out when using Bluetooth.

I currently have David's suggestion of assigning an id via an IDA 
allocator implemented in my working copy.  This can be used to both 
initialize the LEDs relative to other Sony controllers and provide a 
sane unique identifier for the battery identification string instead of 
the constantly incrementing atomic integer used now.  This is the most 
ideal solution IMO since it clearly communicates the current power and 
connection status of the controller, lets the user easily differentiate 
between controllers 1, 2, etc... and it provides defaults that most 
users shouldn't find annoying.  Of course, these are just defaults so if 
you want to use a BlueZ plugin or something else to set the values via 
userspace, there is nothing stopping you.

I'm going wait on v2 of this series until Benjamin Tissoires finishes 
his latest patches since I think patch #2 and beyond will conflict with 
his work.  Patch #1 is just a trivial one-line fix though, so that 
should still be good to go.