Re: [PATCH v2 04/11] power: bq25890: protect view of the chip's state

[Sebastian Reichel]

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@xxxxxxxxxxxx>
> ---

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
>

Attachment: signature.asc
Description: PGP signature