Re: [PATCH v2] rtc: add support for Freescale SNVS RTC

[Sascha Hauer]

On Sat, Apr 14, 2012 at 03:07:17PM +0800, Ying-Chun Liu (PaulLiu) wrote:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@xxxxxxxxxx>
> 
> This adds an RTC driver for the Low Power (LP) section of SNVS.
> It hooks into the /dev/rtc interface and supports ioctl to complete RTC
> features. This driver supports device tree bindings.
> It only uses the RTC hw in non-secure mode.
> 
> Signed-off-by: Anish Trivedi <anish@xxxxxxxxxxxxx>
> Signed-off-by: Eric Miao <eric.miao@xxxxxxxxxx>
> Signed-off-by: Anson Huang <b20788@xxxxxxxxxxxxx>
> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@xxxxxxxxxx>
> Cc: Alessandro Zummo <a.zummo@xxxxxxxxxxxx>
> Cc: Shawn Guo <shawn.guo@xxxxxxxxxx>
> Cc: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> ---
>  Documentation/devicetree/bindings/rtc/snvs-rtc.txt |   18 +
>  drivers/rtc/Kconfig                                |   10 +
>  drivers/rtc/Makefile                               |    1 +
>  drivers/rtc/rtc-snvs.c                             |  644 ++++++++++++++++++++
>  4 files changed, 673 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/snvs-rtc.txt
>  create mode 100644 drivers/rtc/rtc-snvs.c
> 
> +#define CNTR_TO_SECS_SH 15	/* Converts 47-bit counter to 32-bit seconds */
> +
> +struct rtc_drv_data {
> +	struct rtc_device *rtc;
> +	void __iomem *ioaddr;
> +	int irq;
> +	bool irq_enable;
> +};
> +
> +static DEFINE_SPINLOCK(rtc_lock);

Put this three lines above.

> +	writel(time >> (32 - CNTR_TO_SECS_SH), ioaddr + SNVS_LPSRTCMR);
> +
> +	/* Enable RTC again */
> +	writel(lp_cr | SNVS_LPCR_SRTC_ENV, ioaddr + SNVS_LPCR);
> +	timeout = jiffies + msecs_to_jiffies(3000);
> +	while (!(readl(ioaddr + SNVS_LPCR) & SNVS_LPCR_SRTC_ENV)) {
> +		if (time_after(jiffies, timeout)) {
> +			dev_err(dev, "timeout on enabling RTC back\n");
> +			return -EBUSY;
> +		}
> +	}
> +
> +	rtc_write_sync_lp(ioaddr);
> +
> +	new_time_47bit = (((u64) (readl(ioaddr + SNVS_LPSRTCMR) &
> +		((0x1 << CNTR_TO_SECS_SH) - 1)) << 32) |
> +		((u64) readl(ioaddr + SNVS_LPSRTCLR)));
> +
> +	time_diff = new_time_47bit - old_time_47bit;

time_diff, old_time_47bit and new_time_47bit are set and never read.
Remove.

> +
> +	return 0;
> +}
> +
> +static int snvs_rtc_proc(struct device *dev, struct seq_file *seq)
> +{
> +	struct rtc_drv_data *pdata = dev_get_drvdata(dev);
> +	void __iomem *ioaddr = pdata->ioaddr;
> +
> +	seq_printf(seq, "alarm_IRQ\t: %s\n",
> +		   (((readl(ioaddr + SNVS_LPCR)) & SNVS_LPCR_LPTA_EN) !=
> +		    0) ? "yes" : "no");
> +
> +	return 0;
> +}
> +
> +static int snvs_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
> +{
> +	struct rtc_drv_data *pdata = dev_get_drvdata(dev);
> +	void __iomem *ioaddr = pdata->ioaddr;
> +	u32 lp_cr;
> +	unsigned long lock_flags = 0;
> +
> +	spin_lock_irqsave(&rtc_lock, lock_flags);
> +
> +	if (enable) {
> +		if (!pdata->irq_enable) {
> +			enable_irq(pdata->irq);
> +			pdata->irq_enable = true;
> +		}

Why this enable_irq/disable_irq everywhere? You seem to have irq enable
bits in your registers which should be enough.

> +/* SNVS RTC Power management control */
> +static int __devinit snvs_rtc_probe(struct platform_device *pdev)
> +{
> +	struct timespec tv;
> +	struct rtc_device *rtc;
> +	struct rtc_drv_data *pdata = NULL;

No need to initialize this. I think you should use another name for this
variable. People usually expect pdata for platform_data. Driver data is
usually called priv, or snvs_rtc in this case.


> +	void __iomem *ioaddr;
> +	u32 lp_cr;
> +	int ret = 0;

No need to initialize this.

> +
> +	ioaddr = of_iomap(pdev->dev.of_node, 0);
> +	if (!ioaddr)
> +		return -EADDRNOTAVAIL;
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	pdata->ioaddr = ioaddr;
> +
> +	/* Configure and enable the RTC */
> +	pdata->irq = platform_get_irq(pdev, 0);
> +	if (pdata->irq >= 0) {

Is there any hardware without an interrupt? If no, just bail out here
and get rid of the additional complexity.

> +		if (request_irq(pdata->irq, snvs_rtc_interrupt, IRQF_SHARED,
> +				pdev->name, pdev) < 0) {
> +			dev_warn(&pdev->dev, "interrupt not available.\n");
> +			pdata->irq = -1;
> +		} else {
> +			disable_irq(pdata->irq);
> +			pdata->irq_enable = false;
> +		}
> +	}
> +
> +	/* initialize glitch detect */
> +	writel(SNVS_LPPGDR_INIT, ioaddr + SNVS_LPPGDR);
> +	udelay(100);
> +
> +	/* clear lp interrupt status */
> +	writel(0xFFFFFFFF, ioaddr + SNVS_LPSR);
> +
> +	/* Enable RTC */
> +	lp_cr = readl(ioaddr + SNVS_LPCR);
> +	if ((lp_cr & SNVS_LPCR_SRTC_ENV) == 0)
> +		writel(lp_cr | SNVS_LPCR_SRTC_ENV, ioaddr + SNVS_LPCR);
> +	udelay(100);
> +
> +	writel(0xFFFFFFFF, ioaddr + SNVS_LPSR);
> +	udelay(100);
> +
> +	platform_set_drvdata(pdev, pdata);
> +
> +	rtc = rtc_device_register(pdev->name, &pdev->dev,
> +				  &snvs_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(rtc)) {
> +		ret = PTR_ERR(rtc);
> +		goto err_out;
> +	}
> +
> +	pdata->rtc = rtc;
> +
> +	tv.tv_nsec = 0;
> +	tv.tv_sec = rtc_read_lp_counter(ioaddr);
> +
> +	/* By default, devices should wakeup if they can */
> +	/* So snvs is set as "should wakeup" as it can */
> +	device_init_wakeup(&pdev->dev, 1);
> +
> +	return ret;

return 0 here instead to make it clear that this is the success path.

> +
> +err_out:
> +	iounmap(ioaddr);
> +	if (pdata->irq >= 0)
> +		free_irq(pdata->irq, pdev);
> +
> +	return ret;
> +}
> +
> +static int __devexit snvs_rtc_remove(struct platform_device *pdev)
> +{
> +	struct rtc_drv_data *pdata = platform_get_drvdata(pdev);
> +	rtc_device_unregister(pdata->rtc);
> +	if (pdata->irq >= 0)
> +		free_irq(pdata->irq, pdev);
> +	iounmap(pdata->ioaddr);
> +
> +	return 0;
> +}
> +

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/