From: Andreas Kemnade <andreas@kemnade.info>
To: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: lee.jones@linaro.org, a.zummo@towertech.it,
linux-kernel@vger.kernel.org, linux-rtc@vger.kernel.org,
phh@phh.me, b.galvani@gmail.com, stefan@agner.ch,
letux-kernel@openphoenux.org
Subject: Re: [PATCH v2 5/5] rtc: rtc-rc5t619: add ricoh rc5t619 RTC driver
Date: Fri, 29 Nov 2019 07:59:40 +0100 [thread overview]
Message-ID: <20191129075940.3b1c2631@kemnade.info> (raw)
In-Reply-To: <20191128105751.GM299836@piout.net>
[-- Attachment #1: Type: text/plain, Size: 4099 bytes --]
Hi,
On Thu, 28 Nov 2019 11:57:51 +0100
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
> Hello,
>
> checkpatch.pl --strict complains about multiple blank lines and alignment.
>
I have not used the strict flag there. But I think I can make
--strict happy.
> On 31/10/2019 22:38:35+0100, Andreas Kemnade wrote:
> > +static int rc5t619_rtc_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct rn5t618 *rn5t618 = dev_get_drvdata(pdev->dev.parent);
> > + struct rc5t619_rtc *rtc;
> > + uint8_t alarm_flag;
> > + unsigned int ctrl2;
> > + int err;
> > +
> > + rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL);
> > + if (IS_ERR(rtc)) {
> > + err = PTR_ERR(rtc);
> > + return -ENOMEM;
> > + }
> > +
> > + rtc->rn5t618 = rn5t618;
> > +
> > + dev_set_drvdata(dev, rtc);
> > + rtc->irq = -1;
> > +
> > + if (rn5t618->irq_data)
> > + rtc->irq = regmap_irq_get_virq(rn5t618->irq_data,
> > + RN5T618_IRQ_RTC);
> > +
> > + if (rtc->irq < 0) {
> > + dev_err(dev, "no irq specified, wakeup is disabled\n");
>
> I don't think it is worth having an error message here, especially since
> you have a second one later.
>
agreed.
> > + rtc->irq = -1;
> > + }
> > +
> > + err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2, &ctrl2);
> > + if (err < 0)
> > + return err;
> > +
> > + /* get interrupt flag */
> > + err = rc5t619_rtc_alarm_is_enabled(dev, &alarm_flag);
> > + if (err)
> > + return err;
> > +
> > + /* disable rtc periodic function */
> > + err = rc5t619_rtc_periodic_disable(&pdev->dev);
> > + if (err)
> > + return err;
> > +
> > + /* disable interrupt */
> > + err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> > + if (err)
> > + return err;
>
> Is it really useful to disable the alarm to reenable them later?
>
Well, yes, seems to be nonsense.
Am I right that I do not need to prevent alarm irqs between
alloc() and register()?
> > +
> > + if (ctrl2 & CTRL2_PON) {
> > + alarm_flag = 0;
> > + err = rc5t619_rtc_alarm_flag_clr(&pdev->dev);
> > + if (err)
> > + return err;
> > + }
> > +
> > + rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
> > +
>
> Please remove this blank line.
>
Ok.
> > + if (IS_ERR(rtc->rtc)) {
> > + err = PTR_ERR(rtc->rtc);
> > + dev_err(dev, "RTC device register: err %d\n", err);
> > + return err;
> > + }
> > +
> > + rtc->rtc->ops = &rc5t619_rtc_ops;
> > + rtc->rtc->range_min = RTC_TIMESTAMP_BEGIN_1900;
> > + rtc->rtc->range_max = RTC_TIMESTAMP_END_2099;
> > +
> > + /* set interrupt and enable it */
> > + if (rtc->irq != -1) {
> > + device_init_wakeup(&pdev->dev, 1);
> > +
> > + err = devm_request_threaded_irq(&pdev->dev,
> > rtc->irq, NULL,
> > + rc5t619_rtc_irq,
> > + IRQF_ONESHOT,
> > + "rtc-rc5t619",
> > + &pdev->dev);
> > + if (err < 0) {
> > + dev_err(&pdev->dev, "request IRQ:%d
> > fail\n", rtc->irq);
> > + rtc->irq = -1;
> > +
> > + err = rc5t619_rtc_alarm_enable(&pdev->dev,
> > 0);
> > + if (err)
> > + return err;
> > +
> > + } else {
> > + /* enable wake */
>
> I think you should move device_init_wakeup() here, unless your parse
> the wakeup-source property.
>
yes, makes sense.
> > + enable_irq_wake(rtc->irq);
> > + /* enable alarm_d */
> > + err = rc5t619_rtc_alarm_enable(&pdev->dev,
> > alarm_flag);
> > + if (err) {
> > + dev_err(&pdev->dev, "failed rtc
> > setup\n");
> > + return -EBUSY;
> > + }
> > + }
> > + } else {
> > + /* system don't want to using alarm interrupt, so
> > close it */
> > + err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> > + if (err) {
> > + dev_err(&pdev->dev, "disable rtc alarm
> > error\n");
>
> I don't think this message is necessary.
>
yes, agreed, that would be just another symptom of an i2c problem.
> > + return err;
> > + }
> > +
> > + dev_err(&pdev->dev, "ricoh61x interrupt is
> > disabled\n");
>
> Maybe dev_warn as the driver just continues on.
>
Ok.
Regards,
Andreas
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2019-11-29 7:00 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-31 21:38 [PATCH v2 0/5] Add rtc support for rn5t618 mfd Andreas Kemnade
2019-10-31 21:38 ` [PATCH v2 1/5] mfd: rn5t618: prepare for irq handling Andreas Kemnade
2019-11-16 0:41 ` kbuild test robot
2019-10-31 21:38 ` [PATCH v2 2/5] mfd: rn5t618: add irq support Andreas Kemnade
2019-11-06 21:48 ` Andreas Kemnade
2019-11-16 6:52 ` kbuild test robot
2019-11-20 7:54 ` Pierre-Hugues Husson
2019-11-20 8:13 ` Andreas Kemnade
2019-10-31 21:38 ` [PATCH v2 3/5] mfd: rn5t618: add rtc related registers Andreas Kemnade
2019-10-31 21:38 ` [PATCH v2 4/5] mfd: rn5t618: add more subdevices Andreas Kemnade
2019-10-31 21:38 ` [PATCH v2 5/5] rtc: rtc-rc5t619: add ricoh rc5t619 RTC driver Andreas Kemnade
2019-11-28 10:57 ` Alexandre Belloni
2019-11-29 6:59 ` Andreas Kemnade [this message]
2019-11-29 8:55 ` Alexandre Belloni
2019-11-29 11:25 ` Andreas Kemnade
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=20191129075940.3b1c2631@kemnade.info \
--to=andreas@kemnade.info \
--cc=a.zummo@towertech.it \
--cc=alexandre.belloni@bootlin.com \
--cc=b.galvani@gmail.com \
--cc=lee.jones@linaro.org \
--cc=letux-kernel@openphoenux.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rtc@vger.kernel.org \
--cc=phh@phh.me \
--cc=stefan@agner.ch \
/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).