[v2,2/3] rtc: Add Realtek RTD1295

Commit Message

Andreas Färber Aug. 27, 2017, 11:30 a.m. UTC

Hi Alexandre,

Am 27.08.2017 um 11:13 schrieb Alexandre Belloni:
> Not much to add, apart from the spinlock issue already spotted by Andrew.
> 
> On 27/08/2017 at 02:33:27 +0200, Andreas Färber wrote:
>> +struct rtd119x_rtc {
>> +	void __iomem *base;
>> +	struct clk *clk;
>> +	struct rtc_device *rtcdev;
>> +	unsigned base_year;
> 
> checkpatch complains this should be made unsigned int

Ouch, I forgot to add my pre-commit hook in this tree and wasn't aware
of that rule yet. The RFC had it already. Fixed.

>> +	spinlock_t lock;
>> +};
>> +
>> +static inline int rtd119x_rtc_year_days(int year)
>> +{
>> +	return rtc_year_days(1, 12, year);
> 
> I'm not sure it is worth wrapping rtc_year_days
[snip]

Well, I found your rtc_year_days rather confusing and had to play with
the arguments until I got it working as expected, so I wanted an inline
function (or macro) as abstraction from my three callers.

Sadly the naming is rather confusing as I am looking for the number of
days 365..366, whereas your rtc_year_days is meant to return 0..365 and
I would just like to extract the 12th array element but need to counter
the -1 subtraction. rtc_year_days(31, 11, year) + 1 is not intuitive
either - reads like November (and ranges are not documented).

What about exporting a convenient rtc_days_in_year(year) from rtc-lib.c
accessing the table directly without rtc_year_days detour? Alternatively
an inline function in rtc.h to the same effect without the array?

Also despite is_leap_year() returning a bool || expression you keep
using it as array index or integer to add. That assumes true == 1,
whereas to my knowledge only false is guaranteed to be 0 and any
non-zero value means true. So I'd expect the code to be like this:

 EXPORT_SYMBOL(rtc_month_days);

@@ -41,7 +41,7 @@ EXPORT_SYMBOL(rtc_month_days);
  */
 int rtc_year_days(unsigned int day, unsigned int month, unsigned int year)
 {
-       return rtc_ydays[is_leap_year(year)][month] + day-1;
+       return rtc_ydays[is_leap_year(year) ? 1 : 0][month] + day-1;
 }
 EXPORT_SYMBOL(rtc_year_days);

@@ -69,7 +69,7 @@ void rtc_time64_to_tm(time64_t time, struct rtc_time *tm)
                - LEAPS_THRU_END_OF(1970 - 1);
        if (days < 0) {
                year -= 1;
-               days += 365 + is_leap_year(year);
+               days += 365 + (is_leap_year(year) ? 1 : 0);
        }
        tm->tm_year = year - 1900;
        tm->tm_yday = days + 1;

The above rtc_time64_to_tm() hunk could be converted to the proposed
rtc_days_in_year(). rtc-mcp795.c has another candidate.

By reusing rtc_year_days I elegantly avoided is_leap_year in my code,
but I could spell out 365 + (is_leap_year(year) ? 1 : 0) in my function
if preferred. I dislike duplicating expressions in code.

What do you think?

Regards,
Andreas

Comments

Andrew Lunn Aug. 27, 2017, 1:37 p.m. UTC | #1

> >> +static inline int rtd119x_rtc_year_days(int year)
> >> +{
> >> +	return rtc_year_days(1, 12, year);
> > 
> > I'm not sure it is worth wrapping rtc_year_days
> [snip]
> 
> Well, I found your rtc_year_days rather confusing and had to play with
> the arguments until I got it working as expected, so I wanted an inline
> function (or macro) as abstraction from my three callers.

I agree with that. I was wondering why 1st December was being used. I
would say this API does not do too well on Rusty's API Design
Manifesto.

It does at least get the day/month/year in the right order ;-)

	Andrew

Alexandre Belloni Aug. 27, 2017, 7:26 p.m. UTC | #2

On 27/08/2017 at 15:37:51 +0200, Andrew Lunn wrote:
> > >> +static inline int rtd119x_rtc_year_days(int year)
> > >> +{
> > >> +	return rtc_year_days(1, 12, year);
> > > 
> > > I'm not sure it is worth wrapping rtc_year_days
> > [snip]
> > 
> > Well, I found your rtc_year_days rather confusing and had to play with
> > the arguments until I got it working as expected, so I wanted an inline
> > function (or macro) as abstraction from my three callers.
> 
> I agree with that. I was wondering why 1st December was being used. I
> would say this API does not do too well on Rusty's API Design
> Manifesto.
> 

It follows the convention that tm_mon is 0-11. Making it easy to read
for anyone used to the RTC subsystem. I can agree it is not the most
intuitive one. That choice predates the RTC subsystem (made in 1996), I
won't give any name ;)

> It does at least get the day/month/year in the right order ;-)
> 
> 	Andrew

Alexandre Belloni Aug. 28, 2017, 3:50 p.m. UTC | #3

Hi,

On 27/08/2017 at 13:30:59 +0200, Andreas Färber wrote:
> Well, I found your rtc_year_days rather confusing and had to play with
> the arguments until I got it working as expected, so I wanted an inline
> function (or macro) as abstraction from my three callers.
> 
> Sadly the naming is rather confusing as I am looking for the number of
> days 365..366, whereas your rtc_year_days is meant to return 0..365 and
> I would just like to extract the 12th array element but need to counter
> the -1 subtraction. rtc_year_days(31, 11, year) + 1 is not intuitive
> either - reads like November (and ranges are not documented).
> 
> What about exporting a convenient rtc_days_in_year(year) from rtc-lib.c
> accessing the table directly without rtc_year_days detour? Alternatively
> an inline function in rtc.h to the same effect without the array?
> 

This could have been done but what you did in your v3 is fine too. It
will always be time to move that to the core later.

> Also despite is_leap_year() returning a bool || expression you keep
> using it as array index or integer to add. That assumes true == 1,
> whereas to my knowledge only false is guaranteed to be 0 and any
> non-zero value means true. So I'd expect the code to be like this:

sizeof(bool) (actually _Bool) is 1 so it can only be 0 or 1.

Patch

diff --git a/drivers/rtc/rtc-lib.c b/drivers/rtc/rtc-lib.c
index 1ae7da5cfc60..8983a408fc30 100644
--- a/drivers/rtc/rtc-lib.c
+++ b/drivers/rtc/rtc-lib.c
@@ -32,7 +32,7 @@  static const unsigned short rtc_ydays[2][13] = {
  */
 int rtc_month_days(unsigned int month, unsigned int year)
 {
-       return rtc_days_in_month[month] + (is_leap_year(year) && month
== 1);
+       return rtc_days_in_month[month] + ((is_leap_year(year) && month
== 1) ? 1 : 0);
 }