| Message ID | 20220523110513.407516-7-alexander.stein@ew.tq-group.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | hwmon: pwm-fan: switch regulator dynamically | expand |
Hello, On Mon, May 23, 2022 at 01:05:13PM +0200, Alexander Stein wrote: > Each pwm device has already a pwm_state. Use this one instead of > managing an own copy of it. > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > --- > drivers/hwmon/pwm-fan.c | 49 +++++++++++++++++++++++++---------------- > 1 file changed, 30 insertions(+), 19 deletions(-) > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > index e5d4b3b1cc49..e0ce81cdf5e0 100644 > --- a/drivers/hwmon/pwm-fan.c > +++ b/drivers/hwmon/pwm-fan.c > @@ -40,7 +40,6 @@ struct pwm_fan_ctx { > > struct mutex lock; > struct pwm_device *pwm; > - struct pwm_state pwm_state; > struct regulator *reg_en; > enum pwm_fan_enable_mode enable_mode; > bool regulator_enabled; > @@ -142,7 +141,7 @@ static int pwm_fan_switch_power(struct pwm_fan_ctx *ctx, bool on) > > static int pwm_fan_power_on(struct pwm_fan_ctx *ctx) > { > - struct pwm_state *state = &ctx->pwm_state; > + struct pwm_state state; > int ret; > > if (ctx->enabled) > @@ -154,8 +153,9 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx) > return ret; > } > > - state->enabled = true; > - ret = pwm_apply_state(ctx->pwm, state); > + pwm_get_state(ctx->pwm, &state); > + state.enabled = true; > + ret = pwm_apply_state(ctx->pwm, &state); > if (ret) { > dev_err(ctx->dev, "failed to enable PWM\n"); > goto disable_regulator; IMHO this isn't a net win. You trade the overhead of pwm_get_state against some memory savings. I personally am not a big fan of the get_state + modify + apply codeflow. The PWM framework does internal caching of the last applied state, but the details are a bit ugly. (i.e. pwm_get_state returns the last applied state, unless there was no state applied before. In that case it returns what .get_state returned during request time, unless there is no .get_state callback ... not sure if the device tree stuff somehow goes into that, didn't find it on a quick glance) Also there is a (small) danger, that pwm_state contains something that isn't intended by the driver, e.g. a wrong polarity. So I like the consumer to fully specify what they intend and not use pwm_get_state(). Best regards Uwe
Hi Uwe, Am Montag, 23. Mai 2022, 14:46:14 CEST schrieb Uwe Kleine-König: > * PGP Signed by an unknown key > > Hello, > > On Mon, May 23, 2022 at 01:05:13PM +0200, Alexander Stein wrote: > > Each pwm device has already a pwm_state. Use this one instead of > > managing an own copy of it. > > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > --- > > > > drivers/hwmon/pwm-fan.c | 49 +++++++++++++++++++++++++---------------- > > 1 file changed, 30 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > > index e5d4b3b1cc49..e0ce81cdf5e0 100644 > > --- a/drivers/hwmon/pwm-fan.c > > +++ b/drivers/hwmon/pwm-fan.c > > @@ -40,7 +40,6 @@ struct pwm_fan_ctx { > > > > struct mutex lock; > > struct pwm_device *pwm; > > > > - struct pwm_state pwm_state; > > > > struct regulator *reg_en; > > enum pwm_fan_enable_mode enable_mode; > > bool regulator_enabled; > > > > @@ -142,7 +141,7 @@ static int pwm_fan_switch_power(struct pwm_fan_ctx > > *ctx, bool on)> > > static int pwm_fan_power_on(struct pwm_fan_ctx *ctx) > > { > > > > - struct pwm_state *state = &ctx->pwm_state; > > + struct pwm_state state; > > > > int ret; > > > > if (ctx->enabled) > > > > @@ -154,8 +153,9 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx) > > > > return ret; > > > > } > > > > - state->enabled = true; > > - ret = pwm_apply_state(ctx->pwm, state); > > + pwm_get_state(ctx->pwm, &state); > > + state.enabled = true; > > + ret = pwm_apply_state(ctx->pwm, &state); > > > > if (ret) { > > > > dev_err(ctx->dev, "failed to enable PWM\n"); > > goto disable_regulator; > > IMHO this isn't a net win. You trade the overhead of pwm_get_state > against some memory savings. I personally am not a big fan of the > get_state + modify + apply codeflow. The PWM framework does internal > caching of the last applied state, but the details are a bit ugly. (i.e. > pwm_get_state returns the last applied state, unless there was no state > applied before. In that case it returns what .get_state returned during > request time, unless there is no .get_state callback ... not sure if the > device tree stuff somehow goes into that, didn't find it on a quick > glance) > > Also there is a (small) danger, that pwm_state contains something that > isn't intended by the driver, e.g. a wrong polarity. So I like the > consumer to fully specify what they intend and not use pwm_get_state(). Ah, I see. I have no hard feelings for this patch. I just wondered why the PWM state is duplicated. and wanted to get rid of it. If there is a specific reason for this, I'm ok with that. Best regards, Alexander
On 5/23/22 06:55, Alexander Stein wrote: > Hi Uwe, > > Am Montag, 23. Mai 2022, 14:46:14 CEST schrieb Uwe Kleine-König: >> * PGP Signed by an unknown key >> >> Hello, >> >> On Mon, May 23, 2022 at 01:05:13PM +0200, Alexander Stein wrote: >>> Each pwm device has already a pwm_state. Use this one instead of >>> managing an own copy of it. >>> >>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> >>> --- >>> >>> drivers/hwmon/pwm-fan.c | 49 +++++++++++++++++++++++++---------------- >>> 1 file changed, 30 insertions(+), 19 deletions(-) >>> >>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c >>> index e5d4b3b1cc49..e0ce81cdf5e0 100644 >>> --- a/drivers/hwmon/pwm-fan.c >>> +++ b/drivers/hwmon/pwm-fan.c >>> @@ -40,7 +40,6 @@ struct pwm_fan_ctx { >>> >>> struct mutex lock; >>> struct pwm_device *pwm; >>> >>> - struct pwm_state pwm_state; >>> >>> struct regulator *reg_en; >>> enum pwm_fan_enable_mode enable_mode; >>> bool regulator_enabled; >>> >>> @@ -142,7 +141,7 @@ static int pwm_fan_switch_power(struct pwm_fan_ctx >>> *ctx, bool on)> >>> static int pwm_fan_power_on(struct pwm_fan_ctx *ctx) >>> { >>> >>> - struct pwm_state *state = &ctx->pwm_state; >>> + struct pwm_state state; >>> >>> int ret; >>> >>> if (ctx->enabled) >>> >>> @@ -154,8 +153,9 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx) >>> >>> return ret; >>> >>> } >>> >>> - state->enabled = true; >>> - ret = pwm_apply_state(ctx->pwm, state); >>> + pwm_get_state(ctx->pwm, &state); >>> + state.enabled = true; >>> + ret = pwm_apply_state(ctx->pwm, &state); >>> >>> if (ret) { >>> >>> dev_err(ctx->dev, "failed to enable PWM\n"); >>> goto disable_regulator; >> >> IMHO this isn't a net win. You trade the overhead of pwm_get_state >> against some memory savings. I personally am not a big fan of the >> get_state + modify + apply codeflow. The PWM framework does internal >> caching of the last applied state, but the details are a bit ugly. (i.e. >> pwm_get_state returns the last applied state, unless there was no state >> applied before. In that case it returns what .get_state returned during >> request time, unless there is no .get_state callback ... not sure if the >> device tree stuff somehow goes into that, didn't find it on a quick >> glance) >> >> Also there is a (small) danger, that pwm_state contains something that >> isn't intended by the driver, e.g. a wrong polarity. So I like the >> consumer to fully specify what they intend and not use pwm_get_state(). > > Ah, I see. I have no hard feelings for this patch. I just wondered why the PWM > state is duplicated. and wanted to get rid of it. If there is a specific > reason for this, I'm ok with that. > I don't see the value of continuous runtime overhead to save a few bytes of data, so I don't see a reason to _not_ cache the state locally. This is similar to caching a clock frequency locally instead of calling the clock subsystem again and again to read it. Sure, nowadays CPUs are more powerful than they used to be, but I don't see that as reason or argument for wasting their power. Guenter
Hello Guenter, Uwe, Am Montag, 23. Mai 2022, 16:18:57 CEST schrieb Guenter Roeck: > On 5/23/22 06:55, Alexander Stein wrote: > > Hi Uwe, > > > > Am Montag, 23. Mai 2022, 14:46:14 CEST schrieb Uwe Kleine-König: > >> * PGP Signed by an unknown key > >> > >> Hello, > >> > >> On Mon, May 23, 2022 at 01:05:13PM +0200, Alexander Stein wrote: > >>> Each pwm device has already a pwm_state. Use this one instead of > >>> managing an own copy of it. > >>> > >>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > >>> --- > >>> > >>> drivers/hwmon/pwm-fan.c | 49 +++++++++++++++++++++++++---------------- > >>> 1 file changed, 30 insertions(+), 19 deletions(-) > >>> > >>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > >>> index e5d4b3b1cc49..e0ce81cdf5e0 100644 > >>> --- a/drivers/hwmon/pwm-fan.c > >>> +++ b/drivers/hwmon/pwm-fan.c > >>> @@ -40,7 +40,6 @@ struct pwm_fan_ctx { > >>> > >>> struct mutex lock; > >>> struct pwm_device *pwm; > >>> > >>> - struct pwm_state pwm_state; > >>> > >>> struct regulator *reg_en; > >>> enum pwm_fan_enable_mode enable_mode; > >>> bool regulator_enabled; > >>> > >>> @@ -142,7 +141,7 @@ static int pwm_fan_switch_power(struct pwm_fan_ctx > >>> *ctx, bool on)> > >>> > >>> static int pwm_fan_power_on(struct pwm_fan_ctx *ctx) > >>> { > >>> > >>> - struct pwm_state *state = &ctx->pwm_state; > >>> + struct pwm_state state; > >>> > >>> int ret; > >>> > >>> if (ctx->enabled) > >>> > >>> @@ -154,8 +153,9 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx) > >>> > >>> return ret; > >>> > >>> } > >>> > >>> - state->enabled = true; > >>> - ret = pwm_apply_state(ctx->pwm, state); > >>> + pwm_get_state(ctx->pwm, &state); > >>> + state.enabled = true; > >>> + ret = pwm_apply_state(ctx->pwm, &state); > >>> > >>> if (ret) { > >>> > >>> dev_err(ctx->dev, "failed to enable PWM\n"); > >>> goto disable_regulator; > >> > >> IMHO this isn't a net win. You trade the overhead of pwm_get_state > >> against some memory savings. I personally am not a big fan of the > >> get_state + modify + apply codeflow. The PWM framework does internal > >> caching of the last applied state, but the details are a bit ugly. (i.e. > >> pwm_get_state returns the last applied state, unless there was no state > >> applied before. In that case it returns what .get_state returned during > >> request time, unless there is no .get_state callback ... not sure if the > >> device tree stuff somehow goes into that, didn't find it on a quick > >> glance) > >> > >> Also there is a (small) danger, that pwm_state contains something that > >> isn't intended by the driver, e.g. a wrong polarity. So I like the > >> consumer to fully specify what they intend and not use pwm_get_state(). > > > > Ah, I see. I have no hard feelings for this patch. I just wondered why the > > PWM state is duplicated. and wanted to get rid of it. If there is a > > specific reason for this, I'm ok with that. > > I don't see the value of continuous runtime overhead to save a few bytes of > data, so I don't see a reason to _not_ cache the state locally. This is > similar to caching a clock frequency locally instead of calling the clock > subsystem again and again to read it. Sure, nowadays CPUs are more powerful > than they used to be, but I don't see that as reason or argument for > wasting their power. Ok, seems reasonable. I'm fully fine with patch 6 being dropped. What about the other patches? Best regards, Alexander
Hello Alexander, On Tue, Jun 21, 2022 at 08:41:37AM +0200, Alexander Stein wrote: > Am Montag, 23. Mai 2022, 16:18:57 CEST schrieb Guenter Roeck: > > On 5/23/22 06:55, Alexander Stein wrote: > > > Hi Uwe, > > > > > > Am Montag, 23. Mai 2022, 14:46:14 CEST schrieb Uwe Kleine-König: > > >> * PGP Signed by an unknown key > > >> > > >> Hello, > > >> > > >> On Mon, May 23, 2022 at 01:05:13PM +0200, Alexander Stein wrote: > > >>> Each pwm device has already a pwm_state. Use this one instead of > > >>> managing an own copy of it. > > >>> > > >>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > >>> --- > > >>> > > >>> drivers/hwmon/pwm-fan.c | 49 +++++++++++++++++++++++++---------------- > > >>> 1 file changed, 30 insertions(+), 19 deletions(-) > > >>> > > >>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > > >>> index e5d4b3b1cc49..e0ce81cdf5e0 100644 > > >>> --- a/drivers/hwmon/pwm-fan.c > > >>> +++ b/drivers/hwmon/pwm-fan.c > > >>> @@ -40,7 +40,6 @@ struct pwm_fan_ctx { > > >>> > > >>> struct mutex lock; > > >>> struct pwm_device *pwm; > > >>> > > >>> - struct pwm_state pwm_state; > > >>> > > >>> struct regulator *reg_en; > > >>> enum pwm_fan_enable_mode enable_mode; > > >>> bool regulator_enabled; > > >>> > > >>> @@ -142,7 +141,7 @@ static int pwm_fan_switch_power(struct pwm_fan_ctx > > >>> *ctx, bool on)> > > >>> > > >>> static int pwm_fan_power_on(struct pwm_fan_ctx *ctx) > > >>> { > > >>> > > >>> - struct pwm_state *state = &ctx->pwm_state; > > >>> + struct pwm_state state; > > >>> > > >>> int ret; > > >>> > > >>> if (ctx->enabled) > > >>> > > >>> @@ -154,8 +153,9 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx) > > >>> > > >>> return ret; > > >>> > > >>> } > > >>> > > >>> - state->enabled = true; > > >>> - ret = pwm_apply_state(ctx->pwm, state); > > >>> + pwm_get_state(ctx->pwm, &state); > > >>> + state.enabled = true; > > >>> + ret = pwm_apply_state(ctx->pwm, &state); > > >>> > > >>> if (ret) { > > >>> > > >>> dev_err(ctx->dev, "failed to enable PWM\n"); > > >>> goto disable_regulator; > > >> > > >> IMHO this isn't a net win. You trade the overhead of pwm_get_state > > >> against some memory savings. I personally am not a big fan of the > > >> get_state + modify + apply codeflow. The PWM framework does internal > > >> caching of the last applied state, but the details are a bit ugly. (i.e. > > >> pwm_get_state returns the last applied state, unless there was no state > > >> applied before. In that case it returns what .get_state returned during > > >> request time, unless there is no .get_state callback ... not sure if the > > >> device tree stuff somehow goes into that, didn't find it on a quick > > >> glance) > > >> > > >> Also there is a (small) danger, that pwm_state contains something that > > >> isn't intended by the driver, e.g. a wrong polarity. So I like the > > >> consumer to fully specify what they intend and not use pwm_get_state(). > > > > > > Ah, I see. I have no hard feelings for this patch. I just wondered why the > > > PWM state is duplicated. and wanted to get rid of it. If there is a > > > specific reason for this, I'm ok with that. > > > > I don't see the value of continuous runtime overhead to save a few bytes of > > data, so I don't see a reason to _not_ cache the state locally. This is > > similar to caching a clock frequency locally instead of calling the clock > > subsystem again and again to read it. Sure, nowadays CPUs are more powerful > > than they used to be, but I don't see that as reason or argument for > > wasting their power. > > Ok, seems reasonable. I'm fully fine with patch 6 being dropped. What about > the other patches? +1 for dropping patch #6. Otherwise (with my PWM expert hat on) I have no further criticism. But I didn't look deep enough into the patches for an Ack, I guess I'm also missing some hwmon foo to objectively review further. Best regards Uwe
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index e5d4b3b1cc49..e0ce81cdf5e0 100644 --- a/drivers/hwmon/pwm-fan.c +++ b/drivers/hwmon/pwm-fan.c @@ -40,7 +40,6 @@ struct pwm_fan_ctx { struct mutex lock; struct pwm_device *pwm; - struct pwm_state pwm_state; struct regulator *reg_en; enum pwm_fan_enable_mode enable_mode; bool regulator_enabled; @@ -142,7 +141,7 @@ static int pwm_fan_switch_power(struct pwm_fan_ctx *ctx, bool on) static int pwm_fan_power_on(struct pwm_fan_ctx *ctx) { - struct pwm_state *state = &ctx->pwm_state; + struct pwm_state state; int ret; if (ctx->enabled) @@ -154,8 +153,9 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx) return ret; } - state->enabled = true; - ret = pwm_apply_state(ctx->pwm, state); + pwm_get_state(ctx->pwm, &state); + state.enabled = true; + ret = pwm_apply_state(ctx->pwm, &state); if (ret) { dev_err(ctx->dev, "failed to enable PWM\n"); goto disable_regulator; @@ -172,19 +172,20 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx) static int pwm_fan_power_off(struct pwm_fan_ctx *ctx) { - struct pwm_state *state = &ctx->pwm_state; bool enable_regulator = false; + struct pwm_state state; if (!ctx->enabled) return 0; pwm_fan_enable_mode_2_state(ctx->enable_mode, - state, + &state, &enable_regulator); - state->enabled = false; - state->duty_cycle = 0; - pwm_apply_state(ctx->pwm, state); + pwm_get_state(ctx->pwm, &state); + state.enabled = false; + state.duty_cycle = 0; + pwm_apply_state(ctx->pwm, &state); pwm_fan_switch_power(ctx, enable_regulator); @@ -195,7 +196,7 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx) static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm) { - struct pwm_state *state = &ctx->pwm_state; + struct pwm_state state; unsigned long period; int ret = 0; @@ -204,9 +205,10 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm) /* pwm-fan hard disabled */ return 0; - period = state->period; - state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM); - ret = pwm_apply_state(ctx->pwm, state); + pwm_get_state(ctx->pwm, &state); + period = state.period; + state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM); + ret = pwm_apply_state(ctx->pwm, &state); if (ret) return ret; ret = pwm_fan_power_on(ctx); @@ -266,15 +268,16 @@ static int pwm_fan_update_enable(struct pwm_fan_ctx *ctx, long val) * Nothing to do if currently enabled */ if (!ctx->enabled) { - struct pwm_state *state = &ctx->pwm_state; bool enable_regulator = false; + struct pwm_state state; - state->duty_cycle = 0; + pwm_get_state(ctx->pwm, &state); + state.duty_cycle = 0; pwm_fan_enable_mode_2_state(val, - state, + &state, &enable_regulator); - pwm_apply_state(ctx->pwm, state); + pwm_apply_state(ctx->pwm, &state); pwm_fan_switch_power(ctx, enable_regulator); pwm_fan_update_state(ctx, 0); } @@ -473,6 +476,7 @@ static int pwm_fan_probe(struct platform_device *pdev) struct thermal_cooling_device *cdev; struct device *dev = &pdev->dev; struct pwm_fan_ctx *ctx; + struct pwm_state state; struct device *hwmon; int ret; const struct hwmon_channel_info **channels; @@ -501,18 +505,25 @@ static int pwm_fan_probe(struct platform_device *pdev) ctx->reg_en = NULL; } - pwm_init_state(ctx->pwm, &ctx->pwm_state); + pwm_init_state(ctx->pwm, &state); /* * set_pwm assumes that MAX_PWM * (period - 1) fits into an unsigned * long. Check this here to prevent the fan running at a too low * frequency. */ - if (ctx->pwm_state.period > ULONG_MAX / MAX_PWM + 1) { + if (state.period > ULONG_MAX / MAX_PWM + 1) { dev_err(dev, "Configured period too big\n"); return -EINVAL; } + /* Apply modified PWM default state */ + ret = pwm_apply_state(ctx->pwm, &state); + if (ret) { + dev_err(dev, "failed to apply initial PWM state: %d\n", ret); + return -EINVAL; + } + ctx->enable_mode = pwm_disable_reg_enable; /*
Each pwm device has already a pwm_state. Use this one instead of managing an own copy of it. Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> --- drivers/hwmon/pwm-fan.c | 49 +++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 19 deletions(-)