RE: [PATCHv9 Resend 1/4] pwm: Add Freescale FTM PWM driver support

["Li.Xiubo@freescale.com" <Li.Xiubo@freescale.com>]

Hi Thierry,

Thanks very much, I will fix them all.

:)

--
Best Regards,
Xiubo

> Sorry for taking so long to get back to you. Things have been quite busy
> lately. A few more comments below, but we're getting there.
> 
> On Wed, Feb 19, 2014 at 04:38:54PM +0800, Xiubo Li wrote:
> [...]
> > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> [...]
> > +static unsigned long fsl_pwm_calculate_period(struct fsl_pwm_chip *fpc,
> > +					      unsigned long period_ns)
> > +{
> > +	struct clk *cnt_clk[3];
> > +	enum fsl_pwm_clk m0, m1;
> > +	unsigned long fix_rate, ext_rate, cycles;
> > +
> > +	fpc->counter_clk = fpc->sys_clk;
> > +	cycles = fsl_pwm_calculate_period_cycles(fpc, period_ns,
> > +			FSL_PWM_CLK_SYS);
> > +	if (cycles)
> > +		return cycles;
> > +
> > +	cnt_clk[FSL_PWM_CLK_FIX] = devm_clk_get(fpc->chip.dev, "ftm_fix");
> > +	if (IS_ERR(cnt_clk[FSL_PWM_CLK_FIX]))
> > +		return PTR_ERR(cnt_clk[FSL_PWM_CLK_FIX]);
> > +
> > +	cnt_clk[FSL_PWM_CLK_EXT] = devm_clk_get(fpc->chip.dev, "ftm_ext");
> > +	if (IS_ERR(cnt_clk[FSL_PWM_CLK_EXT]))
> > +		return PTR_ERR(cnt_clk[FSL_PWM_CLK_EXT]);
> > +
> > +	fpc->counter_clk_en = devm_clk_get(fpc->chip.dev, "ftm_cnt_clk_en");
> > +	if (IS_ERR(fpc->counter_clk_en))
> > +		return PTR_ERR(fpc->counter_clk_en);
> 
> You shouldn't do this. You're obtaining a reference to each of these
> clocks whenever pwm_config() is called. And devres will only clean those
> up after the driver is unbound. Can't you simply keep a reference to
> these within struct fsl_pwm_chip?
> 
> > +static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc)
> > +{
> > +	u32 val;
> > +	int ret;
> > +
> > +	if (fpc->counter_clk_enable++)
> 
> This function is always called with the fpc->lock held, so you could
> make this much easier by incrementing the .counter_clk_enable field only
> at the very end of the function. That way...
> 
> > +		return 0;
> > +
> > +	ret = clk_prepare_enable(fpc->counter_clk);
> > +	if (ret) {
> > +		fpc->counter_clk_enable--;
> 
> ... this won't be necessary...
> 
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(fpc->counter_clk_en);
> > +	if (ret) {
> > +		fpc->counter_clk_enable--;
> 
> ... and neither will this.
> 
> > +static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> > +	u32 val;
> > +	int ret;
> > +
> > +	val = readl(fpc->base + FTM_OUTMASK);
> > +	val &= ~BIT(pwm->hwpwm);
> > +	writel(val, fpc->base + FTM_OUTMASK);
> > +
> > +	mutex_lock(&fpc->lock);
> 
> I think you want to extend the lock to cover the FTM_OUTMASK register
> access as well because there could be a race between pwm_enable() and
> pwm_disable().
> 
> > +	ret = fsl_counter_clock_enable(fpc);
> > +	mutex_unlock(&fpc->lock);
> > +
> > +	return ret;
> > +}
> 
> Can this function be moved somewhere else so fsl_counter_clock_enable()
> and fsl_counter_clock_disable() are grouped together?
> 
> > +static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc)
> > +{
> > +	u32 val;
> > +
> > +	if (--fpc->counter_clk_enable)
> > +		return;
> 
> This is going to break. Consider the case where you call pwm_disable()
> on a PWM device and fpc->counter_clk_enable == 1. In that case, this
> will decrement counter_clk_enable to 0 and proceed with the remainder of
> this function.
> 
> Now you call pwm_disable() again. The above will decrement again and
> cause fpc->counter_clk_enable to wrap around to UINT_MAX.
> 
> So I think a more correct implementation would be:
> 
> 	/*
> 	 * already disabled, do nothing (perhaps output warning message
> 	 * to catch unbalanced calls? )
> 	 */
> 	if (fpc->counter_clk_enable == 0)
> 		return;
> 
> 	/* there are still users, so can't disable yet */
> 	if (--fpc->counter_clk_enable > 0)
> 		return;
> 
> 	/* no users left, disable clock */
> 
> > +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> > +	u32 val;
> > +
> > +	val = readl(fpc->base + FTM_OUTMASK);
> > +	val |= BIT(pwm->hwpwm);
> > +	writel(val, fpc->base + FTM_OUTMASK);
> > +
> > +	mutex_lock(&fpc->lock);
> 
> This lock should also include the access to FTM_OUTMASK above.
> 
> > +static int fsl_pwm_probe(struct platform_device *pdev)
> > +{
> [...]
> > +	fpc->sys_clk = devm_clk_get(&pdev->dev, "ftm_sys");
> > +	if (IS_ERR(fpc->sys_clk)) {
> > +		dev_err(&pdev->dev,
> > +				"failed to get \"ftm_sys\" clock\n");
> 
> The above easily fits on a single line, no need for the wrapping.
> 
> > +		return PTR_ERR(fpc->sys_clk);
> > +	}
> > +
> > +	ret = clk_prepare_enable(fpc->sys_clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	writel(0x00, fpc->base + FTM_CNTIN);
> > +	writel(0x00, fpc->base + FTM_OUTINIT);
> > +	writel(0xFF, fpc->base + FTM_OUTMASK);
> > +	clk_disable_unprepare(fpc->sys_clk);
> 
> This looks out of place somehow, perhaps it should be moved off into a
> separate function? fsl_pwm_init() perhaps.
> 
> Thierry