| Message ID | 1384856285-19593-3-git-send-email-pali.rohar@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue 2013-11-19 11:18:04, Pali Rohár wrote: > This patch removing set_mode_hook function from board data and replacing it with > new string variable of notifier power supply device. After this change it is > possible to add DT support because driver does not need specific board function > anymore. Only static data and name of power supply device is required. > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> Reviewed-by: Pavel Machek <pavel@ucw.cz> Pavel
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
Hi On Tue, Nov 19, 2013 at 11:18 AM, Pali Rohár <pali.rohar@gmail.com> wrote: > This patch removing set_mode_hook function from board data and replacing it with > new string variable of notifier power supply device. After this change it is > possible to add DT support because driver does not need specific board function > anymore. Only static data and name of power supply device is required. > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > --- > drivers/power/bq2415x_charger.c | 77 +++++++++++++++++++++++++-------- > include/linux/power/bq2415x_charger.h | 48 +++----------------- > 2 files changed, 65 insertions(+), 60 deletions(-) > > diff --git a/drivers/power/bq2415x_charger.c b/drivers/power/bq2415x_charger.c > index 0727f92..d89583d 100644 > --- a/drivers/power/bq2415x_charger.c > +++ b/drivers/power/bq2415x_charger.c > @@ -1,7 +1,7 @@ > /* > * bq2415x charger driver > * > - * Copyright (C) 2011-2012 Pali Rohár <pali.rohar@gmail.com> > + * Copyright (C) 2011-2013 Pali Rohár <pali.rohar@gmail.com> > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License as published by > @@ -170,6 +170,7 @@ struct bq2415x_device { > struct bq2415x_platform_data init_data; > struct power_supply charger; > struct delayed_work work; > + struct notifier_block nb; > enum bq2415x_mode reported_mode;/* mode reported by hook function */ > enum bq2415x_mode mode; /* current configured mode */ > enum bq2415x_chip chip; > @@ -791,24 +792,53 @@ static int bq2415x_set_mode(struct bq2415x_device *bq, enum bq2415x_mode mode) > > } > > -/* hook function called by other driver which set reported mode */ > -static void bq2415x_hook_function(enum bq2415x_mode mode, void *data) > +static int bq2415x_notifier_call(struct notifier_block *nb, > + unsigned long val, void *v) > { > - struct bq2415x_device *bq = data; > + struct bq2415x_device *bq = > + container_of(nb, struct bq2415x_device, nb); > + struct power_supply *psy = v; > + enum bq2415x_mode mode; > + union power_supply_propval prop; > + int ret; > + int mA; > > - if (!bq) > - return; > + if (val != PSY_EVENT_PROP_CHANGED) > + return NOTIFY_OK; > + > + if (strcmp(psy->name, bq->init_data.notify_device) != 0) > + return NOTIFY_OK; > + > + dev_dbg(bq->dev, "notifier call was called\n"); > + > + ret = psy->get_property(psy, POWER_SUPPLY_PROP_CURRENT_MAX, &prop); > + if (ret != 0) > + return NOTIFY_OK; So you can read this value without any type of synchronization with the power_supply_core and sysfs implementation? Michael > + > + mA = prop.intval; > + > + if (mA == 0) > + mode = BQ2415X_MODE_OFF; > + else if (mA < 500) > + mode = BQ2415X_MODE_NONE; > + else if (mA < 1800) > + mode = BQ2415X_MODE_HOST_CHARGER; > + else > + mode = BQ2415X_MODE_DEDICATED_CHARGER; > + > + if (bq->reported_mode == mode) > + return NOTIFY_OK; > > - dev_dbg(bq->dev, "hook function was called\n"); > bq->reported_mode = mode; > > /* if automode is not enabled do not tell about reported_mode */ > if (bq->automode < 1) > - return; > + return NOTIFY_OK; > > sysfs_notify(&bq->charger.dev->kobj, NULL, "reported_mode"); > bq2415x_set_mode(bq, bq->reported_mode); > > + return NOTIFY_OK; > } > > /**** timer functions ****/ > @@ -1508,6 +1538,7 @@ static int bq2415x_probe(struct i2c_client *client, > int num; > char *name; > struct bq2415x_device *bq; > + struct power_supply *psy; > > if (!client->dev.platform_data) { > dev_err(&client->dev, "platform data not set\n"); > @@ -1569,16 +1600,27 @@ static int bq2415x_probe(struct i2c_client *client, > goto error_4; > } > > - if (bq->init_data.set_mode_hook) { > - if (bq->init_data.set_mode_hook( > - bq2415x_hook_function, bq)) { > - bq->automode = 1; > + if (bq->init_data.notify_device) { > + bq->nb.notifier_call = bq2415x_notifier_call; > + ret = power_supply_reg_notifier(&bq->nb); > + if (ret) { > + dev_err(bq->dev, "failed to reg notifier: %d\n", ret); > + goto error_5; > + } > + psy = power_supply_get_by_name(bq->init_data.notify_device); > + if (psy) { > + /* Query for initial reported_mode and set it */ > + bq2415x_notifier_call(&bq->nb, > + PSY_EVENT_PROP_CHANGED, psy); > bq2415x_set_mode(bq, bq->reported_mode); > - dev_info(bq->dev, "automode enabled\n"); > } else { > - bq->automode = -1; > - dev_info(bq->dev, "automode failed\n"); > + dev_info(bq->dev, "notifier power supply device (%s) " > + "for automode is not registred yet... " > + "automode will not work without that device\n", > + bq->init_data.notify_device); > } > + bq->automode = 1; > + dev_info(bq->dev, "automode enabled\n"); > } else { > bq->automode = -1; > dev_info(bq->dev, "automode not supported\n"); > @@ -1590,6 +1632,7 @@ static int bq2415x_probe(struct i2c_client *client, > dev_info(bq->dev, "driver registered\n"); > return 0; > > +error_5: > error_4: > bq2415x_sysfs_exit(bq); > error_3: > @@ -1610,8 +1653,8 @@ static int bq2415x_remove(struct i2c_client *client) > { > struct bq2415x_device *bq = i2c_get_clientdata(client); > > - if (bq->init_data.set_mode_hook) > - bq->init_data.set_mode_hook(NULL, NULL); > + if (bq->init_data.notify_device) > + power_supply_unreg_notifier(&bq->nb); > > bq2415x_sysfs_exit(bq); > bq2415x_power_supply_exit(bq); > diff --git a/include/linux/power/bq2415x_charger.h b/include/linux/power/bq2415x_charger.h > index 8dcc0f4..50762af 100644 > --- a/include/linux/power/bq2415x_charger.h > +++ b/include/linux/power/bq2415x_charger.h > @@ -1,7 +1,7 @@ > /* > * bq2415x charger driver > * > - * Copyright (C) 2011-2012 Pali Rohár <pali.rohar@gmail.com> > + * Copyright (C) 2011-2013 Pali Rohár <pali.rohar@gmail.com> > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License as published by > @@ -31,46 +31,9 @@ > * termination current. It it is less or equal to zero, configuring charge > * and termination current will not be possible. > * > - * Function set_mode_hook is needed for automode (setting correct current > - * limit when charger is connected/disconnected or setting boost mode). > - * When is NULL, automode function is disabled. When is not NULL, it must > - * have this prototype: > - * > - * int (*set_mode_hook)( > - * void (*hook)(enum bq2415x_mode mode, void *data), > - * void *data) > - * > - * hook is hook function (see below) and data is pointer to driver private > - * data > - * > - * bq2415x driver will call it as: > - * > - * platform_data->set_mode_hook(bq2415x_hook_function, bq2415x_device); > - * > - * Board/platform function set_mode_hook return non zero value when hook > - * function was successful registered. Platform code should call that hook > - * function (which get from pointer, with data) every time when charger > - * was connected/disconnected or require to enable boost mode. bq2415x > - * driver then will set correct current limit, enable/disable charger or > - * boost mode. > - * > - * Hook function has this prototype: > - * > - * void hook(enum bq2415x_mode mode, void *data); > - * > - * mode is bq2415x mode (charger or boost) > - * data is pointer to driver private data (which get from > - * set_charger_type_hook) > - * > - * When bq driver is being unloaded, it call function: > - * > - * platform_data->set_mode_hook(NULL, NULL); > - * > - * (hook function and driver private data are NULL) > - * > - * After that board/platform code must not call driver hook function! It > - * is possible that pointer to hook function will not be valid and calling > - * will cause undefined result. > + * For automode support is needed to provide name of power supply device > + * in value notify_device. Device driver must immediately report property > + * POWER_SUPPLY_PROP_CURRENT_MAX when current changed. > */ > > /* Supported modes with maximal current limit */ > @@ -89,8 +52,7 @@ struct bq2415x_platform_data { > int charge_current; /* mA */ > int termination_current; /* mA */ > int resistor_sense; /* m ohm */ > - int (*set_mode_hook)(void (*hook)(enum bq2415x_mode mode, void *data), > - void *data); > + const char *notify_device; /* name */ > }; > > #endif > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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).
On Sunday 24 November 2013 18:18:03 Michael Trimarchi wrote: > Hi > > On Tue, Nov 19, 2013 at 11:18 AM, Pali Rohár <pali.rohar@gmail.com> wrote: > > This patch removing set_mode_hook function from board data > > and replacing it with new string variable of notifier power > > supply device. After this change it is possible to add DT > > support because driver does not need specific board > > function anymore. Only static data and name of power supply > > device is required. > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > --- > > > > drivers/power/bq2415x_charger.c | 77 > > +++++++++++++++++++++++++-------- > > include/linux/power/bq2415x_charger.h | 48 > > +++----------------- 2 files changed, 65 insertions(+), 60 > > deletions(-) > > ... > > > > - struct bq2415x_device *bq = data; > > + struct bq2415x_device *bq = > > + container_of(nb, struct bq2415x_device, nb); > > + struct power_supply *psy = v; > > + enum bq2415x_mode mode; > > + union power_supply_propval prop; > > + int ret; > > + int mA; > > > > - if (!bq) > > - return; > > + if (val != PSY_EVENT_PROP_CHANGED) > > + return NOTIFY_OK; > > + > > + if (strcmp(psy->name, bq->init_data.notify_device) > > != 0) + return NOTIFY_OK; > > + > > + dev_dbg(bq->dev, "notifier call was called\n"); > > + > > + ret = psy->get_property(psy, > > POWER_SUPPLY_PROP_CURRENT_MAX, &prop); + if (ret != > > 0) > > + return NOTIFY_OK; > > So you can read this value without any type of synchronization > with the power_supply_core > and sysfs implementation? > > > Michael > I do not see reason why I cannot use it. When isp1704 driver send PSY_EVENT_PROP_CHANGED then property POWER_SUPPLY_PROP_CURRENT_MAX is already updated and can be read by get_property function.
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
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...
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
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
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.
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
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.
Hi On Sun, Nov 24, 2013 at 8:01 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Sunday 24 November 2013 18:18:03 Michael Trimarchi wrote: >> Hi >> >> On Tue, Nov 19, 2013 at 11:18 AM, Pali Rohár > <pali.rohar@gmail.com> wrote: >> > This patch removing set_mode_hook function from board data >> > and replacing it with new string variable of notifier power >> > supply device. After this change it is possible to add DT >> > support because driver does not need specific board >> > function anymore. Only static data and name of power supply >> > device is required. >> > >> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> >> > --- >> > >> > drivers/power/bq2415x_charger.c | 77 >> > +++++++++++++++++++++++++-------- >> > include/linux/power/bq2415x_charger.h | 48 >> > +++----------------- 2 files changed, 65 insertions(+), 60 >> > deletions(-) >> > > ... >> > >> > - struct bq2415x_device *bq = data; >> > + struct bq2415x_device *bq = >> > + container_of(nb, struct bq2415x_device, nb); >> > + struct power_supply *psy = v; >> > + enum bq2415x_mode mode; >> > + union power_supply_propval prop; >> > + int ret; >> > + int mA; >> > >> > - if (!bq) >> > - return; >> > + if (val != PSY_EVENT_PROP_CHANGED) >> > + return NOTIFY_OK; >> > + >> > + if (strcmp(psy->name, bq->init_data.notify_device) >> > != 0) + return NOTIFY_OK; >> > + >> > + dev_dbg(bq->dev, "notifier call was called\n"); >> > + >> > + ret = psy->get_property(psy, >> > POWER_SUPPLY_PROP_CURRENT_MAX, &prop); + if (ret != >> > 0) >> > + return NOTIFY_OK; >> >> So you can read this value without any type of synchronization >> with the power_supply_core >> and sysfs implementation? >> >> >> Michael >> > > I do not see reason why I cannot use it. When isp1704 driver send > PSY_EVENT_PROP_CHANGED then property > POWER_SUPPLY_PROP_CURRENT_MAX is already updated and can be read > by get_property function. > https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html I found and equivalent scenario that I was trying to said Michael > -- > Pali Rohár > pali.rohar@gmail.com -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Anton On Sun, Dec 1, 2013 at 11:37 PM, Anton Vorontsov <anton@enomsg.org> wrote: > On Mon, Nov 25, 2013 at 08:16:34PM +0100, Michael Trimarchi wrote: > ... >> >> So you can read this value without any type of synchronization >> >> with the power_supply_core >> >> and sysfs implementation? > ... >> https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html >> >> I found and equivalent scenario that I was trying to said > > That's a good question, actually. Even though in Pali's case the notifier > is atomic (so that we are pretty confident that the object won't be > unregistered), there is still a possiblity of a dead lock, for example. So So if the get_property is a sleeping function it will be a deadlock. Right? Michael > notifiers should be careful to not hold any locks, because the other > driver might call get_property(), which might acquire locks. > > Thanks, > > Anton -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 02, 2013 at 01:02:40AM +0100, Michael Trimarchi wrote: > On Sun, Dec 1, 2013 at 11:37 PM, Anton Vorontsov <anton@enomsg.org> wrote: > > On Mon, Nov 25, 2013 at 08:16:34PM +0100, Michael Trimarchi wrote: > > ... > >> >> So you can read this value without any type of synchronization > >> >> with the power_supply_core > >> >> and sysfs implementation? > > ... > >> https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html > >> > >> I found and equivalent scenario that I was trying to said > > > > That's a good question, actually. Even though in Pali's case the notifier > > is atomic (so that we are pretty confident that the object won't be > > unregistered), there is still a possiblity of a dead lock, for example. So > > So if the get_property is a sleeping function it will be a deadlock. Right? All kind of bad things might happen, yes. But before that I would expect a bunch of warnings from might_sleep() stuff. I would recommend to test the patches using preempt/smp kernels + various DEBUG_ kernel options set. Thanks, Anton -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On Mon, Dec 2, 2013 at 1:24 AM, Anton Vorontsov <anton@enomsg.org> wrote: > On Mon, Dec 02, 2013 at 01:02:40AM +0100, Michael Trimarchi wrote: >> On Sun, Dec 1, 2013 at 11:37 PM, Anton Vorontsov <anton@enomsg.org> wrote: >> > On Mon, Nov 25, 2013 at 08:16:34PM +0100, Michael Trimarchi wrote: >> > ... >> >> >> So you can read this value without any type of synchronization >> >> >> with the power_supply_core >> >> >> and sysfs implementation? >> > ... >> >> https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html >> >> >> >> I found and equivalent scenario that I was trying to said >> > >> > That's a good question, actually. Even though in Pali's case the notifier >> > is atomic (so that we are pretty confident that the object won't be >> > unregistered), there is still a possiblity of a dead lock, for example. So >> >> So if the get_property is a sleeping function it will be a deadlock. Right? > > All kind of bad things might happen, yes. But before that I would expect a > bunch of warnings from might_sleep() stuff. > > I would recommend to test the patches using preempt/smp kernels + various > DEBUG_ kernel options set. > Is it more simple to make it not atomic and use a mutex in get_property? Michael > Thanks, > > Anton -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 19 November 2013 11:18:04 Pali Rohár wrote: > This patch removing set_mode_hook function from board data and > replacing it with new string variable of notifier power > supply device. After this change it is possible to add DT > support because driver does not need specific board function > anymore. Only static data and name of power supply device is > required. > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > --- > drivers/power/bq2415x_charger.c | 77 > +++++++++++++++++++++++++-------- > include/linux/power/bq2415x_charger.h | 48 > +++----------------- 2 files changed, 65 insertions(+), 60 > deletions(-) > Hi Anton, is there any problem with this patch?
On Tue, Nov 19, 2013 at 02:24:16PM +0100, Pavel Machek wrote: > On Tue 2013-11-19 11:18:04, Pali Rohár wrote: > > This patch removing set_mode_hook function from board data and replacing it with > > new string variable of notifier power supply device. After this change it is > > possible to add DT support because driver does not need specific board function > > anymore. Only static data and name of power supply device is required. > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > Reviewed-by: Pavel Machek <pavel@ucw.cz> Applied, thanks! Anton -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 02, 2013 at 02:45:06AM +0100, Michael Trimarchi wrote: > On Mon, Dec 2, 2013 at 1:24 AM, Anton Vorontsov <anton@enomsg.org> wrote: > > On Mon, Dec 02, 2013 at 01:02:40AM +0100, Michael Trimarchi wrote: > >> On Sun, Dec 1, 2013 at 11:37 PM, Anton Vorontsov <anton@enomsg.org> wrote: > >> > On Mon, Nov 25, 2013 at 08:16:34PM +0100, Michael Trimarchi wrote: > >> > ... > >> >> >> So you can read this value without any type of synchronization > >> >> >> with the power_supply_core > >> >> >> and sysfs implementation? > >> > ... > >> >> https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html > >> >> > >> >> I found and equivalent scenario that I was trying to said > >> > > >> > That's a good question, actually. Even though in Pali's case the notifier > >> > is atomic (so that we are pretty confident that the object won't be > >> > unregistered), there is still a possiblity of a dead lock, for example. So > >> > >> So if the get_property is a sleeping function it will be a deadlock. Right? > > > > All kind of bad things might happen, yes. But before that I would expect a > > bunch of warnings from might_sleep() stuff. > > > > I would recommend to test the patches using preempt/smp kernels + various > > DEBUG_ kernel options set. > > > > Is it more simple to make it not atomic and use a mutex in get_property? I just built a kernel with CONFIG_DEBUG_ATOMIC_SLEEP to test another driver and got the following output for bq2415x: [ 7.667449] Workqueue: events power_supply_changed_work [ 7.673034] [<c0015c28>] (unwind_backtrace+0x0/0xe0) from [<c0011e1c>] (show_stack+0x10/0x14) [ 7.682098] [<c0011e1c>] (show_stack+0x10/0x14) from [<c052cdd0>] (dump_stack+0x78/0xac) [ 7.690704] [<c052cdd0>] (dump_stack+0x78/0xac) from [<c052a044>] (__schedule_bug+0x48/0x60) [ 7.699645] [<c052a044>] (__schedule_bug+0x48/0x60) from [<c053071c>] (__schedule+0x74/0x638) [ 7.708618] [<c053071c>] (__schedule+0x74/0x638) from [<c05301fc>] (schedule_timeout+0x1dc/0x24c) [ 7.718017] [<c05301fc>] (schedule_timeout+0x1dc/0x24c) from [<c05316ec>] (wait_for_common+0x138/0x17c) [ 7.727966] [<c05316ec>] (wait_for_common+0x138/0x17c) from [<c0362a70>] (omap_i2c_xfer+0x340/0x4a0) [ 7.737640] [<c0362a70>] (omap_i2c_xfer+0x340/0x4a0) from [<c035d928>] (__i2c_transfer+0x40/0x74) [ 7.747039] [<c035d928>] (__i2c_transfer+0x40/0x74) from [<c035e22c>] (i2c_transfer+0x6c/0x90) [ 7.756195] [<c035e22c>] (i2c_transfer+0x6c/0x90) from [<c037ad24>] (bq2415x_i2c_write+0x48/0x78) [ 7.765563] [<c037ad24>] (bq2415x_i2c_write+0x48/0x78) from [<c037ae60>] (bq2415x_set_weak_battery_voltage+0x4c/0x50) [ 7.776824] [<c037ae60>] (bq2415x_set_weak_battery_voltage+0x4c/0x50) from [<c037bce8>] (bq2415x_set_mode+0xdc/0x14c) [ 7.788085] [<c037bce8>] (bq2415x_set_mode+0xdc/0x14c) from [<c037bfb8>] (bq2415x_notifier_call+0xa8/0xb4) [ 7.798309] [<c037bfb8>] (bq2415x_notifier_call+0xa8/0xb4) from [<c005f228>] (notifier_call_chain+0x38/0x68) [ 7.808715] [<c005f228>] (notifier_call_chain+0x38/0x68) from [<c005f284>] (__atomic_notifier_call_chain+0x2c/0x3c) [ 7.819732] [<c005f284>] (__atomic_notifier_call_chain+0x2c/0x3c) from [<c005f2a8>] (atomic_notifier_call_chain+0x14/0x18) [ 7.831420] [<c005f2a8>] (atomic_notifier_call_chain+0x14/0x18) from [<c0378078>] (power_supply_changed_work+0x6c/0xb8) [ 7.842864] [<c0378078>] (power_supply_changed_work+0x6c/0xb8) from [<c00556c0>] (process_one_work+0x248/0x440) [ 7.853546] [<c00556c0>] (process_one_work+0x248/0x440) from [<c0055d6c>] (worker_thread+0x208/0x350) [ 7.863372] [<c0055d6c>] (worker_thread+0x208/0x350) from [<c005b0ac>] (kthread+0xc8/0xdc) [ 7.872131] [<c005b0ac>] (kthread+0xc8/0xdc) from [<c000e138>] (ret_from_fork+0x14/0x3c) -- Sebastian
Hi On Sun, Jan 19, 2014 at 9:54 PM, Sebastian Reichel <sre@ring0.de> wrote: > On Mon, Dec 02, 2013 at 02:45:06AM +0100, Michael Trimarchi wrote: >> On Mon, Dec 2, 2013 at 1:24 AM, Anton Vorontsov <anton@enomsg.org> wrote: >> > On Mon, Dec 02, 2013 at 01:02:40AM +0100, Michael Trimarchi wrote: >> >> On Sun, Dec 1, 2013 at 11:37 PM, Anton Vorontsov <anton@enomsg.org> wrote: >> >> > On Mon, Nov 25, 2013 at 08:16:34PM +0100, Michael Trimarchi wrote: >> >> > ... >> >> >> >> So you can read this value without any type of synchronization >> >> >> >> with the power_supply_core >> >> >> >> and sysfs implementation? >> >> > ... >> >> >> https://lists.ubuntu.com/archives/kernel-team/2013-January/025206.html >> >> >> >> >> >> I found and equivalent scenario that I was trying to said >> >> > >> >> > That's a good question, actually. Even though in Pali's case the notifier >> >> > is atomic (so that we are pretty confident that the object won't be >> >> > unregistered), there is still a possiblity of a dead lock, for example. So >> >> >> >> So if the get_property is a sleeping function it will be a deadlock. Right? >> > >> > All kind of bad things might happen, yes. But before that I would expect a >> > bunch of warnings from might_sleep() stuff. >> > >> > I would recommend to test the patches using preempt/smp kernels + various >> > DEBUG_ kernel options set. >> > >> >> Is it more simple to make it not atomic and use a mutex in get_property? > > I just built a kernel with CONFIG_DEBUG_ATOMIC_SLEEP to test another > driver and got the following output for bq2415x: > > [ 7.667449] Workqueue: events power_supply_changed_work > [ 7.673034] [<c0015c28>] (unwind_backtrace+0x0/0xe0) from [<c0011e1c>] (show_stack+0x10/0x14) > [ 7.682098] [<c0011e1c>] (show_stack+0x10/0x14) from [<c052cdd0>] (dump_stack+0x78/0xac) > [ 7.690704] [<c052cdd0>] (dump_stack+0x78/0xac) from [<c052a044>] (__schedule_bug+0x48/0x60) > [ 7.699645] [<c052a044>] (__schedule_bug+0x48/0x60) from [<c053071c>] (__schedule+0x74/0x638) > [ 7.708618] [<c053071c>] (__schedule+0x74/0x638) from [<c05301fc>] (schedule_timeout+0x1dc/0x24c) > [ 7.718017] [<c05301fc>] (schedule_timeout+0x1dc/0x24c) from [<c05316ec>] (wait_for_common+0x138/0x17c) > [ 7.727966] [<c05316ec>] (wait_for_common+0x138/0x17c) from [<c0362a70>] (omap_i2c_xfer+0x340/0x4a0) > [ 7.737640] [<c0362a70>] (omap_i2c_xfer+0x340/0x4a0) from [<c035d928>] (__i2c_transfer+0x40/0x74) > [ 7.747039] [<c035d928>] (__i2c_transfer+0x40/0x74) from [<c035e22c>] (i2c_transfer+0x6c/0x90) > [ 7.756195] [<c035e22c>] (i2c_transfer+0x6c/0x90) from [<c037ad24>] (bq2415x_i2c_write+0x48/0x78) > [ 7.765563] [<c037ad24>] (bq2415x_i2c_write+0x48/0x78) from [<c037ae60>] (bq2415x_set_weak_battery_voltage+0x4c/0x50) > [ 7.776824] [<c037ae60>] (bq2415x_set_weak_battery_voltage+0x4c/0x50) from [<c037bce8>] (bq2415x_set_mode+0xdc/0x14c) > [ 7.788085] [<c037bce8>] (bq2415x_set_mode+0xdc/0x14c) from [<c037bfb8>] (bq2415x_notifier_call+0xa8/0xb4) > [ 7.798309] [<c037bfb8>] (bq2415x_notifier_call+0xa8/0xb4) from [<c005f228>] (notifier_call_chain+0x38/0x68) > [ 7.808715] [<c005f228>] (notifier_call_chain+0x38/0x68) from [<c005f284>] (__atomic_notifier_call_chain+0x2c/0x3c) > [ 7.819732] [<c005f284>] (__atomic_notifier_call_chain+0x2c/0x3c) from [<c005f2a8>] (atomic_notifier_call_chain+0x14/0x18) > [ 7.831420] [<c005f2a8>] (atomic_notifier_call_chain+0x14/0x18) from [<c0378078>] (power_supply_changed_work+0x6c/0xb8) > [ 7.842864] [<c0378078>] (power_supply_changed_work+0x6c/0xb8) from [<c00556c0>] (process_one_work+0x248/0x440) > [ 7.853546] [<c00556c0>] (process_one_work+0x248/0x440) from [<c0055d6c>] (worker_thread+0x208/0x350) > [ 7.863372] [<c0055d6c>] (worker_thread+0x208/0x350) from [<c005b0ac>] (kthread+0xc8/0xdc) > [ 7.872131] [<c005b0ac>] (kthread+0xc8/0xdc) from [<c000e138>] (ret_from_fork+0x14/0x3c) > > -- Sebastian I have already give my opinion about this problem Michael -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
And another one using that evil mail-followup-to header:
Mail-Followup-To: Pali Rohár <pali.rohar@gmail.com>,
Anton Vorontsov <anton@enomsg.org>,
Michael Trimarchi <michael@amarulasolutions.com>,
David Woodhouse <dwmw2@infradead.org>,
Tony Lindgren <tony@atomide.com>,
Russell King <linux@arm.linux.org.uk>, linux-kernel@vger.kernel.org,
Linux OMAP Mailing List <linux-omap@vger.kernel.org>,
freemangordon@abv.bg, aaro.koskinen@iki.fi, pavel@ucw.cz
which results in all the above being moved into the To: header by
unsuspecting recipients who reply to your message.
On Mon, Jan 20, 2014 at 10:21:29AM +0000, Russell King - ARM Linux wrote: > And another one using that evil mail-followup-to header: > > Mail-Followup-To: Pali Rohár <pali.rohar@gmail.com>, > Anton Vorontsov <anton@enomsg.org>, > Michael Trimarchi <michael@amarulasolutions.com>, > David Woodhouse <dwmw2@infradead.org>, > Tony Lindgren <tony@atomide.com>, > Russell King <linux@arm.linux.org.uk>, linux-kernel@vger.kernel.org, > Linux OMAP Mailing List <linux-omap@vger.kernel.org>, > freemangordon@abv.bg, aaro.koskinen@iki.fi, pavel@ucw.cz > > which results in all the above being moved into the To: header by > unsuspecting recipients who reply to your message. Thanks for the hint. Should be fixed now. -- Sebastian
diff --git a/drivers/power/bq2415x_charger.c b/drivers/power/bq2415x_charger.c index 0727f92..d89583d 100644 --- a/drivers/power/bq2415x_charger.c +++ b/drivers/power/bq2415x_charger.c @@ -1,7 +1,7 @@ /* * bq2415x charger driver * - * Copyright (C) 2011-2012 Pali Rohár <pali.rohar@gmail.com> + * Copyright (C) 2011-2013 Pali Rohár <pali.rohar@gmail.com> * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -170,6 +170,7 @@ struct bq2415x_device { struct bq2415x_platform_data init_data; struct power_supply charger; struct delayed_work work; + struct notifier_block nb; enum bq2415x_mode reported_mode;/* mode reported by hook function */ enum bq2415x_mode mode; /* current configured mode */ enum bq2415x_chip chip; @@ -791,24 +792,53 @@ static int bq2415x_set_mode(struct bq2415x_device *bq, enum bq2415x_mode mode) } -/* hook function called by other driver which set reported mode */ -static void bq2415x_hook_function(enum bq2415x_mode mode, void *data) +static int bq2415x_notifier_call(struct notifier_block *nb, + unsigned long val, void *v) { - struct bq2415x_device *bq = data; + struct bq2415x_device *bq = + container_of(nb, struct bq2415x_device, nb); + struct power_supply *psy = v; + enum bq2415x_mode mode; + union power_supply_propval prop; + int ret; + int mA; - if (!bq) - return; + if (val != PSY_EVENT_PROP_CHANGED) + return NOTIFY_OK; + + if (strcmp(psy->name, bq->init_data.notify_device) != 0) + return NOTIFY_OK; + + dev_dbg(bq->dev, "notifier call was called\n"); + + ret = psy->get_property(psy, POWER_SUPPLY_PROP_CURRENT_MAX, &prop); + if (ret != 0) + return NOTIFY_OK; + + mA = prop.intval; + + if (mA == 0) + mode = BQ2415X_MODE_OFF; + else if (mA < 500) + mode = BQ2415X_MODE_NONE; + else if (mA < 1800) + mode = BQ2415X_MODE_HOST_CHARGER; + else + mode = BQ2415X_MODE_DEDICATED_CHARGER; + + if (bq->reported_mode == mode) + return NOTIFY_OK; - dev_dbg(bq->dev, "hook function was called\n"); bq->reported_mode = mode; /* if automode is not enabled do not tell about reported_mode */ if (bq->automode < 1) - return; + return NOTIFY_OK; sysfs_notify(&bq->charger.dev->kobj, NULL, "reported_mode"); bq2415x_set_mode(bq, bq->reported_mode); + return NOTIFY_OK; } /**** timer functions ****/ @@ -1508,6 +1538,7 @@ static int bq2415x_probe(struct i2c_client *client, int num; char *name; struct bq2415x_device *bq; + struct power_supply *psy; if (!client->dev.platform_data) { dev_err(&client->dev, "platform data not set\n"); @@ -1569,16 +1600,27 @@ static int bq2415x_probe(struct i2c_client *client, goto error_4; } - if (bq->init_data.set_mode_hook) { - if (bq->init_data.set_mode_hook( - bq2415x_hook_function, bq)) { - bq->automode = 1; + if (bq->init_data.notify_device) { + bq->nb.notifier_call = bq2415x_notifier_call; + ret = power_supply_reg_notifier(&bq->nb); + if (ret) { + dev_err(bq->dev, "failed to reg notifier: %d\n", ret); + goto error_5; + } + psy = power_supply_get_by_name(bq->init_data.notify_device); + if (psy) { + /* Query for initial reported_mode and set it */ + bq2415x_notifier_call(&bq->nb, + PSY_EVENT_PROP_CHANGED, psy); bq2415x_set_mode(bq, bq->reported_mode); - dev_info(bq->dev, "automode enabled\n"); } else { - bq->automode = -1; - dev_info(bq->dev, "automode failed\n"); + dev_info(bq->dev, "notifier power supply device (%s) " + "for automode is not registred yet... " + "automode will not work without that device\n", + bq->init_data.notify_device); } + bq->automode = 1; + dev_info(bq->dev, "automode enabled\n"); } else { bq->automode = -1; dev_info(bq->dev, "automode not supported\n"); @@ -1590,6 +1632,7 @@ static int bq2415x_probe(struct i2c_client *client, dev_info(bq->dev, "driver registered\n"); return 0; +error_5: error_4: bq2415x_sysfs_exit(bq); error_3: @@ -1610,8 +1653,8 @@ static int bq2415x_remove(struct i2c_client *client) { struct bq2415x_device *bq = i2c_get_clientdata(client); - if (bq->init_data.set_mode_hook) - bq->init_data.set_mode_hook(NULL, NULL); + if (bq->init_data.notify_device) + power_supply_unreg_notifier(&bq->nb); bq2415x_sysfs_exit(bq); bq2415x_power_supply_exit(bq); diff --git a/include/linux/power/bq2415x_charger.h b/include/linux/power/bq2415x_charger.h index 8dcc0f4..50762af 100644 --- a/include/linux/power/bq2415x_charger.h +++ b/include/linux/power/bq2415x_charger.h @@ -1,7 +1,7 @@ /* * bq2415x charger driver * - * Copyright (C) 2011-2012 Pali Rohár <pali.rohar@gmail.com> + * Copyright (C) 2011-2013 Pali Rohár <pali.rohar@gmail.com> * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -31,46 +31,9 @@ * termination current. It it is less or equal to zero, configuring charge * and termination current will not be possible. * - * Function set_mode_hook is needed for automode (setting correct current - * limit when charger is connected/disconnected or setting boost mode). - * When is NULL, automode function is disabled. When is not NULL, it must - * have this prototype: - * - * int (*set_mode_hook)( - * void (*hook)(enum bq2415x_mode mode, void *data), - * void *data) - * - * hook is hook function (see below) and data is pointer to driver private - * data - * - * bq2415x driver will call it as: - * - * platform_data->set_mode_hook(bq2415x_hook_function, bq2415x_device); - * - * Board/platform function set_mode_hook return non zero value when hook - * function was successful registered. Platform code should call that hook - * function (which get from pointer, with data) every time when charger - * was connected/disconnected or require to enable boost mode. bq2415x - * driver then will set correct current limit, enable/disable charger or - * boost mode. - * - * Hook function has this prototype: - * - * void hook(enum bq2415x_mode mode, void *data); - * - * mode is bq2415x mode (charger or boost) - * data is pointer to driver private data (which get from - * set_charger_type_hook) - * - * When bq driver is being unloaded, it call function: - * - * platform_data->set_mode_hook(NULL, NULL); - * - * (hook function and driver private data are NULL) - * - * After that board/platform code must not call driver hook function! It - * is possible that pointer to hook function will not be valid and calling - * will cause undefined result. + * For automode support is needed to provide name of power supply device + * in value notify_device. Device driver must immediately report property + * POWER_SUPPLY_PROP_CURRENT_MAX when current changed. */ /* Supported modes with maximal current limit */ @@ -89,8 +52,7 @@ struct bq2415x_platform_data { int charge_current; /* mA */ int termination_current; /* mA */ int resistor_sense; /* m ohm */ - int (*set_mode_hook)(void (*hook)(enum bq2415x_mode mode, void *data), - void *data); + const char *notify_device; /* name */ }; #endif
This patch removing set_mode_hook function from board data and replacing it with new string variable of notifier power supply device. After this change it is possible to add DT support because driver does not need specific board function anymore. Only static data and name of power supply device is required. Signed-off-by: Pali Rohár <pali.rohar@gmail.com> --- drivers/power/bq2415x_charger.c | 77 +++++++++++++++++++++++++-------- include/linux/power/bq2415x_charger.h | 48 +++----------------- 2 files changed, 65 insertions(+), 60 deletions(-)