[PATCH V2] regmap: of_regmap_get_endian() cleanup

[PATCH V2] regmap: of_regmap_get_endian() cleanup

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Commit d647c199510c ("regmap: add DT endianness binding support") had
some issues. Commit ba1b53feb8ca ("regmap: Fix DT endianess parsing
logic") fixed the main problem. This patch fixes the other.

Specifically, restore the overall default of REGMAP_ENDIAN_BIG if none of
the config, DT, or the bus specify any endianness. Without this,
of_regmap_get_endian() could return REGMAP_ENDIAN_DEFAULT, which the
calling code can't handle. Since all busses do specify an endianness in
the current code, this makes no difference right now, but I saw no
justification in the patch description for removing this final default.

Also, clean up the code a bit:

* s/of_regmap_get_endian/regmap_get_endian/ since the function isn't DT-
  specific, even if the reason it was originally added was to add some
  DT-specific features.
* After potentially reading an endianess specification from DT, the code
  checks whether DT did specify an endianness, and if so, returns it. Move
  this test outside the whole switch statement so that if the
  REGMAP_ENDIAN_REG case ever modifies *endian, this check will pick that
  up. This partially reverts part of commit ba1b53feb8ca ("regmap: Fix DT
  endianess parsing logic"), while maintaining the bug-fix that commit
  made to this code.
* Make the comments briefer, and only refer to the specific action taken
  at their location. This makes most of the comments independent of DT,
  and easier to follow.

Cc: Xiubo Li <Li.Xiubo@freescale.com>
Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Cc: Thierry Reding <treding@nvidia.com>
Fixes: d647c199510c ("regmap: add DT endianness binding support")
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
V2: Rebase on Javier's earlier fix, amend commit description due to this.

 drivers/base/regmap/regmap.c | 54 ++++++++++++++++----------------------------
 1 file changed, 20 insertions(+), 34 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 055a9c3a3b12..bb4502a48be5 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -454,7 +454,7 @@ enum regmap_endian_type {
 	REGMAP_ENDIAN_VAL,
 };
 
-static int of_regmap_get_endian(struct device *dev,
+static int regmap_get_endian(struct device *dev,
 				const struct regmap_bus *bus,
 				const struct regmap_config *config,
 				enum regmap_endian_type type,
@@ -465,15 +465,7 @@ static int of_regmap_get_endian(struct device *dev,
 	if (!endian || !config)
 		return -EINVAL;
 
-	/*
-	 * Firstly, try to parse the endianness from driver's config,
-	 * this is to be compatible with the none DT or the old drivers.
-	 * From the driver's config the endianness value maybe:
-	 *   REGMAP_ENDIAN_BIG,
-	 *   REGMAP_ENDIAN_LITTLE,
-	 *   REGMAP_ENDIAN_NATIVE,
-	 *   REGMAP_ENDIAN_DEFAULT.
-	 */
+	/* Retrieve the endianness specification from the regmap config */
 	switch (type) {
 	case REGMAP_ENDIAN_REG:
 		*endian = config->reg_format_endian;
@@ -485,31 +477,17 @@ static int of_regmap_get_endian(struct device *dev,
 		return -EINVAL;
 	}
 
-	/*
-	 * If the endianness parsed from driver config is
-	 * REGMAP_ENDIAN_DEFAULT, that means maybe we are using the DT
-	 * node to specify the endianness information.
-	 */
+	/* If the regmap config specified a non-default value, use that */
 	if (*endian != REGMAP_ENDIAN_DEFAULT)
 		return 0;
 
-	/*
-	 * Secondly, try to parse the endianness from DT node if the
-	 * driver config does not specify it.
-	 * From the DT node the endianness value maybe:
-	 *   REGMAP_ENDIAN_BIG,
-	 *   REGMAP_ENDIAN_LITTLE,
-	 */
+	/* Parse the device's DT node for an endianness specification */
 	switch (type) {
 	case REGMAP_ENDIAN_VAL:
 		if (of_property_read_bool(np, "big-endian"))
 			*endian = REGMAP_ENDIAN_BIG;
 		else if (of_property_read_bool(np, "little-endian"))
 			*endian = REGMAP_ENDIAN_LITTLE;
-
-		if (*endian != REGMAP_ENDIAN_DEFAULT)
-			return 0;
-
 		break;
 	case REGMAP_ENDIAN_REG:
 		break;
@@ -517,10 +495,11 @@ static int of_regmap_get_endian(struct device *dev,
 		return -EINVAL;
 	}
 
-	/*
-	 * Finally, try to parse the endianness from regmap bus config
-	 * if in device's DT node the endianness property is absent.
-	 */
+	/* If the endianness was specified in DT, use that */
+	if (*endian != REGMAP_ENDIAN_DEFAULT)
+		return 0;
+
+	/* Retrieve the endianness specification from the bus config */
 	switch (type) {
 	case REGMAP_ENDIAN_REG:
 		if (bus && bus->reg_format_endian_default)
@@ -534,6 +513,13 @@ static int of_regmap_get_endian(struct device *dev,
 		return -EINVAL;
 	}
 
+	/* If the bus specified a non-default value, use that */
+	if (*endian != REGMAP_ENDIAN_DEFAULT)
+		return 0;
+
+	/* Use this if no other value was found */
+	*endian = REGMAP_ENDIAN_BIG;
+
 	return 0;
 }
 
@@ -640,13 +626,13 @@ struct regmap *regmap_init(struct device *dev,
 		map->reg_read  = _regmap_bus_read;
 	}
 
-	ret = of_regmap_get_endian(dev, bus, config, REGMAP_ENDIAN_REG,
-				   &reg_endian);
+	ret = regmap_get_endian(dev, bus, config, REGMAP_ENDIAN_REG,
+				&reg_endian);
 	if (ret)
 		return ERR_PTR(ret);
 
-	ret = of_regmap_get_endian(dev, bus, config, REGMAP_ENDIAN_VAL,
-				   &val_endian);
+	ret = regmap_get_endian(dev, bus, config, REGMAP_ENDIAN_VAL,
+				&val_endian);
 	if (ret)
 		return ERR_PTR(ret);
 
-- 
1.9.1

Re: [PATCH V2] regmap: of_regmap_get_endian() cleanup

[Mark Brown]

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

On Tue, Aug 19, 2014 at 10:49:07AM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Commit d647c199510c ("regmap: add DT endianness binding support") had
> some issues. Commit ba1b53feb8ca ("regmap: Fix DT endianess parsing
> logic") fixed the main problem. This patch fixes the other.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V2] regmap: of_regmap_get_endian() cleanup

[Javier Martinez Canillas]

Hello Stephen,

On 08/19/2014 06:49 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Commit d647c199510c ("regmap: add DT endianness binding support") had
> some issues. Commit ba1b53feb8ca ("regmap: Fix DT endianess parsing
> logic") fixed the main problem. This patch fixes the other.
> 
> Specifically, restore the overall default of REGMAP_ENDIAN_BIG if none of
> the config, DT, or the bus specify any endianness. Without this,
> of_regmap_get_endian() could return REGMAP_ENDIAN_DEFAULT, which the
> calling code can't handle. Since all busses do specify an endianness in
> the current code, this makes no difference right now, but I saw no
> justification in the patch description for removing this final default.
> 

Yes, I also wondered about the second issue you are mentioning when I was
fixing the main problem. But since Xiubo's patch also set
REGMAP_ENDIAN_BIG as default for both the I2C and SPI buses (making no
difference as you said) I assumed that the intention was to make buses to
explicitly define their default instead of rely on a global one so I just
kept that way.

But I agree with you that is better to go back to the default
REGMAP_ENDIAN_BIG as it used to be before since buses can change the
default if needed anyways.

Best regards,
Javier

Re: [PATCH V2] regmap: of_regmap_get_endian() cleanup

[Geert Uytterhoeven]

Hi Mark,

On Tue, Aug 19, 2014 at 11:17 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Aug 19, 2014 at 10:49:07AM -0600, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Commit d647c199510c ("regmap: add DT endianness binding support") had
>> some issues. Commit ba1b53feb8ca ("regmap: Fix DT endianess parsing
>> logic") fixed the main problem. This patch fixes the other.
>
> Applied, thanks.

Doh, wish I'd seen this thread earlier...

This morning I was greeted by some crashes, as your spi/for-next branch
has just pulled in spi/topic/fsl-dspi, which contains regmap/dt-endian, but
not the fixes :-(

You may want to pull those in, too.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH V2] regmap: of_regmap_get_endian() cleanup

[Mark Brown]

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

On Wed, Aug 27, 2014 at 03:14:21PM +0200, Geert Uytterhoeven wrote:

> This morning I was greeted by some crashes, as your spi/for-next branch
> has just pulled in spi/topic/fsl-dspi, which contains regmap/dt-endian, but
> not the fixes :-(

Yes, probably.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]