On Sun, Jan 29, 2023 at 02:33:09AM +0100, Angel Iglesias 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.
...
> +#define NVM_READ false
> +#define NVM_WRITE true
How is it helpful and why it's not namespaced properly (can collide with
NVM framework)?
...
> + /* 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));
regmap_read_poll_timeout()?
> + if (!(reg & BMP580_STATUS_NVM_RDY_MASK)) {
if (ret) {
...
return ret;
}
> + dev_err(data->dev,
> + "reached timeout waiting for nvm operation completion\n");
> + return -ETIMEDOUT;
> + }
...
> + while (bytes >= sizeof(u16)) {
sizeof(*dst) ?
Or sizeof(data->le16)?
> + addr = bmp580_nvmem_addrs[offset / sizeof(u16)];
Ditto.
...
> + bytes -= sizeof(u16);
> + offset += sizeof(u16);
Ditto.
> + }
...
> +static int bmp580_nvmem_write(void *priv, unsigned int offset, void *val,
> + size_t bytes)
> +{
Same comments as per above function.
> }
On Mon, 2023-01-30 at 14:29 +0200, Andy Shevchenko wrote:
> On Sun, Jan 29, 2023 at 02:33:09AM +0100, Angel Iglesias 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.
>
> ...
>
> > +#define NVM_READ false
> > +#define NVM_WRITE true
>
> How is it helpful and why it's not namespaced properly (can collide with
> NVM framework)?
I thought it could provide clarity using those aliases calling
bmp580_nvm_operation() helper function on nvmen callbacks, instead of using
just true or false. Not my brightest idea for sure.
> ...
>
> > + /* 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));
>
> regmap_read_poll_timeout()?
Yes, thanks for pointing out, that's way better than the current loop.
>
> > + if (!(reg & BMP580_STATUS_NVM_RDY_MASK)) {
>
> if (ret) {
> ...
> return ret;
> }
>
> > + dev_err(data->dev,
> > + "reached timeout waiting for nvm operation
> > completion\n");
> > + return -ETIMEDOUT;
> > + }
>
> ...
>
> > + while (bytes >= sizeof(u16)) {
>
> sizeof(*dst) ?
>
> Or sizeof(data->le16)?
>
> > + addr = bmp580_nvmem_addrs[offset / sizeof(u16)];
>
> Ditto.
>
> ...
>
> > + bytes -= sizeof(u16);
> > + offset += sizeof(u16);
>
> Ditto.
>
> > + }
>
> ...
>
> > +static int bmp580_nvmem_write(void *priv, unsigned int offset, void *val,
> > + size_t bytes)
> > +{
>
> Same comments as per above function.
Got it!
> > }
>
>
Thanks for your time,
Angel
@@ -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>
@@ -1290,6 +1291,88 @@ static int bmp580_soft_reset(struct bmp280_data *data)
return 0;
}
+#define NVM_READ false
+#define NVM_WRITE true
+
+/**
+ * bmp580_nvm_operation() - Helper function to commit NVM memory operations
+ * @data: sensor data struct
+ * @is_write: flag to signal write operation
+ */
+static int bmp580_nvm_operation(struct bmp280_data *data, bool is_write)
+{
+ unsigned long deadline;
+ unsigned int reg;
+ int ret;
+
+ /* 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 (is_write) {
+ /* Send NVM 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 NVM 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;
+ }
+
+ /* Check 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;
+ }
+
+ 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
@@ -1384,8 +1467,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_nvm_operation(data, NVM_READ);
+ 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_nvm_operation(data, NVM_WRITE);
+ 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;
@@ -1413,7 +1628,8 @@ static int bmp580_preinit(struct bmp280_data *data)
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)
@@ -104,6 +104,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