| Message ID | 1427387955-5129-7-git-send-email-linux.amoon@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > 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); > + > 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.
On 04/08/2015 01:44 AM, 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. >> >> 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); >> + >> 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. > I don't have a copy of the original patch, so I can't apply it. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guenter, Initially the board bootup the cooling level state is 0. So update the duty cycle and this power off the fan. As their is no state change the fan will not spin. Once the temperature sensor is reached to alert temperature it changes state. With the state change the fan cools the CPU and then stop's I have observed this state change with tmon utility in linux/tools/thermal/tmon/ -Anand Moon On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote: > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote: > Hi Guenter, > > Initially the board bootup the cooling level state is 0. > So update the duty cycle and this power off the fan. > As their is no state change the fan will not spin. > > Once the temperature sensor is reached to alert temperature it changes state. > With the state change the fan cools the CPU and then stop's > > I have observed this state change with tmon utility in linux/tools/thermal/tmon/ > Sorry, I am missing something. I still don't see what problem you are fixing with this patch. What behavior is wrong with the current code, and how does your patch fix it ? Guenter > -Anand Moon > > On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote: > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guenter, Sorry my blunder mistake. Sorry for the noise. I just tested with spiking this patch and my observation and testing were wrong we can skip this patch. I will send an v2 patch series removing the patch 5 and patch 6. With correct dts changes. Thanks for pointing my mistake. -Anand Moon On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote: > On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote: >> Hi Guenter, >> >> Initially the board bootup the cooling level state is 0. >> So update the duty cycle and this power off the fan. >> As their is no state change the fan will not spin. >> >> Once the temperature sensor is reached to alert temperature it changes state. >> With the state change the fan cools the CPU and then stop's >> >> I have observed this state change with tmon utility in linux/tools/thermal/tmon/ >> > Sorry, I am missing something. I still don't see what problem you are fixing > with this patch. What behavior is wrong with the current code, and how does your > patch fix it ? > > Guenter > >> -Anand Moon >> >> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote: >> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guenter/Lukasz, Earlier I send v2 version of the patch spiking this one. Markus Riechl came back to me with below mail. So This patch confirms fixes the bug. I will send v3 version of the patch. Earlier I was in delima about the bug. -Anand Moon ------------------------------------------- Hi Anand, I tested your patch. After booting the fan is spinning despite only 44°C. /sys/class/thermal/cooling_device0/curstate is 0. /sys/class/hwmon/hwmon4/pwm1 is 0 when I echo 1 > cur_state and then echo 0 > cur_state again, the fan switches to off and behaves as expected. It looks like there is a bug in initializing the pwm output immediately after booting. Best Regards, -- Markus Reichl On 8 April 2015 at 23:19, Anand Moon <linux.amoon@gmail.com> wrote: > Hi Guenter, > > Sorry my blunder mistake. Sorry for the noise. > > I just tested with spiking this patch and my observation and testing > were wrong we can skip this patch. > > I will send an v2 patch series removing the patch 5 and patch 6. > > With correct dts changes. > > Thanks for pointing my mistake. > > -Anand Moon > > On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote: >> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote: >>> Hi Guenter, >>> >>> Initially the board bootup the cooling level state is 0. >>> So update the duty cycle and this power off the fan. >>> As their is no state change the fan will not spin. >>> >>> Once the temperature sensor is reached to alert temperature it changes state. >>> With the state change the fan cools the CPU and then stop's >>> >>> I have observed this state change with tmon utility in linux/tools/thermal/tmon/ >>> >> Sorry, I am missing something. I still don't see what problem you are fixing >> with this patch. What behavior is wrong with the current code, and how does your >> patch fix it ? >> >> Guenter >> >>> -Anand Moon >>> >>> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote: >>> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey Anand, On Fri, 2015-04-10 at 16:58 +0530, Anand Moon wrote: > Hi Guenter/Lukasz, > > Earlier I send v2 version of the patch spiking this one. > > Markus Riechl came back to me with below mail. > So This patch confirms fixes the bug. > > I will send v3 version of the patch. Earlier I was in delima about the bug. > > -Anand Moon > ------------------------------------------- > Hi Anand, > > I tested your patch. > > After booting the fan is spinning despite only 44°C. > > /sys/class/thermal/cooling_device0/curstate is 0. > /sys/class/hwmon/hwmon4/pwm1 is 0 > > when I echo 1 > cur_state and then echo 0 > cur_state again, > the fan switches to off and behaves as expected. > > It looks like there is a bug in initializing the pwm output > immediately after booting. The problem here will be that at boot the PWM runs at full duty. With the current exynos PWM drive if you disable the PWM it will stop pulsing but remain high if it was at 100% duty. My patch on which you depend upon fixed a race where disabling the pwm right after changing the duty cycle (e.g. to 0%) also kept the signal high. From looking at other PWM users at the time it seemed that most if not all always first set to duty to 0% and then disable the pwm. Which should work fine on exynos now. However iirc Thierry recently clarified that the expected result of pwm_disable is not just that the modulation stops but also that the output signal goes low, although that's not very explicit in the current pwm documentation.. The exynos PWM driver will need another fix tweak to make that true. > Best Regards, > -- > Markus Reichl > > On 8 April 2015 at 23:19, Anand Moon <linux.amoon@gmail.com> wrote: > > Hi Guenter, > > > > Sorry my blunder mistake. Sorry for the noise. > > > > I just tested with spiking this patch and my observation and testing > > were wrong we can skip this patch. > > > > I will send an v2 patch series removing the patch 5 and patch 6. > > > > With correct dts changes. > > > > Thanks for pointing my mistake. > > > > -Anand Moon > > > > On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote: > >> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote: > >>> Hi Guenter, > >>> > >>> Initially the board bootup the cooling level state is 0. > >>> So update the duty cycle and this power off the fan. > >>> As their is no state change the fan will not spin. > >>> > >>> Once the temperature sensor is reached to alert temperature it changes state. > >>> With the state change the fan cools the CPU and then stop's > >>> > >>> I have observed this state change with tmon utility in linux/tools/thermal/tmon/ > >>> > >> Sorry, I am missing something. I still don't see what problem you are fixing > >> with this patch. What behavior is wrong with the current code, and how does your > >> patch fix it ? > >> > >> Guenter > >> > >>> -Anand Moon > >>> > >>> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote: > >>> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sjoerd, I don't much advance knowledge on internal signaling of pwm-samsung module. So do I need to send this patch again ? -Anand Moon On 10 April 2015 at 17:30, Sjoerd Simons <sjoerd.simons@collabora.co.uk> wrote: > Hey Anand, > > On Fri, 2015-04-10 at 16:58 +0530, Anand Moon wrote: >> Hi Guenter/Lukasz, >> >> Earlier I send v2 version of the patch spiking this one. >> >> Markus Riechl came back to me with below mail. >> So This patch confirms fixes the bug. >> >> I will send v3 version of the patch. Earlier I was in delima about the bug. >> >> -Anand Moon >> ------------------------------------------- >> Hi Anand, >> >> I tested your patch. >> >> After booting the fan is spinning despite only 44°C. >> >> /sys/class/thermal/cooling_device0/curstate is 0. >> /sys/class/hwmon/hwmon4/pwm1 is 0 >> >> when I echo 1 > cur_state and then echo 0 > cur_state again, >> the fan switches to off and behaves as expected. >> >> It looks like there is a bug in initializing the pwm output >> immediately after booting. > > The problem here will be that at boot the PWM runs at full duty. With > the current exynos PWM drive if you disable the PWM it will stop pulsing > but remain high if it was at 100% duty. My patch on which you depend > upon fixed a race where disabling the pwm right after changing the duty > cycle (e.g. to 0%) also kept the signal high. > > From looking at other PWM users at the time it seemed that most if not > all always first set to duty to 0% and then disable the pwm. Which > should work fine on exynos now. However iirc Thierry recently clarified > that the expected result of pwm_disable is not just that the modulation > stops but also that the output signal goes low, although that's not very > explicit in the current pwm documentation.. The exynos PWM driver will > need another fix tweak to make that true. > > > >> Best Regards, > > > >> -- >> Markus Reichl >> >> On 8 April 2015 at 23:19, Anand Moon <linux.amoon@gmail.com> wrote: >> > Hi Guenter, >> > >> > Sorry my blunder mistake. Sorry for the noise. >> > >> > I just tested with spiking this patch and my observation and testing >> > were wrong we can skip this patch. >> > >> > I will send an v2 patch series removing the patch 5 and patch 6. >> > >> > With correct dts changes. >> > >> > Thanks for pointing my mistake. >> > >> > -Anand Moon >> > >> > On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote: >> >>> Hi Guenter, >> >>> >> >>> Initially the board bootup the cooling level state is 0. >> >>> So update the duty cycle and this power off the fan. >> >>> As their is no state change the fan will not spin. >> >>> >> >>> Once the temperature sensor is reached to alert temperature it changes state. >> >>> With the state change the fan cools the CPU and then stop's >> >>> >> >>> I have observed this state change with tmon utility in linux/tools/thermal/tmon/ >> >>> >> >> Sorry, I am missing something. I still don't see what problem you are fixing >> >> with this patch. What behavior is wrong with the current code, and how does your >> >> patch fix it ? >> >> >> >> Guenter >> >> >> >>> -Anand Moon >> >>> >> >>> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote: >> >>> > 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/10/2015 05:59 AM, Anand Moon wrote: > Hi Sjoerd, > > I don't much advance knowledge on internal signaling of pwm-samsung module. > > So do I need to send this patch again ? > From the context, it seems that the fix in hwmon would only paint over a problem in the actual pwm driver, correct ? If you resubmit the patch I would expect you to explain this in the commit log. Guenter > -Anand Moon > > > On 10 April 2015 at 17:30, Sjoerd Simons <sjoerd.simons@collabora.co.uk> wrote: >> Hey Anand, >> >> On Fri, 2015-04-10 at 16:58 +0530, Anand Moon wrote: >>> Hi Guenter/Lukasz, >>> >>> Earlier I send v2 version of the patch spiking this one. >>> >>> Markus Riechl came back to me with below mail. >>> So This patch confirms fixes the bug. >>> >>> I will send v3 version of the patch. Earlier I was in delima about the bug. >>> >>> -Anand Moon >>> ------------------------------------------- >>> Hi Anand, >>> >>> I tested your patch. >>> >>> After booting the fan is spinning despite only 44°C. >>> >>> /sys/class/thermal/cooling_device0/curstate is 0. >>> /sys/class/hwmon/hwmon4/pwm1 is 0 >>> >>> when I echo 1 > cur_state and then echo 0 > cur_state again, >>> the fan switches to off and behaves as expected. >>> >>> It looks like there is a bug in initializing the pwm output >>> immediately after booting. >> >> The problem here will be that at boot the PWM runs at full duty. With >> the current exynos PWM drive if you disable the PWM it will stop pulsing >> but remain high if it was at 100% duty. My patch on which you depend >> upon fixed a race where disabling the pwm right after changing the duty >> cycle (e.g. to 0%) also kept the signal high. >> >> From looking at other PWM users at the time it seemed that most if not >> all always first set to duty to 0% and then disable the pwm. Which >> should work fine on exynos now. However iirc Thierry recently clarified >> that the expected result of pwm_disable is not just that the modulation >> stops but also that the output signal goes low, although that's not very >> explicit in the current pwm documentation.. The exynos PWM driver will >> need another fix tweak to make that true. >> >> >> >>> Best Regards, >> >> >> >>> -- >>> Markus Reichl >>> >>> On 8 April 2015 at 23:19, Anand Moon <linux.amoon@gmail.com> wrote: >>>> Hi Guenter, >>>> >>>> Sorry my blunder mistake. Sorry for the noise. >>>> >>>> I just tested with spiking this patch and my observation and testing >>>> were wrong we can skip this patch. >>>> >>>> I will send an v2 patch series removing the patch 5 and patch 6. >>>> >>>> With correct dts changes. >>>> >>>> Thanks for pointing my mistake. >>>> >>>> -Anand Moon >>>> >>>> On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote: >>>>> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote: >>>>>> Hi Guenter, >>>>>> >>>>>> Initially the board bootup the cooling level state is 0. >>>>>> So update the duty cycle and this power off the fan. >>>>>> As their is no state change the fan will not spin. >>>>>> >>>>>> Once the temperature sensor is reached to alert temperature it changes state. >>>>>> With the state change the fan cools the CPU and then stop's >>>>>> >>>>>> I have observed this state change with tmon utility in linux/tools/thermal/tmon/ >>>>>> >>>>> Sorry, I am missing something. I still don't see what problem you are fixing >>>>> with this patch. What behavior is wrong with the current code, and how does your >>>>> patch fix it ? >>>>> >>>>> Guenter >>>>> >>>>>> -Anand Moon >>>>>> >>>>>> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote: >>>>>>> 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 >> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guenter, I will do so. -Anand Moon On 10 April 2015 at 18:39, Guenter Roeck <linux@roeck-us.net> wrote: > On 04/10/2015 05:59 AM, Anand Moon wrote: >> >> Hi Sjoerd, >> >> I don't much advance knowledge on internal signaling of pwm-samsung >> module. >> >> So do I need to send this patch again ? >> > > From the context, it seems that the fix in hwmon would only paint > over a problem in the actual pwm driver, correct ? > > If you resubmit the patch I would expect you to explain this in the > commit log. > > Guenter > > >> -Anand Moon >> >> >> On 10 April 2015 at 17:30, Sjoerd Simons <sjoerd.simons@collabora.co.uk> >> wrote: >>> >>> Hey Anand, >>> >>> On Fri, 2015-04-10 at 16:58 +0530, Anand Moon wrote: >>>> >>>> Hi Guenter/Lukasz, >>>> >>>> Earlier I send v2 version of the patch spiking this one. >>>> >>>> Markus Riechl came back to me with below mail. >>>> So This patch confirms fixes the bug. >>>> >>>> I will send v3 version of the patch. Earlier I was in delima about the >>>> bug. >>>> >>>> -Anand Moon >>>> ------------------------------------------- >>>> Hi Anand, >>>> >>>> I tested your patch. >>>> >>>> After booting the fan is spinning despite only 44°C. >>>> >>>> /sys/class/thermal/cooling_device0/curstate is 0. >>>> /sys/class/hwmon/hwmon4/pwm1 is 0 >>>> >>>> when I echo 1 > cur_state and then echo 0 > cur_state again, >>>> the fan switches to off and behaves as expected. >>>> >>>> It looks like there is a bug in initializing the pwm output >>>> immediately after booting. >>> >>> >>> The problem here will be that at boot the PWM runs at full duty. With >>> the current exynos PWM drive if you disable the PWM it will stop pulsing >>> but remain high if it was at 100% duty. My patch on which you depend >>> upon fixed a race where disabling the pwm right after changing the duty >>> cycle (e.g. to 0%) also kept the signal high. >>> >>> From looking at other PWM users at the time it seemed that most if not >>> all always first set to duty to 0% and then disable the pwm. Which >>> should work fine on exynos now. However iirc Thierry recently clarified >>> that the expected result of pwm_disable is not just that the modulation >>> stops but also that the output signal goes low, although that's not very >>> explicit in the current pwm documentation.. The exynos PWM driver will >>> need another fix tweak to make that true. >>> >>> >>> >>>> Best Regards, >>> >>> >>> >>> >>>> -- >>>> Markus Reichl >>>> >>>> On 8 April 2015 at 23:19, Anand Moon <linux.amoon@gmail.com> wrote: >>>>> >>>>> Hi Guenter, >>>>> >>>>> Sorry my blunder mistake. Sorry for the noise. >>>>> >>>>> I just tested with spiking this patch and my observation and testing >>>>> were wrong we can skip this patch. >>>>> >>>>> I will send an v2 patch series removing the patch 5 and patch 6. >>>>> >>>>> With correct dts changes. >>>>> >>>>> Thanks for pointing my mistake. >>>>> >>>>> -Anand Moon >>>>> >>>>> On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote: >>>>>> >>>>>> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote: >>>>>>> >>>>>>> Hi Guenter, >>>>>>> >>>>>>> Initially the board bootup the cooling level state is 0. >>>>>>> So update the duty cycle and this power off the fan. >>>>>>> As their is no state change the fan will not spin. >>>>>>> >>>>>>> Once the temperature sensor is reached to alert temperature it >>>>>>> changes state. >>>>>>> With the state change the fan cools the CPU and then stop's >>>>>>> >>>>>>> I have observed this state change with tmon utility in >>>>>>> linux/tools/thermal/tmon/ >>>>>>> >>>>>> Sorry, I am missing something. I still don't see what problem you are >>>>>> fixing >>>>>> with this patch. What behavior is wrong with the current code, and how >>>>>> does your >>>>>> patch fix it ? >>>>>> >>>>>> Guenter >>>>>> >>>>>>> -Anand Moon >>>>>>> >>>>>>> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote: >>>>>>>> >>>>>>>> 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 >>> >>> >>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2015-04-10 at 06:09 -0700, Guenter Roeck wrote: > On 04/10/2015 05:59 AM, Anand Moon wrote: > > Hi Sjoerd, > > > > I don't much advance knowledge on internal signaling of pwm-samsung module. > > > > So do I need to send this patch again ? > > > > From the context, it seems that the fix in hwmon would only paint > over a problem in the actual pwm driver, correct ? Yes/no/maybe :). Imho this is something to clarify in the pwm API documentation. As currently all it says is: "pwm_disable - stop a PWM output toggling", Which is what the exynos driver does. Thierry, could you clearify what the intention is here? I'm happy to prepare a pwm driver patch if needed to solve this? > If you resubmit the patch I would expect you to explain this in the > commit log. > > Guenter > > > -Anand Moon > > > > > > On 10 April 2015 at 17:30, Sjoerd Simons <sjoerd.simons@collabora.co.uk> wrote: > >> Hey Anand, > >> > >> On Fri, 2015-04-10 at 16:58 +0530, Anand Moon wrote: > >>> Hi Guenter/Lukasz, > >>> > >>> Earlier I send v2 version of the patch spiking this one. > >>> > >>> Markus Riechl came back to me with below mail. > >>> So This patch confirms fixes the bug. > >>> > >>> I will send v3 version of the patch. Earlier I was in delima about the bug. > >>> > >>> -Anand Moon > >>> ------------------------------------------- > >>> Hi Anand, > >>> > >>> I tested your patch. > >>> > >>> After booting the fan is spinning despite only 44°C. > >>> > >>> /sys/class/thermal/cooling_device0/curstate is 0. > >>> /sys/class/hwmon/hwmon4/pwm1 is 0 > >>> > >>> when I echo 1 > cur_state and then echo 0 > cur_state again, > >>> the fan switches to off and behaves as expected. > >>> > >>> It looks like there is a bug in initializing the pwm output > >>> immediately after booting. > >> > >> The problem here will be that at boot the PWM runs at full duty. With > >> the current exynos PWM drive if you disable the PWM it will stop pulsing > >> but remain high if it was at 100% duty. My patch on which you depend > >> upon fixed a race where disabling the pwm right after changing the duty > >> cycle (e.g. to 0%) also kept the signal high. > >> > >> From looking at other PWM users at the time it seemed that most if not > >> all always first set to duty to 0% and then disable the pwm. Which > >> should work fine on exynos now. However iirc Thierry recently clarified > >> that the expected result of pwm_disable is not just that the modulation > >> stops but also that the output signal goes low, although that's not very > >> explicit in the current pwm documentation.. The exynos PWM driver will > >> need another fix tweak to make that true. > >> > >> > >> > >>> Best Regards, > >> > >> > >> > >>> -- > >>> Markus Reichl > >>> > >>> On 8 April 2015 at 23:19, Anand Moon <linux.amoon@gmail.com> wrote: > >>>> Hi Guenter, > >>>> > >>>> Sorry my blunder mistake. Sorry for the noise. > >>>> > >>>> I just tested with spiking this patch and my observation and testing > >>>> were wrong we can skip this patch. > >>>> > >>>> I will send an v2 patch series removing the patch 5 and patch 6. > >>>> > >>>> With correct dts changes. > >>>> > >>>> Thanks for pointing my mistake. > >>>> > >>>> -Anand Moon > >>>> > >>>> On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote: > >>>>> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote: > >>>>>> Hi Guenter, > >>>>>> > >>>>>> Initially the board bootup the cooling level state is 0. > >>>>>> So update the duty cycle and this power off the fan. > >>>>>> As their is no state change the fan will not spin. > >>>>>> > >>>>>> Once the temperature sensor is reached to alert temperature it changes state. > >>>>>> With the state change the fan cools the CPU and then stop's > >>>>>> > >>>>>> I have observed this state change with tmon utility in linux/tools/thermal/tmon/ > >>>>>> > >>>>> Sorry, I am missing something. I still don't see what problem you are fixing > >>>>> with this patch. What behavior is wrong with the current code, and how does your > >>>>> patch fix it ? > >>>>> > >>>>> Guenter > >>>>> > >>>>>> -Anand Moon > >>>>>> > >>>>>> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote: > >>>>>>> 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 > >> > >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 10, 2015 at 03:30:01PM +0200, Sjoerd Simons wrote: > On Fri, 2015-04-10 at 06:09 -0700, Guenter Roeck wrote: > > On 04/10/2015 05:59 AM, Anand Moon wrote: > > > Hi Sjoerd, > > > > > > I don't much advance knowledge on internal signaling of pwm-samsung module. > > > > > > So do I need to send this patch again ? > > > > > > > From the context, it seems that the fix in hwmon would only paint > > over a problem in the actual pwm driver, correct ? > > Yes/no/maybe :). Imho this is something to clarify in the pwm API > documentation. As currently all it says is: > "pwm_disable - stop a PWM output toggling", > > Which is what the exynos driver does. > > Thierry, could you clearify what the intention is here? I'm happy to > prepare a pwm driver patch if needed to solve this? I think the safest thing to do is for users to do both. You call pwm_config() with a zero duty cycle to make it clear what the status is that you want. Then you call pwm_disable() to state that you don't need the output signal anymore, so that any clocks needed by the PWM can be stopped. Doing so gives the driver the most information and should make the user more resilient against any possible quirks in drivers. Drivers that on pwm_disable() still invert the signal should still be fixed, though. In the cases that I'm aware of the sequence of setting the duty cycle to 0 and calling pwm_disable() won't actually paper over any bugs. It's precisely that sequence that triggers the bug. Merely calling pwm_disable() should trigger it as well, pwm_config(0) without pwm_disable() would hide it. Thierry > > If you resubmit the patch I would expect you to explain this in the > > commit log. > > > > Guenter > > > > > -Anand Moon > > > > > > > > > On 10 April 2015 at 17:30, Sjoerd Simons <sjoerd.simons@collabora.co.uk> wrote: > > >> Hey Anand, > > >> > > >> On Fri, 2015-04-10 at 16:58 +0530, Anand Moon wrote: > > >>> Hi Guenter/Lukasz, > > >>> > > >>> Earlier I send v2 version of the patch spiking this one. > > >>> > > >>> Markus Riechl came back to me with below mail. > > >>> So This patch confirms fixes the bug. > > >>> > > >>> I will send v3 version of the patch. Earlier I was in delima about the bug. > > >>> > > >>> -Anand Moon > > >>> ------------------------------------------- > > >>> Hi Anand, > > >>> > > >>> I tested your patch. > > >>> > > >>> After booting the fan is spinning despite only 44°C. > > >>> > > >>> /sys/class/thermal/cooling_device0/curstate is 0. > > >>> /sys/class/hwmon/hwmon4/pwm1 is 0 > > >>> > > >>> when I echo 1 > cur_state and then echo 0 > cur_state again, > > >>> the fan switches to off and behaves as expected. > > >>> > > >>> It looks like there is a bug in initializing the pwm output > > >>> immediately after booting. > > >> > > >> The problem here will be that at boot the PWM runs at full duty. With > > >> the current exynos PWM drive if you disable the PWM it will stop pulsing > > >> but remain high if it was at 100% duty. My patch on which you depend > > >> upon fixed a race where disabling the pwm right after changing the duty > > >> cycle (e.g. to 0%) also kept the signal high. > > >> > > >> From looking at other PWM users at the time it seemed that most if not > > >> all always first set to duty to 0% and then disable the pwm. Which > > >> should work fine on exynos now. However iirc Thierry recently clarified > > >> that the expected result of pwm_disable is not just that the modulation > > >> stops but also that the output signal goes low, although that's not very > > >> explicit in the current pwm documentation.. The exynos PWM driver will > > >> need another fix tweak to make that true. > > >> > > >> > > >> > > >>> Best Regards, > > >> > > >> > > >> > > >>> -- > > >>> Markus Reichl > > >>> > > >>> On 8 April 2015 at 23:19, Anand Moon <linux.amoon@gmail.com> wrote: > > >>>> Hi Guenter, > > >>>> > > >>>> Sorry my blunder mistake. Sorry for the noise. > > >>>> > > >>>> I just tested with spiking this patch and my observation and testing > > >>>> were wrong we can skip this patch. > > >>>> > > >>>> I will send an v2 patch series removing the patch 5 and patch 6. > > >>>> > > >>>> With correct dts changes. > > >>>> > > >>>> Thanks for pointing my mistake. > > >>>> > > >>>> -Anand Moon > > >>>> > > >>>> On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote: > > >>>>> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote: > > >>>>>> Hi Guenter, > > >>>>>> > > >>>>>> Initially the board bootup the cooling level state is 0. > > >>>>>> So update the duty cycle and this power off the fan. > > >>>>>> As their is no state change the fan will not spin. > > >>>>>> > > >>>>>> Once the temperature sensor is reached to alert temperature it changes state. > > >>>>>> With the state change the fan cools the CPU and then stop's > > >>>>>> > > >>>>>> I have observed this state change with tmon utility in linux/tools/thermal/tmon/ > > >>>>>> > > >>>>> Sorry, I am missing something. I still don't see what problem you are fixing > > >>>>> with this patch. What behavior is wrong with the current code, and how does your > > >>>>> patch fix it ? > > >>>>> > > >>>>> Guenter > > >>>>> > > >>>>>> -Anand Moon > > >>>>>> > > >>>>>> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote: > > >>>>>>> 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 > > >> > > >> > > > > > > >
Thierry Reding <thierry.reding@gmail.com> writes: > On Fri, Apr 10, 2015 at 03:30:01PM +0200, Sjoerd Simons wrote: >> >> Yes/no/maybe :). Imho this is something to clarify in the pwm API >> documentation. As currently all it says is: >> "pwm_disable - stop a PWM output toggling", >> >> Which is what the exynos driver does. >> >> Thierry, could you clearify what the intention is here? I'm happy to >> prepare a pwm driver patch if needed to solve this? > > I think the safest thing to do is for users to do both. You call > pwm_config() with a zero duty cycle to make it clear what the status is > that you want. Then you call pwm_disable() to state that you don't need > the output signal anymore, so that any clocks needed by the PWM can be > stopped. Doing so gives the driver the most information and should make > the user more resilient against any possible quirks in drivers. > It would be great if the documentation were more clear on this matter regardless. This is something I can imagine having to spend substantial amounts of time Googling whereas a simple note in the documentation would have removed all ambiguity. Cheers, - Ben
On Fri, Apr 10, 2015 at 01:25:52PM -0400, Ben Gamari wrote: > Thierry Reding <thierry.reding@gmail.com> writes: > > > On Fri, Apr 10, 2015 at 03:30:01PM +0200, Sjoerd Simons wrote: > >> > >> Yes/no/maybe :). Imho this is something to clarify in the pwm API > >> documentation. As currently all it says is: > >> "pwm_disable - stop a PWM output toggling", > >> > >> Which is what the exynos driver does. > >> > >> Thierry, could you clearify what the intention is here? I'm happy to > >> prepare a pwm driver patch if needed to solve this? > > > > I think the safest thing to do is for users to do both. You call > > pwm_config() with a zero duty cycle to make it clear what the status is > > that you want. Then you call pwm_disable() to state that you don't need > > the output signal anymore, so that any clocks needed by the PWM can be > > stopped. Doing so gives the driver the most information and should make > > the user more resilient against any possible quirks in drivers. > > > It would be great if the documentation were more clear on this matter > regardless. This is something I can imagine having to spend substantial > amounts of time Googling whereas a simple note in the documentation would > have removed all ambiguity. > Especially since, in this case, the output signal _is_ still needed. It appears that pwm_disable() is only expected to stop the clock, not the signal itself. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + 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);
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. 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(-)