> -----Original Message----- > From: Hans de Goede [mailto:hdegoede@xxxxxxxxxx] > Sent: Tuesday, February 23, 2010 1:33 AM > To: George Joseph > Cc: lm-sensors@xxxxxxxxxxxxxx > Subject: Re: reformat: [PATCH] hwmon: Driver for Andigilog aSC7621 family monitoring > chips > > Hi, > > On 02/23/2010 03:26 AM, George Joseph wrote: > >> -----Original Message----- > >> From: Hans de Goede [mailto:hdegoede@xxxxxxxxxx] > >> Sent: Monday, February 22, 2010 6:55 AM > >> To: George Joseph > >> Cc: lm-sensors@xxxxxxxxxxxxxx > >> Subject: Re: reformat: [PATCH] hwmon: Driver for Andigilog aSC7621 family monitoring > >> chips > >> > >> Hi, > >> > >> Here is a full review of the driver, see comments inline. > >> > >> Allthough the structure of the driver is still a bit weird, > >> I must admit it works well. I don't know what it brushed me > >> the wrong way last time, sorry about that. > >> > >> If you can do a new revision addressing the few minor comments > >> I have, then I can ack this soon, and Jean will hopefully take it > >> for 2.6.34 . > >> > >> Regards, > >> > >> Hans > >> > > > > Thanks Hans. I've made most of the changes but I've got 2 comments below. > > > > <snip> > > >>> +static u32 asc7621_range_map[] = { > >>> + 2000, 2500, 3330, 4000, 5000, 6670, 8000, 10000, > >>> + 13330, 16000, 20000, 26670, 32000, 40000, 53330, 80000, > >>> +}; > >>> + > >>> +static ssize_t show_ap2_temp(struct device *dev, > >>> + struct device_attribute *attr, char *buf) > >>> +{ > >>> + SETUP_SHOW_data_param(dev, attr); > >>> + long auto_point1; > >>> + u8 regval; > >>> + int temp; > >>> + > >>> + mutex_lock(&data->update_lock); > >>> + auto_point1 = ((s8) data->reg[param->msb[1]]) * 1000; > >>> + regval = > >>> + ((data->reg[param->msb[0]]>> param->shift[0])& param->mask[0]); > >>> + temp = auto_point1 + asc7621_range_map[SENSORS_LIMIT(regval, 0, 15)]; > >> > >> The SENSORS_LIMIT here is not needed, as the mask already does the limiting. > > > > The reason I use the SENSORS_LIMIT here (and the other places you commented) > > is because a typo in the shift or mask specified in the PREAD/PWRITE macros > > could cause regval to get set to an index outside the lookup array. I'll > > remove the SENSORS_LIMITs if you want but I'd prefer to keep them. > > > > If you want to keep them that is fine, I found this usage of SENSOR_LIMIT > a bit strange, and unneeded. But if you prefer to keep them, ok. I just feel safer. Thanks. > > >> > >>> + mutex_unlock(&data->update_lock); > >>> + > >>> + return sprintf(buf, "%d\n", temp); > >>> + > >>> +} > >>> + > >>> +static ssize_t store_ap2_temp(struct device *dev, > >>> + struct device_attribute *attr, > >>> + const char *buf, size_t count) > >>> +{ > >>> + SETUP_STORE_data_param(dev, attr); > >>> + long reqval; > >>> + long auto_point1; > >>> + int i = 0; > >>> + u8 currval = 0; > >>> + u8 newval = 255; > >>> + > >>> + if (strict_strtol(buf, 10,&reqval)) > >>> + return -EINVAL; > >>> + > >>> + mutex_lock(&data->update_lock); > >>> + auto_point1 = data->reg[param->msb[1]] * 1000; > >>> + for (i = ARRAY_SIZE(asc7621_range_map) - 1; i>= 0; i--) { > >>> + if (reqval>= auto_point1 + asc7621_range_map[i]) { > >>> + newval = i; > >>> + break; > >>> + } > >>> + } > >>> + if (newval == 255) { > >>> + mutex_unlock(&data->update_lock); > >>> + return -EINVAL; > >>> + } > >>> + > >> > >> For autopoint1 you use SENSORS_LIMIT, so it seems more consistent > >> to me to do the same here, so instead of returning EINVAL simply > >> use newval = 0 > > > > I'm not sure I follow. > > > > The store function used for autopoint1, will simply clamp the user > given input between -128000 and +127000 if it is out of that range. Here when > reqval is larger then autopoint1 + 80000, you do the same, but when it is > smaller then autopoint1 + 2000 you return -EINVAL. That seems inconsistent > to me. So I would like to see this store function clamp autopoint2 to > autopoint1 + 2000 (so set newval to 0) when the input-value < autopoint1 + 2000. > > NB > You could add a check for input-value < autopoint1 and return -EINVAL there, > that makes sense, but just clamping is fine too. Ah Ha! Now I get it. I'll fix. > > Regards, > > Hans _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
