Add support for pressure sensor Bosch BMP580

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.

Quoted 3 lines · 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.

As per v1, use pointers in the ID tables.

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · 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.

Quoted 2 lines

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

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 <[email protected]>

drivers/iio/pressure/bmp280-core.c +6 -2

index c0aff78489b4..46959a91408f 100644
@@ -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:

drivers/iio/pressure/bmp280-core.c +2 -2

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

drivers/iio/pressure/bmp280-i2c.c +14 -14

index 14eab086d24a..59921e8cd592 100644
@@ -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);

drivers/iio/pressure/bmp280-spi.c +9 -9

index 011c68e07ebf..4a2df5b5d838 100644
@@ -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);

drivers/iio/pressure/bmp280.h +8 -0

index c791325c7416..efc31bc84708 100644
@@ -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;

Quoted 3 lines · On Mon, 26 Dec 2022 15:29:20 +0100 Angel Iglesias <[email protected]>…

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.

Quoted 62 lines

Signed-off-by Angel Iglesias <[email protected]>

drivers/iio/pressure/bmp280-core.c +0 -0

index c0aff78489b4..46959a91408f 100644
@@ -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;

drivers/iio/pressure/bmp280-core.c +0 -0

@@ -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,

drivers/iio/pressure/bmp280-core.c +0 -0

@@ -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,

drivers/iio/pressure/bmp280-core.c +0 -0

@@ -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,

drivers/iio/pressure/bmp280-core.c +0 -0

@@ -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,

drivers/iio/pressure/bmp280-core.c +0 -0

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

Quoted 8 lines

                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.

Quoted 131 lines

                break;
        default:

drivers/iio/pressure/bmp280-core.c +2 -2

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

drivers/iio/pressure/bmp280-i2c.c +0 -0

index 14eab086d24a..59921e8cd592 100644
@@ -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:

drivers/iio/pressure/bmp280-i2c.c +0 -0

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

drivers/iio/pressure/bmp280-spi.c +0 -0

index 011c68e07ebf..4a2df5b5d838 100644
@@ -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:

drivers/iio/pressure/bmp280-spi.c +0 -0

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

drivers/iio/pressure/bmp280.h +0 -0

index c791325c7416..efc31bc84708 100644
@@ -191,6 +191,14 @@
  #define BMP280_PRESS_SKIPPED           0x80000
  #define BMP280_HUMIDITY_SKIPPED                0x8000

drivers/iio/pressure/bmp280.h +8 -0

+/* 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;

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

drivers/iio/pressure/bmp280-core.c +1 -0

@@ -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)

}

Quoted 2 lines · Angel Iglesias

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

...

Quoted 10 lines

drivers/iio/pressure/bmp280-core.c +1 -0

@@ -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)

drivers/iio/pressure/bmp280-core.c static const struct bmp280_chip_info bme280_chip_info = · line 986

Quoted 6 lines

drivers/iio/pressure/bmp280-core.c +1 -0

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

...

drivers/iio/pressure/bmp280-core.c static const struct bmp280_chip_info bmp180_chip_info · line 1619

Quoted 7 lines

drivers/iio/pressure/bmp280-core.c +1 -0

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

...

Quoted 6 lines

drivers/iio/pressure/bmp280-core.c +6 -0

+       /*
+        * 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);

Quoted 1 line

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

Huh?! I guess you wanted

Quoted 2 lines

                if (ret < 0)
                        return ret;

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

Quoted 1 line

        }

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

Signed-off-by Angel Iglesias <[email protected]>

drivers/iio/pressure/bmp280-core.c +1 -0

index 46959a91408f..c37cf2caec68 100644
@@ -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 *);
 };
 
 /*

drivers/iio/pressure/bmp280-core.c +9 -0

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

drivers/iio/pressure/bmp280-core.c +0 -0

 	}

Quoted 19 lines · On Mon, 26 Dec 2022 15:29:21 +0100 Angel Iglesias <[email protected]>…

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

Signed-off-by Angel Iglesias <[email protected]>

drivers/iio/pressure/bmp280-core.c +1 -0

index 46959a91408f..c37cf2caec68 100644
@@ -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 *);
  };

 /*

drivers/iio/pressure/bmp280-core.c +0 -0

@@ -935,6 +936,7 @@ static const struct bmp280_chip_info bmp280_chip_info =

drivers/iio/pressure/bmp280-core.c static const struct bmp280_chip_info bmp280_chip_info = { · line 939

Quoted 5 lines

drivers/iio/pressure/bmp280-core.c +1 -0

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

Quoted 46 lines

drivers/iio/pressure/bmp280-core.c +0 -0

  };

 static int bme280_chip_config(struct bmp280_data *data)

drivers/iio/pressure/bmp280-core.c +1 -0

@@ -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,
  };

 /*

drivers/iio/pressure/bmp280-core.c +0 -0

@@ -1220,6 +1223,12 @@ static const int bmp380_odr_table[][2] = {
         [BMP380_ODR_0_0015HZ]   = {0, 1526},
  };

drivers/iio/pressure/bmp280-core.c +6 -0

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

drivers/iio/pressure/bmp280-core.c +1 -0

@@ -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)

drivers/iio/pressure/bmp280-core.c +1 -0

@@ -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)

drivers/iio/pressure/bmp280-core.c +0 -0

@@ -1762,9 +1773,13 @@ int bmp280_common_probe(struct device *dev,
                 return -EINVAL;
         }

Quoted 8 lines

-       /* 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.

Quoted 3 lines

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

Error message printed on success...

Quoted 3 lines

                if (ret < 0)
                        return ret;

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

Quoted 1 line

        }

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 <[email protected]>

drivers/iio/pressure/bmp280.h +0 -0

index efc31bc84708..27d2abc17d01 100644

+#define BMP580_CHIP_ID 0x50
#define BMP380_CHIP_ID 0x50

  #define BMP180_CHIP_ID			0x55
  #define BMP280_CHIP_ID			0x58

Le 26/12/2022 à 15:29, Angel Iglesias a écrit :

Quoted 9 lines

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 <[email protected]>

[...]

Quoted 4 lines

drivers/iio/pressure/bmp280.h +0 -0

index efc31bc84708..27d2abc17d01 100644

[...]

Quoted 2 lines

+#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

Quoted 2 lines

  #define BMP180_CHIP_ID                        0x55 #define BMP280_CHIP_ID                        0x58

Quoted 20 lines · Christophe JAILLET

Le 26/12/2022 à 15:29, Angel Iglesias a écrit :

Quoted 9 lines

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 <[email protected]>

[...]

Quoted 4 lines

drivers/iio/pressure/bmp280.h +0 -0

index efc31bc84708..27d2abc17d01 100644

[...]

Quoted 7 lines

Quoted 2 lines

+#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

Quoted 5 lines

CJ

Quoted 2 lines

  #define BMP180_CHIP_ID                        0x55 #define BMP280_CHIP_ID                        0x58

Quoted 42 lines · On Thu, 29 Dec 2022 19:23:16 +0100 Angel Iglesias <[email protected]>…

Quoted 21 lines · Christophe JAILLET

Le 26/12/2022 à 15:29, Angel Iglesias a écrit :

Quoted 9 lines

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 <[email protected]>

[...]

drivers/iio/pressure/bmp280.h

Quoted 5 lines

drivers/iio/pressure/bmp280.h +0 -0

index efc31bc84708..27d2abc17d01 100644

[...]

Quoted 7 lines

Quoted 2 lines

+#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

Quoted 9 lines

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

Quoted 10 lines

Angel

Quoted 5 lines

CJ

Quoted 2 lines

  #define BMP180_CHIP_ID                        0x55 #define BMP280_CHIP_ID                        0x58

Quoted 60 lines · Jonathan Cameron

Quoted 42 lines · On Thu, 29 Dec 2022 19:23:16 +0100 Angel Iglesias <[email protected]>…

Quoted 21 lines · Christophe JAILLET

Le 26/12/2022 à 15:29, Angel Iglesias a écrit :

Quoted 9 lines

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 <[email protected]>

[...]

drivers/iio/pressure/bmp280.h

Quoted 5 lines

drivers/iio/pressure/bmp280.h +0 -0

index efc31bc84708..27d2abc17d01 100644

[...]

Quoted 7 lines

Quoted 2 lines

+#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

Quoted 9 lines

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?

Quoted 1 line

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

Quoted 11 lines

Quoted 10 lines

Angel

Quoted 5 lines

CJ

Quoted 2 lines

  #define BMP180_CHIP_ID                        0x55 #define BMP280_CHIP_ID                        0x58

Quoted 84 lines · Jonathan Cameron

Quoted 61 lines · On Thu, 29 Dec 2022 19:23:16 +0100 Angel Iglesias <[email protected]>…

Quoted 21 lines · Christophe JAILLET

Le 26/12/2022 à 15:29, Angel Iglesias a écrit :

Quoted 9 lines

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 <[email protected]>

[...]

drivers/iio/pressure/bmp280.h

Quoted 5 lines

(quoted diff) +0 -0

b/drivers/iio/pressure/bmp280.h index
efc31bc84708..27d2abc17d01 100644

drivers/iio/pressure/bmp280.h +0 -0

[...]

Quoted 7 lines

Quoted 2 lines

+#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

Quoted 8 lines

*
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?

Quoted 1 line

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

Quoted 11 lines

Quoted 10 lines

Angel

Quoted 5 lines

CJ

Quoted 2 lines

  #define BMP180_CHIP_ID                        0x55 #define BMP280_CHIP_ID                        0x58

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 <[email protected]>

drivers/iio/pressure/Kconfig +2 -2

 	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

drivers/iio/pressure/Kconfig +5 -0

+{
+	unsigned long deadline;
+	unsigned int reg;
+	int ret;
+

drivers/iio/pressure/Kconfig +83 -0

+		/* 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);

drivers/iio/pressure/Kconfig +4 -0

+	}
+
+	return 0;
+}

drivers/iio/pressure/Kconfig +14 -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);

drivers/iio/pressure/Kconfig +2 -0

+	return IIO_VAL_FRACTIONAL;
+}

drivers/iio/pressure/Kconfig +14 -0

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

drivers/iio/pressure/Kconfig +18 -0

+
+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,

drivers/iio/pressure/Kconfig +6 -0

+	.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;
 	}

drivers/iio/pressure/Kconfig +1 -0

+		}
 	}
 
 	ret = data->chip_info->chip_config(data);

drivers/iio/pressure/Kconfig +5 -0

+		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

drivers/iio/pressure/bmp280-i2c.c +0 -0

index 59921e8cd592..c52d2b477bb7 100644

drivers/iio/pressure/bmp280-i2c.c +3 -0

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

drivers/iio/pressure/bmp280-spi.c +0 -0

index 4a2df5b5d838..5653c3c33081 100644

drivers/iio/pressure/bmp280-spi.c +3 -0

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

drivers/iio/pressure/bmp280.h +0 -0

index efc31bc84708..27d2abc17d01 100644
@@ -3,6 +3,107 @@

+#define BMP580_CHIP_ID 0x50
#define BMP380_CHIP_ID 0x50

#define BMP180_CHIP_ID 0x55
#define BMP280_CHIP_ID 0x58

drivers/iio/pressure/bmp280.h +2 -0

@@ -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,

Quoted 7 lines · On Mon, 26 Dec 2022 15:29:22 +0100 Angel Iglesias <[email protected]>…

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 <[email protected]>

Hi Angel,

Some comments inline,

Thanks,

Jonathan

Quoted 6 lines

drivers/iio/pressure/Kconfig +0 -0

index c9453389e4f7..1c18e3b2c501 100644
@@ -17,14 +17,14 @@ config ABP060MG

Quoted 7 lines

          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.

Quoted 16 lines

        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

...

Quoted 10 lines

drivers/iio/pressure/Kconfig +10 -0

+
+/*
+ * 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.

Quoted 2 lines

+ */
+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.

Quoted 9 lines

drivers/iio/pressure/Kconfig +9 -0

+{
+       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,

Quoted 8 lines

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.

Quoted 7 lines

+               /* 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.

Quoted 106 lines

+               /* 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;
+               }

Quoted 8 lines

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

Quoted 4 lines

+       }

drivers/iio/pressure/Kconfig +3 -0

+
+       return 0;
+}

...

Quoted 17 lines

drivers/iio/pressure/Kconfig +7 -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-

Quoted 1 line

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

Quoted 8 lines

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

Quoted 2 lines

+       return IIO_VAL_FRACTIONAL;
+}

...

drivers/iio/pressure/Kconfig

Quoted 9 lines

drivers/iio/pressure/Kconfig +9 -0

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

Quoted 6 lines

+       /* 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.

Quoted 16 lines

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

drivers/iio/pressure/Kconfig +4 -0

+
+       return 0;
+}
+

...

Quoted 4 lines

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

Quoted 25 lines

drivers/iio/pressure/Kconfig +10 -0

+
+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),

drivers/iio/pressure/Kconfig +7 -0

+
+       .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 =

Quoted 7 lines

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.

Quoted 17 lines

+       .preinit = bmp580_preinit,

drivers/iio/pressure/Kconfig +2 -0

+};
+

 static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas) {
        const int conversion_time_max[] = { 4500, 7500, 13500, 25500 };

drivers/iio/pressure/Kconfig +0 -0

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

drivers/iio/pressure/Kconfig int bmp280_common_probe(struct device *dev, · line 2216

Quoted 8 lines

drivers/iio/pressure/Kconfig +2 -2

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

Quoted 14 lines

drivers/iio/pressure/Kconfig +1 -0

+               }
         }

        ret = data->chip_info->chip_config(data);

drivers/iio/pressure/Kconfig +0 -0

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

Quoted 2 lines

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.

Quoted 16 lines

+               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

drivers/iio/pressure/bmp280-i2c.c +0 -0

index 59921e8cd592..c52d2b477bb7 100644
@@ -22,6 +22,9 @@ static int bmp280_i2c_probe(struct i2c_client *client)

Quoted 5 lines

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

Quoted 27 lines

+               break;
        default:
                return -EINVAL;
        }

drivers/iio/pressure/bmp280-i2c.c +2 -0

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

drivers/iio/pressure/bmp280-spi.c +0 -0

index 4a2df5b5d838..5653c3c33081 100644
@@ -69,6 +69,9 @@ static int bmp280_spi_probe(struct spi_device *spi)

Quoted 5 lines

        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.

Quoted 26 lines

+               break;
        default:
                return -EINVAL;
        }

drivers/iio/pressure/bmp280-spi.c +2 -0

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

drivers/iio/pressure/bmp280.h +0 -0

index efc31bc84708..27d2abc17d01 100644
@@ -3,6 +3,107 @@

...

Quoted 2 lines

+#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.

Quoted 10 lines

 #define BMP180_CHIP_ID                 0x55
 #define BMP280_CHIP_ID                 0x58

drivers/iio/pressure/bmp280.h +1 -0

@@ -197,12 +299,14 @@ enum bmp280_variant {
         BMP280,
         BME280,
         BMP380,
+       BMP580,
  };

Quoted 8 lines

 /* 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,

Quoted 10 lines

Quoted 5 lines

+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?

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 <[email protected]>

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 <[email protected]>

drivers/iio/pressure/bmp280-core.c +26 -0

index 44901c6eb2f9..578d145be55d 100644
@@ -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)) {

drivers/iio/pressure/bmp280-core.c +54 -0

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

drivers/iio/pressure/bmp280-core.c +11 -0

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

Quoted 5 lines · On Mon, 26 Dec 2022 15:29:24 +0100 Angel Iglesias <[email protected]>…

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 <[email protected]>

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

Jonathan

Quoted 50 lines

drivers/iio/pressure/bmp280-core.c +1 -0

index 44901c6eb2f9..578d145be55d 100644
@@ -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},
  };

drivers/iio/pressure/bmp280-core.c +15 -0

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

drivers/iio/pressure/bmp280-core.c +6 -0

+
+       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,

Quoted 7 lines

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.

Quoted 5 lines

+               if (ret)
+                       goto exit;
+
+               ret = regmap_bulk_read(data->regmap, BMP580_REG_NVM_DATA_LSB, &data->le16,

Quoted 8 lines

+                                      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).

Quoted 61 lines

+               offset += sizeof(u16);
+       }

drivers/iio/pressure/bmp280-core.c +22 -0

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

drivers/iio/pressure/bmp280-core.c +5 -0

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

drivers/iio/pressure/bmp280-core.c +4 -0

+
+               ret = bmp580_cmd(data, BMP580_NVM_WRITE_CMD);
+               if (ret)
+                       goto exit;

drivers/iio/pressure/bmp280-core.c static const int bmp580_odr_table[][2] = { · line 1742

Quoted 10 lines

drivers/iio/pressure/bmp280-core.c +10 -0

+
+               /* 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)

drivers/iio/pressure/bmp280-core.c static const int bmp580_odr_table[][2] = { · line 1689

Quoted 11 lines

+               offset += sizeof(u16);
+       }

drivers/iio/pressure/bmp280-core.c +9 -0

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

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 <[email protected]>