[PATCH] staging: comedi: s626: use comedi_timeout() on remaining loops

[PATCH] staging: comedi: s626: use comedi_timeout() on remaining loops

[Chase Southwood]

There were just a handful of more while loops in this file that need timeouts,
and this patch takes care of them, using comedi_timeout().  A couple of new
callbacks are introduced, but there was one case where the appropriate callback
function was already defined; in this case I have just used that.

Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---
 drivers/staging/comedi/drivers/s626.c | 66 ++++++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index 95fadf3..4dc5659 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -295,10 +295,24 @@ static void s626_debi_replace(struct comedi_device *dev, unsigned int addr,
 
 /* **************  EEPROM ACCESS FUNCTIONS  ************** */
 
+static int s626_i2c_handshake_eoc(struct comedi_device *dev,
+				 struct comedi_subdevice *s,
+				 struct comedi_insn *insn,
+				 unsigned long context)
+{
+	unsigned int status;
+
+	status = s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
+	if (status)
+		return 0;
+	return -EBUSY;
+}
+
 static uint32_t s626_i2c_handshake(struct comedi_device *dev, uint32_t val)
 {
 	struct s626_private *devpriv = dev->private;
 	unsigned int ctrl;
+	uint32_t ret;
 
 	/* Write I2C command to I2C Transfer Control shadow register */
 	writel(val, devpriv->mmio + S626_P_I2CCTRL);
@@ -308,8 +322,9 @@ static uint32_t s626_i2c_handshake(struct comedi_device *dev, uint32_t val)
 	 * wait for upload confirmation.
 	 */
 	s626_mc_enable(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
-	while (!s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2))
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_i2c_handshake_eoc, 0);
+	if (ret)
+		return ret;
 
 	/* Wait until I2C bus transfer is finished or an error occurs */
 	do {
@@ -2029,8 +2044,9 @@ static int s626_ai_insn_read(struct comedi_device *dev,
 	/* Wait for the data to arrive in FB BUFFER 1 register. */
 
 	/* Wait for ADC done */
-	while (!(readl(devpriv->mmio + S626_P_PSR) & S626_PSR_GPIO2))
-		;
+	ret = comedi_timeout(dev, s, insn, s626_ai_eoc, 0);
+	if (ret)
+		return ret;
 
 	/* Fetch ADC data from audio interface's input shift register. */
 
@@ -2641,6 +2657,36 @@ static int s626_allocate_dma_buffers(struct comedi_device *dev)
 	return 0;
 }
 
+enum {
+	s626_initialize_wait_i2c_abort,
+	s626_initialize_wait_i2c_err_reset
+};
+
+static int s626_initialize_eoc(struct comedi_device *dev,
+			      struct comedi_subdevice *s,
+			      struct comedi_insn *insn,
+			      unsigned long context)
+{
+	struct s626_private *devpriv = dev->private;
+	unsigned int status;
+
+	switch (context) {
+	case s626_initialize_wait_i2c_abort:
+		status = readl(devpriv->mmio + S626_P_MC2);
+		if (status & S626_MC2_UPLD_IIC)
+			return 0;
+		break;
+	case s626_initialize_wait_i2c_err_reset:
+		status = s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
+		if (status)
+			return 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return -EBUSY;
+}
+
 static int s626_initialize(struct comedi_device *dev)
 {
 	struct s626_private *devpriv = dev->private;
@@ -2681,8 +2727,10 @@ static int s626_initialize(struct comedi_device *dev)
 	writel(S626_I2C_CLKSEL | S626_I2C_ABORT,
 	       devpriv->mmio + S626_P_I2CSTAT);
 	s626_mc_enable(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
-	while (!(readl(devpriv->mmio + S626_P_MC2) & S626_MC2_UPLD_IIC))
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_initialize_eoc,
+			     s626_initialize_wait_i2c_abort);
+	if (ret)
+		return ret;
 
 	/*
 	 * Per SAA7146 data sheet, write to STATUS
@@ -2691,8 +2739,10 @@ static int s626_initialize(struct comedi_device *dev)
 	for (i = 0; i < 2; i++) {
 		writel(S626_I2C_CLKSEL, devpriv->mmio + S626_P_I2CSTAT);
 		s626_mc_enable(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
-		while (!s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2))
-			;
+		ret = comedi_timeout(dev, NULL, NULL, s626_initialize_eoc,
+				     s626_initialize_wait_i2c_err_reset);
+		if (ret)
+			return ret;
 	}
 
 	/*
-- 
1.8.5.3

Re: [PATCH] staging: comedi: s626: use comedi_timeout() on remaining loops

[Ian Abbott]

On 2014-03-25 05:09, Chase Southwood wrote:
> There were just a handful of more while loops in this file that need timeouts,
> and this patch takes care of them, using comedi_timeout().  A couple of new
> callbacks are introduced, but there was one case where the appropriate callback
> function was already defined; in this case I have just used that.
>
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>

I think it can be simplified a bit as described below.

> ---
>   drivers/staging/comedi/drivers/s626.c | 66 ++++++++++++++++++++++++++++++-----
>   1 file changed, 58 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
> index 95fadf3..4dc5659 100644
> --- a/drivers/staging/comedi/drivers/s626.c
> +++ b/drivers/staging/comedi/drivers/s626.c
> @@ -295,10 +295,24 @@ static void s626_debi_replace(struct comedi_device *dev, unsigned int addr,
>
>   /* **************  EEPROM ACCESS FUNCTIONS  ************** */
>
> +static int s626_i2c_handshake_eoc(struct comedi_device *dev,
> +				 struct comedi_subdevice *s,
> +				 struct comedi_insn *insn,
> +				 unsigned long context)
> +{
> +	unsigned int status;
> +
> +	status = s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
> +	if (status)
> +		return 0;
> +	return -EBUSY;
> +}
> +

So s626_i2c_handshake_eoc() tests the S626_MC2_UPLD_IIC bit of the 
S626_P_MC2 register and returns 0 if the bit is high or -EBUSY if it is low.

>   static uint32_t s626_i2c_handshake(struct comedi_device *dev, uint32_t val)
>   {
>   	struct s626_private *devpriv = dev->private;
>   	unsigned int ctrl;
> +	uint32_t ret;
>
>   	/* Write I2C command to I2C Transfer Control shadow register */
>   	writel(val, devpriv->mmio + S626_P_I2CCTRL);
> @@ -308,8 +322,9 @@ static uint32_t s626_i2c_handshake(struct comedi_device *dev, uint32_t val)
>   	 * wait for upload confirmation.
>   	 */
>   	s626_mc_enable(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
> -	while (!s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2))
> -		;
> +	ret = comedi_timeout(dev, NULL, NULL, s626_i2c_handshake_eoc, 0);
> +	if (ret)
> +		return ret;

That's fine.  Both the original while loop and the comedi_timeout() call 
are waiting for the S626_MC_UPLD_IIC bit to go high.

>
>   	/* Wait until I2C bus transfer is finished or an error occurs */
>   	do {
> @@ -2029,8 +2044,9 @@ static int s626_ai_insn_read(struct comedi_device *dev,
>   	/* Wait for the data to arrive in FB BUFFER 1 register. */
>
>   	/* Wait for ADC done */
> -	while (!(readl(devpriv->mmio + S626_P_PSR) & S626_PSR_GPIO2))
> -		;
> +	ret = comedi_timeout(dev, s, insn, s626_ai_eoc, 0);
> +	if (ret)
> +		return ret;
>
>   	/* Fetch ADC data from audio interface's input shift register. */
>
> @@ -2641,6 +2657,36 @@ static int s626_allocate_dma_buffers(struct comedi_device *dev)
>   	return 0;
>   }
>
> +enum {
> +	s626_initialize_wait_i2c_abort,
> +	s626_initialize_wait_i2c_err_reset
> +};
> +
> +static int s626_initialize_eoc(struct comedi_device *dev,
> +			      struct comedi_subdevice *s,
> +			      struct comedi_insn *insn,
> +			      unsigned long context)
> +{
> +	struct s626_private *devpriv = dev->private;
> +	unsigned int status;
> +
> +	switch (context) {
> +	case s626_initialize_wait_i2c_abort:
> +		status = readl(devpriv->mmio + S626_P_MC2);
> +		if (status & S626_MC2_UPLD_IIC)
> +			return 0;
> +		break;

The 's626_initialize_wait_i2c_abort' case returns 0 if the 
S626_MC2_UPLD_IIC bit is high and the function around it will return 
-EBUSY otherwise.  That's basically the same as the 
s626_i2c_handshake_eoc() callback except it doesn't call s626_mc_test() 
to read the register and check the bit, it does it itself.

> +	case s626_initialize_wait_i2c_err_reset:
> +		status = s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
> +		if (status)
> +			return 0;
> +		break;

The 's626_initialize_wait_i2c_err_reset' case is also basically the same 
as the s626_i2c_handshake_eoc() callback.

> +	default:
> +		return -EINVAL;
> +	}
> +	return -EBUSY;
> +}
> +

It seems both supported context case values of the s626_initialize_eoc() 
callback basically do the same thing as the s626_i2c_handshake_eoc() 
callback, so you could use that callback instead of this one.

>   static int s626_initialize(struct comedi_device *dev)
>   {
>   	struct s626_private *devpriv = dev->private;
> @@ -2681,8 +2727,10 @@ static int s626_initialize(struct comedi_device *dev)
>   	writel(S626_I2C_CLKSEL | S626_I2C_ABORT,
>   	       devpriv->mmio + S626_P_I2CSTAT);
>   	s626_mc_enable(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
> -	while (!(readl(devpriv->mmio + S626_P_MC2) & S626_MC2_UPLD_IIC))
> -		;
> +	ret = comedi_timeout(dev, NULL, NULL, s626_initialize_eoc,
> +			     s626_initialize_wait_i2c_abort);
> +	if (ret)
> +		return ret;

That's fine.  The original while loop is waiting for the 
S626_MC2_UPLD_IIC bit to go high and so is the comedi_timeout() call. 
But you could re-use the s626_i2c_handshake_eoc() callback instead of 
the s626_initialize_wait_i2c_abort() as described above.

>
>   	/*
>   	 * Per SAA7146 data sheet, write to STATUS
> @@ -2691,8 +2739,10 @@ static int s626_initialize(struct comedi_device *dev)
>   	for (i = 0; i < 2; i++) {
>   		writel(S626_I2C_CLKSEL, devpriv->mmio + S626_P_I2CSTAT);
>   		s626_mc_enable(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
> -		while (!s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2))
> -			;
> +		ret = comedi_timeout(dev, NULL, NULL, s626_initialize_eoc,
> +				     s626_initialize_wait_i2c_err_reset);
> +		if (ret)
> +			return ret;

As above, the original while loop is waiting for the S626_MC2_UPLD_IIC 
bit to go high, so you could re-use the s626_i2c_handshake_eoc() callback.

>   	}
>
>   	/*
>

Best regards,
Ian.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH] staging: comedi: s626: use comedi_timeout() on remaining loops

[Chase Southwood]

Ian,

>On Tuesday, March 25, 2014 7:13 AM, Ian Abbott <abbotti@mev.co.uk> wrote:
>>On 2014-03-25 05:09, Chase Southwood wrote:
>>There were just a handful of more while loops in this file that need timeouts,
>>and this patch takes care of them, using comedi_timeout().  A couple of new
>>callbacks are introduced, but there was one case where the appropriate callback
>>function was already defined; in this case I have just used that.
>>
>>Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
>
>I think it can be simplified a bit as described below.


[snip]

Thank you for all of this great feedback, I'll get this fixed up as quick as I can.

Thanks,
Chase

[PATCH v2] staging: comedi: s626: use comedi_timeout() on remaining loops

[Chase Southwood]

There were just a handful of more while loops in this file that needed
timeouts, and this patch takes care of them.  One new callback is
introduced, and all of the proper comedi_timeout() calls are then used.

Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---
2: s626_i2c_handshake_eoc() can be used in s626_initialize() as noted by
Ian.  So, s626_initialize_eoc() has been removed, and its uses swapped
for s626_i2c_handshake_eoc().

 drivers/staging/comedi/drivers/s626.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index 95fadf3..865cf93 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -295,10 +295,24 @@ static void s626_debi_replace(struct comedi_device *dev, unsigned int addr,
 
 /* **************  EEPROM ACCESS FUNCTIONS  ************** */
 
+static int s626_i2c_handshake_eoc(struct comedi_device *dev,
+				 struct comedi_subdevice *s,
+				 struct comedi_insn *insn,
+				 unsigned long context)
+{
+	unsigned int status;
+
+	status = s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
+	if (status)
+		return 0;
+	return -EBUSY;
+}
+
 static uint32_t s626_i2c_handshake(struct comedi_device *dev, uint32_t val)
 {
 	struct s626_private *devpriv = dev->private;
 	unsigned int ctrl;
+	uint32_t ret;
 
 	/* Write I2C command to I2C Transfer Control shadow register */
 	writel(val, devpriv->mmio + S626_P_I2CCTRL);
@@ -308,8 +322,9 @@ static uint32_t s626_i2c_handshake(struct comedi_device *dev, uint32_t val)
 	 * wait for upload confirmation.
 	 */
 	s626_mc_enable(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
-	while (!s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2))
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_i2c_handshake_eoc, 0);
+	if (ret)
+		return ret;
 
 	/* Wait until I2C bus transfer is finished or an error occurs */
 	do {
@@ -2029,8 +2044,9 @@ static int s626_ai_insn_read(struct comedi_device *dev,
 	/* Wait for the data to arrive in FB BUFFER 1 register. */
 
 	/* Wait for ADC done */
-	while (!(readl(devpriv->mmio + S626_P_PSR) & S626_PSR_GPIO2))
-		;
+	ret = comedi_timeout(dev, s, insn, s626_ai_eoc, 0);
+	if (ret)
+		return ret;
 
 	/* Fetch ADC data from audio interface's input shift register. */
 
@@ -2681,8 +2697,9 @@ static int s626_initialize(struct comedi_device *dev)
 	writel(S626_I2C_CLKSEL | S626_I2C_ABORT,
 	       devpriv->mmio + S626_P_I2CSTAT);
 	s626_mc_enable(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
-	while (!(readl(devpriv->mmio + S626_P_MC2) & S626_MC2_UPLD_IIC))
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_i2c_handshake_eoc, 0);
+	if (ret)
+		return ret;
 
 	/*
 	 * Per SAA7146 data sheet, write to STATUS
@@ -2691,8 +2708,9 @@ static int s626_initialize(struct comedi_device *dev)
 	for (i = 0; i < 2; i++) {
 		writel(S626_I2C_CLKSEL, devpriv->mmio + S626_P_I2CSTAT);
 		s626_mc_enable(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
-		while (!s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2))
-			;
+		ret = comedi_timeout(dev, NULL, NULL, s626_i2c_handshake_eoc, 0);
+		if (ret)
+			return ret;
 	}
 
 	/*
-- 
1.8.5.3

Re: [PATCH v2] staging: comedi: s626: use comedi_timeout() on remaining loops

[Ian Abbott]

On 2014/03/26 03:43 AM, Chase Southwood wrote:
> There were just a handful of more while loops in this file that needed
> timeouts, and this patch takes care of them.  One new callback is
> introduced, and all of the proper comedi_timeout() calls are then used.
> 
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> ---
> 2: s626_i2c_handshake_eoc() can be used in s626_initialize() as noted by
> Ian.  So, s626_initialize_eoc() has been removed, and its uses swapped
> for s626_i2c_handshake_eoc().

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH v2] staging: comedi: s626: use comedi_timeout() on remaining loops

[Dan Carpenter]

On Tue, Mar 25, 2014 at 10:43:58PM -0500, Chase Southwood wrote:
> There were just a handful of more while loops in this file that needed
> timeouts, and this patch takes care of them.  One new callback is
> introduced, and all of the proper comedi_timeout() calls are then used.
> 
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> ---
> 2: s626_i2c_handshake_eoc() can be used in s626_initialize() as noted by
> Ian.  So, s626_initialize_eoc() has been removed, and its uses swapped
> for s626_i2c_handshake_eoc().
> 
>  drivers/staging/comedi/drivers/s626.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
> index 95fadf3..865cf93 100644
> --- a/drivers/staging/comedi/drivers/s626.c
> +++ b/drivers/staging/comedi/drivers/s626.c
> @@ -295,10 +295,24 @@ static void s626_debi_replace(struct comedi_device *dev, unsigned int addr,
>  
>  /* **************  EEPROM ACCESS FUNCTIONS  ************** */
>  
> +static int s626_i2c_handshake_eoc(struct comedi_device *dev,
> +				 struct comedi_subdevice *s,
> +				 struct comedi_insn *insn,
> +				 unsigned long context)
> +{
> +	unsigned int status;

This should probably be bool.

> +
> +	status = s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
> +	if (status)
> +		return 0;
> +	return -EBUSY;
> +}
> +
>  static uint32_t s626_i2c_handshake(struct comedi_device *dev, uint32_t val)
>  {
>  	struct s626_private *devpriv = dev->private;
>  	unsigned int ctrl;
> +	uint32_t ret;

This should be int.  I get really suspicious when people start using
uint32_t types.  Why does it have to be 32 bits?  Unsigned is wrong as
well.

regards,
dan carpenter

Re: [PATCH v2] staging: comedi: s626: use comedi_timeout() on remaining loops

[Chase Southwood]

>On Thursday, April 3, 2014 3:38 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:

>>On Tue, Mar 25, 2014 at 10:43:58PM -0500, Chase Southwood wrote:
>>There were just a handful of more while loops in this file that needed
>>timeouts, and this patch takes care of them.  One new callback is
>>introduced, and all of the proper comedi_timeout() calls are then used.
>>
>>Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
>>---
>>2: s626_i2c_handshake_eoc() can be used in s626_initialize() as noted by
>>Ian.  So, s626_initialize_eoc() has been removed, and its uses swapped
>>for s626_i2c_handshake_eoc().
>>
>> drivers/staging/comedi/drivers/s626.c | 34 ++++++++++++++++++++++++++--------
>> 1 file changed, 26 insertions(+), 8 deletions(-)
>>
>>diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
>>index 95fadf3..865cf93 100644
>>--- a/drivers/staging/comedi/drivers/s626.c
>>+++ b/drivers/staging/comedi/drivers/s626.c
>>@@ -295,10 +295,24 @@ static void s626_debi_replace(struct comedi_device *dev, unsigned int addr,
>> 
>> /* **************  EEPROM ACCESS FUNCTIONS  ************** */
>> 
>>+static int s626_i2c_handshake_eoc(struct comedi_device *dev,
>>+                 struct comedi_subdevice *s,
>>+                 struct comedi_insn *insn,
>>+                 unsigned long context)
>>+{
>>+    unsigned int status;
>
>This should probably be bool.
>

Oh, whoops...yeah s626_mc_test() definitely returns bool...I'll fix this up.

>>+
>>+    status = s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
>>+    if (status)
>>+        return 0;
>>+    return -EBUSY;
>>+}
>>+
>> static uint32_t s626_i2c_handshake(struct comedi_device *dev, uint32_t val)
>> {
>>     struct s626_private *devpriv = dev->private;
>>     unsigned int ctrl;
>>+    uint32_t ret;
>
>This should be int.  I get really suspicious when people start using
>uint32_t types.  Why does it have to be 32 bits?  Unsigned is wrong as
>well.

Yeah...I originally did that to conform to the current return type of the function,
not sure how I didn't manage to see that trying to return a negative error code as
an unsigned int is clearly a bug.  Sorry, I'll fix this up as well.

Thanks for the review, Dan.

Chase

[PATCH v3] staging: comedi: s626: use comedi_timeout() on remaining loops

[Chase Southwood]

There were just a handful of more while loops in this file that needed
timeouts, and this patch takes care of them.  One new callback is
introduced, and all of the proper comedi_timeout() calls are then used.
The return type of s626_i2c_handshake() has been changed from uint32_t to
int so that a negative error code from comedi_timeout() can be propagated
if necessary.

Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---
2: s626_i2c_handshake_eoc() can be used in s626_initialize() as noted by
Ian. So, s626_initialize_eoc() has been removed, and its uses swapped
for s626_i2c_handshake_eoc().

3: *Type of 'status' variable in s626_i2c_handshake_eoc() has been
corrected to bool (the return type of s626_mc_test()).
*Return type of s626_i2c_handshake() has been changed to int to allow
returning negative error codes. 
*Type of 'ret' variable in the same function has been changed to int for
the same reason.
 drivers/staging/comedi/drivers/s626.c | 36 ++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index 95fadf3..6da43de 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -295,10 +295,24 @@ static void s626_debi_replace(struct comedi_device *dev, unsigned int addr,
 
 /* **************  EEPROM ACCESS FUNCTIONS  ************** */
 
-static uint32_t s626_i2c_handshake(struct comedi_device *dev, uint32_t val)
+static int s626_i2c_handshake_eoc(struct comedi_device *dev,
+				 struct comedi_subdevice *s,
+				 struct comedi_insn *insn,
+				 unsigned long context)
+{
+	bool status;
+
+	status = s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
+	if (status)
+		return 0;
+	return -EBUSY;
+}
+
+static int s626_i2c_handshake(struct comedi_device *dev, uint32_t val)
 {
 	struct s626_private *devpriv = dev->private;
 	unsigned int ctrl;
+	int ret;
 
 	/* Write I2C command to I2C Transfer Control shadow register */
 	writel(val, devpriv->mmio + S626_P_I2CCTRL);
@@ -308,8 +322,9 @@ static uint32_t s626_i2c_handshake(struct comedi_device *dev, uint32_t val)
 	 * wait for upload confirmation.
 	 */
 	s626_mc_enable(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
-	while (!s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2))
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_i2c_handshake_eoc, 0);
+	if (ret)
+		return ret;
 
 	/* Wait until I2C bus transfer is finished or an error occurs */
 	do {
@@ -2029,8 +2044,9 @@ static int s626_ai_insn_read(struct comedi_device *dev,
 	/* Wait for the data to arrive in FB BUFFER 1 register. */
 
 	/* Wait for ADC done */
-	while (!(readl(devpriv->mmio + S626_P_PSR) & S626_PSR_GPIO2))
-		;
+	ret = comedi_timeout(dev, s, insn, s626_ai_eoc, 0);
+	if (ret)
+		return ret;
 
 	/* Fetch ADC data from audio interface's input shift register. */
 
@@ -2681,8 +2697,9 @@ static int s626_initialize(struct comedi_device *dev)
 	writel(S626_I2C_CLKSEL | S626_I2C_ABORT,
 	       devpriv->mmio + S626_P_I2CSTAT);
 	s626_mc_enable(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
-	while (!(readl(devpriv->mmio + S626_P_MC2) & S626_MC2_UPLD_IIC))
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_i2c_handshake_eoc, 0);
+	if (ret)
+		return ret;
 
 	/*
 	 * Per SAA7146 data sheet, write to STATUS
@@ -2691,8 +2708,9 @@ static int s626_initialize(struct comedi_device *dev)
 	for (i = 0; i < 2; i++) {
 		writel(S626_I2C_CLKSEL, devpriv->mmio + S626_P_I2CSTAT);
 		s626_mc_enable(dev, S626_MC2_UPLD_IIC, S626_P_MC2);
-		while (!s626_mc_test(dev, S626_MC2_UPLD_IIC, S626_P_MC2))
-			;
+		ret = comedi_timeout(dev, NULL, NULL, s626_i2c_handshake_eoc, 0);
+		if (ret)
+			return ret;
 	}
 
 	/*
-- 
1.9.0

Re: [PATCH v3] staging: comedi: s626: use comedi_timeout() on remaining loops

[Ian Abbott]

On 2014-04-04 00:43, Chase Southwood wrote:
> There were just a handful of more while loops in this file that needed
> timeouts, and this patch takes care of them.  One new callback is
> introduced, and all of the proper comedi_timeout() calls are then used.
> The return type of s626_i2c_handshake() has been changed from uint32_t to
> int so that a negative error code from comedi_timeout() can be propagated
> if necessary.
>
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> ---
> 2: s626_i2c_handshake_eoc() can be used in s626_initialize() as noted by
> Ian. So, s626_initialize_eoc() has been removed, and its uses swapped
> for s626_i2c_handshake_eoc().
>
> 3: *Type of 'status' variable in s626_i2c_handshake_eoc() has been
> corrected to bool (the return type of s626_mc_test()).
> *Return type of s626_i2c_handshake() has been changed to int to allow
> returning negative error codes.
> *Type of 'ret' variable in the same function has been changed to int for
> the same reason.
>   drivers/staging/comedi/drivers/s626.c | 36 ++++++++++++++++++++++++++---------
>   1 file changed, 27 insertions(+), 9 deletions(-)

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-