s3c24xx-adc: resume convertions interrupted by going to suspend

Fix for a bug that shows when the s3c2410 TS driver requests a conversion from the s3c-adc driver and the machine goes into suspend. In this case the touchscreen stops working.

Does anyone on maillist interested in this bugfix? I got no response for 2 days...

?? ?????????????????? ???? 20 ?????????????? 2009 17:19:30 ?????????? Vasily Khoruzhick ??????????????:

Quoted 3 lines

Fix for a bug that shows when the s3c2410 TS driver requests a conversion from the s3c-adc driver and the machine goes into suspend. In this case the touchscreen stops working.

Does anyone on maillist interested in this bugfix? I got no response for 2 days...

Quoted 2 lines

Does anyone on maillist interested in this bugfix? I got no response for 2 days...

I've been in and out of bed feeling ill, I can't sit on this list 24x7 looking at bugfixes.

? ????????? ?? 20 ??????? 2009 17:19:30 ????? Vasily Khoruzhick ???????:

Quoted 3 lines

Fix for a bug that shows when the s3c2410 TS driver requests a conversion from the s3c-adc driver and the machine goes into suspend. In this case the touchscreen stops working.

Quoted 5 lines

(quoted diff) +0 -0

@@ -224,6 +225,7 @@ struct s3c_adc_client *s3c_adc_register(struct
 platform_device *pdev,
 
 	client->is_ts = is_ts;
 

Review

Quoted 5 lines

(quoted diff) +0 -0

 	client->select_cb = select;
 

	client->convert_cb = conv;

(quoted diff) +1 -0

+	client->selected = 0;

You don't need to clear client->selected. kzalloc takes care of this.

? ????????? ?? 24 ??????? 2009 09:59:25 ????? Nelson Castillo ???????:

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -0

@@ -224,6 +225,7 @@ struct s3c_adc_client *s3c_adc_register(struct
 platform_device *pdev,
 
 	client->is_ts = is_ts;
 
 	client->select_cb = select;
 

Quoted 5 lines

Review

Quoted 3 lines

	client->convert_cb = conv;

(quoted diff) +1 -0

+	client->selected = 0;

You don't need to clear client->selected. kzalloc takes care of this.

Ok, here's a fixed version.

Fix for a bug that shows when the s3c2410 TS driver requests a conversion from the s3c-adc driver and the machine goes into suspend. In this case the touchscreen stops working.

From ff091bbc781c7182611be3de3e9b3a078d770336 Mon Sep 17 00:00:00 2001
From: Vasily Khoruzhick <[email protected]>
Date: Tue, 20 Oct 2009 17:14:26 +0300
Subject: [PATCH] s3c24xx-adc: resume convertions interrupted by going to suspend

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 arch/arm/plat-s3c24xx/adc.c |   47 ++++++++++++++++++++++++++++++++++++------
 1 files changed, 40 insertions(+), 7 deletions(-)

arch/arm/plat-s3c24xx/adc.c +2 -0

index 4d36b78..45024e6 100644
@@ -46,6 +46,7 @@ struct s3c_adc_client {
 	int			 result;
 	unsigned char		 is_ts;
 	unsigned char		 channel;
+	unsigned int		 selected;
 
 	void	(*select_cb)(struct s3c_adc_client *c, unsigned selected);
 	void	(*convert_cb)(struct s3c_adc_client *c,

@@ -67,6 +68,7 @@ struct adc_device {
 };
 
 static struct adc_device *adc_dev;
+static struct work_struct resume_work;
 
 static LIST_HEAD(adc_pending);
 

arch/arm/plat-s3c24xx/adc.c +27 -6

 	con &= ~S3C2410_ADCCON_MUXMASK;
 	con &= ~S3C2410_ADCCON_STDBM;

@@ -109,13 +114,9 @@ static void s3c_adc_try(struct adc_device *adc)
 {
 	struct s3c_adc_client *next = adc->ts_pend;
 
-	if (!next && !list_empty(&adc_pending)) {
+	if (!next && !list_empty(&adc_pending))
 		next = list_first_entry(&adc_pending,
 					struct s3c_adc_client, pend);
-		list_del(&next->pend);
-	} else
-		adc->ts_pend = NULL;
-
 	if (next) {
 		adc_dbg(adc, "new client is %p\n", next);
 		adc->cur = next;

@@ -224,6 +225,7 @@ struct s3c_adc_client *s3c_adc_register(struct platform_device *pdev,
 	client->is_ts = is_ts;
 	client->select_cb = select;
 	client->convert_cb = conv;
+	client->selected = 0;
 
 	return client;
 }

@@ -278,13 +280,20 @@ static irqreturn_t s3c_adc_irq(int irq, void *pw)
 	if (client->nr_samples > 0) {
 		/* fire another conversion for this */
 
+		client->selected = 1;
 		client->select_cb(client, 1);
 		s3c_adc_convert(adc);
 	} else {
 		local_irq_save(flags);
-		(client->select_cb)(client, 0);
+		client->selected = 0;
+		if (!adc->cur->is_ts)
+			list_del(&adc->cur->pend);
+		else
+			adc->ts_pend = NULL;
 		adc->cur = NULL;
 
+		(client->select_cb)(client, 0);
+
 		s3c_adc_try(adc);
 		local_irq_restore(flags);
 	}

@@ -389,18 +398,42 @@ static int s3c_adc_suspend(struct platform_device *pdev, pm_message_t state)
 	writel(con, adc->regs + S3C2410_ADCCON);
 
 	clk_disable(adc->clk);
+	disable_irq(IRQ_ADC);
+	if (!list_empty(&adc_pending) || adc->ts_pend)
+		dev_info(&pdev->dev, "We still have adc clients pending\n");
 
 	return 0;
 }
 
+static void adc_resume_work(struct work_struct *work)
+{
+	if (!adc_dev) {
+		/* Have no ADC here */
+		return;
+	}
+
+	if (!list_empty(&adc_pending) || adc_dev->ts_pend)
+		/* We still have adc clients pending */
+		s3c_adc_try(adc_dev);
+}
+
+
 static int s3c_adc_resume(struct platform_device *pdev)
 {
 	struct adc_device *adc = platform_get_drvdata(pdev);
 
+	enable_irq(IRQ_ADC);
 	clk_enable(adc->clk);

Review

Quoted 5 lines

(quoted diff) +4 -1

-	client->select_cb(client, 1);
+	if (!client->selected) {
+		client->selected = 1;
+		client->select_cb(client, 1);
+	}

You seem to have added something other than just a bugfix to this patch, you've changed the behaviour of the select callback. Is this really necessary for the function of the fix or something that dropped in by accident?

Quoted 4 lines

(quoted diff) +2 -0

 	writel(adc->prescale | S3C2410_ADCCON_PRSCEN,
 	       adc->regs + S3C2410_ADCCON);
+	/* Schedule task if there are clients pending. */
+	if (!list_empty(&adc_pending) || adc_dev->ts_pend) {

Review

Quoted 5 lines

(quoted diff) +5 -0

+		INIT_WORK(&resume_work, adc_resume_work);
+		if (!schedule_work(&resume_work))
+			dev_err(&pdev->dev,
+				"Failed to schedule adc_resume work!\n");
+	}

Is the work being used here to avoid trying to do things that may sleep? IIRC, none of the calls should need to sleep.

I was just wondering whether it would be better to abort any calls in progress before sleep and get the clients themselves to restart once they themselves have been resumed. We may end up with the case where the callbacks are being triggered before the client has been resumed.