Re: [PATCH v3 2/2] pwm: Add Broadcom BCM7038 PWM controller support

[Florian Fainelli <f.fainelli@gmail.com>]

On 07/09/15 12:15, Ariel D'Alessandro wrote:
> Hi Florian,
> 
> I wrote some observations below that maybe can be useful.
> 
> El 28/08/15 a las 22:21, Florian Fainelli escribió:
>> Add support for the BCM7038-style PWM controller found in all BCM7xxx STB SoCs.
>> This controller has a hardcoded 2 channels per controller, and cascades a
>> variable frequency generator on top of a fixed frequency generator which offers
>> a range of a 148ns period all the way to ~622ms periods.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

NB: you can trim your replies so we do not have to skip through lengthy
uncommented parts of the patch.

[snip]

>> +
>> +static inline u32 pwm_readl(struct brcmstb_pwm_dev *p, u32 off)
> 
> The function name 'pwm_readl' sounds to be very common. It might be
> better to use a prefix here, don't you think? Maybe brcmstb_pwm_readl?

Sure, if that makes it clearer, these are local and inlined, so the
chances for getting a namespace conflict are very thin, but fair enough,
will rename to match the rest of the functions.

[snip]

>> +		/*
>> +		 * We can be called with separate duty and period updates,
>> +		 * so do not reject dc == 0 right away
>> +		 */
>> +		if (pc == PWM_PERIOD_MIN ||
>> +		   (dc < PWM_ON_MIN && duty_ns))
> 
> No break needed here, this expression can be written on a single line
> increasing readability.
> 
>> +			return -EINVAL;
>> +
>> +		/* We converged on a calculation */
>> +		if (pc <= PWM_ON_PERIOD_MAX && dc <= PWM_ON_PERIOD_MAX)
>> +			break;
>> +
>> +		/*
>> +		 * The cword needs to be a power of 2 for the variable
>> +		 * frequency generator to output a 50% duty cycle variable
>> +		 * frequency which is used as input clock to the fixed
>> +		 * frequency generator.
>> +		 */
>> +		cword >>= 1;
>> +
>> +		/*
>> +		 * Desired periods are too large, we do not have a divider
>> +		 * for them
>> +		 */
>> +		if (cword < CONST_VAR_F_MIN)
>> +			return -EINVAL;
>> +	}
>> +
>> +done:
>> +	/*
>> +	 * Configure the defined "cword" value to have the variable frequency
>> +	 * generator output a base frequency for the constant frequency
>> +	 * generator to derive from.
>> +	 */
>> +	pwm_writel(b, cword >> 8, PWM_CWORD_MSB + ch * PWM_CH_SIZE);
>> +	pwm_writel(b, cword & 0xff, PWM_CWORD_LSB + ch * PWM_CH_SIZE);
>> +
>> +	/* Select constant frequency signal output */
>> +	reg = pwm_readl(b, PWM_CTRL2);
>> +	reg |= (CTRL2_OUT_SELECT << (ch * CTRL_CHAN_OFFS));
> 
> A nitpick here, outer parenthesis can be avoided.
> 
>> +	pwm_writel(b, reg, PWM_CTRL2);
> 
> This read-modify-write sequence may be protected by some locking
> mechanism. Notice that, as written in the docs: "PWM core does not
> enforce any locking to pwm_enable(), pwm_disable() and pwm_config()".

Right, I will add required locking here, thanks!

[snip]

>> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	p->base = devm_ioremap_resource(&pdev->dev, r);
>> +	if (!p->base)
>> +		return -ENOMEM;
> 
> I think you're missing some cleanup routine here. You have a previous
> call to clk_prepare_enable(), so you may have a call to
> clk_disable_unprepare() here in order to exit cleanly.

Good catch yes.

> 
>> +
>> +	ret = pwmchip_add(&p->chip);
>> +	if (ret)
>> +		dev_err(&pdev->dev, "failed to add PWM chip %d\n", ret);
> 
> Cleanup needed here too.
> 
>> +
>> +	return ret;
>> +}
>> +
>> +static int brcmstb_pwm_remove(struct platform_device *pdev)
>> +{
>> +	struct brcmstb_pwm_dev *p = platform_get_drvdata(pdev);
>> +
>> +	clk_disable_unprepare(p->clk);
>> +
>> +	return pwmchip_remove(&p->chip);
> 
> AFAIK, pwmchip_remove() may return busy if the PWM chip provides a PWM
> device that is still requested, so you shouldn't disable the clock
> before you ensure the PWM chip has been successfuly removed.

Absolutely.

> 
> It think you could do something like:
> 
> 	ret = pwmchip_remove(&p->chip);
> 	if (ret)
> 		return ret;
> 
> 	clk_disable_unprepare(p->clk);
> 	return 0;
> 

-- 
Florian