Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1411020 > unrolled thread

[PATCH v7 0/6] mfd: (multiple trees): max8997: Add regmap

Back to article view | Back to linux.kernel

Contents

  [PATCH v7 0/6] mfd: (multiple trees): max8997: Add regmap Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-06-01 11:30 +0200
    Re: [PATCH v7 1/6] mfd: max8997: Use regmap to access registers Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-06-01 13:40 +0200
      Re: [PATCH v7 1/6] mfd: max8997: Use regmap to access registers Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-06-02 06:50 +0200
        Re: [PATCH v7 1/6] mfd: max8997: Use regmap to access registers Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-06-02 09:10 +0200
    Re: [PATCH v7 1/6] mfd: max8997: Use regmap to access registers Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-06-08 16:40 +0200

#1411020 — [PATCH v7 0/6] mfd: (multiple trees): max8997: Add regmap

Hi,


I'm pushing old Robert's work. I miss only ack from regulator subsystem.
Mark, could you take a look at patch 1/6?

Changes since v6
================
1. Add some acks.
2. Remove patches already applied.


Best regards,
Krzysztof


Krzysztof Kozlowski (3):
  extcon: max8997: Fix handling error code of regmap_irq_get_virq()
  rtc: max8997: Check for ERRNO of regmap_irq_get_virq()
  extcon: max8997: Fix inconsistent indenting

Robert Baldyga (3):
  mfd: max8997: Use regmap to access registers
  mfd: max8997: handle IRQs using regmap
  mfd: max8997: Change irq names to upper case

 drivers/extcon/extcon-max8997.c       |  72 +++----
 drivers/input/misc/max8997_haptic.c   |  34 +--
 drivers/leds/leds-max8997.c           |  13 +-
 drivers/mfd/Kconfig                   |   3 +-
 drivers/mfd/Makefile                  |   2 +-
 drivers/mfd/max8997-irq.c             | 383 ----------------------------------
 drivers/mfd/max8997.c                 | 245 +++++++++++++++-------
 drivers/power/max8997_charger.c       |  33 +--
 drivers/regulator/max8997-regulator.c |  87 ++++----
 drivers/rtc/rtc-max8997.c             |  60 +++---
 include/linux/mfd/max8997-private.h   |  82 ++++++--
 11 files changed, 385 insertions(+), 629 deletions(-)
 delete mode 100644 drivers/mfd/max8997-irq.c

-- 
1.9.1

[toc] | [next] | [standalone]

#1411118 — Re: [PATCH v7 1/6] mfd: max8997: Use regmap to access registers

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 <r.baldyga@samsung.com>

> 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 <linux/mfd/max8997.h>
>   #include <linux/mfd/max8997-private.h>
>   #include <linux/platform_device.h>
> +#include <linux/regmap.h>
>
>   #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);

[toc] | [prev] | [next] | [standalone]

#1411819 — Re: [PATCH v7 1/6] mfd: max8997: Use regmap to access registers

On 06/01/2016 01:37 PM, Jacek Anaszewski wrote:
> 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.

The problem you mention existed before the patch. It was using sleeping
primitives (mutex) before adding regmap so I understand you don't have
anything against this patch, right?

I can fix the issue but it will be a little bit trickier because I don't
have the hardware. Other guys in the team tested the patchset for me so
I rely on them in that matter. Anyway I'll work on it.

Best regards,
Krzysztof

[toc] | [prev] | [next] | [standalone]

#1411928 — Re: [PATCH v7 1/6] mfd: max8997: Use regmap to access registers

On 06/02/2016 06:43 AM, Krzysztof Kozlowski wrote:
> On 06/01/2016 01:37 PM, Jacek Anaszewski wrote:
>> 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.
>
> The problem you mention existed before the patch. It was using sleeping
> primitives (mutex) before adding regmap so I understand you don't have
> anything against this patch, right?

Right, I just wanted to indicate the problem. The patch itself is ok.

> I can fix the issue but it will be a little bit trickier because I don't
> have the hardware. Other guys in the team tested the patchset for me so
> I rely on them in that matter. Anyway I'll work on it.

Ack.

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1417534 — Re: [PATCH v7 1/6] mfd: max8997: Use regmap to access registers

On 06/08/2016 04:26 PM, Lee Jones wrote:
> On Wed, 01 Jun 2016, Krzysztof Kozlowski wrote:
> 
>> From: Robert Baldyga <r.baldyga@samsung.com>
>>
>> This patch modifies max8997 driver and each associated function driver,
>> to use regmap instead of operating directly on i2c bus. It will allow to
>> simplify IRQ handling using regmap-irq.
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>
>> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Acked-by: Bryan Wu <cooloney@gmail.com>
>> Acked-by: Lee Jones <lee.jones@linaro.org>
>> Acked-by: Sebastian Reichel <sre@kernel.org>
>> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>> [k.kozlowski: Collect acks, rebase on v4.6-rc6]
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>> ---
>>  drivers/extcon/extcon-max8997.c       |  31 ++++----
>>  drivers/input/misc/max8997_haptic.c   |  34 ++++----
>>  drivers/leds/leds-max8997.c           |  13 ++--
>>  drivers/mfd/Kconfig                   |   1 +
>>  drivers/mfd/max8997-irq.c             |  64 ++++++---------
>>  drivers/mfd/max8997.c                 | 141 +++++++++++++++-------------------
>>  drivers/power/max8997_charger.c       |  33 ++++----
>>  drivers/regulator/max8997-regulator.c |  87 ++++++++++-----------
> 
> Is it just Mark's Ack you require?

Yes, only Mark's is missing here. All other patches got everything they
need.

Best regards,
Krzysztof

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web