Hi, I found this patch has some confliction with the Mark's newest Remgap-Tree's origin/topic/dt-endian branch. Javier Martinez has already fix some of these. Thanks, BRs Xiubo > -----Original Message----- > From: Thierry Reding [mailto:thierry.reding@xxxxxxxxx] > Sent: Tuesday, August 19, 2014 1:22 PM > To: Stephen Warren > Cc: Mark Brown; Thierry Reding; linux-kernel@xxxxxxxxxxxxxxx; linux- > next@xxxxxxxxxxxxxxx; Stephen Warren; Xiubo Li-B47053 > Subject: Re: [PATCH] regmap: fix of_regmap_get_endian() > > On Mon, Aug 18, 2014 at 04:14:04PM -0600, Stephen Warren wrote: > > From: Stephen Warren <swarren@xxxxxxxxxx> > > > > Commit d647c199510c ("regmap: add DT endianness binding support") has > > some issues. Specifically, if config->reg_format_endian is not explicitly > > set, it will be zero, i.e. REGMAP_ENDIAN_DEFAULT. The switch statement > > that looks up the *endian from DT for the type==REGMAP_ENDIAN_VAL case > > doesn't change *endian in the type==REGMAP_ENDIAN_REG case. However, the > > test immediately following, compares *endian against REGMAP_ENDIAN_NATIVE, > > and if not equal, returns *endian as is. This ends up returning > > REGMAP_ENDIAN_DEFAULT, which the calling code does not expect, eventually > > leading to e.g.: > > > > Unable to handle kernel NULL pointer dereference at virtual address 00000024 > > ... > > [<c02efd64>] (regcache_cache_only) from [<c0465fa8>] > (tegra30_ahub_probe+0x1b8/0x430) > > [<c0465fa8>] (tegra30_ahub_probe) from [<c02e196c>] > (platform_drv_probe+0x2c/0x5c) > > [<c02e196c>] (platform_drv_probe) from [<c02e0580>] > (driver_probe_device+0x10c/0x22c) > > [<c02e0580>] (driver_probe_device) from [<c02e072c>] > (__driver_attach+0x8c/0x90) > > > > This patch solves this by: > > * When looking up the endianness from DT, don't change *endian at all if > > there is no DT property; leave it set to REGMAP_ENDIAN_DEFAULT so the > > code falls through to other data sources in the same way as before. > > Now, the "unspecified" case acts the same for both REGMAP_ENDIAN_REG and > > REGMAP_ENDIAN_VAL. > > * After potentially looking up the endianness from DT, check *endian > > against REGMAP_ENDIAN_DEFAULT instead of REGMAP_ENDIAN_NATIVE to avoid > > returning unexpected values. > > > > Also, clean up the code a bit: > > > > * 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. > > * Restore the overall default of REGMAP_ENDIAN_BIG if none of the config, > > DT, or the bus specify any endianness. Since all busses specify an > > endianness now, this makes no difference, but I saw no justification in > > the patch description for changing the default default. > > * 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. > > > > Reported-by: Thierry Reding <treding@xxxxxxxxxx> > > Fixes: d647c199510c ("regmap: add DT endianness binding support") > > Cc: Xiubo Li <Li.Xiubo@xxxxxxxxxxxxx> > > Signed-off-by: Stephen Warren <swarren@xxxxxxxxxx> > > --- > > drivers/base/regmap/regmap.c | 58 ++++++++++++++--------------------------- > --- > > 1 file changed, 18 insertions(+), 40 deletions(-) > > Thanks for fixing this, Stephen. > > Tested-by: Thierry Reding <treding@xxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-next" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
