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
