Messages in this thread | | | Date | Sat, 4 Aug 2012 11:05:23 +0100 | From | Mark Brown <> | Subject | Re: [PATCH v2] regmap: Add regmap dummy driver |
| |
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.
| |