lkml.org 
[lkml]   [2012]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v3 1/2] rtc: add new lp8788 rtc driver
Date
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




\
 
 \ /
  Last update: 2012-12-21 17:21    [W:0.053 / U:1.184 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site