Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering
@ 2020-03-17 10:18 Andy Shevchenko
  2020-03-17 10:18 ` [PATCH v1 2/5] iio: pressure: bmp280: Use IIO_DEVICE_ATTR_RO macro Andy Shevchenko
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Andy Shevchenko @ 2020-03-17 10:18 UTC (permalink / raw)
  To: Jonathan Cameron, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler
  Cc: Andy Shevchenko, Linus Walleij

With DEBUG_SHIRQ enabled we have a kernel crash

[  116.482696] BUG: kernel NULL pointer dereference, address: 0000000000000000

...

[  116.606571] Call Trace:
[  116.609023]  <IRQ>
[  116.611047]  complete+0x34/0x50
[  116.614206]  bmp085_eoc_irq+0x9/0x10 [bmp280]

because DEBUG_SHIRQ mechanism fires an IRQ before registration and drivers
ought to be able to handle an interrupt happening before request_irq() returns.

Fixes: aae953949651 ("iio: pressure: bmp280: add support for BMP085 EOC interrupt")
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/pressure/bmp280-core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 29c209cc1108..5e229b95308e 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -712,8 +712,7 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
 	unsigned int delay_us;
 	unsigned int ctrl;
 
-	if (data->use_eoc)
-		init_completion(&data->done);
+	reinit_completion(&data->done);
 
 	ret = regmap_write(data->regmap, BMP280_REG_CTRL_MEAS, ctrl_meas);
 	if (ret)
@@ -969,6 +968,9 @@ static int bmp085_fetch_eoc_irq(struct device *dev,
 			"trying to enforce it\n");
 		irq_trig = IRQF_TRIGGER_RISING;
 	}
+
+	init_completion(&data->done);
+
 	ret = devm_request_threaded_irq(dev,
 			irq,
 			bmp085_eoc_irq,
-- 
2.25.1


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

* [PATCH v1 2/5] iio: pressure: bmp280: Use IIO_DEVICE_ATTR_RO macro
  2020-03-17 10:18 [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering Andy Shevchenko
@ 2020-03-17 10:18 ` Andy Shevchenko
  2020-03-22 17:16   ` Jonathan Cameron
  2020-03-17 10:18 ` [PATCH v1 3/5] iio: pressure: bmp280: Explicitly mark GPIO optional Andy Shevchenko
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2020-03-17 10:18 UTC (permalink / raw)
  To: Jonathan Cameron, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler
  Cc: Andy Shevchenko

Use the IIO_DEVICE_ATTR_RO macro to create the device attributes.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/pressure/bmp280-core.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 5e229b95308e..25be3ff1a6ab 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -588,7 +588,7 @@ static ssize_t bmp280_show_avail(char *buf, const int *vals, const int n)
 	return len;
 }
 
-static ssize_t bmp280_show_temp_oversampling_avail(struct device *dev,
+static ssize_t in_temp_oversampling_ratio_available_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	struct bmp280_data *data = iio_priv(dev_to_iio_dev(dev));
@@ -596,8 +596,9 @@ static ssize_t bmp280_show_temp_oversampling_avail(struct device *dev,
 	return bmp280_show_avail(buf, data->chip_info->oversampling_temp_avail,
 				 data->chip_info->num_oversampling_temp_avail);
 }
+static IIO_DEVICE_ATTR_RO(in_temp_oversampling_ratio_available, 0);
 
-static ssize_t bmp280_show_press_oversampling_avail(struct device *dev,
+static ssize_t in_pressure_oversampling_ratio_available_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	struct bmp280_data *data = iio_priv(dev_to_iio_dev(dev));
@@ -605,17 +606,12 @@ static ssize_t bmp280_show_press_oversampling_avail(struct device *dev,
 	return bmp280_show_avail(buf, data->chip_info->oversampling_press_avail,
 				 data->chip_info->num_oversampling_press_avail);
 }
-
-static IIO_DEVICE_ATTR(in_temp_oversampling_ratio_available,
-	S_IRUGO, bmp280_show_temp_oversampling_avail, NULL, 0);
-
-static IIO_DEVICE_ATTR(in_pressure_oversampling_ratio_available,
-	S_IRUGO, bmp280_show_press_oversampling_avail, NULL, 0);
+static IIO_DEVICE_ATTR_RO(in_pressure_oversampling_ratio_available, 0);
 
 static struct attribute *bmp280_attributes[] = {
 	&iio_dev_attr_in_temp_oversampling_ratio_available.dev_attr.attr,
 	&iio_dev_attr_in_pressure_oversampling_ratio_available.dev_attr.attr,
-	NULL,
+	NULL
 };
 
 static const struct attribute_group bmp280_attrs_group = {
-- 
2.25.1


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

* [PATCH v1 3/5] iio: pressure: bmp280: Explicitly mark GPIO optional
  2020-03-17 10:18 [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering Andy Shevchenko
  2020-03-17 10:18 ` [PATCH v1 2/5] iio: pressure: bmp280: Use IIO_DEVICE_ATTR_RO macro Andy Shevchenko
@ 2020-03-17 10:18 ` Andy Shevchenko
  2020-03-17 10:18 ` [PATCH v1 4/5] iio: pressure: bmp280: Drop unneeded explicit castings Andy Shevchenko
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2020-03-17 10:18 UTC (permalink / raw)
  To: Jonathan Cameron, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler
  Cc: Andy Shevchenko

Show by using a corresponding API call that GPIO is optional.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/pressure/bmp280-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 25be3ff1a6ab..6c2e47d92a11 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -1080,9 +1080,9 @@ int bmp280_common_probe(struct device *dev,
 	usleep_range(data->start_up_time, data->start_up_time + 100);
 
 	/* Bring chip out of reset if there is an assigned GPIO line */
-	gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
 	/* Deassert the signal */
-	if (!IS_ERR(gpiod)) {
+	if (gpiod) {
 		dev_info(dev, "release reset\n");
 		gpiod_set_value(gpiod, 0);
 	}
-- 
2.25.1


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

* [PATCH v1 4/5] iio: pressure: bmp280: Drop unneeded explicit castings
  2020-03-17 10:18 [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering Andy Shevchenko
  2020-03-17 10:18 ` [PATCH v1 2/5] iio: pressure: bmp280: Use IIO_DEVICE_ATTR_RO macro Andy Shevchenko
  2020-03-17 10:18 ` [PATCH v1 3/5] iio: pressure: bmp280: Explicitly mark GPIO optional Andy Shevchenko
@ 2020-03-17 10:18 ` Andy Shevchenko
  2020-03-22 17:21   ` Jonathan Cameron
  2020-03-17 10:18 ` [PATCH v1 5/5] iio: pressure: bmp280: Join string literals back Andy Shevchenko
  2020-03-22 17:12 ` [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering Jonathan Cameron
  4 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2020-03-17 10:18 UTC (permalink / raw)
  To: Jonathan Cameron, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler
  Cc: Andy Shevchenko

In few places the unnecessary explicit castings are being used.
Drop them for good.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/pressure/bmp280-core.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 6c2e47d92a11..5db41b1df7eb 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -337,8 +337,7 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	__be32 tmp = 0;
 	s32 adc_temp, comp_temp;
 
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
-			       (u8 *) &tmp, 3);
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, &tmp, 3);
 	if (ret < 0) {
 		dev_err(data->dev, "failed to read temperature\n");
 		return ret;
@@ -377,8 +376,7 @@ static int bmp280_read_press(struct bmp280_data *data,
 	if (ret < 0)
 		return ret;
 
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
-			       (u8 *) &tmp, 3);
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, &tmp, 3);
 	if (ret < 0) {
 		dev_err(data->dev, "failed to read pressure\n");
 		return ret;
@@ -400,8 +398,8 @@ static int bmp280_read_press(struct bmp280_data *data,
 
 static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
 {
+	__be16 tmp;
 	int ret;
-	__be16 tmp = 0;
 	s32 adc_humidity;
 	u32 comp_humidity;
 
@@ -410,8 +408,7 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
 	if (ret < 0)
 		return ret;
 
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
-			       (u8 *) &tmp, 2);
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB, &tmp, 2);
 	if (ret < 0) {
 		dev_err(data->dev, "failed to read humidity\n");
 		return ret;
@@ -747,14 +744,14 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
 
 static int bmp180_read_adc_temp(struct bmp280_data *data, int *val)
 {
+	__be16 tmp;
 	int ret;
-	__be16 tmp = 0;
 
 	ret = bmp180_measure(data, BMP180_MEAS_TEMP);
 	if (ret)
 		return ret;
 
-	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, (u8 *)&tmp, 2);
+	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, &tmp, 2);
 	if (ret)
 		return ret;
 
@@ -851,7 +848,7 @@ static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
 	if (ret)
 		return ret;
 
-	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, (u8 *)&tmp, 3);
+	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, &tmp, 3);
 	if (ret)
 		return ret;
 
-- 
2.25.1


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

* [PATCH v1 5/5] iio: pressure: bmp280: Join string literals back
  2020-03-17 10:18 [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering Andy Shevchenko
                   ` (2 preceding siblings ...)
  2020-03-17 10:18 ` [PATCH v1 4/5] iio: pressure: bmp280: Drop unneeded explicit castings Andy Shevchenko
@ 2020-03-17 10:18 ` Andy Shevchenko
  2020-03-22 17:12 ` [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering Jonathan Cameron
  4 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2020-03-17 10:18 UTC (permalink / raw)
  To: Jonathan Cameron, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler
  Cc: Andy Shevchenko

For easy grepping on debug purposes join string literals back in
the messages.

No functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/pressure/bmp280-core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 5db41b1df7eb..b8ff6806c00b 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -957,8 +957,7 @@ static int bmp085_fetch_eoc_irq(struct device *dev,
 
 	irq_trig = irqd_get_trigger_type(irq_get_irq_data(irq));
 	if (irq_trig != IRQF_TRIGGER_RISING) {
-		dev_err(dev, "non-rising trigger given for EOC interrupt, "
-			"trying to enforce it\n");
+		dev_err(dev, "non-rising trigger given for EOC interrupt, trying to enforce it\n");
 		irq_trig = IRQF_TRIGGER_RISING;
 	}
 
-- 
2.25.1


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

* Re: [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering
  2020-03-17 10:18 [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering Andy Shevchenko
                   ` (3 preceding siblings ...)
  2020-03-17 10:18 ` [PATCH v1 5/5] iio: pressure: bmp280: Join string literals back Andy Shevchenko
@ 2020-03-22 17:12 ` Jonathan Cameron
  2020-03-22 21:15   ` Andy Shevchenko
  4 siblings, 1 reply; 14+ messages in thread
From: Jonathan Cameron @ 2020-03-22 17:12 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Linus Walleij


On Tue, 17 Mar 2020 12:18:09 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> With DEBUG_SHIRQ enabled we have a kernel crash
> 
> [  116.482696] BUG: kernel NULL pointer dereference, address: 0000000000000000
> 
> ...
> 
> [  116.606571] Call Trace:
> [  116.609023]  <IRQ>
> [  116.611047]  complete+0x34/0x50
> [  116.614206]  bmp085_eoc_irq+0x9/0x10 [bmp280]
> 
> because DEBUG_SHIRQ mechanism fires an IRQ before registration and drivers
> ought to be able to handle an interrupt happening before request_irq() returns.
> 
> Fixes: aae953949651 ("iio: pressure: bmp280: add support for BMP085 EOC interrupt")
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Hmm. Nasty corner case but fair enough to fix it up.
I guess we should audit other drivers for similar paths...

> ---
>  drivers/iio/pressure/bmp280-core.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 29c209cc1108..5e229b95308e 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -712,8 +712,7 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
>  	unsigned int delay_us;
>  	unsigned int ctrl;
>  
> -	if (data->use_eoc)
> -		init_completion(&data->done);
> +	reinit_completion(&data->done);

reinit on the completion when we don't even have an interrupt
(hence data->use_eoc == false) seems excessive.  Why did
you drop the conditional?

Jonathan



>  
>  	ret = regmap_write(data->regmap, BMP280_REG_CTRL_MEAS, ctrl_meas);
>  	if (ret)
> @@ -969,6 +968,9 @@ static int bmp085_fetch_eoc_irq(struct device *dev,
>  			"trying to enforce it\n");
>  		irq_trig = IRQF_TRIGGER_RISING;
>  	}
> +
> +	init_completion(&data->done);
> +
>  	ret = devm_request_threaded_irq(dev,
>  			irq,
>  			bmp085_eoc_irq,


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

* Re: [PATCH v1 2/5] iio: pressure: bmp280: Use IIO_DEVICE_ATTR_RO macro
  2020-03-17 10:18 ` [PATCH v1 2/5] iio: pressure: bmp280: Use IIO_DEVICE_ATTR_RO macro Andy Shevchenko
@ 2020-03-22 17:16   ` Jonathan Cameron
  2020-03-22 21:17     ` Andy Shevchenko
  0 siblings, 1 reply; 14+ messages in thread
From: Jonathan Cameron @ 2020-03-22 17:16 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler

On Tue, 17 Mar 2020 12:18:10 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Use the IIO_DEVICE_ATTR_RO macro to create the device attributes.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Hmm. Minor gain but fair enough I guess.

Could consider using the core support _available attributes instead.

I'm not usually fussy about moving drivers over to that unless we have
an in kernel consumer, but if you are going to touch the code it
might be nice to move to them :)

Jonathan

> ---
>  drivers/iio/pressure/bmp280-core.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 5e229b95308e..25be3ff1a6ab 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -588,7 +588,7 @@ static ssize_t bmp280_show_avail(char *buf, const int *vals, const int n)
>  	return len;
>  }
>  
> -static ssize_t bmp280_show_temp_oversampling_avail(struct device *dev,
> +static ssize_t in_temp_oversampling_ratio_available_show(struct device *dev,
>  				struct device_attribute *attr, char *buf)
>  {
>  	struct bmp280_data *data = iio_priv(dev_to_iio_dev(dev));
> @@ -596,8 +596,9 @@ static ssize_t bmp280_show_temp_oversampling_avail(struct device *dev,
>  	return bmp280_show_avail(buf, data->chip_info->oversampling_temp_avail,
>  				 data->chip_info->num_oversampling_temp_avail);
>  }
> +static IIO_DEVICE_ATTR_RO(in_temp_oversampling_ratio_available, 0);
>  
> -static ssize_t bmp280_show_press_oversampling_avail(struct device *dev,
> +static ssize_t in_pressure_oversampling_ratio_available_show(struct device *dev,
>  				struct device_attribute *attr, char *buf)
>  {
>  	struct bmp280_data *data = iio_priv(dev_to_iio_dev(dev));
> @@ -605,17 +606,12 @@ static ssize_t bmp280_show_press_oversampling_avail(struct device *dev,
>  	return bmp280_show_avail(buf, data->chip_info->oversampling_press_avail,
>  				 data->chip_info->num_oversampling_press_avail);
>  }
> -
> -static IIO_DEVICE_ATTR(in_temp_oversampling_ratio_available,
> -	S_IRUGO, bmp280_show_temp_oversampling_avail, NULL, 0);
> -
> -static IIO_DEVICE_ATTR(in_pressure_oversampling_ratio_available,
> -	S_IRUGO, bmp280_show_press_oversampling_avail, NULL, 0);
> +static IIO_DEVICE_ATTR_RO(in_pressure_oversampling_ratio_available, 0);
>  
>  static struct attribute *bmp280_attributes[] = {
>  	&iio_dev_attr_in_temp_oversampling_ratio_available.dev_attr.attr,
>  	&iio_dev_attr_in_pressure_oversampling_ratio_available.dev_attr.attr,
> -	NULL,
> +	NULL
>  };
>  
>  static const struct attribute_group bmp280_attrs_group = {


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

* Re: [PATCH v1 4/5] iio: pressure: bmp280: Drop unneeded explicit castings
  2020-03-17 10:18 ` [PATCH v1 4/5] iio: pressure: bmp280: Drop unneeded explicit castings Andy Shevchenko
@ 2020-03-22 17:21   ` Jonathan Cameron
  2020-03-22 21:20     ` Andy Shevchenko
  0 siblings, 1 reply; 14+ messages in thread
From: Jonathan Cameron @ 2020-03-22 17:21 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler

On Tue, 17 Mar 2020 12:18:12 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> In few places the unnecessary explicit castings are being used.
> Drop them for good.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

A passing comment inline, but seems sensible to me.

Jonathan

> ---
>  drivers/iio/pressure/bmp280-core.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 6c2e47d92a11..5db41b1df7eb 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -337,8 +337,7 @@ static int bmp280_read_temp(struct bmp280_data *data,
>  	__be32 tmp = 0;
>  	s32 adc_temp, comp_temp;
>  
> -	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
> -			       (u8 *) &tmp, 3);
> +	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, &tmp, 3);
>  	if (ret < 0) {
>  		dev_err(data->dev, "failed to read temperature\n");
>  		return ret;
> @@ -377,8 +376,7 @@ static int bmp280_read_press(struct bmp280_data *data,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
> -			       (u8 *) &tmp, 3);
> +	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, &tmp, 3);
>  	if (ret < 0) {
>  		dev_err(data->dev, "failed to read pressure\n");
>  		return ret;
> @@ -400,8 +398,8 @@ static int bmp280_read_press(struct bmp280_data *data,
>  
>  static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
>  {
> +	__be16 tmp;
>  	int ret;
> -	__be16 tmp = 0;

I haven't checked it but normally that sort of initialization got added
because a compiler got 'clever' and decided that it might be used uninitialized.

However, it's a bit odd in this case as there are lots of other calls
of the same thing that don't bother initializing.  So must not be that...

>  	s32 adc_humidity;
>  	u32 comp_humidity;
>  
> @@ -410,8 +408,7 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
> -			       (u8 *) &tmp, 2);
> +	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB, &tmp, 2);
>  	if (ret < 0) {
>  		dev_err(data->dev, "failed to read humidity\n");
>  		return ret;
> @@ -747,14 +744,14 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
>  
>  static int bmp180_read_adc_temp(struct bmp280_data *data, int *val)
>  {
> +	__be16 tmp;
>  	int ret;
> -	__be16 tmp = 0;
>  
>  	ret = bmp180_measure(data, BMP180_MEAS_TEMP);
>  	if (ret)
>  		return ret;
>  
> -	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, (u8 *)&tmp, 2);
> +	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, &tmp, 2);
>  	if (ret)
>  		return ret;
>  
> @@ -851,7 +848,7 @@ static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
>  	if (ret)
>  		return ret;
>  
> -	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, (u8 *)&tmp, 3);
> +	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, &tmp, 3);
>  	if (ret)
>  		return ret;
>  


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

* Re: [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering
  2020-03-22 17:12 ` [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering Jonathan Cameron
@ 2020-03-22 21:15   ` Andy Shevchenko
  2020-03-23  9:40     ` Jonathan Cameron
  0 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2020-03-22 21:15 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Andy Shevchenko, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Linus Walleij

On Sun, Mar 22, 2020 at 7:14 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Tue, 17 Mar 2020 12:18:09 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > With DEBUG_SHIRQ enabled we have a kernel crash
> >
> > [  116.482696] BUG: kernel NULL pointer dereference, address: 0000000000000000
> >
> > ...
> >
> > [  116.606571] Call Trace:
> > [  116.609023]  <IRQ>
> > [  116.611047]  complete+0x34/0x50
> > [  116.614206]  bmp085_eoc_irq+0x9/0x10 [bmp280]
> >
> > because DEBUG_SHIRQ mechanism fires an IRQ before registration and drivers
> > ought to be able to handle an interrupt happening before request_irq() returns.
> >
> > Fixes: aae953949651 ("iio: pressure: bmp280: add support for BMP085 EOC interrupt")
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Hmm. Nasty corner case but fair enough to fix it up.
> I guess we should audit other drivers for similar paths...

One can easily test, no device even needed (however in this case I have one)

...

> > -     if (data->use_eoc)
> > -             init_completion(&data->done);
> > +     reinit_completion(&data->done);
>
> reinit on the completion when we don't even have an interrupt
> (hence data->use_eoc == false) seems excessive.  Why did
> you drop the conditional?

It's harmless and gives less code I believe.
But if you insist I can put it back.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v1 2/5] iio: pressure: bmp280: Use IIO_DEVICE_ATTR_RO macro
  2020-03-22 17:16   ` Jonathan Cameron
@ 2020-03-22 21:17     ` Andy Shevchenko
  2020-03-23  9:38       ` Jonathan Cameron
  0 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2020-03-22 21:17 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Andy Shevchenko, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler

On Sun, Mar 22, 2020 at 7:17 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 17 Mar 2020 12:18:10 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > Use the IIO_DEVICE_ATTR_RO macro to create the device attributes.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Hmm. Minor gain but fair enough I guess.
>
> Could consider using the core support _available attributes instead.
>
> I'm not usually fussy about moving drivers over to that unless we have
> an in kernel consumer, but if you are going to touch the code it
> might be nice to move to them :)

Any good driver as an example? Or even conversion change done for one?

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v1 4/5] iio: pressure: bmp280: Drop unneeded explicit castings
  2020-03-22 17:21   ` Jonathan Cameron
@ 2020-03-22 21:20     ` Andy Shevchenko
  0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2020-03-22 21:20 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Andy Shevchenko, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler

On Sun, Mar 22, 2020 at 7:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Tue, 17 Mar 2020 12:18:12 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > In few places the unnecessary explicit castings are being used.
> > Drop them for good.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> A passing comment inline, but seems sensible to me.

...

> > +     ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, &tmp, 3);

(left for a context)

...

> > +     __be16 tmp;
> >       int ret;
> > -     __be16 tmp = 0;
>
> I haven't checked it but normally that sort of initialization got added
> because a compiler got 'clever' and decided that it might be used uninitialized.
>
> However, it's a bit odd in this case as there are lots of other calls
> of the same thing that don't bother initializing.  So must not be that...

When it's 3 bytes read (24 bit) it's required to get correct sign from
the value or in general to avoid garbage in one byte.
But here are two bytes to read to the 16 bit variable. No
initialization required.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v1 2/5] iio: pressure: bmp280: Use IIO_DEVICE_ATTR_RO macro
  2020-03-22 21:17     ` Andy Shevchenko
@ 2020-03-23  9:38       ` Jonathan Cameron
  0 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2020-03-23  9:38 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Andy Shevchenko, linux-iio, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler

On Sun, 22 Mar 2020 23:17:07 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Mar 22, 2020 at 7:17 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 17 Mar 2020 12:18:10 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >  
> > > Use the IIO_DEVICE_ATTR_RO macro to create the device attributes.
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>  
> >
> > Hmm. Minor gain but fair enough I guess.
> >
> > Could consider using the core support _available attributes instead.
> >
> > I'm not usually fussy about moving drivers over to that unless we have
> > an in kernel consumer, but if you are going to touch the code it
> > might be nice to move to them :)  
> 
> Any good driver as an example? Or even conversion change done for one?
> 


Grep read_avail and you will find a few.  drivers/iio/light/tsl2772.c looks
superficially similar to what would be needed here.

Jonathan


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

* Re: [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering
  2020-03-22 21:15   ` Andy Shevchenko
@ 2020-03-23  9:40     ` Jonathan Cameron
  2020-03-23 10:08       ` Andy Shevchenko
  0 siblings, 1 reply; 14+ messages in thread
From: Jonathan Cameron @ 2020-03-23  9:40 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Andy Shevchenko, linux-iio, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Linus Walleij

On Sun, 22 Mar 2020 23:15:13 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Mar 22, 2020 at 7:14 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Tue, 17 Mar 2020 12:18:09 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >  
> > > With DEBUG_SHIRQ enabled we have a kernel crash
> > >
> > > [  116.482696] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > >
> > > ...
> > >
> > > [  116.606571] Call Trace:
> > > [  116.609023]  <IRQ>
> > > [  116.611047]  complete+0x34/0x50
> > > [  116.614206]  bmp085_eoc_irq+0x9/0x10 [bmp280]
> > >
> > > because DEBUG_SHIRQ mechanism fires an IRQ before registration and drivers
> > > ought to be able to handle an interrupt happening before request_irq() returns.
> > >
> > > Fixes: aae953949651 ("iio: pressure: bmp280: add support for BMP085 EOC interrupt")
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>  
> >
> > Hmm. Nasty corner case but fair enough to fix it up.
> > I guess we should audit other drivers for similar paths...  
> 
> One can easily test, no device even needed (however in this case I have one)
> 
> ...
> 
> > > -     if (data->use_eoc)
> > > -             init_completion(&data->done);
> > > +     reinit_completion(&data->done);  
> >
> > reinit on the completion when we don't even have an interrupt
> > (hence data->use_eoc == false) seems excessive.  Why did
> > you drop the conditional?  
> 
> It's harmless and gives less code I believe.
> But if you insist I can put it back.
> 

I agree it's harmless but breaks the logical flow by doing
something unnecessary so either we need a comment to say that
or easier option, just put the condition back in.

Jonathan



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

* Re: [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering
  2020-03-23  9:40     ` Jonathan Cameron
@ 2020-03-23 10:08       ` Andy Shevchenko
  0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2020-03-23 10:08 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Jonathan Cameron, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Linus Walleij

On Mon, Mar 23, 2020 at 09:40:18AM +0000, Jonathan Cameron wrote:
> On Sun, 22 Mar 2020 23:15:13 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Sun, Mar 22, 2020 at 7:14 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > On Tue, 17 Mar 2020 12:18:09 +0200
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> > > > -     if (data->use_eoc)
> > > > -             init_completion(&data->done);
> > > > +     reinit_completion(&data->done);  
> > >
> > > reinit on the completion when we don't even have an interrupt
> > > (hence data->use_eoc == false) seems excessive.  Why did
> > > you drop the conditional?  
> > 
> > It's harmless and gives less code I believe.
> > But if you insist I can put it back.
> > 
> 
> I agree it's harmless but breaks the logical flow by doing
> something unnecessary so either we need a comment to say that
> or easier option, just put the condition back in.

I will do this for v2.
Thank you for review!

-- 
With Best Regards,
Andy Shevchenko



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

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-17 10:18 [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering Andy Shevchenko
2020-03-17 10:18 ` [PATCH v1 2/5] iio: pressure: bmp280: Use IIO_DEVICE_ATTR_RO macro Andy Shevchenko
2020-03-22 17:16   ` Jonathan Cameron
2020-03-22 21:17     ` Andy Shevchenko
2020-03-23  9:38       ` Jonathan Cameron
2020-03-17 10:18 ` [PATCH v1 3/5] iio: pressure: bmp280: Explicitly mark GPIO optional Andy Shevchenko
2020-03-17 10:18 ` [PATCH v1 4/5] iio: pressure: bmp280: Drop unneeded explicit castings Andy Shevchenko
2020-03-22 17:21   ` Jonathan Cameron
2020-03-22 21:20     ` Andy Shevchenko
2020-03-17 10:18 ` [PATCH v1 5/5] iio: pressure: bmp280: Join string literals back Andy Shevchenko
2020-03-22 17:12 ` [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering Jonathan Cameron
2020-03-22 21:15   ` Andy Shevchenko
2020-03-23  9:40     ` Jonathan Cameron
2020-03-23 10:08       ` Andy Shevchenko

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
	public-inbox-index linux-iio

Example config snippet for mirrors

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.git