Hello,
sorry, I still found a problem that I want to have addressed. I'll point
out a few minor things en passant.
But after that I really have a good feeling and like the driver now.
On Wed, Oct 14, 2020 at 02:14:12AM +0800, vijayakannan.ayyathurai@xxxxxxxxx wrote:
> +static void keembay_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> + unsigned long long pwm_h_count, pwm_l_count;
> + unsigned long clk_rate;
> + u32 buff;
> +
> + clk_rate = clk_get_rate(priv->clk);
> +
> + /* Read channel enabled status */
> + buff = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm));
> + if (buff & KMB_PWM_EN_BIT)
> + state->enabled = true;
> + else
> + state->enabled = false;
> +
> + /* Read period and duty cycle */
> + buff = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
> + pwm_l_count = FIELD_GET(KMB_PWM_LOW_MASK, buff) * NSEC_PER_SEC;
> + pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * NSEC_PER_SEC;
<minor nit>: The variable names are not optimal. I'd use "highlow"
instead of "buff". pwm_l_count would be appropriate for
FIELD_GET(KMB_PWM_LOW_MASK, buff); when multiplied with NSEC_PER_SEC
it's not really matching. Maybe just use "low"?! (and "high" instead of
pwm_h_count)
> + state->duty_cycle = DIV_ROUND_UP_ULL(pwm_h_count, clk_rate);
> + state->period = DIV_ROUND_UP_ULL(pwm_h_count + pwm_l_count, clk_rate);
state->polarity = PWM_POLARITY_NORMAL; (That's the important bit here.)
> +}
> +
> +static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> + struct pwm_state current_state;
> + u16 pwm_h_count, pwm_l_count;
> + unsigned long long div;
> + unsigned long clk_rate;
> + u32 pwm_count = 0;
> +
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -ENOSYS;
> +
> + keembay_pwm_update_bits(priv, KMB_PWM_LEADIN_MASK, 0,
> + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm));
A comment describing the effect of this register would be great.
> + keembay_pwm_get_state(chip, pwm, ¤t_state);
> +
> + if (!state->enabled) {
> + if (current_state.enabled)
> + keembay_pwm_disable(priv, pwm->hwpwm);
> + return 0;
> + }
> +
> [...]
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
