From: Guenter Roeck <linux@roeck-us.net>
To: "Björn Gerhart" <gerhart@posteo.de>
Cc: Hardware Monitoring <linux-hwmon@vger.kernel.org>
Subject: Re: [PATCH] hwmon: (nct6775) Integrated new model nct6116
Date: Wed, 10 Jul 2019 13:38:45 -0700 [thread overview]
Message-ID: <20190710203845.GA6330@roeck-us.net> (raw)
In-Reply-To: <76CF99AD-62AF-4929-AA67-D441A68B00A0@posteo.de>
On Wed, Jul 10, 2019 at 09:29:55PM +0200, Björn Gerhart wrote:
> oh no, the patch is WRONG as I noticed right now. Sorry for the noise! I’ll re-work it and come back!
>
Now I have to figure out what you mean ;-).
Couple of comments anyway below.
Guenter
> > Am 10.07.2019 um 21:19 schrieb Björn Gerhart <gerhart@posteo.de>:
> >
> > The nct6116 is the successor model of nct6106. Mainly I’m unsure about my changes related to:
> > - NCT6116_BEEP_BITS
> > - int cr24 (cr23 is new, and I’m unsure if the code has to get modified therefore)
> > - NCT6116_REG_FAN_PULSES
> > - NCT6116_FAN_PULSE_SHIFT
> > - NCT6116_ALARM_BITS
> >
> > With the nct6116 wired on my mainboard, the sensors command seems to work as I would expect.
> >
> > Index: nct6775.c
> > ===================================================================
> > --- nct6775.c (revision 16313)
> > +++ nct6775.c (working copy)
> > @@ -34,6 +34,7 @@
> > *
> > * Chip #vin #fan #pwm #temp chip IDs man ID
> > * nct6106d 9 3 3 6+3 0xc450 0xc1 0x5ca3
> > + * nct6116d 9 5 5 3+3 0xd280 0xc1 0x5ca3
> > * nct6775f 9 4 3 6+3 0xb470 0xc1 0x5ca3
> > * nct6776f 9 5 3 6+3 0xc330 0xc1 0x5ca3
> > * nct6779d 15 5 5 2+6 0xc560 0xc1 0x5ca3
> > @@ -72,12 +73,13 @@
> >
> > #define USE_ALTERNATE
> >
> > -enum kinds { nct6106, nct6775, nct6776, nct6779, nct6791, nct6792, nct6793,
> > - nct6795, nct6796, nct6797, nct6798 };
> > +enum kinds { nct6106, nct6116, nct6775, nct6776, nct6779, nct6791, nct6792,
> > + nct6793, nct6795, nct6796, nct6797, nct6798 };
> >
> > /* used to set data->name = nct6775_device_names[data->sio_kind] */
> > static const char * const nct6775_device_names[] = {
> > "nct6106",
> > + "nct6116",
> > "nct6775",
> > "nct6776",
> > "nct6779",
> > @@ -92,6 +94,7 @@
> >
> > static const char * const nct6775_sio_names[] __initconst = {
> > "NCT6106D",
> > + "NCT6116D",
> > "NCT6775F",
> > "NCT6776D/F",
> > "NCT6779D",
> > @@ -129,6 +132,7 @@
> > #define SIO_REG_ADDR 0x60 /* Logical device address (2 bytes) */
> >
> > #define SIO_NCT6106_ID 0xc450
> > +#define SIO_NCT6116_ID 0xd280
> > #define SIO_NCT6775_ID 0xb470
> > #define SIO_NCT6776_ID 0xc330
> > #define SIO_NCT6779_ID 0xc560
> > @@ -819,18 +823,29 @@
> > 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x07, 0x08, 0x09 };
> >
> > static const u16 NCT6106_REG_TEMP[] = { 0x10, 0x11, 0x12, 0x13, 0x14, 0x15 };
> > +static const u16 NCT6116_REG_TEMP[] = { 0x10, 0x11, 0x12 };
> > static const u16 NCT6106_REG_TEMP_MON[] = { 0x18, 0x19, 0x1a };
> > static const u16 NCT6106_REG_TEMP_HYST[] = {
> > 0xc3, 0xc7, 0xcb, 0xcf, 0xd3, 0xd7 };
> > +static const u16 NCT6116_REG_TEMP_HYST[] = {
> > + 0xc3, 0xc7, 0xcb };
Please move all NCT6116 related declarations/defines together.
> > static const u16 NCT6106_REG_TEMP_OVER[] = {
> > 0xc2, 0xc6, 0xca, 0xce, 0xd2, 0xd6 };
> > +static const u16 NCT6116_REG_TEMP_OVER[] = {
> > + 0xc2, 0xc6, 0xca };
> > static const u16 NCT6106_REG_TEMP_CRIT_L[] = {
> > 0xc0, 0xc4, 0xc8, 0xcc, 0xd0, 0xd4 };
> > +static const u16 NCT6116_REG_TEMP_CRIT_L[] = {
> > + 0xc0, 0xc4, 0xc8 };
> > static const u16 NCT6106_REG_TEMP_CRIT_H[] = {
> > 0xc1, 0xc5, 0xc9, 0xcf, 0xd1, 0xd5 };
> > +static const u16 NCT6116_REG_TEMP_CRIT_H[] = {
> > + 0xc1, 0xc5, 0xc9 };
> > static const u16 NCT6106_REG_TEMP_OFFSET[] = { 0x311, 0x312, 0x313 };
> > static const u16 NCT6106_REG_TEMP_CONFIG[] = {
> > 0xb7, 0xb8, 0xb9, 0xba, 0xbb, 0xbc };
> > +static const u16 NCT6116_REG_TEMP_CONFIG[] = {
> > + 0xb7, 0xb8, 0xb9 };
> >
> > static const u16 NCT6106_REG_FAN[] = { 0x20, 0x22, 0x24 };
> > static const u16 NCT6106_REG_FAN_MIN[] = { 0xe0, 0xe2, 0xe4 };
> > @@ -845,6 +860,8 @@
> > static const u16 NCT6106_REG_TEMP_SEL[] = { 0x110, 0x120, 0x130 };
> > static const u16 NCT6106_REG_TEMP_SOURCE[] = {
> > 0xb0, 0xb1, 0xb2, 0xb3, 0xb4, 0xb5 };
> > +static const u16 NCT6116_REG_TEMP_SOURCE[] = {
> > + 0xb0, 0xb1, 0xb2 };
> >
> > static const u16 NCT6106_REG_CRITICAL_TEMP[] = { 0x11a, 0x12a, 0x13a };
> > static const u16 NCT6106_REG_CRITICAL_TEMP_TOLERANCE[] = {
> > @@ -886,6 +903,16 @@
> > 48, -1 /* intrusion0, intrusion1 */
> > };
> >
> > +static const s8 NCT6116_ALARM_BITS[] = {
> > + 0, 1, 2, 3, 4, 5, 7, 8, /* in0.. in7 */
> > + 9, -1, -1, -1, -1, -1, -1, /* in8..in14 */
> > + -1, /* unused */
> > + 32, 33, 34, -1, -1, /* fan1..fan5 */
> > + -1, -1, -1, /* unused */
> > + 16, 17, 18, /* temp1..temp3 */
> > + 48, -1 /* intrusion0, intrusion1 */
> > +};
> > +
> > static const u16 NCT6106_REG_BEEP[NUM_REG_BEEP] = {
> > 0x3c0, 0x3c1, 0x3c2, 0x3c3, 0x3c4 };
> >
> > @@ -1308,6 +1335,10 @@
> > return reg == 0x20 || reg == 0x22 || reg == 0x24 ||
> > reg == 0xe0 || reg == 0xe2 || reg == 0xe4 ||
> > reg == 0x111 || reg == 0x121 || reg == 0x131;
> > + case nct6116:
> > + return reg == 0x20 || reg == 0x22 || reg == 0x24 ||
> > + reg == 0xe0 || reg == 0xe2 || reg == 0xe4 ||
> > + reg == 0x111 || reg == 0x121 || reg == 0x131;
Same as case statement for nct6106; please merge.
> > case nct6775:
> > return (((reg & 0xff00) == 0x100 ||
> > (reg & 0xff00) == 0x200) &&
> > @@ -1687,6 +1718,7 @@
> > data->auto_pwm[i][data->auto_pwm_num] = 0xff;
> > break;
> > case nct6106:
> > + case nct6116:
> > case nct6779:
> > case nct6791:
> > case nct6792:
> > @@ -3123,6 +3155,7 @@
> > case nct6776:
> > break; /* always enabled, nothing to do */
> > case nct6106:
> > + case nct6116:
> > case nct6779:
> > case nct6791:
> > case nct6792:
> > @@ -3549,6 +3582,11 @@
> >
> > fan3pin = !(cr24 & 0x80);
> > pwm3pin = cr24 & 0x08;
> > + } else if (data->kind == nct6116) {
> > + int cr24 = superio_inb(sioreg, 0x24);
> > +
> > + fan3pin = !(cr24 & 0x80);
> > + pwm3pin = cr24 & 0x08;
This is the same as the if statement for nct6106. Please merge into a single
if statement.
> > } else {
> > /*
> > * NCT6779D, NCT6791D, NCT6792D, NCT6793D, NCT6795D, NCT6796D,
> > @@ -3820,6 +3858,78 @@
> > reg_temp_crit_h = NCT6106_REG_TEMP_CRIT_H;
> >
> > break;
> > + case nct6116:
> > + data->in_num = 9;
> > + data->pwm_num = 3;
> > + data->auto_pwm_num = 4;
> > + data->temp_fixed_num = 3;
> > + data->num_temp_alarms = 3;
> > + data->num_temp_beeps = 3;
> > +
> > + data->fan_from_reg = fan_from_reg13;
> > + data->fan_from_reg_min = fan_from_reg13;
> > +
> > + data->temp_label = nct6776_temp_label;
> > + data->temp_mask = NCT6776_TEMP_MASK;
> > + data->virt_temp_mask = NCT6776_VIRT_TEMP_MASK;
> > +
> > + data->REG_VBAT = NCT6106_REG_VBAT;
> > + data->REG_DIODE = NCT6106_REG_DIODE;
> > + data->DIODE_MASK = NCT6106_DIODE_MASK;
> > + data->REG_VIN = NCT6106_REG_IN;
> > + data->REG_IN_MINMAX[0] = NCT6106_REG_IN_MIN;
> > + data->REG_IN_MINMAX[1] = NCT6106_REG_IN_MAX;
> > + data->REG_TARGET = NCT6106_REG_TARGET;
> > + data->REG_FAN = NCT6106_REG_FAN;
> > + data->REG_FAN_MODE = NCT6106_REG_FAN_MODE;
> > + data->REG_FAN_MIN = NCT6106_REG_FAN_MIN;
> > + data->REG_FAN_PULSES = NCT6106_REG_FAN_PULSES;
> > + data->FAN_PULSE_SHIFT = NCT6106_FAN_PULSE_SHIFT;
> > + data->REG_FAN_TIME[0] = NCT6106_REG_FAN_STOP_TIME;
> > + data->REG_FAN_TIME[1] = NCT6106_REG_FAN_STEP_UP_TIME;
> > + data->REG_FAN_TIME[2] = NCT6106_REG_FAN_STEP_DOWN_TIME;
> > + data->REG_PWM[0] = NCT6106_REG_PWM;
> > + data->REG_PWM[1] = NCT6106_REG_FAN_START_OUTPUT;
> > + data->REG_PWM[2] = NCT6106_REG_FAN_STOP_OUTPUT;
> > + data->REG_PWM[5] = NCT6106_REG_WEIGHT_DUTY_STEP;
> > + data->REG_PWM[6] = NCT6106_REG_WEIGHT_DUTY_BASE;
> > + data->REG_PWM_READ = NCT6106_REG_PWM_READ;
> > + data->REG_PWM_MODE = NCT6106_REG_PWM_MODE;
> > + data->PWM_MODE_MASK = NCT6106_PWM_MODE_MASK;
> > + data->REG_AUTO_TEMP = NCT6106_REG_AUTO_TEMP;
> > + data->REG_AUTO_PWM = NCT6106_REG_AUTO_PWM;
> > + data->REG_CRITICAL_TEMP = NCT6106_REG_CRITICAL_TEMP;
> > + data->REG_CRITICAL_TEMP_TOLERANCE
> > + = NCT6106_REG_CRITICAL_TEMP_TOLERANCE;
> > + data->REG_CRITICAL_PWM_ENABLE = NCT6106_REG_CRITICAL_PWM_ENABLE;
> > + data->CRITICAL_PWM_ENABLE_MASK
> > + = NCT6106_CRITICAL_PWM_ENABLE_MASK;
> > + data->REG_CRITICAL_PWM = NCT6106_REG_CRITICAL_PWM;
> > + data->REG_TEMP_OFFSET = NCT6106_REG_TEMP_OFFSET;
> > + data->REG_TEMP_SOURCE = NCT6116_REG_TEMP_SOURCE;
> > + data->REG_TEMP_SEL = NCT6106_REG_TEMP_SEL;
> > + data->REG_WEIGHT_TEMP_SEL = NCT6106_REG_WEIGHT_TEMP_SEL;
> > + data->REG_WEIGHT_TEMP[0] = NCT6106_REG_WEIGHT_TEMP_STEP;
> > + data->REG_WEIGHT_TEMP[1] = NCT6106_REG_WEIGHT_TEMP_STEP_TOL;
> > + data->REG_WEIGHT_TEMP[2] = NCT6106_REG_WEIGHT_TEMP_BASE;
> > + data->REG_ALARM = NCT6106_REG_ALARM;
> > + data->ALARM_BITS = NCT6116_ALARM_BITS;
> > + data->REG_BEEP = NCT6106_REG_BEEP;
> > + data->BEEP_BITS = NCT6106_BEEP_BITS;
> > +
> > + reg_temp = NCT6116_REG_TEMP;
> > + reg_temp_mon = NCT6106_REG_TEMP_MON;
> > + num_reg_temp = ARRAY_SIZE(NCT6116_REG_TEMP);
> > + num_reg_temp_mon = ARRAY_SIZE(NCT6106_REG_TEMP_MON);
> > + reg_temp_over = NCT6116_REG_TEMP_OVER;
> > + reg_temp_hyst = NCT6116_REG_TEMP_HYST;
> > + reg_temp_config = NCT6116_REG_TEMP_CONFIG;
> > + reg_temp_alternate = NCT6106_REG_TEMP_ALTERNATE;
> > + reg_temp_crit = NCT6106_REG_TEMP_CRIT;
> > + reg_temp_crit_l = NCT6116_REG_TEMP_CRIT_L;
> > + reg_temp_crit_h = NCT6116_REG_TEMP_CRIT_H;
> > +
> > + break;
> > case nct6775:
> > data->in_num = 9;
> > data->pwm_num = 3;
> > @@ -4365,6 +4475,7 @@
> > data->have_vid = (cr2a & 0x60) == 0x40;
> > break;
> > case nct6106:
> > + case nct6116:
> > case nct6779:
> > case nct6791:
> > case nct6792:
> > @@ -4396,6 +4507,9 @@
> > case nct6106:
> > tmp |= 0xe0;
> > break;
> > + case nct6116:
> > + tmp |= 0xe0;
> > + break;
Identical code; please merge with nct6106 case.
> > case nct6775:
> > tmp |= 0x1e;
> > break;
> > @@ -4589,6 +4703,9 @@
> > case SIO_NCT6106_ID:
> > sio_data->kind = nct6106;
> > break;
> > + case SIO_NCT6116_ID:
> > + sio_data->kind = nct6116;
> > + break;
> > case SIO_NCT6775_ID:
> > sio_data->kind = nct6775;
> > break;
> >
>
next prev parent reply other threads:[~2019-07-10 20:38 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-10 19:19 [PATCH] hwmon: (nct6775) Integrated new model nct6116 Björn Gerhart
2019-07-10 19:29 ` Björn Gerhart
2019-07-10 20:38 ` Guenter Roeck [this message]
2019-07-15 16:47 Björn Gerhart
2019-07-16 13:44 ` Guenter Roeck
2019-07-18 20:19 ` Björn Gerhart
2019-07-19 18:01 ` Björn Gerhart
2019-07-19 19:37 ` Guenter Roeck
2019-07-22 17:46 ` Björn Gerhart
2019-07-22 20:12 ` Guenter Roeck
2019-07-23 16:06 ` Björn Gerhart
2019-07-23 16:23 ` Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190710203845.GA6330@roeck-us.net \
--to=linux@roeck-us.net \
--cc=gerhart@posteo.de \
--cc=linux-hwmon@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.