regmap: Add debugfs file for forcing field writes
Commit Message
`_regmap_update_bits` checks if the current register value differs from
the new value, and only writes to the register if they differ. When
testing hardware drivers, it might be desirable to always force a
register write, for example when writing to a `regmap_field`.
This enables and simplifies testing and verification of the hardware
interaction. For example, when using a hardware mock/simulation model,
one can then more easily verify that the driver makes the correct
expected register writes during certain events. Add a bool variable
`force_write_field` and a corresponding debugfs entry.
Signed-off-by: Waqar Hameed <waqarh@axis.com>
---
drivers/base/regmap/internal.h | 3 +++
drivers/base/regmap/regmap-debugfs.c | 3 +++
drivers/base/regmap/regmap.c | 2 +-
3 files changed, 7 insertions(+), 1 deletion(-)
base-commit: 858fd168a95c5b9669aac8db6c14a9aeab446375
Comments
On Mon, Jun 12, 2023 at 04:53:35PM +0200, Waqar Hameed wrote:
> `_regmap_update_bits` checks if the current register value differs from
> the new value, and only writes to the register if they differ. When
> testing hardware drivers, it might be desirable to always force a
> register write, for example when writing to a `regmap_field`.
>
> This enables and simplifies testing and verification of the hardware
> interaction. For example, when using a hardware mock/simulation model,
> one can then more easily verify that the driver makes the correct
> expected register writes during certain events. Add a bool variable
> `force_write_field` and a corresponding debugfs entry.
If we're going to do something like this which could interfere with
driver operation then it should be guarded like the write support is so
that people using it have to modify the kernel to get the feature, or at
the very least taint the kernel. This is less invasive but still might
cause issues if someone is relying on read/modify/write behaviour.
On Mon, Jun 12, 2023 at 16:00 +0100 Mark Brown <broonie@kernel.org> wrote:
> If we're going to do something like this which could interfere with
> driver operation then it should be guarded like the write support is so
> that people using it have to modify the kernel to get the feature, or at
> the very least taint the kernel. This is less invasive but still might
> cause issues if someone is relying on read/modify/write behaviour.
I understand your point. Should we introduce a new macro like
`REGMAP_ALLOW_WRITE_DEBUGFS` (which requires direct code modification to
enable it) to guard this or introduce a new kernel configuration?
On Tue, Jun 13, 2023 at 12:24:28PM +0200, Waqar Hameed wrote:
> On Mon, Jun 12, 2023 at 16:00 +0100 Mark Brown <broonie@kernel.org> wrote:
>
> > If we're going to do something like this which could interfere with
> > driver operation then it should be guarded like the write support is so
> > that people using it have to modify the kernel to get the feature, or at
> > the very least taint the kernel. This is less invasive but still might
> > cause issues if someone is relying on read/modify/write behaviour.
> I understand your point. Should we introduce a new macro like
> `REGMAP_ALLOW_WRITE_DEBUGFS` (which requires direct code modification to
> enable it) to guard this or introduce a new kernel configuration?
I'd add a macro.
@@ -125,6 +125,9 @@ struct regmap {
int reg_stride;
int reg_stride_order;
+ /* If set, will always write field to HW. */
+ bool force_write_field;
+
/* regcache specific members */
const struct regcache_ops *cache_ops;
enum regcache_type cache_type;
@@ -636,6 +636,9 @@ void regmap_debugfs_init(struct regmap *map)
®map_cache_bypass_fops);
}
+ debugfs_create_bool("force_write_field", 0600, map->debugfs,
+ &map->force_write_field);
+
next = rb_first(&map->range_tree);
while (next) {
range_node = rb_entry(next, struct regmap_range_node, node);
@@ -3273,7 +3273,7 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
tmp = orig & ~mask;
tmp |= val & mask;
- if (force_write || (tmp != orig)) {
+ if (force_write || (tmp != orig) || map->force_write_field) {
ret = _regmap_write(map, reg, tmp);
if (ret == 0 && change)
*change = true;