Re: [PATCH] leds: triggers: add invert to heartbeat

From: JiÅÃ Prchal
Date: Wed Oct 07 2015 - 10:15:41 EST


Hi Jacek,
comments below...

On 7.10.2015 15:32, Jacek Anaszewski wrote:
Hi Jiri,

Thanks for the patch. It's nice in general, but please see my
comments below.

On 10/07/2015 11:31 AM, Jiri Prchal wrote:
This patcht adds possibility to invert heartbeat blinking. The inverted LED is
more time ON then OFF.
It's because it looks better when the heartbeat LED is next to other LED which
is most time ON.
The invert value is exported same way via sysfs in file invert like oneshot. I
get inspiration from this trigger.

Please adjust commit message so as it wouldn't exceed 75 characters line
length limit. scripts/checkpatch.pl is useful for catching this kind of
problems.

Signed-off-by: Jiri Prchal <jiri.prchal@xxxxxxxxxxx>
---
drivers/leds/trigger/ledtrig-heartbeat.c | 49 ++++++++++++++++++++++++++++++--
1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c
index fea6871..c09c30b 100644
--- a/drivers/leds/trigger/ledtrig-heartbeat.c
+++ b/drivers/leds/trigger/ledtrig-heartbeat.c
@@ -27,6 +27,7 @@ struct heartbeat_trig_data {
unsigned int phase;
unsigned int period;
struct timer_list timer;
+ unsigned int invert;
};

static void led_heartbeat_function(unsigned long data)
@@ -56,21 +57,27 @@ static void led_heartbeat_function(unsigned long data)
msecs_to_jiffies(heartbeat_data->period);
delay = msecs_to_jiffies(70);
heartbeat_data->phase++;
- brightness = led_cdev->max_brightness;
+ if (!heartbeat_data->invert)
+ brightness = led_cdev->max_brightness;
break;
case 1:
delay = heartbeat_data->period / 4 - msecs_to_jiffies(70);
heartbeat_data->phase++;
+ if (heartbeat_data->invert)
+ brightness = led_cdev->max_brightness;

As you are at it, could you change, in a separate patch,
led_cdev->max_brightness to led_cdev->brightness? I think there is no
reason for which we shouldn't allow other values than max.
I changed it but it doesn't blink at all.


break;
case 2:
delay = msecs_to_jiffies(70);
heartbeat_data->phase++;
- brightness = led_cdev->max_brightness;
+ if (!heartbeat_data->invert)
+ brightness = led_cdev->max_brightness;
break;
default:
delay = heartbeat_data->period - heartbeat_data->period / 4 -
msecs_to_jiffies(70);
heartbeat_data->phase = 0;
+ if (heartbeat_data->invert)
+ brightness = led_cdev->max_brightness;
break;
}

@@ -78,15 +85,50 @@ static void led_heartbeat_function(unsigned long data)
mod_timer(&heartbeat_data->timer, jiffies + delay);
}

+static ssize_t led_invert_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data;
+
+ return sprintf(buf, "%u\n", heartbeat_data->invert);
+}
+
+static ssize_t led_invert_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t size)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data;
+ unsigned long state;
+ int ret;
+
+ ret = kstrtoul(buf, 0, &state);
+ if (ret)
+ return ret;
+
+ heartbeat_data->invert = !!state;
+
+ return size;
+}
+
+static DEVICE_ATTR(invert, 0644, led_invert_show, led_invert_store);
+
static void heartbeat_trig_activate(struct led_classdev *led_cdev)
{
struct heartbeat_trig_data *heartbeat_data;
+ int rc;

heartbeat_data = kzalloc(sizeof(*heartbeat_data), GFP_KERNEL);
if (!heartbeat_data)
return;

led_cdev->trigger_data = heartbeat_data;
+ rc = device_create_file(led_cdev->dev, &dev_attr_invert);
+ if (rc) {
+ kfree(led_cdev->trigger_data);
+ return;
+ }
+
setup_timer(&heartbeat_data->timer,
led_heartbeat_function, (unsigned long) led_cdev);
heartbeat_data->phase = 0;
@@ -100,9 +142,12 @@ static void heartbeat_trig_deactivate(struct led_classdev *led_cdev)

if (led_cdev->activated) {
del_timer_sync(&heartbeat_data->timer);
+ device_remove_file(led_cdev->dev, &dev_attr_invert);
kfree(heartbeat_data);
led_cdev->activated = false;
}
+
+ led_set_brightness(led_cdev, LED_OFF);

I believe this is a fix. Could you split it into a separate patch
and explain its merit?
I'm not sure of necessity of it, I copied it from oneshot.
Without it could leave LED in ON state. But never happens to me.
Should I produce separate patch to discuss it in separate thread?


}

static struct led_trigger heartbeat_led_trigger = {



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/