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

Groups > linux.kernel > #1314499 > unrolled thread

[PATCH v2 00/10] rtc: max77686: Extend driver and add max77802 support

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 00/10] rtc: max77686: Extend driver and add max77802 support Javier Martinez Canillas <javier@osg.samsung.com> - 2016-01-21 21:30 +0100
    [PATCH v2 01/10] rtc: max77686: Fix max77686_rtc_read_alarm() return value Javier Martinez Canillas <javier@osg.samsung.com> - 2016-01-21 21:30 +0100
      Re: [PATCH v2 01/10] rtc: max77686: Fix max77686_rtc_read_alarm()  return value Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-01-22 02:00 +0100
      Re: [PATCH v2 01/10] rtc: max77686: Fix max77686_rtc_read_alarm()  return value Laxman Dewangan <ldewangan@nvidia.com> - 2016-01-22 10:50 +0100
    [PATCH v2 02/10] rtc: max77686: Use ARRAY_SIZE() instead of current array length Javier Martinez Canillas <javier@osg.samsung.com> - 2016-01-21 21:30 +0100
      Re: [PATCH v2 02/10] rtc: max77686: Use ARRAY_SIZE() instead of  current array length Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-01-22 02:00 +0100
      Re: [PATCH v2 02/10] rtc: max77686: Use ARRAY_SIZE() instead of current  array length Laxman Dewangan <ldewangan@nvidia.com> - 2016-01-22 11:00 +0100
        Re: [PATCH v2 02/10] rtc: max77686: Use ARRAY_SIZE() instead of  current array length Javier Martinez Canillas <javier@osg.samsung.com> - 2016-01-22 12:50 +0100
    [PATCH v2 07/10] rtc: max77686: Use dev_warn() instead of pr_warn() Javier Martinez Canillas <javier@osg.samsung.com> - 2016-01-21 21:30 +0100
    [PATCH v2 03/10] rtc: max77686: Use usleep_range() instead of msleep() Javier Martinez Canillas <javier@osg.samsung.com> - 2016-01-21 21:30 +0100
      Re: [PATCH v2 03/10] rtc: max77686: Use usleep_range() instead of  msleep() Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-01-22 02:10 +0100
      Re: [PATCH v2 03/10] rtc: max77686: Use usleep_range() instead of  msleep() Laxman Dewangan <ldewangan@nvidia.com> - 2016-01-22 11:00 +0100
        Re: [PATCH v2 03/10] rtc: max77686: Use usleep_range() instead of  msleep() Javier Martinez Canillas <javier@osg.samsung.com> - 2016-01-22 13:10 +0100
          Re: [PATCH v2 03/10] rtc: max77686: Use usleep_range() instead of  msleep() Laxman Dewangan <ldewangan@nvidia.com> - 2016-01-25 12:30 +0100
    [PATCH v2 10/10] ARM: multi_v7_defconfig: Remove MAX77802 RTC Kconfig symbol Javier Martinez Canillas <javier@osg.samsung.com> - 2016-01-21 21:30 +0100
      Re: [PATCH v2 10/10] ARM: multi_v7_defconfig: Remove MAX77802 RTC  Kconfig symbol Laxman Dewangan <ldewangan@nvidia.com> - 2016-01-22 11:10 +0100
        Re: [PATCH v2 10/10] ARM: multi_v7_defconfig: Remove MAX77802 RTC  Kconfig symbol Javier Martinez Canillas <javier@osg.samsung.com> - 2016-01-22 13:10 +0100
    [PATCH v2 09/10] ARM: exynos_defconfig: Remove MAX77802 RTC Kconfig symbol Javier Martinez Canillas <javier@osg.samsung.com> - 2016-01-21 21:30 +0100
    Re: [PATCH v2 00/10] rtc: max77686: Extend driver and add max77802  support Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-01-25 17:30 +0100
      Re: [PATCH v2 00/10] rtc: max77686: Extend driver and add max77802  support Javier Martinez Canillas <javier@osg.samsung.com> - 2016-01-26 00:50 +0100

#1314499 — [PATCH v2 00/10] rtc: max77686: Extend driver and add max77802 support

Hello,

On a recent disussion [0] with Krzysztof Kozlowski and Laxman Dewangan,
we came to the conclusion that the max77686 and max77802 RTC are almost
the same with only a few differences so there shouldn't be two separate
drivers and is better to extend max77686 driver and delete rtc-max77802.

By making the driver more generic, other RTC IP blocks from Maxim PMICs
could be supported as well like the max77620.

This is a v2 of a series that do this, that address issues pointed out
by Krzysztof Kozlowski. The v1 can be found at [1].

I've tested this patch-set on an Exynos5800 Peach Pi Chromebook that has
a max77802 PMIC and the RTC was working correctly but I don't have a
machine with max77686 so I will really appreaciate if someone can test
that no regressions were introduced.

On an IRC conversation, Alexandre suggested to use the field support in
the regmap API to avoid needing a translation table. I spent some time
to look at it and I'm not so sure if it fits that well in this case.

It's true that we could model each register as if it has a single field
and provide a different reg address but I'm not sure if that would make
things more clear or cause more confusion for future code archaeologists.

In any case, I think this series are a move in the right direction since
removes code duplication and a complete driver and also allows others to
reuse the driver for another RTC chip. We can later simplify and use the
regmap field API or extend the regmap core if that could make things even
simpler but I propose to do it as a follow up.

[0]: http://www.spinics.net/lists/devicetree/msg110348.html
[1]: https://lwn.net/Articles/672568/

Changes in v2:
- Add Krzysztof Kozlowski's Reviewed-by tag to patch #2.
- Add Krzysztof Kozlowski's Reviewed-by tag to patch #3.
- Fix typo error in changelog. Suggested by Krzysztof Kozlowski.
- Add a max77686 prefix to rtc_driver_data. Suggested by Krzysztof Kozlowski.
- Comment about the .delay and .mask fields. Suggested by Krzysztof Kozlowski.
- Change .mask type to u8. Suggested by Krzysztof Kozlowski.
- Make .drv_data field const. Suggested by Krzysztof Kozlowski.
- Don't cast to drop const on .drv_data asign. Suggested by Krzysztof Kozlowski.
- Use platform_get_device_id() macro. Suggested by Krzysztof Kozlowski.
- Rename rtc_reg to max77686_rtc_reg_offset. Suggested by Krzysztof Kozlowski.
- Comment what's mapped by max77686_map. Suggested by Krzysztof Kozlowski.
- Use max77686_map array indexes in init. Suggested by Krzysztof Kozlowski.
- Remove REG_RTC_UPDATE1 since is not used by neither max77686 nor max77802.
- Add a MAX77802 prefix to ALARM_ENABLE_VALUE. Suggested by Krzysztof Kozlowski.
- Rename .rtcae to .alarm_enable_reg and .rtcrm to .separate_i2c_addr.
  Suggested by Krzysztof Kozlowski.
- Don't use func and LINE in error messages. Suggested by Krzysztof Kozlowski.
- Remove REG_RTC_AE2 since is not used by neither max77686 nor max77802.
- Check if REG_RTC_AE1 has a valid address before accessing it.
- Add Krzysztof Kozlowski's Reviewed-by tag to patch #8.
- Add Krzysztof Kozlowski's Reviewed-by tag to patch #9.
- Add Krzysztof Kozlowski's Reviewed-by tag to patch #10.

Javier Martinez Canillas (10):
  rtc: max77686: Fix max77686_rtc_read_alarm() return value
  rtc: max77686: Use ARRAY_SIZE() instead of current array length
  rtc: max77686: Use usleep_range() instead of msleep()
  rtc: max77686: Use a driver data struct instead hard-coded values
  rtc: max77686: Add an indirection level to access RTC registers
  rtc: max77686: Add max77802 support
  rtc: max77686: Use dev_warn() instead of pr_warn()
  rtc: Remove Maxim 77802 driver
  ARM: exynos_defconfig: Remove MAX77802 RTC Kconfig symbol
  ARM: multi_v7_defconfig: Remove MAX77802 RTC Kconfig symbol

 arch/arm/configs/exynos_defconfig   |   1 -
 arch/arm/configs/multi_v7_defconfig |   1 -
 drivers/rtc/Kconfig                 |  10 -
 drivers/rtc/Makefile                |   1 -
 drivers/rtc/rtc-max77686.c          | 332 +++++++++++++++++++-----
 drivers/rtc/rtc-max77802.c          | 502 ------------------------------------
 6 files changed, 266 insertions(+), 581 deletions(-)
 delete mode 100644 drivers/rtc/rtc-max77802.c

-- 
2.5.0

[toc] | [next] | [standalone]

#1314501 — [PATCH v2 01/10] rtc: max77686: Fix max77686_rtc_read_alarm() return value

The function is always returning zero even in case of failures since
the ret value was not propagated to the callers. Fix the error path.

Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

Changes in v2: None

 drivers/rtc/rtc-max77686.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 7184a0eda793..6653c3d11b66 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -235,7 +235,7 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 
 out:
 	mutex_unlock(&info->lock);
-	return 0;
+	return ret;
 }
 
 static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
-- 
2.5.0

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

#1314674 — Re: [PATCH v2 01/10] rtc: max77686: Fix max77686_rtc_read_alarm() return value

On 22.01.2016 05:23, Javier Martinez Canillas wrote:
> The function is always returning zero even in case of failures since
> the ret value was not propagated to the callers. Fix the error path.
> 
> Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
> Changes in v2: None
> 
>  drivers/rtc/rtc-max77686.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index 7184a0eda793..6653c3d11b66 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -235,7 +235,7 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  
>  out:
>  	mutex_unlock(&info->lock);
> -	return 0;
> +	return ret;
>  }
>  
>  static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Tested on Trats2 board with max77686:
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

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

#1314850 — Re: [PATCH v2 01/10] rtc: max77686: Fix max77686_rtc_read_alarm() return value

On Friday 22 January 2016 01:53 AM, Javier Martinez Canillas wrote:
> The function is always returning zero even in case of failures since
> the ret value was not propagated to the callers. Fix the error path.
>
> Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>

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

#1314502 — [PATCH v2 02/10] rtc: max77686: Use ARRAY_SIZE() instead of current array length

It is better to use the ARRAY_SIZE() macro instead of the array length
to avoid bugs if the array is later changed and the length not updated.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

---

Changes in v2:
- Add Krzysztof Kozlowski's Reviewed-by tag to patch #2.

 drivers/rtc/rtc-max77686.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 6653c3d11b66..98fabdb308b9 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -406,7 +406,8 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
 
 	info->rtc_24hr_mode = 1;
 
-	ret = regmap_bulk_write(info->max77686->rtc_regmap, MAX77686_RTC_CONTROLM, data, 2);
+	ret = regmap_bulk_write(info->max77686->rtc_regmap,
+				MAX77686_RTC_CONTROLM, data, ARRAY_SIZE(data));
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to write controlm reg(%d)\n",
 				__func__, ret);
-- 
2.5.0

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

#1314675 — Re: [PATCH v2 02/10] rtc: max77686: Use ARRAY_SIZE() instead of current array length

On 22.01.2016 05:23, Javier Martinez Canillas wrote:
> It is better to use the ARRAY_SIZE() macro instead of the array length
> to avoid bugs if the array is later changed and the length not updated.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 
> ---

Tested on Trats2 board with max77686:
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

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

#1314859 — Re: [PATCH v2 02/10] rtc: max77686: Use ARRAY_SIZE() instead of current array length

On Friday 22 January 2016 01:53 AM, Javier Martinez Canillas wrote:
> It is better to use the ARRAY_SIZE() macro instead of the array length
> to avoid bugs if the array is later changed and the length not updated.
>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>
>
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>


Similar stuffs are there on multiple places:
u8 data[RTC_NR_TIME];

:::
ret = regmap_bulk_read(info->max77686->rtc_regmap,
                                  MAX77686_ALARM1_SEC, data, RTC_NR_TIME);


Should we say:
ret = regmap_bulk_read(info->max77686->rtc_regmap,
                                  MAX77686_ALARM1_SEC, data, 
ARRAY_SIZE(data));

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

#1314923 — Re: [PATCH v2 02/10] rtc: max77686: Use ARRAY_SIZE() instead of current array length

Hello Laxman,

Thanks a lot for your feedback and acks.

On 01/22/2016 06:39 AM, Laxman Dewangan wrote:
>
> On Friday 22 January 2016 01:53 AM, Javier Martinez Canillas wrote:
>> It is better to use the ARRAY_SIZE() macro instead of the array length
>> to avoid bugs if the array is later changed and the length not updated.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>
>>
> Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
>
>
> Similar stuffs are there on multiple places:
> u8 data[RTC_NR_TIME];
>
> :::
> ret = regmap_bulk_read(info->max77686->rtc_regmap,
>                                   MAX77686_ALARM1_SEC, data, RTC_NR_TIME);
>
>
> Should we say:
> ret = regmap_bulk_read(info->max77686->rtc_regmap,
>                                   MAX77686_ALARM1_SEC, data, ARRAY_SIZE(data));
>

Very good point, I'll change those on this patch as well for v3.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

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

#1314504 — [PATCH v2 07/10] rtc: max77686: Use dev_warn() instead of pr_warn()

It is better to use dev_*() log functions instead of pr_*() to print
information about the device in the kernel log in a standardized way.

This also allows to remove the local pr_fmt() defined macro.

Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

Changes in v2: None

 drivers/rtc/rtc-max77686.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 3e6d6bff5154..e1739d19ce9a 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -12,8 +12,6 @@
  *
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/slab.h>
 #include <linux/rtc.h>
 #include <linux/delay.h>
@@ -245,8 +243,9 @@ static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data,
 		data[RTC_YEAR] = tm->tm_year > 100 ? (tm->tm_year - 100) : 0;
 
 		if (tm->tm_year < 100) {
-			pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
-				1900 + tm->tm_year);
+			dev_warn(info->dev,
+				 "RTC can't handle year %d. Assume it's 2000\n",
+				 1900 + tm->tm_year);
 			return -EINVAL;
 		}
 	} else {
-- 
2.5.0

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

#1314505 — [PATCH v2 03/10] rtc: max77686: Use usleep_range() instead of msleep()

Documentation/timers/timers-howto.txt suggest to use usleep_range()
instead of msleep() for small msec (1ms - 20ms) since msleep() will
often sleep for 20ms for any value in that range.

This is fine in this case since 16ms is the _minimum_ delay required
by max77686 for an RTC update but by using usleep_range() instead of
msleep(), the driver can support other RTC IP blocks with a shorter
minimum delay (i.e: in the range of usecs instead of msecs).

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

---

Changes in v2:
- Add Krzysztof Kozlowski's Reviewed-by tag to patch #3.
- Fix typo error in changelog. Suggested by Krzysztof Kozlowski.

 drivers/rtc/rtc-max77686.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 98fabdb308b9..71ef2240b3fc 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -41,7 +41,7 @@
 #define ALARM_ENABLE_SHIFT		7
 #define ALARM_ENABLE_MASK		(1 << ALARM_ENABLE_SHIFT)
 
-#define MAX77686_RTC_UPDATE_DELAY	16
+#define MAX77686_RTC_UPDATE_DELAY	16000
 
 enum {
 	RTC_SEC = 0,
@@ -130,7 +130,8 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
 				__func__, ret, data);
 	else {
 		/* Minimum 16ms delay required before RTC update. */
-		msleep(MAX77686_RTC_UPDATE_DELAY);
+		usleep_range(MAX77686_RTC_UPDATE_DELAY,
+			     MAX77686_RTC_UPDATE_DELAY * 2);
 	}
 
 	return ret;
-- 
2.5.0

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

#1314678 — Re: [PATCH v2 03/10] rtc: max77686: Use usleep_range() instead of msleep()

On 22.01.2016 05:23, Javier Martinez Canillas wrote:
> Documentation/timers/timers-howto.txt suggest to use usleep_range()
> instead of msleep() for small msec (1ms - 20ms) since msleep() will
> often sleep for 20ms for any value in that range.
> 
> This is fine in this case since 16ms is the _minimum_ delay required
> by max77686 for an RTC update but by using usleep_range() instead of
> msleep(), the driver can support other RTC IP blocks with a shorter
> minimum delay (i.e: in the range of usecs instead of msecs).
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 
> ---
> 
> Changes in v2:
> - Add Krzysztof Kozlowski's Reviewed-by tag to patch #3.
> - Fix typo error in changelog. Suggested by Krzysztof Kozlowski.
> 
>  drivers/rtc/rtc-max77686.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 

Tested on Trats2 board with max77686:
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

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

#1314858 — Re: [PATCH v2 03/10] rtc: max77686: Use usleep_range() instead of msleep()

On Friday 22 January 2016 01:53 AM, Javier Martinez Canillas wrote:
>   	RTC_SEC = 0,
> @@ -130,7 +130,8 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
>   				__func__, ret, data);
>   	else {
>   		/* Minimum 16ms delay required before RTC update. */
> -		msleep(MAX77686_RTC_UPDATE_DELAY);
> +		usleep_range(MAX77686_RTC_UPDATE_DELAY,
> +			     MAX77686_RTC_UPDATE_DELAY * 2);
>   	}
>

Instead of making usleep_range(16000, 32000), can we make small range as
usleep_range(16000, 17000)?

I am using as usleep_range(16000, 16000) always.

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

#1314934 — Re: [PATCH v2 03/10] rtc: max77686: Use usleep_range() instead of msleep()

Hello Laxman,

On 01/22/2016 06:41 AM, Laxman Dewangan wrote:
>
> On Friday 22 January 2016 01:53 AM, Javier Martinez Canillas wrote:
>>       RTC_SEC = 0,
>> @@ -130,7 +130,8 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
>>                   __func__, ret, data);
>>       else {
>>           /* Minimum 16ms delay required before RTC update. */
>> -        msleep(MAX77686_RTC_UPDATE_DELAY);
>> +        usleep_range(MAX77686_RTC_UPDATE_DELAY,
>> +                 MAX77686_RTC_UPDATE_DELAY * 2);
>>       }
>>
>
> Instead of making usleep_range(16000, 32000), can we make small range as
> usleep_range(16000, 17000)?
>

Yes, I also didn't know how to make the delay smaller. If I do for example

usleep_range(delay, delay + 10000), then the 10000 delta would be too big
for max77802 (50 times the minimum required 200 delay).

So I used delay * 2 for two reasons:

1) That way is generic enough and can work for any delay

2) My understanding is that most of times the delay should be precise and
    is not that bad if sometimes the delay is the worst case (2 * X) since
    after all the delay is the minimum required.

I also see that usleep_range(X, X * 2) is a used pattern across the kernel.

> I am using as usleep_range(16000, 16000) always.

According to Documentation/timers/timers-howto.txt, that's not a good
idea since usleep_range() is implemented using high-resolution timers
so by not using a range, the kernel won't be able to merge the wakeup
with other wakeups which leads to much more interrupts being triggered.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

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

#1316508 — Re: [PATCH v2 03/10] rtc: max77686: Use usleep_range() instead of msleep()

On Friday 22 January 2016 05:35 PM, Javier Martinez Canillas wrote:
> Hello Laxman,
>
> On 01/22/2016 06:41 AM, Laxman Dewangan wrote:
>>
>> On Friday 22 January 2016 01:53 AM, Javier Martinez Canillas wrote:
>>>       RTC_SEC = 0,
>>> @@ -130,7 +130,8 @@ static int max77686_rtc_update(struct 
>>> max77686_rtc_info *info,
>>>                   __func__, ret, data);
>>>       else {
>>>           /* Minimum 16ms delay required before RTC update. */
>>> -        msleep(MAX77686_RTC_UPDATE_DELAY);
>>> +        usleep_range(MAX77686_RTC_UPDATE_DELAY,
>>> +                 MAX77686_RTC_UPDATE_DELAY * 2);
>>>       }
>>>
>>
>> Instead of making usleep_range(16000, 32000), can we make small range as
>> usleep_range(16000, 17000)?
>>
>
> Yes, I also didn't know how to make the delay smaller. If I do for 
> example
>
> usleep_range(delay, delay + 10000), then the 10000 delta would be too big
> for max77802 (50 times the minimum required 200 delay).
>
> So I used delay * 2 for two reasons:
>
> 1) That way is generic enough and can work for any delay
>
> 2) My understanding is that most of times the delay should be precise and
>    is not that bad if sometimes the delay is the worst case (2 * X) since
>    after all the delay is the minimum required.
>
> I also see that usleep_range(X, X * 2) is a used pattern across the 
> kernel.

OK, fine to me here.

Acked-by: Laxman Dewangan <ldewangan@nvidia.com>

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

#1314506 — [PATCH v2 10/10] ARM: multi_v7_defconfig: Remove MAX77802 RTC Kconfig symbol

The driver has been removed so the Kconfig symbol is not valid anymore.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>


---

Changes in v2:
- Add Krzysztof Kozlowski's Reviewed-by tag to patch #10.

 arch/arm/configs/multi_v7_defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 314f6be2dca2..bdb42c09332c 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -664,7 +664,6 @@ CONFIG_RTC_DRV_MAX8907=y
 CONFIG_RTC_DRV_MAX8997=m
 CONFIG_RTC_DRV_MAX77686=y
 CONFIG_RTC_DRV_RK808=m
-CONFIG_RTC_DRV_MAX77802=m
 CONFIG_RTC_DRV_RS5C372=m
 CONFIG_RTC_DRV_PALMAS=y
 CONFIG_RTC_DRV_ST_LPC=y
-- 
2.5.0

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

#1314870 — Re: [PATCH v2 10/10] ARM: multi_v7_defconfig: Remove MAX77802 RTC Kconfig symbol

On Friday 22 January 2016 01:53 AM, Javier Martinez Canillas wrote:
> The driver has been removed so the Kconfig symbol is not valid anymore.
>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>
>
> ---
>
> Changes in v2:
> - Add Krzysztof Kozlowski's Reviewed-by tag to patch #10.
>
>   arch/arm/configs/multi_v7_defconfig | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
> index 314f6be2dca2..bdb42c09332c 100644
> --- a/arch/arm/configs/multi_v7_defconfig
> +++ b/arch/arm/configs/multi_v7_defconfig
> @@ -664,7 +664,6 @@ CONFIG_RTC_DRV_MAX8907=y
>   CONFIG_RTC_DRV_MAX8997=m
>   CONFIG_RTC_DRV_MAX77686=y
>   CONFIG_RTC_DRV_RK808=m
> -CONFIG_RTC_DRV_MAX77802=m

Do you need to make

CONFIG_RTC_DRV_MAX77686=m

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

#1314932 — Re: [PATCH v2 10/10] ARM: multi_v7_defconfig: Remove MAX77802 RTC Kconfig symbol

Hello Laxman,

On 01/22/2016 06:57 AM, Laxman Dewangan wrote:
>
> On Friday 22 January 2016 01:53 AM, Javier Martinez Canillas wrote:
>> The driver has been removed so the Kconfig symbol is not valid anymore.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>
>>
>> ---
>>
>> Changes in v2:
>> - Add Krzysztof Kozlowski's Reviewed-by tag to patch #10.
>>
>>   arch/arm/configs/multi_v7_defconfig | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
>> index 314f6be2dca2..bdb42c09332c 100644
>> --- a/arch/arm/configs/multi_v7_defconfig
>> +++ b/arch/arm/configs/multi_v7_defconfig
>> @@ -664,7 +664,6 @@ CONFIG_RTC_DRV_MAX8907=y
>>   CONFIG_RTC_DRV_MAX8997=m
>>   CONFIG_RTC_DRV_MAX77686=y
>>   CONFIG_RTC_DRV_RK808=m
>> -CONFIG_RTC_DRV_MAX77802=m
>
> Do you need to make
>
> CONFIG_RTC_DRV_MAX77686=m
>

Yes we should, the RTC_DRV_MAX778686 Kconfig symbol was enabled in multi_v7
before the "build as much as possible as a module" policy was asked so got
enabled built-in. The RTC_DRV_MAX77802 was introduced later so it was asked
to be built as a module instead.

I think we could do that as a separate patch though, once this series land.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

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

#1314508 — [PATCH v2 09/10] ARM: exynos_defconfig: Remove MAX77802 RTC Kconfig symbol

The driver has been removed so the Kconfig symbol is not valid anymore.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

---

Changes in v2:
- Add Krzysztof Kozlowski's Reviewed-by tag to patch #9.

 arch/arm/configs/exynos_defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/configs/exynos_defconfig b/arch/arm/configs/exynos_defconfig
index 24dcd2bb1215..cdbe6dbc6e75 100644
--- a/arch/arm/configs/exynos_defconfig
+++ b/arch/arm/configs/exynos_defconfig
@@ -193,7 +193,6 @@ CONFIG_LEDS_TRIGGER_HEARTBEAT=y
 CONFIG_RTC_CLASS=y
 CONFIG_RTC_DRV_MAX8997=y
 CONFIG_RTC_DRV_MAX77686=y
-CONFIG_RTC_DRV_MAX77802=y
 CONFIG_RTC_DRV_S5M=y
 CONFIG_RTC_DRV_S3C=y
 CONFIG_DMADEVICES=y
-- 
2.5.0

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

#1316961 — Re: [PATCH v2 00/10] rtc: max77686: Extend driver and add max77802 support

Hi,

On 21/01/2016 at 17:23:23 -0300, Javier Martinez Canillas wrote :
> On a recent disussion [0] with Krzysztof Kozlowski and Laxman Dewangan,
> we came to the conclusion that the max77686 and max77802 RTC are almost
> the same with only a few differences so there shouldn't be two separate
> drivers and is better to extend max77686 driver and delete rtc-max77802.
> 
> By making the driver more generic, other RTC IP blocks from Maxim PMICs
> could be supported as well like the max77620.
> 
> This is a v2 of a series that do this, that address issues pointed out
> by Krzysztof Kozlowski. The v1 can be found at [1].
> 
> I've tested this patch-set on an Exynos5800 Peach Pi Chromebook that has
> a max77802 PMIC and the RTC was working correctly but I don't have a
> machine with max77686 so I will really appreaciate if someone can test
> that no regressions were introduced.
> 
> On an IRC conversation, Alexandre suggested to use the field support in
> the regmap API to avoid needing a translation table. I spent some time
> to look at it and I'm not so sure if it fits that well in this case.
> 
> It's true that we could model each register as if it has a single field
> and provide a different reg address but I'm not sure if that would make
> things more clear or cause more confusion for future code archaeologists.
> 

Yeah, Mark suggested that regmap_field may be what we were looking for
but I'm not convinced it really fits.

> In any case, I think this series are a move in the right direction since
> removes code duplication and a complete driver and also allows others to
> reuse the driver for another RTC chip. We can later simplify and use the
> regmap field API or extend the regmap core if that could make things even
> simpler but I propose to do it as a follow up.
> 

I don't have any objection or other comment on that series. So
basically, I'm waiting for v3 and I'll apply it.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

#1317415 — Re: [PATCH v2 00/10] rtc: max77686: Extend driver and add max77802 support

Hello Alexandre,

On 01/25/2016 01:06 PM, Alexandre Belloni wrote:
> Hi,
>
> On 21/01/2016 at 17:23:23 -0300, Javier Martinez Canillas wrote :
>> On a recent disussion [0] with Krzysztof Kozlowski and Laxman Dewangan,
>> we came to the conclusion that the max77686 and max77802 RTC are almost
>> the same with only a few differences so there shouldn't be two separate
>> drivers and is better to extend max77686 driver and delete rtc-max77802.
>>
>> By making the driver more generic, other RTC IP blocks from Maxim PMICs
>> could be supported as well like the max77620.
>>
>> This is a v2 of a series that do this, that address issues pointed out
>> by Krzysztof Kozlowski. The v1 can be found at [1].
>>
>> I've tested this patch-set on an Exynos5800 Peach Pi Chromebook that has
>> a max77802 PMIC and the RTC was working correctly but I don't have a
>> machine with max77686 so I will really appreaciate if someone can test
>> that no regressions were introduced.
>>
>> On an IRC conversation, Alexandre suggested to use the field support in
>> the regmap API to avoid needing a translation table. I spent some time
>> to look at it and I'm not so sure if it fits that well in this case.
>>
>> It's true that we could model each register as if it has a single field
>> and provide a different reg address but I'm not sure if that would make
>> things more clear or cause more confusion for future code archaeologists.
>>
>
> Yeah, Mark suggested that regmap_field may be what we were looking for
> but I'm not convinced it really fits.
>

Ok.
  
>> In any case, I think this series are a move in the right direction since
>> removes code duplication and a complete driver and also allows others to
>> reuse the driver for another RTC chip. We can later simplify and use the
>> regmap field API or extend the regmap core if that could make things even
>> simpler but I propose to do it as a follow up.
>>
>
> I don't have any objection or other comment on that series. So
> basically, I'm waiting for v3 and I'll apply it.
>
>

Great, I'll post a v3 tomorrow then. Thanks!

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web