native support for wl1271 on ZOOM

-----Original Message-----
From: linux-omap-owner at vger.kernel.org [mailto:linux-omap- owner at vger.kernel.org] On Behalf Of Ohad Ben-Cohen
Sent: Wednesday, August 11, 2010 11:52 PM
To: linux-wireless at vger.kernel.org; linux-mmc at vger.kernel.org; linux- omap at vger.kernel.org
Cc: Ido Yariv; Mark Brown; linux-arm-kernel at lists.infradead.org; Chikkature Rajashekar, Madhusudhan; Luciano Coelho; akpm at linux- foundation.org; San Mehat; Roger Quadros; Tony Lindgren; Nicolas Pitre; Pandita, Vikram; Kalle Valo; Ohad Ben-Cohen
Subject: [PATCH v4 2/8] wireless: wl1271: support return value for the set power func

Make it possible for the set power method to indicate a success/failure return value. This is needed to support more complex power on/off operations such as bringing up (and down) sdio functions.

Signed-off-by Ohad Ben-Cohen <[email protected]>

---
 drivers/net/wireless/wl12xx/wl1271.h      |    2 +-
 drivers/net/wireless/wl12xx/wl1271_io.h   |    8 +++++---
 drivers/net/wireless/wl12xx/wl1271_main.c |    4 +++-
 drivers/net/wireless/wl12xx/wl1271_sdio.c |    4 +++-
 drivers/net/wireless/wl12xx/wl1271_spi.c  |    4 +++-
 5 files changed, 15 insertions(+), 7 deletions(-)

drivers/net/wireless/wl12xx/wl1271.h +1 -1

index dd3cee6..faa5925 100644
@@ -313,7 +313,7 @@ struct wl1271_if_operations {
 		     bool fixed);
 	void (*reset)(struct wl1271 *wl);
 	void (*init)(struct wl1271 *wl);
-	void (*power)(struct wl1271 *wl, bool enable);
+	int (*power)(struct wl1271 *wl, bool enable);
 	struct device* (*dev)(struct wl1271 *wl);
 	void (*enable_irq)(struct wl1271 *wl);
 	void (*disable_irq)(struct wl1271 *wl);

drivers/net/wireless/wl12xx/wl1271_io.h +0 -0

index bc806c7..4a5b92c 100644

drivers/net/wireless/wl12xx/wl1271_io.h +3 -0

+	if (ret == 0)
+		set_bit(WL1271_FLAG_GPIO_POWER, &wl->flags);
+	return ret;
 }
 

drivers/net/wireless/wl12xx/wl1271_main.c +3 -1

index 9d68f00..e6e0852 100644
@@ -621,7 +621,9 @@ static int wl1271_chip_wakeup(struct wl1271 *wl)
 	int ret = 0;
 
 	msleep(WL1271_PRE_POWER_ON_SLEEP);
-	wl1271_power_on(wl);
+	ret = wl1271_power_on(wl);
+	if (ret < 0)
+		goto out;
 	msleep(WL1271_POWER_ON_SLEEP);
 	wl1271_io_reset(wl);
 	wl1271_io_init(wl);

drivers/net/wireless/wl12xx/wl1271_sdio.c +3 -1

index 7059b5c..c41293a 100644
@@ -152,7 +152,7 @@ static void wl1271_sdio_raw_write(struct wl1271 *wl,
 int addr, void *buf,
 
 }
 
-static void wl1271_sdio_set_power(struct wl1271 *wl, bool enable)
+static int wl1271_sdio_set_power(struct wl1271 *wl, bool enable)
 {
 	struct sdio_func *func = wl_to_func(wl);

@@ -167,6 +167,8 @@ static void wl1271_sdio_set_power(struct wl1271 *wl,
 bool enable)
 		sdio_disable_func(func);
 		sdio_release_host(func);
 	}
+
+	return 0;
 }
 

static struct wl1271_if_operations sdio_ops = {

drivers/net/wireless/wl12xx/wl1271_spi.c +3 -2

index c3fdab7..de56d8d 100644
@@ -312,10 +312,12 @@ static irqreturn_t wl1271_irq(int irq, void *cookie)
 	return IRQ_HANDLED;
 }
 
-static void wl1271_spi_set_power(struct wl1271 *wl, bool enable)
+static int wl1271_spi_set_power(struct wl1271 *wl, bool enable)
 {
 	if (wl->set_power)
 		wl->set_power(enable);
+
+	return 0;
 }
 
 static struct wl1271_if_operations spi_ops = {
--

1.7.0.4

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

Quoted 3 lines

- ? ? wl->if_ops->power(wl, true);
- ? ? set_bit(WL1271_FLAG_GPIO_POWER, &wl->flags);
+ ? ? int ret = wl->if_ops->power(wl, true);

Just a minor comment, need a blank line here?

Quoted 73 lines

+ ? ? if (ret == 0)
+ ? ? ? ? ? ? set_bit(WL1271_FLAG_GPIO_POWER, &wl->flags);
+ ? ? return ret;
?}

drivers/net/wireless/wl12xx/wl1271_main.c +3 -1

index 9d68f00..e6e0852 100644
@@ -621,7 +621,9 @@ static int wl1271_chip_wakeup(struct wl1271 *wl)
 ? ? ? int ret = 0;
 
 ? ? ? msleep(WL1271_PRE_POWER_ON_SLEEP);
- ? ? wl1271_power_on(wl);
+ ? ? ret = wl1271_power_on(wl);
+ ? ? if (ret < 0)
+ ? ? ? ? ? ? goto out;
 ? ? ? msleep(WL1271_POWER_ON_SLEEP);
 ? ? ? wl1271_io_reset(wl);
 ? ? ? wl1271_io_init(wl);

drivers/net/wireless/wl12xx/wl1271_sdio.c +3 -1

index 7059b5c..c41293a 100644
@@ -152,7 +152,7 @@ static void wl1271_sdio_raw_write(struct wl1271 *wl,
 int addr, void *buf,
 
 ?}
 
-static void wl1271_sdio_set_power(struct wl1271 *wl, bool enable)
+static int wl1271_sdio_set_power(struct wl1271 *wl, bool enable)
 ?{
 ? ? ? struct sdio_func *func = wl_to_func(wl);

@@ -167,6 +167,8 @@ static void wl1271_sdio_set_power(struct wl1271 *wl,
 bool enable)
 ? ? ? ? ? ? ? sdio_disable_func(func);
 ? ? ? ? ? ? ? sdio_release_host(func);
 ? ? ? }
+
+ ? ? return 0;
 ?}
 

?static struct wl1271_if_operations sdio_ops = {

drivers/net/wireless/wl12xx/wl1271_spi.c +3 -2

index c3fdab7..de56d8d 100644
@@ -312,10 +312,12 @@ static irqreturn_t wl1271_irq(int irq, void *cookie)
 ? ? ? return IRQ_HANDLED;
 ?}
 
-static void wl1271_spi_set_power(struct wl1271 *wl, bool enable)
+static int wl1271_spi_set_power(struct wl1271 *wl, bool enable)
 ?{
 ? ? ? if (wl->set_power)
 ? ? ? ? ? ? ? wl->set_power(enable);
+
+ ? ? return 0;
 ?}
 
 ?static struct wl1271_if_operations spi_ops = {
--

1.7.0.4

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

-----Original Message-----
From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Ohad Ben-Cohen
Sent: Wednesday, August 11, 2010 11:52 PM
To: [email protected]; [email protected]; linux- [email protected]
Cc: Ido Yariv; Mark Brown; [email protected]; Chikkature Rajashekar, Madhusudhan; Luciano Coelho; akpm@linux- foundation.org; San Mehat; Roger Quadros; Tony Lindgren; Nicolas Pitre; Pandita, Vikram; Kalle Valo; Ohad Ben-Cohen
Subject: [PATCH v4 2/8] wireless: wl1271: support return value for the set power func

Make it possible for the set power method to indicate a success/failure return value. This is needed to support more complex power on/off operations such as bringing up (and down) sdio functions.

Signed-off-by Ohad Ben-Cohen <[email protected]>

---
 drivers/net/wireless/wl12xx/wl1271.h      |    2 +-
 drivers/net/wireless/wl12xx/wl1271_io.h   |    8 +++++---
 drivers/net/wireless/wl12xx/wl1271_main.c |    4 +++-
 drivers/net/wireless/wl12xx/wl1271_sdio.c |    4 +++-
 drivers/net/wireless/wl12xx/wl1271_spi.c  |    4 +++-
 5 files changed, 15 insertions(+), 7 deletions(-)

drivers/net/wireless/wl12xx/wl1271.h +1 -1

index dd3cee6..faa5925 100644
@@ -313,7 +313,7 @@ struct wl1271_if_operations {
 		     bool fixed);
 	void (*reset)(struct wl1271 *wl);
 	void (*init)(struct wl1271 *wl);
-	void (*power)(struct wl1271 *wl, bool enable);
+	int (*power)(struct wl1271 *wl, bool enable);
 	struct device* (*dev)(struct wl1271 *wl);
 	void (*enable_irq)(struct wl1271 *wl);
 	void (*disable_irq)(struct wl1271 *wl);

drivers/net/wireless/wl12xx/wl1271_io.h +0 -0

index bc806c7..4a5b92c 100644

drivers/net/wireless/wl12xx/wl1271_io.h +3 -0

+	if (ret == 0)
+		set_bit(WL1271_FLAG_GPIO_POWER, &wl->flags);
+	return ret;
 }
 

drivers/net/wireless/wl12xx/wl1271_main.c +3 -1

index 9d68f00..e6e0852 100644
@@ -621,7 +621,9 @@ static int wl1271_chip_wakeup(struct wl1271 *wl)
 	int ret = 0;
 
 	msleep(WL1271_PRE_POWER_ON_SLEEP);
-	wl1271_power_on(wl);
+	ret = wl1271_power_on(wl);
+	if (ret < 0)
+		goto out;
 	msleep(WL1271_POWER_ON_SLEEP);
 	wl1271_io_reset(wl);
 	wl1271_io_init(wl);

drivers/net/wireless/wl12xx/wl1271_sdio.c +3 -1

index 7059b5c..c41293a 100644
@@ -152,7 +152,7 @@ static void wl1271_sdio_raw_write(struct wl1271 *wl,
 int addr, void *buf,
 
 }
 
-static void wl1271_sdio_set_power(struct wl1271 *wl, bool enable)
+static int wl1271_sdio_set_power(struct wl1271 *wl, bool enable)
 {
 	struct sdio_func *func = wl_to_func(wl);

@@ -167,6 +167,8 @@ static void wl1271_sdio_set_power(struct wl1271 *wl,
 bool enable)
 		sdio_disable_func(func);
 		sdio_release_host(func);
 	}
+
+	return 0;
 }
 

static struct wl1271_if_operations sdio_ops = {

drivers/net/wireless/wl12xx/wl1271_spi.c +3 -2

index c3fdab7..de56d8d 100644
@@ -312,10 +312,12 @@ static irqreturn_t wl1271_irq(int irq, void *cookie)
 	return IRQ_HANDLED;
 }
 
-static void wl1271_spi_set_power(struct wl1271 *wl, bool enable)
+static int wl1271_spi_set_power(struct wl1271 *wl, bool enable)
 {
 	if (wl->set_power)
 		wl->set_power(enable);
+
+	return 0;
 }
 
 static struct wl1271_if_operations spi_ops = {
--

1.7.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

-----Original Message-----
From: linux-omap-owner at vger.kernel.org [mailto:linux-omap- owner at vger.kernel.org] On Behalf Of Ohad Ben-Cohen
Sent: Wednesday, August 11, 2010 11:52 PM
To: linux-wireless at vger.kernel.org; linux-mmc at vger.kernel.org; linux- omap at vger.kernel.org
Cc: Ido Yariv; Mark Brown; linux-arm-kernel at lists.infradead.org; Chikkature Rajashekar, Madhusudhan; Luciano Coelho; akpm at linux- foundation.org; San Mehat; Roger Quadros; Tony Lindgren; Nicolas Pitre; Pandita, Vikram; Kalle Valo; Ohad Ben-Cohen
Subject: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data

Dynamically create and register a platform driver, that will be used to to receive board-specific information like irq and reference clock numbers.

The driver is created dynamically in order to avoid the 1-device limitation of a fixed platform driver name.

Signed-off-by Ohad Ben-Cohen <[email protected]>

---
 drivers/net/wireless/wl12xx/wl1271.h      |   12 ++++
 drivers/net/wireless/wl12xx/wl1271_sdio.c |  103
+++++++++++++++++++++++++++--
 2 files changed, 108 insertions(+), 7 deletions(-)

drivers/net/wireless/wl12xx/wl1271.h +12 -0

index faa5925..013eabb 100644
@@ -31,6 +31,7 @@
 #include <linux/list.h>
 #include <linux/bitops.h>
 #include <net/mac80211.h>
+#include <linux/platform_device.h>
 
 #include "wl1271_conf.h"
 #include "wl1271_ini.h"

@@ -319,8 +320,19 @@ struct wl1271_if_operations {
 	void (*disable_irq)(struct wl1271 *wl);
 };
 
+/* exact size needed for "wl1271_plat.x" */
+#define WL12XX_PLAT_NAME_LEN 14
+
+struct wl12xx_plat_instance {
+	struct platform_driver pdriver;
+	char name[WL12XX_PLAT_NAME_LEN];
+	struct wl12xx_platform_data *pdata;
+	struct completion data_ready;
+};
+
 struct wl1271 {
 	struct platform_device *plat_dev;
+	struct wl12xx_plat_instance *pinstance;
 	struct ieee80211_hw *hw;
 	bool mac80211_registered;
 

drivers/net/wireless/wl12xx/wl1271_sdio.c +4 -0

index c41293a..68b512b 100644
@@ -28,7 +28,11 @@
 #include <linux/mmc/sdio_func.h>
 #include <linux/mmc/sdio_ids.h>
 #include <linux/mmc/card.h>
+#include <linux/mmc/host.h>
+#include <linux/wl12xx.h>
 #include <linux/gpio.h>
+#include <linux/platform_device.h>
+#include <linux/completion.h>
 
 #include "wl1271.h"
 #include "wl12xx_80211.h"

drivers/net/wireless/wl12xx/wl1271_sdio.c +68 -0

+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		wl1271_error("no platform data");
+		return -ENODEV;
+	}
+
+	pdriver = container_of(pdev->dev.driver, struct platform_driver,
+								driver);
+	pinstance = container_of(pdriver, struct wl12xx_plat_instance,
 pdriver);
+
+	pinstance->pdata = pdata;
+
+	complete(&pinstance->data_ready);
+
+	return 0;
+}
+
+static struct wl12xx_platform_data *wl1271_get_data(struct wl1271 *wl,
 int id)
+{
+	int ret;
+	struct wl12xx_plat_instance *pinstance;
+
+	#define WL1271_PLAT_NAME "wl1271_plat.%d"
+
+	pinstance = kzalloc(sizeof(*pinstance), GFP_KERNEL);
+	if (!pinstance)
+		return ERR_PTR(-ENOMEM);
+
+	init_completion(&pinstance->data_ready);
+
+	pinstance->pdriver.probe = wl1271_plat_probe;
+
+	ret = snprintf(pinstance->name, sizeof(pinstance->name),
+						WL1271_PLAT_NAME, id);
+	if (ret >= WL12XX_PLAT_NAME_LEN) {
+		wl1271_error("truncated plat drv name\n");
+		goto out_free;
+	}
+
+	pinstance->pdriver.driver.name = pinstance->name;
+	pinstance->pdriver.driver.owner = THIS_MODULE;
+
+	ret = platform_driver_register(&pinstance->pdriver);
+	if (ret < 0) {
+		wl1271_error("failed to register plat driver: %d", ret);
+		goto out_free;
+	}
+
+	ret = wait_for_completion_interruptible_timeout(&pinstance-
 >data_ready,
+						msecs_to_jiffies(15000));
+	if (ret <= 0) {
+		wl1271_error("can't get platform device (%d)", ret);
+		ret = (ret == 0 ? -EAGAIN : ret);
+		goto unreg;
+	}
+
+	wl->pinstance = pinstance;
+
+	return pinstance->pdata;
+
+unreg:
+	platform_driver_unregister(&pinstance->pdriver);
+out_free:
+	kfree(pinstance);
+	return ERR_PTR(ret);
+}
+
 static int __devinit wl1271_probe(struct sdio_func *func,
 				  const struct sdio_device_id *id)
 {
 	struct ieee80211_hw *hw;
+	struct wl12xx_platform_data *wlan_data;

drivers/net/wireless/wl12xx/wl1271_sdio.c +9 -2

 	struct wl1271 *wl;
 	int ret;

@@ -205,17 +283,24 @@ static int __devinit wl1271_probe(struct sdio_func
 *func,
 	/* Grab access to FN0 for ELP reg. */
 	func->card->quirks |= MMC_QUIRK_LENIENT_FN0;
 
+	wlan_data = wl1271_get_data(wl, func->card->host->index);
+	if (IS_ERR(wlan_data)) {
+		ret = PTR_ERR(wlan_data);
+		wl1271_error("missing wlan data (needed for irq/ref_clk)!");
+		goto out_free;
+	}
+
 	wl->irq = gpio_to_irq(RX71_WL1271_IRQ_GPIO);
 	if (wl->irq < 0) {
 		ret = wl->irq;
 		wl1271_error("could not get irq!");
-		goto out_free;
+		goto put_data;
 	}
 
 	ret = request_irq(wl->irq, wl1271_irq, 0, DRIVER_NAME, wl);
 	if (ret < 0) {
 		wl1271_error("request_irq() failed: %d", ret);
-		goto out_free;
+		goto put_data;
 	}
 

drivers/net/wireless/wl12xx/wl1271_sdio.c +9 -6

 	set_irq_type(wl->irq, IRQ_TYPE_EDGE_RISING);

@@ -236,13 +321,13 @@ static int __devinit wl1271_probe(struct sdio_func
 *func,
 
 	return 0;
 
- out_irq:
+out_irq:
 	free_irq(wl->irq, wl);
-
-
- out_free:
+put_data:
+	platform_driver_unregister(&wl->pinstance->pdriver);
+	kfree(wl->pinstance);
+out_free:
 	wl1271_free_hw(wl);
-
 	return ret;
 }

@@ -253,6 +338,10 @@ static void __devexit wl1271_remove(struct sdio_func
 *func)
 	free_irq(wl->irq, wl);
 
 	wl1271_unregister_hw(wl);
+
+	platform_driver_unregister(&wl->pinstance->pdriver);
+	kfree(wl->pinstance);
+
 	wl1271_free_hw(wl);
 }
 
--

1.7.0.4

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

drivers/net/wireless/wl12xx/wl1271_sdio.c static struct wl1271_if_operations sdio_ops = { · line 194

Quoted 10 lines

drivers/net/wireless/wl12xx/wl1271_sdio.c +6 -0

@@ -182,10 +186,84 @@ static struct wl1271_if_operations sdio_ops = {
 	.disable_irq	= wl1271_sdio_disable_interrupts
 };
 
+static int wl1271_plat_probe(struct platform_device *pdev)
+{
+	struct wl12xx_platform_data *pdata;
+	struct platform_driver *pdriver;
+	struct wl12xx_plat_instance *pinstance;
+

What about checking for pdev here before extracting pdata?

drivers/net/wireless/wl12xx/wl1271_sdio.c static struct wl1271_if_operations sdio_ops = { · line 202

Quoted 8 lines

drivers/net/wireless/wl12xx/wl1271_sdio.c +8 -0

+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		wl1271_error("no platform data");
+		return -ENODEV;
+	}
+
+	pdriver = container_of(pdev->dev.driver, struct platform_driver,
+								driver);

-----Original Message-----
From: Felipe Balbi [mailto:felipe.balbi at nokia.com]
Sent: Thursday, August 12, 2010 12:18 AM
To: DebBarma, Tarun Kanti
Cc: Ohad Ben-Cohen; linux-wireless at vger.kernel.org; linux-
mmc at vger.kernel.org; linux-omap at vger.kernel.org; Ido Yariv; Mark Brown; linux-arm-kernel at lists.infradead.org; Chikkature Rajashekar, Madhusudhan; Coelho Luciano (Nokia-MS/Helsinki); akpm at linux-foundation.org; San Mehat; Quadros Roger (Nokia-MS/Helsinki); Tony Lindgren; Nicolas Pitre; Pandita, Vikram; Kalle Valo
Subject: Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data

hi,

Quoted 11 lines · Tarun Kanti

drivers/net/wireless/wl12xx/wl1271_sdio.c static struct wl1271_if_operations sdio_ops = { · line 194

Quoted 10 lines

drivers/net/wireless/wl12xx/wl1271_sdio.c +6 -0

@@ -182,10 +186,84 @@ static struct wl1271_if_operations sdio_ops = {
 	.disable_irq	= wl1271_sdio_disable_interrupts
 };
 
+static int wl1271_plat_probe(struct platform_device *pdev)
+{
+	struct wl12xx_platform_data *pdata;
+	struct platform_driver *pdriver;
+	struct wl12xx_plat_instance *pinstance;
+

What about checking for pdev here before extracting pdata?

why ? if probe is called pdev is guaranteed to be valid, no ?

Quoted 8 lines

drivers/net/wireless/wl12xx/wl1271_sdio.c static struct wl1271_if_operations sdio_ops = { · line 202

Quoted 8 lines

drivers/net/wireless/wl12xx/wl1271_sdio.c +8 -0

+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		wl1271_error("no platform data");
+		return -ENODEV;
+	}
+
+	pdriver = container_of(pdev->dev.driver, struct platform_driver,
+								driver);

but you shouldn't fiddle with the driver structure here. What are you actually trying to achieve here ? What's pinstance supposed to do ? are you trying to handle cases where you might have several wl12xx chips on the same board ? If that's the case you should have several platform devices, one for each chip. They'll only have different ids.

--
balbi

DefectiveByDesign.org

True; however if we go by that argument than we can also assume pdata is valid, so that we would not need the below check.

Still, I would go ahead and find out if there is any scenario where pdev can go wrong during device registration. Thanks.

-----Original Message-----
From: Felipe Balbi [mailto:felipe.balbi at nokia.com]
Sent: Thursday, August 12, 2010 12:27 AM
To: DebBarma, Tarun Kanti
Cc: Balbi Felipe (Nokia-MS/Helsinki); Ohad Ben-Cohen; linux- wireless at vger.kernel.org; linux-mmc at vger.kernel.org; linux- omap at vger.kernel.org; Ido Yariv; Mark Brown; linux-arm-
kernel at lists.infradead.org; Chikkature Rajashekar, Madhusudhan; Coelho Luciano (Nokia-MS/Helsinki); akpm at linux-foundation.org; San Mehat; Quadros Roger (Nokia-MS/Helsinki); Tony Lindgren; Nicolas Pitre; Pandita, Vikram; Kalle Valo
Subject: Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data

Hi,

Quoted 2 lines · Tarun Kanti

True; however if we go by that argument than we can also assume pdata is valid, so that we would not need the below check.

of course not. You can have devices that just play well with default values or devices where you don't need the flexibility of platform data. That's why we check.

platform_device pointers on the other hand, are guaranteed to be always true, if it isn't then you should oops, you deserve to oops because something is really really wrong.

Quoted 2 lines

Still, I would go ahead and find out if there is any scenario where pdev can go wrong during device registration. Thanks.

if that scenario ever happens, it's either a bug on your implementation or driver-core. In both cases you deserve to oops so we catch such problems at early stages.

--
balbi

DefectiveByDesign.org

Quoted 21 lines

-----Original Message-----
From: Felipe Balbi [mailto:felipe.balbi at nokia.com]
Sent: Thursday, August 12, 2010 12:27 AM
To: DebBarma, Tarun Kanti
Cc: Balbi Felipe (Nokia-MS/Helsinki); Ohad Ben-Cohen; linux- wireless at vger.kernel.org; linux-mmc at vger.kernel.org; linux- omap at vger.kernel.org; Ido Yariv; Mark Brown; linux-arm-
kernel at lists.infradead.org; Chikkature Rajashekar, Madhusudhan; Coelho Luciano (Nokia-MS/Helsinki); akpm at linux-foundation.org; San Mehat; Quadros Roger (Nokia-MS/Helsinki); Tony Lindgren; Nicolas Pitre; Pandita, Vikram; Kalle Valo
Subject: Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data

Hi,

Quoted 2 lines · Tarun Kanti

True; however if we go by that argument than we can also assume pdata is valid, so that we would not need the below check.

Quoted 8 lines

of course not. You can have devices that just play well with default values or devices where you don't need the flexibility of platform data. That's why we check.

platform_device pointers on the other hand, are guaranteed to be always true, if it isn't then you should oops, you deserve to oops because something is really really wrong.

Sounds perfect!
What that means is _probe() function makes sense only for cases where we have valid platform data because we are returning right at the top if pdata is not valid. If this is the case I was curious to know why not framework make another check for valid pdata before calling _probe() instead of coming all the way to _probe() and then returning!

Quoted 29 lines · Tarun Kanti

Quoted 21 lines

-----Original Message-----
From: Felipe Balbi [mailto:felipe.balbi at nokia.com]
Sent: Thursday, August 12, 2010 12:27 AM
To: DebBarma, Tarun Kanti
Cc: Balbi Felipe (Nokia-MS/Helsinki); Ohad Ben-Cohen; linux- wireless at vger.kernel.org; linux-mmc at vger.kernel.org; linux- omap at vger.kernel.org; Ido Yariv; Mark Brown; linux-arm-
kernel at lists.infradead.org; Chikkature Rajashekar, Madhusudhan; Coelho Luciano (Nokia-MS/Helsinki); akpm at linux-foundation.org; San Mehat; Quadros Roger (Nokia-MS/Helsinki); Tony Lindgren; Nicolas Pitre; Pandita, Vikram; Kalle Valo
Subject: Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data

Hi,

Quoted 2 lines · Tarun Kanti

True; however if we go by that argument than we can also assume pdata is valid, so that we would not need the below check.

Quoted 8 lines

of course not. You can have devices that just play well with default values or devices where you don't need the flexibility of platform data. That's why we check.

platform_device pointers on the other hand, are guaranteed to be always true, if it isn't then you should oops, you deserve to oops because something is really really wrong.

Quoted 6 lines

Sounds perfect!
What that means is _probe() function makes sense only for cases where we have valid platform data because we are returning right at the top if pdata is not valid. If this is the case I was curious to know why not framework make another check for valid pdata before calling _probe() instead of coming all the way to _probe() and then returning!

Platform devices are not for passing platform data around - they're for declaring platform hardware devices that we want drivers to handle - and it depends on the driver whether having platform data is appropriate or not.

This proposal is, IMHO, abusing the platform device/driver support to achieve its own goals. ?I've outlined a far simpler and easiler solution which avoids this kind of abuse, and given suggestions on how to extend it to support multiple instances.

I've outlined a far simpler and easiler solution
which avoids this kind of abuse, and given suggestions on how to extend it to support multiple instances.

Quoted 4 lines · Russell King - ARM Linux

I've outlined a far simpler and easiler solution
which avoids this kind of abuse, and given suggestions on how to extend it to support multiple instances.

Do you mean:
http://www.spinics.net/lists/arm-kernel/msg95338.html
?

I was under the impression you eventually discarded that approach in that post.

Quoted 7 lines · Ohad Ben-Cohen

Quoted 4 lines · Russell King - ARM Linux

I've outlined a far simpler and easiler solution
which avoids this kind of abuse, and given suggestions on how to extend it to support multiple instances.

Quoted 6 lines

Do you mean:
http://www.spinics.net/lists/arm-kernel/msg95338.html
?

I was under the impression you eventually discarded that approach in that post.

Indeed.

I'm talking about my first suggestion:

http://lists.arm.linux.org.uk/lurker/message/20100804.114148.bf961e83.en.html

which was later expanded by Vitaly:

http://lists.arm.linux.org.uk/lurker/message/20100804.140119.f476c29c.en.html

I don't think anyone had any negative comments against that proposal.

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Thursday, August 12, 2010 2:55 AM
To: DebBarma, Tarun Kanti
Cc: felipe.balbi at nokia.com; Ohad Ben-Cohen; Kalle Valo; Pandita, Vikram; akpm at linux-foundation.org; Quadros Roger (Nokia-MS/Helsinki); Tony Lindgren; linux-wireless at vger.kernel.org; Mark Brown; linux- mmc at vger.kernel.org; Nicolas Pitre; San Mehat; Chikkature Rajashekar, Madhusudhan; Coelho Luciano (Nokia-MS/Helsinki); linux-
omap at vger.kernel.org; Ido Yariv; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data

Quoted 10 lines · Tarun Kanti

Quoted 10 lines

-----Original Message-----
From: Felipe Balbi [mailto:felipe.balbi at nokia.com]
Sent: Thursday, August 12, 2010 12:27 AM
To: DebBarma, Tarun Kanti
Cc: Balbi Felipe (Nokia-MS/Helsinki); Ohad Ben-Cohen; linux- wireless at vger.kernel.org; linux-mmc at vger.kernel.org; linux- omap at vger.kernel.org; Ido Yariv; Mark Brown; linux-arm- kernel at lists.infradead.org; Chikkature Rajashekar, Madhusudhan; Coelho Luciano (Nokia-MS/Helsinki); akpm at linux-foundation.org; San Mehat;

Quadros

Quoted 1 line

Quoted 1 line

Roger (Nokia-MS/Helsinki); Tony Lindgren; Nicolas Pitre; Pandita,

Vikram;

Quoted 2 lines

Quoted 2 lines

Kalle Valo
Subject: Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to

get

Quoted 5 lines

Quoted 5 lines

board data

Hi,

On Wed, Aug 11, 2010 at 08:52:54PM +0200, ext DebBarma, Tarun Kanti

Quoted 5 lines · wrote:

Quoted 5 lines

Quoted 2 lines

True; however if we go by that argument than we can also assume pdata is valid, so that we would not need the below check.

of course not. You can have devices that just play well with default values or devices where you don't need the flexibility of platform

data.

Quoted 3 lines

Quoted 3 lines

That's why we check.

platform_device pointers on the other hand, are guaranteed to be

always

Quoted 9 lines

Quoted 3 lines

true, if it isn't then you should oops, you deserve to oops because something is really really wrong.

Sounds perfect!
What that means is _probe() function makes sense only for cases where we have valid platform data because we are returning right at the top if pdata is not valid. If this is the case I was curious to know why not framework make another check for valid pdata before calling _probe() instead of coming all the way to _probe() and then returning!

Platform devices are not for passing platform data around - they're for declaring platform hardware devices that we want drivers to handle - and it depends on the driver whether having platform data is appropriate or not.

This proposal is, IMHO, abusing the platform device/driver support to achieve its own goals. I've outlined a far simpler and easiler solution which avoids this kind of abuse, and given suggestions on how to extend it to support multiple instances.

Sounds perfect!
What that means is _probe() function makes sense only for cases where we have valid platform data because we are returning right at the top if pdata is not valid. If this is the case I was curious to know why not framework make another check for valid pdata before calling _probe() instead of coming all the way to _probe() and then returning!

True; however if we go by that argument than we can also assume pdata is valid, so that we would not need the below check.

Quoted 8 lines

Quoted 8 lines

+ ? ? ? pdata = pdev->dev.platform_data;
+ ? ? ? if (!pdata) {
+ ? ? ? ? ? ? ? wl1271_error("no platform data");
+ ? ? ? ? ? ? ? return -ENODEV;
+ ? ? ? }
+
+ ? ? ? pdriver = container_of(pdev->dev.driver, struct platform_driver,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? driver);

but you shouldn't fiddle with the driver structure here. What are you actually trying to achieve here ? What's pinstance supposed to do ? are you trying to handle cases where you might have several wl12xx chips on the same board ?

If that's the case you should have several platform devices, one for each chip. They'll only have different ids.

--
balbi

DefectiveByDesign.org

Think of several wl1271 devices, each of which is represented by two devices; an SDIO function, and a platform device. The SDIO function stands for a specific MMC controller the device is hardwired to, and the platform device stands for the external irq line that the device is hardwired to.

Quoted 5 lines · Ohad Ben-Cohen

Think of several wl1271 devices, each of which is represented by two devices; an SDIO function, and a platform device. The SDIO function stands for a specific MMC controller the device is hardwired to, and the platform device stands for the external irq line that the device is hardwired to.

my view on this is that you're hunting the false problem here. I wonder how likely it is that two hardwired WL12xx chips are there on the same board.

It's theoretically possible that there's one hardwired device and one hot-pluggable one but the latter one can't have platform_data anyway.

So in my opinion, we either should keep things simple and consider 2 hardwired WL12XX's to be a very very unlikely case, or we'll end up discussing board_info once again.

Thanks,
? Vitaly

Think of several wl1271 devices, each of which is represented by two devices; an SDIO function, and a platform device. The SDIO function stands for a specific MMC controller the device is hardwired to, and the platform device stands for the external irq line that the device is hardwired to.

We must couple the SDIO function with the correct platform device, otherwise it will start getting the wrong interrupts. So after the SDIO function is probed, it registers a platform driver with the unique name that represents the platform device that it is coupled with. When the platform device is probed, it needs to deliver its platform data info to the specific SDIO function that it is bound with.

To avoid using some global data structure in that driver, we allocate a unique platform driver per each device, which makes it possible for the platform driver probe to find the SDIO function context by means of container_of.

Alternatively, we can also use some global structure in that file, most probably idr, which would also make it possible to reach the SDIO function contexts without going through the driver structure. The idr indexes would then be the MMC controller index, which should match the platform device id.

I'll try this out, it might actually look nicer.