Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode
On Mon, Jan 20, 2014 at 10:21:29AM +, Russell King - ARM Linux wrote: > And another one using that evil mail-followup-to header: > > Mail-Followup-To: Pali Rohár , > Anton Vorontsov , > Michael Trimarchi , > David Woodhouse , > Tony Lindgren , > Russell King , linux-kernel@vger.kernel.org, > Linux OMAP Mailing List , > freemangor...@abv.bg, aaro.koski...@iki.fi, pa...@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 signature.asc Description: Digital signature
Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode
And another one using that evil mail-followup-to header: Mail-Followup-To: Pali Rohár , Anton Vorontsov , Michael Trimarchi , David Woodhouse , Tony Lindgren , Russell King , linux-kernel@vger.kernel.org, Linux OMAP Mailing List , freemangor...@abv.bg, aaro.koski...@iki.fi, pa...@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". -- 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
And another one using that evil mail-followup-to header: Mail-Followup-To: Pali Rohár pali.ro...@gmail.com, Anton Vorontsov an...@enomsg.org, Michael Trimarchi mich...@amarulasolutions.com, David Woodhouse dw...@infradead.org, Tony Lindgren t...@atomide.com, Russell King li...@arm.linux.org.uk, linux-kernel@vger.kernel.org, Linux OMAP Mailing List linux-o...@vger.kernel.org, freemangor...@abv.bg, aaro.koski...@iki.fi, pa...@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. -- 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, Jan 20, 2014 at 10:21:29AM +, Russell King - ARM Linux wrote: And another one using that evil mail-followup-to header: Mail-Followup-To: Pali Rohár pali.ro...@gmail.com, Anton Vorontsov an...@enomsg.org, Michael Trimarchi mich...@amarulasolutions.com, David Woodhouse dw...@infradead.org, Tony Lindgren t...@atomide.com, Russell King li...@arm.linux.org.uk, linux-kernel@vger.kernel.org, Linux OMAP Mailing List linux-o...@vger.kernel.org, freemangor...@abv.bg, aaro.koski...@iki.fi, pa...@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 signature.asc Description: Digital signature
Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode
Hi On Sun, Jan 19, 2014 at 9:54 PM, Sebastian Reichel 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 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? > > 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] [] (unwind_backtrace+0x0/0xe0) from [] > (show_stack+0x10/0x14) > [7.682098] [] (show_stack+0x10/0x14) from [] > (dump_stack+0x78/0xac) > [7.690704] [] (dump_stack+0x78/0xac) from [] > (__schedule_bug+0x48/0x60) > [7.699645] [] (__schedule_bug+0x48/0x60) from [] > (__schedule+0x74/0x638) > [7.708618] [] (__schedule+0x74/0x638) from [] > (schedule_timeout+0x1dc/0x24c) > [7.718017] [] (schedule_timeout+0x1dc/0x24c) from [] > (wait_for_common+0x138/0x17c) > [7.727966] [] (wait_for_common+0x138/0x17c) from [] > (omap_i2c_xfer+0x340/0x4a0) > [7.737640] [] (omap_i2c_xfer+0x340/0x4a0) from [] > (__i2c_transfer+0x40/0x74) > [7.747039] [] (__i2c_transfer+0x40/0x74) from [] > (i2c_transfer+0x6c/0x90) > [7.756195] [] (i2c_transfer+0x6c/0x90) from [] > (bq2415x_i2c_write+0x48/0x78) > [7.765563] [] (bq2415x_i2c_write+0x48/0x78) from [] > (bq2415x_set_weak_battery_voltage+0x4c/0x50) > [7.776824] [] (bq2415x_set_weak_battery_voltage+0x4c/0x50) from > [] (bq2415x_set_mode+0xdc/0x14c) > [7.788085] [] (bq2415x_set_mode+0xdc/0x14c) from [] > (bq2415x_notifier_call+0xa8/0xb4) > [7.798309] [] (bq2415x_notifier_call+0xa8/0xb4) from > [] (notifier_call_chain+0x38/0x68) > [7.808715] [] (notifier_call_chain+0x38/0x68) from [] > (__atomic_notifier_call_chain+0x2c/0x3c) > [7.819732] [] (__atomic_notifier_call_chain+0x2c/0x3c) from > [] (atomic_notifier_call_chain+0x14/0x18) > [7.831420] [] (atomic_notifier_call_chain+0x14/0x18) from > [] (power_supply_changed_work+0x6c/0xb8) > [7.842864] [] (power_supply_changed_work+0x6c/0xb8) from > [] (process_one_work+0x248/0x440) > [7.853546] [] (process_one_work+0x248/0x440) from [] > (worker_thread+0x208/0x350) > [7.863372] [] (worker_thread+0x208/0x350) from [] > (kthread+0xc8/0xdc) > [7.872131] [] (kthread+0xc8/0xdc) from [] > (ret_from_fork+0x14/0x3c) > > -- Sebastian I have already give my opinion about this problem Michael -- 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 02:45:06AM +0100, Michael Trimarchi wrote: > 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? 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] [] (unwind_backtrace+0x0/0xe0) from [] (show_stack+0x10/0x14) [7.682098] [] (show_stack+0x10/0x14) from [] (dump_stack+0x78/0xac) [7.690704] [] (dump_stack+0x78/0xac) from [] (__schedule_bug+0x48/0x60) [7.699645] [] (__schedule_bug+0x48/0x60) from [] (__schedule+0x74/0x638) [7.708618] [] (__schedule+0x74/0x638) from [] (schedule_timeout+0x1dc/0x24c) [7.718017] [] (schedule_timeout+0x1dc/0x24c) from [] (wait_for_common+0x138/0x17c) [7.727966] [] (wait_for_common+0x138/0x17c) from [] (omap_i2c_xfer+0x340/0x4a0) [7.737640] [] (omap_i2c_xfer+0x340/0x4a0) from [] (__i2c_transfer+0x40/0x74) [7.747039] [] (__i2c_transfer+0x40/0x74) from [] (i2c_transfer+0x6c/0x90) [7.756195] [] (i2c_transfer+0x6c/0x90) from [] (bq2415x_i2c_write+0x48/0x78) [7.765563] [] (bq2415x_i2c_write+0x48/0x78) from [] (bq2415x_set_weak_battery_voltage+0x4c/0x50) [7.776824] [] (bq2415x_set_weak_battery_voltage+0x4c/0x50) from [] (bq2415x_set_mode+0xdc/0x14c) [7.788085] [] (bq2415x_set_mode+0xdc/0x14c) from [] (bq2415x_notifier_call+0xa8/0xb4) [7.798309] [] (bq2415x_notifier_call+0xa8/0xb4) from [] (notifier_call_chain+0x38/0x68) [7.808715] [] (notifier_call_chain+0x38/0x68) from [] (__atomic_notifier_call_chain+0x2c/0x3c) [7.819732] [] (__atomic_notifier_call_chain+0x2c/0x3c) from [] (atomic_notifier_call_chain+0x14/0x18) [7.831420] [] (atomic_notifier_call_chain+0x14/0x18) from [] (power_supply_changed_work+0x6c/0xb8) [7.842864] [] (power_supply_changed_work+0x6c/0xb8) from [] (process_one_work+0x248/0x440) [7.853546] [] (process_one_work+0x248/0x440) from [] (worker_thread+0x208/0x350) [7.863372] [] (worker_thread+0x208/0x350) from [] (kthread+0xc8/0xdc) [7.872131] [] (kthread+0xc8/0xdc) from [] (ret_from_fork+0x14/0x3c) -- Sebastian signature.asc Description: Digital signature
Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode
On Mon, Dec 02, 2013 at 02:45:06AM +0100, Michael Trimarchi wrote: 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? 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 signature.asc Description: Digital signature
Re: [PATCH v2 2/3] bq2415x_charger: Use power_supply notifier for automode
Hi On Sun, Jan 19, 2014 at 9:54 PM, Sebastian Reichel s...@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 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? 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 -- 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 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 > > Reviewed-by: Pavel Machek Applied, 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 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.ro...@gmail.com Reviewed-by: Pavel Machek pa...@ucw.cz Applied, 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 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 > --- > 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.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 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.ro...@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.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, 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
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
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
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
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
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
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
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 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 Reviewed-by: Pavel Machek 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 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.ro...@gmail.com Reviewed-by: Pavel Machek pa...@ucw.cz 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/