hwmon: (aspeed-pwm-tacho) reduce fan_tach period
Subsystems (2)
PATCH v3 1/2 hwmon: (aspeed-pwm-tacho) reduce fan_tach period
Reduce the fan_tach period such that the fan controller uses a shorter period to measure the rpm.
Testing: Tested on an ast2400 sitting on a quanta-q71l.
---
v3: Added missing change log
v2: Updated commit message language
--- drivers/hwmon/aspeed-pwm-tacho.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/hwmon/aspeed-pwm-tacho.c +1 -1
@@ -160,7 +160,7 @@* 11: reserved. */ #define M_TACH_MODE 0x02/* 10b */ -#define M_TACH_UNIT 0x1000 +#define M_TACH_UNIT 0x00c0#define INIT_FAN_CTRL 0xFFstruct aspeed_pwm_tacho_data {
Signature · 1 line
2.13.1.611.g7e3b11ae1-goog
Guenter Roeck
maintainerQuoted 3 lines · Patrick Venture
Reduce the
fan_tachperiod such that the fan controller uses a shorter period to measure the rpm.
This explains what you are doing, but not why. What is the problem you are trying to solve, and why doesn't it create other problems ? Presumably there was a reason for the larger period used earlier. If not, if it was just a conservative setting, here is the place to say it.
I'd update the information myself, but I don't think the underlying specification is public, or at least I did not find it.
Guenter
Quoted 24 lines
Testing: Tested on an ast2400 sitting on a quanta-q71l.
Signed-off-by Patrick Venture <[email protected]>---
v3: Added missing change log
v2: Updated commit message language--- drivers/hwmon/aspeed-pwm-tacho.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/hwmon/aspeed-pwm-tacho.c +1
-1@@ -160,7 +160,7 @@* 11: reserved. */ #define M_TACH_MODE 0x02/* 10b */ -#define M_TACH_UNIT 0x1000 +#define M_TACH_UNIT 0x00c0#define INIT_FAN_CTRL 0xFFstruct aspeed_pwm_tacho_data {
Joel Stanley
maintainerQuoted 7 lines · Guenter Roeck
Quoted 4 lines · Patrick Venture
Reduce the
fan_tachperiod such that the fan controller uses a shorter period to measure the rpm.
Quoted 8 lines
This explains what you are doing, but not why. What is the problem you are trying to solve, and why doesn't it create other problems ? Presumably there was a reason for the larger period used earlier. If not, if it was just a conservative setting, here is the place to say it.
I'd update the information myself, but I don't think the underlying specification
is public, or at least I did not find it.
The datasheet for the Aspeed SoC is not public. Patrick and I can help answer questions, and I've added Ryan from Aspeed who can answer questions in more detail.
As I understand it this setting is a trade off between giving the tach unit enough time to measure complete fan rotations under all conditions (ie, when the fans are going slow) and getting a timely measurement. With the driver as-is we have a large delay between readings, which makes writing a control loop impossible.
I'm not convinced that this driver had enough testing before it was merged. I agree that Patrick should provide reasoning for his changes, but I also encourage his efforts as he has spent time looking at the hardware in detail.
Cheers,
Joel
Quoted 29 lines
Guenter
Quoted 24 lines
Testing: Tested on an ast2400 sitting on a quanta-q71l.
Signed-off-by Patrick Venture <[email protected]>---
v3: Added missing change log
v2: Updated commit message language--- drivers/hwmon/aspeed-pwm-tacho.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/hwmon/aspeed-pwm-tacho.c +1
-1@@ -160,7 +160,7 @@* 11: reserved. */ #define M_TACH_MODE 0x02/* 10b */ -#define M_TACH_UNIT 0x1000 +#define M_TACH_UNIT 0x00c0#define INIT_FAN_CTRL 0xFFstruct aspeed_pwm_tacho_data {
Joel Stanley
maintainerQuoted 2 lines · Joel Stanley
I also encourage his efforts as he has spent time looking at the hardware in detail.
And I see that there was a v4 that was merged. Thanks!
Joel
Guenter Roeck
maintainerQuoted 21 lines · Joel Stanley
Quoted 7 lines · Guenter Roeck
Quoted 4 lines · Patrick Venture
Reduce the
fan_tachperiod such that the fan controller uses a shorter period to measure the rpm.Quoted 8 lines
This explains what you are doing, but not why. What is the problem you are trying to solve, and why doesn't it create other problems ? Presumably there was a reason for the larger period used earlier. If not, if it was just a conservative setting, here is the place to say it.
I'd update the information myself, but I don't think the underlying specification
is public, or at least I did not find it.The datasheet for the Aspeed SoC is not public. Patrick and I can help answer questions, and I've added Ryan from Aspeed who can answer questions in more detail.
Quoted 8 lines
As I understand it this setting is a trade off between giving the tach unit enough time to measure complete fan rotations under all conditions (ie, when the fans are going slow) and getting a timely measurement. With the driver as-is we have a large delay between readings, which makes writing a control loop impossible.
I'm not convinced that this driver had enough testing before it was merged. I agree that Patrick should provide reasoning for his changes,
The driver or the latest set of patches ?
Everyone is encouraged to review patches. If there are concerns with a driver, those should be raised during the review process. I am not really the fastest reacting maintainer nowadays, so I would think there should have been enough time for such feedback. In case concerns were raised and I missed it, my apologies.
Thanks,
Guenter
Quoted 37 lines
but I also encourage his efforts as he has spent time looking at the hardware in detail.
Cheers,
Joel
Quoted 29 lines
Guenter
Quoted 24 lines
Testing: Tested on an ast2400 sitting on a quanta-q71l.
Signed-off-by Patrick Venture <[email protected]>---
v3: Added missing change log
v2: Updated commit message language--- drivers/hwmon/aspeed-pwm-tacho.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/hwmon/aspeed-pwm-tacho.c +1
-1@@ -160,7 +160,7 @@* 11: reserved. */ #define M_TACH_MODE 0x02/* 10b */ -#define M_TACH_UNIT 0x1000 +#define M_TACH_UNIT 0x00c0#define INIT_FAN_CTRL 0xFFstruct aspeed_pwm_tacho_data {
Joel Stanley
maintainerQuoted 41 lines · Guenter Roeck
Quoted 28 lines · Joel Stanley
Quoted 16 lines · Guenter Roeck
Quoted 4 lines · Patrick Venture
Reduce the
fan_tachperiod such that the fan controller uses a shorter period to measure the rpm.This explains what you are doing, but not why. What is the problem you are
trying to solve, and why doesn't it create other problems ? Presumably there
was a reason for the larger period used earlier. If not, if it was just a conservative setting, here is the place to say it.Quoted 3 lines
I'd update the information myself, but I don't think the underlying specification
is public, or at least I did not find it.The datasheet for the Aspeed SoC is not public. Patrick and I can help answer questions, and I've added Ryan from Aspeed who can answer questions in more detail.
Quoted 8 lines
As I understand it this setting is a trade off between giving the tach unit enough time to measure complete fan rotations under all conditions (ie, when the fans are going slow) and getting a timely measurement. With the driver as-is we have a large delay between readings, which makes writing a control loop impossible.
I'm not convinced that this driver had enough testing before it was merged. I agree that Patrick should provide reasoning for his changes,
The driver or the latest set of patches ?
Quoted 7 lines
Everyone is encouraged to review patches. If there are concerns with a driver,
those should be raised during the review process. I am not really the fastest
reacting maintainer nowadays, so I would think there should have been enough time for such feedback. In case concerns were raised and I missed it, my apologies.
The driver. I was too slow to voice my objections, so you were fine on your part.
Cheers,
Joel