From: Guenter Roeck <linux@roeck-us.net>
To: Billy Tsai <billy_tsai@aspeedtech.com>
Cc: jdelvare@suse.com, joel@jms.id.au, andrew@aj.id.au,
linux-hwmon@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org,
BMC-SW@aspeedtech.com
Subject: Re: [PATCH] hwmon: (aspeed-pwm-tacho) Using falling edge.
Date: Thu, 24 Jun 2021 05:44:28 -0700 [thread overview]
Message-ID: <20210624124428.GB1670703@roeck-us.net> (raw)
In-Reply-To: <20210624035821.25375-1-billy_tsai@aspeedtech.com>
On Thu, Jun 24, 2021 at 11:58:21AM +0800, Billy Tsai wrote:
> The tach shouldn't use both edges to measure. When the tach input
> duty cycle isn't 50% the return value will inaccurate.
>
A tachometer doesn't have a duty cycle. A pwm has a duty cycle, but that
is completely independent of the pwm duty cycle used to set the fan speed.
So this patch does not really make sense with the above explanation.
The impact of this patch is likely that the reported fan speed is reduced
by 50%. It may well be that the driver currently reports twice the real fan
speed. I have no idea if that is the case, but if it is it should not be
conditional. The description above states "when the tach input cycle isn't
50%", suggesting that this is conditional on some other configuration.
I don't know what that might be either.
So, sorry, I can't accept this patch without a more detailed and accurate
description and explanation why it is needed.
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.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 3d8239fd66ed..0a70a0e22acf 100644
> --- a/drivers/hwmon/aspeed-pwm-tacho.c
> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
> @@ -158,7 +158,7 @@
> * 10: both
> * 11: reserved.
> */
> -#define M_TACH_MODE 0x02 /* 10b */
> +#define M_TACH_MODE 0x00 /* 10b */
That comment is now wrong.
Guenter
> #define M_TACH_UNIT 0x0210
> #define INIT_FAN_CTRL 0xFF
>
> --
> 2.25.1
>
next prev parent reply other threads:[~2021-06-24 12:44 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-24 3:58 [PATCH] hwmon: (aspeed-pwm-tacho) Using falling edge Billy Tsai
2021-06-24 12:44 ` Guenter Roeck [this message]
2021-06-25 3:35 ` Billy Tsai
2021-06-28 13:56 ` Guenter Roeck
2021-06-29 5:26 ` Billy Tsai
2021-06-29 13:14 ` Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210624124428.GB1670703@roeck-us.net \
--to=linux@roeck-us.net \
--cc=BMC-SW@aspeedtech.com \
--cc=andrew@aj.id.au \
--cc=billy_tsai@aspeedtech.com \
--cc=jdelvare@suse.com \
--cc=joel@jms.id.au \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-aspeed@lists.ozlabs.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).