Re: [PATCH v2 5/5] rtc: rtc-rc5t619: add ricoh rc5t619 RTC driver

Andreas Kemnade
2019-11-29T07:00:21Z

[-- 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 --]