Re: [PATCH v2] regmap: Add debugfs file for forcing field writes

[Greg Kroah-Hartman]

On Tue, Jun 13, 2023 at 01:42:27PM +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 to enable this. Since this feature could interfere with driver
> operation, guard it with a macro.
> 
> Signed-off-by: Waqar Hameed <waqar.hameed@xxxxxxxx>
> ---
> Changes in v2:
> * Add macro to guard the debugfs entry.
> * Fix `Signed-off-by` in commit message to match actual email address.
> * Link to v1: https://lore.kernel.org/all/pndttvcu3ut.fsf@xxxxxxxx/
> 
>  drivers/base/regmap/internal.h       |  3 +++
>  drivers/base/regmap/regmap-debugfs.c | 11 +++++++++++
>  drivers/base/regmap/regmap.c         |  2 +-
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
> index 9bd0dfd1e259..6472b3222b82 100644
> --- a/drivers/base/regmap/internal.h
> +++ b/drivers/base/regmap/internal.h
> @@ -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;
> diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
> index c491fabe3617..f36027591e1a 100644
> --- a/drivers/base/regmap/regmap-debugfs.c
> +++ b/drivers/base/regmap/regmap-debugfs.c
> @@ -636,6 +636,17 @@ void regmap_debugfs_init(struct regmap *map)
>  				    &regmap_cache_bypass_fops);
>  	}
>  
> +	/*
> +	 * This could interfere with driver operation. Therefore, don't provide
> +	 * any real compile time configuration option for this feature. One will
> +	 * have to modify the source code directly in order to use it.
> +	 */
> +#undef REGMAP_ALLOW_FORCE_WRITE_FIELD_DEBUGFS
> +#ifdef REGMAP_ALLOW_FORCE_WRITE_FIELD_DEBUGFS
> +	debugfs_create_bool("force_write_field", 0600, map->debugfs,
> +			    &map->force_write_field);
> +#endif

Please no, that means this will never ever ever get used, and if it
happens to break the build or runtime, no one will ever notice it.

If you need this for your device/tree/distro, great, just keep it as an
out-of-tree patch with the huge header "NEVER ENABLE THIS IN A REAL
SYSTEM" or something like that.

But as-is, we can't take this, sorry.

greg k-h