| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <561528DE.4070507@aksignal.cz>
Date: Wed, 7 Oct 2015 16:14:54 +0200
From: Jiří Prchal <jiri.prchal@...ignal.cz>
To: Jacek Anaszewski <j.anaszewski@...sung.com>
Cc: rpurdie@...ys.net, linux-leds@...r.kernel.org, cooloney@...il.com,
kyungmin.park@...sung.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] leds: triggers: add invert to heartbeat
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@...ignal.cz>
>> ---
>> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists