Patchwork Re: [PATCH RFT] rtc: max8997: Fix bit settings for enable smpl

login
register
mail settings
Submitter Jingoo Han
Date April 15, 2013, 1:02 a.m.
Message ID <002101ce3974$e9910cf0$bcb326d0$%han@samsung.com>
Download mbox | patch
Permalink /patch/236483/
State New
Headers show

Comments

Jingoo Han - April 15, 2013, 1:02 a.m.
On Monday, April 15, 2013 8:58 AM, Jonghwa Leewrote:
> 
> Hi,
> On 2013년 04월 14일 01:00, Axel Lin wrote:
> 
> > Current code looks wrong to me because it writes 0 to SMPLT_MASK bits
> > for both 'enable' and 'disable' cases.
> > It looks to me that it should write (3 << SMPLT_SHIFT) to SMPLT_MASK
> > bits when enable smpl.
> >
> 
> 
> Above all, you mentioned SMPL but below fixings are related to WTSR.
> Anyway, it can be helpful both of them. SMPLT_SHIFT and WTSR_SHIFT is not for
> enable bits but is for the timer. In control register we can set enable/disable
> bit and also timer's time for each of them. In current codes, it set 0 for SMPL
> timer, and 3 for WTSR timer,  which means that allowing 0.5secs for SMPL timer
> 1000msecs for WTSR timer.
> 
> Register map for WTSR/SMPL control register
> b'0~1 : WTSR timer
> b'2~3 : SMPL timer
> b'6   : Enable bit for WTSR
> b'7   : Enable bit for SMPL
> 
> So, I think the codes aren't wrong. But I agree that I might give you confusion
> because of the mask macro's name.

To enhance the readability, the following macros would be better.

> Thanks,
> Jonghwa.
> 
> > I think it is less error prone by setting val with:
> > 	val = enable ? mask : 0;
> >
> > Signed-off-by: Axel Lin <axel.lin@ingics.com>
> > ---
> > Hi,
> > I don't have the datasheet, so I'm not really sure this patch is correct or not.
> > I'd appreciate if someone can review and test this patch.
> >
> > Regards,
> > Axel.
> >  drivers/rtc/rtc-max8997.c |   12 ++----------
> >  1 file changed, 2 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-max8997.c b/drivers/rtc/rtc-max8997.c
> > index 5693619..46eeee0 100644
> > --- a/drivers/rtc/rtc-max8997.c
> > +++ b/drivers/rtc/rtc-max8997.c
> > @@ -375,12 +375,8 @@ static void max8997_rtc_enable_wtsr(struct max8997_rtc_info *info, bool enable)
> >  	if (!wtsr_en)
> >  		return;
> >
> > -	if (enable)
> > -		val = (1 << WTSR_EN_SHIFT) | (3 << WTSRT_SHIFT);
> > -	else
> > -		val = 0;
> > -
> >  	mask = WTSR_EN_MASK | WTSRT_MASK;
> > +	val = enable ? mask : 0;
> >
> >  	dev_info(info->dev, "%s: %s WTSR\n", __func__,
> >  			enable ? "enable" : "disable");
> > @@ -403,12 +399,8 @@ static void max8997_rtc_enable_smpl(struct max8997_rtc_info *info, bool enable)
> >  	if (!smpl_en)
> >  		return;
> >
> > -	if (enable)
> > -		val = (1 << SMPL_EN_SHIFT) | (0 << SMPLT_SHIFT);
> > -	else
> > -		val = 0;
> > -
> >  	mask = SMPL_EN_MASK | SMPLT_MASK;
> > +	val = enable ? mask : 0;
> >
> >  	dev_info(info->dev, "%s: %s SMPL\n", __func__,
> >  			enable ? "enable" : "disable");
Jonghwa Lee - April 15, 2013, 1:25 a.m.
Hi,
On 2013년 04월 15일 10:02, Jingoo Han wrote:

> On Monday, April 15, 2013 8:58 AM, Jonghwa Leewrote:
>>
>> Hi,
>> On 2013년 04월 14일 01:00, Axel Lin wrote:
>>
>>> Current code looks wrong to me because it writes 0 to SMPLT_MASK bits
>>> for both 'enable' and 'disable' cases.
>>> It looks to me that it should write (3 << SMPLT_SHIFT) to SMPLT_MASK
>>> bits when enable smpl.
>>>
>>
>>
>> Above all, you mentioned SMPL but below fixings are related to WTSR.
>> Anyway, it can be helpful both of them. SMPLT_SHIFT and WTSR_SHIFT is not for
>> enable bits but is for the timer. In control register we can set enable/disable
>> bit and also timer's time for each of them. In current codes, it set 0 for SMPL
>> timer, and 3 for WTSR timer,  which means that allowing 0.5secs for SMPL timer
>> 1000msecs for WTSR timer.
>>
>> Register map for WTSR/SMPL control register
>> b'0~1 : WTSR timer
>> b'2~3 : SMPL timer
>> b'6   : Enable bit for WTSR
>> b'7   : Enable bit for SMPL
>>
>> So, I think the codes aren't wrong. But I agree that I might give you confusion
>> because of the mask macro's name.
> 
> To enhance the readability, the following macros would be better.
> 


Yes, it looks better. :)

Thanks,
Jonghwa

> --- a/drivers/rtc/rtc-max8997.c
> +++ b/drivers/rtc/rtc-max8997.c
> @@ -376,9 +376,9 @@ static void max8997_rtc_enable_wtsr(struct max8997_rtc_info *info, bool enable)
>                 return;
> 
>         if (enable)
> -               val = (1 << WTSR_EN_SHIFT) | (3 << WTSRT_SHIFT);
> +               val = WTSR_ENABLE | WTSR_TIMER(3);
>         else
> -               val = 0;
> +               val = WTSR_DISABLE;
> 
>         mask = WTSR_EN_MASK | WTSRT_MASK;
> 
> @@ -404,9 +404,9 @@ static void max8997_rtc_enable_smpl(struct max8997_rtc_info *info, bool enable)
>                 return;
> 
>         if (enable)
> -               val = (1 << SMPL_EN_SHIFT) | (0 << SMPLT_SHIFT);
> +               val = SMPL_ENABLE | SMPL_TIMER(0);
>         else
> -               val = 0;
> +               val = SMPL_DISABLE;
> 
> 
>>
>> Thanks,
>> Jonghwa.
>>
>>> I think it is less error prone by setting val with:
>>> 	val = enable ? mask : 0;
>>>
>>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
>>> ---
>>> Hi,
>>> I don't have the datasheet, so I'm not really sure this patch is correct or not.
>>> I'd appreciate if someone can review and test this patch.
>>>
>>> Regards,
>>> Axel.
>>>  drivers/rtc/rtc-max8997.c |   12 ++----------
>>>  1 file changed, 2 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/rtc/rtc-max8997.c b/drivers/rtc/rtc-max8997.c
>>> index 5693619..46eeee0 100644
>>> --- a/drivers/rtc/rtc-max8997.c
>>> +++ b/drivers/rtc/rtc-max8997.c
>>> @@ -375,12 +375,8 @@ static void max8997_rtc_enable_wtsr(struct max8997_rtc_info *info, bool enable)
>>>  	if (!wtsr_en)
>>>  		return;
>>>
>>> -	if (enable)
>>> -		val = (1 << WTSR_EN_SHIFT) | (3 << WTSRT_SHIFT);
>>> -	else
>>> -		val = 0;
>>> -
>>>  	mask = WTSR_EN_MASK | WTSRT_MASK;
>>> +	val = enable ? mask : 0;
>>>
>>>  	dev_info(info->dev, "%s: %s WTSR\n", __func__,
>>>  			enable ? "enable" : "disable");
>>> @@ -403,12 +399,8 @@ static void max8997_rtc_enable_smpl(struct max8997_rtc_info *info, bool enable)
>>>  	if (!smpl_en)
>>>  		return;
>>>
>>> -	if (enable)
>>> -		val = (1 << SMPL_EN_SHIFT) | (0 << SMPLT_SHIFT);
>>> -	else
>>> -		val = 0;
>>> -
>>>  	mask = SMPL_EN_MASK | SMPLT_MASK;
>>> +	val = enable ? mask : 0;
>>>
>>>  	dev_info(info->dev, "%s: %s SMPL\n", __func__,
>>>  			enable ? "enable" : "disable");
> 
> 
>
Axel Lin - April 15, 2013, 1:56 a.m.
2013/4/15 Jingoo Han <jg1.han@samsung.com>:
> On Monday, April 15, 2013 8:58 AM, Jonghwa Leewrote:
>>
>> Hi,
>> On 2013년 04월 14일 01:00, Axel Lin wrote:
>>
>> > Current code looks wrong to me because it writes 0 to SMPLT_MASK bits
>> > for both 'enable' and 'disable' cases.
>> > It looks to me that it should write (3 << SMPLT_SHIFT) to SMPLT_MASK
>> > bits when enable smpl.
>> >
>>
>>
>> Above all, you mentioned SMPL but below fixings are related to WTSR.
>> Anyway, it can be helpful both of them. SMPLT_SHIFT and WTSR_SHIFT is not for
>> enable bits but is for the timer. In control register we can set enable/disable
>> bit and also timer's time for each of them. In current codes, it set 0 for SMPL
>> timer, and 3 for WTSR timer,  which means that allowing 0.5secs for SMPL timer
>> 1000msecs for WTSR timer.
>>
>> Register map for WTSR/SMPL control register
>> b'0~1 : WTSR timer
>> b'2~3 : SMPL timer
>> b'6   : Enable bit for WTSR
>> b'7   : Enable bit for SMPL
>>
>> So, I think the codes aren't wrong. But I agree that I might give you confusion
>> because of the mask macro's name.
>
> To enhance the readability, the following macros would be better.
>
> --- a/drivers/rtc/rtc-max8997.c
> +++ b/drivers/rtc/rtc-max8997.c
> @@ -376,9 +376,9 @@ static void max8997_rtc_enable_wtsr(struct max8997_rtc_info *info, bool enable)
>                 return;
>
>         if (enable)
> -               val = (1 << WTSR_EN_SHIFT) | (3 << WTSRT_SHIFT);
> +               val = WTSR_ENABLE | WTSR_TIMER(3);
>         else
> -               val = 0;
> +               val = WTSR_DISABLE;
>
>         mask = WTSR_EN_MASK | WTSRT_MASK;
>
> @@ -404,9 +404,9 @@ static void max8997_rtc_enable_smpl(struct max8997_rtc_info *info, bool enable)
>                 return;
>
>         if (enable)
> -               val = (1 << SMPL_EN_SHIFT) | (0 << SMPLT_SHIFT);
> +               val = SMPL_ENABLE | SMPL_TIMER(0);
>         else
> -               val = 0;
> +               val = SMPL_DISABLE;

I think the original code looks ok.

The only thing missing is the documentation for SMPL timer and WTSR timer bits.

Regards,
Axel

Patch

--- a/drivers/rtc/rtc-max8997.c
+++ b/drivers/rtc/rtc-max8997.c
@@ -376,9 +376,9 @@  static void max8997_rtc_enable_wtsr(struct max8997_rtc_info *info, bool enable)
                return;

        if (enable)
-               val = (1 << WTSR_EN_SHIFT) | (3 << WTSRT_SHIFT);
+               val = WTSR_ENABLE | WTSR_TIMER(3);
        else
-               val = 0;
+               val = WTSR_DISABLE;

        mask = WTSR_EN_MASK | WTSRT_MASK;

@@ -404,9 +404,9 @@  static void max8997_rtc_enable_smpl(struct max8997_rtc_info *info, bool enable)
                return;

        if (enable)
-               val = (1 << SMPL_EN_SHIFT) | (0 << SMPLT_SHIFT);
+               val = SMPL_ENABLE | SMPL_TIMER(0);
        else
-               val = 0;
+               val = SMPL_DISABLE;


>