| Message ID | 20230715153113.1307220-1-ahmad@khalifa.ws (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (nct6775) Add support for 18 IN readings for nct6799 | expand |
On 7/15/23 08:31, Ahmad Khalifa wrote: > nct6799 supports 18 voltage readings where this driver stops at 16. > You are adding three sets of registers, though, not just two. I think you meant to say that the driver stops at 15. > - Add additional VIN/IN_MIN/IN_MAX register values > - Update ALARM bits for the 16 supported values only as support > for IN > 16 requires rebasing all other ALARM bits for other > Turns out NCT6798 has officially 16 and inofficially 17 voltage inputs. > Signed-off-by: Ahmad Khalifa <ahmad@khalifa.ws> > --- > drivers/hwmon/nct6775-core.c | 33 +++++++++++++++++++++++++++++---- > drivers/hwmon/nct6775.h | 4 ++-- > 2 files changed, 31 insertions(+), 6 deletions(-) > > diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c > index 7163a2473fa0..1006765d8483 100644 > --- a/drivers/hwmon/nct6775-core.c > +++ b/drivers/hwmon/nct6775-core.c > @@ -82,14 +82,16 @@ static const char * const nct6775_device_names[] = { > > /* Common and NCT6775 specific data */ > > -/* Voltage min/max registers for nr=7..14 are in bank 5 */ > +/* Voltage min/max registers for nr=7..14 are in bank 5 > + * 15-17 for NCT6799 only > + */ Please use standard multi-line comments. This is not the networking subsystem. > > static const u16 NCT6775_REG_IN_MAX[] = { > 0x2b, 0x2d, 0x2f, 0x31, 0x33, 0x35, 0x37, 0x554, 0x556, 0x558, 0x55a, > - 0x55c, 0x55e, 0x560, 0x562 }; > + 0x55c, 0x55e, 0x560, 0x562, 0x564, 0x570, 0x572 }; > static const u16 NCT6775_REG_IN_MIN[] = { > 0x2c, 0x2e, 0x30, 0x32, 0x34, 0x36, 0x38, 0x555, 0x557, 0x559, 0x55b, > - 0x55d, 0x55f, 0x561, 0x563 }; > + 0x55d, 0x55f, 0x561, 0x563, 0x565, 0x571, 0x573 }; > static const u16 NCT6775_REG_IN[] = { > 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x550, 0x551, 0x552 > }; > @@ -340,9 +342,15 @@ static const u16 NCT6776_REG_TSI_TEMP[] = { > > /* NCT6779 specific data */ > > +/* 15-17 for NCT6799 only, register labels are: > + * CPUVC, VIN1, AVSB, 3VCC, VIN0, VIN8, VIN4, 3VSB > + * VBAT, VTT, VIN5, VIN6, VIN2, VIN3, VIN7, VIN9 > + * VHIF, VIN10 > + */ > static const u16 NCT6779_REG_IN[] = { > 0x480, 0x481, 0x482, 0x483, 0x484, 0x485, 0x486, 0x487, > - 0x488, 0x489, 0x48a, 0x48b, 0x48c, 0x48d, 0x48e }; > + 0x488, 0x489, 0x48a, 0x48b, 0x48c, 0x48d, 0x48e, 0x48f, > + 0x470, 0x471}; > > static const u16 NCT6779_REG_ALARM[NUM_REG_ALARM] = { > 0x459, 0x45A, 0x45B, 0x568 }; > @@ -697,6 +705,21 @@ static const char *const nct6799_temp_label[] = { > #define NCT6799_TEMP_MASK 0xbfff2ffe > #define NCT6799_VIRT_TEMP_MASK 0x80000c00 > > +/* NCT6799 layout of alarm bits is indexed by the REG_VIN > + * order, which is > + * CPUVC, VIN1, AVSB, 3VCC, VIN0, VIN8, VIN4, 3VSB > + * VBAT, VTT, VIN5, VIN6, VIN2, VIN3, VIN7, VIN9 > + * no space for 16-17: VHIF, VIN10 (bits 31, -1) Why not use bit 31 ? > + */ > +static const s8 NCT6799_ALARM_BITS[] = { > + 0, 1, 2, 3, 8, -1, 20, 16, /* in0.. in7 */ > + 17, 24, 25, 26, 27, 28, 29, /* in8..in14 */ > + 30, /* in15 */ > + 6, 7, 11, 10, 23, 33, /* fan1..fan6 */ > + -1, -1, /* unused */ > + 4, 5, 13, -1, -1, -1, /* temp1..temp6 */ > + 12, 9 }; /* intrusion0, intrusion1 */ > + > /* NCT6102D/NCT6106D specific data */ > > #define NCT6106_REG_VBAT 0x318 > @@ -3972,6 +3995,8 @@ int nct6775_probe(struct device *dev, struct nct6775_data *data, > case nct6797: > case nct6798: > case nct6799: > + data->in_num = 18; > + data->ALARM_BITS = NCT6799_ALARM_BITS; > data->REG_TSI_TEMP = NCT6796_REG_TSI_TEMP; > num_reg_tsi_temp = ARRAY_SIZE(NCT6796_REG_TSI_TEMP); > break; > diff --git a/drivers/hwmon/nct6775.h b/drivers/hwmon/nct6775.h > index 44f79c5726a9..0607db9d1415 100644 > --- a/drivers/hwmon/nct6775.h > +++ b/drivers/hwmon/nct6775.h > @@ -97,7 +97,7 @@ struct nct6775_data { > /* Register values */ > u8 bank; /* current register bank */ > u8 in_num; /* number of in inputs we have */ > - u8 in[15][3]; /* [0]=in, [1]=in_max, [2]=in_min */ > + u8 in[18][3]; /* [0]=in, [1]=in_max, [2]=in_min */ > unsigned int rpm[NUM_FAN]; > u16 fan_min[NUM_FAN]; > u8 fan_pulses[NUM_FAN]; > @@ -165,7 +165,7 @@ struct nct6775_data { > u16 have_temp; > u16 have_temp_fixed; > u16 have_tsi_temp; > - u16 have_in; > + u32 have_in; /* upgrade to 32 as 6799 needs 18 */ Unnecessary comment > > /* Remember extra register values over suspend/resume */ > u8 vbat; > > base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5 > prerequisite-patch-id: 36e3467bd9ea72cb3ad2bef638a8389a9537d111 > prerequisite-patch-id: 85db508f68cabb50472c0cc5ef3953fc46bee3b1
On 15/07/2023 17:48, Guenter Roeck wrote: > On 7/15/23 08:31, Ahmad Khalifa wrote: >> nct6799 supports 18 voltage readings where this driver stops at 16. > You are adding three sets of registers, though, not just two. I think > you meant to say that the driver stops at 15. Yes, currently 15 IN defined. It was influenced by the ALARM bits comment. I'll change it. >> +/* NCT6799 layout of alarm bits is indexed by the REG_VIN >> + * order, which is >> + * CPUVC, VIN1, AVSB, 3VCC, VIN0, VIN8, VIN4, 3VSB >> + * VBAT, VTT, VIN5, VIN6, VIN2, VIN3, VIN7, VIN9 >> + * no space for 16-17: VHIF, VIN10 (bits 31, -1) > > Why not use bit 31 ? Well, this is the part that made me say "driver supports up to 16". The ALARM bits have FAN_BASE starting at index 16, so the IN alarms can only take up 0-15, unless all alarm bits have extra padding added to push FAN_BASE/TEMP_BASE/INTRUSION_BASE up. I took the easy option here and left out the 16 IN alarm. Did I count this wrong? nct6775_fan_is_visible() `data->ALARM_BITS[FAN_ALARM_BASE + fan]`
On 7/15/23 10:03, Ahmad Khalifa wrote: > On 15/07/2023 17:48, Guenter Roeck wrote: >> On 7/15/23 08:31, Ahmad Khalifa wrote: >>> nct6799 supports 18 voltage readings where this driver stops at 16. >> You are adding three sets of registers, though, not just two. I think >> you meant to say that the driver stops at 15. > > Yes, currently 15 IN defined. It was influenced by the ALARM bits > comment. I'll change it. > >>> +/* NCT6799 layout of alarm bits is indexed by the REG_VIN >>> + * order, which is >>> + * CPUVC, VIN1, AVSB, 3VCC, VIN0, VIN8, VIN4, 3VSB >>> + * VBAT, VTT, VIN5, VIN6, VIN2, VIN3, VIN7, VIN9 >>> + * no space for 16-17: VHIF, VIN10 (bits 31, -1) >> >> Why not use bit 31 ? > > Well, this is the part that made me say "driver supports up to 16". > The ALARM bits have FAN_BASE starting at index 16, so the IN alarms > can only take up 0-15, unless all alarm bits have extra padding > added to push FAN_BASE/TEMP_BASE/INTRUSION_BASE up. > > I took the easy option here and left out the 16 IN alarm. > > Did I count this wrong? > nct6775_fan_is_visible() > `data->ALARM_BITS[FAN_ALARM_BASE + fan]` > > The 16 was just for convenience when setting the alarm base values, and to keep some space for voltages. That doesn't mean the driver as a whole supports 16 voltage inputs (or 8 fan control inputs, for that matter). We need to revisit the entire alarm handling at some point. nct6798 and nct6799 support 8 sets of temperature limits and alarms, and that isn't currently supported either. 'alarms' is already 64 bits wide, so it should be possible to shift the bits around and make space for more alarms (such as 24 voltages, 8 fans, and 8 or even 16 temperatures). Thanks, Guenter
On 15/07/2023 18:48, Guenter Roeck wrote: > On 7/15/23 10:03, Ahmad Khalifa wrote: >>> Why not use bit 31 ? >> >> Well, this is the part that made me say "driver supports up to 16". >> The ALARM bits have FAN_BASE starting at index 16, so the IN alarms >> can only take up 0-15, unless all alarm bits have extra padding >> added to push FAN_BASE/TEMP_BASE/INTRUSION_BASE up. >> >> I took the easy option here and left out the 16 IN alarm. >> >> Did I count this wrong? >> nct6775_fan_is_visible() >> `data->ALARM_BITS[FAN_ALARM_BASE + fan]` > > The 16 was just for convenience when setting the alarm base values, > and to keep some space for voltages. That doesn't mean the driver as > a whole supports 16 voltage inputs (or 8 fan control inputs, for > that matter). > > We need to revisit the entire alarm handling at some point. > nct6798 and nct6799 support 8 sets of temperature limits and > alarms, and that isn't currently supported either. 'alarms' > is already 64 bits wide, so it should be possible to shift > the bits around and make space for more alarms (such as 24 > voltages, 8 fans, and 8 or even 16 temperatures). Good point, my next patch for temps was going to have this comment: +/* 8 source readings available, but we keep to 6 to remain in bounds + * temp 7/8: 0x676, 0x678 + * src 7/8: 0xc2a, 0xc2b + */ Might as well expand the Alarms first then add all 18 IN and 8 TEMPs (And with comments not in the style of the networking subsystem) Should I update the ALARM bits first, then send in the the full IN and TEMP registers afterwards? Rather than add-expand-addmore
On 7/15/23 11:12, Ahmad Khalifa wrote: > On 15/07/2023 18:48, Guenter Roeck wrote: >> On 7/15/23 10:03, Ahmad Khalifa wrote: >>>> Why not use bit 31 ? >>> >>> Well, this is the part that made me say "driver supports up to 16". >>> The ALARM bits have FAN_BASE starting at index 16, so the IN alarms >>> can only take up 0-15, unless all alarm bits have extra padding >>> added to push FAN_BASE/TEMP_BASE/INTRUSION_BASE up. >>> >>> I took the easy option here and left out the 16 IN alarm. >>> >>> Did I count this wrong? >>> nct6775_fan_is_visible() >>> `data->ALARM_BITS[FAN_ALARM_BASE + fan]` >> >> The 16 was just for convenience when setting the alarm base values, >> and to keep some space for voltages. That doesn't mean the driver as >> a whole supports 16 voltage inputs (or 8 fan control inputs, for >> that matter). >> >> We need to revisit the entire alarm handling at some point. >> nct6798 and nct6799 support 8 sets of temperature limits and >> alarms, and that isn't currently supported either. 'alarms' >> is already 64 bits wide, so it should be possible to shift >> the bits around and make space for more alarms (such as 24 >> voltages, 8 fans, and 8 or even 16 temperatures). > > Good point, my next patch for temps was going to have this comment: > > +/* 8 source readings available, but we keep to 6 to remain in bounds > + * temp 7/8: 0x676, 0x678 > + * src 7/8: 0xc2a, 0xc2b > + */ > > Might as well expand the Alarms first then add all 18 IN and 8 TEMPs > (And with comments not in the style of the networking subsystem) > > Should I update the ALARM bits first, then send in the the full IN > and TEMP registers afterwards? Rather than add-expand-addmore > > Yes, I would prefer that. Do you have the datasheet for NCT6798D ? We should make sure that this chip is covered as well. Thanks, Guenter
On 15/07/2023 19:30, Guenter Roeck wrote: > On 7/15/23 11:12, Ahmad Khalifa wrote: >> On 15/07/2023 18:48, Guenter Roeck wrote: >>> On 7/15/23 10:03, Ahmad Khalifa wrote: >>>>> Why not use bit 31 ? >>>> >>>> Well, this is the part that made me say "driver supports up to 16". >>>> The ALARM bits have FAN_BASE starting at index 16, so the IN alarms >>>> can only take up 0-15, unless all alarm bits have extra padding >>>> added to push FAN_BASE/TEMP_BASE/INTRUSION_BASE up. >>>> >>>> I took the easy option here and left out the 16 IN alarm. >>>> >>>> Did I count this wrong? >>>> nct6775_fan_is_visible() >>>> `data->ALARM_BITS[FAN_ALARM_BASE + fan]` >>> >>> The 16 was just for convenience when setting the alarm base values, >>> and to keep some space for voltages. That doesn't mean the driver as >>> a whole supports 16 voltage inputs (or 8 fan control inputs, for >>> that matter). >>> >>> We need to revisit the entire alarm handling at some point. >>> nct6798 and nct6799 support 8 sets of temperature limits and >>> alarms, and that isn't currently supported either. 'alarms' >>> is already 64 bits wide, so it should be possible to shift >>> the bits around and make space for more alarms (such as 24 >>> voltages, 8 fans, and 8 or even 16 temperatures). >> >> Good point, my next patch for temps was going to have this comment: >> >> +/* 8 source readings available, but we keep to 6 to remain in bounds >> + * temp 7/8: 0x676, 0x678 >> + * src 7/8: 0xc2a, 0xc2b >> + */ >> >> Might as well expand the Alarms first then add all 18 IN and 8 TEMPs >> (And with comments not in the style of the networking subsystem) >> >> Should I update the ALARM bits first, then send in the the full IN >> and TEMP registers afterwards? Rather than add-expand-addmore >> >> > Yes, I would prefer that. Do you have the datasheet for NCT6798D ? > We should make sure that this chip is covered as well. No, I only have NCT6796D and NCT6796D-S. Please do share if possible.
diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c index 7163a2473fa0..1006765d8483 100644 --- a/drivers/hwmon/nct6775-core.c +++ b/drivers/hwmon/nct6775-core.c @@ -82,14 +82,16 @@ static const char * const nct6775_device_names[] = { /* Common and NCT6775 specific data */ -/* Voltage min/max registers for nr=7..14 are in bank 5 */ +/* Voltage min/max registers for nr=7..14 are in bank 5 + * 15-17 for NCT6799 only + */ static const u16 NCT6775_REG_IN_MAX[] = { 0x2b, 0x2d, 0x2f, 0x31, 0x33, 0x35, 0x37, 0x554, 0x556, 0x558, 0x55a, - 0x55c, 0x55e, 0x560, 0x562 }; + 0x55c, 0x55e, 0x560, 0x562, 0x564, 0x570, 0x572 }; static const u16 NCT6775_REG_IN_MIN[] = { 0x2c, 0x2e, 0x30, 0x32, 0x34, 0x36, 0x38, 0x555, 0x557, 0x559, 0x55b, - 0x55d, 0x55f, 0x561, 0x563 }; + 0x55d, 0x55f, 0x561, 0x563, 0x565, 0x571, 0x573 }; static const u16 NCT6775_REG_IN[] = { 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x550, 0x551, 0x552 }; @@ -340,9 +342,15 @@ static const u16 NCT6776_REG_TSI_TEMP[] = { /* NCT6779 specific data */ +/* 15-17 for NCT6799 only, register labels are: + * CPUVC, VIN1, AVSB, 3VCC, VIN0, VIN8, VIN4, 3VSB + * VBAT, VTT, VIN5, VIN6, VIN2, VIN3, VIN7, VIN9 + * VHIF, VIN10 + */ static const u16 NCT6779_REG_IN[] = { 0x480, 0x481, 0x482, 0x483, 0x484, 0x485, 0x486, 0x487, - 0x488, 0x489, 0x48a, 0x48b, 0x48c, 0x48d, 0x48e }; + 0x488, 0x489, 0x48a, 0x48b, 0x48c, 0x48d, 0x48e, 0x48f, + 0x470, 0x471}; static const u16 NCT6779_REG_ALARM[NUM_REG_ALARM] = { 0x459, 0x45A, 0x45B, 0x568 }; @@ -697,6 +705,21 @@ static const char *const nct6799_temp_label[] = { #define NCT6799_TEMP_MASK 0xbfff2ffe #define NCT6799_VIRT_TEMP_MASK 0x80000c00 +/* NCT6799 layout of alarm bits is indexed by the REG_VIN + * order, which is + * CPUVC, VIN1, AVSB, 3VCC, VIN0, VIN8, VIN4, 3VSB + * VBAT, VTT, VIN5, VIN6, VIN2, VIN3, VIN7, VIN9 + * no space for 16-17: VHIF, VIN10 (bits 31, -1) + */ +static const s8 NCT6799_ALARM_BITS[] = { + 0, 1, 2, 3, 8, -1, 20, 16, /* in0.. in7 */ + 17, 24, 25, 26, 27, 28, 29, /* in8..in14 */ + 30, /* in15 */ + 6, 7, 11, 10, 23, 33, /* fan1..fan6 */ + -1, -1, /* unused */ + 4, 5, 13, -1, -1, -1, /* temp1..temp6 */ + 12, 9 }; /* intrusion0, intrusion1 */ + /* NCT6102D/NCT6106D specific data */ #define NCT6106_REG_VBAT 0x318 @@ -3972,6 +3995,8 @@ int nct6775_probe(struct device *dev, struct nct6775_data *data, case nct6797: case nct6798: case nct6799: + data->in_num = 18; + data->ALARM_BITS = NCT6799_ALARM_BITS; data->REG_TSI_TEMP = NCT6796_REG_TSI_TEMP; num_reg_tsi_temp = ARRAY_SIZE(NCT6796_REG_TSI_TEMP); break; diff --git a/drivers/hwmon/nct6775.h b/drivers/hwmon/nct6775.h index 44f79c5726a9..0607db9d1415 100644 --- a/drivers/hwmon/nct6775.h +++ b/drivers/hwmon/nct6775.h @@ -97,7 +97,7 @@ struct nct6775_data { /* Register values */ u8 bank; /* current register bank */ u8 in_num; /* number of in inputs we have */ - u8 in[15][3]; /* [0]=in, [1]=in_max, [2]=in_min */ + u8 in[18][3]; /* [0]=in, [1]=in_max, [2]=in_min */ unsigned int rpm[NUM_FAN]; u16 fan_min[NUM_FAN]; u8 fan_pulses[NUM_FAN]; @@ -165,7 +165,7 @@ struct nct6775_data { u16 have_temp; u16 have_temp_fixed; u16 have_tsi_temp; - u16 have_in; + u32 have_in; /* upgrade to 32 as 6799 needs 18 */ /* Remember extra register values over suspend/resume */ u8 vbat;
nct6799 supports 18 voltage readings where this driver stops at 16. - Add additional VIN/IN_MIN/IN_MAX register values - Update ALARM bits for the 16 supported values only as support for IN > 16 requires rebasing all other ALARM bits for other Signed-off-by: Ahmad Khalifa <ahmad@khalifa.ws> --- drivers/hwmon/nct6775-core.c | 33 +++++++++++++++++++++++++++++---- drivers/hwmon/nct6775.h | 4 ++-- 2 files changed, 31 insertions(+), 6 deletions(-) base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5 prerequisite-patch-id: 36e3467bd9ea72cb3ad2bef638a8389a9537d111 prerequisite-patch-id: 85db508f68cabb50472c0cc5ef3953fc46bee3b1