From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752828AbbJHHn3 (ORCPT ); Thu, 8 Oct 2015 03:43:29 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:47857 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751990AbbJHHn1 (ORCPT ); Thu, 8 Oct 2015 03:43:27 -0400 MIME-version: 1.0 Content-type: text/plain; charset=UTF-8; format=flowed X-AuditID: cbfec7f4-f79c56d0000012ee-d7-56161e9cd5d1 Content-transfer-encoding: 8BIT Message-id: <56161E9A.2000408@samsung.com> Date: Thu, 08 Oct 2015 09:43:22 +0200 From: Jacek Anaszewski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804 Thunderbird/17.0.8 To: jiri.prchal@aksignal.cz Cc: rpurdie@rpsys.net, linux-leds@vger.kernel.org, cooloney@gmail.com, kyungmin.park@samsung.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] leds: triggers: add invert to heartbeat References: <56151ED2.5020300@samsung.com> <561528DE.4070507@aksignal.cz> In-reply-to: <561528DE.4070507@aksignal.cz> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrKLMWRmVeSWpSXmKPExsVy+t/xK7pz5MTCDFpXSVsc3TmRyeLhYUuL s01v2C0u75rDZrH1zTpGi927nrI6sHmc+TWVzWPnrLvsHnvm/2D16NuyitHj8ya5ANYoLpuU 1JzMstQifbsEroz1Kz4xFaw2qdh/ZSpzA+MqjS5GTg4JAROJGYfPsELYYhIX7q1n62Lk4hAS WMoocWFRL1iCV0BQ4sfkeyxdjBwczALyEkcuZYOEmQXMJB61rGOGqH/GKPHqwAp2iHotid8/ 5zOD2CwCqhL9L54xgdhsAoYSP1+8BrNFBSIk/pzeBzZfREBaYs3pp4wQQxsYJa61c4DYwgI2 Ej++HWCEWDCfUWL+1kMsIAlOAW2JfW+PM01gFJiF5L5ZCPfNQnLfAkbmVYyiqaXJBcVJ6bmG esWJucWleel6yfm5mxghgf1lB+PiY1aHGAU4GJV4eH8Yi4QJsSaWFVfmHmKU4GBWEuFdJi0W JsSbklhZlVqUH19UmpNafIhRmoNFSZx37q73IUIC6YklqdmpqQWpRTBZJg5OqQZGdokT3xu+ XrJzyX7z/mbhX7+A+eWmAQleyWxcSfeTV92YJLJ96eRbstqKsarrTi2uz9rtyFOr2GB7KyRa UTt7WaybaKrufN9Vi3XPXihk7WHry3GcNLEyKXNCxDPvgrDVr90P/r21Y1GLgXlexv206Ss+ xpvoN15ftOHZc7kp+3acuh93V+OJEktxRqKhFnNRcSIA4sGGkWgCAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jiri, On 10/07/2015 04:14 PM, Jiří Prchal wrote: > 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. Indeed, we can't rely on led_cdev->brightness as it is being zeroed in the meantime. We would have to use another variable for storing current brightness. Probably we could use led_cdev->delayed_set_value, similarly as in case of software blinking. It would require changes in led_set_brightness(), so that it updated led_cdev->delayed_set_value not only when timer trigger is enabled, but also in case other triggers are. I'll take care of that after recent patch set with LED core improvements is applied. >> >>> 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? This is not necessary. led_trigger_remove() calls let_trigger_set(led_cdev, NULL), which in turn calls led_set_brightness(led_cdev, LED_OFF). Please remove above line and adjust commit message format so that checkpatch.pl doesn't complain. >> >>> } >>> >>> static struct led_trigger heartbeat_led_trigger = { >>> >> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-leds" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Best Regards, Jacek Anaszewski