Thanks Guenter! Please see my comments below.
> Am 16.07.2019 um 15:44 schrieb Guenter Roeck <linux@xxxxxxxxxxxx>:
>
> On 7/15/19 9:47 AM, Björn Gerhart wrote:
>> Hello again and thanks for the early feedback! This is the re-worked patch of the one I sent last week. It’s created against kernel 5.1.15 and already depends on the preceding patch concerning the same code.
>> The nct6116 is the successor model of nct6106. Mainly, I’m unsure if certain details of the spec are mapped correctly to the code. These details are:
>> - NCT6116_BEEP_BITS
>
> looks good to me.
>
>> - int cr24 (cr23 is new, and I’m unsure if the code has to get modified therefore)
>
> I don't see cr23h in the datasheet available to me. What am I missing ?
>
My fault - there is no cr23h at all.
> But, yes, there are differences:
>
> nct6116:
> cr1ah[2] determines AUXFANIN1/AUXFANOUT1 settings
> cr1b[1-1] and cr2a[1] determine AUXFANIN2/AUXFANOUT2 settings
> cr2b[7] is an alternate for AUXFANIN1/AUXFANOUT1
> cr2b[4] enable AUXFANIN0/AUXFANOUT0 (same as AUXFANIN/AUXFANOUT ?) on pin 6, 14
> cr24[3] enables AUXFANOUT on pin 14
>
> nct6106:
> cr24[7] enables AUXFANIN on pin 6
> cr24[3] enables AUXFANOUT on pin 14
>
This part turns to be the most tricky one. For me the meaning/interpretation of all the possible options is quite confusing. Especially when some contents might get mirrored to different registers.
The nct6116 covers 5 fans in total, and I identified fan4 as AUXFAN1 and fan5 as AUXFAN2. So respectively, AUXFAN0 should be the same as AUXFAN and reference to fan3.
Following, the piece of code which I hope to reflect this part of the datasheet properly:
{ else if (data->kind == nct6116) {
int cr1a = superio_inb(sioreg, 0x1a);
int cr1b = superio_inb(sioreg, 0x1b);
int cr24 = superio_inb(sioreg, 0x24);
int cr2a = superio_inb(sioreg, 0x2a);
pwm3pin = cr24 & 0x08;
fan3pin = cr2b & 0x10;
pwm4pin = cr1a & 0x04;
if !(cr1b & 0x03) {
pwm5pin = cr2a & 0x02;
}
The registers described in the spec do not differ between pwmpin and fanpin. Which one shall get used?
If the same content is good for in and out, is pwm4pin == fan4pin?
>> - 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.
>> Signed-off-by: Bjoern Gerhart <gerhart@xxxxxxxxx>
>> ---
>> --- nct6775-5.1.15-1.el7.elrepo.x86_64-bugfixed/nct6775.c 2019-07-10 17:08:30.710690605 +0200
>> +++ nct6775/nct6775.c 2019-07-10 17:30:38.012416414 +0200
>> @@ -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
>> @@ -910,6 +914,119 @@
>> [12] = 0x205,
>> };
>> +/* NCT6112D/NCT6114D/NCT6116D specific data */
>> +
>> +#define NCT6116_REG_VBAT 0x318
>> +#define NCT6116_REG_DIODE 0x319
>> +#define NCT6116_DIODE_MASK 0x01
>> +
>> +static const u16 NCT6116_REG_IN_MAX[] = {
>> + 0x90, 0x92, 0x94, 0x96, 0x98, 0x9a, 0x9e, 0xa0, 0xa2 };
>> +static const u16 NCT6116_REG_IN_MIN[] = {
>> + 0x91, 0x93, 0x95, 0x97, 0x99, 0x9b, 0x9f, 0xa1, 0xa3 };
>> +static const u16 NCT6116_REG_IN[] = {
>> + 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x07, 0x08, 0x09 };
>> +
>> +static const u16 NCT6116_REG_TEMP[] = { 0x10, 0x11, 0x12 };
>> +static const u16 NCT6116_REG_TEMP_MON[] = { 0x18, 0x19, 0x1a };
>> +static const u16 NCT6116_REG_TEMP_HYST[] = { 0xc3, 0xc7, 0xcb };
>> +static const u16 NCT6116_REG_TEMP_OVER[] = { 0xc2, 0xc6, 0xca };
>> +static const u16 NCT6116_REG_TEMP_CRIT_L[] = { 0xc0, 0xc4, 0xc8 };
>> +static const u16 NCT6116_REG_TEMP_CRIT_H[] = { 0xc1, 0xc5, 0xc9 };
>> +static const u16 NCT6116_REG_TEMP_OFFSET[] = { 0x311, 0x312, 0x313 };
>> +static const u16 NCT6116_REG_TEMP_CONFIG[] = { 0xb7, 0xb8, 0xb9 };
>> +
>> +static const u16 NCT6116_REG_FAN[] = { 0x20, 0x22, 0x24, 0x26, 0x28 };
>> +static const u16 NCT6116_REG_FAN_MIN[] = { 0xe0, 0xe2, 0xe4, 0xe6, 0xe8 };
>> +static const u16 NCT6116_REG_FAN_PULSES[] = { 0xf6, 0xf6, 0xf6, 0xf6, 0xf5 };
>> +static const u16 NCT6116_FAN_PULSE_SHIFT[] = { 0, 2, 4, 6, 6 };
>> +
>> +static const u8 NCT6116_REG_PWM_MODE[] = { 0xf3, 0xf3, 0xf3 };
>> +static const u8 NCT6116_PWM_MODE_MASK[] = { 0x01, 0x02, 0x04 };
>> +static const u16 NCT6116_REG_PWM[] = { 0x119, 0x129, 0x139, 0x199, 0x1a9 };
>> +static const u16 NCT6116_REG_PWM_READ[] = { 0x4a, 0x4b, 0x4c };
>> +static const u16 NCT6116_REG_FAN_MODE[] = { 0x113, 0x123, 0x133, 0x193, 0x1a3 };
>> +static const u16 NCT6116_REG_TEMP_SEL[] = { 0x110, 0x120, 0x130, 0x190, 0x1a0 };
>> +static const u16 NCT6116_REG_TEMP_SOURCE[] = { 0xb0, 0xb1, 0xb2 };
>> +
>> +static const u16 NCT6116_REG_CRITICAL_TEMP[] = {
>> + 0x11a, 0x12a, 0x13a, 0x19a, 0x1aa };
>> +static const u16 NCT6116_REG_CRITICAL_TEMP_TOLERANCE[] = {
>> + 0x11b, 0x12b, 0x13b, 0x19b, 0x1ab };
>> +
>> +static const u16 NCT6116_REG_CRITICAL_PWM_ENABLE[] = {
>> + 0x11c, 0x12c, 0x13c, 0x19c, 0x1ac };
>> +#define NCT6116_CRITICAL_PWM_ENABLE_MASK 0x10
>> +static const u16 NCT6116_REG_CRITICAL_PWM[] = {
>> + 0x11d, 0x12d, 0x13d, 0x19d, 0x1ad };
>> +
>> +static const u16 NCT6116_REG_FAN_STEP_UP_TIME[] = {
>> + 0x114, 0x124, 0x134, 0x194, 0x1a4 };
>> +static const u16 NCT6116_REG_FAN_STEP_DOWN_TIME[] = {
>> + 0x115, 0x125, 0x135, 0x195, 0x1a5 };
>> +static const u16 NCT6116_REG_FAN_STOP_OUTPUT[] = {
>> + 0x116, 0x126, 0x136, 0x196, 0x1a6 };
>> +static const u16 NCT6116_REG_FAN_START_OUTPUT[] = {
>> + 0x117, 0x127, 0x137, 0x197, 0x1a7 };
>> +static const u16 NCT6116_REG_FAN_STOP_TIME[] = {
>> + 0x118, 0x128, 0x138, 0x198, 0x1a8 };
>> +static const u16 NCT6116_REG_TOLERANCE_H[] = {
>> + 0x112, 0x122, 0x132, 0x192, 0x1a2 };
>> +
>> +static const u16 NCT6116_REG_TARGET[] = {
>> + 0x111, 0x121, 0x131, 0x191, 0x1a1 };
>> +
>> +static const u16 NCT6116_REG_WEIGHT_TEMP_SEL[] = { 0x168, 0x178, 0x188 };
>> +static const u16 NCT6116_REG_WEIGHT_TEMP_STEP[] = { 0x169, 0x179, 0x189 };
>> +static const u16 NCT6116_REG_WEIGHT_TEMP_STEP_TOL[] = { 0x16a, 0x17a, 0x18a };
>> +static const u16 NCT6116_REG_WEIGHT_DUTY_STEP[] = { 0x16b, 0x17b, 0x18b };
>> +static const u16 NCT6116_REG_WEIGHT_TEMP_BASE[] = { 0x16c, 0x17c, 0x18c };
>> +static const u16 NCT6116_REG_WEIGHT_DUTY_BASE[] = { 0x16d, 0x17d, 0x18d };
>> +
>> +static const u16 NCT6116_REG_AUTO_TEMP[] = {
>> + 0x160, 0x170, 0x180, 0x1d0, 0x1e0 };
>> +static const u16 NCT6116_REG_AUTO_PWM[] = {
>> + 0x164, 0x174, 0x184, 0x1d4, 0x1e4 };
>> +
>> +static const u16 NCT6116_REG_ALARM[NUM_REG_ALARM] = {
>> + 0x77, 0x78, 0x79, 0x7a, 0x7b, 0x7c, 0x7d };
>> +
>> +static const s8 NCT6116_ALARM_BITS[] = {
>> + 0, 1, 2, 3, 4, 5, 7, 8, /* in0.. in7 */
>> + 9, -1, -1, -1, -1, -1, -1, /* in8..in9 */
>> + -1, /* unused */
>> + 32, 33, 34, 35, 36, /* fan1..fan5 */
>> + -1, -1, -1, /* unused */
>> + 16, 17, 18, -1, -1, -1, /* temp1..temp6 */
>> + 48, -1 /* intrusion0, intrusion1 */
>> +};
>> +
>> +static const u16 NCT6116_REG_BEEP[NUM_REG_BEEP] = {
>> + 0x3c0, 0x3c1, 0x3c2, 0x3c3, 0x3c4 };
>> +
>> +static const s8 NCT6116_BEEP_BITS[] = {
>> + 0, 1, 2, 3, 4, 5, 7, 8, /* in0.. in7 */
>> + 9, 10, 11, 12, -1, -1, -1, /* in8..in14 */
>> + 32, /* global beep enable */
>> + 24, 25, 26, 27, 28, /* fan1..fan5 */
>> + -1, -1, -1, /* unused */
>> + 16, 17, 18, -1, -1, -1, /* temp1..temp6 */
>> + 34, -1 /* intrusion0, intrusion1 */
>> +};
>> +
>> +static const u16 NCT6116_REG_TEMP_ALTERNATE[32] = {
>> + [14] = 0x51,
>> + [15] = 0x52,
>> + [16] = 0x54,
>> +};
>> +
>> +static const u16 NCT6116_REG_TEMP_CRIT[32] = {
>> + [11] = 0x204,
>> + [12] = 0x205,
>> +};
>> +
> Unfortunately, this time you duplicated data for _all_ registers, not just
> the different ones. Idea is to retain the registers which don't change
> and only list the changed registers for a new chip.
>
>> +
>> +
> No more than one empty line, please.
>
>> static enum pwm_enable reg_to_pwm_enable(int pwm, int mode)
>> {
>> if (mode == 0 && pwm == 255)
>> @@ -1308,6 +1425,11 @@
>> 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 == 0x26 || reg == 0x28 || reg == 0xe0 || reg == 0xe2 ||
>> + reg == 0xe4 || reg == 0xe6 || reg == 0xe8 || reg == 0x111 ||
>> + reg == 0x121 || reg == 0x131 || reg == 0x191 || reg == 0x1a1;
>> case nct6775:
>> return (((reg & 0xff00) == 0x100 ||
>> (reg & 0xff00) == 0x200) &&
>> @@ -1687,6 +1809,7 @@
>> data->auto_pwm[i][data->auto_pwm_num] = 0xff;
>> break;
>> case nct6106:
>> + case nct6116:
>> case nct6779:
>> case nct6791:
>> case nct6792:
>> @@ -3123,6 +3246,7 @@
>> case nct6776:
>> break; /* always enabled, nothing to do */
>> case nct6106:
>> + case nct6116:
>> case nct6779:
>> case nct6791:
>> case nct6792:
>> @@ -3544,7 +3668,7 @@
>> fan4min = fan4pin;
>> pwm3pin = fan3pin;
>> - } else if (data->kind == nct6106) {
>> + } else if ((data->kind == nct6106) || (data->kind == nct6116)) {
>
> nct6116 will need a separate if statement per above.
>
>> int cr24 = superio_inb(sioreg, 0x24);
>> fan3pin = !(cr24 & 0x80);
>> @@ -3821,6 +3945,79 @@
>> 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 = NCT6116_REG_VBAT;
>> + data->REG_DIODE = NCT6116_REG_DIODE;
>> + data->DIODE_MASK = NCT6116_DIODE_MASK;
>> + data->REG_VIN = NCT6116_REG_IN;
>> + data->REG_IN_MINMAX[0] = NCT6116_REG_IN_MIN;
>> + data->REG_IN_MINMAX[1] = NCT6116_REG_IN_MAX;
>> + data->REG_TARGET = NCT6116_REG_TARGET;
>> + data->REG_FAN = NCT6116_REG_FAN;
>> + data->REG_FAN_MODE = NCT6116_REG_FAN_MODE;
>> + data->REG_FAN_MIN = NCT6116_REG_FAN_MIN;
>> + data->REG_FAN_PULSES = NCT6116_REG_FAN_PULSES;
>> + data->FAN_PULSE_SHIFT = NCT6116_FAN_PULSE_SHIFT;
>> + data->REG_FAN_TIME[0] = NCT6116_REG_FAN_STOP_TIME;
>> + data->REG_FAN_TIME[1] = NCT6116_REG_FAN_STEP_UP_TIME;
>> + data->REG_FAN_TIME[2] = NCT6116_REG_FAN_STEP_DOWN_TIME;
>> + data->REG_TOLERANCE_H = NCT6116_REG_TOLERANCE_H;
>> + data->REG_PWM[0] = NCT6116_REG_PWM;
>> + data->REG_PWM[1] = NCT6116_REG_FAN_START_OUTPUT;
>> + data->REG_PWM[2] = NCT6116_REG_FAN_STOP_OUTPUT;
>> + data->REG_PWM[5] = NCT6116_REG_WEIGHT_DUTY_STEP;
>> + data->REG_PWM[6] = NCT6116_REG_WEIGHT_DUTY_BASE;
>> + data->REG_PWM_READ = NCT6116_REG_PWM_READ;
>> + data->REG_PWM_MODE = NCT6116_REG_PWM_MODE;
>> + data->PWM_MODE_MASK = NCT6116_PWM_MODE_MASK;
>> + data->REG_AUTO_TEMP = NCT6116_REG_AUTO_TEMP;
>> + data->REG_AUTO_PWM = NCT6116_REG_AUTO_PWM;
>> + data->REG_CRITICAL_TEMP = NCT6116_REG_CRITICAL_TEMP;
>> + data->REG_CRITICAL_TEMP_TOLERANCE
>> + = NCT6116_REG_CRITICAL_TEMP_TOLERANCE;
>> + data->REG_CRITICAL_PWM_ENABLE = NCT6116_REG_CRITICAL_PWM_ENABLE;
>> + data->CRITICAL_PWM_ENABLE_MASK
>> + = NCT6116_CRITICAL_PWM_ENABLE_MASK;
>> + data->REG_CRITICAL_PWM = NCT6116_REG_CRITICAL_PWM;
>> + data->REG_TEMP_OFFSET = NCT6116_REG_TEMP_OFFSET;
>> + data->REG_TEMP_SOURCE = NCT6116_REG_TEMP_SOURCE;
>> + data->REG_TEMP_SEL = NCT6116_REG_TEMP_SEL;
>> + data->REG_WEIGHT_TEMP_SEL = NCT6116_REG_WEIGHT_TEMP_SEL;
>> + data->REG_WEIGHT_TEMP[0] = NCT6116_REG_WEIGHT_TEMP_STEP;
>> + data->REG_WEIGHT_TEMP[1] = NCT6116_REG_WEIGHT_TEMP_STEP_TOL;
>> + data->REG_WEIGHT_TEMP[2] = NCT6116_REG_WEIGHT_TEMP_BASE;
>> + data->REG_ALARM = NCT6116_REG_ALARM;
>> + data->ALARM_BITS = NCT6116_ALARM_BITS;
>> + data->REG_BEEP = NCT6116_REG_BEEP;
>> + data->BEEP_BITS = NCT6116_BEEP_BITS;
>> +
>> + reg_temp = NCT6116_REG_TEMP;
>> + reg_temp_mon = NCT6116_REG_TEMP_MON;
>> + num_reg_temp = ARRAY_SIZE(NCT6116_REG_TEMP);
>> + num_reg_temp_mon = ARRAY_SIZE(NCT6116_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 = NCT6116_REG_TEMP_ALTERNATE;
>> + reg_temp_crit = NCT6116_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;
>> @@ -4366,6 +4563,7 @@
>> data->have_vid = (cr2a & 0x60) == 0x40;
>> break;
>> case nct6106:
>> + case nct6116:
>> case nct6779:
>> case nct6791:
>> case nct6792:
>> @@ -4397,6 +4595,9 @@
>> case nct6106:
>> tmp |= 0xe0;
>> break;
>> + case nct6116:
>> + tmp |= 0xe0;
>> + break;
>> case nct6775:
>> tmp |= 0x1e;
>> break;
>> @@ -4590,6 +4791,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;
