From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752098AbcDNGmr (ORCPT ); Thu, 14 Apr 2016 02:42:47 -0400 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:40714 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751453AbcDNGmp (ORCPT ); Thu, 14 Apr 2016 02:42:45 -0400 Date: Thu, 14 Apr 2016 07:42:29 +0100 From: Mark Brown To: Srinivas Kandagatla Cc: Greg Kroah-Hartman , Maxime Ripard , linux-kernel@vger.kernel.org, rjendra@qti.qualcomm.com Message-ID: <20160414064229.GC18024@sirena.org.uk> References: <1460569154-25030-1-git-send-email-srinivas.kandagatla@linaro.org> <1460569154-25030-4-git-send-email-srinivas.kandagatla@linaro.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="f+W+jCU1fRNres8c" Content-Disposition: inline In-Reply-To: <1460569154-25030-4-git-send-email-srinivas.kandagatla@linaro.org> X-Cookie: McDonald's -- Because you're worth it. User-Agent: Mutt/1.5.24 (2015-08-30) X-SA-Exim-Connect-IP: 2a01:348:6:8808:fab::3 X-SA-Exim-Mail-From: broonie@sirena.org.uk Subject: Re: [PATCH v1 3/3] nvmem: core: fix regmap accessor usage X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000) X-SA-Exim-Scanned: Yes (on mezzanine.sirena.org.uk) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --f+W+jCU1fRNres8c Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > 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 =3D 0; i < word_count; i++) { > + ret =3D regmap_read(map, reg + i * nvmem->stride, &ival); > + if (ret !=3D 0) > + return ret; > + > + switch (nvmem->word_size) { > + case 4: > + u32_buf[i] =3D 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. --f+W+jCU1fRNres8c Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXDzvUAAoJECTWi3JdVIfQEjsH/1DHDak3144G8yIKM4jKlES5 CAd/VN4cOeAPa9xTQWmGilVWHe0TeHiiGFqSffjMhxpgC2pOR97mMquyrBx/Q1zp PW7P4mRI6H1hKJjWVS4GjfXzZhF1qswyXSGNzOKTxd1Z4HeDuTnzR4l/kzzsp2Ai 0HwsYI1M7ZQwT/gdFps5ZIs7ogDm6xerfIwOj3GtDzIZSXSuuAd6RA+dv7nVdznm X4WVm2nFP39hUZXXGgqXICpHELHez+HDYyN1h39tHSSZ+4mxUEYz/VgMjQDAIma5 dxLIetivmupCdkBXusXUtbcFurU0WEKlcDUImJYmoIrYRcElj3+FhSUJ2H+2ySw= =OmYa -----END PGP SIGNATURE----- --f+W+jCU1fRNres8c--