Re: [PATCH v2] regmap: Add regmap dummy driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Fri, Jul 27, 2012 at 02:58:20PM +0100, Dimitris Papastamos wrote:
> Add a pseudo-driver for debugging and stress-testing the
> regmap/regcache APIs.  A standard set of tools for working

Overall this looks good, most of the stuff below is fairly small.  As a
very high level comment it'd be really helpful to split this into a
series of commits, for example adding just the dummy device then
building out the functionality.  It'd make review much easier.

> with this driver (mainly sh scripts) will be put in a repo
> at https://github.com/quantumdream/regmap-tools

Any reason not to put this in the tools directory?

> Some of these tests will require one to build with
> REGMAP_ALLOW_WRITE_DEBUGFS defined.

Can we add a write mechanism specifically for this dummy driver?

> +	/* Set when regdummy defaults have been modified.
> +	 * This is useful to know so we don't reinit the
> +	 * cache if there is no reason to do so. */
> +	unsigned int dirty:1;

Should we perhaps just reinit anyway?  It's not like this is performance
critical...

> +/* Default volatile register callback, this should
> + * normally be configured by the user via a debugfs
> + * entry */
> +static bool regdummy_volatile_reg(struct device *dev,
> +				  unsigned int reg)
> +{
> +	return false;
> +}

All these functions just seem to be implementing the default behaviour,
why are they needed?

> +		/* If we're in the region the user is trying to read */
> +		if (p >= *ppos) {
> +			/* ...but not beyond it */
> +			if (buf_pos >= count - 1 - tot_len)
> +				break;

Any potential for code reuse?  This stuff does look awfully familiar!

> +	/* Allocate the new register defaults */
> +	regdef_num_new = rdevp->regs_size_new / config->reg_stride;
> +	regdef_num_raw_new = regdef_num_new * sizeof(*regdef_new);
> +	regdef_new = kzalloc(regdef_num_raw_new, GFP_KERNEL);

Can we factor this stuff out - there's a lot of overlap with the vanilla
init?

> +static struct platform_device regdummy_device = {
> +	.name = "regdummy",
> +	.id = 0,
> +};

Set id to -1 if there's only one of them.