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

[Alim Akhtar]

Hi Krzysztof,

On Thu, Oct 29, 2015 at 5:05 PM, Krzysztof Kozlowski
<k.kozlowski@xxxxxxxxxxx> wrote:
> 2015-10-29 20:20 GMT+09:00 Alim Akhtar <alim.akhtar@xxxxxxxxxxx>:
>>>> 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)
>
> 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 majordomo@xxxxxxxxxxxxxxx
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/