From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> To: "Jonathan Neuschäfer" <j.neuschaefer@gmx.net> Cc: linux-kernel@vger.kernel.org, Alexandre Belloni <alexandre.belloni@bootlin.com>, Heiko Stuebner <heiko@sntech.de>, linux-pwm@vger.kernel.org, Linus Walleij <linus.walleij@linaro.org>, Thierry Reding <thierry.reding@gmail.com>, Fabio Estevam <festevam@gmail.com>, linux-rtc@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>, Mauro Carvalho Chehab <mchehab+huawei@kernel.org>, Sam Ravnborg <sam@ravnborg.org>, Daniel Palmer <daniel@0x0f.com>, Andy Shevchenko <andy.shevchenko@gmail.com>, Andreas Kemnade <andreas@kemnade.info>, NXP Linux Team <linux-imx@nxp.com>, devicetree@vger.kernel.org, Stephan Gerhold <stephan@gerhold.net>, allen <allen.chen@ite.com.tw>, Sascha Hauer <s.hauer@pengutronix.de>, Lubomir Rintel <lkundrak@v3.sk>, Rob Herring <robh+dt@kernel.org>, Lee Jones <lee.jones@linaro.org>, linux-arm-kernel@lists.infradead.org, Alessandro Zummo <a.zummo@towertech.it>, Mark Brown <broonie@kernel.org>, Pengutronix Kernel Team <kernel@pengutronix.de>, Heiko Stuebner <heiko.stuebner@theobroma-systems.com>, Josua Mayer <josua.mayer@jm0.eu>, Shawn Guo <shawnguo@kernel.org>, "David S. Miller" <davem@davemloft.net> Subject: Re: [PATCH v3 5/7] rtc: New driver for RTC in Netronix embedded controller Date: Fri, 25 Sep 2020 07:44:24 +0200 [thread overview] Message-ID: <20200925054424.snlr3lggnsv575wu@pengutronix.de> (raw) In-Reply-To: <20200924192455.2484005-6-j.neuschaefer@gmx.net> [-- Attachment #1: Type: text/plain, Size: 3299 bytes --] Hello Jonathan, On Thu, Sep 24, 2020 at 09:24:53PM +0200, Jonathan Neuschäfer wrote: > +#define NTXEC_REG_WRITE_YEAR 0x10 > +#define NTXEC_REG_WRITE_MONTH 0x11 > +#define NTXEC_REG_WRITE_DAY 0x12 > +#define NTXEC_REG_WRITE_HOUR 0x13 > +#define NTXEC_REG_WRITE_MINUTE 0x14 > +#define NTXEC_REG_WRITE_SECOND 0x15 > + > +#define NTXEC_REG_READ_YM 0x20 > +#define NTXEC_REG_READ_DH 0x21 > +#define NTXEC_REG_READ_MS 0x23 Is this an official naming? I think at least ..._MS is a poor name. Maybe consider ..._MINSEC instead and make the other two names a bit longer for consistency? > +static int ntxec_read_time(struct device *dev, struct rtc_time *tm) > +{ > + struct ntxec_rtc *rtc = dev_get_drvdata(dev); > + unsigned int value; > + int res; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value); > + if (res < 0) > + return res; > + > + tm->tm_year = (value >> 8) + 100; > + tm->tm_mon = (value & 0xff) - 1; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value); > + if (res < 0) > + return res; > + > + tm->tm_mday = value >> 8; > + tm->tm_hour = value & 0xff; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value); > + if (res < 0) > + return res; > + > + tm->tm_min = value >> 8; > + tm->tm_sec = value & 0xff; > + > + return 0; > +} > + > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct ntxec_rtc *rtc = dev_get_drvdata(dev); > + int res = 0; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min)); > + if (res) > + return res; > + > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec)); I wonder: Is this racy? If you write minute, does the seconds reset to zero or something like that? Or can it happen, that after writing the minute register and before writing the second register the seconds overflow and you end up with the time set to a minute later than intended? If so it might be worth to set the seconds to 0 at the start of the function (with an explaining comment). .read_time has a similar race. What happens if minutes overflow between reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS? > +static struct platform_driver ntxec_rtc_driver = { > + .driver = { > + .name = "ntxec-rtc", > + }, > + .probe = ntxec_rtc_probe, No .remove function? > +}; > +module_platform_driver(ntxec_rtc_driver); > + > +MODULE_AUTHOR("Jonathan Neuschäfer <j.neuschaefer@gmx.net>"); > +MODULE_DESCRIPTION("RTC driver for Netronix EC"); > +MODULE_LICENSE("GPL"); Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> To: "Jonathan Neuschäfer" <j.neuschaefer@gmx.net> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>, Heiko Stuebner <heiko@sntech.de>, devicetree@vger.kernel.org, Linus Walleij <linus.walleij@linaro.org>, Thierry Reding <thierry.reding@gmail.com>, Sam Ravnborg <sam@ravnborg.org>, linux-rtc@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>, Mauro Carvalho Chehab <mchehab+huawei@kernel.org>, Lee Jones <lee.jones@linaro.org>, Daniel Palmer <daniel@0x0f.com>, Andy Shevchenko <andy.shevchenko@gmail.com>, Andreas Kemnade <andreas@kemnade.info>, NXP Linux Team <linux-imx@nxp.com>, linux-pwm@vger.kernel.org, Stephan Gerhold <stephan@gerhold.net>, allen <allen.chen@ite.com.tw>, Sascha Hauer <s.hauer@pengutronix.de>, Lubomir Rintel <lkundrak@v3.sk>, Rob Herring <robh+dt@kernel.org>, Fabio Estevam <festevam@gmail.com>, linux-arm-kernel@lists.infradead.org, Alessandro Zummo <a.zummo@towertech.it>, linux-kernel@vger.kernel.org, Mark Brown <broonie@kernel.org>, Pengutronix Kernel Team <kernel@pengutronix.de>, Heiko Stuebner <heiko.stuebner@theobroma-systems.com>, Josua Mayer <josua.mayer@jm0.eu>, Shawn Guo <shawnguo@kernel.org>, "David S. Miller" <davem@davemloft.net> Subject: Re: [PATCH v3 5/7] rtc: New driver for RTC in Netronix embedded controller Date: Fri, 25 Sep 2020 07:44:24 +0200 [thread overview] Message-ID: <20200925054424.snlr3lggnsv575wu@pengutronix.de> (raw) In-Reply-To: <20200924192455.2484005-6-j.neuschaefer@gmx.net> [-- Attachment #1.1: Type: text/plain, Size: 3299 bytes --] Hello Jonathan, On Thu, Sep 24, 2020 at 09:24:53PM +0200, Jonathan Neuschäfer wrote: > +#define NTXEC_REG_WRITE_YEAR 0x10 > +#define NTXEC_REG_WRITE_MONTH 0x11 > +#define NTXEC_REG_WRITE_DAY 0x12 > +#define NTXEC_REG_WRITE_HOUR 0x13 > +#define NTXEC_REG_WRITE_MINUTE 0x14 > +#define NTXEC_REG_WRITE_SECOND 0x15 > + > +#define NTXEC_REG_READ_YM 0x20 > +#define NTXEC_REG_READ_DH 0x21 > +#define NTXEC_REG_READ_MS 0x23 Is this an official naming? I think at least ..._MS is a poor name. Maybe consider ..._MINSEC instead and make the other two names a bit longer for consistency? > +static int ntxec_read_time(struct device *dev, struct rtc_time *tm) > +{ > + struct ntxec_rtc *rtc = dev_get_drvdata(dev); > + unsigned int value; > + int res; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value); > + if (res < 0) > + return res; > + > + tm->tm_year = (value >> 8) + 100; > + tm->tm_mon = (value & 0xff) - 1; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value); > + if (res < 0) > + return res; > + > + tm->tm_mday = value >> 8; > + tm->tm_hour = value & 0xff; > + > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value); > + if (res < 0) > + return res; > + > + tm->tm_min = value >> 8; > + tm->tm_sec = value & 0xff; > + > + return 0; > +} > + > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct ntxec_rtc *rtc = dev_get_drvdata(dev); > + int res = 0; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour)); > + if (res) > + return res; > + > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min)); > + if (res) > + return res; > + > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec)); I wonder: Is this racy? If you write minute, does the seconds reset to zero or something like that? Or can it happen, that after writing the minute register and before writing the second register the seconds overflow and you end up with the time set to a minute later than intended? If so it might be worth to set the seconds to 0 at the start of the function (with an explaining comment). .read_time has a similar race. What happens if minutes overflow between reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS? > +static struct platform_driver ntxec_rtc_driver = { > + .driver = { > + .name = "ntxec-rtc", > + }, > + .probe = ntxec_rtc_probe, No .remove function? > +}; > +module_platform_driver(ntxec_rtc_driver); > + > +MODULE_AUTHOR("Jonathan Neuschäfer <j.neuschaefer@gmx.net>"); > +MODULE_DESCRIPTION("RTC driver for Netronix EC"); > +MODULE_LICENSE("GPL"); Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-09-25 5:44 UTC|newest] Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-24 19:24 [PATCH v3 0/7] Netronix embedded controller driver for Kobo and Tolino ebook readers Jonathan Neuschäfer 2020-09-24 19:24 ` Jonathan Neuschäfer 2020-09-24 19:24 ` [PATCH v3 1/7] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer 2020-09-24 19:24 ` Jonathan Neuschäfer 2020-10-12 7:19 ` Uwe Kleine-König 2020-10-12 7:19 ` Uwe Kleine-König 2020-10-12 11:48 ` Lee Jones 2020-10-12 11:48 ` Lee Jones 2020-09-24 19:24 ` [PATCH v3 2/7] dt-bindings: mfd: Add binding for Netronix embedded controller Jonathan Neuschäfer 2020-09-24 19:24 ` Jonathan Neuschäfer 2020-09-29 18:52 ` Rob Herring 2020-09-29 18:52 ` Rob Herring 2020-09-24 19:24 ` [PATCH v3 3/7] mfd: Add base driver " Jonathan Neuschäfer 2020-09-24 19:24 ` Jonathan Neuschäfer 2020-09-25 9:29 ` Andy Shevchenko 2020-09-25 9:29 ` Andy Shevchenko 2020-09-25 21:32 ` Jonathan Neuschäfer 2020-09-25 21:32 ` Jonathan Neuschäfer 2020-09-29 16:37 ` Andy Shevchenko 2020-09-29 16:37 ` Andy Shevchenko 2020-10-02 22:54 ` Jonathan Neuschäfer 2020-10-02 22:54 ` Jonathan Neuschäfer 2020-09-24 19:24 ` [PATCH v3 4/7] pwm: ntxec: Add driver for PWM function in Netronix EC Jonathan Neuschäfer 2020-09-24 19:24 ` Jonathan Neuschäfer 2020-09-25 6:30 ` Uwe Kleine-König 2020-09-25 6:30 ` Uwe Kleine-König 2020-09-27 21:10 ` Jonathan Neuschäfer 2020-09-27 21:10 ` Jonathan Neuschäfer 2020-09-28 8:35 ` Uwe Kleine-König 2020-09-28 8:35 ` Uwe Kleine-König 2020-10-02 23:36 ` Jonathan Neuschäfer 2020-10-02 23:36 ` Jonathan Neuschäfer 2020-10-03 8:17 ` Andy Shevchenko 2020-10-03 8:17 ` Andy Shevchenko 2020-09-24 19:24 ` [PATCH v3 5/7] rtc: New driver for RTC in Netronix embedded controller Jonathan Neuschäfer 2020-09-24 19:24 ` Jonathan Neuschäfer 2020-09-24 20:40 ` Andreas Kemnade 2020-09-24 20:40 ` Andreas Kemnade 2020-10-02 23:41 ` Jonathan Neuschäfer 2020-10-02 23:41 ` Jonathan Neuschäfer 2020-09-25 5:44 ` Uwe Kleine-König [this message] 2020-09-25 5:44 ` Uwe Kleine-König 2020-10-04 1:43 ` Jonathan Neuschäfer 2020-10-04 1:43 ` Jonathan Neuschäfer 2020-10-04 8:42 ` Alexandre Belloni 2020-10-04 8:42 ` Alexandre Belloni 2020-10-11 19:09 ` Jonathan Neuschäfer 2020-10-11 19:09 ` Jonathan Neuschäfer 2020-10-05 7:35 ` Uwe Kleine-König 2020-10-05 7:35 ` Uwe Kleine-König 2020-09-25 9:36 ` Alexandre Belloni 2020-09-25 9:36 ` Alexandre Belloni 2020-09-25 22:00 ` Jonathan Neuschäfer 2020-09-25 22:00 ` Jonathan Neuschäfer 2020-09-24 19:24 ` [PATCH v3 6/7] MAINTAINERS: Add entry for " Jonathan Neuschäfer 2020-09-24 19:24 ` Jonathan Neuschäfer 2020-09-25 5:08 ` [PATCH v3 7/7] ARM: dts: imx50-kobo-aura: Add " Jonathan Neuschäfer 2020-09-25 5:08 ` Jonathan Neuschäfer 2020-10-07 7:46 ` Krzysztof Kozlowski 2020-10-07 7:46 ` Krzysztof Kozlowski 2020-10-11 19:42 ` Jonathan Neuschäfer 2020-10-11 19:42 ` Jonathan Neuschäfer
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200925054424.snlr3lggnsv575wu@pengutronix.de \ --to=u.kleine-koenig@pengutronix.de \ --cc=a.zummo@towertech.it \ --cc=alexandre.belloni@bootlin.com \ --cc=allen.chen@ite.com.tw \ --cc=andreas@kemnade.info \ --cc=andy.shevchenko@gmail.com \ --cc=arnd@arndb.de \ --cc=broonie@kernel.org \ --cc=daniel@0x0f.com \ --cc=davem@davemloft.net \ --cc=devicetree@vger.kernel.org \ --cc=festevam@gmail.com \ --cc=heiko.stuebner@theobroma-systems.com \ --cc=heiko@sntech.de \ --cc=j.neuschaefer@gmx.net \ --cc=josua.mayer@jm0.eu \ --cc=kernel@pengutronix.de \ --cc=lee.jones@linaro.org \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pwm@vger.kernel.org \ --cc=linux-rtc@vger.kernel.org \ --cc=lkundrak@v3.sk \ --cc=mchehab+huawei@kernel.org \ --cc=robh+dt@kernel.org \ --cc=s.hauer@pengutronix.de \ --cc=sam@ravnborg.org \ --cc=shawnguo@kernel.org \ --cc=stephan@gerhold.net \ --cc=thierry.reding@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.