Messages in this thread | | | Date | Thu, 29 Oct 2015 16:50:18 +0530 | From | Alim Akhtar <> | Subject | Re: [PATCH v4 4/4] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC |
| |
On 10/29/2015 04:33 PM, Krzysztof Kozlowski wrote: > 2015-10-29 16:47 GMT+09:00 Alim Akhtar <alim.akhtar@samsung.com>: >> 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 <alexandre.belloni@free-electrons.com> >>>> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> >>>> --- >>>> 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); >>>> 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" <update the a new date> >> Thu Oct 29 14:10:40 UTC 2015 >> # >> # hwclock --systohc <copy system clock to hardware clock/rtc> >> # hwclock <read back 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" <update the a new date> >> Thu Oct 29 14:30:10 UTC 2015 >> # date >> Thu Oct 29 14:30:12 UTC 2015 >> # hwclock --systohc <copy system clock to hardware clock/rtc> >> # hwclock <read back 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 >
| |