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().
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
