diff mbox

[4/7] ARM: mx25: Retrieve IIM base from dt

Message ID 1426077835-31081-4-git-send-email-festevam@gmail.com
State New
Headers show

Commit Message

Fabio Estevam March 11, 2015, 12:43 p.m. UTC 
From: Fabio Estevam <fabio.estevam@freescale.com>

We should use dt to retrieve the IIM base address.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/mach-imx/cpu-imx25.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Russell King - ARM Linux March 11, 2015, 1:24 p.m. UTC | #1 
On Wed, Mar 11, 2015 at 09:43:52AM -0300, Fabio Estevam wrote:
> +	void __iomem *iim_base;
> +	struct device_node *np;
> +
> +	np = of_find_compatible_node(NULL, NULL, "fsl,imx25-iim");
> +	iim_base = of_iomap(np, 0);
> +	WARN_ON(!iim_base);
> +	rev = readl(iim_base + MXC_IIMSREV);
> +	iounmap(iim_base);

Is there much point to that WARN_ON()?  Are you avoiding the political
BUG_ON(!iim_base) ? :)

Since a NULL pointer there will lead to a NULL pointer dereference, it's
silly to use WARN_ON() there - we'll get a backtrace etc from the WARN_ON
followed immediately by an oops dump due to the readl(), crashing the
kernel.

Better to just use BUG_ON() and only get one set of diagnostics.
Fabio Estevam March 11, 2015, 2:20 p.m. UTC | #2 
On Wed, Mar 11, 2015 at 10:24 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Mar 11, 2015 at 09:43:52AM -0300, Fabio Estevam wrote:
>> +     void __iomem *iim_base;
>> +     struct device_node *np;
>> +
>> +     np = of_find_compatible_node(NULL, NULL, "fsl,imx25-iim");
>> +     iim_base = of_iomap(np, 0);
>> +     WARN_ON(!iim_base);
>> +     rev = readl(iim_base + MXC_IIMSREV);
>> +     iounmap(iim_base);
>
> Is there much point to that WARN_ON()?  Are you avoiding the political
> BUG_ON(!iim_base) ? :)
>
> Since a NULL pointer there will lead to a NULL pointer dereference, it's
> silly to use WARN_ON() there - we'll get a backtrace etc from the WARN_ON
> followed immediately by an oops dump due to the readl(), crashing the
> kernel.
>
> Better to just use BUG_ON() and only get one set of diagnostics.

Thanks, will do it in v2.
diff mbox

Patch

diff --git a/arch/arm/mach-imx/cpu-imx25.c b/arch/arm/mach-imx/cpu-imx25.c
index 96ec64b..0d2d860 100644
--- a/arch/arm/mach-imx/cpu-imx25.c
+++ b/arch/arm/mach-imx/cpu-imx25.c
@@ -11,6 +11,8 @@ 
  */
 #include <linux/module.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
 
 #include "iim.h"
 #include "hardware.h"
@@ -20,8 +22,15 @@  static int mx25_cpu_rev = -1;
 static int mx25_read_cpu_rev(void)
 {
 	u32 rev;
+	void __iomem *iim_base;
+	struct device_node *np;
+
+	np = of_find_compatible_node(NULL, NULL, "fsl,imx25-iim");
+	iim_base = of_iomap(np, 0);
+	WARN_ON(!iim_base);
+	rev = readl(iim_base + MXC_IIMSREV);
+	iounmap(iim_base);
 
-	rev = __raw_readl(MX25_IO_ADDRESS(MX25_IIM_BASE_ADDR + MXC_IIMSREV));
 	switch (rev) {
 	case 0x00:
 		return IMX_CHIP_REVISION_1_0;