From: Sebastian Reichel <sre@kernel.org>
To: Alexandre Belloni <alexandre.belloni@free-electrons.com>
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 02:56:34 +0100 [thread overview]
Message-ID: <20170222015634.ugadzezywlrjduyx@earth> (raw)
In-Reply-To: <20170221235212.hik3whcytw6xyevd@piout.net>
[-- Attachment #1: Type: text/plain, Size: 4169 bytes --]
Hi Alexandre,
On Wed, Feb 22, 2017 at 12:52:12AM +0100, Alexandre Belloni wrote:
> The patch has a few checkpatch issues. Some of those should really be
> fixed. Can you do that?
of course.
> 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?
The logic (incl. comments) in this section are from the vendor
kernel driver and there is no documentation for CPCAP as far as
I know. I don't know if the hardware has logic to prevent a race
condition for the cpcap_tm.tod1 == 255 case.
> > + 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.
Yes, but cpcap_get_vendor should get into mainline with the
4.11 mfd pull request. So if you base your 4.12 for-next tree
on 4.11-rc1 everything should be fine.
> > + 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.
Yes. RTC works perfectly fine with just the alarm irq. It also
works perfectly fine with just the 1 Hz irq (except for wakeup).
I would like to keep the irq in the driver, so that it's explicitly
disabled. On Droid 4 mainline kernel is booted via kexec from
Android (AKA bootloader) and Motorola's Android kernel uses the
1 Hz IRQ for some proprietary "secure clock daemon".
I will add a comment.
> > + 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.
I successfully tested wakeup before sending this. But in case your
prefer it to be called before registering the RTC I can move the
call accordingly.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2017-02-22 1:56 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
2017-02-22 1:56 ` Sebastian Reichel [this message]
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=20170222015634.ugadzezywlrjduyx@earth \
--to=sre@kernel.org \
--cc=a.zummo@towertech.it \
--cc=alexandre.belloni@free-electrons.com \
--cc=linux-kernel@vger.kernel.org \
--cc=rtc-linux@googlegroups.com \
--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).