[PATCH 1/3] iio: stmpe-adc: Remove unnecessary assignment

[PATCH 1/3] iio: stmpe-adc: Remove unnecessary assignment

[Philippe Schenker]

From: Philippe Schenker <philippe.schenker@toradex.com>

Remove unnecessary assignment. This could potentially cause an issue, if
the wait function runs into a timeout. Furthermore is this assignment also
not there in stmpe_read_temp()

Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
---

 drivers/iio/adc/stmpe-adc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/iio/adc/stmpe-adc.c b/drivers/iio/adc/stmpe-adc.c
index 37f4b74a5d32..87141177fbda 100644
--- a/drivers/iio/adc/stmpe-adc.c
+++ b/drivers/iio/adc/stmpe-adc.c
@@ -78,8 +78,6 @@ static int stmpe_read_voltage(struct stmpe_adc *info,
 	stmpe_reg_write(info->stmpe, STMPE_REG_ADC_CAPT,
 			STMPE_ADC_CH(info->channel));
 
-	*val = info->value;
-
 	ret = wait_for_completion_interruptible_timeout
 		(&info->completion, STMPE_ADC_TIMEOUT);
 
-- 
2.21.0

[PATCH 2/3] iio: stmpe-adc: Make wait_completion non interruptible

[Philippe Schenker]

From: Philippe Schenker <philippe.schenker@toradex.com>

In some cases, the wait_completion got interrupted. This caused the
error-handling to mutex_unlock the function. The before turned on
interrupt then got called anyway. In the ISR then completion()
was called causing problems.

Making this wait_completion non interruptible solves the problem.

Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
---

 drivers/iio/adc/stmpe-adc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/stmpe-adc.c b/drivers/iio/adc/stmpe-adc.c
index 87141177fbda..baa41ffc0d76 100644
--- a/drivers/iio/adc/stmpe-adc.c
+++ b/drivers/iio/adc/stmpe-adc.c
@@ -78,8 +78,7 @@ static int stmpe_read_voltage(struct stmpe_adc *info,
 	stmpe_reg_write(info->stmpe, STMPE_REG_ADC_CAPT,
 			STMPE_ADC_CH(info->channel));
 
-	ret = wait_for_completion_interruptible_timeout
-		(&info->completion, STMPE_ADC_TIMEOUT);
+	ret = wait_for_completion_timeout(&info->completion, STMPE_ADC_TIMEOUT);
 
 	if (ret <= 0) {
 		mutex_unlock(&info->lock);
@@ -113,8 +112,7 @@ static int stmpe_read_temp(struct stmpe_adc *info,
 	stmpe_reg_write(info->stmpe, STMPE_REG_TEMP_CTRL,
 			STMPE_START_ONE_TEMP_CONV);
 
-	ret = wait_for_completion_interruptible_timeout
-		(&info->completion, STMPE_ADC_TIMEOUT);
+	ret = wait_for_completion_timeout(&info->completion, STMPE_ADC_TIMEOUT);
 
 	if (ret <= 0) {
 		mutex_unlock(&info->lock);
-- 
2.21.0

RE: [PATCH 2/3] iio: stmpe-adc: Make wait_completion non interruptible

[David Laight]

From: Philippe Schenker
> Sent: 03 May 2019 14:57
> In some cases, the wait_completion got interrupted. This caused the
> error-handling to mutex_unlock the function. The before turned on
> interrupt then got called anyway. In the ISR then completion()
> was called causing problems.
> 
> Making this wait_completion non interruptible solves the problem.

Won't the same thing happen if the interrupt occurs just after
the timeout?

	David
 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 2/3] iio: stmpe-adc: Make wait_completion non interruptible

[Philippe Schenker]

On Fri, 2019-05-03 at 14:39 +0000, David Laight wrote:
> From: Philippe Schenker
> > Sent: 03 May 2019 14:57
> > In some cases, the wait_completion got interrupted. This caused the
> > error-handling to mutex_unlock the function. The before turned on
> > interrupt then got called anyway. In the ISR then completion()
> > was called causing problems.
> > 
> > Making this wait_completion non interruptible solves the problem.
> 
> Won't the same thing happen if the interrupt occurs just after
> the timeout?
> 
> 	David
>  
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
> UK
> Registration No: 1397386 (Wales)
> 

You're of course right... Thanks for pointing this out. I will send a v2 with a
better solution then.

Philippe

Re: [PATCH 2/3] iio: stmpe-adc: Make wait_completion non interruptible

[Philippe Schenker]

On Tue, 2019-05-07 at 08:23 +0000, David Laight wrote:
> From: Jonathan Cameron
> > Sent: 05 May 2019 16:44
> > On Fri, 3 May 2019 15:58:38 +0000
> > Philippe Schenker <philippe.schenker@toradex.com> wrote:
> > 
> > > On Fri, 2019-05-03 at 14:39 +0000, David Laight wrote:
> > > > From: Philippe Schenker
> > > > > Sent: 03 May 2019 14:57
> > > > > In some cases, the wait_completion got interrupted. This caused the
> > > > > error-handling to mutex_unlock the function. The before turned on
> > > > > interrupt then got called anyway. In the ISR then completion()
> > > > > was called causing problems.
> > > > > 
> > > > > Making this wait_completion non interruptible solves the problem.
> > > > 
> > > > Won't the same thing happen if the interrupt occurs just after
> > > > the timeout?
> > > > 
> > > > 	David
> > > > 
> > > > 
> > > > -
> > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> > > > MK1 1PT,
> > > > UK
> > > > Registration No: 1397386 (Wales)
> > > > 
> > > 
> > > You're of course right... Thanks for pointing this out. I will send a v2
> > > with a
> > > better solution then.
> > > 
> > 
> > Isn't the timeout long enough that it should only happen if the hardware has
> > a fault? If that's the case, I wouldn't worry too much about possibility of
> > an interrupt causing confusion as long as it isn't catastrophic.
> 
> The 'confusion' is likely to be 'catastrophic' unless the code is written
> to handle it properly.
> 
> Cancelling callbacks is hard to get right and often not done properly.
> Timing out an interrupt is much the same problem.
> 
> 	David

I sorted it out now, there where also some more bugs I found and corrected.

@Jonathan: I will send a completely new series of patches that will include
patch 3/3 from this series but not the one you already applied. This due to
increased patch number and different order...
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
> UK
> Registration No: 1397386 (Wales)
> 

[PATCH 3/3] iio: stmpe-adc: Enable all stmpe-adc interrupts just once

[Philippe Schenker]

From: Philippe Schenker <philippe.schenker@toradex.com>

This commit will enable the interrupts of all channels handled by this
driver only once in the probe function.

This will improve performance because one byte less has to be written over
i2c on each read out of the adc. On the fastest ADC mode this will improve
read out speed by 15%.

Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

---

 drivers/iio/adc/stmpe-adc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/stmpe-adc.c b/drivers/iio/adc/stmpe-adc.c
index baa41ffc0d76..427c890c6e7d 100644
--- a/drivers/iio/adc/stmpe-adc.c
+++ b/drivers/iio/adc/stmpe-adc.c
@@ -72,9 +72,6 @@ static int stmpe_read_voltage(struct stmpe_adc *info,
 		return -EINVAL;
 	}
 
-	stmpe_reg_write(info->stmpe, STMPE_REG_ADC_INT_EN,
-			STMPE_ADC_CH(info->channel));
-
 	stmpe_reg_write(info->stmpe, STMPE_REG_ADC_CAPT,
 			STMPE_ADC_CH(info->channel));
 
@@ -328,6 +325,9 @@ static int stmpe_adc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	stmpe_reg_write(info->stmpe, STMPE_REG_ADC_INT_EN,
+			~(norequest_mask & 0xFF));
+
 	return devm_iio_device_register(&pdev->dev, indio_dev);
 }
 
-- 
2.21.0

Re: [PATCH 3/3] iio: stmpe-adc: Enable all stmpe-adc interrupts just once

[Jonathan Cameron]

On Fri,  3 May 2019 15:57:25 +0200
Philippe Schenker <dev@pschenker.ch> wrote:

> From: Philippe Schenker <philippe.schenker@toradex.com>
> 
> This commit will enable the interrupts of all channels handled by this
> driver only once in the probe function.
> 
> This will improve performance because one byte less has to be written over
> i2c on each read out of the adc. On the fastest ADC mode this will improve
> read out speed by 15%.
> 
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
Makes sense. I'll pick this up once patch 2 discussion is sorted.

Jonathan

> 
> ---
> 
>  drivers/iio/adc/stmpe-adc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/stmpe-adc.c b/drivers/iio/adc/stmpe-adc.c
> index baa41ffc0d76..427c890c6e7d 100644
> --- a/drivers/iio/adc/stmpe-adc.c
> +++ b/drivers/iio/adc/stmpe-adc.c
> @@ -72,9 +72,6 @@ static int stmpe_read_voltage(struct stmpe_adc *info,
>  		return -EINVAL;
>  	}
>  
> -	stmpe_reg_write(info->stmpe, STMPE_REG_ADC_INT_EN,
> -			STMPE_ADC_CH(info->channel));
> -
>  	stmpe_reg_write(info->stmpe, STMPE_REG_ADC_CAPT,
>  			STMPE_ADC_CH(info->channel));
>  
> @@ -328,6 +325,9 @@ static int stmpe_adc_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	stmpe_reg_write(info->stmpe, STMPE_REG_ADC_INT_EN,
> +			~(norequest_mask & 0xFF));
> +
>  	return devm_iio_device_register(&pdev->dev, indio_dev);
>  }
>  

Re: [PATCH 3/3] iio: stmpe-adc: Enable all stmpe-adc interrupts just once

[Philippe Schenker]

On Sun, 2019-05-05 at 16:45 +0100, Jonathan Cameron wrote:
> On Fri,  3 May 2019 15:57:25 +0200
> Philippe Schenker <dev@pschenker.ch> wrote:
> 
> > From: Philippe Schenker <philippe.schenker@toradex.com>
> > 
> > This commit will enable the interrupts of all channels handled by this
> > driver only once in the probe function.
> > 
> > This will improve performance because one byte less has to be written over
> > i2c on each read out of the adc. On the fastest ADC mode this will improve
> > read out speed by 15%.
> > 
> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> Makes sense. I'll pick this up once patch 2 discussion is sorted.
> 
> Jonathan

Please ignore this patch here as I send a completely new series that includes
this one.
> 
> > ---
> > 
> >  drivers/iio/adc/stmpe-adc.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/stmpe-adc.c b/drivers/iio/adc/stmpe-adc.c
> > index baa41ffc0d76..427c890c6e7d 100644
> > --- a/drivers/iio/adc/stmpe-adc.c
> > +++ b/drivers/iio/adc/stmpe-adc.c
> > @@ -72,9 +72,6 @@ static int stmpe_read_voltage(struct stmpe_adc *info,
> >  		return -EINVAL;
> >  	}
> >  
> > -	stmpe_reg_write(info->stmpe, STMPE_REG_ADC_INT_EN,
> > -			STMPE_ADC_CH(info->channel));
> > -
> >  	stmpe_reg_write(info->stmpe, STMPE_REG_ADC_CAPT,
> >  			STMPE_ADC_CH(info->channel));
> >  
> > @@ -328,6 +325,9 @@ static int stmpe_adc_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> >  
> > +	stmpe_reg_write(info->stmpe, STMPE_REG_ADC_INT_EN,
> > +			~(norequest_mask & 0xFF));
> > +
> >  	return devm_iio_device_register(&pdev->dev, indio_dev);
> >  }
> >  

Re: [PATCH 1/3] iio: stmpe-adc: Remove unnecessary assignment

[Jonathan Cameron]

On Fri,  3 May 2019 15:57:23 +0200
Philippe Schenker <dev@pschenker.ch> wrote:

> From: Philippe Schenker <philippe.schenker@toradex.com>
> 
> Remove unnecessary assignment. This could potentially cause an issue, if
> the wait function runs into a timeout. Furthermore is this assignment also
> not there in stmpe_read_temp()
> 
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
This would probably have benefited from a statement that *val is set
twice currently. Good find.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> ---
> 
>  drivers/iio/adc/stmpe-adc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/stmpe-adc.c b/drivers/iio/adc/stmpe-adc.c
> index 37f4b74a5d32..87141177fbda 100644
> --- a/drivers/iio/adc/stmpe-adc.c
> +++ b/drivers/iio/adc/stmpe-adc.c
> @@ -78,8 +78,6 @@ static int stmpe_read_voltage(struct stmpe_adc *info,
>  	stmpe_reg_write(info->stmpe, STMPE_REG_ADC_CAPT,
>  			STMPE_ADC_CH(info->channel));
>  
> -	*val = info->value;
> -
>  	ret = wait_for_completion_interruptible_timeout
>  		(&info->completion, STMPE_ADC_TIMEOUT);
>