diff mbox

Errors at boot time from OMAP4430SDP (and a whinge about serial stuff)

Message ID CAK=WgbbPmuOgssRRSinsV3rXg7TUa9fpOP-Zg+VatZgqgVYixw@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ohad Ben Cohen Oct. 21, 2012, 2:54 p.m. UTC 
On Fri, Oct 19, 2012 at 7:07 PM, Tony Lindgren <tony@atomide.com> wrote:
...
> We could optimize away a minimal amount of code for many configurations
> with the ifdef? :)

Sure, here goes (compile tested only):

From 6b82365da2c04986e647d06c285197efece7cb34 Mon Sep 17 00:00:00 2001
From: Ohad Ben-Cohen <ohad@wizery.com>
Date: Sun, 14 Oct 2012 20:16:01 +0200
Subject: [PATCH] ARM: OMAP: don't print any error when wl12xx isn't
 configured

Stop intimidating users with scary wlan error messages in case wl12xx
support wasn't even built.

In addition, when wl12xx_set_platform_data() fails, don't bother
registering wl12xx's fixed regulator device (on the relevant
boards).

While we're at it, extract the wlan init code to a dedicated function to
make (the relevant boards') init code look a bit nicer.

Compile tested only.

Reported-by: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 arch/arm/mach-davinci/board-da850-evm.c      |  8 ++-----
 arch/arm/mach-omap2/board-4430sdp.c          |  7 +++---
 arch/arm/mach-omap2/board-omap3evm.c         |  6 ++---
 arch/arm/mach-omap2/board-omap3pandora.c     |  9 +++-----
 arch/arm/mach-omap2/board-omap4panda.c       | 20 ++++++++++++-----
 arch/arm/mach-omap2/board-zoom-peripherals.c | 15 ++++++++-----
 arch/arm/mach-omap2/common-board-devices.c   | 33 ++++++++++++++++++++++++++++
 arch/arm/mach-omap2/common-board-devices.h   |  2 ++
 8 files changed, 71 insertions(+), 29 deletions(-)

Comments

Igor Grinberg Oct. 23, 2012, 7:37 a.m. UTC | #1 
On 10/21/12 16:54, Ohad Ben-Cohen wrote:
> On Fri, Oct 19, 2012 at 7:07 PM, Tony Lindgren <tony@atomide.com> wrote:
> ...
>> We could optimize away a minimal amount of code for many configurations
>> with the ifdef? :)
> 
> Sure, here goes (compile tested only):
> 
>>From 6b82365da2c04986e647d06c285197efece7cb34 Mon Sep 17 00:00:00 2001
> From: Ohad Ben-Cohen <ohad@wizery.com>
> Date: Sun, 14 Oct 2012 20:16:01 +0200
> Subject: [PATCH] ARM: OMAP: don't print any error when wl12xx isn't
>  configured
> 
> Stop intimidating users with scary wlan error messages in case wl12xx
> support wasn't even built.
> 
> In addition, when wl12xx_set_platform_data() fails, don't bother
> registering wl12xx's fixed regulator device (on the relevant
> boards).
> 
> While we're at it, extract the wlan init code to a dedicated function to
> make (the relevant boards') init code look a bit nicer.
> 
> Compile tested only.
> 
> Reported-by: Russell King <linux@arm.linux.org.uk>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>

Apart, from the minor comment below,
Acked-by: Igor Grinberg <grinberg@compulab.co.il>

> ---
>  arch/arm/mach-davinci/board-da850-evm.c      |  8 ++-----
>  arch/arm/mach-omap2/board-4430sdp.c          |  7 +++---
>  arch/arm/mach-omap2/board-omap3evm.c         |  6 ++---
>  arch/arm/mach-omap2/board-omap3pandora.c     |  9 +++-----
>  arch/arm/mach-omap2/board-omap4panda.c       | 20 ++++++++++++-----
>  arch/arm/mach-omap2/board-zoom-peripherals.c | 15 ++++++++-----
>  arch/arm/mach-omap2/common-board-devices.c   | 33 ++++++++++++++++++++++++++++
>  arch/arm/mach-omap2/common-board-devices.h   |  2 ++
>  8 files changed, 71 insertions(+), 29 deletions(-)

[...]

> diff --git a/arch/arm/mach-omap2/common-board-devices.c
> b/arch/arm/mach-omap2/common-board-devices.c
> index 48daac2..02351e9 100644
> --- a/arch/arm/mach-omap2/common-board-devices.c
> +++ b/arch/arm/mach-omap2/common-board-devices.c
> @@ -23,6 +23,7 @@
>  #include <linux/gpio.h>
>  #include <linux/spi/spi.h>
>  #include <linux/spi/ads7846.h>
> +#include <linux/wl12xx.h>
> 
>  #include <linux/platform_data/spi-omap2-mcspi.h>
>  #include <linux/platform_data/mtd-nand-omap2.h>
> @@ -141,3 +142,35 @@ void __init omap_nand_flash_init(int options,
> struct mtd_partition *parts,
>  {
>  }
>  #endif
> +
> +#ifdef CONFIG_WL12XX_PLATFORM_DATA
> +int __init wl12xx_board_init(struct wl12xx_platform_data *wlan_data, int gpio)
> +{
> +	int ret;
> +
> +	wlan_data->irq = gpio_to_irq(gpio);
> +	if (wlan_data->irq < 0) {
> +		ret = wlan_data->irq;
> +		pr_err("wl12xx: gpio_to_irq(%d) failed: %d\n", gpio, ret);
> +		return ret;
> +	}
> +
> +	ret = wl12xx_set_platform_data(wlan_data);
> +	/* bail out silently in case wl12xx isn't configured */
> +	if (ret == -ENOSYS)
> +		return ret;

Since we have the function ifdef'ed, I don't think we need
the ENOSYS check, do we?

> +
> +	/* bail out verbosely on any other error */
> +	if (ret) {
> +		pr_err("error setting wl12xx data: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +#else /* !CONFIG_WL12XX_PLATFORM_DATA */
> +int __init wl12xx_board_init(struct wl12xx_platform_data *wlan_data, int gpio)
> +{
> +	return 0;
> +}
> +#endif

[...]
Ohad Ben Cohen Oct. 23, 2012, 7:51 a.m. UTC | #2 
On Tue, Oct 23, 2012 at 9:37 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>> +     ret = wl12xx_set_platform_data(wlan_data);
>> +     /* bail out silently in case wl12xx isn't configured */
>> +     if (ret == -ENOSYS)
>> +             return ret;
>
> Since we have the function ifdef'ed, I don't think we need
> the ENOSYS check, do we?

If we want to be strict, we better not remove it.

It's an interface that hides the internal implementation, and it's
just better not to assume anything beyond the return values and their
meanings. This way if WLAN folks change something in the future, we
don't need to update all the boards code again.

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Igor Grinberg Oct. 23, 2012, 9:46 a.m. UTC | #3 
On 10/23/12 09:51, Ohad Ben-Cohen wrote:
> On Tue, Oct 23, 2012 at 9:37 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>> +     ret = wl12xx_set_platform_data(wlan_data);
>>> +     /* bail out silently in case wl12xx isn't configured */
>>> +     if (ret == -ENOSYS)
>>> +             return ret;
>>
>> Since we have the function ifdef'ed, I don't think we need
>> the ENOSYS check, do we?
> 
> If we want to be strict, we better not remove it.
> 
> It's an interface that hides the internal implementation, and it's
> just better not to assume anything beyond the return values and their
> meanings. This way if WLAN folks change something in the future, we
> don't need to update all the boards code again.

Well, with this argument, we can add this (and many other checks) to
many more places in the code...
I just wanted to point out that most probably ret == -ENOSYS will
never happen since the #ifdef is added, but no problem from my side,
it does not hurt to have 4 more lines just in case, right?

Thanks for the patch and the explanation!

You have my ack already...
Tony Lindgren Oct. 24, 2012, 1:54 a.m. UTC | #4 
* Ohad Ben-Cohen <ohad@wizery.com> [121021 07:56]:
> On Fri, Oct 19, 2012 at 7:07 PM, Tony Lindgren <tony@atomide.com> wrote:
> --- a/arch/arm/mach-davinci/board-da850-evm.c
> +++ b/arch/arm/mach-davinci/board-da850-evm.c
> @@ -1401,13 +1401,9 @@ static __init int da850_wl12xx_init(void)
>  		goto free_wlan_en;
>  	}
> 
> -	da850_wl12xx_wlan_data.irq = gpio_to_irq(DA850_WLAN_IRQ);
> -
> -	ret = wl12xx_set_platform_data(&da850_wl12xx_wlan_data);
> -	if (ret) {
> -		pr_err("Could not set wl12xx data: %d\n", ret);
> +	ret = wl12xx_board_init(&da850_wl12xx_wlan_data, DA850_WLAN_IRQ);
> +	if (ret)
>  		goto free_wlan_irq;
> -	}
> 
>  	return 0;

I don't think mach-davinci has wl12xx_board_init() available?
Maybe leave this out or define also also somewhere for mach-davinci?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/mach-davinci/board-da850-evm.c
b/arch/arm/mach-davinci/board-da850-evm.c
index 32ee3f8..7243c22 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -1401,13 +1401,9 @@  static __init int da850_wl12xx_init(void)
 		goto free_wlan_en;
 	}

-	da850_wl12xx_wlan_data.irq = gpio_to_irq(DA850_WLAN_IRQ);
-
-	ret = wl12xx_set_platform_data(&da850_wl12xx_wlan_data);
-	if (ret) {
-		pr_err("Could not set wl12xx data: %d\n", ret);
+	ret = wl12xx_board_init(&da850_wl12xx_wlan_data, DA850_WLAN_IRQ);
+	if (ret)
 		goto free_wlan_irq;
-	}

 	return 0;

diff --git a/arch/arm/mach-omap2/board-4430sdp.c
b/arch/arm/mach-omap2/board-4430sdp.c
index 3669c12..bc48679 100644
--- a/arch/arm/mach-omap2/board-4430sdp.c
+++ b/arch/arm/mach-omap2/board-4430sdp.c
@@ -825,10 +825,11 @@  static void __init omap4_sdp4430_wifi_init(void)
 	int ret;

 	omap4_sdp4430_wifi_mux_init();
-	omap4_sdp4430_wlan_data.irq = gpio_to_irq(GPIO_WIFI_IRQ);
-	ret = wl12xx_set_platform_data(&omap4_sdp4430_wlan_data);
+
+	ret = wl12xx_board_init(&omap4_sdp4430_wlan_data, GPIO_WIFI_IRQ);
 	if (ret)
-		pr_err("Error setting wl12xx data: %d\n", ret);
+		return;
+
 	ret = platform_device_register(&omap_vwlan_device);
 	if (ret)
 		pr_err("Error registering wl12xx device: %d\n", ret);
diff --git a/arch/arm/mach-omap2/board-omap3evm.c
b/arch/arm/mach-omap2/board-omap3evm.c
index b9b776b..8e3a075 100644
--- a/arch/arm/mach-omap2/board-omap3evm.c
+++ b/arch/arm/mach-omap2/board-omap3evm.c
@@ -636,10 +636,10 @@  static void __init omap3_evm_wl12xx_init(void)
 	int ret;

 	/* WL12xx WLAN Init */
-	omap3evm_wlan_data.irq = gpio_to_irq(OMAP3EVM_WLAN_IRQ_GPIO);
-	ret = wl12xx_set_platform_data(&omap3evm_wlan_data);
+	ret = wl12xx_board_init(&omap3evm_wlan_data, OMAP3EVM_WLAN_IRQ_GPIO);
 	if (ret)
-		pr_err("error setting wl12xx data: %d\n", ret);
+		return;
+
 	ret = platform_device_register(&omap3evm_wlan_regulator);
 	if (ret)
 		pr_err("error registering wl12xx device: %d\n", ret);
diff --git a/arch/arm/mach-omap2/board-omap3pandora.c
b/arch/arm/mach-omap2/board-omap3pandora.c
index 00a1f4a..bfdc7aa 100644
--- a/arch/arm/mach-omap2/board-omap3pandora.c
+++ b/arch/arm/mach-omap2/board-omap3pandora.c
@@ -543,13 +543,10 @@  static void __init pandora_wl1251_init(void)
 	if (ret < 0)
 		goto fail;

-	pandora_wl1251_pdata.irq = gpio_to_irq(PANDORA_WIFI_IRQ_GPIO);
-	if (pandora_wl1251_pdata.irq < 0)
-		goto fail_irq;
-
 	pandora_wl1251_pdata.use_eeprom = true;
-	ret = wl12xx_set_platform_data(&pandora_wl1251_pdata);
-	if (ret < 0)
+
+	ret = wl12xx_board_init(&pandora_wl1251_pdata, PANDORA_WIFI_IRQ_GPIO);
+	if (ret)
 		goto fail_irq;

 	return;
diff --git a/arch/arm/mach-omap2/board-omap4panda.c
b/arch/arm/mach-omap2/board-omap4panda.c
index bfcd397..f203cd9 100644
--- a/arch/arm/mach-omap2/board-omap4panda.c
+++ b/arch/arm/mach-omap2/board-omap4panda.c
@@ -486,24 +486,32 @@  static void omap4_panda_init_rev(void)
 	}
 }

+static void __init omap4_panda_wlan_init(void)
+{
+	int ret;
+
+	ret = wl12xx_board_init(&omap_panda_wlan_data, GPIO_WIFI_IRQ);
+	if (ret)
+		return;
+
+	ret = platform_device_register(&omap_vwlan_device);
+	if (ret)
+		pr_err("error registering wl12xx's fixed regulator: %d\n", ret);
+}
+
 static void __init omap4_panda_init(void)
 {
 	int package = OMAP_PACKAGE_CBS;
-	int ret;

 	if (omap_rev() == OMAP4430_REV_ES1_0)
 		package = OMAP_PACKAGE_CBL;
 	omap4_mux_init(board_mux, NULL, package);

-	omap_panda_wlan_data.irq = gpio_to_irq(GPIO_WIFI_IRQ);
-	ret = wl12xx_set_platform_data(&omap_panda_wlan_data);
-	if (ret)
-		pr_err("error setting wl12xx data: %d\n", ret);
+	omap4_panda_wlan_init();

 	omap4_panda_init_rev();
 	omap4_panda_i2c_init();
 	platform_add_devices(panda_devices, ARRAY_SIZE(panda_devices));
-	platform_device_register(&omap_vwlan_device);
 	omap_serial_init();
 	omap_sdrc_init(NULL, NULL);
 	omap4_twl6030_hsmmc_init(mmc);
diff --git a/arch/arm/mach-omap2/board-zoom-peripherals.c
b/arch/arm/mach-omap2/board-zoom-peripherals.c
index c166fe1..c81a2c7 100644
--- a/arch/arm/mach-omap2/board-zoom-peripherals.c
+++ b/arch/arm/mach-omap2/board-zoom-peripherals.c
@@ -286,19 +286,24 @@  static void enable_board_wakeup_source(void)
 		OMAP_WAKEUP_EN | OMAP_PIN_INPUT_PULLUP);
 }

-void __init zoom_peripherals_init(void)
+static void __init zoom_wlan_init(void)
 {
 	int ret;

-	omap_zoom_wlan_data.irq = gpio_to_irq(OMAP_ZOOM_WLAN_IRQ_GPIO);
-	ret = wl12xx_set_platform_data(&omap_zoom_wlan_data);
+	ret = wl12xx_board_init(&omap_zoom_wlan_data, OMAP_ZOOM_WLAN_IRQ_GPIO);
+	if (ret)
+		return;

+	ret = platform_device_register(&omap_vwlan_device);
 	if (ret)
-		pr_err("error setting wl12xx data: %d\n", ret);
+		pr_err("error registering wl12xx's fixed regulator: %d\n", ret);
+}

+void __init zoom_peripherals_init(void)
+{
 	omap_hsmmc_init(mmc);
+	zoom_wlan_init();
 	omap_i2c_init();
-	platform_device_register(&omap_vwlan_device);
 	usb_musb_init(NULL);
 	enable_board_wakeup_source();
 	omap_serial_init();
diff --git a/arch/arm/mach-omap2/common-board-devices.c
b/arch/arm/mach-omap2/common-board-devices.c
index 48daac2..02351e9 100644
--- a/arch/arm/mach-omap2/common-board-devices.c
+++ b/arch/arm/mach-omap2/common-board-devices.c
@@ -23,6 +23,7 @@ 
 #include <linux/gpio.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/ads7846.h>
+#include <linux/wl12xx.h>

 #include <linux/platform_data/spi-omap2-mcspi.h>
 #include <linux/platform_data/mtd-nand-omap2.h>
@@ -141,3 +142,35 @@  void __init omap_nand_flash_init(int options,
struct mtd_partition *parts,
 {
 }
 #endif
+
+#ifdef CONFIG_WL12XX_PLATFORM_DATA
+int __init wl12xx_board_init(struct wl12xx_platform_data *wlan_data, int gpio)
+{
+	int ret;
+
+	wlan_data->irq = gpio_to_irq(gpio);
+	if (wlan_data->irq < 0) {
+		ret = wlan_data->irq;
+		pr_err("wl12xx: gpio_to_irq(%d) failed: %d\n", gpio, ret);
+		return ret;
+	}
+
+	ret = wl12xx_set_platform_data(wlan_data);
+	/* bail out silently in case wl12xx isn't configured */
+	if (ret == -ENOSYS)
+		return ret;
+
+	/* bail out verbosely on any other error */
+	if (ret) {
+		pr_err("error setting wl12xx data: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+#else /* !CONFIG_WL12XX_PLATFORM_DATA */
+int __init wl12xx_board_init(struct wl12xx_platform_data *wlan_data, int gpio)
+{
+	return 0;
+}
+#endif
diff --git a/arch/arm/mach-omap2/common-board-devices.h
b/arch/arm/mach-omap2/common-board-devices.h
index a0b4a428..0e7398f 100644
--- a/arch/arm/mach-omap2/common-board-devices.h
+++ b/arch/arm/mach-omap2/common-board-devices.h
@@ -7,9 +7,11 @@ 

 struct mtd_partition;
 struct ads7846_platform_data;
+struct wl12xx_platform_data;

 void omap_ads7846_init(int bus_num, int gpio_pendown, int gpio_debounce,
 		       struct ads7846_platform_data *board_pdata);
 void omap_nand_flash_init(int opts, struct mtd_partition *parts, int n_parts);
+int wl12xx_board_init(struct wl12xx_platform_data *wlan_data, int gpio);

 #endif /* __OMAP_COMMON_BOARD_DEVICES__ */