| Message ID | 1365868840.7445.5.camel@phoenix |
|---|---|
| State | Superseded |
| Headers | show |
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");
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");
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(-)