Re: [rtc-linux] Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-29 Thread Alim Akhtar 
Hi Krzysztof,

On Thu, Oct 29, 2015 at 5:05 PM, Krzysztof Kozlowski
 wrote:
> 2015-10-29 20:20 GMT+09:00 Alim Akhtar :
 I am testing this patch before sending them, what I have found is if you
 don't update WUDR the time does not changes in rtc.
 e.g.
 if you don't do above changes then you will see below:
 -
 # date --set="Oct 29 14:10:40 2015" 
 Thu Oct 29 14:10:40 UTC 2015
 #
 # hwclock --systohc 
 # hwclock 
 Thu Oct 29 12:52:32 2015  .922889 seconds
 
 which is not excepted.

 And with the above change I see:

 
   # date --set="Oct 29 14:30:10 2015" 
 Thu Oct 29 14:30:10 UTC 2015
 # date
 Thu Oct 29 14:30:12 UTC 2015
 # hwclock --systohc 
 # hwclock 
 Thu Oct 29 14:30:21 2015  .333006 seconds
 

 Which is as expected.
>>>
>>>
>>> Okay, but I said that vendor is setting only WUDR which is bit 4. You
>>
>> bit:4 is AUDR (for alarm). not WUDR and bit: 1 is WDUR(for timer)
>
> Damn, the mismatch in names is confusing. The vendor sets WUDR which
> on S2MPS15 is bit 1.
>
> You are setting bit 1 (okay) and bit 4. This is different which makes
> me asking questions.
>
hmm..ok let me have a relook. Perhaps I will add a switch case here to
handle each device_type,
keeping the current behavior as same.

>>>
>>> are setting bit 1 and bit 4. Your reply - about the need of setting
>>> WUDR (bit 4 I guess) - does not explain my concerns. Do you have to
>>> set bit 1?
>>>
>> Yes, I think we need to set both for timer and alarm.
>> I have send you a snapshot of the rtc_update register in UM via internal
>> email. PTAL.
>
> Okay, I will take a look when I get to the office tomorrow.
>
>>

>>  ret = regmap_write(info->regmap, info->regs->rtc_udr_update,
>> data);
>>  if (ret < 0) {
>>  dev_err(info->dev, "failed to write update reg(%d)\n",
>> ret);
>> @@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct
>> s5m_rtc_info *info)
>>  case S5M8767X:
>>  data &= ~S5M_RTC_TIME_EN_MASK;
>>  break;
>> +   case S2MPS15X:
>> +   data |= S2MPS15_RTC_AUDR_MASK;
>> +   break;
>>  case S2MPS14X:
>>  data |= S2MPS_RTC_RUDR_MASK;
>>  break;
>
>
>
> Another difference: you are setting here exactly the same values as
> S2MPS13 - bit 1 and bit 4. However vendor code sets only bit 4 (called
> WUDR there)?
>
 No they are not, AUDR on s2mps13 is bit:1 where as it is bit:4 on
 s2mps15.
>
>
> What exactly is necessary to update alarm and time registers on S2MPS15?
>
 As explained above in example, it is required to update the 'write alarm
 buffer(AUDR)' and 'write time buffer(WUDR)' bits in-order to get rtc
 working.
>>>
>>>
>>> For updating time and alarm registers? Or only for updating alarm
>>> registers?
>>>
>> example was only for the timer register.
>>
>
> Anyway setting alarm above is strange. You are setting bit 4 twice. One as:
>   data |= info->regs->rtc_udr_mask;
> and then:
>   case S2MPS15X:
> data |= S2MPS15_RTC_AUDR_MASK;
>
ok, I can see what you are saying...good catch.

> Yeah, I know this is misleading. Could you document expected behavior
> exactly somewhere in the code or in the commit message? "bit fields
> are changed for WUDR and AUDR." seems not enough. Instead I would be
> happy to see a statement: "to set time registers, AUDR and WUDR must
> be set, to set alarm   etc. where AUDR and WUDR on S2MPS15 are
> reversed"
>
This is a nice suggestions, will add.

> Best regards,
> Krzysztof
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 
> in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Regards,
Alim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [rtc-linux] Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-29 Thread Krzysztof Kozlowski 
2015-10-29 20:20 GMT+09:00 Alim Akhtar :
>>> I am testing this patch before sending them, what I have found is if you
>>> don't update WUDR the time does not changes in rtc.
>>> e.g.
>>> if you don't do above changes then you will see below:
>>> -
>>> # date --set="Oct 29 14:10:40 2015" 
>>> Thu Oct 29 14:10:40 UTC 2015
>>> #
>>> # hwclock --systohc 
>>> # hwclock 
>>> Thu Oct 29 12:52:32 2015  .922889 seconds
>>> 
>>> which is not excepted.
>>>
>>> And with the above change I see:
>>>
>>> 
>>>   # date --set="Oct 29 14:30:10 2015" 
>>> Thu Oct 29 14:30:10 UTC 2015
>>> # date
>>> Thu Oct 29 14:30:12 UTC 2015
>>> # hwclock --systohc 
>>> # hwclock 
>>> Thu Oct 29 14:30:21 2015  .333006 seconds
>>> 
>>>
>>> Which is as expected.
>>
>>
>> Okay, but I said that vendor is setting only WUDR which is bit 4. You
>
> bit:4 is AUDR (for alarm). not WUDR and bit: 1 is WDUR(for timer)

Damn, the mismatch in names is confusing. The vendor sets WUDR which
on S2MPS15 is bit 1.

You are setting bit 1 (okay) and bit 4. This is different which makes
me asking questions.

>>
>> are setting bit 1 and bit 4. Your reply - about the need of setting
>> WUDR (bit 4 I guess) - does not explain my concerns. Do you have to
>> set bit 1?
>>
> Yes, I think we need to set both for timer and alarm.
> I have send you a snapshot of the rtc_update register in UM via internal
> email. PTAL.

Okay, I will take a look when I get to the office tomorrow.

>
>>>
>  ret = regmap_write(info->regmap, info->regs->rtc_udr_update,
> data);
>  if (ret < 0) {
>  dev_err(info->dev, "failed to write update reg(%d)\n",
> ret);
> @@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct
> s5m_rtc_info *info)
>  case S5M8767X:
>  data &= ~S5M_RTC_TIME_EN_MASK;
>  break;
> +   case S2MPS15X:
> +   data |= S2MPS15_RTC_AUDR_MASK;
> +   break;
>  case S2MPS14X:
>  data |= S2MPS_RTC_RUDR_MASK;
>  break;



 Another difference: you are setting here exactly the same values as
 S2MPS13 - bit 1 and bit 4. However vendor code sets only bit 4 (called
 WUDR there)?

>>> No they are not, AUDR on s2mps13 is bit:1 where as it is bit:4 on
>>> s2mps15.


 What exactly is necessary to update alarm and time registers on S2MPS15?

>>> As explained above in example, it is required to update the 'write alarm
>>> buffer(AUDR)' and 'write time buffer(WUDR)' bits in-order to get rtc
>>> working.
>>
>>
>> For updating time and alarm registers? Or only for updating alarm
>> registers?
>>
> example was only for the timer register.
>

Anyway setting alarm above is strange. You are setting bit 4 twice. One as:
  data |= info->regs->rtc_udr_mask;
and then:
  case S2MPS15X:
data |= S2MPS15_RTC_AUDR_MASK;

Yeah, I know this is misleading. Could you document expected behavior
exactly somewhere in the code or in the commit message? "bit fields
are changed for WUDR and AUDR." seems not enough. Instead I would be
happy to see a statement: "to set time registers, AUDR and WUDR must
be set, to set alarm   etc. where AUDR and WUDR on S2MPS15 are
reversed"

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-29 Thread Alim Akhtar 



On 10/29/2015 04:33 PM, Krzysztof Kozlowski wrote:

2015-10-29 16:47 GMT+09:00 Alim Akhtar :

Hello Krzysztof,


On 10/29/2015 11:24 AM, Krzysztof Kozlowski wrote:


On 29.10.2015 13:58, Alim Akhtar wrote:


RTC found in s2mps15 is almost same as one found on s2mps14/13
with few differences in RTC_UPDATE register fields, like
bit fields are changed for WUDR and AUDR.
This patch add required changes to enable s2mps15 rtc timer.

Cc: Alexandre Belloni 
Signed-off-by: Alim Akhtar 
---
   drivers/rtc/rtc-s5m.c   |   20 ++--
   include/linux/mfd/samsung/rtc.h |4 
   2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index f2504b4eef34..0d106a99958f 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -188,6 +188,7 @@ static inline int
s5m_check_peding_alarm_interrupt(struct s5m_rtc_info *info,
 ret = regmap_read(info->regmap, S5M_RTC_STATUS, );
 val &= S5M_ALARM0_STATUS;
 break;
+   case S2MPS15X:
 case S2MPS14X:
 case S2MPS13X:
 ret = regmap_read(info->s5m87xx->regmap_pmic,
S2MPS14_REG_ST2,
@@ -223,6 +224,9 @@ static inline int s5m8767_rtc_set_time_reg(struct
s5m_rtc_info *info)
 if (info->device_type == S5M8763X || info->device_type ==
S5M8767X)
 data |= S5M_RTC_TIME_EN_MASK;

+   if (info->device_type == S2MPS15X)
+   data |= S2MPS15_RTC_WUDR_MASK;
+




You are setting here bit 1 and bit 4. However vendor code sets only bit
4 (called WUDR there). That's confusing. Why the difference? Or maybe I
am looking at wrong vendor tree (SM-G920F)?


Sorry, I don't have access to vendor code that you are looking into, so
won't be able to comment on that.


Everyone have the access. It is on opensource.samsung.com, as required by GPL.


Ok, I didn't check there.


I am testing this patch before sending them, what I have found is if you
don't update WUDR the time does not changes in rtc.
e.g.
if you don't do above changes then you will see below:
-
# date --set="Oct 29 14:10:40 2015" 
Thu Oct 29 14:10:40 UTC 2015
#
# hwclock --systohc 
# hwclock 
Thu Oct 29 12:52:32 2015  .922889 seconds

which is not excepted.

And with the above change I see:


  # date --set="Oct 29 14:30:10 2015" 
Thu Oct 29 14:30:10 UTC 2015
# date
Thu Oct 29 14:30:12 UTC 2015
# hwclock --systohc 
# hwclock 
Thu Oct 29 14:30:21 2015  .333006 seconds


Which is as expected.


Okay, but I said that vendor is setting only WUDR which is bit 4. You

bit:4 is AUDR (for alarm). not WUDR and bit: 1 is WDUR(for timer)

are setting bit 1 and bit 4. Your reply - about the need of setting
WUDR (bit 4 I guess) - does not explain my concerns. Do you have to
set bit 1?


Yes, I think we need to set both for timer and alarm.
I have send you a snapshot of the rtc_update register in UM via internal 
email. PTAL.



 ret = regmap_write(info->regmap, info->regs->rtc_udr_update,
data);
 if (ret < 0) {
 dev_err(info->dev, "failed to write update reg(%d)\n",
ret);
@@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct
s5m_rtc_info *info)
 case S5M8767X:
 data &= ~S5M_RTC_TIME_EN_MASK;
 break;
+   case S2MPS15X:
+   data |= S2MPS15_RTC_AUDR_MASK;
+   break;
 case S2MPS14X:
 data |= S2MPS_RTC_RUDR_MASK;
 break;



Another difference: you are setting here exactly the same values as
S2MPS13 - bit 1 and bit 4. However vendor code sets only bit 4 (called
WUDR there)?


No they are not, AUDR on s2mps13 is bit:1 where as it is bit:4 on s2mps15.


What exactly is necessary to update alarm and time registers on S2MPS15?


As explained above in example, it is required to update the 'write alarm
buffer(AUDR)' and 'write time buffer(WUDR)' bits in-order to get rtc
working.


For updating time and alarm registers? Or only for updating alarm registers?


example was only for the timer register.


Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-29 Thread Krzysztof Kozlowski 
2015-10-29 16:47 GMT+09:00 Alim Akhtar :
> Hello Krzysztof,
>
>
> On 10/29/2015 11:24 AM, Krzysztof Kozlowski wrote:
>>
>> On 29.10.2015 13:58, Alim Akhtar wrote:
>>>
>>> RTC found in s2mps15 is almost same as one found on s2mps14/13
>>> with few differences in RTC_UPDATE register fields, like
>>> bit fields are changed for WUDR and AUDR.
>>> This patch add required changes to enable s2mps15 rtc timer.
>>>
>>> Cc: Alexandre Belloni 
>>> Signed-off-by: Alim Akhtar 
>>> ---
>>>   drivers/rtc/rtc-s5m.c   |   20 ++--
>>>   include/linux/mfd/samsung/rtc.h |4 
>>>   2 files changed, 22 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
>>> index f2504b4eef34..0d106a99958f 100644
>>> --- a/drivers/rtc/rtc-s5m.c
>>> +++ b/drivers/rtc/rtc-s5m.c
>>> @@ -188,6 +188,7 @@ static inline int
>>> s5m_check_peding_alarm_interrupt(struct s5m_rtc_info *info,
>>> ret = regmap_read(info->regmap, S5M_RTC_STATUS, );
>>> val &= S5M_ALARM0_STATUS;
>>> break;
>>> +   case S2MPS15X:
>>> case S2MPS14X:
>>> case S2MPS13X:
>>> ret = regmap_read(info->s5m87xx->regmap_pmic,
>>> S2MPS14_REG_ST2,
>>> @@ -223,6 +224,9 @@ static inline int s5m8767_rtc_set_time_reg(struct
>>> s5m_rtc_info *info)
>>> if (info->device_type == S5M8763X || info->device_type ==
>>> S5M8767X)
>>> data |= S5M_RTC_TIME_EN_MASK;
>>>
>>> +   if (info->device_type == S2MPS15X)
>>> +   data |= S2MPS15_RTC_WUDR_MASK;
>>> +
>>
>>
>>
>> You are setting here bit 1 and bit 4. However vendor code sets only bit
>> 4 (called WUDR there). That's confusing. Why the difference? Or maybe I
>> am looking at wrong vendor tree (SM-G920F)?
>>
> Sorry, I don't have access to vendor code that you are looking into, so
> won't be able to comment on that.

Everyone have the access. It is on opensource.samsung.com, as required by GPL.

>
> I am testing this patch before sending them, what I have found is if you
> don't update WUDR the time does not changes in rtc.
> e.g.
> if you don't do above changes then you will see below:
> -
> # date --set="Oct 29 14:10:40 2015" 
> Thu Oct 29 14:10:40 UTC 2015
> #
> # hwclock --systohc 
> # hwclock 
> Thu Oct 29 12:52:32 2015  .922889 seconds
> 
> which is not excepted.
>
> And with the above change I see:
>
> 
>  # date --set="Oct 29 14:30:10 2015" 
> Thu Oct 29 14:30:10 UTC 2015
> # date
> Thu Oct 29 14:30:12 UTC 2015
> # hwclock --systohc 
> # hwclock 
> Thu Oct 29 14:30:21 2015  .333006 seconds
> 
>
> Which is as expected.

Okay, but I said that vendor is setting only WUDR which is bit 4. You
are setting bit 1 and bit 4. Your reply - about the need of setting
WUDR (bit 4 I guess) - does not explain my concerns. Do you have to
set bit 1?

>
>>> ret = regmap_write(info->regmap, info->regs->rtc_udr_update,
>>> data);
>>> if (ret < 0) {
>>> dev_err(info->dev, "failed to write update reg(%d)\n",
>>> ret);
>>> @@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct
>>> s5m_rtc_info *info)
>>> case S5M8767X:
>>> data &= ~S5M_RTC_TIME_EN_MASK;
>>> break;
>>> +   case S2MPS15X:
>>> +   data |= S2MPS15_RTC_AUDR_MASK;
>>> +   break;
>>> case S2MPS14X:
>>> data |= S2MPS_RTC_RUDR_MASK;
>>> break;
>>
>>
>> Another difference: you are setting here exactly the same values as
>> S2MPS13 - bit 1 and bit 4. However vendor code sets only bit 4 (called
>> WUDR there)?
>>
> No they are not, AUDR on s2mps13 is bit:1 where as it is bit:4 on s2mps15.
>>
>> What exactly is necessary to update alarm and time registers on S2MPS15?
>>
> As explained above in example, it is required to update the 'write alarm
> buffer(AUDR)' and 'write time buffer(WUDR)' bits in-order to get rtc
> working.

For updating time and alarm registers? Or only for updating alarm registers?

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-29 Thread Alim Akhtar 

Hello Krzysztof,

On 10/29/2015 11:24 AM, Krzysztof Kozlowski wrote:

On 29.10.2015 13:58, Alim Akhtar wrote:

RTC found in s2mps15 is almost same as one found on s2mps14/13
with few differences in RTC_UPDATE register fields, like
bit fields are changed for WUDR and AUDR.
This patch add required changes to enable s2mps15 rtc timer.

Cc: Alexandre Belloni 
Signed-off-by: Alim Akhtar 
---
  drivers/rtc/rtc-s5m.c   |   20 ++--
  include/linux/mfd/samsung/rtc.h |4 
  2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index f2504b4eef34..0d106a99958f 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -188,6 +188,7 @@ static inline int s5m_check_peding_alarm_interrupt(struct 
s5m_rtc_info *info,
ret = regmap_read(info->regmap, S5M_RTC_STATUS, );
val &= S5M_ALARM0_STATUS;
break;
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
ret = regmap_read(info->s5m87xx->regmap_pmic, S2MPS14_REG_ST2,
@@ -223,6 +224,9 @@ static inline int s5m8767_rtc_set_time_reg(struct 
s5m_rtc_info *info)
if (info->device_type == S5M8763X || info->device_type == S5M8767X)
data |= S5M_RTC_TIME_EN_MASK;

+   if (info->device_type == S2MPS15X)
+   data |= S2MPS15_RTC_WUDR_MASK;
+



You are setting here bit 1 and bit 4. However vendor code sets only bit
4 (called WUDR there). That's confusing. Why the difference? Or maybe I
am looking at wrong vendor tree (SM-G920F)?

Sorry, I don't have access to vendor code that you are looking into, so 
won't be able to comment on that.


I am testing this patch before sending them, what I have found is if you 
don't update WUDR the time does not changes in rtc.

e.g.
if you don't do above changes then you will see below:
-
# date --set="Oct 29 14:10:40 2015" 
Thu Oct 29 14:10:40 UTC 2015
#
# hwclock --systohc 
# hwclock 
Thu Oct 29 12:52:32 2015  .922889 seconds

which is not excepted.

And with the above change I see:


 # date --set="Oct 29 14:30:10 2015" 
Thu Oct 29 14:30:10 UTC 2015
# date
Thu Oct 29 14:30:12 UTC 2015
# hwclock --systohc 
# hwclock 
Thu Oct 29 14:30:21 2015  .333006 seconds


Which is as expected.


ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
if (ret < 0) {
dev_err(info->dev, "failed to write update reg(%d)\n", ret);
@@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
s5m_rtc_info *info)
case S5M8767X:
data &= ~S5M_RTC_TIME_EN_MASK;
break;
+   case S2MPS15X:
+   data |= S2MPS15_RTC_AUDR_MASK;
+   break;
case S2MPS14X:
data |= S2MPS_RTC_RUDR_MASK;
break;


Another difference: you are setting here exactly the same values as
S2MPS13 - bit 1 and bit 4. However vendor code sets only bit 4 (called
WUDR there)?


No they are not, AUDR on s2mps13 is bit:1 where as it is bit:4 on s2mps15.

What exactly is necessary to update alarm and time registers on S2MPS15?

As explained above in example, it is required to update the 'write alarm 
buffer(AUDR)' and 'write time buffer(WUDR)' bits in-order to get rtc 
working.



@@ -275,7 +282,6 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
s5m_rtc_info *info)
if (info->device_type == S2MPS13X)
regmap_update_bits(info->regmap, info->regs->rtc_udr_update,
   S2MPS13_RTC_AUDR_MASK, 0);
-


This is not related to the patch.


uhh...will change.

Best regards,
Krzysztof


Thanks again.



return ret;
  }

@@ -317,7 +323,8 @@ static int s5m_rtc_read_time(struct device *dev, struct 
rtc_time *tm)
u8 data[info->regs->regs_count];
int ret;

-   if (info->device_type == S2MPS14X || info->device_type == S2MPS13X) {
+   if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
+   info->device_type == S2MPS13X) {
ret = regmap_update_bits(info->regmap,
info->regs->rtc_udr_update,
S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
@@ -339,6 +346,7 @@ static int s5m_rtc_read_time(struct device *dev, struct 
rtc_time *tm)
break;

case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
s5m8767_data_to_tm(data, tm, info->rtc_24hr_mode);
@@ -366,6 +374,7 @@ static int s5m_rtc_set_time(struct device *dev, struct 
rtc_time *tm)
s5m8763_tm_to_data(tm, data);
break;
case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
ret = s5m8767_tm_to_data(tm, data);
@@ -414,6 +423,7 @@ static int s5m_rtc_read_alarm(struct device *dev, struct 
rtc_wkalrm *alrm)
break;

case S5M8767X:
+   case

Re: [rtc-linux] Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-29 Thread Krzysztof Kozlowski 
2015-10-29 20:20 GMT+09:00 Alim Akhtar :
>>> I am testing this patch before sending them, what I have found is if you
>>> don't update WUDR the time does not changes in rtc.
>>> e.g.
>>> if you don't do above changes then you will see below:
>>> -
>>> # date --set="Oct 29 14:10:40 2015" 
>>> Thu Oct 29 14:10:40 UTC 2015
>>> #
>>> # hwclock --systohc 
>>> # hwclock 
>>> Thu Oct 29 12:52:32 2015  .922889 seconds
>>> 
>>> which is not excepted.
>>>
>>> And with the above change I see:
>>>
>>> 
>>>   # date --set="Oct 29 14:30:10 2015" 
>>> Thu Oct 29 14:30:10 UTC 2015
>>> # date
>>> Thu Oct 29 14:30:12 UTC 2015
>>> # hwclock --systohc 
>>> # hwclock 
>>> Thu Oct 29 14:30:21 2015  .333006 seconds
>>> 
>>>
>>> Which is as expected.
>>
>>
>> Okay, but I said that vendor is setting only WUDR which is bit 4. You
>
> bit:4 is AUDR (for alarm). not WUDR and bit: 1 is WDUR(for timer)

Damn, the mismatch in names is confusing. The vendor sets WUDR which
on S2MPS15 is bit 1.

You are setting bit 1 (okay) and bit 4. This is different which makes
me asking questions.

>>
>> are setting bit 1 and bit 4. Your reply - about the need of setting
>> WUDR (bit 4 I guess) - does not explain my concerns. Do you have to
>> set bit 1?
>>
> Yes, I think we need to set both for timer and alarm.
> I have send you a snapshot of the rtc_update register in UM via internal
> email. PTAL.

Okay, I will take a look when I get to the office tomorrow.

>
>>>
>  ret = regmap_write(info->regmap, info->regs->rtc_udr_update,
> data);
>  if (ret < 0) {
>  dev_err(info->dev, "failed to write update reg(%d)\n",
> ret);
> @@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct
> s5m_rtc_info *info)
>  case S5M8767X:
>  data &= ~S5M_RTC_TIME_EN_MASK;
>  break;
> +   case S2MPS15X:
> +   data |= S2MPS15_RTC_AUDR_MASK;
> +   break;
>  case S2MPS14X:
>  data |= S2MPS_RTC_RUDR_MASK;
>  break;



 Another difference: you are setting here exactly the same values as
 S2MPS13 - bit 1 and bit 4. However vendor code sets only bit 4 (called
 WUDR there)?

>>> No they are not, AUDR on s2mps13 is bit:1 where as it is bit:4 on
>>> s2mps15.


 What exactly is necessary to update alarm and time registers on S2MPS15?

>>> As explained above in example, it is required to update the 'write alarm
>>> buffer(AUDR)' and 'write time buffer(WUDR)' bits in-order to get rtc
>>> working.
>>
>>
>> For updating time and alarm registers? Or only for updating alarm
>> registers?
>>
> example was only for the timer register.
>

Anyway setting alarm above is strange. You are setting bit 4 twice. One as:
  data |= info->regs->rtc_udr_mask;
and then:
  case S2MPS15X:
data |= S2MPS15_RTC_AUDR_MASK;

Yeah, I know this is misleading. Could you document expected behavior
exactly somewhere in the code or in the commit message? "bit fields
are changed for WUDR and AUDR." seems not enough. Instead I would be
happy to see a statement: "to set time registers, AUDR and WUDR must
be set, to set alarm   etc. where AUDR and WUDR on S2MPS15 are
reversed"

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [rtc-linux] Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-29 Thread Alim Akhtar 
Hi Krzysztof,

On Thu, Oct 29, 2015 at 5:05 PM, Krzysztof Kozlowski
 wrote:
> 2015-10-29 20:20 GMT+09:00 Alim Akhtar :
 I am testing this patch before sending them, what I have found is if you
 don't update WUDR the time does not changes in rtc.
 e.g.
 if you don't do above changes then you will see below:
 -
 # date --set="Oct 29 14:10:40 2015" 
 Thu Oct 29 14:10:40 UTC 2015
 #
 # hwclock --systohc 
 # hwclock 
 Thu Oct 29 12:52:32 2015  .922889 seconds
 
 which is not excepted.

 And with the above change I see:

 
   # date --set="Oct 29 14:30:10 2015" 
 Thu Oct 29 14:30:10 UTC 2015
 # date
 Thu Oct 29 14:30:12 UTC 2015
 # hwclock --systohc 
 # hwclock 
 Thu Oct 29 14:30:21 2015  .333006 seconds
 

 Which is as expected.
>>>
>>>
>>> Okay, but I said that vendor is setting only WUDR which is bit 4. You
>>
>> bit:4 is AUDR (for alarm). not WUDR and bit: 1 is WDUR(for timer)
>
> Damn, the mismatch in names is confusing. The vendor sets WUDR which
> on S2MPS15 is bit 1.
>
> You are setting bit 1 (okay) and bit 4. This is different which makes
> me asking questions.
>
hmm..ok let me have a relook. Perhaps I will add a switch case here to
handle each device_type,
keeping the current behavior as same.

>>>
>>> are setting bit 1 and bit 4. Your reply - about the need of setting
>>> WUDR (bit 4 I guess) - does not explain my concerns. Do you have to
>>> set bit 1?
>>>
>> Yes, I think we need to set both for timer and alarm.
>> I have send you a snapshot of the rtc_update register in UM via internal
>> email. PTAL.
>
> Okay, I will take a look when I get to the office tomorrow.
>
>>

>>  ret = regmap_write(info->regmap, info->regs->rtc_udr_update,
>> data);
>>  if (ret < 0) {
>>  dev_err(info->dev, "failed to write update reg(%d)\n",
>> ret);
>> @@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct
>> s5m_rtc_info *info)
>>  case S5M8767X:
>>  data &= ~S5M_RTC_TIME_EN_MASK;
>>  break;
>> +   case S2MPS15X:
>> +   data |= S2MPS15_RTC_AUDR_MASK;
>> +   break;
>>  case S2MPS14X:
>>  data |= S2MPS_RTC_RUDR_MASK;
>>  break;
>
>
>
> Another difference: you are setting here exactly the same values as
> S2MPS13 - bit 1 and bit 4. However vendor code sets only bit 4 (called
> WUDR there)?
>
 No they are not, AUDR on s2mps13 is bit:1 where as it is bit:4 on
 s2mps15.
>
>
> What exactly is necessary to update alarm and time registers on S2MPS15?
>
 As explained above in example, it is required to update the 'write alarm
 buffer(AUDR)' and 'write time buffer(WUDR)' bits in-order to get rtc
 working.
>>>
>>>
>>> For updating time and alarm registers? Or only for updating alarm
>>> registers?
>>>
>> example was only for the timer register.
>>
>
> Anyway setting alarm above is strange. You are setting bit 4 twice. One as:
>   data |= info->regs->rtc_udr_mask;
> and then:
>   case S2MPS15X:
> data |= S2MPS15_RTC_AUDR_MASK;
>
ok, I can see what you are saying...good catch.

> Yeah, I know this is misleading. Could you document expected behavior
> exactly somewhere in the code or in the commit message? "bit fields
> are changed for WUDR and AUDR." seems not enough. Instead I would be
> happy to see a statement: "to set time registers, AUDR and WUDR must
> be set, to set alarm   etc. where AUDR and WUDR on S2MPS15 are
> reversed"
>
This is a nice suggestions, will add.

> Best regards,
> Krzysztof
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 
> in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Regards,
Alim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-29 Thread Alim Akhtar 

Hello Krzysztof,

On 10/29/2015 11:24 AM, Krzysztof Kozlowski wrote:

On 29.10.2015 13:58, Alim Akhtar wrote:

RTC found in s2mps15 is almost same as one found on s2mps14/13
with few differences in RTC_UPDATE register fields, like
bit fields are changed for WUDR and AUDR.
This patch add required changes to enable s2mps15 rtc timer.

Cc: Alexandre Belloni 
Signed-off-by: Alim Akhtar 
---
  drivers/rtc/rtc-s5m.c   |   20 ++--
  include/linux/mfd/samsung/rtc.h |4 
  2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index f2504b4eef34..0d106a99958f 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -188,6 +188,7 @@ static inline int s5m_check_peding_alarm_interrupt(struct 
s5m_rtc_info *info,
ret = regmap_read(info->regmap, S5M_RTC_STATUS, );
val &= S5M_ALARM0_STATUS;
break;
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
ret = regmap_read(info->s5m87xx->regmap_pmic, S2MPS14_REG_ST2,
@@ -223,6 +224,9 @@ static inline int s5m8767_rtc_set_time_reg(struct 
s5m_rtc_info *info)
if (info->device_type == S5M8763X || info->device_type == S5M8767X)
data |= S5M_RTC_TIME_EN_MASK;

+   if (info->device_type == S2MPS15X)
+   data |= S2MPS15_RTC_WUDR_MASK;
+



You are setting here bit 1 and bit 4. However vendor code sets only bit
4 (called WUDR there). That's confusing. Why the difference? Or maybe I
am looking at wrong vendor tree (SM-G920F)?

Sorry, I don't have access to vendor code that you are looking into, so 
won't be able to comment on that.


I am testing this patch before sending them, what I have found is if you 
don't update WUDR the time does not changes in rtc.

e.g.
if you don't do above changes then you will see below:
-
# date --set="Oct 29 14:10:40 2015" 
Thu Oct 29 14:10:40 UTC 2015
#
# hwclock --systohc 
# hwclock 
Thu Oct 29 12:52:32 2015  .922889 seconds

which is not excepted.

And with the above change I see:


 # date --set="Oct 29 14:30:10 2015" 
Thu Oct 29 14:30:10 UTC 2015
# date
Thu Oct 29 14:30:12 UTC 2015
# hwclock --systohc 
# hwclock 
Thu Oct 29 14:30:21 2015  .333006 seconds


Which is as expected.


ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
if (ret < 0) {
dev_err(info->dev, "failed to write update reg(%d)\n", ret);
@@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
s5m_rtc_info *info)
case S5M8767X:
data &= ~S5M_RTC_TIME_EN_MASK;
break;
+   case S2MPS15X:
+   data |= S2MPS15_RTC_AUDR_MASK;
+   break;
case S2MPS14X:
data |= S2MPS_RTC_RUDR_MASK;
break;


Another difference: you are setting here exactly the same values as
S2MPS13 - bit 1 and bit 4. However vendor code sets only bit 4 (called
WUDR there)?


No they are not, AUDR on s2mps13 is bit:1 where as it is bit:4 on s2mps15.

What exactly is necessary to update alarm and time registers on S2MPS15?

As explained above in example, it is required to update the 'write alarm 
buffer(AUDR)' and 'write time buffer(WUDR)' bits in-order to get rtc 
working.



@@ -275,7 +282,6 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
s5m_rtc_info *info)
if (info->device_type == S2MPS13X)
regmap_update_bits(info->regmap, info->regs->rtc_udr_update,
   S2MPS13_RTC_AUDR_MASK, 0);
-


This is not related to the patch.


uhh...will change.

Best regards,
Krzysztof


Thanks again.



return ret;
  }

@@ -317,7 +323,8 @@ static int s5m_rtc_read_time(struct device *dev, struct 
rtc_time *tm)
u8 data[info->regs->regs_count];
int ret;

-   if (info->device_type == S2MPS14X || info->device_type == S2MPS13X) {
+   if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
+   info->device_type == S2MPS13X) {
ret = regmap_update_bits(info->regmap,
info->regs->rtc_udr_update,
S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
@@ -339,6 +346,7 @@ static int s5m_rtc_read_time(struct device *dev, struct 
rtc_time *tm)
break;

case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
s5m8767_data_to_tm(data, tm, info->rtc_24hr_mode);
@@ -366,6 +374,7 @@ static int s5m_rtc_set_time(struct device *dev, struct 
rtc_time *tm)
s5m8763_tm_to_data(tm, data);
break;
case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
ret = s5m8767_tm_to_data(tm, data);
@@ -414,6 +423,7 @@ static int s5m_rtc_read_alarm(struct device *dev, struct 
rtc_wkalrm

Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-29 Thread Krzysztof Kozlowski 
2015-10-29 16:47 GMT+09:00 Alim Akhtar :
> Hello Krzysztof,
>
>
> On 10/29/2015 11:24 AM, Krzysztof Kozlowski wrote:
>>
>> On 29.10.2015 13:58, Alim Akhtar wrote:
>>>
>>> RTC found in s2mps15 is almost same as one found on s2mps14/13
>>> with few differences in RTC_UPDATE register fields, like
>>> bit fields are changed for WUDR and AUDR.
>>> This patch add required changes to enable s2mps15 rtc timer.
>>>
>>> Cc: Alexandre Belloni 
>>> Signed-off-by: Alim Akhtar 
>>> ---
>>>   drivers/rtc/rtc-s5m.c   |   20 ++--
>>>   include/linux/mfd/samsung/rtc.h |4 
>>>   2 files changed, 22 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
>>> index f2504b4eef34..0d106a99958f 100644
>>> --- a/drivers/rtc/rtc-s5m.c
>>> +++ b/drivers/rtc/rtc-s5m.c
>>> @@ -188,6 +188,7 @@ static inline int
>>> s5m_check_peding_alarm_interrupt(struct s5m_rtc_info *info,
>>> ret = regmap_read(info->regmap, S5M_RTC_STATUS, );
>>> val &= S5M_ALARM0_STATUS;
>>> break;
>>> +   case S2MPS15X:
>>> case S2MPS14X:
>>> case S2MPS13X:
>>> ret = regmap_read(info->s5m87xx->regmap_pmic,
>>> S2MPS14_REG_ST2,
>>> @@ -223,6 +224,9 @@ static inline int s5m8767_rtc_set_time_reg(struct
>>> s5m_rtc_info *info)
>>> if (info->device_type == S5M8763X || info->device_type ==
>>> S5M8767X)
>>> data |= S5M_RTC_TIME_EN_MASK;
>>>
>>> +   if (info->device_type == S2MPS15X)
>>> +   data |= S2MPS15_RTC_WUDR_MASK;
>>> +
>>
>>
>>
>> You are setting here bit 1 and bit 4. However vendor code sets only bit
>> 4 (called WUDR there). That's confusing. Why the difference? Or maybe I
>> am looking at wrong vendor tree (SM-G920F)?
>>
> Sorry, I don't have access to vendor code that you are looking into, so
> won't be able to comment on that.

Everyone have the access. It is on opensource.samsung.com, as required by GPL.

>
> I am testing this patch before sending them, what I have found is if you
> don't update WUDR the time does not changes in rtc.
> e.g.
> if you don't do above changes then you will see below:
> -
> # date --set="Oct 29 14:10:40 2015" 
> Thu Oct 29 14:10:40 UTC 2015
> #
> # hwclock --systohc 
> # hwclock 
> Thu Oct 29 12:52:32 2015  .922889 seconds
> 
> which is not excepted.
>
> And with the above change I see:
>
> 
>  # date --set="Oct 29 14:30:10 2015" 
> Thu Oct 29 14:30:10 UTC 2015
> # date
> Thu Oct 29 14:30:12 UTC 2015
> # hwclock --systohc 
> # hwclock 
> Thu Oct 29 14:30:21 2015  .333006 seconds
> 
>
> Which is as expected.

Okay, but I said that vendor is setting only WUDR which is bit 4. You
are setting bit 1 and bit 4. Your reply - about the need of setting
WUDR (bit 4 I guess) - does not explain my concerns. Do you have to
set bit 1?

>
>>> ret = regmap_write(info->regmap, info->regs->rtc_udr_update,
>>> data);
>>> if (ret < 0) {
>>> dev_err(info->dev, "failed to write update reg(%d)\n",
>>> ret);
>>> @@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct
>>> s5m_rtc_info *info)
>>> case S5M8767X:
>>> data &= ~S5M_RTC_TIME_EN_MASK;
>>> break;
>>> +   case S2MPS15X:
>>> +   data |= S2MPS15_RTC_AUDR_MASK;
>>> +   break;
>>> case S2MPS14X:
>>> data |= S2MPS_RTC_RUDR_MASK;
>>> break;
>>
>>
>> Another difference: you are setting here exactly the same values as
>> S2MPS13 - bit 1 and bit 4. However vendor code sets only bit 4 (called
>> WUDR there)?
>>
> No they are not, AUDR on s2mps13 is bit:1 where as it is bit:4 on s2mps15.
>>
>> What exactly is necessary to update alarm and time registers on S2MPS15?
>>
> As explained above in example, it is required to update the 'write alarm
> buffer(AUDR)' and 'write time buffer(WUDR)' bits in-order to get rtc
> working.

For updating time and alarm registers? Or only for updating alarm registers?

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-29 Thread Alim Akhtar 



On 10/29/2015 04:33 PM, Krzysztof Kozlowski wrote:

2015-10-29 16:47 GMT+09:00 Alim Akhtar :

Hello Krzysztof,


On 10/29/2015 11:24 AM, Krzysztof Kozlowski wrote:


On 29.10.2015 13:58, Alim Akhtar wrote:


RTC found in s2mps15 is almost same as one found on s2mps14/13
with few differences in RTC_UPDATE register fields, like
bit fields are changed for WUDR and AUDR.
This patch add required changes to enable s2mps15 rtc timer.

Cc: Alexandre Belloni 
Signed-off-by: Alim Akhtar 
---
   drivers/rtc/rtc-s5m.c   |   20 ++--
   include/linux/mfd/samsung/rtc.h |4 
   2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index f2504b4eef34..0d106a99958f 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -188,6 +188,7 @@ static inline int
s5m_check_peding_alarm_interrupt(struct s5m_rtc_info *info,
 ret = regmap_read(info->regmap, S5M_RTC_STATUS, );
 val &= S5M_ALARM0_STATUS;
 break;
+   case S2MPS15X:
 case S2MPS14X:
 case S2MPS13X:
 ret = regmap_read(info->s5m87xx->regmap_pmic,
S2MPS14_REG_ST2,
@@ -223,6 +224,9 @@ static inline int s5m8767_rtc_set_time_reg(struct
s5m_rtc_info *info)
 if (info->device_type == S5M8763X || info->device_type ==
S5M8767X)
 data |= S5M_RTC_TIME_EN_MASK;

+   if (info->device_type == S2MPS15X)
+   data |= S2MPS15_RTC_WUDR_MASK;
+




You are setting here bit 1 and bit 4. However vendor code sets only bit
4 (called WUDR there). That's confusing. Why the difference? Or maybe I
am looking at wrong vendor tree (SM-G920F)?


Sorry, I don't have access to vendor code that you are looking into, so
won't be able to comment on that.


Everyone have the access. It is on opensource.samsung.com, as required by GPL.


Ok, I didn't check there.


I am testing this patch before sending them, what I have found is if you
don't update WUDR the time does not changes in rtc.
e.g.
if you don't do above changes then you will see below:
-
# date --set="Oct 29 14:10:40 2015" 
Thu Oct 29 14:10:40 UTC 2015
#
# hwclock --systohc 
# hwclock 
Thu Oct 29 12:52:32 2015  .922889 seconds

which is not excepted.

And with the above change I see:


  # date --set="Oct 29 14:30:10 2015" 
Thu Oct 29 14:30:10 UTC 2015
# date
Thu Oct 29 14:30:12 UTC 2015
# hwclock --systohc 
# hwclock 
Thu Oct 29 14:30:21 2015  .333006 seconds


Which is as expected.


Okay, but I said that vendor is setting only WUDR which is bit 4. You

bit:4 is AUDR (for alarm). not WUDR and bit: 1 is WDUR(for timer)

are setting bit 1 and bit 4. Your reply - about the need of setting
WUDR (bit 4 I guess) - does not explain my concerns. Do you have to
set bit 1?


Yes, I think we need to set both for timer and alarm.
I have send you a snapshot of the rtc_update register in UM via internal 
email. PTAL.



 ret = regmap_write(info->regmap, info->regs->rtc_udr_update,
data);
 if (ret < 0) {
 dev_err(info->dev, "failed to write update reg(%d)\n",
ret);
@@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct
s5m_rtc_info *info)
 case S5M8767X:
 data &= ~S5M_RTC_TIME_EN_MASK;
 break;
+   case S2MPS15X:
+   data |= S2MPS15_RTC_AUDR_MASK;
+   break;
 case S2MPS14X:
 data |= S2MPS_RTC_RUDR_MASK;
 break;



Another difference: you are setting here exactly the same values as
S2MPS13 - bit 1 and bit 4. However vendor code sets only bit 4 (called
WUDR there)?


No they are not, AUDR on s2mps13 is bit:1 where as it is bit:4 on s2mps15.


What exactly is necessary to update alarm and time registers on S2MPS15?


As explained above in example, it is required to update the 'write alarm
buffer(AUDR)' and 'write time buffer(WUDR)' bits in-order to get rtc
working.


For updating time and alarm registers? Or only for updating alarm registers?


example was only for the timer register.


Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-28 Thread Krzysztof Kozlowski 
On 29.10.2015 13:58, Alim Akhtar wrote:
> RTC found in s2mps15 is almost same as one found on s2mps14/13
> with few differences in RTC_UPDATE register fields, like
> bit fields are changed for WUDR and AUDR.
> This patch add required changes to enable s2mps15 rtc timer.
> 
> Cc: Alexandre Belloni 
> Signed-off-by: Alim Akhtar 
> ---
>  drivers/rtc/rtc-s5m.c   |   20 ++--
>  include/linux/mfd/samsung/rtc.h |4 
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index f2504b4eef34..0d106a99958f 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -188,6 +188,7 @@ static inline int s5m_check_peding_alarm_interrupt(struct 
> s5m_rtc_info *info,
>   ret = regmap_read(info->regmap, S5M_RTC_STATUS, );
>   val &= S5M_ALARM0_STATUS;
>   break;
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   ret = regmap_read(info->s5m87xx->regmap_pmic, S2MPS14_REG_ST2,
> @@ -223,6 +224,9 @@ static inline int s5m8767_rtc_set_time_reg(struct 
> s5m_rtc_info *info)
>   if (info->device_type == S5M8763X || info->device_type == S5M8767X)
>   data |= S5M_RTC_TIME_EN_MASK;
>  
> + if (info->device_type == S2MPS15X)
> + data |= S2MPS15_RTC_WUDR_MASK;
> +


You are setting here bit 1 and bit 4. However vendor code sets only bit
4 (called WUDR there). That's confusing. Why the difference? Or maybe I
am looking at wrong vendor tree (SM-G920F)?

>   ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
>   if (ret < 0) {
>   dev_err(info->dev, "failed to write update reg(%d)\n", ret);
> @@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
> s5m_rtc_info *info)
>   case S5M8767X:
>   data &= ~S5M_RTC_TIME_EN_MASK;
>   break;
> + case S2MPS15X:
> + data |= S2MPS15_RTC_AUDR_MASK;
> + break;
>   case S2MPS14X:
>   data |= S2MPS_RTC_RUDR_MASK;
>   break;

Another difference: you are setting here exactly the same values as
S2MPS13 - bit 1 and bit 4. However vendor code sets only bit 4 (called
WUDR there)?

What exactly is necessary to update alarm and time registers on S2MPS15?

> @@ -275,7 +282,6 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
> s5m_rtc_info *info)
>   if (info->device_type == S2MPS13X)
>   regmap_update_bits(info->regmap, info->regs->rtc_udr_update,
>  S2MPS13_RTC_AUDR_MASK, 0);
> -

This is not related to the patch.

Best regards,
Krzysztof


>   return ret;
>  }
>  
> @@ -317,7 +323,8 @@ static int s5m_rtc_read_time(struct device *dev, struct 
> rtc_time *tm)
>   u8 data[info->regs->regs_count];
>   int ret;
>  
> - if (info->device_type == S2MPS14X || info->device_type == S2MPS13X) {
> + if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
> + info->device_type == S2MPS13X) {
>   ret = regmap_update_bits(info->regmap,
>   info->regs->rtc_udr_update,
>   S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
> @@ -339,6 +346,7 @@ static int s5m_rtc_read_time(struct device *dev, struct 
> rtc_time *tm)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   s5m8767_data_to_tm(data, tm, info->rtc_24hr_mode);
> @@ -366,6 +374,7 @@ static int s5m_rtc_set_time(struct device *dev, struct 
> rtc_time *tm)
>   s5m8763_tm_to_data(tm, data);
>   break;
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   ret = s5m8767_tm_to_data(tm, data);
> @@ -414,6 +423,7 @@ static int s5m_rtc_read_alarm(struct device *dev, struct 
> rtc_wkalrm *alrm)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   s5m8767_data_to_tm(data, >time, info->rtc_24hr_mode);
> @@ -463,6 +473,7 @@ static int s5m_rtc_stop_alarm(struct s5m_rtc_info *info)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   for (i = 0; i < info->regs->regs_count; i++)
> @@ -508,6 +519,7 @@ static int s5m_rtc_start_alarm(struct s5m_rtc_info *info)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   data[RTC_SEC] |= ALARM_ENABLE_MASK;
> @@ -548,6 +560,7 @@ static int s5m_rtc_set_alarm(struct device *dev, struct 
> rtc_wkalrm *alrm)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   s5m8767_tm_to_data(>time, data);
> @@ -631,6 +644,7 @@ static int s5m8767_rtc_init_reg(struct s5m_rtc_info *info)

Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-28 Thread Alim Akhtar 



On 10/29/2015 10:37 AM, Krzysztof Kozlowski wrote:

On 29.10.2015 13:58, Alim Akhtar wrote:

RTC found in s2mps15 is almost same as one found on s2mps14/13
with few differences in RTC_UPDATE register fields, like
bit fields are changed for WUDR and AUDR.
This patch add required changes to enable s2mps15 rtc timer.

Cc: Alexandre Belloni 
Signed-off-by: Alim Akhtar 
---
  drivers/rtc/rtc-s5m.c   |   20 ++--
  include/linux/mfd/samsung/rtc.h |4 
  2 files changed, 22 insertions(+), 2 deletions(-)



Oh, this is totally different than previous patch and changelog does not
mention these changes.

I am still learning to make thing more correct in first attempt, these 
input will help :-), thanks.



Was the previous RTC patch tested?

Yes, it was tested, but the coverage was limited to wakealarm.

Now I have tested wakealarm and setting hw clock date/time using hwclock 
utility, setting system time/date to rtc and visa-versa, both works as 
expected.



Here the AUDR and WUDR seem to be reversed... oh, that is strange. :)

Yah, indeed this was a surprised for me also, but this how it is 
implemented in s2mps15.



Best regards,
Krzysztof



diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index f2504b4eef34..0d106a99958f 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -188,6 +188,7 @@ static inline int s5m_check_peding_alarm_interrupt(struct 
s5m_rtc_info *info,
ret = regmap_read(info->regmap, S5M_RTC_STATUS, );
val &= S5M_ALARM0_STATUS;
break;
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
ret = regmap_read(info->s5m87xx->regmap_pmic, S2MPS14_REG_ST2,
@@ -223,6 +224,9 @@ static inline int s5m8767_rtc_set_time_reg(struct 
s5m_rtc_info *info)
if (info->device_type == S5M8763X || info->device_type == S5M8767X)
data |= S5M_RTC_TIME_EN_MASK;

+   if (info->device_type == S2MPS15X)
+   data |= S2MPS15_RTC_WUDR_MASK;
+
ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
if (ret < 0) {
dev_err(info->dev, "failed to write update reg(%d)\n", ret);
@@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
s5m_rtc_info *info)
case S5M8767X:
data &= ~S5M_RTC_TIME_EN_MASK;
break;
+   case S2MPS15X:
+   data |= S2MPS15_RTC_AUDR_MASK;
+   break;
case S2MPS14X:
data |= S2MPS_RTC_RUDR_MASK;
break;
@@ -275,7 +282,6 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
s5m_rtc_info *info)
if (info->device_type == S2MPS13X)
regmap_update_bits(info->regmap, info->regs->rtc_udr_update,
   S2MPS13_RTC_AUDR_MASK, 0);
-
return ret;
  }

@@ -317,7 +323,8 @@ static int s5m_rtc_read_time(struct device *dev, struct 
rtc_time *tm)
u8 data[info->regs->regs_count];
int ret;

-   if (info->device_type == S2MPS14X || info->device_type == S2MPS13X) {
+   if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
+   info->device_type == S2MPS13X) {
ret = regmap_update_bits(info->regmap,
info->regs->rtc_udr_update,
S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
@@ -339,6 +346,7 @@ static int s5m_rtc_read_time(struct device *dev, struct 
rtc_time *tm)
break;

case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
s5m8767_data_to_tm(data, tm, info->rtc_24hr_mode);
@@ -366,6 +374,7 @@ static int s5m_rtc_set_time(struct device *dev, struct 
rtc_time *tm)
s5m8763_tm_to_data(tm, data);
break;
case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
ret = s5m8767_tm_to_data(tm, data);
@@ -414,6 +423,7 @@ static int s5m_rtc_read_alarm(struct device *dev, struct 
rtc_wkalrm *alrm)
break;

case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
s5m8767_data_to_tm(data, >time, info->rtc_24hr_mode);
@@ -463,6 +473,7 @@ static int s5m_rtc_stop_alarm(struct s5m_rtc_info *info)
break;

case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
for (i = 0; i < info->regs->regs_count; i++)
@@ -508,6 +519,7 @@ static int s5m_rtc_start_alarm(struct s5m_rtc_info *info)
break;

case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
data[RTC_SEC] |= ALARM_ENABLE_MASK;
@@ -548,6 +560,7 @@ static int s5m_rtc_set_alarm(struct device *dev, struct 
rtc_wkalrm *alrm)
break;

case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:

Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-28 Thread Krzysztof Kozlowski 
On 29.10.2015 13:58, Alim Akhtar wrote:
> RTC found in s2mps15 is almost same as one found on s2mps14/13
> with few differences in RTC_UPDATE register fields, like
> bit fields are changed for WUDR and AUDR.
> This patch add required changes to enable s2mps15 rtc timer.
> 
> Cc: Alexandre Belloni 
> Signed-off-by: Alim Akhtar 
> ---
>  drivers/rtc/rtc-s5m.c   |   20 ++--
>  include/linux/mfd/samsung/rtc.h |4 
>  2 files changed, 22 insertions(+), 2 deletions(-)


Oh, this is totally different than previous patch and changelog does not
mention these changes.

Was the previous RTC patch tested?
Here the AUDR and WUDR seem to be reversed... oh, that is strange. :)

Best regards,
Krzysztof

> 
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index f2504b4eef34..0d106a99958f 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -188,6 +188,7 @@ static inline int s5m_check_peding_alarm_interrupt(struct 
> s5m_rtc_info *info,
>   ret = regmap_read(info->regmap, S5M_RTC_STATUS, );
>   val &= S5M_ALARM0_STATUS;
>   break;
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   ret = regmap_read(info->s5m87xx->regmap_pmic, S2MPS14_REG_ST2,
> @@ -223,6 +224,9 @@ static inline int s5m8767_rtc_set_time_reg(struct 
> s5m_rtc_info *info)
>   if (info->device_type == S5M8763X || info->device_type == S5M8767X)
>   data |= S5M_RTC_TIME_EN_MASK;
>  
> + if (info->device_type == S2MPS15X)
> + data |= S2MPS15_RTC_WUDR_MASK;
> +
>   ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
>   if (ret < 0) {
>   dev_err(info->dev, "failed to write update reg(%d)\n", ret);
> @@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
> s5m_rtc_info *info)
>   case S5M8767X:
>   data &= ~S5M_RTC_TIME_EN_MASK;
>   break;
> + case S2MPS15X:
> + data |= S2MPS15_RTC_AUDR_MASK;
> + break;
>   case S2MPS14X:
>   data |= S2MPS_RTC_RUDR_MASK;
>   break;
> @@ -275,7 +282,6 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
> s5m_rtc_info *info)
>   if (info->device_type == S2MPS13X)
>   regmap_update_bits(info->regmap, info->regs->rtc_udr_update,
>  S2MPS13_RTC_AUDR_MASK, 0);
> -
>   return ret;
>  }
>  
> @@ -317,7 +323,8 @@ static int s5m_rtc_read_time(struct device *dev, struct 
> rtc_time *tm)
>   u8 data[info->regs->regs_count];
>   int ret;
>  
> - if (info->device_type == S2MPS14X || info->device_type == S2MPS13X) {
> + if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
> + info->device_type == S2MPS13X) {
>   ret = regmap_update_bits(info->regmap,
>   info->regs->rtc_udr_update,
>   S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
> @@ -339,6 +346,7 @@ static int s5m_rtc_read_time(struct device *dev, struct 
> rtc_time *tm)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   s5m8767_data_to_tm(data, tm, info->rtc_24hr_mode);
> @@ -366,6 +374,7 @@ static int s5m_rtc_set_time(struct device *dev, struct 
> rtc_time *tm)
>   s5m8763_tm_to_data(tm, data);
>   break;
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   ret = s5m8767_tm_to_data(tm, data);
> @@ -414,6 +423,7 @@ static int s5m_rtc_read_alarm(struct device *dev, struct 
> rtc_wkalrm *alrm)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   s5m8767_data_to_tm(data, >time, info->rtc_24hr_mode);
> @@ -463,6 +473,7 @@ static int s5m_rtc_stop_alarm(struct s5m_rtc_info *info)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   for (i = 0; i < info->regs->regs_count; i++)
> @@ -508,6 +519,7 @@ static int s5m_rtc_start_alarm(struct s5m_rtc_info *info)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   data[RTC_SEC] |= ALARM_ENABLE_MASK;
> @@ -548,6 +560,7 @@ static int s5m_rtc_set_alarm(struct device *dev, struct 
> rtc_wkalrm *alrm)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   s5m8767_tm_to_data(>time, data);
> @@ -631,6 +644,7 @@ static int s5m8767_rtc_init_reg(struct s5m_rtc_info *info)
>   ret = regmap_raw_write(info->regmap, S5M_ALARM0_CONF, data, 2);
>   break;
>  
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   data[0] = (0 << BCD_EN_SHIFT) | (1 << MODEL24_SHIFT);
> @@

Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-28 Thread Krzysztof Kozlowski 
On 29.10.2015 13:58, Alim Akhtar wrote:
> RTC found in s2mps15 is almost same as one found on s2mps14/13
> with few differences in RTC_UPDATE register fields, like
> bit fields are changed for WUDR and AUDR.
> This patch add required changes to enable s2mps15 rtc timer.
> 
> Cc: Alexandre Belloni 
> Signed-off-by: Alim Akhtar 
> ---
>  drivers/rtc/rtc-s5m.c   |   20 ++--
>  include/linux/mfd/samsung/rtc.h |4 
>  2 files changed, 22 insertions(+), 2 deletions(-)


Oh, this is totally different than previous patch and changelog does not
mention these changes.

Was the previous RTC patch tested?
Here the AUDR and WUDR seem to be reversed... oh, that is strange. :)

Best regards,
Krzysztof

> 
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index f2504b4eef34..0d106a99958f 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -188,6 +188,7 @@ static inline int s5m_check_peding_alarm_interrupt(struct 
> s5m_rtc_info *info,
>   ret = regmap_read(info->regmap, S5M_RTC_STATUS, );
>   val &= S5M_ALARM0_STATUS;
>   break;
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   ret = regmap_read(info->s5m87xx->regmap_pmic, S2MPS14_REG_ST2,
> @@ -223,6 +224,9 @@ static inline int s5m8767_rtc_set_time_reg(struct 
> s5m_rtc_info *info)
>   if (info->device_type == S5M8763X || info->device_type == S5M8767X)
>   data |= S5M_RTC_TIME_EN_MASK;
>  
> + if (info->device_type == S2MPS15X)
> + data |= S2MPS15_RTC_WUDR_MASK;
> +
>   ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
>   if (ret < 0) {
>   dev_err(info->dev, "failed to write update reg(%d)\n", ret);
> @@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
> s5m_rtc_info *info)
>   case S5M8767X:
>   data &= ~S5M_RTC_TIME_EN_MASK;
>   break;
> + case S2MPS15X:
> + data |= S2MPS15_RTC_AUDR_MASK;
> + break;
>   case S2MPS14X:
>   data |= S2MPS_RTC_RUDR_MASK;
>   break;
> @@ -275,7 +282,6 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
> s5m_rtc_info *info)
>   if (info->device_type == S2MPS13X)
>   regmap_update_bits(info->regmap, info->regs->rtc_udr_update,
>  S2MPS13_RTC_AUDR_MASK, 0);
> -
>   return ret;
>  }
>  
> @@ -317,7 +323,8 @@ static int s5m_rtc_read_time(struct device *dev, struct 
> rtc_time *tm)
>   u8 data[info->regs->regs_count];
>   int ret;
>  
> - if (info->device_type == S2MPS14X || info->device_type == S2MPS13X) {
> + if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
> + info->device_type == S2MPS13X) {
>   ret = regmap_update_bits(info->regmap,
>   info->regs->rtc_udr_update,
>   S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
> @@ -339,6 +346,7 @@ static int s5m_rtc_read_time(struct device *dev, struct 
> rtc_time *tm)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   s5m8767_data_to_tm(data, tm, info->rtc_24hr_mode);
> @@ -366,6 +374,7 @@ static int s5m_rtc_set_time(struct device *dev, struct 
> rtc_time *tm)
>   s5m8763_tm_to_data(tm, data);
>   break;
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   ret = s5m8767_tm_to_data(tm, data);
> @@ -414,6 +423,7 @@ static int s5m_rtc_read_alarm(struct device *dev, struct 
> rtc_wkalrm *alrm)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   s5m8767_data_to_tm(data, >time, info->rtc_24hr_mode);
> @@ -463,6 +473,7 @@ static int s5m_rtc_stop_alarm(struct s5m_rtc_info *info)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   for (i = 0; i < info->regs->regs_count; i++)
> @@ -508,6 +519,7 @@ static int s5m_rtc_start_alarm(struct s5m_rtc_info *info)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   data[RTC_SEC] |= ALARM_ENABLE_MASK;
> @@ -548,6 +560,7 @@ static int s5m_rtc_set_alarm(struct device *dev, struct 
> rtc_wkalrm *alrm)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   s5m8767_tm_to_data(>time, data);
> @@ -631,6 +644,7 @@ static int s5m8767_rtc_init_reg(struct s5m_rtc_info *info)
>   ret = regmap_raw_write(info->regmap, S5M_ALARM0_CONF, data, 2);
>   break;
>  
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>

Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-28 Thread Alim Akhtar 



On 10/29/2015 10:37 AM, Krzysztof Kozlowski wrote:

On 29.10.2015 13:58, Alim Akhtar wrote:

RTC found in s2mps15 is almost same as one found on s2mps14/13
with few differences in RTC_UPDATE register fields, like
bit fields are changed for WUDR and AUDR.
This patch add required changes to enable s2mps15 rtc timer.

Cc: Alexandre Belloni 
Signed-off-by: Alim Akhtar 
---
  drivers/rtc/rtc-s5m.c   |   20 ++--
  include/linux/mfd/samsung/rtc.h |4 
  2 files changed, 22 insertions(+), 2 deletions(-)



Oh, this is totally different than previous patch and changelog does not
mention these changes.

I am still learning to make thing more correct in first attempt, these 
input will help :-), thanks.



Was the previous RTC patch tested?

Yes, it was tested, but the coverage was limited to wakealarm.

Now I have tested wakealarm and setting hw clock date/time using hwclock 
utility, setting system time/date to rtc and visa-versa, both works as 
expected.



Here the AUDR and WUDR seem to be reversed... oh, that is strange. :)

Yah, indeed this was a surprised for me also, but this how it is 
implemented in s2mps15.



Best regards,
Krzysztof



diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index f2504b4eef34..0d106a99958f 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -188,6 +188,7 @@ static inline int s5m_check_peding_alarm_interrupt(struct 
s5m_rtc_info *info,
ret = regmap_read(info->regmap, S5M_RTC_STATUS, );
val &= S5M_ALARM0_STATUS;
break;
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
ret = regmap_read(info->s5m87xx->regmap_pmic, S2MPS14_REG_ST2,
@@ -223,6 +224,9 @@ static inline int s5m8767_rtc_set_time_reg(struct 
s5m_rtc_info *info)
if (info->device_type == S5M8763X || info->device_type == S5M8767X)
data |= S5M_RTC_TIME_EN_MASK;

+   if (info->device_type == S2MPS15X)
+   data |= S2MPS15_RTC_WUDR_MASK;
+
ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
if (ret < 0) {
dev_err(info->dev, "failed to write update reg(%d)\n", ret);
@@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
s5m_rtc_info *info)
case S5M8767X:
data &= ~S5M_RTC_TIME_EN_MASK;
break;
+   case S2MPS15X:
+   data |= S2MPS15_RTC_AUDR_MASK;
+   break;
case S2MPS14X:
data |= S2MPS_RTC_RUDR_MASK;
break;
@@ -275,7 +282,6 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
s5m_rtc_info *info)
if (info->device_type == S2MPS13X)
regmap_update_bits(info->regmap, info->regs->rtc_udr_update,
   S2MPS13_RTC_AUDR_MASK, 0);
-
return ret;
  }

@@ -317,7 +323,8 @@ static int s5m_rtc_read_time(struct device *dev, struct 
rtc_time *tm)
u8 data[info->regs->regs_count];
int ret;

-   if (info->device_type == S2MPS14X || info->device_type == S2MPS13X) {
+   if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
+   info->device_type == S2MPS13X) {
ret = regmap_update_bits(info->regmap,
info->regs->rtc_udr_update,
S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
@@ -339,6 +346,7 @@ static int s5m_rtc_read_time(struct device *dev, struct 
rtc_time *tm)
break;

case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
s5m8767_data_to_tm(data, tm, info->rtc_24hr_mode);
@@ -366,6 +374,7 @@ static int s5m_rtc_set_time(struct device *dev, struct 
rtc_time *tm)
s5m8763_tm_to_data(tm, data);
break;
case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
ret = s5m8767_tm_to_data(tm, data);
@@ -414,6 +423,7 @@ static int s5m_rtc_read_alarm(struct device *dev, struct 
rtc_wkalrm *alrm)
break;

case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
s5m8767_data_to_tm(data, >time, info->rtc_24hr_mode);
@@ -463,6 +473,7 @@ static int s5m_rtc_stop_alarm(struct s5m_rtc_info *info)
break;

case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
for (i = 0; i < info->regs->regs_count; i++)
@@ -508,6 +519,7 @@ static int s5m_rtc_start_alarm(struct s5m_rtc_info *info)
break;

case S5M8767X:
+   case S2MPS15X:
case S2MPS14X:
case S2MPS13X:
data[RTC_SEC] |= ALARM_ENABLE_MASK;
@@ -548,6 +560,7 @@ static int s5m_rtc_set_alarm(struct device *dev, struct 
rtc_wkalrm *alrm)
break;

case S5M8767X:
+

Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-28 Thread Krzysztof Kozlowski 
On 29.10.2015 13:58, Alim Akhtar wrote:
> RTC found in s2mps15 is almost same as one found on s2mps14/13
> with few differences in RTC_UPDATE register fields, like
> bit fields are changed for WUDR and AUDR.
> This patch add required changes to enable s2mps15 rtc timer.
> 
> Cc: Alexandre Belloni 
> Signed-off-by: Alim Akhtar 
> ---
>  drivers/rtc/rtc-s5m.c   |   20 ++--
>  include/linux/mfd/samsung/rtc.h |4 
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index f2504b4eef34..0d106a99958f 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -188,6 +188,7 @@ static inline int s5m_check_peding_alarm_interrupt(struct 
> s5m_rtc_info *info,
>   ret = regmap_read(info->regmap, S5M_RTC_STATUS, );
>   val &= S5M_ALARM0_STATUS;
>   break;
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   ret = regmap_read(info->s5m87xx->regmap_pmic, S2MPS14_REG_ST2,
> @@ -223,6 +224,9 @@ static inline int s5m8767_rtc_set_time_reg(struct 
> s5m_rtc_info *info)
>   if (info->device_type == S5M8763X || info->device_type == S5M8767X)
>   data |= S5M_RTC_TIME_EN_MASK;
>  
> + if (info->device_type == S2MPS15X)
> + data |= S2MPS15_RTC_WUDR_MASK;
> +


You are setting here bit 1 and bit 4. However vendor code sets only bit
4 (called WUDR there). That's confusing. Why the difference? Or maybe I
am looking at wrong vendor tree (SM-G920F)?

>   ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
>   if (ret < 0) {
>   dev_err(info->dev, "failed to write update reg(%d)\n", ret);
> @@ -252,6 +256,9 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
> s5m_rtc_info *info)
>   case S5M8767X:
>   data &= ~S5M_RTC_TIME_EN_MASK;
>   break;
> + case S2MPS15X:
> + data |= S2MPS15_RTC_AUDR_MASK;
> + break;
>   case S2MPS14X:
>   data |= S2MPS_RTC_RUDR_MASK;
>   break;

Another difference: you are setting here exactly the same values as
S2MPS13 - bit 1 and bit 4. However vendor code sets only bit 4 (called
WUDR there)?

What exactly is necessary to update alarm and time registers on S2MPS15?

> @@ -275,7 +282,6 @@ static inline int s5m8767_rtc_set_alarm_reg(struct 
> s5m_rtc_info *info)
>   if (info->device_type == S2MPS13X)
>   regmap_update_bits(info->regmap, info->regs->rtc_udr_update,
>  S2MPS13_RTC_AUDR_MASK, 0);
> -

This is not related to the patch.

Best regards,
Krzysztof


>   return ret;
>  }
>  
> @@ -317,7 +323,8 @@ static int s5m_rtc_read_time(struct device *dev, struct 
> rtc_time *tm)
>   u8 data[info->regs->regs_count];
>   int ret;
>  
> - if (info->device_type == S2MPS14X || info->device_type == S2MPS13X) {
> + if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
> + info->device_type == S2MPS13X) {
>   ret = regmap_update_bits(info->regmap,
>   info->regs->rtc_udr_update,
>   S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
> @@ -339,6 +346,7 @@ static int s5m_rtc_read_time(struct device *dev, struct 
> rtc_time *tm)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   s5m8767_data_to_tm(data, tm, info->rtc_24hr_mode);
> @@ -366,6 +374,7 @@ static int s5m_rtc_set_time(struct device *dev, struct 
> rtc_time *tm)
>   s5m8763_tm_to_data(tm, data);
>   break;
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   ret = s5m8767_tm_to_data(tm, data);
> @@ -414,6 +423,7 @@ static int s5m_rtc_read_alarm(struct device *dev, struct 
> rtc_wkalrm *alrm)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   s5m8767_data_to_tm(data, >time, info->rtc_24hr_mode);
> @@ -463,6 +473,7 @@ static int s5m_rtc_stop_alarm(struct s5m_rtc_info *info)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   for (i = 0; i < info->regs->regs_count; i++)
> @@ -508,6 +519,7 @@ static int s5m_rtc_start_alarm(struct s5m_rtc_info *info)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   data[RTC_SEC] |= ALARM_ENABLE_MASK;
> @@ -548,6 +560,7 @@ static int s5m_rtc_set_alarm(struct device *dev, struct 
> rtc_wkalrm *alrm)
>   break;
>  
>   case S5M8767X:
> + case S2MPS15X:
>   case S2MPS14X:
>   case S2MPS13X:
>   s5m8767_tm_to_data(>time, data);
> @@ -631,6 +644,7