From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753552AbcFALh0 (ORCPT ); Wed, 1 Jun 2016 07:37:26 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:40225 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750697AbcFALhX (ORCPT ); Wed, 1 Jun 2016 07:37:23 -0400 X-AuditID: cbfec7f4-f796c6d000001486-72-574ec8efa831 Message-id: <574EC8EE.5000509@samsung.com> Date: Wed, 01 Jun 2016 13:37:18 +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: Krzysztof Kozlowski Cc: Kukjin Kim , MyungJoo Ham , Chanwoo Choi , Richard Purdie , linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org, linux-pm@vger.kernel.org, r.baldyga@hackerion.com, Bartlomiej Zolnierkiewicz Subject: Re: [PATCH v7 1/6] mfd: max8997: Use regmap to access registers References: <1464773339-756-1-git-send-email-k.kozlowski@samsung.com> <1464774841-1439-1-git-send-email-k.kozlowski@samsung.com> In-reply-to: <1464774841-1439-1-git-send-email-k.kozlowski@samsung.com> Content-type: text/plain; charset=ISO-8859-1; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrJLMWRmVeSWpSXmKPExsVy+t/xq7rvT/iFG8xp1LXYOGM9q8X1L89Z LV6/MLTof/ya2WLT42usFpd3zWGz2PpmHaPF594jjBYzzu9jsrjduILN4u6/T4wWu3c9ZXXg 8Tiy8xibx6ZVnWwem5fUe+yZ/4PVo2/LKkaPz5vkAtiiuGxSUnMyy1KL9O0SuDK2r1jLXPBe pmLxhiWMDYwd4l2MHBwSAiYSd2dwdzFyApliEhfurWfrYuTiEBJYyiix8sAnKOcZo8TJSU1s IFW8AloSnzdvZQSxWQRUJdbs72AHsdkEDCV+vnjNBGKLCkRI/Dm9jxWiXlDix+R7LCC2CFDN wd3bmUCGMgt0MUt0LbvBDJIQFvCQ6Gu/zwixrYlRYsrSY2AbOAXcJe7PuQO2mVnAWmLlpG2M ELa8xOY1b5knMArMQrJkFpKyWUjKFjAyr2IUTS1NLihOSs811CtOzC0uzUvXS87P3cQIiZAv OxgXH7M6xCjAwajEw6tw0TdciDWxrLgy9xCjBAezkgjvk2N+4UK8KYmVValF+fFFpTmpxYcY pTlYlMR55+56HyIkkJ5YkpqdmlqQWgSTZeLglGpg9Gc71pae8n7Dwt+S2gZ6dRP+PJokMCn0 hM/dZS+f+kpvSg+/9P1a0DTfzX6FtzgP8vgtqNdU2PeSeZ6c+9N1/IYcJmy8sqt/+MwvKErJ X/RucpOIU4X6cU/mwgKGR4wRK6rnBuj3W2yfemtNq/pPj9g9U+xMJsa/jTwm+rHj5v28hZpv tKdxK7EUZyQaajEXFScCAIlO39GMAgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Krzysztof, One thing drew my attention while reviewing this again: max8997_led_brightness_set() can sleep, but the brightness_set op it is assigned to must not sleep. At the time when this driver was merged we were delegating brightness setting to workqueues task in LED class drivers that can sleep during this call. This must have been overlooked, which is even more likely, taking into account that the initial patch doesn't have LED maintainer's ack. The non-sleeping requirement is motivated by the fact that brightness can be set from softirq context, e.g. when timer trigger is enabled. Currently LED class drivers don't have to use workqueue on their own, but are required to use brightness_set_blocking op instead of brightness_set if they can sleep while setting brightness. Apart of that, I think that operations in max8997_led_brightness_set() should be protected with mutex to assure leaving the device in a consistent state in case of concurrent calls. I am aware that this is out of this patch scope, but I'd be grateful if you could apply those changes and test them on hardware if you have an access to. Thanks, Jacek Anaszewski On 06/01/2016 11:53 AM, Krzysztof Kozlowski wrote: > From: Robert Baldyga > diff --git a/drivers/leds/leds-max8997.c b/drivers/leds/leds-max8997.c > index 4edf74f1d6d4..e5f0dc2e9edf 100644 > --- a/drivers/leds/leds-max8997.c > +++ b/drivers/leds/leds-max8997.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > > #define MAX8997_LED_FLASH_SHIFT 3 > #define MAX8997_LED_FLASH_CUR_MASK 0xf8 > @@ -52,7 +53,6 @@ static void max8997_led_set_mode(struct max8997_led *led, > enum max8997_led_mode mode) > { > int ret; > - struct i2c_client *client = led->iodev->i2c; > u8 mask = 0, val; > > switch (mode) { > @@ -88,8 +88,8 @@ static void max8997_led_set_mode(struct max8997_led *led, > } > > if (mask) { > - ret = max8997_update_reg(client, MAX8997_REG_LEN_CNTL, val, > - mask); > + ret = regmap_update_bits(led->iodev->regmap, > + MAX8997_REG_LEN_CNTL, mask, val); > if (ret) > dev_err(led->iodev->dev, > "failed to update register(%d)\n", ret); > @@ -101,7 +101,6 @@ static void max8997_led_set_mode(struct max8997_led *led, > static void max8997_led_enable(struct max8997_led *led, bool enable) > { > int ret; > - struct i2c_client *client = led->iodev->i2c; > u8 val = 0, mask = MAX8997_LED_BOOST_ENABLE_MASK; > > if (led->enabled == enable) > @@ -109,7 +108,8 @@ static void max8997_led_enable(struct max8997_led *led, bool enable) > > val = enable ? MAX8997_LED_BOOST_ENABLE_MASK : 0; > > - ret = max8997_update_reg(client, MAX8997_REG_BOOST_CNTL, val, mask); > + ret = regmap_update_bits(led->iodev->regmap, > + MAX8997_REG_BOOST_CNTL, mask, val); > if (ret) > dev_err(led->iodev->dev, > "failed to update register(%d)\n", ret); > @@ -121,7 +121,6 @@ static void max8997_led_set_current(struct max8997_led *led, > enum led_brightness value) > { > int ret; > - struct i2c_client *client = led->iodev->i2c; > u8 val = 0, mask = 0, reg = 0; > > switch (led->led_mode) { > @@ -142,7 +141,7 @@ static void max8997_led_set_current(struct max8997_led *led, > } > > if (mask) { > - ret = max8997_update_reg(client, reg, val, mask); > + ret = regmap_update_bits(led->iodev->regmap, reg, mask, val); > if (ret) > dev_err(led->iodev->dev, > "failed to update register(%d)\n", ret);