Re: [PATCH v10 1/2] pwm: Add PWM driver for Intel Keem Bay

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 4403 bytes --]

Hello,

On Thu, Oct 08, 2020 at 01:40:30AM +0800, vijayakannan.ayyathurai@intel.com wrote:
> +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));
> +
> +	keembay_pwm_get_state(chip, pwm, &current_state);
> +
> +	if (!state->enabled) {
> +		if (current_state.enabled)
> +			keembay_pwm_disable(priv, pwm->hwpwm);
> +		return 0;
> +	}
> +
> +	/*
> +	 * The upper 16 bits of the KMB_PWM_HIGHLOW_OFFSET register contain
> +	 * the high time of the waveform, while the last 16 bits contain
> +	 * the low time of the waveform, in terms of clock cycles.
> +	 *
> +	 * high time = clock rate * duty cycle
> +	 * low time =  clock rate * (period - duty cycle)
> +	 */
> +
> +	clk_rate = clk_get_rate(priv->clk);
> +	/* Configure waveform high time */
> +	div = clk_rate * state->duty_cycle;
> +	div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC);
> +	if (div > KMB_PWM_COUNT_MAX)
> +		return -ERANGE;
> +
> +	pwm_h_count = div;
> +	/* Configure waveform low time */
> +	div = clk_rate * (state->period - state->duty_cycle);
> +	div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC - pwm_h_count);

In reply to your v7 I suggested the example:

	clk_rate = 333334 [Hz]
	state.duty_cycle = 1000500 [ns]
	state.period = 2001000 [ns]

where the expected outcome is

	pwm_h_count = 333
	pwm_l_count = 334

. Please reread my feedback there. I tried to construct an example where
the value is more wrong, but with the constraint that pwm_h_count must
be <= KMB_PWM_COUNT_MAX this isn't easy. The best I could come up with
is:

	clk_rate = 1000000000
	state.duty_cycle = 65535 [ns]
	state.period = 131070 [ns]

where the right value for pwm_l_count is 65535 while you calculate
65539 (and then quit with -ERANGE).

> +	if (div > KMB_PWM_COUNT_MAX)
> +		return -ERANGE;
> +
> +	pwm_l_count = div;
> +
> +	pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) |
> +		    FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count);
> +
> +	writel(pwm_count, priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
> +
> +	if (state->enabled && !current_state.enabled)
> +		keembay_pwm_enable(priv, pwm->hwpwm);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops keembay_pwm_ops = {
> +	.owner = THIS_MODULE,
> +	.apply = keembay_pwm_apply,
> +	.get_state = keembay_pwm_get_state,
> +};
> +
> +static int keembay_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct keembay_pwm *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get clock\n");
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		return ret;
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base)) {
> +		clk_disable_unprepare(priv->clk);
> +		return PTR_ERR(priv->base);
> +	}
> +
> +	priv->chip.base = -1;
> +	priv->chip.dev = dev;
> +	priv->chip.ops = &keembay_pwm_ops;
> +	priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS;
> +
> +	ret = pwmchip_add(&priv->chip);
> +	if (ret) {
> +		dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret));
> +		clk_disable_unprepare(priv->clk);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +}
> +
> +static int keembay_pwm_remove(struct platform_device *pdev)
> +{
> +	struct keembay_pwm *priv = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(priv->clk);
> +
> +	return pwmchip_remove(&priv->chip);

You have to call pwmchip_remove first. Otherwise you're stopping the PWM
while the framework still believes everything to be fine.

> +}

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]