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

[Krzysztof Kozlowski]

2015-10-29 16:47 GMT+09:00 Alim Akhtar <alim.akhtar@xxxxxxxxxxx>:
> 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@xxxxxxxxxxxxxxxxxx>
>>> Signed-off-by: Alim Akhtar <alim.akhtar@xxxxxxxxxxx>
>>> ---
>>>   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.

>
> 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
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/