subject:"Re\: \[PATCH v2 2\/3\] bq2415x_charger\: Use power

Hi

On Mon, Dec 2, 2013 at 1:24 AM, Anton Vorontsov  wrote:
> On Mon, Dec 02, 2013 at 01:02:40AM +0100, Michael Trimarchi wrote:
>> On Sun, Dec 1, 2013 at 11:37 PM, Anton Vorontsov  wrote:
>> > On Mon, Nov 25, 2013 at 08:16:34PM +0100, Michael Trimarchi wrote:
>> > ...
>> >> >> So you can read this value without any type of synchronization
>> >> >> with the power_supply_core
>> >> >> and sysfs implementation?
>> > ...
>> >> https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html
>> >>
>> >> I found and equivalent scenario that I was trying to said
>> >
>> > That's a good question, actually. Even though in Pali's case the notifier
>> > is atomic (so that we are pretty confident that the object won't be
>> > unregistered), there is still a possiblity of a dead lock, for example. So
>>
>> So if the get_property is a sleeping function it will be a deadlock. Right?
>
> All kind of bad things might happen, yes. But before that I would expect a
> bunch of warnings from might_sleep() stuff.
>
> I would recommend to test the patches using preempt/smp kernels + various
> DEBUG_ kernel options set.
>

Is it more simple to make it not atomic and use a mutex in get_property?

Michael

> Thanks,
>
> Anton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Mon, Dec 02, 2013 at 01:02:40AM +0100, Michael Trimarchi wrote:
> On Sun, Dec 1, 2013 at 11:37 PM, Anton Vorontsov  wrote:
> > On Mon, Nov 25, 2013 at 08:16:34PM +0100, Michael Trimarchi wrote:
> > ...
> >> >> So you can read this value without any type of synchronization
> >> >> with the power_supply_core
> >> >> and sysfs implementation?
> > ...
> >> https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html
> >>
> >> I found and equivalent scenario that I was trying to said
> >
> > That's a good question, actually. Even though in Pali's case the notifier
> > is atomic (so that we are pretty confident that the object won't be
> > unregistered), there is still a possiblity of a dead lock, for example. So
> 
> So if the get_property is a sleeping function it will be a deadlock. Right?

All kind of bad things might happen, yes. But before that I would expect a
bunch of warnings from might_sleep() stuff.

I would recommend to test the patches using preempt/smp kernels + various
DEBUG_ kernel options set.

Thanks,

Anton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

Hi Anton

On Sun, Dec 1, 2013 at 11:37 PM, Anton Vorontsov  wrote:
> On Mon, Nov 25, 2013 at 08:16:34PM +0100, Michael Trimarchi wrote:
> ...
>> >> So you can read this value without any type of synchronization
>> >> with the power_supply_core
>> >> and sysfs implementation?
> ...
>> https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html
>>
>> I found and equivalent scenario that I was trying to said
>
> That's a good question, actually. Even though in Pali's case the notifier
> is atomic (so that we are pretty confident that the object won't be
> unregistered), there is still a possiblity of a dead lock, for example. So

So if the get_property is a sleeping function it will be a deadlock. Right?

Michael

> notifiers should be careful to not hold any locks, because the other
> driver might call get_property(), which might acquire locks.
>
> Thanks,
>
> Anton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Mon, Nov 25, 2013 at 08:16:34PM +0100, Michael Trimarchi wrote:
...
> >> So you can read this value without any type of synchronization
> >> with the power_supply_core
> >> and sysfs implementation?
...
> https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html
> 
> I found and equivalent scenario that I was trying to said

That's a good question, actually. Even though in Pali's case the notifier
is atomic (so that we are pretty confident that the object won't be
unregistered), there is still a possiblity of a dead lock, for example. So
notifiers should be careful to not hold any locks, because the other
driver might call get_property(), which might acquire locks.

Thanks,

Anton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Mon, Nov 25, 2013 at 08:16:34PM +0100, Michael Trimarchi wrote:
...
  So you can read this value without any type of synchronization
  with the power_supply_core
  and sysfs implementation?
...
 https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html
 
 I found and equivalent scenario that I was trying to said

That's a good question, actually. Even though in Pali's case the notifier
is atomic (so that we are pretty confident that the object won't be
unregistered), there is still a possiblity of a dead lock, for example. So
notifiers should be careful to not hold any locks, because the other
driver might call get_property(), which might acquire locks.

Thanks,

Anton
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

Hi Anton

On Sun, Dec 1, 2013 at 11:37 PM, Anton Vorontsov an...@enomsg.org wrote:
 On Mon, Nov 25, 2013 at 08:16:34PM +0100, Michael Trimarchi wrote:
 ...
  So you can read this value without any type of synchronization
  with the power_supply_core
  and sysfs implementation?
 ...
 https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html

 I found and equivalent scenario that I was trying to said

 That's a good question, actually. Even though in Pali's case the notifier
 is atomic (so that we are pretty confident that the object won't be
 unregistered), there is still a possiblity of a dead lock, for example. So

So if the get_property is a sleeping function it will be a deadlock. Right?

Michael

 notifiers should be careful to not hold any locks, because the other
 driver might call get_property(), which might acquire locks.

 Thanks,

 Anton
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Mon, Dec 02, 2013 at 01:02:40AM +0100, Michael Trimarchi wrote:
 On Sun, Dec 1, 2013 at 11:37 PM, Anton Vorontsov an...@enomsg.org wrote:
  On Mon, Nov 25, 2013 at 08:16:34PM +0100, Michael Trimarchi wrote:
  ...
   So you can read this value without any type of synchronization
   with the power_supply_core
   and sysfs implementation?
  ...
  https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html
 
  I found and equivalent scenario that I was trying to said
 
  That's a good question, actually. Even though in Pali's case the notifier
  is atomic (so that we are pretty confident that the object won't be
  unregistered), there is still a possiblity of a dead lock, for example. So
 
 So if the get_property is a sleeping function it will be a deadlock. Right?

All kind of bad things might happen, yes. But before that I would expect a
bunch of warnings from might_sleep() stuff.

I would recommend to test the patches using preempt/smp kernels + various
DEBUG_ kernel options set.

Thanks,

Anton
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

Hi

On Mon, Dec 2, 2013 at 1:24 AM, Anton Vorontsov an...@enomsg.org wrote:
 On Mon, Dec 02, 2013 at 01:02:40AM +0100, Michael Trimarchi wrote:
 On Sun, Dec 1, 2013 at 11:37 PM, Anton Vorontsov an...@enomsg.org wrote:
  On Mon, Nov 25, 2013 at 08:16:34PM +0100, Michael Trimarchi wrote:
  ...
   So you can read this value without any type of synchronization
   with the power_supply_core
   and sysfs implementation?
  ...
  https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html
 
  I found and equivalent scenario that I was trying to said
 
  That's a good question, actually. Even though in Pali's case the notifier
  is atomic (so that we are pretty confident that the object won't be
  unregistered), there is still a possiblity of a dead lock, for example. So

 So if the get_property is a sleeping function it will be a deadlock. Right?

 All kind of bad things might happen, yes. But before that I would expect a
 bunch of warnings from might_sleep() stuff.

 I would recommend to test the patches using preempt/smp kernels + various
 DEBUG_ kernel options set.


Is it more simple to make it not atomic and use a mutex in get_property?

Michael

 Thanks,

 Anton
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Thursday 28 November 2013 01:25:50 Sebastian Reichel wrote:
> On Wed, Nov 27, 2013 at 10:16:47PM +0100, Pali Rohár wrote:
> > On Monday 25 November 2013 22:50:01 Sebastian Reichel wrote:
> > > > 2 seems more generic to me, but as rx51-battery is
> > > > missing the functionality to send events on temperature
> > > > change, I guess 1 will be easier to implement.
> > > 
> > > The temperature must be polled anyway, if the ADC does not
> > > support interrupts.
> > 
> > Yes, ADC does not support interrupts, temperature must be
> > polled. Also bq27200 chip does not support interrupts, but
> > bq27x00_battery driver using delayed work which every 60s
> > poll all values (timeout can be configured via modprobe
> > param). So similar code can be added to rx51_battery.ko
> > too.
> 
> I think the safest implementation would be:
> 
> bq2415x polls the temperature from rx51-battery in the bq2415x
> watchdog handler. That way discontinuation of the charge
> process is guaranteed.
> 
> To avoid useless ADC conversion the rx51-battery driver caches
> the converted temperature value for a reasonable time (e.g.
> 10 seconds). This helps if multiple users want to read the
> battery temperature (e.g. userspace).
> 
> This also means, that the kernel stuff can handle charging
> autonomously and the userland daemon checks the battery
> temperature only for emergency shutdown (I guess the
> temperatures for stopping the charging and emergency shutdown
> are different).
> 
> IMHO it makes sense to move the emergency shutdown also into
> the kernel (but different driver!) in the future, but that's
> another topic :)
> 
> -- Sebastian

Just to note, here is original nokia table of temperature limits:

https://gitorious.org/rx51-bme-replacement/dsme-thermalobject-surface/source/master:modules/thermalobject_surface.c#L40

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

2013-11-27 Thread Sebastian Reichel

On Wed, Nov 27, 2013 at 10:16:47PM +0100, Pali Rohár wrote:
> On Monday 25 November 2013 22:50:01 Sebastian Reichel wrote:
> > > 2 seems more generic to me, but as rx51-battery is missing
> > > the functionality to send events on temperature change, I
> > > guess 1 will be easier to implement.
> > 
> > The temperature must be polled anyway, if the ADC does not
> > support interrupts.
> > 
> 
> Yes, ADC does not support interrupts, temperature must be polled. 
> Also bq27200 chip does not support interrupts, but 
> bq27x00_battery driver using delayed work which every 60s poll 
> all values (timeout can be configured via modprobe param). So 
> similar code can be added to rx51_battery.ko too.

I think the safest implementation would be:

bq2415x polls the temperature from rx51-battery in the bq2415x
watchdog handler. That way discontinuation of the charge process
is guaranteed.

To avoid useless ADC conversion the rx51-battery driver caches the
converted temperature value for a reasonable time (e.g. 10 seconds).
This helps if multiple users want to read the battery temperature
(e.g. userspace).

This also means, that the kernel stuff can handle charging
autonomously and the userland daemon checks the battery temperature
only for emergency shutdown (I guess the temperatures for stopping
the charging and emergency shutdown are different).

IMHO it makes sense to move the emergency shutdown also into the
kernel (but different driver!) in the future, but that's another
topic :)

-- Sebastian

signature.asc
Description: Digital signature

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Monday 25 November 2013 22:50:01 Sebastian Reichel wrote:
> > 2 seems more generic to me, but as rx51-battery is missing
> > the functionality to send events on temperature change, I
> > guess 1 will be easier to implement.
> 
> The temperature must be polled anyway, if the ADC does not
> support interrupts.
> 

Yes, ADC does not support interrupts, temperature must be polled. 
Also bq27200 chip does not support interrupts, but 
bq27x00_battery driver using delayed work which every 60s poll 
all values (timeout can be configured via modprobe param). So 
similar code can be added to rx51_battery.ko too.

-- 
Pali Rohár
pali.ro...@gmail.com

signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Monday 25 November 2013 22:50:01 Sebastian Reichel wrote:
  2 seems more generic to me, but as rx51-battery is missing
  the functionality to send events on temperature change, I
  guess 1 will be easier to implement.
 
 The temperature must be polled anyway, if the ADC does not
 support interrupts.
 

Yes, ADC does not support interrupts, temperature must be polled. 
Also bq27200 chip does not support interrupts, but 
bq27x00_battery driver using delayed work which every 60s poll 
all values (timeout can be configured via modprobe param). So 
similar code can be added to rx51_battery.ko too.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

2013-11-27 Thread Sebastian Reichel

On Wed, Nov 27, 2013 at 10:16:47PM +0100, Pali Rohár wrote:
 On Monday 25 November 2013 22:50:01 Sebastian Reichel wrote:
   2 seems more generic to me, but as rx51-battery is missing
   the functionality to send events on temperature change, I
   guess 1 will be easier to implement.
  
  The temperature must be polled anyway, if the ADC does not
  support interrupts.
  
 
 Yes, ADC does not support interrupts, temperature must be polled. 
 Also bq27200 chip does not support interrupts, but 
 bq27x00_battery driver using delayed work which every 60s poll 
 all values (timeout can be configured via modprobe param). So 
 similar code can be added to rx51_battery.ko too.

I think the safest implementation would be:

bq2415x polls the temperature from rx51-battery in the bq2415x
watchdog handler. That way discontinuation of the charge process
is guaranteed.

To avoid useless ADC conversion the rx51-battery driver caches the
converted temperature value for a reasonable time (e.g. 10 seconds).
This helps if multiple users want to read the battery temperature
(e.g. userspace).

This also means, that the kernel stuff can handle charging
autonomously and the userland daemon checks the battery temperature
only for emergency shutdown (I guess the temperatures for stopping
the charging and emergency shutdown are different).

IMHO it makes sense to move the emergency shutdown also into the
kernel (but different driver!) in the future, but that's another
topic :)

-- Sebastian


signature.asc
Description: Digital signature

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Thursday 28 November 2013 01:25:50 Sebastian Reichel wrote:
On Wed, Nov 27, 2013 at 10:16:47PM +0100, Pali Rohár wrote:
On Monday 25 November 2013 22:50:01 Sebastian Reichel wrote:
2 seems more generic to me, but as rx51-battery is
missing the functionality to send events on temperature
change, I guess 1 will be easier to implement.

The temperature must be polled anyway, if the ADC does not
support interrupts.

Yes, ADC does not support interrupts, temperature must be
polled. Also bq27200 chip does not support interrupts, but
bq27x00_battery driver using delayed work which every 60s
poll all values (timeout can be configured via modprobe
param). So similar code can be added to rx51_battery.ko
too.

I think the safest implementation would be:

bq2415x polls the temperature from rx51-battery in the bq2415x
watchdog handler. That way discontinuation of the charge
process is guaranteed.

To avoid useless ADC conversion the rx51-battery driver caches
the converted temperature value for a reasonable time (e.g.
10 seconds). This helps if multiple users want to read the
battery temperature (e.g. userspace).

This also means, that the kernel stuff can handle charging
autonomously and the userland daemon checks the battery
temperature only for emergency shutdown (I guess the
temperatures for stopping the charging and emergency shutdown
are different).

IMHO it makes sense to move the emergency shutdown also into
the kernel (but different driver!) in the future, but that's
another topic :)

-- Sebastian

Just to note, here is original nokia table of temperature limits:

https://gitorious.org/rx51-bme-replacement/dsme-thermalobject-surface/source/master:modules/thermalobject_surface.c#L40

--
Pali Rohár
pali.ro...@gmail.com

signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

Hi,

On Mon, Nov 25, 2013 at 08:32:46PM +0200, Ивайло Димитров wrote:
> So, AIUI there are 2 options:
> 
> 1. charger driver polls the battery driver every n (60?) seconds.
> 2. battery driver sends PSY_EVENT_PROP_CHANGED on every degree
>up or down.
>  
> In both cases if the temperature is outside of the safe margins, the
> charging should be stopped.

ACK.

> 2 seems more generic to me, but as rx51-battery is missing the
> functionality to send events on temperature change, I guess 1 will
> be easier to implement.

The temperature must be polled anyway, if the ADC does not support
interrupts.

> And I think there should be some method (sysfs entry?, /dev/bqxxx
> opened?) to tell the charger driver to stop polling the battery 
> driver once (and if) the userspace has started to take care of the 
> battery temperature - makes no sense to duplicate the checks IMO.

I would just cache the temperature value in the battery driver for a
couple of seconds. Then userspace can check for very high temperatures
(-> system shutdown) and the charging driver can check for
temperatures, which are critical for charging.

-- Sebastian


signature.asc
Description: Digital signature

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

2013-11-25 Thread Michael Trimarchi

Hi

On Sun, Nov 24, 2013 at 8:01 PM, Pali Rohár  wrote:
> On Sunday 24 November 2013 18:18:03 Michael Trimarchi wrote:
>> Hi
>>
>> On Tue, Nov 19, 2013 at 11:18 AM, Pali Rohár
>  wrote:
>> > This patch removing set_mode_hook function from board data
>> > and replacing it with new string variable of notifier power
>> > supply device. After this change it is possible to add DT
>> > support because driver does not need specific board
>> > function anymore. Only static data and name of power supply
>> > device is required.
>> >
>> > Signed-off-by: Pali Rohár 
>> > ---
>> >
>> >  drivers/power/bq2415x_charger.c   |   77
>> >  +
>> >  include/linux/power/bq2415x_charger.h |   48
>> >  +++- 2 files changed, 65 insertions(+), 60
>> >  deletions(-)
>> >
> ...
>> >
>> > -   struct bq2415x_device *bq = data;
>> > +   struct bq2415x_device *bq =
>> > +   container_of(nb, struct bq2415x_device, nb);
>> > +   struct power_supply *psy = v;
>> > +   enum bq2415x_mode mode;
>> > +   union power_supply_propval prop;
>> > +   int ret;
>> > +   int mA;
>> >
>> > -   if (!bq)
>> > -   return;
>> > +   if (val != PSY_EVENT_PROP_CHANGED)
>> > +   return NOTIFY_OK;
>> > +
>> > +   if (strcmp(psy->name, bq->init_data.notify_device)
>> > != 0) +   return NOTIFY_OK;
>> > +
>> > +   dev_dbg(bq->dev, "notifier call was called\n");
>> > +
>> > +   ret = psy->get_property(psy,
>> > POWER_SUPPLY_PROP_CURRENT_MAX, ); +   if (ret !=
>> > 0)
>> > +   return NOTIFY_OK;
>>
>> So you can read this value without any type of synchronization
>> with the power_supply_core
>> and sysfs implementation?
>>
>>
>> Michael
>>
>
> I do not see reason why I cannot use it. When isp1704 driver send
> PSY_EVENT_PROP_CHANGED then property
> POWER_SUPPLY_PROP_CURRENT_MAX is already updated and can be read
> by get_property function.
>

https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html

I found and equivalent scenario that I was trying to said

Michael


> --
> Pali Rohár
> pali.ro...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Monday 25 November 2013 16:18:39 Pavel Machek wrote:
> On Mon 2013-11-25 15:10:00, Pali Rohár wrote:
> > On Monday 25 November 2013 15:01:27 Pavel Machek wrote:
> > > Where can I get dsme sources?
> > > 
> > >   Pavel
> > 
> > dsme daemon: https://gitorious.org/community-ssu/dsme
> > dsme thermal plugin:
> > https://gitorious.org/rx51-bme-replacement/dsme-thermalobje
> > ct-surface
> > 
> > dsme thermal plugin is responsible for checking temperature
> > of battery and send it to dsme damon. dsme daemon itself
> > checking temperature limits and do something (e.g reboot
> > device)
> 
> Thanks for links!
> 
> AFAICT dsme daemon does some kind of system management,
> not directly controlling charging, right? It does monitor
> system temperature.
> 

Yes.

> Now.. Imagine phone left in car in charger (on sun). Likely
> temperature will reach high values, kernel is charging, dsme
> will reboot the system, but kernel will start charging again,
> dsme will reboot again, ...
> 
> And it is not only high temperatures that are problem for
> li-ion charging; battery should not be fast charged below 5C
> and should not be charged below 0C. (Again, both are likely
> to happen if you leave your phone in car).
> 
> AFAICT, we should simply disable charging below 5C or above
> 45C.
> 
>   Pavel

Similar patch (as this) can be added to check for temperature 
from specified power supply driver (in timer reset thread). This 
can be easy and will fix this problem.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

  > Оригинално писмо 
 >От:  Sebastian Reichel 
 >Относно: Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for
 automode
 >До: Ивайло Димитров 
 >Изпратено на: Понеделник, 2013, Ноември 25 19:14:21 EET
 >
 >
 >On Mon, Nov 25, 2013 at 07:01:54PM +0200, Ивайло Димитров wrote:
 >>> Now.. Imagine phone left in car in charger (on sun). Likely
 >>> temperature will reach high values, kernel is charging, dsme will
 >>> reboot the system, but kernel will start charging again, dsme will
 >>> reboot again, ...
 >>>
 >>> And it is not only high temperatures that are problem for li-ion
 >>> charging; battery should not be fast charged below 5C and should not
 >>> be charged below 0C. (Again, both are likely to happen if you leave
 >>> your phone in car).
 >>>
 >>> AFAICT, we should simply disable charging below 5C or above 45C.
 >> 
 >> AFAIK dsme will not restart it, but power it off, so the above
 >> scenario won't happen.
 >
 >Just assume what happens, when dsme does not start (e.g. system boot
 >hangs).
 >
 >If everything is controlled from userspace, charger would not start
 >(-> safe!). If everything is controlled by the kernel, temperature
 >safety checks are taken (-> safe!). But in the currently proposed
 >variant: No safety checks.
 >
 >-- Sebastian
 >
 >

Hmm, you have a point here :)

So, AIUI there are 2 options:

1. charger driver polls the battery driver every n (60?) seconds.
2. battery driver sends PSY_EVENT_PROP_CHANGED on every degree up or
down
 
In both cases if the temperature is outside of the safe margins, the
charging should be stopped.

2 seems more generic to me, but as rx51-battery is missing the
functionality to send events on temperature change, I guess 1 will
be easier to implement.

And I think there should be some method (sysfs entry?, /dev/bqxxx
opened?) to tell the charger driver to stop polling the battery 
driver once (and if) the userspace has started to take care of the 
battery temperature - makes no sense to duplicate the checks IMO.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Mon, Nov 25, 2013 at 07:01:54PM +0200, Ивайло Димитров wrote:
>> Now.. Imagine phone left in car in charger (on sun). Likely
>> temperature will reach high values, kernel is charging, dsme will
>> reboot the system, but kernel will start charging again, dsme will
>> reboot again, ...
>>
>> And it is not only high temperatures that are problem for li-ion
>> charging; battery should not be fast charged below 5C and should not
>> be charged below 0C. (Again, both are likely to happen if you leave
>> your phone in car).
>>
>> AFAICT, we should simply disable charging below 5C or above 45C.
> 
> AFAIK dsme will not restart it, but power it off, so the above
> scenario won't happen.

Just assume what happens, when dsme does not start (e.g. system boot
hangs).

If everything is controlled from userspace, charger would not start
(-> safe!). If everything is controlled by the kernel, temperature
safety checks are taken (-> safe!). But in the currently proposed
variant: No safety checks.

-- Sebastian



signature.asc
Description: Digital signature

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

 




 > Оригинално писмо 
 >От:  Pavel Machek 
 >Относно: Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for
 automode
 >До: Pali Rohár 
 >Изпратено на: Понеделник, 2013, Ноември 25 17:18:39 EET
 >
 >
 >On Mon 2013-11-25 15:10:00, Pali Rohár wrote:
 >> On Monday 25 November 2013 15:01:27 Pavel Machek wrote:
 >> > Where can I get dsme sources?
 >> >   Pavel
 >> 
 >> dsme daemon: https://gitorious.org/community-ssu/dsme
 >> dsme thermal plugin: 
 >> https://gitorious.org/rx51-bme-replacement/dsme-thermalobject-surface
 >> 
 >> dsme thermal plugin is responsible for checking temperature of 
 >> battery and send it to dsme damon. dsme daemon itself checking 
 >> temperature limits and do something (e.g reboot device)
 >
 >Thanks for links!
 >
 >AFAICT dsme daemon does some kind of system management, not
 >directly controlling charging, right? It does monitor system temperature.
 >
 >Now.. Imagine phone left in car in charger (on sun). Likely
 >temperature will reach high values, kernel is charging, dsme will
 >reboot the system, but kernel will start charging again, dsme will
 >reboot again, ...
 >
 >And it is not only high temperatures that are problem for li-ion
 >charging; battery should not be fast charged below 5C and should not
 >be charged below 0C. (Again, both are likely to happen if you leave
 >your phone in car).
 >
 >AFAICT, we should simply disable charging below 5C or above 45C.
 >
 >  Pavel
 >-- 
 >(english) http://www.livejournal.com/~pavelmachek
 >(cesky, pictures) 
 >http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
 >

AFAIK dsme will not restart it, but power it off, so the above scenario won't 
happen.

Regards,
Ivo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Mon 2013-11-25 15:10:00, Pali Rohár wrote:
> On Monday 25 November 2013 15:01:27 Pavel Machek wrote:
> > Where can I get dsme sources?
> > Pavel
> 
> dsme daemon: https://gitorious.org/community-ssu/dsme
> dsme thermal plugin: 
> https://gitorious.org/rx51-bme-replacement/dsme-thermalobject-surface
> 
> dsme thermal plugin is responsible for checking temperature of 
> battery and send it to dsme damon. dsme daemon itself checking 
> temperature limits and do something (e.g reboot device)

Thanks for links!

AFAICT dsme daemon does some kind of system management, not
directly controlling charging, right? It does monitor system temperature.

Now.. Imagine phone left in car in charger (on sun). Likely
temperature will reach high values, kernel is charging, dsme will
reboot the system, but kernel will start charging again, dsme will
reboot again, ...

And it is not only high temperatures that are problem for li-ion
charging; battery should not be fast charged below 5C and should not
be charged below 0C. (Again, both are likely to happen if you leave
your phone in car).

AFAICT, we should simply disable charging below 5C or above 45C.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Monday 25 November 2013 15:01:27 Pavel Machek wrote:
> Where can I get dsme sources?
>   Pavel

dsme daemon: https://gitorious.org/community-ssu/dsme
dsme thermal plugin: 
https://gitorious.org/rx51-bme-replacement/dsme-thermalobject-surface

dsme thermal plugin is responsible for checking temperature of 
battery and send it to dsme damon. dsme daemon itself checking 
temperature limits and do something (e.g reboot device)

in dsme thermal plugin git history you can find both old version 
(which asking temperature from proprietary nokia bme daemon) and 
new version which reading temperature from power supply kernel 
modules.

-- 
Pali Rohár
pali.ro...@gmail.com

signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

Hi!

On Sun 2013-11-24 20:41:46, Pali Rohár wrote:
> On Sunday 24 November 2013 20:26:09 Sebastian Reichel wrote:
> > On Sun, Nov 24, 2013 at 08:01:16PM +0100, Pali Rohár wrote:
> > > Currently on Maemo 5 this is handled in userspace (with open
> > > source dsme daemon).
> > 
> > I assume it currently also takes care of the bq2415x watchdog?
> > That means if the daemon dies charging will stop, because the
> > watchdog does no longer trigger.
> > 
> > When your patch is applied you have introduced a safety issue.
> > When the daemon dies charging will continue and temperature is
> > no longer checked.
> > 
> > -- Sebastian
> 
> No dsme checking battery temperature and does not handle bq24510 
> timer (previously this was in closed bme daemon which 
> functionality is now in kernel drivers). But dsme daemon also 
> kicking tlw4030 watchdog, so when daemon dies after 30s tlw4030 
> reboot device.
> 
> But right you can implement correctly this in userspace (e.g. 
> when daemon not running/crashed, you can restart daemon or reboot 
> system or disable charing, whatever...) and you do not need to 
> have it in kernel...

Charging handling really should be in kernel.

1st) you want charging to work with init=/bin/bash, and you want it
with regular (not n900-specific) debian/arm.

2st) normally hardware, firmware or kernel does the charging. (PCs,
Sharp Zaurus). Yes, it pulls us farther away from Maemo...

Where can I get dsme sources?
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

Hi!

On Sun 2013-11-24 20:41:46, Pali Rohár wrote:
 On Sunday 24 November 2013 20:26:09 Sebastian Reichel wrote:
  On Sun, Nov 24, 2013 at 08:01:16PM +0100, Pali Rohár wrote:
   Currently on Maemo 5 this is handled in userspace (with open
   source dsme daemon).
  
  I assume it currently also takes care of the bq2415x watchdog?
  That means if the daemon dies charging will stop, because the
  watchdog does no longer trigger.
  
  When your patch is applied you have introduced a safety issue.
  When the daemon dies charging will continue and temperature is
  no longer checked.
  
  -- Sebastian
 
 No dsme checking battery temperature and does not handle bq24510 
 timer (previously this was in closed bme daemon which 
 functionality is now in kernel drivers). But dsme daemon also 
 kicking tlw4030 watchdog, so when daemon dies after 30s tlw4030 
 reboot device.
 
 But right you can implement correctly this in userspace (e.g. 
 when daemon not running/crashed, you can restart daemon or reboot 
 system or disable charing, whatever...) and you do not need to 
 have it in kernel...

Charging handling really should be in kernel.

1st) you want charging to work with init=/bin/bash, and you want it
with regular (not n900-specific) debian/arm.

2st) normally hardware, firmware or kernel does the charging. (PCs,
Sharp Zaurus). Yes, it pulls us farther away from Maemo...

Where can I get dsme sources?
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Monday 25 November 2013 15:01:27 Pavel Machek wrote:
 Where can I get dsme sources?
   Pavel

dsme daemon: https://gitorious.org/community-ssu/dsme
dsme thermal plugin: 
https://gitorious.org/rx51-bme-replacement/dsme-thermalobject-surface

dsme thermal plugin is responsible for checking temperature of 
battery and send it to dsme damon. dsme daemon itself checking 
temperature limits and do something (e.g reboot device)

in dsme thermal plugin git history you can find both old version 
(which asking temperature from proprietary nokia bme daemon) and 
new version which reading temperature from power supply kernel 
modules.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Mon 2013-11-25 15:10:00, Pali Rohár wrote:
 On Monday 25 November 2013 15:01:27 Pavel Machek wrote:
  Where can I get dsme sources?
  Pavel
 
 dsme daemon: https://gitorious.org/community-ssu/dsme
 dsme thermal plugin: 
 https://gitorious.org/rx51-bme-replacement/dsme-thermalobject-surface
 
 dsme thermal plugin is responsible for checking temperature of 
 battery and send it to dsme damon. dsme daemon itself checking 
 temperature limits and do something (e.g reboot device)

Thanks for links!

AFAICT dsme daemon does some kind of system management, not
directly controlling charging, right? It does monitor system temperature.

Now.. Imagine phone left in car in charger (on sun). Likely
temperature will reach high values, kernel is charging, dsme will
reboot the system, but kernel will start charging again, dsme will
reboot again, ...

And it is not only high temperatures that are problem for li-ion
charging; battery should not be fast charged below 5C and should not
be charged below 0C. (Again, both are likely to happen if you leave
your phone in car).

AFAICT, we should simply disable charging below 5C or above 45C.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

 




  Оригинално писмо 
 От:  Pavel Machek 
 Относно: Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for
 automode
 До: Pali Rohár 
 Изпратено на: Понеделник, 2013, Ноември 25 17:18:39 EET
 
 
 On Mon 2013-11-25 15:10:00, Pali Rohár wrote:
  On Monday 25 November 2013 15:01:27 Pavel Machek wrote:
   Where can I get dsme sources?
 Pavel
  
  dsme daemon: https://gitorious.org/community-ssu/dsme
  dsme thermal plugin: 
  https://gitorious.org/rx51-bme-replacement/dsme-thermalobject-surface
  
  dsme thermal plugin is responsible for checking temperature of 
  battery and send it to dsme damon. dsme daemon itself checking 
  temperature limits and do something (e.g reboot device)
 
 Thanks for links!
 
 AFAICT dsme daemon does some kind of system management, not
 directly controlling charging, right? It does monitor system temperature.
 
 Now.. Imagine phone left in car in charger (on sun). Likely
 temperature will reach high values, kernel is charging, dsme will
 reboot the system, but kernel will start charging again, dsme will
 reboot again, ...
 
 And it is not only high temperatures that are problem for li-ion
 charging; battery should not be fast charged below 5C and should not
 be charged below 0C. (Again, both are likely to happen if you leave
 your phone in car).
 
 AFAICT, we should simply disable charging below 5C or above 45C.
 
   Pavel
 -- 
 (english) http://www.livejournal.com/~pavelmachek
 (cesky, pictures) 
 http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
 

AFAIK dsme will not restart it, but power it off, so the above scenario won't 
happen.

Regards,
Ivo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Mon, Nov 25, 2013 at 07:01:54PM +0200, Ивайло Димитров wrote:
 Now.. Imagine phone left in car in charger (on sun). Likely
 temperature will reach high values, kernel is charging, dsme will
 reboot the system, but kernel will start charging again, dsme will
 reboot again, ...

 And it is not only high temperatures that are problem for li-ion
 charging; battery should not be fast charged below 5C and should not
 be charged below 0C. (Again, both are likely to happen if you leave
 your phone in car).

 AFAICT, we should simply disable charging below 5C or above 45C.
 
 AFAIK dsme will not restart it, but power it off, so the above
 scenario won't happen.

Just assume what happens, when dsme does not start (e.g. system boot
hangs).

If everything is controlled from userspace, charger would not start
(- safe!). If everything is controlled by the kernel, temperature
safety checks are taken (- safe!). But in the currently proposed
variant: No safety checks.

-- Sebastian



signature.asc
Description: Digital signature

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

   Оригинално писмо 
 От:  Sebastian Reichel 
 Относно: Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for
 automode
 До: Ивайло Димитров 
 Изпратено на: Понеделник, 2013, Ноември 25 19:14:21 EET
 
 
 On Mon, Nov 25, 2013 at 07:01:54PM +0200, Ивайло Димитров wrote:
  Now.. Imagine phone left in car in charger (on sun). Likely
  temperature will reach high values, kernel is charging, dsme will
  reboot the system, but kernel will start charging again, dsme will
  reboot again, ...
 
  And it is not only high temperatures that are problem for li-ion
  charging; battery should not be fast charged below 5C and should not
  be charged below 0C. (Again, both are likely to happen if you leave
  your phone in car).
 
  AFAICT, we should simply disable charging below 5C or above 45C.
  
  AFAIK dsme will not restart it, but power it off, so the above
  scenario won't happen.
 
 Just assume what happens, when dsme does not start (e.g. system boot
 hangs).
 
 If everything is controlled from userspace, charger would not start
 (- safe!). If everything is controlled by the kernel, temperature
 safety checks are taken (- safe!). But in the currently proposed
 variant: No safety checks.
 
 -- Sebastian
 
 

Hmm, you have a point here :)

So, AIUI there are 2 options:

1. charger driver polls the battery driver every n (60?) seconds.
2. battery driver sends PSY_EVENT_PROP_CHANGED on every degree up or
down
 
In both cases if the temperature is outside of the safe margins, the
charging should be stopped.

2 seems more generic to me, but as rx51-battery is missing the
functionality to send events on temperature change, I guess 1 will
be easier to implement.

And I think there should be some method (sysfs entry?, /dev/bqxxx
opened?) to tell the charger driver to stop polling the battery 
driver once (and if) the userspace has started to take care of the 
battery temperature - makes no sense to duplicate the checks IMO.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Monday 25 November 2013 16:18:39 Pavel Machek wrote:
 On Mon 2013-11-25 15:10:00, Pali Rohár wrote:
  On Monday 25 November 2013 15:01:27 Pavel Machek wrote:
   Where can I get dsme sources?
   
 Pavel
  
  dsme daemon: https://gitorious.org/community-ssu/dsme
  dsme thermal plugin:
  https://gitorious.org/rx51-bme-replacement/dsme-thermalobje
  ct-surface
  
  dsme thermal plugin is responsible for checking temperature
  of battery and send it to dsme damon. dsme daemon itself
  checking temperature limits and do something (e.g reboot
  device)
 
 Thanks for links!
 
 AFAICT dsme daemon does some kind of system management,
 not directly controlling charging, right? It does monitor
 system temperature.
 

Yes.

 Now.. Imagine phone left in car in charger (on sun). Likely
 temperature will reach high values, kernel is charging, dsme
 will reboot the system, but kernel will start charging again,
 dsme will reboot again, ...
 
 And it is not only high temperatures that are problem for
 li-ion charging; battery should not be fast charged below 5C
 and should not be charged below 0C. (Again, both are likely
 to happen if you leave your phone in car).
 
 AFAICT, we should simply disable charging below 5C or above
 45C.
 
   Pavel

Similar patch (as this) can be added to check for temperature 
from specified power supply driver (in timer reset thread). This 
can be easy and will fix this problem.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

2013-11-25 Thread Michael Trimarchi

Hi

On Sun, Nov 24, 2013 at 8:01 PM, Pali Rohár pali.ro...@gmail.com wrote:
 On Sunday 24 November 2013 18:18:03 Michael Trimarchi wrote:
 Hi

 On Tue, Nov 19, 2013 at 11:18 AM, Pali Rohár
 pali.ro...@gmail.com wrote:
  This patch removing set_mode_hook function from board data
  and replacing it with new string variable of notifier power
  supply device. After this change it is possible to add DT
  support because driver does not need specific board
  function anymore. Only static data and name of power supply
  device is required.
 
  Signed-off-by: Pali Rohár pali.ro...@gmail.com
  ---
 
   drivers/power/bq2415x_charger.c   |   77
   +
   include/linux/power/bq2415x_charger.h |   48
   +++- 2 files changed, 65 insertions(+), 60
   deletions(-)
 
 ...
 
  -   struct bq2415x_device *bq = data;
  +   struct bq2415x_device *bq =
  +   container_of(nb, struct bq2415x_device, nb);
  +   struct power_supply *psy = v;
  +   enum bq2415x_mode mode;
  +   union power_supply_propval prop;
  +   int ret;
  +   int mA;
 
  -   if (!bq)
  -   return;
  +   if (val != PSY_EVENT_PROP_CHANGED)
  +   return NOTIFY_OK;
  +
  +   if (strcmp(psy-name, bq-init_data.notify_device)
  != 0) +   return NOTIFY_OK;
  +
  +   dev_dbg(bq-dev, notifier call was called\n);
  +
  +   ret = psy-get_property(psy,
  POWER_SUPPLY_PROP_CURRENT_MAX, prop); +   if (ret !=
  0)
  +   return NOTIFY_OK;

 So you can read this value without any type of synchronization
 with the power_supply_core
 and sysfs implementation?


 Michael


 I do not see reason why I cannot use it. When isp1704 driver send
 PSY_EVENT_PROP_CHANGED then property
 POWER_SUPPLY_PROP_CURRENT_MAX is already updated and can be read
 by get_property function.


https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html

I found and equivalent scenario that I was trying to said

Michael


 --
 Pali Rohár
 pali.ro...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

Hi,

On Mon, Nov 25, 2013 at 08:32:46PM +0200, Ивайло Димитров wrote:
 So, AIUI there are 2 options:
 
 1. charger driver polls the battery driver every n (60?) seconds.
 2. battery driver sends PSY_EVENT_PROP_CHANGED on every degree
up or down.
  
 In both cases if the temperature is outside of the safe margins, the
 charging should be stopped.

ACK.

 2 seems more generic to me, but as rx51-battery is missing the
 functionality to send events on temperature change, I guess 1 will
 be easier to implement.

The temperature must be polled anyway, if the ADC does not support
interrupts.

 And I think there should be some method (sysfs entry?, /dev/bqxxx
 opened?) to tell the charger driver to stop polling the battery 
 driver once (and if) the userspace has started to take care of the 
 battery temperature - makes no sense to duplicate the checks IMO.

I would just cache the temperature value in the battery driver for a
couple of seconds. Then userspace can check for very high temperatures
(- system shutdown) and the charging driver can check for
temperatures, which are critical for charging.

-- Sebastian


signature.asc
Description: Digital signature

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Sun, Nov 24, 2013 at 08:41:46PM +0100, Pali Rohár wrote:
> On Sunday 24 November 2013 20:26:09 Sebastian Reichel wrote:
> > On Sun, Nov 24, 2013 at 08:01:16PM +0100, Pali Rohár wrote:
> > > Currently on Maemo 5 this is handled in userspace (with open
> > > source dsme daemon).
> > 
> > I assume it currently also takes care of the bq2415x watchdog?
> > That means if the daemon dies charging will stop, because the
> > watchdog does no longer trigger.
> > 
> > When your patch is applied you have introduced a safety issue.
> > When the daemon dies charging will continue and temperature is
> > no longer checked.
> > 
> > -- Sebastian
> 
> No dsme checking battery temperature and does not handle bq24510 
> timer (previously this was in closed bme daemon which 
> functionality is now in kernel drivers).

Ok, so then the temperature checks are done by bme.

> But dsme daemon also kicking tlw4030 watchdog, so when daemon dies
> after 30s tlw4030 reboot device.

Yes, but that does not matter if it does not take care of the
charging. If the dsme daemon is not started for some reason
phone will not be rebootet and phone charges.

> But right you can implement correctly this in userspace (e.g. 
> when daemon not running/crashed, you can restart daemon or reboot 
> system or disable charing, whatever...) and you do not need to 
> have it in kernel...

That does not change the fact, that the kernel openes a safety issue
in default configuration. To be on the safe side charging must be
disabled until some safety checking daemon enables it.

-- Sebastian


signature.asc
Description: Digital signature

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Sunday 24 November 2013 20:26:09 Sebastian Reichel wrote:
> On Sun, Nov 24, 2013 at 08:01:16PM +0100, Pali Rohár wrote:
> > Currently on Maemo 5 this is handled in userspace (with open
> > source dsme daemon).
> 
> I assume it currently also takes care of the bq2415x watchdog?
> That means if the daemon dies charging will stop, because the
> watchdog does no longer trigger.
> 
> When your patch is applied you have introduced a safety issue.
> When the daemon dies charging will continue and temperature is
> no longer checked.
> 
> -- Sebastian

No dsme checking battery temperature and does not handle bq24510 
timer (previously this was in closed bme daemon which 
functionality is now in kernel drivers). But dsme daemon also 
kicking tlw4030 watchdog, so when daemon dies after 30s tlw4030 
reboot device.

But right you can implement correctly this in userspace (e.g. 
when daemon not running/crashed, you can restart daemon or reboot 
system or disable charing, whatever...) and you do not need to 
have it in kernel...

-- 
Pali Rohár
pali.ro...@gmail.com

signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Sun, Nov 24, 2013 at 08:01:16PM +0100, Pali Rohár wrote:
> > I'm wondering if the watchdog thread should check some values,
> > like e.g. battery temperature. It should stop charging the
> > battery if some critical battery temperature threshold is
> > reached.
> 
> For checking battery temperature is needed third driver 
> (rx51_battery.ko).

I know, but that doesn't mean one can skip the check.

> And I think this should not be implemented in driver itself but in
> charger manager framework...

There is a reason, that the HW has a watchdog. I think the safety
relevant stuff should be done in the watchdog thread. This ensures,
that charging depends on reading the safety relevant sensors.

Of course the watchdog stuff could be moved into the charger manager
framework...

> Currently on Maemo 5 this is handled in userspace (with open 
> source dsme daemon).

I assume it currently also takes care of the bq2415x watchdog? That
means if the daemon dies charging will stop, because the watchdog
does no longer trigger.

When your patch is applied you have introduced a safety issue.
When the daemon dies charging will continue and temperature is
no longer checked.

-- Sebastian

signature.asc
Description: Digital signature

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Sunday 24 November 2013 18:18:03 Michael Trimarchi wrote:
> Hi
> 
> On Tue, Nov 19, 2013 at 11:18 AM, Pali Rohár 
 wrote:
> > This patch removing set_mode_hook function from board data
> > and replacing it with new string variable of notifier power
> > supply device. After this change it is possible to add DT
> > support because driver does not need specific board
> > function anymore. Only static data and name of power supply
> > device is required.
> > 
> > Signed-off-by: Pali Rohár 
> > ---
> > 
> >  drivers/power/bq2415x_charger.c   |   77
> >  +
> >  include/linux/power/bq2415x_charger.h |   48
> >  +++- 2 files changed, 65 insertions(+), 60
> >  deletions(-)
> > 
...
> > 
> > -   struct bq2415x_device *bq = data;
> > +   struct bq2415x_device *bq =
> > +   container_of(nb, struct bq2415x_device, nb);
> > +   struct power_supply *psy = v;
> > +   enum bq2415x_mode mode;
> > +   union power_supply_propval prop;
> > +   int ret;
> > +   int mA;
> > 
> > -   if (!bq)
> > -   return;
> > +   if (val != PSY_EVENT_PROP_CHANGED)
> > +   return NOTIFY_OK;
> > +
> > +   if (strcmp(psy->name, bq->init_data.notify_device)
> > != 0) +   return NOTIFY_OK;
> > +
> > +   dev_dbg(bq->dev, "notifier call was called\n");
> > +
> > +   ret = psy->get_property(psy,
> > POWER_SUPPLY_PROP_CURRENT_MAX, ); +   if (ret !=
> > 0)
> > +   return NOTIFY_OK;
> 
> So you can read this value without any type of synchronization
> with the power_supply_core
> and sysfs implementation?
> 
> 
> Michael
> 

I do not see reason why I cannot use it. When isp1704 driver send 
PSY_EVENT_PROP_CHANGED then property 
POWER_SUPPLY_PROP_CURRENT_MAX is already updated and can be read 
by get_property function.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Sunday 24 November 2013 18:00:00 Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Nov 19, 2013 at 11:18:04AM +0100, Pali Rohár wrote:
> > This patch removing set_mode_hook function from board data
> > and replacing it with new string variable of notifier power
> > supply device. After this change it is possible to add DT
> > support because driver does not need specific board
> > function anymore. Only static data and name of power supply
> > device is required.
> 
> I'm wondering if the watchdog thread should check some values,
> like e.g. battery temperature. It should stop charging the
> battery if some critical battery temperature threshold is
> reached.
> 
> -- Sebastian

For checking battery temperature is needed third driver 
(rx51_battery.ko). And I think this should not be implemented in 
driver itself but in charger manager framework...

Currently on Maemo 5 this is handled in userspace (with open 
source dsme daemon).

-- 
Pali Rohár
pali.ro...@gmail.com

signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

2013-11-24 Thread Michael Trimarchi

Hi

On Tue, Nov 19, 2013 at 11:18 AM, Pali Rohár  wrote:
> This patch removing set_mode_hook function from board data and replacing it 
> with
> new string variable of notifier power supply device. After this change it is
> possible to add DT support because driver does not need specific board 
> function
> anymore. Only static data and name of power supply device is required.
>
> Signed-off-by: Pali Rohár 
> ---
>  drivers/power/bq2415x_charger.c   |   77 
> +
>  include/linux/power/bq2415x_charger.h |   48 +++-
>  2 files changed, 65 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/power/bq2415x_charger.c b/drivers/power/bq2415x_charger.c
> index 0727f92..d89583d 100644
> --- a/drivers/power/bq2415x_charger.c
> +++ b/drivers/power/bq2415x_charger.c
> @@ -1,7 +1,7 @@
>  /*
>   * bq2415x charger driver
>   *
> - * Copyright (C) 2011-2012  Pali Rohár 
> + * Copyright (C) 2011-2013  Pali Rohár 
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
> @@ -170,6 +170,7 @@ struct bq2415x_device {
> struct bq2415x_platform_data init_data;
> struct power_supply charger;
> struct delayed_work work;
> +   struct notifier_block nb;
> enum bq2415x_mode reported_mode;/* mode reported by hook function */
> enum bq2415x_mode mode; /* current configured mode */
> enum bq2415x_chip chip;
> @@ -791,24 +792,53 @@ static int bq2415x_set_mode(struct bq2415x_device *bq, 
> enum bq2415x_mode mode)
>
>  }
>
> -/* hook function called by other driver which set reported mode */
> -static void bq2415x_hook_function(enum bq2415x_mode mode, void *data)
> +static int bq2415x_notifier_call(struct notifier_block *nb,
> +   unsigned long val, void *v)
>  {
> -   struct bq2415x_device *bq = data;
> +   struct bq2415x_device *bq =
> +   container_of(nb, struct bq2415x_device, nb);
> +   struct power_supply *psy = v;
> +   enum bq2415x_mode mode;
> +   union power_supply_propval prop;
> +   int ret;
> +   int mA;
>
> -   if (!bq)
> -   return;
> +   if (val != PSY_EVENT_PROP_CHANGED)
> +   return NOTIFY_OK;
> +
> +   if (strcmp(psy->name, bq->init_data.notify_device) != 0)
> +   return NOTIFY_OK;
> +
> +   dev_dbg(bq->dev, "notifier call was called\n");
> +
> +   ret = psy->get_property(psy, POWER_SUPPLY_PROP_CURRENT_MAX, );
> +   if (ret != 0)
> +   return NOTIFY_OK;

So you can read this value without any type of synchronization with
the power_supply_core
and sysfs implementation?


Michael

> +
> +   mA = prop.intval;
> +
> +   if (mA == 0)
> +   mode = BQ2415X_MODE_OFF;
> +   else if (mA < 500)
> +   mode = BQ2415X_MODE_NONE;
> +   else if (mA < 1800)
> +   mode = BQ2415X_MODE_HOST_CHARGER;
> +   else
> +   mode = BQ2415X_MODE_DEDICATED_CHARGER;
> +
> +   if (bq->reported_mode == mode)
> +   return NOTIFY_OK;
>
> -   dev_dbg(bq->dev, "hook function was called\n");
> bq->reported_mode = mode;
>
> /* if automode is not enabled do not tell about reported_mode */
> if (bq->automode < 1)
> -   return;
> +   return NOTIFY_OK;
>
> sysfs_notify(>charger.dev->kobj, NULL, "reported_mode");
> bq2415x_set_mode(bq, bq->reported_mode);
>
> +   return NOTIFY_OK;
>  }
>
>  / timer functions /
> @@ -1508,6 +1538,7 @@ static int bq2415x_probe(struct i2c_client *client,
> int num;
> char *name;
> struct bq2415x_device *bq;
> +   struct power_supply *psy;
>
> if (!client->dev.platform_data) {
> dev_err(>dev, "platform data not set\n");
> @@ -1569,16 +1600,27 @@ static int bq2415x_probe(struct i2c_client *client,
> goto error_4;
> }
>
> -   if (bq->init_data.set_mode_hook) {
> -   if (bq->init_data.set_mode_hook(
> -   bq2415x_hook_function, bq)) {
> -   bq->automode = 1;
> +   if (bq->init_data.notify_device) {
> +   bq->nb.notifier_call = bq2415x_notifier_call;
> +   ret = power_supply_reg_notifier(>nb);
> +   if (ret) {
> +   dev_err(bq->dev, "failed to reg notifier: %d\n", ret);
> +   goto error_5;
> +   }
> +   psy = power_supply_get_by_name(bq->init_data.notify_device);
> +   if (psy) {
> +   /* Query for initial reported_mode and set it */
> +   bq2415x_notifier_call(>nb,
> +   PSY_EVENT_PROP_CHANGED, psy);
> bq2415x_set_mode(bq, bq->reported_mode);
> -   dev_info(bq->dev, "automode

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

Hi,

On Tue, Nov 19, 2013 at 11:18:04AM +0100, Pali Rohár wrote:
> This patch removing set_mode_hook function from board data and replacing it 
> with
> new string variable of notifier power supply device. After this change it is
> possible to add DT support because driver does not need specific board 
> function
> anymore. Only static data and name of power supply device is required.

I'm wondering if the watchdog thread should check some values, like
e.g. battery temperature. It should stop charging the battery if
some critical battery temperature threshold is reached.

-- Sebastian


signature.asc
Description: Digital signature

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

Hi,

On Tue, Nov 19, 2013 at 11:18:04AM +0100, Pali Rohár wrote:
 This patch removing set_mode_hook function from board data and replacing it 
 with
 new string variable of notifier power supply device. After this change it is
 possible to add DT support because driver does not need specific board 
 function
 anymore. Only static data and name of power supply device is required.

I'm wondering if the watchdog thread should check some values, like
e.g. battery temperature. It should stop charging the battery if
some critical battery temperature threshold is reached.

-- Sebastian


signature.asc
Description: Digital signature

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

2013-11-24 Thread Michael Trimarchi

Hi

On Tue, Nov 19, 2013 at 11:18 AM, Pali Rohár pali.ro...@gmail.com wrote:
 This patch removing set_mode_hook function from board data and replacing it 
 with
 new string variable of notifier power supply device. After this change it is
 possible to add DT support because driver does not need specific board 
 function
 anymore. Only static data and name of power supply device is required.

 Signed-off-by: Pali Rohár pali.ro...@gmail.com
 ---
  drivers/power/bq2415x_charger.c   |   77 
 +
  include/linux/power/bq2415x_charger.h |   48 +++-
  2 files changed, 65 insertions(+), 60 deletions(-)

 diff --git a/drivers/power/bq2415x_charger.c b/drivers/power/bq2415x_charger.c
 index 0727f92..d89583d 100644
 --- a/drivers/power/bq2415x_charger.c
 +++ b/drivers/power/bq2415x_charger.c
 @@ -1,7 +1,7 @@
  /*
   * bq2415x charger driver
   *
 - * Copyright (C) 2011-2012  Pali Rohár pali.ro...@gmail.com
 + * Copyright (C) 2011-2013  Pali Rohár pali.ro...@gmail.com
   *
   * This program is free software; you can redistribute it and/or modify
   * it under the terms of the GNU General Public License as published by
 @@ -170,6 +170,7 @@ struct bq2415x_device {
 struct bq2415x_platform_data init_data;
 struct power_supply charger;
 struct delayed_work work;
 +   struct notifier_block nb;
 enum bq2415x_mode reported_mode;/* mode reported by hook function */
 enum bq2415x_mode mode; /* current configured mode */
 enum bq2415x_chip chip;
 @@ -791,24 +792,53 @@ static int bq2415x_set_mode(struct bq2415x_device *bq, 
 enum bq2415x_mode mode)

  }

 -/* hook function called by other driver which set reported mode */
 -static void bq2415x_hook_function(enum bq2415x_mode mode, void *data)
 +static int bq2415x_notifier_call(struct notifier_block *nb,
 +   unsigned long val, void *v)
  {
 -   struct bq2415x_device *bq = data;
 +   struct bq2415x_device *bq =
 +   container_of(nb, struct bq2415x_device, nb);
 +   struct power_supply *psy = v;
 +   enum bq2415x_mode mode;
 +   union power_supply_propval prop;
 +   int ret;
 +   int mA;

 -   if (!bq)
 -   return;
 +   if (val != PSY_EVENT_PROP_CHANGED)
 +   return NOTIFY_OK;
 +
 +   if (strcmp(psy-name, bq-init_data.notify_device) != 0)
 +   return NOTIFY_OK;
 +
 +   dev_dbg(bq-dev, notifier call was called\n);
 +
 +   ret = psy-get_property(psy, POWER_SUPPLY_PROP_CURRENT_MAX, prop);
 +   if (ret != 0)
 +   return NOTIFY_OK;

So you can read this value without any type of synchronization with
the power_supply_core
and sysfs implementation?


Michael

 +
 +   mA = prop.intval;
 +
 +   if (mA == 0)
 +   mode = BQ2415X_MODE_OFF;
 +   else if (mA  500)
 +   mode = BQ2415X_MODE_NONE;
 +   else if (mA  1800)
 +   mode = BQ2415X_MODE_HOST_CHARGER;
 +   else
 +   mode = BQ2415X_MODE_DEDICATED_CHARGER;
 +
 +   if (bq-reported_mode == mode)
 +   return NOTIFY_OK;

 -   dev_dbg(bq-dev, hook function was called\n);
 bq-reported_mode = mode;

 /* if automode is not enabled do not tell about reported_mode */
 if (bq-automode  1)
 -   return;
 +   return NOTIFY_OK;

 sysfs_notify(bq-charger.dev-kobj, NULL, reported_mode);
 bq2415x_set_mode(bq, bq-reported_mode);

 +   return NOTIFY_OK;
  }

  / timer functions /
 @@ -1508,6 +1538,7 @@ static int bq2415x_probe(struct i2c_client *client,
 int num;
 char *name;
 struct bq2415x_device *bq;
 +   struct power_supply *psy;

 if (!client-dev.platform_data) {
 dev_err(client-dev, platform data not set\n);
 @@ -1569,16 +1600,27 @@ static int bq2415x_probe(struct i2c_client *client,
 goto error_4;
 }

 -   if (bq-init_data.set_mode_hook) {
 -   if (bq-init_data.set_mode_hook(
 -   bq2415x_hook_function, bq)) {
 -   bq-automode = 1;
 +   if (bq-init_data.notify_device) {
 +   bq-nb.notifier_call = bq2415x_notifier_call;
 +   ret = power_supply_reg_notifier(bq-nb);
 +   if (ret) {
 +   dev_err(bq-dev, failed to reg notifier: %d\n, ret);
 +   goto error_5;
 +   }
 +   psy = power_supply_get_by_name(bq-init_data.notify_device);
 +   if (psy) {
 +   /* Query for initial reported_mode and set it */
 +   bq2415x_notifier_call(bq-nb,
 +   PSY_EVENT_PROP_CHANGED, psy);
 bq2415x_set_mode(bq, bq-reported_mode);
 -   dev_info(bq-dev, automode enabled\n);
 } else {
 -

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Sunday 24 November 2013 18:00:00 Sebastian Reichel wrote:
 Hi,
 
 On Tue, Nov 19, 2013 at 11:18:04AM +0100, Pali Rohár wrote:
  This patch removing set_mode_hook function from board data
  and replacing it with new string variable of notifier power
  supply device. After this change it is possible to add DT
  support because driver does not need specific board
  function anymore. Only static data and name of power supply
  device is required.
 
 I'm wondering if the watchdog thread should check some values,
 like e.g. battery temperature. It should stop charging the
 battery if some critical battery temperature threshold is
 reached.
 
 -- Sebastian

For checking battery temperature is needed third driver 
(rx51_battery.ko). And I think this should not be implemented in 
driver itself but in charger manager framework...

Currently on Maemo 5 this is handled in userspace (with open 
source dsme daemon).

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Sunday 24 November 2013 18:18:03 Michael Trimarchi wrote:
 Hi
 
 On Tue, Nov 19, 2013 at 11:18 AM, Pali Rohár 
pali.ro...@gmail.com wrote:
  This patch removing set_mode_hook function from board data
  and replacing it with new string variable of notifier power
  supply device. After this change it is possible to add DT
  support because driver does not need specific board
  function anymore. Only static data and name of power supply
  device is required.
  
  Signed-off-by: Pali Rohár pali.ro...@gmail.com
  ---
  
   drivers/power/bq2415x_charger.c   |   77
   +
   include/linux/power/bq2415x_charger.h |   48
   +++- 2 files changed, 65 insertions(+), 60
   deletions(-)
  
...
  
  -   struct bq2415x_device *bq = data;
  +   struct bq2415x_device *bq =
  +   container_of(nb, struct bq2415x_device, nb);
  +   struct power_supply *psy = v;
  +   enum bq2415x_mode mode;
  +   union power_supply_propval prop;
  +   int ret;
  +   int mA;
  
  -   if (!bq)
  -   return;
  +   if (val != PSY_EVENT_PROP_CHANGED)
  +   return NOTIFY_OK;
  +
  +   if (strcmp(psy-name, bq-init_data.notify_device)
  != 0) +   return NOTIFY_OK;
  +
  +   dev_dbg(bq-dev, notifier call was called\n);
  +
  +   ret = psy-get_property(psy,
  POWER_SUPPLY_PROP_CURRENT_MAX, prop); +   if (ret !=
  0)
  +   return NOTIFY_OK;
 
 So you can read this value without any type of synchronization
 with the power_supply_core
 and sysfs implementation?
 
 
 Michael
 

I do not see reason why I cannot use it. When isp1704 driver send 
PSY_EVENT_PROP_CHANGED then property 
POWER_SUPPLY_PROP_CURRENT_MAX is already updated and can be read 
by get_property function.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Sun, Nov 24, 2013 at 08:01:16PM +0100, Pali Rohár wrote:
  I'm wondering if the watchdog thread should check some values,
  like e.g. battery temperature. It should stop charging the
  battery if some critical battery temperature threshold is
  reached.
 
 For checking battery temperature is needed third driver 
 (rx51_battery.ko).

I know, but that doesn't mean one can skip the check.

 And I think this should not be implemented in driver itself but in
 charger manager framework...

There is a reason, that the HW has a watchdog. I think the safety
relevant stuff should be done in the watchdog thread. This ensures,
that charging depends on reading the safety relevant sensors.

Of course the watchdog stuff could be moved into the charger manager
framework...

 Currently on Maemo 5 this is handled in userspace (with open 
 source dsme daemon).

I assume it currently also takes care of the bq2415x watchdog? That
means if the daemon dies charging will stop, because the watchdog
does no longer trigger.

When your patch is applied you have introduced a safety issue.
When the daemon dies charging will continue and temperature is
no longer checked.

-- Sebastian


signature.asc
Description: Digital signature

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode

On Sunday 24 November 2013 20:26:09 Sebastian Reichel wrote:
 On Sun, Nov 24, 2013 at 08:01:16PM +0100, Pali Rohár wrote:
  Currently on Maemo 5 this is handled in userspace (with open
  source dsme daemon).
 
 I assume it currently also takes care of the bq2415x watchdog?
 That means if the daemon dies charging will stop, because the
 watchdog does no longer trigger.
 
 When your patch is applied you have introduced a safety issue.
 When the daemon dies charging will continue and temperature is
 no longer checked.
 
 -- Sebastian

No dsme checking battery temperature and does not handle bq24510 
timer (previously this was in closed bme daemon which 
functionality is now in kernel drivers). But dsme daemon also 
kicking tlw4030 watchdog, so when daemon dies after 30s tlw4030 
reboot device.

But right you can implement correctly this in userspace (e.g. 
when daemon not running/crashed, you can restart daemon or reboot 
system or disable charing, whatever...) and you do not need to 
have it in kernel...

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode