Re: [PATCH v4 6/6] hwmon: pwm-fan: Remove internal duplicated pwm_state

[Alexander Stein <alexander.stein@ew.tq-group.com>]

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