Hi Guenter, On Thu, 8 Dec 2016 07:34:35 -0800, Guenter Roeck wrote: > On 12/08/2016 06:33 AM, Jean Delvare wrote: > > On Sun, 4 Dec 2016 20:55:29 -0800, Guenter Roeck wrote: > >> @@ -215,11 +216,11 @@ static int adm1026_scaling[] = { /* .001 Volts */ > >> #define DIV_TO_REG(val) ((val) >= 8 ? 3 : (val) >= 4 ? 2 : (val) >= 2 ? 1 : 0) > >> > >> /* Temperature is reported in 1 degC increments */ > >> -#define TEMP_TO_REG(val) (clamp_val(((val) + ((val) < 0 ? -500 : 500)) \ > >> - / 1000, -127, 127)) > >> +#define TEMP_TO_REG(val) DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), \ > >> + 1000) > >> #define TEMP_FROM_REG(val) ((val) * 1000) > >> -#define OFFSET_TO_REG(val) (clamp_val(((val) + ((val) < 0 ? -500 : 500)) \ > >> - / 1000, -127, 127)) > >> +#define OFFSET_TO_REG(val) DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), \ > >> + 1000) > > > > Sorry for nitpicking but the original code had -127 °C as the negative > > limit. You are changing it to -128 °C without a justification. If it > > matters, it should be at least documented in the commit message. If > > not, it should be left as it was. > > I had seen -128 as value reported by the chip with one of my register dumps, > which messes up my module tests because it tries to rewrite the value it read > into the attribute, and that fails. Also, the datasheet lists -128 degrees C > as a valid register value. OK, I understand. > This is one of those philosophical questions, for which I don't have a really > good answer. Should we clamp to the register limits or to the chip specification ? > I tend to clamp to register limits, because I think that it should always be possible > to write back what was read, but we are highly inconsistent in the various drivers. > Any opinion ? I have noticed that inconsistency too. Given that we do not have attributes to expose the limits to user-space, I consider it a nice feature to clamp the requested limits to what the chip is actually specified for. But I don't have a strong opinion on it either, and am not going to spend time "fixing" all drivers not doing it. And you have a point with being able to write back what was read, especially if the power-on value isn't withing the specified limits. > >> (...) > >> @@ -593,7 +595,10 @@ static ssize_t set_in16_min(struct device *dev, struct device_attribute *attr, > >> return err; > >> > >> mutex_lock(&data->update_lock); > >> - data->in_min[16] = INS_TO_REG(16, val + NEG12_OFFSET); > >> + data->in_min[16] = INS_TO_REG(16, > >> + clamp_val(val, INT_MIN, > >> + INT_MAX - NEG12_OFFSET) + > >> + NEG12_OFFSET); > >> adm1026_write_value(client, ADM1026_REG_IN_MIN[16], data->in_min[16]); > >> mutex_unlock(&data->update_lock); > >> return count; > >> @@ -618,7 +623,10 @@ static ssize_t set_in16_max(struct device *dev, struct device_attribute *attr, > >> return err; > >> > >> mutex_lock(&data->update_lock); > >> - data->in_max[16] = INS_TO_REG(16, val+NEG12_OFFSET); > >> + data->in_max[16] = INS_TO_REG(16, > >> + clamp_val(val, INT_MIN, > >> + INT_MAX - NEG12_OFFSET) + > >> + NEG12_OFFSET); > >> adm1026_write_value(client, ADM1026_REG_IN_MAX[16], data->in_max[16]); > >> mutex_unlock(&data->update_lock); > >> return count; > > > > On these code paths, you end up calling clamp_val() twice. This could > > certainly be avoided, but I'm too lazy to do the math ;-) > > > I know. Problem here is that there are two overflows, one in the calling code > (since it does arithmetic on the input value), and another one in INS_TO_REG(). > When I wrote this, I didn't have a good idea how to avoid the first overflow > without a second clamp. > > One possibility would be to define INS_TO_REG_NOCLAMP(). Would that be worth it ? No, don't bother. God only knows if there's any user left for this driver ;-) -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html