| Message ID | 1369092090-5384-1-git-send-email-ynvich@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, 21 May 2013 03:21:30 +0400 Sergey Yanovich <ynvich@gmail.com> wrote: > This chip has a control register and can prevent altering saved clock. > Without this patch we could have: > ----8<---- > (arm)root@pac14:~# date > Tue May 21 03:08:27 MSK 2013 > (arm)root@pac14:~# /etc/init.d/hwclock.sh show > Tue May 21 11:13:58 2013 -0.067322 seconds > (arm)root@pac14:~# /etc/init.d/hwclock.sh stop > [info] Saving the system clock. > [info] Hardware Clock updated to Tue May 21 03:09:01 MSK 2013. > (arm)root@pac14:~# /etc/init.d/hwclock.sh show > Tue May 21 11:14:15 2013 -0.624272 seconds > ----8<---- > > ... > > --- a/drivers/rtc/rtc-ds1302.c > +++ b/drivers/rtc/rtc-ds1302.c > @@ -23,8 +23,12 @@ > #define RTC_CMD_READ 0x81 /* Read command */ > #define RTC_CMD_WRITE 0x80 /* Write command */ > > +#define RTC_CMD_WRITE_ENABLE 0x00 /* Write enable */ > +#define RTC_CMD_WRITE_DISABLE 0x80 /* Write disable */ > + > #define RTC_ADDR_RAM0 0x20 /* Address of RAM0 */ > #define RTC_ADDR_TCR 0x08 /* Address of trickle charge register */ > +#define RTC_ADDR_CTRL 0x07 /* Address of control register */ > #define RTC_ADDR_YEAR 0x06 /* Address of year register */ > #define RTC_ADDR_DAY 0x05 /* Address of day of week register */ > #define RTC_ADDR_MON 0x04 /* Address of month register */ > @@ -313,6 +317,7 @@ static int __init ds1302_rtc_probe(struct platform_device *pdev) > return PTR_ERR(rtc); > > platform_set_drvdata(pdev, rtc); > + ds1302_writebyte(RTC_ADDR_CTRL, RTC_CMD_WRITE_ENABLE); > > return 0; > } > @@ -321,6 +326,7 @@ static int ds1302_rtc_remove(struct platform_device *pdev) > { > struct rtc_device *rtc = platform_get_drvdata(pdev); > > + ds1302_writebyte(RTC_ADDR_CTRL, RTC_CMD_WRITE_DISABLE); > rtc_device_unregister(rtc); > platform_set_drvdata(pdev, NULL); ds1302_rtc_remove() no longer exists in my tree - it got whittled away to nothing by http://ozlabs.org/~akpm/mmots/broken-out/rtc-rtc-ds1302-remove-unnecessary-platform_set_drvdata.patch and http://ozlabs.org/~akpm/mmots/broken-out/drivers-rtc-rtc-ds1302c-remove-empty-function.patch Perhaps it should be re-added for this?
On Wed, 2013-05-29 at 15:53 -0700, Andrew Morton wrote: > On Tue, 21 May 2013 03:21:30 +0400 Sergey Yanovich <ynvich@gmail.com> wrote: > @@ -321,6 +326,7 @@ static int ds1302_rtc_remove(struct platform_device *pdev) > > { > > struct rtc_device *rtc = platform_get_drvdata(pdev); > > > > + ds1302_writebyte(RTC_ADDR_CTRL, RTC_CMD_WRITE_DISABLE); > > rtc_device_unregister(rtc); > > platform_set_drvdata(pdev, NULL); > > ds1302_rtc_remove() no longer exists in my tree - it got whittled away > to nothing by > http://ozlabs.org/~akpm/mmots/broken-out/rtc-rtc-ds1302-remove-unnecessary-platform_set_drvdata.patch > and > http://ozlabs.org/~akpm/mmots/broken-out/drivers-rtc-rtc-ds1302c-remove-empty-function.patch > > Perhaps it should be re-added for this? There are 2 options. I would be happy with either. 1. I've chosen 'probe/remove' to enable/disable write access. 2. Another option is to wrap enable/disable around ds1302_rtc_set_time(). IIUC, the former saves a few bytes of memory. However, now, when ds1302_rtc_remove() is gone, the latter looks better. So I could rewrite the patch either way.
On Thu, 30 May 2013 14:14:42 +0400, Sergey Yanovich <ynvich@gmail.com> wrote: > On Wed, 2013-05-29 at 15:53 -0700, Andrew Morton wrote: >> On Tue, 21 May 2013 03:21:30 +0400 Sergey Yanovich <ynvich@gmail.com> >> wrote: >> @@ -321,6 +326,7 @@ static int ds1302_rtc_remove(struct platform_device >> *pdev) >> > { >> > struct rtc_device *rtc = platform_get_drvdata(pdev); >> > >> > + ds1302_writebyte(RTC_ADDR_CTRL, RTC_CMD_WRITE_DISABLE); >> > rtc_device_unregister(rtc); >> > platform_set_drvdata(pdev, NULL); >> >> ds1302_rtc_remove() no longer exists in my tree - it got whittled away >> to nothing by >> http://ozlabs.org/~akpm/mmots/broken-out/rtc-rtc-ds1302-remove-unnecessary-platform_set_drvdata.patch >> and >> http://ozlabs.org/~akpm/mmots/broken-out/drivers-rtc-rtc-ds1302c-remove-empty-function.patch >> >> Perhaps it should be re-added for this? > > There are 2 options. I would be happy with either. > > 1. I've chosen 'probe/remove' to enable/disable write access. > > 2. Another option is to wrap enable/disable around > ds1302_rtc_set_time(). > > IIUC, the former saves a few bytes of memory. However, now, when > ds1302_rtc_remove() is gone, the latter looks better. So I could rewrite > the patch either way. Option two looks actually safer to me, as it ensures that an unexpected reboot outside of the set_time section doesn't leave write access enabled. You never know what firmware could do while you're not looking... M.
diff --git a/drivers/rtc/rtc-ds1302.c b/drivers/rtc/rtc-ds1302.c index 73eafb9..626aec2 100644 --- a/drivers/rtc/rtc-ds1302.c +++ b/drivers/rtc/rtc-ds1302.c @@ -23,8 +23,12 @@ #define RTC_CMD_READ 0x81 /* Read command */ #define RTC_CMD_WRITE 0x80 /* Write command */ +#define RTC_CMD_WRITE_ENABLE 0x00 /* Write enable */ +#define RTC_CMD_WRITE_DISABLE 0x80 /* Write disable */ + #define RTC_ADDR_RAM0 0x20 /* Address of RAM0 */ #define RTC_ADDR_TCR 0x08 /* Address of trickle charge register */ +#define RTC_ADDR_CTRL 0x07 /* Address of control register */ #define RTC_ADDR_YEAR 0x06 /* Address of year register */ #define RTC_ADDR_DAY 0x05 /* Address of day of week register */ #define RTC_ADDR_MON 0x04 /* Address of month register */ @@ -313,6 +317,7 @@ static int __init ds1302_rtc_probe(struct platform_device *pdev) return PTR_ERR(rtc); platform_set_drvdata(pdev, rtc); + ds1302_writebyte(RTC_ADDR_CTRL, RTC_CMD_WRITE_ENABLE); return 0; } @@ -321,6 +326,7 @@ static int ds1302_rtc_remove(struct platform_device *pdev) { struct rtc_device *rtc = platform_get_drvdata(pdev); + ds1302_writebyte(RTC_ADDR_CTRL, RTC_CMD_WRITE_DISABLE); rtc_device_unregister(rtc); platform_set_drvdata(pdev, NULL);
This chip has a control register and can prevent altering saved clock. Without this patch we could have: ----8<---- (arm)root@pac14:~# date Tue May 21 03:08:27 MSK 2013 (arm)root@pac14:~# /etc/init.d/hwclock.sh show Tue May 21 11:13:58 2013 -0.067322 seconds (arm)root@pac14:~# /etc/init.d/hwclock.sh stop [info] Saving the system clock. [info] Hardware Clock updated to Tue May 21 03:09:01 MSK 2013. (arm)root@pac14:~# /etc/init.d/hwclock.sh show Tue May 21 11:14:15 2013 -0.624272 seconds ----8<---- Signed-off-by: Sergey Yanovich <ynvich@gmail.com> --- drivers/rtc/rtc-ds1302.c | 6 ++++++ 1 file changed, 6 insertions(+)