Re: [PATCH v1 3/3] nvmem: core: fix regmap accessor usage

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]

On Wed, Apr 13, 2016 at 06:39:14PM +0100, Srinivas Kandagatla wrote:
> With the recent patch of removing the raw accessors form regmap-mmio,
> broke the qfprom support. This patch attempts to fix that regression
> by adding check before calling regmap raw accessors functions.
> 
> Without this patch nvmem providers based on regmap mmio would not work.

Ugh, this seems incredibly confused.  Why is this even using regmap?  It
doesn't seem to use anything from the framework - it only ever accesses
the device with raw accessors which bypass pretty much everything the
framework has to offer.  The raw accessors are there for things like
copying preformatted images to devices where we have a specific need to
do a subset of operations where the framework doesn't add anything, they
shouldn't be the only thing a driver ever does.  If they are then all
regmap is doing is adding a bunch of overhead and making the code more
complex.

Looking at the regmap_bus implementations in the subsystem is very
worrying too, there's lots of dummy functions in there which are never a
good sign and pretty much every bus looks like it really should be using
reg_read() and reg_write() operations but is contorting itself to do
bytestream access instead - things like parsing data out of the buffers
are not good signs as they indicate that the buses are hooking in at the
wrong abstraction layer.

> +	if (regmap_can_raw_read(map))
> +		return regmap_raw_read(map, reg, val, bytes);
> +
> +	for (i = 0; i < word_count; i++) {
> +		ret = regmap_read(map, reg + i * nvmem->stride, &ival);
> +		if (ret != 0)
> +			return ret;
> +
> +		switch (nvmem->word_size) {
> +		case 4:
> +			u32_buf[i] = ival;
> +			break;

This is clearly an abstraction failure and probably broken for systems
where the device endianness does not match the CPU endianness (like most
big endian systems, the device hardware normally stays in little endian
mode).

We need to figure out what this stuff is trying to do before we go any
further, I'm honestly not entirely clear.  I *think* that if regmap is a
good fit then it probably wants to use the bulk operations rather than
the raw operations (the bulk operations are AFAICT what is being open
coded above) but bulk I/O still does endianness handling and I'm not
sure if that's desired or not.  If the nvmem code really is just trying
to get bytestreams then regmap really isn't what it should be using,
it's all about dealing with registers and trying to force bytestreams
through it seems like it's just going to lead to fragility.  Either
whatever is happening should be abstracted within regmap or we shouldn't
be using regmap.

I'll try to have another look at this later.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]