LKML Archive on lore.kernel.org
 help / color / Atom feed
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
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 --]

  parent reply index

Thread overview: 31+ 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 ` [PATCH v3 1/7] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer
2020-10-12  7:19   ` Uwe Kleine-König
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-29 18:52   ` Rob Herring
2020-09-24 19:24 ` [PATCH v3 3/7] mfd: Add base driver " Jonathan Neuschäfer
2020-09-25  9:29   ` Andy Shevchenko
2020-09-25 21:32     ` Jonathan Neuschäfer
2020-09-29 16:37       ` Andy Shevchenko
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-25  6:30   ` Uwe Kleine-König
2020-09-27 21:10     ` Jonathan Neuschäfer
2020-09-28  8:35       ` Uwe Kleine-König
2020-10-02 23:36         ` Jonathan Neuschäfer
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 20:40   ` Andreas Kemnade
2020-10-02 23:41     ` Jonathan Neuschäfer
2020-09-25  5:44   ` Uwe Kleine-König [this message]
2020-10-04  1:43     ` Jonathan Neuschäfer
2020-10-04  8:42       ` Alexandre Belloni
2020-10-11 19:09         ` Jonathan Neuschäfer
2020-10-05  7:35       ` Uwe Kleine-König
2020-09-25  9:36   ` Alexandre Belloni
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-25  5:08 ` [PATCH v3 7/7] ARM: dts: imx50-kobo-aura: Add " Jonathan Neuschäfer
2020-10-07  7:46   ` Krzysztof Kozlowski
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: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git