[PATCH v2 0/3] iio: pressure: bmp280: Add support for BMP390

[PATCH v2 0/3] iio: pressure: bmp280: Add support for BMP390

[Angel Iglesias]

Adds support for the Bosch BMP390 pressure sensors. This new series shares
the regmap present on the BMP380 family of sensors.

Changelog from v1:
- Added patch 1 optimizing data type used to store device ids (from
  unsigned integer to unsigned 8-bit integer).
- Enhanced implementation and fixed error string building on patch 2
  pointed by Andy Shevchenko <andriy.shevchenko@linux.intel.com>.
- Reorder IDs definitions on shared sensor header file on patch 3.
- Include a link to the datasheet for the bmp390 sensor in patch 3.

Previous versions:
v1:
https://lore.kernel.org/all/cover.1692305434.git.ang.iglesiasg@gmail.com/

Angel Iglesias (3):
  iio: pressure: bmp280: Use uint8 to store chip ids
  iio: pressure: bmp280: Allow multiple chips id per family of devices
  iio: pressure: bmp280: Add support for BMP390

 drivers/iio/pressure/bmp280-core.c | 54 ++++++++++++++++++++++++------
 drivers/iio/pressure/bmp280.h      |  4 ++-
 2 files changed, 47 insertions(+), 11 deletions(-)


base-commit: 14b7447cec15ee8dfdfe0da66ba1e280ded7e00a
-- 
2.42.0

[PATCH v2 1/3] iio: pressure: bmp280: Use uint8 to store chip ids

[Angel Iglesias]

Represent the device id reg values using uint8 to optimize memory use.

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

diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 5c0563ce7572..ff72e82f55ad 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -418,7 +418,7 @@ struct bmp280_data {
 
 struct bmp280_chip_info {
 	unsigned int id_reg;
-	const unsigned int chip_id;
+	uint8_t chip_id;
 
 	const struct regmap_config *regmap_config;
 
-- 
2.42.0

[PATCH v2 2/3] iio: pressure: bmp280: Allow multiple chips id per family of devices

[Angel Iglesias]

Improve device detection in certain chip families known to have various
chip ids.

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

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 6089f3f9d8f4..4a544a20d6cb 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -40,6 +40,7 @@
 #include <linux/completion.h>
 #include <linux/pm_runtime.h>
 #include <linux/random.h>
+#include <linux/overflow.h>
 
 #include <asm/unaligned.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 uint8_t 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 uint8_t 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 uint8_t 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 uint8_t 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 uint8_t 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,7 +2089,7 @@ int bmp280_common_probe(struct device *dev,
 	struct bmp280_data *data;
 	struct gpio_desc *gpiod;
 	unsigned int chip_id;
-	int ret;
+	int ret, i;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)
@@ -2142,10 +2154,31 @@ 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(5, data->chip_info->num_chip_id, &nbuf))
+			return ret;
+
+		buf = kmalloc_array(nbuf, sizeof(char), GFP_KERNEL);
+		if (!buf)
+			return ret;
+
+		for (i = 0; i < data->chip_info->num_chip_id; i++)
+			snprintf(&buf[i*5], nbuf, "0x%x ", data->chip_info->chip_id[i]);
+		buf[nbuf-1] = '\0';
+
+		dev_err(dev, "bad chip id: expected [ %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 ff72e82f55ad..c2acc9315223 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;
-	uint8_t chip_id;
+	const uint8_t *chip_id;
+	int num_chip_id;
 
 	const struct regmap_config *regmap_config;
 
-- 
2.42.0

[PATCH v2 3/3] iio: pressure: bmp280: Add support for BMP390

[Angel Iglesias]

Adds BMP390 device id to the supported ids on bmp380 sensor family

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

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 4a544a20d6cb..d679a5644d16 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -13,6 +13,7 @@
  * 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
  * https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp388-ds001.pdf
+ * https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp390-ds002.pdf
  * https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp581-ds004.pdf
  *
  * Notice:
@@ -1226,7 +1227,7 @@ 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 uint8_t bmp380_chip_ids[] = { BMP380_CHIP_ID };
+static const uint8_t bmp380_chip_ids[] = { BMP380_CHIP_ID, BMP390_CHIP_ID };
 
 const struct bmp280_chip_info bmp380_chip_info = {
 	.id_reg = BMP380_REG_ID,
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index c2acc9315223..d536e123ef14 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -292,6 +292,7 @@
 #define BMP580_CHIP_ID_ALT		0x51
 #define BMP180_CHIP_ID			0x55
 #define BMP280_CHIP_ID			0x58
+#define BMP390_CHIP_ID			0x60
 #define BME280_CHIP_ID			0x60
 #define BMP280_SOFT_RESET_VAL		0xB6
 
-- 
2.42.0

Re: [PATCH v2 1/3] iio: pressure: bmp280: Use uint8 to store chip ids

[Andy Shevchenko]

On Wed, Aug 23, 2023 at 05:58:05PM +0200, Angel Iglesias wrote:
> Represent the device id reg values using uint8 to optimize memory use.

This doesn't correspond to the code, in the code you used uint8_t.

...

>  struct bmp280_chip_info {
>  	unsigned int id_reg;
> -	const unsigned int chip_id;
> +	uint8_t chip_id;

While this will compile and even work properly in kernel we use uXX types,
here u8.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] iio: pressure: bmp280: Use uint8 to store chip ids

[Andy Shevchenko]

On Wed, Aug 23, 2023 at 07:10:33PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 23, 2023 at 05:58:05PM +0200, Angel Iglesias wrote:
> > Represent the device id reg values using uint8 to optimize memory use.
> 
> This doesn't correspond to the code, in the code you used uint8_t.

...

> >  struct bmp280_chip_info {
> >  	unsigned int id_reg;
> > -	const unsigned int chip_id;
> > +	uint8_t chip_id;
> 
> While this will compile and even work properly in kernel we use uXX types,
> here u8.

Actually this patch doesn't optimize memory use. The alignment will create
a gap anyway.

That said, this patch is simply redundant.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] iio: pressure: bmp280: Allow multiple chips id per family of devices

[Andy Shevchenko]

On Wed, Aug 23, 2023 at 05:58:06PM +0200, Angel Iglesias wrote:
> Improve device detection in certain chip families known to have various
> chip ids.

IDs

...

>  #include <linux/completion.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/random.h>
> +#include <linux/overflow.h>

Please, preserve ordering.

...

>  	struct bmp280_data *data;
>  	struct gpio_desc *gpiod;
>  	unsigned int chip_id;
> -	int ret;
> +	int ret, i;

	unsigned int i;

...

> +	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(5, data->chip_info->num_chip_id, &nbuf))
> +			return ret;
> +
> +		buf = kmalloc_array(nbuf, sizeof(char), GFP_KERNEL);

We almost never do a array allocation for byte sizes. Instead of the above you
need to use

		buf = kmalloc_array(data->chip_info->num_chip_id, 5, GFP_KERNEL);

> +		if (!buf)

This check assumes that num_chip_id is never 0, so...

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

> +		buf[nbuf-1] = '\0';

...this is redundant assignment. sprintf() guarantees the NUL termination.

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

"...expected one of..."

> +		kfree(buf);
> +		return ret;

Oh, I didn't get that you allocated memory only to print a message...


>  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] iio: pressure: bmp280: Add support for BMP390

[Andy Shevchenko]

On Wed, Aug 23, 2023 at 05:58:07PM +0200, Angel Iglesias wrote:
> Adds BMP390 device id to the supported ids on bmp380 sensor family

Add
ID
IDs

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] iio: pressure: bmp280: Use uint8 to store chip ids

[Angel Iglesias]

On Wed, 2023-08-23 at 19:17 +0300, Andy Shevchenko wrote:
> On Wed, Aug 23, 2023 at 07:10:33PM +0300, Andy Shevchenko wrote:
> > On Wed, Aug 23, 2023 at 05:58:05PM +0200, Angel Iglesias wrote:
> > > Represent the device id reg values using uint8 to optimize memory use.
> > 
> > This doesn't correspond to the code, in the code you used uint8_t.
> 
> ...
> 
> > >  struct bmp280_chip_info {
> > >         unsigned int id_reg;
> > > -       const unsigned int chip_id;
> > > +       uint8_t chip_id;
> > 
> > While this will compile and even work properly in kernel we use uXX types,
> > here u8.
> 
> Actually this patch doesn't optimize memory use. The alignment will create
> a gap anyway.
> 
> That said, this patch is simply redundant.
> 
OK, I'll drop it on v3 then

Re: [PATCH v2 2/3] iio: pressure: bmp280: Allow multiple chips id per family of devices

[Angel Iglesias]

On Wed, 2023-08-23 at 19:26 +0300, Andy Shevchenko wrote:
> On Wed, Aug 23, 2023 at 05:58:06PM +0200, Angel Iglesias wrote:
> > Improve device detection in certain chip families known to have various
> > chip ids.
> 
> IDs
> 
> ...
> 
> >  #include <linux/completion.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/random.h>
> > +#include <linux/overflow.h>
> 
> Please, preserve ordering.
> 
> ...
> 
> >         struct bmp280_data *data;
> >         struct gpio_desc *gpiod;
> >         unsigned int chip_id;
> > -       int ret;
> > +       int ret, i;
> 
>         unsigned int i;
> 
> ...
> 
> > +       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(5, data->chip_info->num_chip_id,
> > &nbuf))
> > +                       return ret;
> > +
> > +               buf = kmalloc_array(nbuf, sizeof(char), GFP_KERNEL);
> 
> We almost never do a array allocation for byte sizes. Instead of the above you
> need to use
> 
>                 buf = kmalloc_array(data->chip_info->num_chip_id, 5,
> GFP_KERNEL);
> 
> > +               if (!buf)
> 
> This check assumes that num_chip_id is never 0, so...
> 
> > +                       return ret;
> > +
> > +               for (i = 0; i < data->chip_info->num_chip_id; i++)
> > +                       snprintf(&buf[i*5], nbuf, "0x%x ", data->chip_info-
> > >chip_id[i]);
> 
> > +               buf[nbuf-1] = '\0';
> 
> ...this is redundant assignment. sprintf() guarantees the NUL termination.
> 
> > +
> > +               dev_err(dev, "bad chip id: expected [ %s ] got 0x%x\n", buf,
> > chip_id);
> 
> "...expected one of..."
> 
> > +               kfree(buf);
> > +               return ret;
> 
> Oh, I didn't get that you allocated memory only to print a message...

Yes, I just wanted to print the known IDs like before in a readable manner. If
it is overkill, I'll change it for a more generic error message.

> 
> >         }
> 
Kind regards,
Angel