diff mbox

[v3,2/3] mwifiex: parse chip specific gpio from device tree

Message ID 1454926528-17480-2-git-send-email-akarwar@marvell.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show

Commit Message

Amitkumar Karwar Feb. 8, 2016, 10:15 a.m. UTC 
From: Xinming Hu <huxm@marvell.com>

This patch parse chip specific gpio parameter from device
tree. Corresponding binding file is also updated.

Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
 Documentation/devicetree/bindings/mwifiex.txt  | 6 ++++--
 drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 9 +++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

Comments

Arnd Bergmann Feb. 8, 2016, 12:11 p.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring (Arm) Feb. 8, 2016, 9:47 p.m. UTC | #2 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amitkumar Karwar Feb. 9, 2016, 2:03 p.m. UTC | #3 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Feb. 9, 2016, 2:19 p.m. UTC | #4 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amitkumar Karwar Feb. 9, 2016, 2:26 p.m. UTC | #5 
> > > > 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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)