On Fri, 28 Feb 2014 22:59:00 -0500
Frank Praznik <frank.praznik@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
> 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
