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

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

[Matt Ranostay]

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

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

[Matt Ranostay]

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

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Matt Ranostay]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3712 bytes --]

On Fri 2016-09-16 20:42:54, 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>

working mail address would be nice here.

sysfs files should be documented.

Also... what is it good for?

Do you have a device that needs non-standard interval?
									Pavel

> ---
>  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);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Matt Ranostay]

On Fri, Oct 7, 2016 at 11:49 AM, Pavel Machek <pavel@ucw.cz> wrote:
> On Fri 2016-09-16 20:42:54, 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>
>
> working mail address would be nice here.
>
> sysfs files should be documented.
>

Ok can do in next revision

> Also... what is it good for?
>
> Do you have a device that needs non-standard interval?

Basically we need to have the ability to dynamically change the
intervals. So closer to a battery drain we need to up the reporting
intervals.

Thanks,

Matt

>                                                                         Pavel
>
>> ---
>>  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);
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]


> Ok can do in next revision
> 
> > Also... what is it good for?
> >
> > Do you have a device that needs non-standard interval?
> 
> Basically we need to have the ability to dynamically change the
> intervals. So closer to a battery drain we need to up the reporting
> intervals.

Could you elaborate on that? What intervals do you normally need, what
do you need around empty battery, and why is that?

I'm thinking about adding kernel thread to shut down the system if
battery goes critically empty. It is kernel job to protect the
hardware, and that should include the battery...

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

On Sun 2016-10-23 20:08:22, Matt Ranostay wrote:
> On Fri, Oct 7, 2016 at 11:49 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > On Fri 2016-09-16 20:42:54, 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>
> >
> > working mail address would be nice here.
> >
> > sysfs files should be documented.
> >
> 
> Ok can do in next revision
> 
> > Also... what is it good for?
> >
> > Do you have a device that needs non-standard interval?
> 
> Basically we need to have the ability to dynamically change the
> intervals. So closer to a battery drain we need to up the reporting
> intervals.

Umm, there seems to be mechanism there to change that already...?

static const struct kernel_param_ops param_ops_poll_interval = {
        .get = param_get_uint,
 	.set = poll_interval_param_set,
};
		

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Matt Ranostay]

On Mon, Oct 24, 2016 at 12:51 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Sun 2016-10-23 20:08:22, Matt Ranostay wrote:
>> On Fri, Oct 7, 2016 at 11:49 AM, Pavel Machek <pavel@ucw.cz> wrote:
>> > On Fri 2016-09-16 20:42:54, 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>
>> >
>> > working mail address would be nice here.
>> >
>> > sysfs files should be documented.
>> >
>>
>> Ok can do in next revision
>>
>> > Also... what is it good for?
>> >
>> > Do you have a device that needs non-standard interval?
>>
>> Basically we need to have the ability to dynamically change the
>> intervals. So closer to a battery drain we need to up the reporting
>> intervals.
>
> Umm, there seems to be mechanism there to change that already...?
>

Ah right. Commenting on the wrong patchset oops :).

> static const struct kernel_param_ops param_ops_poll_interval = {
>         .get = param_get_uint,
>         .set = poll_interval_param_set,
> };
>
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Matt Ranostay]

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

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3185 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]