LinuxLists.cc - [PATCH 0/3] wl1251 SDIO patches

[permalink] [raw]

Subject: Re: [PATCH 2/3] wl1251: add runtime PM support for SDIO

Ohad Ben-Cohen <[email protected]> writes:

> On Sun, Nov 7, 2010 at 12:18 PM, Kalle Valo <[email protected]> wrote:
>>> +static int wl1251_suspend(struct device *dev)
>>> +{
>>> + /*
>>> + * Tell MMC/SDIO core it's OK to power down the card
>>> + * (if it isn't already), but not to remove it completely
>>> + */
>>> + return 0;
>>> +}
>>
>> Sorry, I'm not familiar with pm_ops and I don't fully understand the
>> comment above. Does the comment mean that by returning 0 we can
>> accomplish all that?
>
> Yes. By returning 0 we let the MMC core power down our card, but not
> remove it.

Good, thanks for confirming that.

--
Kalle Valo

2010-11-03 22:14:01

[permalink] [raw]

Subject: [PATCH 3/3] wl1251: use wl12xx_platform_data to pass data

Make use the newly added method to pass platform data for wl1251 too.
This allows to eliminate some redundant code.

Cc: Tony Lindgren <[email protected]>
Cc: Ohad Ben-Cohen <[email protected]>
Signed-off-by: Grazvydas Ignotas <[email protected]>
---
This touches arch/arm/mach-omap2/* but I think it should go through
the wireless tree to avoid cross-tree dependencies, if Tony and others
are ok with this.

arch/arm/mach-omap2/board-omap3pandora.c | 32 +++++++-------------------
drivers/net/wireless/wl1251/sdio.c | 35 +----------------------------
drivers/net/wireless/wl12xx/Kconfig | 2 +-
3 files changed, 12 insertions(+), 57 deletions(-)

diff --git a/arch/arm/mach-omap2/board-omap3pandora.c b/arch/arm/mach-omap2/board-omap3pandora.c
index 89ed1be..8be2615 100644
--- a/arch/arm/mach-omap2/board-omap3pandora.c
+++ b/arch/arm/mach-omap2/board-omap3pandora.c
@@ -642,31 +642,13 @@ static void __init omap3pandora_init_irq(void)
omap_gpio_init();
}

-static void pandora_wl1251_set_power(bool enable)
-{
- /*
- * Keep power always on until wl1251_sdio driver learns to re-init
- * the chip after powering it down and back up.
- */
-}
-
-static struct wl12xx_platform_data pandora_wl1251_pdata = {
- .set_power = pandora_wl1251_set_power,
- .use_eeprom = true,
-};
-
-static struct platform_device pandora_wl1251_data = {
- .name = "wl1251_data",
- .id = -1,
- .dev = {
- .platform_data = &pandora_wl1251_pdata,
- },
-};
-
-static void pandora_wl1251_init(void)
+static void __init pandora_wl1251_init(void)
{
+ struct wl12xx_platform_data pandora_wl1251_pdata;
int ret;

+ memset(&pandora_wl1251_pdata, 0, sizeof(pandora_wl1251_pdata));
+
ret = gpio_request(PANDORA_WIFI_IRQ_GPIO, "wl1251 irq");
if (ret < 0)
goto fail;
@@ -679,6 +661,11 @@ static void pandora_wl1251_init(void)
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)
+ goto fail_irq;
+
return;

fail_irq:
@@ -691,7 +678,6 @@ static struct platform_device *omap3pandora_devices[] __initdata = {
&pandora_leds_gpio,
&pandora_keys_gpio,
&pandora_dss_device,
- &pandora_wl1251_data,
&pandora_vwlan_device,
};

diff --git a/drivers/net/wireless/wl1251/sdio.c b/drivers/net/wireless/wl1251/sdio.c
index 0a5db21..c27cc8c 100644
--- a/drivers/net/wireless/wl1251/sdio.c
+++ b/drivers/net/wireless/wl1251/sdio.c
@@ -43,8 +43,6 @@ struct wl1251_sdio {
u32 elp_val;
};

-static struct wl12xx_platform_data *wl12xx_board_data;
-
static struct sdio_func *wl_to_func(struct wl1251 *wl)
{
struct wl1251_sdio *wl_sdio = wl->if_priv;
@@ -215,30 +213,6 @@ static struct wl1251_if_operations wl1251_sdio_ops = {
.power = wl1251_sdio_set_power,
};

-static int wl1251_platform_probe(struct platform_device *pdev)
-{
- if (pdev->id != -1) {
- wl1251_error("can only handle single device");
- return -ENODEV;
- }
-
- wl12xx_board_data = pdev->dev.platform_data;
- return 0;
-}
-
-/*
- * Dummy platform_driver for passing platform_data to this driver,
- * until we have a way to pass this through SDIO subsystem or
- * some other way.
- */
-static struct platform_driver wl1251_platform_driver = {
- .driver = {
- .name = "wl1251_data",
- .owner = THIS_MODULE,
- },
- .probe = wl1251_platform_probe,
-};
-
static int wl1251_sdio_probe(struct sdio_func *func,
const struct sdio_device_id *id)
{
@@ -246,6 +220,7 @@ static int wl1251_sdio_probe(struct sdio_func *func,
struct wl1251 *wl;
struct ieee80211_hw *hw;
struct wl1251_sdio *wl_sdio;
+ const struct wl12xx_platform_data *wl12xx_board_data;

hw = wl1251_alloc_hw();
if (IS_ERR(hw))
@@ -272,6 +247,7 @@ static int wl1251_sdio_probe(struct sdio_func *func,
wl->if_priv = wl_sdio;
wl->if_ops = &wl1251_sdio_ops;

+ wl12xx_board_data = wl12xx_get_platform_data();
if (wl12xx_board_data != NULL) {
wl->set_power = wl12xx_board_data->set_power;
wl->irq = wl12xx_board_data->irq;
@@ -376,12 +352,6 @@ static int __init wl1251_sdio_init(void)
{
int err;

- err = platform_driver_register(&wl1251_platform_driver);
- if (err) {
- wl1251_error("failed to register platform driver: %d", err);
- return err;
- }
-
err = sdio_register_driver(&wl1251_sdio_driver);
if (err)
wl1251_error("failed to register sdio driver: %d", err);
@@ -391,7 +361,6 @@ static int __init wl1251_sdio_init(void)
static void __exit wl1251_sdio_exit(void)
{
sdio_unregister_driver(&wl1251_sdio_driver);
- platform_driver_unregister(&wl1251_platform_driver);
wl1251_notice("unloaded");
}

diff --git a/drivers/net/wireless/wl12xx/Kconfig b/drivers/net/wireless/wl12xx/Kconfig
index b447559..1f4d73f 100644
--- a/drivers/net/wireless/wl12xx/Kconfig
+++ b/drivers/net/wireless/wl12xx/Kconfig
@@ -42,5 +42,5 @@ config WL1271_SDIO

config WL12XX_PLATFORM_DATA
bool
- depends on WL1271_SDIO != n
+ depends on WL1271_SDIO != n || WL1251_SDIO != n
default y
--
1.6.3.3

2010-11-07 10:23:43

[permalink] [raw]

Subject: Re: [PATCH 2/3] wl1251: add runtime PM support for SDIO

On Sun, Nov 7, 2010 at 12:20 PM, Kalle Valo <[email protected]> wrote:
> Maybe a comment stating the reason for the callback would make it more
> clear?

Definitely.

>
> --
> Kalle Valo
>

2010-11-07 10:15:33

[permalink] [raw]

Subject: Re: [PATCH 1/3] wl1251: add power callback to wl1251_if_operations

Grazvydas Ignotas <[email protected]> writes:

> Call interface specific power callback before calling board specific
> one. Also allow that callback to fail. This is how it's done for
> wl1271 and will be used for runtime_pm support.

Thanks, looks good to me.

> Signed-off-by: Grazvydas Ignotas <[email protected]>

Acked-by: Kalle Valo <[email protected]>

--
Kalle Valo

2010-11-07 10:20:10

[permalink] [raw]

Subject: Re: [PATCH 2/3] wl1251: add runtime PM support for SDIO

Ohad Ben-Cohen <[email protected]> writes:

>>> Why do you still need that ->set_power() handler ?
>>
>> On pandora besides power, we also have a GPIO to control clock buffer
>> for wl1251, so I thought I could enable it here.
>
> Oh, I see. The name set_power then is a bit misleading.. ;)

Maybe a comment stating the reason for the callback would make it more
clear?

--
Kalle Valo

2010-11-08 13:26:23

[permalink] [raw]

Subject: Re: [PATCH 2/3] wl1251: add runtime PM support for SDIO

On Sun, Nov 7, 2010 at 12:31 PM, Kalle Valo <[email protected]> wrote:
> Ohad Ben-Cohen <[email protected]> writes:
>
>> On Sun, Nov 7, 2010 at 12:18 PM, Kalle Valo <[email protected]> wrote:
>>>> +static int wl1251_suspend(struct device *dev)
>>>> +{
>>>> + ? ? /*
>>>> + ? ? ?* Tell MMC/SDIO core it's OK to power down the card
>>>> + ? ? ?* (if it isn't already), but not to remove it completely
>>>> + ? ? ?*/
>>>> + ? ? return 0;
>>>> +}
>>>
>>> Sorry, I'm not familiar with pm_ops and I don't fully understand the
>>> comment above. Does the comment mean that by returning 0 we can
>>> accomplish all that?
>>
>> Yes. By returning 0 we let the MMC core power down our card, but not
>> remove it.
>
> Good, thanks for confirming that.

Thanks, updating comments and resending this one in a moment.

2010-11-03 22:13:57

[permalink] [raw]

Subject: [PATCH 1/3] wl1251: add power callback to wl1251_if_operations

Call interface specific power callback before calling board specific
one. Also allow that callback to fail. This is how it's done for
wl1271 and will be used for runtime_pm support.

Signed-off-by: Grazvydas Ignotas <[email protected]>
---
drivers/net/wireless/wl1251/main.c | 15 +++++++++------
drivers/net/wireless/wl1251/sdio.c | 8 ++++++--
drivers/net/wireless/wl1251/spi.c | 9 +++++++++
drivers/net/wireless/wl1251/wl1251.h | 1 +
4 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/wl1251/main.c b/drivers/net/wireless/wl1251/main.c
index 7a87625..012e1a4 100644
--- a/drivers/net/wireless/wl1251/main.c
+++ b/drivers/net/wireless/wl1251/main.c
@@ -52,14 +52,14 @@ void wl1251_disable_interrupts(struct wl1251 *wl)
wl->if_ops->disable_irq(wl);
}

-static void wl1251_power_off(struct wl1251 *wl)
+static int wl1251_power_off(struct wl1251 *wl)
{
- wl->set_power(false);
+ return wl->if_ops->power(wl, false);
}

-static void wl1251_power_on(struct wl1251 *wl)
+static int wl1251_power_on(struct wl1251 *wl)
{
- wl->set_power(true);
+ return wl->if_ops->power(wl, true);
}

static int wl1251_fetch_firmware(struct wl1251 *wl)
@@ -152,9 +152,12 @@ static void wl1251_fw_wakeup(struct wl1251 *wl)

static int wl1251_chip_wakeup(struct wl1251 *wl)
{
- int ret = 0;
+ int ret;
+
+ ret = wl1251_power_on(wl);
+ if (ret < 0)
+ return ret;

- wl1251_power_on(wl);
msleep(WL1251_POWER_ON_SLEEP);
wl->if_ops->reset(wl);

diff --git a/drivers/net/wireless/wl1251/sdio.c b/drivers/net/wireless/wl1251/sdio.c
index 74ba9ce..0285190 100644
--- a/drivers/net/wireless/wl1251/sdio.c
+++ b/drivers/net/wireless/wl1251/sdio.c
@@ -171,8 +171,12 @@ static void wl1251_disable_line_irq(struct wl1251 *wl)
return disable_irq(wl->irq);
}

-static void wl1251_sdio_set_power(bool enable)
+static int wl1251_sdio_set_power(struct wl1251 *wl, bool enable)
{
+ if (wl->set_power)
+ wl->set_power(enable);
+
+ return 0;
}

static struct wl1251_if_operations wl1251_sdio_ops = {
@@ -181,6 +185,7 @@ static struct wl1251_if_operations wl1251_sdio_ops = {
.write_elp = wl1251_sdio_write_elp,
.read_elp = wl1251_sdio_read_elp,
.reset = wl1251_sdio_reset,
+ .power = wl1251_sdio_set_power,
};

static int wl1251_platform_probe(struct platform_device *pdev)
@@ -239,7 +244,6 @@ static int wl1251_sdio_probe(struct sdio_func *func,
wl_sdio->func = func;
wl->if_priv = wl_sdio;
wl->if_ops = &wl1251_sdio_ops;
- wl->set_power = wl1251_sdio_set_power;

if (wl12xx_board_data != NULL) {
wl->set_power = wl12xx_board_data->set_power;
diff --git a/drivers/net/wireless/wl1251/spi.c b/drivers/net/wireless/wl1251/spi.c
index 88fa8e6..ac872b3 100644
--- a/drivers/net/wireless/wl1251/spi.c
+++ b/drivers/net/wireless/wl1251/spi.c
@@ -215,12 +215,21 @@ static void wl1251_spi_disable_irq(struct wl1251 *wl)
return disable_irq(wl->irq);
}

+static int wl1251_spi_set_power(struct wl1251 *wl, bool enable)
+{
+ if (wl->set_power)
+ wl->set_power(enable);
+
+ return 0;
+}
+
static const struct wl1251_if_operations wl1251_spi_ops = {
.read = wl1251_spi_read,
.write = wl1251_spi_write,
.reset = wl1251_spi_reset_wake,
.enable_irq = wl1251_spi_enable_irq,
.disable_irq = wl1251_spi_disable_irq,
+ .power = wl1251_spi_set_power,
};

static int __devinit wl1251_spi_probe(struct spi_device *spi)
diff --git a/drivers/net/wireless/wl1251/wl1251.h b/drivers/net/wireless/wl1251/wl1251.h
index e113d4c..13fbeec 100644
--- a/drivers/net/wireless/wl1251/wl1251.h
+++ b/drivers/net/wireless/wl1251/wl1251.h
@@ -256,6 +256,7 @@ struct wl1251_if_operations {
void (*write)(struct wl1251 *wl, int addr, void *buf, size_t len);
void (*read_elp)(struct wl1251 *wl, int addr, u32 *val);
void (*write_elp)(struct wl1251 *wl, int addr, u32 val);
+ int (*power)(struct wl1251 *wl, bool enable);
void (*reset)(struct wl1251 *wl);
void (*enable_irq)(struct wl1251 *wl);
void (*disable_irq)(struct wl1251 *wl);
--
1.6.3.3

2010-11-07 10:25:56

[permalink] [raw]

Subject: Re: [PATCH 2/3] wl1251: add runtime PM support for SDIO

On Sun, Nov 7, 2010 at 12:18 PM, Kalle Valo <[email protected]> wrote:
>> +static int wl1251_suspend(struct device *dev)
>> +{
>> + ? ? /*
>> + ? ? ?* Tell MMC/SDIO core it's OK to power down the card
>> + ? ? ?* (if it isn't already), but not to remove it completely
>> + ? ? ?*/
>> + ? ? return 0;
>> +}
>
> Sorry, I'm not familiar with pm_ops and I don't fully understand the
> comment above. Does the comment mean that by returning 0 we can
> accomplish all that?

Yes. By returning 0 we let the MMC core power down our card, but not remove it.

> Or instead is it a fixme comment that we should
> do that, but it's not implemented yet?
>
> --
> Kalle Valo
>

2010-11-07 10:18:40

[permalink] [raw]

Subject: Re: [PATCH 2/3] wl1251: add runtime PM support for SDIO

Grazvydas Ignotas <[email protected]> writes:

> Add runtime PM support, similar to how it's done for wl1271.
> This allows to power down the card when the driver is loaded but
> network is not in use.

Few minor comments:

> + /* Undo decrement done above in wl1271_probe */

Should be wl1251_probe().

> +static int wl1251_suspend(struct device *dev)
> +{
> + /*
> + * Tell MMC/SDIO core it's OK to power down the card
> + * (if it isn't already), but not to remove it completely
> + */
> + return 0;
> +}

Sorry, I'm not familiar with pm_ops and I don't fully understand the
comment above. Does the comment mean that by returning 0 we can
accomplish all that? Or instead is it a fixme comment that we should
do that, but it's not implemented yet?

--
Kalle Valo

2010-11-08 15:19:50

by Tony Lindgren

[permalink] [raw]

Subject: Re: [PATCH 3/3] wl1251: use wl12xx_platform_data to pass data

* Luciano Coelho <[email protected]> [101108 01:05]:
> On Sun, 2010-11-07 at 11:28 +0100, ext Kalle Valo wrote:
> > Grazvydas Ignotas <[email protected]> writes:
> >
> > > Make use the newly added method to pass platform data for wl1251 too.
> > > This allows to eliminate some redundant code.
> > >
> > > Cc: Tony Lindgren <[email protected]>
> > > Cc: Ohad Ben-Cohen <[email protected]>
> > > Signed-off-by: Grazvydas Ignotas <[email protected]>
> >
> > For the wl1251 part:
> >
> > Acked-by: Kalle Valo <[email protected]>
> >
> > > ---
> > > This touches arch/arm/mach-omap2/* but I think it should go through
> > > the wireless tree to avoid cross-tree dependencies, if Tony and others
> > > are ok with this.
> >
> > I agree, better to push this through the wireless tree.
> >
> > > arch/arm/mach-omap2/board-omap3pandora.c | 32 +++++++-------------------
> > > drivers/net/wireless/wl1251/sdio.c | 35 +----------------------------
> > > drivers/net/wireless/wl12xx/Kconfig | 2 +-
> >
> > We need to CC Luciano for the wl12xx change.
> >
> > > --- a/drivers/net/wireless/wl12xx/Kconfig
> > > +++ b/drivers/net/wireless/wl12xx/Kconfig
> > > @@ -42,5 +42,5 @@ config WL1271_SDIO
> > >
> > > config WL12XX_PLATFORM_DATA
> > > bool
> > > - depends on WL1271_SDIO != n
> > > + depends on WL1271_SDIO != n || WL1251_SDIO != n
> > > default y
> >
> > Oh, I didn't take this into account when I moved wl1251 out from the
> > wl12xx directory. Now wl1251 has a dependency to wl12xx stuff.
> >
> > Oh well, it's a small issue. I guess we can live with that :)
>
> Yeah, I didn't realize this either. I think we can live with that, but
> at some point we should clean this up and either have separate platform
> data for wl1251 and wl12xx or move the the platform data one level up,
> to wireless. But for now:
>
> Acked-by: Luciano Coelho <[email protected]>
>
> And I agree with pushing this through wireless-testing as well, if it's
> okay with Tony and John.

Yes, ack from me too:

Acked-by: Tony Lindgren <[email protected]>

2010-11-04 13:51:58

[permalink] [raw]

Subject: Re: [PATCH 2/3] wl1251: add runtime PM support for SDIO

On Wed, Nov 3, 2010 at 6:13 PM, Grazvydas Ignotas <[email protected]> wrote:
> Add runtime PM support, similar to how it's done for wl1271.
> This allows to power down the card when the driver is loaded but
> network is not in use.
>
> CC: Ohad Ben-Cohen <[email protected]>
> Signed-off-by: Grazvydas Ignotas <[email protected]>
> ---
> ?drivers/net/wireless/wl1251/sdio.c | ? 62 ++++++++++++++++++++++++++++++++++--
> ?1 files changed, 59 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/wl1251/sdio.c b/drivers/net/wireless/wl1251/sdio.c
> index 0285190..0a5db21 100644
> --- a/drivers/net/wireless/wl1251/sdio.c
> +++ b/drivers/net/wireless/wl1251/sdio.c
> @@ -26,6 +26,7 @@
> ?#include <linux/platform_device.h>
> ?#include <linux/wl12xx.h>
> ?#include <linux/irq.h>
> +#include <linux/pm_runtime.h>
>
> ?#include "wl1251.h"
>
> @@ -173,10 +174,36 @@ static void wl1251_disable_line_irq(struct wl1251 *wl)
>
> ?static int wl1251_sdio_set_power(struct wl1251 *wl, bool enable)
> ?{
> - ? ? ? if (wl->set_power)
> - ? ? ? ? ? ? ? wl->set_power(enable);
> + ? ? ? struct sdio_func *func = wl_to_func(wl);
> + ? ? ? int ret;
>
> - ? ? ? return 0;
> + ? ? ? if (enable) {
> + ? ? ? ? ? ? ? /* Power up the card */
> + ? ? ? ? ? ? ? if (wl->set_power)
> + ? ? ? ? ? ? ? ? ? ? ? wl->set_power(true);

Why do you still need that ->set_power() handler ?

> +
> + ? ? ? ? ? ? ? ret = pm_runtime_get_sync(&func->dev);

This call should do all the power-related work for you (assuming you
have a fixed regulator in place).

> + ? ? ? ? ? ? ? if (ret < 0)
> + ? ? ? ? ? ? ? ? ? ? ? goto out;
> +
> + ? ? ? ? ? ? ? sdio_claim_host(func);
> + ? ? ? ? ? ? ? sdio_enable_func(func);
> + ? ? ? ? ? ? ? sdio_release_host(func);
> + ? ? ? } else {
> + ? ? ? ? ? ? ? sdio_claim_host(func);
> + ? ? ? ? ? ? ? sdio_disable_func(func);
> + ? ? ? ? ? ? ? sdio_release_host(func);
> +
> + ? ? ? ? ? ? ? ret = pm_runtime_put_sync(&func->dev);
> + ? ? ? ? ? ? ? if (ret < 0)
> + ? ? ? ? ? ? ? ? ? ? ? goto out;
> +
> + ? ? ? ? ? ? ? if (wl->set_power)
> + ? ? ? ? ? ? ? ? ? ? ? wl->set_power(false);
> + ? ? ? }
> +
> +out:
> + ? ? ? return ret;
> ?}
>
> ?static struct wl1251_if_operations wl1251_sdio_ops = {
> @@ -277,6 +304,10 @@ static int wl1251_sdio_probe(struct sdio_func *func,
> ? ? ? ? ? ? ? ?goto out_free_irq;
>
> ? ? ? ?sdio_set_drvdata(func, wl);
> +
> + ? ? ? /* Tell PM core that we don't need the card to be powered now */
> + ? ? ? pm_runtime_put_noidle(&func->dev);
> +
> ? ? ? ?return ret;
>
> ?out_free_irq:
> @@ -298,6 +329,9 @@ static void __devexit wl1251_sdio_remove(struct sdio_func *func)
> ? ? ? ?struct wl1251 *wl = sdio_get_drvdata(func);
> ? ? ? ?struct wl1251_sdio *wl_sdio = wl->if_priv;
>
> + ? ? ? /* Undo decrement done above in wl1271_probe */
> + ? ? ? pm_runtime_get_noresume(&func->dev);
> +
> ? ? ? ?if (wl->irq)
> ? ? ? ? ? ? ? ?free_irq(wl->irq, wl);
> ? ? ? ?kfree(wl_sdio);
> @@ -309,11 +343,33 @@ static void __devexit wl1251_sdio_remove(struct sdio_func *func)
> ? ? ? ?sdio_release_host(func);
> ?}
>
> +static int wl1251_suspend(struct device *dev)
> +{
> + ? ? ? /*
> + ? ? ? ?* Tell MMC/SDIO core it's OK to power down the card
> + ? ? ? ?* (if it isn't already), but not to remove it completely
> + ? ? ? ?*/
> + ? ? ? return 0;
> +}
> +
> +static int wl1251_resume(struct device *dev)
> +{
> + ? ? ? return 0;
> +}
> +
> +static const struct dev_pm_ops wl1251_sdio_pm_ops = {
> + ? ? ? .suspend ? ? ? ?= wl1251_suspend,
> + ? ? ? .resume ? ? ? ? = wl1251_resume,
> +};
> +
> ?static struct sdio_driver wl1251_sdio_driver = {
> ? ? ? ?.name ? ? ? ? ? = "wl1251_sdio",
> ? ? ? ?.id_table ? ? ? = wl1251_devices,
> ? ? ? ?.probe ? ? ? ? ?= wl1251_sdio_probe,
> ? ? ? ?.remove ? ? ? ? = __devexit_p(wl1251_sdio_remove),
> + ? ? ? .drv = {
> + ? ? ? ? ? ? ? .pm ? ? = &wl1251_sdio_pm_ops,
> + ? ? ? },
> ?};
>
> ?static int __init wl1251_sdio_init(void)
> --
> 1.6.3.3
>
>

2010-11-07 10:28:27

[permalink] [raw]

Subject: Re: [PATCH 3/3] wl1251: use wl12xx_platform_data to pass data

Grazvydas Ignotas <[email protected]> writes:

> Make use the newly added method to pass platform data for wl1251 too.
> This allows to eliminate some redundant code.
>
> Cc: Tony Lindgren <[email protected]>
> Cc: Ohad Ben-Cohen <[email protected]>
> Signed-off-by: Grazvydas Ignotas <[email protected]>

For the wl1251 part:

Acked-by: Kalle Valo <[email protected]>

> ---
> This touches arch/arm/mach-omap2/* but I think it should go through
> the wireless tree to avoid cross-tree dependencies, if Tony and others
> are ok with this.

I agree, better to push this through the wireless tree.

> arch/arm/mach-omap2/board-omap3pandora.c | 32 +++++++-------------------
> drivers/net/wireless/wl1251/sdio.c | 35 +----------------------------
> drivers/net/wireless/wl12xx/Kconfig | 2 +-

We need to CC Luciano for the wl12xx change.

> --- a/drivers/net/wireless/wl12xx/Kconfig
> +++ b/drivers/net/wireless/wl12xx/Kconfig
> @@ -42,5 +42,5 @@ config WL1271_SDIO
>
> config WL12XX_PLATFORM_DATA
> bool
> - depends on WL1271_SDIO != n
> + depends on WL1271_SDIO != n || WL1251_SDIO != n
> default y

Oh, I didn't take this into account when I moved wl1251 out from the
wl12xx directory. Now wl1251 has a dependency to wl12xx stuff.

Oh well, it's a small issue. I guess we can live with that :)

--
Kalle Valo

2010-11-04 14:37:45

[permalink] [raw]

Subject: Re: [PATCH 2/3] wl1251: add runtime PM support for SDIO

On Thu, Nov 4, 2010 at 3:51 PM, Ohad Ben-Cohen <[email protected]> wrote:
> On Wed, Nov 3, 2010 at 6:13 PM, Grazvydas Ignotas <[email protected]> wrote:
>> Add runtime PM support, similar to how it's done for wl1271.
>> This allows to power down the card when the driver is loaded but
>> network is not in use.
>>
>> CC: Ohad Ben-Cohen <[email protected]>
>> Signed-off-by: Grazvydas Ignotas <[email protected]>
>> ---
>> ?drivers/net/wireless/wl1251/sdio.c | ? 62 ++++++++++++++++++++++++++++++++++--
>> ?1 files changed, 59 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/wireless/wl1251/sdio.c b/drivers/net/wireless/wl1251/sdio.c
>> index 0285190..0a5db21 100644
>> --- a/drivers/net/wireless/wl1251/sdio.c
>> +++ b/drivers/net/wireless/wl1251/sdio.c
>> @@ -26,6 +26,7 @@
>> ?#include <linux/platform_device.h>
>> ?#include <linux/wl12xx.h>
>> ?#include <linux/irq.h>
>> +#include <linux/pm_runtime.h>
>>
>> ?#include "wl1251.h"
>>
>> @@ -173,10 +174,36 @@ static void wl1251_disable_line_irq(struct wl1251 *wl)
>>
>> ?static int wl1251_sdio_set_power(struct wl1251 *wl, bool enable)
>> ?{
>> - ? ? ? if (wl->set_power)
>> - ? ? ? ? ? ? ? wl->set_power(enable);
>> + ? ? ? struct sdio_func *func = wl_to_func(wl);
>> + ? ? ? int ret;
>>
>> - ? ? ? return 0;
>> + ? ? ? if (enable) {
>> + ? ? ? ? ? ? ? /* Power up the card */
>> + ? ? ? ? ? ? ? if (wl->set_power)
>> + ? ? ? ? ? ? ? ? ? ? ? wl->set_power(true);
>
> Why do you still need that ->set_power() handler ?

On pandora besides power, we also have a GPIO to control clock buffer
for wl1251, so I thought I could enable it here. Perhaps it could be
set up as regulator too, but I'm not sure how to set it up so that
clock is enabled before main power. Also it's not really a power
supply so it somehow felt wrong to set up regulator for it.

2010-11-03 22:14:08

[permalink] [raw]

Subject: [PATCH 2/3] wl1251: add runtime PM support for SDIO

Add runtime PM support, similar to how it's done for wl1271.
This allows to power down the card when the driver is loaded but
network is not in use.

CC: Ohad Ben-Cohen <[email protected]>
Signed-off-by: Grazvydas Ignotas <[email protected]>
---
drivers/net/wireless/wl1251/sdio.c | 62 ++++++++++++++++++++++++++++++++++--
1 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/wl1251/sdio.c b/drivers/net/wireless/wl1251/sdio.c
index 0285190..0a5db21 100644
--- a/drivers/net/wireless/wl1251/sdio.c
+++ b/drivers/net/wireless/wl1251/sdio.c
@@ -26,6 +26,7 @@
#include <linux/platform_device.h>
#include <linux/wl12xx.h>
#include <linux/irq.h>
+#include <linux/pm_runtime.h>

#include "wl1251.h"

@@ -173,10 +174,36 @@ static void wl1251_disable_line_irq(struct wl1251 *wl)

static int wl1251_sdio_set_power(struct wl1251 *wl, bool enable)
{
- if (wl->set_power)
- wl->set_power(enable);
+ struct sdio_func *func = wl_to_func(wl);
+ int ret;

- return 0;
+ if (enable) {
+ /* Power up the card */
+ if (wl->set_power)
+ wl->set_power(true);
+
+ ret = pm_runtime_get_sync(&func->dev);
+ if (ret < 0)
+ goto out;
+
+ sdio_claim_host(func);
+ sdio_enable_func(func);
+ sdio_release_host(func);
+ } else {
+ sdio_claim_host(func);
+ sdio_disable_func(func);
+ sdio_release_host(func);
+
+ ret = pm_runtime_put_sync(&func->dev);
+ if (ret < 0)
+ goto out;
+
+ if (wl->set_power)
+ wl->set_power(false);
+ }
+
+out:
+ return ret;
}

static struct wl1251_if_operations wl1251_sdio_ops = {
@@ -277,6 +304,10 @@ static int wl1251_sdio_probe(struct sdio_func *func,
goto out_free_irq;

sdio_set_drvdata(func, wl);
+
+ /* Tell PM core that we don't need the card to be powered now */
+ pm_runtime_put_noidle(&func->dev);
+
return ret;

out_free_irq:
@@ -298,6 +329,9 @@ static void __devexit wl1251_sdio_remove(struct sdio_func *func)
struct wl1251 *wl = sdio_get_drvdata(func);
struct wl1251_sdio *wl_sdio = wl->if_priv;

+ /* Undo decrement done above in wl1271_probe */
+ pm_runtime_get_noresume(&func->dev);
+
if (wl->irq)
free_irq(wl->irq, wl);
kfree(wl_sdio);
@@ -309,11 +343,33 @@ static void __devexit wl1251_sdio_remove(struct sdio_func *func)
sdio_release_host(func);
}

+static int wl1251_suspend(struct device *dev)
+{
+ /*
+ * Tell MMC/SDIO core it's OK to power down the card
+ * (if it isn't already), but not to remove it completely
+ */
+ return 0;
+}
+
+static int wl1251_resume(struct device *dev)
+{
+ return 0;
+}
+
+static const struct dev_pm_ops wl1251_sdio_pm_ops = {
+ .suspend = wl1251_suspend,
+ .resume = wl1251_resume,
+};
+
static struct sdio_driver wl1251_sdio_driver = {
.name = "wl1251_sdio",
.id_table = wl1251_devices,
.probe = wl1251_sdio_probe,
.remove = __devexit_p(wl1251_sdio_remove),
+ .drv = {
+ .pm = &wl1251_sdio_pm_ops,
+ },
};

static int __init wl1251_sdio_init(void)
--
1.6.3.3

2010-11-04 22:05:20