| Message ID | 20170624033946.120501-1-venture@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 06/23/2017 08:39 PM, Patrick Venture wrote: > Reduce the fan_tach period 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 > Testing: Tested on an ast2400 sitting on a quanta-q71l. > > Signed-off-by: Patrick Venture <venture@google.com> > --- > v3: Added missing change log > v2: Updated commit message language > --- > drivers/hwmon/aspeed-pwm-tacho.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c > index 86e2ea8287a7..b2ab5612d8a4 100644 > --- a/drivers/hwmon/aspeed-pwm-tacho.c > +++ b/drivers/hwmon/aspeed-pwm-tacho.c > @@ -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 0xFF > > struct aspeed_pwm_tacho_data { > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jun 24, 2017 at 11:23 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On 06/23/2017 08:39 PM, Patrick Venture wrote: >> >> Reduce the fan_tach period 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. 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 > > Guenter > > >> Testing: Tested on an ast2400 sitting on a quanta-q71l. >> >> Signed-off-by: Patrick Venture <venture@google.com> >> --- >> v3: Added missing change log >> v2: Updated commit message language >> --- >> drivers/hwmon/aspeed-pwm-tacho.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c >> b/drivers/hwmon/aspeed-pwm-tacho.c >> index 86e2ea8287a7..b2ab5612d8a4 100644 >> --- a/drivers/hwmon/aspeed-pwm-tacho.c >> +++ b/drivers/hwmon/aspeed-pwm-tacho.c >> @@ -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 0xFF >> struct aspeed_pwm_tacho_data { >> > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 26, 2017 at 10:38 PM, Joel Stanley <joel@jms.id.au> wrote: > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/26/2017 06:08 AM, Joel Stanley wrote: > On Sat, Jun 24, 2017 at 11:23 PM, Guenter Roeck <linux@roeck-us.net> wrote: >> On 06/23/2017 08:39 PM, Patrick Venture wrote: > >>> >>> Reduce the fan_tach period 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. > > 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, 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 > but I also encourage his efforts as he has spent time looking at the > hardware in detail. > > Cheers, > > Joel > >> >> Guenter >> >> >>> Testing: Tested on an ast2400 sitting on a quanta-q71l. >>> >>> Signed-off-by: Patrick Venture <venture@google.com> >>> --- >>> v3: Added missing change log >>> v2: Updated commit message language >>> --- >>> drivers/hwmon/aspeed-pwm-tacho.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c >>> b/drivers/hwmon/aspeed-pwm-tacho.c >>> index 86e2ea8287a7..b2ab5612d8a4 100644 >>> --- a/drivers/hwmon/aspeed-pwm-tacho.c >>> +++ b/drivers/hwmon/aspeed-pwm-tacho.c >>> @@ -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 0xFF >>> struct aspeed_pwm_tacho_data { >>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 26, 2017 at 11:09 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On 06/26/2017 06:08 AM, Joel Stanley wrote: >> >> On Sat, Jun 24, 2017 at 11:23 PM, Guenter Roeck <linux@roeck-us.net> >> wrote: >>> >>> On 06/23/2017 08:39 PM, Patrick Venture wrote: >> >> >>>> >>>> Reduce the fan_tach period 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. >> >> >> 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, > > > 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. The driver. I was too slow to voice my objections, so you were fine on your part. Cheers, Joel -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c index 86e2ea8287a7..b2ab5612d8a4 100644 --- a/drivers/hwmon/aspeed-pwm-tacho.c +++ b/drivers/hwmon/aspeed-pwm-tacho.c @@ -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 0xFF struct aspeed_pwm_tacho_data {
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. Signed-off-by: Patrick Venture <venture@google.com> --- v3: Added missing change log v2: Updated commit message language --- drivers/hwmon/aspeed-pwm-tacho.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)