From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757275AbbIVHJv (ORCPT ); Tue, 22 Sep 2015 03:09:51 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:9069 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756509AbbIVHJt (ORCPT ); Tue, 22 Sep 2015 03:09:49 -0400 X-AuditID: cbfec7f5-f794b6d000001495-51-5600feba5a2e Message-id: <5600FEB9.10409@samsung.com> Date: Tue, 22 Sep 2015 09:09:45 +0200 From: Jacek Anaszewski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804 Thunderbird/17.0.8 MIME-version: 1.0 To: Sakari Ailus Cc: linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org, andrew@lunn.ch Subject: Re: [PATCH 1/5] leds: core: Drivers shouldn't enforce SYNC/ASYNC brightness setting References: <1442845770-17800-1-git-send-email-j.anaszewski@samsung.com> <1442845770-17800-2-git-send-email-j.anaszewski@samsung.com> <5600F83C.4070302@linux.intel.com> In-reply-to: <5600F83C.4070302@linux.intel.com> Content-type: text/plain; charset=ISO-8859-1; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrALMWRmVeSWpSXmKPExsVy+t/xK7q7/jGEGdxol7A4f/cQs8XlXXPY LLa+Wcdo8WnLNyYHFo95JwM9du74zOTxeZNcAHMUl01Kak5mWWqRvl0CV8bLWd8ZCxrMKrY0 32JpYNyr3cXIwSEhYCKxps+0i5ETyBSTuHBvPVsXIxeHkMBSRolZa9ezgiSEBJ4xSkzYrg5i 8wpoSEzed4sJxGYRUJU4drWTEcRmEzCU+PniNVhcVCBC4s/pfawQ9YISPybfYwHZJSKgLzHp gRlImFnAR2LJsUdgJcICcRIXV+9khti7hlHi0EEQh5ODE6h+1aS5bBAN1hIrJ21jhLDlJTav ecs8gVFgFpIVs5CUzUJStoCReRWjaGppckFxUnqukV5xYm5xaV66XnJ+7iZGSLh+3cG49JjV IUYBDkYlHl6PPoYwIdbEsuLK3EOMEhzMSiK8ng+BQrwpiZVVqUX58UWlOanFhxilOViUxHln 7nofIiSQnliSmp2aWpBaBJNl4uCUamC04QuV3Ck5eddursp3XJ/KDk2N6/37vkiSjZXPyW6F cMfpHdUeORdktzDtvXssOTX8+IyDLV4ikrUPDjp3OawT+je3wF02ziFb7eh9ScajOvOjHRhL dz4WSFbl81jr2PUwvfpLopR3Z/LGSd9dNPea3L/2wPx/gcvJpYa+mkv/xZdZyPrKaiuxFGck GmoxFxUnAgBYsqnyUwIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Sakari, Thanks for the review. Just to avoid a confusion - this patch set depends on [1], which is not merged yet and also needs a review. I reimplemented and split [2] into two patch sets, so that [1] contained only modifications required for removing work queues from LED class drivers, but yesterday decided to send also the remaining part. [1] https://lkml.org/lkml/2015/9/16/299 [2] https://lkml.org/lkml/2015/8/20/426 On 09/22/2015 08:42 AM, Sakari Ailus wrote: > Hi Jacek, > > Thanks for the patchset. A few comments below. > > Jacek Anaszewski wrote: >> This patch removes SET_BRIGHTNESS_ASYNC and SET_BRIGHTNESS flags. >> led_set_brightness now calls led_set_brightness_nosleep instead of >> choosing between sync and async op basing on the flags defined by the >> driver. >> >> From now on, if a user wants to make sure that brightness will be set >> synchronously, they have to use led_set_brightness_sync API. It is now >> being made publicly available since it has become apparent that it is >> a caller who should decide whether brightness is to be set in >> a synchronous or an asynchronous way. >> >> Signed-off-by: Jacek Anaszewski >> --- >> drivers/leds/led-class-flash.c | 4 ---- >> drivers/leds/led-class.c | 2 -- >> drivers/leds/led-core.c | 33 +++++++++++++++++++-------------- >> drivers/leds/leds.h | 13 ------------- >> include/linux/leds.h | 19 ++++++++++++++++--- >> 5 files changed, 35 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c >> index 300a2c9..f53783b 100644 >> --- a/drivers/leds/led-class-flash.c >> +++ b/drivers/leds/led-class-flash.c >> @@ -316,10 +316,6 @@ int led_classdev_flash_register(struct device *parent, >> if (ret < 0) >> return ret; >> >> - /* Setting a torch brightness needs to have immediate effect */ >> - led_cdev->flags &= ~SET_BRIGHTNESS_ASYNC; >> - led_cdev->flags |= SET_BRIGHTNESS_SYNC; >> - >> return 0; >> } >> EXPORT_SYMBOL_GPL(led_classdev_flash_register); >> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c >> index 7385f98..37eec1a 100644 >> --- a/drivers/leds/led-class.c >> +++ b/drivers/leds/led-class.c >> @@ -215,8 +215,6 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) >> if (!led_cdev->max_brightness) >> led_cdev->max_brightness = LED_FULL; >> >> - led_cdev->flags |= SET_BRIGHTNESS_ASYNC; >> - >> led_update_brightness(led_cdev); >> >> led_init_core(led_cdev); >> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >> index d8649f1..d0eb838 100644 >> --- a/drivers/leds/led-core.c >> +++ b/drivers/leds/led-core.c >> @@ -215,8 +215,6 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink); >> void led_set_brightness(struct led_classdev *led_cdev, >> enum led_brightness brightness) >> { >> - int ret = 0; >> - >> /* >> * In case blinking is on delay brightness setting >> * until the next timer tick. >> @@ -235,19 +233,9 @@ void led_set_brightness(struct led_classdev *led_cdev, >> led_cdev->blink_brightness = brightness; >> } >> return; >> - } >> - >> - if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) { >> + } else { > > Useless use of else. Right. >> led_set_brightness_nosleep(led_cdev, brightness); >> - return; >> - } else if (led_cdev->flags & SET_BRIGHTNESS_SYNC) >> - ret = led_set_brightness_sync(led_cdev, brightness); >> - else >> - ret = -EINVAL; >> - >> - if (ret < 0) >> - dev_dbg(led_cdev->dev, "Setting LED brightness failed (%d)\n", >> - ret); >> + } >> } >> EXPORT_SYMBOL(led_set_brightness); >> >> @@ -269,6 +257,23 @@ void led_set_brightness_nosleep(struct led_classdev *led_cdev, >> } >> EXPORT_SYMBOL(led_set_brightness_nosleep); >> >> +int led_set_brightness_sync(struct led_classdev *led_cdev, >> + enum led_brightness value) >> +{ >> + WARN_ON(led_cdev->blink_delay_on || led_cdev->blink_delay_off); > > How about simply returning an error instead? Indeed, it will be more relevant here. >> + >> + led_cdev->brightness = min(value, led_cdev->max_brightness); >> + >> + if (led_cdev->flags & LED_SUSPENDED) >> + return 0; >> + >> + if (led_cdev->brightness_set_blocking) >> + return led_cdev->brightness_set_blocking(led_cdev, >> + led_cdev->brightness); >> + return -EINVAL; >> +} >> +EXPORT_SYMBOL(led_set_brightness_sync); >> + >> int led_update_brightness(struct led_classdev *led_cdev) >> { >> int ret = 0; >> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h >> index 04b8e41..62d9eba 100644 >> --- a/drivers/leds/leds.h >> +++ b/drivers/leds/leds.h >> @@ -16,19 +16,6 @@ >> #include >> #include >> >> -static inline int led_set_brightness_sync(struct led_classdev *led_cdev, >> - enum led_brightness value) >> -{ >> - int ret = 0; >> - >> - led_cdev->brightness = min(value, led_cdev->max_brightness); >> - >> - if (!(led_cdev->flags & LED_SUSPENDED)) >> - ret = led_cdev->brightness_set_blocking(led_cdev, >> - led_cdev->brightness); >> - return ret; >> -} >> - >> static inline int led_get_brightness(struct led_classdev *led_cdev) >> { >> return led_cdev->brightness; >> diff --git a/include/linux/leds.h b/include/linux/leds.h >> index ae3c178..2019929 100644 >> --- a/include/linux/leds.h >> +++ b/include/linux/leds.h >> @@ -47,9 +47,7 @@ struct led_classdev { >> #define LED_BLINK_CHANGE (1 << 20) >> #define LED_BLINK_DISABLE (1 << 21) >> #define LED_SYSFS_DISABLE (1 << 22) >> -#define SET_BRIGHTNESS_ASYNC (1 << 23) >> -#define SET_BRIGHTNESS_SYNC (1 << 24) >> -#define LED_DEV_CAP_FLASH (1 << 25) >> +#define LED_DEV_CAP_FLASH (1 << 23) >> >> /* Set LED brightness level */ >> /* Must not sleep, use a workqueue if needed */ >> @@ -159,6 +157,21 @@ extern void led_blink_set_oneshot(struct led_classdev *led_cdev, >> */ >> extern void led_set_brightness(struct led_classdev *led_cdev, >> enum led_brightness brightness); >> + >> +/** >> + * led_set_brightness_sync - set LED brightness synchronously >> + * @led_cdev: the LED to set >> + * @brightness: the brightness to set it to >> + * >> + * Set an LED's brightness immediately. This function will block >> + * the caller for the time required for accessing device register, >> + * and it can sleep. >> + * >> + * Returns: 0 on success or negative error value on failure >> + */ >> +extern int led_set_brightness_sync(struct led_classdev *led_cdev, >> + enum led_brightness value); >> + >> /** >> * led_update_brightness - update LED brightness >> * @led_cdev: the LED to query >> > -- Best Regards, Jacek Anaszewski