[PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering

[PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering

[Andy Shevchenko]

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

[PATCH v1 2/5] iio: pressure: bmp280: Use IIO_DEVICE_ATTR_RO macro

[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

[PATCH v1 3/5] iio: pressure: bmp280: Explicitly mark GPIO optional

[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

[PATCH v1 4/5] iio: pressure: bmp280: Drop unneeded explicit castings

[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

[PATCH v1 5/5] iio: pressure: bmp280: Join string literals back

[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

Re: [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering

[Jonathan Cameron]

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,

Re: [PATCH v1 2/5] iio: pressure: bmp280: Use IIO_DEVICE_ATTR_RO macro

[Jonathan Cameron]

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 = {

Re: [PATCH v1 4/5] iio: pressure: bmp280: Drop unneeded explicit castings

[Jonathan Cameron]

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;
>  

Re: [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering

[Andy Shevchenko]

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

Re: [PATCH v1 2/5] iio: pressure: bmp280: Use IIO_DEVICE_ATTR_RO macro

[Andy Shevchenko]

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

Re: [PATCH v1 4/5] iio: pressure: bmp280: Drop unneeded explicit castings

[Andy Shevchenko]

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

Re: [PATCH v1 2/5] iio: pressure: bmp280: Use IIO_DEVICE_ATTR_RO macro

[Jonathan Cameron]

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

Re: [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering

[Jonathan Cameron]

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

Re: [PATCH v1 1/5] iio: pressure: bmp280: Tolerate IRQ before registering

[Andy Shevchenko]

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