From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
To: Sebastian Reichel <sre@kernel.org>
Cc: Tony Lindgren <tony@atomide.com>,
Alessandro Zummo <a.zummo@towertech.it>,
rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCHv2] rtc: cpcap: new rtc driver
Date: Wed, 22 Feb 2017 00:52:12 +0100 [thread overview]
Message-ID: <20170221235212.hik3whcytw6xyevd@piout.net> (raw)
In-Reply-To: <20170221061650.12636-1-sre@kernel.org>
Hi,
The patch has a few checkpatch issues. Some of those should really be
fixed. Can you do that?
Else, it is mostly fine, a few comments below.
On 21/02/2017 at 07:16:50 +0100, Sebastian Reichel wrote:
> +static int cpcap_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct cpcap_rtc *rtc;
> + struct cpcap_time cpcap_tm;
> + int ret = 0;
> +
> + rtc = dev_get_drvdata(dev);
> +
> + rtc2cpcap_time(&cpcap_tm, tm);
> +
> + if (rtc->alarm_enabled)
> + disable_irq(rtc->alarm_irq);
> + if (rtc->update_enabled)
> + disable_irq(rtc->update_irq);
> +
> + if (rtc->vendor == CPCAP_VENDOR_ST) {
> + /* The TOD1 and TOD2 registers MUST be written in this order
> + * for the change to properly set. */
Does this mean there is a race condition?
> + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1,
> + TOD1_MASK, cpcap_tm.tod1);
> + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD2,
> + TOD2_MASK, cpcap_tm.tod2);
> + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_DAY,
> + DAY_MASK, cpcap_tm.day);
> + } else {
> + /* Clearing the upper lower 8 bits of the TOD guarantees that
> + * the upper half of TOD (TOD2) will not increment for 0xFF RTC
> + * ticks (255 seconds). During this time we can safely write
> + * to DAY, TOD2, then TOD1 (in that order) and expect RTC to be
> + * synchronized to the exact time requested upon the final write
> + * to TOD1. */
> + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1,
> + TOD1_MASK, 0);
> + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_DAY,
> + DAY_MASK, cpcap_tm.day);
> + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD2,
> + TOD2_MASK, cpcap_tm.tod2);
> + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1,
> + TOD1_MASK, cpcap_tm.tod1);
> + }
> +
> + err = cpcap_get_vendor(dev, rtc->regmap, &rtc->vendor);
I think this means it depends on the mfd tree.
> + if (err)
> + return err;
> +
> + rtc->alarm_irq= platform_get_irq(pdev, 0);
> + err = devm_request_threaded_irq(dev, rtc->alarm_irq, NULL,
> + cpcap_rtc_alarm_irq, IRQ_NONE,
> + "rtc_alarm", rtc);
> + if (err) {
> + dev_err(dev, "Could not request alarm irq: %d\n", err);
> + return err;
> + }
> + disable_irq(rtc->alarm_irq);
> +
> + rtc->update_irq= platform_get_irq(pdev, 1);
> + err = devm_request_threaded_irq(dev, rtc->update_irq, NULL,
> + cpcap_rtc_update_irq, IRQ_NONE,
> + "rtc_1hz", rtc);
I don't think this IRQ is actually useful. It doesn't really harm but
the tests should pass without it.
> + if (err) {
> + dev_err(dev, "Could not request update irq: %d\n", err);
> + return err;
> + }
> + disable_irq(rtc->update_irq);
> +
> + err = device_init_wakeup(dev, 1);
If you use device_init_wakeup, I think it needs to be called before
devm_rtc_device_register() to properly work.
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
next prev parent reply other threads:[~2017-02-21 23:52 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-20 7:35 [PATCH 0/1] Motorola CPCAP PMIC RTC Sebastian Reichel
2017-02-20 7:35 ` [PATCH 1/1] rtc: cpcap: new rtc driver Sebastian Reichel
2017-02-20 16:31 ` Tony Lindgren
2017-02-20 16:38 ` Alexandre Belloni
2017-02-20 17:21 ` Tony Lindgren
2017-02-20 17:27 ` Tony Lindgren
2017-02-20 19:35 ` Sebastian Reichel
2017-02-21 6:16 ` [PATCHv2] " Sebastian Reichel
2017-02-21 23:52 ` Alexandre Belloni [this message]
2017-02-22 1:56 ` Sebastian Reichel
2017-02-22 8:18 ` Alexandre Belloni
2017-02-23 1:03 ` [PATCHv3 1/2] dt-bindings: Add vendor prefix for Motorola Sebastian Reichel
2017-02-23 1:03 ` [PATCHv3 2/2] rtc: cpcap: new rtc driver Sebastian Reichel
2017-02-27 23:49 ` Rob Herring
2017-02-27 23:48 ` [PATCHv3 1/2] dt-bindings: Add vendor prefix for Motorola Rob Herring
2017-03-02 0:27 ` [PATCHv4 " Sebastian Reichel
2017-03-02 0:27 ` [PATCHv4 2/2] rtc: cpcap: new rtc driver Sebastian Reichel
2017-03-02 14:11 ` Rob Herring
2017-03-09 0:34 ` Alexandre Belloni
2017-03-09 0:33 ` [PATCHv4 1/2] dt-bindings: Add vendor prefix for Motorola Alexandre Belloni
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=20170221235212.hik3whcytw6xyevd@piout.net \
--to=alexandre.belloni@free-electrons.com \
--cc=a.zummo@towertech.it \
--cc=linux-kernel@vger.kernel.org \
--cc=rtc-linux@googlegroups.com \
--cc=sre@kernel.org \
--cc=tony@atomide.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).