diff mbox series

[4/5] iio: pressure: bmp280: Allow multiple chips id per family of devices

Message ID 9f8489d82325b2dfb5c8c71c3d558d509b2b01bf.1697381932.git.ang.iglesiasg@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series Add support for BMP390 and various driver cleanups | expand

Commit Message

Angel Iglesias Oct. 15, 2023, 3:16 p.m. UTC 
Improve device detection in certain chip families known to have various
chip ids.

Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>

Comments

Andy Shevchenko Oct. 16, 2023, 7:53 a.m. UTC | #1 
On Sun, Oct 15, 2023 at 05:16:26PM +0200, Angel Iglesias wrote:
> Improve device detection in certain chip families known to have various
> chip ids.

...

> +#include <linux/overflow.h>

Probably you don't need this, see below.

...

>  	ret = regmap_read(regmap, data->chip_info->id_reg, &chip_id);
>  	if (ret < 0)
>  		return ret;

> +	if (i == data->chip_info->num_chip_id) {
> +		size_t nbuf;
> +		char *buf;
> +
> +		// 0x<id>, so four chars per number plus one space + ENDL
> +		if (check_mul_overflow(data->chip_info->num_chip_id, 5, &nbuf))
> +			return ret;

First of all, it _implicitly_ returns 0 here...

> +		buf = kmalloc_array(data->chip_info->num_chip_id, 5, GFP_KERNEL);
> +		if (!buf)
> +			return ret;

...and here.

Second, kmalloc_array() has that check inside.

Third, define this magic 5 either as strlen(), or a constant (in latter case
with the comment of its meaning).

> +		for (i = 0; i < data->chip_info->num_chip_id; i++)
> +			snprintf(&buf[i*5], nbuf - i*5, "0x%x ", data->chip_info->chip_id[i]);

Fourth, use incremental position, i.e. use retuned value from snprintf().

> +		dev_err(dev, "bad chip id: expected one of [ %s ] got 0x%x\n", buf, chip_id);
> +		kfree(buf);

> +		return ret;

As per "first" and "second" above.

>  	}
Jonathan Cameron Oct. 16, 2023, 8:38 a.m. UTC | #2 
On Sun, 15 Oct 2023 17:16:26 +0200
Angel Iglesias <ang.iglesiasg@gmail.com> wrote:

> Improve device detection in certain chip families known to have various
> chip ids.
> 
> Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>

A few minor things inline.

J
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index ea02a623bb58..e3bb4d7906a9 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -38,6 +38,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/irq.h> /* For irq_get_irq_data() */
>  #include <linux/completion.h>
> +#include <linux/overflow.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/random.h>
>  
> @@ -794,10 +795,12 @@ static int bmp280_chip_config(struct bmp280_data *data)
>  }
>  
>  static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
> +static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID };
>  
>  const struct bmp280_chip_info bmp280_chip_info = {
>  	.id_reg = BMP280_REG_ID,
> -	.chip_id = BMP280_CHIP_ID,
> +	.chip_id = bmp280_chip_ids,
> +	.num_chip_id = ARRAY_SIZE(bmp280_chip_ids),
>  	.regmap_config = &bmp280_regmap_config,
>  	.start_up_time = 2000,
>  	.channels = bmp280_channels,
> @@ -846,9 +849,12 @@ static int bme280_chip_config(struct bmp280_data *data)
>  	return bmp280_chip_config(data);
>  }
>  
> +static const u8 bme280_chip_ids[] = { BME280_CHIP_ID };
> +
>  const struct bmp280_chip_info bme280_chip_info = {
>  	.id_reg = BMP280_REG_ID,
> -	.chip_id = BME280_CHIP_ID,
> +	.chip_id = bme280_chip_ids,
> +	.num_chip_id = ARRAY_SIZE(bme280_chip_ids),
>  	.regmap_config = &bmp280_regmap_config,
>  	.start_up_time = 2000,
>  	.channels = bmp280_channels,
> @@ -1220,10 +1226,12 @@ static int bmp380_chip_config(struct bmp280_data *data)
>  
>  static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 };
>  static const int bmp380_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128};
> +static const u8 bmp380_chip_ids[] = { BMP380_CHIP_ID };
>  
>  const struct bmp280_chip_info bmp380_chip_info = {
>  	.id_reg = BMP380_REG_ID,
> -	.chip_id = BMP380_CHIP_ID,
> +	.chip_id = bmp380_chip_ids,
> +	.num_chip_id = ARRAY_SIZE(bmp380_chip_ids),
>  	.regmap_config = &bmp380_regmap_config,
>  	.start_up_time = 2000,
>  	.channels = bmp380_channels,
> @@ -1720,10 +1728,12 @@ static int bmp580_chip_config(struct bmp280_data *data)
>  }
>  
>  static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
> +static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT };
>  
>  const struct bmp280_chip_info bmp580_chip_info = {
>  	.id_reg = BMP580_REG_CHIP_ID,
> -	.chip_id = BMP580_CHIP_ID,
> +	.chip_id = bmp580_chip_ids,
> +	.num_chip_id = ARRAY_SIZE(bmp580_chip_ids),
>  	.regmap_config = &bmp580_regmap_config,
>  	.start_up_time = 2000,
>  	.channels = bmp380_channels,
> @@ -1983,10 +1993,12 @@ static int bmp180_chip_config(struct bmp280_data *data)
>  
>  static const int bmp180_oversampling_temp_avail[] = { 1 };
>  static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 };
> +static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID };
>  
>  const struct bmp280_chip_info bmp180_chip_info = {
>  	.id_reg = BMP280_REG_ID,
> -	.chip_id = BMP180_CHIP_ID,
> +	.chip_id = bmp180_chip_ids,
> +	.num_chip_id = ARRAY_SIZE(bmp180_chip_ids),
>  	.regmap_config = &bmp180_regmap_config,
>  	.start_up_time = 2000,
>  	.channels = bmp280_channels,
> @@ -2077,6 +2089,7 @@ int bmp280_common_probe(struct device *dev,
>  	struct bmp280_data *data;
>  	struct gpio_desc *gpiod;
>  	unsigned int chip_id;
> +	unsigned int i;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> @@ -2142,10 +2155,30 @@ int bmp280_common_probe(struct device *dev,
>  	ret = regmap_read(regmap, data->chip_info->id_reg, &chip_id);
>  	if (ret < 0)
>  		return ret;
> -	if (chip_id != data->chip_info->chip_id) {
> -		dev_err(dev, "bad chip id: expected %x got %x\n",
> -			data->chip_info->chip_id, chip_id);
> -		return -EINVAL;
> +
> +	for (i = 0; i < data->chip_info->num_chip_id; i++) {
> +		if (chip_id == data->chip_info->chip_id[i])
> +			break;
> +	}
> +
> +	if (i == data->chip_info->num_chip_id) {
> +		size_t nbuf;
> +		char *buf;
> +
> +		// 0x<id>, so four chars per number plus one space + ENDL

Trivial but /* */ comment syntax for IIO please.

> +		if (check_mul_overflow(data->chip_info->num_chip_id, 5, &nbuf))
> +			return ret;
> +
> +		buf = kmalloc_array(data->chip_info->num_chip_id, 5, GFP_KERNEL);
> +		if (!buf)
> +			return ret;
> +
> +		for (i = 0; i < data->chip_info->num_chip_id; i++)
> +			snprintf(&buf[i*5], nbuf - i*5, "0x%x ", data->chip_info->chip_id[i]);

I'd uses multiple dev_err() lines and skip this complexity. Doesn't need to be as pretty as this
given it's an error reprot :)  However, we should support fallback compatibles. So this should
be a dev_warn at most and we should go on with whatever firmware said.  Note the original code didn't
do this (I used to get this wrong in reviews) but it would be good to improve it whilst here.

> +
> +		dev_err(dev, "bad chip id: expected one of [ %s ] got 0x%x\n", buf, chip_id);
In probe so dev_err_probe() preferred and with the autocleanup this can be return dev_err_probe()

> +		kfree(buf);

The new autocleanup stuff is nice for this sort of case.

char *buf __free(kfree) = NULL;



> +		return ret;
>  	}
>  
>  	if (data->chip_info->preinit) {
> diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> index 5c0563ce7572..a230fcfc4a85 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -418,7 +418,8 @@ struct bmp280_data {
>  
>  struct bmp280_chip_info {
>  	unsigned int id_reg;
> -	const unsigned int chip_id;
> +	const u8 *chip_id;
> +	int num_chip_id;
>  
>  	const struct regmap_config *regmap_config;
>
diff mbox series

Patch

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index ea02a623bb58..e3bb4d7906a9 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -38,6 +38,7 @@ 
 #include <linux/interrupt.h>
 #include <linux/irq.h> /* For irq_get_irq_data() */
 #include <linux/completion.h>
+#include <linux/overflow.h>
 #include <linux/pm_runtime.h>
 #include <linux/random.h>
 
@@ -794,10 +795,12 @@  static int bmp280_chip_config(struct bmp280_data *data)
 }
 
 static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
+static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID };
 
 const struct bmp280_chip_info bmp280_chip_info = {
 	.id_reg = BMP280_REG_ID,
-	.chip_id = BMP280_CHIP_ID,
+	.chip_id = bmp280_chip_ids,
+	.num_chip_id = ARRAY_SIZE(bmp280_chip_ids),
 	.regmap_config = &bmp280_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp280_channels,
@@ -846,9 +849,12 @@  static int bme280_chip_config(struct bmp280_data *data)
 	return bmp280_chip_config(data);
 }
 
+static const u8 bme280_chip_ids[] = { BME280_CHIP_ID };
+
 const struct bmp280_chip_info bme280_chip_info = {
 	.id_reg = BMP280_REG_ID,
-	.chip_id = BME280_CHIP_ID,
+	.chip_id = bme280_chip_ids,
+	.num_chip_id = ARRAY_SIZE(bme280_chip_ids),
 	.regmap_config = &bmp280_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp280_channels,
@@ -1220,10 +1226,12 @@  static int bmp380_chip_config(struct bmp280_data *data)
 
 static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 };
 static const int bmp380_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128};
+static const u8 bmp380_chip_ids[] = { BMP380_CHIP_ID };
 
 const struct bmp280_chip_info bmp380_chip_info = {
 	.id_reg = BMP380_REG_ID,
-	.chip_id = BMP380_CHIP_ID,
+	.chip_id = bmp380_chip_ids,
+	.num_chip_id = ARRAY_SIZE(bmp380_chip_ids),
 	.regmap_config = &bmp380_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp380_channels,
@@ -1720,10 +1728,12 @@  static int bmp580_chip_config(struct bmp280_data *data)
 }
 
 static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
+static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT };
 
 const struct bmp280_chip_info bmp580_chip_info = {
 	.id_reg = BMP580_REG_CHIP_ID,
-	.chip_id = BMP580_CHIP_ID,
+	.chip_id = bmp580_chip_ids,
+	.num_chip_id = ARRAY_SIZE(bmp580_chip_ids),
 	.regmap_config = &bmp580_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp380_channels,
@@ -1983,10 +1993,12 @@  static int bmp180_chip_config(struct bmp280_data *data)
 
 static const int bmp180_oversampling_temp_avail[] = { 1 };
 static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 };
+static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID };
 
 const struct bmp280_chip_info bmp180_chip_info = {
 	.id_reg = BMP280_REG_ID,
-	.chip_id = BMP180_CHIP_ID,
+	.chip_id = bmp180_chip_ids,
+	.num_chip_id = ARRAY_SIZE(bmp180_chip_ids),
 	.regmap_config = &bmp180_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp280_channels,
@@ -2077,6 +2089,7 @@  int bmp280_common_probe(struct device *dev,
 	struct bmp280_data *data;
 	struct gpio_desc *gpiod;
 	unsigned int chip_id;
+	unsigned int i;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
@@ -2142,10 +2155,30 @@  int bmp280_common_probe(struct device *dev,
 	ret = regmap_read(regmap, data->chip_info->id_reg, &chip_id);
 	if (ret < 0)
 		return ret;
-	if (chip_id != data->chip_info->chip_id) {
-		dev_err(dev, "bad chip id: expected %x got %x\n",
-			data->chip_info->chip_id, chip_id);
-		return -EINVAL;
+
+	for (i = 0; i < data->chip_info->num_chip_id; i++) {
+		if (chip_id == data->chip_info->chip_id[i])
+			break;
+	}
+
+	if (i == data->chip_info->num_chip_id) {
+		size_t nbuf;
+		char *buf;
+
+		// 0x<id>, so four chars per number plus one space + ENDL
+		if (check_mul_overflow(data->chip_info->num_chip_id, 5, &nbuf))
+			return ret;
+
+		buf = kmalloc_array(data->chip_info->num_chip_id, 5, GFP_KERNEL);
+		if (!buf)
+			return ret;
+
+		for (i = 0; i < data->chip_info->num_chip_id; i++)
+			snprintf(&buf[i*5], nbuf - i*5, "0x%x ", data->chip_info->chip_id[i]);
+
+		dev_err(dev, "bad chip id: expected one of [ %s ] got 0x%x\n", buf, chip_id);
+		kfree(buf);
+		return ret;
 	}
 
 	if (data->chip_info->preinit) {
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 5c0563ce7572..a230fcfc4a85 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -418,7 +418,8 @@  struct bmp280_data {
 
 struct bmp280_chip_info {
 	unsigned int id_reg;
-	const unsigned int chip_id;
+	const u8 *chip_id;
+	int num_chip_id;
 
 	const struct regmap_config *regmap_config;