Hi Uwe,
Thanks for reviewing this patch.
-----Original Message-----
From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
Sent: Wednesday, 14 October, 2020 12:19 PM
To: Ayyathurai, Vijayakannan <vijayakannan.ayyathurai@xxxxxxxxx>
Cc: thierry.reding@xxxxxxxxx; robh+dt@xxxxxxxxxx; linux-pwm@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Wan Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>; andriy.shevchenko@xxxxxxxxxxxxxxx; mgross@xxxxxxxxxxxxxxx; Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@xxxxxxxxx>
Subject: Re: [PATCH v11 1/2] pwm: Add PWM driver for Intel Keem Bay
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)
Yes. I have also thought of it. So it will look like below in the next version.
highlow = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
low = FIELD_GET(KMB_PWM_LOW_MASK, buff) * NSEC_PER_SEC;
high = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * NSEC_PER_SEC;
Also in other places of it as well.
> + 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.)
Ok. I will incorporate in next version.
> +}
> +
> +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.
This register is for setting up the number of repetition and leadin low time for that particular channel. I will update it accordingly in the next version.
> + 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/ |
Thanks,
Vijay
