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/ |