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

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Mark Brown <broonie@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Maxime Ripard <maxime.ripard@free-electrons.com>,
	linux-kernel@vger.kernel.org, rjendra@qti.qualcomm.com
Subject: Re: [PATCH v1 3/3] nvmem: core: fix regmap accessor usage
Date: Thu, 14 Apr 2016 13:35:03 +0100	[thread overview]
Message-ID: <570F8E77.5090202@linaro.org> (raw)
In-Reply-To: <20160414064229.GC18024@sirena.org.uk>

On 14/04/16 07:42, Mark Brown wrote:
> 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.
>
...
>> +	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.
Thanks for your comments,

I totally agree that there is an abstraction failure here in both sides. 
It should be fixed. moving to using bulk apis would solve the nvmem 
problem for now. But for long term, using regmap should be totally 
removed from nvmem and directly use the reg read/write callbacks from 
nvmem providers, This would be much robust solution. This was indeed 
Maxime's first proposal. I will try to fix it up and see how it looks 
without regmap.

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

thanks,
srini