linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] iio: stmpe-adc: Remove unnecessary assignment
@ 2019-05-03 13:57 Philippe Schenker
  2019-05-03 13:57 ` [PATCH 2/3] iio: stmpe-adc: Make wait_completion non interruptible Philippe Schenker
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Philippe Schenker @ 2019-05-03 13:57 UTC (permalink / raw)
  To: linux-iio, Jonathan Cameron, Stefan Agner, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler
  Cc: dev, Philippe Schenker, Max Krummenacher, Alexandre Torgue,
	linux-kernel, Lee Jones, Maxime Coquelin, linux-stm32,
	linux-arm-kernel

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] iio: stmpe-adc: Make wait_completion non interruptible
  2019-05-03 13:57 [PATCH 1/3] iio: stmpe-adc: Remove unnecessary assignment Philippe Schenker
@ 2019-05-03 13:57 ` Philippe Schenker
  2019-05-03 14:39   ` David Laight
  2019-05-03 13:57 ` [PATCH 3/3] iio: stmpe-adc: Enable all stmpe-adc interrupts just once Philippe Schenker
  2019-05-05 15:39 ` [PATCH 1/3] iio: stmpe-adc: Remove unnecessary assignment Jonathan Cameron
  2 siblings, 1 reply; 9+ messages in thread
From: Philippe Schenker @ 2019-05-03 13:57 UTC (permalink / raw)
  To: linux-iio, Jonathan Cameron, Stefan Agner, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler
  Cc: dev, Philippe Schenker, Max Krummenacher, Alexandre Torgue,
	linux-kernel, Lee Jones, Maxime Coquelin, linux-stm32,
	linux-arm-kernel

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] iio: stmpe-adc: Enable all stmpe-adc interrupts just once
  2019-05-03 13:57 [PATCH 1/3] iio: stmpe-adc: Remove unnecessary assignment Philippe Schenker
  2019-05-03 13:57 ` [PATCH 2/3] iio: stmpe-adc: Make wait_completion non interruptible Philippe Schenker
@ 2019-05-03 13:57 ` Philippe Schenker
  2019-05-05 15:45   ` Jonathan Cameron
  2019-05-05 15:39 ` [PATCH 1/3] iio: stmpe-adc: Remove unnecessary assignment Jonathan Cameron
  2 siblings, 1 reply; 9+ messages in thread
From: Philippe Schenker @ 2019-05-03 13:57 UTC (permalink / raw)
  To: linux-iio, Jonathan Cameron, Stefan Agner, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler
  Cc: dev, Philippe Schenker, Max Krummenacher, Alexandre Torgue,
	linux-kernel, Lee Jones, Maxime Coquelin, linux-stm32,
	linux-arm-kernel

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* RE: [PATCH 2/3] iio: stmpe-adc: Make wait_completion non interruptible
  2019-05-03 13:57 ` [PATCH 2/3] iio: stmpe-adc: Make wait_completion non interruptible Philippe Schenker
@ 2019-05-03 14:39   ` David Laight
  2019-05-03 15:58     ` Philippe Schenker
  0 siblings, 1 reply; 9+ messages in thread
From: David Laight @ 2019-05-03 14:39 UTC (permalink / raw)
  To: 'Philippe Schenker',
	linux-iio, Jonathan Cameron, Stefan Agner, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler
  Cc: Philippe Schenker, Max Krummenacher, Alexandre Torgue,
	linux-kernel, Lee Jones, Maxime Coquelin, linux-stm32,
	linux-arm-kernel

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)


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] iio: stmpe-adc: Make wait_completion non interruptible
  2019-05-03 14:39   ` David Laight
@ 2019-05-03 15:58     ` Philippe Schenker
       [not found]       ` <20190505164409.7976f43e@archlinux>
  0 siblings, 1 reply; 9+ messages in thread
From: Philippe Schenker @ 2019-05-03 15:58 UTC (permalink / raw)
  To: stefan, jic23, pmeerw, knaack.h, linux-iio, David.Laight, lars
  Cc: Max Krummenacher, Marcel Ziswiler, linux-stm32, lee.jones,
	linux-kernel, mcoquelin.stm32, linux-arm-kernel,
	alexandre.torgue

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] iio: stmpe-adc: Remove unnecessary assignment
  2019-05-03 13:57 [PATCH 1/3] iio: stmpe-adc: Remove unnecessary assignment Philippe Schenker
  2019-05-03 13:57 ` [PATCH 2/3] iio: stmpe-adc: Make wait_completion non interruptible Philippe Schenker
  2019-05-03 13:57 ` [PATCH 3/3] iio: stmpe-adc: Enable all stmpe-adc interrupts just once Philippe Schenker
@ 2019-05-05 15:39 ` Jonathan Cameron
  2 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2019-05-05 15:39 UTC (permalink / raw)
  To: Philippe Schenker
  Cc: linux-iio, Stefan Agner, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Philippe Schenker, Max Krummenacher,
	Alexandre Torgue, linux-kernel, Lee Jones, Maxime Coquelin,
	linux-stm32, linux-arm-kernel

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);
>  


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] iio: stmpe-adc: Enable all stmpe-adc interrupts just once
  2019-05-03 13:57 ` [PATCH 3/3] iio: stmpe-adc: Enable all stmpe-adc interrupts just once Philippe Schenker
@ 2019-05-05 15:45   ` Jonathan Cameron
  2019-05-07 14:32     ` Philippe Schenker
  0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2019-05-05 15:45 UTC (permalink / raw)
  To: Philippe Schenker
  Cc: linux-iio, Stefan Agner, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Philippe Schenker, Max Krummenacher,
	Alexandre Torgue, linux-kernel, Lee Jones, Maxime Coquelin,
	linux-stm32, linux-arm-kernel

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);
>  }
>  


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] iio: stmpe-adc: Make wait_completion non interruptible
       [not found]         ` <4df31129d19c4128a4bbc5e0575886af@AcuMS.aculab.com>
@ 2019-05-07 14:31           ` Philippe Schenker
  0 siblings, 0 replies; 9+ messages in thread
From: Philippe Schenker @ 2019-05-07 14:31 UTC (permalink / raw)
  To: jic23, David.Laight
  Cc: stefan, Marcel Ziswiler, alexandre.torgue, Max Krummenacher,
	linux-stm32, lee.jones, pmeerw, knaack.h, mcoquelin.stm32, lars,
	linux-arm-kernel, linux-kernel, linux-iio

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)
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] iio: stmpe-adc: Enable all stmpe-adc interrupts just once
  2019-05-05 15:45   ` Jonathan Cameron
@ 2019-05-07 14:32     ` Philippe Schenker
  0 siblings, 0 replies; 9+ messages in thread
From: Philippe Schenker @ 2019-05-07 14:32 UTC (permalink / raw)
  To: jic23
  Cc: stefan, Max Krummenacher, alexandre.torgue, linux-iio, lee.jones,
	linux-kernel, knaack.h, mcoquelin.stm32, lars, linux-arm-kernel,
	pmeerw, linux-stm32

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);
> >  }
> >  

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-05-07 14:32 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-03 13:57 [PATCH 1/3] iio: stmpe-adc: Remove unnecessary assignment Philippe Schenker
2019-05-03 13:57 ` [PATCH 2/3] iio: stmpe-adc: Make wait_completion non interruptible Philippe Schenker
2019-05-03 14:39   ` David Laight
2019-05-03 15:58     ` Philippe Schenker
     [not found]       ` <20190505164409.7976f43e@archlinux>
     [not found]         ` <4df31129d19c4128a4bbc5e0575886af@AcuMS.aculab.com>
2019-05-07 14:31           ` Philippe Schenker
2019-05-03 13:57 ` [PATCH 3/3] iio: stmpe-adc: Enable all stmpe-adc interrupts just once Philippe Schenker
2019-05-05 15:45   ` Jonathan Cameron
2019-05-07 14:32     ` Philippe Schenker
2019-05-05 15:39 ` [PATCH 1/3] iio: stmpe-adc: Remove unnecessary assignment Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).