* [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator @ 2022-05-04 12:45 Alexander Stein 2022-05-05 22:00 ` Guenter Roeck 0 siblings, 1 reply; 15+ messages in thread From: Alexander Stein @ 2022-05-04 12:45 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz, Jean Delvare, Guenter Roeck, Thierry Reding, Uwe Kleine-König, Lee Jones Cc: Markus Niebel, linux-hwmon, linux-pwm, Alexander Stein From: Markus Niebel <Markus.Niebel@ew.tq-group.com> A pwm value equal to zero is meant to switch off the pwm hence also switching off the fan. Currently the optional regulator is always on. When using this driver on boards with an inverted pwm signal polarity this can cause running the fan at maximum speed when setting pwm to zero. The proposed changes switch the regulator off, when PWM is currently enabled but pwm is requested to set to zero and switch der regulator on, when PWM is currently disabled but pwm shall be set to a no zero value. Add __set_pwm_and_regulator and rewrite __set_pwm to handle regulator switching for the following conditions: - probe: pwm duty -> max, pwm state is unkwown: use __set_pwm and enable regulator separately to keep the devm action - off: new pwm duty is zero, pwm currently enabled: disable regulator - on: new pwm duty is non zero, pwm currently disabled: enable regulator Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> --- Changes in v2: * Added my own missing S-o-b drivers/hwmon/pwm-fan.c | 144 ++++++++++++++++++++++++++-------------- 1 file changed, 93 insertions(+), 51 deletions(-) diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index f12b9a28a232..b47d59fbe836 100644 --- a/drivers/hwmon/pwm-fan.c +++ b/drivers/hwmon/pwm-fan.c @@ -97,18 +97,50 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm) unsigned long period; int ret = 0; struct pwm_state *state = &ctx->pwm_state; + /* track changes of reg_en for error handling */ + enum regulator_change { + untouched, + enabled, + disabled, + } reg_change = untouched; mutex_lock(&ctx->lock); + if (ctx->pwm_value == pwm) goto exit_set_pwm_err; + if (ctx->reg_en) { + if (pwm && !state->enabled) { + reg_change = enabled; + ret = regulator_enable(ctx->reg_en); + } else if (!pwm && state->enabled) { + reg_change = disabled; + ret = regulator_disable(ctx->reg_en); + } + if (ret) + goto exit_set_pwm_err; + } + period = state->period; state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM); state->enabled = pwm ? true : false; ret = pwm_apply_state(ctx->pwm, state); - if (!ret) + if (!ret) { ctx->pwm_value = pwm; + } else if (reg_change != untouched) { + /* + * revert regulator changes to keep consistency between + * pwm and regulator + */ + int err; + + if (reg_change == enabled) + err = regulator_disable(ctx->reg_en); + else if (reg_change == disabled) + err = regulator_enable(ctx->reg_en); + } + exit_set_pwm_err: mutex_unlock(&ctx->lock); return ret; @@ -280,18 +312,50 @@ static int pwm_fan_of_get_cooling_data(struct device *dev, return 0; } -static void pwm_fan_regulator_disable(void *data) +/* + * disable fan and regulator + * if cleanup is true, disable pwm regardless of regulator disable result + * this makes the function dual use for unloading driver and suspend + */ + +static int __pwm_fan_disable_or_cleanup(struct pwm_fan_ctx *ctx, bool cleanup) { - regulator_disable(data); + int ret; + + if (ctx->pwm_value) { + /* keep ctx->pwm_state unmodified for pwm_fan_resume() */ + struct pwm_state state = ctx->pwm_state; + + /* regulator is only enabled if pwm_value is not zero */ + if (ctx->pwm_value && ctx->reg_en) { + ret = regulator_disable(ctx->reg_en); + if (ret) { + pr_err("Failed to disable fan supply: %d\n", ret); + if (!cleanup) + return ret; + } + } + + state.duty_cycle = 0; + state.enabled = false; + ret = pwm_apply_state(ctx->pwm, &state); + } + + return ret; } -static void pwm_fan_pwm_disable(void *__ctx) +static void pwm_fan_cleanup(void *__ctx) { struct pwm_fan_ctx *ctx = __ctx; - - ctx->pwm_state.enabled = false; - pwm_apply_state(ctx->pwm, &ctx->pwm_state); del_timer_sync(&ctx->rpm_timer); + __pwm_fan_disable_or_cleanup(ctx, true); +} + +static int pwm_fan_disable(void *__ctx) +{ + struct pwm_fan_ctx *ctx = __ctx; + + return __pwm_fan_disable_or_cleanup(ctx, false); } static int pwm_fan_probe(struct platform_device *pdev) @@ -324,19 +388,14 @@ static int pwm_fan_probe(struct platform_device *pdev) return PTR_ERR(ctx->reg_en); ctx->reg_en = NULL; - } else { - ret = regulator_enable(ctx->reg_en); - if (ret) { - dev_err(dev, "Failed to enable fan supply: %d\n", ret); - return ret; - } - ret = devm_add_action_or_reset(dev, pwm_fan_regulator_disable, - ctx->reg_en); - if (ret) - return ret; } pwm_init_state(ctx->pwm, &ctx->pwm_state); + /* + * Ensure the PWM is switched on (including the regulator), + * independently from any previous PWM state + */ + ctx->pwm_state.enabled = false; /* * __set_pwm assumes that MAX_PWM * (period - 1) fits into an unsigned @@ -348,14 +407,17 @@ static int pwm_fan_probe(struct platform_device *pdev) return -EINVAL; } - /* Set duty cycle to maximum allowed and enable PWM output */ + /* + * Set duty cycle to maximum allowed and enable PWM output as well as + * the regulator. In case of error nothing is changed + */ ret = __set_pwm(ctx, MAX_PWM); if (ret) { dev_err(dev, "Failed to configure PWM: %d\n", ret); return ret; } timer_setup(&ctx->rpm_timer, sample_timer, 0); - ret = devm_add_action_or_reset(dev, pwm_fan_pwm_disable, ctx); + ret = devm_add_action_or_reset(dev, pwm_fan_cleanup, ctx); if (ret) return ret; @@ -461,42 +523,22 @@ static int pwm_fan_probe(struct platform_device *pdev) return 0; } -static int pwm_fan_disable(struct device *dev) -{ - struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); - int ret; - - if (ctx->pwm_value) { - /* keep ctx->pwm_state unmodified for pwm_fan_resume() */ - struct pwm_state state = ctx->pwm_state; - - state.duty_cycle = 0; - state.enabled = false; - ret = pwm_apply_state(ctx->pwm, &state); - if (ret < 0) - return ret; - } - - if (ctx->reg_en) { - ret = regulator_disable(ctx->reg_en); - if (ret) { - dev_err(dev, "Failed to disable fan supply: %d\n", ret); - return ret; - } - } - - return 0; -} - static void pwm_fan_shutdown(struct platform_device *pdev) { - pwm_fan_disable(&pdev->dev); + struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev); + + pwm_fan_cleanup(ctx); } #ifdef CONFIG_PM_SLEEP static int pwm_fan_suspend(struct device *dev) { - return pwm_fan_disable(dev); + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); + int ret; + + ret = pwm_fan_disable(ctx); + + return ret; } static int pwm_fan_resume(struct device *dev) @@ -504,6 +546,9 @@ static int pwm_fan_resume(struct device *dev) struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); int ret; + if (ctx->pwm_value == 0) + return 0; + if (ctx->reg_en) { ret = regulator_enable(ctx->reg_en); if (ret) { @@ -512,9 +557,6 @@ static int pwm_fan_resume(struct device *dev) } } - if (ctx->pwm_value == 0) - return 0; - return pwm_apply_state(ctx->pwm, &ctx->pwm_state); } #endif -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator 2022-05-04 12:45 [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator Alexander Stein @ 2022-05-05 22:00 ` Guenter Roeck 2022-05-06 7:15 ` (EXT) " Markus Niebel 2022-05-06 7:15 ` Alexander Stein 0 siblings, 2 replies; 15+ messages in thread From: Guenter Roeck @ 2022-05-05 22:00 UTC (permalink / raw) To: Alexander Stein Cc: Bartlomiej Zolnierkiewicz, Jean Delvare, Thierry Reding, Uwe Kleine-König, Lee Jones, Markus Niebel, linux-hwmon, linux-pwm On Wed, May 04, 2022 at 02:45:51PM +0200, Alexander Stein wrote: > From: Markus Niebel <Markus.Niebel@ew.tq-group.com> > > A pwm value equal to zero is meant to switch off the pwm > hence also switching off the fan. Currently the optional > regulator is always on. When using this driver on boards > with an inverted pwm signal polarity this can cause running > the fan at maximum speed when setting pwm to zero. > The appropriate solution in this case would be to tell the software that the pwm is inverted. Turning off the regulator in that situation is a bad idea since setting the pwm value to 1 would set it to almost full speed. That does not really make sense. Guenter > The proposed changes switch the regulator off, when PWM is > currently enabled but pwm is requested to set to zero > and switch der regulator on, when PWM is currently disabled > but pwm shall be set to a no zero value. > > Add __set_pwm_and_regulator and rewrite __set_pwm to > handle regulator switching for the following conditions: > > - probe: pwm duty -> max, pwm state is unkwown: use __set_pwm > and enable regulator separately to keep the devm action > - off: new pwm duty is zero, pwm currently enabled: disable > regulator > - on: new pwm duty is non zero, pwm currently disabled: enable > regulator > > Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > --- > Changes in v2: > * Added my own missing S-o-b > > drivers/hwmon/pwm-fan.c | 144 ++++++++++++++++++++++++++-------------- > 1 file changed, 93 insertions(+), 51 deletions(-) > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > index f12b9a28a232..b47d59fbe836 100644 > --- a/drivers/hwmon/pwm-fan.c > +++ b/drivers/hwmon/pwm-fan.c > @@ -97,18 +97,50 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm) > unsigned long period; > int ret = 0; > struct pwm_state *state = &ctx->pwm_state; > + /* track changes of reg_en for error handling */ > + enum regulator_change { > + untouched, > + enabled, > + disabled, > + } reg_change = untouched; > > mutex_lock(&ctx->lock); > + > if (ctx->pwm_value == pwm) > goto exit_set_pwm_err; > > + if (ctx->reg_en) { > + if (pwm && !state->enabled) { > + reg_change = enabled; > + ret = regulator_enable(ctx->reg_en); > + } else if (!pwm && state->enabled) { > + reg_change = disabled; > + ret = regulator_disable(ctx->reg_en); > + } > + if (ret) > + goto exit_set_pwm_err; > + } > + > period = state->period; > state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM); > state->enabled = pwm ? true : false; > > ret = pwm_apply_state(ctx->pwm, state); > - if (!ret) > + if (!ret) { > ctx->pwm_value = pwm; > + } else if (reg_change != untouched) { > + /* > + * revert regulator changes to keep consistency between > + * pwm and regulator > + */ > + int err; > + > + if (reg_change == enabled) > + err = regulator_disable(ctx->reg_en); > + else if (reg_change == disabled) > + err = regulator_enable(ctx->reg_en); > + } > + > exit_set_pwm_err: > mutex_unlock(&ctx->lock); > return ret; > @@ -280,18 +312,50 @@ static int pwm_fan_of_get_cooling_data(struct device *dev, > return 0; > } > > -static void pwm_fan_regulator_disable(void *data) > +/* > + * disable fan and regulator > + * if cleanup is true, disable pwm regardless of regulator disable result > + * this makes the function dual use for unloading driver and suspend > + */ > + > +static int __pwm_fan_disable_or_cleanup(struct pwm_fan_ctx *ctx, bool cleanup) > { > - regulator_disable(data); > + int ret; > + > + if (ctx->pwm_value) { > + /* keep ctx->pwm_state unmodified for pwm_fan_resume() */ > + struct pwm_state state = ctx->pwm_state; > + > + /* regulator is only enabled if pwm_value is not zero */ > + if (ctx->pwm_value && ctx->reg_en) { > + ret = regulator_disable(ctx->reg_en); > + if (ret) { > + pr_err("Failed to disable fan supply: %d\n", ret); > + if (!cleanup) > + return ret; > + } > + } > + > + state.duty_cycle = 0; > + state.enabled = false; > + ret = pwm_apply_state(ctx->pwm, &state); > + } > + > + return ret; > } > > -static void pwm_fan_pwm_disable(void *__ctx) > +static void pwm_fan_cleanup(void *__ctx) > { > struct pwm_fan_ctx *ctx = __ctx; > - > - ctx->pwm_state.enabled = false; > - pwm_apply_state(ctx->pwm, &ctx->pwm_state); > del_timer_sync(&ctx->rpm_timer); > + __pwm_fan_disable_or_cleanup(ctx, true); > +} > + > +static int pwm_fan_disable(void *__ctx) > +{ > + struct pwm_fan_ctx *ctx = __ctx; > + > + return __pwm_fan_disable_or_cleanup(ctx, false); > } > > static int pwm_fan_probe(struct platform_device *pdev) > @@ -324,19 +388,14 @@ static int pwm_fan_probe(struct platform_device *pdev) > return PTR_ERR(ctx->reg_en); > > ctx->reg_en = NULL; > - } else { > - ret = regulator_enable(ctx->reg_en); > - if (ret) { > - dev_err(dev, "Failed to enable fan supply: %d\n", ret); > - return ret; > - } > - ret = devm_add_action_or_reset(dev, pwm_fan_regulator_disable, > - ctx->reg_en); > - if (ret) > - return ret; > } > > pwm_init_state(ctx->pwm, &ctx->pwm_state); > + /* > + * Ensure the PWM is switched on (including the regulator), > + * independently from any previous PWM state > + */ > + ctx->pwm_state.enabled = false; > > /* > * __set_pwm assumes that MAX_PWM * (period - 1) fits into an unsigned > @@ -348,14 +407,17 @@ static int pwm_fan_probe(struct platform_device *pdev) > return -EINVAL; > } > > - /* Set duty cycle to maximum allowed and enable PWM output */ > + /* > + * Set duty cycle to maximum allowed and enable PWM output as well as > + * the regulator. In case of error nothing is changed > + */ > ret = __set_pwm(ctx, MAX_PWM); > if (ret) { > dev_err(dev, "Failed to configure PWM: %d\n", ret); > return ret; > } > timer_setup(&ctx->rpm_timer, sample_timer, 0); > - ret = devm_add_action_or_reset(dev, pwm_fan_pwm_disable, ctx); > + ret = devm_add_action_or_reset(dev, pwm_fan_cleanup, ctx); > if (ret) > return ret; > > @@ -461,42 +523,22 @@ static int pwm_fan_probe(struct platform_device *pdev) > return 0; > } > > -static int pwm_fan_disable(struct device *dev) > -{ > - struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); > - int ret; > - > - if (ctx->pwm_value) { > - /* keep ctx->pwm_state unmodified for pwm_fan_resume() */ > - struct pwm_state state = ctx->pwm_state; > - > - state.duty_cycle = 0; > - state.enabled = false; > - ret = pwm_apply_state(ctx->pwm, &state); > - if (ret < 0) > - return ret; > - } > - > - if (ctx->reg_en) { > - ret = regulator_disable(ctx->reg_en); > - if (ret) { > - dev_err(dev, "Failed to disable fan supply: %d\n", ret); > - return ret; > - } > - } > - > - return 0; > -} > - > static void pwm_fan_shutdown(struct platform_device *pdev) > { > - pwm_fan_disable(&pdev->dev); > + struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev); > + > + pwm_fan_cleanup(ctx); > } > > #ifdef CONFIG_PM_SLEEP > static int pwm_fan_suspend(struct device *dev) > { > - return pwm_fan_disable(dev); > + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); > + int ret; > + > + ret = pwm_fan_disable(ctx); > + > + return ret; > } > > static int pwm_fan_resume(struct device *dev) > @@ -504,6 +546,9 @@ static int pwm_fan_resume(struct device *dev) > struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); > int ret; > > + if (ctx->pwm_value == 0) > + return 0; > + > if (ctx->reg_en) { > ret = regulator_enable(ctx->reg_en); > if (ret) { > @@ -512,9 +557,6 @@ static int pwm_fan_resume(struct device *dev) > } > } > > - if (ctx->pwm_value == 0) > - return 0; > - > return pwm_apply_state(ctx->pwm, &ctx->pwm_state); > } > #endif > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator 2022-05-05 22:00 ` Guenter Roeck @ 2022-05-06 7:15 ` Markus Niebel 2022-05-06 7:15 ` Alexander Stein 1 sibling, 0 replies; 15+ messages in thread From: Markus Niebel @ 2022-05-06 7:15 UTC (permalink / raw) To: Guenter Roeck, Alexander Stein Cc: Bartlomiej Zolnierkiewicz, Jean Delvare, Thierry Reding, Uwe Kleine-König, Lee Jones, linux-hwmon, linux-pwm Am Donnerstag, den 05.05.2022, 15:00 -0700 schrieb Guenter Roeck: > On Wed, May 04, 2022 at 02:45:51PM +0200, Alexander Stein wrote: > > From: Markus Niebel <Markus.Niebel@ew.tq-group.com> > > > > A pwm value equal to zero is meant to switch off the pwm > > hence also switching off the fan. Currently the optional > > regulator is always on. When using this driver on boards > > with an inverted pwm signal polarity this can cause running > > the fan at maximum speed when setting pwm to zero. > > > > The appropriate solution in this case would be to tell the > software that the pwm is inverted. Turning off the regulator > in that situation is a bad idea since setting the pwm value to > 1 would set it to almost full speed. That does not really make > sense. > > Guenter > The regulator is inverted by dt settings in this case. This means, slowing down to 1 works as expected, but going to zero will speed up to max in our case (The inversion is due to the logic of the board hardware) The logic of switching off the regulator is the same as done in drivers/video/backlight/pwm_bl.c - when brightness is set to zero, the backlight is switched off. Markus > > The proposed changes switch the regulator off, when PWM is > > currently enabled but pwm is requested to set to zero > > and switch der regulator on, when PWM is currently disabled > > but pwm shall be set to a no zero value. > > > > Add __set_pwm_and_regulator and rewrite __set_pwm to > > handle regulator switching for the following conditions: > > > > - probe: pwm duty -> max, pwm state is unkwown: use __set_pwm > > and enable regulator separately to keep the devm action > > - off: new pwm duty is zero, pwm currently enabled: disable > > regulator > > - on: new pwm duty is non zero, pwm currently disabled: enable > > regulator > > > > Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > --- > > Changes in v2: > > * Added my own missing S-o-b > > > > drivers/hwmon/pwm-fan.c | 144 ++++++++++++++++++++++++++-------------- > > 1 file changed, 93 insertions(+), 51 deletions(-) > > > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > > index f12b9a28a232..b47d59fbe836 100644 > > --- a/drivers/hwmon/pwm-fan.c > > +++ b/drivers/hwmon/pwm-fan.c > > @@ -97,18 +97,50 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long > > pwm) > > unsigned long period; > > int ret = 0; > > struct pwm_state *state = &ctx->pwm_state; > > + /* track changes of reg_en for error handling */ > > + enum regulator_change { > > + untouched, > > + enabled, > > + disabled, > > + } reg_change = untouched; > > > > mutex_lock(&ctx->lock); > > + > > if (ctx->pwm_value == pwm) > > goto exit_set_pwm_err; > > > > + if (ctx->reg_en) { > > + if (pwm && !state->enabled) { > > + reg_change = enabled; > > + ret = regulator_enable(ctx->reg_en); > > + } else if (!pwm && state->enabled) { > > + reg_change = disabled; > > + ret = regulator_disable(ctx->reg_en); > > + } > > + if (ret) > > + goto exit_set_pwm_err; > > + } > > + > > period = state->period; > > state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM); > > state->enabled = pwm ? true : false; > > > > ret = pwm_apply_state(ctx->pwm, state); > > - if (!ret) > > + if (!ret) { > > ctx->pwm_value = pwm; > > + } else if (reg_change != untouched) { > > + /* > > + * revert regulator changes to keep consistency between > > + * pwm and regulator > > + */ > > + int err; > > + > > + if (reg_change == enabled) > > + err = regulator_disable(ctx->reg_en); > > + else if (reg_change == disabled) > > + err = regulator_enable(ctx->reg_en); > > + } > > + > > exit_set_pwm_err: > > mutex_unlock(&ctx->lock); > > return ret; > > @@ -280,18 +312,50 @@ static int pwm_fan_of_get_cooling_data(struct device *dev, > > return 0; > > } > > > > -static void pwm_fan_regulator_disable(void *data) > > +/* > > + * disable fan and regulator > > + * if cleanup is true, disable pwm regardless of regulator disable result > > + * this makes the function dual use for unloading driver and suspend > > + */ > > + > > +static int __pwm_fan_disable_or_cleanup(struct pwm_fan_ctx *ctx, bool cleanup) > > { > > - regulator_disable(data); > > + int ret; > > + > > + if (ctx->pwm_value) { > > + /* keep ctx->pwm_state unmodified for pwm_fan_resume() */ > > + struct pwm_state state = ctx->pwm_state; > > + > > + /* regulator is only enabled if pwm_value is not zero */ > > + if (ctx->pwm_value && ctx->reg_en) { > > + ret = regulator_disable(ctx->reg_en); > > + if (ret) { > > + pr_err("Failed to disable fan supply: %d\n", ret); > > + if (!cleanup) > > + return ret; > > + } > > + } > > + > > + state.duty_cycle = 0; > > + state.enabled = false; > > + ret = pwm_apply_state(ctx->pwm, &state); > > + } > > + > > + return ret; > > } > > > > -static void pwm_fan_pwm_disable(void *__ctx) > > +static void pwm_fan_cleanup(void *__ctx) > > { > > struct pwm_fan_ctx *ctx = __ctx; > > - > > - ctx->pwm_state.enabled = false; > > - pwm_apply_state(ctx->pwm, &ctx->pwm_state); > > del_timer_sync(&ctx->rpm_timer); > > + __pwm_fan_disable_or_cleanup(ctx, true); > > +} > > + > > +static int pwm_fan_disable(void *__ctx) > > +{ > > + struct pwm_fan_ctx *ctx = __ctx; > > + > > + return __pwm_fan_disable_or_cleanup(ctx, false); > > } > > > > static int pwm_fan_probe(struct platform_device *pdev) > > @@ -324,19 +388,14 @@ static int pwm_fan_probe(struct platform_device *pdev) > > return PTR_ERR(ctx->reg_en); > > > > ctx->reg_en = NULL; > > - } else { > > - ret = regulator_enable(ctx->reg_en); > > - if (ret) { > > - dev_err(dev, "Failed to enable fan supply: %d\n", ret); > > - return ret; > > - } > > - ret = devm_add_action_or_reset(dev, pwm_fan_regulator_disable, > > - ctx->reg_en); > > - if (ret) > > - return ret; > > } > > > > pwm_init_state(ctx->pwm, &ctx->pwm_state); > > + /* > > + * Ensure the PWM is switched on (including the regulator), > > + * independently from any previous PWM state > > + */ > > + ctx->pwm_state.enabled = false; > > > > /* > > * __set_pwm assumes that MAX_PWM * (period - 1) fits into an unsigned > > @@ -348,14 +407,17 @@ static int pwm_fan_probe(struct platform_device *pdev) > > return -EINVAL; > > } > > > > - /* Set duty cycle to maximum allowed and enable PWM output */ > > + /* > > + * Set duty cycle to maximum allowed and enable PWM output as well as > > + * the regulator. In case of error nothing is changed > > + */ > > ret = __set_pwm(ctx, MAX_PWM); > > if (ret) { > > dev_err(dev, "Failed to configure PWM: %d\n", ret); > > return ret; > > } > > timer_setup(&ctx->rpm_timer, sample_timer, 0); > > - ret = devm_add_action_or_reset(dev, pwm_fan_pwm_disable, ctx); > > + ret = devm_add_action_or_reset(dev, pwm_fan_cleanup, ctx); > > if (ret) > > return ret; > > > > @@ -461,42 +523,22 @@ static int pwm_fan_probe(struct platform_device *pdev) > > return 0; > > } > > > > -static int pwm_fan_disable(struct device *dev) > > -{ > > - struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); > > - int ret; > > - > > - if (ctx->pwm_value) { > > - /* keep ctx->pwm_state unmodified for pwm_fan_resume() */ > > - struct pwm_state state = ctx->pwm_state; > > - > > - state.duty_cycle = 0; > > - state.enabled = false; > > - ret = pwm_apply_state(ctx->pwm, &state); > > - if (ret < 0) > > - return ret; > > - } > > - > > - if (ctx->reg_en) { > > - ret = regulator_disable(ctx->reg_en); > > - if (ret) { > > - dev_err(dev, "Failed to disable fan supply: %d\n", ret); > > - return ret; > > - } > > - } > > - > > - return 0; > > -} > > - > > static void pwm_fan_shutdown(struct platform_device *pdev) > > { > > - pwm_fan_disable(&pdev->dev); > > + struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev); > > + > > + pwm_fan_cleanup(ctx); > > } > > > > #ifdef CONFIG_PM_SLEEP > > static int pwm_fan_suspend(struct device *dev) > > { > > - return pwm_fan_disable(dev); > > + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); > > + int ret; > > + > > + ret = pwm_fan_disable(ctx); > > + > > + return ret; > > } > > > > static int pwm_fan_resume(struct device *dev) > > @@ -504,6 +546,9 @@ static int pwm_fan_resume(struct device *dev) > > struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); > > int ret; > > > > + if (ctx->pwm_value == 0) > > + return 0; > > + > > if (ctx->reg_en) { > > ret = regulator_enable(ctx->reg_en); > > if (ret) { > > @@ -512,9 +557,6 @@ static int pwm_fan_resume(struct device *dev) > > } > > } > > > > - if (ctx->pwm_value == 0) > > - return 0; > > - > > return pwm_apply_state(ctx->pwm, &ctx->pwm_state); > > } > > #endif > > -- > > 2.25.1 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator 2022-05-05 22:00 ` Guenter Roeck 2022-05-06 7:15 ` (EXT) " Markus Niebel @ 2022-05-06 7:15 ` Alexander Stein 2022-05-06 8:20 ` Uwe Kleine-König 1 sibling, 1 reply; 15+ messages in thread From: Alexander Stein @ 2022-05-06 7:15 UTC (permalink / raw) To: Guenter Roeck Cc: Bartlomiej Zolnierkiewicz, Jean Delvare, Thierry Reding, Uwe Kleine-König, Lee Jones, Markus Niebel, linux-hwmon, linux-pwm Hello Guenter, Am Freitag, 6. Mai 2022, 00:00:37 CEST schrieb Guenter Roeck: > On Wed, May 04, 2022 at 02:45:51PM +0200, Alexander Stein wrote: > > From: Markus Niebel <Markus.Niebel@ew.tq-group.com> > > > > A pwm value equal to zero is meant to switch off the pwm > > hence also switching off the fan. Currently the optional > > regulator is always on. When using this driver on boards > > with an inverted pwm signal polarity this can cause running > > the fan at maximum speed when setting pwm to zero. > > The appropriate solution in this case would be to tell the > software that the pwm is inverted. Turning off the regulator > in that situation is a bad idea since setting the pwm value to > 1 would set it to almost full speed. That does not really make > sense. The pwm-fan driver is already configured for inverted PWM (ommited some properties for shortness): fan0: pwm-fan { compatible = "pwm-fan"; fan-supply = <®_pwm_fan>; pwms = <&pwm3 0 40000 PWM_POLARITY_INVERTED>; cooling-levels = <0 32 64 128 196 240>; [...] }; The problem here is that the pwm-fan driver currently enables the regulator unconditionally, but the PWM only when the fan is enabled, refer to __set_pwm(). This results in a fan at full speed when pwm-fan is idle, as pwm state is not enabled. If you keep the regulator enabled all the time, you have to drive the PWM at full duty to get an idle fan, which seems insensible for me in regards to power consumption. To implement this PWM enable inversion in pwm-fan driver seems even more complex to me. I also don't see any problem when disabling the regulator of the PWM fan. If the regulator is disabled, the fan won't move at all, regardless of PWM duty. That's why I favor to disable the regulator for the fan when it is idle. I hope this clarifies the motivation for this change. Best regards, Alexander > > The proposed changes switch the regulator off, when PWM is > > currently enabled but pwm is requested to set to zero > > and switch der regulator on, when PWM is currently disabled > > but pwm shall be set to a no zero value. > > > > Add __set_pwm_and_regulator and rewrite __set_pwm to > > handle regulator switching for the following conditions: > > > > - probe: pwm duty -> max, pwm state is unkwown: use __set_pwm > > > > and enable regulator separately to keep the devm action > > > > - off: new pwm duty is zero, pwm currently enabled: disable > > > > regulator > > > > - on: new pwm duty is non zero, pwm currently disabled: enable > > > > regulator > > > > Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > --- > > Changes in v2: > > * Added my own missing S-o-b > > > > drivers/hwmon/pwm-fan.c | 144 ++++++++++++++++++++++++++-------------- > > 1 file changed, 93 insertions(+), 51 deletions(-) > > > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > > index f12b9a28a232..b47d59fbe836 100644 > > --- a/drivers/hwmon/pwm-fan.c > > +++ b/drivers/hwmon/pwm-fan.c > > @@ -97,18 +97,50 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, > > unsigned long pwm)> > > unsigned long period; > > int ret = 0; > > struct pwm_state *state = &ctx->pwm_state; > > > > + /* track changes of reg_en for error handling */ > > + enum regulator_change { > > + untouched, > > + enabled, > > + disabled, > > + } reg_change = untouched; > > > > mutex_lock(&ctx->lock); > > > > + > > > > if (ctx->pwm_value == pwm) > > > > goto exit_set_pwm_err; > > > > + if (ctx->reg_en) { > > + if (pwm && !state->enabled) { > > + reg_change = enabled; > > + ret = regulator_enable(ctx->reg_en); > > + } else if (!pwm && state->enabled) { > > + reg_change = disabled; > > + ret = regulator_disable(ctx->reg_en); > > + } > > + if (ret) > > + goto exit_set_pwm_err; > > + } > > + > > > > period = state->period; > > state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM); > > state->enabled = pwm ? true : false; > > > > ret = pwm_apply_state(ctx->pwm, state); > > > > - if (!ret) > > + if (!ret) { > > > > ctx->pwm_value = pwm; > > > > + } else if (reg_change != untouched) { > > + /* > > + * revert regulator changes to keep consistency between > > + * pwm and regulator > > + */ > > + int err; > > + > > + if (reg_change == enabled) > > + err = regulator_disable(ctx->reg_en); > > + else if (reg_change == disabled) > > + err = regulator_enable(ctx->reg_en); > > + } > > + > > > > exit_set_pwm_err: > > mutex_unlock(&ctx->lock); > > return ret; > > > > @@ -280,18 +312,50 @@ static int pwm_fan_of_get_cooling_data(struct device > > *dev,> > > return 0; > > > > } > > > > -static void pwm_fan_regulator_disable(void *data) > > +/* > > + * disable fan and regulator > > + * if cleanup is true, disable pwm regardless of regulator disable result > > + * this makes the function dual use for unloading driver and suspend > > + */ > > + > > +static int __pwm_fan_disable_or_cleanup(struct pwm_fan_ctx *ctx, bool > > cleanup)> > > { > > > > - regulator_disable(data); > > + int ret; > > + > > + if (ctx->pwm_value) { > > + /* keep ctx->pwm_state unmodified for pwm_fan_resume() */ > > + struct pwm_state state = ctx->pwm_state; > > + > > + /* regulator is only enabled if pwm_value is not zero */ > > + if (ctx->pwm_value && ctx->reg_en) { > > + ret = regulator_disable(ctx->reg_en); > > + if (ret) { > > + pr_err("Failed to disable fan supply: %d\n", ret); > > + if (!cleanup) > > + return ret; > > + } > > + } > > + > > + state.duty_cycle = 0; > > + state.enabled = false; > > + ret = pwm_apply_state(ctx->pwm, &state); > > + } > > + > > + return ret; > > > > } > > > > -static void pwm_fan_pwm_disable(void *__ctx) > > +static void pwm_fan_cleanup(void *__ctx) > > > > { > > > > struct pwm_fan_ctx *ctx = __ctx; > > > > - > > - ctx->pwm_state.enabled = false; > > - pwm_apply_state(ctx->pwm, &ctx->pwm_state); > > > > del_timer_sync(&ctx->rpm_timer); > > > > + __pwm_fan_disable_or_cleanup(ctx, true); > > +} > > + > > +static int pwm_fan_disable(void *__ctx) > > +{ > > + struct pwm_fan_ctx *ctx = __ctx; > > + > > + return __pwm_fan_disable_or_cleanup(ctx, false); > > > > } > > > > static int pwm_fan_probe(struct platform_device *pdev) > > > > @@ -324,19 +388,14 @@ static int pwm_fan_probe(struct platform_device > > *pdev)> > > return PTR_ERR(ctx->reg_en); > > > > ctx->reg_en = NULL; > > > > - } else { > > - ret = regulator_enable(ctx->reg_en); > > - if (ret) { > > - dev_err(dev, "Failed to enable fan supply: %d\n", ret); > > - return ret; > > - } > > - ret = devm_add_action_or_reset(dev, pwm_fan_regulator_disable, > > - ctx->reg_en); > > - if (ret) > > - return ret; > > > > } > > > > pwm_init_state(ctx->pwm, &ctx->pwm_state); > > > > + /* > > + * Ensure the PWM is switched on (including the regulator), > > + * independently from any previous PWM state > > + */ > > + ctx->pwm_state.enabled = false; > > > > /* > > > > * __set_pwm assumes that MAX_PWM * (period - 1) fits into an unsigned > > > > @@ -348,14 +407,17 @@ static int pwm_fan_probe(struct platform_device > > *pdev)> > > return -EINVAL; > > > > } > > > > - /* Set duty cycle to maximum allowed and enable PWM output */ > > + /* > > + * Set duty cycle to maximum allowed and enable PWM output as well as > > + * the regulator. In case of error nothing is changed > > + */ > > > > ret = __set_pwm(ctx, MAX_PWM); > > if (ret) { > > > > dev_err(dev, "Failed to configure PWM: %d\n", ret); > > return ret; > > > > } > > timer_setup(&ctx->rpm_timer, sample_timer, 0); > > > > - ret = devm_add_action_or_reset(dev, pwm_fan_pwm_disable, ctx); > > + ret = devm_add_action_or_reset(dev, pwm_fan_cleanup, ctx); > > > > if (ret) > > > > return ret; > > > > @@ -461,42 +523,22 @@ static int pwm_fan_probe(struct platform_device > > *pdev)> > > return 0; > > > > } > > > > -static int pwm_fan_disable(struct device *dev) > > -{ > > - struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); > > - int ret; > > - > > - if (ctx->pwm_value) { > > - /* keep ctx->pwm_state unmodified for pwm_fan_resume() */ > > - struct pwm_state state = ctx->pwm_state; > > - > > - state.duty_cycle = 0; > > - state.enabled = false; > > - ret = pwm_apply_state(ctx->pwm, &state); > > - if (ret < 0) > > - return ret; > > - } > > - > > - if (ctx->reg_en) { > > - ret = regulator_disable(ctx->reg_en); > > - if (ret) { > > - dev_err(dev, "Failed to disable fan supply: %d\n", ret); > > - return ret; > > - } > > - } > > - > > - return 0; > > -} > > - > > > > static void pwm_fan_shutdown(struct platform_device *pdev) > > { > > > > - pwm_fan_disable(&pdev->dev); > > + struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev); > > + > > + pwm_fan_cleanup(ctx); > > > > } > > > > #ifdef CONFIG_PM_SLEEP > > static int pwm_fan_suspend(struct device *dev) > > { > > > > - return pwm_fan_disable(dev); > > + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); > > + int ret; > > + > > + ret = pwm_fan_disable(ctx); > > + > > + return ret; > > > > } > > > > static int pwm_fan_resume(struct device *dev) > > > > @@ -504,6 +546,9 @@ static int pwm_fan_resume(struct device *dev) > > > > struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); > > int ret; > > > > + if (ctx->pwm_value == 0) > > + return 0; > > + > > > > if (ctx->reg_en) { > > > > ret = regulator_enable(ctx->reg_en); > > if (ret) { > > > > @@ -512,9 +557,6 @@ static int pwm_fan_resume(struct device *dev) > > > > } > > > > } > > > > - if (ctx->pwm_value == 0) > > - return 0; > > - > > > > return pwm_apply_state(ctx->pwm, &ctx->pwm_state); > > > > } > > #endif ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator 2022-05-06 7:15 ` Alexander Stein @ 2022-05-06 8:20 ` Uwe Kleine-König 2022-05-06 8:35 ` (EXT) " Alexander Stein 0 siblings, 1 reply; 15+ messages in thread From: Uwe Kleine-König @ 2022-05-06 8:20 UTC (permalink / raw) To: Alexander Stein Cc: Guenter Roeck, Bartlomiej Zolnierkiewicz, Jean Delvare, Thierry Reding, Lee Jones, Markus Niebel, linux-hwmon, linux-pwm [-- Attachment #1: Type: text/plain, Size: 1813 bytes --] Hello, On Fri, May 06, 2022 at 09:15:55AM +0200, Alexander Stein wrote: > Am Freitag, 6. Mai 2022, 00:00:37 CEST schrieb Guenter Roeck: > > On Wed, May 04, 2022 at 02:45:51PM +0200, Alexander Stein wrote: > > > From: Markus Niebel <Markus.Niebel@ew.tq-group.com> > > > > > > A pwm value equal to zero is meant to switch off the pwm > > > hence also switching off the fan. Currently the optional > > > regulator is always on. When using this driver on boards > > > with an inverted pwm signal polarity this can cause running > > > the fan at maximum speed when setting pwm to zero. > > > > The appropriate solution in this case would be to tell the > > software that the pwm is inverted. Turning off the regulator > > in that situation is a bad idea since setting the pwm value to > > 1 would set it to almost full speed. That does not really make > > sense. > > The pwm-fan driver is already configured for inverted PWM (ommited some > properties for shortness): > fan0: pwm-fan { > compatible = "pwm-fan"; > fan-supply = <®_pwm_fan>; > pwms = <&pwm3 0 40000 PWM_POLARITY_INVERTED>; > cooling-levels = <0 32 64 128 196 240>; > [...] > }; > > The problem here is that the pwm-fan driver currently enables the regulator > unconditionally, but the PWM only when the fan is enabled, refer to > __set_pwm(). This results in a fan at full speed when pwm-fan is idle, as pwm > state is not enabled. Which PWM driver are you using? There is an implicit assumption in some PWM consumers that a disabled PWM emits the inactive level. However not all PWMs do this. Is this such a case? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: (EXT) Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator 2022-05-06 8:20 ` Uwe Kleine-König @ 2022-05-06 8:35 ` Alexander Stein 2022-05-06 10:23 ` Uwe Kleine-König 0 siblings, 1 reply; 15+ messages in thread From: Alexander Stein @ 2022-05-06 8:35 UTC (permalink / raw) To: Uwe Kleine-König Cc: Guenter Roeck, Bartlomiej Zolnierkiewicz, Jean Delvare, Thierry Reding, Lee Jones, Markus Niebel, linux-hwmon, linux-pwm Am Freitag, 6. Mai 2022, 10:20:01 CEST schrieb Uwe Kleine-König: > * PGP Signed by an unknown key > > Hello, > > On Fri, May 06, 2022 at 09:15:55AM +0200, Alexander Stein wrote: > > Am Freitag, 6. Mai 2022, 00:00:37 CEST schrieb Guenter Roeck: > > > On Wed, May 04, 2022 at 02:45:51PM +0200, Alexander Stein wrote: > > > > From: Markus Niebel <Markus.Niebel@ew.tq-group.com> > > > > > > > > A pwm value equal to zero is meant to switch off the pwm > > > > hence also switching off the fan. Currently the optional > > > > regulator is always on. When using this driver on boards > > > > with an inverted pwm signal polarity this can cause running > > > > the fan at maximum speed when setting pwm to zero. > > > > > > The appropriate solution in this case would be to tell the > > > software that the pwm is inverted. Turning off the regulator > > > in that situation is a bad idea since setting the pwm value to > > > 1 would set it to almost full speed. That does not really make > > > sense. > > > > The pwm-fan driver is already configured for inverted PWM (ommited some > > properties for shortness): > > fan0: pwm-fan { > > > > compatible = "pwm-fan"; > > fan-supply = <®_pwm_fan>; > > pwms = <&pwm3 0 40000 PWM_POLARITY_INVERTED>; > > cooling-levels = <0 32 64 128 196 240>; > > > > [...] > > }; > > > > The problem here is that the pwm-fan driver currently enables the > > regulator > > unconditionally, but the PWM only when the fan is enabled, refer to > > __set_pwm(). This results in a fan at full speed when pwm-fan is idle, as > > pwm state is not enabled. > > Which PWM driver are you using? It's pwm-imx27 on a imx8mp based board. > There is an implicit assumption in some PWM consumers that a disabled > PWM emits the inactive level. However not all PWMs do this. Is this such > a case? Oh, I was not aware of that assumption. As far I can tell, this assumption might actually be violated in pwm-imx27. If state->enabled==false then the EN Bit in PWMCR is not set which most probably renders the output polarity in POUTC as inactive. Best regards, Alexander ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: (EXT) Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator 2022-05-06 8:35 ` (EXT) " Alexander Stein @ 2022-05-06 10:23 ` Uwe Kleine-König 2022-05-06 12:23 ` (EXT) " Alexander Stein 0 siblings, 1 reply; 15+ messages in thread From: Uwe Kleine-König @ 2022-05-06 10:23 UTC (permalink / raw) To: Alexander Stein Cc: Guenter Roeck, Bartlomiej Zolnierkiewicz, Jean Delvare, Thierry Reding, Lee Jones, Markus Niebel, linux-hwmon, linux-pwm [-- Attachment #1: Type: text/plain, Size: 3275 bytes --] On Fri, May 06, 2022 at 10:35:21AM +0200, Alexander Stein wrote: > Am Freitag, 6. Mai 2022, 10:20:01 CEST schrieb Uwe Kleine-König: > > * PGP Signed by an unknown key > > > > Hello, > > > > On Fri, May 06, 2022 at 09:15:55AM +0200, Alexander Stein wrote: > > > Am Freitag, 6. Mai 2022, 00:00:37 CEST schrieb Guenter Roeck: > > > > On Wed, May 04, 2022 at 02:45:51PM +0200, Alexander Stein wrote: > > > > > From: Markus Niebel <Markus.Niebel@ew.tq-group.com> > > > > > > > > > > A pwm value equal to zero is meant to switch off the pwm > > > > > hence also switching off the fan. Currently the optional > > > > > regulator is always on. When using this driver on boards > > > > > with an inverted pwm signal polarity this can cause running > > > > > the fan at maximum speed when setting pwm to zero. > > > > > > > > The appropriate solution in this case would be to tell the > > > > software that the pwm is inverted. Turning off the regulator > > > > in that situation is a bad idea since setting the pwm value to > > > > 1 would set it to almost full speed. That does not really make > > > > sense. > > > > > > The pwm-fan driver is already configured for inverted PWM (ommited some > > > properties for shortness): > > > fan0: pwm-fan { > > > > > > compatible = "pwm-fan"; > > > fan-supply = <®_pwm_fan>; > > > pwms = <&pwm3 0 40000 PWM_POLARITY_INVERTED>; > > > cooling-levels = <0 32 64 128 196 240>; > > > > > > [...] > > > }; > > > > > > The problem here is that the pwm-fan driver currently enables the > > > regulator > > > unconditionally, but the PWM only when the fan is enabled, refer to > > > __set_pwm(). This results in a fan at full speed when pwm-fan is idle, as > > > pwm state is not enabled. > > > > Which PWM driver are you using? > > It's pwm-imx27 on a imx8mp based board. This is one of the known offenders. > > There is an implicit assumption in some PWM consumers that a disabled > > PWM emits the inactive level. However not all PWMs do this. Is this such > > a case? > > Oh, I was not aware of that assumption. As far I can tell, this assumption > might actually be violated in pwm-imx27. The pwm-imx27 driver is a known offender. IMHO the problem is that there is no properly documented definition what "disabled" means for a PWM. I had some discussions about that in the past with Thierry, but with no agreement. Either we have do define that the output of a PWM is undefined when it's disabled (then the pwm-fan needs fixing) or the output must be the inactive level (then the pwm-imx27 must be fixed to not unset the EN bit when configured for an inverted polarity). In my eyes the first is the sensible thing to do. See https://lore.kernel.org/linux-pwm/20180806155129.cjcc7okmwtaujf43@pengutronix.de/ for one of the previous discussions. > If state->enabled==false then the EN Bit in PWMCR is not set which most > probably renders the output polarity in POUTC as inactive. It drives the output to 0 which in the inverted polarity case is a 100% relative duty. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator 2022-05-06 10:23 ` Uwe Kleine-König @ 2022-05-06 12:23 ` Alexander Stein 2022-05-06 14:12 ` Guenter Roeck 0 siblings, 1 reply; 15+ messages in thread From: Alexander Stein @ 2022-05-06 12:23 UTC (permalink / raw) To: Uwe Kleine-König Cc: Guenter Roeck, Bartlomiej Zolnierkiewicz, Jean Delvare, Thierry Reding, Lee Jones, Markus Niebel, linux-hwmon, linux-pwm Hello Uwe, Am Freitag, 6. Mai 2022, 12:23:01 CEST schrieb Uwe Kleine-König: > * PGP Signed by an unknown key > > On Fri, May 06, 2022 at 10:35:21AM +0200, Alexander Stein wrote: > > Am Freitag, 6. Mai 2022, 10:20:01 CEST schrieb Uwe Kleine-König: > > > > Old Signed by an unknown key > > > > > > Hello, > > > > > > On Fri, May 06, 2022 at 09:15:55AM +0200, Alexander Stein wrote: > > > > Am Freitag, 6. Mai 2022, 00:00:37 CEST schrieb Guenter Roeck: > > > > > On Wed, May 04, 2022 at 02:45:51PM +0200, Alexander Stein wrote: > > > > > > From: Markus Niebel <Markus.Niebel@ew.tq-group.com> > > > > > > > > > > > > A pwm value equal to zero is meant to switch off the pwm > > > > > > hence also switching off the fan. Currently the optional > > > > > > regulator is always on. When using this driver on boards > > > > > > with an inverted pwm signal polarity this can cause running > > > > > > the fan at maximum speed when setting pwm to zero. > > > > > > > > > > The appropriate solution in this case would be to tell the > > > > > software that the pwm is inverted. Turning off the regulator > > > > > in that situation is a bad idea since setting the pwm value to > > > > > 1 would set it to almost full speed. That does not really make > > > > > sense. > > > > > > > > The pwm-fan driver is already configured for inverted PWM (ommited > > > > some > > > > properties for shortness): > > > > fan0: pwm-fan { > > > > > > > > compatible = "pwm-fan"; > > > > fan-supply = <®_pwm_fan>; > > > > pwms = <&pwm3 0 40000 PWM_POLARITY_INVERTED>; > > > > cooling-levels = <0 32 64 128 196 240>; > > > > > > > > [...] > > > > }; > > > > > > > > The problem here is that the pwm-fan driver currently enables the > > > > regulator > > > > unconditionally, but the PWM only when the fan is enabled, refer to > > > > __set_pwm(). This results in a fan at full speed when pwm-fan is idle, > > > > as > > > > pwm state is not enabled. > > > > > > Which PWM driver are you using? > > > > It's pwm-imx27 on a imx8mp based board. > > This is one of the known offenders. > > > > There is an implicit assumption in some PWM consumers that a disabled > > > PWM emits the inactive level. However not all PWMs do this. Is this such > > > a case? > > > > Oh, I was not aware of that assumption. As far I can tell, this assumption > > might actually be violated in pwm-imx27. > > The pwm-imx27 driver is a known offender. > > IMHO the problem is that there is no properly documented definition what > "disabled" means for a PWM. I had some discussions about that in the > past with Thierry, but with no agreement. Either we have do define that > the output of a PWM is undefined when it's disabled (then the pwm-fan > needs fixing) or the output must be the inactive level (then the > pwm-imx27 must be fixed to not unset the EN bit when configured for an > inverted polarity). In my eyes the first is the sensible thing to do. > > See > https://lore.kernel.org/linux-pwm/20180806155129.cjcc7okmwtaujf43@pengutroni > x.de/ for one of the previous discussions. Thanks for the link. I took a look into it. I'm on your side here, IMHO pwm_disable() implies that the PWM perphery is disabled, including any clocks or powerdomain. This is what pwm-imx27 actually does. This might lead to a, probably platform dependent, (undefined?) state of the PWM output pin. This implies it is not possible to disable the PWM periphery for inverted signals, if the disabled state is not the inactive level. You know all about it already. Then again from pwm-fan side I want be able to disable the FAN, turning of regulator and PWM, so powersaving is possible. That's what this patch is about. This is similar also what pwm_bl is doing. Independent of the exact semantics, it makes sense to disable the regulator in pwm-fan as well when the fan shall be disabled. > > If state->enabled==false then the EN Bit in PWMCR is not set which most > > probably renders the output polarity in POUTC as inactive. > > It drives the output to 0 which in the inverted polarity case is a 100% > relative duty. Thanks for confirmation, this matches my observations. Best regards, Alexander ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator 2022-05-06 12:23 ` (EXT) " Alexander Stein @ 2022-05-06 14:12 ` Guenter Roeck 2022-05-06 14:29 ` Uwe Kleine-König 0 siblings, 1 reply; 15+ messages in thread From: Guenter Roeck @ 2022-05-06 14:12 UTC (permalink / raw) To: Alexander Stein Cc: Uwe Kleine-König, Bartlomiej Zolnierkiewicz, Jean Delvare, Thierry Reding, Lee Jones, Markus Niebel, linux-hwmon, linux-pwm On Fri, May 06, 2022 at 02:23:11PM +0200, Alexander Stein wrote: > Hello Uwe, > > Am Freitag, 6. Mai 2022, 12:23:01 CEST schrieb Uwe Kleine-König: > > * PGP Signed by an unknown key > > > > On Fri, May 06, 2022 at 10:35:21AM +0200, Alexander Stein wrote: > > > Am Freitag, 6. Mai 2022, 10:20:01 CEST schrieb Uwe Kleine-König: > > > > > Old Signed by an unknown key > > > > > > > > Hello, > > > > > > > > On Fri, May 06, 2022 at 09:15:55AM +0200, Alexander Stein wrote: > > > > > Am Freitag, 6. Mai 2022, 00:00:37 CEST schrieb Guenter Roeck: > > > > > > On Wed, May 04, 2022 at 02:45:51PM +0200, Alexander Stein wrote: > > > > > > > From: Markus Niebel <Markus.Niebel@ew.tq-group.com> > > > > > > > > > > > > > > A pwm value equal to zero is meant to switch off the pwm > > > > > > > hence also switching off the fan. Currently the optional > > > > > > > regulator is always on. When using this driver on boards > > > > > > > with an inverted pwm signal polarity this can cause running > > > > > > > the fan at maximum speed when setting pwm to zero. > > > > > > > > > > > > The appropriate solution in this case would be to tell the > > > > > > software that the pwm is inverted. Turning off the regulator > > > > > > in that situation is a bad idea since setting the pwm value to > > > > > > 1 would set it to almost full speed. That does not really make > > > > > > sense. > > > > > > > > > > The pwm-fan driver is already configured for inverted PWM (ommited > > > > > some > > > > > properties for shortness): > > > > > fan0: pwm-fan { > > > > > > > > > > compatible = "pwm-fan"; > > > > > fan-supply = <®_pwm_fan>; > > > > > pwms = <&pwm3 0 40000 PWM_POLARITY_INVERTED>; > > > > > cooling-levels = <0 32 64 128 196 240>; > > > > > > > > > > [...] > > > > > }; > > > > > > > > > > The problem here is that the pwm-fan driver currently enables the > > > > > regulator > > > > > unconditionally, but the PWM only when the fan is enabled, refer to > > > > > __set_pwm(). This results in a fan at full speed when pwm-fan is idle, > > > > > as > > > > > pwm state is not enabled. > > > > > > > > Which PWM driver are you using? > > > > > > It's pwm-imx27 on a imx8mp based board. > > > > This is one of the known offenders. > > > > > > There is an implicit assumption in some PWM consumers that a disabled > > > > PWM emits the inactive level. However not all PWMs do this. Is this such > > > > a case? > > > > > > Oh, I was not aware of that assumption. As far I can tell, this assumption > > > might actually be violated in pwm-imx27. > > > > The pwm-imx27 driver is a known offender. > > > > IMHO the problem is that there is no properly documented definition what > > "disabled" means for a PWM. I had some discussions about that in the > > past with Thierry, but with no agreement. Either we have do define that > > the output of a PWM is undefined when it's disabled (then the pwm-fan > > needs fixing) or the output must be the inactive level (then the > > pwm-imx27 must be fixed to not unset the EN bit when configured for an > > inverted polarity). In my eyes the first is the sensible thing to do. > > > > See > > https://lore.kernel.org/linux-pwm/20180806155129.cjcc7okmwtaujf43@pengutroni > > x.de/ for one of the previous discussions. > > Thanks for the link. I took a look into it. I'm on your side here, IMHO > pwm_disable() implies that the PWM perphery is disabled, including any clocks > or powerdomain. This is what pwm-imx27 actually does. This might lead to a, > probably platform dependent, (undefined?) state of the PWM output pin. > This implies it is not possible to disable the PWM periphery for inverted > signals, if the disabled state is not the inactive level. You know all about > it already. > Then again from pwm-fan side I want be able to disable the FAN, turning of > regulator and PWM, so powersaving is possible. That's what this patch is > about. This is similar also what pwm_bl is doing. > Independent of the exact semantics, it makes sense to disable the regulator in > pwm-fan as well when the fan shall be disabled. > There are fans which never stop if pwm==0, such as some CPU fans. I don't think it is a good idea to force those off by turning off their power. The problem in the driver is that it treats pwm==0 as "disable pwm", not as "set pwm output to 0", Part of the probem may be that the ABI doesn't have a good representation for "disable pwm output", which is what is really wanted/needed here. I think the best solution would be to implement and use pwmX_enable, and define in the driver documentation that pwm1_enable=0 reflects "disable pwm" and pwm1_enable=1 reflects "emable manual pwm control:. At the same time, stop associating "pwm==0" with "disable pwm", but just set the pwm output value to 0. Guenter > > > If state->enabled==false then the EN Bit in PWMCR is not set which most > > > probably renders the output polarity in POUTC as inactive. > > > > It drives the output to 0 which in the inverted polarity case is a 100% > > relative duty. > > Thanks for confirmation, this matches my observations. > > Best regards, > Alexander > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator 2022-05-06 14:12 ` Guenter Roeck @ 2022-05-06 14:29 ` Uwe Kleine-König 2022-05-06 18:31 ` Guenter Roeck 0 siblings, 1 reply; 15+ messages in thread From: Uwe Kleine-König @ 2022-05-06 14:29 UTC (permalink / raw) To: Guenter Roeck Cc: Alexander Stein, Jean Delvare, Thierry Reding, Lee Jones, Markus Niebel, linux-hwmon, linux-pwm [-- Attachment #1: Type: text/plain, Size: 2559 bytes --] [Dropped Bartlomiej Zolnierkiewicz from Cc:; my mailer daemon claims the email address doens't exist.] Hello Guenter, On Fri, May 06, 2022 at 07:12:44AM -0700, Guenter Roeck wrote: > On Fri, May 06, 2022 at 02:23:11PM +0200, Alexander Stein wrote: > > Am Freitag, 6. Mai 2022, 12:23:01 CEST schrieb Uwe Kleine-König: > > > See > > > https://lore.kernel.org/linux-pwm/20180806155129.cjcc7okmwtaujf43@pengutronix.de/ > > > for one of the previous discussions. > > > > Thanks for the link. I took a look into it. I'm on your side here, IMHO > > pwm_disable() implies that the PWM perphery is disabled, including any clocks > > or powerdomain. This is what pwm-imx27 actually does. This might lead to a, > > probably platform dependent, (undefined?) state of the PWM output pin. > > This implies it is not possible to disable the PWM periphery for inverted > > signals, if the disabled state is not the inactive level. You know all about > > it already. > > Then again from pwm-fan side I want be able to disable the FAN, turning of > > regulator and PWM, so powersaving is possible. That's what this patch is > > about. This is similar also what pwm_bl is doing. > > Independent of the exact semantics, it makes sense to disable the regulator in > > pwm-fan as well when the fan shall be disabled. > > There are fans which never stop if pwm==0, such as some CPU fans. I don't I assume with pwm==0 you actually mean duty_cycle == 0? > think it is a good idea to force those off by turning off their power. The > problem in the driver is that it treats pwm==0 as "disable pwm", not as > "set pwm output to 0", Part of the probem may be that the ABI doesn't have > a good representation for "disable pwm output", which is what is really > wanted/needed here. Disable pwm output == set pwm output to High-Z? Not all PWMs are able to provide that. > I think the best solution would be to implement and > use pwmX_enable, and define in the driver documentation that pwm1_enable=0 > reflects "disable pwm" and pwm1_enable=1 reflects "emable manual pwm > control:. At the same time, stop associating "pwm==0" with "disable pwm", > but just set the pwm output value to 0. Are you talking about the PWM framework here, or only the pwm-fan driver? I'd expect there are better names than pwm1_enable for the intended semantic. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator 2022-05-06 14:29 ` Uwe Kleine-König @ 2022-05-06 18:31 ` Guenter Roeck 2022-05-09 7:39 ` Alexander Stein 0 siblings, 1 reply; 15+ messages in thread From: Guenter Roeck @ 2022-05-06 18:31 UTC (permalink / raw) To: Uwe Kleine-König Cc: Alexander Stein, Jean Delvare, Thierry Reding, Lee Jones, Markus Niebel, linux-hwmon, linux-pwm On Fri, May 06, 2022 at 04:29:13PM +0200, Uwe Kleine-König wrote: > [Dropped Bartlomiej Zolnierkiewicz from Cc:; my mailer daemon claims the > email address doens't exist.] > > Hello Guenter, > > On Fri, May 06, 2022 at 07:12:44AM -0700, Guenter Roeck wrote: > > On Fri, May 06, 2022 at 02:23:11PM +0200, Alexander Stein wrote: > > > Am Freitag, 6. Mai 2022, 12:23:01 CEST schrieb Uwe Kleine-König: > > > > See > > > > https://lore.kernel.org/linux-pwm/20180806155129.cjcc7okmwtaujf43@pengutronix.de/ > > > > for one of the previous discussions. > > > > > > Thanks for the link. I took a look into it. I'm on your side here, IMHO > > > pwm_disable() implies that the PWM perphery is disabled, including any clocks > > > or powerdomain. This is what pwm-imx27 actually does. This might lead to a, > > > probably platform dependent, (undefined?) state of the PWM output pin. > > > This implies it is not possible to disable the PWM periphery for inverted > > > signals, if the disabled state is not the inactive level. You know all about > > > it already. > > > Then again from pwm-fan side I want be able to disable the FAN, turning of > > > regulator and PWM, so powersaving is possible. That's what this patch is > > > about. This is similar also what pwm_bl is doing. > > > Independent of the exact semantics, it makes sense to disable the regulator in > > > pwm-fan as well when the fan shall be disabled. > > > > There are fans which never stop if pwm==0, such as some CPU fans. I don't > > I assume with pwm==0 you actually mean duty_cycle == 0? > Correct. The "pwm" attribute sets the duty cycle. > > think it is a good idea to force those off by turning off their power. The > > problem in the driver is that it treats pwm==0 as "disable pwm", not as > > "set pwm output to 0", Part of the probem may be that the ABI doesn't have > > a good representation for "disable pwm output", which is what is really > > wanted/needed here. > > Disable pwm output == set pwm output to High-Z? Not all PWMs are able to > provide that. > It is up to us to define whate it means exactly. If you are ok that "set duty cycle to 0" reflects "set duty cycle to 0, disable pwm, and turn off regulator", I would hope that you are ok with using the _enable attribute to do the same and leaving pwm==0 to do what it is supposed to do, ie to keep pwm control enabled and set the duty cycle to 0. Thanks, Guenter > > I think the best solution would be to implement and > > use pwmX_enable, and define in the driver documentation that pwm1_enable=0 > > reflects "disable pwm" and pwm1_enable=1 reflects "emable manual pwm > > control:. At the same time, stop associating "pwm==0" with "disable pwm", > > but just set the pwm output value to 0. > > Are you talking about the PWM framework here, or only the pwm-fan > driver? > > I'd expect there are better names than pwm1_enable for the intended > semantic. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator 2022-05-06 18:31 ` Guenter Roeck @ 2022-05-09 7:39 ` Alexander Stein 2022-05-09 10:59 ` Uwe Kleine-König 2022-05-09 22:19 ` Guenter Roeck 0 siblings, 2 replies; 15+ messages in thread From: Alexander Stein @ 2022-05-09 7:39 UTC (permalink / raw) To: Uwe Kleine-König, Guenter Roeck Cc: Jean Delvare, Thierry Reding, Lee Jones, Markus Niebel, linux-hwmon, linux-pwm Am Freitag, 6. Mai 2022, 20:31:24 CEST schrieb Guenter Roeck: > On Fri, May 06, 2022 at 04:29:13PM +0200, Uwe Kleine-König wrote: > > [Dropped Bartlomiej Zolnierkiewicz from Cc:; my mailer daemon claims the > > email address doens't exist.] > > > > Hello Guenter, > > > > On Fri, May 06, 2022 at 07:12:44AM -0700, Guenter Roeck wrote: > > > On Fri, May 06, 2022 at 02:23:11PM +0200, Alexander Stein wrote: > > > > Am Freitag, 6. Mai 2022, 12:23:01 CEST schrieb Uwe Kleine-König: > > > > > See > > > > > https://lore.kernel.org/linux-pwm/20180806155129.cjcc7okmwtaujf43@pe > > > > > ngutronix.de/ for one of the previous discussions. > > > > > > > > Thanks for the link. I took a look into it. I'm on your side here, > > > > IMHO > > > > pwm_disable() implies that the PWM perphery is disabled, including any > > > > clocks or powerdomain. This is what pwm-imx27 actually does. This > > > > might lead to a, probably platform dependent, (undefined?) state of > > > > the PWM output pin. This implies it is not possible to disable the > > > > PWM periphery for inverted signals, if the disabled state is not the > > > > inactive level. You know all about it already. > > > > Then again from pwm-fan side I want be able to disable the FAN, > > > > turning of > > > > regulator and PWM, so powersaving is possible. That's what this patch > > > > is > > > > about. This is similar also what pwm_bl is doing. > > > > Independent of the exact semantics, it makes sense to disable the > > > > regulator in pwm-fan as well when the fan shall be disabled. > > > > > > There are fans which never stop if pwm==0, such as some CPU fans. I > > > don't > > > > I assume with pwm==0 you actually mean duty_cycle == 0? > > Correct. The "pwm" attribute sets the duty cycle. > > > > think it is a good idea to force those off by turning off their power. > > > The > > > problem in the driver is that it treats pwm==0 as "disable pwm", not as > > > "set pwm output to 0", Part of the probem may be that the ABI doesn't > > > have > > > a good representation for "disable pwm output", which is what is really > > > wanted/needed here. > > > > Disable pwm output == set pwm output to High-Z? Not all PWMs are able to > > provide that. > > It is up to us to define whate it means exactly. If you are ok that "set > duty cycle to 0" reflects "set duty cycle to 0, disable pwm, and turn off > regulator", I would hope that you are ok with using the _enable attribute > to do the same and leaving pwm==0 to do what it is supposed to do, ie to > keep pwm control enabled and set the duty cycle to 0. Just to make sure to be on the same side and summarize a bit. What you mean is to add a new sysfs attribute to pwm-fan driver which controls what pwm_duty==0 implies. I would suggest to name is 'keep_pwm_enabled' (but I am open for other suggestions) with the following meaning: 1 - pwm_duty==0 just means that. Set PWM duty to 0 and keep PWM (and fan regulator) enabled. 0 - pwm_duty==0 means that the PWM duty is set to 0, PWM is disabled and any PWM fan regulator is disabled as well. For pwm_duty!=0 this setting is irrelevant. Having the default to be '1' seems sensible in order to not brake boards as regulator will be kept enabled. PWM duty and/or PWM disable is irrelevant as PWM inversion is not yet supported properly anyway. IMHO this should address all the mentioned issues. With 'keep_pwm_enabled=1' only the duty is set and the regulators are not forced to be disabled. E.g. the CPU fans mentioned by Guenter. This is also the case for hardware where the regulator is shared or not switchable at all. On hardware where it is safe to disable the regulator and PWM keep_pwm_enabled=0' allows the system to poweroff PWM and powersupply for the fan to improve powersavings. With this it is up to the administrator to provide the correct setting for this attribute as it highly depends on the actual hardware and/or usage. Best regards, Alexander ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator 2022-05-09 7:39 ` Alexander Stein @ 2022-05-09 10:59 ` Uwe Kleine-König 2022-05-09 12:10 ` Alexander Stein 2022-05-09 22:19 ` Guenter Roeck 1 sibling, 1 reply; 15+ messages in thread From: Uwe Kleine-König @ 2022-05-09 10:59 UTC (permalink / raw) To: Alexander Stein, Thierry Reding Cc: Guenter Roeck, Jean Delvare, Lee Jones, Markus Niebel, linux-hwmon, linux-pwm [-- Attachment #1: Type: text/plain, Size: 7433 bytes --] Hello Alexander, hello Guenter, On Mon, May 09, 2022 at 09:39:15AM +0200, Alexander Stein wrote: > Am Freitag, 6. Mai 2022, 20:31:24 CEST schrieb Guenter Roeck: > > On Fri, May 06, 2022 at 04:29:13PM +0200, Uwe Kleine-König wrote: > > > [Dropped Bartlomiej Zolnierkiewicz from Cc:; my mailer daemon claims the > > > email address doens't exist.] > > > > > > Hello Guenter, > > > > > > On Fri, May 06, 2022 at 07:12:44AM -0700, Guenter Roeck wrote: > > > > On Fri, May 06, 2022 at 02:23:11PM +0200, Alexander Stein wrote: > > > > > Am Freitag, 6. Mai 2022, 12:23:01 CEST schrieb Uwe Kleine-König: > > > > > > See > > > > > > https://lore.kernel.org/linux-pwm/20180806155129.cjcc7okmwtaujf43@pe > > > > > > ngutronix.de/ for one of the previous discussions. > > > > > > > > > > Thanks for the link. I took a look into it. I'm on your side here, > > > > > IMHO > > > > > pwm_disable() implies that the PWM perphery is disabled, including any > > > > > clocks or powerdomain. This is what pwm-imx27 actually does. This > > > > > might lead to a, probably platform dependent, (undefined?) state of > > > > > the PWM output pin. This implies it is not possible to disable the > > > > > PWM periphery for inverted signals, if the disabled state is not the > > > > > inactive level. You know all about it already. > > > > > Then again from pwm-fan side I want be able to disable the FAN, > > > > > turning of > > > > > regulator and PWM, so powersaving is possible. That's what this patch > > > > > is > > > > > about. This is similar also what pwm_bl is doing. > > > > > Independent of the exact semantics, it makes sense to disable the > > > > > regulator in pwm-fan as well when the fan shall be disabled. > > > > > > > > There are fans which never stop if pwm==0, such as some CPU fans. I > > > > don't > > > > > > I assume with pwm==0 you actually mean duty_cycle == 0? > > > > Correct. The "pwm" attribute sets the duty cycle. > > > > > > think it is a good idea to force those off by turning off their power. > > > > The > > > > problem in the driver is that it treats pwm==0 as "disable pwm", not as > > > > "set pwm output to 0", Part of the probem may be that the ABI doesn't > > > > have > > > > a good representation for "disable pwm output", which is what is really > > > > wanted/needed here. > > > > > > Disable pwm output == set pwm output to High-Z? Not all PWMs are able to > > > provide that. > > > > It is up to us to define whate it means exactly. If you are ok that "set > > duty cycle to 0" reflects "set duty cycle to 0, disable pwm, and turn off > > regulator", I would hope that you are ok with using the _enable attribute > > to do the same and leaving pwm==0 to do what it is supposed to do, ie to > > keep pwm control enabled and set the duty cycle to 0. > > Just to make sure to be on the same side and summarize a bit. What you mean is > to add a new sysfs attribute to pwm-fan driver which controls what pwm_duty==0 > implies. I would suggest to name is 'keep_pwm_enabled' (but I am open for > other suggestions) with the following meaning: > 1 - pwm_duty==0 just means that. Set PWM duty to 0 and keep PWM (and fan > regulator) enabled. > > 0 - pwm_duty==0 means that the PWM duty is set to 0, PWM is disabled and any > PWM fan regulator is disabled as well. I'm not convinced. A property that is called "keep_pwm_enabled" shouldn't have any effect on the regulator. Maybe we need two properties, one for the PWM and one for the regulator? > With this it is up to the administrator to provide the correct setting for > this attribute as it highly depends on the actual hardware and/or usage. I wonder if that is a devicetree (or firmware) property instead of a sysfs knob. A related problem is that there is no official definition for the PWM framework what a consumer can expect from a disabled PWM, and some have adopted the expectation "constant inactive output" as this is what several lowlevel implementations provide. The two obvious candidates for such an expectation are: a) emit the inactive level b) no guarantees about the output I think there should be an explicit definition and which of them is picked has an influence on the decision how to properly model a PWM fan. (If you say now the design of a device tree model shouldn't depend on what the Linux PWM framework considers as right behaviour for a disabled PWM, you're right. But you have to have some concept of "disabled PWM" and the thoughts to pick one definition or the other are the same, so it's sensible to come to the same conclusion for both formally different questions.) I'm convinced that b) is the sane way to pick. The reasons are: - Some hardware cannot be disabled while emitting a constant 0 or 1. - There is already a way for consumers to express: I want a constant inactive output. (i.e. .duty_cycle = 0, .enabled = 1, .period = $big) When adopting b) there is some expressiveness missing though and that has to do with transitions to new configurations. If a PWM runs at 100% relative duty cycle (i.e. .duty_cycle == .period) and the consumer then calls pwm_apply(mypwm, {.duty_cycle = 0, .period = 5000, .enabled = true }) and some time later pwm_apply(mypwm, {.duty_cycle = 5000, .period = 5000, .enabled = true }) they might expect that the PWM is low for a duration that is a multiple of 5ms. However I think that doesn't have a practical relevance and quite a few PWM hardware implementations cannot complete a period on a configuration change anyhow. So I believe it's safe to disable a PWM after pwm_apply(mypwm, {.duty_cycle = 0, .period = 5000, .enabled = true }) *iff* it provides a constant inactive output. To fix that, we'd have to distinguish between "sync" and "async" configuration updates. However I see no need to do that now, as it doesn't solve a real life problem. When adopting a) however a PWM that doesn't maintain an inactive output when disabled, must not be disabled in such a case (so we'd actually need to do diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c --- a/drivers/pwm/pwm-imx27.c +++ b/drivers/pwm/pwm-imx27.c @@ -280,7 +280,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, cr |= FIELD_PREP(MX3_PWMCR_POUTC, MX3_PWMCR_POUTC_INVERTED); - if (state->enabled) + /* + * When the EN bit is not set, the hardware emits a constant low output. + * There is no formal definition about the right behaviour, but some + * consumers expect an inactive output from a disabled PWM. So only + * clear EN if normal polarity is configured. + */ + if (state->enabled || state->polarity == PWM_POLARITY_INVERSED) cr |= MX3_PWMCR_EN; writel(cr, imx->mmio_base + MX3_PWMCR); for the imx27 case[1]). So the downside of adopting a) is that there is no way for the lowlevel driver to know that it can safely disable the hardware and so safe some power. Best regards Uwe [1] or something more complicated. I remember a patch that switched the pinmuxing to GPIO that emitted a constant low output which I consider not being worth the power savings (if there are any). -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator 2022-05-09 10:59 ` Uwe Kleine-König @ 2022-05-09 12:10 ` Alexander Stein 0 siblings, 0 replies; 15+ messages in thread From: Alexander Stein @ 2022-05-09 12:10 UTC (permalink / raw) To: Thierry Reding, Uwe Kleine-König Cc: Guenter Roeck, Jean Delvare, Lee Jones, Markus Niebel, linux-hwmon, linux-pwm Hello Uwe, Am Montag, 9. Mai 2022, 12:59:21 CEST schrieb Uwe Kleine-König: > * PGP Signed by an unknown key > > Hello Alexander, hello Guenter, > > On Mon, May 09, 2022 at 09:39:15AM +0200, Alexander Stein wrote: > > Am Freitag, 6. Mai 2022, 20:31:24 CEST schrieb Guenter Roeck: > > > On Fri, May 06, 2022 at 04:29:13PM +0200, Uwe Kleine-König wrote: > > > > [Dropped Bartlomiej Zolnierkiewicz from Cc:; my mailer daemon claims > > > > the > > > > email address doens't exist.] > > > > > > > > Hello Guenter, > > > > > > > > On Fri, May 06, 2022 at 07:12:44AM -0700, Guenter Roeck wrote: > > > > > On Fri, May 06, 2022 at 02:23:11PM +0200, Alexander Stein wrote: > > > > > > Am Freitag, 6. Mai 2022, 12:23:01 CEST schrieb Uwe Kleine-König: > > > > > > > See > > > > > > > https://lore.kernel.org/linux-pwm/20180806155129.cjcc7okmwtaujf4 > > > > > > > 3@pe > > > > > > > ngutronix.de/ for one of the previous discussions. > > > > > > > > > > > > Thanks for the link. I took a look into it. I'm on your side here, > > > > > > IMHO > > > > > > pwm_disable() implies that the PWM perphery is disabled, including > > > > > > any > > > > > > clocks or powerdomain. This is what pwm-imx27 actually does. This > > > > > > might lead to a, probably platform dependent, (undefined?) state > > > > > > of > > > > > > the PWM output pin. This implies it is not possible to disable the > > > > > > PWM periphery for inverted signals, if the disabled state is not > > > > > > the > > > > > > inactive level. You know all about it already. > > > > > > Then again from pwm-fan side I want be able to disable the FAN, > > > > > > turning of > > > > > > regulator and PWM, so powersaving is possible. That's what this > > > > > > patch > > > > > > is > > > > > > about. This is similar also what pwm_bl is doing. > > > > > > Independent of the exact semantics, it makes sense to disable the > > > > > > regulator in pwm-fan as well when the fan shall be disabled. > > > > > > > > > > There are fans which never stop if pwm==0, such as some CPU fans. I > > > > > don't > > > > > > > > I assume with pwm==0 you actually mean duty_cycle == 0? > > > > > > Correct. The "pwm" attribute sets the duty cycle. > > > > > > > > think it is a good idea to force those off by turning off their > > > > > power. > > > > > The > > > > > problem in the driver is that it treats pwm==0 as "disable pwm", not > > > > > as > > > > > "set pwm output to 0", Part of the probem may be that the ABI > > > > > doesn't > > > > > have > > > > > a good representation for "disable pwm output", which is what is > > > > > really > > > > > wanted/needed here. > > > > > > > > Disable pwm output == set pwm output to High-Z? Not all PWMs are able > > > > to > > > > provide that. > > > > > > It is up to us to define whate it means exactly. If you are ok that "set > > > duty cycle to 0" reflects "set duty cycle to 0, disable pwm, and turn > > > off > > > regulator", I would hope that you are ok with using the _enable > > > attribute > > > to do the same and leaving pwm==0 to do what it is supposed to do, ie to > > > keep pwm control enabled and set the duty cycle to 0. > > > > Just to make sure to be on the same side and summarize a bit. What you > > mean is to add a new sysfs attribute to pwm-fan driver which controls > > what pwm_duty==0 implies. I would suggest to name is 'keep_pwm_enabled' > > (but I am open for other suggestions) with the following meaning: > > 1 - pwm_duty==0 just means that. Set PWM duty to 0 and keep PWM (and fan > > regulator) enabled. > > > > 0 - pwm_duty==0 means that the PWM duty is set to 0, PWM is disabled and > > any PWM fan regulator is disabled as well. > > I'm not convinced. A property that is called "keep_pwm_enabled" > shouldn't have any effect on the regulator. Maybe we need two > properties, one for the PWM and one for the regulator? Good point, that name might be misleading. 'keep_fan_enabled' or 'keep_device_enabled' comes to might mind right now. The intention is to switch both PWM and regulator simultaneously, or not. But having two orthogonal properties don't make sense to me. regulator | pwm | comment ----------+---------+-------- disable | disable | keep_device_enabled=0 disable | enable | does not make sense for me. enable | disable | Current state, which is not correct in some cases enable | enable | keep_device_enabled=1 That's why I would keep a single property, but I might be missing something. If there is no regulator at all, then keep_device_enabled=0 is the same as what currently happens. > > With this it is up to the administrator to provide the correct setting for > > this attribute as it highly depends on the actual hardware and/or usage. > > I wonder if that is a devicetree (or firmware) property instead of > a sysfs knob. > > A related problem is that there is no official definition for the PWM > framework what a consumer can expect from a disabled PWM, and some have > adopted the expectation "constant inactive output" as this is what > several lowlevel implementations provide. I guess there are examples for both a) and b). pwm_bl disables both PWM and regulator in pwm_backlight_power_off(). AFAICS imx6dl-yapp4-common.dtsi assumes pwm duty 0 means to disable both regulator and PWM, due to inverted polarity and pwm-imx27 specific behavior. > The two obvious candidates for such an expectation are: > > a) emit the inactive level > b) no guarantees about the output > > I think there should be an explicit definition and which of them is > picked has an influence on the decision how to properly model a PWM fan. > (If you say now the design of a device tree model shouldn't depend on > what the Linux PWM framework considers as right behaviour for a disabled > PWM, you're right. But you have to have some concept of "disabled PWM" > and the thoughts to pick one definition or the other are the same, so > it's sensible to come to the same conclusion for both formally different > questions.) > > I'm convinced that b) is the sane way to pick. The reasons are: > > - Some hardware cannot be disabled while emitting a constant 0 or 1. > - There is already a way for consumers to express: I want a constant > inactive output. (i.e. .duty_cycle = 0, .enabled = 1, .period = $big) Assuming b) for this driver has the benefit that a) can also be implemented using the above mentioned knob. This configuration option is apparently needed anyway to specify if the PWM and regulator can be turned off or need to be kept enabled. IMHO if the PWM and regulator can be turned off, it should be possible to do so. > When adopting b) there is some expressiveness missing though and that > has to do with transitions to new configurations. If a PWM runs at 100% > relative duty cycle (i.e. .duty_cycle == .period) and the consumer then > calls > > pwm_apply(mypwm, {.duty_cycle = 0, .period = 5000, .enabled = true }) > > and some time later > > pwm_apply(mypwm, {.duty_cycle = 5000, .period = 5000, .enabled = true }) > > they might expect that the PWM is low for a duration that is a multiple > of 5ms. However I think that doesn't have a practical relevance and > quite a few PWM hardware implementations cannot complete a period on a > configuration change anyhow. So I believe it's safe to disable a PWM > after > > pwm_apply(mypwm, {.duty_cycle = 0, .period = 5000, .enabled = true }) > > *iff* it provides a constant inactive output. To fix that, we'd have to > distinguish between "sync" and "async" configuration updates. However I > see no need to do that now, as it doesn't solve a real life problem. When adapting b) it should not matter whether a constant inactive output is provided. But maybe I didn't get your point here :/ > When adopting a) however a PWM that doesn't maintain an inactive output > when disabled, must not be disabled in such a case (so we'd actually > need to do > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > --- a/drivers/pwm/pwm-imx27.c > +++ b/drivers/pwm/pwm-imx27.c > @@ -280,7 +280,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, > struct pwm_device *pwm, cr |= FIELD_PREP(MX3_PWMCR_POUTC, > MX3_PWMCR_POUTC_INVERTED); > > - if (state->enabled) > + /* > + * When the EN bit is not set, the hardware emits a constant low output. > + * There is no formal definition about the right behaviour, but some > + * consumers expect an inactive output from a disabled PWM. So only > + * clear EN if normal polarity is configured. > + */ > + if (state->enabled || state->polarity == PWM_POLARITY_INVERSED) > cr |= MX3_PWMCR_EN; > > writel(cr, imx->mmio_base + MX3_PWMCR); > > for the imx27 case[1]). So the downside of adopting a) is that there is > no way for the lowlevel driver to know that it can safely disable the > hardware and so safe some power. From my point of view when a state->enable=false is applied, the PWM should be disabled regardless of any polarity configuration. As apparently not every PWM controller can provide an inactive output level in disabled state this needs to be handled in an upper layer, in this case pwm-fan. The important thing then to know is whether disabling PWM can be done safely. Best regards, Alexander ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator 2022-05-09 7:39 ` Alexander Stein 2022-05-09 10:59 ` Uwe Kleine-König @ 2022-05-09 22:19 ` Guenter Roeck 1 sibling, 0 replies; 15+ messages in thread From: Guenter Roeck @ 2022-05-09 22:19 UTC (permalink / raw) To: Alexander Stein, Uwe Kleine-König Cc: Jean Delvare, Thierry Reding, Lee Jones, Markus Niebel, linux-hwmon, linux-pwm On 5/9/22 00:39, Alexander Stein wrote: > Am Freitag, 6. Mai 2022, 20:31:24 CEST schrieb Guenter Roeck: >> On Fri, May 06, 2022 at 04:29:13PM +0200, Uwe Kleine-König wrote: >>> [Dropped Bartlomiej Zolnierkiewicz from Cc:; my mailer daemon claims the >>> email address doens't exist.] >>> >>> Hello Guenter, >>> >>> On Fri, May 06, 2022 at 07:12:44AM -0700, Guenter Roeck wrote: >>>> On Fri, May 06, 2022 at 02:23:11PM +0200, Alexander Stein wrote: >>>>> Am Freitag, 6. Mai 2022, 12:23:01 CEST schrieb Uwe Kleine-König: >>>>>> See >>>>>> https://lore.kernel.org/linux-pwm/20180806155129.cjcc7okmwtaujf43@pe >>>>>> ngutronix.de/ for one of the previous discussions. >>>>> >>>>> Thanks for the link. I took a look into it. I'm on your side here, >>>>> IMHO >>>>> pwm_disable() implies that the PWM perphery is disabled, including any >>>>> clocks or powerdomain. This is what pwm-imx27 actually does. This >>>>> might lead to a, probably platform dependent, (undefined?) state of >>>>> the PWM output pin. This implies it is not possible to disable the >>>>> PWM periphery for inverted signals, if the disabled state is not the >>>>> inactive level. You know all about it already. >>>>> Then again from pwm-fan side I want be able to disable the FAN, >>>>> turning of >>>>> regulator and PWM, so powersaving is possible. That's what this patch >>>>> is >>>>> about. This is similar also what pwm_bl is doing. >>>>> Independent of the exact semantics, it makes sense to disable the >>>>> regulator in pwm-fan as well when the fan shall be disabled. >>>> >>>> There are fans which never stop if pwm==0, such as some CPU fans. I >>>> don't >>> >>> I assume with pwm==0 you actually mean duty_cycle == 0? >> >> Correct. The "pwm" attribute sets the duty cycle. >> >>>> think it is a good idea to force those off by turning off their power. >>>> The >>>> problem in the driver is that it treats pwm==0 as "disable pwm", not as >>>> "set pwm output to 0", Part of the probem may be that the ABI doesn't >>>> have >>>> a good representation for "disable pwm output", which is what is really >>>> wanted/needed here. >>> >>> Disable pwm output == set pwm output to High-Z? Not all PWMs are able to >>> provide that. >> >> It is up to us to define whate it means exactly. If you are ok that "set >> duty cycle to 0" reflects "set duty cycle to 0, disable pwm, and turn off >> regulator", I would hope that you are ok with using the _enable attribute >> to do the same and leaving pwm==0 to do what it is supposed to do, ie to >> keep pwm control enabled and set the duty cycle to 0. > > Just to make sure to be on the same side and summarize a bit. What you mean is > to add a new sysfs attribute to pwm-fan driver which controls what pwm_duty==0 > implies. I would suggest to name is 'keep_pwm_enabled' (but I am open for > other suggestions) with the following meaning: > 1 - pwm_duty==0 just means that. Set PWM duty to 0 and keep PWM (and fan > regulator) enabled. > No, I am not suggesting that. I am suggesting to add support for the existing standard attribute pwm1_enable, and define what its values mean in the driver documentation. There is no need to provide a non-standard attribute. Guenter > 0 - pwm_duty==0 means that the PWM duty is set to 0, PWM is disabled and any > PWM fan regulator is disabled as well. > > For pwm_duty!=0 this setting is irrelevant. Having the default to be '1' seems > sensible in order to not brake boards as regulator will be kept enabled. PWM > duty and/or PWM disable is irrelevant as PWM inversion is not yet supported > properly anyway. > > IMHO this should address all the mentioned issues. With 'keep_pwm_enabled=1' > only the duty is set and the regulators are not forced to be disabled. E.g. > the CPU fans mentioned by Guenter. This is also the case for hardware where > the regulator is shared or not switchable at all. > On hardware where it is safe to disable the regulator and PWM > keep_pwm_enabled=0' allows the system to poweroff PWM and powersupply for the > fan to improve powersavings. > > With this it is up to the administrator to provide the correct setting for > this attribute as it highly depends on the actual hardware and/or usage. > > Best regards, > Alexander > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-05-09 22:19 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-04 12:45 [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator Alexander Stein 2022-05-05 22:00 ` Guenter Roeck 2022-05-06 7:15 ` (EXT) " Markus Niebel 2022-05-06 7:15 ` Alexander Stein 2022-05-06 8:20 ` Uwe Kleine-König 2022-05-06 8:35 ` (EXT) " Alexander Stein 2022-05-06 10:23 ` Uwe Kleine-König 2022-05-06 12:23 ` (EXT) " Alexander Stein 2022-05-06 14:12 ` Guenter Roeck 2022-05-06 14:29 ` Uwe Kleine-König 2022-05-06 18:31 ` Guenter Roeck 2022-05-09 7:39 ` Alexander Stein 2022-05-09 10:59 ` Uwe Kleine-König 2022-05-09 12:10 ` Alexander Stein 2022-05-09 22:19 ` Guenter Roeck
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.