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

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

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

[Sebastian Reichel]

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

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


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

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

[Sebastian Reichel]

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

On Mon, Jan 20, 2014 at 10:21:29AM +0000, Russell King - ARM Linux wrote:
> And another one using that evil mail-followup-to header:
> 
> Mail-Followup-To: Pali Rohár <pali.rohar@gmail.com>,
>         Anton Vorontsov <anton@enomsg.org>,
>         Michael Trimarchi <michael@amarulasolutions.com>,
>         David Woodhouse <dwmw2@infradead.org>,
>         Tony Lindgren <tony@atomide.com>,
>         Russell King <linux@arm.linux.org.uk>, linux-kernel@vger.kernel.org,
>         Linux OMAP Mailing List <linux-omap@vger.kernel.org>,
>         freemangordon@abv.bg, aaro.koskinen@iki.fi, pavel@ucw.cz
> 
> which results in all the above being moved into the To: header by
> unsuspecting recipients who reply to your message.

Thanks for the hint. Should be fixed now.

-- Sebastian

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

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

[Russell King - ARM Linux]

And another one using that evil mail-followup-to header:

Mail-Followup-To: Pali Rohár <pali.rohar@gmail.com>,
        Anton Vorontsov <anton@enomsg.org>,
        Michael Trimarchi <michael@amarulasolutions.com>,
        David Woodhouse <dwmw2@infradead.org>,
        Tony Lindgren <tony@atomide.com>,
        Russell King <linux@arm.linux.org.uk>, linux-kernel@vger.kernel.org,
        Linux OMAP Mailing List <linux-omap@vger.kernel.org>,
        freemangordon@abv.bg, aaro.koskinen@iki.fi, pavel@ucw.cz

which results in all the above being moved into the To: header by
unsuspecting recipients who reply to your message.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Michael Trimarchi]

Hi

On Sun, Jan 19, 2014 at 9:54 PM, Sebastian Reichel <sre@ring0.de> wrote:
> On Mon, Dec 02, 2013 at 02:45:06AM +0100, Michael Trimarchi wrote:
>> On Mon, Dec 2, 2013 at 1:24 AM, Anton Vorontsov <anton@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 <anton@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?
>
> I just built a kernel with CONFIG_DEBUG_ATOMIC_SLEEP to test another
> driver and got the following output for bq2415x:
>
> [    7.667449] Workqueue: events power_supply_changed_work
> [    7.673034] [<c0015c28>] (unwind_backtrace+0x0/0xe0) from [<c0011e1c>] (show_stack+0x10/0x14)
> [    7.682098] [<c0011e1c>] (show_stack+0x10/0x14) from [<c052cdd0>] (dump_stack+0x78/0xac)
> [    7.690704] [<c052cdd0>] (dump_stack+0x78/0xac) from [<c052a044>] (__schedule_bug+0x48/0x60)
> [    7.699645] [<c052a044>] (__schedule_bug+0x48/0x60) from [<c053071c>] (__schedule+0x74/0x638)
> [    7.708618] [<c053071c>] (__schedule+0x74/0x638) from [<c05301fc>] (schedule_timeout+0x1dc/0x24c)
> [    7.718017] [<c05301fc>] (schedule_timeout+0x1dc/0x24c) from [<c05316ec>] (wait_for_common+0x138/0x17c)
> [    7.727966] [<c05316ec>] (wait_for_common+0x138/0x17c) from [<c0362a70>] (omap_i2c_xfer+0x340/0x4a0)
> [    7.737640] [<c0362a70>] (omap_i2c_xfer+0x340/0x4a0) from [<c035d928>] (__i2c_transfer+0x40/0x74)
> [    7.747039] [<c035d928>] (__i2c_transfer+0x40/0x74) from [<c035e22c>] (i2c_transfer+0x6c/0x90)
> [    7.756195] [<c035e22c>] (i2c_transfer+0x6c/0x90) from [<c037ad24>] (bq2415x_i2c_write+0x48/0x78)
> [    7.765563] [<c037ad24>] (bq2415x_i2c_write+0x48/0x78) from [<c037ae60>] (bq2415x_set_weak_battery_voltage+0x4c/0x50)
> [    7.776824] [<c037ae60>] (bq2415x_set_weak_battery_voltage+0x4c/0x50) from [<c037bce8>] (bq2415x_set_mode+0xdc/0x14c)
> [    7.788085] [<c037bce8>] (bq2415x_set_mode+0xdc/0x14c) from [<c037bfb8>] (bq2415x_notifier_call+0xa8/0xb4)
> [    7.798309] [<c037bfb8>] (bq2415x_notifier_call+0xa8/0xb4) from [<c005f228>] (notifier_call_chain+0x38/0x68)
> [    7.808715] [<c005f228>] (notifier_call_chain+0x38/0x68) from [<c005f284>] (__atomic_notifier_call_chain+0x2c/0x3c)
> [    7.819732] [<c005f284>] (__atomic_notifier_call_chain+0x2c/0x3c) from [<c005f2a8>] (atomic_notifier_call_chain+0x14/0x18)
> [    7.831420] [<c005f2a8>] (atomic_notifier_call_chain+0x14/0x18) from [<c0378078>] (power_supply_changed_work+0x6c/0xb8)
> [    7.842864] [<c0378078>] (power_supply_changed_work+0x6c/0xb8) from [<c00556c0>] (process_one_work+0x248/0x440)
> [    7.853546] [<c00556c0>] (process_one_work+0x248/0x440) from [<c0055d6c>] (worker_thread+0x208/0x350)
> [    7.863372] [<c0055d6c>] (worker_thread+0x208/0x350) from [<c005b0ac>] (kthread+0xc8/0xdc)
> [    7.872131] [<c005b0ac>] (kthread+0xc8/0xdc) from [<c000e138>] (ret_from_fork+0x14/0x3c)
>
> -- Sebastian


I have already give my opinion about this problem

Michael

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

[Sebastian Reichel]

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

On Mon, Dec 02, 2013 at 02:45:06AM +0100, Michael Trimarchi wrote:
> On Mon, Dec 2, 2013 at 1:24 AM, Anton Vorontsov <anton@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 <anton@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?

I just built a kernel with CONFIG_DEBUG_ATOMIC_SLEEP to test another
driver and got the following output for bq2415x:

[    7.667449] Workqueue: events power_supply_changed_work
[    7.673034] [<c0015c28>] (unwind_backtrace+0x0/0xe0) from [<c0011e1c>] (show_stack+0x10/0x14)
[    7.682098] [<c0011e1c>] (show_stack+0x10/0x14) from [<c052cdd0>] (dump_stack+0x78/0xac)
[    7.690704] [<c052cdd0>] (dump_stack+0x78/0xac) from [<c052a044>] (__schedule_bug+0x48/0x60)
[    7.699645] [<c052a044>] (__schedule_bug+0x48/0x60) from [<c053071c>] (__schedule+0x74/0x638)
[    7.708618] [<c053071c>] (__schedule+0x74/0x638) from [<c05301fc>] (schedule_timeout+0x1dc/0x24c)
[    7.718017] [<c05301fc>] (schedule_timeout+0x1dc/0x24c) from [<c05316ec>] (wait_for_common+0x138/0x17c)
[    7.727966] [<c05316ec>] (wait_for_common+0x138/0x17c) from [<c0362a70>] (omap_i2c_xfer+0x340/0x4a0)
[    7.737640] [<c0362a70>] (omap_i2c_xfer+0x340/0x4a0) from [<c035d928>] (__i2c_transfer+0x40/0x74)
[    7.747039] [<c035d928>] (__i2c_transfer+0x40/0x74) from [<c035e22c>] (i2c_transfer+0x6c/0x90)
[    7.756195] [<c035e22c>] (i2c_transfer+0x6c/0x90) from [<c037ad24>] (bq2415x_i2c_write+0x48/0x78)
[    7.765563] [<c037ad24>] (bq2415x_i2c_write+0x48/0x78) from [<c037ae60>] (bq2415x_set_weak_battery_voltage+0x4c/0x50)
[    7.776824] [<c037ae60>] (bq2415x_set_weak_battery_voltage+0x4c/0x50) from [<c037bce8>] (bq2415x_set_mode+0xdc/0x14c)
[    7.788085] [<c037bce8>] (bq2415x_set_mode+0xdc/0x14c) from [<c037bfb8>] (bq2415x_notifier_call+0xa8/0xb4)
[    7.798309] [<c037bfb8>] (bq2415x_notifier_call+0xa8/0xb4) from [<c005f228>] (notifier_call_chain+0x38/0x68)
[    7.808715] [<c005f228>] (notifier_call_chain+0x38/0x68) from [<c005f284>] (__atomic_notifier_call_chain+0x2c/0x3c)
[    7.819732] [<c005f284>] (__atomic_notifier_call_chain+0x2c/0x3c) from [<c005f2a8>] (atomic_notifier_call_chain+0x14/0x18)
[    7.831420] [<c005f2a8>] (atomic_notifier_call_chain+0x14/0x18) from [<c0378078>] (power_supply_changed_work+0x6c/0xb8)
[    7.842864] [<c0378078>] (power_supply_changed_work+0x6c/0xb8) from [<c00556c0>] (process_one_work+0x248/0x440)
[    7.853546] [<c00556c0>] (process_one_work+0x248/0x440) from [<c0055d6c>] (worker_thread+0x208/0x350)
[    7.863372] [<c0055d6c>] (worker_thread+0x208/0x350) from [<c005b0ac>] (kthread+0xc8/0xdc)
[    7.872131] [<c005b0ac>] (kthread+0xc8/0xdc) from [<c000e138>] (ret_from_fork+0x14/0x3c)

-- Sebastian

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

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

[Anton Vorontsov]

On Tue, Nov 19, 2013 at 02:24:16PM +0100, Pavel Machek wrote:
> On Tue 2013-11-19 11:18:04, 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 <pali.rohar@gmail.com>
> 
> Reviewed-by: Pavel Machek <pavel@ucw.cz>

Applied, thanks!

Anton

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

[Pali Rohár]

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

On Tuesday 19 November 2013 11:18:04 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 <pali.rohar@gmail.com>
> ---
>  drivers/power/bq2415x_charger.c       |   77
> +++++++++++++++++++++++++--------
> include/linux/power/bq2415x_charger.h |   48
> +++----------------- 2 files changed, 65 insertions(+), 60
> deletions(-)
> 

Hi Anton, is there any problem with this patch?

-- 
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/3] bq2415x_charger: Use power_supply notifier for automode

[Michael Trimarchi]

Hi

On Mon, Dec 2, 2013 at 1:24 AM, Anton Vorontsov <anton@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 <anton@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

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

[Anton Vorontsov]

On Mon, Dec 02, 2013 at 01:02:40AM +0100, Michael Trimarchi wrote:
> On Sun, Dec 1, 2013 at 11:37 PM, Anton Vorontsov <anton@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

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

[Michael Trimarchi]

Hi Anton

On Sun, Dec 1, 2013 at 11:37 PM, Anton Vorontsov <anton@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

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

[Anton Vorontsov]

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

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

[Pali Rohár]

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

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.rohar@gmail.com

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

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

[Sebastian Reichel]

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

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

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

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

[Pali Rohár]

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

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.rohar@gmail.com

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

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

[Sebastian Reichel]

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

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

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

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

[Michael Trimarchi]

Hi

On Sun, Nov 24, 2013 at 8:01 PM, Pali Rohár <pali.rohar@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.rohar@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.rohar@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.rohar@gmail.com

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

[Michael Trimarchi]

Hi

On Sun, Nov 24, 2013 at 8:01 PM, Pali Rohár <pali.rohar@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.rohar@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.rohar@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.rohar@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pali Rohár]

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

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.rohar@gmail.com

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

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.

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

[Pavel Machek]

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

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

[Pali Rohár]

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

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.rohar@gmail.com

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

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

[Pavel Machek]

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

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

[Sebastian Reichel]

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

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

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

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

[Pali Rohár]

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

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.rohar@gmail.com

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

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

[Sebastian Reichel]

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

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

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

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

[Pali Rohár]

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

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.rohar@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.rohar@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.rohar@gmail.com

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

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

[Pali Rohár]

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

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.rohar@gmail.com

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

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

[Michael Trimarchi]

Hi

On Tue, Nov 19, 2013 at 11:18 AM, Pali Rohár <pali.rohar@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.rohar@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.rohar@gmail.com>
> + * Copyright (C) 2011-2013  Pali Rohár <pali.rohar@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 {
> -                       bq->automode = -1;
> -                       dev_info(bq->dev, "automode failed\n");
> +                       dev_info(bq->dev, "notifier power supply device (%s) "
> +                               "for automode is not registred yet... "
> +                               "automode will not work without that device\n",
> +                               bq->init_data.notify_device);
>                 }
> +               bq->automode = 1;
> +               dev_info(bq->dev, "automode enabled\n");
>         } else {
>                 bq->automode = -1;
>                 dev_info(bq->dev, "automode not supported\n");
> @@ -1590,6 +1632,7 @@ static int bq2415x_probe(struct i2c_client *client,
>         dev_info(bq->dev, "driver registered\n");
>         return 0;
>
> +error_5:
>  error_4:
>         bq2415x_sysfs_exit(bq);
>  error_3:
> @@ -1610,8 +1653,8 @@ static int bq2415x_remove(struct i2c_client *client)
>  {
>         struct bq2415x_device *bq = i2c_get_clientdata(client);
>
> -       if (bq->init_data.set_mode_hook)
> -               bq->init_data.set_mode_hook(NULL, NULL);
> +       if (bq->init_data.notify_device)
> +               power_supply_unreg_notifier(&bq->nb);
>
>         bq2415x_sysfs_exit(bq);
>         bq2415x_power_supply_exit(bq);
> diff --git a/include/linux/power/bq2415x_charger.h b/include/linux/power/bq2415x_charger.h
> index 8dcc0f4..50762af 100644
> --- a/include/linux/power/bq2415x_charger.h
> +++ b/include/linux/power/bq2415x_charger.h
> @@ -1,7 +1,7 @@
>  /*
>   * bq2415x charger driver
>   *
> - * Copyright (C) 2011-2012  Pali Rohár <pali.rohar@gmail.com>
> + * Copyright (C) 2011-2013  Pali Rohár <pali.rohar@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
> @@ -31,46 +31,9 @@
>   * termination current. It it is less or equal to zero, configuring charge
>   * and termination current will not be possible.
>   *
> - * Function set_mode_hook is needed for automode (setting correct current
> - * limit when charger is connected/disconnected or setting boost mode).
> - * When is NULL, automode function is disabled. When is not NULL, it must
> - * have this prototype:
> - *
> - *    int (*set_mode_hook)(
> - *      void (*hook)(enum bq2415x_mode mode, void *data),
> - *      void *data)
> - *
> - * hook is hook function (see below) and data is pointer to driver private
> - * data
> - *
> - * bq2415x driver will call it as:
> - *
> - *    platform_data->set_mode_hook(bq2415x_hook_function, bq2415x_device);
> - *
> - * Board/platform function set_mode_hook return non zero value when hook
> - * function was successful registered. Platform code should call that hook
> - * function (which get from pointer, with data) every time when charger
> - * was connected/disconnected or require to enable boost mode. bq2415x
> - * driver then will set correct current limit, enable/disable charger or
> - * boost mode.
> - *
> - * Hook function has this prototype:
> - *
> - *    void hook(enum bq2415x_mode mode, void *data);
> - *
> - * mode is bq2415x mode (charger or boost)
> - * data is pointer to driver private data (which get from
> - * set_charger_type_hook)
> - *
> - * When bq driver is being unloaded, it call function:
> - *
> - *    platform_data->set_mode_hook(NULL, NULL);
> - *
> - * (hook function and driver private data are NULL)
> - *
> - * After that board/platform code must not call driver hook function! It
> - * is possible that pointer to hook function will not be valid and calling
> - * will cause undefined result.
> + * For automode support is needed to provide name of power supply device
> + * in value notify_device. Device driver must immediately report property
> + * POWER_SUPPLY_PROP_CURRENT_MAX when current changed.
>   */
>
>  /* Supported modes with maximal current limit */
> @@ -89,8 +52,7 @@ struct bq2415x_platform_data {
>         int charge_current;             /* mA */
>         int termination_current;        /* mA */
>         int resistor_sense;             /* m ohm */
> -       int (*set_mode_hook)(void (*hook)(enum bq2415x_mode mode, void *data),
> -                            void *data);
> +       const char *notify_device;      /* name */
>  };
>
>  #endif
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Michael Trimarchi]

Hi

On Tue, Nov 19, 2013 at 11:18 AM, Pali Rohár <pali.rohar@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.rohar@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.rohar@gmail.com>
> + * Copyright (C) 2011-2013  Pali Rohár <pali.rohar@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 {
> -                       bq->automode = -1;
> -                       dev_info(bq->dev, "automode failed\n");
> +                       dev_info(bq->dev, "notifier power supply device (%s) "
> +                               "for automode is not registred yet... "
> +                               "automode will not work without that device\n",
> +                               bq->init_data.notify_device);
>                 }
> +               bq->automode = 1;
> +               dev_info(bq->dev, "automode enabled\n");
>         } else {
>                 bq->automode = -1;
>                 dev_info(bq->dev, "automode not supported\n");
> @@ -1590,6 +1632,7 @@ static int bq2415x_probe(struct i2c_client *client,
>         dev_info(bq->dev, "driver registered\n");
>         return 0;
>
> +error_5:
>  error_4:
>         bq2415x_sysfs_exit(bq);
>  error_3:
> @@ -1610,8 +1653,8 @@ static int bq2415x_remove(struct i2c_client *client)
>  {
>         struct bq2415x_device *bq = i2c_get_clientdata(client);
>
> -       if (bq->init_data.set_mode_hook)
> -               bq->init_data.set_mode_hook(NULL, NULL);
> +       if (bq->init_data.notify_device)
> +               power_supply_unreg_notifier(&bq->nb);
>
>         bq2415x_sysfs_exit(bq);
>         bq2415x_power_supply_exit(bq);
> diff --git a/include/linux/power/bq2415x_charger.h b/include/linux/power/bq2415x_charger.h
> index 8dcc0f4..50762af 100644
> --- a/include/linux/power/bq2415x_charger.h
> +++ b/include/linux/power/bq2415x_charger.h
> @@ -1,7 +1,7 @@
>  /*
>   * bq2415x charger driver
>   *
> - * Copyright (C) 2011-2012  Pali Rohár <pali.rohar@gmail.com>
> + * Copyright (C) 2011-2013  Pali Rohár <pali.rohar@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
> @@ -31,46 +31,9 @@
>   * termination current. It it is less or equal to zero, configuring charge
>   * and termination current will not be possible.
>   *
> - * Function set_mode_hook is needed for automode (setting correct current
> - * limit when charger is connected/disconnected or setting boost mode).
> - * When is NULL, automode function is disabled. When is not NULL, it must
> - * have this prototype:
> - *
> - *    int (*set_mode_hook)(
> - *      void (*hook)(enum bq2415x_mode mode, void *data),
> - *      void *data)
> - *
> - * hook is hook function (see below) and data is pointer to driver private
> - * data
> - *
> - * bq2415x driver will call it as:
> - *
> - *    platform_data->set_mode_hook(bq2415x_hook_function, bq2415x_device);
> - *
> - * Board/platform function set_mode_hook return non zero value when hook
> - * function was successful registered. Platform code should call that hook
> - * function (which get from pointer, with data) every time when charger
> - * was connected/disconnected or require to enable boost mode. bq2415x
> - * driver then will set correct current limit, enable/disable charger or
> - * boost mode.
> - *
> - * Hook function has this prototype:
> - *
> - *    void hook(enum bq2415x_mode mode, void *data);
> - *
> - * mode is bq2415x mode (charger or boost)
> - * data is pointer to driver private data (which get from
> - * set_charger_type_hook)
> - *
> - * When bq driver is being unloaded, it call function:
> - *
> - *    platform_data->set_mode_hook(NULL, NULL);
> - *
> - * (hook function and driver private data are NULL)
> - *
> - * After that board/platform code must not call driver hook function! It
> - * is possible that pointer to hook function will not be valid and calling
> - * will cause undefined result.
> + * For automode support is needed to provide name of power supply device
> + * in value notify_device. Device driver must immediately report property
> + * POWER_SUPPLY_PROP_CURRENT_MAX when current changed.
>   */
>
>  /* Supported modes with maximal current limit */
> @@ -89,8 +52,7 @@ struct bq2415x_platform_data {
>         int charge_current;             /* mA */
>         int termination_current;        /* mA */
>         int resistor_sense;             /* m ohm */
> -       int (*set_mode_hook)(void (*hook)(enum bq2415x_mode mode, void *data),
> -                            void *data);
> +       const char *notify_device;      /* name */
>  };
>
>  #endif
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sebastian Reichel]

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

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

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

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

[Pavel Machek]

On Tue 2013-11-19 11:18:04, 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 <pali.rohar@gmail.com>

Reviewed-by: Pavel Machek <pavel@ucw.cz>
									Pavel

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

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

[Pali Rohár]

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.rohar@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.rohar@gmail.com>
+ * Copyright (C) 2011-2013  Pali Rohár <pali.rohar@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;
+
+	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 {
-			bq->automode = -1;
-			dev_info(bq->dev, "automode failed\n");
+			dev_info(bq->dev, "notifier power supply device (%s) "
+				"for automode is not registred yet... "
+				"automode will not work without that device\n",
+				bq->init_data.notify_device);
 		}
+		bq->automode = 1;
+		dev_info(bq->dev, "automode enabled\n");
 	} else {
 		bq->automode = -1;
 		dev_info(bq->dev, "automode not supported\n");
@@ -1590,6 +1632,7 @@ static int bq2415x_probe(struct i2c_client *client,
 	dev_info(bq->dev, "driver registered\n");
 	return 0;
 
+error_5:
 error_4:
 	bq2415x_sysfs_exit(bq);
 error_3:
@@ -1610,8 +1653,8 @@ static int bq2415x_remove(struct i2c_client *client)
 {
 	struct bq2415x_device *bq = i2c_get_clientdata(client);
 
-	if (bq->init_data.set_mode_hook)
-		bq->init_data.set_mode_hook(NULL, NULL);
+	if (bq->init_data.notify_device)
+		power_supply_unreg_notifier(&bq->nb);
 
 	bq2415x_sysfs_exit(bq);
 	bq2415x_power_supply_exit(bq);
diff --git a/include/linux/power/bq2415x_charger.h b/include/linux/power/bq2415x_charger.h
index 8dcc0f4..50762af 100644
--- a/include/linux/power/bq2415x_charger.h
+++ b/include/linux/power/bq2415x_charger.h
@@ -1,7 +1,7 @@
 /*
  * bq2415x charger driver
  *
- * Copyright (C) 2011-2012  Pali Rohár <pali.rohar@gmail.com>
+ * Copyright (C) 2011-2013  Pali Rohár <pali.rohar@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
@@ -31,46 +31,9 @@
  * termination current. It it is less or equal to zero, configuring charge
  * and termination current will not be possible.
  *
- * Function set_mode_hook is needed for automode (setting correct current
- * limit when charger is connected/disconnected or setting boost mode).
- * When is NULL, automode function is disabled. When is not NULL, it must
- * have this prototype:
- *
- *    int (*set_mode_hook)(
- *      void (*hook)(enum bq2415x_mode mode, void *data),
- *      void *data)
- *
- * hook is hook function (see below) and data is pointer to driver private
- * data
- *
- * bq2415x driver will call it as:
- *
- *    platform_data->set_mode_hook(bq2415x_hook_function, bq2415x_device);
- *
- * Board/platform function set_mode_hook return non zero value when hook
- * function was successful registered. Platform code should call that hook
- * function (which get from pointer, with data) every time when charger
- * was connected/disconnected or require to enable boost mode. bq2415x
- * driver then will set correct current limit, enable/disable charger or
- * boost mode.
- *
- * Hook function has this prototype:
- *
- *    void hook(enum bq2415x_mode mode, void *data);
- *
- * mode is bq2415x mode (charger or boost)
- * data is pointer to driver private data (which get from
- * set_charger_type_hook)
- *
- * When bq driver is being unloaded, it call function:
- *
- *    platform_data->set_mode_hook(NULL, NULL);
- *
- * (hook function and driver private data are NULL)
- *
- * After that board/platform code must not call driver hook function! It
- * is possible that pointer to hook function will not be valid and calling
- * will cause undefined result.
+ * For automode support is needed to provide name of power supply device
+ * in value notify_device. Device driver must immediately report property
+ * POWER_SUPPLY_PROP_CURRENT_MAX when current changed.
  */
 
 /* Supported modes with maximal current limit */
@@ -89,8 +52,7 @@ struct bq2415x_platform_data {
 	int charge_current;		/* mA */
 	int termination_current;	/* mA */
 	int resistor_sense;		/* m ohm */
-	int (*set_mode_hook)(void (*hook)(enum bq2415x_mode mode, void *data),
-			     void *data);
+	const char *notify_device;	/* name */
 };
 
 #endif
-- 
1.7.9.5