Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1485477 > unrolled thread

[PATCH 0/2] power: bq27xxx_battery: add configurable poll_interval

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] power: bq27xxx_battery: add configurable poll_interval Matt Ranostay <mranostay@gmail.com> - 2016-09-17 05:50 +0200
    [PATCH 2/2] power: bq27xxx_battery: allow kernel poll_interval parameter runtime update Matt Ranostay <mranostay@gmail.com> - 2016-09-17 05:50 +0200
      Re: [PATCH 2/2] power: bq27xxx_battery: allow kernel poll_interval  parameter runtime update Sebastian Reichel <sre@kernel.org> - 2016-09-19 21:50 +0200
    [PATCH 1/2] power: bq27xxx_battery: add configurable poll_interval by sysfs Matt Ranostay <mranostay@gmail.com> - 2016-09-17 05:50 +0200
      Re: [PATCH 1/2] power: bq27xxx_battery: add configurable  poll_interval by sysfs Sebastian Reichel <sre@kernel.org> - 2016-09-19 21:50 +0200
        Re: [PATCH 1/2] power: bq27xxx_battery: add configurable  poll_interval by sysfs Matt Ranostay <mranostay@gmail.com> - 2016-09-20 05:30 +0200

#1485477 — [PATCH 0/2] power: bq27xxx_battery: add configurable poll_interval

Patchset series fixes the kernel_parameter poll_interval not scheduling
the next event with the new interval. Also creates a sysfs entry that is
useful for udev attribute assignment.

Matt Ranostay (2):
  power: bq27xxx_battery: add configurable poll_interval by sysfs
  power: bq27xxx_battery: allow kernel poll_interval parameter runtime
    update

 drivers/power/supply/bq27xxx_battery.c | 87 +++++++++++++++++++++++++++++++++-
 include/linux/power/bq27xxx_battery.h  |  1 +
 2 files changed, 86 insertions(+), 2 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1485478 — [PATCH 2/2] power: bq27xxx_battery: allow kernel poll_interval parameter runtime update

Fix issue with poll_interval being not updated till the previous
interval expired.

Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
---
 drivers/power/supply/bq27xxx_battery.c | 38 +++++++++++++++++++++++++++++++++-
 include/linux/power/bq27xxx_battery.h  |  1 +
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 955424e10ae2..96fd3eec98da 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -39,6 +39,7 @@
 
 #include <linux/device.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/param.h>
 #include <linux/jiffies.h>
 #include <linux/workqueue.h>
@@ -390,8 +391,35 @@ static struct {
 	BQ27XXX_PROP(BQ27421, bq27421_battery_props),
 };
 
+static DEFINE_MUTEX(param_lock);
+static LIST_HEAD(bq27xxx_battery_devices);
+
+static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
+{
+	struct bq27xxx_device_info *di;
+	int ret;
+
+	ret = param_set_uint(val, kp);
+	if (ret < 0)
+		return ret;
+
+	mutex_lock(&param_lock);
+	list_for_each_entry(di, &bq27xxx_battery_devices, list) {
+		cancel_delayed_work_sync(&di->work);
+		schedule_delayed_work(&di->work, 0);
+	}
+	mutex_unlock(&param_lock);
+
+	return ret;
+}
+
+static const struct kernel_param_ops param_ops_poll_interval = {
+	.get = param_get_ushort,
+	.set = poll_interval_param_set,
+};
+
 static unsigned int poll_interval = 360;
-module_param(poll_interval, uint, 0644);
+module_param_cb(poll_interval, &param_ops_poll_interval, &poll_interval, 0644);
 MODULE_PARM_DESC(poll_interval,
 		 "battery poll interval in seconds - 0 disables polling");
 
@@ -1011,6 +1039,10 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 
 	bq27xxx_battery_update(di);
 
+	mutex_lock(&param_lock);
+	list_add(&di->list, &bq27xxx_battery_devices);
+	mutex_unlock(&param_lock);
+
 	return 0;
 
 err_out:
@@ -1036,6 +1068,10 @@ void bq27xxx_battery_teardown(struct bq27xxx_device_info *di)
 
 	power_supply_unregister(di->bat);
 
+	mutex_lock(&param_lock);
+	list_del(&di->list);
+	mutex_unlock(&param_lock);
+
 	mutex_destroy(&di->lock);
 }
 EXPORT_SYMBOL_GPL(bq27xxx_battery_teardown);
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index b50c0492629d..e30deb046156 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -58,6 +58,7 @@ struct bq27xxx_device_info {
 	unsigned long last_update;
 	struct delayed_work work;
 	struct power_supply *bat;
+	struct list_head list;
 	struct mutex lock;
 	u8 *regs;
 };
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1486800 — Re: [PATCH 2/2] power: bq27xxx_battery: allow kernel poll_interval parameter runtime update

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Fri, Sep 16, 2016 at 08:42:55PM -0700, Matt Ranostay wrote:
> Fix issue with poll_interval being not updated till the previous
> interval expired.
> 
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 38 +++++++++++++++++++++++++++++++++-
>  include/linux/power/bq27xxx_battery.h  |  1 +
>  2 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 955424e10ae2..96fd3eec98da 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -39,6 +39,7 @@
>  
>  #include <linux/device.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/param.h>
>  #include <linux/jiffies.h>
>  #include <linux/workqueue.h>
> @@ -390,8 +391,35 @@ static struct {
>  	BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>  };
>  
> +static DEFINE_MUTEX(param_lock);
> +static LIST_HEAD(bq27xxx_battery_devices);
> +
> +static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
> +{
> +	struct bq27xxx_device_info *di;
> +	int ret;
> +
> +	ret = param_set_uint(val, kp);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_lock(&param_lock);
> +	list_for_each_entry(di, &bq27xxx_battery_devices, list) {
> +		cancel_delayed_work_sync(&di->work);
> +		schedule_delayed_work(&di->work, 0);
> +	}
> +	mutex_unlock(&param_lock);
> +
> +	return ret;
> +}
> +
> +static const struct kernel_param_ops param_ops_poll_interval = {
> +	.get = param_get_ushort,

param_get_uint?

> +	.set = poll_interval_param_set,
> +};
> +
>  static unsigned int poll_interval = 360;
> -module_param(poll_interval, uint, 0644);
> +module_param_cb(poll_interval, &param_ops_poll_interval, &poll_interval, 0644);
>  MODULE_PARM_DESC(poll_interval,
>  		 "battery poll interval in seconds - 0 disables polling");
>  
> @@ -1011,6 +1039,10 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>  
>  	bq27xxx_battery_update(di);
>  
> +	mutex_lock(&param_lock);
> +	list_add(&di->list, &bq27xxx_battery_devices);
> +	mutex_unlock(&param_lock);
> +
>  	return 0;
>  
>  err_out:
> @@ -1036,6 +1068,10 @@ void bq27xxx_battery_teardown(struct bq27xxx_device_info *di)
>  
>  	power_supply_unregister(di->bat);
>  
> +	mutex_lock(&param_lock);
> +	list_del(&di->list);
> +	mutex_unlock(&param_lock);
> +
>  	mutex_destroy(&di->lock);
>  }
>  EXPORT_SYMBOL_GPL(bq27xxx_battery_teardown);
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index b50c0492629d..e30deb046156 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -58,6 +58,7 @@ struct bq27xxx_device_info {
>  	unsigned long last_update;
>  	struct delayed_work work;
>  	struct power_supply *bat;
> +	struct list_head list;
>  	struct mutex lock;
>  	u8 *regs;
>  };

Looks fine otherwise. When you redo the patch it would be nice to
rename param_lock into something like bq27xxx_list_lock instead,
though.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1485479 — [PATCH 1/2] power: bq27xxx_battery: add configurable poll_interval by sysfs

Allow the poll_interval to be runtime configurable via an sysfs entry.
This is needed for udev control of the poll interval.

Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
---
 drivers/power/supply/bq27xxx_battery.c | 48 +++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 3f57dd54803a..955424e10ae2 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -395,6 +395,36 @@ module_param(poll_interval, uint, 0644);
 MODULE_PARM_DESC(poll_interval,
 		 "battery poll interval in seconds - 0 disables polling");
 
+
+static ssize_t show_poll_interval(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", poll_interval);
+}
+
+static ssize_t store_poll_interval(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t size)
+{
+	struct bq27xxx_device_info *di = dev_get_drvdata(dev);
+	int tmp = poll_interval;
+
+	if (sscanf(buf, "%d\n", &poll_interval) != 1)
+		return -EINVAL;
+
+	if (poll_interval < 0)
+		return -EINVAL;
+
+	if (tmp != poll_interval) {
+		cancel_delayed_work_sync(&di->work);
+		schedule_delayed_work(&di->work, 0);
+	}
+
+	return size;
+}
+
+static DEVICE_ATTR(poll_interval, 0644, show_poll_interval, store_poll_interval);
+
 /*
  * Common code for BQ27xxx devices
  */
@@ -946,6 +976,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 {
 	struct power_supply_desc *psy_desc;
 	struct power_supply_config psy_cfg = { .drv_data = di, };
+	int ret;
 
 	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
 	mutex_init(&di->lock);
@@ -961,11 +992,19 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 	psy_desc->num_properties = bq27xxx_battery_props[di->chip].size;
 	psy_desc->get_property = bq27xxx_battery_get_property;
 	psy_desc->external_power_changed = bq27xxx_external_power_changed;
+	dev_set_drvdata(di->dev, di);
+
+	ret = sysfs_create_file(&di->dev->kobj, &dev_attr_poll_interval.attr);
+	if (ret) {
+		dev_err(di->dev, "failed to register poll_interval sysfs entry");
+		return ret;
+	}
 
 	di->bat = power_supply_register_no_ws(di->dev, psy_desc, &psy_cfg);
 	if (IS_ERR(di->bat)) {
 		dev_err(di->dev, "failed to register battery\n");
-		return PTR_ERR(di->bat);
+		ret = PTR_ERR(di->bat);
+		goto err_out;
 	}
 
 	dev_info(di->dev, "support ver. %s enabled\n", DRIVER_VERSION);
@@ -973,6 +1012,11 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 	bq27xxx_battery_update(di);
 
 	return 0;
+
+err_out:
+	sysfs_remove_file(&di->dev->kobj, &dev_attr_poll_interval.attr);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(bq27xxx_battery_setup);
 
@@ -988,6 +1032,8 @@ void bq27xxx_battery_teardown(struct bq27xxx_device_info *di)
 
 	cancel_delayed_work_sync(&di->work);
 
+	sysfs_remove_file(&di->dev->kobj, &dev_attr_poll_interval.attr);
+
 	power_supply_unregister(di->bat);
 
 	mutex_destroy(&di->lock);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1486801 — Re: [PATCH 1/2] power: bq27xxx_battery: add configurable poll_interval by sysfs

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Fri, Sep 16, 2016 at 08:42:54PM -0700, Matt Ranostay wrote:
> Allow the poll_interval to be runtime configurable via an sysfs
> entry.  This is needed for udev control of the poll interval.
> 
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 48 +++++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)

New sysfs attributes should be documented in Documentation/ABI.

Also I'm not too keen to add this, as there is already the sysfs
entry for the module parameter. I don't see any reason why udev
should not be able to change that value, so fix udev instead of
duplicating functionality in the kernel.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1487040 — Re: [PATCH 1/2] power: bq27xxx_battery: add configurable poll_interval by sysfs

On Mon, Sep 19, 2016 at 12:46 PM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Fri, Sep 16, 2016 at 08:42:54PM -0700, Matt Ranostay wrote:
>> Allow the poll_interval to be runtime configurable via an sysfs
>> entry.  This is needed for udev control of the poll interval.
>>
>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>> ---
>>  drivers/power/supply/bq27xxx_battery.c | 48 +++++++++++++++++++++++++++++++++-
>>  1 file changed, 47 insertions(+), 1 deletion(-)
>
> New sysfs attributes should be documented in Documentation/ABI.

Yeah I should know better :)

>
> Also I'm not too keen to add this, as there is already the sysfs
> entry for the module parameter. I don't see any reason why udev
> should not be able to change that value, so fix udev instead of
> duplicating functionality in the kernel.

Yeah duplication is bad.  We are wondering if having a
POWER_SUPPLY_PROP_UPDATE_INTERVAL would be an more acceptable
solution. Of course this would need to be made generic and not a per
driver solution as it is now.

Thanks,

Matt

>
> -- Sebastian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web