Messages in this thread | | | Subject | Re: [RFC] regmap: allow volatile register writes with cached only read maps | From | Jorge Ramirez-Ortiz <> | Date | Thu, 17 May 2018 09:12:49 +0200 |
| |
On 05/13/2018 04:22 AM, Mark Brown wrote: > On Fri, May 11, 2018 at 12:29:42PM +0200, Jorge Ramirez-Ortiz wrote: >> On 05/11/2018 04:00 AM, Mark Brown wrote: >>> 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? > 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 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. > I don't understand your patch as-is. > >> 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). > 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. > >> So I dont see where/how your recommendation fits; maybe you could clarify a >> bit more please? > 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?
| |