From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755237AbbIHQ7x (ORCPT ); Tue, 8 Sep 2015 12:59:53 -0400 Received: from mail-pa0-f51.google.com ([209.85.220.51]:35109 "EHLO mail-pa0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754779AbbIHQ7i (ORCPT ); Tue, 8 Sep 2015 12:59:38 -0400 Message-ID: <55EF1359.70109@gmail.com> Date: Tue, 08 Sep 2015 09:56:57 -0700 From: Florian Fainelli User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: "Ariel D'Alessandro" , Florian Fainelli , linux-kernel@vger.kernel.org CC: bcm-kernel-feedback-list@broadcom.com, thierry.reding@gmail.com, devicetree@vger.kernel.org, linux-pwm@vger.kernel.org Subject: Re: [PATCH v3 2/2] pwm: Add Broadcom BCM7038 PWM controller support References: <1440811309-1289-1-git-send-email-f.fainelli@gmail.com> <1440811309-1289-3-git-send-email-f.fainelli@gmail.com> <55EDE24F.7020402@vanguardiasur.com.ar> In-Reply-To: <55EDE24F.7020402@vanguardiasur.com.ar> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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