rtc/rtc-spear: remove unnecessary check against rtc_valid_tm and tm2bcd
PATCH 1/1 rtc/rtc-spear: remove unnecessary check against rtc_valid_tm and tm2bcd
rtc_valid_tm is always return-EINVAL if the time doesn't represent a valid date and time and 0 if its.
so we can simply do rtc_valid_tm(tm) rather doing rtc_valid_tm(tm) != 0 checking. and also tm2bcd() does return -EINVAL if the time doesn't represent a valid date and time and 0 if its.
and also removing err because is_write_complete will return -EIO if our write to the RTC registers didn't happen and 0 if its.
---
This is only tested by compiling the code with arm-linux-gcc
/bin/arm-linux-gnueabihf-gcc --version
arm-linux-gnueabihf-gcc (crosstool-NG linaro-1.13.1-2012.05-20120523 - Linaro GCC 2012.05) 4.7.1 20120514 (prerelease)
drivers/rtc/rtc-spear.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
drivers/rtc/rtc-spear.c +5 -8
@@ -172,7 +172,7 @@ static irqreturn_t spear_rtc_irq(int irq, void *dev_id)static int tm2bcd (struct rtc_time *tm) { -if (rtc_valid_tm (tm) !=0 ) +if (rtc_valid_tm (tm))return -EINVAL ; tm->tm_sec =bin2bcd (tm->tm_sec); tm->tm_min =bin2bcd (tm->tm_min);
@@ -235,9 +235,9 @@ static int spear_rtc_read_time(struct device *dev, struct rtc_time *tm)static int spear_rtc_set_time (struct device *dev,struct rtc_time *tm) {struct spear_rtc_config *config =dev_get_drvdata (dev); -unsigned int time, date, err =0 ; +unsigned int time, date; -if (tm2bcd (tm) <0 ) +if (tm2bcd (tm))return -EINVAL ;rtc_wait_not_busy (config);
@@ -247,11 +247,8 @@ static int spear_rtc_set_time(struct device *dev, struct rtc_time *tm) (tm->tm_year <<YEAR_SHIFT );writel (time, config->ioaddr +TIME_REG );writel (date, config->ioaddr +DATE_REG ); - err =is_write_complete (config); -if (err <0 ) -return err; -return 0 ; +return is_write_complete (config); }/*
@@ -297,7 +294,7 @@ static int spear_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)struct spear_rtc_config *config =dev_get_drvdata (dev);unsigned int time, date, err =0 ; -if (tm2bcd (&alm->time) <0 ) +if (tm2bcd (&alm->time))return -EINVAL ;rtc_wait_not_busy (config);
Signature · 1 line
1.7.9.5
Hi Devendra,
Quoted 27 lines · Devendra Naga
rtc_valid_tmis always return-EINVALif the time doesn't represent a valid date and time and 0 if its.so we can simply do
rtc_valid_tm(tm)rather doingrtc_valid_tm(tm)!= 0 checking. and alsotm2bcd()does return-EINVALif the time doesn't represent a valid date and time and 0 if its.and also removing err because
is_write_completewill return-EIOif our write to the RTC registers didn't happen and 0 if its.Signed-off-by Devendra Naga <[email protected]>---
This is only tested by compiling the code with arm-linux-gcc
/bin/arm-linux-gnueabihf-gcc--versionarm-linux-gnueabihf-gcc (crosstool-NG linaro-1.13.1-2012.05-20120523 - Linaro GCC 2012.05) 4.7.1 20120514 (prerelease)
drivers/rtc/rtc-spear.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)drivers/rtc/rtc-spear.c +0
-0
drivers/rtc/rtc-spear.c static irqreturn_t spear_rtc_irq(int irq, void *dev_id) · line 176
Quoted 6 lines
drivers/rtc/rtc-spear.c +1
-1@@ -172,7 +172,7 @@ static irqreturn_t spear_rtc_irq(int irq, void *dev_id)static int tm2bcd (struct rtc_time *tm) { -if (rtc_valid_tm (tm) !=0 ) +if (rtc_valid_tm (tm))return -EINVAL ;
The name of this routine conflicts with its behavior (In my opinion :) ) rtc_valid_tm() should have returned
- false or true
OR
less than zero (invalid) or greater than zero (valid)
Probably second one is opted for it. so, it would make more sense to have something like
if (rtc_valid_tm() < 0)
return -EINVAL;
instead of what you went for. Because that looked incorrect to me from readability point of view. i.e. if rtc_valid_tm, that's an error. What do you say?
drivers/rtc/rtc-spear.c static int spear_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 241
Quoted 12 lines
drivers/rtc/rtc-spear.c +2
-2tm->tm_sec =bin2bcd (tm->tm_sec); tm->tm_min =bin2bcd (tm->tm_min);@@ -235,9 +235,9 @@ static int spear_rtc_read_time(struct device *dev, struct rtc_time *tm)static int spear_rtc_set_time (struct device *dev,struct rtc_time *tm) {struct spear_rtc_config *config =dev_get_drvdata (dev); -unsigned int time, date, err =0 ; +unsigned int time, date; -if (tm2bcd (tm) <0 ) +if (tm2bcd (tm))return -EINVAL ;
Same applied here.
drivers/rtc/rtc-spear.c static int spear_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 252
Quoted 12 lines
drivers/rtc/rtc-spear.c +1
-4rtc_wait_not_busy (config);@@ -247,11 +247,8 @@ static int spear_rtc_set_time(struct device *dev, struct rtc_time *tm) (tm->tm_year <<YEAR_SHIFT );writel (time, config->ioaddr +TIME_REG );writel (date, config->ioaddr +DATE_REG ); - err =is_write_complete (config); -if (err <0 ) -return err; -return 0 ; +return is_write_complete (config); }
This one must have returned true or false, because of nature of its name. So, still checking for < 0 is better.
--
Viresh
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Hi Viresh,
Quoted 44 lines · viresh kumar
Hi Devendra,
Quoted 27 lines · Devendra Naga
rtc_valid_tmis always return-EINVALif the time doesn't represent a valid date and time and 0 if its.so we can simply do
rtc_valid_tm(tm)rather doingrtc_valid_tm(tm)!= 0 checking. and alsotm2bcd()does return-EINVALif the time doesn't represent a valid date and time and 0 if its.and also removing err because
is_write_completewill return-EIOif our write to the RTC registers didn't happen and 0 if its.Signed-off-by Devendra Naga <[email protected]>---
This is only tested by compiling the code with arm-linux-gcc
/bin/arm-linux-gnueabihf-gcc--versionarm-linux-gnueabihf-gcc (crosstool-NG linaro-1.13.1-2012.05-20120523 - Linaro GCC 2012.05) 4.7.1 20120514 (prerelease)
drivers/rtc/rtc-spear.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)drivers/rtc/rtc-spear.c +0
-0drivers/rtc/rtc-spear.c static irqreturn_t spear_rtc_irq(int irq, void *dev_id) · line 176
Quoted 6 lines
drivers/rtc/rtc-spear.c +1
-1@@ -172,7 +172,7 @@ static irqreturn_t spear_rtc_irq(int irq, void *dev_id) static int tm2bcd(struct rtc_time *tm) { - if (rtc_valid_tm(tm) != 0) + if (rtc_valid_tm(tm)) return -EINVAL;The name of this routine conflicts with its behavior (In my opinion :) )
rtc_valid_tm()should have returned
- false or true
OR
less than zero (invalid) or greater than zero (valid)
Quoted 8 lines
Probably second one is opted for it. so, it would make more sense to have something like
if (
rtc_valid_tm()< 0)
return-EINVAL;instead of what you went for. Because that looked incorrect to me from readability point of view. i.e. if
rtc_valid_tm, that's an error. What do you say?
You are right. having rtc_valid_tm() < 0 makes sense.
Quoted 24 lines
drivers/rtc/rtc-spear.c static int spear_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 241
Quoted 12 lines
drivers/rtc/rtc-spear.c +2
-2tm->tm_sec = bin2bcd(tm->tm_sec); tm->tm_min = bin2bcd(tm->tm_min);@@ -235,9 +235,9 @@ static int spear_rtc_read_time(struct device *dev, struct rtc_time *tm) static int spear_rtc_set_time(struct device *dev, struct rtc_time *tm) { struct spear_rtc_config *config = dev_get_drvdata(dev); - unsigned int time, date, err = 0; + unsigned int time, date; - if (tm2bcd(tm) < 0) + if (tm2bcd(tm)) return -EINVAL;Same applied here.
Quoted 9 lines
drivers/rtc/rtc-spear.c +0
-3rtc_wait_not_busy(config);@@ -247,11 +247,8 @@ static int spear_rtc_set_time(struct device *dev, struct rtc_time *tm) (tm->tm_year << YEAR_SHIFT); writel(time, config->ioaddr + TIME_REG); writel(date, config->ioaddr + DATE_REG); - err = is_write_complete(config); - if (err < 0) - return err;
Quoted 7 lines
drivers/rtc/rtc-spear.c static int spear_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 252
Quoted 3 lines
drivers/rtc/rtc-spear.c +1
-1- return 0; + return is_write_complete(config); }This one must have returned true or false, because of nature of its name. So, still checking for < 0 is better.
right. but the function doesn't do as it expected to return true or false. anyway, it returns -EIO in fail case and 0 in success.
The original code was easier to read than with the current code.
I think its better to leave this driver with out doing these kinds of changes.
Quoted 3 lines
--
Viresh
Thanks,
Devendra.
Quoted 2 lines
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Quoted 11 lines · Devendra Naga
rtc_valid_tmis always return-EINVALif the time doesn't represent a valid date and time and 0 if its.so we can simply do
rtc_valid_tm(tm)rather doingrtc_valid_tm(tm)!= 0 checking. and alsotm2bcd()does return-EINVALif the time doesn't represent a valid date and time and 0 if its.and also removing err because
is_write_completewill return-EIOif our write to the RTC registers didn't happen and 0 if its.Signed-off-by Devendra Naga<[email protected]>
~Rajeev