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,
> + ®val);
> + 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
