From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751531AbdBWKdN (ORCPT ); Thu, 23 Feb 2017 05:33:13 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:39365 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751308AbdBWKch (ORCPT ); Thu, 23 Feb 2017 05:32:37 -0500 Date: Thu, 23 Feb 2017 11:32:26 +0100 From: Boris Brezillon To: m18063 Cc: Alexandre Belloni , , , , , , , , , , Subject: Re: [PATCH 2/2] drivers: pwm: pwm-atmel: add support to allow run time changing of pwm parameters Message-ID: <20170223113226.7b24ffb6@bbrezillon> In-Reply-To: References: <1487839120-13650-1-git-send-email-claudiu.beznea@microchip.com> <1487839120-13650-3-git-send-email-claudiu.beznea@microchip.com> <20170223092148.5qmxy62xkufwbsrw@piout.net> 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 Hi Claudiu, On Thu, 23 Feb 2017 12:25:58 +0200 m18063 wrote: > Hi, > > > On 23.02.2017 11:21, Alexandre Belloni wrote: > > On 23/02/2017 at 10:38:40 +0200, Claudiu Beznea wrote: > >> sama5d2 supports changing of pwm parameters like period and > >> duty factor without first to disable pwm. Since pwm code > >> is supported by more than one SoC add allow_runtime_cfg > >> parameter to atmel_pwm_chip data structure. This will be > >> filled statically for every SoC, saved in pwm specific > >> structure at probing time and checked while configuring > >> the device. Based on this, pwm clock will not be > >> enabled/disabled while configuring if it still enabled. > >> > >> Signed-off-by: Claudiu Beznea > >> --- > >> drivers/pwm/pwm-atmel.c | 24 ++++++++++++++++++------ > >> 1 file changed, 18 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > >> index 4406639..9e1dece 100644 > >> --- a/drivers/pwm/pwm-atmel.c > >> +++ b/drivers/pwm/pwm-atmel.c > >> @@ -68,6 +68,8 @@ struct atmel_pwm_chip { > >> > >> void (*config)(struct pwm_chip *chip, struct pwm_device *pwm, > >> unsigned long dty, unsigned long prd); > >> + > >> + bool allow_runtime_cfg; > >> }; > >> > >> static inline struct atmel_pwm_chip *to_atmel_pwm_chip(struct pwm_chip *chip) > >> @@ -114,7 +116,8 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > >> u32 val; > >> int ret; > >> > >> - if (pwm_is_enabled(pwm) && (period_ns != pwm_get_period(pwm))) { > >> + if (!atmel_pwm->allow_runtime_cfg && > >> + pwm_is_enabled(pwm) && (period_ns != pwm_get_period(pwm))) { > >> dev_err(chip->dev, "cannot change PWM period while enabled\n"); > >> return -EBUSY; > >> } > >> @@ -139,10 +142,12 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > >> do_div(div, period_ns); > >> dty = prd - div; > >> > >> - ret = clk_enable(atmel_pwm->clk); > >> - if (ret) { > >> - dev_err(chip->dev, "failed to enable PWM clock\n"); > >> - return ret; > >> + if (!pwm_is_enabled(pwm)) { > >> + ret = clk_enable(atmel_pwm->clk); > >> + if (ret) { > >> + dev_err(chip->dev, "failed to enable PWM clock\n"); > >> + return ret; > >> + } > >> } > >> > > It is probably worth switching to atomic PWM instead of changing this > > function. This would simplify the whole driver. > I was thinking to switch to atomic PWM in a future patch. Actually, I think it's better to do it before adding support for the new IP (even before patch 1), but maybe I'm the only one to think so :-). Note that switching to the atomic API is not a big (actually, it should even simplify the code). Regards, Boris From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Subject: Re: [PATCH 2/2] drivers: pwm: pwm-atmel: add support to allow run time changing of pwm parameters Date: Thu, 23 Feb 2017 11:32:26 +0100 Message-ID: <20170223113226.7b24ffb6@bbrezillon> References: <1487839120-13650-1-git-send-email-claudiu.beznea@microchip.com> <1487839120-13650-3-git-send-email-claudiu.beznea@microchip.com> <20170223092148.5qmxy62xkufwbsrw@piout.net> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: m18063 Cc: Alexandre Belloni , thierry.reding@gmail.com, robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, linux-pwm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org Hi Claudiu, On Thu, 23 Feb 2017 12:25:58 +0200 m18063 wrote: > Hi, > > > On 23.02.2017 11:21, Alexandre Belloni wrote: > > On 23/02/2017 at 10:38:40 +0200, Claudiu Beznea wrote: > >> sama5d2 supports changing of pwm parameters like period and > >> duty factor without first to disable pwm. Since pwm code > >> is supported by more than one SoC add allow_runtime_cfg > >> parameter to atmel_pwm_chip data structure. This will be > >> filled statically for every SoC, saved in pwm specific > >> structure at probing time and checked while configuring > >> the device. Based on this, pwm clock will not be > >> enabled/disabled while configuring if it still enabled. > >> > >> Signed-off-by: Claudiu Beznea > >> --- > >> drivers/pwm/pwm-atmel.c | 24 ++++++++++++++++++------ > >> 1 file changed, 18 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > >> index 4406639..9e1dece 100644 > >> --- a/drivers/pwm/pwm-atmel.c > >> +++ b/drivers/pwm/pwm-atmel.c > >> @@ -68,6 +68,8 @@ struct atmel_pwm_chip { > >> > >> void (*config)(struct pwm_chip *chip, struct pwm_device *pwm, > >> unsigned long dty, unsigned long prd); > >> + > >> + bool allow_runtime_cfg; > >> }; > >> > >> static inline struct atmel_pwm_chip *to_atmel_pwm_chip(struct pwm_chip *chip) > >> @@ -114,7 +116,8 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > >> u32 val; > >> int ret; > >> > >> - if (pwm_is_enabled(pwm) && (period_ns != pwm_get_period(pwm))) { > >> + if (!atmel_pwm->allow_runtime_cfg && > >> + pwm_is_enabled(pwm) && (period_ns != pwm_get_period(pwm))) { > >> dev_err(chip->dev, "cannot change PWM period while enabled\n"); > >> return -EBUSY; > >> } > >> @@ -139,10 +142,12 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > >> do_div(div, period_ns); > >> dty = prd - div; > >> > >> - ret = clk_enable(atmel_pwm->clk); > >> - if (ret) { > >> - dev_err(chip->dev, "failed to enable PWM clock\n"); > >> - return ret; > >> + if (!pwm_is_enabled(pwm)) { > >> + ret = clk_enable(atmel_pwm->clk); > >> + if (ret) { > >> + dev_err(chip->dev, "failed to enable PWM clock\n"); > >> + return ret; > >> + } > >> } > >> > > It is probably worth switching to atomic PWM instead of changing this > > function. This would simplify the whole driver. > I was thinking to switch to atomic PWM in a future patch. Actually, I think it's better to do it before adding support for the new IP (even before patch 1), but maybe I'm the only one to think so :-). Note that switching to the atomic API is not a big (actually, it should even simplify the code). Regards, Boris From mboxrd@z Thu Jan 1 00:00:00 1970 From: boris.brezillon@free-electrons.com (Boris Brezillon) Date: Thu, 23 Feb 2017 11:32:26 +0100 Subject: [PATCH 2/2] drivers: pwm: pwm-atmel: add support to allow run time changing of pwm parameters In-Reply-To: References: <1487839120-13650-1-git-send-email-claudiu.beznea@microchip.com> <1487839120-13650-3-git-send-email-claudiu.beznea@microchip.com> <20170223092148.5qmxy62xkufwbsrw@piout.net> Message-ID: <20170223113226.7b24ffb6@bbrezillon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Claudiu, On Thu, 23 Feb 2017 12:25:58 +0200 m18063 wrote: > Hi, > > > On 23.02.2017 11:21, Alexandre Belloni wrote: > > On 23/02/2017 at 10:38:40 +0200, Claudiu Beznea wrote: > >> sama5d2 supports changing of pwm parameters like period and > >> duty factor without first to disable pwm. Since pwm code > >> is supported by more than one SoC add allow_runtime_cfg > >> parameter to atmel_pwm_chip data structure. This will be > >> filled statically for every SoC, saved in pwm specific > >> structure at probing time and checked while configuring > >> the device. Based on this, pwm clock will not be > >> enabled/disabled while configuring if it still enabled. > >> > >> Signed-off-by: Claudiu Beznea > >> --- > >> drivers/pwm/pwm-atmel.c | 24 ++++++++++++++++++------ > >> 1 file changed, 18 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > >> index 4406639..9e1dece 100644 > >> --- a/drivers/pwm/pwm-atmel.c > >> +++ b/drivers/pwm/pwm-atmel.c > >> @@ -68,6 +68,8 @@ struct atmel_pwm_chip { > >> > >> void (*config)(struct pwm_chip *chip, struct pwm_device *pwm, > >> unsigned long dty, unsigned long prd); > >> + > >> + bool allow_runtime_cfg; > >> }; > >> > >> static inline struct atmel_pwm_chip *to_atmel_pwm_chip(struct pwm_chip *chip) > >> @@ -114,7 +116,8 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > >> u32 val; > >> int ret; > >> > >> - if (pwm_is_enabled(pwm) && (period_ns != pwm_get_period(pwm))) { > >> + if (!atmel_pwm->allow_runtime_cfg && > >> + pwm_is_enabled(pwm) && (period_ns != pwm_get_period(pwm))) { > >> dev_err(chip->dev, "cannot change PWM period while enabled\n"); > >> return -EBUSY; > >> } > >> @@ -139,10 +142,12 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > >> do_div(div, period_ns); > >> dty = prd - div; > >> > >> - ret = clk_enable(atmel_pwm->clk); > >> - if (ret) { > >> - dev_err(chip->dev, "failed to enable PWM clock\n"); > >> - return ret; > >> + if (!pwm_is_enabled(pwm)) { > >> + ret = clk_enable(atmel_pwm->clk); > >> + if (ret) { > >> + dev_err(chip->dev, "failed to enable PWM clock\n"); > >> + return ret; > >> + } > >> } > >> > > It is probably worth switching to atomic PWM instead of changing this > > function. This would simplify the whole driver. > I was thinking to switch to atomic PWM in a future patch. Actually, I think it's better to do it before adding support for the new IP (even before patch 1), but maybe I'm the only one to think so :-). Note that switching to the atomic API is not a big (actually, it should even simplify the code). Regards, Boris