| 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
| ||
|
Message-ID: <CAHdPZaOrVQ-oU_+N621izgFedjvWBCJGh2vDub4HjzVemusG7w@mail.gmail.com>
Date: Fri, 21 Dec 2012 17:59:30 +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 1:25 PM, Kim, Milo <Milo.Kim@...com> wrote:
> TI LP8788 PMU supports regulators, battery charger, RTC, ADC, backlight driver
> and current sinks.
> This patch enables LP8788 RTC module.
>
Nice one, but i just have some queries or questions below,
> Patch v3.
> remove __devinit and __devexit.
> add weekday definition and comments.
>
> Patch v2.
> use IRQ domain for handling alarm IRQ.
> replace module_init() and module_exit() with module_platform_driver().
> clean up code for _probe() and _remove().
>
> Patch v1.
> initial patch
>
> Signed-off-by: Milo(Woogyom) Kim <milo.kim@...com>
> ---
> diff --git a/drivers/rtc/rtc-lp8788.c b/drivers/rtc/rtc-lp8788.c
> new file mode 100644
> index 0000000..2363e52
> --- /dev/null
> +++ b/drivers/rtc/rtc-lp8788.c
> @@ -0,0 +1,345 @@
> +/*
> + * TI LP8788 MFD - rtc driver
> + *
> + * Copyright 2012 Texas Instruments
> + *
> + * Author: Milo(Woogyom) Kim <milo.kim@...com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/err.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mfd/lp8788.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/rtc.h>
> +#include <linux/slab.h>
> +
> +/* register address */
> +#define LP8788_INTEN_3 0x05
> +#define LP8788_RTC_UNLOCK 0x64
> +#define LP8788_RTC_SEC 0x70
> +#define LP8788_ALM1_SEC 0x77
> +#define LP8788_ALM1_EN 0x7D
> +#define LP8788_ALM2_SEC 0x7E
> +#define LP8788_ALM2_EN 0x84
> +
> +/* mask/shift bits */
> +#define LP8788_INT_RTC_ALM1_M BIT(1) /* Addr 05h */
> +#define LP8788_INT_RTC_ALM1_S 1
> +#define LP8788_INT_RTC_ALM2_M BIT(2) /* Addr 05h */
> +#define LP8788_INT_RTC_ALM2_S 2
> +#define LP8788_ALM_EN_M BIT(7) /* Addr 7Dh or 84h */
> +#define LP8788_ALM_EN_S 7
> +
> +#define DEFAULT_ALARM_SEL LP8788_ALARM_1
> +#define LP8788_MONTH_OFFSET 1
> +#define LP8788_BASE_YEAR 2000
> +#define MAX_WDAY_BITS 7
> +#define LP8788_WDAY_SET 1
> +#define RTC_UNLOCK 0x1
> +#define RTC_LATCH 0x2
> +#define ALARM_IRQ_FLAG (RTC_IRQF | RTC_AF)
> +
> +enum lp8788_time {
> + LPTIME_SEC,
> + LPTIME_MIN,
> + LPTIME_HOUR,
> + LPTIME_MDAY,
> + LPTIME_MON,
> + LPTIME_YEAR,
> + LPTIME_WDAY,
> + LPTIME_MAX,
> +};
> +
> +struct lp8788_rtc {
> + struct lp8788 *lp;
> + struct rtc_device *rdev;
> + enum lp8788_alarm_sel alarm;
> + int irq;
> +};
> +
> +static const u8 addr_alarm_sec[LP8788_ALARM_MAX] = {
> + LP8788_ALM1_SEC,
> + LP8788_ALM2_SEC,
> +};
> +
> +static const u8 addr_alarm_en[LP8788_ALARM_MAX] = {
> + LP8788_ALM1_EN,
> + LP8788_ALM2_EN,
> +};
> +
> +static const u8 mask_alarm_en[LP8788_ALARM_MAX] = {
> + LP8788_INT_RTC_ALM1_M,
> + LP8788_INT_RTC_ALM2_M,
> +};
> +
> +static const u8 shift_alarm_en[LP8788_ALARM_MAX] = {
> + LP8788_INT_RTC_ALM1_S,
> + LP8788_INT_RTC_ALM2_S,
> +};
> +
<snip>
> +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.
> +}
> +
> +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?
> + return 0;
> + }
> +
> + r = platform_get_resource_byname(pdev, IORESOURCE_IRQ, LP8788_ALM_IRQ);
> + if (!r)
> + return 0;
> +
> + if (rtc->alarm == LP8788_ALARM_1)
> + irq = r->start;
> + else
> + irq = r->end;
> +
> + rtc->irq = irq_create_mapping(irqdm, irq);
> +
> + return request_threaded_irq(rtc->irq, NULL, lp8788_alarm_irq_handler,
> + 0, LP8788_ALM_IRQ, rtc);
> +}
> +
> +static void lp8788_alarm_irq_unregister(struct lp8788_rtc *rtc)
> +{
> + if (rtc->irq)
> + free_irq(rtc->irq, rtc);
> +}
> +
> +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?
> + 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?
> + 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 ?
> + return 0;
> +}
> +
> --
> 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/
--
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