linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/8] native support for wl1271 on ZOOM
@ 2010-08-11 18:21 Ohad Ben-Cohen
  2010-08-11 18:21 ` [PATCH v4 1/8] wireless: wl1271: make wl12xx.h common to both spi and sdio Ohad Ben-Cohen
                   ` (7 more replies)
  0 siblings, 8 replies; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-11 18:21 UTC (permalink / raw)
  To: linux-wireless, linux-mmc, linux-omap
  Cc: Ido Yariv, Mark Brown, linux-arm-kernel,
	Chikkature Rajashekar Madhusudhan, Luciano Coelho, akpm,
	San Mehat, Roger Quadros, Tony Lindgren, Nicolas Pitre,
	Pandita Vikram, Kalle Valo, Ohad Ben-Cohen

This patch series adds native support for wl1271 on ZOOM.

Changes since v3:
- slot is marked nonremovable
- no explicit slot ocr mask
- dont split hsmmc mmc23 power control

Patches are based on linux-next (as of Aug 7th), and tested
on ZOOM2.

Please note that I am going to have a very limited email access
in the next three weeks, so I might not respond right away.

Thanks,

Ohad Ben-Cohen (8):
  wireless: wl1271: make wl12xx.h common to both spi and sdio
  wireless: wl1271: support return value for the set power func
  wireless: wl1271: add platform driver to get board data
  wireless: wl1271: take irq info from private board data
  wireless: wl1271: make ref_clock configurable by board
  omap: hsmmc: remove unused variable
  omap: zoom: add fixed regulator device for wlan
  omap: zoom: add mmc3/wl1271 device support

 arch/arm/mach-omap2/board-zoom-peripherals.c |   69 +++++++++++++++
 drivers/mmc/host/omap_hsmmc.c                |    1 -
 drivers/net/wireless/wl12xx/wl1251_sdio.c    |    2 +-
 drivers/net/wireless/wl12xx/wl1251_spi.c     |    2 +-
 drivers/net/wireless/wl12xx/wl1271.h         |   15 +++-
 drivers/net/wireless/wl12xx/wl1271_boot.c    |    9 +-
 drivers/net/wireless/wl12xx/wl1271_boot.h    |    1 -
 drivers/net/wireless/wl12xx/wl1271_io.h      |    8 +-
 drivers/net/wireless/wl12xx/wl1271_main.c    |    4 +-
 drivers/net/wireless/wl12xx/wl1271_sdio.c    |  120 +++++++++++++++++++++++---
 drivers/net/wireless/wl12xx/wl1271_spi.c     |    8 ++-
 include/linux/spi/wl12xx.h                   |   34 -------
 include/linux/wl12xx.h                       |   35 ++++++++
 13 files changed, 245 insertions(+), 63 deletions(-)
 delete mode 100644 include/linux/spi/wl12xx.h
 create mode 100644 include/linux/wl12xx.h


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v4 1/8] wireless: wl1271: make wl12xx.h common to both spi and sdio
  2010-08-11 18:21 [PATCH v4 0/8] native support for wl1271 on ZOOM Ohad Ben-Cohen
@ 2010-08-11 18:21 ` Ohad Ben-Cohen
  2010-08-11 18:21 ` [PATCH v4 2/8] wireless: wl1271: support return value for the set power func Ohad Ben-Cohen
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-11 18:21 UTC (permalink / raw)
  To: linux-wireless, linux-mmc, linux-omap
  Cc: Ido Yariv, Mark Brown, linux-arm-kernel,
	Chikkature Rajashekar Madhusudhan, Luciano Coelho, akpm,
	San Mehat, Roger Quadros, Tony Lindgren, Nicolas Pitre,
	Pandita Vikram, Kalle Valo, Ohad Ben-Cohen

Move wl12xx.h outside of the spi-specific location,
so it can be shared with both spi and sdio solutions.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/net/wireless/wl12xx/wl1251_sdio.c |    2 +-
 drivers/net/wireless/wl12xx/wl1251_spi.c  |    2 +-
 drivers/net/wireless/wl12xx/wl1271_spi.c  |    2 +-
 include/linux/spi/wl12xx.h                |   34 -----------------------------
 include/linux/wl12xx.h                    |   34 +++++++++++++++++++++++++++++
 5 files changed, 37 insertions(+), 37 deletions(-)
 delete mode 100644 include/linux/spi/wl12xx.h
 create mode 100644 include/linux/wl12xx.h

diff --git a/drivers/net/wireless/wl12xx/wl1251_sdio.c b/drivers/net/wireless/wl12xx/wl1251_sdio.c
index b901b61..a319df1 100644
--- a/drivers/net/wireless/wl12xx/wl1251_sdio.c
+++ b/drivers/net/wireless/wl12xx/wl1251_sdio.c
@@ -24,7 +24,7 @@
 #include <linux/mmc/sdio_func.h>
 #include <linux/mmc/sdio_ids.h>
 #include <linux/platform_device.h>
-#include <linux/spi/wl12xx.h>
+#include <linux/wl12xx.h>
 #include <linux/irq.h>
 
 #include "wl1251.h"
diff --git a/drivers/net/wireless/wl12xx/wl1251_spi.c b/drivers/net/wireless/wl12xx/wl1251_spi.c
index 27fdfaa..a6faf3e 100644
--- a/drivers/net/wireless/wl12xx/wl1251_spi.c
+++ b/drivers/net/wireless/wl12xx/wl1251_spi.c
@@ -26,7 +26,7 @@
 #include <linux/slab.h>
 #include <linux/crc7.h>
 #include <linux/spi/spi.h>
-#include <linux/spi/wl12xx.h>
+#include <linux/wl12xx.h>
 
 #include "wl1251.h"
 #include "wl1251_reg.h"
diff --git a/drivers/net/wireless/wl12xx/wl1271_spi.c b/drivers/net/wireless/wl12xx/wl1271_spi.c
index 4cb99c5..c3fdab7 100644
--- a/drivers/net/wireless/wl12xx/wl1271_spi.c
+++ b/drivers/net/wireless/wl12xx/wl1271_spi.c
@@ -25,7 +25,7 @@
 #include <linux/module.h>
 #include <linux/crc7.h>
 #include <linux/spi/spi.h>
-#include <linux/spi/wl12xx.h>
+#include <linux/wl12xx.h>
 #include <linux/slab.h>
 
 #include "wl1271.h"
diff --git a/include/linux/spi/wl12xx.h b/include/linux/spi/wl12xx.h
deleted file mode 100644
index a223ecb..0000000
--- a/include/linux/spi/wl12xx.h
+++ /dev/null
@@ -1,34 +0,0 @@
-/*
- * This file is part of wl12xx
- *
- * Copyright (C) 2009 Nokia Corporation
- *
- * Contact: Kalle Valo <kalle.valo@nokia.com>
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
- * 02110-1301 USA
- *
- */
-
-#ifndef _LINUX_SPI_WL12XX_H
-#define _LINUX_SPI_WL12XX_H
-
-struct wl12xx_platform_data {
-	void (*set_power)(bool enable);
-	/* SDIO only: IRQ number if WLAN_IRQ line is used, 0 for SDIO IRQs */
-	int irq;
-	bool use_eeprom;
-};
-
-#endif
diff --git a/include/linux/wl12xx.h b/include/linux/wl12xx.h
new file mode 100644
index 0000000..137ac89
--- /dev/null
+++ b/include/linux/wl12xx.h
@@ -0,0 +1,34 @@
+/*
+ * This file is part of wl12xx
+ *
+ * Copyright (C) 2009 Nokia Corporation
+ *
+ * Contact: Kalle Valo <kalle.valo@nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ *
+ */
+
+#ifndef _LINUX_WL12XX_H
+#define _LINUX_WL12XX_H
+
+struct wl12xx_platform_data {
+	void (*set_power)(bool enable);
+	/* SDIO only: IRQ number if WLAN_IRQ line is used, 0 for SDIO IRQs */
+	int irq;
+	bool use_eeprom;
+};
+
+#endif
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 2/8] wireless: wl1271: support return value for the set power func
  2010-08-11 18:21 [PATCH v4 0/8] native support for wl1271 on ZOOM Ohad Ben-Cohen
  2010-08-11 18:21 ` [PATCH v4 1/8] wireless: wl1271: make wl12xx.h common to both spi and sdio Ohad Ben-Cohen
@ 2010-08-11 18:21 ` Ohad Ben-Cohen
  2010-08-11 18:35   ` DebBarma, Tarun Kanti
  2010-08-11 18:21 ` [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data Ohad Ben-Cohen
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-11 18:21 UTC (permalink / raw)
  To: linux-wireless, linux-mmc, linux-omap
  Cc: Ido Yariv, Mark Brown, linux-arm-kernel,
	Chikkature Rajashekar Madhusudhan, Luciano Coelho, akpm,
	San Mehat, Roger Quadros, Tony Lindgren, Nicolas Pitre,
	Pandita Vikram, Kalle Valo, Ohad Ben-Cohen

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 <ohad@wizery.com>
---
 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(-)

diff --git a/drivers/net/wireless/wl12xx/wl1271.h b/drivers/net/wireless/wl12xx/wl1271.h
index dd3cee6..faa5925 100644
--- a/drivers/net/wireless/wl12xx/wl1271.h
+++ b/drivers/net/wireless/wl12xx/wl1271.h
@@ -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);
diff --git a/drivers/net/wireless/wl12xx/wl1271_io.h b/drivers/net/wireless/wl12xx/wl1271_io.h
index bc806c7..4a5b92c 100644
--- a/drivers/net/wireless/wl12xx/wl1271_io.h
+++ b/drivers/net/wireless/wl12xx/wl1271_io.h
@@ -144,10 +144,12 @@ static inline void wl1271_power_off(struct wl1271 *wl)
 	clear_bit(WL1271_FLAG_GPIO_POWER, &wl->flags);
 }
 
-static inline void wl1271_power_on(struct wl1271 *wl)
+static inline int wl1271_power_on(struct wl1271 *wl)
 {
-	wl->if_ops->power(wl, true);
-	set_bit(WL1271_FLAG_GPIO_POWER, &wl->flags);
+	int ret = wl->if_ops->power(wl, true);
+	if (ret == 0)
+		set_bit(WL1271_FLAG_GPIO_POWER, &wl->flags);
+	return ret;
 }
 
 
diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
index 9d68f00..e6e0852 100644
--- a/drivers/net/wireless/wl12xx/wl1271_main.c
+++ b/drivers/net/wireless/wl12xx/wl1271_main.c
@@ -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);
diff --git a/drivers/net/wireless/wl12xx/wl1271_sdio.c b/drivers/net/wireless/wl12xx/wl1271_sdio.c
index 7059b5c..c41293a 100644
--- a/drivers/net/wireless/wl12xx/wl1271_sdio.c
+++ b/drivers/net/wireless/wl12xx/wl1271_sdio.c
@@ -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 = {
diff --git a/drivers/net/wireless/wl12xx/wl1271_spi.c b/drivers/net/wireless/wl12xx/wl1271_spi.c
index c3fdab7..de56d8d 100644
--- a/drivers/net/wireless/wl12xx/wl1271_spi.c
+++ b/drivers/net/wireless/wl12xx/wl1271_spi.c
@@ -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


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 18:21 [PATCH v4 0/8] native support for wl1271 on ZOOM Ohad Ben-Cohen
  2010-08-11 18:21 ` [PATCH v4 1/8] wireless: wl1271: make wl12xx.h common to both spi and sdio Ohad Ben-Cohen
  2010-08-11 18:21 ` [PATCH v4 2/8] wireless: wl1271: support return value for the set power func Ohad Ben-Cohen
@ 2010-08-11 18:21 ` Ohad Ben-Cohen
  2010-08-11 18:42   ` DebBarma, Tarun Kanti
  2010-08-11 18:21 ` [PATCH v4 4/8] wireless: wl1271: take irq info from private " Ohad Ben-Cohen
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-11 18:21 UTC (permalink / raw)
  To: linux-wireless, linux-mmc, linux-omap
  Cc: Ido Yariv, Mark Brown, linux-arm-kernel,
	Chikkature Rajashekar Madhusudhan, Luciano Coelho, akpm,
	San Mehat, Roger Quadros, Tony Lindgren, Nicolas Pitre,
	Pandita Vikram, Kalle Valo, Ohad Ben-Cohen

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 <ohad@wizery.com>
---
 drivers/net/wireless/wl12xx/wl1271.h      |   12 ++++
 drivers/net/wireless/wl12xx/wl1271_sdio.c |  103 +++++++++++++++++++++++++++--
 2 files changed, 108 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/wl1271.h b/drivers/net/wireless/wl12xx/wl1271.h
index faa5925..013eabb 100644
--- a/drivers/net/wireless/wl12xx/wl1271.h
+++ b/drivers/net/wireless/wl12xx/wl1271.h
@@ -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;
 
diff --git a/drivers/net/wireless/wl12xx/wl1271_sdio.c b/drivers/net/wireless/wl12xx/wl1271_sdio.c
index c41293a..68b512b 100644
--- a/drivers/net/wireless/wl12xx/wl1271_sdio.c
+++ b/drivers/net/wireless/wl12xx/wl1271_sdio.c
@@ -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"
@@ -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;
+
+	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;
 	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;
 	}
 
 	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


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 4/8] wireless: wl1271: take irq info from private board data
  2010-08-11 18:21 [PATCH v4 0/8] native support for wl1271 on ZOOM Ohad Ben-Cohen
                   ` (2 preceding siblings ...)
  2010-08-11 18:21 ` [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data Ohad Ben-Cohen
@ 2010-08-11 18:21 ` Ohad Ben-Cohen
  2010-08-11 18:21 ` [PATCH v4 5/8] wireless: wl1271: make ref_clock configurable by board Ohad Ben-Cohen
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-11 18:21 UTC (permalink / raw)
  To: linux-wireless, linux-mmc, linux-omap
  Cc: Ido Yariv, Mark Brown, linux-arm-kernel,
	Chikkature Rajashekar Madhusudhan, Luciano Coelho, akpm,
	San Mehat, Roger Quadros, Tony Lindgren, Nicolas Pitre,
	Pandita Vikram, Kalle Valo, Ohad Ben-Cohen

Remove the hard coded irq information, and instead take
the irq information from the board's platform data.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/net/wireless/wl12xx/wl1271_sdio.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/wl1271_sdio.c b/drivers/net/wireless/wl12xx/wl1271_sdio.c
index 68b512b..228f5af 100644
--- a/drivers/net/wireless/wl12xx/wl1271_sdio.c
+++ b/drivers/net/wireless/wl12xx/wl1271_sdio.c
@@ -38,9 +38,6 @@
 #include "wl12xx_80211.h"
 #include "wl1271_io.h"
 
-
-#define RX71_WL1271_IRQ_GPIO		42
-
 #ifndef SDIO_VENDOR_ID_TI
 #define SDIO_VENDOR_ID_TI		0x0097
 #endif
@@ -191,6 +188,7 @@ static int wl1271_plat_probe(struct platform_device *pdev)
 	struct wl12xx_platform_data *pdata;
 	struct platform_driver *pdriver;
 	struct wl12xx_plat_instance *pinstance;
+	int irq;
 
 	pdata = pdev->dev.platform_data;
 	if (!pdata) {
@@ -198,6 +196,13 @@ static int wl1271_plat_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		wl1271_error("no platform irq data");
+		return -ENODEV;
+	}
+	pdata->irq = irq;
+
 	pdriver = container_of(pdev->dev.driver, struct platform_driver,
 								driver);
 	pinstance = container_of(pdriver, struct wl12xx_plat_instance, pdriver);
@@ -290,12 +295,7 @@ static int __devinit wl1271_probe(struct sdio_func *func,
 		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 put_data;
-	}
+	wl->irq = wlan_data->irq;
 
 	ret = request_irq(wl->irq, wl1271_irq, 0, DRIVER_NAME, wl);
 	if (ret < 0) {
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 5/8] wireless: wl1271: make ref_clock configurable by board
  2010-08-11 18:21 [PATCH v4 0/8] native support for wl1271 on ZOOM Ohad Ben-Cohen
                   ` (3 preceding siblings ...)
  2010-08-11 18:21 ` [PATCH v4 4/8] wireless: wl1271: take irq info from private " Ohad Ben-Cohen
@ 2010-08-11 18:21 ` Ohad Ben-Cohen
  2010-08-11 18:21 ` [PATCH v4 6/8] omap: hsmmc: remove unused variable Ohad Ben-Cohen
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-11 18:21 UTC (permalink / raw)
  To: linux-wireless, linux-mmc, linux-omap
  Cc: Ido Yariv, Mark Brown, linux-arm-kernel,
	Chikkature Rajashekar Madhusudhan, Luciano Coelho, akpm,
	San Mehat, Roger Quadros, Tony Lindgren, Nicolas Pitre,
	Pandita Vikram, Kalle Valo, Ohad Ben-Cohen

The wl1271 device is using a reference clock that may change
between board to board.

Make the ref_clock parameter configurable by the board
files that set up the device, instead of having a hard coded
value in the driver source itself.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/net/wireless/wl12xx/wl1271.h      |    1 +
 drivers/net/wireless/wl12xx/wl1271_boot.c |    9 +++++----
 drivers/net/wireless/wl12xx/wl1271_boot.h |    1 -
 drivers/net/wireless/wl12xx/wl1271_sdio.c |    1 +
 drivers/net/wireless/wl12xx/wl1271_spi.c  |    2 ++
 include/linux/wl12xx.h                    |    1 +
 6 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/wl1271.h b/drivers/net/wireless/wl12xx/wl1271.h
index 013eabb..f0988a4 100644
--- a/drivers/net/wireless/wl12xx/wl1271.h
+++ b/drivers/net/wireless/wl12xx/wl1271.h
@@ -342,6 +342,7 @@ struct wl1271 {
 
 	void (*set_power)(bool enable);
 	int irq;
+	int ref_clock;
 
 	spinlock_t wl_lock;
 
diff --git a/drivers/net/wireless/wl12xx/wl1271_boot.c b/drivers/net/wireless/wl12xx/wl1271_boot.c
index f36430b..95c636a 100644
--- a/drivers/net/wireless/wl12xx/wl1271_boot.c
+++ b/drivers/net/wireless/wl12xx/wl1271_boot.c
@@ -457,17 +457,18 @@ int wl1271_boot(struct wl1271 *wl)
 {
 	int ret = 0;
 	u32 tmp, clk, pause;
+	int ref_clock = wl->ref_clock;
 
 	wl1271_boot_hw_version(wl);
 
-	if (REF_CLOCK == 0 || REF_CLOCK == 2 || REF_CLOCK == 4)
+	if (ref_clock == 0 || ref_clock == 2 || ref_clock == 4)
 		/* ref clk: 19.2/38.4/38.4-XTAL */
 		clk = 0x3;
-	else if (REF_CLOCK == 1 || REF_CLOCK == 3)
+	else if (ref_clock == 1 || ref_clock == 3)
 		/* ref clk: 26/52 */
 		clk = 0x5;
 
-	if (REF_CLOCK != 0) {
+	if (ref_clock != 0) {
 		u16 val;
 		/* Set clock type (open drain) */
 		val = wl1271_top_reg_read(wl, OCP_REG_CLK_TYPE);
@@ -516,7 +517,7 @@ int wl1271_boot(struct wl1271 *wl)
 	wl1271_debug(DEBUG_BOOT, "clk2 0x%x", clk);
 
 	/* 2 */
-	clk |= (REF_CLOCK << 1) << 4;
+	clk |= (ref_clock << 1) << 4;
 	wl1271_write32(wl, DRPW_SCRATCH_START, clk);
 
 	wl1271_set_partition(wl, &part_table[PART_WORK]);
diff --git a/drivers/net/wireless/wl12xx/wl1271_boot.h b/drivers/net/wireless/wl12xx/wl1271_boot.h
index f829699..f73b0b1 100644
--- a/drivers/net/wireless/wl12xx/wl1271_boot.h
+++ b/drivers/net/wireless/wl12xx/wl1271_boot.h
@@ -46,7 +46,6 @@ struct wl1271_static_data {
 /* delay between retries */
 #define INIT_LOOP_DELAY 50
 
-#define REF_CLOCK            2
 #define WU_COUNTER_PAUSE_VAL 0x3FF
 #define WELP_ARM_COMMAND_VAL 0x4
 
diff --git a/drivers/net/wireless/wl12xx/wl1271_sdio.c b/drivers/net/wireless/wl12xx/wl1271_sdio.c
index 228f5af..2f558a6 100644
--- a/drivers/net/wireless/wl12xx/wl1271_sdio.c
+++ b/drivers/net/wireless/wl12xx/wl1271_sdio.c
@@ -296,6 +296,7 @@ static int __devinit wl1271_probe(struct sdio_func *func,
 	}
 
 	wl->irq = wlan_data->irq;
+	wl->ref_clock = wlan_data->board_ref_clock;
 
 	ret = request_irq(wl->irq, wl1271_irq, 0, DRIVER_NAME, wl);
 	if (ret < 0) {
diff --git a/drivers/net/wireless/wl12xx/wl1271_spi.c b/drivers/net/wireless/wl12xx/wl1271_spi.c
index de56d8d..ced0a9e 100644
--- a/drivers/net/wireless/wl12xx/wl1271_spi.c
+++ b/drivers/net/wireless/wl12xx/wl1271_spi.c
@@ -372,6 +372,8 @@ static int __devinit wl1271_probe(struct spi_device *spi)
 		goto out_free;
 	}
 
+	wl->ref_clock = pdata->board_ref_clock;
+
 	wl->irq = spi->irq;
 	if (wl->irq < 0) {
 		wl1271_error("irq missing in platform data");
diff --git a/include/linux/wl12xx.h b/include/linux/wl12xx.h
index 137ac89..ef6eed9 100644
--- a/include/linux/wl12xx.h
+++ b/include/linux/wl12xx.h
@@ -29,6 +29,7 @@ struct wl12xx_platform_data {
 	/* SDIO only: IRQ number if WLAN_IRQ line is used, 0 for SDIO IRQs */
 	int irq;
 	bool use_eeprom;
+	int board_ref_clock;
 };
 
 #endif
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 6/8] omap: hsmmc: remove unused variable
  2010-08-11 18:21 [PATCH v4 0/8] native support for wl1271 on ZOOM Ohad Ben-Cohen
                   ` (4 preceding siblings ...)
  2010-08-11 18:21 ` [PATCH v4 5/8] wireless: wl1271: make ref_clock configurable by board Ohad Ben-Cohen
@ 2010-08-11 18:21 ` Ohad Ben-Cohen
  2010-08-11 18:21 ` [PATCH v4 7/8] omap: zoom: add fixed regulator device for wlan Ohad Ben-Cohen
  2010-08-11 18:21 ` [PATCH v4 8/8] omap: zoom: add mmc3/wl1271 device support Ohad Ben-Cohen
  7 siblings, 0 replies; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-11 18:21 UTC (permalink / raw)
  To: linux-wireless, linux-mmc, linux-omap
  Cc: Ido Yariv, Mark Brown, linux-arm-kernel,
	Chikkature Rajashekar Madhusudhan, Luciano Coelho, akpm,
	San Mehat, Roger Quadros, Tony Lindgren, Nicolas Pitre,
	Pandita Vikram, Kalle Valo, Ohad Ben-Cohen

Make this go away:

drivers/mmc/host/omap_hsmmc.c: In function 'omap_hsmmc_suspend':
drivers/mmc/host/omap_hsmmc.c:2328: warning: unused variable 'state'

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/mmc/host/omap_hsmmc.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index b032828..d50e917 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -2272,7 +2272,6 @@ static int omap_hsmmc_suspend(struct device *dev)
 	int ret = 0;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_hsmmc_host *host = platform_get_drvdata(pdev);
-	pm_message_t state = PMSG_SUSPEND; /* unused by MMC core */
 
 	if (host && host->suspended)
 		return 0;
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 7/8] omap: zoom: add fixed regulator device for wlan
  2010-08-11 18:21 [PATCH v4 0/8] native support for wl1271 on ZOOM Ohad Ben-Cohen
                   ` (5 preceding siblings ...)
  2010-08-11 18:21 ` [PATCH v4 6/8] omap: hsmmc: remove unused variable Ohad Ben-Cohen
@ 2010-08-11 18:21 ` Ohad Ben-Cohen
  2010-08-11 18:21 ` [PATCH v4 8/8] omap: zoom: add mmc3/wl1271 device support Ohad Ben-Cohen
  7 siblings, 0 replies; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-11 18:21 UTC (permalink / raw)
  To: linux-wireless, linux-mmc, linux-omap
  Cc: Ido Yariv, Mark Brown, linux-arm-kernel,
	Chikkature Rajashekar Madhusudhan, Luciano Coelho, akpm,
	San Mehat, Roger Quadros, Tony Lindgren, Nicolas Pitre,
	Pandita Vikram, Kalle Valo, Ohad Ben-Cohen

Add a fixed regulator vmmc device to enable power control
of the wl1271 wlan device.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 arch/arm/mach-omap2/board-zoom-peripherals.c |   35 ++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/board-zoom-peripherals.c b/arch/arm/mach-omap2/board-zoom-peripherals.c
index 6b39849..de88635 100644
--- a/arch/arm/mach-omap2/board-zoom-peripherals.c
+++ b/arch/arm/mach-omap2/board-zoom-peripherals.c
@@ -16,6 +16,7 @@
 #include <linux/gpio.h>
 #include <linux/i2c/twl.h>
 #include <linux/regulator/machine.h>
+#include <linux/regulator/fixed.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -27,6 +28,8 @@
 #include "mux.h"
 #include "hsmmc.h"
 
+#define OMAP_ZOOM_WLAN_PMENA_GPIO	(101)
+
 /* Zoom2 has Qwerty keyboard*/
 static int board_keymap[] = {
 	KEY(0, 0, KEY_E),
@@ -106,6 +109,11 @@ static struct regulator_consumer_supply zoom_vmmc2_supply = {
 	.supply		= "vmmc",
 };
 
+static struct regulator_consumer_supply zoom_vmmc3_supply = {
+	.supply		= "vmmc",
+	.dev_name	= "mmci-omap-hs.2",
+};
+
 /* VMMC1 for OMAP VDD_MMC1 (i/o) and MMC1 card */
 static struct regulator_init_data zoom_vmmc1 = {
 	.constraints = {
@@ -151,6 +159,32 @@ static struct regulator_init_data zoom_vsim = {
 	.consumer_supplies      = &zoom_vsim_supply,
 };
 
+static struct regulator_init_data zoom_vmmc3 = {
+	.constraints = {
+		.valid_ops_mask	= REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = 1,
+	.consumer_supplies = &zoom_vmmc3_supply,
+};
+
+static struct fixed_voltage_config zoom_vwlan = {
+	.supply_name = "vwl1271",
+	.microvolts = 1800000, /* 1.8V */
+	.gpio = OMAP_ZOOM_WLAN_PMENA_GPIO,
+	.startup_delay = 70000, /* 70msec */
+	.enable_high = 1,
+	.enabled_at_boot = 0,
+	.init_data = &zoom_vmmc3,
+};
+
+static struct platform_device omap_vwlan_device = {
+	.name		= "reg-fixed-voltage",
+	.id		= 1,
+	.dev = {
+		.platform_data = &zoom_vwlan,
+	},
+};
+
 static struct omap2_hsmmc_info mmc[] __initdata = {
 	{
 		.name		= "external",
@@ -280,6 +314,7 @@ static void enable_board_wakeup_source(void)
 void __init zoom_peripherals_init(void)
 {
 	omap_i2c_init();
+	platform_device_register(&omap_vwlan_device);
 	usb_musb_init(&musb_board_data);
 	enable_board_wakeup_source();
 }
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v4 8/8] omap: zoom: add mmc3/wl1271 device support
  2010-08-11 18:21 [PATCH v4 0/8] native support for wl1271 on ZOOM Ohad Ben-Cohen
                   ` (6 preceding siblings ...)
  2010-08-11 18:21 ` [PATCH v4 7/8] omap: zoom: add fixed regulator device for wlan Ohad Ben-Cohen
@ 2010-08-11 18:21 ` Ohad Ben-Cohen
  7 siblings, 0 replies; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-11 18:21 UTC (permalink / raw)
  To: linux-wireless, linux-mmc, linux-omap
  Cc: Ido Yariv, Mark Brown, linux-arm-kernel,
	Chikkature Rajashekar Madhusudhan, Luciano Coelho, akpm,
	San Mehat, Roger Quadros, Tony Lindgren, Nicolas Pitre,
	Pandita Vikram, Kalle Valo, Ohad Ben-Cohen

Add MMC3 support on ZOOM, which has the wl1271 device hardwired to.

The wl1271 is a 4-wire, 1.8V, embedded SDIO WLAN device with an
external IRQ line, and power-controlled by a GPIO-based fixed regulator.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 arch/arm/mach-omap2/board-zoom-peripherals.c |   34 ++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/board-zoom-peripherals.c b/arch/arm/mach-omap2/board-zoom-peripherals.c
index de88635..625ae56 100644
--- a/arch/arm/mach-omap2/board-zoom-peripherals.c
+++ b/arch/arm/mach-omap2/board-zoom-peripherals.c
@@ -17,6 +17,8 @@
 #include <linux/i2c/twl.h>
 #include <linux/regulator/machine.h>
 #include <linux/regulator/fixed.h>
+#include <linux/mmc/host.h>
+#include <linux/wl12xx.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -29,6 +31,7 @@
 #include "hsmmc.h"
 
 #define OMAP_ZOOM_WLAN_PMENA_GPIO	(101)
+#define OMAP_ZOOM_WLAN_IRQ_GPIO		(162)
 
 /* Zoom2 has Qwerty keyboard*/
 static int board_keymap[] = {
@@ -185,6 +188,28 @@ static struct platform_device omap_vwlan_device = {
 	},
 };
 
+struct wl12xx_platform_data omap_zoom_wlan_data = {
+	/* ZOOM ref clock is 26 MHz */
+	.board_ref_clock = 1,
+};
+
+static struct resource omap_zoom_wl1271_resources[] = {
+	{
+		.start = OMAP_GPIO_IRQ(OMAP_ZOOM_WLAN_IRQ_GPIO),
+		.end = OMAP_GPIO_IRQ(OMAP_ZOOM_WLAN_IRQ_GPIO),
+		.flags = IORESOURCE_IRQ,
+	}
+};
+static struct platform_device omap_zoom_wl1271 = {
+	.name		= "wl1271_plat.2",
+	.id		= -1,
+	.resource	= omap_zoom_wl1271_resources,
+	.num_resources	= ARRAY_SIZE(omap_zoom_wl1271_resources),
+	.dev = {
+		.platform_data = &omap_zoom_wlan_data,
+	},
+};
+
 static struct omap2_hsmmc_info mmc[] __initdata = {
 	{
 		.name		= "external",
@@ -202,6 +227,14 @@ static struct omap2_hsmmc_info mmc[] __initdata = {
 		.nonremovable	= true,
 		.power_saving	= true,
 	},
+	{
+		.name		= "wl1271",
+		.mmc		= 3,
+		.wires		= 4,
+		.gpio_wp	= -EINVAL,
+		.gpio_cd	= -EINVAL,
+		.nonremovable	= true,
+	},
 	{}      /* Terminator */
 };
 
@@ -313,6 +346,7 @@ static void enable_board_wakeup_source(void)
 
 void __init zoom_peripherals_init(void)
 {
+	platform_device_register(&omap_zoom_wl1271);
 	omap_i2c_init();
 	platform_device_register(&omap_vwlan_device);
 	usb_musb_init(&musb_board_data);
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* RE: [PATCH v4 2/8] wireless: wl1271: support return value for the set power func
  2010-08-11 18:21 ` [PATCH v4 2/8] wireless: wl1271: support return value for the set power func Ohad Ben-Cohen
@ 2010-08-11 18:35   ` DebBarma, Tarun Kanti
  2010-08-11 22:19     ` Ohad Ben-Cohen
  0 siblings, 1 reply; 28+ messages in thread
From: DebBarma, Tarun Kanti @ 2010-08-11 18:35 UTC (permalink / raw)
  To: Ohad Ben-Cohen, linux-wireless, linux-mmc, linux-omap
  Cc: Ido Yariv, Mark Brown, linux-arm-kernel, Chikkature Rajashekar,
	Madhusudhan, Luciano Coelho, akpm, San Mehat, Roger Quadros,
	Tony Lindgren, Nicolas Pitre, Pandita, Vikram, Kalle Valo

Ohad,

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Ohad Ben-Cohen
> Sent: Wednesday, August 11, 2010 11:52 PM
> To: linux-wireless@vger.kernel.org; linux-mmc@vger.kernel.org; linux-
> omap@vger.kernel.org
> Cc: Ido Yariv; Mark Brown; linux-arm-kernel@lists.infradead.org;
> 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 <ohad@wizery.com>
> ---
>  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(-)
> 
> diff --git a/drivers/net/wireless/wl12xx/wl1271.h
> b/drivers/net/wireless/wl12xx/wl1271.h
> index dd3cee6..faa5925 100644
> --- a/drivers/net/wireless/wl12xx/wl1271.h
> +++ b/drivers/net/wireless/wl12xx/wl1271.h
> @@ -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);
> diff --git a/drivers/net/wireless/wl12xx/wl1271_io.h
> b/drivers/net/wireless/wl12xx/wl1271_io.h
> index bc806c7..4a5b92c 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_io.h
> +++ b/drivers/net/wireless/wl12xx/wl1271_io.h
> @@ -144,10 +144,12 @@ static inline void wl1271_power_off(struct wl1271
> *wl)
>  	clear_bit(WL1271_FLAG_GPIO_POWER, &wl->flags);
>  }
> 
> -static inline void wl1271_power_on(struct wl1271 *wl)
> +static inline int wl1271_power_on(struct wl1271 *wl)
>  {
> -	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?
> +	if (ret == 0)
> +		set_bit(WL1271_FLAG_GPIO_POWER, &wl->flags);
> +	return ret;
>  }
> 
> 
> diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c
> b/drivers/net/wireless/wl12xx/wl1271_main.c
> index 9d68f00..e6e0852 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_main.c
> +++ b/drivers/net/wireless/wl12xx/wl1271_main.c
> @@ -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);
> diff --git a/drivers/net/wireless/wl12xx/wl1271_sdio.c
> b/drivers/net/wireless/wl12xx/wl1271_sdio.c
> index 7059b5c..c41293a 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_sdio.c
> +++ b/drivers/net/wireless/wl12xx/wl1271_sdio.c
> @@ -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 = {
> diff --git a/drivers/net/wireless/wl12xx/wl1271_spi.c
> b/drivers/net/wireless/wl12xx/wl1271_spi.c
> index c3fdab7..de56d8d 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_spi.c
> +++ b/drivers/net/wireless/wl12xx/wl1271_spi.c
> @@ -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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 18:21 ` [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data Ohad Ben-Cohen
@ 2010-08-11 18:42   ` DebBarma, Tarun Kanti
  2010-08-11 18:47     ` Felipe Balbi
  0 siblings, 1 reply; 28+ messages in thread
From: DebBarma, Tarun Kanti @ 2010-08-11 18:42 UTC (permalink / raw)
  To: Ohad Ben-Cohen, linux-wireless, linux-mmc, linux-omap
  Cc: Ido Yariv, Mark Brown, linux-arm-kernel, Chikkature Rajashekar,
	Madhusudhan, Luciano Coelho, akpm, San Mehat, Roger Quadros,
	Tony Lindgren, Nicolas Pitre, Pandita, Vikram, Kalle Valo

Ohad,

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Ohad Ben-Cohen
> Sent: Wednesday, August 11, 2010 11:52 PM
> To: linux-wireless@vger.kernel.org; linux-mmc@vger.kernel.org; linux-
> omap@vger.kernel.org
> Cc: Ido Yariv; Mark Brown; linux-arm-kernel@lists.infradead.org;
> 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 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 <ohad@wizery.com>
> ---
>  drivers/net/wireless/wl12xx/wl1271.h      |   12 ++++
>  drivers/net/wireless/wl12xx/wl1271_sdio.c |  103
> +++++++++++++++++++++++++++--
>  2 files changed, 108 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/wl12xx/wl1271.h
> b/drivers/net/wireless/wl12xx/wl1271.h
> index faa5925..013eabb 100644
> --- a/drivers/net/wireless/wl12xx/wl1271.h
> +++ b/drivers/net/wireless/wl12xx/wl1271.h
> @@ -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;
> 
> diff --git a/drivers/net/wireless/wl12xx/wl1271_sdio.c
> b/drivers/net/wireless/wl12xx/wl1271_sdio.c
> index c41293a..68b512b 100644
> --- a/drivers/net/wireless/wl12xx/wl1271_sdio.c
> +++ b/drivers/net/wireless/wl12xx/wl1271_sdio.c
> @@ -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"
> @@ -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?

> +	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;
>  	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;
>  	}
> 
>  	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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 18:42   ` DebBarma, Tarun Kanti
@ 2010-08-11 18:47     ` Felipe Balbi
  2010-08-11 18:52       ` DebBarma, Tarun Kanti
  2010-08-11 20:10       ` Ohad Ben-Cohen
  0 siblings, 2 replies; 28+ messages in thread
From: Felipe Balbi @ 2010-08-11 18:47 UTC (permalink / raw)
  To: ext DebBarma, Tarun Kanti
  Cc: Ohad Ben-Cohen, linux-wireless, linux-mmc, linux-omap, Ido Yariv,
	Mark Brown, linux-arm-kernel, Chikkature Rajashekar, Madhusudhan,
	Coelho Luciano (Nokia-MS/Helsinki),
	akpm, San Mehat, Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, Nicolas Pitre, Pandita, Vikram, Kalle Valo

hi,

On Wed, Aug 11, 2010 at 08:42:18PM +0200, ext DebBarma, Tarun Kanti wrote:
>> @@ -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 ?

>> +	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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 18:47     ` Felipe Balbi
@ 2010-08-11 18:52       ` DebBarma, Tarun Kanti
  2010-08-11 18:57         ` Felipe Balbi
  2010-08-11 21:21         ` Russell King - ARM Linux
  2010-08-11 20:10       ` Ohad Ben-Cohen
  1 sibling, 2 replies; 28+ messages in thread
From: DebBarma, Tarun Kanti @ 2010-08-11 18:52 UTC (permalink / raw)
  To: felipe.balbi
  Cc: Ohad Ben-Cohen, linux-wireless, linux-mmc, linux-omap, Ido Yariv,
	Mark Brown, linux-arm-kernel, Chikkature Rajashekar, Madhusudhan,
	Coelho Luciano (Nokia-MS/Helsinki),
	akpm, San Mehat, Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, Nicolas Pitre, Pandita, Vikram, Kalle Valo


> -----Original Message-----
> From: Felipe Balbi [mailto:felipe.balbi@nokia.com]
> Sent: Thursday, August 12, 2010 12:18 AM
> To: DebBarma, Tarun Kanti
> Cc: Ohad Ben-Cohen; linux-wireless@vger.kernel.org; linux-
> mmc@vger.kernel.org; linux-omap@vger.kernel.org; Ido Yariv; Mark Brown;
> linux-arm-kernel@lists.infradead.org; Chikkature Rajashekar, Madhusudhan;
> Coelho Luciano (Nokia-MS/Helsinki); akpm@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,
> 
> On Wed, Aug 11, 2010 at 08:42:18PM +0200, ext DebBarma, Tarun Kanti wrote:
> >> @@ -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 ?
> 
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.

> >> +	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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 18:52       ` DebBarma, Tarun Kanti
@ 2010-08-11 18:57         ` Felipe Balbi
  2010-08-11 19:27           ` DebBarma, Tarun Kanti
  2010-08-11 21:21         ` Russell King - ARM Linux
  1 sibling, 1 reply; 28+ messages in thread
From: Felipe Balbi @ 2010-08-11 18:57 UTC (permalink / raw)
  To: ext DebBarma, Tarun Kanti
  Cc: Balbi Felipe (Nokia-MS/Helsinki),
	Ohad Ben-Cohen, linux-wireless, linux-mmc, linux-omap, Ido Yariv,
	Mark Brown, linux-arm-kernel, Chikkature Rajashekar, Madhusudhan,
	Coelho Luciano (Nokia-MS/Helsinki),
	akpm, San Mehat, Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, Nicolas Pitre, Pandita, Vikram, Kalle Valo

Hi,

On Wed, Aug 11, 2010 at 08:52:54PM +0200, ext DebBarma, Tarun Kanti wrote:
>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.

>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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 18:57         ` Felipe Balbi
@ 2010-08-11 19:27           ` DebBarma, Tarun Kanti
  2010-08-11 21:25             ` Russell King - ARM Linux
  2010-08-12  5:21             ` Felipe Balbi
  0 siblings, 2 replies; 28+ messages in thread
From: DebBarma, Tarun Kanti @ 2010-08-11 19:27 UTC (permalink / raw)
  To: felipe.balbi
  Cc: Ohad Ben-Cohen, linux-wireless, linux-mmc, linux-omap, Ido Yariv,
	Mark Brown, linux-arm-kernel, Chikkature Rajashekar, Madhusudhan,
	Coelho Luciano (Nokia-MS/Helsinki),
	akpm, San Mehat, Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, Nicolas Pitre, Pandita, Vikram, Kalle Valo


> -----Original Message-----
> From: Felipe Balbi [mailto:felipe.balbi@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@vger.kernel.org; linux-mmc@vger.kernel.org; linux-
> omap@vger.kernel.org; Ido Yariv; Mark Brown; linux-arm-
> kernel@lists.infradead.org; Chikkature Rajashekar, Madhusudhan; Coelho
> Luciano (Nokia-MS/Helsinki); akpm@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,
> 
> On Wed, Aug 11, 2010 at 08:52:54PM +0200, ext DebBarma, Tarun Kanti wrote:
> >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.
>
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! 
 
> >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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 18:47     ` Felipe Balbi
  2010-08-11 18:52       ` DebBarma, Tarun Kanti
@ 2010-08-11 20:10       ` Ohad Ben-Cohen
  2010-08-11 21:34         ` Vitaly Wool
  2010-08-12  5:27         ` Felipe Balbi
  1 sibling, 2 replies; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-11 20:10 UTC (permalink / raw)
  To: felipe.balbi
  Cc: ext DebBarma, Tarun Kanti, linux-wireless, linux-mmc, linux-omap,
	Ido Yariv, Mark Brown, linux-arm-kernel, Chikkature Rajashekar,
	Madhusudhan, Coelho Luciano (Nokia-MS/Helsinki),
	akpm, San Mehat, Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, Nicolas Pitre, Pandita, Vikram, Kalle Valo

Hi Felipe,

On Wed, Aug 11, 2010 at 9:47 PM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
>>> +       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 ?

Yes.

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.

Thanks,
Ohad.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 18:52       ` DebBarma, Tarun Kanti
  2010-08-11 18:57         ` Felipe Balbi
@ 2010-08-11 21:21         ` Russell King - ARM Linux
  1 sibling, 0 replies; 28+ messages in thread
From: Russell King - ARM Linux @ 2010-08-11 21:21 UTC (permalink / raw)
  To: DebBarma, Tarun Kanti
  Cc: felipe.balbi, Ohad Ben-Cohen, Kalle Valo, Pandita, Vikram, akpm,
	Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, linux-wireless, Mark Brown, linux-mmc,
	Nicolas Pitre, San Mehat, Chikkature Rajashekar, Madhusudhan,
	Coelho Luciano (Nokia-MS/Helsinki),
	linux-omap, Ido Yariv, linux-arm-kernel

On Thu, Aug 12, 2010 at 12:22:54AM +0530, DebBarma, Tarun Kanti wrote:
> 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.

If pdev was ever NULL in a probe function, the kernel deserves to OOPS
so that you have a backtrace to fix the bugger.

Basically, a probe function is only called when the driver finds a
matching device to bind to - so the device _must_ already exist and
be valid.  It's basically guaranteed.  So checking for a NULL pdev is
not only a waste of space, it's a waste of CPU time and developer time
writing the check as well.

On the other hand, platform data passed in via a platform device _is_
liable to be NULL if whatever created the platform device didn't
set the platform data up.  So we can't guarantee that the platform
data will exist.  So a NULL check is appropriate here.

So, if an API in normal operation requires non-NULL data to be passed,
don't bother checking for a NULL pointer.  If you're passed a NULL
pointer in this situation, you deserve to OOPS so you get a backtrace
to fix the problem rather than silently ignoring the problem.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 19:27           ` DebBarma, Tarun Kanti
@ 2010-08-11 21:25             ` Russell King - ARM Linux
  2010-08-11 22:15               ` Ohad Ben-Cohen
                                 ` (2 more replies)
  2010-08-12  5:21             ` Felipe Balbi
  1 sibling, 3 replies; 28+ messages in thread
From: Russell King - ARM Linux @ 2010-08-11 21:25 UTC (permalink / raw)
  To: DebBarma, Tarun Kanti
  Cc: felipe.balbi, Ohad Ben-Cohen, Kalle Valo, Pandita, Vikram, akpm,
	Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, linux-wireless, Mark Brown, linux-mmc,
	Nicolas Pitre, San Mehat, Chikkature Rajashekar, Madhusudhan,
	Coelho Luciano (Nokia-MS/Helsinki),
	linux-omap, Ido Yariv, linux-arm-kernel

On Thu, Aug 12, 2010 at 12:57:18AM +0530, DebBarma, Tarun Kanti wrote:
> 
> > -----Original Message-----
> > From: Felipe Balbi [mailto:felipe.balbi@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@vger.kernel.org; linux-mmc@vger.kernel.org; linux-
> > omap@vger.kernel.org; Ido Yariv; Mark Brown; linux-arm-
> > kernel@lists.infradead.org; Chikkature Rajashekar, Madhusudhan; Coelho
> > Luciano (Nokia-MS/Helsinki); akpm@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,
> > 
> > On Wed, Aug 11, 2010 at 08:52:54PM +0200, ext DebBarma, Tarun Kanti wrote:
> > >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.
> >
> 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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 20:10       ` Ohad Ben-Cohen
@ 2010-08-11 21:34         ` Vitaly Wool
  2010-08-11 22:18           ` Ohad Ben-Cohen
  2010-08-12  5:27         ` Felipe Balbi
  1 sibling, 1 reply; 28+ messages in thread
From: Vitaly Wool @ 2010-08-11 21:34 UTC (permalink / raw)
  To: Ohad Ben-Cohen
  Cc: felipe.balbi, Kalle Valo, Pandita, Vikram, akpm,
	Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, linux-wireless, Nicolas Pitre, Mark Brown,
	linux-mmc, Ido Yariv, San Mehat, Chikkature Rajashekar,
	Madhusudhan, Coelho Luciano (Nokia-MS/Helsinki),
	linux-omap, ext DebBarma, Tarun Kanti, linux-arm-kernel

Hi Ohad,

On Wed, Aug 11, 2010 at 10:10 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 21:25             ` Russell King - ARM Linux
@ 2010-08-11 22:15               ` Ohad Ben-Cohen
  2010-08-12  6:40               ` Ohad Ben-Cohen
  2010-08-16  4:21               ` DebBarma, Tarun Kanti
  2 siblings, 0 replies; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-11 22:15 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: DebBarma, Tarun Kanti, felipe.balbi, Kalle Valo, Pandita, Vikram,
	akpm, Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, linux-wireless, Mark Brown, linux-mmc,
	Nicolas Pitre, San Mehat, Chikkature Rajashekar, Madhusudhan,
	Coelho Luciano (Nokia-MS/Helsinki),
	linux-omap, Ido Yariv, linux-arm-kernel

On Thu, Aug 12, 2010 at 12:25 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Aug 12, 2010 at 12:57:18AM +0530, DebBarma, Tarun Kanti wrote:
>>
>> > -----Original Message-----
>> > From: Felipe Balbi [mailto:felipe.balbi@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@vger.kernel.org; linux-mmc@vger.kernel.org; linux-
>> > omap@vger.kernel.org; Ido Yariv; Mark Brown; linux-arm-
>> > kernel@lists.infradead.org; Chikkature Rajashekar, Madhusudhan; Coelho
>> > Luciano (Nokia-MS/Helsinki); akpm@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,
>> >
>> > On Wed, Aug 11, 2010 at 08:52:54PM +0200, ext DebBarma, Tarun Kanti wrote:
>> > >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.
>> >
>> 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.

Thanks, Russell.

I'll give your suggestion a spin when I get back (I'm going to be away
for some time now with no devices with me).

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 21:34         ` Vitaly Wool
@ 2010-08-11 22:18           ` Ohad Ben-Cohen
  0 siblings, 0 replies; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-11 22:18 UTC (permalink / raw)
  To: Vitaly Wool
  Cc: felipe.balbi, Kalle Valo, Pandita, Vikram, akpm,
	Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, linux-wireless, Nicolas Pitre, Mark Brown,
	linux-mmc, Ido Yariv, San Mehat, Chikkature Rajashekar,
	Madhusudhan, Coelho Luciano (Nokia-MS/Helsinki),
	linux-omap, ext DebBarma, Tarun Kanti, linux-arm-kernel

Hi Vitaly,

On Thu, Aug 12, 2010 at 12:34 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
> On Wed, Aug 11, 2010 at 10:10 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> 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.

We do have strange-looking setups with two hardwired devices
(especially on new bring-ups), so it would be nice not to limit the
driver to a single device. But I agree, it's far from being important.

Thanks,
Ohad.

>
> 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
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 2/8] wireless: wl1271: support return value for the set power func
  2010-08-11 18:35   ` DebBarma, Tarun Kanti
@ 2010-08-11 22:19     ` Ohad Ben-Cohen
  0 siblings, 0 replies; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-11 22:19 UTC (permalink / raw)
  To: DebBarma, Tarun Kanti
  Cc: linux-wireless, linux-mmc, linux-omap, Ido Yariv, Mark Brown,
	linux-arm-kernel, Chikkature Rajashekar, Madhusudhan,
	Luciano Coelho, akpm, San Mehat, Roger Quadros, Tony Lindgren,
	Nicolas Pitre, Pandita, Vikram, Kalle Valo

On Wed, Aug 11, 2010 at 9:35 PM, DebBarma, Tarun Kanti
<tarun.kanti@ti.com> wrote:
>> -     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?


Can't hurt, thanks.


>> +     if (ret == 0)
>> +             set_bit(WL1271_FLAG_GPIO_POWER, &wl->flags);
>> +     return ret;
>>  }
>>
>>
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c
>> b/drivers/net/wireless/wl12xx/wl1271_main.c
>> index 9d68f00..e6e0852 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_main.c
>> +++ b/drivers/net/wireless/wl12xx/wl1271_main.c
>> @@ -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);
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_sdio.c
>> b/drivers/net/wireless/wl12xx/wl1271_sdio.c
>> index 7059b5c..c41293a 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_sdio.c
>> +++ b/drivers/net/wireless/wl12xx/wl1271_sdio.c
>> @@ -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 = {
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_spi.c
>> b/drivers/net/wireless/wl12xx/wl1271_spi.c
>> index c3fdab7..de56d8d 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_spi.c
>> +++ b/drivers/net/wireless/wl12xx/wl1271_spi.c
>> @@ -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@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 19:27           ` DebBarma, Tarun Kanti
  2010-08-11 21:25             ` Russell King - ARM Linux
@ 2010-08-12  5:21             ` Felipe Balbi
  1 sibling, 0 replies; 28+ messages in thread
From: Felipe Balbi @ 2010-08-12  5:21 UTC (permalink / raw)
  To: ext DebBarma, Tarun Kanti
  Cc: Balbi Felipe (Nokia-MS/Helsinki),
	Ohad Ben-Cohen, linux-wireless, linux-mmc, linux-omap, Ido Yariv,
	Mark Brown, linux-arm-kernel, Chikkature Rajashekar, Madhusudhan,
	Coelho Luciano (Nokia-MS/Helsinki),
	akpm, San Mehat, Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, Nicolas Pitre, Pandita, Vikram, Kalle Valo

Hi,

On Wed, Aug 11, 2010 at 09:27:18PM +0200, ext DebBarma, Tarun Kanti wrote:
>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!

because pdata is completely optional. pdev OTOH isn't. Without a matched 
platform_device, your probe() won't get called.

-- 
balbi

DefectiveByDesign.org

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board  data
  2010-08-11 20:10       ` Ohad Ben-Cohen
  2010-08-11 21:34         ` Vitaly Wool
@ 2010-08-12  5:27         ` Felipe Balbi
  1 sibling, 0 replies; 28+ messages in thread
From: Felipe Balbi @ 2010-08-12  5:27 UTC (permalink / raw)
  To: ext Ohad Ben-Cohen
  Cc: Balbi Felipe (Nokia-MS/Helsinki),
	ext DebBarma, Tarun Kanti, linux-wireless, linux-mmc, linux-omap,
	Ido Yariv, Mark Brown, linux-arm-kernel, Chikkature Rajashekar,
	Madhusudhan, Coelho Luciano (Nokia-MS/Helsinki),
	akpm, San Mehat, Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, Nicolas Pitre, Pandita, Vikram, Kalle Valo

Hi,

On Wed, Aug 11, 2010 at 10:10:38PM +0200, ext Ohad Ben-Cohen wrote:
>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.

you need no globals and you don't need to fiddle with driver structures 
either.

Just make an sdio driver that platform_device_alloc() and pass in a 
platform_data, builds struct resources etc for each device found on the 
sdio bus. Similarly for the SPI-connected wifi chip.

You should never fiddle with internal structures. If you want to look at 
an example implementation, take a look at drivers/mfd/twl-core.c, there 
we use an i2c driver to instantiate several platform_devices. Nobody 
fiddles with driver structures and all the drivers are logically 
separated into their own source files.

-- 
balbi

DefectiveByDesign.org

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 21:25             ` Russell King - ARM Linux
  2010-08-11 22:15               ` Ohad Ben-Cohen
@ 2010-08-12  6:40               ` Ohad Ben-Cohen
  2010-08-12  9:55                 ` Russell King - ARM Linux
  2010-08-16  4:21               ` DebBarma, Tarun Kanti
  2 siblings, 1 reply; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-12  6:40 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: DebBarma, Tarun Kanti, felipe.balbi, Kalle Valo, Pandita, Vikram,
	akpm, Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, linux-wireless, Mark Brown, linux-mmc,
	Nicolas Pitre, San Mehat, Chikkature Rajashekar, Madhusudhan,
	Coelho Luciano (Nokia-MS/Helsinki),
	linux-omap, Ido Yariv, linux-arm-kernel

On Thu, Aug 12, 2010 at 12:25 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>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.

Please tell me if you still support it,

Thanks,
Ohad.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-12  6:40               ` Ohad Ben-Cohen
@ 2010-08-12  9:55                 ` Russell King - ARM Linux
  2010-08-13  0:01                   ` Ohad Ben-Cohen
  0 siblings, 1 reply; 28+ messages in thread
From: Russell King - ARM Linux @ 2010-08-12  9:55 UTC (permalink / raw)
  To: Ohad Ben-Cohen
  Cc: DebBarma, Tarun Kanti, felipe.balbi, Kalle Valo, Pandita, Vikram,
	akpm, Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, linux-wireless, Mark Brown, linux-mmc,
	Nicolas Pitre, San Mehat, Chikkature Rajashekar, Madhusudhan,
	Coelho Luciano (Nokia-MS/Helsinki),
	linux-omap, Ido Yariv, linux-arm-kernel

On Thu, Aug 12, 2010 at 09:40:19AM +0300, Ohad Ben-Cohen wrote:
> On Thu, Aug 12, 2010 at 12:25 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> >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.

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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-12  9:55                 ` Russell King - ARM Linux
@ 2010-08-13  0:01                   ` Ohad Ben-Cohen
  0 siblings, 0 replies; 28+ messages in thread
From: Ohad Ben-Cohen @ 2010-08-13  0:01 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: DebBarma, Tarun Kanti, felipe.balbi, Kalle Valo, Pandita, Vikram,
	akpm, Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, linux-wireless, Mark Brown, linux-mmc,
	Nicolas Pitre, San Mehat, Chikkature Rajashekar, Madhusudhan,
	Coelho Luciano (Nokia-MS/Helsinki),
	linux-omap, Ido Yariv, linux-arm-kernel

On Thursday, August 12, 2010, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Aug 12, 2010 at 09:40:19AM +0300, Ohad Ben-Cohen wrote:
>> On Thu, Aug 12, 2010 at 12:25 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> >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.
>
> 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.

Thanks, I see what you mean now.

I'll give this a spin when I'm back (I'm away until e/o August)
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data
  2010-08-11 21:25             ` Russell King - ARM Linux
  2010-08-11 22:15               ` Ohad Ben-Cohen
  2010-08-12  6:40               ` Ohad Ben-Cohen
@ 2010-08-16  4:21               ` DebBarma, Tarun Kanti
  2 siblings, 0 replies; 28+ messages in thread
From: DebBarma, Tarun Kanti @ 2010-08-16  4:21 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: felipe.balbi, Ohad Ben-Cohen, Kalle Valo, Pandita, Vikram, akpm,
	Quadros Roger (Nokia-MS/Helsinki),
	Tony Lindgren, linux-wireless, Mark Brown, linux-mmc,
	Nicolas Pitre, San Mehat, Chikkature Rajashekar, Madhusudhan,
	Coelho Luciano (Nokia-MS/Helsinki),
	linux-omap, Ido Yariv, linux-arm-kernel


> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Thursday, August 12, 2010 2:55 AM
> To: DebBarma, Tarun Kanti
> Cc: felipe.balbi@nokia.com; Ohad Ben-Cohen; Kalle Valo; Pandita, Vikram;
> akpm@linux-foundation.org; Quadros Roger (Nokia-MS/Helsinki); Tony
> Lindgren; linux-wireless@vger.kernel.org; Mark Brown; linux-
> mmc@vger.kernel.org; Nicolas Pitre; San Mehat; Chikkature Rajashekar,
> Madhusudhan; Coelho Luciano (Nokia-MS/Helsinki); linux-
> omap@vger.kernel.org; Ido Yariv; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v4 3/8] wireless: wl1271: add platform driver to get
> board data
> 
> On Thu, Aug 12, 2010 at 12:57:18AM +0530, DebBarma, Tarun Kanti wrote:
> >
> > > -----Original Message-----
> > > From: Felipe Balbi [mailto:felipe.balbi@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@vger.kernel.org; linux-mmc@vger.kernel.org; linux-
> > > omap@vger.kernel.org; Ido Yariv; Mark Brown; linux-arm-
> > > kernel@lists.infradead.org; Chikkature Rajashekar, Madhusudhan; Coelho
> > > Luciano (Nokia-MS/Helsinki); akpm@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,
> > >
> > > On Wed, Aug 11, 2010 at 08:52:54PM +0200, ext DebBarma, Tarun Kanti
> wrote:
> > > >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.
> > >
> > 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 guess I have not put my question in the right perspective. As you have said, platform devices are not meant for passing data around, it appeared to me contradictory when in the probe() we are just checking the pdata and proceeding/returning based upon its value, as if platform device has no other functions. So, the question should have been what else we could do in probe() with platform device other than manipulation of pdata, other than modification of the framework in the real sense.

Anyways, I will go through the suggestions you mentioned. Thanks for the clarifications.



^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2010-08-16  4:22 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-11 18:21 [PATCH v4 0/8] native support for wl1271 on ZOOM Ohad Ben-Cohen
2010-08-11 18:21 ` [PATCH v4 1/8] wireless: wl1271: make wl12xx.h common to both spi and sdio Ohad Ben-Cohen
2010-08-11 18:21 ` [PATCH v4 2/8] wireless: wl1271: support return value for the set power func Ohad Ben-Cohen
2010-08-11 18:35   ` DebBarma, Tarun Kanti
2010-08-11 22:19     ` Ohad Ben-Cohen
2010-08-11 18:21 ` [PATCH v4 3/8] wireless: wl1271: add platform driver to get board data Ohad Ben-Cohen
2010-08-11 18:42   ` DebBarma, Tarun Kanti
2010-08-11 18:47     ` Felipe Balbi
2010-08-11 18:52       ` DebBarma, Tarun Kanti
2010-08-11 18:57         ` Felipe Balbi
2010-08-11 19:27           ` DebBarma, Tarun Kanti
2010-08-11 21:25             ` Russell King - ARM Linux
2010-08-11 22:15               ` Ohad Ben-Cohen
2010-08-12  6:40               ` Ohad Ben-Cohen
2010-08-12  9:55                 ` Russell King - ARM Linux
2010-08-13  0:01                   ` Ohad Ben-Cohen
2010-08-16  4:21               ` DebBarma, Tarun Kanti
2010-08-12  5:21             ` Felipe Balbi
2010-08-11 21:21         ` Russell King - ARM Linux
2010-08-11 20:10       ` Ohad Ben-Cohen
2010-08-11 21:34         ` Vitaly Wool
2010-08-11 22:18           ` Ohad Ben-Cohen
2010-08-12  5:27         ` Felipe Balbi
2010-08-11 18:21 ` [PATCH v4 4/8] wireless: wl1271: take irq info from private " Ohad Ben-Cohen
2010-08-11 18:21 ` [PATCH v4 5/8] wireless: wl1271: make ref_clock configurable by board Ohad Ben-Cohen
2010-08-11 18:21 ` [PATCH v4 6/8] omap: hsmmc: remove unused variable Ohad Ben-Cohen
2010-08-11 18:21 ` [PATCH v4 7/8] omap: zoom: add fixed regulator device for wlan Ohad Ben-Cohen
2010-08-11 18:21 ` [PATCH v4 8/8] omap: zoom: add mmc3/wl1271 device support Ohad Ben-Cohen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).