linux-kernel - Re: [PATCH v3 1/2] rtc: add new lp8788 rtc driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Fri, 21 Dec 2012 23:15:17 +0530
From:	"devendra.aaru" <devendra.aaru@...il.com>
To:	"Kim, Milo" <Milo.Kim@...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

Hello,


On Fri, Dec 21, 2012 at 9:39 PM, Kim, Milo <Milo.Kim@...com> wrote:
> 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!
>
yes,  i agree.

A very good explanation indeed thanks.

> Best Regards,
> Milo
>
>

thanks,
--
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/