Linux RTC
 help / color / mirror / Atom feed
* Re: [RFC 1/3] dt-bindings: rtc: Add Realtek RTD1295
From: Andreas Färber @ 2017-08-27 17:26 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Rob Herring, linux-rtc, Alessandro Zummo, Roc He, devicetree,
	?????????, linux-kernel, Alexandre Belloni, Mark Rutland,
	linux-arm-kernel
In-Reply-To: <20170827134729.GE13622@lunn.ch>

Hi Andrew,

Am 27.08.2017 um 15:47 schrieb Andrew Lunn:
>> Thanks. Did you read the RFC question in the cover letter as well and
>> have any comments? Downstream has an rtc-base-year = <2014>; property
>> that I had left out in this RFC and due to your ack not included in v2.
>>
>> Should we default to 2014 in the driver and add an optional base-year
>> property once we encounter a diverging device, or should we make it
>> required from the beginning? I did not spot any other rtc binding with
>> such a property and would appreciate a clarification.
> 
> From the perspective of the hardware, does it care what the base is?

The hardware stores a 15-bit number of days since Jan 1st of that base
year. It does not store the base year.

The datasheet does not name such a base year. No manual is available.

The driver needs to get it from somewhere for calculating day/month/year
in read_time and days in set_time.

The read_offset/set_offset API appeared to be something different.

> A device using a different base will initially return the wrong
> time. But once the correct time has been written back, it will be O.K.
> 
> This only becomes an issue if a device is used with different OSs,
> which have different bases. Swapping back and forth between OSs then
> becomes an issue.

These are TV boxes, so yes, I'm dual-booting into Android with a vendor
4.1 kernel and would like to keep date compatibility.

https://en.opensuse.org/HCL:Zidoo_X9S
https://en.opensuse.org/HCL:ProBox2_Ava
https://en.opensuse.org/HCL:Lake1

As stated in the v2 rtc commit message, 2014 is the base year
encountered on all three devices that I've had access to.
@Jiang, if you're using a different base year, please speak up!

> KISS suggests not having a base in DT until it is actually
> required. Since it is an additional property, it does not break
> backwards compatibility when added.

That's what I've attempted here - but for RDA8810PL serial Rob said he
did not want future changes to my binding, therefore I am asking for his
confirmation here.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

^ permalink raw reply

* Re: [RFC 1/3] dt-bindings: rtc: Add Realtek RTD1295
From: Andrew Lunn @ 2017-08-27 13:47 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Rob Herring, linux-rtc, Alessandro Zummo, Roc He, devicetree,
	?????????, linux-kernel, Alexandre Belloni, Mark Rutland,
	linux-arm-kernel
In-Reply-To: <629b9ed0-7b2d-7c5c-20b8-17289a76f097@suse.de>

> Thanks. Did you read the RFC question in the cover letter as well and
> have any comments? Downstream has an rtc-base-year = <2014>; property
> that I had left out in this RFC and due to your ack not included in v2.
> 
> Should we default to 2014 in the driver and add an optional base-year
> property once we encounter a diverging device, or should we make it
> required from the beginning? I did not spot any other rtc binding with
> such a property and would appreciate a clarification.

Hi Andreas

>From the perspective of the hardware, does it care what the base is?

A device using a different base will initially return the wrong
time. But once the correct time has been written back, it will be O.K.

This only becomes an issue if a device is used with different OSs,
which have different bases. Swapping back and forth between OSs then
becomes an issue.

KISS suggests not having a base in DT until it is actually
required. Since it is an additional property, it does not break
backwards compatibility when added.

	  Andrew

^ permalink raw reply

* Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295
From: Andrew Lunn @ 2017-08-27 13:37 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Alexandre Belloni, linux-rtc, Alessandro Zummo, Roc He, ?????????,
	linux-kernel, linux-arm-kernel
In-Reply-To: <d20fbc21-bd8f-c191-ed51-03487868e7b0@suse.de>

> >> +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

^ permalink raw reply

* Re: [PATCH] rtc: core: let rtc_set_time properly populate element wday
From: Heiner Kallweit @ 2017-08-27 12:39 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: linux-rtc
In-Reply-To: <20170826195947.jnipgyuxg3ceqie4@piout.net>

Am 26.08.2017 um 21:59 schrieb Alexandre Belloni:
> On 26/08/2017 at 21:56:07 +0200, Alexandre Belloni wrote:
>> There is no point in fixing that. Nobody uses wday.
>>
> 
> And that was going to be my comment on your other patch. You may as well
> just remove the whole wday correction from ds1307.
> 
Actually I was wondering too when wday should ever be used, except
theoretically for an alarm, however commit e29385fab0bf
"rtc: ds1307: Fix relying on reset value for weekday" was added just
a year ago.

I think then we can remove the usage of wday in general, including
writing / reading wday in ds1307_get/set_time.

>> On 26/08/2017 at 21:16:34 +0200, Heiner Kallweit wrote:
>>> Some drivers use element wday of the struct rtc_time which is passed to
>>> callback set_time. However element wday may incorrectly or not be
>>> populated if struct rtc_time is coming from userspace via rtc_dev_ioctl.
>>> rtc_valid_tm() doesn't check element wday.
>>>
>>> Therefore convert struct rtc_time to time64_t and then use
>>> rtc_time64_to_tm to generate a struct rtc_time with all elements properly
>>> populated.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/rtc/interface.c | 24 ++++++++++++++++--------
>>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
>>> index 8cec9a02..5a53c590 100644
>>> --- a/drivers/rtc/interface.c
>>> +++ b/drivers/rtc/interface.c
>>> @@ -59,28 +59,36 @@ EXPORT_SYMBOL_GPL(rtc_read_time);
>>>  
>>>  int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
>>>  {
>>> +	time64_t secs64;
>>>  	int err;
>>>  
>>>  	err = rtc_valid_tm(tm);
>>>  	if (err != 0)
>>>  		return err;
>>>  
>>> +	secs64 = rtc_tm_to_time64(tm);
>>> +
>>>  	err = mutex_lock_interruptible(&rtc->ops_lock);
>>>  	if (err)
>>>  		return err;
>>>  
>>>  	if (!rtc->ops)
>>>  		err = -ENODEV;
>>> -	else if (rtc->ops->set_time)
>>> -		err = rtc->ops->set_time(rtc->dev.parent, tm);
>>> -	else if (rtc->ops->set_mmss64) {
>>> -		time64_t secs64 = rtc_tm_to_time64(tm);
>>> -
>>> +	else if (rtc->ops->set_time) {
>>> +		struct rtc_time tmp;
>>> +
>>> +		/*
>>> +		 * element wday of tm may not be set, therefore use
>>> +		 * rtc_time64_to_tm to generate a struct rtc_time
>>> +		 * with all elements being properly populated
>>> +		 */
>>> +		rtc_time64_to_tm(secs64, &tmp);
>>> +		err = rtc->ops->set_time(rtc->dev.parent, &tmp);
>>> +	} else if (rtc->ops->set_mmss64)
>>>  		err = rtc->ops->set_mmss64(rtc->dev.parent, secs64);
>>> -	} else if (rtc->ops->set_mmss) {
>>> -		time64_t secs64 = rtc_tm_to_time64(tm);
>>> +	else if (rtc->ops->set_mmss)
>>>  		err = rtc->ops->set_mmss(rtc->dev.parent, secs64);
>>> -	} else
>>> +	else
>>>  		err = -EINVAL;
>>>  
>>>  	pm_stay_awake(rtc->dev.parent);
>>> -- 
>>> 2.14.1
>>>
>>
>> -- 
>> Alexandre Belloni, Free Electrons
>> Embedded Linux and Kernel engineering
>> http://free-electrons.com
> 

^ permalink raw reply

* Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295
From: Andreas Färber @ 2017-08-27 11:30 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Alessandro Zummo, linux-rtc, linux-arm-kernel, linux-kernel,
	Roc He, 蒋丽琴, Andrew Lunn
In-Reply-To: <20170827091301.wbuzxkh7opz4blrc@piout.net>

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:

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

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

^ permalink raw reply related

* Re: [RFC 1/3] dt-bindings: rtc: Add Realtek RTD1295
From: Andreas Färber @ 2017-08-27 10:41 UTC (permalink / raw)
  To: Rob Herring
  Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc, linux-arm-kernel,
	linux-kernel, Roc He, 蒋丽琴, Mark Rutland,
	devicetree
In-Reply-To: <20170823002911.y35nn7jkt34dvjbc@rob-hp-laptop>

Hi Rob,

Am 23.08.2017 um 02:29 schrieb Rob Herring:
> On Sun, Aug 20, 2017 at 03:36:29AM +0200, Andreas Färber wrote:
>> Add a binding for the RTC on the Realtek RTD119x/RTD129x SoC families.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  .../devicetree/bindings/rtc/realtek,rtd119x.txt          | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/rtc/realtek,rtd119x.txt
> 
> Acked-by: Rob Herring <robh@kernel.org>

Thanks. Did you read the RFC question in the cover letter as well and
have any comments? Downstream has an rtc-base-year = <2014>; property
that I had left out in this RFC and due to your ack not included in v2.

Should we default to 2014 in the driver and add an optional base-year
property once we encounter a diverging device, or should we make it
required from the beginning? I did not spot any other rtc binding with
such a property and would appreciate a clarification.

Thanks in advance,

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

^ permalink raw reply

* Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295
From: Andreas Färber @ 2017-08-27 10:28 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Andrew Lunn, Alessandro Zummo, linux-rtc, linux-arm-kernel,
	?????????, linux-kernel, Roc He
In-Reply-To: <20170827082714.nya7m3movufujtjk@piout.net>

Am 27.08.2017 um 10:27 schrieb Alexandre Belloni:
> On 27/08/2017 at 04:30:08 +0200, Andreas Färber wrote:
>> Am 27.08.2017 um 04:05 schrieb Andrew Lunn:
>> By that same argument we could ask why so many drivers (and mine, too)
>> are calling rtc_valid_tm() when __rtc_read_time() calls it again...
> 
> Most of them probably predates the introduction of the check in
> __rtc_read_time().

Replacing with 0 here then.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

^ permalink raw reply

* Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295
From: Andreas Färber @ 2017-08-27 10:21 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: linux-rtc, Alessandro Zummo, Roc He, ?????????, linux-kernel,
	Alexandre Belloni, linux-arm-kernel
In-Reply-To: <20170827032745.GC13622@lunn.ch>

Hi Andrew,

Am 27.08.2017 um 05:27 schrieb Andrew Lunn:
> On Sun, Aug 27, 2017 at 04:30:08AM +0200, Andreas Färber wrote:
>> Am 27.08.2017 um 04:05 schrieb Andrew Lunn:
>>> n Sun, Aug 27, 2017 at 02:33:27AM +0200, Andreas Färber wrote:
>>>> +struct rtd119x_rtc {
>>>> +	void __iomem *base;
>>>> +	struct clk *clk;
>>>> +	struct rtc_device *rtcdev;
>>>> +	unsigned base_year;
>>>> +	spinlock_t lock;
>>>
>>> Where is this lock initialised? I would expect a call to
>>> spin_lock_init() somewhere.
>>
>> Hm, the spinlock in my irq mux series doesn't have that call either; my
>> reset driver did have it. The zero initialization appears to work OK,
>> but you're probably right that it should be there.
> 
> Hi Andreas
> 
> I suspect you will have problems if you enable spin lock debug code,
> like CONFIG_DEBUG_SPINLOCK.

Thanks for that hint. Surprisingly the irq mux is still fine with that
option enabled, but the rtc now shows:

[    1.193722] BUG: spinlock bad magic on CPU#0, swapper/0/1
[    1.199264]  lock: 0xffff80007bd40db8, .magic: 00000000, .owner:
<none>/-1, .owner_cpu: 0
[    1.207648] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
4.13.0-rc6-next-20170825-00061-g88513c6f5687 #180
[    1.217269] Hardware name: Zidoo X9S (DT)
[    1.221378] Call trace:
[    1.223900] [<ffff0000082878f4>] dump_backtrace+0x0/0x320
[    1.229439] [<ffff000008287c28>] show_stack+0x14/0x1c
[    1.234622] [<ffff00000875c6ac>] dump_stack+0x94/0xbc
[    1.239804] [<ffff0000082eba4c>] spin_bug+0x84/0xa4
[    1.244806] [<ffff0000082ebaf8>] do_raw_spin_lock+0x30/0xd4
[    1.250522] [<ffff000008770868>] _raw_spin_lock_irqsave+0x28/0x38
[    1.256772] [<ffff00000864cc14>] rtd119x_rtc_read_time+0x24/0x14c
[    1.263019] [<ffff00000864918c>] __rtc_read_time+0x3c/0x64
[    1.268644] [<ffff0000086491e8>] rtc_read_time+0x34/0x5c
[    1.274091] [<ffff000008649cc8>] __rtc_read_alarm+0x24/0x2f0
[    1.279893] [<ffff000008648b3c>] rtc_device_register+0xa0/0x144
[    1.285962] [<ffff000008648d88>] devm_rtc_device_register+0x5c/0xa4
[    1.292390] [<ffff00000864d038>] rtd119x_rtc_probe+0x174/0x1b4
[    1.298373] [<ffff000008555038>] platform_drv_probe+0x54/0xa8
[    1.304265] [<ffff0000085537dc>] driver_probe_device+0x1fc/0x2b8
[    1.310423] [<ffff00000855390c>] __driver_attach+0x74/0xa4
[    1.316051] [<ffff000008551d58>] bus_for_each_dev+0x80/0x90
[    1.321765] [<ffff000008553198>] driver_attach+0x20/0x28
[    1.327211] [<ffff000008552d8c>] bus_add_driver+0x194/0x1e0
[    1.332925] [<ffff000008554234>] driver_register+0x98/0xd0
[    1.338550] [<ffff000008554f90>] __platform_driver_register+0x48/0x50
[    1.345155] [<ffff0000089924f0>] rtd119x_rtc_driver_init+0x18/0x20
[    1.351493] [<ffff000008283284>] do_one_initcall+0x118/0x13c
[    1.357299] [<ffff000008970dd0>] kernel_init_freeable+0x220/0x224
[    1.363548] [<ffff00000876c128>] kernel_init+0x10/0xf8
[    1.368819] [<ffff000008284200>] ret_from_fork+0x10/0x18
[    1.375104] rtd1295-rtc 9801b600.rtc: rtc core: registered rtc as rtc0

Will fix.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

^ permalink raw reply

* Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295
From: Alexandre Belloni @ 2017-08-27  9:13 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Alessandro Zummo, linux-rtc, linux-arm-kernel, linux-kernel,
	Roc He, 蒋丽琴
In-Reply-To: <20170827003328.28370-3-afaerber@suse.de>

Hi,

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

> +	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

> +static int rtd119x_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rtd119x_rtc *data = dev_get_drvdata(dev);
> +	unsigned long flags;
> +	s32 day;
> +	u32 sec;
> +	unsigned year;

unsigned int

> +	int tries = 0;
> +
> +	spin_lock_irqsave(&data->lock, flags);
> +
> + while (true) {
> +     tm->tm_sec = (readl_relaxed(data->base + RTD_RTCSEC) & RTD_RTCSEC_RTCSEC_MASK) >> 1;
> +     tm->tm_min  = readl_relaxed(data->base + RTD_RTCMIN) & RTD_RTCMIN_RTCMIN_MASK;
> +     tm->tm_hour = readl_relaxed(data->base + RTD_RTCHR) & RTD_RTCHR_RTCHR_MASK;
> +     day  =  readl_relaxed(data->base + RTD_RTCDATE1) & RTD_RTCDATE1_RTCDATE1_MASK;
> +     day |= (readl_relaxed(data->base + RTD_RTCDATE2) & RTD_RTCDATE2_RTCDATE2_MASK) << 8;
> +     sec  = (readl_relaxed(data->base + RTD_RTCSEC) & RTD_RTCSEC_RTCSEC_MASK) >> 1;
> +     tries++;
> +
> +     if (sec == tm->tm_sec)
> +         break;
> +
> +     if (tries >= 3) {
> +         spin_unlock_irqrestore(&data->lock, flags);
> +         return -EINVAL;
> +     }
> + }
> + if (tries > 1)
> +     dev_dbg(dev, "%s: needed %i tries\n", __func__, tries);
> +
> + spin_unlock_irqrestore(&data->lock, flags);
> +
> + year = data->base_year;
> + while (day >= rtd119x_rtc_year_days(year)) {
> +     day -= rtd119x_rtc_year_days(year);
> +     year++;
> + }
> + tm->tm_year = year - 1900;
> + tm->tm_yday = day;
> +
> + tm->tm_mon = 0;
> + while (day >= rtc_month_days(tm->tm_mon, year)) {
> +     day -= rtc_month_days(tm->tm_mon, year);
> +     tm->tm_mon++;
> + }
> + tm->tm_mday = day + 1;
> +
> + return rtc_valid_tm(tm);

As you spotted, you can return 0 here.

> +}
> +
> +static int rtd119x_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rtd119x_rtc *data = dev_get_drvdata(dev);
> +	unsigned long flags;
> +	unsigned day;

ditto


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply

* Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295
From: Alexandre Belloni @ 2017-08-27  8:27 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Andrew Lunn, Alessandro Zummo, linux-rtc, linux-arm-kernel,
	?????????, linux-kernel, Roc He
In-Reply-To: <a2a21bc4-b7a9-cadf-920c-f078c673dbb9@suse.de>

On 27/08/2017 at 04:30:08 +0200, Andreas Färber wrote:
> Am 27.08.2017 um 04:05 schrieb Andrew Lunn:
> > n Sun, Aug 27, 2017 at 02:33:27AM +0200, Andreas Färber wrote:
> >> +struct rtd119x_rtc {
> >> +	void __iomem *base;
> >> +	struct clk *clk;
> >> +	struct rtc_device *rtcdev;
> >> +	unsigned base_year;
> >> +	spinlock_t lock;
> > 
> > Where is this lock initialised? I would expect a call to
> > spin_lock_init() somewhere.
> 
> Hm, the spinlock in my irq mux series doesn't have that call either; my
> reset driver did have it. The zero initialization appears to work OK,
> but you're probably right that it should be there.
> 
> > I also wonder what this lock is protecting, which rtc->ops_lock does
> > not protect?
> 
> By that same argument we could ask why so many drivers (and mine, too)
> are calling rtc_valid_tm() when __rtc_read_time() calls it again...
> 

Most of them probably predates the introduction of the check in
__rtc_read_time().


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply

* Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295
From: Andrew Lunn @ 2017-08-27  3:27 UTC (permalink / raw)
  To: Andreas Färber
  Cc: linux-rtc, Alessandro Zummo, Roc He, ?????????, linux-kernel,
	Alexandre Belloni, linux-arm-kernel
In-Reply-To: <a2a21bc4-b7a9-cadf-920c-f078c673dbb9@suse.de>

On Sun, Aug 27, 2017 at 04:30:08AM +0200, Andreas Färber wrote:
> Am 27.08.2017 um 04:05 schrieb Andrew Lunn:
> > n Sun, Aug 27, 2017 at 02:33:27AM +0200, Andreas Färber wrote:
> >> +struct rtd119x_rtc {
> >> +	void __iomem *base;
> >> +	struct clk *clk;
> >> +	struct rtc_device *rtcdev;
> >> +	unsigned base_year;
> >> +	spinlock_t lock;
> > 
> > Where is this lock initialised? I would expect a call to
> > spin_lock_init() somewhere.
> 
> Hm, the spinlock in my irq mux series doesn't have that call either; my
> reset driver did have it. The zero initialization appears to work OK,
> but you're probably right that it should be there.

Hi Andreas

I suspect you will have problems if you enable spin lock debug code,
like CONFIG_DEBUG_SPINLOCK.

     Andrew

^ permalink raw reply

* Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295
From: Andreas Färber @ 2017-08-27  2:30 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc, linux-arm-kernel,
	?????????, linux-kernel, Roc He
In-Reply-To: <20170827020501.GB13622@lunn.ch>

Am 27.08.2017 um 04:05 schrieb Andrew Lunn:
> n Sun, Aug 27, 2017 at 02:33:27AM +0200, Andreas Färber wrote:
>> +struct rtd119x_rtc {
>> +	void __iomem *base;
>> +	struct clk *clk;
>> +	struct rtc_device *rtcdev;
>> +	unsigned base_year;
>> +	spinlock_t lock;
> 
> Where is this lock initialised? I would expect a call to
> spin_lock_init() somewhere.

Hm, the spinlock in my irq mux series doesn't have that call either; my
reset driver did have it. The zero initialization appears to work OK,
but you're probably right that it should be there.

> I also wonder what this lock is protecting, which rtc->ops_lock does
> not protect?

By that same argument we could ask why so many drivers (and mine, too)
are calling rtc_valid_tm() when __rtc_read_time() calls it again...

The downstream code is locking inside individual functions such as
check_acr or set_enabled; I adopted it for whole operations only, but
you're right that so far it matches the RTC class ops granularity, so I
can drop it again. The latching discussion had reminded me of locking.

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

^ permalink raw reply

* Re: [PATCH v2 2/3] rtc: Add Realtek RTD1295
From: Andrew Lunn @ 2017-08-27  2:05 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc, linux-arm-kernel,
	?????????, linux-kernel, Roc He
In-Reply-To: <20170827003328.28370-3-afaerber@suse.de>

n Sun, Aug 27, 2017 at 02:33:27AM +0200, Andreas Färber wrote:
> Based on QNAP's arch/arm/mach-rtk119x/driver/rtk_rtc_drv.c code and
> mach-rtk119x/driver/dc2vo/fpga/include/mis_reg.h register definitions.
> 
> The base year 2014 was observed on all of Zidoo X9S, ProBox2 Ava and
> Beelink Lake I.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  v1 -> v2:
>  * Dropped open/release in favor of probe/remove (Alexandre)
>  * read_time: Reordered register accesses (Alexandre)
>  * read_time/set_time: Refactored day calculations to avoid time64_t (Alexandre)
>  * read_time: Retry if seconds change (Alexandre)
>  * probe: Added missing RTCACR initialization code
>  * set_time: Fixed year check (off by 1900)
>  * set_time: Fixed new seconds (off by factor two)
>  * Cleaned up debug output (Andrew)
>  * Added spinlocks around register accesses
>  * Added masks for register fields
>  
>  drivers/rtc/Kconfig       |   8 ++
>  drivers/rtc/Makefile      |   1 +
>  drivers/rtc/rtc-rtd119x.c | 254 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 263 insertions(+)
>  create mode 100644 drivers/rtc/rtc-rtd119x.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 22efa21b1d81..d5a46f311ecb 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1765,6 +1765,14 @@ config RTC_DRV_CPCAP
>  	   Say y here for CPCAP rtc found on some Motorola phones
>  	   and tablets such as Droid 4.
>  
> +config RTC_DRV_RTD119X
> +	bool "Realtek RTD129x RTC"
> +	depends on ARCH_REALTEK || COMPILE_TEST
> +	default ARCH_REALTEK
> +	help
> +	  If you say yes here, you get support for the RTD1295 SoC
> +	  Real Time Clock.
> +
>  comment "HID Sensor RTC drivers"
>  
>  config RTC_DRV_HID_SENSOR_TIME
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index acd366b41c85..55a0a5ca45b0 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -131,6 +131,7 @@ obj-$(CONFIG_RTC_DRV_RP5C01)	+= rtc-rp5c01.o
>  obj-$(CONFIG_RTC_DRV_RS5C313)	+= rtc-rs5c313.o
>  obj-$(CONFIG_RTC_DRV_RS5C348)	+= rtc-rs5c348.o
>  obj-$(CONFIG_RTC_DRV_RS5C372)	+= rtc-rs5c372.o
> +obj-$(CONFIG_RTC_DRV_RTD119X)	+= rtc-rtd119x.o
>  obj-$(CONFIG_RTC_DRV_RV3029C2)	+= rtc-rv3029c2.o
>  obj-$(CONFIG_RTC_DRV_RV8803)	+= rtc-rv8803.o
>  obj-$(CONFIG_RTC_DRV_RX4581)	+= rtc-rx4581.o
> diff --git a/drivers/rtc/rtc-rtd119x.c b/drivers/rtc/rtc-rtd119x.c
> new file mode 100644
> index 000000000000..27fa68a5af30
> --- /dev/null
> +++ b/drivers/rtc/rtc-rtd119x.c
> @@ -0,0 +1,254 @@
> +/*
> + * Realtek RTD129x RTC
> + *
> + * Copyright (c) 2017 Andreas Färber
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/rtc.h>
> +#include <linux/spinlock.h>
> +
> +#define RTD_RTCSEC		0x00
> +#define RTD_RTCMIN		0x04
> +#define RTD_RTCHR		0x08
> +#define RTD_RTCDATE1		0x0c
> +#define RTD_RTCDATE2		0x10
> +#define RTD_RTCACR		0x28
> +#define RTD_RTCEN		0x2c
> +#define RTD_RTCCR		0x30
> +
> +#define RTD_RTCSEC_RTCSEC_MASK		0x7f
> +
> +#define RTD_RTCMIN_RTCMIN_MASK		0x3f
> +
> +#define RTD_RTCHR_RTCHR_MASK		0x1f
> +
> +#define RTD_RTCDATE1_RTCDATE1_MASK	0xff
> +
> +#define RTD_RTCDATE2_RTCDATE2_MASK	0x7f
> +
> +#define RTD_RTCACR_RTCPWR		BIT(7)
> +
> +#define RTD_RTCEN_RTCEN_MASK		0xff
> +
> +#define RTD_RTCCR_RTCRST		BIT(6)
> +
> +struct rtd119x_rtc {
> +	void __iomem *base;
> +	struct clk *clk;
> +	struct rtc_device *rtcdev;
> +	unsigned base_year;
> +	spinlock_t lock;

Where is this lock initialised? I would expect a call to
spin_lock_init() somewhere.

I also wonder what this lock is protecting, which rtc->ops_lock does
not protect?

    Andrew

^ permalink raw reply

* [PATCH v2 3/3] arm64: dts: realtek: Add RTD1295 RTC node
From: Andreas Färber @ 2017-08-27  0:33 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc, linux-arm-kernel
  Cc: linux-kernel, Roc He, 蒋丽琴,
	Andreas Färber, Rob Herring, Mark Rutland, Catalin Marinas,
	Will Deacon, devicetree
In-Reply-To: <20170827003328.28370-1-afaerber@suse.de>

Add RTC node to the Realtek RTD1295 Device Tree.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v1 -> v2: Unchanged
 
 Depends on the pending clock bindings.
 
 arch/arm64/boot/dts/realtek/rtd1295.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/realtek/rtd1295.dtsi b/arch/arm64/boot/dts/realtek/rtd1295.dtsi
index 503e2d5fc334..8ae0949ad89e 100644
--- a/arch/arm64/boot/dts/realtek/rtd1295.dtsi
+++ b/arch/arm64/boot/dts/realtek/rtd1295.dtsi
@@ -192,6 +192,12 @@
 			status = "disabled";
 		};
 
+		rtc@9801b600 {
+			compatible = "realtek,rtd1295-rtc";
+			reg = <0x9801b600 0x100>;
+			clocks = <&clkc RTD1295_CLK_EN_MISC_RTC>;
+		};
+
 		gic: interrupt-controller@ff011000 {
 			compatible = "arm,gic-400";
 			reg = <0xff011000 0x1000>,
-- 
2.12.3

^ permalink raw reply related

* [PATCH v2 2/3] rtc: Add Realtek RTD1295
From: Andreas Färber @ 2017-08-27  0:33 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc, linux-arm-kernel
  Cc: linux-kernel, Roc He, 蒋丽琴,
	Andreas Färber
In-Reply-To: <20170827003328.28370-1-afaerber@suse.de>

Based on QNAP's arch/arm/mach-rtk119x/driver/rtk_rtc_drv.c code and
mach-rtk119x/driver/dc2vo/fpga/include/mis_reg.h register definitions.

The base year 2014 was observed on all of Zidoo X9S, ProBox2 Ava and
Beelink Lake I.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v1 -> v2:
 * Dropped open/release in favor of probe/remove (Alexandre)
 * read_time: Reordered register accesses (Alexandre)
 * read_time/set_time: Refactored day calculations to avoid time64_t (Alexandre)
 * read_time: Retry if seconds change (Alexandre)
 * probe: Added missing RTCACR initialization code
 * set_time: Fixed year check (off by 1900)
 * set_time: Fixed new seconds (off by factor two)
 * Cleaned up debug output (Andrew)
 * Added spinlocks around register accesses
 * Added masks for register fields
 
 drivers/rtc/Kconfig       |   8 ++
 drivers/rtc/Makefile      |   1 +
 drivers/rtc/rtc-rtd119x.c | 254 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 263 insertions(+)
 create mode 100644 drivers/rtc/rtc-rtd119x.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 22efa21b1d81..d5a46f311ecb 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1765,6 +1765,14 @@ config RTC_DRV_CPCAP
 	   Say y here for CPCAP rtc found on some Motorola phones
 	   and tablets such as Droid 4.
 
+config RTC_DRV_RTD119X
+	bool "Realtek RTD129x RTC"
+	depends on ARCH_REALTEK || COMPILE_TEST
+	default ARCH_REALTEK
+	help
+	  If you say yes here, you get support for the RTD1295 SoC
+	  Real Time Clock.
+
 comment "HID Sensor RTC drivers"
 
 config RTC_DRV_HID_SENSOR_TIME
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index acd366b41c85..55a0a5ca45b0 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -131,6 +131,7 @@ obj-$(CONFIG_RTC_DRV_RP5C01)	+= rtc-rp5c01.o
 obj-$(CONFIG_RTC_DRV_RS5C313)	+= rtc-rs5c313.o
 obj-$(CONFIG_RTC_DRV_RS5C348)	+= rtc-rs5c348.o
 obj-$(CONFIG_RTC_DRV_RS5C372)	+= rtc-rs5c372.o
+obj-$(CONFIG_RTC_DRV_RTD119X)	+= rtc-rtd119x.o
 obj-$(CONFIG_RTC_DRV_RV3029C2)	+= rtc-rv3029c2.o
 obj-$(CONFIG_RTC_DRV_RV8803)	+= rtc-rv8803.o
 obj-$(CONFIG_RTC_DRV_RX4581)	+= rtc-rx4581.o
diff --git a/drivers/rtc/rtc-rtd119x.c b/drivers/rtc/rtc-rtd119x.c
new file mode 100644
index 000000000000..27fa68a5af30
--- /dev/null
+++ b/drivers/rtc/rtc-rtd119x.c
@@ -0,0 +1,254 @@
+/*
+ * Realtek RTD129x RTC
+ *
+ * Copyright (c) 2017 Andreas Färber
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+#include <linux/spinlock.h>
+
+#define RTD_RTCSEC		0x00
+#define RTD_RTCMIN		0x04
+#define RTD_RTCHR		0x08
+#define RTD_RTCDATE1		0x0c
+#define RTD_RTCDATE2		0x10
+#define RTD_RTCACR		0x28
+#define RTD_RTCEN		0x2c
+#define RTD_RTCCR		0x30
+
+#define RTD_RTCSEC_RTCSEC_MASK		0x7f
+
+#define RTD_RTCMIN_RTCMIN_MASK		0x3f
+
+#define RTD_RTCHR_RTCHR_MASK		0x1f
+
+#define RTD_RTCDATE1_RTCDATE1_MASK	0xff
+
+#define RTD_RTCDATE2_RTCDATE2_MASK	0x7f
+
+#define RTD_RTCACR_RTCPWR		BIT(7)
+
+#define RTD_RTCEN_RTCEN_MASK		0xff
+
+#define RTD_RTCCR_RTCRST		BIT(6)
+
+struct rtd119x_rtc {
+	void __iomem *base;
+	struct clk *clk;
+	struct rtc_device *rtcdev;
+	unsigned base_year;
+	spinlock_t lock;
+};
+
+static inline int rtd119x_rtc_year_days(int year)
+{
+	return rtc_year_days(1, 12, year);
+}
+
+static void rtd119x_rtc_reset(struct device *dev)
+{
+	struct rtd119x_rtc *data = dev_get_drvdata(dev);
+	u32 val;
+
+	val = readl_relaxed(data->base + RTD_RTCCR);
+	val |= RTD_RTCCR_RTCRST;
+	writel_relaxed(val, data->base + RTD_RTCCR);
+
+	val &= ~RTD_RTCCR_RTCRST;
+	writel(val, data->base + RTD_RTCCR);
+}
+
+static void rtd119x_rtc_set_enabled(struct device *dev, bool enable)
+{
+	struct rtd119x_rtc *data = dev_get_drvdata(dev);
+	u32 val;
+
+	val = readl_relaxed(data->base + RTD_RTCEN);
+	if (enable) {
+		if ((val & RTD_RTCEN_RTCEN_MASK) == 0x5a)
+			return;
+		writel_relaxed(0x5a, data->base + RTD_RTCEN);
+	} else {
+		writel_relaxed(0, data->base + RTD_RTCEN);
+	}
+}
+
+static int rtd119x_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rtd119x_rtc *data = dev_get_drvdata(dev);
+	unsigned long flags;
+	s32 day;
+	u32 sec;
+	unsigned year;
+	int tries = 0;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	while (true) {
+		tm->tm_sec = (readl_relaxed(data->base + RTD_RTCSEC) & RTD_RTCSEC_RTCSEC_MASK) >> 1;
+		tm->tm_min  = readl_relaxed(data->base + RTD_RTCMIN) & RTD_RTCMIN_RTCMIN_MASK;
+		tm->tm_hour = readl_relaxed(data->base + RTD_RTCHR) & RTD_RTCHR_RTCHR_MASK;
+		day  =  readl_relaxed(data->base + RTD_RTCDATE1) & RTD_RTCDATE1_RTCDATE1_MASK;
+		day |= (readl_relaxed(data->base + RTD_RTCDATE2) & RTD_RTCDATE2_RTCDATE2_MASK) << 8;
+		sec  = (readl_relaxed(data->base + RTD_RTCSEC) & RTD_RTCSEC_RTCSEC_MASK) >> 1;
+		tries++;
+
+		if (sec == tm->tm_sec)
+			break;
+
+		if (tries >= 3) {
+			spin_unlock_irqrestore(&data->lock, flags);
+			return -EINVAL;
+		}
+	}
+	if (tries > 1)
+		dev_dbg(dev, "%s: needed %i tries\n", __func__, tries);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	year = data->base_year;
+	while (day >= rtd119x_rtc_year_days(year)) {
+		day -= rtd119x_rtc_year_days(year);
+		year++;
+	}
+	tm->tm_year = year - 1900;
+	tm->tm_yday = day;
+
+	tm->tm_mon = 0;
+	while (day >= rtc_month_days(tm->tm_mon, year)) {
+		day -= rtc_month_days(tm->tm_mon, year);
+		tm->tm_mon++;
+	}
+	tm->tm_mday = day + 1;
+
+	return rtc_valid_tm(tm);
+}
+
+static int rtd119x_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rtd119x_rtc *data = dev_get_drvdata(dev);
+	unsigned long flags;
+	unsigned day;
+	int i;
+
+	if (1900 + tm->tm_year < data->base_year)
+		return -EINVAL;
+
+	day = 0;
+	for (i = data->base_year; i < 1900 + tm->tm_year; i++)
+		day += rtd119x_rtc_year_days(i);
+
+	day += tm->tm_yday;
+	if (day > 0x7fff)
+		return -EINVAL;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	rtd119x_rtc_set_enabled(dev, false);
+
+	writel_relaxed((tm->tm_sec << 1) & RTD_RTCSEC_RTCSEC_MASK, data->base + RTD_RTCSEC);
+	writel_relaxed(tm->tm_min & RTD_RTCMIN_RTCMIN_MASK, data->base + RTD_RTCMIN);
+	writel_relaxed(tm->tm_hour & RTD_RTCHR_RTCHR_MASK, data->base + RTD_RTCHR);
+	writel_relaxed(day & RTD_RTCDATE1_RTCDATE1_MASK, data->base + RTD_RTCDATE1);
+	writel_relaxed((day >> 8) & RTD_RTCDATE2_RTCDATE2_MASK, data->base + RTD_RTCDATE2);
+
+	rtd119x_rtc_set_enabled(dev, true);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	return 0;
+}
+
+static const struct rtc_class_ops rtd119x_rtc_ops = {
+	.read_time	= rtd119x_rtc_read_time,
+	.set_time	= rtd119x_rtc_set_time,
+};
+
+static const struct of_device_id rtd119x_rtc_dt_ids[] = {
+	 { .compatible = "realtek,rtd1295-rtc" },
+	 { }
+};
+
+static int rtd119x_rtc_probe(struct platform_device *pdev)
+{
+	struct rtd119x_rtc *data;
+	struct resource *res;
+	u32 val;
+	int ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, data);
+	data->base_year = 2014;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data->base))
+		return PTR_ERR(data->base);
+
+	data->clk = of_clk_get(pdev->dev.of_node, 0);
+	if (IS_ERR(data->clk))
+		return PTR_ERR(data->clk);
+
+	ret = clk_prepare_enable(data->clk);
+	if (ret) {
+		clk_put(data->clk);
+		return ret;
+	}
+
+	val = readl_relaxed(data->base + RTD_RTCACR);
+	if (!(val & RTD_RTCACR_RTCPWR)) {
+		writel_relaxed(RTD_RTCACR_RTCPWR, data->base + RTD_RTCACR);
+
+		rtd119x_rtc_reset(&pdev->dev);
+
+		writel_relaxed(0, data->base + RTD_RTCMIN);
+		writel_relaxed(0, data->base + RTD_RTCHR);
+		writel_relaxed(0, data->base + RTD_RTCDATE1);
+		writel_relaxed(0, data->base + RTD_RTCDATE2);
+	}
+
+	rtd119x_rtc_set_enabled(&pdev->dev, true);
+
+	data->rtcdev = devm_rtc_device_register(&pdev->dev, "rtc",
+				&rtd119x_rtc_ops, THIS_MODULE);
+	if (IS_ERR(data->rtcdev)) {
+		dev_err(&pdev->dev, "failed to register rtc device");
+		clk_disable_unprepare(data->clk);
+		clk_put(data->clk);
+		return PTR_ERR(data->rtcdev);
+	}
+
+	return 0;
+}
+
+static int rtd119x_rtc_remove(struct platform_device *pdev)
+{
+	struct rtd119x_rtc *data = platform_get_drvdata(pdev);
+
+	rtd119x_rtc_set_enabled(&pdev->dev, false);
+
+	clk_disable_unprepare(data->clk);
+
+	return 0;
+}
+
+static struct platform_driver rtd119x_rtc_driver = {
+	.probe = rtd119x_rtc_probe,
+	.remove = rtd119x_rtc_remove,
+	.driver = {
+		.name = "rtd1295-rtc",
+		.of_match_table	= rtd119x_rtc_dt_ids,
+	},
+};
+builtin_platform_driver(rtd119x_rtc_driver);
-- 
2.12.3

^ permalink raw reply related

* [PATCH v2 0/3] arm64: Realtek RTD1295 RTC
From: Andreas Färber @ 2017-08-27  0:33 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc, linux-arm-kernel
  Cc: linux-kernel, Roc He, 蒋丽琴,
	Andreas Färber, devicetree, Andrew Lunn

Hello,

This series adds the RTC for the Realtek RTD1295 SoC.
Based on my RTD1295 clk series.

There being no public source code for RTD1295, the implementation is based on
register offsets seen in the vendor DT, as well as older mach-rtk119x code
published by QNAP.

The base year is still hardcoded in v2. Calculations changed.

The DT node depends on the clk series for clock index and header.

More experimental patches at:
https://github.com/afaerber/linux/commits/rtd1295-next

Have a lot of fun!

Cheers,
Andreas

v1 -> v2:
* Updated rtc driver to no longer use open/release (Alexandre)
* Cleaned up debug output (Andrew)
* Avoided COMPILE_TEST division errors (kbuild)
* Various cleanups and extensions

Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Cc: linux-rtc@vger.kernel.org
Cc: Roc He <hepeng@zidoo.tv>
Cc: 蒋丽琴 <jiang.liqin@geniatech.com>
Cc: devicetree@vger.kernel.org
Cc: Andrew Lunn <andrew@lunn.ch>

Andreas Färber (3):
  dt-bindings: rtc: Add Realtek RTD1295
  rtc: Add Realtek RTD1295
  arm64: dts: realtek: Add RTD1295 RTC node

 .../devicetree/bindings/rtc/realtek,rtd119x.txt    |  16 ++
 arch/arm64/boot/dts/realtek/rtd1295.dtsi           |   6 +
 drivers/rtc/Kconfig                                |   8 +
 drivers/rtc/Makefile                               |   1 +
 drivers/rtc/rtc-rtd119x.c                          | 254 +++++++++++++++++++++
 5 files changed, 285 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/realtek,rtd119x.txt
 create mode 100644 drivers/rtc/rtc-rtd119x.c

-- 
2.12.3

^ permalink raw reply

* [PATCH v2 1/3] dt-bindings: rtc: Add Realtek RTD1295
From: Andreas Färber @ 2017-08-27  0:33 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc, linux-arm-kernel
  Cc: linux-kernel, Roc He, 蒋丽琴,
	Andreas Färber, Rob Herring, Mark Rutland, devicetree
In-Reply-To: <20170827003328.28370-1-afaerber@suse.de>

Add a binding for the RTC on the Realtek RTD119x/RTD129x SoC families.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v1 -> v2: Unchanged
 
 .../devicetree/bindings/rtc/realtek,rtd119x.txt          | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/realtek,rtd119x.txt

diff --git a/Documentation/devicetree/bindings/rtc/realtek,rtd119x.txt b/Documentation/devicetree/bindings/rtc/realtek,rtd119x.txt
new file mode 100644
index 000000000000..bbf1ccb5df31
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/realtek,rtd119x.txt
@@ -0,0 +1,16 @@
+Realtek RTD129x Real-Time Clock
+===============================
+
+Required properties:
+- compatible :  Should be "realtek,rtd1295-rtc"
+- reg        :  Specifies the physical base address and size
+- clocks     :  Specifies the clock gate
+
+
+Example:
+
+	rtc@9801b600 {
+		compatible = "realtek,rtd1295-clk";
+		reg = <0x9801b600 0x100>;
+		clocks = <&clkc RTD1295_CLK_EN_MISC_RTC>;
+	};
-- 
2.12.3

^ permalink raw reply related

* Re: [PATCH] rtc: core: let rtc_set_time properly populate element wday
From: Alexandre Belloni @ 2017-08-26 19:59 UTC (permalink / raw)
  To: Heiner Kallweit; +Cc: linux-rtc
In-Reply-To: <20170826195607.s6p7u7vr3hxhwwqs@piout.net>

On 26/08/2017 at 21:56:07 +0200, Alexandre Belloni wrote:
> There is no point in fixing that. Nobody uses wday.
> 

And that was going to be my comment on your other patch. You may as well
just remove the whole wday correction from ds1307.

> On 26/08/2017 at 21:16:34 +0200, Heiner Kallweit wrote:
> > Some drivers use element wday of the struct rtc_time which is passed to
> > callback set_time. However element wday may incorrectly or not be
> > populated if struct rtc_time is coming from userspace via rtc_dev_ioctl.
> > rtc_valid_tm() doesn't check element wday.
> > 
> > Therefore convert struct rtc_time to time64_t and then use
> > rtc_time64_to_tm to generate a struct rtc_time with all elements properly
> > populated.
> > 
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > ---
> >  drivers/rtc/interface.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> > index 8cec9a02..5a53c590 100644
> > --- a/drivers/rtc/interface.c
> > +++ b/drivers/rtc/interface.c
> > @@ -59,28 +59,36 @@ EXPORT_SYMBOL_GPL(rtc_read_time);
> >  
> >  int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
> >  {
> > +	time64_t secs64;
> >  	int err;
> >  
> >  	err = rtc_valid_tm(tm);
> >  	if (err != 0)
> >  		return err;
> >  
> > +	secs64 = rtc_tm_to_time64(tm);
> > +
> >  	err = mutex_lock_interruptible(&rtc->ops_lock);
> >  	if (err)
> >  		return err;
> >  
> >  	if (!rtc->ops)
> >  		err = -ENODEV;
> > -	else if (rtc->ops->set_time)
> > -		err = rtc->ops->set_time(rtc->dev.parent, tm);
> > -	else if (rtc->ops->set_mmss64) {
> > -		time64_t secs64 = rtc_tm_to_time64(tm);
> > -
> > +	else if (rtc->ops->set_time) {
> > +		struct rtc_time tmp;
> > +
> > +		/*
> > +		 * element wday of tm may not be set, therefore use
> > +		 * rtc_time64_to_tm to generate a struct rtc_time
> > +		 * with all elements being properly populated
> > +		 */
> > +		rtc_time64_to_tm(secs64, &tmp);
> > +		err = rtc->ops->set_time(rtc->dev.parent, &tmp);
> > +	} else if (rtc->ops->set_mmss64)
> >  		err = rtc->ops->set_mmss64(rtc->dev.parent, secs64);
> > -	} else if (rtc->ops->set_mmss) {
> > -		time64_t secs64 = rtc_tm_to_time64(tm);
> > +	else if (rtc->ops->set_mmss)
> >  		err = rtc->ops->set_mmss(rtc->dev.parent, secs64);
> > -	} else
> > +	else
> >  		err = -EINVAL;
> >  
> >  	pm_stay_awake(rtc->dev.parent);
> > -- 
> > 2.14.1
> > 
> 
> -- 
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply

* Re: [PATCH] rtc: core: let rtc_set_time properly populate element wday
From: Alexandre Belloni @ 2017-08-26 19:56 UTC (permalink / raw)
  To: Heiner Kallweit; +Cc: linux-rtc
In-Reply-To: <3b43da73-ff4e-4519-d1c7-266b17cc5d05@gmail.com>

There is no point in fixing that. Nobody uses wday.

On 26/08/2017 at 21:16:34 +0200, Heiner Kallweit wrote:
> Some drivers use element wday of the struct rtc_time which is passed to
> callback set_time. However element wday may incorrectly or not be
> populated if struct rtc_time is coming from userspace via rtc_dev_ioctl.
> rtc_valid_tm() doesn't check element wday.
> 
> Therefore convert struct rtc_time to time64_t and then use
> rtc_time64_to_tm to generate a struct rtc_time with all elements properly
> populated.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/rtc/interface.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index 8cec9a02..5a53c590 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -59,28 +59,36 @@ EXPORT_SYMBOL_GPL(rtc_read_time);
>  
>  int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
>  {
> +	time64_t secs64;
>  	int err;
>  
>  	err = rtc_valid_tm(tm);
>  	if (err != 0)
>  		return err;
>  
> +	secs64 = rtc_tm_to_time64(tm);
> +
>  	err = mutex_lock_interruptible(&rtc->ops_lock);
>  	if (err)
>  		return err;
>  
>  	if (!rtc->ops)
>  		err = -ENODEV;
> -	else if (rtc->ops->set_time)
> -		err = rtc->ops->set_time(rtc->dev.parent, tm);
> -	else if (rtc->ops->set_mmss64) {
> -		time64_t secs64 = rtc_tm_to_time64(tm);
> -
> +	else if (rtc->ops->set_time) {
> +		struct rtc_time tmp;
> +
> +		/*
> +		 * element wday of tm may not be set, therefore use
> +		 * rtc_time64_to_tm to generate a struct rtc_time
> +		 * with all elements being properly populated
> +		 */
> +		rtc_time64_to_tm(secs64, &tmp);
> +		err = rtc->ops->set_time(rtc->dev.parent, &tmp);
> +	} else if (rtc->ops->set_mmss64)
>  		err = rtc->ops->set_mmss64(rtc->dev.parent, secs64);
> -	} else if (rtc->ops->set_mmss) {
> -		time64_t secs64 = rtc_tm_to_time64(tm);
> +	else if (rtc->ops->set_mmss)
>  		err = rtc->ops->set_mmss(rtc->dev.parent, secs64);
> -	} else
> +	else
>  		err = -EINVAL;
>  
>  	pm_stay_awake(rtc->dev.parent);
> -- 
> 2.14.1
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply

* [PATCH] rtc: core: let rtc_set_time properly populate element wday
From: Heiner Kallweit @ 2017-08-26 19:16 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: linux-rtc

Some drivers use element wday of the struct rtc_time which is passed to
callback set_time. However element wday may incorrectly or not be
populated if struct rtc_time is coming from userspace via rtc_dev_ioctl.
rtc_valid_tm() doesn't check element wday.

Therefore convert struct rtc_time to time64_t and then use
rtc_time64_to_tm to generate a struct rtc_time with all elements properly
populated.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/rtc/interface.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 8cec9a02..5a53c590 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -59,28 +59,36 @@ EXPORT_SYMBOL_GPL(rtc_read_time);
 
 int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
 {
+	time64_t secs64;
 	int err;
 
 	err = rtc_valid_tm(tm);
 	if (err != 0)
 		return err;
 
+	secs64 = rtc_tm_to_time64(tm);
+
 	err = mutex_lock_interruptible(&rtc->ops_lock);
 	if (err)
 		return err;
 
 	if (!rtc->ops)
 		err = -ENODEV;
-	else if (rtc->ops->set_time)
-		err = rtc->ops->set_time(rtc->dev.parent, tm);
-	else if (rtc->ops->set_mmss64) {
-		time64_t secs64 = rtc_tm_to_time64(tm);
-
+	else if (rtc->ops->set_time) {
+		struct rtc_time tmp;
+
+		/*
+		 * element wday of tm may not be set, therefore use
+		 * rtc_time64_to_tm to generate a struct rtc_time
+		 * with all elements being properly populated
+		 */
+		rtc_time64_to_tm(secs64, &tmp);
+		err = rtc->ops->set_time(rtc->dev.parent, &tmp);
+	} else if (rtc->ops->set_mmss64)
 		err = rtc->ops->set_mmss64(rtc->dev.parent, secs64);
-	} else if (rtc->ops->set_mmss) {
-		time64_t secs64 = rtc_tm_to_time64(tm);
+	else if (rtc->ops->set_mmss)
 		err = rtc->ops->set_mmss(rtc->dev.parent, secs64);
-	} else
+	else
 		err = -EINVAL;
 
 	pm_stay_awake(rtc->dev.parent);
-- 
2.14.1

^ permalink raw reply related

* Re: [PATCH 0/5] rtc: ds1307: factor out more stuff from ds1307_probe and improve ds1307_set_time
From: Alexandre Belloni @ 2017-08-26 10:44 UTC (permalink / raw)
  To: Heiner Kallweit; +Cc: linux-rtc
In-Reply-To: <9ae42f67-9562-16c4-252d-9f629a31b69e@gmail.com>

On 26/08/2017 at 12:16:53 +0200, Heiner Kallweit wrote:
> That's exactly my point. The driver is too big already.
> 
> The patches so far increase size of the driver a little, only subsequent
> patches start to reduce it.
> 
> More things like even exporting clocks work the same on all chips
> supporting this feature. Just the layout of the alarm registers usually is
> quite different. Therefore it's my plan to create such a ds1307_lib with all
> the generic code.
> 
> If it helps I can provide the full patch set (as far as I came so far) via
> Github, then you can check whether it's the right direction also from your
> point of view (w/o having to review each single patch in detail already).
> 

Yes, please do that.

> By the way: This current patch set with the 5 patches I have to change,
> so there will be a v2. No need for you to spend reviewing effort on it now.
> 

Ok, thanks.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply

* Re: [PATCH 0/5] rtc: ds1307: factor out more stuff from ds1307_probe and improve ds1307_set_time
From: Heiner Kallweit @ 2017-08-26 10:16 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: linux-rtc
In-Reply-To: <20170826081904.ibsxi7crvi7qop3m@piout.net>

Am 26.08.2017 um 10:19 schrieb Alexandre Belloni:
> Hi,
> 
> Could you explain were you are going with this because this is a lot of
> churn that takes a lot of my reviewing time and this doesn't seem to end
> soon.
> 
> I want that driver to stop growing, maybe by creating a library. Some of
> the supported RTCs doesn't share much more than the time and date layout
> and should be in a separate driver.
> 
That's exactly my point. The driver is too big already.

The patches so far increase size of the driver a little, only subsequent
patches start to reduce it.

More things like even exporting clocks work the same on all chips
supporting this feature. Just the layout of the alarm registers usually is
quite different. Therefore it's my plan to create such a ds1307_lib with all
the generic code.

If it helps I can provide the full patch set (as far as I came so far) via
Github, then you can check whether it's the right direction also from your
point of view (w/o having to review each single patch in detail already).

By the way: This current patch set with the 5 patches I have to change,
so there will be a v2. No need for you to spend reviewing effort on it now.

Regards, Heiner


> On 25/08/2017 at 21:30:09 +0200, Heiner Kallweit wrote:
>> Final goal of the refactoring is to abstract everything that the chips
>> have in common and handle it in generic code. Then we can get rid of
>> all the "switch (chip)" and "if (chip == xxx)" code.
>>
>> To give one example:
>> A lot of chips have a bit for setting 12hr / 24hr mode. However some
>> chips have this config bit in the timekeeping registers, others in
>> a config register, and on some chips it's inverted.
>> But the functionality of the bit is always the same.
>>
>> Ultimately adding support for a chip just requires to add one config
>> structure member.
>>
>> The way to reach this goal is a long one and to faciliate reviewing
>> the patches I'll split them into series of 5 to 10 patches.
>>
>> Heiner Kallweit (5):
>>   rtc: ds1307: factor out determining the chip type
>>   rtc: ds1307: factor out trickle charger initialization
>>   rtc: ds1307: factor out fixing the weekday
>>   rtc: ds1307: introduce constants for the timekeeping register masks
>>   rtc: ds1307: improve ds1307_set_time to respect config flag bits
>>
>>  drivers/rtc/rtc-ds1307.c | 256 ++++++++++++++++++++++++++---------------------
>>  1 file changed, 140 insertions(+), 116 deletions(-)
>>
>> -- 
>> 2.14.1
>>
> 

^ permalink raw reply

* Re: [PATCH 0/5] rtc: ds1307: factor out more stuff from ds1307_probe and improve ds1307_set_time
From: Alexandre Belloni @ 2017-08-26  8:19 UTC (permalink / raw)
  To: Heiner Kallweit; +Cc: linux-rtc
In-Reply-To: <cb381496-4d55-996d-e2ae-18880330e386@gmail.com>

Hi,

Could you explain were you are going with this because this is a lot of
churn that takes a lot of my reviewing time and this doesn't seem to end
soon.

I want that driver to stop growing, maybe by creating a library. Some of
the supported RTCs doesn't share much more than the time and date layout
and should be in a separate driver.

On 25/08/2017 at 21:30:09 +0200, Heiner Kallweit wrote:
> Final goal of the refactoring is to abstract everything that the chips
> have in common and handle it in generic code. Then we can get rid of
> all the "switch (chip)" and "if (chip == xxx)" code.
> 
> To give one example:
> A lot of chips have a bit for setting 12hr / 24hr mode. However some
> chips have this config bit in the timekeeping registers, others in
> a config register, and on some chips it's inverted.
> But the functionality of the bit is always the same.
> 
> Ultimately adding support for a chip just requires to add one config
> structure member.
> 
> The way to reach this goal is a long one and to faciliate reviewing
> the patches I'll split them into series of 5 to 10 patches.
> 
> Heiner Kallweit (5):
>   rtc: ds1307: factor out determining the chip type
>   rtc: ds1307: factor out trickle charger initialization
>   rtc: ds1307: factor out fixing the weekday
>   rtc: ds1307: introduce constants for the timekeeping register masks
>   rtc: ds1307: improve ds1307_set_time to respect config flag bits
> 
>  drivers/rtc/rtc-ds1307.c | 256 ++++++++++++++++++++++++++---------------------
>  1 file changed, 140 insertions(+), 116 deletions(-)
> 
> -- 
> 2.14.1
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply

* Re: [PATCH v3 0/5] Add MediaTek PMIC keys support
From: Chen Zhong @ 2017-08-26  2:30 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rob Herring, Mark Rutland, Matthias Brugger, Lee Jones,
	Eddie Huang, Alessandro Zummo, Alexandre Belloni, Andi Shyti,
	Javier Martinez Canillas, Linus Walleij, Jaechul Lee, linux-input,
	devicetree, linux-arm-kernel, linux-mediatek, linux-kernel,
	linux-rtc
In-Reply-To: <1503642753-12385-1-git-send-email-chen.zhong@mediatek.com>

Sorry for missing the change history.

Changes since v2:
- use standard properties for keycodes and debounce time
- change to use platform_get_irq in leaf drivers
- use better ways to define IRQ resources

Changes since v1:
- create irq mappings in mfd core driver instead of leaf drivers
- remove some unused parts in mtk-pmic-keys driver

On Fri, 2017-08-25 at 14:32 +0800, Chen Zhong wrote:
> MediaTek PMIC are multi-function devices that can handle key interrupts,
> typically there are two keys attached to PMIC, which called pwrkey
> and homekey. PWRKEY usually used to wake up system from sleep. Homekey
> can used as volume down key due to board design. Long press keys can
> shutdown PMIC, the mode can be choose to be one key only or two keys
> together.
> This series add support for key functions for MediaTek PMIC MT6397/MT6323.
> 
> Chen Zhong (5):
>   mfd: mt6397: create irq mappings in mfd core driver
>   dt-bindings: input: Add document bindings for mtk-pmic-keys
>   dt-bindings: mfd: Add bindings for the keys as subnode of PMIC
>   input: Add MediaTek PMIC keys support
>   mfd: mt6397: Add PMIC keys support to MT6397 driver
> 
>  .../devicetree/bindings/input/mtk-pmic-keys.txt    |  38 +++
>  Documentation/devicetree/bindings/mfd/mt6397.txt   |   6 +
>  drivers/input/keyboard/Kconfig                     |   9 +
>  drivers/input/keyboard/Makefile                    |   1 +
>  drivers/input/keyboard/mtk-pmic-keys.c             | 308 +++++++++++++++++++++
>  drivers/mfd/mt6397-core.c                          |  26 +-
>  drivers/rtc/rtc-mt6397.c                           |   7 +-
>  7 files changed, 388 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/input/mtk-pmic-keys.txt
>  create mode 100644 drivers/input/keyboard/mtk-pmic-keys.c
> 

^ permalink raw reply

* [PATCH 5/5] rtc: ds1307: improve ds1307_set_time to respect config flag bits
From: Heiner Kallweit @ 2017-08-25 20:06 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: linux-rtc
In-Reply-To: <cb381496-4d55-996d-e2ae-18880330e386@gmail.com>

Some chips use the unused bits in the timekeeping registers for config
bits. Therefore, when setting the time, we should read the timekeeping
registers and leave the unused bits intact when setting the date / time
values.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 68 ++++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index a43a80b2..1255b165 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -447,9 +447,8 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 {
 	struct ds1307	*ds1307 = dev_get_drvdata(dev);
 	const struct chip_desc *chip = &chips[ds1307->type];
-	int		result;
-	int		tmp;
 	u8		*buf = ds1307->regs;
+	int		ret;
 
 	dev_dbg(dev, "%s secs=%d, mins=%d, "
 		"hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
@@ -457,51 +456,52 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 		t->tm_hour, t->tm_mday,
 		t->tm_mon, t->tm_year, t->tm_wday);
 
-	if (t->tm_year < 100)
+	if (t->tm_year < 100 || t->tm_year > 299)
 		return -EINVAL;
 
-#ifdef CONFIG_RTC_DRV_DS1307_CENTURY
-	if (t->tm_year > (chip->century_bit ? 299 : 199))
-		return -EINVAL;
-#else
-	if (t->tm_year > 199)
-		return -EINVAL;
-#endif
+	if (!IS_ENABLED(CONFIG_RTC_DRV_DS1307_CENTURY) || !chip->century_bit)
+		if (t->tm_year > 199)
+			return -EINVAL;
 
-	buf[DS1307_REG_SECS] = bin2bcd(t->tm_sec);
-	buf[DS1307_REG_MIN] = bin2bcd(t->tm_min);
-	buf[DS1307_REG_HOUR] = bin2bcd(t->tm_hour);
-	buf[DS1307_REG_WDAY] = bin2bcd(t->tm_wday + 1);
-	buf[DS1307_REG_MDAY] = bin2bcd(t->tm_mday);
-	buf[DS1307_REG_MONTH] = bin2bcd(t->tm_mon + 1);
+	ret = regmap_bulk_read(ds1307->regmap, chip->offset, buf, 7);
+	if (ret) {
+		dev_err(dev, "%s error %d\n", "read", ret);
+		return ret;
+	}
 
+	/*
+	 * Several chips use upper bits of these registers for
+	 * config bits, so leave them intact.
+	 */
+	buf[DS1307_REG_SECS] &= ~DS1307_SECS_MASK;
+	buf[DS1307_REG_SECS] |= bin2bcd(t->tm_sec);
+	buf[DS1307_REG_MIN] &= ~DS1307_MIN_MASK;
+	buf[DS1307_REG_MIN] |= bin2bcd(t->tm_min);
+	buf[DS1307_REG_HOUR] &= ~DS1307_HOUR_MASK;
+	buf[DS1307_REG_HOUR] |= bin2bcd(t->tm_hour);
+	buf[DS1307_REG_WDAY] &= ~DS1307_WDAY_MASK;
+	buf[DS1307_REG_WDAY] |= bin2bcd(t->tm_wday + 1);
+	buf[DS1307_REG_MDAY] &= ~DS1307_MDAY_MASK;
+	buf[DS1307_REG_MDAY] |= bin2bcd(t->tm_mday);
+	buf[DS1307_REG_MONTH] &= ~DS1307_MONTH_MASK;
+	buf[DS1307_REG_MONTH] |= bin2bcd(t->tm_mon + 1);
+
+	buf[DS1307_REG_YEAR] &= ~DS1307_YEAR_MASK;
 	/* assume 20YY not 19YY */
-	tmp = t->tm_year - 100;
-	buf[DS1307_REG_YEAR] = bin2bcd(tmp);
+	buf[DS1307_REG_YEAR] |= bin2bcd(t->tm_year - 100);
 
 	if (chip->century_enable_bit)
 		buf[chip->century_reg] |= chip->century_enable_bit;
 	if (t->tm_year > 199 && chip->century_bit)
 		buf[chip->century_reg] |= chip->century_bit;
 
-	if (ds1307->type == mcp794xx) {
-		/*
-		 * these bits were cleared when preparing the date/time
-		 * values and need to be set again before writing the
-		 * buffer out to the device.
-		 */
-		buf[DS1307_REG_SECS] |= MCP794XX_BIT_ST;
-		buf[DS1307_REG_WDAY] |= MCP794XX_BIT_VBATEN;
-	}
-
 	dev_dbg(dev, "%s: %7ph\n", "write", buf);
 
-	result = regmap_bulk_write(ds1307->regmap, chip->offset, buf, 7);
-	if (result) {
-		dev_err(dev, "%s error %d\n", "write", result);
-		return result;
-	}
-	return 0;
+	ret = regmap_bulk_write(ds1307->regmap, chip->offset, buf, 7);
+	if (ret)
+		dev_err(dev, "%s error %d\n", "write", ret);
+
+	return ret;
 }
 
 static int ds1337_read_alarm(struct device *dev, struct rtc_wkalrm *t)
-- 
2.14.1

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox