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

login
register
mail settings
Submitter Axel Lin
Date April 13, 2013, 4 p.m.
Message ID <1365868840.7445.5.camel@phoenix>
Download mbox | patch
Permalink /patch/236363/
State New
Headers show

Comments

Axel Lin - April 13, 2013, 4 p.m.
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.

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(-)
Jonghwa Lee - April 14, 2013, 11:57 p.m.
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.

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");

Patch

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");