[v2,04/11] power: bq25890: protect view of the chip's state
diff mbox series

Message ID 3291ca81bf8eb1b0401579ae08e7835e71dfc1ff.1588517058.git.mirq-linux@rere.qmqm.pl
State Superseded
Headers show
Series
  • power: supply: bq25890: fix and extend
Related show

Commit Message

Michał Mirosław May 3, 2020, 3:21 p.m. UTC
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(-)

Comments

Sebastian Reichel May 3, 2020, 9:10 p.m. UTC | #1
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
>

Patch
diff mbox series

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