[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <A874F61F95741C4A9BA573A70FE3998F69E10977@DQHE02.ent.ti.com>
Date: Fri, 21 Dec 2012 16:09:30 +0000
From: "Kim, Milo" <Milo.Kim@...com>
To: "devendra.aaru" <devendra.aaru@...il.com>
CC: "a.zummo@...ertech.it" <a.zummo@...ertech.it>,
Andrew Morton <akpm@...ux-foundation.org>,
"rtc-linux@...glegroups.com" <rtc-linux@...glegroups.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Samuel Ortiz <sameo@...ux.intel.com>
Subject: RE: [PATCH v3 1/2] rtc: add new lp8788 rtc driver
Hi Devendra,
> > +static int lp8788_alarm_irq_enable(struct device *dev, unsigned int
> enable)
> > +{
> > + struct lp8788_rtc *rtc = dev_get_drvdata(dev);
> > + struct lp8788 *lp = rtc->lp;
> > + u8 mask, shift;
> > +
> > + mask = mask_alarm_en[rtc->alarm];
> > + shift = shift_alarm_en[rtc->alarm];
> > +
> > + return lp8788_update_bits(lp, LP8788_INTEN_3, mask, enable <<
> shift);
>
> i have no idea about this function but just asking,
>
> this will be called from ioctl right? so what if user calls RTC_AIE_ON
> and you return the IRQ status ? enable or disable like that?
>
> i guess a failure in setting for no irq and a success otherwise.
Please go to my answer below - at the end of mail.
>
> > +}
> > +
> > +static const struct rtc_class_ops lp8788_rtc_ops = {
> > + .read_time = lp8788_rtc_read_time,
> > + .set_time = lp8788_rtc_set_time,
> > + .read_alarm = lp8788_read_alarm,
> > + .set_alarm = lp8788_set_alarm,
> > + .alarm_irq_enable = lp8788_alarm_irq_enable,
> > +};
> > +
> > +static irqreturn_t lp8788_alarm_irq_handler(int irq, void *ptr)
> > +{
> > + struct lp8788_rtc *rtc = ptr;
> > +
> > + rtc_update_irq(rtc->rdev, 1, ALARM_IRQ_FLAG);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int lp8788_alarm_irq_register(struct platform_device *pdev,
> > + struct lp8788_rtc *rtc)
> > +{
> > + struct resource *r;
> > + struct lp8788 *lp = rtc->lp;
> > + struct irq_domain *irqdm = lp->irqdm;
> > + int irq;
> > +
> > + rtc->irq = 0;
> > +
> > + if (!lp->pdata) {
> > + dev_warn(lp->dev, "no platform data for rtc irq\n");
>
> but we didn't registered irqs right? if we return 0 here means that we
> have a vaild irq no?
Oh, it's my mistake.
Alarm IRQ number is configured in the mfd-lp8788 driver and map it by irq-domain.
Therefore no platform data is required here.
> > +static int lp8788_rtc_probe(struct platform_device *pdev)
> > +{
> > + struct lp8788 *lp = dev_get_drvdata(pdev->dev.parent);
> > + struct lp8788_rtc *rtc;
> > + struct device *dev = &pdev->dev;
> > +
> > + rtc = devm_kzalloc(lp->dev, sizeof(struct lp8788_rtc),
> GFP_KERNEL);
>
> lp->dev may be parent device, is &pdev->dev, or dev may be correct?
Great catch! You're correct. Thanks!
>
> > + if (!rtc)
> > + return -ENOMEM;
> > +
> > + rtc->lp = lp;
> > + rtc->alarm = lp->pdata ? lp->pdata->alarm_sel :
> DEFAULT_ALARM_SEL;
> > + platform_set_drvdata(pdev, rtc);
> > +
> > + device_init_wakeup(dev, 1);
> > +
> > + rtc->rdev = rtc_device_register("lp8788_rtc", dev,
> > + &lp8788_rtc_ops, THIS_MODULE);
> > + if (IS_ERR(rtc->rdev)) {
> > + dev_err(dev, "can not register rtc device\n");
>
> return PTR_ERR(rtc->rdev); may be better?
Agree, thanks!
>
> > + return -ENODEV;
> > + }
> > +
> > + if (lp8788_alarm_irq_register(pdev, rtc))
> > + dev_warn(lp->dev, "no rtc irq handler\n");
> > +
>
> even if there is no alarm irq, we can still use the rtc to get the
> time, but there wont be RTC_AIE_ON ioctl enabled ?
If ALARM IRQ can't be registered, there are two handling conditions.
(a) Should it return as error in _probe() and no RTC is used anymore?
: Very simple handling but we can't set/get the RTC time even no problem.
(b) Just returns OK in _probe() and error handling when RTC_AIE_ON/OFF IOCTL is
called?
: It guarantees RTC time functions but alarm IRQ doesn't work.
Please see the code below.
static int lp8788_alarm_irq_enable(struct device *dev, unsigned int enable)
{
struct lp8788_rtc *rtc = dev_get_drvdata(dev);
struct lp8788 *lp = rtc->lp;
u8 mask, shift;
+ if (!rtc->irq)
+ return -EIO;
mask = mask_alarm_en[rtc->alarm];
shift = shift_alarm_en[rtc->alarm];
return lp8788_update_bits(lp, LP8788_INTEN_3, mask, enable << shift);
}
'rtc->irq' is updated only when lp8788_alarm_irq_register() returns true.
If no alarm IRQ is specified, RTC_AIE_ON/OFF returns as EIO.
Then, RTC set/get time works well and alarm IRQ doesn't work.
I believe case (b) is better solution, but I'd like to have your opinion.
Thanks!
Best Regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists