On 14/09/08 19:50 +0200, Hans de Goede wrote: > Jean Delvare wrote: > > Hi Hans, Jordan, > > > > Le samedi 13 septembre 2008, Hans de Goede a ?crit : > >> Jordan Crouse wrote: > >>> Okay - I have generated a new patch. I implemented all of your suggestions - > >>> the only one I had any concerns about was the hystersis (hystersis makes > >>> more sense to me as an offset rather then an absolute), but consistancy > >>> among hwmon drivers is rather more important. > >>> > >>> There is one possible issue in the patch - I pulled the decimal point > >>> from the pwmX_freq numbers since I wasn't sure if we wanted to express > >>> the number in milihertz. If we do, then it is an easy fix. > >>> > >>> Compile tested and run on an ADT7475 platform. > >>> > >> Hi Jordan et all, > >> > >> I've given this a quick review (not as thorough as I would have liked to do but > >> I simply don't have enough time for a really thorough review) and I've found no > >> issues. So this patch is now: > >> > >> Reviewed-by: Hans de Goede <hdegoede at redhat.com> > > > > Am I the only one seeing these warnings at build time? > > > > No as said I didn't have all that much time, so I just reviewed it I didn't > actually try to build it (I assumed Jordan alreayd build it). > > > CC [M] drivers/hwmon/adt7475.o > > drivers/hwmon/adt7475.c: In function ?adt7475_update_device?: > > drivers/hwmon/adt7475.c:1043: warning: array subscript is above array bounds > > > > <snip> > > > Apparently struct adt7475_data should be changed from > > s16 temp[6][3]; > > to > > s16 temp[7][3]; > > > > Looking closer at the code: you are completely right > > Also while looking closer: > > This snippet in adt7475_update_device() : > > data->temp[HYSTERSIS][0] = (u16) adt7475_read(REG_REMOTE1_HYSTERSIS); > data->temp[HYSTERSIS][1] = (u16) adt7475_read(REG_REMOTE1_HYSTERSIS); > data->temp[HYSTERSIS][2] = (u16) adt7475_read(REG_REMOTE2_HYSTERSIS); > > Should be rewritten to: > data->temp[HYSTERSIS][0] = (u16) adt7475_read(REG_REMOTE1_HYSTERSIS); > data->temp[HYSTERSIS][1] = data->temp[HYSTERSIS][0]; > data->temp[HYSTERSIS][2] = (u16) adt7475_read(REG_REMOTE2_HYSTERSIS); > > To avoid 1 (slow) unnecessary i2c transaction. New patch attached that addreses these concerns, thanks. Jordan -- Jordan Crouse Systems Software Development Engineer Advanced Micro Devices, Inc. -------------- next part -------------- A non-text attachment was scrubbed... Name: adt7475.patch Type: text/x-diff Size: 40320 bytes Desc: not available Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20080926/27b8b32c/attachment.bin
