From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753874AbbJGOPi (ORCPT ); Wed, 7 Oct 2015 10:15:38 -0400 Received: from [188.175.113.102] ([188.175.113.102]:43021 "EHLO router.aksignal.cz" rhost-flags-FAIL-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1751187AbbJGOPg (ORCPT ); Wed, 7 Oct 2015 10:15:36 -0400 Reply-To: jiri.prchal@aksignal.cz Subject: Re: [PATCH] leds: triggers: add invert to heartbeat References: <56151ED2.5020300@samsung.com> To: Jacek Anaszewski Cc: rpurdie@rpsys.net, linux-leds@vger.kernel.org, cooloney@gmail.com, kyungmin.park@samsung.com, linux-kernel@vger.kernel.org From: =?UTF-8?B?SmnFmcOtIFByY2hhbA==?= Organization: AK signal Brno Message-ID: <561528DE.4070507@aksignal.cz> Date: Wed, 7 Oct 2015 16:14:54 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <56151ED2.5020300@samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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 = { >> > >