Re: [PATCH v3 3/8] iio: tcs3472: convert remaining locking to guard(mutex)

[Jonathan Cameron]

>  
>  static irqreturn_t tcs3472_event_handler(int irq, void *priv)
> @@ -445,16 +433,16 @@ static int tcs3472_powerdown(struct tcs3472_data *data)
>  	int ret;
>  	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
>  
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>  
>  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>  					data->enable & ~enable_mask);
> -	if (!ret)
> -		data->enable &= ~enable_mask;
> +	if (ret)
> +		return ret;
>  
> -	mutex_unlock(&data->lock);
> +	data->enable &= ~enable_mask;

Hi Aldo,

I'm going to comment on a similar (but worse) case in review of patch 8.
If you do rework that to avoid falsely setting cached state that we can't
rely on being right, you could switch to using a local variable here.

	u8 enable = data->enable & ~(TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON);

	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, enable);
  	if (ret)
		return ret;

	data->enable = enable;

	return 0;

(similar for other cases).  Given it's unrelated to what you are doing here
that doesn't stop me picking up this patch as is.

I'm working through the series, but so far 1-3 applied to the testing branch
of iio.git.

Thanks,

Jonathan


> -	return ret;
> +	return 0;
>  }
>  
>  static int tcs3472_probe(struct i2c_client *client)
> @@ -583,16 +571,16 @@ static int tcs3472_resume(struct device *dev)
>  	int ret;
>  	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
>  
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>  
>  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>  		data->enable | enable_mask);
> -	if (!ret)
> -		data->enable |= enable_mask;
> +	if (ret)
> +		return ret;
>  
> -	mutex_unlock(&data->lock);
> +	data->enable |= enable_mask;
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static DEFINE_SIMPLE_DEV_PM_OPS(tcs3472_pm_ops, tcs3472_suspend,