Thread (17 messages) 17 messages, 4 authors, 2014-03-04

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

From: Antonio Ospite <hidden>
Date: 2014-03-01 14:36:21

On Fri, 28 Feb 2014 22:59:00 -0500
Frank Praznik [off-list ref] 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.
quoted hunk ↗ jump to hunk
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 <redacted>
---
 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.
quoted hunk ↗ jump to hunk
+	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?
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help