2019-05-03 14:03:03

by Philippe Schenker

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

From: Philippe Schenker <[email protected]>

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 <[email protected]>
---

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


2019-05-03 14:30:25

by Philippe Schenker

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

From: Philippe Schenker <[email protected]>

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 <[email protected]>

---

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

2019-05-03 14:31:17

by Philippe Schenker

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

From: Philippe Schenker <[email protected]>

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 <[email protected]>
---

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

2019-05-03 14:42:06

by David Laight

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

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)

2019-05-03 16:09:24

by Philippe Schenker

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

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

2019-05-05 15:40:50

by Jonathan Cameron

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

On Fri, 3 May 2019 15:57:23 +0200
Philippe Schenker <[email protected]> wrote:

> From: Philippe Schenker <[email protected]>
>
> 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 <[email protected]>
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);
>

2019-05-05 15:46:16

by Jonathan Cameron

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

On Fri, 3 May 2019 15:57:25 +0200
Philippe Schenker <[email protected]> wrote:

> From: Philippe Schenker <[email protected]>
>
> 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 <[email protected]>
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);
> }
>

2019-05-07 14:33:08

by Philippe Schenker

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

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 <[email protected]> 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)
>

2019-05-07 14:33:51

by Philippe Schenker

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

On Sun, 2019-05-05 at 16:45 +0100, Jonathan Cameron wrote:
> On Fri, 3 May 2019 15:57:25 +0200
> Philippe Schenker <[email protected]> wrote:
>
> > From: Philippe Schenker <[email protected]>
> >
> > 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 <[email protected]>
> 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);
> > }
> >