diff mbox series

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

Message ID 20220523110513.407516-7-alexander.stein@ew.tq-group.com (mailing list archive)
State New
Headers show
Series hwmon: pwm-fan: switch regulator dynamically | expand

Commit Message

Alexander Stein May 23, 2022, 11:05 a.m. UTC
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(-)

Comments

Uwe Kleine-König May 23, 2022, 12:46 p.m. UTC | #1
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
Alexander Stein May 23, 2022, 1:55 p.m. UTC | #2
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
Guenter Roeck May 23, 2022, 2:18 p.m. UTC | #3
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
Alexander Stein June 21, 2022, 6:41 a.m. UTC | #4
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
Uwe Kleine-König June 21, 2022, 7:22 a.m. UTC | #5
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 mbox series

Patch

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;
 
 	/*