On 07/27/2013 11:38 PM, Jean Delvare wrote: > Hi Wei, > > On Fri, 12 Jul 2013 15:48:07 +0800, Wei Ni wrote: >> Using enums for the indexes and nrs of temp8 and temp11. >> This make the code much more readable. > > I can't say I'm thrilled by this patch. The improved readability is > questionable. In the original code, each line already had one constant > which made it clear what the code was doing. So the gain is marginal, > I'd say, and this costs almost 50 lines of code. > > Let me review it nevertheless: > >> >> Signed-off-by: Wei Ni <wni@xxxxxxxxxx> >> --- >> drivers/hwmon/lm90.c | 179 ++++++++++++++++++++++++++++++++------------------ >> 1 file changed, 114 insertions(+), 65 deletions(-) >> >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c >> index 1cc3d19..8cb5dd0 100644 >> --- a/drivers/hwmon/lm90.c >> +++ b/drivers/hwmon/lm90.c >> @@ -310,6 +310,59 @@ static const struct lm90_params lm90_params[] = { >> }; >> >> /* >> + * TEMP8 register index >> + */ >> +enum lm90_temp8_reg_index { >> + TEMP8_LOCAL_LOW = 0, /* 0: local low limit */ >> + TEMP8_LOCAL_HIGH, /* 1: local high limit */ >> + TEMP8_LOCAL_CRIT, /* 2: local critical limit */ >> + TEMP8_REMOTE_CRIT, /* 3: remote critical limit */ >> + TEMP8_LOCAL_EMERG, /* 4: local emergency limit >> + * (max6659 and max6695/96) >> + */ >> + TEMP8_REMOTE_EMERG, /* 5: remote emergency limit >> + * (max6659 and max6695/96) >> + */ >> + TEMP8_REMOTE2_CRIT, /* 6: remote 2 critical limit >> + * (max6695/96 only) >> + */ >> + TEMP8_REMOTE2_EMERG, /* 7: remote 2 emergency limit >> + * (max6695/96 only) >> + */ >> + TEMP8_REG_NUM >> +}; >> + >> +/* >> + * TEMP11 register index >> + */ >> +enum lm90_temp11_reg_index { >> + TEMP11_REMOTE_TEMP = 0, /* 0: remote input */ >> + TEMP11_REMOTE_LOW, /* 1: remote low limit */ >> + TEMP11_REMOTE_HIGH, /* 2: remote high limit */ >> + TEMP11_REMOTE_OFFSET, /* 3: remote offset >> + * (except max6646, max6657/58/59, >> + * and max6695/96) >> + */ >> + TEMP11_LOCAL_TEMP, /* 4: local input */ >> + TEMP11_REMOTE2_TEMP, /* 5: remote 2 input (max6695/96 only) */ >> + TEMP11_REMOTE2_LOW, /* 6: remote 2 low limit (max6695/96 only) */ >> + TEMP11_REMOTE2_HIGH, /* 7: remote 2 high limit (max6695/96 only) */ >> + TEMP11_REG_NUM >> +}; > > The "TEMP8_" and "TEMP11_" prefixes aren't really needed, as there is no > overlapping between both sets. Removing these prefixes (except for the > terminators, where they are needed and make sense) would make the rest > of the code more readable IMHO, as it would avoid redundant information > on most lines, and avoid line splitting in some cases. Yes, make sense, I will change it. > > Also, the comments are mostly useless now, they were there to document > what each number was referring to, but now this is exactly what the new > constants are doing. Yes, we can remove these comments, but I think it's better to remain those exception and only things. > >> + >> +/* >> + * TEMP11 register NR >> + */ >> +enum lm90_temp11_reg_nr { >> + NR_CHAN_0_REMOTE_LOW = 0, /* 0: channel 0, remote low limit */ >> + NR_CHAN_0_REMOTE_HIGH, /* 1: channel 0, remote high limit */ >> + NR_CHAN_0_REMOTE_OFFSET, /* 2: channel 0, remote offset */ >> + NR_CHAN_1_REMOTE_LOW, /* 3: channel 1, remote low limit */ >> + NR_CHAN_1_REMOTE_HIGH, /* 4: channel 1, remote high limit */ > > The conventions used in the descriptions diverge from the ones used > above. "channel 0 remote" here is just "remove" above, and "channel 1 > remote" is "remote 2" above. This is quite confusing. > >> + NR_NUM /* number of the NRs for temp11 */ > > The fact that you were unable to come up with a proper name for this > number is a clear indication that this enum should not exist in the > first place. > > These numbers are used only once, to pass specific information to > set_temp11. This was easy enough when these were just numbers and I > really had no reason not to do that. Ok, so how about to remove these changes, and keep the original codes to use numbers. > > If you now want to use clean constants, this should be done with logic > and consistency. Your proposal makes sense for the data->temp8 and > data->temp11 array indexing, because they are used more than once. But > introducing a new set of constants with weird names just for one use > case doesn't help readability, it makes things worse actually. > > So if you insist of making the code more readable by the means of named > constants, then you should simply adjust the reg array in write_temp11 > so that it can be indexed with your TEMP11_* constants above (as is > data->temp11.) This will leave some holes in the array but I don't > think we care about a few lost bytes here. > >> +}; >> + >> +/* >> * Client data (each client gets its own) >> */ >> >> @@ -331,25 +384,8 @@ struct lm90_data { >> u8 reg_local_ext; /* local extension register offset */ >> >> /* registers values */ >> - s8 temp8[8]; /* 0: local low limit >> - * 1: local high limit >> - * 2: local critical limit >> - * 3: remote critical limit >> - * 4: local emergency limit (max6659 and max6695/96) >> - * 5: remote emergency limit (max6659 and max6695/96) >> - * 6: remote 2 critical limit (max6695/96 only) >> - * 7: remote 2 emergency limit (max6695/96 only) >> - */ >> - s16 temp11[8]; /* 0: remote input >> - * 1: remote low limit >> - * 2: remote high limit >> - * 3: remote offset (except max6646, max6657/58/59, >> - * and max6695/96) >> - * 4: local input >> - * 5: remote 2 input (max6695/96 only) >> - * 6: remote 2 low limit (max6695/96 only) >> - * 7: remote 2 high limit (max6695/96 only) >> - */ >> + s8 temp8[TEMP8_REG_NUM]; >> + s16 temp11[TEMP11_REG_NUM]; >> u8 temp_hyst; >> u16 alarms; /* bitvector (upper 8 bits for max6695/96) */ >> }; >> @@ -491,37 +527,42 @@ static struct lm90_data *lm90_update_device(struct device *dev) >> u8 alarms; >> >> dev_dbg(&client->dev, "Updating lm90 data.\n"); >> - lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]); >> - lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, &data->temp8[1]); >> - lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, &data->temp8[2]); >> - lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]); >> + lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, >> + &data->temp8[TEMP8_LOCAL_LOW]); >> + lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, >> + &data->temp8[TEMP8_LOCAL_HIGH]); >> + lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, >> + &data->temp8[TEMP8_LOCAL_CRIT]); >> + lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, >> + &data->temp8[TEMP8_REMOTE_CRIT]); >> lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst); >> >> if (data->reg_local_ext) { >> lm90_read16(client, LM90_REG_R_LOCAL_TEMP, >> data->reg_local_ext, >> - &data->temp11[4]); >> + &data->temp11[TEMP11_LOCAL_TEMP]); >> } else { >> if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP, >> &h) == 0) >> - data->temp11[4] = h << 8; >> + data->temp11[TEMP11_LOCAL_TEMP] = h << 8; >> } >> lm90_read16(client, LM90_REG_R_REMOTE_TEMPH, >> - LM90_REG_R_REMOTE_TEMPL, &data->temp11[0]); >> + LM90_REG_R_REMOTE_TEMPL, >> + &data->temp11[TEMP11_REMOTE_TEMP]); > > Please don't break alignment. Yes, I will do it. > >> >> if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) { >> - data->temp11[1] = h << 8; >> + data->temp11[TEMP11_REMOTE_LOW] = h << 8; >> if ((data->flags & LM90_HAVE_REM_LIMIT_EXT) >> && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL, >> &l) == 0) >> - data->temp11[1] |= l; >> + data->temp11[TEMP11_REMOTE_LOW] |= l; >> } >> if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) { >> - data->temp11[2] = h << 8; >> + data->temp11[TEMP11_REMOTE_HIGH] = h << 8; >> if ((data->flags & LM90_HAVE_REM_LIMIT_EXT) >> && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL, >> &l) == 0) >> - data->temp11[2] |= l; >> + data->temp11[TEMP11_REMOTE_HIGH] |= l; >> } >> >> if (data->flags & LM90_HAVE_OFFSET) { >> @@ -529,13 +570,14 @@ static struct lm90_data *lm90_update_device(struct device *dev) >> &h) == 0 >> && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL, >> &l) == 0) >> - data->temp11[3] = (h << 8) | l; >> + data->temp11[TEMP11_REMOTE_OFFSET] = >> + (h << 8) | l; >> } >> if (data->flags & LM90_HAVE_EMERGENCY) { >> lm90_read_reg(client, MAX6659_REG_R_LOCAL_EMERG, >> - &data->temp8[4]); >> + &data->temp8[TEMP8_LOCAL_EMERG]); >> lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG, >> - &data->temp8[5]); >> + &data->temp8[TEMP8_REMOTE_EMERG]); >> } >> lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); >> data->alarms = alarms; /* save as 16 bit value */ >> @@ -543,15 +585,16 @@ static struct lm90_data *lm90_update_device(struct device *dev) >> if (data->kind == max6696) { >> lm90_select_remote_channel(client, data, 1); >> lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, >> - &data->temp8[6]); >> + &data->temp8[TEMP8_REMOTE2_CRIT]); >> lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG, >> - &data->temp8[7]); >> + &data->temp8[TEMP8_REMOTE2_EMERG]); >> lm90_read16(client, LM90_REG_R_REMOTE_TEMPH, >> - LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]); >> + LM90_REG_R_REMOTE_TEMPL, >> + &data->temp11[TEMP11_REMOTE2_TEMP]); > > Please don't break alignment. > >> if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h)) >> - data->temp11[6] = h << 8; >> + data->temp11[TEMP11_REMOTE2_LOW] = h << 8; >> if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h)) >> - data->temp11[7] = h << 8; >> + data->temp11[TEMP11_REMOTE2_HIGH] = h << 8; >> lm90_select_remote_channel(client, data, 0); >> >> if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2, >> @@ -745,7 +788,7 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr, >> >> static void write_temp8(struct device *dev, int index, long val) >> { >> - static const u8 reg[8] = { >> + static const u8 reg[TEMP8_REG_NUM] = { >> LM90_REG_W_LOCAL_LOW, >> LM90_REG_W_LOCAL_HIGH, >> LM90_REG_W_LOCAL_CRIT, >> @@ -828,7 +871,7 @@ static void write_temp11(struct device *dev, int nr, int index, long val) >> u8 high; >> u8 low; >> int channel; >> - } reg[5] = { >> + } reg[NR_NUM] = { >> { LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 0 }, >> { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 0 }, >> { LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL, 0 }, >> @@ -919,11 +962,12 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy, >> >> mutex_lock(&data->update_lock); >> if (data->kind == adt7461) >> - temp = temp_from_u8_adt7461(data, data->temp8[2]); >> + temp = temp_from_u8_adt7461(data, >> + data->temp8[TEMP8_LOCAL_CRIT]); > > Please align on opening parenthesis. > >> else if (data->kind == max6646) >> - temp = temp_from_u8(data->temp8[2]); >> + temp = temp_from_u8(data->temp8[TEMP8_LOCAL_CRIT]); >> else >> - temp = temp_from_s8(data->temp8[2]); >> + temp = temp_from_s8(data->temp8[TEMP8_LOCAL_CRIT]); >> >> data->temp_hyst = hyst_to_reg(temp - val); >> i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST, >> @@ -977,25 +1021,28 @@ static ssize_t set_update_interval(struct device *dev, >> return count; >> } >> >> -static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, 0, 4); >> -static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, 0, 0); >> +static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, >> + NR_CHAN_0_REMOTE_LOW, TEMP11_LOCAL_TEMP); >> +static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, >> + NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_TEMP); > > NR_CHAN_0_REMOTE_LOW makes no sense for read-only attributes, as the > number is only used in write_temp11(). The original "0" was only > because we can't leave the parameter empty. > >> static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 0); >> + set_temp8, TEMP8_LOCAL_LOW); >> static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11, >> - set_temp11, 0, 1); >> + set_temp11, NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_LOW); >> static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 1); >> + set_temp8, TEMP8_LOCAL_HIGH); >> static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11, >> - set_temp11, 1, 2); >> + set_temp11, NR_CHAN_0_REMOTE_HIGH, TEMP11_REMOTE_HIGH); >> static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 2); >> + set_temp8, TEMP8_LOCAL_CRIT); >> static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 3); >> + set_temp8, TEMP8_REMOTE_CRIT); >> static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst, >> - set_temphyst, 2); >> -static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 3); >> + set_temphyst, TEMP8_LOCAL_CRIT); >> +static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, >> + TEMP8_REMOTE_CRIT); >> static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11, >> - set_temp11, 2, 3); >> + set_temp11, NR_CHAN_0_REMOTE_OFFSET, TEMP11_REMOTE_OFFSET); >> >> /* Individual alarm files */ >> static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0); >> @@ -1043,13 +1090,13 @@ static const struct attribute_group lm90_group = { >> * Additional attributes for devices with emergency sensors >> */ >> static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 4); >> + set_temp8, TEMP8_LOCAL_EMERG); >> static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 5); >> + set_temp8, TEMP8_REMOTE_EMERG); >> static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst, >> - NULL, 4); >> + NULL, TEMP8_LOCAL_EMERG); >> static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst, >> - NULL, 5); >> + NULL, TEMP8_REMOTE_EMERG); >> >> static struct attribute *lm90_emergency_attributes[] = { >> &sensor_dev_attr_temp1_emergency.dev_attr.attr, >> @@ -1079,18 +1126,20 @@ static const struct attribute_group lm90_emergency_alarm_group = { >> /* >> * Additional attributes for devices with 3 temperature sensors >> */ >> -static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5); >> +static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, >> + NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE2_TEMP); > > Here again this NR_CHAN_0_REMOTE_LOW makes no sense for a read-only > attribute, it was really "0" for "we don't care". > >> static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11, >> - set_temp11, 3, 6); >> + set_temp11, NR_CHAN_1_REMOTE_LOW, TEMP11_REMOTE2_LOW); >> static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11, >> - set_temp11, 4, 7); >> + set_temp11, NR_CHAN_1_REMOTE_HIGH, TEMP11_REMOTE2_HIGH); >> static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 6); >> -static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 6); >> + set_temp8, TEMP8_REMOTE2_CRIT); >> +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, >> + TEMP8_REMOTE2_CRIT); >> static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 7); >> + set_temp8, TEMP8_REMOTE2_EMERG); >> static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst, >> - NULL, 7); >> + NULL, TEMP8_REMOTE2_EMERG); >> >> static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9); >> static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10); > > Will all these changes done, I may consider apply the modified patch, > but no guarantee, as I'm still not sure I like it. I'll make my mind > when I see the result. I really appreciate you reviewed my patches so carefully. I will update changes in my next version. Thanks. Wei. > > -- > Jean Delvare > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html