| Message ID | 3291ca81bf8eb1b0401579ae08e7835e71dfc1ff.1588517058.git.mirq-linux@rere.qmqm.pl (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | power: supply: bq25890: fix and extend | expand |
Hi, On Sun, May 03, 2020 at 05:21:11PM +0200, Michał Mirosław wrote: > Extend bq->lock over whole updating of the chip's state. Might get > useful later for switching ADC modes correctly. > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > --- Thanks, queued. -- Sebastian > drivers/power/supply/bq25890_charger.c | 82 ++++++++------------------ > 1 file changed, 26 insertions(+), 56 deletions(-) > > diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c > index c4a69fd69f34..9339e216651f 100644 > --- a/drivers/power/supply/bq25890_charger.c > +++ b/drivers/power/supply/bq25890_charger.c > @@ -510,74 +510,50 @@ static int bq25890_get_chip_state(struct bq25890_device *bq, > return 0; > } > > -static bool bq25890_state_changed(struct bq25890_device *bq, > - struct bq25890_state *new_state) > -{ > - struct bq25890_state old_state; > - > - mutex_lock(&bq->lock); > - old_state = bq->state; > - mutex_unlock(&bq->lock); > - > - return (old_state.chrg_status != new_state->chrg_status || > - old_state.chrg_fault != new_state->chrg_fault || > - old_state.online != new_state->online || > - old_state.bat_fault != new_state->bat_fault || > - old_state.boost_fault != new_state->boost_fault || > - old_state.vsys_status != new_state->vsys_status); > -} > - > -static void bq25890_handle_state_change(struct bq25890_device *bq, > - struct bq25890_state *new_state) > +static irqreturn_t __bq25890_handle_irq(struct bq25890_device *bq) > { > + struct bq25890_state new_state; > int ret; > - struct bq25890_state old_state; > > - mutex_lock(&bq->lock); > - old_state = bq->state; > - mutex_unlock(&bq->lock); > + ret = bq25890_get_chip_state(bq, &new_state); > + if (ret < 0) > + return IRQ_NONE; > > - if (!new_state->online) { /* power removed */ > + if (!memcmp(&bq->state, &new_state, sizeof(new_state))) > + return IRQ_NONE; > + > + if (!new_state.online && bq->state.online) { /* power removed */ > /* disable ADC */ > ret = bq25890_field_write(bq, F_CONV_START, 0); > if (ret < 0) > goto error; > - } else if (!old_state.online) { /* power inserted */ > + } else if (new_state.online && !bq->state.online) { /* power inserted */ > /* enable ADC, to have control of charge current/voltage */ > ret = bq25890_field_write(bq, F_CONV_START, 1); > if (ret < 0) > goto error; > } > > - return; > + bq->state = new_state; > + power_supply_changed(bq->charger); > > + return IRQ_HANDLED; > error: > - dev_err(bq->dev, "Error communicating with the chip.\n"); > + dev_err(bq->dev, "Error communicating with the chip: %pe\n", > + ERR_PTR(ret)); > + return IRQ_HANDLED; > } > > static irqreturn_t bq25890_irq_handler_thread(int irq, void *private) > { > struct bq25890_device *bq = private; > - int ret; > - struct bq25890_state state; > - > - ret = bq25890_get_chip_state(bq, &state); > - if (ret < 0) > - goto handled; > - > - if (!bq25890_state_changed(bq, &state)) > - goto handled; > - > - bq25890_handle_state_change(bq, &state); > + irqreturn_t ret; > > mutex_lock(&bq->lock); > - bq->state = state; > + ret = __bq25890_handle_irq(bq); > mutex_unlock(&bq->lock); > > - power_supply_changed(bq->charger); > - > -handled: > - return IRQ_HANDLED; > + return ret; > } > > static int bq25890_chip_reset(struct bq25890_device *bq) > @@ -607,7 +583,6 @@ static int bq25890_hw_init(struct bq25890_device *bq) > { > int ret; > int i; > - struct bq25890_state state; > > const struct { > enum bq25890_fields id; > @@ -655,16 +630,12 @@ static int bq25890_hw_init(struct bq25890_device *bq) > return ret; > } > > - ret = bq25890_get_chip_state(bq, &state); > + ret = bq25890_get_chip_state(bq, &bq->state); > if (ret < 0) { > dev_dbg(bq->dev, "Get state failed %d\n", ret); > return ret; > } > > - mutex_lock(&bq->lock); > - bq->state = state; > - mutex_unlock(&bq->lock); > - > return 0; > } > > @@ -1001,19 +972,16 @@ static int bq25890_suspend(struct device *dev) > static int bq25890_resume(struct device *dev) > { > int ret; > - struct bq25890_state state; > struct bq25890_device *bq = dev_get_drvdata(dev); > > - ret = bq25890_get_chip_state(bq, &state); > + mutex_lock(&bq->lock); > + > + ret = bq25890_get_chip_state(bq, &bq->state); > if (ret < 0) > return ret; > > - mutex_lock(&bq->lock); > - bq->state = state; > - mutex_unlock(&bq->lock); > - > /* Re-enable ADC only if charger is plugged in. */ > - if (state.online) { > + if (bq->state.online) { > ret = bq25890_field_write(bq, F_CONV_START, 1); > if (ret < 0) > return ret; > @@ -1022,6 +990,8 @@ static int bq25890_resume(struct device *dev) > /* signal userspace, maybe state changed while suspended */ > power_supply_changed(bq->charger); > > + mutex_unlock(&bq->lock); > + > return 0; > } > #endif > -- > 2.20.1 >
diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c index c4a69fd69f34..9339e216651f 100644 --- a/drivers/power/supply/bq25890_charger.c +++ b/drivers/power/supply/bq25890_charger.c @@ -510,74 +510,50 @@ static int bq25890_get_chip_state(struct bq25890_device *bq, return 0; } -static bool bq25890_state_changed(struct bq25890_device *bq, - struct bq25890_state *new_state) -{ - struct bq25890_state old_state; - - mutex_lock(&bq->lock); - old_state = bq->state; - mutex_unlock(&bq->lock); - - return (old_state.chrg_status != new_state->chrg_status || - old_state.chrg_fault != new_state->chrg_fault || - old_state.online != new_state->online || - old_state.bat_fault != new_state->bat_fault || - old_state.boost_fault != new_state->boost_fault || - old_state.vsys_status != new_state->vsys_status); -} - -static void bq25890_handle_state_change(struct bq25890_device *bq, - struct bq25890_state *new_state) +static irqreturn_t __bq25890_handle_irq(struct bq25890_device *bq) { + struct bq25890_state new_state; int ret; - struct bq25890_state old_state; - mutex_lock(&bq->lock); - old_state = bq->state; - mutex_unlock(&bq->lock); + ret = bq25890_get_chip_state(bq, &new_state); + if (ret < 0) + return IRQ_NONE; - if (!new_state->online) { /* power removed */ + if (!memcmp(&bq->state, &new_state, sizeof(new_state))) + return IRQ_NONE; + + if (!new_state.online && bq->state.online) { /* power removed */ /* disable ADC */ ret = bq25890_field_write(bq, F_CONV_START, 0); if (ret < 0) goto error; - } else if (!old_state.online) { /* power inserted */ + } else if (new_state.online && !bq->state.online) { /* power inserted */ /* enable ADC, to have control of charge current/voltage */ ret = bq25890_field_write(bq, F_CONV_START, 1); if (ret < 0) goto error; } - return; + bq->state = new_state; + power_supply_changed(bq->charger); + return IRQ_HANDLED; error: - dev_err(bq->dev, "Error communicating with the chip.\n"); + dev_err(bq->dev, "Error communicating with the chip: %pe\n", + ERR_PTR(ret)); + return IRQ_HANDLED; } static irqreturn_t bq25890_irq_handler_thread(int irq, void *private) { struct bq25890_device *bq = private; - int ret; - struct bq25890_state state; - - ret = bq25890_get_chip_state(bq, &state); - if (ret < 0) - goto handled; - - if (!bq25890_state_changed(bq, &state)) - goto handled; - - bq25890_handle_state_change(bq, &state); + irqreturn_t ret; mutex_lock(&bq->lock); - bq->state = state; + ret = __bq25890_handle_irq(bq); mutex_unlock(&bq->lock); - power_supply_changed(bq->charger); - -handled: - return IRQ_HANDLED; + return ret; } static int bq25890_chip_reset(struct bq25890_device *bq) @@ -607,7 +583,6 @@ static int bq25890_hw_init(struct bq25890_device *bq) { int ret; int i; - struct bq25890_state state; const struct { enum bq25890_fields id; @@ -655,16 +630,12 @@ static int bq25890_hw_init(struct bq25890_device *bq) return ret; } - ret = bq25890_get_chip_state(bq, &state); + ret = bq25890_get_chip_state(bq, &bq->state); if (ret < 0) { dev_dbg(bq->dev, "Get state failed %d\n", ret); return ret; } - mutex_lock(&bq->lock); - bq->state = state; - mutex_unlock(&bq->lock); - return 0; } @@ -1001,19 +972,16 @@ static int bq25890_suspend(struct device *dev) static int bq25890_resume(struct device *dev) { int ret; - struct bq25890_state state; struct bq25890_device *bq = dev_get_drvdata(dev); - ret = bq25890_get_chip_state(bq, &state); + mutex_lock(&bq->lock); + + ret = bq25890_get_chip_state(bq, &bq->state); if (ret < 0) return ret; - mutex_lock(&bq->lock); - bq->state = state; - mutex_unlock(&bq->lock); - /* Re-enable ADC only if charger is plugged in. */ - if (state.online) { + if (bq->state.online) { ret = bq25890_field_write(bq, F_CONV_START, 1); if (ret < 0) return ret; @@ -1022,6 +990,8 @@ static int bq25890_resume(struct device *dev) /* signal userspace, maybe state changed while suspended */ power_supply_changed(bq->charger); + mutex_unlock(&bq->lock); + return 0; } #endif
Extend bq->lock over whole updating of the chip's state. Might get useful later for switching ADC modes correctly. Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> --- drivers/power/supply/bq25890_charger.c | 82 ++++++++------------------ 1 file changed, 26 insertions(+), 56 deletions(-)