Hi Juerg, Thanks for the review. On Mon, 28 Apr 2008 22:03:40 -0700, Juerg Haefliger wrote: > > /* These are the zone temperature range encodings in .001 degree C */ > > -static int lm85_range_map[] = { > > - 2000, 2500, 3300, 4000, 5000, 6600, > > - 8000, 10000, 13300, 16000, 20000, 26600, > > - 32000, 40000, 53300, 80000 > > - }; > > -static int RANGE_TO_REG( int range ) > > +static int lm85_range_map[] = { > > + 2000, 2500, 3300, 4000, 5000, 6600, > > + 8000, 10000, 13300, 16000, 20000, 26600, > > + 32000, 40000, 53300, 80000 > > Inconsistent alignment. Either use a single space between values on the > 1st line or align to the 3rd line, i.e, move the first two lines to the > right by one space. Good catch, I'll go for the first option, and while I'm here, the values fit on 2 lines so I'll do that. > > (...) > > static ssize_t show_in_min(struct device *dev, struct device_attribute *attr, > > Double spaces before struct. Fixed. > > (...) > > @@ -1171,82 +1172,81 @@ static int lm85_detect(struct i2c_adapte > > /* If auto-detecting, Determine the chip type. */ > > if (kind <= 0) { > > dev_dbg(&adapter->dev, "Autodetecting device at %d,0x%02x ...\n", > > - i2c_adapter_id(adapter), address ); > > - if( company == LM85_COMPANY_NATIONAL > > - && verstep == LM85_VERSTEP_LM85C ) { > > - kind = lm85c ; > > - } else if( company == LM85_COMPANY_NATIONAL > > - && verstep == LM85_VERSTEP_LM85B ) { > > - kind = lm85b ; > > - } else if( company == LM85_COMPANY_NATIONAL > > - && (verstep & LM85_VERSTEP_VMASK) == LM85_VERSTEP_GENERIC ) { > > + i2c_adapter_id(adapter), address); > > + if (company == LM85_COMPANY_NATIONAL > > + && verstep == LM85_VERSTEP_LM85C) { > > + kind = lm85c; > > + } else if (company == LM85_COMPANY_NATIONAL > > + && verstep == LM85_VERSTEP_LM85B) { > > + kind = lm85b; > > + } else if (company == LM85_COMPANY_NATIONAL > > + && (verstep & LM85_VERSTEP_VMASK) == LM85_VERSTEP_GENERIC) { > > dev_err(&adapter->dev, "Unrecognized version/stepping 0x%02x" > > " Defaulting to LM85.\n", verstep); > > - kind = any_chip ; > > - } else if( company == LM85_COMPANY_ANALOG_DEV > > - && verstep == LM85_VERSTEP_ADM1027 ) { > > - kind = adm1027 ; > > - } else if( company == LM85_COMPANY_ANALOG_DEV > > + kind = any_chip; > > + } else if (company == LM85_COMPANY_ANALOG_DEV > > + && verstep == LM85_VERSTEP_ADM1027) { > > + kind = adm1027; > > + } else if (company == LM85_COMPANY_ANALOG_DEV > > && (verstep == LM85_VERSTEP_ADT7463 > > - || verstep == LM85_VERSTEP_ADT7463C) ) { > > - kind = adt7463 ; > > - } else if( company == LM85_COMPANY_ANALOG_DEV > > - && (verstep & LM85_VERSTEP_VMASK) == LM85_VERSTEP_GENERIC ) { > > + || verstep == LM85_VERSTEP_ADT7463C)) { > > + kind = adt7463; > > + } else if (company == LM85_COMPANY_ANALOG_DEV > > + && (verstep & LM85_VERSTEP_VMASK) == LM85_VERSTEP_GENERIC) { > > dev_err(&adapter->dev, "Unrecognized version/stepping 0x%02x" > > - " Defaulting to Generic LM85.\n", verstep ); > > - kind = any_chip ; > > - } else if( company == LM85_COMPANY_SMSC > > + " Defaulting to Generic LM85.\n", verstep); > > Leading space should go to the end of the previous line. This is also > true for other strings throughout the code. > > > > + kind = any_chip; > > + } else if (company == LM85_COMPANY_SMSC > > && (verstep == LM85_VERSTEP_EMC6D100_A0 > > - || verstep == LM85_VERSTEP_EMC6D100_A1) ) { > > + || verstep == LM85_VERSTEP_EMC6D100_A1)) { > > /* Unfortunately, we can't tell a '100 from a '101 > > * from the registers. Since a '101 is a '100 > > * in a package with fewer pins and therefore no > > * 3.3V, 1.5V or 1.8V inputs, perhaps if those > > * inputs read 0, then it's a '101. > > */ > > - kind = emc6d100 ; > > - } else if( company == LM85_COMPANY_SMSC > > + kind = emc6d100; > > + } else if (company == LM85_COMPANY_SMSC > > && verstep == LM85_VERSTEP_EMC6D102) { > > - kind = emc6d102 ; > > - } else if( company == LM85_COMPANY_SMSC > > + kind = emc6d102; > > + } else if (company == LM85_COMPANY_SMSC > > && (verstep & LM85_VERSTEP_VMASK) == LM85_VERSTEP_GENERIC) { > > dev_err(&adapter->dev, "lm85: Detected SMSC chip\n"); > > dev_err(&adapter->dev, "lm85: Unrecognized version/stepping 0x%02x" > > - " Defaulting to Generic LM85.\n", verstep ); > > - kind = any_chip ; > > - } else if( kind == any_chip > > + " Defaulting to Generic LM85.\n", verstep); > > Same here. Patch 7/10 (Rework the device detection) replaces this whole section of code by something totally different, that's why I didn't bother cleaning up the split strings. > > (...) > > - for(i = 0; i <= 4; i++) > > - data->in_ext[i] = ((val>>(i * 2))&0x03) << 2; > > + for (i = 0; i <= 4; i++) > > + data->in_ext[i] = ((val>>(i * 2)) & 0x03) << 2; > > Missing spaces around >>. > > > > > > - for(i = 0; i <= 2; i++) > > - data->temp_ext[i] = (val>>((i + 4) * 2))&0x0c; > > + for (i = 0; i <= 2; i++) > > + data->temp_ext[i] = (val>>((i + 4) * 2)) & 0x0c; > > Same here. Fixed. > > > > } > > > > data->vid = lm85_read_value(client, LM85_REG_VID); > > @@ -1480,21 +1482,21 @@ static struct lm85_data *lm85_update_dev > > > > data->alarms = lm85_read_value(client, LM85_REG_ALARM1); > > > > - if ( data->type == adt7463 ) { > > - if( data->therm_total < ULONG_MAX - 256 ) { > > + if (data->type == adt7463) { > > + if (data->therm_total < ULONG_MAX - 256) { > > data->therm_total += > > - lm85_read_value(client, ADT7463_REG_THERM ); > > + lm85_read_value(client, ADT7463_REG_THERM); > > } > > - } else if ( data->type == emc6d100 ) { > > + } else if (data->type == emc6d100) { > > /* Three more voltage sensors */ > > for (i = 5; i <= 7; ++i) { > > - data->in[i] = > > - lm85_read_value(client, EMC6D100_REG_IN(i)); > > + data->in[i] = lm85_read_value(client, > > + EMC6D100_REG_IN(i)); > > } > > /* More alarm bits */ > > - data->alarms |= > > - lm85_read_value(client, EMC6D100_REG_ALARM3) << 16; > > - } else if (data->type == emc6d102 ) { > > + data->alarms |= lm85_read_value(client, > > + EMC6D100_REG_ALARM3) << 16; > > + } else if (data->type == emc6d102) { > > /* Have to read LSB bits after the MSB ones because > > the reading of the MSB bits has frozen the > > LSBs (backward from the ADM1027). > > @@ -1518,11 +1520,11 @@ static struct lm85_data *lm85_update_dev > > data->temp_ext[2] = (ext1 >> 4) & 0x0f; > > } > > > > - data->last_reading = jiffies ; > > - }; /* last_reading */ > > + data->last_reading = jiffies; > > + } /* last_reading */ > > > > - if ( !data->valid || > > - time_after(jiffies, data->last_config + LM85_CONFIG_INTERVAL) ) { > > + if (!data->valid || > > + time_after(jiffies, data->last_config + LM85_CONFIG_INTERVAL)) { > > /* Things that don't change often */ > > dev_dbg(&client->dev, "Reading config values\n"); > > > > @@ -1540,12 +1542,12 @@ static struct lm85_data *lm85_update_dev > > LM85_REG_IN_MAX(4)); > > } > > > > - if ( data->type == emc6d100 ) { > > + if (data->type == emc6d100) { > > for (i = 5; i <= 7; ++i) { > > - data->in_min[i] = > > - lm85_read_value(client, EMC6D100_REG_IN_MIN(i)); > > - data->in_max[i] = > > - lm85_read_value(client, EMC6D100_REG_IN_MAX(i)); > > + data->in_min[i] = lm85_read_value(client, > > + EMC6D100_REG_IN_MIN(i)); > > + data->in_max[i] = lm85_read_value(client, > > + EMC6D100_REG_IN_MAX(i)); > > } > > } > > > > @@ -1562,12 +1564,12 @@ static struct lm85_data *lm85_update_dev > > } > > > > for (i = 0; i <= 2; ++i) { > > - int val ; > > + int val; > > data->autofan[i].config = > > lm85_read_value(client, LM85_REG_AFAN_CONFIG(i)); > > val = lm85_read_value(client, LM85_REG_AFAN_RANGE(i)); > > - data->autofan[i].freq = val & 0x07 ; > > - data->zone[i].range = (val >> 4) & 0x0f ; > > + data->autofan[i].freq = val & 0x07; > > + data->zone[i].range = (val >> 4) & 0x0f; > > data->autofan[i].min_pwm = > > lm85_read_value(client, LM85_REG_AFAN_MINPWM(i)); > > data->zone[i].limit = > > @@ -1577,50 +1579,50 @@ static struct lm85_data *lm85_update_dev > > } > > > > i = lm85_read_value(client, LM85_REG_AFAN_SPIKE1); > > - data->smooth[0] = i & 0x0f ; > > - data->syncpwm3 = i & 0x10 ; /* Save PWM3 config */ > > - data->autofan[0].min_off = (i & 0x20) != 0 ; > > - data->autofan[1].min_off = (i & 0x40) != 0 ; > > - data->autofan[2].min_off = (i & 0x80) != 0 ; > > + data->smooth[0] = i & 0x0f; > > + data->syncpwm3 = i & 0x10; /* Save PWM3 config */ > > + data->autofan[0].min_off = (i & 0x20) != 0; > > + data->autofan[1].min_off = (i & 0x40) != 0; > > + data->autofan[2].min_off = (i & 0x80) != 0; > > i = lm85_read_value(client, LM85_REG_AFAN_SPIKE2); > > - data->smooth[1] = (i>>4) & 0x0f ; > > - data->smooth[2] = i & 0x0f ; > > + data->smooth[1] = (i>>4) & 0x0f; > > Missing spaces around >>. > > > > + data->smooth[2] = i & 0x0f; > > > > i = lm85_read_value(client, LM85_REG_AFAN_HYST1); > > - data->zone[0].hyst = (i>>4) & 0x0f ; > > - data->zone[1].hyst = i & 0x0f ; > > + data->zone[0].hyst = (i>>4) & 0x0f; > > Same here. > > > > + data->zone[1].hyst = i & 0x0f; > > > > i = lm85_read_value(client, LM85_REG_AFAN_HYST2); > > - data->zone[2].hyst = (i>>4) & 0x0f ; > > + data->zone[2].hyst = (i>>4) & 0x0f; > > And here. All fixed. > There are also 2 spaces on line 1613 (static void __exit > sm_lm85_exit(void)). Fixed. > And a couple of comments with double spaces after > periods. That's frowned upon nowadays AFAIK. I'm not aware of any such rule, and checkpatch doesn't complain, so I'll leave them as is. > Some of the comments also > have multiple leading spaces where there shouldn't be any IMHO (like > line 1606). checkpatch doesn't complain about this either, so I won't change it (although I wouldn't do that in my own code, obviously.) I'll submit an updated patch quickly. I guess that some of the next patches in the series will need a refresh, too. Thanks, -- Jean Delvare