Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753172AbcL2QV3 (ORCPT ); Thu, 29 Dec 2016 11:21:29 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:60329 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753119AbcL2QV1 (ORCPT ); Thu, 29 Dec 2016 11:21:27 -0500 Date: Thu, 29 Dec 2016 17:21:17 +0100 From: Boris Brezillon To: Lukasz Majewski Cc: Thierry Reding , Sascha Hauer , Stefan Agner , linux-pwm@vger.kernel.org, Bhuvanchandra DV , linux-kernel@vger.kernel.org, Fabio Estevam , Fabio Estevam , Lothar Wassmann , kernel@pengutronix.de Subject: Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2 Message-ID: <20161229172117.523a42a4@bbrezillon> In-Reply-To: <1482792961-12702-8-git-send-email-l.majewski@majess.pl> References: <1477259146-19167-1-git-send-email-l.majewski@majess.pl> <1482792961-12702-1-git-send-email-l.majewski@majess.pl> <1482792961-12702-8-git-send-email-l.majewski@majess.pl> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3355 Lines: 115 Hi Lukasz, On Mon, 26 Dec 2016 23:55:57 +0100 Lukasz Majewski wrote: > This commit provides apply() callback implementation for i.MX's PWMv2. > > Suggested-by: Stefan Agner > Suggested-by: Boris Brezillon > Signed-off-by: Lukasz Majewski > Reviewed-by: Boris Brezillon > --- > Changes for v3: > - Remove ipg clock enable/disable functions > > Changes for v2: > - None > --- > drivers/pwm/pwm-imx.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 70 insertions(+) > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > index ebe9b0c..cd53c05 100644 > --- a/drivers/pwm/pwm-imx.c > +++ b/drivers/pwm/pwm-imx.c > @@ -159,6 +159,75 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip, > } > } > > +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + unsigned long period_cycles, duty_cycles, prescale; > + struct imx_chip *imx = to_imx_chip(chip); > + struct pwm_state cstate; > + unsigned long long c; > + u32 cr = 0; > + int ret; > + > + pwm_get_state(pwm, &cstate); > + > + c = clk_get_rate(imx->clk_per); > + c *= state->period; > + > + do_div(c, 1000000000); > + period_cycles = c; > + > + prescale = period_cycles / 0x10000 + 1; > + > + period_cycles /= prescale; > + c = (unsigned long long)period_cycles * state->duty_cycle; > + do_div(c, state->period); > + duty_cycles = c; > + > + /* > + * according to imx pwm RM, the real period value should be > + * PERIOD value in PWMPR plus 2. > + */ > + if (period_cycles > 2) > + period_cycles -= 2; > + else > + period_cycles = 0; > + > + /* Enable the clock if the PWM is being enabled. */ > + if (state->enabled && !cstate.enabled) { > + ret = clk_prepare_enable(imx->clk_per); > + if (ret) > + return ret; > + } > + > + /* > + * Wait for a free FIFO slot if the PWM is already enabled, and flush > + * the FIFO if the PWM was disabled and is about to be enabled. > + */ > + if (cstate.enabled) > + imx_pwm_wait_fifo_slot(chip, pwm); > + else if (state->enabled) > + imx_pwm_sw_reset(chip); > + > + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > + writel(period_cycles, imx->mmio_base + MX3_PWMPR); > + > + cr |= MX3_PWMCR_PRESCALER(prescale) | > + MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN | > + MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH; > + > + if (state->enabled) > + cr |= MX3_PWMCR_EN; > + > + writel(cr, imx->mmio_base + MX3_PWMCR); > + > + /* Disable the clock if the PWM is being disabled. */ > + if (!state->enabled && cstate.enabled) > + clk_disable_unprepare(imx->clk_per); > + > + return 0; > +} > + Stefan suggested to rework this function to avoid unneeded duty/period calculation and reg write when disabling the PWM. Why didn't you send a v4 addressing that instead of resending the exact same v3? Same goes for the regression introduced in patch 2: I think it's better to keep things bisectable on all platforms (even if it appeared to work by chance on imx7, it did work before this change). That's just my opinion, but when you get reviews on a patch series, it's better to address them directly (especially when issues can be easily fixed) than provide follow-up patches. Regards, Boris