| Message ID | 1411122377-10426-6-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2014-09-19 at 12:26 +0200, Javier Martinez Canillas wrote: > The function max77686_rtc_calculate_wday() is used to > calculate the day of the week to be filled in struct > rtc_time but that function only calculates the number > of bits shifted. So the ffs() function can be used to > find the first bit set instead of a special function. This isn't the same logic. Perhaps you want fls. > diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c [] > -static inline int max77686_rtc_calculate_wday(u8 shifted) > -{ > - int counter = -1; > - while (shifted) { > - shifted >>= 1; > - counter++; > - } > - return counter; > -} > - > static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm, > int rtc_24hr_mode) > { > @@ -93,7 +83,7 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm, > tm->tm_hour += 12; > } > > - tm->tm_wday = max77686_rtc_calculate_wday(data[RTC_WEEKDAY] & 0x7f); > + tm->tm_wday = ffs(data[RTC_WEEKDAY] & 0x7f) - 1;
Hello Joe, On 09/19/2014 04:39 PM, Joe Perches wrote: > On Fri, 2014-09-19 at 12:26 +0200, Javier Martinez Canillas wrote: >> The function max77686_rtc_calculate_wday() is used to >> calculate the day of the week to be filled in struct >> rtc_time but that function only calculates the number >> of bits shifted. So the ffs() function can be used to >> find the first bit set instead of a special function. > > This isn't the same logic. Perhaps you want fls. > Right, the removed function has the same logic than fls() - 1 but the value stored in data[RTC_WEEKDAY] is: data[RTC_WEEKDAY] = 1 << tm->tm_wday; so for this particular case, it doesn't matter since ffs() == fls() always. >> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c > [] >> -static inline int max77686_rtc_calculate_wday(u8 shifted) >> -{ >> - int counter = -1; >> - while (shifted) { >> - shifted >>= 1; >> - counter++; >> - } >> - return counter; >> -} >> - >> static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm, >> int rtc_24hr_mode) >> { >> @@ -93,7 +83,7 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm, >> tm->tm_hour += 12; >> } >> >> - tm->tm_wday = max77686_rtc_calculate_wday(data[RTC_WEEKDAY] & 0x7f); >> + tm->tm_wday = ffs(data[RTC_WEEKDAY] & 0x7f) - 1; I did wonder which function to use and the question is when you want to know the number of shifted bits, do you look for the first or the last bit set? Of course is the same for powers of two but I did a naive search to have an usage count: $ git grep "shift = ffs(" | wc -l 39 $ git grep "shift = fls(" | wc -l 17 so it seems that ffs() is twice more popular than fls() for this case so I decided to use ffs() but I don't have a strong opinion tbh. Best regards, Javier
On Fri, 2014-09-19 at 21:27 +0200, Javier Martinez Canillas wrote: > Hello Joe, Hello Javier. > On 09/19/2014 04:39 PM, Joe Perches wrote: > > On Fri, 2014-09-19 at 12:26 +0200, Javier Martinez Canillas wrote: > >> The function max77686_rtc_calculate_wday() is used to > >> calculate the day of the week to be filled in struct > >> rtc_time but that function only calculates the number > >> of bits shifted. So the ffs() function can be used to > >> find the first bit set instead of a special function. > > > > This isn't the same logic. Perhaps you want fls. > > > > Right, the removed function has the same logic than fls() - 1 but the value > stored in data[RTC_WEEKDAY] is: > > data[RTC_WEEKDAY] = 1 << tm->tm_wday; > > so for this particular case, it doesn't matter since ffs() == fls() always. I didn't look that far. I just wanted to show that the logic wasn't the same. Perhaps you can specify that ffs==fls in the changelog.
Hello Joe, On 09/19/2014 09:52 PM, Joe Perches wrote: > On Fri, 2014-09-19 at 21:27 +0200, Javier Martinez Canillas wrote: >> On 09/19/2014 04:39 PM, Joe Perches wrote: >> > On Fri, 2014-09-19 at 12:26 +0200, Javier Martinez Canillas wrote: >> >> The function max77686_rtc_calculate_wday() is used to >> >> calculate the day of the week to be filled in struct >> >> rtc_time but that function only calculates the number >> >> of bits shifted. So the ffs() function can be used to >> >> find the first bit set instead of a special function. >> > >> > This isn't the same logic. Perhaps you want fls. >> > >> >> Right, the removed function has the same logic than fls() - 1 but the value >> stored in data[RTC_WEEKDAY] is: >> >> data[RTC_WEEKDAY] = 1 << tm->tm_wday; >> >> so for this particular case, it doesn't matter since ffs() == fls() always. > > I didn't look that far. > > I just wanted to show that the logic wasn't the same. > No worries, thanks a lot for your feedback. Probably it would had been good to mention that in the commit message. > Perhaps you can specify that ffs==fls in the changelog. > I won't say I'm thrilled to do a whole re-spin of the series just to change that ;) Specially since I already did too many revisions and this series have been floating around for months but is up to Andrew to decide if is worth it. Best regards, Javier
diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c index b177ba1..2659bd3 100644 --- a/drivers/rtc/rtc-max77686.c +++ b/drivers/rtc/rtc-max77686.c @@ -70,16 +70,6 @@ enum MAX77686_RTC_OP { MAX77686_RTC_READ, }; -static inline int max77686_rtc_calculate_wday(u8 shifted) -{ - int counter = -1; - while (shifted) { - shifted >>= 1; - counter++; - } - return counter; -} - static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm, int rtc_24hr_mode) { @@ -93,7 +83,7 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm, tm->tm_hour += 12; } - tm->tm_wday = max77686_rtc_calculate_wday(data[RTC_WEEKDAY] & 0x7f); + tm->tm_wday = ffs(data[RTC_WEEKDAY] & 0x7f) - 1; tm->tm_mday = data[RTC_DATE] & 0x1f; tm->tm_mon = (data[RTC_MONTH] & 0x0f) - 1; tm->tm_year = (data[RTC_YEAR] & 0x7f) + 100;
The function max77686_rtc_calculate_wday() is used to calculate the day of the week to be filled in struct rtc_time but that function only calculates the number of bits shifted. So the ffs() function can be used to find the first bit set instead of a special function. Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> --- drivers/rtc/rtc-max77686.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-)