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@xxxxxxxxxxxxxxx>
---
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
