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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 4256 bytes --]

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

-- 
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 --]