Re: [PATCH] mwifiex: fix compile warning of unused variable

[Shawn Lin]

Hi Kalle,

On 2017/7/6 15:57, Kalle Valo wrote:

Shawn Lin <shawn@rock-chips.com> writes:


We got a compile warning shows below:

drivers/net/wireless/marvell/mwifiex/sdio.c: In function
'mwifiex_sdio_remove':
drivers/net/wireless/marvell/mwifiex/sdio.c:377:6: warning: variable
'ret' set but not used [-Wunused-but-set-variable]

Per the code, it didn't check if mwifiex_sdio_read_fw_status
finish successfully. We should at least check the return of
mwifiex_sdio_read_fw_status, otherwise the following check of
firmware_stat and adapter->mfg_mode is pointless as the device
is probably dead.


I don't see that warning, any ideas why? My compiler:

gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609


I was using gcc-arm-eabi-4.8- , but it doesn't matter.

Could you add W=1 to compile the code, for instance, "make W=1  -j32"

[PATCH] mwifiex: fix compile warning of unused variable

[Shawn Lin]

We got a compile warning shows below:

drivers/net/wireless/marvell/mwifiex/sdio.c: In function
'mwifiex_sdio_remove':
drivers/net/wireless/marvell/mwifiex/sdio.c:377:6: warning: variable
'ret' set but not used [-Wunused-but-set-variable]

Per the code, it didn't check if mwifiex_sdio_read_fw_status
finish successfully. We should at least check the return of
mwifiex_sdio_read_fw_status, otherwise the following check of
firmware_stat and adapter->mfg_mode is pointless as the device
is probably dead.

Signed-off-by: Shawn Lin <shawn@rock-chips.com>
---

 drivers/net/wireless/marvell/mwifiex/sdio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index f81a006..fd5183c 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -390,7 +390,8 @@ static int mwifiex_check_winner_status(struct 
mwifiex_adapter *adapter)
mwifiex_dbg(adapter, INFO, "info: SDIO func num=%d\n", func->num);
 
ret = mwifiex_sdio_read_fw_status(adapter, _stat);
-   if (firmware_stat == FIRMWARE_READY_SDIO && !adapter->mfg_mode) {
+   if (!ret && firmware_stat == FIRMWARE_READY_SDIO &&
+   !adapter->mfg_mode) {
mwifiex_deauthenticate_all(adapter);
 
priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
-- 
1.9.1

[PATCH] mwifiex: debugfs: remove redunant check of mwifiex_dfs_dir

[Shawn Lin]

debugfs_remove already check mwifiex_dfs_dir, so remove it.

Signed-off-by: Shawn Lin <shawn@rock-chips.com>
---

 drivers/net/wireless/marvell/mwifiex/debugfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c 
b/drivers/net/wireless/marvell/mwifiex/debugfs.c
index ae2b69d..f6f105a 100644
--- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
+++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
@@ -1046,6 +1046,5 @@
 void
 mwifiex_debugfs_remove(void)
 {
-   if (mwifiex_dfs_dir)
-   debugfs_remove(mwifiex_dfs_dir);
+   debugfs_remove(mwifiex_dfs_dir);
 }
-- 
1.9.1

[PATCH] mwifiex: simplify the code around ra_list

[Shawn Lin]

We don't need to check if the list is empty separately
as we could use list_first_entry_or_null to cover it.

Signed-off-by: Shawn Lin <shawn@rock-chips.com>
---

 drivers/net/wireless/marvell/mwifiex/tdls.c | 7 ++-
 drivers/net/wireless/marvell/mwifiex/wmm.c  | 8 ++--
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c 
b/drivers/net/wireless/marvell/mwifiex/tdls.c
index 7d0d3ff..d76ce87 100644
--- a/drivers/net/wireless/marvell/mwifiex/tdls.c
+++ b/drivers/net/wireless/marvell/mwifiex/tdls.c
@@ -55,11 +55,8 @@ static void mwifiex_restore_tdls_packets(struct 
mwifiex_private *priv,
tx_info->flags |= MWIFIEX_BUF_FLAG_TDLS_PKT;
} else {
tid_list = >wmm.tid_tbl_ptr[tid_down].ra_list;
-   if (!list_empty(tid_list))
-   ra_list = list_first_entry(tid_list,
- struct mwifiex_ra_list_tbl, list);
-   else
-   ra_list = NULL;
+   ra_list = list_first_entry_or_null(tid_list,
+   struct mwifiex_ra_list_tbl, list);
tx_info->flags &= ~MWIFIEX_BUF_FLAG_TDLS_PKT;
}
 
diff --git a/drivers/net/wireless/marvell/mwifiex/wmm.c 
b/drivers/net/wireless/marvell/mwifiex/wmm.c
index e4ff3b9..744b014 100644
--- a/drivers/net/wireless/marvell/mwifiex/wmm.c
+++ b/drivers/net/wireless/marvell/mwifiex/wmm.c
@@ -868,12 +868,8 @@ struct mwifiex_ra_list_tbl *
return;
default:
list_head = priv->wmm.tid_tbl_ptr[tid_down].ra_list;
-   if (!list_empty(_head))
-   ra_list = list_first_entry(
-   _head, struct mwifiex_ra_list_tbl,
-   list);
-   else
-   ra_list = NULL;
+   ra_list = list_first_entry_or_null(_head,
+   struct mwifiex_ra_list_tbl, list);
break;
}
} else {
-- 
1.9.1

Re: [PATCH v3 2/2] mmc: pwrseq: add support for Marvell SD8787 chip

[Shawn Lin]

On 2017/1/16 5:41, Matt Ranostay wrote:

On Thu, Jan 12, 2017 at 11:16 PM, Shawn Lin <shawn@rock-chips.com> wrote:

On 2017/1/13 13:29, Matt Ranostay wrote:


Allow power sequencing for the Marvell SD8787 Wifi/BT chip.
This can be abstracted to other chipsets if needed in the future.

Cc: Tony Lindgren <t...@atomide.com>
Cc: Ulf Hansson <ulf.hans...@linaro.org>
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
---
 drivers/mmc/core/Kconfig |  10 
 drivers/mmc/core/Makefile|   1 +
 drivers/mmc/core/pwrseq_sd8787.c | 117
+++
 3 files changed, 128 insertions(+)
 create mode 100644 drivers/mmc/core/pwrseq_sd8787.c

diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
index cdfa8520a4b1..fc1ecdaaa9ca 100644
--- a/drivers/mmc/core/Kconfig
+++ b/drivers/mmc/core/Kconfig
@@ -12,6 +12,16 @@ config PWRSEQ_EMMC
  This driver can also be built as a module. If so, the module
  will be called pwrseq_emmc.

+config PWRSEQ_SD8787
+   tristate "HW reset support for SD8787 BT + Wifi module"
+   depends on OF && (MWIFIEX || BT_MRVL_SDIO)
+   help
+ This selects hardware reset support for the SD8787 BT + Wifi
+ module. By default this option is set to n.
+
+ This driver can also be built as a module. If so, the module
+ will be called pwrseq_sd8787.
+



I don't like this way, as we have a chance to list lots
configure options here. wifi A,B,C,D...Z, all of them need a
new section here if needed?

Instead, could you just extent pwrseq_simple.c and add you
.compatible = "mmc-pwrseq-sd8787", "mmc-pwrseq-simple"?


You mean all the chipset pwrseqs linked into the pwrseq-simple module?


What I mean was if you just extent the pwrseq-simple a bit, you could
just add your chipset pwrseqs linked into the pwrseq-simple. But if you
need a different *pattern* of pwrseqs, you should need a new name, for
instance, pwrseq-sdio.c etc... But please don't use the name of
sd8787? So if I use a wifi named ABC but using the same pwrseq pattenr,
should I include your "mmc-pwrseq- sd8787" for that or I need a new
mmc-pwrseq-ABC.c?



Ulf your thoughts on this?






 config PWRSEQ_SIMPLE
tristate "Simple HW reset support for MMC"
default y
diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
index b2a257dc644f..0f81464fa824 100644
--- a/drivers/mmc/core/Makefile
+++ b/drivers/mmc/core/Makefile
@@ -10,6 +10,7 @@ mmc_core-y:= core.o bus.o host.o \
   quirks.o slot-gpio.o
 mmc_core-$(CONFIG_OF)  += pwrseq.o
 obj-$(CONFIG_PWRSEQ_SIMPLE)+= pwrseq_simple.o
+obj-$(CONFIG_PWRSEQ_SD8787)+= pwrseq_sd8787.o
 obj-$(CONFIG_PWRSEQ_EMMC)  += pwrseq_emmc.o
 mmc_core-$(CONFIG_DEBUG_FS)+= debugfs.o
 obj-$(CONFIG_MMC_BLOCK)+= mmc_block.o
diff --git a/drivers/mmc/core/pwrseq_sd8787.c
b/drivers/mmc/core/pwrseq_sd8787.c
new file mode 100644
index ..f4080fe6439e
--- /dev/null
+++ b/drivers/mmc/core/pwrseq_sd8787.c
@@ -0,0 +1,117 @@
+/*
+ * pwrseq_sd8787.c - power sequence support for Marvell SD8787 BT + Wifi
chip
+ *
+ * Copyright (C) 2016 Matt Ranostay <matt@ranostay.consulting>
+ *
+ * Based on the original work pwrseq_simple.c
+ *  Copyright (C) 2014 Linaro Ltd
+ *  Author: Ulf Hansson <ulf.hans...@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include "pwrseq.h"
+
+struct mmc_pwrseq_sd8787 {
+   struct mmc_pwrseq pwrseq;
+   struct gpio_desc *reset_gpio;
+   struct gpio_desc *pwrdn_gpio;
+};
+
+#define to_pwrseq_sd8787(p) container_of(p, struct mmc_pwrseq_sd8787,
pwrseq)
+
+static void mmc_pwrseq_sd8787_pre_power_on(struct mmc_host *host)
+{
+   struct mmc_pwrseq_sd8787 *pwrseq = to_pwrseq_sd8787(host->pwrseq);
+
+   gpiod_set_value_cansleep(pwrseq->reset_gpio, 1);
+
+   msleep(300);
+   gpiod_set_value_cansleep(pwrseq->pwrdn_gpio, 1);
+}
+
+static void mmc_pwrseq_sd8787_power_off(struct mmc_host *host)
+{
+   struct mmc_pwrseq_sd8787 *pwrseq = to_pwrseq_sd8787(host->pwrseq);
+
+   gpiod_set_value_cansleep(pwrseq->pwrdn_gpio, 0);
+   gpiod_set_value_cansleep(pwrseq->reset_gpio, 0);
+}
+
+static const struct mmc_pwrseq_ops mmc_pwrs

Re: [PATCH v3 2/2] mmc: pwrseq: add support for Marvell SD8787 chip

[Shawn Lin]

D_OUT_LOW);
+   if (IS_ERR(pwrseq->pwrdn_gpio))
+   return PTR_ERR(pwrseq->pwrdn_gpio);
+
+   pwrseq->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+   if (IS_ERR(pwrseq->reset_gpio))
+   return PTR_ERR(pwrseq->reset_gpio);
+
+   pwrseq->pwrseq.dev = dev;
+   pwrseq->pwrseq.ops = _pwrseq_sd8787_ops;
+   pwrseq->pwrseq.owner = THIS_MODULE;
+   platform_set_drvdata(pdev, pwrseq);
+
+   return mmc_pwrseq_register(>pwrseq);
+}
+
+static int mmc_pwrseq_sd8787_remove(struct platform_device *pdev)
+{
+   struct mmc_pwrseq_sd8787 *pwrseq = platform_get_drvdata(pdev);
+
+   mmc_pwrseq_unregister(>pwrseq);
+
+   return 0;
+}
+
+static struct platform_driver mmc_pwrseq_sd8787_driver = {
+   .probe = mmc_pwrseq_sd8787_probe,
+   .remove = mmc_pwrseq_sd8787_remove,
+   .driver = {
+   .name = "pwrseq_sd8787",
+   .of_match_table = mmc_pwrseq_sd8787_of_match,
+   },
+};
+
+module_platform_driver(mmc_pwrseq_sd8787_driver);
+MODULE_LICENSE("GPL v2");




--
Best Regards
Shawn Lin

Re: [PATCH v3 1/2] mmc: API for accessing host supported maximum segment count and size

[Shawn Lin]

Hi

On 2016-6-6 19:53, Amitkumar Karwar wrote:

From: Xinming Hu 

sdio device drivers need be able to get the host supported max_segs
and max_seg_size, so that they know the buffer size to allocate while
utilizing the scatter/gather DMA buffer list.

This patch provides API for this purpose.

Signed-off-by: Xinming Hu 
Signed-off-by: Amitkumar Karwar 
---
v2: v2 was submitted with minor improvement like replacing BUG_ON() with 
WARN_ON()
v3: Addressed below review comments from Ulf Hansson
 a) In v3, patch has been split into two separate patches.
 b) Patch 1/2 introduces an API to fetch max_seg_size and max_segs
 c) Replaced WARN_ON() with proper error code when sg_ptr->length is invalid
 d) Instead of duplicating the code in mmc_io_rw_extended(), extra bool 
parameter
has been added to this function and used it in new APIs for SG.
---
  drivers/mmc/core/sdio_io.c | 27 ++
  .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c  |  6 ++---
  include/linux/mmc/sdio_func.h  |  3 +++
  3 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index 78cb4d5..a546c89 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -720,3 +720,30 @@ int sdio_set_host_pm_flags(struct sdio_func *func, 
mmc_pm_flag_t flags)
return 0;
  }
  EXPORT_SYMBOL_GPL(sdio_set_host_pm_flags);
+
+/**
+ * sdio_get_host_max_seg_size - get host maximum segment size
+ * @func: SDIO function attached to host
+ */
+unsigned int sdio_get_host_max_seg_size(struct sdio_func *func)
+{
+   WARN_ON(!func);
+   WARN_ON(!func->card);
+
+   return func->card->host->max_seg_size;
+}
+EXPORT_SYMBOL_GPL(sdio_get_host_max_seg_size);
+
+/**
+ * sdio_get_host_max_seg_count - get host maximum segment count
+ * @func: SDIO function attached to host
+ */
+unsigned short sdio_get_host_max_seg_count(struct sdio_func *func)
+{
+   WARN_ON(!func);
+   WARN_ON(!func->card);
+


I believe these two WARN_ON may be called too late because ...

drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c

void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev)

func = sdiodev->func[2];
host = func->card->host;

you have unconditionally thought it should be ready.


+   return func->card->host->max_segs;
+}
+EXPORT_SYMBOL_GPL(sdio_get_host_max_seg_count);
+
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index c4b89d2..ba579f4 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -896,9 +896,9 @@ void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev 
*sdiodev)
max_blocks = min_t(uint, host->max_blk_count, 511u);
sdiodev->max_request_size = min_t(uint, host->max_req_size,
  max_blocks * func->cur_blksize);
-   sdiodev->max_segment_count = min_t(uint, host->max_segs,
-  SG_MAX_SINGLE_ALLOC);
-   sdiodev->max_segment_size = host->max_seg_size;
+   sdiodev->max_segment_count = min_t(uint, SG_MAX_SINGLE_ALLOC,
+  sdio_get_host_max_seg_count(func));
+   sdiodev->max_segment_size = sdio_get_host_max_seg_size(func);
  
  	if (!sdiodev->sg_support)

return;
diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
index aab032a..b2b91df 100644
--- a/include/linux/mmc/sdio_func.h
+++ b/include/linux/mmc/sdio_func.h
@@ -159,4 +159,7 @@ extern void sdio_f0_writeb(struct sdio_func *func, unsigned 
char b,
  extern mmc_pm_flag_t sdio_get_host_pm_caps(struct sdio_func *func);
  extern int sdio_set_host_pm_flags(struct sdio_func *func, mmc_pm_flag_t 
flags);
  
+unsigned short sdio_get_host_max_seg_count(struct sdio_func *func);

+unsigned int sdio_get_host_max_seg_size(struct sdio_func *func);
+
  #endif /* LINUX_MMC_SDIO_FUNC_H */



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