* [PATCH v2 4/6] power: bq24190_charger: Call power_supply_changed() for relevant component
@ 2017-01-16 6:08 Liam Breck
2017-01-16 18:31 ` Mark Greer
0 siblings, 1 reply; 6+ messages in thread
From: Liam Breck @ 2017-01-16 6:08 UTC (permalink / raw)
To: linux-pm
Cc: Sebastian Reichel, Tony Lindgren, Mark A . Greer, Liam Breck,
Matt Ranostay
[-- Attachment #1: Type: text/plain, Size: 1 bytes --]
[-- Attachment #2: patch-v2-4of6.txt --]
[-- Type: text/plain, Size: 4640 bytes --]
We wrongly get uevents for bq24190-charger and bq24190-battery on every
register change.
Fix by checking the association with charger and battery before
emitting uevent(s).
Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190 Battery Charger")
Cc: Mark A. Greer <mgreer@animalcreek.com>
Cc: Matt Ranostay <matt@ranostay.consulting>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
drivers/power/supply/bq24190_charger.c | 50 +++++++++++++++++++---------------
1 file changed, 27 insertions(+), 23 deletions(-)
diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 62194d8..ba5a5b2 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -159,7 +159,6 @@ struct bq24190_dev_info {
unsigned int gpio_int;
unsigned int irq;
struct mutex f_reg_lock;
- bool first_time;
bool charger_health_valid;
bool battery_health_valid;
bool battery_status_valid;
@@ -1197,7 +1196,10 @@ static const struct power_supply_desc bq24190_battery_desc = {
static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
{
struct bq24190_dev_info *bdi = data;
- bool alert_userspace = false;
+ const u8 battery_mask_ss = BQ24190_REG_SS_CHRG_STAT_MASK;
+ const u8 battery_mask_f = BQ24190_REG_F_BAT_FAULT_MASK
+ | BQ24190_REG_F_NTC_FAULT_MASK;
+ bool alert_charger = false, alert_battery = false;
u8 ss_reg = 0, f_reg = 0;
int ret;
@@ -1225,8 +1227,12 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
ret);
}
+ if ((bdi->ss_reg & battery_mask_ss) != (ss_reg & battery_mask_ss))
+ alert_battery = true;
+ if ((bdi->ss_reg & ~battery_mask_ss) != (ss_reg & ~battery_mask_ss))
+ alert_charger = true;
+
bdi->ss_reg = ss_reg;
- alert_userspace = true;
}
mutex_lock(&bdi->f_reg_lock);
@@ -1239,33 +1245,23 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
}
if (f_reg != bdi->f_reg) {
+ if ((bdi->f_reg & battery_mask_f) != (f_reg & battery_mask_f))
+ alert_battery = true;
+ if ((bdi->f_reg & ~battery_mask_f) != (f_reg & ~battery_mask_f))
+ alert_charger = true;
+
bdi->f_reg = f_reg;
bdi->charger_health_valid = true;
bdi->battery_health_valid = true;
bdi->battery_status_valid = true;
-
- alert_userspace = true;
}
mutex_unlock(&bdi->f_reg_lock);
- /*
- * Sometimes bq24190 gives a steady trickle of interrupts even
- * though the watchdog timer is turned off and neither the STATUS
- * nor FAULT registers have changed. Weed out these sprurious
- * interrupts so userspace isn't alerted for no reason.
- * In addition, the chip always generates an interrupt after
- * register reset so we should ignore that one (the very first
- * interrupt received).
- */
- if (alert_userspace) {
- if (!bdi->first_time) {
- power_supply_changed(bdi->charger);
- power_supply_changed(bdi->battery);
- } else {
- bdi->first_time = false;
- }
- }
+ if (alert_charger)
+ power_supply_changed(bdi->charger);
+ if (alert_battery)
+ power_supply_changed(bdi->battery);
out:
pm_runtime_put_sync(bdi->dev);
@@ -1300,6 +1296,10 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
goto out;
ret = bq24190_set_mode_host(bdi);
+ if (ret < 0)
+ goto out;
+
+ ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
out:
pm_runtime_put_sync(bdi->dev);
return ret;
@@ -1375,7 +1375,8 @@ static int bq24190_probe(struct i2c_client *client,
bdi->model = id->driver_data;
strncpy(bdi->model_name, id->name, I2C_NAME_SIZE);
mutex_init(&bdi->f_reg_lock);
- bdi->first_time = true;
+ bdi->f_reg = 0;
+ bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
bdi->charger_health_valid = false;
bdi->battery_health_valid = false;
bdi->battery_status_valid = false;
@@ -1491,13 +1492,16 @@ static int bq24190_pm_resume(struct device *dev)
struct i2c_client *client = to_i2c_client(dev);
struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+ bdi->f_reg = 0;
+ bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
bdi->charger_health_valid = false;
bdi->battery_health_valid = false;
bdi->battery_status_valid = false;
pm_runtime_get_sync(bdi->dev);
bq24190_register_reset(bdi);
bq24190_set_mode_host(bdi);
+ bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
pm_runtime_put_sync(bdi->dev);
/* Things may have changed while suspended so alert upper layer */
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 4/6] power: bq24190_charger: Call power_supply_changed() for relevant component
2017-01-16 6:08 [PATCH v2 4/6] power: bq24190_charger: Call power_supply_changed() for relevant component Liam Breck
@ 2017-01-16 18:31 ` Mark Greer
2017-01-16 18:58 ` Mark Greer
2017-01-16 19:55 ` Liam Breck
0 siblings, 2 replies; 6+ messages in thread
From: Mark Greer @ 2017-01-16 18:31 UTC (permalink / raw)
To: Liam Breck
Cc: linux-pm, Sebastian Reichel, Tony Lindgren, Liam Breck, Matt Ranostay
On Sun, Jan 15, 2017 at 10:08:04PM -0800, Liam Breck wrote:
>
> We wrongly get uevents for bq24190-charger and bq24190-battery on every
> register change.
>
> Fix by checking the association with charger and battery before
> emitting uevent(s).
>
> Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190 Battery Charger")
> Cc: Mark A. Greer <mgreer@animalcreek.com>
> Cc: Matt Ranostay <matt@ranostay.consulting>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
> drivers/power/supply/bq24190_charger.c | 50 +++++++++++++++++++---------------
> 1 file changed, 27 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 62194d8..ba5a5b2 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1225,8 +1227,12 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
> ret);
> }
>
> + if ((bdi->ss_reg & battery_mask_ss) != (ss_reg & battery_mask_ss))
> + alert_battery = true;
> + if ((bdi->ss_reg & ~battery_mask_ss) != (ss_reg & ~battery_mask_ss))
Lines are > 80 characters. Please break up.
The rest looks good.
Mark
--
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 4/6] power: bq24190_charger: Call power_supply_changed() for relevant component
2017-01-16 18:31 ` Mark Greer
@ 2017-01-16 18:58 ` Mark Greer
2017-01-16 19:55 ` Liam Breck
1 sibling, 0 replies; 6+ messages in thread
From: Mark Greer @ 2017-01-16 18:58 UTC (permalink / raw)
To: Liam Breck
Cc: linux-pm, Sebastian Reichel, Tony Lindgren, Liam Breck, Matt Ranostay
On Mon, Jan 16, 2017 at 11:31:25AM -0700, Mark Greer wrote:
> On Sun, Jan 15, 2017 at 10:08:04PM -0800, Liam Breck wrote:
> >
>
> > We wrongly get uevents for bq24190-charger and bq24190-battery on every
> > register change.
> >
> > Fix by checking the association with charger and battery before
> > emitting uevent(s).
> >
> > Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190 Battery Charger")
> > Cc: Mark A. Greer <mgreer@animalcreek.com>
> > Cc: Matt Ranostay <matt@ranostay.consulting>
> > Cc: Tony Lindgren <tony@atomide.com>
> > Signed-off-by: Liam Breck <kernel@networkimprov.net>
> > ---
> > drivers/power/supply/bq24190_charger.c | 50 +++++++++++++++++++---------------
> > 1 file changed, 27 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> > index 62194d8..ba5a5b2 100644
> > --- a/drivers/power/supply/bq24190_charger.c
> > +++ b/drivers/power/supply/bq24190_charger.c
>
> > @@ -1225,8 +1227,12 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
> > ret);
> > }
> >
> > + if ((bdi->ss_reg & battery_mask_ss) != (ss_reg & battery_mask_ss))
> > + alert_battery = true;
> > + if ((bdi->ss_reg & ~battery_mask_ss) != (ss_reg & ~battery_mask_ss))
>
> Lines are > 80 characters. Please break up.
>
> The rest looks good.
Assuming you fix the >80 chars issues:
Acked-by: Mark Greer <mgreer@animalcreek.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 4/6] power: bq24190_charger: Call power_supply_changed() for relevant component
2017-01-16 18:31 ` Mark Greer
2017-01-16 18:58 ` Mark Greer
@ 2017-01-16 19:55 ` Liam Breck
2017-01-17 1:54 ` Mark Greer
1 sibling, 1 reply; 6+ messages in thread
From: Liam Breck @ 2017-01-16 19:55 UTC (permalink / raw)
To: Mark Greer
Cc: linux-pm, Sebastian Reichel, Tony Lindgren, Liam Breck, Matt Ranostay
On Mon, Jan 16, 2017 at 10:31 AM, Mark Greer <mgreer@animalcreek.com> wrote:
> On Sun, Jan 15, 2017 at 10:08:04PM -0800, Liam Breck wrote:
>>
>
>> We wrongly get uevents for bq24190-charger and bq24190-battery on every
>> register change.
>>
>> Fix by checking the association with charger and battery before
>> emitting uevent(s).
>>
>> Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190 Battery Charger")
>> Cc: Mark A. Greer <mgreer@animalcreek.com>
>> Cc: Matt Ranostay <matt@ranostay.consulting>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> ---
>> drivers/power/supply/bq24190_charger.c | 50 +++++++++++++++++++---------------
>> 1 file changed, 27 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
>> index 62194d8..ba5a5b2 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>
>> @@ -1225,8 +1227,12 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>> ret);
>> }
>>
>> + if ((bdi->ss_reg & battery_mask_ss) != (ss_reg & battery_mask_ss))
>> + alert_battery = true;
>> + if ((bdi->ss_reg & ~battery_mask_ss) != (ss_reg & ~battery_mask_ss))
>
> Lines are > 80 characters. Please break up.
I did this deliberately, because the same logic appears for f_reg in
that function, and this way you can see that at a glance.
My research on 80-char limit found that it's not a hard rule.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 4/6] power: bq24190_charger: Call power_supply_changed() for relevant component
2017-01-16 19:55 ` Liam Breck
@ 2017-01-17 1:54 ` Mark Greer
2017-01-17 2:30 ` Tony Lindgren
0 siblings, 1 reply; 6+ messages in thread
From: Mark Greer @ 2017-01-17 1:54 UTC (permalink / raw)
To: Liam Breck
Cc: linux-pm, Sebastian Reichel, Tony Lindgren, Liam Breck, Matt Ranostay
On Mon, Jan 16, 2017 at 11:55:22AM -0800, Liam Breck wrote:
> On Mon, Jan 16, 2017 at 10:31 AM, Mark Greer <mgreer@animalcreek.com> wrote:
> > On Sun, Jan 15, 2017 at 10:08:04PM -0800, Liam Breck wrote:
> >>
> >
> >> We wrongly get uevents for bq24190-charger and bq24190-battery on every
> >> register change.
> >>
> >> Fix by checking the association with charger and battery before
> >> emitting uevent(s).
> >>
> >> Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190 Battery Charger")
> >> Cc: Mark A. Greer <mgreer@animalcreek.com>
> >> Cc: Matt Ranostay <matt@ranostay.consulting>
> >> Cc: Tony Lindgren <tony@atomide.com>
> >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> >> ---
> >> drivers/power/supply/bq24190_charger.c | 50 +++++++++++++++++++---------------
> >> 1 file changed, 27 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> >> index 62194d8..ba5a5b2 100644
> >> --- a/drivers/power/supply/bq24190_charger.c
> >> +++ b/drivers/power/supply/bq24190_charger.c
> >
> >> @@ -1225,8 +1227,12 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
> >> ret);
> >> }
> >>
> >> + if ((bdi->ss_reg & battery_mask_ss) != (ss_reg & battery_mask_ss))
> >> + alert_battery = true;
> >> + if ((bdi->ss_reg & ~battery_mask_ss) != (ss_reg & ~battery_mask_ss))
> >
> > Lines are > 80 characters. Please break up.
>
> I did this deliberately, because the same logic appears for f_reg in
> that function, and this way you can see that at a glance.
That's because those lines are <= 80 chars. :)
> My research on 80-char limit found that it's not a hard rule.
Section 2 of Documentation/process/coding-style.rst makes it pretty clear
along with exceptions.
Mark
--
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 4/6] power: bq24190_charger: Call power_supply_changed() for relevant component
2017-01-17 1:54 ` Mark Greer
@ 2017-01-17 2:30 ` Tony Lindgren
0 siblings, 0 replies; 6+ messages in thread
From: Tony Lindgren @ 2017-01-17 2:30 UTC (permalink / raw)
To: Mark Greer
Cc: Liam Breck, linux-pm, Sebastian Reichel, Liam Breck, Matt Ranostay
* Mark Greer <mgreer@animalcreek.com> [170116 17:55]:
> On Mon, Jan 16, 2017 at 11:55:22AM -0800, Liam Breck wrote:
> > On Mon, Jan 16, 2017 at 10:31 AM, Mark Greer <mgreer@animalcreek.com> wrote:
> > > On Sun, Jan 15, 2017 at 10:08:04PM -0800, Liam Breck wrote:
> > >>
> > >
> > >> We wrongly get uevents for bq24190-charger and bq24190-battery on every
> > >> register change.
> > >>
> > >> Fix by checking the association with charger and battery before
> > >> emitting uevent(s).
> > >>
> > >> Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190 Battery Charger")
> > >> Cc: Mark A. Greer <mgreer@animalcreek.com>
> > >> Cc: Matt Ranostay <matt@ranostay.consulting>
> > >> Cc: Tony Lindgren <tony@atomide.com>
> > >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> > >> ---
> > >> drivers/power/supply/bq24190_charger.c | 50 +++++++++++++++++++---------------
> > >> 1 file changed, 27 insertions(+), 23 deletions(-)
> > >>
> > >> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> > >> index 62194d8..ba5a5b2 100644
> > >> --- a/drivers/power/supply/bq24190_charger.c
> > >> +++ b/drivers/power/supply/bq24190_charger.c
> > >
> > >> @@ -1225,8 +1227,12 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
> > >> ret);
> > >> }
> > >>
> > >> + if ((bdi->ss_reg & battery_mask_ss) != (ss_reg & battery_mask_ss))
> > >> + alert_battery = true;
> > >> + if ((bdi->ss_reg & ~battery_mask_ss) != (ss_reg & ~battery_mask_ss))
> > >
> > > Lines are > 80 characters. Please break up.
> >
> > I did this deliberately, because the same logic appears for f_reg in
> > that function, and this way you can see that at a glance.
>
> That's because those lines are <= 80 chars. :)
>
> > My research on 80-char limit found that it's not a hard rule.
>
> Section 2 of Documentation/process/coding-style.rst makes it pretty clear
> along with exceptions.
Yeah and it's a good idea to avoid anything that can cause extra discussion
for nothing ;)
Tony
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-17 2:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-16 6:08 [PATCH v2 4/6] power: bq24190_charger: Call power_supply_changed() for relevant component Liam Breck
2017-01-16 18:31 ` Mark Greer
2017-01-16 18:58 ` Mark Greer
2017-01-16 19:55 ` Liam Breck
2017-01-17 1:54 ` Mark Greer
2017-01-17 2:30 ` Tony Lindgren
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.