* [PATCH v2 0/5] Add support for pressure sensor Bosch BMP580 @ 2022-12-26 14:29 Angel Iglesias 2022-12-26 14:29 ` [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants Angel Iglesias ` (5 more replies) 0 siblings, 6 replies; 28+ messages in thread From: Angel Iglesias @ 2022-12-26 14:29 UTC (permalink / raw) To: linux-iio Cc: Angel Iglesias, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Andy Shevchenko, Nikita Yushchenko, Rafael J. Wysocki, Paul Cercueil, Ulf Hansson, Andreas Klinger, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants 2022-12-26 14:29 [PATCH v2 0/5] Add support for pressure sensor Bosch BMP580 Angel Iglesias @ 2022-12-26 14:29 ` Angel Iglesias 2022-12-27 21:37 ` Andy Shevchenko 2022-12-30 18:14 ` Jonathan Cameron 2022-12-26 14:29 ` [PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback Angel Iglesias ` (4 subsequent siblings) 5 siblings, 2 replies; 28+ messages in thread From: Angel Iglesias @ 2022-12-26 14:29 UTC (permalink / raw) To: linux-iio Cc: Angel Iglesias, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Andy Shevchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants 2022-12-26 14:29 ` [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants Angel Iglesias @ 2022-12-27 21:37 ` Andy Shevchenko 2023-01-01 11:04 ` Angel Iglesias 2022-12-30 18:14 ` Jonathan Cameron 1 sibling, 1 reply; 28+ messages in thread From: Andy Shevchenko @ 2022-12-27 21:37 UTC (permalink / raw) To: Angel Iglesias Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants 2022-12-27 21:37 ` Andy Shevchenko @ 2023-01-01 11:04 ` Angel Iglesias 2023-01-08 12:41 ` Jonathan Cameron 0 siblings, 1 reply; 28+ messages in thread From: Angel Iglesias @ 2023-01-01 11:04 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants 2023-01-01 11:04 ` Angel Iglesias @ 2023-01-08 12:41 ` Jonathan Cameron 0 siblings, 0 replies; 28+ messages in thread From: Jonathan Cameron @ 2023-01-08 12:41 UTC (permalink / raw) To: Angel Iglesias Cc: Andy Shevchenko, linux-iio, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants 2022-12-26 14:29 ` [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants Angel Iglesias 2022-12-27 21:37 ` Andy Shevchenko @ 2022-12-30 18:14 ` Jonathan Cameron 2023-01-01 10:56 ` Angel Iglesias 1 sibling, 1 reply; 28+ messages in thread From: Jonathan Cameron @ 2022-12-30 18:14 UTC (permalink / raw) To: Angel Iglesias Cc: linux-iio, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Andy Shevchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel 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; ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants 2022-12-30 18:14 ` Jonathan Cameron @ 2023-01-01 10:56 ` Angel Iglesias 0 siblings, 0 replies; 28+ messages in thread From: Angel Iglesias @ 2023-01-01 10:56 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-iio, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Andy Shevchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel 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; > ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback 2022-12-26 14:29 [PATCH v2 0/5] Add support for pressure sensor Bosch BMP580 Angel Iglesias 2022-12-26 14:29 ` [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants Angel Iglesias @ 2022-12-26 14:29 ` Angel Iglesias 2022-12-27 21:41 ` Andy Shevchenko 2022-12-30 18:18 ` Jonathan Cameron 2022-12-26 14:29 ` [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 Angel Iglesias ` (3 subsequent siblings) 5 siblings, 2 replies; 28+ messages in thread From: Angel Iglesias @ 2022-12-26 14:29 UTC (permalink / raw) To: linux-iio Cc: Angel Iglesias, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Andy Shevchenko, Nikita Yushchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback 2022-12-26 14:29 ` [PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback Angel Iglesias @ 2022-12-27 21:41 ` Andy Shevchenko 2023-01-01 11:06 ` Angel Iglesias 2022-12-30 18:18 ` Jonathan Cameron 1 sibling, 1 reply; 28+ messages in thread From: Andy Shevchenko @ 2022-12-27 21:41 UTC (permalink / raw) To: Angel Iglesias Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback 2022-12-27 21:41 ` Andy Shevchenko @ 2023-01-01 11:06 ` Angel Iglesias 0 siblings, 0 replies; 28+ messages in thread From: Angel Iglesias @ 2023-01-01 11:06 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel 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(...); > > > } > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback 2022-12-26 14:29 ` [PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback Angel Iglesias 2022-12-27 21:41 ` Andy Shevchenko @ 2022-12-30 18:18 ` Jonathan Cameron 2023-01-01 11:09 ` Angel Iglesias 1 sibling, 1 reply; 28+ messages in thread From: Jonathan Cameron @ 2022-12-30 18:18 UTC (permalink / raw) To: Angel Iglesias Cc: linux-iio, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Andy Shevchenko, Nikita Yushchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel 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"); > } ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback 2022-12-30 18:18 ` Jonathan Cameron @ 2023-01-01 11:09 ` Angel Iglesias 0 siblings, 0 replies; 28+ messages in thread From: Angel Iglesias @ 2023-01-01 11:09 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-iio, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Andy Shevchenko, Nikita Yushchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 2022-12-26 14:29 [PATCH v2 0/5] Add support for pressure sensor Bosch BMP580 Angel Iglesias 2022-12-26 14:29 ` [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants Angel Iglesias 2022-12-26 14:29 ` [PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback Angel Iglesias @ 2022-12-26 14:29 ` Angel Iglesias 2022-12-29 17:35 ` Christophe JAILLET 2022-12-30 18:45 ` Jonathan Cameron 2022-12-26 14:29 ` [PATCH v2 4/5] dt-bindings: iio: pressure: bmp085: Add BMP580 compatible string Angel Iglesias ` (2 subsequent siblings) 5 siblings, 2 replies; 28+ messages in thread From: Angel Iglesias @ 2022-12-26 14:29 UTC (permalink / raw) To: linux-iio Cc: Angel Iglesias, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Andy Shevchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel 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, ®); + 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, ®); + 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, ®); + 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, ®); + 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, ®); + 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, ®); + 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 2022-12-26 14:29 ` [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 Angel Iglesias @ 2022-12-29 17:35 ` Christophe JAILLET 2022-12-29 18:23 ` Angel Iglesias 2022-12-30 18:45 ` Jonathan Cameron 1 sibling, 1 reply; 28+ messages in thread From: Christophe JAILLET @ 2022-12-29 17:35 UTC (permalink / raw) To: ang.iglesiasg Cc: ak, andriy.shevchenko, devicetree, jic23, krzysztof.kozlowski+dt, lars, linux-iio, linux-kernel, nikita.yoush, paul, rafael.j.wysocki, robh+dt, ulf.hansson 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 2022-12-29 17:35 ` Christophe JAILLET @ 2022-12-29 18:23 ` Angel Iglesias 2022-12-30 18:22 ` Jonathan Cameron 0 siblings, 1 reply; 28+ messages in thread From: Angel Iglesias @ 2022-12-29 18:23 UTC (permalink / raw) To: Christophe JAILLET Cc: ak, andriy.shevchenko, devicetree, jic23, krzysztof.kozlowski+dt, lars, linux-iio, linux-kernel, nikita.yoush, paul, rafael.j.wysocki, robh+dt, ulf.hansson 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 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 2022-12-29 18:23 ` Angel Iglesias @ 2022-12-30 18:22 ` Jonathan Cameron 2023-01-01 11:16 ` Angel Iglesias 0 siblings, 1 reply; 28+ messages in thread From: Jonathan Cameron @ 2022-12-30 18:22 UTC (permalink / raw) To: Angel Iglesias Cc: Christophe JAILLET, ak, andriy.shevchenko, devicetree, krzysztof.kozlowski+dt, lars, linux-iio, linux-kernel, nikita.yoush, paul, rafael.j.wysocki, robh+dt, ulf.hansson 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 > > > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 2022-12-30 18:22 ` Jonathan Cameron @ 2023-01-01 11:16 ` Angel Iglesias 2023-01-08 12:35 ` Jonathan Cameron 0 siblings, 1 reply; 28+ messages in thread From: Angel Iglesias @ 2023-01-01 11:16 UTC (permalink / raw) To: Jonathan Cameron Cc: Christophe JAILLET, ak, andriy.shevchenko, devicetree, krzysztof.kozlowski+dt, lars, linux-iio, linux-kernel, nikita.yoush, paul, rafael.j.wysocki, robh+dt, ulf.hansson 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 > > > > > > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 2023-01-01 11:16 ` Angel Iglesias @ 2023-01-08 12:35 ` Jonathan Cameron 2023-01-12 10:38 ` Contact Bosch-Sensortec (BST/SA) 0 siblings, 1 reply; 28+ messages in thread From: Jonathan Cameron @ 2023-01-08 12:35 UTC (permalink / raw) To: Angel Iglesias Cc: Christophe JAILLET, ak, andriy.shevchenko, devicetree, krzysztof.kozlowski+dt, lars, linux-iio, linux-kernel, nikita.yoush, paul, rafael.j.wysocki, robh+dt, ulf.hansson, contact 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 > > > > > > > > > > ^ permalink raw reply [flat|nested] 28+ messages in thread
* RE: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 2023-01-08 12:35 ` Jonathan Cameron @ 2023-01-12 10:38 ` Contact Bosch-Sensortec (BST/SA) 0 siblings, 0 replies; 28+ messages in thread From: Contact Bosch-Sensortec (BST/SA) @ 2023-01-12 10:38 UTC (permalink / raw) To: Jonathan Cameron, Angel Iglesias Cc: Christophe JAILLET, ak, andriy.shevchenko, devicetree, krzysztof.kozlowski+dt, lars, linux-iio, linux-kernel, nikita.yoush, paul, rafael.j.wysocki, robh+dt, ulf.hansson 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 > > > > > > > > > > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 2022-12-26 14:29 ` [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 Angel Iglesias 2022-12-29 17:35 ` Christophe JAILLET @ 2022-12-30 18:45 ` Jonathan Cameron 2023-01-01 11:46 ` Angel Iglesias 1 sibling, 1 reply; 28+ messages in thread From: Jonathan Cameron @ 2022-12-30 18:45 UTC (permalink / raw) To: Angel Iglesias Cc: linux-iio, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Andy Shevchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel 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, ®); > + 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, ®); > + 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, ®); > + 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, ®); > + 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, ®); > + 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, ®); > + 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, ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 2022-12-30 18:45 ` Jonathan Cameron @ 2023-01-01 11:46 ` Angel Iglesias 2023-01-08 12:38 ` Jonathan Cameron 0 siblings, 1 reply; 28+ messages in thread From: Angel Iglesias @ 2023-01-01 11:46 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-iio, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Andy Shevchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel 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, ®); > > + 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, > > ®); > > + 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, ®); > > + 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, > > ®); > > + 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, ®); > > + 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, ®); > > + 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 2023-01-01 11:46 ` Angel Iglesias @ 2023-01-08 12:38 ` Jonathan Cameron 0 siblings, 0 replies; 28+ messages in thread From: Jonathan Cameron @ 2023-01-08 12:38 UTC (permalink / raw) To: Angel Iglesias Cc: linux-iio, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Andy Shevchenko, Paul Cercueil, Ulf Hansson, Rafael J. Wysocki, Andreas Klinger, devicetree, linux-kernel > > > +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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 4/5] dt-bindings: iio: pressure: bmp085: Add BMP580 compatible string 2022-12-26 14:29 [PATCH v2 0/5] Add support for pressure sensor Bosch BMP580 Angel Iglesias ` (2 preceding siblings ...) 2022-12-26 14:29 ` [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 Angel Iglesias @ 2022-12-26 14:29 ` Angel Iglesias 2022-12-27 8:11 ` Krzysztof Kozlowski 2022-12-26 14:29 ` [PATCH v2 5/5] iio: pressure: bmp280: Add nvmem operations for BMP580 Angel Iglesias 2022-12-26 22:05 ` [PATCH v2 4/5] dt-bindings: iio: pressure: bmp085: Add BMP580 compatible string Rob Herring 5 siblings, 1 reply; 28+ messages in thread From: Angel Iglesias @ 2022-12-26 14:29 UTC (permalink / raw) To: linux-iio Cc: Angel Iglesias, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Andy Shevchenko, Paul Cercueil, Rafael J. Wysocki, Ulf Hansson, Andreas Klinger, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] dt-bindings: iio: pressure: bmp085: Add BMP580 compatible string 2022-12-26 14:29 ` [PATCH v2 4/5] dt-bindings: iio: pressure: bmp085: Add BMP580 compatible string Angel Iglesias @ 2022-12-27 8:11 ` Krzysztof Kozlowski 0 siblings, 0 replies; 28+ messages in thread From: Krzysztof Kozlowski @ 2022-12-27 8:11 UTC (permalink / raw) To: Angel Iglesias, linux-iio Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Andy Shevchenko, Paul Cercueil, Rafael J. Wysocki, Ulf Hansson, Andreas Klinger, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 5/5] iio: pressure: bmp280: Add nvmem operations for BMP580 2022-12-26 14:29 [PATCH v2 0/5] Add support for pressure sensor Bosch BMP580 Angel Iglesias ` (3 preceding siblings ...) 2022-12-26 14:29 ` [PATCH v2 4/5] dt-bindings: iio: pressure: bmp085: Add BMP580 compatible string Angel Iglesias @ 2022-12-26 14:29 ` Angel Iglesias 2022-12-30 18:49 ` Jonathan Cameron 2022-12-26 22:05 ` [PATCH v2 4/5] dt-bindings: iio: pressure: bmp085: Add BMP580 compatible string Rob Herring 5 siblings, 1 reply; 28+ messages in thread From: Angel Iglesias @ 2022-12-26 14:29 UTC (permalink / raw) To: linux-iio Cc: Angel Iglesias, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Andy Shevchenko, Ulf Hansson, Rafael J. Wysocki, Paul Cercueil, Andreas Klinger, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/5] iio: pressure: bmp280: Add nvmem operations for BMP580 2022-12-26 14:29 ` [PATCH v2 5/5] iio: pressure: bmp280: Add nvmem operations for BMP580 Angel Iglesias @ 2022-12-30 18:49 ` Jonathan Cameron 2023-01-01 11:48 ` Angel Iglesias 0 siblings, 1 reply; 28+ messages in thread From: Jonathan Cameron @ 2022-12-30 18:49 UTC (permalink / raw) To: Angel Iglesias Cc: linux-iio, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Andy Shevchenko, Ulf Hansson, Rafael J. Wysocki, Paul Cercueil, Andreas Klinger, devicetree, linux-kernel 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; > +} > + ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/5] iio: pressure: bmp280: Add nvmem operations for BMP580 2022-12-30 18:49 ` Jonathan Cameron @ 2023-01-01 11:48 ` Angel Iglesias 0 siblings, 0 replies; 28+ messages in thread From: Angel Iglesias @ 2023-01-01 11:48 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-iio, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Nikita Yushchenko, Andy Shevchenko, Ulf Hansson, Rafael J. Wysocki, Paul Cercueil, Andreas Klinger, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] dt-bindings: iio: pressure: bmp085: Add BMP580 compatible string 2022-12-26 14:29 [PATCH v2 0/5] Add support for pressure sensor Bosch BMP580 Angel Iglesias ` (4 preceding siblings ...) 2022-12-26 14:29 ` [PATCH v2 5/5] iio: pressure: bmp280: Add nvmem operations for BMP580 Angel Iglesias @ 2022-12-26 22:05 ` Rob Herring 5 siblings, 0 replies; 28+ messages in thread From: Rob Herring @ 2022-12-26 22:05 UTC (permalink / raw) To: Angel Iglesias Cc: devicetree, Ulf Hansson, Rob Herring, Andreas Klinger, Rafael J. Wysocki, Krzysztof Kozlowski, Lars-Peter Clausen, Nikita Yushchenko, Andy Shevchenko, Paul Cercueil, linux-iio, Jonathan Cameron, linux-kernel 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> ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2023-01-12 10:44 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-26 14:29 [PATCH v2 0/5] Add support for pressure sensor Bosch BMP580 Angel Iglesias 2022-12-26 14:29 ` [PATCH v2 1/5] iio: pressure: bmp280: Add enumeration to handle chip variants Angel Iglesias 2022-12-27 21:37 ` Andy Shevchenko 2023-01-01 11:04 ` Angel Iglesias 2023-01-08 12:41 ` Jonathan Cameron 2022-12-30 18:14 ` Jonathan Cameron 2023-01-01 10:56 ` Angel Iglesias 2022-12-26 14:29 ` [PATCH v2 2/5] iio: pressure: bmp280: Add preinit callback Angel Iglesias 2022-12-27 21:41 ` Andy Shevchenko 2023-01-01 11:06 ` Angel Iglesias 2022-12-30 18:18 ` Jonathan Cameron 2023-01-01 11:09 ` Angel Iglesias 2022-12-26 14:29 ` [PATCH v2 3/5] iio: pressure: bmp280: Add support for new sensor BMP580 Angel Iglesias 2022-12-29 17:35 ` Christophe JAILLET 2022-12-29 18:23 ` Angel Iglesias 2022-12-30 18:22 ` Jonathan Cameron 2023-01-01 11:16 ` Angel Iglesias 2023-01-08 12:35 ` Jonathan Cameron 2023-01-12 10:38 ` Contact Bosch-Sensortec (BST/SA) 2022-12-30 18:45 ` Jonathan Cameron 2023-01-01 11:46 ` Angel Iglesias 2023-01-08 12:38 ` Jonathan Cameron 2022-12-26 14:29 ` [PATCH v2 4/5] dt-bindings: iio: pressure: bmp085: Add BMP580 compatible string Angel Iglesias 2022-12-27 8:11 ` Krzysztof Kozlowski 2022-12-26 14:29 ` [PATCH v2 5/5] iio: pressure: bmp280: Add nvmem operations for BMP580 Angel Iglesias 2022-12-30 18:49 ` Jonathan Cameron 2023-01-01 11:48 ` Angel Iglesias 2022-12-26 22:05 ` [PATCH v2 4/5] dt-bindings: iio: pressure: bmp085: Add BMP580 compatible string Rob Herring
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.