Hi,
On 2021/5/23, 12:07 AM,Uwe Kleine-Königwrote:
Hello,
On Tue, May 18, 2021 at 08:55:17AM +0800, Billy Tsai wrote:
> > +static u64 aspeed_pwm_get_period(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct aspeed_pwm_data *priv = aspeed_pwm_chip_to_data(chip);
> > + unsigned long rate;
> > + u32 index = pwm->hwpwm;
> > + u32 val;
> > + u64 period, div_h, div_l, clk_period;
> > +
> > + rate = clk_get_rate(priv->clk);
> > + regmap_read(priv->regmap, PWM_ASPEED_CTRL_CH(index), &val);
> > + div_h = FIELD_GET(PWM_ASPEED_CTRL_CLK_DIV_H, val);
> > + div_l = FIELD_GET(PWM_ASPEED_CTRL_CLK_DIV_L, val);
> > + regmap_read(priv->regmap, PWM_ASPEED_DUTY_CYCLE_CH(index), &val);
> > + clk_period = FIELD_GET(PWM_ASPEED_DUTY_CYCLE_PERIOD, val);
> > + period = (NSEC_PER_SEC * BIT(div_h) * (div_l + 1) * (clk_period + 1));
> The outer pair of parenthesis on the RHS isn't necessary. The maximal
> value that period can have here is:
> 1000000000 * 2**15 * 256 * 256
> This fits into an u64, but as all but the last factor are 32 bit values
> you might get an overflow here.
I don’t know in which case the value will overflow, when my parameter types are all u64.
Can you tell me what is "the last factor"?
> > +static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > +{
> > + struct device *dev = chip->dev;
> > + struct aspeed_pwm_data *priv = aspeed_pwm_chip_to_data(chip);
> > + u32 index = pwm->hwpwm;
> > + int ret;
> > +
> > + dev_dbg(dev, "apply period: %lldns, duty_cycle: %lldns", state->period,
> > + state->duty_cycle);
> > +
> > + regmap_update_bits(priv->regmap, PWM_ASPEED_CTRL_CH(index),
> > + PWM_ASPEED_CTRL_PIN_ENABLE,
> > + state->enabled ? PWM_ASPEED_CTRL_PIN_ENABLE : 0);
> > + /*
> > + * Fixed the period to the max value and rising point to 0
> > + * for high resolution and simplify frequency calculation.
> > + */
> > + regmap_update_bits(priv->regmap, PWM_ASPEED_DUTY_CYCLE_CH(index),
> > + (PWM_ASPEED_DUTY_CYCLE_PERIOD |
> > + PWM_ASPEED_DUTY_CYCLE_RISING_POINT),
> > + FIELD_PREP(PWM_ASPEED_DUTY_CYCLE_PERIOD,
> > + PWM_ASPEED_FIXED_PERIOD));
> > +
> > + ret = aspeed_pwm_set_period(chip, pwm, state);
> > + if (ret)
> > + return ret;
> > + aspeed_pwm_set_duty(chip, pwm, state);
> aspeed_pwm_set_duty calls aspeed_pwm_get_period() which is a bit
> ineffective after just having set the period.
When I call aspeed_pwm_set_period it doesn't mean the period is equal to what I set (It may
lose some precision Ex: When I set the period 40000ns, the actual period I set is 39680ns) and
I didn't get this information when I call aspeed_pwm_set_period. Thus, I need to get the actual
period first before set duty.
