[v1] iio: bmp280: fix eoc interrupt usage

[PATCH] iio: bmp280: fix eoc interrupt usage

Posted by Andreas Klinger 2 years, 2 months ago

Only the bmp085 can have an End-Of-Conversion (EOC) interrupt. But the
bmp085 and bmp180 share the same chip id. Therefore it's necessary to
distinguish the case in which the interrupt is set.

Fix the if statement so that only when the interrupt is set and the chip
id is recognized the interrupt is requested.

This bug exists since the support of EOC interrupt was introduced.
Fixes: aae953949651 ("iio: pressure: bmp280: add support for BMP085 EOC interrupt")

Also add a link to bmp085 datasheet for reference.

Suggested-by: Sergei Korolev <dssoftsk@gmail.com>
Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/pressure/bmp280-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 6089f3f9d8f4..9b7beeb1c088 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -9,6 +9,7 @@
  * Driver for Bosch Sensortec BMP180 and BMP280 digital pressure sensor.
  *
  * Datasheet:
+ * https://www.sparkfun.com/datasheets/Components/General/BST-BMP085-DS000-05.pdf
  * https://cdn-shop.adafruit.com/datasheets/BST-BMP180-DS000-09.pdf
  * https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp280-ds001.pdf
  * https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bme280-ds002.pdf
@@ -2179,7 +2180,7 @@ int bmp280_common_probe(struct device *dev,
 	 * however as it happens, the BMP085 shares the chip ID of BMP180
 	 * so we look for an IRQ if we have that.
 	 */
-	if (irq > 0 || (chip_id  == BMP180_CHIP_ID)) {
+	if (irq > 0 && (chip_id  == BMP180_CHIP_ID)) {
 		ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
 		if (ret)
 			return ret;
-- 
2.39.2

Re: [PATCH] iio: bmp280: fix eoc interrupt usage

Posted by Jonathan Cameron 2 years, 2 months ago

On Wed, 18 Oct 2023 17:28:16 +0200
Andreas Klinger <ak@it-klinger.de> wrote:

> Only the bmp085 can have an End-Of-Conversion (EOC) interrupt. But the
> bmp085 and bmp180 share the same chip id. Therefore it's necessary to
> distinguish the case in which the interrupt is set.
> 
> Fix the if statement so that only when the interrupt is set and the chip
> id is recognized the interrupt is requested.
> 
> This bug exists since the support of EOC interrupt was introduced.
> Fixes: aae953949651 ("iio: pressure: bmp280: add support for BMP085 EOC interrupt")
> 
> Also add a link to bmp085 datasheet for reference.
> 
Fixes tag is part of the tags block so needs to be down here.
> Suggested-by: Sergei Korolev <dssoftsk@gmail.com>
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>

However, it's also already fixed upstream I think.
That's not filtered back around to my togreg branch though as that has an outstanding
pull request.

Jonathan

> ---
>  drivers/iio/pressure/bmp280-core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 6089f3f9d8f4..9b7beeb1c088 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -9,6 +9,7 @@
>   * Driver for Bosch Sensortec BMP180 and BMP280 digital pressure sensor.
>   *
>   * Datasheet:
> + * https://www.sparkfun.com/datasheets/Components/General/BST-BMP085-DS000-05.pdf
>   * https://cdn-shop.adafruit.com/datasheets/BST-BMP180-DS000-09.pdf
>   * https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp280-ds001.pdf
>   * https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bme280-ds002.pdf
> @@ -2179,7 +2180,7 @@ int bmp280_common_probe(struct device *dev,
>  	 * however as it happens, the BMP085 shares the chip ID of BMP180
>  	 * so we look for an IRQ if we have that.
>  	 */
> -	if (irq > 0 || (chip_id  == BMP180_CHIP_ID)) {
> +	if (irq > 0 && (chip_id  == BMP180_CHIP_ID)) {
>  		ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
>  		if (ret)
>  			return ret;

Re: [PATCH] iio: bmp280: fix eoc interrupt usage

Posted by Linus Walleij 2 years, 2 months ago

On Wed, Oct 18, 2023 at 5:29 PM Andreas Klinger <ak@it-klinger.de> wrote:


> Only the bmp085 can have an End-Of-Conversion (EOC) interrupt. But the
> bmp085 and bmp180 share the same chip id. Therefore it's necessary to
> distinguish the case in which the interrupt is set.
>
> Fix the if statement so that only when the interrupt is set and the chip
> id is recognized the interrupt is requested.
>
> This bug exists since the support of EOC interrupt was introduced.
> Fixes: aae953949651 ("iio: pressure: bmp280: add support for BMP085 EOC interrupt")
>
> Also add a link to bmp085 datasheet for reference.
>
> Suggested-by: Sergei Korolev <dssoftsk@gmail.com>
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] iio: bmp280: fix eoc interrupt usage

Posted by Andy Shevchenko 2 years, 2 months ago

On Wed, Oct 18, 2023 at 05:28:16PM +0200, Andreas Klinger wrote:
> Only the bmp085 can have an End-Of-Conversion (EOC) interrupt. But the
> bmp085 and bmp180 share the same chip id. Therefore it's necessary to
> distinguish the case in which the interrupt is set.
> 
> Fix the if statement so that only when the interrupt is set and the chip
> id is recognized the interrupt is requested.
> 
> This bug exists since the support of EOC interrupt was introduced.
> Fixes: aae953949651 ("iio: pressure: bmp280: add support for BMP085 EOC interrupt")
> 
> Also add a link to bmp085 datasheet for reference.

...

> -	if (irq > 0 || (chip_id  == BMP180_CHIP_ID)) {
> +	if (irq > 0 && (chip_id  == BMP180_CHIP_ID)) {

While at it, perhaps drop extra space?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2] iio: bmp280: fix eoc interrupt usage

Posted by Andreas Klinger 2 years, 1 month ago

Only the bmp085 can have an End-Of-Conversion (EOC) interrupt. But the
bmp085 and bmp180 share the same chip id. Therefore it's necessary to
distinguish the case in which the interrupt is set.

Fix the if statement so that only when the interrupt is set and the chip
id is recognized the interrupt is requested.

This bug exists since the support of EOC interrupt was introduced.
Fixes: aae953949651 ("iio: pressure: bmp280: add support for BMP085 EOC interrupt")

Also add a link to bmp085 datasheet for reference.

Suggested-by: Sergei Korolev <dssoftsk@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 v1 -> v2: Remove extra space (seen by Andy)

 drivers/iio/pressure/bmp280-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 6089f3f9d8f4..ef9b3c4f2340 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -9,6 +9,7 @@
  * Driver for Bosch Sensortec BMP180 and BMP280 digital pressure sensor.
  *
  * Datasheet:
+ * https://www.sparkfun.com/datasheets/Components/General/BST-BMP085-DS000-05.pdf
  * https://cdn-shop.adafruit.com/datasheets/BST-BMP180-DS000-09.pdf
  * https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp280-ds001.pdf
  * https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bme280-ds002.pdf
@@ -2179,7 +2180,7 @@ int bmp280_common_probe(struct device *dev,
 	 * however as it happens, the BMP085 shares the chip ID of BMP180
 	 * so we look for an IRQ if we have that.
 	 */
-	if (irq > 0 || (chip_id  == BMP180_CHIP_ID)) {
+	if (irq > 0 && (chip_id == BMP180_CHIP_ID)) {
 		ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
 		if (ret)
 			return ret;
-- 
2.39.2

Re: [PATCH v2] iio: bmp280: fix eoc interrupt usage

Posted by Andy Shevchenko 2 years, 1 month ago

On Thu, Oct 19, 2023 at 06:22:09PM +0200, Andreas Klinger wrote:
> Only the bmp085 can have an End-Of-Conversion (EOC) interrupt. But the
> bmp085 and bmp180 share the same chip id. Therefore it's necessary to
> distinguish the case in which the interrupt is set.
> 
> Fix the if statement so that only when the interrupt is set and the chip
> id is recognized the interrupt is requested.
> 
> This bug exists since the support of EOC interrupt was introduced.

> Fixes: aae953949651 ("iio: pressure: bmp280: add support for BMP085 EOC interrupt")

As Jonathan already commented, this is part of a tag block below...

> Also add a link to bmp085 datasheet for reference.
> 

...somewhere here.

> Suggested-by: Sergei Korolev <dssoftsk@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> ---
>  v1 -> v2: Remove extra space (seen by Andy)


And seems Jonathan mentioned that this is already fixed in his tree.
Did I understand that correctly?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: bmp280: fix eoc interrupt usage

Posted by Andreas Klinger 2 years, 1 month ago

Hi Andy,

Andy Shevchenko <andriy.shevchenko@linux.intel.com> schrieb am Do, 19. Okt 19:54:
> On Thu, Oct 19, 2023 at 06:22:09PM +0200, Andreas Klinger wrote:
> > Only the bmp085 can have an End-Of-Conversion (EOC) interrupt. But the
> > bmp085 and bmp180 share the same chip id. Therefore it's necessary to
> > distinguish the case in which the interrupt is set.
> > 
> > Fix the if statement so that only when the interrupt is set and the chip
> > id is recognized the interrupt is requested.
> > 
> > This bug exists since the support of EOC interrupt was introduced.
> 
> > Fixes: aae953949651 ("iio: pressure: bmp280: add support for BMP085 EOC interrupt")
> 
> As Jonathan already commented, this is part of a tag block below...
> 
> > Also add a link to bmp085 datasheet for reference.
> > 
> 
> ...somewhere here.
> 
> > Suggested-by: Sergei Korolev <dssoftsk@gmail.com>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> > ---
> >  v1 -> v2: Remove extra space (seen by Andy)
> 
> 
> And seems Jonathan mentioned that this is already fixed in his tree.
> Did I understand that correctly?

I just read it in the archive. For some reason I didn't get Jonathans mail
yesterday. Sorry for the spam.

Andreas

Re: [PATCH v2] iio: bmp280: fix eoc interrupt usage

Posted by Jonathan Cameron 2 years, 1 month ago

On Thu, 19 Oct 2023 19:15:22 +0200
Andreas Klinger <ak@it-klinger.de> wrote:

> Hi Andy,
> 
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> schrieb am Do, 19. Okt 19:54:
> > On Thu, Oct 19, 2023 at 06:22:09PM +0200, Andreas Klinger wrote:  
> > > Only the bmp085 can have an End-Of-Conversion (EOC) interrupt. But the
> > > bmp085 and bmp180 share the same chip id. Therefore it's necessary to
> > > distinguish the case in which the interrupt is set.
> > > 
> > > Fix the if statement so that only when the interrupt is set and the chip
> > > id is recognized the interrupt is requested.
> > > 
> > > This bug exists since the support of EOC interrupt was introduced.  
> >   
> > > Fixes: aae953949651 ("iio: pressure: bmp280: add support for BMP085 EOC interrupt")  
> > 
> > As Jonathan already commented, this is part of a tag block below...
> >   
> > > Also add a link to bmp085 datasheet for reference.
> > >   
> > 
> > ...somewhere here.
> >   
> > > Suggested-by: Sergei Korolev <dssoftsk@gmail.com>
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> > > ---
> > >  v1 -> v2: Remove extra space (seen by Andy)  
> > 
> > 
> > And seems Jonathan mentioned that this is already fixed in his tree.
> > Did I understand that correctly?  
> 
> I just read it in the archive. For some reason I didn't get Jonathans mail
> yesterday. Sorry for the spam.

btw, don't reply to an earlier version.  New version is new email thread.
Otherwise things get very tricky to follow once we have lots of versions

Jonathan

> 
> Andreas
>