Re: [PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11
Hi Wei,
On Mon, 29 Jul 2013 19:15:12 +0800, Wei Ni wrote:
> On 07/27/2013 11:38 PM, Jean Delvare wrote:
> > On Fri, 12 Jul 2013 15:48:07 +0800, Wei Ni wrote:
> >> +/*
> >> + * 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
> >> +};
> > (...)
> > 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.
Yes, I agree.
> >> +
> >> +/*
> >> + * 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.
Fine with me. We can always change the code later to use the TEMP11
index values instead if anyone cares, this can be done separately.
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11
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
>> ---
>> 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
Re: [PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11
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 w...@nvidia.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.
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
Re: [PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11
Hi Wei,
On Mon, 29 Jul 2013 19:15:12 +0800, Wei Ni wrote:
On 07/27/2013 11:38 PM, Jean Delvare wrote:
On Fri, 12 Jul 2013 15:48:07 +0800, Wei Ni wrote:
+/*
+ * 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
+};
(...)
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.
Yes, I agree.
+
+/*
+ * 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.
Fine with me. We can always change the code later to use the TEMP11
index values instead if anyone cares, this can be done separately.
--
Jean Delvare
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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
> ---
> 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 */
>
>
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 w...@nvidia.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
-
[PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11
Using enums for the indexes and nrs of temp8 and temp11.
This make the code much more readable.
Signed-off-by: Wei Ni
---
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
+};
+
+/*
+ * 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 */
+ NR_NUM /* number of the NRs for temp11 */
+};
+
+/*
* 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(>dev, "Updating lm90 data.\n");
- lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, >temp8[0]);
- lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, >temp8[1]);
- lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, >temp8[2]);
- lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, >temp8[3]);
+ lm90_read_reg(client, LM90_REG_R_LOCAL_LOW,
+ >temp8[TEMP8_LOCAL_LOW]);
+ lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH,
+ >temp8[TEMP8_LOCAL_HIGH]);
+ lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT,
+ >temp8[TEMP8_LOCAL_CRIT]);
+ lm90_read_reg(client,
[PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11
Using enums for the indexes and nrs of temp8 and temp11.
This make the code much more readable.
Signed-off-by: Wei Ni w...@nvidia.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
+};
+
+/*
+ * 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 */
+ NR_NUM /* number of the NRs for temp11 */
+};
+
+/*
* 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]);
+
