Hi Guenter, I will do so. -Anand Moon On 10 April 2015 at 18:39, Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> >> 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@xxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxx> >>>>>>>>>> --- >>>>>>>>>> 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@xxxxxxxxxxx> >>>>>>>>> >>>>>>>>> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |