Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/3] iio: adc: hx711: optimize sampling of data
@ 2019-09-07 10:18 Andreas Klinger
  2019-09-08 13:49 ` Jonathan Cameron
  0 siblings, 1 reply; 3+ messages in thread
From: Andreas Klinger @ 2019-09-07 10:18 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw; +Cc: linux-iio, linux-kernel

Fix bug in sampling function hx711_cycle() when interrupt occures while
PD_SCK is high. If PD_SCK is high for at least 60 us power down mode of
the sensor is entered which in turn leads to a wrong measurement.

Move query of DOUT at the latest point of time which is at the end of
PD_SCK low period.

Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/adc/hx711.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/hx711.c b/drivers/iio/adc/hx711.c
index 88c7fe15003b..0678964dbd21 100644
--- a/drivers/iio/adc/hx711.c
+++ b/drivers/iio/adc/hx711.c
@@ -101,13 +101,14 @@ struct hx711_data {
 static int hx711_cycle(struct hx711_data *hx711_data)
 {
 	int val;
+	unsigned long flags;
 
 	/*
 	 * if preempted for more then 60us while PD_SCK is high:
 	 * hx711 is going in reset
 	 * ==> measuring is false
 	 */
-	preempt_disable();
+	local_irq_save(flags);
 	gpiod_set_value(hx711_data->gpiod_pd_sck, 1);
 
 	/*
@@ -117,7 +118,6 @@ static int hx711_cycle(struct hx711_data *hx711_data)
 	 */
 	ndelay(hx711_data->data_ready_delay_ns);
 
-	val = gpiod_get_value(hx711_data->gpiod_dout);
 	/*
 	 * here we are not waiting for 0.2 us as suggested by the datasheet,
 	 * because the oscilloscope showed in a test scenario
@@ -125,7 +125,7 @@ static int hx711_cycle(struct hx711_data *hx711_data)
 	 * and 0.56 us for PD_SCK low on TI Sitara with 800 MHz
 	 */
 	gpiod_set_value(hx711_data->gpiod_pd_sck, 0);
-	preempt_enable();
+	local_irq_restore(flags);
 
 	/*
 	 * make it a square wave for addressing cases with capacitance on
@@ -133,7 +133,8 @@ static int hx711_cycle(struct hx711_data *hx711_data)
 	 */
 	ndelay(hx711_data->data_ready_delay_ns);
 
-	return val;
+	/* sample as late as possible */
+	return gpiod_get_value(hx711_data->gpiod_dout);
 }
 
 static int hx711_read(struct hx711_data *hx711_data)
-- 
2.13.3

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

* Re: [PATCH 1/3] iio: adc: hx711: optimize sampling of data
  2019-09-07 10:18 [PATCH 1/3] iio: adc: hx711: optimize sampling of data Andreas Klinger
@ 2019-09-08 13:49 ` Jonathan Cameron
  2019-09-09 12:35   ` Andreas Klinger
  0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Cameron @ 2019-09-08 13:49 UTC (permalink / raw)
  To: Andreas Klinger; +Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel

On Sat, 7 Sep 2019 12:18:00 +0200
Andreas Klinger <ak@it-klinger.de> wrote:

> Fix bug in sampling function hx711_cycle() when interrupt occures while
> PD_SCK is high. If PD_SCK is high for at least 60 us power down mode of
> the sensor is entered which in turn leads to a wrong measurement.
> 
> Move query of DOUT at the latest point of time which is at the end of
> PD_SCK low period.
> 
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>

Hi Andreas,

One thing I'm not clear on from these is how much a 'fix' they
are.  That just effects whether we mark them for stable / push them
out as quickly as possible or not.  So has this been seen in 
normal operation?

+ please add fixes tags to the two fixes.

For patch 3, it's in the very low importance category so it may
well get forgotten if these two go through the fixes tree
(up to you to remind me!)

Thanks,

Jonathan

> ---
>  drivers/iio/adc/hx711.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/hx711.c b/drivers/iio/adc/hx711.c
> index 88c7fe15003b..0678964dbd21 100644
> --- a/drivers/iio/adc/hx711.c
> +++ b/drivers/iio/adc/hx711.c
> @@ -101,13 +101,14 @@ struct hx711_data {
>  static int hx711_cycle(struct hx711_data *hx711_data)
>  {
>  	int val;
> +	unsigned long flags;
>  
>  	/*
>  	 * if preempted for more then 60us while PD_SCK is high:
>  	 * hx711 is going in reset
>  	 * ==> measuring is false
>  	 */
> -	preempt_disable();
> +	local_irq_save(flags);
>  	gpiod_set_value(hx711_data->gpiod_pd_sck, 1);
>  
>  	/*
> @@ -117,7 +118,6 @@ static int hx711_cycle(struct hx711_data *hx711_data)
>  	 */
>  	ndelay(hx711_data->data_ready_delay_ns);
>  
> -	val = gpiod_get_value(hx711_data->gpiod_dout);
>  	/*
>  	 * here we are not waiting for 0.2 us as suggested by the datasheet,
>  	 * because the oscilloscope showed in a test scenario
> @@ -125,7 +125,7 @@ static int hx711_cycle(struct hx711_data *hx711_data)
>  	 * and 0.56 us for PD_SCK low on TI Sitara with 800 MHz
>  	 */
>  	gpiod_set_value(hx711_data->gpiod_pd_sck, 0);
> -	preempt_enable();
> +	local_irq_restore(flags);
>  
>  	/*
>  	 * make it a square wave for addressing cases with capacitance on
> @@ -133,7 +133,8 @@ static int hx711_cycle(struct hx711_data *hx711_data)
>  	 */
>  	ndelay(hx711_data->data_ready_delay_ns);
>  
> -	return val;
> +	/* sample as late as possible */
> +	return gpiod_get_value(hx711_data->gpiod_dout);
>  }
>  
>  static int hx711_read(struct hx711_data *hx711_data)


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

* Re: [PATCH 1/3] iio: adc: hx711: optimize sampling of data
  2019-09-08 13:49 ` Jonathan Cameron
@ 2019-09-09 12:35   ` Andreas Klinger
  0 siblings, 0 replies; 3+ messages in thread
From: Andreas Klinger @ 2019-09-09 12:35 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel

Hi Jonathan,

only patch 1 fixes a bug which is occurring on systems. In my test it happened
every one of about 10 - 15.000 measurements. But this depends on the interrupt
load of the system. With a high interrupt load there might be much more wrong
measurements.

Patch 2 is a performance optimization which prevents unneded reads. There is no
bug which is fixed by it.

I'll send out the patch set with more detailed commit messages.

Thanks,

Andreas


Jonathan Cameron <jic23@kernel.org> schrieb am So, 08. Sep 14:49:
> On Sat, 7 Sep 2019 12:18:00 +0200
> Andreas Klinger <ak@it-klinger.de> wrote:
> 
> > Fix bug in sampling function hx711_cycle() when interrupt occures while
> > PD_SCK is high. If PD_SCK is high for at least 60 us power down mode of
> > the sensor is entered which in turn leads to a wrong measurement.
> > 
> > Move query of DOUT at the latest point of time which is at the end of
> > PD_SCK low period.
> > 
> > Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> 
> Hi Andreas,
> 
> One thing I'm not clear on from these is how much a 'fix' they
> are.  That just effects whether we mark them for stable / push them
> out as quickly as possible or not.  So has this been seen in 
> normal operation?
> 
> + please add fixes tags to the two fixes.
> 
> For patch 3, it's in the very low importance category so it may
> well get forgotten if these two go through the fixes tree
> (up to you to remind me!)
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/hx711.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/hx711.c b/drivers/iio/adc/hx711.c
> > index 88c7fe15003b..0678964dbd21 100644
> > --- a/drivers/iio/adc/hx711.c
> > +++ b/drivers/iio/adc/hx711.c
> > @@ -101,13 +101,14 @@ struct hx711_data {
> >  static int hx711_cycle(struct hx711_data *hx711_data)
> >  {
> >  	int val;
> > +	unsigned long flags;
> >  
> >  	/*
> >  	 * if preempted for more then 60us while PD_SCK is high:
> >  	 * hx711 is going in reset
> >  	 * ==> measuring is false
> >  	 */
> > -	preempt_disable();
> > +	local_irq_save(flags);
> >  	gpiod_set_value(hx711_data->gpiod_pd_sck, 1);
> >  
> >  	/*
> > @@ -117,7 +118,6 @@ static int hx711_cycle(struct hx711_data *hx711_data)
> >  	 */
> >  	ndelay(hx711_data->data_ready_delay_ns);
> >  
> > -	val = gpiod_get_value(hx711_data->gpiod_dout);
> >  	/*
> >  	 * here we are not waiting for 0.2 us as suggested by the datasheet,
> >  	 * because the oscilloscope showed in a test scenario
> > @@ -125,7 +125,7 @@ static int hx711_cycle(struct hx711_data *hx711_data)
> >  	 * and 0.56 us for PD_SCK low on TI Sitara with 800 MHz
> >  	 */
> >  	gpiod_set_value(hx711_data->gpiod_pd_sck, 0);
> > -	preempt_enable();
> > +	local_irq_restore(flags);
> >  
> >  	/*
> >  	 * make it a square wave for addressing cases with capacitance on
> > @@ -133,7 +133,8 @@ static int hx711_cycle(struct hx711_data *hx711_data)
> >  	 */
> >  	ndelay(hx711_data->data_ready_delay_ns);
> >  
> > -	return val;
> > +	/* sample as late as possible */
> > +	return gpiod_get_value(hx711_data->gpiod_dout);
> >  }
> >  
> >  static int hx711_read(struct hx711_data *hx711_data)
> 

-- 
Andreas Klinger
Grabenreith 27
84508 Burgkirchen
+49 8623 919966
ak@it-klinger.de
www.it-klinger.de
www.grabenreith.de

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

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-07 10:18 [PATCH 1/3] iio: adc: hx711: optimize sampling of data Andreas Klinger
2019-09-08 13:49 ` Jonathan Cameron
2019-09-09 12:35   ` Andreas Klinger

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org linux-iio@archiver.kernel.org
	public-inbox-index linux-iio


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox