Hi Ohad,
On 10/17/12 11:10, Ohad Ben-Cohen wrote:
> On Tue, Oct 16, 2012 at 8:26 PM, Tony Lindgren <tony@xxxxxxxxxxx> wrote:
>> Hmm looking at it repeats the same code over again. Can you
>> rather add some wl12xx_board_init() helper function to mach-omap2/devices.c
>> to do it?
>
> Nice, see below. Note that I can only compile test this now, which may
> be ok because it's pretty trivial. But do let me know if you want me
> to get it tested.
The patch looks good, though minor comment below.
>
>>From b940fb88a97494ad3a0a093279a5f176c0b29e44 Mon Sep 17 00:00:00 2001
> From: Ohad Ben-Cohen <ohad@xxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxx>
> Signed-off-by: Ohad Ben-Cohen <ohad@xxxxxxxxxx>
> ---
> 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.h | 2 ++
> arch/arm/mach-omap2/devices.c | 26 ++++++++++++++++++++++++++
> 8 files changed, 64 insertions(+), 29 deletions(-)
[...]
> diff --git a/arch/arm/mach-omap2/common-board-devices.h
> b/arch/arm/mach-omap2/common-board-devices.h
> index a0b4a428..52e91424 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 __init wl12xx_board_init(struct wl12xx_platform_data *wlan_data, int gpio);
You are adding declarations inside the common-board-devices.h,
but the implementation inside the devices.c.
I can see that devices.c has only on-soc devices in it.
So, I would expect to have the wl12xx_board_init() function implementation
inside the common-board-devices.c file.
Another minor nit: I don't think you need to mark the declaration as __init,
only the implementation, or is it for documenting it so?
>
> #endif /* __OMAP_COMMON_BOARD_DEVICES__ */
> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
> index c8c2117..9e86bb9 100644
> --- a/arch/arm/mach-omap2/devices.c
> +++ b/arch/arm/mach-omap2/devices.c
> @@ -19,6 +19,7 @@
> #include <linux/of.h>
> #include <linux/pinctrl/machine.h>
> #include <linux/platform_data/omap4-keypad.h>
> +#include <linux/wl12xx.h>
>
> #include <asm/mach-types.h>
> #include <asm/mach/map.h>
> @@ -38,6 +39,31 @@
> #define L3_MODULES_MAX_LEN 12
> #define L3_MODULES 3
>
> +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;
Instead of the above, wouldn't it be better to have:
#if defined(CONFIG_WL12XX_PLATFORM_DATA)
int __init wl12xx_board_init(struct wl12xx_platform_data *wlan_data, int irq_gpio)
{
...
}
#else
inline int wl12xx_board_init(struct wl12xx_platform_data *wlan_data, int irq_gpio)
{
return 0;
}
#endif
> +
> + /* bail out verbosely on any other error */
> + if (ret) {
> + pr_err("error setting wl12xx data: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int __init omap3_l3_init(void)
> {
> struct omap_hwmod *oh;
--
Regards,
Igor.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
