Re: [PATCH v2 2/2] power: bq27xxx_battery: add poll interval property query

Re: [PATCH v2 2/2] power: bq27xxx_battery: add poll interval property query

[Pavel Machek]

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

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

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

Re: [PATCH v2 2/2] power: bq27xxx_battery: add poll interval property query

[Matt Ranostay]

On Mon, Oct 24, 2016 at 1:14 PM, Pavel Machek <pavel@ucw.cz> 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

Re: [PATCH v2 2/2] power: bq27xxx_battery: add poll interval property query

[Matt Ranostay]

On Tue, Oct 25, 2016 at 11:47 AM, Matt Ranostay <mranostay@gmail.com> wrote:
> On Mon, Oct 24, 2016 at 1:14 PM, Pavel Machek <pavel@ucw.cz> 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

Re: [PATCH v2 2/2] power: bq27xxx_battery: add poll interval property query

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 870 bytes --]

On Monday 31 October 2016 21:22:18 Matt Ranostay wrote:
> On Tue, Oct 25, 2016 at 11:47 AM, Matt Ranostay <mranostay@gmail.com>
> wrote:
> > On Mon, Oct 24, 2016 at 1:14 PM, Pavel Machek <pavel@ucw.cz> 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
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2 2/2] power: bq27xxx_battery: add poll interval property query

[Pavel Machek]

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

On Mon 2016-10-31 13:22:18, Matt Ranostay wrote:
> On Tue, Oct 25, 2016 at 11:47 AM, Matt Ranostay <mranostay@gmail.com> wrote:
> > On Mon, Oct 24, 2016 at 1:14 PM, Pavel Machek <pavel@ucw.cz> 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

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

Re: [PATCH v2 2/2] power: bq27xxx_battery: add poll interval property query

[Matt Ranostay]

On Mon, Oct 31, 2016 at 2:38 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Mon 2016-10-31 13:22:18, Matt Ranostay wrote:
>> On Tue, Oct 25, 2016 at 11:47 AM, Matt Ranostay <mranostay@gmail.com> wrote:
>> > On Mon, Oct 24, 2016 at 1:14 PM, Pavel Machek <pavel@ucw.cz> 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

Re: [PATCH v2 2/2] power: bq27xxx_battery: add poll interval property query

[Pavel Machek]

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

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

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

Re: [PATCH v2 2/2] power: bq27xxx_battery: add poll interval property query

[Matt Ranostay]

On Wed, Nov 2, 2016 at 1:22 AM, Pavel Machek <pavel@ucw.cz> 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

Re: [PATCH v2 2/2] power: bq27xxx_battery: add poll interval property query

[Pavel Machek]

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

On Thu 2016-11-03 22:00:56, Matt Ranostay wrote:
> On Wed, Nov 2, 2016 at 1:22 AM, Pavel Machek <pavel@ucw.cz> 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

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

Re: [PATCH v2 2/2] power: bq27xxx_battery: add poll interval property query

[Tony Lindgren]

* Pavel Machek <pavel@ucw.cz> [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

Re: [PATCH v2 2/2] power: bq27xxx_battery: add poll interval property query

[Pavel Machek]

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

On Fri 2016-11-04 07:58:55, Tony Lindgren wrote:
> * Pavel Machek <pavel@ucw.cz> [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

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

Re: [PATCH v2 2/2] power: bq27xxx_battery: add poll interval property query

[Matt Ranostay]

On Fri, Nov 4, 2016 at 1:29 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Fri 2016-11-04 07:58:55, Tony Lindgren wrote:
>> * Pavel Machek <pavel@ucw.cz> [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

Re: [PATCH v2 2/2] power: bq27xxx_battery: add poll interval property query

[Pavel Machek]

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

On Fri 2016-11-04 13:39:19, Matt Ranostay wrote:
> On Fri, Nov 4, 2016 at 1:29 PM, Pavel Machek <pavel@ucw.cz> wrote:
> > On Fri 2016-11-04 07:58:55, Tony Lindgren wrote:
> >> * Pavel Machek <pavel@ucw.cz> [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

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