regmap: allow volatile register writes with cached only read maps

Regmap only allows volatile access to registers when the client supports both reads and writes.

This commit bypasses that limitation and enables volatile writes to selected registers while maintaining cached accesses on all reads. For this, the client does not need to configure the reg_read callback.

---
 drivers/base/regmap/internal.h       |  1 +
 drivers/base/regmap/regcache.c       | 10 +++++++++-
 drivers/base/regmap/regmap-debugfs.c | 17 +++++++++++++++--
 drivers/base/regmap/regmap.c         | 11 +++++++++++
 4 files changed, 36 insertions(+), 3 deletions(-)

index 53785e0..496c825 100644
@@ -180,6 +180,7 @@ bool regmap_cached(struct regmap *map, unsigned int reg);
 bool regmap_writeable(struct regmap *map, unsigned int reg);
 bool regmap_readable(struct regmap *map, unsigned int reg);
 bool regmap_volatile(struct regmap *map, unsigned int reg);
+bool regmap_volatile_writes(struct regmap *map, unsigned int reg);
 bool regmap_precious(struct regmap *map, unsigned int reg);
 
 int _regmap_write(struct regmap *map, unsigned int reg,

index 7735603..51cceaa 100644
@@ -273,8 +273,16 @@ int regcache_write(struct regmap *map,
 
 	BUG_ON(!map->cache_ops);
 
-	if (!regmap_volatile(map, reg))
+	if (!regmap_volatile(map, reg)) {
+
+		if (!regmap_readable(map, reg)) {
+			/* check if the client configured volatile writes */
+			if (regmap_volatile_writes(map, reg))
+				return 0;
+		}
+
 		return map->cache_ops->write(map, reg, value);
+	}
 
 	return 0;
 }

index 87b562e..89fb771 100644
@@ -415,20 +415,33 @@ static const struct file_operations regmap_reg_ranges_fops = {
 static int regmap_access_show(struct seq_file *s, void *ignored)
 {
 	struct regmap *map = s->private;
+	char reg_volatile;
 	int i, reg_len;
 
 	reg_len = regmap_calc_reg_len(map->max_register);
 
 	for (i = 0; i <= map->max_register; i += map->reg_stride) {
-		/* Ignore registers which are neither readable nor writable */
+		 reg_volatile = 'n';
+
+		 /* Ignore registers which are neither readable nor writable */
 		if (!regmap_readable(map, i) && !regmap_writeable(map, i))
 			continue;
 
+		if (regmap_volatile(map, i)) {
+			reg_volatile = 'y';
+			goto done;
+		}
+
+		if (!regmap_readable(map, i)) {
+			if (regmap_volatile_writes(map, i))
+				reg_volatile = 'y';
+		}
+done:
 		/* Format the register */
 		seq_printf(s, "%.*x: %c %c %c %c\n", reg_len, i,
 			   regmap_readable(map, i) ? 'y' : 'n',
 			   regmap_writeable(map, i) ? 'y' : 'n',
-			   regmap_volatile(map, i) ? 'y' : 'n',
+			   reg_volatile,
 			   regmap_precious(map, i) ? 'y' : 'n');
 	}
 

index 3bc8488..7d4b671 100644
@@ -154,6 +154,17 @@ bool regmap_volatile(struct regmap *map, unsigned int reg)
 		return true;
 }
 
+bool regmap_volatile_writes(struct regmap *map, unsigned int reg)
+{
+	if (map->volatile_reg)
+		return map->volatile_reg(map->dev, reg);
+
+	if (map->volatile_table)
+		return regmap_check_range_table(map, reg, map->volatile_table);
+
+	return false;
+}
+
 bool regmap_precious(struct regmap *map, unsigned int reg)
 {
 	if (!regmap_readable(map, reg))

2.7.4

I don't understand what voltile access means for write only devices. Volatile means that we don't read the cache but go direct to the hardware so if we can't read the hardware that's pretty redundant, a volatile read that goes to the cache is just a default read.

oops, sorry will try to be a bit more clear with an example.

This patch tries to support a map that provides:

1. only cached reads: (as a consequence every regmap write must succeed).
2. cached writes: do not access the hardware unless the value differs from what is in the cache already or (3) applies.
3. support for selectable volatile writes: those that will always access the device no matter what the cache holds.

Something like this:

static const struct regmap_config foo_regmap = {
    .reg_write        = foo_write_reg,

    .reg_bits        = 32,
    .val_bits        = 32,
    .reg_stride        = 1,

    .volatile_reg        = foo_volatile_reg,

    .max_register        = CODEC_ENABLE_DEBUG_CTRL_REG, .reg_defaults        = foo_reg_defaults, .num_reg_defaults    = ARRAY_SIZE(foo_reg_defaults), .cache_type        = REGCACHE_RBTREE,
};

I dont think - I could be wrong- that this is something that we can support today since the current code seems to require that the regmap is readable (ie, that it implements reg_read). But it could also be that I am missing something in my config? This is why I sent an RFC instead of a PATCH, because I am not 100% sure that I am not missing something.

We don't currently suppress writes except when regmap_update_bits() notices that the modification was a noop. You probably want to be using regmap_write_bits() here instead of regmap_update_bits(), that will always do the write.

but isnt that interface at a different level?
I am not sure if you are asking me to review my patch or just discarding the RFC and highlighting that I have a configuration problem.

In my use case and what triggered this RFC (config below), an 'amixer set' might never reach the driver's .reg_write interface even though the register is configured as volatile (to me this is not consistent since volatile_reg is being silently ignored).

So I dont see where/how your recommendation fits; maybe you could clarify a bit more please?

static const struct regmap_config foo_regmap = {
    .reg_write           = foo_write_reg,

    .reg_bits            = 32,
    .val_bits            = 32,
    .reg_stride          = 1,

    .volatile_reg        = foo_volatile_reg,

    .max_register        = CODEC_ENABLE_DEBUG_CTRL_REG, .reg_defaults        = foo_reg_defaults, .num_reg_defaults    = ARRAY_SIZE(foo_reg_defaults), .cache_type          = REGCACHE_RBTREE,
};

It's at the level where we suppress writes - the write suppression isn't a feature of the caching, it's something that regmap_update_bits() does if it notices that it won't change anything. It'll happen even if there's no cache at all.

I don't understand your patch as-is.

I'm not seeing any inconsistency there. Volatility means the register can't be cached as it might change underneath us, it doesn't have any impact on writes and it's happening at a lower level. Like I say if you absolutely need a write to happen you should be explicitly doing a write, though if you need a write to happen for a noop control change it sounds like there's something weird with that control that's possibly a problem anyway.

As I've been saying if you explicitly need a write to happen don't use regmap_update_bits(), do something that guarantees you'll get a write like regmap_write() or regmap_write_bits().

I do understand your point but Mark, that interface you mention sits above the user request (as a client the user does not call regmap_update_bits or regmap_write_bits or regmap_write(): none of those functions mean anything to the foo_regmap definition below - that is why we have an interface).

The client just uses this request:

static const struct regmap_config foo_regmap = {
    .reg_write           = foo_write_reg,

    .reg_bits            = 32,
    .val_bits            = 32,
    .reg_stride          = 1,

    .volatile_reg        = foo_volatile_reg,

    .max_register        = CODEC_ENABLE_DEBUG_CTRL_REG, .reg_defaults        = foo_reg_defaults, .num_reg_defaults    = ARRAY_SIZE(foo_reg_defaults), .cache_type          = REGCACHE_RBTREE,
};

and all this request means is that it expects foo_volatile_regs to be taken into consideration when accessing the reg_write callback: so whoever is calling the interface reg_write (be it regmap_update_bits or regmap_write_bits or whoever it is, I dont know) must make sure the volatile request applies.

the RFC patch that I submitted achieves exactly that.

does this make more sense now?

...

Right, but your code can expect a lot of things and not have them happen if they're not good expectations - that's explicitly not what we've been meaning by volatile registers (they're just registers that can't be cached). I mean, you mentioned that you were doing this for an ALSA control and since ALSA reports noop updates to userspace it'd be entirely reasonable for an implementation change there were to suppress the write before it gets to regmap (this is the sort of thing I was thinking of when I said I was suspicious of what you're doing there, this doesn't sound like a normal ALSA control).

I get what you're saying, it just doesn't feel like this is the right place to solve whatever your end use case is, it feels like it's going to be fragile one way or another and end up adding more complexity - having a hand coded ALSA control for this feels more secure at that level never mind anything that's going on in regmap.