Re: [PATCH] rtc: driver for the DryIce block found in i.MX25 chips

[Andrew Morton <akpm@linux-foundation.org>]

On Wed,  2 Jun 2010 08:37:21 +0300
Baruch Siach <baruch@tkos.co.il> wrote:

> This driver is based on code from Freescale which accompanies their i.MX25 PDK
> board, with some cleanup.

Do we know who at freescale worked on this?  Are there any signoffs or
attributions we could be adding?

>
> ...
>
> +static inline void di_int_enable(struct imxdi_dev *imxdi, u32 intr)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&imxdi->irq_lock, flags);
> +	__raw_writel(__raw_readl(imxdi->ioaddr + DIER) | intr,
> +			imxdi->ioaddr + DIER);
> +	spin_unlock_irqrestore(&imxdi->irq_lock, flags);
> +}
> +
> +/*
> + * disable a dryice interrupt
> + */
> +static inline void di_int_disable(struct imxdi_dev *imxdi, u32 intr)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&imxdi->irq_lock, flags);
> +	__raw_writel(__raw_readl(imxdi->ioaddr + DIER) & ~intr,
> +			imxdi->ioaddr + DIER);
> +	spin_unlock_irqrestore(&imxdi->irq_lock, flags);
> +}

You may find that you'll end up with a smaller and faster driver if
these are uninlined.

You may well find that gcc went and uninlined them anwyay.

> +/*
> + * This function attempts to clear the dryice write-error flag.
> + *
> + * A dryice write error is similar to a bus fault and should not occur in
> + * normal operation.  Clearing the flag requires another write, so the root
> + * cause of the problem may need to be fixed before the flag can be cleared.
> + */
> +static void clear_write_error(struct imxdi_dev *imxdi)
> +{
> +	int cnt;
> +
> +	dev_warn(&imxdi->pdev->dev, "WARNING: Register write error!\n");
> +
> +	for (;;) {
> +		/* clear the write error flag */
> +		__raw_writel(DSR_WEF, imxdi->ioaddr + DSR);
> +
> +		/* wait for it to take effect */
> +		for (cnt = 0; cnt < 100; cnt++) {
> +			if ((__raw_readl(imxdi->ioaddr + DSR) & DSR_WEF) == 0)
> +				return;
> +			udelay(10);
> +		}
> +		dev_err(&imxdi->pdev->dev,
> +			"ERROR: Cannot clear write-error flag!\n");
> +	}
> +}

The potential infinite loop is a worry.

> +/*
> + * Write a dryice register and wait until it completes.
> + *
> + * This function uses interrupts to determine when the
> + * write has completed.
> + */
> +static int di_write_wait(struct imxdi_dev *imxdi, u32 val, int reg)
> +{
> +	int ret;
> +	int rc = 0;
> +
> +	/* serialize register writes */
> +	mutex_lock(&imxdi->write_mutex);
> +
> +	/* enable the write-complete interrupt */
> +	di_int_enable(imxdi, DIER_WCIE);
> +
> +	imxdi->dsr = 0;
> +
> +	/* do the register write */
> +	__raw_writel(val, imxdi->ioaddr + reg);
> +
> +	/* wait for the write to finish */
> +	ret = wait_event_interruptible_timeout(imxdi->write_wait,
> +			imxdi->dsr & (DSR_WCF | DSR_WEF), msecs_to_jiffies(1));

This wait will terminate immediately if the calling task has
signal_pending().

> +	if (ret == 0)
> +		dev_warn(&imxdi->pdev->dev,
> +				"Write-wait timeout "
> +				"val = 0x%08x reg = 0x%08x\n", val, reg);

But the code incorrectly assumes that the hardware signalled readiness.

> +	/* check for write error */
> +	if (imxdi->dsr & DSR_WEF) {
> +		clear_write_error(imxdi);
> +		rc = -EIO;
> +	}
> +	mutex_unlock(&imxdi->write_mutex);
> +	return rc;
> +}
> +
>
> ...
>
> +static int __devexit dryice_rtc_remove(struct platform_device *pdev)
> +{
> +	struct imxdi_dev *imxdi = platform_get_drvdata(pdev);
> +
> +	flush_scheduled_work();

We only need to wait for imxdi->work to complete, so a simple
flush_work() would suffice here.

> +	/* mask all interrupts */
> +	__raw_writel(0, imxdi->ioaddr + DIER);
> +
> +	rtc_device_unregister(imxdi->rtc);
> +
> +	clk_disable(imxdi->clk);
> +	clk_put(imxdi->clk);
> +
> +	return 0;
> +}
>
> ...
>