linux-kernel - Re: [v2 3/3] hwmon: Add Aspeed ast2600 TACH support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <271C521D-8F20-4C86-B3DA-9C0AD74242D4@aspeedtech.com>
Date:   Wed, 2 Nov 2022 06:54:43 +0000
From:   Billy Tsai <billy_tsai@...eedtech.com>
To:     Guenter Roeck <linux@...ck-us.net>
CC:     "jdelvare@...e.com" <jdelvare@...e.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "joel@....id.au" <joel@....id.au>,
        "andrew@...id.au" <andrew@...id.au>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        "thierry.reding@...il.com" <thierry.reding@...il.com>,
        "u.kleine-koenig@...gutronix.de" <u.kleine-koenig@...gutronix.de>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        BMC-SW <BMC-SW@...eedtech.com>,
        "garnermic@...a.com" <garnermic@...a.com>
Subject: Re: [v2 3/3] hwmon: Add Aspeed ast2600 TACH support

Hi Guenter,

On 2022/11/1, 9:15 PM, "Guenter Roeck" <groeck7@...il.com on behalf of linux@...ck-us.net> wrote:

    On Tue, Nov 01, 2022 at 05:51:56PM +0800, Billy Tsai wrote:
    > > +
    > > +	/* Restart the Tach channel to guarantee the value is fresh */
    > > +	aspeed_tach_ch_enable(priv, fan_tach_ch, false);
    > > +	aspeed_tach_ch_enable(priv, fan_tach_ch, true);

    > Is that really needed ? Doesn't the controller measure values continuously ?

Yes, the controller will measure values continuously by hardware. I will remove it. 
If the user want to get the fresh value, it should be done by the application layer
(e.g. read two times).

    > > +
    > > +	if (ret) {
    > > +		/* return 0 if we didn't get an answer because of timeout*/
    > > +		if (ret == -ETIMEDOUT)
    > > +			return 0;
    > > +		else
    > > +			return ret;

    > else after return is unnecessary, and why would a timeout be be ignored ?

When the user sets the correct fan information (i.e., min_rpm, max_rpm), the read
poll timeout will only occur if the tach pin does not get any signal (i.e. rpm=0).

    > > +	}
    > > +
    > > +	raw_data = val & TACH_ASPEED_VALUE_MASK;
    > > +	/*
    > > +	 * We need the mode to determine if the raw_data is double (from
    > > +	 * counting both edges).
    > > +	 */
    > > +	if (priv->tach_channel[fan_tach_ch].tach_edge == BOTH_EDGES)
    > > +		raw_data <<= 1;
    > > +
    > > +	tach_div = raw_data * (priv->tach_channel[fan_tach_ch].divisor) *
    > > +		   (priv->tach_channel[fan_tach_ch].pulse_pr);
    > > +
    > > +	clk_source = clk_get_rate(priv->clk);
    > > +	dev_dbg(priv->dev, "clk %ld, raw_data %d , tach_div %d\n", clk_source,
    > > +		raw_data, tach_div);
    > > +
    > > +	if (tach_div == 0)
    > > +		return -EDOM;

    > If the fan is off or not connected, would that return an error ?
    > If so, that would be inappropriate; it should return a speed
    > of 0 in that case.

It will be handled by the regmap_read_poll_timeout.

Thanks

Best Regards,
Billy Tsai