Messages in this thread | | | Subject | Re: [PATCH] rtc-ds1302: handle write protection | Date | Thu, 30 May 2013 12:20:33 +0200 | From | Marc Zyngier <> |
| |
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. -- Who you jivin' with that Cosmik Debris?
| |