Re: [PATCH v3 1/2] hwmon: (jc42) Convert register access and caching to regmap/regcache

[Guenter Roeck]

On Fri, Oct 21, 2022 at 06:49:59PM +0200, Martin Blumenstingl wrote:
> Switch the jc42 driver to use an I2C regmap to access the registers.
> Also move over to regmap's built-in caching instead of adding a
> custom caching implementation. This works for JC42_REG_TEMP_UPPER,
> JC42_REG_TEMP_LOWER and JC42_REG_TEMP_CRITICAL as these values never
> change except when explicitly written. The cache For JC42_REG_TEMP is
> dropped (regmap can't cache it because it's volatile, meaning it can
> change at any time) as well for simplicity and consistency with other
> drivers.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
> ---

...

>  	case hwmon_temp_crit_hyst:
> +		ret = regmap_read(data->regmap, JC42_REG_TEMP_CRITICAL,
> +				  &regval);
> +		if (ret)
> +			return ret;
> +
>  		/*
>  		 * JC42.4 compliant chips only support four hysteresis values.
>  		 * Pick best choice and go from there.
> @@ -356,7 +349,7 @@ static int jc42_write(struct device *dev, enum hwmon_sensor_types type,
>  		val = clamp_val(val, (data->extended ? JC42_TEMP_MIN_EXTENDED
>  						     : JC42_TEMP_MIN) - 6000,
>  				JC42_TEMP_MAX);
> -		diff = jc42_temp_from_reg(data->temp[t_crit]) - val;
> +		diff = jc42_temp_from_reg(regval) - val;
>  		hyst = 0;
>  		if (diff > 0) {
>  			if (diff < 2250)
> @@ -368,17 +361,14 @@ static int jc42_write(struct device *dev, enum hwmon_sensor_types type,
>  		}
>  		data->config = (data->config & ~JC42_CFG_HYST_MASK) |
>  				(hyst << JC42_CFG_HYST_SHIFT);
> -		ret = i2c_smbus_write_word_swapped(data->client,
> -						   JC42_REG_CONFIG,
> -						   data->config);
> +		ret = regmap_write(data->regmap, JC42_REG_CONFIG,
> +				   data->config);
>  		break;

This code sequence still requires a mutex since another thread could modify
the upper limit (and/or the hysteresis) while the hysteresis is in the process
of being written. Worst case there could be a mismatch between the value in
data->config and the value actually written into the chip. Granted, that is
unlikely to happen, but the race still exists.

Thanks,
Guenter