linux-kernel - Re: [PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130729174831.207ed356@endymion.delvare>
Date:	Mon, 29 Jul 2013 17:48:31 +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 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 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/