Hi Guenter,
On Sun, 29 Mar 2015 23:33:47 -0700, Guenter Roeck wrote:
> IT8620E supports up to 6 fan tachometers.
>
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
> drivers/hwmon/it87.c | 51 ++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index ee6d30960fce..24e9369f1b74 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -215,11 +215,11 @@ static bool fix_pwm_polarity;
>
> /* Monitors: 9 voltage (0 to 7, battery), 3 temp (1 to 3), 3 fan (1 to 3) */
The comment above it out of date, BTW.
>
> -static const u8 IT87_REG_FAN[] = { 0x0d, 0x0e, 0x0f, 0x80, 0x82 };
> -static const u8 IT87_REG_FAN_MIN[] = { 0x10, 0x11, 0x12, 0x84, 0x86 };
> -static const u8 IT87_REG_FANX[] = { 0x18, 0x19, 0x1a, 0x81, 0x83 };
> -static const u8 IT87_REG_FANX_MIN[] = { 0x1b, 0x1c, 0x1d, 0x85, 0x87 };
> -static const u8 IT87_REG_TEMP_OFFSET[] = { 0x56, 0x57, 0x59 };
> +static const u8 IT87_REG_FAN[] = {0x0d, 0x0e, 0x0f, 0x80, 0x82, 0x4c};
> +static const u8 IT87_REG_FAN_MIN[] = {0x10, 0x11, 0x12, 0x84, 0x86, 0x4e};
> +static const u8 IT87_REG_FANX[] = {0x18, 0x19, 0x1a, 0x81, 0x83, 0x4d};
> +static const u8 IT87_REG_FANX_MIN[] = {0x1b, 0x1c, 0x1d, 0x85, 0x87, 0x4f};
> +static const u8 IT87_REG_TEMP_OFFSET[] = {0x56, 0x57, 0x59};
I see why you removed the spaces inside the curly braces, but that's
inconsistent with enum chips and the style used in the vast majority of
other hwmon drivers. I would rather align with spaces instead of tabs
so that you can move everything by a few columns to the left and
everything fits again. (Until the next chip that will support 7 fans,
that is ^^.)
(That's typically the kind of comment you're free to ignore if you
prefer the way you did.)
>
> #define IT87_REG_FAN_MAIN_CTRL 0x13
> #define IT87_REG_FAN_CTL 0x14
> (...)
> @@ -2569,13 +2586,17 @@ static void it87_init_device(struct platform_device *pdev)
>
> /* Check for additional fans */
> if (has_five_fans(data)) {
> - tmp = it87_read_value(data, IT87_REG_FAN_16BIT);
> if (tmp & (1 << 4))
> data->has_fan |= (1 << 3); /* fan4 enabled */
> if (tmp & (1 << 5))
> data->has_fan |= (1 << 4); /* fan5 enabled */
> }
>
> + if (has_six_fans(data)) {
> + if (tmp & (1 << 2))
> + data->has_fan |= (1 << 5); /* fan6 enabled */
> + }
> +
May I suggest the following optimization?
/* Check for additional fans */
if (has_five_fans(data)) {
if (tmp & (1 << 4))
data->has_fan |= (1 << 3); /* fan4 enabled */
if (tmp & (1 << 5))
data->has_fan |= (1 << 4); /* fan5 enabled */
if (has_six_fans(data) && (tmp & (1 << 2)))
data->has_fan |= (1 << 5); /* fan6 enabled */
}
This fits in fewer lines and saves a test for 3-fan chips.
Looks good otherwise:
Reviewed-by: Jean Delvare <jdelvare@xxxxxxx>
--
Jean Delvare
SUSE L3 Support
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
