diff mbox

[1/3] ath10k: add sysfs entry to configure quiet period

Message ID 1424437452-28161-1-git-send-email-rmanohar@qti.qualcomm.com (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show

Commit Message

Rajkumar Manoharan Feb. 20, 2015, 1:04 p.m. UTC
Add support to configure quiet period via sysfs entry. This will
be helpful to experiment different quiet period values along with
different duty cycle ratio.

To configure quiet period as 30ms,

echo 30 >/sys/class/ieee80211/phyX/device/quiet_period

Signed-off-by: Rajkumar Manoharan <rmanohar@qti.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/thermal.c | 57 +++++++++++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/thermal.h |  1 +
 2 files changed, 55 insertions(+), 3 deletions(-)

Comments

Kalle Valo March 10, 2015, 3:43 p.m. UTC | #1
Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:

> Add support to configure quiet period via sysfs entry. This will
> be helpful to experiment different quiet period values along with
> different duty cycle ratio.
>
> To configure quiet period as 30ms,
>
> echo 30 >/sys/class/ieee80211/phyX/device/quiet_period

What's the justification? To me this looks like an ugly driver private
hack. Why can't you use nl80211 or something else?
Rajkumar Manoharan March 10, 2015, 4:53 p.m. UTC | #2
On Tue, Mar 10, 2015 at 05:43:58PM +0200, Kalle Valo wrote:
> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
> 
> > Add support to configure quiet period via sysfs entry. This will
> > be helpful to experiment different quiet period values along with
> > different duty cycle ratio.
> >
> > To configure quiet period as 30ms,
> >
> > echo 30 >/sys/class/ieee80211/phyX/device/quiet_period
> 
> What's the justification? To me this looks like an ugly driver private
> hack. Why can't you use nl80211 or something else?

As this is purely for testing purpose to play around with different
quiet period along with various throttling state, sysfs entry is used
instead of netlink testmode command. Instead of debugfs, sysfs entry is
selected to align with existing thermal interface. 

-Rajkumar
--
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
Kalle Valo March 12, 2015, 1:21 p.m. UTC | #3
Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:

> On Tue, Mar 10, 2015 at 05:43:58PM +0200, Kalle Valo wrote:
>> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
>> 
>> > Add support to configure quiet period via sysfs entry. This will
>> > be helpful to experiment different quiet period values along with
>> > different duty cycle ratio.
>> >
>> > To configure quiet period as 30ms,
>> >
>> > echo 30 >/sys/class/ieee80211/phyX/device/quiet_period
>> 
>> What's the justification? To me this looks like an ugly driver private
>> hack. Why can't you use nl80211 or something else?
>
> As this is purely for testing purpose to play around with different
> quiet period along with various throttling state, sysfs entry is used
> instead of netlink testmode command. Instead of debugfs, sysfs entry is
> selected to align with existing thermal interface. 

Please correct me if I'm wrong, but I consider sysfs as a stable user
space interface. It's not meant for random testing stuff.
Rajkumar Manoharan March 12, 2015, 1:50 p.m. UTC | #4
On Thu, Mar 12, 2015 at 03:21:34PM +0200, Kalle Valo wrote:
> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
> 
> > On Tue, Mar 10, 2015 at 05:43:58PM +0200, Kalle Valo wrote:
> >> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
> >> 
> >> > Add support to configure quiet period via sysfs entry. This will
> >> > be helpful to experiment different quiet period values along with
> >> > different duty cycle ratio.
> >> >
> >> > To configure quiet period as 30ms,
> >> >
> >> > echo 30 >/sys/class/ieee80211/phyX/device/quiet_period
> >> 
> >> What's the justification? To me this looks like an ugly driver private
> >> hack. Why can't you use nl80211 or something else?
> >
> > As this is purely for testing purpose to play around with different
> > quiet period along with various throttling state, sysfs entry is used
> > instead of netlink testmode command. Instead of debugfs, sysfs entry is
> > selected to align with existing thermal interface. 
> 
> Please correct me if I'm wrong, but I consider sysfs as a stable user
> space interface. It's not meant for random testing stuff.
> 
Completely agree. Will move this change under debugfs. Does it sound
good?

-Rajkumar
--
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/drivers/net/wireless/ath/ath10k/thermal.c b/drivers/net/wireless/ath/ath10k/thermal.c
index 0d89ab5..8992c17 100644
--- a/drivers/net/wireless/ath/ath10k/thermal.c
+++ b/drivers/net/wireless/ath/ath10k/thermal.c
@@ -96,8 +96,7 @@  static int ath10k_thermal_set_cur_dutycycle(struct thermal_cooling_device *cdev,
 		ret = -ENETDOWN;
 		goto out;
 	}
-	period = max(ATH10K_QUIET_PERIOD_MIN,
-		     (ATH10K_QUIET_PERIOD_DEFAULT / num_bss));
+	period = ar->thermal.quiet_period;
 	duration = (period * (100 - duty_cycle)) / 100;
 	enabled = duration ? 1 : 0;
 
@@ -184,6 +183,48 @@  static struct attribute *ath10k_hwmon_attrs[] = {
 };
 ATTRIBUTE_GROUPS(ath10k_hwmon);
 
+static ssize_t ath10k_thermal_show_quiet_period(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	struct ath10k *ar = dev_get_drvdata(dev);
+	u32 period;
+
+	mutex_lock(&ar->conf_mutex);
+	period = ar->thermal.quiet_period;
+	mutex_unlock(&ar->conf_mutex);
+
+	return sprintf(buf, "%u\n", period);
+}
+
+static ssize_t ath10k_thermal_store_quiet_period(struct device *dev,
+						 struct device_attribute *attr,
+						 const char *buf, size_t count)
+{
+	struct ath10k *ar = dev_get_drvdata(dev);
+	u32 period;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &period);
+	if (ret)
+		return ret;
+
+	if (period < ATH10K_QUIET_PERIOD_MIN) {
+		ath10k_warn(ar, "Quiet period %u can not be lesser than 25ms\n",
+			    period);
+		return -EINVAL;
+	}
+	mutex_lock(&ar->conf_mutex);
+	ar->thermal.quiet_period = period;
+	mutex_unlock(&ar->conf_mutex);
+
+	return count;
+}
+
+static DEVICE_ATTR(quiet_period, S_IRUGO | S_IWUSR,
+		   ath10k_thermal_show_quiet_period,
+		   ath10k_thermal_store_quiet_period);
+
 int ath10k_thermal_register(struct ath10k *ar)
 {
 	struct thermal_cooling_device *cdev;
@@ -208,6 +249,13 @@  int ath10k_thermal_register(struct ath10k *ar)
 
 	ar->thermal.cdev = cdev;
 
+	ret = sysfs_create_file(&ar->dev->kobj, &dev_attr_quiet_period.attr);
+	if (ret) {
+		ath10k_err(ar, "failed to create quiet period sysfs entry\n");
+		goto err_remove_link;
+	}
+	ar->thermal.quiet_period = ATH10K_QUIET_PERIOD_DEFAULT;
+
 	/* Do not register hwmon device when temperature reading is not
 	 * supported by firmware
 	 */
@@ -226,10 +274,12 @@  int ath10k_thermal_register(struct ath10k *ar)
 		ath10k_err(ar, "failed to register hwmon device: %ld\n",
 			   PTR_ERR(hwmon_dev));
 		ret = -EINVAL;
-		goto err_remove_link;
+		goto err_remove_file;
 	}
 	return 0;
 
+err_remove_file:
+	sysfs_remove_file(&ar->dev->kobj, &dev_attr_quiet_period.attr);
 err_remove_link:
 	sysfs_remove_link(&ar->dev->kobj, "thermal_sensor");
 err_cooling_destroy:
@@ -241,4 +291,5 @@  void ath10k_thermal_unregister(struct ath10k *ar)
 {
 	thermal_cooling_device_unregister(ar->thermal.cdev);
 	sysfs_remove_link(&ar->dev->kobj, "cooling_device");
+	sysfs_remove_file(&ar->dev->kobj, &dev_attr_quiet_period.attr);
 }
diff --git a/drivers/net/wireless/ath/ath10k/thermal.h b/drivers/net/wireless/ath/ath10k/thermal.h
index 5e87d9a..050f41d 100644
--- a/drivers/net/wireless/ath/ath10k/thermal.h
+++ b/drivers/net/wireless/ath/ath10k/thermal.h
@@ -29,6 +29,7 @@  struct ath10k_thermal {
 
 	/* protected by conf_mutex */
 	u32 duty_cycle;
+	u32 quiet_period;
 	/* temperature value in Celcius degree
 	 * protected by data_lock
 	 */