From: Guenter Roeck <linux@roeck-us.net> To: Lukasz Majewski <l.majewski@samsung.com> Cc: Anand Moon <linux.amoon@gmail.com>, Eduardo Valentin <edubezval@gmail.com>, Sjoerd Simons <sjoerd.simons@collabora.co.uk>, Russell King <linux@arm.linux.org.uk>, Kukjin Kim <kgene@kernel.org>, devicetree@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-pwm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan Date: Wed, 8 Apr 2015 08:32:14 -0700 [thread overview] Message-ID: <20150408153214.GA15942@roeck-us.net> (raw) In-Reply-To: <20150408104415.07e1c821@amdc2363> On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote: > Hi Anand, > > > Below changes depend on following patch. > > https://patchwork.kernel.org/patch/5944061/ > > > > Update the pwm_config with duty then update the pwm_disable > > to poweroff the cpu fan. > > Unfortunately, the patch does not include an explanation why it is needed. The original code presumably did not update the duty cycle because pwm was about to be disabled anyway. That kind of made sense to me. Updating the duty cycle to 0 just to disable the pwm channel right afterwards does not immediately make sense. Given that, I would expect to see a rationale here. Why is this patch needed ? Does it fix a bug ? If yes, pelase describe the bug. If not, what is the purpose of this patch ? Maybe that is all explained in patch 0/6, which I was not copied on. Even if so, the reationale will be needed in the changelog to explain to future developers why this change was made. Thanks, Guenter > > Tested on OdroidXU3 board. > > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > > --- > > drivers/hwmon/pwm-fan.c | 10 ++++------ > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > > index 7c83dc4..f25c841 100644 > > --- a/drivers/hwmon/pwm-fan.c > > +++ b/drivers/hwmon/pwm-fan.c > > @@ -44,26 +44,24 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, > > unsigned long pwm) int ret = 0; > > > > mutex_lock(&ctx->lock); > > + [ please refrain from unnecessary whitespace changes ] > > if (ctx->pwm_value == pwm) > > goto exit_set_pwm_err; > > > > - if (pwm == 0) { > > - pwm_disable(ctx->pwm); > > - goto exit_set_pwm; > > - } > > - > > duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM); > > ret = pwm_config(ctx->pwm, duty, ctx->pwm->period); > > if (ret) > > goto exit_set_pwm_err; > > > > + if (pwm == 0) > > + pwm_disable(ctx->pwm); > > + > > if (ctx->pwm_value == 0) { > > ret = pwm_enable(ctx->pwm); > > if (ret) > > goto exit_set_pwm_err; > > } > > > > -exit_set_pwm: > > ctx->pwm_value = pwm; > > exit_set_pwm_err: > > mutex_unlock(&ctx->lock); > > Reviewed-by: Lukasz Majewski <l.majewski@samsung.com> > > BTW: I've added Guenter to CC. > > -- > Best regards, > > Lukasz Majewski > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
WARNING: multiple messages have this Message-ID (diff)
From: linux@roeck-us.net (Guenter Roeck) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan Date: Wed, 8 Apr 2015 08:32:14 -0700 [thread overview] Message-ID: <20150408153214.GA15942@roeck-us.net> (raw) In-Reply-To: <20150408104415.07e1c821@amdc2363> On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote: > Hi Anand, > > > Below changes depend on following patch. > > https://patchwork.kernel.org/patch/5944061/ > > > > Update the pwm_config with duty then update the pwm_disable > > to poweroff the cpu fan. > > Unfortunately, the patch does not include an explanation why it is needed. The original code presumably did not update the duty cycle because pwm was about to be disabled anyway. That kind of made sense to me. Updating the duty cycle to 0 just to disable the pwm channel right afterwards does not immediately make sense. Given that, I would expect to see a rationale here. Why is this patch needed ? Does it fix a bug ? If yes, pelase describe the bug. If not, what is the purpose of this patch ? Maybe that is all explained in patch 0/6, which I was not copied on. Even if so, the reationale will be needed in the changelog to explain to future developers why this change was made. Thanks, Guenter > > Tested on OdroidXU3 board. > > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > > --- > > drivers/hwmon/pwm-fan.c | 10 ++++------ > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > > index 7c83dc4..f25c841 100644 > > --- a/drivers/hwmon/pwm-fan.c > > +++ b/drivers/hwmon/pwm-fan.c > > @@ -44,26 +44,24 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, > > unsigned long pwm) int ret = 0; > > > > mutex_lock(&ctx->lock); > > + [ please refrain from unnecessary whitespace changes ] > > if (ctx->pwm_value == pwm) > > goto exit_set_pwm_err; > > > > - if (pwm == 0) { > > - pwm_disable(ctx->pwm); > > - goto exit_set_pwm; > > - } > > - > > duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM); > > ret = pwm_config(ctx->pwm, duty, ctx->pwm->period); > > if (ret) > > goto exit_set_pwm_err; > > > > + if (pwm == 0) > > + pwm_disable(ctx->pwm); > > + > > if (ctx->pwm_value == 0) { > > ret = pwm_enable(ctx->pwm); > > if (ret) > > goto exit_set_pwm_err; > > } > > > > -exit_set_pwm: > > ctx->pwm_value = pwm; > > exit_set_pwm_err: > > mutex_unlock(&ctx->lock); > > Reviewed-by: Lukasz Majewski <l.majewski@samsung.com> > > BTW: I've added Guenter to CC. > > -- > Best regards, > > Lukasz Majewski > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
next prev parent reply other threads:[~2015-04-08 15:32 UTC|newest] Thread overview: 86+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-03-26 16:39 Exynos5422 odroidxu3 pwm-fan control using thermal sensors Anand Moon 2015-03-26 16:39 ` Anand Moon 2015-03-26 16:39 ` [PATCH 1/6] ARM: dts :exynos5422-odroidxu3 Add pwm-fan node to the Odroid-XU3 board Anand Moon 2015-03-26 16:39 ` Anand Moon 2015-04-08 7:46 ` Lukasz Majewski 2015-04-08 7:46 ` Lukasz Majewski 2015-03-26 16:39 ` [PATCH 2/6] ARM: dts exynos5420 update the cooling cells for core cpu0 Anand Moon 2015-03-26 16:39 ` Anand Moon 2015-04-08 7:47 ` Lukasz Majewski 2015-04-08 7:47 ` Lukasz Majewski 2015-03-26 16:39 ` [PATCH 3/6] ARM:dts exynos5422 Update the thermal sensor for tmu_cpu0 Anand Moon 2015-03-26 16:39 ` Anand Moon 2015-04-08 8:02 ` Lukasz Majewski 2015-04-08 8:02 ` Lukasz Majewski 2015-04-08 8:02 ` Lukasz Majewski 2015-04-08 9:12 ` Anand Moon 2015-04-08 9:12 ` Anand Moon 2015-04-08 9:12 ` Anand Moon 2015-04-08 9:30 ` Lukasz Majewski 2015-04-08 9:30 ` Lukasz Majewski 2015-04-08 9:30 ` Lukasz Majewski 2015-04-08 9:44 ` Anand Moon 2015-04-08 9:44 ` Anand Moon 2015-04-08 9:44 ` Anand Moon 2015-03-26 16:39 ` [PATCH 4/6] ARM: dts: OdroidXU3: Enable TMU at Exynos5422 base Anand Moon 2015-03-26 16:39 ` Anand Moon 2015-04-08 8:03 ` Lukasz Majewski 2015-04-08 8:03 ` Lukasz Majewski 2015-03-26 16:39 ` [PATCH 5/6] pwm: samsung: Fix output race on disabling Anand Moon 2015-03-26 16:39 ` Anand Moon 2015-03-26 16:39 ` Anand Moon 2015-04-08 8:28 ` Lukasz Majewski 2015-04-08 8:28 ` Lukasz Majewski 2015-04-08 8:42 ` Sjoerd Simons 2015-04-08 8:42 ` Sjoerd Simons 2015-04-08 8:53 ` Anand Moon 2015-04-08 8:53 ` Anand Moon 2015-04-08 8:53 ` Anand Moon 2015-03-26 16:39 ` [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan Anand Moon 2015-03-26 16:39 ` Anand Moon 2015-04-08 8:44 ` Lukasz Majewski 2015-04-08 8:44 ` Lukasz Majewski 2015-04-08 13:11 ` Guenter Roeck 2015-04-08 13:11 ` Guenter Roeck 2015-04-08 15:32 ` Guenter Roeck [this message] 2015-04-08 15:32 ` Guenter Roeck 2015-04-08 16:02 ` Anand Moon 2015-04-08 16:02 ` Anand Moon 2015-04-08 16:02 ` Anand Moon 2015-04-08 16:53 ` Guenter Roeck 2015-04-08 16:53 ` Guenter Roeck 2015-04-08 16:53 ` Guenter Roeck 2015-04-08 17:49 ` Anand Moon 2015-04-08 17:49 ` Anand Moon 2015-04-08 17:49 ` Anand Moon 2015-04-10 11:28 ` Anand Moon 2015-04-10 11:28 ` Anand Moon 2015-04-10 11:28 ` Anand Moon 2015-04-10 12:00 ` Sjoerd Simons 2015-04-10 12:00 ` Sjoerd Simons 2015-04-10 12:00 ` Sjoerd Simons 2015-04-10 12:59 ` Anand Moon 2015-04-10 12:59 ` Anand Moon 2015-04-10 12:59 ` Anand Moon 2015-04-10 13:09 ` Guenter Roeck 2015-04-10 13:09 ` Guenter Roeck 2015-04-10 13:09 ` Guenter Roeck 2015-04-10 13:17 ` Anand Moon 2015-04-10 13:17 ` Anand Moon 2015-04-10 13:17 ` Anand Moon 2015-04-10 13:30 ` Sjoerd Simons 2015-04-10 13:30 ` Sjoerd Simons 2015-04-10 13:30 ` Sjoerd Simons 2015-04-10 13:58 ` Thierry Reding 2015-04-10 13:58 ` Thierry Reding 2015-04-10 13:58 ` Thierry Reding 2015-04-10 17:25 ` Ben Gamari 2015-04-10 17:25 ` Ben Gamari 2015-04-10 17:25 ` Ben Gamari 2015-04-10 17:58 ` Guenter Roeck 2015-04-10 17:58 ` Guenter Roeck 2015-04-10 17:58 ` Guenter Roeck 2015-04-02 10:02 ` Exynos5422 odroidxu3 pwm-fan control using thermal sensors Markus Reichl 2015-04-02 10:02 ` Markus Reichl 2015-04-02 10:06 ` Markus Reichl 2015-04-02 10:06 ` Markus Reichl
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20150408153214.GA15942@roeck-us.net \ --to=linux@roeck-us.net \ --cc=devicetree@vger.kernel.org \ --cc=edubezval@gmail.com \ --cc=kgene@kernel.org \ --cc=l.majewski@samsung.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pwm@vger.kernel.org \ --cc=linux-samsung-soc@vger.kernel.org \ --cc=linux.amoon@gmail.com \ --cc=linux@arm.linux.org.uk \ --cc=sjoerd.simons@collabora.co.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.