[PATCH v2 0/5] Add support for pressure sensor Bosch BMP580

[PATCH v2 0/5] Add support for pressure sensor Bosch BMP580

[Angel Iglesias]

This patchset adds support for the new pressure sensors BMP580 extending
the bmp280 driver.

Patch 1 introduces a variant enumeration and refactors sensor verification
logic adding a chip_id field to the chip_info struct. This change is
required because BMP380 and BMP580 have the same chip_id and values would
collide using the chip_id as the driver_data value.
Patch 2 introduces new preinit callback and unifies init logic across all
supported variants.
Patch 3 extends the bmp280 driver with the new logic to read measurements
and configure the operation parameters for the BMP580 sensors.
Patch 4 updates the devicetree binding docs with the new sensor id.
Patch 5 adds the NVMEM operations to read and program the NVM user range
contained in the non-volatile memory of the BMP580 sensors.

Changes in V2:
* For patch 3, fixed missing retcodes reported by the kernel test robot.
* For patch 5, fixed logic paths that left the sensor mutex locked
  reported by the kernel test robot.

Angel Iglesias (5):
  iio: pressure: bmp280: Add enumeration to handle chip variants
  iio: pressure: bmp280: Add preinit callback
  iio: pressure: bmp280: Add support for new sensor BMP580
  dt-bindings: iio: pressure: bmp085: Add BMP580 compatible string
  iio: pressure: bmp280: Add nvmem operations for BMP580

 .../bindings/iio/pressure/bmp085.yaml         |   2 +
 drivers/iio/pressure/Kconfig                  |   6 +-
 drivers/iio/pressure/bmp280-core.c            | 617 +++++++++++++++++-
 drivers/iio/pressure/bmp280-i2c.c             |  33 +-
 drivers/iio/pressure/bmp280-regmap.c          |  60 ++
 drivers/iio/pressure/bmp280-spi.c             |  23 +-
 drivers/iio/pressure/bmp280.h                 | 115 ++++
 7 files changed, 815 insertions(+), 41 deletions(-)


base-commit: e807541c2b273677e82ef50b5747ec7ae7d652b9
-- 
2.39.0

[PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants

[Angel Iglesias]

Adds enumeration to improve handling the different supported sensors
on driver initialization. This avoid collisions if different variants
share the same device idetifier on ID register.

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 c0aff78489b4..46959a91408f 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -186,6 +186,7 @@ struct bmp280_data {
 
 struct bmp280_chip_info {
 	unsigned int id_reg;
+	const unsigned int chip_id;
 
 	const struct iio_chan_spec *channels;
 	int num_channels;
@@ -907,6 +908,7 @@ static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
 
 static const struct bmp280_chip_info bmp280_chip_info = {
 	.id_reg = BMP280_REG_ID,
+	.chip_id = BMP280_CHIP_ID,
 	.start_up_time = 2000,
 	.channels = bmp280_channels,
 	.num_channels = 2,
@@ -955,6 +957,7 @@ static int bme280_chip_config(struct bmp280_data *data)
 
 static const struct bmp280_chip_info bme280_chip_info = {
 	.id_reg = BMP280_REG_ID,
+	.chip_id = BME280_CHIP_ID,
 	.start_up_time = 2000,
 	.channels = bmp280_channels,
 	.num_channels = 3,
@@ -1321,6 +1324,7 @@ static const int bmp380_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 12
 
 static const struct bmp280_chip_info bmp380_chip_info = {
 	.id_reg = BMP380_REG_ID,
+	.chip_id = BMP380_CHIP_ID,
 	.start_up_time = 2000,
 	.channels = bmp380_channels,
 	.num_channels = 2,
@@ -1581,6 +1585,7 @@ static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 };
 
 static const struct bmp280_chip_info bmp180_chip_info = {
 	.id_reg = BMP280_REG_ID,
+	.chip_id = BMP180_CHIP_ID,
 	.start_up_time = 2000,
 	.channels = bmp280_channels,
 	.num_channels = 2,
@@ -1685,16 +1690,16 @@ int bmp280_common_probe(struct device *dev,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	switch (chip) {
-	case BMP180_CHIP_ID:
+	case BMP180:
 		chip_info = &bmp180_chip_info;
 		break;
-	case BMP280_CHIP_ID:
+	case BMP280:
 		chip_info = &bmp280_chip_info;
 		break;
-	case BME280_CHIP_ID:
+	case BME280:
 		chip_info = &bme280_chip_info;
 		break;
-	case BMP380_CHIP_ID:
+	case BMP380:
 		chip_info = &bmp380_chip_info;
 		break;
 	default:
@@ -1751,9 +1756,9 @@ 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 != chip) {
+	if (chip_id != data->chip_info->chip_id) {
 		dev_err(dev, "bad chip id: expected %x got %x\n",
-			chip, chip_id);
+			data->chip_info->chip_id, chip_id);
 		return -EINVAL;
 	}
 
diff --git a/drivers/iio/pressure/bmp280-i2c.c b/drivers/iio/pressure/bmp280-i2c.c
index 14eab086d24a..59921e8cd592 100644
--- a/drivers/iio/pressure/bmp280-i2c.c
+++ b/drivers/iio/pressure/bmp280-i2c.c
@@ -12,14 +12,14 @@ static int bmp280_i2c_probe(struct i2c_client *client)
 	const struct i2c_device_id *id = i2c_client_get_device_id(client);
 
 	switch (id->driver_data) {
-	case BMP180_CHIP_ID:
+	case BMP180:
 		regmap_config = &bmp180_regmap_config;
 		break;
-	case BMP280_CHIP_ID:
-	case BME280_CHIP_ID:
+	case BMP280:
+	case BME280:
 		regmap_config = &bmp280_regmap_config;
 		break;
-	case BMP380_CHIP_ID:
+	case BMP380:
 		regmap_config = &bmp380_regmap_config;
 		break;
 	default:
@@ -40,21 +40,21 @@ static int bmp280_i2c_probe(struct i2c_client *client)
 }
 
 static const struct of_device_id bmp280_of_i2c_match[] = {
-	{ .compatible = "bosch,bmp085", .data = (void *)BMP180_CHIP_ID },
-	{ .compatible = "bosch,bmp180", .data = (void *)BMP180_CHIP_ID },
-	{ .compatible = "bosch,bmp280", .data = (void *)BMP280_CHIP_ID },
-	{ .compatible = "bosch,bme280", .data = (void *)BME280_CHIP_ID },
-	{ .compatible = "bosch,bmp380", .data = (void *)BMP380_CHIP_ID },
+	{ .compatible = "bosch,bmp085", .data = (void *)BMP180 },
+	{ .compatible = "bosch,bmp180", .data = (void *)BMP180 },
+	{ .compatible = "bosch,bmp280", .data = (void *)BMP280 },
+	{ .compatible = "bosch,bme280", .data = (void *)BME280 },
+	{ .compatible = "bosch,bmp380", .data = (void *)BMP380 },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, bmp280_of_i2c_match);
 
 static const struct i2c_device_id bmp280_i2c_id[] = {
-	{"bmp085", BMP180_CHIP_ID },
-	{"bmp180", BMP180_CHIP_ID },
-	{"bmp280", BMP280_CHIP_ID },
-	{"bme280", BME280_CHIP_ID },
-	{"bmp380", BMP380_CHIP_ID },
+	{"bmp085", BMP180 },
+	{"bmp180", BMP180 },
+	{"bmp280", BMP280 },
+	{"bme280", BME280 },
+	{"bmp380", BMP380 },
 	{ },
 };
 MODULE_DEVICE_TABLE(i2c, bmp280_i2c_id);
diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
index 011c68e07ebf..4a2df5b5d838 100644
--- a/drivers/iio/pressure/bmp280-spi.c
+++ b/drivers/iio/pressure/bmp280-spi.c
@@ -59,14 +59,14 @@ static int bmp280_spi_probe(struct spi_device *spi)
 	}
 
 	switch (id->driver_data) {
-	case BMP180_CHIP_ID:
+	case BMP180:
 		regmap_config = &bmp180_regmap_config;
 		break;
-	case BMP280_CHIP_ID:
-	case BME280_CHIP_ID:
+	case BMP280:
+	case BME280:
 		regmap_config = &bmp280_regmap_config;
 		break;
-	case BMP380_CHIP_ID:
+	case BMP380:
 		regmap_config = &bmp380_regmap_config;
 		break;
 	default:
@@ -101,11 +101,11 @@ static const struct of_device_id bmp280_of_spi_match[] = {
 MODULE_DEVICE_TABLE(of, bmp280_of_spi_match);
 
 static const struct spi_device_id bmp280_spi_id[] = {
-	{ "bmp180", BMP180_CHIP_ID },
-	{ "bmp181", BMP180_CHIP_ID },
-	{ "bmp280", BMP280_CHIP_ID },
-	{ "bme280", BME280_CHIP_ID },
-	{ "bmp380", BMP380_CHIP_ID },
+	{ "bmp180", BMP180 },
+	{ "bmp181", BMP180 },
+	{ "bmp280", BMP280 },
+	{ "bme280", BME280 },
+	{ "bmp380", BMP380 },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, bmp280_spi_id);
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index c791325c7416..efc31bc84708 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -191,6 +191,14 @@
 #define BMP280_PRESS_SKIPPED		0x80000
 #define BMP280_HUMIDITY_SKIPPED		0x8000
 
+/* Enum with supported pressure sensor models */
+enum bmp280_variant {
+	BMP180,
+	BMP280,
+	BME280,
+	BMP380,
+};
+
 /* Regmap configurations */
 extern const struct regmap_config bmp180_regmap_config;
 extern const struct regmap_config bmp280_regmap_config;
-- 
2.39.0

Re: [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants

[Andy Shevchenko]

On Mon, Dec 26, 2022 at 03:29:20PM +0100, Angel Iglesias wrote:
> Adds enumeration to improve handling the different supported sensors
> on driver initialization. This avoid collisions if different variants
> share the same device idetifier on ID register.

As per v1, use pointers in the ID tables.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants

[Angel Iglesias]

On Tue, 2022-12-27 at 23:37 +0200, Andy Shevchenko wrote:
> On Mon, Dec 26, 2022 at 03:29:20PM +0100, Angel Iglesias wrote:
> > Adds enumeration to improve handling the different supported sensors
> > on driver initialization. This avoid collisions if different variants
> > share the same device idetifier on ID register.
> 
> As per v1, use pointers in the ID tables.
> 

Taking your suggestion and Jonathan's remarks into account seems to me like the
best approach here is using chip_info pointer for each driver as the pointer set
on the id tables. As in the i2c and spi drivers, the enum is used to fetch the
correct regmap configuration, and later in the shared probe, the chip_info. The
logical follow-up would be adding the regmap configuration to the chip_info,
right?
Or is there a better solution I'm not seeing right now?

Thanks for your time,
Angel

Re: [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants

[Jonathan Cameron]

On Sun, 01 Jan 2023 12:04:40 +0100
Angel Iglesias <ang.iglesiasg@gmail.com> wrote:

> On Tue, 2022-12-27 at 23:37 +0200, Andy Shevchenko wrote:
> > On Mon, Dec 26, 2022 at 03:29:20PM +0100, Angel Iglesias wrote:  
> > > Adds enumeration to improve handling the different supported sensors
> > > on driver initialization. This avoid collisions if different variants
> > > share the same device idetifier on ID register.  
> > 
> > As per v1, use pointers in the ID tables.
> >   
> 
> Taking your suggestion and Jonathan's remarks into account seems to me like the
> best approach here is using chip_info pointer for each driver as the pointer set
> on the id tables.

Yes.

> As in the i2c and spi drivers, the enum is used to fetch the
> correct regmap configuration, and later in the shared probe, the chip_info. The
> logical follow-up would be adding the regmap configuration to the chip_info,
> right?

Makes sense.

> Or is there a better solution I'm not seeing right now?

If I'm understanding what you have above, then this is exactly what we would
want to do here.

Thanks,

Jonathan

> 
> Thanks for your time,
> Angel

Re: [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants

[Jonathan Cameron]

On Mon, 26 Dec 2022 15:29:20 +0100
Angel Iglesias <ang.iglesiasg@gmail.com> wrote:

> Adds enumeration to improve handling the different supported sensors
> on driver initialization. This avoid collisions if different variants
> share the same device idetifier on ID register.

If we get two parts with the same ID that need different handling
then we should probably be shouting at Bosch.  Still I don't mind
tidying this up anyway as using the CHIP_IDs is messy - however...

Please be careful to make sure you have responded (either by changes, or by
replying to review to say why you aren't making changes).  If you are not receiving
all emails, then you can always check lore.kernel.org to make sure you didn't miss
any reviews.

As Andy suggested, switch over to using pointers to the chip_info structure as the
data element in *_device_id tables.  It usually ends up neater in the end than
messing around with an indirection via an enum value.

> 
> 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 c0aff78489b4..46959a91408f 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -186,6 +186,7 @@ struct bmp280_data {
>  
>  struct bmp280_chip_info {
>  	unsigned int id_reg;
> +	const unsigned int chip_id;
>  
>  	const struct iio_chan_spec *channels;
>  	int num_channels;
> @@ -907,6 +908,7 @@ static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
>  
>  static const struct bmp280_chip_info bmp280_chip_info = {
>  	.id_reg = BMP280_REG_ID,
> +	.chip_id = BMP280_CHIP_ID,
>  	.start_up_time = 2000,
>  	.channels = bmp280_channels,
>  	.num_channels = 2,
> @@ -955,6 +957,7 @@ static int bme280_chip_config(struct bmp280_data *data)
>  
>  static const struct bmp280_chip_info bme280_chip_info = {
>  	.id_reg = BMP280_REG_ID,
> +	.chip_id = BME280_CHIP_ID,
>  	.start_up_time = 2000,
>  	.channels = bmp280_channels,
>  	.num_channels = 3,
> @@ -1321,6 +1324,7 @@ static const int bmp380_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 12
>  
>  static const struct bmp280_chip_info bmp380_chip_info = {
>  	.id_reg = BMP380_REG_ID,
> +	.chip_id = BMP380_CHIP_ID,
>  	.start_up_time = 2000,
>  	.channels = bmp380_channels,
>  	.num_channels = 2,
> @@ -1581,6 +1585,7 @@ static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 };
>  
>  static const struct bmp280_chip_info bmp180_chip_info = {
>  	.id_reg = BMP280_REG_ID,
> +	.chip_id = BMP180_CHIP_ID,
>  	.start_up_time = 2000,
>  	.channels = bmp280_channels,
>  	.num_channels = 2,
> @@ -1685,16 +1690,16 @@ int bmp280_common_probe(struct device *dev,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	switch (chip) {
> -	case BMP180_CHIP_ID:
> +	case BMP180:
>  		chip_info = &bmp180_chip_info;
>  		break;
> -	case BMP280_CHIP_ID:
> +	case BMP280:
>  		chip_info = &bmp280_chip_info;
>  		break;
> -	case BME280_CHIP_ID:
> +	case BME280:
>  		chip_info = &bme280_chip_info;
>  		break;
> -	case BMP380_CHIP_ID:
> +	case BMP380:
>  		chip_info = &bmp380_chip_info;

If you use a pointer directly then no need for this switch statement at all.

>  		break;
>  	default:
> @@ -1751,9 +1756,9 @@ 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 != chip) {
> +	if (chip_id != data->chip_info->chip_id) {
>  		dev_err(dev, "bad chip id: expected %x got %x\n",
> -			chip, chip_id);
> +			data->chip_info->chip_id, chip_id);
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/iio/pressure/bmp280-i2c.c b/drivers/iio/pressure/bmp280-i2c.c
> index 14eab086d24a..59921e8cd592 100644
> --- a/drivers/iio/pressure/bmp280-i2c.c
> +++ b/drivers/iio/pressure/bmp280-i2c.c
> @@ -12,14 +12,14 @@ static int bmp280_i2c_probe(struct i2c_client *client)
>  	const struct i2c_device_id *id = i2c_client_get_device_id(client);
>  
>  	switch (id->driver_data) {
> -	case BMP180_CHIP_ID:
> +	case BMP180:
>  		regmap_config = &bmp180_regmap_config;
>  		break;
> -	case BMP280_CHIP_ID:
> -	case BME280_CHIP_ID:
> +	case BMP280:
> +	case BME280:
>  		regmap_config = &bmp280_regmap_config;
>  		break;
> -	case BMP380_CHIP_ID:
> +	case BMP380:
>  		regmap_config = &bmp380_regmap_config;
>  		break;
>  	default:
> @@ -40,21 +40,21 @@ static int bmp280_i2c_probe(struct i2c_client *client)
>  }
>  
>  static const struct of_device_id bmp280_of_i2c_match[] = {
> -	{ .compatible = "bosch,bmp085", .data = (void *)BMP180_CHIP_ID },
> -	{ .compatible = "bosch,bmp180", .data = (void *)BMP180_CHIP_ID },
> -	{ .compatible = "bosch,bmp280", .data = (void *)BMP280_CHIP_ID },
> -	{ .compatible = "bosch,bme280", .data = (void *)BME280_CHIP_ID },
> -	{ .compatible = "bosch,bmp380", .data = (void *)BMP380_CHIP_ID },
> +	{ .compatible = "bosch,bmp085", .data = (void *)BMP180 },
> +	{ .compatible = "bosch,bmp180", .data = (void *)BMP180 },
> +	{ .compatible = "bosch,bmp280", .data = (void *)BMP280 },
> +	{ .compatible = "bosch,bme280", .data = (void *)BME280 },
> +	{ .compatible = "bosch,bmp380", .data = (void *)BMP380 },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, bmp280_of_i2c_match);
>  
>  static const struct i2c_device_id bmp280_i2c_id[] = {
> -	{"bmp085", BMP180_CHIP_ID },
> -	{"bmp180", BMP180_CHIP_ID },
> -	{"bmp280", BMP280_CHIP_ID },
> -	{"bme280", BME280_CHIP_ID },
> -	{"bmp380", BMP380_CHIP_ID },
> +	{"bmp085", BMP180 },
> +	{"bmp180", BMP180 },
> +	{"bmp280", BMP280 },
> +	{"bme280", BME280 },
> +	{"bmp380", BMP380 },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(i2c, bmp280_i2c_id);
> diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
> index 011c68e07ebf..4a2df5b5d838 100644
> --- a/drivers/iio/pressure/bmp280-spi.c
> +++ b/drivers/iio/pressure/bmp280-spi.c
> @@ -59,14 +59,14 @@ static int bmp280_spi_probe(struct spi_device *spi)
>  	}
>  
>  	switch (id->driver_data) {
> -	case BMP180_CHIP_ID:
> +	case BMP180:
>  		regmap_config = &bmp180_regmap_config;
>  		break;
> -	case BMP280_CHIP_ID:
> -	case BME280_CHIP_ID:
> +	case BMP280:
> +	case BME280:
>  		regmap_config = &bmp280_regmap_config;
>  		break;
> -	case BMP380_CHIP_ID:
> +	case BMP380:
>  		regmap_config = &bmp380_regmap_config;
>  		break;
>  	default:
> @@ -101,11 +101,11 @@ static const struct of_device_id bmp280_of_spi_match[] = {
>  MODULE_DEVICE_TABLE(of, bmp280_of_spi_match);
>  
>  static const struct spi_device_id bmp280_spi_id[] = {
> -	{ "bmp180", BMP180_CHIP_ID },
> -	{ "bmp181", BMP180_CHIP_ID },
> -	{ "bmp280", BMP280_CHIP_ID },
> -	{ "bme280", BME280_CHIP_ID },
> -	{ "bmp380", BMP380_CHIP_ID },
> +	{ "bmp180", BMP180 },
> +	{ "bmp181", BMP180 },
> +	{ "bmp280", BMP280 },
> +	{ "bme280", BME280 },
> +	{ "bmp380", BMP380 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(spi, bmp280_spi_id);
> diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> index c791325c7416..efc31bc84708 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -191,6 +191,14 @@
>  #define BMP280_PRESS_SKIPPED		0x80000
>  #define BMP280_HUMIDITY_SKIPPED		0x8000
>  
> +/* Enum with supported pressure sensor models */
> +enum bmp280_variant {
> +	BMP180,
> +	BMP280,
> +	BME280,
> +	BMP380,
> +};
> +
>  /* Regmap configurations */
>  extern const struct regmap_config bmp180_regmap_config;
>  extern const struct regmap_config bmp280_regmap_config;

Re: [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants

[Angel Iglesias]

On Fri, 2022-12-30 at 18:14 +0000, Jonathan Cameron wrote:
> On Mon, 26 Dec 2022 15:29:20 +0100
> Angel Iglesias <ang.iglesiasg@gmail.com> wrote:
> 
> > Adds enumeration to improve handling the different supported sensors
> > on driver initialization. This avoid collisions if different variants
> > share the same device idetifier on ID register.
> 
> If we get two parts with the same ID that need different handling
> then we should probably be shouting at Bosch.  Still I don't mind
> tidying this up anyway as using the CHIP_IDs is messy - however...
> 
> Please be careful to make sure you have responded (either by changes, or by
> replying to review to say why you aren't making changes).  If you are not
> receiving
> all emails, then you can always check lore.kernel.org to make sure you didn't
> miss
> any reviews.

I've been away for the week visiting family and friends. Sorry for leaving your
reviews lingering unanswered.
Yeah, I kinda rushed this part of the patch to send it before christmas, not my
brightest call. I like Andy suggestion, but I have a few questions on handling
the regmap and the chip config using the pointers, please refer to the reply I
sent to Andy review for the details.

Thanks for your time,
Angel

> As Andy suggested, switch over to using pointers to the chip_info structure as
> the
> data element in *_device_id tables.  It usually ends up neater in the end than
> messing around with an indirection via an enum value.
> 
> > 
> > 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 c0aff78489b4..46959a91408f 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -186,6 +186,7 @@ struct bmp280_data {
> >  
> >  struct bmp280_chip_info {
> >         unsigned int id_reg;
> > +       const unsigned int chip_id;
> >  
> >         const struct iio_chan_spec *channels;
> >         int num_channels;
> > @@ -907,6 +908,7 @@ static const int bmp280_oversampling_avail[] = { 1, 2,
> > 4, 8, 16 };
> >  
> >  static const struct bmp280_chip_info bmp280_chip_info = {
> >         .id_reg = BMP280_REG_ID,
> > +       .chip_id = BMP280_CHIP_ID,
> >         .start_up_time = 2000,
> >         .channels = bmp280_channels,
> >         .num_channels = 2,
> > @@ -955,6 +957,7 @@ static int bme280_chip_config(struct bmp280_data *data)
> >  
> >  static const struct bmp280_chip_info bme280_chip_info = {
> >         .id_reg = BMP280_REG_ID,
> > +       .chip_id = BME280_CHIP_ID,
> >         .start_up_time = 2000,
> >         .channels = bmp280_channels,
> >         .num_channels = 3,
> > @@ -1321,6 +1324,7 @@ static const int bmp380_iir_filter_coeffs_avail[] = {
> > 1, 2, 4, 8, 16, 32, 64, 12
> >  
> >  static const struct bmp280_chip_info bmp380_chip_info = {
> >         .id_reg = BMP380_REG_ID,
> > +       .chip_id = BMP380_CHIP_ID,
> >         .start_up_time = 2000,
> >         .channels = bmp380_channels,
> >         .num_channels = 2,
> > @@ -1581,6 +1585,7 @@ static const int bmp180_oversampling_press_avail[] = {
> > 1, 2, 4, 8 };
> >  
> >  static const struct bmp280_chip_info bmp180_chip_info = {
> >         .id_reg = BMP280_REG_ID,
> > +       .chip_id = BMP180_CHIP_ID,
> >         .start_up_time = 2000,
> >         .channels = bmp280_channels,
> >         .num_channels = 2,
> > @@ -1685,16 +1690,16 @@ int bmp280_common_probe(struct device *dev,
> >         indio_dev->modes = INDIO_DIRECT_MODE;
> >  
> >         switch (chip) {
> > -       case BMP180_CHIP_ID:
> > +       case BMP180:
> >                 chip_info = &bmp180_chip_info;
> >                 break;
> > -       case BMP280_CHIP_ID:
> > +       case BMP280:
> >                 chip_info = &bmp280_chip_info;
> >                 break;
> > -       case BME280_CHIP_ID:
> > +       case BME280:
> >                 chip_info = &bme280_chip_info;
> >                 break;
> > -       case BMP380_CHIP_ID:
> > +       case BMP380:
> >                 chip_info = &bmp380_chip_info;
> 
> If you use a pointer directly then no need for this switch statement at all.
> 
> >                 break;
> >         default:
> > @@ -1751,9 +1756,9 @@ 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 != chip) {
> > +       if (chip_id != data->chip_info->chip_id) {
> >                 dev_err(dev, "bad chip id: expected %x got %x\n",
> > -                       chip, chip_id);
> > +                       data->chip_info->chip_id, chip_id);
> >                 return -EINVAL;
> >         }
> >  
> > diff --git a/drivers/iio/pressure/bmp280-i2c.c
> > b/drivers/iio/pressure/bmp280-i2c.c
> > index 14eab086d24a..59921e8cd592 100644
> > --- a/drivers/iio/pressure/bmp280-i2c.c
> > +++ b/drivers/iio/pressure/bmp280-i2c.c
> > @@ -12,14 +12,14 @@ static int bmp280_i2c_probe(struct i2c_client *client)
> >         const struct i2c_device_id *id = i2c_client_get_device_id(client);
> >  
> >         switch (id->driver_data) {
> > -       case BMP180_CHIP_ID:
> > +       case BMP180:
> >                 regmap_config = &bmp180_regmap_config;
> >                 break;
> > -       case BMP280_CHIP_ID:
> > -       case BME280_CHIP_ID:
> > +       case BMP280:
> > +       case BME280:
> >                 regmap_config = &bmp280_regmap_config;
> >                 break;
> > -       case BMP380_CHIP_ID:
> > +       case BMP380:
> >                 regmap_config = &bmp380_regmap_config;
> >                 break;
> >         default:
> > @@ -40,21 +40,21 @@ static int bmp280_i2c_probe(struct i2c_client *client)
> >  }
> >  
> >  static const struct of_device_id bmp280_of_i2c_match[] = {
> > -       { .compatible = "bosch,bmp085", .data = (void *)BMP180_CHIP_ID },
> > -       { .compatible = "bosch,bmp180", .data = (void *)BMP180_CHIP_ID },
> > -       { .compatible = "bosch,bmp280", .data = (void *)BMP280_CHIP_ID },
> > -       { .compatible = "bosch,bme280", .data = (void *)BME280_CHIP_ID },
> > -       { .compatible = "bosch,bmp380", .data = (void *)BMP380_CHIP_ID },
> > +       { .compatible = "bosch,bmp085", .data = (void *)BMP180 },
> > +       { .compatible = "bosch,bmp180", .data = (void *)BMP180 },
> > +       { .compatible = "bosch,bmp280", .data = (void *)BMP280 },
> > +       { .compatible = "bosch,bme280", .data = (void *)BME280 },
> > +       { .compatible = "bosch,bmp380", .data = (void *)BMP380 },
> >         { },
> >  };
> >  MODULE_DEVICE_TABLE(of, bmp280_of_i2c_match);
> >  
> >  static const struct i2c_device_id bmp280_i2c_id[] = {
> > -       {"bmp085", BMP180_CHIP_ID },
> > -       {"bmp180", BMP180_CHIP_ID },
> > -       {"bmp280", BMP280_CHIP_ID },
> > -       {"bme280", BME280_CHIP_ID },
> > -       {"bmp380", BMP380_CHIP_ID },
> > +       {"bmp085", BMP180 },
> > +       {"bmp180", BMP180 },
> > +       {"bmp280", BMP280 },
> > +       {"bme280", BME280 },
> > +       {"bmp380", BMP380 },
> >         { },
> >  };
> >  MODULE_DEVICE_TABLE(i2c, bmp280_i2c_id);
> > diff --git a/drivers/iio/pressure/bmp280-spi.c
> > b/drivers/iio/pressure/bmp280-spi.c
> > index 011c68e07ebf..4a2df5b5d838 100644
> > --- a/drivers/iio/pressure/bmp280-spi.c
> > +++ b/drivers/iio/pressure/bmp280-spi.c
> > @@ -59,14 +59,14 @@ static int bmp280_spi_probe(struct spi_device *spi)
> >         }
> >  
> >         switch (id->driver_data) {
> > -       case BMP180_CHIP_ID:
> > +       case BMP180:
> >                 regmap_config = &bmp180_regmap_config;
> >                 break;
> > -       case BMP280_CHIP_ID:
> > -       case BME280_CHIP_ID:
> > +       case BMP280:
> > +       case BME280:
> >                 regmap_config = &bmp280_regmap_config;
> >                 break;
> > -       case BMP380_CHIP_ID:
> > +       case BMP380:
> >                 regmap_config = &bmp380_regmap_config;
> >                 break;
> >         default:
> > @@ -101,11 +101,11 @@ static const struct of_device_id bmp280_of_spi_match[]
> > = {
> >  MODULE_DEVICE_TABLE(of, bmp280_of_spi_match);
> >  
> >  static const struct spi_device_id bmp280_spi_id[] = {
> > -       { "bmp180", BMP180_CHIP_ID },
> > -       { "bmp181", BMP180_CHIP_ID },
> > -       { "bmp280", BMP280_CHIP_ID },
> > -       { "bme280", BME280_CHIP_ID },
> > -       { "bmp380", BMP380_CHIP_ID },
> > +       { "bmp180", BMP180 },
> > +       { "bmp181", BMP180 },
> > +       { "bmp280", BMP280 },
> > +       { "bme280", BME280 },
> > +       { "bmp380", BMP380 },
> >         { }
> >  };
> >  MODULE_DEVICE_TABLE(spi, bmp280_spi_id);
> > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > index c791325c7416..efc31bc84708 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -191,6 +191,14 @@
> >  #define BMP280_PRESS_SKIPPED           0x80000
> >  #define BMP280_HUMIDITY_SKIPPED                0x8000
> >  
> > +/* Enum with supported pressure sensor models */
> > +enum bmp280_variant {
> > +       BMP180,
> > +       BMP280,
> > +       BME280,
> > +       BMP380,
> > +};
> > +
> >  /* Regmap configurations */
> >  extern const struct regmap_config bmp180_regmap_config;
> >  extern const struct regmap_config bmp280_regmap_config;
> 

[PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback

[Angel Iglesias]

Adds preinit callback to execute operations on probe before applying
initial configuration.

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 46959a91408f..c37cf2caec68 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -217,6 +217,7 @@ struct bmp280_chip_info {
 	int (*read_press)(struct bmp280_data *, int *, int *);
 	int (*read_humid)(struct bmp280_data *, int *, int *);
 	int (*read_calib)(struct bmp280_data *);
+	int (*preinit)(struct bmp280_data *);
 };
 
 /*
@@ -935,6 +936,7 @@ static const struct bmp280_chip_info bmp280_chip_info = {
 	.read_temp = bmp280_read_temp,
 	.read_press = bmp280_read_press,
 	.read_calib = bmp280_read_calib,
+	.preinit = NULL,
 };
 
 static int bme280_chip_config(struct bmp280_data *data)
@@ -979,6 +981,7 @@ static const struct bmp280_chip_info bme280_chip_info = {
 	.read_press = bmp280_read_press,
 	.read_humid = bmp280_read_humid,
 	.read_calib = bme280_read_calib,
+	.preinit = NULL,
 };
 
 /*
@@ -1220,6 +1223,12 @@ static const int bmp380_odr_table[][2] = {
 	[BMP380_ODR_0_0015HZ]	= {0, 1526},
 };
 
+static int bmp380_preinit(struct bmp280_data *data)
+{
+	/* BMP3xx requires soft-reset as part of initialization */
+	return bmp380_cmd(data, BMP380_CMD_SOFT_RESET);
+}
+
 static int bmp380_chip_config(struct bmp280_data *data)
 {
 	bool change = false, aux;
@@ -1349,6 +1358,7 @@ static const struct bmp280_chip_info bmp380_chip_info = {
 	.read_temp = bmp380_read_temp,
 	.read_press = bmp380_read_press,
 	.read_calib = bmp380_read_calib,
+	.preinit = bmp380_preinit,
 };
 
 static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
@@ -1604,6 +1614,7 @@ static const struct bmp280_chip_info bmp180_chip_info = {
 	.read_temp = bmp180_read_temp,
 	.read_press = bmp180_read_press,
 	.read_calib = bmp180_read_calib,
+	.preinit = NULL,
 };
 
 static irqreturn_t bmp085_eoc_irq(int irq, void *d)
@@ -1762,9 +1773,13 @@ int bmp280_common_probe(struct device *dev,
 		return -EINVAL;
 	}
 
-	/* BMP3xx requires soft-reset as part of initialization */
-	if (chip_id == BMP380_CHIP_ID) {
-		ret = bmp380_cmd(data, BMP380_CMD_SOFT_RESET);
+	/*
+	 * Some chips like the BMP3xx have preinit tasks to run
+	 * before applying the initial configuration.
+	 */
+	if (data->chip_info->preinit) {
+		ret = data->chip_info->preinit(data);
+		dev_err(dev, "error running preinit tasks");
 		if (ret < 0)
 			return ret;
 	}
-- 
2.39.0

Re: [PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback

[Andy Shevchenko]

On Mon, Dec 26, 2022 at 03:29:21PM +0100, Angel Iglesias wrote:
> Adds preinit callback to execute operations on probe before applying
> initial configuration.

...

> @@ -935,6 +936,7 @@ static const struct bmp280_chip_info bmp280_chip_info = {
>  	.read_temp = bmp280_read_temp,
>  	.read_press = bmp280_read_press,
>  	.read_calib = bmp280_read_calib,
> +	.preinit = NULL,
>  };
>  
>  static int bme280_chip_config(struct bmp280_data *data)
> @@ -979,6 +981,7 @@ static const struct bmp280_chip_info bme280_chip_info = {
>  	.read_press = bmp280_read_press,
>  	.read_humid = bmp280_read_humid,
>  	.read_calib = bme280_read_calib,
> +	.preinit = NULL,
>  };

Useless changes.

...

> @@ -1604,6 +1614,7 @@ static const struct bmp280_chip_info bmp180_chip_info = {
>  	.read_temp = bmp180_read_temp,
>  	.read_press = bmp180_read_press,
>  	.read_calib = bmp180_read_calib,
> +	.preinit = NULL,
>  };

Ditto.

...

> +	/*
> +	 * Some chips like the BMP3xx have preinit tasks to run
> +	 * before applying the initial configuration.
> +	 */
> +	if (data->chip_info->preinit) {
> +		ret = data->chip_info->preinit(data);

> +		dev_err(dev, "error running preinit tasks");

Huh?! I guess you wanted

>  		if (ret < 0)
>  			return ret;

	if (ret < 0)
		return dev_err_probe(...);

>  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback

[Angel Iglesias]

On Tue, 2022-12-27 at 23:41 +0200, Andy Shevchenko wrote:
> On Mon, Dec 26, 2022 at 03:29:21PM +0100, Angel Iglesias wrote:
> > Adds preinit callback to execute operations on probe before applying
> > initial configuration.
> 
> ...
> 
> > @@ -935,6 +936,7 @@ static const struct bmp280_chip_info bmp280_chip_info =
> > {
> >         .read_temp = bmp280_read_temp,
> >         .read_press = bmp280_read_press,
> >         .read_calib = bmp280_read_calib,
> > +       .preinit = NULL,
> >  };
> >  
> >  static int bme280_chip_config(struct bmp280_data *data)
> > @@ -979,6 +981,7 @@ static const struct bmp280_chip_info bme280_chip_info =
> > {
> >         .read_press = bmp280_read_press,
> >         .read_humid = bmp280_read_humid,
> >         .read_calib = bme280_read_calib,
> > +       .preinit = NULL,
> >  };
> 
> Useless changes.
> 
> ...
> 
> > @@ -1604,6 +1614,7 @@ static const struct bmp280_chip_info bmp180_chip_info
> > = {
> >         .read_temp = bmp180_read_temp,
> >         .read_press = bmp180_read_press,
> >         .read_calib = bmp180_read_calib,
> > +       .preinit = NULL,
> >  };
> 
> Ditto.
> 
> ...
> 
> > +       /*
> > +        * Some chips like the BMP3xx have preinit tasks to run
> > +        * before applying the initial configuration.
> > +        */
> > +       if (data->chip_info->preinit) {
> > +               ret = data->chip_info->preinit(data);
> 
> > +               dev_err(dev, "error running preinit tasks");
> 
> Huh?! I guess you wanted

The dangers of copying paste and rushing things etc. Sorry for this brain fart!

Thanks for your time,
Angel

> >                 if (ret < 0)
> >                         return ret;
> 
>         if (ret < 0)
>                 return dev_err_probe(...);
> 
> >         }
> 

Re: [PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback

[Jonathan Cameron]

On Mon, 26 Dec 2022 15:29:21 +0100
Angel Iglesias <ang.iglesiasg@gmail.com> wrote:

> Adds preinit callback to execute operations on probe before applying
> initial configuration.
> 
> 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 46959a91408f..c37cf2caec68 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -217,6 +217,7 @@ struct bmp280_chip_info {
>  	int (*read_press)(struct bmp280_data *, int *, int *);
>  	int (*read_humid)(struct bmp280_data *, int *, int *);
>  	int (*read_calib)(struct bmp280_data *);
> +	int (*preinit)(struct bmp280_data *);
>  };
>  
>  /*
> @@ -935,6 +936,7 @@ static const struct bmp280_chip_info bmp280_chip_info = {
>  	.read_temp = bmp280_read_temp,
>  	.read_press = bmp280_read_press,
>  	.read_calib = bmp280_read_calib,
> +	.preinit = NULL,
C standard guarantees those are set to NULL anyway + the default is obvious.
Hence don't set them to NULL, just leave the automatic initialization of
unspecified structure elements to handle it for you.

>  };
>  
>  static int bme280_chip_config(struct bmp280_data *data)
> @@ -979,6 +981,7 @@ static const struct bmp280_chip_info bme280_chip_info = {
>  	.read_press = bmp280_read_press,
>  	.read_humid = bmp280_read_humid,
>  	.read_calib = bme280_read_calib,
> +	.preinit = NULL,
>  };
>  
>  /*
> @@ -1220,6 +1223,12 @@ static const int bmp380_odr_table[][2] = {
>  	[BMP380_ODR_0_0015HZ]	= {0, 1526},
>  };
>  
> +static int bmp380_preinit(struct bmp280_data *data)
> +{
> +	/* BMP3xx requires soft-reset as part of initialization */
> +	return bmp380_cmd(data, BMP380_CMD_SOFT_RESET);
> +}
> +
>  static int bmp380_chip_config(struct bmp280_data *data)
>  {
>  	bool change = false, aux;
> @@ -1349,6 +1358,7 @@ static const struct bmp280_chip_info bmp380_chip_info = {
>  	.read_temp = bmp380_read_temp,
>  	.read_press = bmp380_read_press,
>  	.read_calib = bmp380_read_calib,
> +	.preinit = bmp380_preinit,
>  };
>  
>  static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
> @@ -1604,6 +1614,7 @@ static const struct bmp280_chip_info bmp180_chip_info = {
>  	.read_temp = bmp180_read_temp,
>  	.read_press = bmp180_read_press,
>  	.read_calib = bmp180_read_calib,
> +	.preinit = NULL,
>  };
>  
>  static irqreturn_t bmp085_eoc_irq(int irq, void *d)
> @@ -1762,9 +1773,13 @@ int bmp280_common_probe(struct device *dev,
>  		return -EINVAL;
>  	}
>  
> -	/* BMP3xx requires soft-reset as part of initialization */
> -	if (chip_id == BMP380_CHIP_ID) {
> -		ret = bmp380_cmd(data, BMP380_CMD_SOFT_RESET);
> +	/*
> +	 * Some chips like the BMP3xx have preinit tasks to run
> +	 * before applying the initial configuration.
> +	 */
I would drop this comment. It's kind of obvious that some devices need you
to call something here - otherwise why have the clearly optional callback?
The specific BMP3xx requirements are well commented in your new callback above
so don't want to be here as well.

> +	if (data->chip_info->preinit) {
> +		ret = data->chip_info->preinit(data);
> +		dev_err(dev, "error running preinit tasks");

Error message printed on success...

>  		if (ret < 0)
>  			return ret;

			return dev_err_probe(dev, ret, "error running preinit tasks");

>  	}

Re: [PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback

[Angel Iglesias]

On Fri, 2022-12-30 at 18:18 +0000, Jonathan Cameron wrote:
> On Mon, 26 Dec 2022 15:29:21 +0100
> Angel Iglesias <ang.iglesiasg@gmail.com> wrote:
> 
> > Adds preinit callback to execute operations on probe before applying
> > initial configuration.
> > 
> > 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 46959a91408f..c37cf2caec68 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -217,6 +217,7 @@ struct bmp280_chip_info {
> >         int (*read_press)(struct bmp280_data *, int *, int *);
> >         int (*read_humid)(struct bmp280_data *, int *, int *);
> >         int (*read_calib)(struct bmp280_data *);
> > +       int (*preinit)(struct bmp280_data *);
> >  };
> >  
> >  /*
> > @@ -935,6 +936,7 @@ static const struct bmp280_chip_info bmp280_chip_info =
> > {
> >         .read_temp = bmp280_read_temp,
> >         .read_press = bmp280_read_press,
> >         .read_calib = bmp280_read_calib,
> > +       .preinit = NULL,
> C standard guarantees those are set to NULL anyway + the default is obvious.
> Hence don't set them to NULL, just leave the automatic initialization of
> unspecified structure elements to handle it for you.

OK! Note to self: compiler knows best!

> >  };
> >  
> >  static int bme280_chip_config(struct bmp280_data *data)
> > @@ -979,6 +981,7 @@ static const struct bmp280_chip_info bme280_chip_info =
> > {
> >         .read_press = bmp280_read_press,
> >         .read_humid = bmp280_read_humid,
> >         .read_calib = bme280_read_calib,
> > +       .preinit = NULL,
> >  };
> >  
> >  /*
> > @@ -1220,6 +1223,12 @@ static const int bmp380_odr_table[][2] = {
> >         [BMP380_ODR_0_0015HZ]   = {0, 1526},
> >  };
> >  
> > +static int bmp380_preinit(struct bmp280_data *data)
> > +{
> > +       /* BMP3xx requires soft-reset as part of initialization */
> > +       return bmp380_cmd(data, BMP380_CMD_SOFT_RESET);
> > +}
> > +
> >  static int bmp380_chip_config(struct bmp280_data *data)
> >  {
> >         bool change = false, aux;
> > @@ -1349,6 +1358,7 @@ static const struct bmp280_chip_info bmp380_chip_info
> > = {
> >         .read_temp = bmp380_read_temp,
> >         .read_press = bmp380_read_press,
> >         .read_calib = bmp380_read_calib,
> > +       .preinit = bmp380_preinit,
> >  };
> >  
> >  static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
> > @@ -1604,6 +1614,7 @@ static const struct bmp280_chip_info bmp180_chip_info
> > = {
> >         .read_temp = bmp180_read_temp,
> >         .read_press = bmp180_read_press,
> >         .read_calib = bmp180_read_calib,
> > +       .preinit = NULL,
> >  };
> >  
> >  static irqreturn_t bmp085_eoc_irq(int irq, void *d)
> > @@ -1762,9 +1773,13 @@ int bmp280_common_probe(struct device *dev,
> >                 return -EINVAL;
> >         }
> >  
> > -       /* BMP3xx requires soft-reset as part of initialization */
> > -       if (chip_id == BMP380_CHIP_ID) {
> > -               ret = bmp380_cmd(data, BMP380_CMD_SOFT_RESET);
> > +       /*
> > +        * Some chips like the BMP3xx have preinit tasks to run
> > +        * before applying the initial configuration.
> > +        */
> I would drop this comment. It's kind of obvious that some devices need you
> to call something here - otherwise why have the clearly optional callback?
> The specific BMP3xx requirements are well commented in your new callback above
> so don't want to be here as well.
> 
> > +       if (data->chip_info->preinit) {
> > +               ret = data->chip_info->preinit(data);
> > +               dev_err(dev, "error running preinit tasks");
> 
> Error message printed on success...

Yup, sorry about that...

> 
> >                 if (ret < 0)
> >                         return ret;
> 
>                         return dev_err_probe(dev, ret, "error running preinit
> tasks");
> 
> >         }
> 
Thanks for your time!
Angel

[PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580

[Angel Iglesias]

Adds compatibility with the new sensor generation, the BMP580.

The measurement and initialization codepaths are adapted from
the device datasheet and the repository from manufacturer at
https://github.com/boschsensortec/BMP5-Sensor-API.

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

diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index c9453389e4f7..1c18e3b2c501 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -17,14 +17,14 @@ config ABP060MG
 	  will be called abp060mg.
 
 config BMP280
-	tristate "Bosch Sensortec BMP180/BMP280/BMP380 pressure sensor I2C driver"
+	tristate "Bosch Sensortec BMP180/BMP280/BMP380/BMP580 pressure sensor I2C driver"
 	depends on (I2C || SPI_MASTER)
 	select REGMAP
 	select BMP280_I2C if (I2C)
 	select BMP280_SPI if (SPI_MASTER)
 	help
-	  Say yes here to build support for Bosch Sensortec BMP180, BMP280 and
-	  BMP380 pressure and temperature sensors. Also supports the BME280 with
+	  Say yes here to build support for Bosch Sensortec BMP180, BMP280, BMP380
+	  and BMP580 pressure and temperature sensors. Also supports the BME280 with
 	  an additional humidity sensor channel.
 
 	  To compile this driver as a module, choose M here: the core module
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index c37cf2caec68..44901c6eb2f9 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-bmp581-ds004.pdf
  *
  * Notice:
  * The link to the bmp180 datasheet points to an outdated version missing these changes:
@@ -130,6 +131,41 @@ enum bmp380_odr {
 	BMP380_ODR_0_0015HZ,
 };
 
+enum bmp580_odr {
+	BMP580_ODR_240HZ,
+	BMP580_ODR_218HZ,
+	BMP580_ODR_199HZ,
+	BMP580_ODR_179HZ,
+	BMP580_ODR_160HZ,
+	BMP580_ODR_149HZ,
+	BMP580_ODR_140HZ,
+	BMP580_ODR_129HZ,
+	BMP580_ODR_120HZ,
+	BMP580_ODR_110HZ,
+	BMP580_ODR_100HZ,
+	BMP580_ODR_89HZ,
+	BMP580_ODR_80HZ,
+	BMP580_ODR_70HZ,
+	BMP580_ODR_60HZ,
+	BMP580_ODR_50HZ,
+	BMP580_ODR_45HZ,
+	BMP580_ODR_40HZ,
+	BMP580_ODR_35HZ,
+	BMP580_ODR_30HZ,
+	BMP580_ODR_25HZ,
+	BMP580_ODR_20HZ,
+	BMP580_ODR_15HZ,
+	BMP580_ODR_10HZ,
+	BMP580_ODR_5HZ,
+	BMP580_ODR_4HZ,
+	BMP580_ODR_3HZ,
+	BMP580_ODR_2HZ,
+	BMP580_ODR_1HZ,
+	BMP580_ODR_0_5HZ,
+	BMP580_ODR_0_25HZ,
+	BMP580_ODR_0_125HZ,
+};
+
 struct bmp280_data {
 	struct device *dev;
 	struct mutex lock;
@@ -1361,6 +1397,399 @@ static const struct bmp280_chip_info bmp380_chip_info = {
 	.preinit = bmp380_preinit,
 };
 
+enum bmp580_commands {
+	BMP580_SOFT_RESET_CMD,
+	BMP580_NVM_WRITE_CMD,
+	BMP580_NVM_READ_CMD,
+	BMP580_EXT_MODE_CMD,
+};
+
+/*
+ * Helper function to send a command to BMP5XX sensors.
+ *
+ * BMP5xx sensors have a series of commands actionable
+ * writing specific sequences on the CMD register:
+ * SOFT_RESET: performs a reset of the system.
+ * NVM_READ: read the contents of a user position of the nvm memory.
+ * NVM_WRITE: write new data to a user position of the nvm memory.
+ * EXT_MODE: enable extended mode with additional debug pages.
+ */
+static int bmp580_cmd(struct bmp280_data *data, enum bmp580_commands cmd)
+{
+	unsigned long deadline;
+	unsigned int reg;
+	int ret;
+
+	switch (cmd) {
+	case BMP580_SOFT_RESET_CMD:
+		/* Send reset word */
+		ret = regmap_write(data->regmap, BMP580_REG_CMD, BMP580_CMD_SOFT_RESET);
+		if (ret) {
+			dev_err(data->dev, "failed to send reset command to device\n");
+			return ret;
+		}
+		/* Wait 2ms for reset completion */
+		usleep_range(2000, 2500);
+		/* Dummy read of chip_id */
+		ret = regmap_read(data->regmap, BMP580_REG_CHIP_ID, &reg);
+		if (ret) {
+			dev_err(data->dev, "failed to reestablish comms after reset\n");
+			return ret;
+		}
+		/* Check if POR bit is set on interrupt reg */
+		ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &reg);
+		if (ret) {
+			dev_err(data->dev, "error reading interrupt status register\n");
+			return ret;
+		}
+		if (!(reg & BMP580_INT_STATUS_POR_MASK)) {
+			dev_err(data->dev, "error resetting sensor\n");
+			return -EINVAL;
+		}
+		break;
+	case BMP580_NVM_WRITE_CMD:
+	case BMP580_NVM_READ_CMD:
+		/* Check nvm ready flag */
+		ret = regmap_read(data->regmap, BMP580_REG_STATUS, &reg);
+		if (ret) {
+			dev_err(data->dev, "failed to check nvm status\n");
+			return ret;
+		}
+		if (!(reg & BMP580_STATUS_NVM_RDY_MASK)) {
+			dev_err(data->dev, "sensor's nvm is not ready\n");
+			return -EIO;
+		}
+		/* Send NVM operation sequence */
+		ret = regmap_write(data->regmap, BMP580_REG_CMD, BMP580_CMD_NVM_OP_SEQ_0);
+		if (ret) {
+			dev_err(data->dev, "failed to send nvm operation's first sequence\n");
+			return ret;
+		}
+		if (cmd == BMP580_NVM_WRITE_CMD) {
+			/* Send write sequence */
+			ret = regmap_write(data->regmap, BMP580_REG_CMD,
+					   BMP580_CMD_NVM_WRITE_SEQ_1);
+			if (ret) {
+				dev_err(data->dev, "failed to send nvm write sequence\n");
+				return ret;
+			}
+			/* Datasheet says on 4.8.1.2 it takes approximately 10ms */
+			usleep_range(10000, 10500);
+			deadline = jiffies + msecs_to_jiffies(10);
+		} else {
+			/* Send read sequence */
+			ret = regmap_write(data->regmap, BMP580_REG_CMD,
+					   BMP580_CMD_NVM_READ_SEQ_1);
+			if (ret) {
+				dev_err(data->dev, "failed to send nvm read sequence\n");
+				return ret;
+			}
+			/* Datasheet says on 4.8.1.1 it takes approximately 200us */
+			usleep_range(200, 250);
+			deadline = jiffies + usecs_to_jiffies(200);
+		}
+		if (ret) {
+			dev_err(data->dev, "failed to write command sequence\n");
+			return -EIO;
+		}
+		/* Wait until NVM is ready again */
+		do {
+			ret = regmap_read(data->regmap, BMP580_REG_STATUS, &reg);
+			if (ret) {
+				dev_err(data->dev, "failed to check nvm status\n");
+				reg &= ~BMP580_STATUS_NVM_RDY_MASK;
+			}
+		} while (time_before(jiffies, deadline) && !(reg & BMP580_STATUS_NVM_RDY_MASK));
+
+		if (!(reg & BMP580_STATUS_NVM_RDY_MASK)) {
+			dev_err(data->dev,
+				"reached timeout waiting for nvm operation completion\n");
+			return -ETIMEDOUT;
+		}
+		/* Checks nvm error flags */
+		if ((reg & BMP580_STATUS_NVM_ERR_MASK) || (reg & BMP580_STATUS_NVM_CMD_ERR_MASK)) {
+			dev_err(data->dev, "error processing nvm operation\n");
+			return -EIO;
+		}
+		break;
+	case BMP580_EXT_MODE_CMD:
+		ret = regmap_write(data->regmap, BMP580_REG_CMD, BMP580_CMD_EXTMODE_SEQ_0);
+		if (ret) {
+			dev_err(data->dev, "failed to send ext_mode first sequence\n");
+			return ret;
+		}
+		ret = regmap_write(data->regmap, BMP580_REG_CMD, BMP580_CMD_EXTMODE_SEQ_1);
+		if (ret) {
+			dev_err(data->dev, "failed to send ext_mode second sequence\n");
+			return ret;
+		}
+		ret = regmap_write(data->regmap, BMP580_REG_CMD, BMP580_CMD_EXTMODE_SEQ_2);
+		if (ret) {
+			dev_err(data->dev, "failed to send ext_mode second sequence\n");
+			return ret;
+		}
+		break;
+	}
+
+	return 0;
+}
+
+/*
+ * Contrary to previous sensors families, compensation algorithm is builtin.
+ * We are only required to read the register raw data and adapt the ranges
+ * for what is expected on IIO ABI.
+ */
+
+static int bmp580_read_temp(struct bmp280_data *data, int *val)
+{
+	s32 raw_temp;
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf,
+			       sizeof(data->buf));
+	if (ret) {
+		dev_err(data->dev, "failed to read temperature\n");
+		return ret;
+	}
+
+	raw_temp = get_unaligned_le24(data->buf);
+	if (raw_temp == BMP580_TEMP_SKIPPED) {
+		dev_err(data->dev, "reading temperature skipped\n");
+		return -EIO;
+	}
+
+	/*
+	 * Temperature is returned in Celsius degrees in fractional
+	 * form down 2^16. We reescale by x1000 to return milli Celsius
+	 * to respect IIO ABI.
+	 */
+	*val = (raw_temp * 1000) >> 16;
+	return IIO_VAL_INT;
+}
+
+static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
+{
+	u32 raw_press;
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf,
+			       sizeof(data->buf));
+	if (ret) {
+		dev_err(data->dev, "failed to read pressure\n");
+		return ret;
+	}
+
+	raw_press = get_unaligned_le24(data->buf);
+	if (raw_press == BMP580_PRESS_SKIPPED) {
+		dev_err(data->dev, "reading pressure skipped\n");
+		return -EIO;
+	}
+	/*
+	 * Pressure is returned in Pascals in fractional form down 2^16.
+	 * We reescale /1000 to convert to kilopascal to respect IIO ABI.
+	 */
+	*val = raw_press;
+	*val2 = 64000; // 2^6 * 1000
+	return IIO_VAL_FRACTIONAL;
+}
+
+static const int bmp580_odr_table[][2] = {
+	[BMP580_ODR_240HZ] =	{240, 0},
+	[BMP580_ODR_218HZ] =	{218, 0},
+	[BMP580_ODR_199HZ] =	{199, 0},
+	[BMP580_ODR_179HZ] =	{179, 0},
+	[BMP580_ODR_160HZ] =	{160, 0},
+	[BMP580_ODR_149HZ] =	{149, 0},
+	[BMP580_ODR_140HZ] =	{140, 0},
+	[BMP580_ODR_129HZ] =	{129, 0},
+	[BMP580_ODR_120HZ] =	{120, 0},
+	[BMP580_ODR_110HZ] =	{110, 0},
+	[BMP580_ODR_100HZ] =	{100, 0},
+	[BMP580_ODR_89HZ] =	{89, 0},
+	[BMP580_ODR_80HZ] =	{80, 0},
+	[BMP580_ODR_70HZ] =	{70, 0},
+	[BMP580_ODR_60HZ] =	{60, 0},
+	[BMP580_ODR_50HZ] =	{50, 0},
+	[BMP580_ODR_45HZ] =	{45, 0},
+	[BMP580_ODR_40HZ] =	{40, 0},
+	[BMP580_ODR_35HZ] =	{35, 0},
+	[BMP580_ODR_30HZ] =	{30, 0},
+	[BMP580_ODR_25HZ] =	{25, 0},
+	[BMP580_ODR_20HZ] =	{20, 0},
+	[BMP580_ODR_15HZ] =	{15, 0},
+	[BMP580_ODR_10HZ] =	{10, 0},
+	[BMP580_ODR_5HZ] =	{5, 0},
+	[BMP580_ODR_4HZ] =	{4, 0},
+	[BMP580_ODR_3HZ] =	{3, 0},
+	[BMP580_ODR_2HZ] =	{2, 0},
+	[BMP580_ODR_1HZ] =	{1, 0},
+	[BMP580_ODR_0_5HZ] =	{0, 500000},
+	[BMP580_ODR_0_25HZ] =	{0, 250000},
+	[BMP580_ODR_0_125HZ] =	{0, 125000},
+};
+
+static int bmp580_preinit(struct bmp280_data *data)
+{
+	unsigned int reg;
+	int ret;
+
+	/* Issue soft-reset command */
+	ret = bmp580_cmd(data, BMP580_SOFT_RESET_CMD);
+	if (ret)
+		return ret;
+	/* Post powerup sequence */
+	ret = regmap_read(data->regmap, BMP580_REG_CHIP_ID, &reg);
+	if (ret)
+		return ret;
+	if (reg != BMP580_CHIP_ID) {
+		dev_err(data->dev, "preinit: unexpected chip_id\n");
+		return -EINVAL;
+	}
+	ret = regmap_read(data->regmap, BMP580_REG_STATUS, &reg);
+	if (ret)
+		return ret;
+	/* Check nvm status */
+	if (!(reg & BMP580_STATUS_NVM_RDY_MASK) || (reg & BMP580_STATUS_NVM_ERR_MASK)) {
+		dev_err(data->dev, "preinit: nvm error on powerup sequence\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int bmp580_chip_config(struct bmp280_data *data)
+{
+	bool change = false, aux;
+	unsigned int tmp;
+	u8 reg_val;
+	int ret;
+
+	/* Sets sensor in standby mode */
+	ret = regmap_update_bits(data->regmap, BMP580_REG_ODR_CONFIG,
+				 BMP580_MODE_MASK | BMP580_ODR_DEEPSLEEP_DIS,
+				 BMP580_ODR_DEEPSLEEP_DIS |
+				 FIELD_PREP(BMP580_MODE_MASK, BMP580_MODE_SLEEP));
+	if (ret) {
+		dev_err(data->dev, "failed to change sensor to standby mode\n");
+		return ret;
+	}
+	/* From datasheet's table 4: electrical characteristics */
+	usleep_range(2500, 3000);
+
+	/* Set default DSP mode settings */
+	reg_val = FIELD_PREP(BMP580_DSP_COMP_MASK, BMP580_DSP_PRESS_TEMP_COMP_EN) |
+		  BMP580_DSP_SHDW_IIR_TEMP_EN | BMP580_DSP_SHDW_IIR_PRESS_EN;
+
+	ret = regmap_update_bits(data->regmap, BMP580_REG_DSP_CONFIG,
+				 BMP580_DSP_COMP_MASK |
+				 BMP580_DSP_SHDW_IIR_TEMP_EN |
+				 BMP580_DSP_SHDW_IIR_PRESS_EN, reg_val);
+
+	/* Configure oversampling */
+	reg_val = FIELD_PREP(BMP580_OSR_TEMP_MASK, data->oversampling_temp) |
+		  FIELD_PREP(BMP580_OSR_PRESS_MASK, data->oversampling_press) |
+		  BMP580_OSR_PRESS_EN;
+
+	ret = regmap_update_bits_check(data->regmap, BMP580_REG_OSR_CONFIG,
+				       BMP580_OSR_TEMP_MASK | BMP580_OSR_PRESS_MASK |
+				       BMP580_OSR_PRESS_EN,
+				       reg_val, &aux);
+	if (ret) {
+		dev_err(data->dev, "failed to write oversampling register\n");
+		return ret;
+	}
+	change = change || aux;
+
+	/* Configure output data rate */
+	ret = regmap_update_bits_check(data->regmap, BMP580_REG_ODR_CONFIG, BMP580_ODR_MASK,
+				       FIELD_PREP(BMP580_ODR_MASK, data->sampling_freq),
+				       &aux);
+	if (ret) {
+		dev_err(data->dev, "failed to write ODR configuration register\n");
+		return ret;
+	}
+	change = change || aux;
+
+	/* Set filter data */
+	reg_val = FIELD_PREP(BMP580_DSP_IIR_PRESS_MASK, data->iir_filter_coeff) |
+		  FIELD_PREP(BMP580_DSP_IIR_TEMP_MASK, data->iir_filter_coeff);
+
+	ret = regmap_update_bits_check(data->regmap, BMP580_REG_DSP_IIR,
+				       BMP580_DSP_IIR_PRESS_MASK |
+				       BMP580_DSP_IIR_TEMP_MASK,
+				       reg_val, &aux);
+	if (ret) {
+		dev_err(data->dev, "failed to write config register\n");
+		return ret;
+	}
+	change = change || aux;
+
+	/* Restore sensor to normal operation mode */
+	ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
+				BMP580_MODE_MASK,
+				FIELD_PREP(BMP580_MODE_MASK, BMP580_MODE_NORMAL));
+	if (ret) {
+		dev_err(data->dev, "failed to set normal mode\n");
+		return ret;
+	}
+	/* From datasheet's table 4: electrical characteristics */
+	usleep_range(3000, 3500);
+
+	if (change) {
+		/*
+		 * Check if ODR and OSR settings are valid or we are
+		 * operating in a degraded mode.
+		 */
+		ret = regmap_read(data->regmap, BMP580_REG_EFF_OSR, &tmp);
+		if (ret) {
+			dev_err(data->dev, "error reading effective OSR register\n");
+			return ret;
+		}
+		if (!(tmp & BMP580_EFF_OSR_VALID_ODR)) {
+			dev_warn(data->dev, "OSR and ODR incompatible settings detected\n");
+			/* Set current OSR settings from data on effective OSR */
+			data->oversampling_temp = FIELD_GET(BMP580_EFF_OSR_TEMP_MASK, tmp);
+			data->oversampling_press = FIELD_GET(BMP580_EFF_OSR_PRESS_MASK, tmp);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
+static const int bmp580_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
+
+static const struct bmp280_chip_info bmp580_chip_info = {
+	.id_reg = BMP580_REG_CHIP_ID,
+	.chip_id = BMP580_CHIP_ID,
+	.start_up_time = 2000,
+	.channels = bmp380_channels,
+	.num_channels = 2,
+
+	.oversampling_temp_avail = bmp580_oversampling_avail,
+	.num_oversampling_temp_avail = ARRAY_SIZE(bmp580_oversampling_avail),
+	.oversampling_temp_default = ilog2(1),
+
+	.oversampling_press_avail = bmp580_oversampling_avail,
+	.num_oversampling_press_avail = ARRAY_SIZE(bmp580_oversampling_avail),
+	.oversampling_press_default = ilog2(4),
+
+	.sampling_freq_avail = bmp580_odr_table,
+	.num_sampling_freq_avail = ARRAY_SIZE(bmp580_odr_table) * 2,
+	.sampling_freq_default = BMP580_ODR_50HZ,
+
+	.iir_filter_coeffs_avail = bmp580_iir_filter_coeffs_avail,
+	.num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp580_iir_filter_coeffs_avail),
+	.iir_filter_coeff_default = 2,
+
+	.chip_config = bmp580_chip_config,
+	.read_temp = bmp580_read_temp,
+	.read_press = bmp580_read_press,
+	.read_calib = NULL,
+	.preinit = bmp580_preinit,
+};
+
 static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
 {
 	const int conversion_time_max[] = { 4500, 7500, 13500, 25500 };
@@ -1713,6 +2142,9 @@ int bmp280_common_probe(struct device *dev,
 	case BMP380:
 		chip_info = &bmp380_chip_info;
 		break;
+	case BMP580:
+		chip_info = &bmp580_chip_info;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -1779,9 +2211,10 @@ int bmp280_common_probe(struct device *dev,
 	 */
 	if (data->chip_info->preinit) {
 		ret = data->chip_info->preinit(data);
-		dev_err(dev, "error running preinit tasks");
-		if (ret < 0)
+		if (ret) {
+			dev_err(dev, "error running preinit tasks\n");
 			return ret;
+		}
 	}
 
 	ret = data->chip_info->chip_config(data);
@@ -1795,11 +2228,12 @@ int bmp280_common_probe(struct device *dev,
 	 * non-volatile memory during production". Let's read them out at probe
 	 * time once. They will not change.
 	 */
-
-	ret = data->chip_info->read_calib(data);
-	if (ret < 0)
-		return dev_err_probe(data->dev, ret,
-				     "failed to read calibration coefficients\n");
+	if (data->chip_info->read_calib) {
+		ret = data->chip_info->read_calib(data);
+		if (ret < 0)
+			return dev_err_probe(data->dev, ret,
+					     "failed to read calibration coefficients\n");
+	}
 
 	/*
 	 * Attempt to grab an optional EOC IRQ - only the BMP085 has this
diff --git a/drivers/iio/pressure/bmp280-i2c.c b/drivers/iio/pressure/bmp280-i2c.c
index 59921e8cd592..c52d2b477bb7 100644
--- a/drivers/iio/pressure/bmp280-i2c.c
+++ b/drivers/iio/pressure/bmp280-i2c.c
@@ -22,6 +22,9 @@ static int bmp280_i2c_probe(struct i2c_client *client)
 	case BMP380:
 		regmap_config = &bmp380_regmap_config;
 		break;
+	case BMP580:
+		regmap_config = &bmp580_regmap_config;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -45,6 +48,7 @@ static const struct of_device_id bmp280_of_i2c_match[] = {
 	{ .compatible = "bosch,bmp280", .data = (void *)BMP280 },
 	{ .compatible = "bosch,bme280", .data = (void *)BME280 },
 	{ .compatible = "bosch,bmp380", .data = (void *)BMP380 },
+	{ .compatible = "bosch,bmp580", .data = (void *)BMP580 },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, bmp280_of_i2c_match);
@@ -55,6 +59,7 @@ static const struct i2c_device_id bmp280_i2c_id[] = {
 	{"bmp280", BMP280 },
 	{"bme280", BME280 },
 	{"bmp380", BMP380 },
+	{"bmp580", BMP580 },
 	{ },
 };
 MODULE_DEVICE_TABLE(i2c, bmp280_i2c_id);
diff --git a/drivers/iio/pressure/bmp280-regmap.c b/drivers/iio/pressure/bmp280-regmap.c
index c98c67970265..3ee56720428c 100644
--- a/drivers/iio/pressure/bmp280-regmap.c
+++ b/drivers/iio/pressure/bmp280-regmap.c
@@ -115,6 +115,54 @@ static bool bmp380_is_volatile_reg(struct device *dev, unsigned int reg)
 	}
 }
 
+static bool bmp580_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case BMP580_REG_NVM_DATA_MSB:
+	case BMP580_REG_NVM_DATA_LSB:
+	case BMP580_REG_NVM_ADDR:
+	case BMP580_REG_ODR_CONFIG:
+	case BMP580_REG_OSR_CONFIG:
+	case BMP580_REG_INT_SOURCE:
+	case BMP580_REG_INT_CONFIG:
+	case BMP580_REG_OOR_THR_MSB:
+	case BMP580_REG_OOR_THR_LSB:
+	case BMP580_REG_OOR_CONFIG:
+	case BMP580_REG_OOR_RANGE:
+	case BMP580_REG_IF_CONFIG:
+	case BMP580_REG_FIFO_CONFIG:
+	case BMP580_REG_FIFO_SEL:
+	case BMP580_REG_DSP_CONFIG:
+	case BMP580_REG_DSP_IIR:
+	case BMP580_REG_CMD:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool bmp580_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case BMP580_REG_NVM_DATA_MSB:
+	case BMP580_REG_NVM_DATA_LSB:
+	case BMP580_REG_FIFO_COUNT:
+	case BMP580_REG_INT_STATUS:
+	case BMP580_REG_PRESS_XLSB:
+	case BMP580_REG_PRESS_LSB:
+	case BMP580_REG_PRESS_MSB:
+	case BMP580_REG_FIFO_DATA:
+	case BMP580_REG_TEMP_XLSB:
+	case BMP580_REG_TEMP_LSB:
+	case BMP580_REG_TEMP_MSB:
+	case BMP580_REG_EFF_OSR:
+	case BMP580_REG_STATUS:
+		return true;
+	default:
+		return false;
+	}
+}
+
 const struct regmap_config bmp280_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -138,3 +186,15 @@ const struct regmap_config bmp380_regmap_config = {
 	.volatile_reg = bmp380_is_volatile_reg,
 };
 EXPORT_SYMBOL_NS(bmp380_regmap_config, IIO_BMP280);
+
+const struct regmap_config bmp580_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = BMP580_REG_CMD,
+	.cache_type = REGCACHE_RBTREE,
+
+	.writeable_reg = bmp580_is_writeable_reg,
+	.volatile_reg = bmp580_is_volatile_reg,
+};
+EXPORT_SYMBOL_NS(bmp580_regmap_config, IIO_BMP280);
diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
index 4a2df5b5d838..5653c3c33081 100644
--- a/drivers/iio/pressure/bmp280-spi.c
+++ b/drivers/iio/pressure/bmp280-spi.c
@@ -69,6 +69,9 @@ static int bmp280_spi_probe(struct spi_device *spi)
 	case BMP380:
 		regmap_config = &bmp380_regmap_config;
 		break;
+	case BMP580:
+		regmap_config = &bmp580_regmap_config;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -96,6 +99,7 @@ static const struct of_device_id bmp280_of_spi_match[] = {
 	{ .compatible = "bosch,bmp280", },
 	{ .compatible = "bosch,bme280", },
 	{ .compatible = "bosch,bmp380", },
+	{ .compatible = "bosch,bmp580", },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, bmp280_of_spi_match);
@@ -106,6 +110,7 @@ static const struct spi_device_id bmp280_spi_id[] = {
 	{ "bmp280", BMP280 },
 	{ "bme280", BME280 },
 	{ "bmp380", BMP380 },
+	{ "bmp580", BMP580 },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, bmp280_spi_id);
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index efc31bc84708..27d2abc17d01 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -3,6 +3,107 @@
 #include <linux/device.h>
 #include <linux/regmap.h>
 
+/* BMP580 specific registers */
+#define BMP580_REG_CMD			0x7E
+#define BMP580_REG_EFF_OSR		0x38
+#define BMP580_REG_ODR_CONFIG		0x37
+#define BMP580_REG_OSR_CONFIG		0x36
+#define BMP580_REG_IF_CONFIG		0x13
+#define BMP580_REG_REV_ID		0x02
+#define BMP580_REG_CHIP_ID		0x01
+/* OOR allows to configure a pressure alarm */
+#define BMP580_REG_OOR_CONFIG		0x35
+#define BMP580_REG_OOR_RANGE		0x34
+#define BMP580_REG_OOR_THR_MSB		0x33
+#define BMP580_REG_OOR_THR_LSB		0x32
+/* DSP registers (IIR filters) */
+#define BMP580_REG_DSP_IIR		0x31
+#define BMP580_REG_DSP_CONFIG		0x30
+/* NVM access registers */
+#define BMP580_REG_NVM_DATA_MSB		0x2D
+#define BMP580_REG_NVM_DATA_LSB		0x2C
+#define BMP580_REG_NVM_ADDR		0x2B
+/* Status registers */
+#define BMP580_REG_STATUS		0x28
+#define BMP580_REG_INT_STATUS		0x27
+#define BMP580_REG_CHIP_STATUS		0x11
+/* Data registers */
+#define BMP580_REG_FIFO_DATA		0x29
+#define BMP580_REG_PRESS_MSB		0x22
+#define BMP580_REG_PRESS_LSB		0x21
+#define BMP580_REG_PRESS_XLSB		0x20
+#define BMP580_REG_TEMP_MSB		0x1F
+#define BMP580_REG_TEMP_LSB		0x1E
+#define BMP580_REG_TEMP_XLSB		0x1D
+/* FIFO config registers */
+#define BMP580_REG_FIFO_SEL		0x18
+#define BMP580_REG_FIFO_COUNT		0x17
+#define BMP580_REG_FIFO_CONFIG		0x16
+/* Interruptions config registers */
+#define BMP580_REG_INT_SOURCE		0x15
+#define BMP580_REG_INT_CONFIG		0x14
+
+#define BMP580_CMD_NOOP			0x00
+#define BMP580_CMD_EXTMODE_SEQ_0	0x73
+#define BMP580_CMD_EXTMODE_SEQ_1	0xB4
+#define BMP580_CMD_EXTMODE_SEQ_2	0x69
+#define BMP580_CMD_NVM_OP_SEQ_0		0x5D
+#define BMP580_CMD_NVM_READ_SEQ_1	0xA5
+#define BMP580_CMD_NVM_WRITE_SEQ_1	0xA0
+#define BMP580_CMD_SOFT_RESET		0xB6
+
+#define BMP580_INT_STATUS_POR_MASK	BIT(4)
+
+#define BMP580_STATUS_CORE_RDY_MASK	BIT(0)
+#define BMP580_STATUS_NVM_RDY_MASK	BIT(1)
+#define BMP580_STATUS_NVM_ERR_MASK	BIT(2)
+#define BMP580_STATUS_NVM_CMD_ERR_MASK	BIT(3)
+
+#define BMP580_OSR_PRESS_MASK		GENMASK(5, 3)
+#define BMP580_OSR_TEMP_MASK		GENMASK(2, 0)
+#define BMP580_OSR_PRESS_EN		BIT(6)
+#define BMP580_EFF_OSR_PRESS_MASK	GENMASK(5, 3)
+#define BMP580_EFF_OSR_TEMP_MASK	GENMASK(2, 0)
+#define BMP580_EFF_OSR_VALID_ODR	BIT(7)
+
+#define BMP580_ODR_MASK			GENMASK(6, 2)
+#define BMP580_MODE_MASK		GENMASK(1, 0)
+#define BMP580_MODE_SLEEP		0
+#define BMP580_MODE_NORMAL		1
+#define BMP580_MODE_FORCED		2
+#define BMP580_MODE_CONTINOUS		3
+#define BMP580_ODR_DEEPSLEEP_DIS	BIT(7)
+
+#define BMP580_DSP_COMP_MASK		GENMASK(1, 0)
+#define BMP580_DSP_COMP_DIS		0
+#define BMP580_DSP_TEMP_COMP_EN		1
+/*
+ * In section 7.27 of datasheet, modes 2 and 3 are technically the same.
+ * Pressure compensation means also enabling temperature compensation
+ */
+#define BMP580_DSP_PRESS_COMP_EN	2
+#define BMP580_DSP_PRESS_TEMP_COMP_EN	3
+#define BMP580_DSP_IIR_FORCED_FLUSH	BIT(2)
+#define BMP580_DSP_SHDW_IIR_TEMP_EN	BIT(3)
+#define BMP580_DSP_FIFO_IIR_TEMP_EN	BIT(4)
+#define BMP580_DSP_SHDW_IIR_PRESS_EN	BIT(5)
+#define BMP580_DSP_FIFO_IIR_PRESS_EN	BIT(6)
+#define BMP580_DSP_OOR_IIR_PRESS_EN	BIT(7)
+
+#define BMP580_DSP_IIR_PRESS_MASK	GENMASK(5, 3)
+#define BMP580_DSP_IIR_TEMP_MASK	GENMASK(2, 0)
+#define BMP580_FILTER_OFF		0
+#define BMP580_FILTER_1X		1
+#define BMP580_FILTER_3X		2
+#define BMP580_FILTER_7X		3
+#define BMP580_FILTER_15X		4
+#define BMP580_FILTER_31X		5
+#define BMP580_FILTER_63X		6
+#define BMP580_FILTER_127X		7
+
+#define BMP580_TEMP_SKIPPED		0x7f7f7f
+#define BMP580_PRESS_SKIPPED		0x7f7f7f
+
 /* BMP380 specific registers */
 #define BMP380_REG_CMD			0x7E
 #define BMP380_REG_CONFIG		0x1F
@@ -180,6 +281,7 @@
 #define BMP280_REG_RESET		0xE0
 #define BMP280_REG_ID			0xD0
 
+#define BMP580_CHIP_ID			0x50
 #define BMP380_CHIP_ID			0x50
 #define BMP180_CHIP_ID			0x55
 #define BMP280_CHIP_ID			0x58
@@ -197,12 +299,14 @@ enum bmp280_variant {
 	BMP280,
 	BME280,
 	BMP380,
+	BMP580,
 };
 
 /* Regmap configurations */
 extern const struct regmap_config bmp180_regmap_config;
 extern const struct regmap_config bmp280_regmap_config;
 extern const struct regmap_config bmp380_regmap_config;
+extern const struct regmap_config bmp580_regmap_config;
 
 /* Probe called from different transports */
 int bmp280_common_probe(struct device *dev,
-- 
2.39.0

Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580

[Christophe JAILLET]

Le 26/12/2022 à 15:29, Angel Iglesias a écrit :
> Adds compatibility with the new sensor generation, the BMP580.
> 
> The measurement and initialization codepaths are adapted from
> the device datasheet and the repository from manufacturer at
> https://github.com/boschsensortec/BMP5-Sensor-API.
> 
> Signed-off-by: Angel Iglesias <ang.iglesiasg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 

[...]

> diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> index efc31bc84708..27d2abc17d01 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h

[...]

> +#define BMP580_CHIP_ID			0x50
>   #define BMP380_CHIP_ID			0x50

Hi,

this is maybe correct (I've not been able to find the datasheet to check 
myself), but it looks odd to have the same ID for 2 different chips.

CJ

>   #define BMP180_CHIP_ID			0x55
>   #define BMP280_CHIP_ID			0x58

Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580

[Angel Iglesias]

On Thu, 2022-12-29 at 18:35 +0100, Christophe JAILLET wrote:
> Le 26/12/2022 à 15:29, Angel Iglesias a écrit :
> > Adds compatibility with the new sensor generation, the BMP580.
> > 
> > The measurement and initialization codepaths are adapted from
> > the device datasheet and the repository from manufacturer at
> > https://github.com/boschsensortec/BMP5-Sensor-API.
> > 
> > Signed-off-by: Angel Iglesias
> > <ang.iglesiasg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > 
> 
> [...]
> 
> > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > index efc31bc84708..27d2abc17d01 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> 
> [...]
> 
> > +#define BMP580_CHIP_ID                 0x50
> >   #define BMP380_CHIP_ID                        0x50
> 
> Hi,
> 
> this is maybe correct (I've not been able to find the datasheet to check 
> myself), but it looks odd to have the same ID for 2 different chips.

Yes, I also couldn't find a datasheet for the BMP580 or a devkit anywhere. I'm
developing this using the BMP581, which seems to be a variant almost identical.
Something similar happened with the BMP38x; you could find the BMP384 and the
BMP388, but the BMP380 was unavailable everywhere, datasheet included. My guess
is this is a similar situation. In any case, the datasheet of the BMP581 is
available here:
https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp581-ds004.pdf

Regarding the chip id being the same between generations is weird, but at least
the datasheet and the sensor I have uses 0x50 as the chip id. After you
mentioned this, I checked back on the reference API repository from Bosch and it
has both 0x50 and 0x51 as valid IDs:
* https://github.com/boschsensortec/BMP5-Sensor-API/blob/master/bmp5_defs.h#L198
* https://github.com/boschsensortec/BMP5-Sensor-API/blob/master/bmp5.c#L1444

Angel

> CJ
> 
> >   #define BMP180_CHIP_ID                        0x55
> >   #define BMP280_CHIP_ID                        0x58
> 

Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580

[Jonathan Cameron]

On Thu, 29 Dec 2022 19:23:16 +0100
Angel Iglesias <ang.iglesiasg@gmail.com> wrote:

> On Thu, 2022-12-29 at 18:35 +0100, Christophe JAILLET wrote:
> > Le 26/12/2022 à 15:29, Angel Iglesias a écrit :  
> > > Adds compatibility with the new sensor generation, the BMP580.
> > > 
> > > The measurement and initialization codepaths are adapted from
> > > the device datasheet and the repository from manufacturer at
> > > https://github.com/boschsensortec/BMP5-Sensor-API.
> > > 
> > > Signed-off-by: Angel Iglesias
> > > <ang.iglesiasg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > >   
> > 
> > [...]
> >   
> > > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > > index efc31bc84708..27d2abc17d01 100644
> > > --- a/drivers/iio/pressure/bmp280.h
> > > +++ b/drivers/iio/pressure/bmp280.h  
> > 
> > [...]
> >   
> > > +#define BMP580_CHIP_ID                 0x50
> > >   #define BMP380_CHIP_ID                        0x50  
> > 
> > Hi,
> > 
> > this is maybe correct (I've not been able to find the datasheet to check 
> > myself), but it looks odd to have the same ID for 2 different chips.  
> 
> Yes, I also couldn't find a datasheet for the BMP580 or a devkit anywhere. I'm
> developing this using the BMP581, which seems to be a variant almost identical.
> Something similar happened with the BMP38x; you could find the BMP384 and the
> BMP388, but the BMP380 was unavailable everywhere, datasheet included. My guess
> is this is a similar situation. In any case, the datasheet of the BMP581 is
> available here:
> https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp581-ds004.pdf
> 
> Regarding the chip id being the same between generations is weird, but at least
> the datasheet and the sensor I have uses 0x50 as the chip id. After you
> mentioned this, I checked back on the reference API repository from Bosch and it
> has both 0x50 and 0x51 as valid IDs:
> * https://github.com/boschsensortec/BMP5-Sensor-API/blob/master/bmp5_defs.h#L198
> * https://github.com/boschsensortec/BMP5-Sensor-API/blob/master/bmp5.c#L1444
https://github.com/boschsensortec/BMP3-Sensor-API/blob/master/bmp3_defs.h
I was curious on whether we had a wrong value for bmp380, but nope... Same ID.

Huh. As per earlier comment - who wants to moan at Bosch as this is crazy situation?

Jonathan


> 
> Angel
> 
> > CJ
> >   
> > >   #define BMP180_CHIP_ID                        0x55
> > >   #define BMP280_CHIP_ID                        0x58  
> >   
> 

Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580

[Angel Iglesias]

On Fri, 2022-12-30 at 18:22 +0000, Jonathan Cameron wrote:
> On Thu, 29 Dec 2022 19:23:16 +0100
> Angel Iglesias <ang.iglesiasg@gmail.com> wrote:
> 
> > On Thu, 2022-12-29 at 18:35 +0100, Christophe JAILLET wrote:
> > > Le 26/12/2022 à 15:29, Angel Iglesias a écrit :  
> > > > Adds compatibility with the new sensor generation, the BMP580.
> > > > 
> > > > The measurement and initialization codepaths are adapted from
> > > > the device datasheet and the repository from manufacturer at
> > > > https://github.com/boschsensortec/BMP5-Sensor-API.
> > > > 
> > > > Signed-off-by: Angel Iglesias
> > > > <ang.iglesiasg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > >   
> > > 
> > > [...]
> > >   
> > > > diff --git a/drivers/iio/pressure/bmp280.h
> > > > b/drivers/iio/pressure/bmp280.h
> > > > index efc31bc84708..27d2abc17d01 100644
> > > > --- a/drivers/iio/pressure/bmp280.h
> > > > +++ b/drivers/iio/pressure/bmp280.h  
> > > 
> > > [...]
> > >   
> > > > +#define BMP580_CHIP_ID                 0x50
> > > >   #define BMP380_CHIP_ID                        0x50  
> > > 
> > > Hi,
> > > 
> > > this is maybe correct (I've not been able to find the datasheet to check 
> > > myself), but it looks odd to have the same ID for 2 different chips.  
> > 
> > Yes, I also couldn't find a datasheet for the BMP580 or a devkit anywhere.
> > I'm
> > developing this using the BMP581, which seems to be a variant almost
> > identical.
> > Something similar happened with the BMP38x; you could find the BMP384 and
> > the
> > BMP388, but the BMP380 was unavailable everywhere, datasheet included. My
> > guess
> > is this is a similar situation. In any case, the datasheet of the BMP581 is
> > available here:
> > https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp581-ds004.pdf
> > 
> > Regarding the chip id being the same between generations is weird, but at
> > least
> > the datasheet and the sensor I have uses 0x50 as the chip id. After you
> > mentioned this, I checked back on the reference API repository from Bosch
> > and it
> > has both 0x50 and 0x51 as valid IDs:
> > *
> > https://github.com/boschsensortec/BMP5-Sensor-API/blob/master/bmp5_defs.h#L198
> > * https://github.com/boschsensortec/BMP5-Sensor-API/blob/master/bmp5.c#L1444
> https://github.com/boschsensortec/BMP3-Sensor-API/blob/master/bmp3_defs.h
> I was curious on whether we had a wrong value for bmp380, but nope... Same ID.
> 
> Huh. As per earlier comment - who wants to moan at Bosch as this is crazy
> situation?
> 
> Jonathan

Well I'm doing this in my free time beacuse I wanted to setup a meteo station
and got annoyed needing to patch-up userspace code for reading pressure and
temperature sensors on a very underpowered ARM device when there is a kernel
subsystem for this kind of things. The rest is history on the mailing list.
I don't think I have any leverage to have Bosch listening to my complaints

Angel

> 
> > 
> > Angel
> > 
> > > CJ
> > >   
> > > >   #define BMP180_CHIP_ID                        0x55
> > > >   #define BMP280_CHIP_ID                        0x58  
> > >   
> > 
> 

Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580

[Jonathan Cameron]

On Sun, 01 Jan 2023 12:16:28 +0100
Angel Iglesias <ang.iglesiasg@gmail.com> wrote:

> On Fri, 2022-12-30 at 18:22 +0000, Jonathan Cameron wrote:
> > On Thu, 29 Dec 2022 19:23:16 +0100
> > Angel Iglesias <ang.iglesiasg@gmail.com> wrote:
> >   
> > > On Thu, 2022-12-29 at 18:35 +0100, Christophe JAILLET wrote:  
> > > > Le 26/12/2022 à 15:29, Angel Iglesias a écrit :    
> > > > > Adds compatibility with the new sensor generation, the BMP580.
> > > > > 
> > > > > The measurement and initialization codepaths are adapted from
> > > > > the device datasheet and the repository from manufacturer at
> > > > > https://github.com/boschsensortec/BMP5-Sensor-API.
> > > > > 
> > > > > Signed-off-by: Angel Iglesias
> > > > > <ang.iglesiasg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > >     
> > > > 
> > > > [...]
> > > >     
> > > > > diff --git a/drivers/iio/pressure/bmp280.h
> > > > > b/drivers/iio/pressure/bmp280.h
> > > > > index efc31bc84708..27d2abc17d01 100644
> > > > > --- a/drivers/iio/pressure/bmp280.h
> > > > > +++ b/drivers/iio/pressure/bmp280.h    
> > > > 
> > > > [...]
> > > >     
> > > > > +#define BMP580_CHIP_ID                 0x50
> > > > >   #define BMP380_CHIP_ID                        0x50    
> > > > 
> > > > Hi,
> > > > 
> > > > this is maybe correct (I've not been able to find the datasheet to check 
> > > > myself), but it looks odd to have the same ID for 2 different chips.    
> > > 
> > > Yes, I also couldn't find a datasheet for the BMP580 or a devkit anywhere.
> > > I'm
> > > developing this using the BMP581, which seems to be a variant almost
> > > identical.
> > > Something similar happened with the BMP38x; you could find the BMP384 and
> > > the
> > > BMP388, but the BMP380 was unavailable everywhere, datasheet included. My
> > > guess
> > > is this is a similar situation. In any case, the datasheet of the BMP581 is
> > > available here:
> > > https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp581-ds004.pdf
> > > 
> > > Regarding the chip id being the same between generations is weird, but at
> > > least
> > > the datasheet and the sensor I have uses 0x50 as the chip id. After you
> > > mentioned this, I checked back on the reference API repository from Bosch
> > > and it
> > > has both 0x50 and 0x51 as valid IDs:
> > > *
> > > https://github.com/boschsensortec/BMP5-Sensor-API/blob/master/bmp5_defs.h#L198
> > > * https://github.com/boschsensortec/BMP5-Sensor-API/blob/master/bmp5.c#L1444  
> > https://github.com/boschsensortec/BMP3-Sensor-API/blob/master/bmp3_defs.h
> > I was curious on whether we had a wrong value for bmp380, but nope... Same ID.
> > 
> > Huh. As per earlier comment - who wants to moan at Bosch as this is crazy
> > situation?
> > 
> > Jonathan  
> 
> Well I'm doing this in my free time beacuse I wanted to setup a meteo station
> and got annoyed needing to patch-up userspace code for reading pressure and
> temperature sensors on a very underpowered ARM device when there is a kernel
> subsystem for this kind of things. The rest is history on the mailing list.
> I don't think I have any leverage to have Bosch listening to my complaints

Sadly I don't have a good contact in Bosch. So all we can do is +CC the
contact address.

If anyone else has a good channel to point out this silliness please do!

Jonathan

> 
> Angel
> 
> >   
> > > 
> > > Angel
> > >   
> > > > CJ
> > > >     
> > > > >   #define BMP180_CHIP_ID                        0x55
> > > > >   #define BMP280_CHIP_ID                        0x58    
> > > >     
> > >   
> >   
> 

RE: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580

[Contact Bosch-Sensortec (BST/SA)]

Dear Jonathan and Angel,

Aligned with our product management, we can confirm that the CHIP-IDs are identiacal, this is correct.


Best regards,

Bosch Sensortec Sales Support

Bosch Sensortec GmbH | Gerhard-Kindler-Straße 9 | 72770 Reutlingen | GERMANY | www.bosch-sensortec.com 


Sitz: Kusterdingen, Registergericht: Stuttgart HRB 382674,
Ust.IdNr. DE 183276693 - Steuer-Nr. 99012/08040
Geschäftsführung: Stefan Finkbeiner, Jens-Knut Fabrowsky



-----Original Message-----
From: Jonathan Cameron <jic23@kernel.org> 
Sent: Sonntag, 8. Januar 2023 13:35
To: Angel Iglesias <ang.iglesiasg@gmail.com>
Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>; ak@it-klinger.de; andriy.shevchenko@linux.intel.com; devicetree@vger.kernel.org; krzysztof.kozlowski+dt@linaro.org; lars@metafoo.de; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; nikita.yoush@cogentembedded.com; paul@crapouillou.net; rafael.j.wysocki@intel.com; robh+dt@kernel.org; ulf.hansson@linaro.org; Contact Bosch-Sensortec (BST/SA) <contact@bosch-sensortec.com>
Subject: Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580

On Sun, 01 Jan 2023 12:16:28 +0100
Angel Iglesias <ang.iglesiasg@gmail.com> wrote:

> On Fri, 2022-12-30 at 18:22 +0000, Jonathan Cameron wrote:
> > On Thu, 29 Dec 2022 19:23:16 +0100
> > Angel Iglesias <ang.iglesiasg@gmail.com> wrote:
> >   
> > > On Thu, 2022-12-29 at 18:35 +0100, Christophe JAILLET wrote:  
> > > > Le 26/12/2022 à 15:29, Angel Iglesias a écrit :    
> > > > > Adds compatibility with the new sensor generation, the BMP580.
> > > > > 
> > > > > The measurement and initialization codepaths are adapted from 
> > > > > the device datasheet and the repository from manufacturer at 
> > > > > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fboschsensortec%2FBMP5-Sensor-API&data=05%7C01%7Ccontact%40bosch-sensortec.com%7C5df3784517c940394e8308daf172e38e%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C638087774478147184%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=YarsZiHIZ%2FBkNNqK%2FunjfVWGgIwOtvf%2BPXs8ipHQdRQ%3D&reserved=0.
> > > > > 
> > > > > Signed-off-by: Angel Iglesias
> > > > > <ang.iglesiasg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > >     
> > > > 
> > > > [...]
> > > >     
> > > > > diff --git a/drivers/iio/pressure/bmp280.h 
> > > > > b/drivers/iio/pressure/bmp280.h index 
> > > > > efc31bc84708..27d2abc17d01 100644
> > > > > --- a/drivers/iio/pressure/bmp280.h
> > > > > +++ b/drivers/iio/pressure/bmp280.h    
> > > > 
> > > > [...]
> > > >     
> > > > > +#define BMP580_CHIP_ID                 0x50
> > > > >   #define BMP380_CHIP_ID                        0x50    
> > > > 
> > > > Hi,
> > > > 
> > > > this is maybe correct (I've not been able to find the datasheet to check 
> > > > myself), but it looks odd to have the same ID for 2 different chips.    
> > > 
> > > Yes, I also couldn't find a datasheet for the BMP580 or a devkit anywhere.
> > > I'm
> > > developing this using the BMP581, which seems to be a variant 
> > > almost identical.
> > > Something similar happened with the BMP38x; you could find the 
> > > BMP384 and the BMP388, but the BMP380 was unavailable everywhere, 
> > > datasheet included. My guess is this is a similar situation. In 
> > > any case, the datasheet of the BMP581 is available here:
> > > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > www.bosch-sensortec.com%2Fmedia%2Fboschsensortec%2Fdownloads%2Fdat
> > > asheets%2Fbst-bmp581-ds004.pdf&data=05%7C01%7Ccontact%40bosch-sens
> > > ortec.com%7C5df3784517c940394e8308daf172e38e%7C0ae51e1907c84e4bbb6
> > > d648ee58410f4%7C0%7C0%7C638087774478147184%7CUnknown%7CTWFpbGZsb3d
> > > 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> > > D%7C3000%7C%7C%7C&sdata=dNnnClouPfqXntLuTWGQovRw39ehaLeuzKuN%2FNWI
> > > 8y0%3D&reserved=0
> > > 
> > > Regarding the chip id being the same between generations is weird, 
> > > but at least the datasheet and the sensor I have uses 0x50 as the 
> > > chip id. After you mentioned this, I checked back on the reference 
> > > API repository from Bosch and it has both 0x50 and 0x51 as valid 
> > > IDs:
> > > *
> > > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > github.com%2Fboschsensortec%2FBMP5-Sensor-API%2Fblob%2Fmaster%2Fbm
> > > p5_defs.h%23L198&data=05%7C01%7Ccontact%40bosch-sensortec.com%7C5d
> > > f3784517c940394e8308daf172e38e%7C0ae51e1907c84e4bbb6d648ee58410f4%
> > > 7C0%7C0%7C638087774478147184%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wL
> > > jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> > > %7C&sdata=tZgYdnxpvKIndjtJaaFfcAJbBa4%2FcIgKhb4XH6sAO9A%3D&reserve
> > > d=0
> > > * 
> > > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > github.com%2Fboschsensortec%2FBMP5-Sensor-API%2Fblob%2Fmaster%2Fbm
> > > p5.c%23L1444&data=05%7C01%7Ccontact%40bosch-sensortec.com%7C5df378
> > > 4517c940394e8308daf172e38e%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%
> > > 7C0%7C638087774478147184%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM
> > > DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&
> > > sdata=xU9Yh2zXjol0vyF9iltRmkBK3CUlGGdcjrp3AzwvTNU%3D&reserved=0
> > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > thub.com%2Fboschsensortec%2FBMP3-Sensor-API%2Fblob%2Fmaster%2Fbmp3_d
> > efs.h&data=05%7C01%7Ccontact%40bosch-sensortec.com%7C5df3784517c9403
> > 94e8308daf172e38e%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C63808
> > 7774478147184%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> > luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=faQZ4qjLF
> > jfYN2US64FbZJo%2B5lOuFw52mIY3XqoXDKU%3D&reserved=0
> > I was curious on whether we had a wrong value for bmp380, but nope... Same ID.
> > 
> > Huh. As per earlier comment - who wants to moan at Bosch as this is 
> > crazy situation?
> > 
> > Jonathan
> 
> Well I'm doing this in my free time beacuse I wanted to setup a meteo 
> station and got annoyed needing to patch-up userspace code for reading 
> pressure and temperature sensors on a very underpowered ARM device 
> when there is a kernel subsystem for this kind of things. The rest is history on the mailing list.
> I don't think I have any leverage to have Bosch listening to my 
> complaints

Sadly I don't have a good contact in Bosch. So all we can do is +CC the contact address.

If anyone else has a good channel to point out this silliness please do!

Jonathan

> 
> Angel
> 
> >   
> > > 
> > > Angel
> > >   
> > > > CJ
> > > >     
> > > > >   #define BMP180_CHIP_ID                        0x55
> > > > >   #define BMP280_CHIP_ID                        0x58    
> > > >     
> > >   
> >   
> 

Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580

[Jonathan Cameron]

On Mon, 26 Dec 2022 15:29:22 +0100
Angel Iglesias <ang.iglesiasg@gmail.com> wrote:

> Adds compatibility with the new sensor generation, the BMP580.
> 
> The measurement and initialization codepaths are adapted from
> the device datasheet and the repository from manufacturer at
> https://github.com/boschsensortec/BMP5-Sensor-API.
> 
> Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>

Hi Angel,

Some comments inline,

Thanks,

Jonathan

> 
> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> index c9453389e4f7..1c18e3b2c501 100644
> --- a/drivers/iio/pressure/Kconfig
> +++ b/drivers/iio/pressure/Kconfig
> @@ -17,14 +17,14 @@ config ABP060MG
>  	  will be called abp060mg.
>  
>  config BMP280
> -	tristate "Bosch Sensortec BMP180/BMP280/BMP380 pressure sensor I2C driver"
> +	tristate "Bosch Sensortec BMP180/BMP280/BMP380/BMP580 pressure sensor I2C driver"
Future reference:  If this gets much longer we'll have to shorten to
"BMP280 and similar" like we have already had to do for a bunch of other drivers
that support too many parts to fit in the short description.  We carry on listing
them all in the help though.

I think this is the last time we can get aways with it.

I2C driver?  Looks to be handling SPI as well.

>  	depends on (I2C || SPI_MASTER)
>  	select REGMAP
>  	select BMP280_I2C if (I2C)
>  	select BMP280_SPI if (SPI_MASTER)
>  	help
> -	  Say yes here to build support for Bosch Sensortec BMP180, BMP280 and
> -	  BMP380 pressure and temperature sensors. Also supports the BME280 with
> +	  Say yes here to build support for Bosch Sensortec BMP180, BMP280, BMP380
> +	  and BMP580 pressure and temperature sensors. Also supports the BME280 with
>  	  an additional humidity sensor channel.
>  
>  	  To compile this driver as a module, choose M here: the core module

...


> +
> +/*
> + * Helper function to send a command to BMP5XX sensors.
> + *
> + * BMP5xx sensors have a series of commands actionable
> + * writing specific sequences on the CMD register:
> + * SOFT_RESET: performs a reset of the system.
> + * NVM_READ: read the contents of a user position of the nvm memory.
> + * NVM_WRITE: write new data to a user position of the nvm memory.
> + * EXT_MODE: enable extended mode with additional debug pages.
> + */
> +static int bmp580_cmd(struct bmp280_data *data, enum bmp580_commands cmd)

Is there an advantage in rolling these up in one function?  There seems to be no real
shared code.  Also most of this isn't used currently I think, so perhaps should be
introduced alongside code that uses it.

> +{
> +	unsigned long deadline;
> +	unsigned int reg;
> +	int ret;
> +
> +	switch (cmd) {
> +	case BMP580_SOFT_RESET_CMD:
> +		/* Send reset word */
> +		ret = regmap_write(data->regmap, BMP580_REG_CMD, BMP580_CMD_SOFT_RESET);
> +		if (ret) {
> +			dev_err(data->dev, "failed to send reset command to device\n");
> +			return ret;
> +		}
> +		/* Wait 2ms for reset completion */
> +		usleep_range(2000, 2500);

blank line.

> +		/* Dummy read of chip_id */
> +		ret = regmap_read(data->regmap, BMP580_REG_CHIP_ID, &reg);
> +		if (ret) {
> +			dev_err(data->dev, "failed to reestablish comms after reset\n");
> +			return ret;
> +		}

blank line etc.  See below for reasoning.

> +		/* Check if POR bit is set on interrupt reg */
> +		ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &reg);
> +		if (ret) {
> +			dev_err(data->dev, "error reading interrupt status register\n");
> +			return ret;
> +		}
> +		if (!(reg & BMP580_INT_STATUS_POR_MASK)) {
> +			dev_err(data->dev, "error resetting sensor\n");
> +			return -EINVAL;
> +		}
> +		break;
> +	case BMP580_NVM_WRITE_CMD:
> +	case BMP580_NVM_READ_CMD:
> +		/* Check nvm ready flag */
> +		ret = regmap_read(data->regmap, BMP580_REG_STATUS, &reg);
> +		if (ret) {
> +			dev_err(data->dev, "failed to check nvm status\n");
> +			return ret;
> +		}
> +		if (!(reg & BMP580_STATUS_NVM_RDY_MASK)) {
> +			dev_err(data->dev, "sensor's nvm is not ready\n");
> +			return -EIO;
> +		}
> +		/* Send NVM operation sequence */
> +		ret = regmap_write(data->regmap, BMP580_REG_CMD, BMP580_CMD_NVM_OP_SEQ_0);
> +		if (ret) {
> +			dev_err(data->dev, "failed to send nvm operation's first sequence\n");
> +			return ret;
> +		}
> +		if (cmd == BMP580_NVM_WRITE_CMD) {
> +			/* Send write sequence */
> +			ret = regmap_write(data->regmap, BMP580_REG_CMD,
> +					   BMP580_CMD_NVM_WRITE_SEQ_1);
> +			if (ret) {
> +				dev_err(data->dev, "failed to send nvm write sequence\n");
> +				return ret;
> +			}
> +			/* Datasheet says on 4.8.1.2 it takes approximately 10ms */
> +			usleep_range(10000, 10500);
> +			deadline = jiffies + msecs_to_jiffies(10);
> +		} else {
> +			/* Send read sequence */
> +			ret = regmap_write(data->regmap, BMP580_REG_CMD,
> +					   BMP580_CMD_NVM_READ_SEQ_1);
> +			if (ret) {
> +				dev_err(data->dev, "failed to send nvm read sequence\n");
> +				return ret;
> +			}
> +			/* Datasheet says on 4.8.1.1 it takes approximately 200us */
> +			usleep_range(200, 250);
> +			deadline = jiffies + usecs_to_jiffies(200);
> +		}
> +		if (ret) {
> +			dev_err(data->dev, "failed to write command sequence\n");
> +			return -EIO;
> +		}
> +		/* Wait until NVM is ready again */
> +		do {
> +			ret = regmap_read(data->regmap, BMP580_REG_STATUS, &reg);
> +			if (ret) {
> +				dev_err(data->dev, "failed to check nvm status\n");
> +				reg &= ~BMP580_STATUS_NVM_RDY_MASK;
> +			}
> +		} while (time_before(jiffies, deadline) && !(reg & BMP580_STATUS_NVM_RDY_MASK));
> +
> +		if (!(reg & BMP580_STATUS_NVM_RDY_MASK)) {
> +			dev_err(data->dev,
> +				"reached timeout waiting for nvm operation completion\n");
> +			return -ETIMEDOUT;
> +		}
> +		/* Checks nvm error flags */
> +		if ((reg & BMP580_STATUS_NVM_ERR_MASK) || (reg & BMP580_STATUS_NVM_CMD_ERR_MASK)) {
> +			dev_err(data->dev, "error processing nvm operation\n");
> +			return -EIO;
> +		}
> +		break;
> +	case BMP580_EXT_MODE_CMD:
> +		ret = regmap_write(data->regmap, BMP580_REG_CMD, BMP580_CMD_EXTMODE_SEQ_0);
> +		if (ret) {
> +			dev_err(data->dev, "failed to send ext_mode first sequence\n");
> +			return ret;
> +		}
> +		ret = regmap_write(data->regmap, BMP580_REG_CMD, BMP580_CMD_EXTMODE_SEQ_1);
> +		if (ret) {
> +			dev_err(data->dev, "failed to send ext_mode second sequence\n");
> +			return ret;
> +		}
> +		ret = regmap_write(data->regmap, BMP580_REG_CMD, BMP580_CMD_EXTMODE_SEQ_2);
> +		if (ret) {
> +			dev_err(data->dev, "failed to send ext_mode second sequence\n");
> +			return ret;
> +		}
> +		break;

Might as well return from each of the instead of break.  slightly reduces the code
anyone interested in a particular flow needs to look at.


> +	}
> +
> +	return 0;
> +}

...

> +
> +static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
> +{
> +	u32 raw_press;
> +	int ret;
> +
> +	ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf,
> +			       sizeof(data->buf));
> +	if (ret) {
> +		dev_err(data->dev, "failed to read pressure\n");
> +		return ret;
> +	}
> +
> +	raw_press = get_unaligned_le24(data->buf);
> +	if (raw_press == BMP580_PRESS_SKIPPED) {
> +		dev_err(data->dev, "reading pressure skipped\n");
> +		return -EIO;
> +	}
> +	/*
> +	 * Pressure is returned in Pascals in fractional form down 2^16.
> +	 * We reescale /1000 to convert to kilopascal to respect IIO ABI.
> +	 */
> +	*val = raw_press;
> +	*val2 = 64000; // 2^6 * 1000

/* */ syntax for comments in IIO.

> +	return IIO_VAL_FRACTIONAL;
> +}

...

> +static int bmp580_preinit(struct bmp280_data *data)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	/* Issue soft-reset command */
> +	ret = bmp580_cmd(data, BMP580_SOFT_RESET_CMD);
> +	if (ret)
> +		return ret;
Blank line here and similar places.  Nice to keep unrelated blocks of
code separate.

> +	/* Post powerup sequence */
> +	ret = regmap_read(data->regmap, BMP580_REG_CHIP_ID, &reg);
> +	if (ret)
> +		return ret;
> +	if (reg != BMP580_CHIP_ID) {
> +		dev_err(data->dev, "preinit: unexpected chip_id\n");

I'd rather this was just an info print and carry on.  If Bosch brings
another device out that is sufficiently compatible and we use a fallback
dt compatible for it - so that it works with older kernels then we might
want to indicate we aren't sure we can handle the chip correctly but then assume
the DT description was correct and carry on anyway. 

> +		return -EINVAL;
> +	}
> +	ret = regmap_read(data->regmap, BMP580_REG_STATUS, &reg);
> +	if (ret)
> +		return ret;
> +	/* Check nvm status */
> +	if (!(reg & BMP580_STATUS_NVM_RDY_MASK) || (reg & BMP580_STATUS_NVM_ERR_MASK)) {
> +		dev_err(data->dev, "preinit: nvm error on powerup sequence\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +

...

> +static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
> +static const int bmp580_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 };

Up to you, but you could take advantage of the fact this array matches the bmp380 one.
It is arguable that the code is clearer with it not being reused though so your choice.

> +
> +static const struct bmp280_chip_info bmp580_chip_info = {
> +	.id_reg = BMP580_REG_CHIP_ID,
> +	.chip_id = BMP580_CHIP_ID,
> +	.start_up_time = 2000,
> +	.channels = bmp380_channels,
> +	.num_channels = 2,
> +
> +	.oversampling_temp_avail = bmp580_oversampling_avail,
> +	.num_oversampling_temp_avail = ARRAY_SIZE(bmp580_oversampling_avail),
> +	.oversampling_temp_default = ilog2(1),
> +
> +	.oversampling_press_avail = bmp580_oversampling_avail,
> +	.num_oversampling_press_avail = ARRAY_SIZE(bmp580_oversampling_avail),
> +	.oversampling_press_default = ilog2(4),
> +
> +	.sampling_freq_avail = bmp580_odr_table,
> +	.num_sampling_freq_avail = ARRAY_SIZE(bmp580_odr_table) * 2,
> +	.sampling_freq_default = BMP580_ODR_50HZ,
> +
> +	.iir_filter_coeffs_avail = bmp580_iir_filter_coeffs_avail,
> +	.num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp580_iir_filter_coeffs_avail),
> +	.iir_filter_coeff_default = 2,
> +
> +	.chip_config = bmp580_chip_config,
> +	.read_temp = bmp580_read_temp,
> +	.read_press = bmp580_read_press,
> +	.read_calib = NULL,

As in previous patch, I'd prefer not to see explicit NULL being set for
optional elements. Their absence should be enough.

> +	.preinit = bmp580_preinit,
> +};
> +
>  static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
>  {
>  	const int conversion_time_max[] = { 4500, 7500, 13500, 25500 };
> @@ -1713,6 +2142,9 @@ int bmp280_common_probe(struct device *dev,
>  	case BMP380:
>  		chip_info = &bmp380_chip_info;
>  		break;
> +	case BMP580:
> +		chip_info = &bmp580_chip_info;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -1779,9 +2211,10 @@ int bmp280_common_probe(struct device *dev,
>  	 */
>  	if (data->chip_info->preinit) {
>  		ret = data->chip_info->preinit(data);
> -		dev_err(dev, "error running preinit tasks");
> -		if (ret < 0)
> +		if (ret) {
> +			dev_err(dev, "error running preinit tasks\n");
>  			return ret;
Wrong patch.  + use dev_error_probe() like the case below already does.

> +		}
>  	}
>  
>  	ret = data->chip_info->chip_config(data);
> @@ -1795,11 +2228,12 @@ int bmp280_common_probe(struct device *dev,
>  	 * non-volatile memory during production". Let's read them out at probe
>  	 * time once. They will not change.
>  	 */
> -
> -	ret = data->chip_info->read_calib(data);
> -	if (ret < 0)
> -		return dev_err_probe(data->dev, ret,
> -				     "failed to read calibration coefficients\n");
> +	if (data->chip_info->read_calib) {

Small thing, but ideally this no op refactoring should have been a precursor patch
rather than buried in here. It's small enough though that I don't care enough to insist
you break it out.

> +		ret = data->chip_info->read_calib(data);
> +		if (ret < 0)
> +			return dev_err_probe(data->dev, ret,
> +					     "failed to read calibration coefficients\n");
> +	}
>  
>  	/*
>  	 * Attempt to grab an optional EOC IRQ - only the BMP085 has this
> diff --git a/drivers/iio/pressure/bmp280-i2c.c b/drivers/iio/pressure/bmp280-i2c.c
> index 59921e8cd592..c52d2b477bb7 100644
> --- a/drivers/iio/pressure/bmp280-i2c.c
> +++ b/drivers/iio/pressure/bmp280-i2c.c
> @@ -22,6 +22,9 @@ static int bmp280_i2c_probe(struct i2c_client *client)
>  	case BMP380:
>  		regmap_config = &bmp380_regmap_config;
>  		break;
> +	case BMP580:
> +		regmap_config = &bmp580_regmap_config;

whilst here, similar to below, it would be nice to switch to generic
firmware methods as default way to get the match data then fallback to the
i2c_device_id table if that fails (as we did old school i2c probing from
a board file or from userspace).

> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -45,6 +48,7 @@ static const struct of_device_id bmp280_of_i2c_match[] = {
>  	{ .compatible = "bosch,bmp280", .data = (void *)BMP280 },
>  	{ .compatible = "bosch,bme280", .data = (void *)BME280 },
>  	{ .compatible = "bosch,bmp380", .data = (void *)BMP380 },
> +	{ .compatible = "bosch,bmp580", .data = (void *)BMP580 },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, bmp280_of_i2c_match);
> @@ -55,6 +59,7 @@ static const struct i2c_device_id bmp280_i2c_id[] = {
>  	{"bmp280", BMP280 },
>  	{"bme280", BME280 },
>  	{"bmp380", BMP380 },
> +	{"bmp580", BMP580 },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(i2c, bmp280_i2c_id);

> diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
> index 4a2df5b5d838..5653c3c33081 100644
> --- a/drivers/iio/pressure/bmp280-spi.c
> +++ b/drivers/iio/pressure/bmp280-spi.c
> @@ -69,6 +69,9 @@ static int bmp280_spi_probe(struct spi_device *spi)
>  	case BMP380:
>  		regmap_config = &bmp380_regmap_config;
>  		break;
> +	case BMP580:
> +		regmap_config = &bmp580_regmap_config;

If using a pointer as suggested you'd get this for free :)

I'd also like to see this driver move to using the of_device_id table
via device_get_match_data() first then fallback to the spi_device_id table
only if that fails.   Otherwise for dt supprot, we are relying on a match
based on stripping the bosch prefix of the of compatible and that is nasty.

> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -96,6 +99,7 @@ static const struct of_device_id bmp280_of_spi_match[] = {
>  	{ .compatible = "bosch,bmp280", },
>  	{ .compatible = "bosch,bme280", },
>  	{ .compatible = "bosch,bmp380", },
> +	{ .compatible = "bosch,bmp580", },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, bmp280_of_spi_match);
> @@ -106,6 +110,7 @@ static const struct spi_device_id bmp280_spi_id[] = {
>  	{ "bmp280", BMP280 },
>  	{ "bme280", BME280 },
>  	{ "bmp380", BMP380 },
> +	{ "bmp580", BMP580 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(spi, bmp280_spi_id);
> diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> index efc31bc84708..27d2abc17d01 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -3,6 +3,107 @@

...

> +#define BMP580_CHIP_ID			0x50
>  #define BMP380_CHIP_ID			0x50

Well given we can't order them by ID value, probably best
to have the set with a given ID in alphabetical order.  So
swap the two above.

>  #define BMP180_CHIP_ID			0x55
>  #define BMP280_CHIP_ID			0x58
> @@ -197,12 +299,14 @@ enum bmp280_variant {
>  	BMP280,
>  	BME280,
>  	BMP380,
> +	BMP580,
>  };
>  
>  /* Regmap configurations */
>  extern const struct regmap_config bmp180_regmap_config;
>  extern const struct regmap_config bmp280_regmap_config;
>  extern const struct regmap_config bmp380_regmap_config;
> +extern const struct regmap_config bmp580_regmap_config;
>  
>  /* Probe called from different transports */
>  int bmp280_common_probe(struct device *dev,

Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580

[Angel Iglesias]

On Fri, 2022-12-30 at 18:45 +0000, Jonathan Cameron wrote:
> On Mon, 26 Dec 2022 15:29:22 +0100
> Angel Iglesias <ang.iglesiasg@gmail.com> wrote:
> 
> > Adds compatibility with the new sensor generation, the BMP580.
> > 
> > The measurement and initialization codepaths are adapted from
> > the device datasheet and the repository from manufacturer at
> > https://github.com/boschsensortec/BMP5-Sensor-API.
> > 
> > Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>
> 
> Hi Angel,
> 
> Some comments inline,
> 
> Thanks,
> 
> Jonathan
> 
> > 
> > diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> > index c9453389e4f7..1c18e3b2c501 100644
> > --- a/drivers/iio/pressure/Kconfig
> > +++ b/drivers/iio/pressure/Kconfig
> > @@ -17,14 +17,14 @@ config ABP060MG
> >           will be called abp060mg.
> >  
> >  config BMP280
> > -       tristate "Bosch Sensortec BMP180/BMP280/BMP380 pressure sensor I2C
> > driver"
> > +       tristate "Bosch Sensortec BMP180/BMP280/BMP380/BMP580 pressure
> > sensor I2C driver"
> Future reference:  If this gets much longer we'll have to shorten to
> "BMP280 and similar" like we have already had to do for a bunch of other
> drivers
> that support too many parts to fit in the short description.  We carry on
> listing
> them all in the help though.
> 
> I think this is the last time we can get aways with it.
> 
> I2C driver?  Looks to be handling SPI as well.

Welp, you're right, last time I was here forgot to update the wording.

> >         depends on (I2C || SPI_MASTER)
> >         select REGMAP
> >         select BMP280_I2C if (I2C)
> >         select BMP280_SPI if (SPI_MASTER)
> >         help
> > -         Say yes here to build support for Bosch Sensortec BMP180, BMP280
> > and
> > -         BMP380 pressure and temperature sensors. Also supports the BME280
> > with
> > +         Say yes here to build support for Bosch Sensortec BMP180, BMP280,
> > BMP380
> > +         and BMP580 pressure and temperature sensors. Also supports the
> > BME280 with
> >           an additional humidity sensor channel.
> >  
> >           To compile this driver as a module, choose M here: the core module
> 
> ...
> 
> 
> > +
> > +/*
> > + * Helper function to send a command to BMP5XX sensors.
> > + *
> > + * BMP5xx sensors have a series of commands actionable
> > + * writing specific sequences on the CMD register:
> > + * SOFT_RESET: performs a reset of the system.
> > + * NVM_READ: read the contents of a user position of the nvm memory.
> > + * NVM_WRITE: write new data to a user position of the nvm memory.
> > + * EXT_MODE: enable extended mode with additional debug pages.
> > + */
> > +static int bmp580_cmd(struct bmp280_data *data, enum bmp580_commands cmd)
> 
> Is there an advantage in rolling these up in one function?  There seems to be
> no real
> shared code.  Also most of this isn't used currently I think, so perhaps
> should be
> introduced alongside code that uses it.

Probably, would be best to break up this code in two helpers functions for reset
and NVM operations and drop the code to setup debugging ext_mode. I'm not using
the ext_mode and there's no documentation about the new registers exposed in
that mode.

> > +{
> > +       unsigned long deadline;
> > +       unsigned int reg;
> > +       int ret;
> > +
> > +       switch (cmd) {
> > +       case BMP580_SOFT_RESET_CMD:
> > +               /* Send reset word */
> > +               ret = regmap_write(data->regmap, BMP580_REG_CMD,
> > BMP580_CMD_SOFT_RESET);
> > +               if (ret) {
> > +                       dev_err(data->dev, "failed to send reset command to
> > device\n");
> > +                       return ret;
> > +               }
> > +               /* Wait 2ms for reset completion */
> > +               usleep_range(2000, 2500);
> 
> blank line.
> 
> > +               /* Dummy read of chip_id */
> > +               ret = regmap_read(data->regmap, BMP580_REG_CHIP_ID, &reg);
> > +               if (ret) {
> > +                       dev_err(data->dev, "failed to reestablish comms
> > after reset\n");
> > +                       return ret;
> > +               }
> 
> blank line etc.  See below for reasoning.
> 
> > +               /* Check if POR bit is set on interrupt reg */
> > +               ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS,
> > &reg);
> > +               if (ret) {
> > +                       dev_err(data->dev, "error reading interrupt status
> > register\n");
> > +                       return ret;
> > +               }
> > +               if (!(reg & BMP580_INT_STATUS_POR_MASK)) {
> > +                       dev_err(data->dev, "error resetting sensor\n");
> > +                       return -EINVAL;
> > +               }
> > +               break;
> > +       case BMP580_NVM_WRITE_CMD:
> > +       case BMP580_NVM_READ_CMD:
> > +               /* Check nvm ready flag */
> > +               ret = regmap_read(data->regmap, BMP580_REG_STATUS, &reg);
> > +               if (ret) {
> > +                       dev_err(data->dev, "failed to check nvm status\n");
> > +                       return ret;
> > +               }
> > +               if (!(reg & BMP580_STATUS_NVM_RDY_MASK)) {
> > +                       dev_err(data->dev, "sensor's nvm is not ready\n");
> > +                       return -EIO;
> > +               }
> > +               /* Send NVM operation sequence */
> > +               ret = regmap_write(data->regmap, BMP580_REG_CMD,
> > BMP580_CMD_NVM_OP_SEQ_0);
> > +               if (ret) {
> > +                       dev_err(data->dev, "failed to send nvm operation's
> > first sequence\n");
> > +                       return ret;
> > +               }
> > +               if (cmd == BMP580_NVM_WRITE_CMD) {
> > +                       /* Send write sequence */
> > +                       ret = regmap_write(data->regmap, BMP580_REG_CMD,
> > +                                          BMP580_CMD_NVM_WRITE_SEQ_1);
> > +                       if (ret) {
> > +                               dev_err(data->dev, "failed to send nvm write
> > sequence\n");
> > +                               return ret;
> > +                       }
> > +                       /* Datasheet says on 4.8.1.2 it takes approximately
> > 10ms */
> > +                       usleep_range(10000, 10500);
> > +                       deadline = jiffies + msecs_to_jiffies(10);
> > +               } else {
> > +                       /* Send read sequence */
> > +                       ret = regmap_write(data->regmap, BMP580_REG_CMD,
> > +                                          BMP580_CMD_NVM_READ_SEQ_1);
> > +                       if (ret) {
> > +                               dev_err(data->dev, "failed to send nvm read
> > sequence\n");
> > +                               return ret;
> > +                       }
> > +                       /* Datasheet says on 4.8.1.1 it takes approximately
> > 200us */
> > +                       usleep_range(200, 250);
> > +                       deadline = jiffies + usecs_to_jiffies(200);
> > +               }
> > +               if (ret) {
> > +                       dev_err(data->dev, "failed to write command
> > sequence\n");
> > +                       return -EIO;
> > +               }
> > +               /* Wait until NVM is ready again */
> > +               do {
> > +                       ret = regmap_read(data->regmap, BMP580_REG_STATUS,
> > &reg);
> > +                       if (ret) {
> > +                               dev_err(data->dev, "failed to check nvm
> > status\n");
> > +                               reg &= ~BMP580_STATUS_NVM_RDY_MASK;
> > +                       }
> > +               } while (time_before(jiffies, deadline) && !(reg &
> > BMP580_STATUS_NVM_RDY_MASK));
> > +
> > +               if (!(reg & BMP580_STATUS_NVM_RDY_MASK)) {
> > +                       dev_err(data->dev,
> > +                               "reached timeout waiting for nvm operation
> > completion\n");
> > +                       return -ETIMEDOUT;
> > +               }
> > +               /* Checks nvm error flags */
> > +               if ((reg & BMP580_STATUS_NVM_ERR_MASK) || (reg &
> > BMP580_STATUS_NVM_CMD_ERR_MASK)) {
> > +                       dev_err(data->dev, "error processing nvm
> > operation\n");
> > +                       return -EIO;
> > +               }
> > +               break;
> > +       case BMP580_EXT_MODE_CMD:
> > +               ret = regmap_write(data->regmap, BMP580_REG_CMD,
> > BMP580_CMD_EXTMODE_SEQ_0);
> > +               if (ret) {
> > +                       dev_err(data->dev, "failed to send ext_mode first
> > sequence\n");
> > +                       return ret;
> > +               }
> > +               ret = regmap_write(data->regmap, BMP580_REG_CMD,
> > BMP580_CMD_EXTMODE_SEQ_1);
> > +               if (ret) {
> > +                       dev_err(data->dev, "failed to send ext_mode second
> > sequence\n");
> > +                       return ret;
> > +               }
> > +               ret = regmap_write(data->regmap, BMP580_REG_CMD,
> > BMP580_CMD_EXTMODE_SEQ_2);
> > +               if (ret) {
> > +                       dev_err(data->dev, "failed to send ext_mode second
> > sequence\n");
> > +                       return ret;
> > +               }
> > +               break;
> 
> Might as well return from each of the instead of break.  slightly reduces the
> code
> anyone interested in a particular flow needs to look at.
> 
> 
> > +       }
> > +
> > +       return 0;
> > +}
> 
> ...
> 
> > +
> > +static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
> > +{
> > +       u32 raw_press;
> > +       int ret;
> > +
> > +       ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data-
> > >buf,
> > +                              sizeof(data->buf));
> > +       if (ret) {
> > +               dev_err(data->dev, "failed to read pressure\n");
> > +               return ret;
> > +       }
> > +
> > +       raw_press = get_unaligned_le24(data->buf);
> > +       if (raw_press == BMP580_PRESS_SKIPPED) {
> > +               dev_err(data->dev, "reading pressure skipped\n");
> > +               return -EIO;
> > +       }
> > +       /*
> > +        * Pressure is returned in Pascals in fractional form down 2^16.
> > +        * We reescale /1000 to convert to kilopascal to respect IIO ABI.
> > +        */
> > +       *val = raw_press;
> > +       *val2 = 64000; // 2^6 * 1000
> 
> /* */ syntax for comments in IIO.
> 
> > +       return IIO_VAL_FRACTIONAL;
> > +}
> 
> ...
> 
> > +static int bmp580_preinit(struct bmp280_data *data)
> > +{
> > +       unsigned int reg;
> > +       int ret;
> > +
> > +       /* Issue soft-reset command */
> > +       ret = bmp580_cmd(data, BMP580_SOFT_RESET_CMD);
> > +       if (ret)
> > +               return ret;
> Blank line here and similar places.  Nice to keep unrelated blocks of
> code separate.
> 
> > +       /* Post powerup sequence */
> > +       ret = regmap_read(data->regmap, BMP580_REG_CHIP_ID, &reg);
> > +       if (ret)
> > +               return ret;
> > +       if (reg != BMP580_CHIP_ID) {
> > +               dev_err(data->dev, "preinit: unexpected chip_id\n");
> 
> I'd rather this was just an info print and carry on.  If Bosch brings
> another device out that is sufficiently compatible and we use a fallback
> dt compatible for it - so that it works with older kernels then we might
> want to indicate we aren't sure we can handle the chip correctly but then
> assume

This sounds a great idea, but this would need a few extra changes to work as
intended here:
* In the common probe, before calling to preinit, the device ID is read and
compared to expected ID stored in the chip_info. If the IDs are different, we
stop initialization with an error
* I should backport this, at least, to the BMP380 preinit call, as I have
pending testing if this works well with the BMP390, and sounds like a good
candidate. Which, after looking to datasheet, seems to have 0x60 as its chip ID!

> the DT description was correct and carry on anyway. 
> 
> > +               return -EINVAL;
> > +       }
> > +       ret = regmap_read(data->regmap, BMP580_REG_STATUS, &reg);
> > +       if (ret)
> > +               return ret;
> > +       /* Check nvm status */
> > +       if (!(reg & BMP580_STATUS_NVM_RDY_MASK) || (reg &
> > BMP580_STATUS_NVM_ERR_MASK)) {
> > +               dev_err(data->dev, "preinit: nvm error on powerup
> > sequence\n");
> > +               return -EIO;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> 
> ...
> 
> > +static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64,
> > 128 };
> > +static const int bmp580_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32,
> > 64, 128 };
> 
> Up to you, but you could take advantage of the fact this array matches the
> bmp380 one.
> It is arguable that the code is clearer with it not being reused though so
> your choice.

Hum yes, I could reuse the array for the BMP380, maybe a should use a more
generic name for that array to avoid confusion? Something like
bmp280_iir_filter_coeffs_avail, refering to the driver name instead of the
concrete sensor?

> 
> > +
> > +static const struct bmp280_chip_info bmp580_chip_info = {
> > +       .id_reg = BMP580_REG_CHIP_ID,
> > +       .chip_id = BMP580_CHIP_ID,
> > +       .start_up_time = 2000,
> > +       .channels = bmp380_channels,
> > +       .num_channels = 2,
> > +
> > +       .oversampling_temp_avail = bmp580_oversampling_avail,
> > +       .num_oversampling_temp_avail =
> > ARRAY_SIZE(bmp580_oversampling_avail),
> > +       .oversampling_temp_default = ilog2(1),
> > +
> > +       .oversampling_press_avail = bmp580_oversampling_avail,
> > +       .num_oversampling_press_avail =
> > ARRAY_SIZE(bmp580_oversampling_avail),
> > +       .oversampling_press_default = ilog2(4),
> > +
> > +       .sampling_freq_avail = bmp580_odr_table,
> > +       .num_sampling_freq_avail = ARRAY_SIZE(bmp580_odr_table) * 2,
> > +       .sampling_freq_default = BMP580_ODR_50HZ,
> > +
> > +       .iir_filter_coeffs_avail = bmp580_iir_filter_coeffs_avail,
> > +       .num_iir_filter_coeffs_avail =
> > ARRAY_SIZE(bmp580_iir_filter_coeffs_avail),
> > +       .iir_filter_coeff_default = 2,
> > +
> > +       .chip_config = bmp580_chip_config,
> > +       .read_temp = bmp580_read_temp,
> > +       .read_press = bmp580_read_press,
> > +       .read_calib = NULL,
> 
> As in previous patch, I'd prefer not to see explicit NULL being set for
> optional elements. Their absence should be enough.
> 
> > +       .preinit = bmp580_preinit,
> > +};
> > +
> >  static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
> >  {
> >         const int conversion_time_max[] = { 4500, 7500, 13500, 25500 };
> > @@ -1713,6 +2142,9 @@ int bmp280_common_probe(struct device *dev,
> >         case BMP380:
> >                 chip_info = &bmp380_chip_info;
> >                 break;
> > +       case BMP580:
> > +               chip_info = &bmp580_chip_info;
> > +               break;
> >         default:
> >                 return -EINVAL;
> >         }
> > @@ -1779,9 +2211,10 @@ int bmp280_common_probe(struct device *dev,
> >          */
> >         if (data->chip_info->preinit) {
> >                 ret = data->chip_info->preinit(data);
> > -               dev_err(dev, "error running preinit tasks");
> > -               if (ret < 0)
> > +               if (ret) {
> > +                       dev_err(dev, "error running preinit tasks\n");
> >                         return ret;
> Wrong patch.  + use dev_error_probe() like the case below already does.
> 
> > +               }
> >         }
> >  
> >         ret = data->chip_info->chip_config(data);
> > @@ -1795,11 +2228,12 @@ int bmp280_common_probe(struct device *dev,
> >          * non-volatile memory during production". Let's read them out at
> > probe
> >          * time once. They will not change.
> >          */
> > -
> > -       ret = data->chip_info->read_calib(data);
> > -       if (ret < 0)
> > -               return dev_err_probe(data->dev, ret,
> > -                                    "failed to read calibration
> > coefficients\n");
> > +       if (data->chip_info->read_calib) {
> 
> Small thing, but ideally this no op refactoring should have been a precursor
> patch
> rather than buried in here. It's small enough though that I don't care enough
> to insist
> you break it out.

Sure

> 
> > +               ret = data->chip_info->read_calib(data);
> > +               if (ret < 0)
> > +                       return dev_err_probe(data->dev, ret,
> > +                                            "failed to read calibration
> > coefficients\n");
> > +       }
> >  
> >         /*
> >          * Attempt to grab an optional EOC IRQ - only the BMP085 has this
> > diff --git a/drivers/iio/pressure/bmp280-i2c.c
> > b/drivers/iio/pressure/bmp280-i2c.c
> > index 59921e8cd592..c52d2b477bb7 100644
> > --- a/drivers/iio/pressure/bmp280-i2c.c
> > +++ b/drivers/iio/pressure/bmp280-i2c.c
> > @@ -22,6 +22,9 @@ static int bmp280_i2c_probe(struct i2c_client *client)
> >         case BMP380:
> >                 regmap_config = &bmp380_regmap_config;
> >                 break;
> > +       case BMP580:
> > +               regmap_config = &bmp580_regmap_config;
> 
> whilst here, similar to below, it would be nice to switch to generic
> firmware methods as default way to get the match data then fallback to the
> i2c_device_id table if that fails (as we did old school i2c probing from
> a board file or from userspace).
> 
> > +               break;
> >         default:
> >                 return -EINVAL;
> >         }
> > @@ -45,6 +48,7 @@ static const struct of_device_id bmp280_of_i2c_match[] = {
> >         { .compatible = "bosch,bmp280", .data = (void *)BMP280 },
> >         { .compatible = "bosch,bme280", .data = (void *)BME280 },
> >         { .compatible = "bosch,bmp380", .data = (void *)BMP380 },
> > +       { .compatible = "bosch,bmp580", .data = (void *)BMP580 },
> >         { },
> >  };
> >  MODULE_DEVICE_TABLE(of, bmp280_of_i2c_match);
> > @@ -55,6 +59,7 @@ static const struct i2c_device_id bmp280_i2c_id[] = {
> >         {"bmp280", BMP280 },
> >         {"bme280", BME280 },
> >         {"bmp380", BMP380 },
> > +       {"bmp580", BMP580 },
> >         { },
> >  };
> >  MODULE_DEVICE_TABLE(i2c, bmp280_i2c_id);
> 
> > diff --git a/drivers/iio/pressure/bmp280-spi.c
> > b/drivers/iio/pressure/bmp280-spi.c
> > index 4a2df5b5d838..5653c3c33081 100644
> > --- a/drivers/iio/pressure/bmp280-spi.c
> > +++ b/drivers/iio/pressure/bmp280-spi.c
> > @@ -69,6 +69,9 @@ static int bmp280_spi_probe(struct spi_device *spi)
> >         case BMP380:
> >                 regmap_config = &bmp380_regmap_config;
> >                 break;
> > +       case BMP580:
> > +               regmap_config = &bmp580_regmap_config;
> 
> If using a pointer as suggested you'd get this for free :)
> 
> I'd also like to see this driver move to using the of_device_id table
> via device_get_match_data() first then fallback to the spi_device_id table
> only if that fails.   Otherwise for dt supprot, we are relying on a match
> based on stripping the bosch prefix of the of compatible and that is nasty.

Ok! I'll have a look at it and add this as a precursor patch.

> 
> > +               break;
> >         default:
> >                 return -EINVAL;
> >         }
> > @@ -96,6 +99,7 @@ static const struct of_device_id bmp280_of_spi_match[] = {
> >         { .compatible = "bosch,bmp280", },
> >         { .compatible = "bosch,bme280", },
> >         { .compatible = "bosch,bmp380", },
> > +       { .compatible = "bosch,bmp580", },
> >         { },
> >  };
> >  MODULE_DEVICE_TABLE(of, bmp280_of_spi_match);
> > @@ -106,6 +110,7 @@ static const struct spi_device_id bmp280_spi_id[] = {
> >         { "bmp280", BMP280 },
> >         { "bme280", BME280 },
> >         { "bmp380", BMP380 },
> > +       { "bmp580", BMP580 },
> >         { }
> >  };
> >  MODULE_DEVICE_TABLE(spi, bmp280_spi_id);
> > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > index efc31bc84708..27d2abc17d01 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -3,6 +3,107 @@
> 
> ...
> 
> > +#define BMP580_CHIP_ID                 0x50
> >  #define BMP380_CHIP_ID                 0x50
> 
> Well given we can't order them by ID value, probably best
> to have the set with a given ID in alphabetical order.  So
> swap the two above.

Got it

> 
> >  #define BMP180_CHIP_ID                 0x55
> >  #define BMP280_CHIP_ID                 0x58
> > @@ -197,12 +299,14 @@ enum bmp280_variant {
> >         BMP280,
> >         BME280,
> >         BMP380,
> > +       BMP580,
> >  };
> >  
> >  /* Regmap configurations */
> >  extern const struct regmap_config bmp180_regmap_config;
> >  extern const struct regmap_config bmp280_regmap_config;
> >  extern const struct regmap_config bmp380_regmap_config;
> > +extern const struct regmap_config bmp580_regmap_config;
> >  
> >  /* Probe called from different transports */
> >  int bmp280_common_probe(struct device *dev,
> 
Thanks for your time!
Angel

Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580

[Jonathan Cameron]

> > > +static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64,
> > > 128 };
> > > +static const int bmp580_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32,
> > > 64, 128 };  
> > 
> > Up to you, but you could take advantage of the fact this array matches the
> > bmp380 one.
> > It is arguable that the code is clearer with it not being reused though so
> > your choice.  
> 
> Hum yes, I could reuse the array for the BMP380, maybe a should use a more
> generic name for that array to avoid confusion? Something like
> bmp280_iir_filter_coeffs_avail, refering to the driver name instead of the
> concrete sensor?

Don't worry about the naming. Anything clever just tends to cause
problems as more parts are added.  Just stick to the name of the first part
that used it.  We've made the mistake of trying for generic names in the
past and it bites back!


Jonathan

[PATCH v2 4/5] dt-bindings: iio: pressure: bmp085: Add BMP580 compatible string

[Angel Iglesias]

Add bosch,bmp580 to compatible string for the new family of sensors.
This family includes the BMP580 and BMP581 sensors. The register map
in this family presents significant departures from previous generations.

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

diff --git a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
index 72cd2c2d3f17..f52c4794e21b 100644
--- a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
+++ b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
@@ -17,6 +17,7 @@ description: |
     https://www.bosch-sensortec.com/bst/products/all_products/bmp280
     https://www.bosch-sensortec.com/bst/products/all_products/bme280
     https://www.bosch-sensortec.com/bst/products/all_products/bmp380
+    https://www.bosch-sensortec.com/bst/products/all_products/bmp580
 
 properties:
   compatible:
@@ -26,6 +27,7 @@ properties:
       - bosch,bmp280
       - bosch,bme280
       - bosch,bmp380
+      - bosch,bmp580
 
   reg:
     maxItems: 1
-- 
2.39.0

Re: [PATCH v2 4/5] dt-bindings: iio: pressure: bmp085: Add BMP580 compatible string

[Krzysztof Kozlowski]

On 26/12/2022 15:29, Angel Iglesias wrote:
> Add bosch,bmp580 to compatible string for the new family of sensors.
> This family includes the BMP580 and BMP581 sensors. The register map
> in this family presents significant departures from previous generations.
> 
> Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>

This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions. However, there's no need to repost patches *only* to add the
tags. The upstream maintainer will do that for acks received on the
version they apply.

https://elixir.bootlin.com/linux/v5.17/source/Documentation/process/submitting-patches.rst#L540

If a tag was not added on purpose, please state why and what changed.

Best regards,
Krzysztof

[PATCH v2 5/5] iio: pressure: bmp280: Add nvmem operations for BMP580

[Angel Iglesias]

The pressure sensor BMP580 contains a non-volatile memory that stores
trimming and configuration params. That memory provides an programmable
user range of three 2-byte words.

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 44901c6eb2f9..578d145be55d 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -28,6 +28,7 @@
 #include <linux/bitfield.h>
 #include <linux/device.h>
 #include <linux/module.h>
+#include <linux/nvmem-provider.h>
 #include <linux/regmap.h>
 #include <linux/delay.h>
 #include <linux/iio/iio.h>
@@ -1628,8 +1629,140 @@ static const int bmp580_odr_table[][2] = {
 	[BMP580_ODR_0_125HZ] =	{0, 125000},
 };
 
+const int bmp580_nvmem_addrs[] = { 0x20, 0x21, 0x22 };
+
+static int bmp580_nvmem_read(void *priv, unsigned int offset, void *val,
+			     size_t bytes)
+{
+	struct bmp280_data *data = priv;
+	u16 *dst = val;
+	int ret, addr;
+
+	pm_runtime_get_sync(data->dev);
+	mutex_lock(&data->lock);
+
+	/* Set sensor in standby mode */
+	ret = regmap_update_bits(data->regmap, BMP580_REG_ODR_CONFIG,
+				 BMP580_MODE_MASK | BMP580_ODR_DEEPSLEEP_DIS,
+				 BMP580_ODR_DEEPSLEEP_DIS |
+				 FIELD_PREP(BMP580_MODE_MASK, BMP580_MODE_SLEEP));
+	if (ret) {
+		dev_err(data->dev, "failed to change sensor to standby mode\n");
+		goto exit;
+	}
+	/* Wait standby transition time */
+	usleep_range(2500, 3000);
+
+	while (bytes >= sizeof(u16)) {
+		addr = bmp580_nvmem_addrs[offset / sizeof(u16)];
+
+		ret = regmap_write(data->regmap, BMP580_REG_NVM_ADDR,
+				   FIELD_PREP(BMP580_NVM_ROW_ADDR_MASK, addr));
+		if (ret) {
+			dev_err(data->dev, "error writing nvm address\n");
+			goto exit;
+		}
+
+		ret = bmp580_cmd(data, BMP580_NVM_READ_CMD);
+		if (ret)
+			goto exit;
+
+		ret = regmap_bulk_read(data->regmap, BMP580_REG_NVM_DATA_LSB, &data->le16,
+				       sizeof(data->le16));
+		if (ret) {
+			dev_err(data->dev, "error reading nvm data regs\n");
+			goto exit;
+		}
+
+		*dst++ = le16_to_cpu(data->le16);
+		bytes -= sizeof(u16);
+		offset += sizeof(u16);
+	}
+exit:
+	/* Restore chip config */
+	data->chip_info->chip_config(data);
+	mutex_unlock(&data->lock);
+	pm_runtime_mark_last_busy(data->dev);
+	pm_runtime_put_autosuspend(data->dev);
+	return ret;
+}
+
+static int bmp580_nvmem_write(void *priv, unsigned int offset, void *val,
+			      size_t bytes)
+{
+	struct bmp280_data *data = priv;
+	u16 *buf = val;
+	int ret, addr;
+
+	pm_runtime_get_sync(data->dev);
+	mutex_lock(&data->lock);
+
+	/* Set sensor in standby mode */
+	ret = regmap_update_bits(data->regmap, BMP580_REG_ODR_CONFIG,
+				 BMP580_MODE_MASK | BMP580_ODR_DEEPSLEEP_DIS,
+				 BMP580_ODR_DEEPSLEEP_DIS |
+				 FIELD_PREP(BMP580_MODE_MASK, BMP580_MODE_SLEEP));
+	if (ret) {
+		dev_err(data->dev, "failed to change sensor to standby mode\n");
+		goto exit;
+	}
+	/* Wait standby transition time */
+	usleep_range(2500, 3000);
+
+	while (bytes >= sizeof(u16)) {
+		addr = bmp580_nvmem_addrs[offset / sizeof(u16)];
+
+		ret = regmap_write(data->regmap, BMP580_REG_NVM_ADDR, BMP580_NVM_PROG_EN |
+				   FIELD_PREP(BMP580_NVM_ROW_ADDR_MASK, addr));
+		if (ret) {
+			dev_err(data->dev, "error writing nvm address\n");
+			goto exit;
+		}
+		data->le16 = cpu_to_le16(*buf++);
+
+		ret = regmap_bulk_write(data->regmap, BMP580_REG_NVM_DATA_LSB, &data->le16,
+					sizeof(data->le16));
+		if (ret) {
+			dev_err(data->dev, "error writing LSB NVM data regs\n");
+			goto exit;
+		}
+
+		ret = bmp580_cmd(data, BMP580_NVM_WRITE_CMD);
+		if (ret)
+			goto exit;
+
+		/* Disable programming mode bit */
+		ret = regmap_update_bits(data->regmap, BMP580_REG_NVM_ADDR,
+					 BMP580_NVM_PROG_EN, 0);
+		if (ret) {
+			dev_err(data->dev, "error resetting nvm write\n");
+			goto exit;
+		}
+
+		bytes -= sizeof(u16);
+		offset += sizeof(u16);
+	}
+exit:
+	/* Restore chip config */
+	data->chip_info->chip_config(data);
+	mutex_unlock(&data->lock);
+	pm_runtime_mark_last_busy(data->dev);
+	pm_runtime_put_autosuspend(data->dev);
+	return ret;
+}
+
 static int bmp580_preinit(struct bmp280_data *data)
 {
+	struct nvmem_config config = {
+		.dev = data->dev,
+		.priv = data,
+		.name = "bmp580_nvmem",
+		.word_size = sizeof(u16),
+		.stride = sizeof(u16),
+		.size = 3 * sizeof(u16),
+		.reg_read = bmp580_nvmem_read,
+		.reg_write = bmp580_nvmem_write,
+	};
 	unsigned int reg;
 	int ret;
 
@@ -1653,8 +1786,8 @@ static int bmp580_preinit(struct bmp280_data *data)
 		dev_err(data->dev, "preinit: nvm error on powerup sequence\n");
 		return -EIO;
 	}
-
-	return 0;
+	/* Register nvmem device */
+	return PTR_ERR_OR_ZERO(devm_nvmem_register(config.dev, &config));
 }
 
 static int bmp580_chip_config(struct bmp280_data *data)
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 27d2abc17d01..e2a093a4f767 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -101,6 +101,9 @@
 #define BMP580_FILTER_63X		6
 #define BMP580_FILTER_127X		7
 
+#define BMP580_NVM_ROW_ADDR_MASK	GENMASK(5, 0)
+#define BMP580_NVM_PROG_EN		BIT(6)
+
 #define BMP580_TEMP_SKIPPED		0x7f7f7f
 #define BMP580_PRESS_SKIPPED		0x7f7f7f
 
-- 
2.39.0

Re: [PATCH v2 5/5] iio: pressure: bmp280: Add nvmem operations for BMP580

[Jonathan Cameron]

On Mon, 26 Dec 2022 15:29:24 +0100
Angel Iglesias <ang.iglesiasg@gmail.com> wrote:

> The pressure sensor BMP580 contains a non-volatile memory that stores
> trimming and configuration params. That memory provides an programmable
> user range of three 2-byte words.
> 
> Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>

Not much in this one from me other than follow on from earlier patch.
Thanks,

Jonathan

> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 44901c6eb2f9..578d145be55d 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -28,6 +28,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/device.h>
>  #include <linux/module.h>
> +#include <linux/nvmem-provider.h>
>  #include <linux/regmap.h>
>  #include <linux/delay.h>
>  #include <linux/iio/iio.h>
> @@ -1628,8 +1629,140 @@ static const int bmp580_odr_table[][2] = {
>  	[BMP580_ODR_0_125HZ] =	{0, 125000},
>  };
>  
> +const int bmp580_nvmem_addrs[] = { 0x20, 0x21, 0x22 };
> +
> +static int bmp580_nvmem_read(void *priv, unsigned int offset, void *val,
> +			     size_t bytes)
> +{
> +	struct bmp280_data *data = priv;
> +	u16 *dst = val;
> +	int ret, addr;
> +
> +	pm_runtime_get_sync(data->dev);
> +	mutex_lock(&data->lock);
> +
> +	/* Set sensor in standby mode */
> +	ret = regmap_update_bits(data->regmap, BMP580_REG_ODR_CONFIG,
> +				 BMP580_MODE_MASK | BMP580_ODR_DEEPSLEEP_DIS,
> +				 BMP580_ODR_DEEPSLEEP_DIS |
> +				 FIELD_PREP(BMP580_MODE_MASK, BMP580_MODE_SLEEP));
> +	if (ret) {
> +		dev_err(data->dev, "failed to change sensor to standby mode\n");
> +		goto exit;
> +	}
> +	/* Wait standby transition time */
> +	usleep_range(2500, 3000);
> +
> +	while (bytes >= sizeof(u16)) {
> +		addr = bmp580_nvmem_addrs[offset / sizeof(u16)];
> +
> +		ret = regmap_write(data->regmap, BMP580_REG_NVM_ADDR,
> +				   FIELD_PREP(BMP580_NVM_ROW_ADDR_MASK, addr));
> +		if (ret) {
> +			dev_err(data->dev, "error writing nvm address\n");
> +			goto exit;
> +		}
> +
> +		ret = bmp580_cmd(data, BMP580_NVM_READ_CMD);
Ah. Here is the command being used.  Good to pull that code forwards to this patch.

> +		if (ret)
> +			goto exit;
> +
> +		ret = regmap_bulk_read(data->regmap, BMP580_REG_NVM_DATA_LSB, &data->le16,
> +				       sizeof(data->le16));
> +		if (ret) {
> +			dev_err(data->dev, "error reading nvm data regs\n");
> +			goto exit;
> +		}
> +
> +		*dst++ = le16_to_cpu(data->le16);
> +		bytes -= sizeof(u16);

sizeof(le16) seems more appropriate (obviously it's the same value).

> +		offset += sizeof(u16);
> +	}
> +exit:
> +	/* Restore chip config */
> +	data->chip_info->chip_config(data);
> +	mutex_unlock(&data->lock);
> +	pm_runtime_mark_last_busy(data->dev);
> +	pm_runtime_put_autosuspend(data->dev);
> +	return ret;
> +}
> +
> +static int bmp580_nvmem_write(void *priv, unsigned int offset, void *val,
> +			      size_t bytes)
> +{
> +	struct bmp280_data *data = priv;
> +	u16 *buf = val;
> +	int ret, addr;
> +
> +	pm_runtime_get_sync(data->dev);
> +	mutex_lock(&data->lock);
> +
> +	/* Set sensor in standby mode */
> +	ret = regmap_update_bits(data->regmap, BMP580_REG_ODR_CONFIG,
> +				 BMP580_MODE_MASK | BMP580_ODR_DEEPSLEEP_DIS,
> +				 BMP580_ODR_DEEPSLEEP_DIS |
> +				 FIELD_PREP(BMP580_MODE_MASK, BMP580_MODE_SLEEP));
> +	if (ret) {
> +		dev_err(data->dev, "failed to change sensor to standby mode\n");
> +		goto exit;
> +	}
> +	/* Wait standby transition time */
> +	usleep_range(2500, 3000);
> +
> +	while (bytes >= sizeof(u16)) {
> +		addr = bmp580_nvmem_addrs[offset / sizeof(u16)];
> +
> +		ret = regmap_write(data->regmap, BMP580_REG_NVM_ADDR, BMP580_NVM_PROG_EN |
> +				   FIELD_PREP(BMP580_NVM_ROW_ADDR_MASK, addr));
> +		if (ret) {
> +			dev_err(data->dev, "error writing nvm address\n");
> +			goto exit;
> +		}
> +		data->le16 = cpu_to_le16(*buf++);
> +
> +		ret = regmap_bulk_write(data->regmap, BMP580_REG_NVM_DATA_LSB, &data->le16,
> +					sizeof(data->le16));
> +		if (ret) {
> +			dev_err(data->dev, "error writing LSB NVM data regs\n");
> +			goto exit;
> +		}
> +
> +		ret = bmp580_cmd(data, BMP580_NVM_WRITE_CMD);
> +		if (ret)
> +			goto exit;
> +
> +		/* Disable programming mode bit */
> +		ret = regmap_update_bits(data->regmap, BMP580_REG_NVM_ADDR,
> +					 BMP580_NVM_PROG_EN, 0);
> +		if (ret) {
> +			dev_err(data->dev, "error resetting nvm write\n");
> +			goto exit;
> +		}
> +
> +		bytes -= sizeof(u16);

As above, maybe sizeof(le16)

> +		offset += sizeof(u16);
> +	}
> +exit:
> +	/* Restore chip config */
> +	data->chip_info->chip_config(data);
> +	mutex_unlock(&data->lock);
> +	pm_runtime_mark_last_busy(data->dev);
> +	pm_runtime_put_autosuspend(data->dev);
> +	return ret;
> +}
> +

Re: [PATCH v2 5/5] iio: pressure: bmp280: Add nvmem operations for BMP580

[Angel Iglesias]

On Fri, 2022-12-30 at 18:49 +0000, Jonathan Cameron wrote:
> On Mon, 26 Dec 2022 15:29:24 +0100
> Angel Iglesias <ang.iglesiasg@gmail.com> wrote:
> 
> > The pressure sensor BMP580 contains a non-volatile memory that stores
> > trimming and configuration params. That memory provides an programmable
> > user range of three 2-byte words.
> > 
> > Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>
> 
> Not much in this one from me other than follow on from earlier patch.
> Thanks,
> 
> Jonathan
> 
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c
> > b/drivers/iio/pressure/bmp280-core.c
> > index 44901c6eb2f9..578d145be55d 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/bitfield.h>
> >  #include <linux/device.h>
> >  #include <linux/module.h>
> > +#include <linux/nvmem-provider.h>
> >  #include <linux/regmap.h>
> >  #include <linux/delay.h>
> >  #include <linux/iio/iio.h>
> > @@ -1628,8 +1629,140 @@ static const int bmp580_odr_table[][2] = {
> >         [BMP580_ODR_0_125HZ] =  {0, 125000},
> >  };
> >  
> > +const int bmp580_nvmem_addrs[] = { 0x20, 0x21, 0x22 };
> > +
> > +static int bmp580_nvmem_read(void *priv, unsigned int offset, void *val,
> > +                            size_t bytes)
> > +{
> > +       struct bmp280_data *data = priv;
> > +       u16 *dst = val;
> > +       int ret, addr;
> > +
> > +       pm_runtime_get_sync(data->dev);
> > +       mutex_lock(&data->lock);
> > +
> > +       /* Set sensor in standby mode */
> > +       ret = regmap_update_bits(data->regmap, BMP580_REG_ODR_CONFIG,
> > +                                BMP580_MODE_MASK |
> > BMP580_ODR_DEEPSLEEP_DIS,
> > +                                BMP580_ODR_DEEPSLEEP_DIS |
> > +                                FIELD_PREP(BMP580_MODE_MASK,
> > BMP580_MODE_SLEEP));
> > +       if (ret) {
> > +               dev_err(data->dev, "failed to change sensor to standby
> > mode\n");
> > +               goto exit;
> > +       }
> > +       /* Wait standby transition time */
> > +       usleep_range(2500, 3000);
> > +
> > +       while (bytes >= sizeof(u16)) {
> > +               addr = bmp580_nvmem_addrs[offset / sizeof(u16)];
> > +
> > +               ret = regmap_write(data->regmap, BMP580_REG_NVM_ADDR,
> > +                                  FIELD_PREP(BMP580_NVM_ROW_ADDR_MASK,
> > addr));
> > +               if (ret) {
> > +                       dev_err(data->dev, "error writing nvm address\n");
> > +                       goto exit;
> > +               }
> > +
> > +               ret = bmp580_cmd(data, BMP580_NVM_READ_CMD);
> Ah. Here is the command being used.  Good to pull that code forwards to this
> patch.
> 
> > +               if (ret)
> > +                       goto exit;
> > +
> > +               ret = regmap_bulk_read(data->regmap,
> > BMP580_REG_NVM_DATA_LSB, &data->le16,
> > +                                      sizeof(data->le16));
> > +               if (ret) {
> > +                       dev_err(data->dev, "error reading nvm data regs\n");
> > +                       goto exit;
> > +               }
> > +
> > +               *dst++ = le16_to_cpu(data->le16);
> > +               bytes -= sizeof(u16);
> 
> sizeof(le16) seems more appropriate (obviously it's the same value).
> 
> > +               offset += sizeof(u16);
> > +       }
> > +exit:
> > +       /* Restore chip config */
> > +       data->chip_info->chip_config(data);
> > +       mutex_unlock(&data->lock);
> > +       pm_runtime_mark_last_busy(data->dev);
> > +       pm_runtime_put_autosuspend(data->dev);
> > +       return ret;
> > +}
> > +
> > +static int bmp580_nvmem_write(void *priv, unsigned int offset, void *val,
> > +                             size_t bytes)
> > +{
> > +       struct bmp280_data *data = priv;
> > +       u16 *buf = val;
> > +       int ret, addr;
> > +
> > +       pm_runtime_get_sync(data->dev);
> > +       mutex_lock(&data->lock);
> > +
> > +       /* Set sensor in standby mode */
> > +       ret = regmap_update_bits(data->regmap, BMP580_REG_ODR_CONFIG,
> > +                                BMP580_MODE_MASK |
> > BMP580_ODR_DEEPSLEEP_DIS,
> > +                                BMP580_ODR_DEEPSLEEP_DIS |
> > +                                FIELD_PREP(BMP580_MODE_MASK,
> > BMP580_MODE_SLEEP));
> > +       if (ret) {
> > +               dev_err(data->dev, "failed to change sensor to standby
> > mode\n");
> > +               goto exit;
> > +       }
> > +       /* Wait standby transition time */
> > +       usleep_range(2500, 3000);
> > +
> > +       while (bytes >= sizeof(u16)) {
> > +               addr = bmp580_nvmem_addrs[offset / sizeof(u16)];
> > +
> > +               ret = regmap_write(data->regmap, BMP580_REG_NVM_ADDR,
> > BMP580_NVM_PROG_EN |
> > +                                  FIELD_PREP(BMP580_NVM_ROW_ADDR_MASK,
> > addr));
> > +               if (ret) {
> > +                       dev_err(data->dev, "error writing nvm address\n");
> > +                       goto exit;
> > +               }
> > +               data->le16 = cpu_to_le16(*buf++);
> > +
> > +               ret = regmap_bulk_write(data->regmap,
> > BMP580_REG_NVM_DATA_LSB, &data->le16,
> > +                                       sizeof(data->le16));
> > +               if (ret) {
> > +                       dev_err(data->dev, "error writing LSB NVM data
> > regs\n");
> > +                       goto exit;
> > +               }
> > +
> > +               ret = bmp580_cmd(data, BMP580_NVM_WRITE_CMD);
> > +               if (ret)
> > +                       goto exit;
> > +
> > +               /* Disable programming mode bit */
> > +               ret = regmap_update_bits(data->regmap, BMP580_REG_NVM_ADDR,
> > +                                        BMP580_NVM_PROG_EN, 0);
> > +               if (ret) {
> > +                       dev_err(data->dev, "error resetting nvm write\n");
> > +                       goto exit;
> > +               }
> > +
> > +               bytes -= sizeof(u16);
> 
> As above, maybe sizeof(le16)
> 
> > +               offset += sizeof(u16);
> > +       }
> > +exit:
> > +       /* Restore chip config */
> > +       data->chip_info->chip_config(data);
> > +       mutex_unlock(&data->lock);
> > +       pm_runtime_mark_last_busy(data->dev);
> > +       pm_runtime_put_autosuspend(data->dev);
> > +       return ret;
> > +}
> > +
> 
> 
Got it,
Thanks for your time,
Angel

Re: [PATCH v2 4/5] dt-bindings: iio: pressure: bmp085: Add BMP580 compatible string

[Rob Herring]

On Mon, 26 Dec 2022 15:29:23 +0100, Angel Iglesias wrote:
> Add bosch,bmp580 to compatible string for the new family of sensors.
> This family includes the BMP580 and BMP581 sensors. The register map
> in this family presents significant departures from previous generations.
> 
> Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>
> 

Acked-by: Rob Herring <robh@kernel.org>