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
WARNING: multiple messages have this Message-ID (diff)
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: [rtc-linux] 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 -- You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. --- You received this message because you are subscribed to the Google Groups "rtc-linux" group. To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
next prev parent reply other threads:[~2017-02-21 23:52 UTC|newest] Thread overview: 49+ 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 ` [rtc-linux] " Sebastian Reichel 2017-02-20 7:35 ` [PATCH 1/1] rtc: cpcap: new rtc driver Sebastian Reichel 2017-02-20 7:35 ` [rtc-linux] " Sebastian Reichel 2017-02-20 16:31 ` Tony Lindgren 2017-02-20 16:31 ` [rtc-linux] " Tony Lindgren 2017-02-20 16:38 ` Alexandre Belloni 2017-02-20 16:38 ` [rtc-linux] " Alexandre Belloni 2017-02-20 17:21 ` Tony Lindgren 2017-02-20 17:21 ` [rtc-linux] " Tony Lindgren 2017-02-20 17:27 ` Tony Lindgren 2017-02-20 17:27 ` [rtc-linux] " Tony Lindgren 2017-02-20 19:35 ` Sebastian Reichel 2017-02-20 19:35 ` [rtc-linux] " Sebastian Reichel 2017-02-21 6:16 ` [PATCHv2] " Sebastian Reichel 2017-02-21 6:16 ` [rtc-linux] " Sebastian Reichel 2017-02-21 23:52 ` Alexandre Belloni [this message] 2017-02-21 23:52 ` [rtc-linux] " Alexandre Belloni 2017-02-22 1:56 ` Sebastian Reichel 2017-02-22 1:56 ` [rtc-linux] " Sebastian Reichel 2017-02-22 8:18 ` Alexandre Belloni 2017-02-22 8:18 ` [rtc-linux] " Alexandre Belloni 2017-02-23 1:03 ` [PATCHv3 1/2] dt-bindings: Add vendor prefix for Motorola Sebastian Reichel 2017-02-23 1:03 ` Sebastian Reichel 2017-02-23 1:03 ` [rtc-linux] " Sebastian Reichel 2017-02-23 1:03 ` [PATCHv3 2/2] rtc: cpcap: new rtc driver Sebastian Reichel 2017-02-23 1:03 ` Sebastian Reichel 2017-02-23 1:03 ` [rtc-linux] " Sebastian Reichel 2017-02-27 23:49 ` Rob Herring 2017-02-27 23:49 ` Rob Herring 2017-02-27 23:49 ` [rtc-linux] " Rob Herring 2017-02-27 23:48 ` [PATCHv3 1/2] dt-bindings: Add vendor prefix for Motorola Rob Herring 2017-02-27 23:48 ` Rob Herring 2017-02-27 23:48 ` [rtc-linux] " Rob Herring 2017-03-02 0:27 ` [PATCHv4 " Sebastian Reichel 2017-03-02 0:27 ` Sebastian Reichel 2017-03-02 0:27 ` [rtc-linux] " Sebastian Reichel 2017-03-02 0:27 ` [PATCHv4 2/2] rtc: cpcap: new rtc driver Sebastian Reichel 2017-03-02 0:27 ` Sebastian Reichel 2017-03-02 0:27 ` [rtc-linux] " Sebastian Reichel 2017-03-02 14:11 ` Rob Herring 2017-03-02 14:11 ` Rob Herring 2017-03-02 14:11 ` [rtc-linux] " Rob Herring 2017-03-09 0:34 ` Alexandre Belloni 2017-03-09 0:34 ` Alexandre Belloni 2017-03-09 0:34 ` [rtc-linux] " Alexandre Belloni 2017-03-09 0:33 ` [PATCHv4 1/2] dt-bindings: Add vendor prefix for Motorola Alexandre Belloni 2017-03-09 0:33 ` Alexandre Belloni 2017-03-09 0:33 ` [rtc-linux] " 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: 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.