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@xxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
