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?