Pavel + Sebastian this is the patchset that need I some input on :)
Thanks,
Matt
On Wed, Sep 28, 2016 at 12:55 PM, Matt Ranostay <[email protected]> wrote:
> Add POWER_SUPPLY_PROP_POLL_INTERVAL property to query and set the
> polling interval of the fuel gauge.
>
> Cc: Sebastian Reichel <[email protected]>
> Cc: Tony Lindgren <[email protected]
> Signed-off-by: Matt Ranostay <[email protected]>
> ---
> drivers/power/supply/bq27xxx_battery.c | 40 ++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 3b0dbc689d72..d18802ca3327 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -262,6 +262,7 @@ static enum power_supply_property bq27000_battery_props[] = {
> POWER_SUPPLY_PROP_POWER_AVG,
> POWER_SUPPLY_PROP_HEALTH,
> POWER_SUPPLY_PROP_MANUFACTURER,
> + POWER_SUPPLY_PROP_POLL_INTERVAL,
> };
>
> static enum power_supply_property bq27010_battery_props[] = {
> @@ -282,6 +283,7 @@ static enum power_supply_property bq27010_battery_props[] = {
> POWER_SUPPLY_PROP_CYCLE_COUNT,
> POWER_SUPPLY_PROP_HEALTH,
> POWER_SUPPLY_PROP_MANUFACTURER,
> + POWER_SUPPLY_PROP_POLL_INTERVAL,
> };
>
> static enum power_supply_property bq27500_battery_props[] = {
> @@ -300,6 +302,7 @@ static enum power_supply_property bq27500_battery_props[] = {
> POWER_SUPPLY_PROP_CYCLE_COUNT,
> POWER_SUPPLY_PROP_HEALTH,
> POWER_SUPPLY_PROP_MANUFACTURER,
> + POWER_SUPPLY_PROP_POLL_INTERVAL,
> };
>
> static enum power_supply_property bq27530_battery_props[] = {
> @@ -318,6 +321,7 @@ static enum power_supply_property bq27530_battery_props[] = {
> POWER_SUPPLY_PROP_HEALTH,
> POWER_SUPPLY_PROP_CYCLE_COUNT,
> POWER_SUPPLY_PROP_MANUFACTURER,
> + POWER_SUPPLY_PROP_POLL_INTERVAL,
> };
>
> static enum power_supply_property bq27541_battery_props[] = {
> @@ -337,6 +341,7 @@ static enum power_supply_property bq27541_battery_props[] = {
> POWER_SUPPLY_PROP_POWER_AVG,
> POWER_SUPPLY_PROP_HEALTH,
> POWER_SUPPLY_PROP_MANUFACTURER,
> + POWER_SUPPLY_PROP_POLL_INTERVAL,
> };
>
> static enum power_supply_property bq27545_battery_props[] = {
> @@ -355,6 +360,7 @@ static enum power_supply_property bq27545_battery_props[] = {
> POWER_SUPPLY_PROP_CYCLE_COUNT,
> POWER_SUPPLY_PROP_POWER_AVG,
> POWER_SUPPLY_PROP_MANUFACTURER,
> + POWER_SUPPLY_PROP_POLL_INTERVAL,
> };
>
> static enum power_supply_property bq27421_battery_props[] = {
> @@ -370,6 +376,7 @@ static enum power_supply_property bq27421_battery_props[] = {
> POWER_SUPPLY_PROP_CHARGE_NOW,
> POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> POWER_SUPPLY_PROP_MANUFACTURER,
> + POWER_SUPPLY_PROP_POLL_INTERVAL,
> };
>
> #define BQ27XXX_PROP(_id, _prop) \
> @@ -955,6 +962,9 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
> case POWER_SUPPLY_PROP_MANUFACTURER:
> val->strval = BQ27XXX_MANUFACTURER;
> break;
> + case POWER_SUPPLY_PROP_POLL_INTERVAL:
> + val->intval = poll_interval;
> + break;
> default:
> return -EINVAL;
> }
> @@ -962,6 +972,34 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
> return ret;
> }
>
> +static int bq27xxx_battery_set_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
> +
> + if (psp != POWER_SUPPLY_PROP_POLL_INTERVAL)
> + return -EINVAL;
> +
> + if (poll_interval == val->intval)
> + return 0;
> +
> + poll_interval = val->intval;
> + cancel_delayed_work_sync(&di->work);
> + schedule_delayed_work(&di->work, 0);
> +
> + return 0;
> +}
> +
> +static int bq27xxx_property_is_writeable(struct power_supply *psy,
> + enum power_supply_property psp)
> +{
> + if (psp == POWER_SUPPLY_PROP_POLL_INTERVAL)
> + return 1;
> +
> + return 0;
> +}
> +
> static void bq27xxx_external_power_changed(struct power_supply *psy)
> {
> struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
> @@ -988,6 +1026,8 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
> psy_desc->properties = bq27xxx_battery_props[di->chip].props;
> psy_desc->num_properties = bq27xxx_battery_props[di->chip].size;
> psy_desc->get_property = bq27xxx_battery_get_property;
> + psy_desc->set_property = bq27xxx_battery_set_property;
> + psy_desc->property_is_writeable = bq27xxx_property_is_writeable;
> psy_desc->external_power_changed = bq27xxx_external_power_changed;
>
> di->bat = power_supply_register_no_ws(di->dev, psy_desc, &psy_cfg);
> --
> 2.7.4
>
On Mon 2016-10-24 12:58:25, Matt Ranostay wrote:
> Pavel + Sebastian this is the patchset that need I some input on :)
Better then previous one.
But my version of bq27xxx_battery.c already contains this:
static const struct kernel_param_ops param_ops_poll_interval = {
.get = param_get_uint,
.set = poll_interval_param_set,
};
...so it should be possible to set poll interval already.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, Oct 24, 2016 at 1:14 PM, Pavel Machek <[email protected]> wrote:
> On Mon 2016-10-24 12:58:25, Matt Ranostay wrote:
>> Pavel + Sebastian this is the patchset that need I some input on :)
>
> Better then previous one.
>
> But my version of bq27xxx_battery.c already contains this:
This is for allowing udev rule to set the properties as well.
otherwise a kinda crude RUN = " echo value >
/sys/module/bq27xxx_battery/parameters/poll_interval" is required.
>
> static const struct kernel_param_ops param_ops_poll_interval = {
> .get = param_get_uint,
> .set = poll_interval_param_set,
> };
>
> ...so it should be possible to set poll interval already.
>
> Pavel
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue, Oct 25, 2016 at 11:47 AM, Matt Ranostay <[email protected]> wrote:
> On Mon, Oct 24, 2016 at 1:14 PM, Pavel Machek <[email protected]> wrote:
>> On Mon 2016-10-24 12:58:25, Matt Ranostay wrote:
>>> Pavel + Sebastian this is the patchset that need I some input on :)
>>
>> Better then previous one.
>>
>> But my version of bq27xxx_battery.c already contains this:
>
> This is for allowing udev rule to set the properties as well.
> otherwise a kinda crude RUN = " echo value >
> /sys/module/bq27xxx_battery/parameters/poll_interval" is required.
Any thoughts on this?
Thanks,
Matt
>
>>
>> static const struct kernel_param_ops param_ops_poll_interval = {
>> .get = param_get_uint,
>> .set = poll_interval_param_set,
>> };
>>
>> ...so it should be possible to set poll interval already.
>>
>> Pavel
>>
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Monday 31 October 2016 21:22:18 Matt Ranostay wrote:
> On Tue, Oct 25, 2016 at 11:47 AM, Matt Ranostay <[email protected]>
> wrote:
> > On Mon, Oct 24, 2016 at 1:14 PM, Pavel Machek <[email protected]> wrote:
> >> On Mon 2016-10-24 12:58:25, Matt Ranostay wrote:
> >>> Pavel + Sebastian this is the patchset that need I some input on
> >>> :)
> >>
> >> Better then previous one.
> >
> >> But my version of bq27xxx_battery.c already contains this:
> > This is for allowing udev rule to set the properties as well.
> > otherwise a kinda crude RUN = " echo value >
> > /sys/module/bq27xxx_battery/parameters/poll_interval" is required.
>
> Any thoughts on this?
Isn't sysfs /sys/module/bq27xxx_battery/parameters/poll_interval
attribute what should be used to change module parameters like
poll_interval?
--
Pali Rohár
[email protected]
On Mon 2016-10-31 13:22:18, Matt Ranostay wrote:
> On Tue, Oct 25, 2016 at 11:47 AM, Matt Ranostay <[email protected]> wrote:
> > On Mon, Oct 24, 2016 at 1:14 PM, Pavel Machek <[email protected]> wrote:
> >> On Mon 2016-10-24 12:58:25, Matt Ranostay wrote:
> >>> Pavel + Sebastian this is the patchset that need I some input on :)
> >>
> >> Better then previous one.
> >>
> >> But my version of bq27xxx_battery.c already contains this:
> >
> > This is for allowing udev rule to set the properties as well.
> > otherwise a kinda crude RUN = " echo value >
> > /sys/module/bq27xxx_battery/parameters/poll_interval" is required.
>
> Any thoughts on this?
I'd say echo value >
/sys/module/bq27xxx_battery/parameters/poll_interval .. is quite
adequate solution...?
Alternatively, convince us that something else is useful for everyone,
and we can do the right thing (poll more often when battery is nearly
empty), automatically...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, Oct 31, 2016 at 2:38 PM, Pavel Machek <[email protected]> wrote:
> On Mon 2016-10-31 13:22:18, Matt Ranostay wrote:
>> On Tue, Oct 25, 2016 at 11:47 AM, Matt Ranostay <[email protected]> wrote:
>> > On Mon, Oct 24, 2016 at 1:14 PM, Pavel Machek <[email protected]> wrote:
>> >> On Mon 2016-10-24 12:58:25, Matt Ranostay wrote:
>> >>> Pavel + Sebastian this is the patchset that need I some input on :)
>> >>
>> >> Better then previous one.
>> >>
>> >> But my version of bq27xxx_battery.c already contains this:
>> >
>> > This is for allowing udev rule to set the properties as well.
>> > otherwise a kinda crude RUN = " echo value >
>> > /sys/module/bq27xxx_battery/parameters/poll_interval" is required.
>>
>> Any thoughts on this?
>
> I'd say echo value >
> /sys/module/bq27xxx_battery/parameters/poll_interval .. is quite
> adequate solution...?
>
> Alternatively, convince us that something else is useful for everyone,
> and we can do the right thing (poll more often when battery is nearly
> empty), automatically...
Ok should have had the patchset set it per device, and not use the
global poll_interval. Of need to add some logic to see if uses the
global poll_interval or it's own setting.
There are times where you could have multiple batteries connected to
multiple fuel gauges, and want to up the polling interval on certain
ones that are discharging at different rates.
But of course I'll let you guys let me know if this seems useful at all.
>
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> >> >> Better then previous one.
> >> >>
> >> >> But my version of bq27xxx_battery.c already contains this:
> >> >
> >> > This is for allowing udev rule to set the properties as well.
> >> > otherwise a kinda crude RUN = " echo value >
> >> > /sys/module/bq27xxx_battery/parameters/poll_interval" is required.
> >>
> >> Any thoughts on this?
> >
> > I'd say echo value >
> > /sys/module/bq27xxx_battery/parameters/poll_interval .. is quite
> > adequate solution...?
> >
> > Alternatively, convince us that something else is useful for everyone,
> > and we can do the right thing (poll more often when battery is nearly
> > empty), automatically...
>
> Ok should have had the patchset set it per device, and not use the
> global poll_interval. Of need to add some logic to see if uses the
> global poll_interval or it's own setting.
>
> There are times where you could have multiple batteries connected to
> multiple fuel gauges, and want to up the polling interval on certain
> ones that are discharging at different rates.
>
> But of course I'll let you guys let me know if this seems useful at all.
I agree per-device polling would be cleaner.
But unless you have hardware with more than one bq27xxx, I'd avoid the
work...
Now... its also possible that poll_interval should change itself
(within kernel) to do the right thing.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, Nov 2, 2016 at 1:22 AM, Pavel Machek <[email protected]> wrote:
> Hi!
>
>> >> >> Better then previous one.
>> >> >>
>> >> >> But my version of bq27xxx_battery.c already contains this:
>> >> >
>> >> > This is for allowing udev rule to set the properties as well.
>> >> > otherwise a kinda crude RUN = " echo value >
>> >> > /sys/module/bq27xxx_battery/parameters/poll_interval" is required.
>> >>
>> >> Any thoughts on this?
>> >
>> > I'd say echo value >
>> > /sys/module/bq27xxx_battery/parameters/poll_interval .. is quite
>> > adequate solution...?
>> >
>> > Alternatively, convince us that something else is useful for everyone,
>> > and we can do the right thing (poll more often when battery is nearly
>> > empty), automatically...
>>
>> Ok should have had the patchset set it per device, and not use the
>> global poll_interval. Of need to add some logic to see if uses the
>> global poll_interval or it's own setting.
>>
>> There are times where you could have multiple batteries connected to
>> multiple fuel gauges, and want to up the polling interval on certain
>> ones that are discharging at different rates.
>>
>> But of course I'll let you guys let me know if this seems useful at all.
>
> I agree per-device polling would be cleaner.
>
Ok I'll work something up for RFC.
> But unless you have hardware with more than one bq27xxx, I'd avoid the
> work...
>
> Now... its also possible that poll_interval should change itself
> (within kernel) to do the right thing.
>
True but that is state machine territory, but I'll worry about that later...
> Best regards,
>
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu 2016-11-03 22:00:56, Matt Ranostay wrote:
> On Wed, Nov 2, 2016 at 1:22 AM, Pavel Machek <[email protected]> wrote:
> > Hi!
> >
> >> >> >> Better then previous one.
> >> >> >>
> >> >> >> But my version of bq27xxx_battery.c already contains this:
> >> >> >
> >> >> > This is for allowing udev rule to set the properties as well.
> >> >> > otherwise a kinda crude RUN = " echo value >
> >> >> > /sys/module/bq27xxx_battery/parameters/poll_interval" is required.
> >> >>
> >> >> Any thoughts on this?
> >> >
> >> > I'd say echo value >
> >> > /sys/module/bq27xxx_battery/parameters/poll_interval .. is quite
> >> > adequate solution...?
> >> >
> >> > Alternatively, convince us that something else is useful for everyone,
> >> > and we can do the right thing (poll more often when battery is nearly
> >> > empty), automatically...
> >>
> >> Ok should have had the patchset set it per device, and not use the
> >> global poll_interval. Of need to add some logic to see if uses the
> >> global poll_interval or it's own setting.
> >>
> >> There are times where you could have multiple batteries connected to
> >> multiple fuel gauges, and want to up the polling interval on certain
> >> ones that are discharging at different rates.
> >>
> >> But of course I'll let you guys let me know if this seems useful at all.
> >
> > I agree per-device polling would be cleaner.
> >
>
> Ok I'll work something up for RFC.
>
> > But unless you have hardware with more than one bq27xxx, I'd avoid the
> > work...
> >
> > Now... its also possible that poll_interval should change itself
> > (within kernel) to do the right thing.
> >
>
> True but that is state machine territory, but I'll worry about that later...
Do you actually have hardware with more than one bq27xxx?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
* Pavel Machek <[email protected]> [161104 00:10]:
> On Thu 2016-11-03 22:00:56, Matt Ranostay wrote:
> Do you actually have hardware with more than one bq27xxx?
I can at least see the twl4030 battery/charger features
being used together with some bq device to monitor the
battery state. Not sure if that counts as multiple
instances here though :)
Regards,
Tony
On Fri 2016-11-04 07:58:55, Tony Lindgren wrote:
> * Pavel Machek <[email protected]> [161104 00:10]:
> > On Thu 2016-11-03 22:00:56, Matt Ranostay wrote:
> > Do you actually have hardware with more than one bq27xxx?
>
> I can at least see the twl4030 battery/charger features
> being used together with some bq device to monitor the
> battery state. Not sure if that counts as multiple
> instances here though :)
I have that, too, but that was not what i was asking.
Matt wanted support for different polling intervals on different
bq27xxx chips. I'd like to know know if he actually has more than one
bq27xxx in his device...
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Fri, Nov 4, 2016 at 1:29 PM, Pavel Machek <[email protected]> wrote:
> On Fri 2016-11-04 07:58:55, Tony Lindgren wrote:
>> * Pavel Machek <[email protected]> [161104 00:10]:
>> > On Thu 2016-11-03 22:00:56, Matt Ranostay wrote:
>> > Do you actually have hardware with more than one bq27xxx?
>>
>> I can at least see the twl4030 battery/charger features
>> being used together with some bq device to monitor the
>> battery state. Not sure if that counts as multiple
>> instances here though :)
>
> I have that, too, but that was not what i was asking.
>
> Matt wanted support for different polling intervals on different
> bq27xxx chips. I'd like to know know if he actually has more than one
> bq27xxx in his device...
>
Actually only one bq27xxx chip but in theory we could have more.
> Best regards,
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Fri 2016-11-04 13:39:19, Matt Ranostay wrote:
> On Fri, Nov 4, 2016 at 1:29 PM, Pavel Machek <[email protected]> wrote:
> > On Fri 2016-11-04 07:58:55, Tony Lindgren wrote:
> >> * Pavel Machek <[email protected]> [161104 00:10]:
> >> > On Thu 2016-11-03 22:00:56, Matt Ranostay wrote:
> >> > Do you actually have hardware with more than one bq27xxx?
> >>
> >> I can at least see the twl4030 battery/charger features
> >> being used together with some bq device to monitor the
> >> battery state. Not sure if that counts as multiple
> >> instances here though :)
> >
> > I have that, too, but that was not what i was asking.
> >
> > Matt wanted support for different polling intervals on different
> > bq27xxx chips. I'd like to know know if he actually has more than one
> > bq27xxx in his device...
> >
>
> Actually only one bq27xxx chip but in theory we could have more.
Hmm. As we'd have to keep both old and new interfaces to change the
polling interfaces. Lets not due that unless we really need to, ok?
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html