From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753945AbbCQNoG (ORCPT ); Tue, 17 Mar 2015 09:44:06 -0400 Received: from metis.ext.pengutronix.de ([92.198.50.35]:45756 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753764AbbCQNoB (ORCPT ); Tue, 17 Mar 2015 09:44:01 -0400 Date: Tue, 17 Mar 2015 14:43:45 +0100 From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= To: Eddie Huang Cc: Alessandro Zummo , Matthias Brugger , Mark Rutland , devicetree@vger.kernel.org, srv_heupstream@mediatek.com, Pawel Moll , Ian Campbell , rtc-linux@googlegroups.com, yh.chen@mediatek.com, linux-kernel@vger.kernel.org, Tianping Fang , Rob Herring , Sascha Hauer , Kumar Gala , Grant Likely , yingjoe.chen@mediatek.com, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org Subject: Re: [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver Message-ID: <20150317134345.GG10068@pengutronix.de> References: <1422437276-41334-1-git-send-email-eddie.huang@mediatek.com> <1422437276-41334-3-git-send-email-eddie.huang@mediatek.com> <20150316153048.GC10068@pengutronix.de> <1426595474.24415.18.camel@mtksdaap41> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1426595474.24415.18.camel@mtksdaap41> User-Agent: Mutt/1.5.21 (2010-09-15) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Eddie, On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote: > On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-König wrote: > > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote: > > > [...] > > > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset) > > rtc_read is a bad name for a driver. There are already 6 functions with > > this name in the kernel. Better use a unique prefix. > > I will use prefix mtk_ I would prefer a prefix that is unique to the driver. "mtk_" doesn't work to distinguish between the rtc and a (say) spi driver. What you want here is that if someone reports a bug on any mailinglist with a backtrace you are able to immediately see which driver is affected. > > > [...] > > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data) > > > +{ > > > + struct mt6397_rtc *rtc = data; > > > + u16 irqsta, irqen; > > > + > > > + mutex_lock(&rtc->lock); > > > + irqsta = rtc_read(rtc, RTC_IRQ_STA); > > Do you really need to lock for a single read access? > > I think this lock is necessary, because other thread may access rtc > register at the same time, for example, call mtk_rtc_set_alarm to modify > alarm time. That would be a valid reason if mtk_rtc_set_alarm touched that register twice in a single critical section and the handler must not read the value of the first write. Otherwise it should be fine, shouldn't it? > > > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm) > > > +{ > > > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > > > + > > > + tm->tm_year -= RTC_MIN_YEAR_OFFSET; > > > + tm->tm_mon++; > > > + mutex_lock(&rtc->lock); > > > + rtc_write(rtc, RTC_TC_YEA, tm->tm_year); > > > + rtc_write(rtc, RTC_TC_MTH, tm->tm_mon); > > > + rtc_write(rtc, RTC_TC_DOM, tm->tm_mday); > > > + rtc_write(rtc, RTC_TC_HOU, tm->tm_hour); > > > + rtc_write(rtc, RTC_TC_MIN, tm->tm_min); > > > + rtc_write(rtc, RTC_TC_SEC, tm->tm_sec); > > Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you > > write to it but after you wrote RTC_TC_MIN? > > register value will write to hardware after rtc_write_trigger, so the > racy condition not exist. Ah, it seems the hardware guys did their job. Nice. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | From mboxrd@z Thu Jan 1 00:00:00 1970 From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= Subject: Re: [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver Date: Tue, 17 Mar 2015 14:43:45 +0100 Message-ID: <20150317134345.GG10068@pengutronix.de> References: <1422437276-41334-1-git-send-email-eddie.huang@mediatek.com> <1422437276-41334-3-git-send-email-eddie.huang@mediatek.com> <20150316153048.GC10068@pengutronix.de> <1426595474.24415.18.camel@mtksdaap41> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <1426595474.24415.18.camel@mtksdaap41> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Eddie Huang Cc: Alessandro Zummo , Matthias Brugger , Mark Rutland , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, srv_heupstream-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org, Pawel Moll , Ian Campbell , rtc-linux-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, yh.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Tianping Fang , Rob Herring , Sascha Hauer , Kumar Gala , Grant Likely , yingjoe.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org Hello Eddie, On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote: > On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-K=F6nig wrote: > > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote: > > > [...] > > > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset) > > rtc_read is a bad name for a driver. There are already 6 functions = with > > this name in the kernel. Better use a unique prefix. >=20 > I will use prefix mtk_ I would prefer a prefix that is unique to the driver. "mtk_" doesn't work to distinguish between the rtc and a (say) spi driver. What you want here is that if someone reports a bug on any mailinglist with a backtrace you are able to immediately see which driver is affected. > > > [...] > > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data) > > > +{ > > > + struct mt6397_rtc *rtc =3D data; > > > + u16 irqsta, irqen; > > > + > > > + mutex_lock(&rtc->lock); > > > + irqsta =3D rtc_read(rtc, RTC_IRQ_STA); > > Do you really need to lock for a single read access? >=20 > I think this lock is necessary, because other thread may access rtc > register at the same time, for example, call mtk_rtc_set_alarm to mod= ify > alarm time. That would be a valid reason if mtk_rtc_set_alarm touched that register twice in a single critical section and the handler must not read the value of the first write. Otherwise it should be fine, shouldn't it? > > > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time = *tm) > > > +{ > > > + struct mt6397_rtc *rtc =3D dev_get_drvdata(dev); > > > + > > > + tm->tm_year -=3D RTC_MIN_YEAR_OFFSET; > > > + tm->tm_mon++; > > > + mutex_lock(&rtc->lock); > > > + rtc_write(rtc, RTC_TC_YEA, tm->tm_year); > > > + rtc_write(rtc, RTC_TC_MTH, tm->tm_mon); > > > + rtc_write(rtc, RTC_TC_DOM, tm->tm_mday); > > > + rtc_write(rtc, RTC_TC_HOU, tm->tm_hour); > > > + rtc_write(rtc, RTC_TC_MIN, tm->tm_min); > > > + rtc_write(rtc, RTC_TC_SEC, tm->tm_sec); > > Is this racy? I.e. what happens if RTC_TC_SEC overflows just before= you > > write to it but after you wrote RTC_TC_MIN? >=20 > register value will write to hardware after rtc_write_trigger, so the > racy condition not exist. Ah, it seems the hardware guys did their job. Nice. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig = | Industrial Linux Solutions | http://www.pengutronix.de/= | -- To unsubscribe from this list: send the line "unsubscribe devicetree" i= n the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: u.kleine-koenig@pengutronix.de (Uwe =?iso-8859-1?Q?Kleine-K=F6nig?=) Date: Tue, 17 Mar 2015 14:43:45 +0100 Subject: [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver In-Reply-To: <1426595474.24415.18.camel@mtksdaap41> References: <1422437276-41334-1-git-send-email-eddie.huang@mediatek.com> <1422437276-41334-3-git-send-email-eddie.huang@mediatek.com> <20150316153048.GC10068@pengutronix.de> <1426595474.24415.18.camel@mtksdaap41> Message-ID: <20150317134345.GG10068@pengutronix.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hello Eddie, On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote: > On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-K?nig wrote: > > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote: > > > [...] > > > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset) > > rtc_read is a bad name for a driver. There are already 6 functions with > > this name in the kernel. Better use a unique prefix. > > I will use prefix mtk_ I would prefer a prefix that is unique to the driver. "mtk_" doesn't work to distinguish between the rtc and a (say) spi driver. What you want here is that if someone reports a bug on any mailinglist with a backtrace you are able to immediately see which driver is affected. > > > [...] > > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data) > > > +{ > > > + struct mt6397_rtc *rtc = data; > > > + u16 irqsta, irqen; > > > + > > > + mutex_lock(&rtc->lock); > > > + irqsta = rtc_read(rtc, RTC_IRQ_STA); > > Do you really need to lock for a single read access? > > I think this lock is necessary, because other thread may access rtc > register at the same time, for example, call mtk_rtc_set_alarm to modify > alarm time. That would be a valid reason if mtk_rtc_set_alarm touched that register twice in a single critical section and the handler must not read the value of the first write. Otherwise it should be fine, shouldn't it? > > > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm) > > > +{ > > > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > > > + > > > + tm->tm_year -= RTC_MIN_YEAR_OFFSET; > > > + tm->tm_mon++; > > > + mutex_lock(&rtc->lock); > > > + rtc_write(rtc, RTC_TC_YEA, tm->tm_year); > > > + rtc_write(rtc, RTC_TC_MTH, tm->tm_mon); > > > + rtc_write(rtc, RTC_TC_DOM, tm->tm_mday); > > > + rtc_write(rtc, RTC_TC_HOU, tm->tm_hour); > > > + rtc_write(rtc, RTC_TC_MIN, tm->tm_min); > > > + rtc_write(rtc, RTC_TC_SEC, tm->tm_sec); > > Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you > > write to it but after you wrote RTC_TC_MIN? > > register value will write to hardware after rtc_write_trigger, so the > racy condition not exist. Ah, it seems the hardware guys did their job. Nice. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |