Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752839AbbFAL4b (ORCPT ); Mon, 1 Jun 2015 07:56:31 -0400 Received: from smtp51.i.mail.ru ([94.100.177.111]:47359 "EHLO smtp51.i.mail.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752755AbbFAL4O (ORCPT ); Mon, 1 Jun 2015 07:56:14 -0400 Message-ID: <556C4851.7060900@list.ru> Date: Mon, 01 Jun 2015 14:56:01 +0300 From: Stas Sergeev User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Jacek Anaszewski CC: linux-leds@vger.kernel.org, Linux kernel , Stas Sergeev , Bryan Wu , Richard Purdie , Kyungmin Park Subject: Re: [PATCH 01/20] leds: implement LED_BRIGHTNESS_FAST flag References: <555CA58A.10700@list.ru> <555CA5FA.2080308@list.ru> <556C1855.7070903@samsung.com> In-Reply-To: <556C1855.7070903@samsung.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Spam: Not detected X-Mras: Ok Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2911 Lines: 63 01.06.2015 11:31, Jacek Anaszewski пишет: > With this approach, the LED will remain in its current blink state, in > case LED_BRIGHTNESS_FAST flag is not set and delay to be set is below LED_SLOW_MIN_PERIOD. This is because timer is deleted at the beginning > of led_blink_set. Effectively this can result in the LED staying turned > on when e.g. "echo 1 > delay_on" fails. > > I propose to change this part of code as follows: > > if (!(led_cdev->flags & LED_BRIGHTNESS_FAST)) { > if (delay_on < LED_SLOW_MIN_PERIOD || > delay_off < LED_SLOW_MIN_PERIOD) > led_set_brightness_async(led_cdev, LED_OFF); > return -ERANGE; > } > } Sounds good. > By this occasion it turned out that we have inconsistency: > led_set_brightness_async calls brightness_set op, which doesn't > set LED brightness in an asynchronous way in case of every driver. > > Before introduction of SET_BRIGHTNESS_SYNC and SET_BRIGHTNESS_ASYNC > flags there was only brightness_set op. The flags and > brightness_set_sync op was added for flash LED controllers to avoid > delegating the job to work queue and assure that brightness is set > as quickly as possible. > > I mistakenly assumed that all drivers set the brightness with use > of a work queue. Indeed. > Now, to fix the issue all drivers that set brightness in the > asynchronous way need to set the SET_BRIGHTNESS_SYNC flag and rename > their brightness_set ops to brightness_set_sync. > It would be also nice to rename brightness_set op to > brightness_set_async. > > Also, led_set_brightness_async shouldn't be called directly > anywhere, but led_set_brightness should be used, as it > selects the appropriate op basing on the SET_BRIGHTNESS_* flag. > > I'd prefer to do these modifications before merging this patch Sounds good: I wonder if the new flag for FAST drivers will then be needed at all: maybe it would be enough for the driver to define a SYNC method, and we assume the SYNC methods are always fast? In fact, the things are more complicated: some drivers do small udelay()'s but do not use a work-queue. I was not marking them as FAST, although perhaps they could still be marked as SYNC? > set. I can produce the patch set within a few days. Or, maybe you > would like to take care of it? I would appreciate if you do. I very much hate to do any non-trivial changes to the code I can't even properly test (sometimes not even compile-test!). Since you are at it, I'd also suggest to kill the work-queue in led-core.c. Now that we fixed a scenario where it was mistakenly used, I again think it is not needed for what it was initially advertised. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/