linux-kernel - Re: [PATCH] leds: triggers: add invert to heartbeat

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-id: <56151ED2.5020300@samsung.com>
Date:	Wed, 07 Oct 2015 15:32:02 +0200
From:	Jacek Anaszewski <j.anaszewski@...sung.com>
To:	Jiri Prchal <jiri.prchal@...ignal.cz>
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 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.

>   		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?

>   }
>
>   static struct led_trigger heartbeat_led_trigger = {
>


-- 
Best Regards,
Jacek Anaszewski
--
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/