| Message ID | 20170911224155.146738-1-venture@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Hi Patrick, On Tue, Sep 12, 2017 at 8:41 AM, Patrick Venture <venture@google.com> wrote: > The previous value reduced the time required to determine > the fan value, however, it's also used as the final timeout > mechanism. The prevous value would work for any fan speed > greater than around 3k RPM. This increased value, lets the fan > speeds return quickly but will wait longer to handle speeds below 3k > RPM. Given you've had to tweak this value a few times, should we instead make it a device tree property? We could still use the result of your findings to provide a sensible default, but machines that have specific fan configurations could use the property to tweak the driver. Cheers, Joel > > Testing: this value was determined through experimentation on the ast2400 > on the Quanta-q71l. This configurations runs 8 fans attached to the > controller. > > Signed-off-by: Patrick Venture <venture@google.com> > --- > 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 69b97d45e3cb..f914e5f41048 100644 > --- a/drivers/hwmon/aspeed-pwm-tacho.c > +++ b/drivers/hwmon/aspeed-pwm-tacho.c > @@ -161,7 +161,7 @@ > * 11: reserved. > */ > #define M_TACH_MODE 0x02 /* 10b */ > -#define M_TACH_UNIT 0x00c0 > +#define M_TACH_UNIT 0x0210 > #define INIT_FAN_CTRL 0xFF > > /* How long we sleep in us while waiting for an RPM result. */ > -- > 2.14.1.581.gf28d330327-goog > -- 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, Sep 11, 2017 at 06:41:56PM -0700, Patrick Venture wrote: > On Mon, Sep 11, 2017 at 4:56 PM, Joel Stanley <joel@jms.id.au> wrote: > > > Hi Patrick, > > > > On Tue, Sep 12, 2017 at 8:41 AM, Patrick Venture <venture@google.com> > > wrote: > > > The previous value reduced the time required to determine > > > the fan value, however, it's also used as the final timeout > > > mechanism. The prevous value would work for any fan speed > > > greater than around 3k RPM. This increased value, lets the fan > > > speeds return quickly but will wait longer to handle speeds below 3k > > > RPM. > > > > Given you've had to tweak this value a few times, should we instead > > make it a device tree property? > > > > We could still use the result of your findings to provide a sensible > > default, but machines that have specific fan configurations could use > > the property to tweak the driver. > > > > I had a todo item in my last commit for this field to do just that -- make > it a variable. I'm perfectly willing to head down this path. I found that > my previous value was too aggressive for those slow speeds for the > timeout. This path addresses that. I think though this new value, does > make for a good default value as you've commented. I have some cycles > coming up to move this value into something that can be set in a device > tree. > You'll have tto find a means to explain that this is not a configuration parameter but a system property. Either case, I am hesitant to update this again. This seems to be an endless exercise. How do I know that this won't go on forever ? Guenter > > > > Cheers, > > > > Joel > > > > > > > > Testing: this value was determined through experimentation on the ast2400 > > > on the Quanta-q71l. This configurations runs 8 fans attached to the > > > controller. > > > > > > Signed-off-by: Patrick Venture <venture@google.com> > > > --- > > > 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 69b97d45e3cb..f914e5f41048 100644 > > > --- a/drivers/hwmon/aspeed-pwm-tacho.c > > > +++ b/drivers/hwmon/aspeed-pwm-tacho.c > > > @@ -161,7 +161,7 @@ > > > * 11: reserved. > > > */ > > > #define M_TACH_MODE 0x02 /* 10b */ > > > -#define M_TACH_UNIT 0x00c0 > > > +#define M_TACH_UNIT 0x0210 > > > #define INIT_FAN_CTRL 0xFF > > > > > > /* How long we sleep in us while waiting for an RPM result. */ > > > -- > > > 2.14.1.581.gf28d330327-goog > > > > > -- 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, Sep 11, 2017 at 03:41:55PM -0700, Patrick Venture wrote: > The previous value reduced the time required to determine > the fan value, however, it's also used as the final timeout > mechanism. The prevous value would work for any fan speed > greater than around 3k RPM. This increased value, lets the fan > speeds return quickly but will wait longer to handle speeds below 3k > RPM. > > Testing: this value was determined through experimentation on the ast2400 > on the Quanta-q71l. This configurations runs 8 fans attached to the > controller. > > Signed-off-by: Patrick Venture <venture@google.com> With reservations: Reviewed-by: Guenter Roeck <linux@roeck-us.net> This is the last time for me to accept a patch changing the value of M_TACH_UNIT in this driver. Any subsequent change will require a more comprehensive solution. Guenter > --- > 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 69b97d45e3cb..f914e5f41048 100644 > --- a/drivers/hwmon/aspeed-pwm-tacho.c > +++ b/drivers/hwmon/aspeed-pwm-tacho.c > @@ -161,7 +161,7 @@ > * 11: reserved. > */ > #define M_TACH_MODE 0x02 /* 10b */ > -#define M_TACH_UNIT 0x00c0 > +#define M_TACH_UNIT 0x0210 > #define INIT_FAN_CTRL 0xFF > > /* How long we sleep in us while waiting for an RPM result. */ > -- > 2.14.1.581.gf28d330327-goog > > -- > 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 -- 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 69b97d45e3cb..f914e5f41048 100644 --- a/drivers/hwmon/aspeed-pwm-tacho.c +++ b/drivers/hwmon/aspeed-pwm-tacho.c @@ -161,7 +161,7 @@ * 11: reserved. */ #define M_TACH_MODE 0x02 /* 10b */ -#define M_TACH_UNIT 0x00c0 +#define M_TACH_UNIT 0x0210 #define INIT_FAN_CTRL 0xFF /* How long we sleep in us while waiting for an RPM result. */
The previous value reduced the time required to determine the fan value, however, it's also used as the final timeout mechanism. The prevous value would work for any fan speed greater than around 3k RPM. This increased value, lets the fan speeds return quickly but will wait longer to handle speeds below 3k RPM. Testing: this value was determined through experimentation on the ast2400 on the Quanta-q71l. This configurations runs 8 fans attached to the controller. Signed-off-by: Patrick Venture <venture@google.com> --- drivers/hwmon/aspeed-pwm-tacho.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)