From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754144AbbDHPcn (ORCPT ); Wed, 8 Apr 2015 11:32:43 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:52069 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753752AbbDHPck (ORCPT ); Wed, 8 Apr 2015 11:32:40 -0400 Date: Wed, 8 Apr 2015 08:32:14 -0700 From: Guenter Roeck To: Lukasz Majewski Cc: Anand Moon , Eduardo Valentin , Sjoerd Simons , Russell King , Kukjin Kim , 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 Message-ID: <20150408153214.GA15942@roeck-us.net> References: <1427387955-5129-1-git-send-email-linux.amoon@gmail.com> <1427387955-5129-7-git-send-email-linux.amoon@gmail.com> <20150408104415.07e1c821@amdc2363> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150408104415.07e1c821@amdc2363> User-Agent: Mutt/1.5.23 (2014-03-12) X-Authenticated_sender: guenter@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-CTCH-PVer: 0000001 X-CTCH-Spam: Unknown X-CTCH-VOD: Unknown X-CTCH-Flags: 0 X-CTCH-RefID: str=0001.0A020205.55254A18.0215,ss=1,re=0.001,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0 X-CTCH-Score: 0.001 X-CTCH-ScoreCust: 0.000 X-CTCH-Rules: C_4847, X-CTCH-SenderID: linux@roeck-us.net X-CTCH-SenderID-Flags: 0 X-CTCH-SenderID-TotalMessages: 10 X-CTCH-SenderID-TotalSpam: 0 X-CTCH-SenderID-TotalSuspected: 0 X-CTCH-SenderID-TotalConfirmed: 0 X-CTCH-SenderID-TotalBulk: 0 X-CTCH-SenderID-TotalVirus: 0 X-CTCH-SenderID-TotalRecipients: 0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: mailgid no entry from get_relayhosts_entry X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > --- > > 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 > > BTW: I've added Guenter to CC. > > -- > Best regards, > > Lukasz Majewski > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group From mboxrd@z Thu Jan 1 00:00:00 1970 From: linux@roeck-us.net (Guenter Roeck) Date: Wed, 8 Apr 2015 08:32:14 -0700 Subject: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan In-Reply-To: <20150408104415.07e1c821@amdc2363> References: <1427387955-5129-1-git-send-email-linux.amoon@gmail.com> <1427387955-5129-7-git-send-email-linux.amoon@gmail.com> <20150408104415.07e1c821@amdc2363> Message-ID: <20150408153214.GA15942@roeck-us.net> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > > --- > > 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 > > BTW: I've added Guenter to CC. > > -- > Best regards, > > Lukasz Majewski > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group