On 17/12/2015 at 11:37:06 +0000, Opensource [Steve Twiss] wrote :
> On 16 December 2015 23:47 Alexandre Belloni wrote:
> > Subject: Re: [PATCH V1] rtc: da9063: access ordering error during RTC
> > interrupt system power on
> >
> > This seems mostly fine, however ...
>
> Hi Alexandre,
> Thanks for reviewing this.
>
> > On 08/12/2015 at 16:28:39 +0000, Steve Twiss wrote :
> > > irq_alarm = platform_get_irq_byname(pdev, "ALARM");
> > > ret = devm_request_threaded_irq(&pdev->dev, irq_alarm, NULL,
> > > da9063_alarm_event,
> > > IRQF_TRIGGER_LOW |
> > IRQF_ONESHOT,
> > > "ALARM", rtc);
> > > - if (ret) {
> > > + if (ret)
> > > dev_err(&pdev->dev, "Failed to request ALARM IRQ %d:
> > %d\n",
> > > irq_alarm, ret);
> > > - return ret;
> > > - }
> > > -
> >
> > ... now that requesting the interrupt is optional, you probably want to
> > prevent userspace from thinking it will get an interrupt after setting
> > the alarm by returning -EINVAL in da9063_rtc_read_alarm() and
> > da9063_rtc_set_alarm() in that case.
> >
>
> .. I'm not quite certain I understand.
> Does the patch looks worse that it is?
> This part,
>
> + if (ret)
> dev_err(&pdev->dev, "Failed to request ALARM IRQ %d: %d\n",
> irq_alarm, ret);
> - return ret;
>
> looks like it has erased the return ret,
>
> >
> > > - rtc->rtc_dev = devm_rtc_device_register(&pdev->dev,
> > DA9063_DRVNAME_RTC,
> > > - &da9063_rtc_ops, THIS_MODULE);
> > > - if (IS_ERR(rtc->rtc_dev))
> > > - return PTR_ERR(rtc->rtc_dev);
> > >
> > > - da9063_data_to_tm(data, &rtc->alarm_time, rtc);
> > > - rtc->rtc_sync = false;
> > > return ret;
>
> But it does exist at the end of the patch.
> So there will still be an error sent if the IRQ is not requested properly.
> Is this what you meant in your previous e-mail?
>
Indeed, you are right, I'll apply that patch.
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
