| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20130727173830.14cb5b21@endymion.delvare>
Date: Sat, 27 Jul 2013 17:38:30 +0200
From: Jean Delvare <khali@...ux-fr.org>
To: Wei Ni <wni@...dia.com>
Cc: <linux@...ck-us.net>, <thierry.reding@...il.com>,
<lm-sensors@...sensors.org>, <linux-kernel@...r.kernel.org>,
<linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8
and temp11
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@...dia.com>
> ---
> 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.
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.
> +
> +/*
> + * 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.
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.
>
> 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.
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists