linux-kernel - Re: [v2 2/2] pwm: Add Aspeed ast2600 PWM 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]

Date:   Mon, 3 May 2021 08:10:59 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Billy Tsai <billy_tsai@...eedtech.com>
Cc:     "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        "andrew@...id.au" <andrew@...id.au>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "thierry.reding@...il.com" <thierry.reding@...il.com>,
        "joel@....id.au" <joel@....id.au>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        BMC-SW <BMC-SW@...eedtech.com>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [v2 2/2] pwm: Add Aspeed ast2600 PWM support

Hello,

On Mon, May 03, 2021 at 04:42:59AM +0000, Billy Tsai wrote:
> On 2021/4/27, 4:44 AM,Uwe Kleine-Königwrote:
>     >> +/* PWM fixed value */
>     >> +#define PWM_FIXED_PERIOD 0xff
>     >> +
>     >> +struct aspeed_pwm_data {
>     >> +	struct pwm_chip chip;
>     >> +	struct clk *clk;
>     >> +	struct regmap *regmap;
>     >> +	struct reset_control *reset;
>     >> +};
>     >> +
>     >> +static void aspeed_set_pwm_channel_enable(struct regmap *regmap, u8 pwm_channel,
>     >> +					  bool enable)
>     >> +{
>     >> +	regmap_update_bits(regmap, PWM_ASPEED_CTRL_CH(pwm_channel),
>     >> +			   (PWM_CLK_ENABLE | PWM_PIN_ENABLE),
>     >> +			   enable ? (PWM_CLK_ENABLE | PWM_PIN_ENABLE) : 0);
> 
>     > What is the semantic difference between CLK_ENABLE and PIN_ENABLE? Does
>     > the pin stay at it's inactive level if PIN_ENABLE is unset? Does the
>     > output just freeze at it's current level if CLK_ENABLE is unset?
> 
> Yes. 
> When PIN_ENABLE is unset the pwm controller will always output low to the extern.
> When CLK_ENABLE is unset the pwm controller will freeze at it's current level.

These two are relevant to mention at the top of the driver.

>     > The intended goal is to provide the biggest possible period not bigger
>     > than the requested value.
>     
> So, did you mean that if the request period is 100ns and our divide can reach 100.1ns or 95ns
> the user prefer 95ns to 100.1ns?

Yes. It's unclear if the user really prefers 95ns, but to get a
consistent behaviour among the different drivers, that's what I ask you
to implement.

Currently there is no way that allows a consumer to specify which
setting they prefer, I have an idea for a fix though. For that it is
also important that .apply() doesn't yield 100.1 ns.

>     >> +	dev_dbg(dev, "freq: %d, duty_pt: %d", freq, duty_pt);
>     >> +	if (state->enabled) {
>     >> +		aspeed_set_pwm_freq(priv, pwm, freq);
>     >> +		aspeed_set_pwm_duty(priv, pwm, duty_pt);
>     >> +		aspeed_set_pwm_polarity(priv, pwm, state->polarity);
> 
>     > How does the hardware behave in between these calls? If for example the
>     > polarity is changed, does this affect the output immediately? Does this
>     > start a new period?
> 
> The pwm output will inverse immediately. The period will not change.

Please mention that at the top of the driver.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)