"Li, Yanbo" [off-list ref] writes:

quoted

-----Original Message-----
From: Valo, Kalle
Sent: Wednesday, May 27, 2015 5:57 AM
To: Li, Yanbo
Cc: dreamfly281@gmail.com; linux-wireless@vger.kernel.org;
michal.kazior@tieto.com; ath10k@lists.infradead.org
Subject: Re: [PATCH] ath10k: Debugfs entry to enable/disable BTC feature

Yanbo Li [off-list ref] writes:

quoted

As some radio have no connection with BT modules, enable the BTC
feature will has some side effect if the radio's GPIO connect with any
other HW modules. Add the control switcher "btc_feature" at debugfs
and set the feature as disable by default to avoid such case.

To enable this feature, execute:
echo 1 > /sys/kernel/debug/ieee80211/phyX/ath10k/btc_feature
To disable:
echo 0 > /sys/kernel/debug/ieee80211/phyX/ath10k/btc_feature

I suspect "BTC" does not really tell much for most of the people and
acronyms should be always spelled out at least once.

quoted

Signed-off-by: Yanbo Li <redacted>

diff --git a/drivers/net/wireless/ath/ath10k/core.h

b/drivers/net/wireless/ath/ath10k/core.h
index 8444adf42195..6852e7fc5d5f 100644

--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h

@@ -720,6 +720,8 @@ struct ath10k {
 		u32 fw_cold_reset_counter;
 	} stats;

+	bool btc_feature;

Could we use ar->dev_flags instead?

This dev_flags currently used to mark some status, like the cradh, CAC running, 
The BTcoex feature is more likely a HW feature,  there are seems different set. 

Maybe a separately hw_feature_flag is needed as there are some other HW feature 
That we want to enable/disable from user space. 

I think we don't need a separate bitmap, we can use dev_flags just fine
for this.

quoted

quoted

+static ssize_t ath10k_write_btc_feature(struct file *file,
+					const char __user *ubuf,
+					size_t count, loff_t *ppos)
+{
+	struct ath10k *ar = file->private_data;
+	char buf[32];
+	size_t buf_size;
+	bool val;
+
+	buf_size = min(count, (sizeof(buf) - 1));
+	if (copy_from_user(buf, ubuf, buf_size))
+		return -EFAULT;
+
+	buf[buf_size] = '\0';
+	if (strtobool(buf, &val) != 0) {
+		ath10k_warn(ar, "Wrong BTC feature setting\n");
+		return -EINVAL;
+	}
+
+	mutex_lock(&ar->conf_mutex);
+	ar->btc_feature = val;
+	queue_work(ar->workqueue, &ar->restart_work);
+	mutex_unlock(&ar->conf_mutex);

Shouldn't we test ATH10K_STATE_ON first?

The restart process will judge by itself whether the device is on or not, and print below warning in that case:

[797424.496190] ath10k_pci 0000:05:00.0: cannot restart a device that
hasn't been started

That's just buggy, ath10k should not print anything if a setting is
changed while interface is down. It's much better we have the check for
ATH10K_STATE_ON here.

-- 
Kalle Valo