Re: [PATCH 16/29] hwmon: (adt7411) Rely on subsystem locking

["Nuno Sá" <noname.nuno@gmail.com>]

On Fri, 2025-10-17 at 06:02 -0700, Guenter Roeck wrote:
> Attribute access is now serialized in the hardware monitoring core,
> so locking in the driver code is no longer necessary. Drop it.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

>  drivers/hwmon/adt7411.c | 59 +++++++++++------------------------------
>  1 file changed, 16 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7411.c b/drivers/hwmon/adt7411.c
> index 08d0effd97f7..b9991a69e6c6 100644
> --- a/drivers/hwmon/adt7411.c
> +++ b/drivers/hwmon/adt7411.c
> @@ -11,7 +11,6 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/err.h>
> -#include <linux/mutex.h>
>  #include <linux/jiffies.h>
>  #include <linux/i2c.h>
>  #include <linux/hwmon.h>
> @@ -99,8 +98,6 @@ static const u8 adt7411_in_alarm_bits[] = {
>  };
>  
>  struct adt7411_data {
> -	struct mutex device_lock;	/* for "atomic" device accesses */
> -	struct mutex update_lock;
>  	unsigned long next_update;
>  	long vref_cached;
>  	struct i2c_client *client;
> @@ -110,55 +107,41 @@ struct adt7411_data {
>  /*
>   * When reading a register containing (up to 4) lsb, all associated
>   * msb-registers get locked by the hardware. After _one_ of those msb is
> read,
> - * _all_ are unlocked. In order to use this locking correctly, reading
> lsb/msb
> - * is protected here with a mutex, too.
> + * _all_ are unlocked.
>   */
>  static int adt7411_read_10_bit(struct i2c_client *client, u8 lsb_reg,
> -				u8 msb_reg, u8 lsb_shift)
> +			       u8 msb_reg, u8 lsb_shift)
>  {
> -	struct adt7411_data *data = i2c_get_clientdata(client);
>  	int val, tmp;
>  
> -	mutex_lock(&data->device_lock);
> -
>  	val = i2c_smbus_read_byte_data(client, lsb_reg);
>  	if (val < 0)
> -		goto exit_unlock;
> +		return val;
>  
>  	tmp = (val >> lsb_shift) & 3;
>  	val = i2c_smbus_read_byte_data(client, msb_reg);
> +	if (val < 0)
> +		return val;
>  
> -	if (val >= 0)
> -		val = (val << 2) | tmp;
> -
> - exit_unlock:
> -	mutex_unlock(&data->device_lock);
> -
> +	val = (val << 2) | tmp;
>  	return val;
>  }
>  
>  static int adt7411_modify_bit(struct i2c_client *client, u8 reg, u8 bit,
> -				bool flag)
> +			      bool flag)
>  {
> -	struct adt7411_data *data = i2c_get_clientdata(client);
>  	int ret, val;
>  
> -	mutex_lock(&data->device_lock);
> -
>  	ret = i2c_smbus_read_byte_data(client, reg);
>  	if (ret < 0)
> -		goto exit_unlock;
> +		return ret;
>  
>  	if (flag)
>  		val = ret | bit;
>  	else
>  		val = ret & ~bit;
>  
> -	ret = i2c_smbus_write_byte_data(client, reg, val);
> -
> - exit_unlock:
> -	mutex_unlock(&data->device_lock);
> -	return ret;
> +	return i2c_smbus_write_byte_data(client, reg, val);
>  }
>  
>  static ssize_t adt7411_show_bit(struct device *dev,
> @@ -186,12 +169,11 @@ static ssize_t adt7411_set_bit(struct device *dev,
>  	if (ret || flag > 1)
>  		return -EINVAL;
>  
> +	hwmon_lock(dev);
>  	ret = adt7411_modify_bit(client, s_attr2->index, s_attr2->nr, flag);
> -
>  	/* force update */
> -	mutex_lock(&data->update_lock);
>  	data->next_update = jiffies;
> -	mutex_unlock(&data->update_lock);
> +	hwmon_unlock(dev);
>  
>  	return ret < 0 ? ret : count;
>  }
> @@ -294,10 +276,9 @@ static int adt7411_read_in_chan(struct device *dev, u32
> attr, int channel,
>  	int reg, lsb_reg, lsb_shift;
>  	int nr = channel - 1;
>  
> -	mutex_lock(&data->update_lock);
>  	ret = adt7411_update_vref(dev);
>  	if (ret < 0)
> -		goto exit_unlock;
> +		return ret;
>  
>  	switch (attr) {
>  	case hwmon_in_input:
> @@ -307,7 +288,7 @@ static int adt7411_read_in_chan(struct device *dev, u32
> attr, int channel,
>  					  ADT7411_REG_EXT_TEMP_AIN1_MSB + nr,
>  					  lsb_shift);
>  		if (ret < 0)
> -			goto exit_unlock;
> +			return ret;
>  		*val = ret * data->vref_cached / 1024;
>  		ret = 0;
>  		break;
> @@ -318,7 +299,7 @@ static int adt7411_read_in_chan(struct device *dev, u32
> attr, int channel,
>  			: ADT7411_REG_IN_HIGH(channel);
>  		ret = i2c_smbus_read_byte_data(client, reg);
>  		if (ret < 0)
> -			goto exit_unlock;
> +			return ret;
>  		*val = ret * data->vref_cached / 256;
>  		ret = 0;
>  		break;
> @@ -329,8 +310,6 @@ static int adt7411_read_in_chan(struct device *dev, u32
> attr, int channel,
>  		ret = -EOPNOTSUPP;
>  		break;
>  	}
> - exit_unlock:
> -	mutex_unlock(&data->update_lock);
>  	return ret;
>  }
>  
> @@ -457,10 +436,9 @@ static int adt7411_write_in_chan(struct device *dev, u32
> attr, int channel,
>  	struct i2c_client *client = data->client;
>  	int ret, reg;
>  
> -	mutex_lock(&data->update_lock);
>  	ret = adt7411_update_vref(dev);
>  	if (ret < 0)
> -		goto exit_unlock;
> +		return ret;
>  	val = clamp_val(val, 0, 255 * data->vref_cached / 256);
>  	val = DIV_ROUND_CLOSEST(val * 256, data->vref_cached);
>  
> @@ -472,13 +450,10 @@ static int adt7411_write_in_chan(struct device *dev, u32
> attr, int channel,
>  		reg = ADT7411_REG_IN_HIGH(channel);
>  		break;
>  	default:
> -		ret = -EOPNOTSUPP;
> -		goto exit_unlock;
> +		return -EOPNOTSUPP;
>  	}
>  
>  	ret = i2c_smbus_write_byte_data(client, reg, val);
> - exit_unlock:
> -	mutex_unlock(&data->update_lock);
>  	return ret;
>  }
>  
> @@ -679,8 +654,6 @@ static int adt7411_probe(struct i2c_client *client)
>  
>  	i2c_set_clientdata(client, data);
>  	data->client = client;
> -	mutex_init(&data->device_lock);
> -	mutex_init(&data->update_lock);
>  
>  	ret = adt7411_init_device(data);
>  	if (ret < 0)