[PATCH 1/3] spi/s3c64xx: Log error interrupts

[PATCH 1/3] spi/s3c64xx: Log error interrupts

[Mark Brown]

Although the hardware supports interrupts we're not currently using them
at all since for small transfers the overhead is greater than that for
busy waiting and for large transfers we have interrupts from the DMA.
This means that if the hardware reports an error (especially one which
might not stall transfer) we might miss it.

Take a first pass at dealing with such errors by enabling the interrupt
if we can and logging the errors if they happen. Ideally we'd report the
error via the affected transfer but since we're in master mode it's very
difficult to trigger errors at present and this code is much simpler.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-s3c64xx.c |   56 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index dcf7e10..9b16406 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -20,6 +20,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/workqueue.h>
+#include <linux/interrupt.h>
 #include <linux/delay.h>
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
@@ -153,6 +154,7 @@ struct s3c64xx_spi_dma_data {
  * @tx_dmach: Controller's DMA channel for Tx.
  * @sfr_start: BUS address of SPI controller regs.
  * @regs: Pointer to ioremap'ed controller registers.
+ * @irq: interrupt
  * @xfer_completion: To indicate completion of xfer task.
  * @cur_mode: Stores the active configuration of the controller.
  * @cur_bpw: Stores the active bits per word settings.
@@ -930,6 +932,33 @@ setup_exit:
 	return err;
 }
 
+static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
+{
+	struct s3c64xx_spi_driver_data *sdd = data;
+	struct spi_master *spi = sdd->master;
+	unsigned int val;
+
+	val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
+
+	val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
+		S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
+		S3C64XX_SPI_PND_TX_OVERRUN_CLR |
+		S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
+
+	writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
+
+	if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
+		dev_err(&spi->dev, "RX overrun\n");
+	if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
+		dev_err(&spi->dev, "RX underrun\n");
+	if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
+		dev_err(&spi->dev, "TX overrun\n");
+	if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
+		dev_err(&spi->dev, "TX underrun\n");
+
+	return IRQ_HANDLED;
+}
+
 static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int channel)
 {
 	struct s3c64xx_spi_info *sci = sdd->cntrlr_info;
@@ -970,7 +999,7 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 	struct s3c64xx_spi_driver_data *sdd;
 	struct s3c64xx_spi_info *sci;
 	struct spi_master *master;
-	int ret;
+	int ret, irq;
 	char clk_name[16];
 
 	if (pdev->id < 0) {
@@ -1006,6 +1035,12 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_warn(&pdev->dev, "Failed to get IRQ: %d\n", irq);
+		return irq;
+	}
+
 	master = spi_alloc_master(&pdev->dev,
 				sizeof(struct s3c64xx_spi_driver_data));
 	if (master == NULL) {
@@ -1100,10 +1135,21 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 	INIT_WORK(&sdd->work, s3c64xx_spi_work);
 	INIT_LIST_HEAD(&sdd->queue);
 
+	ret = request_irq(irq, s3c64xx_spi_irq, 0, "spi-s3c64xx", sdd);
+	if (ret != 0) {
+		dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n",
+			irq, ret);
+		goto err8;
+	}
+
+	writel(S3C64XX_SPI_INT_RX_OVERRUN_EN | S3C64XX_SPI_INT_RX_UNDERRUN_EN |
+	       S3C64XX_SPI_INT_TX_OVERRUN_EN | S3C64XX_SPI_INT_TX_UNDERRUN_EN,
+	       sdd->regs + S3C64XX_SPI_INT_EN);
+
 	if (spi_register_master(master)) {
 		dev_err(&pdev->dev, "cannot register SPI master\n");
 		ret = -EBUSY;
-		goto err8;
+		goto err9;
 	}
 
 	dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d "
@@ -1115,6 +1161,8 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 
 	return 0;
 
+err9:
+	free_irq(irq, sdd);
 err8:
 	destroy_workqueue(sdd->workqueue);
 err7:
@@ -1153,6 +1201,10 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 
 	spi_unregister_master(master);
 
+	writel(0, sdd->regs + S3C64XX_SPI_INT_EN);
+
+	free_irq(platform_get_irq(pdev, 0), sdd);
+
 	destroy_workqueue(sdd->workqueue);
 
 	clk_disable(sdd->src_clk);
-- 
1.7.7.3

[PATCH 2/3] spi/s3c64xx: Convert to dev_pm_ops

[Mark Brown]

In preparation for the addition of runtime PM ops.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/spi/spi-s3c64xx.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 9b16406..d0c70f2 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1226,9 +1226,9 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
-static int s3c64xx_spi_suspend(struct platform_device *pdev, pm_message_t state)
+static int s3c64xx_spi_suspend(struct device *dev)
 {
-	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
+	struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
 	unsigned long flags;
 
@@ -1248,9 +1248,10 @@ static int s3c64xx_spi_suspend(struct platform_device *pdev, pm_message_t state)
 	return 0;
 }
 
-static int s3c64xx_spi_resume(struct platform_device *pdev)
+static int s3c64xx_spi_resume(struct device *dev)
 {
-	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
+	struct platform_device *pdev = to_platform_device(dev);
+	struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
 	struct s3c64xx_spi_info *sci = sdd->cntrlr_info;
 	unsigned long flags;
@@ -1269,19 +1270,19 @@ static int s3c64xx_spi_resume(struct platform_device *pdev)
 
 	return 0;
 }
-#else
-#define s3c64xx_spi_suspend	NULL
-#define s3c64xx_spi_resume	NULL
 #endif /* CONFIG_PM */
 
+static const struct dev_pm_ops s3c64xx_spi_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(s3c64xx_spi_suspend, s3c64xx_spi_resume)
+};
+
 static struct platform_driver s3c64xx_spi_driver = {
 	.driver = {
 		.name	= "s3c64xx-spi",
 		.owner = THIS_MODULE,
+		.pm = &s3c64xx_spi_pm,
 	},
 	.remove = s3c64xx_spi_remove,
-	.suspend = s3c64xx_spi_suspend,
-	.resume = s3c64xx_spi_resume,
 };
 MODULE_ALIAS("platform:s3c64xx-spi");
 
-- 
1.7.7.3

[PATCH 3/3] spi/s3c64xx: Implement runtime PM support

[Mark Brown]

Enable and disable the clocks to the SPI controller using runtime PM. This
serves the dual purpose of reducing power consumption a little and letting
the core know when the device is idle.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/spi/spi-s3c64xx.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index d0c70f2..b899af66 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -25,6 +25,7 @@
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/spi/spi.h>
 
 #include <mach/dma.h>
@@ -782,6 +783,8 @@ static void s3c64xx_spi_work(struct work_struct *work)
 	while (!acquire_dma(sdd))
 		msleep(10);
 
+	pm_runtime_get_sync(&sdd->pdev->dev);
+
 	spin_lock_irqsave(&sdd->lock, flags);
 
 	while (!list_empty(&sdd->queue)
@@ -810,6 +813,8 @@ static void s3c64xx_spi_work(struct work_struct *work)
 	/* Free DMA channels */
 	sdd->ops->release(sdd->rx_dma.ch, &s3c64xx_spi_dma_client);
 	sdd->ops->release(sdd->tx_dma.ch, &s3c64xx_spi_dma_client);
+
+	pm_runtime_put(&sdd->pdev->dev);
 }
 
 static int s3c64xx_spi_transfer(struct spi_device *spi,
@@ -892,6 +897,8 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 		goto setup_exit;
 	}
 
+	pm_runtime_get_sync(&sdd->pdev->dev);
+
 	/* Check if we can provide the requested rate */
 	if (!sci->clk_from_cmu) {
 		u32 psr, speed;
@@ -924,6 +931,8 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 			err = -EINVAL;
 	}
 
+	pm_runtime_put(&sdd->pdev->dev);
+
 setup_exit:
 
 	/* setup() returns with device de-selected */
@@ -1159,6 +1168,8 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 					mem_res->end, mem_res->start,
 					sdd->rx_dma.dmach, sdd->tx_dma.dmach);
 
+	pm_runtime_enable(&pdev->dev);
+
 	return 0;
 
 err9:
@@ -1192,6 +1203,8 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 	struct resource	*mem_res;
 	unsigned long flags;
 
+	pm_runtime_disable(&pdev->dev);
+
 	spin_lock_irqsave(&sdd->lock, flags);
 	sdd->state |= SUSPND;
 	spin_unlock_irqrestore(&sdd->lock, flags);
@@ -1272,8 +1285,34 @@ static int s3c64xx_spi_resume(struct device *dev)
 }
 #endif /* CONFIG_PM */
 
+#ifdef CONFIG_PM_RUNTIME
+static int s3c64xx_spi_runtime_suspend(struct device *dev)
+{
+	struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
+	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
+
+	clk_disable(sdd->clk);
+	clk_disable(sdd->src_clk);
+
+	return 0;
+}
+
+static int s3c64xx_spi_runtime_resume(struct device *dev)
+{
+	struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
+	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
+
+	clk_enable(sdd->src_clk);
+	clk_enable(sdd->clk);
+
+	return 0;
+}
+#endif /* CONFIG_PM_RUNTIME */
+
 static const struct dev_pm_ops s3c64xx_spi_pm = {
 	SET_SYSTEM_SLEEP_PM_OPS(s3c64xx_spi_suspend, s3c64xx_spi_resume)
+	SET_RUNTIME_PM_OPS(s3c64xx_spi_runtime_suspend,
+			   s3c64xx_spi_runtime_resume, NULL)
 };
 
 static struct platform_driver s3c64xx_spi_driver = {
-- 
1.7.7.3

Re: [PATCH 1/3] spi/s3c64xx: Log error interrupts

[Shubhrajyoti Datta]

Hi Mark,

On Mon, Jan 30, 2012 at 3:10 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Sat, Jan 28, 2012 at 10:30:36PM +0530, Shubhrajyoti Datta wrote:
>
> Please delete irrelevant context from your replies.
>
>> > +   ret = request_irq(irq, s3c64xx_spi_irq, 0, "spi-s3c64xx", sdd);
>
>> Could we have a threaded irq instead and there are prints and that may get
>> executed in interrupt context.
>
> So, clearly we *could*.  However I think that's not going to be as
> helpful.  These errors are never supposed to happen (and if they do
> indicate a fairly serious problem) so the performance impact shouldn't
> be our primary concern and if they do happen then my suspicion is that
> the timing information will be very important.  I expect that knowing
> exactly when the error occurred is likely to help diagnose what caused
> it by virtue of knowing what other things were going on in the system at
> that time.
I agree .

My only concern was that incase someone wants to add prints in the irq
that may be handy.
However as you rightly pointed that timing is surely to be more of a concern.
Thanks for the explanation.

Re: [PATCH 1/3] spi/s3c64xx: Log error interrupts

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

On Sat, Jan 28, 2012 at 10:30:36PM +0530, Shubhrajyoti Datta wrote:

Please delete irrelevant context from your replies.

> > +	ret = request_irq(irq, s3c64xx_spi_irq, 0, "spi-s3c64xx", sdd);

> Could we have a threaded irq instead and there are prints and that may get
> executed in interrupt context.

So, clearly we *could*.  However I think that's not going to be as
helpful.  These errors are never supposed to happen (and if they do
indicate a fairly serious problem) so the performance impact shouldn't
be our primary concern and if they do happen then my suspicion is that
the timing information will be very important.  I expect that knowing
exactly when the error occurred is likely to help diagnose what caused
it by virtue of knowing what other things were going on in the system at
that time.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/s3c64xx: Log error interrupts

[Shubhrajyoti Datta]

Hi Mark,
Some minor doubts/ questions.

On 1/21/12, Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> Although the hardware supports interrupts we're not currently using them
> at all since for small transfers the overhead is greater than that for
> busy waiting and for large transfers we have interrupts from the DMA.
> This means that if the hardware reports an error (especially one which
> might not stall transfer) we might miss it.
>
> Take a first pass at dealing with such errors by enabling the interrupt
> if we can and logging the errors if they happen. Ideally we'd report the
> error via the affected transfer but since we're in master mode it's very
> difficult to trigger errors at present and this code is much simpler.
>
> Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
> Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/spi/spi-s3c64xx.c |   57
> +++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 019a716..d56066b 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -20,6 +20,7 @@
>  #include <linux/init.h>
>  #include <linux/module.h>
>  #include <linux/workqueue.h>
> +#include <linux/interrupt.h>
>  #include <linux/delay.h>
>  #include <linux/clk.h>
>  #include <linux/dma-mapping.h>
> @@ -153,6 +154,7 @@ struct s3c64xx_spi_dma_data {
>   * @tx_dmach: Controller's DMA channel for Tx.
>   * @sfr_start: BUS address of SPI controller regs.
>   * @regs: Pointer to ioremap'ed controller registers.
> + * @irq: interrupt
>   * @xfer_completion: To indicate completion of xfer task.
>   * @cur_mode: Stores the active configuration of the controller.
>   * @cur_bpw: Stores the active bits per word settings.
> @@ -930,6 +932,33 @@ setup_exit:
>  	return err;
>  }
>
> +static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
> +{
> +	struct s3c64xx_spi_driver_data *sdd = data;
> +	struct spi_master *spi = sdd->master;
> +	unsigned int val;
> +
> +	val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
> +
> +	val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
> +		S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
> +		S3C64XX_SPI_PND_TX_OVERRUN_CLR |
> +		S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
> +
> +	writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
> +
> +	if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
> +		dev_err(&spi->dev, "RX overrun\n");
> +	if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
> +		dev_err(&spi->dev, "RX underrun\n");
> +	if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
> +		dev_err(&spi->dev, "TX overrun\n");
> +	if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
> +		dev_err(&spi->dev, "TX underrun\n");
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int
> channel)
>  {
>  	struct s3c64xx_spi_info *sci = sdd->cntrlr_info;
> @@ -970,7 +999,8 @@ static int __init s3c64xx_spi_probe(struct
> platform_device *pdev)
>  	struct s3c64xx_spi_driver_data *sdd;
>  	struct s3c64xx_spi_info *sci;
>  	struct spi_master *master;
> -	int ret;
> +	int ret, irq;
> +	char clk_name[16];
>
>  	if (pdev->id < 0) {
>  		dev_err(&pdev->dev,
> @@ -1010,6 +1040,12 @@ static int __init s3c64xx_spi_probe(struct
> platform_device *pdev)
>  		return -ENXIO;
>  	}
>
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_warn(&pdev->dev, "Failed to get IRQ: %d\n", irq);
> +		return irq;
> +	}
> +
>  	master = spi_alloc_master(&pdev->dev,
>  				sizeof(struct s3c64xx_spi_driver_data));
>  	if (master == NULL) {
> @@ -1104,10 +1140,21 @@ static int __init s3c64xx_spi_probe(struct
> platform_device *pdev)
>  	INIT_WORK(&sdd->work, s3c64xx_spi_work);
>  	INIT_LIST_HEAD(&sdd->queue);
>
> +	ret = request_irq(irq, s3c64xx_spi_irq, 0, "spi-s3c64xx", sdd);

Could we have a threaded irq instead and there are prints and that may get
executed in interrupt context.

> +	if (ret != 0) {
> +		dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n",
> +			irq, ret);
> +		goto err8;
> +	}
> +
> +	writel(S3C64XX_SPI_INT_RX_OVERRUN_EN | S3C64XX_SPI_INT_RX_UNDERRUN_EN |
> +	       S3C64XX_SPI_INT_TX_OVERRUN_EN | S3C64XX_SPI_INT_TX_UNDERRUN_EN,
> +	       sdd->regs + S3C64XX_SPI_INT_EN);
> +
>  	if (spi_register_master(master)) {
>  		dev_err(&pdev->dev, "cannot register SPI master\n");
>  		ret = -EBUSY;
> -		goto err8;
> +		goto err9;
>  	}
>
>  	dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d "
> @@ -1119,6 +1166,8 @@ static int __init s3c64xx_spi_probe(struct
> platform_device *pdev)
>
>  	return 0;
>
> +err9:
> +	free_irq(irq, sdd);
>  err8:
>  	destroy_workqueue(sdd->workqueue);
>  err7:
> @@ -1157,6 +1206,10 @@ static int s3c64xx_spi_remove(struct platform_device
> *pdev)
>
>  	spi_unregister_master(master);
>
> +	writel(0, sdd->regs + S3C64XX_SPI_INT_EN);
> +
> +	free_irq(platform_get_irq(pdev, 0), sdd);
> +
>  	destroy_workqueue(sdd->workqueue);
>
>  	clk_disable(sdd->src_clk);
> --
> 1.7.7.3
>
>
> ------------------------------------------------------------------------------
> Try before you buy = See our experts in action!
> The most comprehensive online learning library for Microsoft developers
> is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
> Metro Style Apps, more. Free future releases when you subscribe now!
> http://p.sf.net/sfu/learndevnow-dev2
> _______________________________________________
> spi-devel-general mailing list
> spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/spi-devel-general
>

------------------------------------------------------------------------------
Try before you buy = See our experts in action!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-dev2

Re: [PATCH 1/3] spi/s3c64xx: Log error interrupts

[Sylwester Nawrocki]

On 01/21/2012 04:29 PM, Mark Brown wrote:
> On Sat, Jan 21, 2012 at 04:27:16PM +0100, Sylwester Nawrocki wrote:
>> On 01/21/2012 02:24 PM, Mark Brown wrote:
> 
>>> -	int ret;
>>> +	int ret, irq;
>>> +	char clk_name[16];
> 
>> clk_name seems to be unused, otherwise looks good.
> 
> That's git cherry-pick being too clever for me when I pulled this out of
> my -next branch for Grant.  clk_name is used in -next by some updates
> which I guess Kukjin is carrying, I hadn't noticed they weren't in the
> SPI tree.  I don't know if it's better to just leave things as they are
> for now given that it'll all come out in the merge window?

Indeed, the orphaned variable is already sitting in -next. I guess it
can be handled during merge.

------------------------------------------------------------------------------
Try before you buy = See our experts in action!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-dev2

Re: [PATCH 1/3] spi/s3c64xx: Log error interrupts

[Mark Brown]

On Sat, Jan 21, 2012 at 04:27:16PM +0100, Sylwester Nawrocki wrote:
> On 01/21/2012 02:24 PM, Mark Brown wrote:

> > -	int ret;
> > +	int ret, irq;
> > +	char clk_name[16];

> clk_name seems to be unused, otherwise looks good.

That's git cherry-pick being too clever for me when I pulled this out of
my -next branch for Grant.  clk_name is used in -next by some updates
which I guess Kukjin is carrying, I hadn't noticed they weren't in the
SPI tree.  I don't know if it's better to just leave things as they are
for now given that it'll all come out in the merge window?

------------------------------------------------------------------------------
Try before you buy = See our experts in action!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-dev2

Re: [PATCH 1/3] spi/s3c64xx: Log error interrupts

[Sylwester Nawrocki]

On 01/21/2012 02:24 PM, Mark Brown wrote:
> Although the hardware supports interrupts we're not currently using them
> at all since for small transfers the overhead is greater than that for
> busy waiting and for large transfers we have interrupts from the DMA.
> This means that if the hardware reports an error (especially one which
> might not stall transfer) we might miss it.
> 
> Take a first pass at dealing with such errors by enabling the interrupt
> if we can and logging the errors if they happen. Ideally we'd report the
> error via the affected transfer but since we're in master mode it's very
> difficult to trigger errors at present and this code is much simpler.
> 
> Signed-off-by: Mark Brown<broonie@opensource.wolfsonmicro.com>
> Acked-by: Linus Walleij<linus.walleij@linaro.org>
> ---
...
>   static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int channel)
>   {
>   	struct s3c64xx_spi_info *sci = sdd->cntrlr_info;
> @@ -970,7 +999,8 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
>   	struct s3c64xx_spi_driver_data *sdd;
>   	struct s3c64xx_spi_info *sci;
>   	struct spi_master *master;
> -	int ret;
> +	int ret, irq;
> +	char clk_name[16];

clk_name seems to be unused, otherwise looks good.

--

Thanks,
Sylwester

[PATCH 1/3] spi/s3c64xx: Log error interrupts

[Mark Brown]

Although the hardware supports interrupts we're not currently using them
at all since for small transfers the overhead is greater than that for
busy waiting and for large transfers we have interrupts from the DMA.
This means that if the hardware reports an error (especially one which
might not stall transfer) we might miss it.

Take a first pass at dealing with such errors by enabling the interrupt
if we can and logging the errors if they happen. Ideally we'd report the
error via the affected transfer but since we're in master mode it's very
difficult to trigger errors at present and this code is much simpler.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-s3c64xx.c |   57 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 019a716..d56066b 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -20,6 +20,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/workqueue.h>
+#include <linux/interrupt.h>
 #include <linux/delay.h>
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
@@ -153,6 +154,7 @@ struct s3c64xx_spi_dma_data {
  * @tx_dmach: Controller's DMA channel for Tx.
  * @sfr_start: BUS address of SPI controller regs.
  * @regs: Pointer to ioremap'ed controller registers.
+ * @irq: interrupt
  * @xfer_completion: To indicate completion of xfer task.
  * @cur_mode: Stores the active configuration of the controller.
  * @cur_bpw: Stores the active bits per word settings.
@@ -930,6 +932,33 @@ setup_exit:
 	return err;
 }
 
+static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
+{
+	struct s3c64xx_spi_driver_data *sdd = data;
+	struct spi_master *spi = sdd->master;
+	unsigned int val;
+
+	val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
+
+	val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
+		S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
+		S3C64XX_SPI_PND_TX_OVERRUN_CLR |
+		S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
+
+	writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
+
+	if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
+		dev_err(&spi->dev, "RX overrun\n");
+	if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
+		dev_err(&spi->dev, "RX underrun\n");
+	if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
+		dev_err(&spi->dev, "TX overrun\n");
+	if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
+		dev_err(&spi->dev, "TX underrun\n");
+
+	return IRQ_HANDLED;
+}
+
 static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int channel)
 {
 	struct s3c64xx_spi_info *sci = sdd->cntrlr_info;
@@ -970,7 +999,8 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 	struct s3c64xx_spi_driver_data *sdd;
 	struct s3c64xx_spi_info *sci;
 	struct spi_master *master;
-	int ret;
+	int ret, irq;
+	char clk_name[16];
 
 	if (pdev->id < 0) {
 		dev_err(&pdev->dev,
@@ -1010,6 +1040,12 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_warn(&pdev->dev, "Failed to get IRQ: %d\n", irq);
+		return irq;
+	}
+
 	master = spi_alloc_master(&pdev->dev,
 				sizeof(struct s3c64xx_spi_driver_data));
 	if (master == NULL) {
@@ -1104,10 +1140,21 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 	INIT_WORK(&sdd->work, s3c64xx_spi_work);
 	INIT_LIST_HEAD(&sdd->queue);
 
+	ret = request_irq(irq, s3c64xx_spi_irq, 0, "spi-s3c64xx", sdd);
+	if (ret != 0) {
+		dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n",
+			irq, ret);
+		goto err8;
+	}
+
+	writel(S3C64XX_SPI_INT_RX_OVERRUN_EN | S3C64XX_SPI_INT_RX_UNDERRUN_EN |
+	       S3C64XX_SPI_INT_TX_OVERRUN_EN | S3C64XX_SPI_INT_TX_UNDERRUN_EN,
+	       sdd->regs + S3C64XX_SPI_INT_EN);
+
 	if (spi_register_master(master)) {
 		dev_err(&pdev->dev, "cannot register SPI master\n");
 		ret = -EBUSY;
-		goto err8;
+		goto err9;
 	}
 
 	dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d "
@@ -1119,6 +1166,8 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 
 	return 0;
 
+err9:
+	free_irq(irq, sdd);
 err8:
 	destroy_workqueue(sdd->workqueue);
 err7:
@@ -1157,6 +1206,10 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 
 	spi_unregister_master(master);
 
+	writel(0, sdd->regs + S3C64XX_SPI_INT_EN);
+
+	free_irq(platform_get_irq(pdev, 0), sdd);
+
 	destroy_workqueue(sdd->workqueue);
 
 	clk_disable(sdd->src_clk);
-- 
1.7.7.3