iio: adc: sc27xx: Change to polling mode to read data

Commit Message

(Exiting) Baolin Wang July 25, 2019, 6:33 a.m. UTC

From: Freeman Liu <[e-mail]>

On Spreadtrum platform, the headphone will read one ADC channel multiple
times to identify the headphone type, and the headphone identification is
sensitive of the ADC reading time. And we found it will take longer time
to reading ADC data by using interrupt mode comparing with the polling
mode, thus we should change to polling mode to improve the efficiency
of reading data, which can identify the headphone type successfully.

Signed-off-by: Freeman Liu <[e-mail]>
Signed-off-by: Baolin Wang <[e-mail]>
---
 drivers/iio/adc/sc27xx_adc.c |   81 ++++++++++++++----------------------------
 1 file changed, 27 insertions(+), 54 deletions(-)

Comments

Jonathan Cameron July 27, 2019, 5:27 p.m. UTC | #1

On Thu, 25 Jul 2019 14:33:50 +0800
Baolin Wang <[e-mail]> wrote:

> From: Freeman Liu <[e-mail]>
> 
> On Spreadtrum platform, the headphone will read one ADC channel multiple
> times to identify the headphone type, and the headphone identification is
> sensitive of the ADC reading time. And we found it will take longer time
> to reading ADC data by using interrupt mode comparing with the polling
> mode, thus we should change to polling mode to improve the efficiency
> of reading data, which can identify the headphone type successfully.
> 
> Signed-off-by: Freeman Liu <[e-mail]>
> Signed-off-by: Baolin Wang <[e-mail]>

Hi,

My concerns with this sort of approach is that we may be sacrificing power
efficiency for some usecases to support one demanding one.

The maximum sleep time is 1 second (I think) which is probably too long
to poll a register for in general.

Is there some way we can bound that time and perhaps switch between
interrupt and polling modes depending on how long we expect to wait?

Thanks,

Jonathan

> ---
>  drivers/iio/adc/sc27xx_adc.c |   81 ++++++++++++++----------------------------
>  1 file changed, 27 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> index f7f7a189..ea864290 100644
> --- a/drivers/iio/adc/sc27xx_adc.c
> +++ b/drivers/iio/adc/sc27xx_adc.c
> @@ -3,7 +3,6 @@
>  
>  #include <linux/hwspinlock.h>
>  #include <linux/iio/iio.h>
> -#include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/nvmem-consumer.h>
>  #include <linux/of.h>
> @@ -46,14 +45,18 @@
>  /* Bits definitions for SC27XX_ADC_INT_CLR registers */
>  #define SC27XX_ADC_IRQ_CLR		BIT(0)
>  
> +/* Bits definitions for SC27XX_ADC_INT_RAW registers */
> +#define SC27XX_ADC_IRQ_RAW		BIT(0)
> +
>  /* Mask definition for SC27XX_ADC_DATA register */
>  #define SC27XX_ADC_DATA_MASK		GENMASK(11, 0)
>  
>  /* Timeout (ms) for the trylock of hardware spinlocks */
>  #define SC27XX_ADC_HWLOCK_TIMEOUT	5000
>  
> -/* Timeout (ms) for ADC data conversion according to ADC datasheet */
> -#define SC27XX_ADC_RDY_TIMEOUT		100
> +/* Timeout (us) for ADC data conversion according to ADC datasheet */
> +#define SC27XX_ADC_RDY_TIMEOUT		1000000

This is 10 x the value I think...

> +#define SC27XX_ADC_POLL_RAW_STATUS	500
>  
>  /* Maximum ADC channel number */
>  #define SC27XX_ADC_CHANNEL_MAX		32
> @@ -72,10 +75,8 @@ struct sc27xx_adc_data {
>  	 * subsystems which will access the unique ADC controller.
>  	 */
>  	struct hwspinlock *hwlock;
> -	struct completion completion;
>  	int channel_scale[SC27XX_ADC_CHANNEL_MAX];
>  	u32 base;
> -	int value;
>  	int irq;
>  };
>  
> @@ -188,9 +189,7 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
>  			   int scale, int *val)
>  {
>  	int ret;
> -	u32 tmp;
> -
> -	reinit_completion(&data->completion);
> +	u32 tmp, value, status;
>  
>  	ret = hwspin_lock_timeout_raw(data->hwlock, SC27XX_ADC_HWLOCK_TIMEOUT);
>  	if (ret) {
> @@ -203,6 +202,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
>  	if (ret)
>  		goto unlock_adc;
>  
> +	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_CLR,
> +				 SC27XX_ADC_IRQ_CLR, SC27XX_ADC_IRQ_CLR);
> +	if (ret)
> +		goto disable_adc;
> +
>  	/* Configure the channel id and scale */
>  	tmp = (scale << SC27XX_ADC_SCALE_SHIFT) & SC27XX_ADC_SCALE_MASK;
>  	tmp |= channel & SC27XX_ADC_CHN_ID_MASK;
> @@ -226,15 +230,22 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
>  	if (ret)
>  		goto disable_adc;
>  
> -	ret = wait_for_completion_timeout(&data->completion,
> -				msecs_to_jiffies(SC27XX_ADC_RDY_TIMEOUT));
> -	if (!ret) {
> -		dev_err(data->dev, "read ADC data timeout\n");
> -		ret = -ETIMEDOUT;
> -	} else {
> -		ret = 0;
> +	ret = regmap_read_poll_timeout(data->regmap,
> +				       data->base + SC27XX_ADC_INT_RAW,
> +				       status, (status & SC27XX_ADC_IRQ_RAW),
> +				       SC27XX_ADC_POLL_RAW_STATUS,
> +				       SC27XX_ADC_RDY_TIMEOUT);
> +	if (ret) {
> +		dev_err(data->dev, "read adc timeout, status = 0x%x\n", status);
> +		goto disable_adc;
>  	}
>  
> +	ret = regmap_read(data->regmap, data->base + SC27XX_ADC_DATA, &value);
> +	if (ret)
> +		goto disable_adc;
> +
> +	value &= SC27XX_ADC_DATA_MASK;
> +
>  disable_adc:
>  	regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
>  			   SC27XX_ADC_EN, 0);
> @@ -242,32 +253,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
>  	hwspin_unlock_raw(data->hwlock);
>  
>  	if (!ret)
> -		*val = data->value;
> +		*val = value;
>  
>  	return ret;
>  }
>  
> -static irqreturn_t sc27xx_adc_isr(int irq, void *dev_id)
> -{
> -	struct sc27xx_adc_data *data = dev_id;
> -	int ret;
> -
> -	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_CLR,
> -				 SC27XX_ADC_IRQ_CLR, SC27XX_ADC_IRQ_CLR);
> -	if (ret)
> -		return IRQ_RETVAL(ret);
> -
> -	ret = regmap_read(data->regmap, data->base + SC27XX_ADC_DATA,
> -			  &data->value);
> -	if (ret)
> -		return IRQ_RETVAL(ret);
> -
> -	data->value &= SC27XX_ADC_DATA_MASK;
> -	complete(&data->completion);
> -
> -	return IRQ_HANDLED;
> -}
> -
>  static void sc27xx_adc_volt_ratio(struct sc27xx_adc_data *data,
>  				  int channel, int scale,
>  				  u32 *div_numerator, u32 *div_denominator)
> @@ -454,11 +444,6 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
>  	if (ret)
>  		goto disable_adc;
>  
> -	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_EN,
> -				 SC27XX_ADC_IRQ_EN, SC27XX_ADC_IRQ_EN);
> -	if (ret)
> -		goto disable_clk;
> -
>  	/* ADC channel scales' calibration from nvmem device */
>  	ret = sc27xx_adc_scale_calibration(data, true);
>  	if (ret)
> @@ -484,9 +469,6 @@ static void sc27xx_adc_disable(void *_data)
>  {
>  	struct sc27xx_adc_data *data = _data;
>  
> -	regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_EN,
> -			   SC27XX_ADC_IRQ_EN, 0);
> -
>  	/* Disable ADC work clock and controller clock */
>  	regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
>  			   SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0);
> @@ -553,7 +535,6 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	init_completion(&sc27xx_data->completion);
>  	sc27xx_data->dev = &pdev->dev;
>  
>  	ret = sc27xx_adc_enable(sc27xx_data);
> @@ -569,14 +550,6 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	ret = devm_request_threaded_irq(&pdev->dev, sc27xx_data->irq, NULL,
> -					sc27xx_adc_isr, IRQF_ONESHOT,
> -					pdev->name, sc27xx_data);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to request ADC irq\n");
> -		return ret;
> -	}
> -
>  	indio_dev->dev.parent = &pdev->dev;
>  	indio_dev->name = dev_name(&pdev->dev);
>  	indio_dev->modes = INDIO_DIRECT_MODE;

(Exiting) Baolin Wang July 29, 2019, 2:19 a.m. UTC | #2

Hi Jonathan,

On Sun, 28 Jul 2019 at 01:27, Jonathan Cameron <[e-mail]> wrote:
>
> On Thu, 25 Jul 2019 14:33:50 +0800
> Baolin Wang <[e-mail]> wrote:
>
> > From: Freeman Liu <[e-mail]>
> >
> > On Spreadtrum platform, the headphone will read one ADC channel multiple
> > times to identify the headphone type, and the headphone identification is
> > sensitive of the ADC reading time. And we found it will take longer time
> > to reading ADC data by using interrupt mode comparing with the polling
> > mode, thus we should change to polling mode to improve the efficiency
> > of reading data, which can identify the headphone type successfully.
> >
> > Signed-off-by: Freeman Liu <[e-mail]>
> > Signed-off-by: Baolin Wang <[e-mail]>
>
> Hi,
>
> My concerns with this sort of approach is that we may be sacrificing power
> efficiency for some usecases to support one demanding one.
>
> The maximum sleep time is 1 second (I think) which is probably too long
> to poll a register for in general.

1 second is the timeout time, that means something wrong when reading
the data taking 1 second, and we will poll the register status every
500 us.
From the testing, polling mode takes less time than interrupt mode
when reading ADC data multiple times, so polling mode did not
sacrifice power
efficiency.

> Is there some way we can bound that time and perhaps switch between
> interrupt and polling modes depending on how long we expect to wait?

I do not think the interrupt mode is needed any more, since the ADC
reading is so fast enough usually. Thanks.

>
> Thanks,
>
> Jonathan
>
> > ---
> >  drivers/iio/adc/sc27xx_adc.c |   81 ++++++++++++++----------------------------
> >  1 file changed, 27 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > index f7f7a189..ea864290 100644
> > --- a/drivers/iio/adc/sc27xx_adc.c
> > +++ b/drivers/iio/adc/sc27xx_adc.c
> > @@ -3,7 +3,6 @@
> >
> >  #include <linux/hwspinlock.h>
> >  #include <linux/iio/iio.h>
> > -#include <linux/interrupt.h>
> >  #include <linux/module.h>
> >  #include <linux/nvmem-consumer.h>
> >  #include <linux/of.h>
> > @@ -46,14 +45,18 @@
> >  /* Bits definitions for SC27XX_ADC_INT_CLR registers */
> >  #define SC27XX_ADC_IRQ_CLR           BIT(0)
> >
> > +/* Bits definitions for SC27XX_ADC_INT_RAW registers */
> > +#define SC27XX_ADC_IRQ_RAW           BIT(0)
> > +
> >  /* Mask definition for SC27XX_ADC_DATA register */
> >  #define SC27XX_ADC_DATA_MASK         GENMASK(11, 0)
> >
> >  /* Timeout (ms) for the trylock of hardware spinlocks */
> >  #define SC27XX_ADC_HWLOCK_TIMEOUT    5000
> >
> > -/* Timeout (ms) for ADC data conversion according to ADC datasheet */
> > -#define SC27XX_ADC_RDY_TIMEOUT               100
> > +/* Timeout (us) for ADC data conversion according to ADC datasheet */
> > +#define SC27XX_ADC_RDY_TIMEOUT               1000000
>
> This is 10 x the value I think...
>
> > +#define SC27XX_ADC_POLL_RAW_STATUS   500
> >
> >  /* Maximum ADC channel number */
> >  #define SC27XX_ADC_CHANNEL_MAX               32
> > @@ -72,10 +75,8 @@ struct sc27xx_adc_data {
> >        * subsystems which will access the unique ADC controller.
> >        */
> >       struct hwspinlock *hwlock;
> > -     struct completion completion;
> >       int channel_scale[SC27XX_ADC_CHANNEL_MAX];
> >       u32 base;
> > -     int value;
> >       int irq;
> >  };
> >
> > @@ -188,9 +189,7 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> >                          int scale, int *val)
> >  {
> >       int ret;
> > -     u32 tmp;
> > -
> > -     reinit_completion(&data->completion);
> > +     u32 tmp, value, status;
> >
> >       ret = hwspin_lock_timeout_raw(data->hwlock, SC27XX_ADC_HWLOCK_TIMEOUT);
> >       if (ret) {
> > @@ -203,6 +202,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> >       if (ret)
> >               goto unlock_adc;
> >
> > +     ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_CLR,
> > +                              SC27XX_ADC_IRQ_CLR, SC27XX_ADC_IRQ_CLR);
> > +     if (ret)
> > +             goto disable_adc;
> > +
> >       /* Configure the channel id and scale */
> >       tmp = (scale << SC27XX_ADC_SCALE_SHIFT) & SC27XX_ADC_SCALE_MASK;
> >       tmp |= channel & SC27XX_ADC_CHN_ID_MASK;
> > @@ -226,15 +230,22 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> >       if (ret)
> >               goto disable_adc;
> >
> > -     ret = wait_for_completion_timeout(&data->completion,
> > -                             msecs_to_jiffies(SC27XX_ADC_RDY_TIMEOUT));
> > -     if (!ret) {
> > -             dev_err(data->dev, "read ADC data timeout\n");
> > -             ret = -ETIMEDOUT;
> > -     } else {
> > -             ret = 0;
> > +     ret = regmap_read_poll_timeout(data->regmap,
> > +                                    data->base + SC27XX_ADC_INT_RAW,
> > +                                    status, (status & SC27XX_ADC_IRQ_RAW),
> > +                                    SC27XX_ADC_POLL_RAW_STATUS,
> > +                                    SC27XX_ADC_RDY_TIMEOUT);
> > +     if (ret) {
> > +             dev_err(data->dev, "read adc timeout, status = 0x%x\n", status);
> > +             goto disable_adc;
> >       }
> >
> > +     ret = regmap_read(data->regmap, data->base + SC27XX_ADC_DATA, &value);
> > +     if (ret)
> > +             goto disable_adc;
> > +
> > +     value &= SC27XX_ADC_DATA_MASK;
> > +
> >  disable_adc:
> >       regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
> >                          SC27XX_ADC_EN, 0);
> > @@ -242,32 +253,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> >       hwspin_unlock_raw(data->hwlock);
> >
> >       if (!ret)
> > -             *val = data->value;
> > +             *val = value;
> >
> >       return ret;
> >  }
> >
> > -static irqreturn_t sc27xx_adc_isr(int irq, void *dev_id)
> > -{
> > -     struct sc27xx_adc_data *data = dev_id;
> > -     int ret;
> > -
> > -     ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_CLR,
> > -                              SC27XX_ADC_IRQ_CLR, SC27XX_ADC_IRQ_CLR);
> > -     if (ret)
> > -             return IRQ_RETVAL(ret);
> > -
> > -     ret = regmap_read(data->regmap, data->base + SC27XX_ADC_DATA,
> > -                       &data->value);
> > -     if (ret)
> > -             return IRQ_RETVAL(ret);
> > -
> > -     data->value &= SC27XX_ADC_DATA_MASK;
> > -     complete(&data->completion);
> > -
> > -     return IRQ_HANDLED;
> > -}
> > -
> >  static void sc27xx_adc_volt_ratio(struct sc27xx_adc_data *data,
> >                                 int channel, int scale,
> >                                 u32 *div_numerator, u32 *div_denominator)
> > @@ -454,11 +444,6 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
> >       if (ret)
> >               goto disable_adc;
> >
> > -     ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_EN,
> > -                              SC27XX_ADC_IRQ_EN, SC27XX_ADC_IRQ_EN);
> > -     if (ret)
> > -             goto disable_clk;
> > -
> >       /* ADC channel scales' calibration from nvmem device */
> >       ret = sc27xx_adc_scale_calibration(data, true);
> >       if (ret)
> > @@ -484,9 +469,6 @@ static void sc27xx_adc_disable(void *_data)
> >  {
> >       struct sc27xx_adc_data *data = _data;
> >
> > -     regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_EN,
> > -                        SC27XX_ADC_IRQ_EN, 0);
> > -
> >       /* Disable ADC work clock and controller clock */
> >       regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
> >                          SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0);
> > @@ -553,7 +535,6 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
> >               return ret;
> >       }
> >
> > -     init_completion(&sc27xx_data->completion);
> >       sc27xx_data->dev = &pdev->dev;
> >
> >       ret = sc27xx_adc_enable(sc27xx_data);
> > @@ -569,14 +550,6 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
> >               return ret;
> >       }
> >
> > -     ret = devm_request_threaded_irq(&pdev->dev, sc27xx_data->irq, NULL,
> > -                                     sc27xx_adc_isr, IRQF_ONESHOT,
> > -                                     pdev->name, sc27xx_data);
> > -     if (ret) {
> > -             dev_err(&pdev->dev, "failed to request ADC irq\n");
> > -             return ret;
> > -     }
> > -
> >       indio_dev->dev.parent = &pdev->dev;
> >       indio_dev->name = dev_name(&pdev->dev);
> >       indio_dev->modes = INDIO_DIRECT_MODE;
>

Jonathan Cameron Aug. 5, 2019, 1:50 p.m. UTC | #3

On Mon, 29 Jul 2019 10:19:48 +0800
Baolin Wang <[e-mail]> wrote:

> Hi Jonathan,
> 
> On Sun, 28 Jul 2019 at 01:27, Jonathan Cameron <[e-mail]> wrote:
> >
> > On Thu, 25 Jul 2019 14:33:50 +0800
> > Baolin Wang <[e-mail]> wrote:
> >  
> > > From: Freeman Liu <[e-mail]>
> > >
> > > On Spreadtrum platform, the headphone will read one ADC channel multiple
> > > times to identify the headphone type, and the headphone identification is
> > > sensitive of the ADC reading time. And we found it will take longer time
> > > to reading ADC data by using interrupt mode comparing with the polling
> > > mode, thus we should change to polling mode to improve the efficiency
> > > of reading data, which can identify the headphone type successfully.
> > >
> > > Signed-off-by: Freeman Liu <[e-mail]>
> > > Signed-off-by: Baolin Wang <[e-mail]>  
> >
> > Hi,
> >
> > My concerns with this sort of approach is that we may be sacrificing power
> > efficiency for some usecases to support one demanding one.
> >
> > The maximum sleep time is 1 second (I think) which is probably too long
> > to poll a register for in general.  
> 
> 1 second is the timeout time, that means something wrong when reading
> the data taking 1 second, and we will poll the register status every
> 500 us.
> From the testing, polling mode takes less time than interrupt mode
> when reading ADC data multiple times, so polling mode did not
> sacrifice power
> efficiency.

Hmm.  I'll go with a probably on that, depends on interrupt response
latency etc so isn't entirely obvious.  Faster response doesn't necessarily
mean lower power.

> 
> > Is there some way we can bound that time and perhaps switch between
> > interrupt and polling modes depending on how long we expect to wait?  
> 
> I do not think the interrupt mode is needed any more, since the ADC
> reading is so fast enough usually. Thanks.
The reason for interrupts in such devices is usually precisely the opposite.

You do it because things are slow enough that you can go to sleep
for a long time before the interrupt occurs.

So question becomes whether there are circumstances in which we are
running with long timescales and would benefit from using interrupts.

Thanks,

Jonathan


> 
> >
> > Thanks,
> >
> > Jonathan
> >  
> > > ---
> > >  drivers/iio/adc/sc27xx_adc.c |   81 ++++++++++++++----------------------------
> > >  1 file changed, 27 insertions(+), 54 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > > index f7f7a189..ea864290 100644
> > > --- a/drivers/iio/adc/sc27xx_adc.c
> > > +++ b/drivers/iio/adc/sc27xx_adc.c
> > > @@ -3,7 +3,6 @@
> > >
> > >  #include <linux/hwspinlock.h>
> > >  #include <linux/iio/iio.h>
> > > -#include <linux/interrupt.h>
> > >  #include <linux/module.h>
> > >  #include <linux/nvmem-consumer.h>
> > >  #include <linux/of.h>
> > > @@ -46,14 +45,18 @@
> > >  /* Bits definitions for SC27XX_ADC_INT_CLR registers */
> > >  #define SC27XX_ADC_IRQ_CLR           BIT(0)
> > >
> > > +/* Bits definitions for SC27XX_ADC_INT_RAW registers */
> > > +#define SC27XX_ADC_IRQ_RAW           BIT(0)
> > > +
> > >  /* Mask definition for SC27XX_ADC_DATA register */
> > >  #define SC27XX_ADC_DATA_MASK         GENMASK(11, 0)
> > >
> > >  /* Timeout (ms) for the trylock of hardware spinlocks */
> > >  #define SC27XX_ADC_HWLOCK_TIMEOUT    5000
> > >
> > > -/* Timeout (ms) for ADC data conversion according to ADC datasheet */
> > > -#define SC27XX_ADC_RDY_TIMEOUT               100
> > > +/* Timeout (us) for ADC data conversion according to ADC datasheet */
> > > +#define SC27XX_ADC_RDY_TIMEOUT               1000000  
> >
> > This is 10 x the value I think...
> >  
> > > +#define SC27XX_ADC_POLL_RAW_STATUS   500
> > >
> > >  /* Maximum ADC channel number */
> > >  #define SC27XX_ADC_CHANNEL_MAX               32
> > > @@ -72,10 +75,8 @@ struct sc27xx_adc_data {
> > >        * subsystems which will access the unique ADC controller.
> > >        */
> > >       struct hwspinlock *hwlock;
> > > -     struct completion completion;
> > >       int channel_scale[SC27XX_ADC_CHANNEL_MAX];
> > >       u32 base;
> > > -     int value;
> > >       int irq;
> > >  };
> > >
> > > @@ -188,9 +189,7 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> > >                          int scale, int *val)
> > >  {
> > >       int ret;
> > > -     u32 tmp;
> > > -
> > > -     reinit_completion(&data->completion);
> > > +     u32 tmp, value, status;
> > >
> > >       ret = hwspin_lock_timeout_raw(data->hwlock, SC27XX_ADC_HWLOCK_TIMEOUT);
> > >       if (ret) {
> > > @@ -203,6 +202,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> > >       if (ret)
> > >               goto unlock_adc;
> > >
> > > +     ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_CLR,
> > > +                              SC27XX_ADC_IRQ_CLR, SC27XX_ADC_IRQ_CLR);
> > > +     if (ret)
> > > +             goto disable_adc;
> > > +
> > >       /* Configure the channel id and scale */
> > >       tmp = (scale << SC27XX_ADC_SCALE_SHIFT) & SC27XX_ADC_SCALE_MASK;
> > >       tmp |= channel & SC27XX_ADC_CHN_ID_MASK;
> > > @@ -226,15 +230,22 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> > >       if (ret)
> > >               goto disable_adc;
> > >
> > > -     ret = wait_for_completion_timeout(&data->completion,
> > > -                             msecs_to_jiffies(SC27XX_ADC_RDY_TIMEOUT));
> > > -     if (!ret) {
> > > -             dev_err(data->dev, "read ADC data timeout\n");
> > > -             ret = -ETIMEDOUT;
> > > -     } else {
> > > -             ret = 0;
> > > +     ret = regmap_read_poll_timeout(data->regmap,
> > > +                                    data->base + SC27XX_ADC_INT_RAW,
> > > +                                    status, (status & SC27XX_ADC_IRQ_RAW),
> > > +                                    SC27XX_ADC_POLL_RAW_STATUS,
> > > +                                    SC27XX_ADC_RDY_TIMEOUT);
> > > +     if (ret) {
> > > +             dev_err(data->dev, "read adc timeout, status = 0x%x\n", status);
> > > +             goto disable_adc;
> > >       }
> > >
> > > +     ret = regmap_read(data->regmap, data->base + SC27XX_ADC_DATA, &value);
> > > +     if (ret)
> > > +             goto disable_adc;
> > > +
> > > +     value &= SC27XX_ADC_DATA_MASK;
> > > +
> > >  disable_adc:
> > >       regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
> > >                          SC27XX_ADC_EN, 0);
> > > @@ -242,32 +253,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> > >       hwspin_unlock_raw(data->hwlock);
> > >
> > >       if (!ret)
> > > -             *val = data->value;
> > > +             *val = value;
> > >
> > >       return ret;
> > >  }
> > >
> > > -static irqreturn_t sc27xx_adc_isr(int irq, void *dev_id)
> > > -{
> > > -     struct sc27xx_adc_data *data = dev_id;
> > > -     int ret;
> > > -
> > > -     ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_CLR,
> > > -                              SC27XX_ADC_IRQ_CLR, SC27XX_ADC_IRQ_CLR);
> > > -     if (ret)
> > > -             return IRQ_RETVAL(ret);
> > > -
> > > -     ret = regmap_read(data->regmap, data->base + SC27XX_ADC_DATA,
> > > -                       &data->value);
> > > -     if (ret)
> > > -             return IRQ_RETVAL(ret);
> > > -
> > > -     data->value &= SC27XX_ADC_DATA_MASK;
> > > -     complete(&data->completion);
> > > -
> > > -     return IRQ_HANDLED;
> > > -}
> > > -
> > >  static void sc27xx_adc_volt_ratio(struct sc27xx_adc_data *data,
> > >                                 int channel, int scale,
> > >                                 u32 *div_numerator, u32 *div_denominator)
> > > @@ -454,11 +444,6 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
> > >       if (ret)
> > >               goto disable_adc;
> > >
> > > -     ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_EN,
> > > -                              SC27XX_ADC_IRQ_EN, SC27XX_ADC_IRQ_EN);
> > > -     if (ret)
> > > -             goto disable_clk;
> > > -
> > >       /* ADC channel scales' calibration from nvmem device */
> > >       ret = sc27xx_adc_scale_calibration(data, true);
> > >       if (ret)
> > > @@ -484,9 +469,6 @@ static void sc27xx_adc_disable(void *_data)
> > >  {
> > >       struct sc27xx_adc_data *data = _data;
> > >
> > > -     regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_EN,
> > > -                        SC27XX_ADC_IRQ_EN, 0);
> > > -
> > >       /* Disable ADC work clock and controller clock */
> > >       regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
> > >                          SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0);
> > > @@ -553,7 +535,6 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
> > >               return ret;
> > >       }
> > >
> > > -     init_completion(&sc27xx_data->completion);
> > >       sc27xx_data->dev = &pdev->dev;
> > >
> > >       ret = sc27xx_adc_enable(sc27xx_data);
> > > @@ -569,14 +550,6 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
> > >               return ret;
> > >       }
> > >
> > > -     ret = devm_request_threaded_irq(&pdev->dev, sc27xx_data->irq, NULL,
> > > -                                     sc27xx_adc_isr, IRQF_ONESHOT,
> > > -                                     pdev->name, sc27xx_data);
> > > -     if (ret) {
> > > -             dev_err(&pdev->dev, "failed to request ADC irq\n");
> > > -             return ret;
> > > -     }
> > > -
> > >       indio_dev->dev.parent = &pdev->dev;
> > >       indio_dev->name = dev_name(&pdev->dev);
> > >       indio_dev->modes = INDIO_DIRECT_MODE;  
> >  
> 
>

(Exiting) Baolin Wang Aug. 6, 2019, 7:39 a.m. UTC | #4

Hi Jonathan,

On Mon, 5 Aug 2019 at 21:50, Jonathan Cameron <[e-mail]> wrote:
>
> On Mon, 29 Jul 2019 10:19:48 +0800
> Baolin Wang <[e-mail]> wrote:
>
> > Hi Jonathan,
> >
> > On Sun, 28 Jul 2019 at 01:27, Jonathan Cameron <[e-mail]> wrote:
> > >
> > > On Thu, 25 Jul 2019 14:33:50 +0800
> > > Baolin Wang <[e-mail]> wrote:
> > >
> > > > From: Freeman Liu <[e-mail]>
> > > >
> > > > On Spreadtrum platform, the headphone will read one ADC channel multiple
> > > > times to identify the headphone type, and the headphone identification is
> > > > sensitive of the ADC reading time. And we found it will take longer time
> > > > to reading ADC data by using interrupt mode comparing with the polling
> > > > mode, thus we should change to polling mode to improve the efficiency
> > > > of reading data, which can identify the headphone type successfully.
> > > >
> > > > Signed-off-by: Freeman Liu <[e-mail]>
> > > > Signed-off-by: Baolin Wang <[e-mail]>
> > >
> > > Hi,
> > >
> > > My concerns with this sort of approach is that we may be sacrificing power
> > > efficiency for some usecases to support one demanding one.
> > >
> > > The maximum sleep time is 1 second (I think) which is probably too long
> > > to poll a register for in general.
> >
> > 1 second is the timeout time, that means something wrong when reading
> > the data taking 1 second, and we will poll the register status every
> > 500 us.
> > From the testing, polling mode takes less time than interrupt mode
> > when reading ADC data multiple times, so polling mode did not
> > sacrifice power
> > efficiency.
>
> Hmm.  I'll go with a probably on that, depends on interrupt response
> latency etc so isn't entirely obvious.  Faster response doesn't necessarily
> mean lower power.
>
> >
> > > Is there some way we can bound that time and perhaps switch between
> > > interrupt and polling modes depending on how long we expect to wait?
> >
> > I do not think the interrupt mode is needed any more, since the ADC
> > reading is so fast enough usually. Thanks.
> The reason for interrupts in such devices is usually precisely the opposite.
>
> You do it because things are slow enough that you can go to sleep
> for a long time before the interrupt occurs.
>
> So question becomes whether there are circumstances in which we are
> running with long timescales and would benefit from using interrupts.

From our testing, the ADC version time is usually about 100us, it will
be faster to get data if we poll every 50us in this case. But if we
change to use interrupt mode, it will take millisecond level time to
get data. That will cause problems for those time sensitive scenarios,
like headphone detection, that's the main reason we can not use
interrupt mode.

For those non-time-sensitive scenarios, yes, I agree with you, the
interrupt mode will get a better power efficiency. But ADC driver can
not know what scenarios asked by consumers, so changing to polling
mode seems the easiest way to solve the problem, and we've applied
this patch in our downstream kernel for a while, we did not see any
other problem.

Thanks for your comments.

Jonathan Cameron Aug. 11, 2019, 8:03 a.m. UTC | #5

On Tue, 6 Aug 2019 15:39:45 +0800
Baolin Wang <[e-mail]> wrote:

> Hi Jonathan,
> 
> On Mon, 5 Aug 2019 at 21:50, Jonathan Cameron <[e-mail]> wrote:
> >
> > On Mon, 29 Jul 2019 10:19:48 +0800
> > Baolin Wang <[e-mail]> wrote:
> >  
> > > Hi Jonathan,
> > >
> > > On Sun, 28 Jul 2019 at 01:27, Jonathan Cameron <[e-mail]> wrote:  
> > > >
> > > > On Thu, 25 Jul 2019 14:33:50 +0800
> > > > Baolin Wang <[e-mail]> wrote:
> > > >  
> > > > > From: Freeman Liu <[e-mail]>
> > > > >
> > > > > On Spreadtrum platform, the headphone will read one ADC channel multiple
> > > > > times to identify the headphone type, and the headphone identification is
> > > > > sensitive of the ADC reading time. And we found it will take longer time
> > > > > to reading ADC data by using interrupt mode comparing with the polling
> > > > > mode, thus we should change to polling mode to improve the efficiency
> > > > > of reading data, which can identify the headphone type successfully.
> > > > >
> > > > > Signed-off-by: Freeman Liu <[e-mail]>
> > > > > Signed-off-by: Baolin Wang <[e-mail]>  
> > > >
> > > > Hi,
> > > >
> > > > My concerns with this sort of approach is that we may be sacrificing power
> > > > efficiency for some usecases to support one demanding one.
> > > >
> > > > The maximum sleep time is 1 second (I think) which is probably too long
> > > > to poll a register for in general.  
> > >
> > > 1 second is the timeout time, that means something wrong when reading
> > > the data taking 1 second, and we will poll the register status every
> > > 500 us.
> > > From the testing, polling mode takes less time than interrupt mode
> > > when reading ADC data multiple times, so polling mode did not
> > > sacrifice power
> > > efficiency.  
> >
> > Hmm.  I'll go with a probably on that, depends on interrupt response
> > latency etc so isn't entirely obvious.  Faster response doesn't necessarily
> > mean lower power.
> >  
> > >  
> > > > Is there some way we can bound that time and perhaps switch between
> > > > interrupt and polling modes depending on how long we expect to wait?  
> > >
> > > I do not think the interrupt mode is needed any more, since the ADC
> > > reading is so fast enough usually. Thanks.  
> > The reason for interrupts in such devices is usually precisely the opposite.
> >
> > You do it because things are slow enough that you can go to sleep
> > for a long time before the interrupt occurs.
> >
> > So question becomes whether there are circumstances in which we are
> > running with long timescales and would benefit from using interrupts.  
> 
> From our testing, the ADC version time is usually about 100us, it will
> be faster to get data if we poll every 50us in this case. But if we
> change to use interrupt mode, it will take millisecond level time to
> get data. That will cause problems for those time sensitive scenarios,
> like headphone detection, that's the main reason we can not use
> interrupt mode.
> 
> For those non-time-sensitive scenarios, yes, I agree with you, the
> interrupt mode will get a better power efficiency. But ADC driver can
> not know what scenarios asked by consumers, so changing to polling
> mode seems the easiest way to solve the problem, and we've applied
> this patch in our downstream kernel for a while, we did not see any
> other problem.
> 
> Thanks for your comments.

OK. It's not ideal but sometimes such is life ;)

So last question - fix or not?  If a fix, can I have a fixes tag
please.

Thanks,

Jonathan

>

(Exiting) Baolin Wang Aug. 12, 2019, 2:37 a.m. UTC | #6

On Sun, 11 Aug 2019 at 16:03, Jonathan Cameron <[e-mail]> wrote:
>
> On Tue, 6 Aug 2019 15:39:45 +0800
> Baolin Wang <[e-mail]> wrote:
>
> > Hi Jonathan,
> >
> > On Mon, 5 Aug 2019 at 21:50, Jonathan Cameron <[e-mail]> wrote:
> > >
> > > On Mon, 29 Jul 2019 10:19:48 +0800
> > > Baolin Wang <[e-mail]> wrote:
> > >
> > > > Hi Jonathan,
> > > >
> > > > On Sun, 28 Jul 2019 at 01:27, Jonathan Cameron <[e-mail]> wrote:
> > > > >
> > > > > On Thu, 25 Jul 2019 14:33:50 +0800
> > > > > Baolin Wang <[e-mail]> wrote:
> > > > >
> > > > > > From: Freeman Liu <[e-mail]>
> > > > > >
> > > > > > On Spreadtrum platform, the headphone will read one ADC channel multiple
> > > > > > times to identify the headphone type, and the headphone identification is
> > > > > > sensitive of the ADC reading time. And we found it will take longer time
> > > > > > to reading ADC data by using interrupt mode comparing with the polling
> > > > > > mode, thus we should change to polling mode to improve the efficiency
> > > > > > of reading data, which can identify the headphone type successfully.
> > > > > >
> > > > > > Signed-off-by: Freeman Liu <[e-mail]>
> > > > > > Signed-off-by: Baolin Wang <[e-mail]>
> > > > >
> > > > > Hi,
> > > > >
> > > > > My concerns with this sort of approach is that we may be sacrificing power
> > > > > efficiency for some usecases to support one demanding one.
> > > > >
> > > > > The maximum sleep time is 1 second (I think) which is probably too long
> > > > > to poll a register for in general.
> > > >
> > > > 1 second is the timeout time, that means something wrong when reading
> > > > the data taking 1 second, and we will poll the register status every
> > > > 500 us.
> > > > From the testing, polling mode takes less time than interrupt mode
> > > > when reading ADC data multiple times, so polling mode did not
> > > > sacrifice power
> > > > efficiency.
> > >
> > > Hmm.  I'll go with a probably on that, depends on interrupt response
> > > latency etc so isn't entirely obvious.  Faster response doesn't necessarily
> > > mean lower power.
> > >
> > > >
> > > > > Is there some way we can bound that time and perhaps switch between
> > > > > interrupt and polling modes depending on how long we expect to wait?
> > > >
> > > > I do not think the interrupt mode is needed any more, since the ADC
> > > > reading is so fast enough usually. Thanks.
> > > The reason for interrupts in such devices is usually precisely the opposite.
> > >
> > > You do it because things are slow enough that you can go to sleep
> > > for a long time before the interrupt occurs.
> > >
> > > So question becomes whether there are circumstances in which we are
> > > running with long timescales and would benefit from using interrupts.
> >
> > From our testing, the ADC version time is usually about 100us, it will
> > be faster to get data if we poll every 50us in this case. But if we
> > change to use interrupt mode, it will take millisecond level time to
> > get data. That will cause problems for those time sensitive scenarios,
> > like headphone detection, that's the main reason we can not use
> > interrupt mode.
> >
> > For those non-time-sensitive scenarios, yes, I agree with you, the
> > interrupt mode will get a better power efficiency. But ADC driver can
> > not know what scenarios asked by consumers, so changing to polling
> > mode seems the easiest way to solve the problem, and we've applied
> > this patch in our downstream kernel for a while, we did not see any
> > other problem.
> >
> > Thanks for your comments.
>
> OK. It's not ideal but sometimes such is life ;)

Thanks for your understanding :)

>
> So last question - fix or not?  If a fix, can I have a fixes tag
> please.

This is a bigger patch, I am afraid it can not be merged into stable
kernel, and original code can work at most scenarios. So I think no
need add stable tag for this patch. Thanks.

Jonathan Cameron Aug. 18, 2019, 7:27 p.m. UTC | #7

On Mon, 12 Aug 2019 10:37:44 +0800
Baolin Wang <[e-mail]> wrote:

> On Sun, 11 Aug 2019 at 16:03, Jonathan Cameron <[e-mail]> wrote:
> >
> > On Tue, 6 Aug 2019 15:39:45 +0800
> > Baolin Wang <[e-mail]> wrote:
> >  
> > > Hi Jonathan,
> > >
> > > On Mon, 5 Aug 2019 at 21:50, Jonathan Cameron <[e-mail]> wrote:  
> > > >
> > > > On Mon, 29 Jul 2019 10:19:48 +0800
> > > > Baolin Wang <[e-mail]> wrote:
> > > >  
> > > > > Hi Jonathan,
> > > > >
> > > > > On Sun, 28 Jul 2019 at 01:27, Jonathan Cameron <[e-mail]> wrote:  
> > > > > >
> > > > > > On Thu, 25 Jul 2019 14:33:50 +0800
> > > > > > Baolin Wang <[e-mail]> wrote:
> > > > > >  
> > > > > > > From: Freeman Liu <[e-mail]>
> > > > > > >
> > > > > > > On Spreadtrum platform, the headphone will read one ADC channel multiple
> > > > > > > times to identify the headphone type, and the headphone identification is
> > > > > > > sensitive of the ADC reading time. And we found it will take longer time
> > > > > > > to reading ADC data by using interrupt mode comparing with the polling
> > > > > > > mode, thus we should change to polling mode to improve the efficiency
> > > > > > > of reading data, which can identify the headphone type successfully.
> > > > > > >
> > > > > > > Signed-off-by: Freeman Liu <[e-mail]>
> > > > > > > Signed-off-by: Baolin Wang <[e-mail]>  
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > My concerns with this sort of approach is that we may be sacrificing power
> > > > > > efficiency for some usecases to support one demanding one.
> > > > > >
> > > > > > The maximum sleep time is 1 second (I think) which is probably too long
> > > > > > to poll a register for in general.  
> > > > >
> > > > > 1 second is the timeout time, that means something wrong when reading
> > > > > the data taking 1 second, and we will poll the register status every
> > > > > 500 us.
> > > > > From the testing, polling mode takes less time than interrupt mode
> > > > > when reading ADC data multiple times, so polling mode did not
> > > > > sacrifice power
> > > > > efficiency.  
> > > >
> > > > Hmm.  I'll go with a probably on that, depends on interrupt response
> > > > latency etc so isn't entirely obvious.  Faster response doesn't necessarily
> > > > mean lower power.
> > > >  
> > > > >  
> > > > > > Is there some way we can bound that time and perhaps switch between
> > > > > > interrupt and polling modes depending on how long we expect to wait?  
> > > > >
> > > > > I do not think the interrupt mode is needed any more, since the ADC
> > > > > reading is so fast enough usually. Thanks.  
> > > > The reason for interrupts in such devices is usually precisely the opposite.
> > > >
> > > > You do it because things are slow enough that you can go to sleep
> > > > for a long time before the interrupt occurs.
> > > >
> > > > So question becomes whether there are circumstances in which we are
> > > > running with long timescales and would benefit from using interrupts.  
> > >
> > > From our testing, the ADC version time is usually about 100us, it will
> > > be faster to get data if we poll every 50us in this case. But if we
> > > change to use interrupt mode, it will take millisecond level time to
> > > get data. That will cause problems for those time sensitive scenarios,
> > > like headphone detection, that's the main reason we can not use
> > > interrupt mode.
> > >
> > > For those non-time-sensitive scenarios, yes, I agree with you, the
> > > interrupt mode will get a better power efficiency. But ADC driver can
> > > not know what scenarios asked by consumers, so changing to polling
> > > mode seems the easiest way to solve the problem, and we've applied
> > > this patch in our downstream kernel for a while, we did not see any
> > > other problem.
> > >
> > > Thanks for your comments.  
> >
> > OK. It's not ideal but sometimes such is life ;)  
> 
> Thanks for your understanding :)
> 
> >
> > So last question - fix or not?  If a fix, can I have a fixes tag
> > please.  
> 
> This is a bigger patch, I am afraid it can not be merged into stable
> kernel, and original code can work at most scenarios. So I think no
> need add stable tag for this patch. Thanks.
> 
Fair enough.  Needed a bit of merge effort as crossed with a generic
cleanup patch it seems.

Anyhow, hopefully I got it right.

Pushed out as testing for the autobuilders to play with it.

Thanks,

Jonathan

(Exiting) Baolin Wang Aug. 19, 2019, 1:12 a.m. UTC | #8

On Mon, 19 Aug 2019 at 03:27, Jonathan Cameron <[e-mail]> wrote:
>
> On Mon, 12 Aug 2019 10:37:44 +0800
> Baolin Wang <[e-mail]> wrote:
>
> > On Sun, 11 Aug 2019 at 16:03, Jonathan Cameron <[e-mail]> wrote:
> > >
> > > On Tue, 6 Aug 2019 15:39:45 +0800
> > > Baolin Wang <[e-mail]> wrote:
> > >
> > > > Hi Jonathan,
> > > >
> > > > On Mon, 5 Aug 2019 at 21:50, Jonathan Cameron <[e-mail]> wrote:
> > > > >
> > > > > On Mon, 29 Jul 2019 10:19:48 +0800
> > > > > Baolin Wang <[e-mail]> wrote:
> > > > >
> > > > > > Hi Jonathan,
> > > > > >
> > > > > > On Sun, 28 Jul 2019 at 01:27, Jonathan Cameron <[e-mail]> wrote:
> > > > > > >
> > > > > > > On Thu, 25 Jul 2019 14:33:50 +0800
> > > > > > > Baolin Wang <[e-mail]> wrote:
> > > > > > >
> > > > > > > > From: Freeman Liu <[e-mail]>
> > > > > > > >
> > > > > > > > On Spreadtrum platform, the headphone will read one ADC channel multiple
> > > > > > > > times to identify the headphone type, and the headphone identification is
> > > > > > > > sensitive of the ADC reading time. And we found it will take longer time
> > > > > > > > to reading ADC data by using interrupt mode comparing with the polling
> > > > > > > > mode, thus we should change to polling mode to improve the efficiency
> > > > > > > > of reading data, which can identify the headphone type successfully.
> > > > > > > >
> > > > > > > > Signed-off-by: Freeman Liu <[e-mail]>
> > > > > > > > Signed-off-by: Baolin Wang <[e-mail]>
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > My concerns with this sort of approach is that we may be sacrificing power
> > > > > > > efficiency for some usecases to support one demanding one.
> > > > > > >
> > > > > > > The maximum sleep time is 1 second (I think) which is probably too long
> > > > > > > to poll a register for in general.
> > > > > >
> > > > > > 1 second is the timeout time, that means something wrong when reading
> > > > > > the data taking 1 second, and we will poll the register status every
> > > > > > 500 us.
> > > > > > From the testing, polling mode takes less time than interrupt mode
> > > > > > when reading ADC data multiple times, so polling mode did not
> > > > > > sacrifice power
> > > > > > efficiency.
> > > > >
> > > > > Hmm.  I'll go with a probably on that, depends on interrupt response
> > > > > latency etc so isn't entirely obvious.  Faster response doesn't necessarily
> > > > > mean lower power.
> > > > >
> > > > > >
> > > > > > > Is there some way we can bound that time and perhaps switch between
> > > > > > > interrupt and polling modes depending on how long we expect to wait?
> > > > > >
> > > > > > I do not think the interrupt mode is needed any more, since the ADC
> > > > > > reading is so fast enough usually. Thanks.
> > > > > The reason for interrupts in such devices is usually precisely the opposite.
> > > > >
> > > > > You do it because things are slow enough that you can go to sleep
> > > > > for a long time before the interrupt occurs.
> > > > >
> > > > > So question becomes whether there are circumstances in which we are
> > > > > running with long timescales and would benefit from using interrupts.
> > > >
> > > > From our testing, the ADC version time is usually about 100us, it will
> > > > be faster to get data if we poll every 50us in this case. But if we
> > > > change to use interrupt mode, it will take millisecond level time to
> > > > get data. That will cause problems for those time sensitive scenarios,
> > > > like headphone detection, that's the main reason we can not use
> > > > interrupt mode.
> > > >
> > > > For those non-time-sensitive scenarios, yes, I agree with you, the
> > > > interrupt mode will get a better power efficiency. But ADC driver can
> > > > not know what scenarios asked by consumers, so changing to polling
> > > > mode seems the easiest way to solve the problem, and we've applied
> > > > this patch in our downstream kernel for a while, we did not see any
> > > > other problem.
> > > >
> > > > Thanks for your comments.
> > >
> > > OK. It's not ideal but sometimes such is life ;)
> >
> > Thanks for your understanding :)
> >
> > >
> > > So last question - fix or not?  If a fix, can I have a fixes tag
> > > please.
> >
> > This is a bigger patch, I am afraid it can not be merged into stable
> > kernel, and original code can work at most scenarios. So I think no
> > need add stable tag for this patch. Thanks.
> >
> Fair enough.  Needed a bit of merge effort as crossed with a generic
> cleanup patch it seems.
>
> Anyhow, hopefully I got it right.

I checked you are right.

>
> Pushed out as testing for the autobuilders to play with it.

Thanks.

Patch

diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
index f7f7a189..ea864290 100644
--- a/drivers/iio/adc/sc27xx_adc.c
+++ b/drivers/iio/adc/sc27xx_adc.c
@@ -3,7 +3,6 @@ 
 
 #include <linux/hwspinlock.h>
 #include <linux/iio/iio.h>
-#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/of.h>
@@ -46,14 +45,18 @@ 
 /* Bits definitions for SC27XX_ADC_INT_CLR registers */
 #define SC27XX_ADC_IRQ_CLR		BIT(0)
 
+/* Bits definitions for SC27XX_ADC_INT_RAW registers */
+#define SC27XX_ADC_IRQ_RAW		BIT(0)
+
 /* Mask definition for SC27XX_ADC_DATA register */
 #define SC27XX_ADC_DATA_MASK		GENMASK(11, 0)
 
 /* Timeout (ms) for the trylock of hardware spinlocks */
 #define SC27XX_ADC_HWLOCK_TIMEOUT	5000
 
-/* Timeout (ms) for ADC data conversion according to ADC datasheet */
-#define SC27XX_ADC_RDY_TIMEOUT		100
+/* Timeout (us) for ADC data conversion according to ADC datasheet */
+#define SC27XX_ADC_RDY_TIMEOUT		1000000
+#define SC27XX_ADC_POLL_RAW_STATUS	500
 
 /* Maximum ADC channel number */
 #define SC27XX_ADC_CHANNEL_MAX		32
@@ -72,10 +75,8 @@  struct sc27xx_adc_data {
 	 * subsystems which will access the unique ADC controller.
 	 */
 	struct hwspinlock *hwlock;
-	struct completion completion;
 	int channel_scale[SC27XX_ADC_CHANNEL_MAX];
 	u32 base;
-	int value;
 	int irq;
 };
 
@@ -188,9 +189,7 @@  static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
 			   int scale, int *val)
 {
 	int ret;
-	u32 tmp;
-
-	reinit_completion(&data->completion);
+	u32 tmp, value, status;
 
 	ret = hwspin_lock_timeout_raw(data->hwlock, SC27XX_ADC_HWLOCK_TIMEOUT);
 	if (ret) {
@@ -203,6 +202,11 @@  static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
 	if (ret)
 		goto unlock_adc;
 
+	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_CLR,
+				 SC27XX_ADC_IRQ_CLR, SC27XX_ADC_IRQ_CLR);
+	if (ret)
+		goto disable_adc;
+
 	/* Configure the channel id and scale */
 	tmp = (scale << SC27XX_ADC_SCALE_SHIFT) & SC27XX_ADC_SCALE_MASK;
 	tmp |= channel & SC27XX_ADC_CHN_ID_MASK;
@@ -226,15 +230,22 @@  static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
 	if (ret)
 		goto disable_adc;
 
-	ret = wait_for_completion_timeout(&data->completion,
-				msecs_to_jiffies(SC27XX_ADC_RDY_TIMEOUT));
-	if (!ret) {
-		dev_err(data->dev, "read ADC data timeout\n");
-		ret = -ETIMEDOUT;
-	} else {
-		ret = 0;
+	ret = regmap_read_poll_timeout(data->regmap,
+				       data->base + SC27XX_ADC_INT_RAW,
+				       status, (status & SC27XX_ADC_IRQ_RAW),
+				       SC27XX_ADC_POLL_RAW_STATUS,
+				       SC27XX_ADC_RDY_TIMEOUT);
+	if (ret) {
+		dev_err(data->dev, "read adc timeout, status = 0x%x\n", status);
+		goto disable_adc;
 	}
 
+	ret = regmap_read(data->regmap, data->base + SC27XX_ADC_DATA, &value);
+	if (ret)
+		goto disable_adc;
+
+	value &= SC27XX_ADC_DATA_MASK;
+
 disable_adc:
 	regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
 			   SC27XX_ADC_EN, 0);
@@ -242,32 +253,11 @@  static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
 	hwspin_unlock_raw(data->hwlock);
 
 	if (!ret)
-		*val = data->value;
+		*val = value;
 
 	return ret;
 }
 
-static irqreturn_t sc27xx_adc_isr(int irq, void *dev_id)
-{
-	struct sc27xx_adc_data *data = dev_id;
-	int ret;
-
-	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_CLR,
-				 SC27XX_ADC_IRQ_CLR, SC27XX_ADC_IRQ_CLR);
-	if (ret)
-		return IRQ_RETVAL(ret);
-
-	ret = regmap_read(data->regmap, data->base + SC27XX_ADC_DATA,
-			  &data->value);
-	if (ret)
-		return IRQ_RETVAL(ret);
-
-	data->value &= SC27XX_ADC_DATA_MASK;
-	complete(&data->completion);
-
-	return IRQ_HANDLED;
-}
-
 static void sc27xx_adc_volt_ratio(struct sc27xx_adc_data *data,
 				  int channel, int scale,
 				  u32 *div_numerator, u32 *div_denominator)
@@ -454,11 +444,6 @@  static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
 	if (ret)
 		goto disable_adc;
 
-	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_EN,
-				 SC27XX_ADC_IRQ_EN, SC27XX_ADC_IRQ_EN);
-	if (ret)
-		goto disable_clk;
-
 	/* ADC channel scales' calibration from nvmem device */
 	ret = sc27xx_adc_scale_calibration(data, true);
 	if (ret)
@@ -484,9 +469,6 @@  static void sc27xx_adc_disable(void *_data)
 {
 	struct sc27xx_adc_data *data = _data;
 
-	regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_EN,
-			   SC27XX_ADC_IRQ_EN, 0);
-
 	/* Disable ADC work clock and controller clock */
 	regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
 			   SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0);
@@ -553,7 +535,6 @@  static int sc27xx_adc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	init_completion(&sc27xx_data->completion);
 	sc27xx_data->dev = &pdev->dev;
 
 	ret = sc27xx_adc_enable(sc27xx_data);
@@ -569,14 +550,6 @@  static int sc27xx_adc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = devm_request_threaded_irq(&pdev->dev, sc27xx_data->irq, NULL,
-					sc27xx_adc_isr, IRQF_ONESHOT,
-					pdev->name, sc27xx_data);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to request ADC irq\n");
-		return ret;
-	}
-
 	indio_dev->dev.parent = &pdev->dev;
 	indio_dev->name = dev_name(&pdev->dev);
 	indio_dev->modes = INDIO_DIRECT_MODE;