From: Xinming Hu <[email protected]>
Platform device and driver provides easy way to
interact with device-tree-enabled system.
This patch registers platform driver and reorganise
existing device tree specific code.
Corresponding device tree binding file is
also created.
Signed-off-by: Xinming Hu <[email protected]>
Signed-off-by: Amitkumar Karwar <[email protected]>
---
v3: Don't update adapter->dt_node if mwifiex_plt_dev is NULL
---
Documentation/devicetree/bindings/mwifiex.txt | 29 +++++++++++
drivers/net/wireless/marvell/mwifiex/Makefile | 1 +
drivers/net/wireless/marvell/mwifiex/main.c | 2 +
drivers/net/wireless/marvell/mwifiex/main.h | 14 +++++
.../net/wireless/marvell/mwifiex/platform_drv.c | 59 ++++++++++++++++++++++
drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 6 +--
drivers/net/wireless/marvell/mwifiex/sta_ioctl.c | 2 +-
7 files changed, 109 insertions(+), 4 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mwifiex.txt
create mode 100644 drivers/net/wireless/marvell/mwifiex/platform_drv.c
diff --git a/Documentation/devicetree/bindings/mwifiex.txt b/Documentation/devicetree/bindings/mwifiex.txt
new file mode 100644
index 0000000..39b6a74
--- /dev/null
+++ b/Documentation/devicetree/bindings/mwifiex.txt
@@ -0,0 +1,29 @@
+mwifiex
+------
+
+Required properties:
+
+ - name : must be "mwifiex"
+ - compatible : must be "marvell,mwifiex"
+
+Optional properties:
+
+ - mwifiex,caldata* : A series of properties with marvell,caldata prefix,
+ represent Calibration data downloaded to the device during
+ initialization. This is an array of unsigned values.
+
+
+Example:
+
+Tx power limit calibration data is configured in below example.
+The calibration data is an array of unsigned values, the length
+can vary between hw versions.
+
+mwifiex {
+ compatible = "marvell,mwifiex";
+
+ mwifiex,caldata_00_txpwrlimit_2g_cfg_set = /bits/ 8 <
+0x01 0x00 0x06 0x00 0x08 0x02 0x89 0x01 ...>;
+
+};
+
diff --git a/drivers/net/wireless/marvell/mwifiex/Makefile b/drivers/net/wireless/marvell/mwifiex/Makefile
index fdfd9bf..1444fd1 100644
--- a/drivers/net/wireless/marvell/mwifiex/Makefile
+++ b/drivers/net/wireless/marvell/mwifiex/Makefile
@@ -42,6 +42,7 @@ mwifiex-y += cfg80211.o
mwifiex-y += ethtool.o
mwifiex-y += 11h.o
mwifiex-y += tdls.o
+mwifiex-y += platform_drv.o
mwifiex-$(CONFIG_DEBUG_FS) += debugfs.o
obj-$(CONFIG_MWIFIEX) += mwifiex.o
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 3cfa946..b93ae69 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1538,6 +1538,7 @@ EXPORT_SYMBOL_GPL(_mwifiex_dbg);
static int
mwifiex_init_module(void)
{
+ mwifiex_platform_drv_init();
#ifdef CONFIG_DEBUG_FS
mwifiex_debugfs_init();
#endif
@@ -1552,6 +1553,7 @@ mwifiex_init_module(void)
static void
mwifiex_cleanup_module(void)
{
+ mwifiex_platform_drv_exit();
#ifdef CONFIG_DEBUG_FS
mwifiex_debugfs_remove();
#endif
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index aea7aee..464d79f 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -37,6 +37,16 @@
#include <linux/idr.h>
#include <linux/inetdevice.h>
#include <linux/devcoredump.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/gfp.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
#include "decl.h"
#include "ioctl.h"
@@ -47,6 +57,7 @@
#include "sdio.h"
extern const char driver_version[];
+extern struct platform_device *mwifiex_plt_dev;
struct mwifiex_adapter;
struct mwifiex_private;
@@ -1597,6 +1608,9 @@ void mwifiex_process_multi_chan_event(struct mwifiex_private *priv,
struct sk_buff *event_skb);
void mwifiex_multi_chan_resync(struct mwifiex_adapter *adapter);
+int mwifiex_platform_drv_init(void);
+void mwifiex_platform_drv_exit(void);
+
#ifdef CONFIG_DEBUG_FS
void mwifiex_debugfs_init(void);
void mwifiex_debugfs_remove(void);
diff --git a/drivers/net/wireless/marvell/mwifiex/platform_drv.c b/drivers/net/wireless/marvell/mwifiex/platform_drv.c
new file mode 100644
index 0000000..f64a12a
--- /dev/null
+++ b/drivers/net/wireless/marvell/mwifiex/platform_drv.c
@@ -0,0 +1,59 @@
+/* Marvell wireless LAN device driver: platform specific driver
+ *
+ * Copyright (C) 2015, Marvell International Ltd.
+ *
+ * This software file (the "File") is distributed by Marvell International
+ * Ltd. under the terms of the GNU General Public License Version 2, June 1991
+ * (the "License"). You may use, redistribute and/or modify this File in
+ * accordance with the terms and conditions of the License, a copy of which
+ * is available on the worldwide web at
+ * http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
+ *
+ * THE FILE IS DISTRIBUTED AS-IS, WITHOUT WARRANTY OF ANY KIND, AND THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE
+ * ARE EXPRESSLY DISCLAIMED. The License provides additional details about
+ * this warranty disclaimer.
+ */
+#include "main.h"
+
+struct platform_device *mwifiex_plt_dev;
+
+static int mwifiex_plt_probe(struct platform_device *pdev)
+{
+ mwifiex_plt_dev = pdev;
+ return 0;
+}
+
+static int mwifiex_plt_remove(struct platform_device *pdev)
+{
+ mwifiex_plt_dev = NULL;
+ return 0;
+}
+
+static const struct of_device_id mwifiex_dt_match[] = {
+ {
+ .compatible = "marvell,mwifiex",
+ },
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, mwifiex_dt_match);
+
+static struct platform_driver mwifiex_platform_driver = {
+ .probe = mwifiex_plt_probe,
+ .remove = mwifiex_plt_remove,
+ .driver = {
+ .name = "mwifiex_plt",
+ .of_match_table = mwifiex_dt_match,
+ }
+};
+
+int mwifiex_platform_drv_init(void)
+{
+ return platform_driver_register(&mwifiex_platform_driver);
+}
+
+void mwifiex_platform_drv_exit(void)
+{
+ platform_driver_unregister(&mwifiex_platform_driver);
+}
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index 30f1526..a8b6939 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -2154,11 +2154,11 @@ int mwifiex_sta_init_cmd(struct mwifiex_private *priv, u8 first_sta, bool init)
* The cal-data can be read from device tree and/or
* a configuration file and downloaded to firmware.
*/
- adapter->dt_node =
- of_find_node_by_name(NULL, "marvell_cfgdata");
+ adapter->dt_node = mwifiex_plt_dev ?
+ mwifiex_plt_dev->dev.of_node : NULL;
if (adapter->dt_node) {
ret = mwifiex_dnld_dt_cfgdata(priv, adapter->dt_node,
- "marvell,caldata");
+ "mwifiex,caldata");
if (ret)
return -1;
}
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
index 5cbee58..c598f66 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
@@ -195,7 +195,7 @@ int mwifiex_fill_new_bss_desc(struct mwifiex_private *priv,
void mwifiex_dnld_txpwr_table(struct mwifiex_private *priv)
{
if (priv->adapter->dt_node) {
- char txpwr[] = {"marvell,00_txpwrlimit"};
+ char txpwr[] = {"mwifiex,00_txpwrlimit"};
memcpy(&txpwr[8], priv->adapter->country_code, 2);
mwifiex_dnld_dt_cfgdata(priv, priv->adapter->dt_node, txpwr);
--
1.8.1.4
From: Xinming Hu <[email protected]>
This patch parse chip specific gpio parameter from device
tree. Corresponding binding file is also updated.
Signed-off-by: Xinming Hu <[email protected]>
Signed-off-by: Amitkumar Karwar <[email protected]>
---
Documentation/devicetree/bindings/mwifiex.txt | 6 ++++--
drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 9 +++++++++
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/mwifiex.txt b/Documentation/devicetree/bindings/mwifiex.txt
index 39b6a74..367c97e 100644
--- a/Documentation/devicetree/bindings/mwifiex.txt
+++ b/Documentation/devicetree/bindings/mwifiex.txt
@@ -11,7 +11,9 @@ Optional properties:
- mwifiex,caldata* : A series of properties with marvell,caldata prefix,
represent Calibration data downloaded to the device during
initialization. This is an array of unsigned values.
-
+ - mwifiex,chip-gpio : Chip's wakeup gpio pin number. This needs to be downloaded
+ to to firmware. Chip notifies wifi wakeup signal to SOC
+ through this pin.
Example:
@@ -24,6 +26,6 @@ mwifiex {
mwifiex,caldata_00_txpwrlimit_2g_cfg_set = /bits/ 8 <
0x01 0x00 0x06 0x00 0x08 0x02 0x89 0x01 ...>;
-
+ mwifiex,chip-gpio = <3>;
};
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index a8b6939..8c011eb 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -2134,6 +2134,7 @@ int mwifiex_sta_init_cmd(struct mwifiex_private *priv, u8 first_sta, bool init)
enum state_11d_t state_11d;
struct mwifiex_ds_11n_tx_cfg tx_cfg;
u8 sdio_sp_rx_aggr_enable;
+ u32 data;
if (first_sta) {
if (priv->adapter->iface_type == MWIFIEX_PCIE) {
@@ -2157,6 +2158,14 @@ int mwifiex_sta_init_cmd(struct mwifiex_private *priv, u8 first_sta, bool init)
adapter->dt_node = mwifiex_plt_dev ?
mwifiex_plt_dev->dev.of_node : NULL;
if (adapter->dt_node) {
+ if (of_property_read_u32(adapter->dt_node,
+ "mwifiex,chip-gpio",
+ &data) == 0) {
+ mwifiex_dbg(adapter, INFO,
+ "chip_gpio = 0x%x\n", data);
+ adapter->hs_cfg.gpio = data;
+ }
+
ret = mwifiex_dnld_dt_cfgdata(priv, adapter->dt_node,
"mwifiex,caldata");
if (ret)
--
1.8.1.4
On Tuesday 09 February 2016 13:44:16 Amitkumar Karwar wrote:
> > > index 0000000..39b6a74
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mwifiex.txt
> > > @@ -0,0 +1,29 @@
> > > +mwifiex
> > > +------
> > > +
> > > +Required properties:
> > > +
> > > + - name : must be "mwifiex"
> >
> > This is a very unusual requirement.
> >
> > Can you use one of the standard device names instead?
> >
>
> We will make use of marvell-sd8xxx in updated version as per Rob Herring's suggestions.
> It includes vendor name and a chipset/device name.
I meant the node name, not the compatible string. This should be something
like "ethernet" or "wlan".
Arnd
On Mon, Feb 08, 2016 at 02:15:26AM -0800, Amitkumar Karwar wrote:
> From: Xinming Hu <[email protected]>
>
> Platform device and driver provides easy way to
> interact with device-tree-enabled system.
>
> This patch registers platform driver and reorganise
> existing device tree specific code.
> Corresponding device tree binding file is
> also created.
Wrap you lines at 72 columns.
> Signed-off-by: Xinming Hu <[email protected]>
> Signed-off-by: Amitkumar Karwar <[email protected]>
> ---
> v3: Don't update adapter->dt_node if mwifiex_plt_dev is NULL
> ---
> Documentation/devicetree/bindings/mwifiex.txt | 29 +++++++++++
This doesn't belong at top level of bindings. bindings/net/wireless/
Also, mwifiex is a linux driver name. Name it after the chips.
marvell-sd8xxx?
> drivers/net/wireless/marvell/mwifiex/Makefile | 1 +
> drivers/net/wireless/marvell/mwifiex/main.c | 2 +
> drivers/net/wireless/marvell/mwifiex/main.h | 14 +++++
> .../net/wireless/marvell/mwifiex/platform_drv.c | 59 ++++++++++++++++++++++
> drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 6 +--
> drivers/net/wireless/marvell/mwifiex/sta_ioctl.c | 2 +-
> 7 files changed, 109 insertions(+), 4 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/mwifiex.txt
> create mode 100644 drivers/net/wireless/marvell/mwifiex/platform_drv.c
>
> diff --git a/Documentation/devicetree/bindings/mwifiex.txt b/Documentation/devicetree/bindings/mwifiex.txt
> new file mode 100644
> index 0000000..39b6a74
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mwifiex.txt
> @@ -0,0 +1,29 @@
> +mwifiex
> +------
> +
> +Required properties:
> +
> + - name : must be "mwifiex"
> + - compatible : must be "marvell,mwifiex"
The naming should be named after the actual chip.
> +
> +Optional properties:
> +
> + - mwifiex,caldata* : A series of properties with marvell,caldata prefix,
mwifiex is not a vendor.
> + represent Calibration data downloaded to the device during
> + initialization. This is an array of unsigned values.
> +
> +
> +Example:
> +
> +Tx power limit calibration data is configured in below example.
> +The calibration data is an array of unsigned values, the length
> +can vary between hw versions.
> +
> +mwifiex {
These chips are SDIO devices, right? This should be a child node of the
SDIO controller.
> + compatible = "marvell,mwifiex";
> +
> + mwifiex,caldata_00_txpwrlimit_2g_cfg_set = /bits/ 8 <
> +0x01 0x00 0x06 0x00 0x08 0x02 0x89 0x01 ...>;
> +
> +};
> +
> > > > Please use the GPIO DT binding. Reading a number from DT is not a
> > > > proper way to get a GPIO number, as you may have more than one
> > > > GPIO controller in a system and it is not obvious to which
> > > > controller this number belongs, or if you need to specify things
> like polarity.
> > >
> > > My read of this is it is not the host SOC gpio, but the WiFi
> > > device's GPIO number. The host GPIO is defined in patch 3. We could
> > > still use the GPIO binding to describe it doing something like
> > > "marvell,<wifi gpio pin
> > > name>-gpios". Then the assignment is based on the property name.
>
> I see.
>
> > Yes. This is not host SOC gpio. It's wifi chip's gpio number.
> > We will use GPIO binding for this in updated version.
>
> No, if it doesn't refer to a number that is interpreted by the host but
> is used internally in the device, then leave it as it is, as Rob
> suggested.
>
It won't be interpreted by host. I will keep it as is and just rename from "mwifiex,chip-gpio" to "marvell,wakeup-gpios"
Regards,
Amitkumar Karwar.
From: Xinming Hu <[email protected]>
This patch implement a framework for board specific wakeup.
driver parse irq/gpio number from device tree, corresponding
resources will be allocated, and used for host suspend/resume.
driver private binding file is also updated in the patch.
Signed-off-by: Xinming Hu <[email protected]>
Signed-off-by: Amitkumar Karwar <[email protected]>
---
v2: Guarded ".pm = mwifiex_plt_pm_ops" line with CONFIG_PM_SLEEP
---
Documentation/devicetree/bindings/mwifiex.txt | 10 +++
.../net/wireless/marvell/mwifiex/platform_drv.c | 95 ++++++++++++++++++++++
2 files changed, 105 insertions(+)
diff --git a/Documentation/devicetree/bindings/mwifiex.txt b/Documentation/devicetree/bindings/mwifiex.txt
index 367c97e..68919c7 100644
--- a/Documentation/devicetree/bindings/mwifiex.txt
+++ b/Documentation/devicetree/bindings/mwifiex.txt
@@ -14,6 +14,11 @@ Optional properties:
- mwifiex,chip-gpio : Chip's wakeup gpio pin number. This needs to be downloaded
to to firmware. Chip notifies wifi wakeup signal to SOC
through this pin.
+ - interrupt-parent: phandle of the parent interrupt controller
+ - interrupts : interrupt number to the cpu
+ - gpios: specify SOC's wakeup GPIO
+ - pinctrl-names : a pinctrl state named "default" must be defined
+ - pinctrl-0 : pin control group to be used for this controller
Example:
@@ -27,5 +32,10 @@ mwifiex {
mwifiex,caldata_00_txpwrlimit_2g_cfg_set = /bits/ 8 <
0x01 0x00 0x06 0x00 0x08 0x02 0x89 0x01 ...>;
mwifiex,chip-gpio = <3>;
+ interrupt-parent = <&pio>;
+ interrupts = <38 IRQ_TYPE_LEVEL_LOW>;
+ gpios = <&pio 38 GPIO_ACTIVE_HIGH>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&wake_pins>;
};
diff --git a/drivers/net/wireless/marvell/mwifiex/platform_drv.c b/drivers/net/wireless/marvell/mwifiex/platform_drv.c
index f64a12a..2eb813c 100644
--- a/drivers/net/wireless/marvell/mwifiex/platform_drv.c
+++ b/drivers/net/wireless/marvell/mwifiex/platform_drv.c
@@ -18,9 +18,63 @@
struct platform_device *mwifiex_plt_dev;
+struct mwifiex_wake_dev {
+ struct device *dev;
+ int irq_wifi;
+ bool wake_by_wifi;
+};
+
+static irqreturn_t mwifiex_wake_irq_wifi(int irq, void *priv)
+{
+ struct mwifiex_wake_dev *ctx = priv;
+
+ if (ctx->irq_wifi >= 0) {
+ ctx->wake_by_wifi = true;
+ disable_irq_nosync(ctx->irq_wifi);
+ }
+
+ return IRQ_HANDLED;
+}
+
static int mwifiex_plt_probe(struct platform_device *pdev)
{
+ int ret;
+ struct mwifiex_wake_dev *ctx;
+ int gpio;
+
mwifiex_plt_dev = pdev;
+
+ ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx->dev = &pdev->dev;
+ ctx->irq_wifi = platform_get_irq(pdev, 0);
+ if (ctx->irq_wifi < 0)
+ dev_err(&pdev->dev, "Failed to get irq_wifi\n");
+
+ gpio = of_get_gpio(pdev->dev.of_node, 0);
+ if (gpio_is_valid(gpio))
+ gpio_direction_input(gpio);
+ else
+ dev_err(&pdev->dev, "gpio wifi is invalid\n");
+
+ if (ctx->irq_wifi >= 0) {
+ ret = devm_request_irq(&pdev->dev, ctx->irq_wifi,
+ mwifiex_wake_irq_wifi,
+ IRQF_TRIGGER_LOW,
+ "wifi_wake", ctx);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Failed to request irq_wifi %d (%d)\n",
+ ctx->irq_wifi, ret);
+ return -EINVAL;
+ }
+ disable_irq(ctx->irq_wifi);
+ }
+
+ platform_set_drvdata(pdev, ctx);
+
return 0;
}
@@ -30,6 +84,44 @@ static int mwifiex_plt_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_PM_SLEEP
+static int mwifiex_plt_suspend(struct device *dev)
+{
+ struct mwifiex_wake_dev *ctx = dev_get_drvdata(dev);
+ int ret;
+
+ if (ctx->irq_wifi >= 0) {
+ ctx->wake_by_wifi = false;
+ enable_irq(ctx->irq_wifi);
+ ret = enable_irq_wake(ctx->irq_wifi);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int mwifiex_plt_resume(struct device *dev)
+{
+ struct mwifiex_wake_dev *ctx = dev_get_drvdata(dev);
+ int ret;
+
+ if (ctx->irq_wifi >= 0) {
+ ret = disable_irq_wake(ctx->irq_wifi);
+ if (!ctx->wake_by_wifi)
+ disable_irq(ctx->irq_wifi);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct dev_pm_ops mwifiex_plt_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(mwifiex_plt_suspend, mwifiex_plt_resume)
+};
+#endif /* CONFIG_PM_SLEEP */
+
static const struct of_device_id mwifiex_dt_match[] = {
{
.compatible = "marvell,mwifiex",
@@ -45,6 +137,9 @@ static struct platform_driver mwifiex_platform_driver = {
.driver = {
.name = "mwifiex_plt",
.of_match_table = mwifiex_dt_match,
+#ifdef CONFIG_PM_SLEEP
+ .pm = &mwifiex_plt_pm_ops,
+#endif
}
};
--
1.8.1.4
On Mon, Feb 08, 2016 at 01:11:03PM +0100, Arnd Bergmann wrote:
> On Monday 08 February 2016 02:15:27 Amitkumar Karwar wrote:
> > if (adapter->dt_node) {
> > + if (of_property_read_u32(adapter->dt_node,
> > + "mwifiex,chip-gpio",
> > + &data) == 0) {
> > + mwifiex_dbg(adapter, INFO,
> > + "chip_gpio = 0x%x\n", data);
> > + adapter->hs_cfg.gpio = data;
> > + }
> > +
> >
>
> Please use the GPIO DT binding. Reading a number from DT is not a proper
> way to get a GPIO number, as you may have more than one GPIO controller
> in a system and it is not obvious to which controller this number belongs,
> or if you need to specify things like polarity.
My read of this is it is not the host SOC gpio, but the WiFi device's
GPIO number. The host GPIO is defined in patch 3. We could still use the
GPIO binding to describe it doing something like "marvell,<wifi gpio pin
name>-gpios". Then the assignment is based on the property name.
Rob
Hi Arnd
> mwifiex_platform_driver = {
> > .driver = {
> > .name = "mwifiex_plt",
> > .of_match_table = mwifiex_dt_match,
> > +#ifdef CONFIG_PM_SLEEP
> > + .pm = &mwifiex_plt_pm_ops,
> > +#endif
> > }
> > };
>
> Just remove the #ifdef here, and mark the functions as __maybe_unused.
>
Sure. I will take care of this.
Regards,
Amitkumar Karwar
Hi Rob,
Thanks for your review.
> From: Rob Herring [mailto:[email protected]]
> Sent: Tuesday, February 09, 2016 3:26 AM
> To: Amitkumar Karwar
> Cc: [email protected]; Nishant Sarmukadam;
> [email protected]; [email protected]; Xinming Hu
> Subject: Re: [PATCH v3 1/3] mwifiex: register platform specific driver
>
> On Mon, Feb 08, 2016 at 02:15:26AM -0800, Amitkumar Karwar wrote:
> > From: Xinming Hu <[email protected]>
> >
> > Platform device and driver provides easy way to interact with
> > device-tree-enabled system.
> >
> > This patch registers platform driver and reorganise existing device
> > tree specific code.
> > Corresponding device tree binding file is also created.
>
> Wrap you lines at 72 columns.
Ack.
> > Signed-off-by: Xinming Hu <[email protected]>
> > Signed-off-by: Amitkumar Karwar <[email protected]>
> > ---
> > v3: Don't update adapter->dt_node if mwifiex_plt_dev is NULL
> > ---
> > Documentation/devicetree/bindings/mwifiex.txt | 29 +++++++++++
>
> This doesn't belong at top level of bindings. bindings/net/wireless/
>
> Also, mwifiex is a linux driver name. Name it after the chips.
> marvell-sd8xxx?
Sure. I will rename as marvell-sd8xxx and move it to bindings/net/wireless/
>
> > drivers/net/wireless/marvell/mwifiex/Makefile | 1 +
> > drivers/net/wireless/marvell/mwifiex/main.c | 2 +
> > drivers/net/wireless/marvell/mwifiex/main.h | 14 +++++
> > .../net/wireless/marvell/mwifiex/platform_drv.c | 59
> ++++++++++++++++++++++
> > drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 6 +--
> > drivers/net/wireless/marvell/mwifiex/sta_ioctl.c | 2 +-
> > 7 files changed, 109 insertions(+), 4 deletions(-) create mode
> > 100644 Documentation/devicetree/bindings/mwifiex.txt
> > create mode 100644
> > drivers/net/wireless/marvell/mwifiex/platform_drv.c
> >
> > diff --git a/Documentation/devicetree/bindings/mwifiex.txt
> > b/Documentation/devicetree/bindings/mwifiex.txt
> > new file mode 100644
> > index 0000000..39b6a74
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mwifiex.txt
> > @@ -0,0 +1,29 @@
> > +mwifiex
> > +------
> > +
> > +Required properties:
> > +
> > + - name : must be "mwifiex"
> > + - compatible : must be "marvell,mwifiex"
>
> The naming should be named after the actual chip.
Ok. How about having names as below?
+ - name : can be "sd8897", "sd8787" or "sd8797"
+ - compatible : must be "marvell,sd8xxx"
>
> > +
> > +Optional properties:
> > +
> > + - mwifiex,caldata* : A series of properties with marvell,caldata
> > + prefix,
>
> mwifiex is not a vendor.
Got it. We will use "marvell,caldata*" here.
>
> > + represent Calibration data downloaded to the device
> during
> > + initialization. This is an array of unsigned values.
> > +
> > +
> > +Example:
> > +
> > +Tx power limit calibration data is configured in below example.
> > +The calibration data is an array of unsigned values, the length can
> > +vary between hw versions.
> > +
> > +mwifiex {
>
> These chips are SDIO devices, right? This should be a child node of the
> SDIO controller.
>
Yes. They are SDIO devices. We will have the node as child node of SDIO controller.
Regards,
Amitkumar Karwar
> From: Arnd Bergmann [mailto:[email protected]]
> Sent: Tuesday, February 09, 2016 7:48 PM
> To: Amitkumar Karwar
> Cc: [email protected]; Nishant Sarmukadam;
> [email protected]; [email protected]; Xinming Hu; Cathy Luo
> Subject: Re: [PATCH v3 1/3] mwifiex: register platform specific driver
>
> On Tuesday 09 February 2016 13:44:16 Amitkumar Karwar wrote:
> > > > index 0000000..39b6a74
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mwifiex.txt
> > > > @@ -0,0 +1,29 @@
> > > > +mwifiex
> > > > +------
> > > > +
> > > > +Required properties:
> > > > +
> > > > + - name : must be "mwifiex"
> > >
> > > This is a very unusual requirement.
> > >
> > > Can you use one of the standard device names instead?
> > >
> >
> > We will make use of marvell-sd8xxx in updated version as per Rob
> Herring's suggestions.
> > It includes vendor name and a chipset/device name.
>
> I meant the node name, not the compatible string. This should be
> something like "ethernet" or "wlan".
Thanks for clarification. I will name the node as "wlan"
Regards,
Amitkumar Karwar
On Tuesday 09 February 2016 14:03:19 Amitkumar Karwar wrote:
>
> > On Mon, Feb 08, 2016 at 01:11:03PM +0100, Arnd Bergmann wrote:
> > > On Monday 08 February 2016 02:15:27 Amitkumar Karwar wrote:
> > > > if (adapter->dt_node) {
> > > > + if (of_property_read_u32(adapter->dt_node,
> > > > + "mwifiex,chip-
> > gpio",
> > > > + &data) == 0) {
> > > > + mwifiex_dbg(adapter, INFO,
> > > > + "chip_gpio = 0x%x\n",
> > data);
> > > > + adapter->hs_cfg.gpio = data;
> > > > + }
> > > > +
> > > >
> > >
> > > Please use the GPIO DT binding. Reading a number from DT is not a
> > > proper way to get a GPIO number, as you may have more than one GPIO
> > > controller in a system and it is not obvious to which controller this
> > > number belongs, or if you need to specify things like polarity.
> >
> > My read of this is it is not the host SOC gpio, but the WiFi device's
> > GPIO number. The host GPIO is defined in patch 3. We could still use the
> > GPIO binding to describe it doing something like "marvell,<wifi gpio pin
> > name>-gpios". Then the assignment is based on the property name.
I see.
> Yes. This is not host SOC gpio. It's wifi chip's gpio number.
> We will use GPIO binding for this in updated version.
No, if it doesn't refer to a number that is interpreted by the host
but is used internally in the device, then leave it as it is, as Rob
suggested.
Arnd
On Monday 08 February 2016 02:15:27 Amitkumar Karwar wrote:
> if (adapter->dt_node) {
> + if (of_property_read_u32(adapter->dt_node,
> + "mwifiex,chip-gpio",
> + &data) == 0) {
> + mwifiex_dbg(adapter, INFO,
> + "chip_gpio = 0x%x\n", data);
> + adapter->hs_cfg.gpio = data;
> + }
> +
>
Please use the GPIO DT binding. Reading a number from DT is not a proper
way to get a GPIO number, as you may have more than one GPIO controller
in a system and it is not obvious to which controller this number belongs,
or if you need to specify things like polarity.
Arnd
Hi Rob/Arnd,
> On Mon, Feb 08, 2016 at 01:11:03PM +0100, Arnd Bergmann wrote:
> > On Monday 08 February 2016 02:15:27 Amitkumar Karwar wrote:
> > > if (adapter->dt_node) {
> > > + if (of_property_read_u32(adapter->dt_node,
> > > + "mwifiex,chip-
> gpio",
> > > + &data) == 0) {
> > > + mwifiex_dbg(adapter, INFO,
> > > + "chip_gpio = 0x%x\n",
> data);
> > > + adapter->hs_cfg.gpio = data;
> > > + }
> > > +
> > >
> >
> > Please use the GPIO DT binding. Reading a number from DT is not a
> > proper way to get a GPIO number, as you may have more than one GPIO
> > controller in a system and it is not obvious to which controller this
> > number belongs, or if you need to specify things like polarity.
>
> My read of this is it is not the host SOC gpio, but the WiFi device's
> GPIO number. The host GPIO is defined in patch 3. We could still use the
> GPIO binding to describe it doing something like "marvell,<wifi gpio pin
> name>-gpios". Then the assignment is based on the property name.
>
Yes. This is not host SOC gpio. It's wifi chip's gpio number.
We will use GPIO binding for this in updated version.
Regards,
Amitkumar Karwar
Hi Arnd,
Thanks for your review.
> From: [email protected] [mailto:linux-wireless-
> [email protected]] On Behalf Of Arnd Bergmann
> Sent: Monday, February 08, 2016 5:40 PM
> To: Amitkumar Karwar
> Cc: [email protected]; Nishant Sarmukadam;
> [email protected]; [email protected]; Xinming Hu
> Subject: Re: [PATCH v3 1/3] mwifiex: register platform specific driver
>
> On Monday 08 February 2016 02:15:26 Amitkumar Karwar wrote:
> > diff --git a/Documentation/devicetree/bindings/mwifiex.txt
> > b/Documentation/devicetree/bindings/mwifiex.txt
> > new file mode 100644
> > index 0000000..39b6a74
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mwifiex.txt
> > @@ -0,0 +1,29 @@
> > +mwifiex
> > +------
> > +
> > +Required properties:
> > +
> > + - name : must be "mwifiex"
>
> This is a very unusual requirement.
>
> Can you use one of the standard device names instead?
>
We will make use of marvell-sd8xxx in updated version as per Rob Herring's suggestions.
It includes vendor name and a chipset/device name.
> > + - compatible : must be "marvell,mwifiex"
> > +
> > +Optional properties:
> > +
> > + - mwifiex,caldata* : A series of properties with marvell,caldata
> prefix,
> > + represent Calibration data downloaded to the
> device during
> > + initialization. This is an array of unsigned
> values.
> > +
> > +
> > +Example:
> > +
> > +Tx power limit calibration data is configured in below example.
> > +The calibration data is an array of unsigned values, the length can
> > +vary between hw versions.
> > +
> > +mwifiex {
> > + compatible = "marvell,mwifiex";
> > +
> > + mwifiex,caldata_00_txpwrlimit_2g_cfg_set = /bits/ 8 <
> > +0x01 0x00 0x06 0x00 0x08 0x02 0x89 0x01 ...>;
> > +
> > +};
>
> Should we list the mac-address here as well, or is there always an
> eeprom for that but not the calibration data?
Yes. we do get MAC address along with other info from EEPROM.
Regards,
Amitkumar Karwar
On Monday 08 February 2016 02:15:28 Amitkumar Karwar wrote:
> @@ -30,6 +84,44 @@ static int mwifiex_plt_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int mwifiex_plt_suspend(struct device *dev)
> +{
> + struct mwifiex_wake_dev *ctx = dev_get_drvdata(dev);
> + int ret;
> +
...
> +static const struct dev_pm_ops mwifiex_plt_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(mwifiex_plt_suspend, mwifiex_plt_resume)
> +};
> +#endif /* CONFIG_PM_SLEEP */
> +
> static const struct of_device_id mwifiex_dt_match[] = {
> {
> .compatible = "marvell,mwifiex",
> @@ -45,6 +137,9 @@ static struct platform_driver mwifiex_platform_driver = {
> .driver = {
> .name = "mwifiex_plt",
> .of_match_table = mwifiex_dt_match,
> +#ifdef CONFIG_PM_SLEEP
> + .pm = &mwifiex_plt_pm_ops,
> +#endif
> }
> };
Just remove the #ifdef here, and mark the functions as __maybe_unused.
Arnd
On Monday 08 February 2016 02:15:26 Amitkumar Karwar wrote:
> diff --git a/Documentation/devicetree/bindings/mwifiex.txt b/Documentation/devicetree/bindings/mwifiex.txt
> new file mode 100644
> index 0000000..39b6a74
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mwifiex.txt
> @@ -0,0 +1,29 @@
> +mwifiex
> +------
> +
> +Required properties:
> +
> + - name : must be "mwifiex"
This is a very unusual requirement.
Can you use one of the standard device names instead?
> + - compatible : must be "marvell,mwifiex"
> +
> +Optional properties:
> +
> + - mwifiex,caldata* : A series of properties with marvell,caldata prefix,
> + represent Calibration data downloaded to the device during
> + initialization. This is an array of unsigned values.
> +
> +
> +Example:
> +
> +Tx power limit calibration data is configured in below example.
> +The calibration data is an array of unsigned values, the length
> +can vary between hw versions.
> +
> +mwifiex {
> + compatible = "marvell,mwifiex";
> +
> + mwifiex,caldata_00_txpwrlimit_2g_cfg_set = /bits/ 8 <
> +0x01 0x00 0x06 0x00 0x08 0x02 0x89 0x01 ...>;
> +
> +};
Should we list the mac-address here as well, or is there always an eeprom
for that but not the calibration data?
Arnd