[PATCH v5] staging: greybus: introduce pwm_ops::apply

[PATCH v5] staging: greybus: introduce pwm_ops::apply

[Song Chen]

Introduce newer .apply function in pwm_ops to replace legacy operations,
like enable, disable, config and set_polarity.

This guarantees atomic changes of the pwm controller configuration.

Signed-off-by: Song Chen <chensong_2000@189.cn>

---
v2:
1, define duty_cycle and period as u64 in gb_pwm_config_operation.
2, define duty and period as u64 in gb_pwm_config_request.
3, disable before configuring duty and period if the eventual goal
   is a disabled state.

v3:
Regarding duty_cycle and period, I read more discussion in this thread,
min, warn or -EINVAL, seems no perfect way acceptable for everyone.
How about we limit their value to INT_MAX and throw a warning at the
same time when they are wrong?

v4:
1, explain why legacy operations are replaced.
2, cap the value of period and duty to U32_MAX.

v5:
1, revise commit message.
---
 drivers/staging/greybus/pwm.c | 59 +++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
index 891a6a672378..3add3032678b 100644
--- a/drivers/staging/greybus/pwm.c
+++ b/drivers/staging/greybus/pwm.c
@@ -204,43 +204,54 @@ static void gb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	gb_pwm_deactivate_operation(pwmc, pwm->hwpwm);
 }
 
-static int gb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			 int duty_ns, int period_ns)
+static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			const struct pwm_state *state)
 {
+	int err;
+	bool enabled = pwm->state.enabled;
+	u64 period = state->period;
+	u64 duty_cycle = state->duty_cycle;
 	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
 
-	return gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_ns, period_ns);
-};
+	/* set polarity */
+	if (state->polarity != pwm->state.polarity) {
+		if (enabled) {
+			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
+			enabled = false;
+		}
+		err = gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, state->polarity);
+		if (err)
+			return err;
+	}
 
-static int gb_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
-			       enum pwm_polarity polarity)
-{
-	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
+	if (!state->enabled) {
+		if (enabled)
+			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
+		return 0;
+	}
 
-	return gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, polarity);
-};
+	/* set period and duty cycle*/
+	if (period > U32_MAX)
+		period = U32_MAX;
 
-static int gb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
+	if (duty_cycle > period)
+		duty_cycle = period;
 
-	return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
-};
+	err = gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_cycle, period);
+	if (err)
+		return err;
 
-static void gb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
+	/* enable/disable */
+	if (!enabled)
+		return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
 
-	gb_pwm_disable_operation(pwmc, pwm->hwpwm);
-};
+	return 0;
+}
 
 static const struct pwm_ops gb_pwm_ops = {
 	.request = gb_pwm_request,
 	.free = gb_pwm_free,
-	.config = gb_pwm_config,
-	.set_polarity = gb_pwm_set_polarity,
-	.enable = gb_pwm_enable,
-	.disable = gb_pwm_disable,
+	.apply = gb_pwm_apply,
 	.owner = THIS_MODULE,
 };
 
-- 
2.25.1

Re: [PATCH v5] staging: greybus: introduce pwm_ops::apply

[Alex Elder]

On 3/15/22 9:21 PM, Song Chen wrote:
> Introduce newer .apply function in pwm_ops to replace legacy operations,
> like enable, disable, config and set_polarity.

It's not just "like" those four, it replaces *exactly* those
four operations.

> This guarantees atomic changes of the pwm controller configuration.
> 
> Signed-off-by: Song Chen <chensong_2000@189.cn>

I see that support for the "atomic" ->apply operation was added
by commit 5ec803edcb703 ("pwm: Add core infrastructure to allow
atomic updates
").  And what you're doing here is removing the
enable, disable, set_polarity, and config operations provided
in this driver to use the new apply callback instead.

And that in turn depends on a prior commit (and another commit
or two after that) 43a276b003ed2 ("pwm: Introduce the pwm_state
concept
") to wrap the current state stored in the device in a
sub-structure.

As I understand it, if the period, duty cycle, polarity, usage power,
or enabled state of the device differs from the current state of
the device, the new ->apply callback changes the device's state to
match what is requested.

Please see my comments below.

> ---
> v2:
> 1, define duty_cycle and period as u64 in gb_pwm_config_operation.
> 2, define duty and period as u64 in gb_pwm_config_request.
> 3, disable before configuring duty and period if the eventual goal
>     is a disabled state.
> 
> v3:
> Regarding duty_cycle and period, I read more discussion in this thread,
> min, warn or -EINVAL, seems no perfect way acceptable for everyone.
> How about we limit their value to INT_MAX and throw a warning at the
> same time when they are wrong?
> 
> v4:
> 1, explain why legacy operations are replaced.
> 2, cap the value of period and duty to U32_MAX.
> 
> v5:
> 1, revise commit message.
> ---
>   drivers/staging/greybus/pwm.c | 59 +++++++++++++++++++++--------------
>   1 file changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
> index 891a6a672378..3add3032678b 100644
> --- a/drivers/staging/greybus/pwm.c
> +++ b/drivers/staging/greybus/pwm.c
> @@ -204,43 +204,54 @@ static void gb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>   	gb_pwm_deactivate_operation(pwmc, pwm->hwpwm);
>   }
>   
> -static int gb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> -			 int duty_ns, int period_ns)
> +static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			const struct pwm_state *state)
>   {
> +	int err;
> +	bool enabled = pwm->state.enabled;
> +	u64 period = state->period;
> +	u64 duty_cycle = state->duty_cycle;

The use of local variables here is inconsistent, and that
can be confusing.  Specifically, the "enabled" variable
represents the *current* state, while the "period" and
"duty_cycle" variables represent the *desired* state.  To
avoid confusion, if you're going to use local variables
like that, they should all represent *either* the current
state *or* the new state.  Please update your patch to
do one or the other.

>   	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>   
> -	return gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_ns, period_ns);
> -};
> +	/* set polarity */
> +	if (state->polarity != pwm->state.polarity) {
> +		if (enabled) {
> +			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
> +			enabled = false;
> +		}
> +		err = gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, state->polarity);
> +		if (err)
> +			return err;
> +	}
>   
> -static int gb_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> -			       enum pwm_polarity polarity)
> -{
> -	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
> +	if (!state->enabled) {
> +		if (enabled)
> +			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
> +		return 0;

If you are disabling the device, you return without updating the
period and duty cycle.  But you *do* set polarity.  Is that
required by the PWM API?  (I don't actually know.)  Or can the
polarity setting be simply ignored as well if the new state is
disabled?

Also, if the polarity changed, the device will have already been
disabled above, so there's no need to do so again (and perhaps
it might be a bad thing to do twice?).

> +	}
>   
> -	return gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, polarity);
> -};

Since you're clamping the values to 32 bits here, your comment
should explain why (because Greybus uses 32-bit values here,
while the API supports 64 bit values).  That would be a much
more useful piece of information than "set period and duty cycle".

> +	/* set period and duty cycle*/

Include a space before "*/" in your comments.

> +	if (period > U32_MAX)
> +		period = U32_MAX;
>   
> -static int gb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> -	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
> +	if (duty_cycle > period)
> +		duty_cycle = period;
>   
> -	return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
> -};
> +	err = gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_cycle, period);
> +	if (err)
> +		return err;

What if the new state set usage_power to true?  It would
be ignored here.  Is it OK to silently ignore it?  Even
if it is, a comment about that would be good to see, so
we know it's intentional.

					-Alex

> -static void gb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> -	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
> +	/* enable/disable */
> +	if (!enabled)
> +		return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
>   
> -	gb_pwm_disable_operation(pwmc, pwm->hwpwm);
> -};
> +	return 0;
> +}
>   
>   static const struct pwm_ops gb_pwm_ops = {
>   	.request = gb_pwm_request,
>   	.free = gb_pwm_free,
> -	.config = gb_pwm_config,
> -	.set_polarity = gb_pwm_set_polarity,
> -	.enable = gb_pwm_enable,
> -	.disable = gb_pwm_disable,
> +	.apply = gb_pwm_apply,
>   	.owner = THIS_MODULE,
>   };
>   

Re: [PATCH v5] staging: greybus: introduce pwm_ops::apply

[Uwe Kleine-König]

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

On Wed, Mar 16, 2022 at 10:14:30AM -0500, Alex Elder wrote:
> On 3/15/22 9:21 PM, Song Chen wrote:
> > diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
> > index 891a6a672378..3add3032678b 100644
> > --- a/drivers/staging/greybus/pwm.c
> > +++ b/drivers/staging/greybus/pwm.c
> > @@ -204,43 +204,54 @@ static void gb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> >   	gb_pwm_deactivate_operation(pwmc, pwm->hwpwm);
> >   }
> > -static int gb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > -			 int duty_ns, int period_ns)
> > +static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			const struct pwm_state *state)
> >   {
> > +	int err;
> > +	bool enabled = pwm->state.enabled;
> > +	u64 period = state->period;
> > +	u64 duty_cycle = state->duty_cycle;
> 
> The use of local variables here is inconsistent, and that
> can be confusing.  Specifically, the "enabled" variable
> represents the *current* state, while the "period" and
> "duty_cycle" variables represent the *desired* state.  To
> avoid confusion, if you're going to use local variables
> like that, they should all represent *either* the current
> state *or* the new state.  Please update your patch to
> do one or the other.

IMHO that it overly picky. I'm ok with the usage as is.

> >   	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
> > -	return gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_ns, period_ns);
> > -};
> > +	/* set polarity */
> > +	if (state->polarity != pwm->state.polarity) {
> > +		if (enabled) {
> > +			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
> > +			enabled = false;
> > +		}
> > +		err = gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, state->polarity);
> > +		if (err)
> > +			return err;
> > +	}
> > -static int gb_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> > -			       enum pwm_polarity polarity)
> > -{
> > -	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
> > +	if (!state->enabled) {
> > +		if (enabled)
> > +			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
> > +		return 0;
> 
> If you are disabling the device, you return without updating the
> period and duty cycle.  But you *do* set polarity.  Is that
> required by the PWM API?  (I don't actually know.)  Or can the
> polarity setting be simply ignored as well if the new state is
> disabled?

All is well here. A disabled PWM is expected to emit the inactive level.
So polarity matters, duty and period don't.

> Also, if the polarity changed, the device will have already been
> disabled above, so there's no need to do so again (and perhaps
> it might be a bad thing to do twice?).

That won't happen, because if the device was disabled for the polarity
change, enabled = false. In fact that is the purpose of the local
variable.

> > +	}
> > -	return gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, polarity);
> > -};
> 
> Since you're clamping the values to 32 bits here, your comment
> should explain why (because Greybus uses 32-bit values here,
> while the API supports 64 bit values).  That would be a much
> more useful piece of information than "set period and duty cycle".
> 
> > +	/* set period and duty cycle*/
> 
> Include a space before "*/" in your comments.

ack

> > +	if (period > U32_MAX)
> > +		period = U32_MAX;
> > -static int gb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > -{
> > -	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
> > +	if (duty_cycle > period)
> > +		duty_cycle = period;
> > -	return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
> > -};
> > +	err = gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_cycle, period);
> > +	if (err)
> > +		return err;
> 
> What if the new state set usage_power to true?  It would
> be ignored here.  Is it OK to silently ignore it?  Even
> if it is, a comment about that would be good to see, so
> we know it's intentional.

ignoring usage_power is OK. All but a single driver do it that way.

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

Re: [greybus-dev] Re: [PATCH v5] staging: greybus: introduce pwm_ops::apply

[Alex Elder]

On 3/16/22 11:29 AM, Uwe Kleine-König wrote:
> On Wed, Mar 16, 2022 at 10:14:30AM -0500, Alex Elder wrote:
>> On 3/15/22 9:21 PM, Song Chen wrote:
>>> diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
>>> index 891a6a672378..3add3032678b 100644
>>> --- a/drivers/staging/greybus/pwm.c
>>> +++ b/drivers/staging/greybus/pwm.c
>>> @@ -204,43 +204,54 @@ static void gb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>>>    	gb_pwm_deactivate_operation(pwmc, pwm->hwpwm);
>>>    }
>>> -static int gb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>> -			 int duty_ns, int period_ns)
>>> +static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>> +			const struct pwm_state *state)
>>>    {
>>> +	int err;
>>> +	bool enabled = pwm->state.enabled;
>>> +	u64 period = state->period;
>>> +	u64 duty_cycle = state->duty_cycle;
>>
>> The use of local variables here is inconsistent, and that
>> can be confusing.  Specifically, the "enabled" variable
>> represents the *current* state, while the "period" and
>> "duty_cycle" variables represent the *desired* state.  To
>> avoid confusion, if you're going to use local variables
>> like that, they should all represent *either* the current
>> state *or* the new state.  Please update your patch to
>> do one or the other.
> 
> IMHO that it overly picky. I'm ok with the usage as is.

I see the "enabled" flag is used in a way that I didn't
notice before.  Changing its name to "disabled" (to mean
"we have disabled the device within this function already")
would allow it to be used in the same way, but would make
it more obvious it's not just a copy of "old" device state.

>>>    	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>>> -	return gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_ns, period_ns);
>>> -};
>>> +	/* set polarity */
>>> +	if (state->polarity != pwm->state.polarity) {
>>> +		if (enabled) {
>>> +			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
>>> +			enabled = false;
>>> +		}
>>> +		err = gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, state->polarity);
>>> +		if (err)
>>> +			return err;
>>> +	}
>>> -static int gb_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>>> -			       enum pwm_polarity polarity)
>>> -{
>>> -	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>>> +	if (!state->enabled) {
>>> +		if (enabled)
>>> +			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
>>> +		return 0;
>>
>> If you are disabling the device, you return without updating the
>> period and duty cycle.  But you *do* set polarity.  Is that
>> required by the PWM API?  (I don't actually know.)  Or can the
>> polarity setting be simply ignored as well if the new state is
>> disabled?
> 
> All is well here. A disabled PWM is expected to emit the inactive level.
> So polarity matters, duty and period don't.

Thanks for clarifying that.  I did not know what was expected.

>> Also, if the polarity changed, the device will have already been
>> disabled above, so there's no need to do so again (and perhaps
>> it might be a bad thing to do twice?).
> 
> That won't happen, because if the device was disabled for the polarity
> change, enabled = false. In fact that is the purpose of the local
> variable.

Now I see, yes, the local variable gets changed when the
disable occurred above.

>>> +	}
>>> -	return gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, polarity);
>>> -};
>>
>> Since you're clamping the values to 32 bits here, your comment
>> should explain why (because Greybus uses 32-bit values here,
>> while the API supports 64 bit values).  That would be a much
>> more useful piece of information than "set period and duty cycle".
>>
>>> +	/* set period and duty cycle*/
>>
>> Include a space before "*/" in your comments.
> 
> ack
> 
>>> +	if (period > U32_MAX)
>>> +		period = U32_MAX;
>>> -static int gb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>>> -{
>>> -	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>>> +	if (duty_cycle > period)
>>> +		duty_cycle = period;
>>> -	return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
>>> -};
>>> +	err = gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_cycle, period);
>>> +	if (err)
>>> +		return err;
>>
>> What if the new state set usage_power to true?  It would
>> be ignored here.  Is it OK to silently ignore it?  Even
>> if it is, a comment about that would be good to see, so
>> we know it's intentional.
> 
> ignoring usage_power is OK. All but a single driver do it that way.

I don't actually see anything that sets usage_power to true,
although "pwm-pca9685.c" tests its value.

I guess it's an advisory parameter that's passed to the apply
callback function.  It's described as optional, but--not being
a "PWM person"--this isn't obvious to me.  Maybe the comments
describing the field or the apply callback could define the
semantics a little better at some point.

					-Alex

> 
> Best regards
> Uwe
> 
> 
> _______________________________________________
> greybus-dev mailing list -- greybus-dev@lists.linaro.org
> To unsubscribe send an email to greybus-dev-leave@lists.linaro.org

Re: [greybus-dev] Re: [PATCH v5] staging: greybus: introduce pwm_ops::apply

[Uwe Kleine-König]

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

On Wed, Mar 16, 2022 at 12:20:11PM -0500, Alex Elder wrote:
> On 3/16/22 11:29 AM, Uwe Kleine-König wrote:
> > On Wed, Mar 16, 2022 at 10:14:30AM -0500, Alex Elder wrote:
> > > On 3/15/22 9:21 PM, Song Chen wrote:
> > > > diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
> > > > index 891a6a672378..3add3032678b 100644
> > > > --- a/drivers/staging/greybus/pwm.c
> > > > +++ b/drivers/staging/greybus/pwm.c
> > > > @@ -204,43 +204,54 @@ static void gb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > > >    	gb_pwm_deactivate_operation(pwmc, pwm->hwpwm);
> > > >    }
> > > > -static int gb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > -			 int duty_ns, int period_ns)
> > > > +static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > +			const struct pwm_state *state)
> > > >    {
> > > > +	int err;
> > > > +	bool enabled = pwm->state.enabled;
> > > > +	u64 period = state->period;
> > > > +	u64 duty_cycle = state->duty_cycle;
> > > 
> > > The use of local variables here is inconsistent, and that
> > > can be confusing.  Specifically, the "enabled" variable
> > > represents the *current* state, while the "period" and
> > > "duty_cycle" variables represent the *desired* state.  To
> > > avoid confusion, if you're going to use local variables
> > > like that, they should all represent *either* the current
> > > state *or* the new state.  Please update your patch to
> > > do one or the other.
> > 
> > IMHO that it overly picky. I'm ok with the usage as is.
> 
> I see the "enabled" flag is used in a way that I didn't
> notice before.  Changing its name to "disabled" (to mean
> "we have disabled the device within this function already")
> would allow it to be used in the same way, but would make
> it more obvious it's not just a copy of "old" device state.
> 
> > > >    	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
> > > > -	return gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_ns, period_ns);
> > > > -};
> > > > +	/* set polarity */
> > > > +	if (state->polarity != pwm->state.polarity) {
> > > > +		if (enabled) {
> > > > +			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
> > > > +			enabled = false;
> > > > +		}
> > > > +		err = gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, state->polarity);
> > > > +		if (err)
> > > > +			return err;
> > > > +	}
> > > > -static int gb_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > -			       enum pwm_polarity polarity)
> > > > -{
> > > > -	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
> > > > +	if (!state->enabled) {
> > > > +		if (enabled)
> > > > +			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
> > > > +		return 0;
> > > 
> > > If you are disabling the device, you return without updating the
> > > period and duty cycle.  But you *do* set polarity.  Is that
> > > required by the PWM API?  (I don't actually know.)  Or can the
> > > polarity setting be simply ignored as well if the new state is
> > > disabled?
> > 
> > All is well here. A disabled PWM is expected to emit the inactive level.
> > So polarity matters, duty and period don't.
> 
> Thanks for clarifying that.  I did not know what was expected.
> 
> > > Also, if the polarity changed, the device will have already been
> > > disabled above, so there's no need to do so again (and perhaps
> > > it might be a bad thing to do twice?).
> > 
> > That won't happen, because if the device was disabled for the polarity
> > change, enabled = false. In fact that is the purpose of the local
> > variable.
> 
> Now I see, yes, the local variable gets changed when the
> disable occurred above.
> 
> > > > +	}
> > > > -	return gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, polarity);
> > > > -};
> > > 
> > > Since you're clamping the values to 32 bits here, your comment
> > > should explain why (because Greybus uses 32-bit values here,
> > > while the API supports 64 bit values).  That would be a much
> > > more useful piece of information than "set period and duty cycle".
> > > 
> > > > +	/* set period and duty cycle*/
> > > 
> > > Include a space before "*/" in your comments.
> > 
> > ack
> > 
> > > > +	if (period > U32_MAX)
> > > > +		period = U32_MAX;
> > > > -static int gb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > > -{
> > > > -	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
> > > > +	if (duty_cycle > period)
> > > > +		duty_cycle = period;
> > > > -	return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
> > > > -};
> > > > +	err = gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_cycle, period);
> > > > +	if (err)
> > > > +		return err;
> > > 
> > > What if the new state set usage_power to true?  It would
> > > be ignored here.  Is it OK to silently ignore it?  Even
> > > if it is, a comment about that would be good to see, so
> > > we know it's intentional.
> > 
> > ignoring usage_power is OK. All but a single driver do it that way.
> 
> I don't actually see anything that sets usage_power to true,
> although "pwm-pca9685.c" tests its value.
> 
> I guess it's an advisory parameter that's passed to the apply
> callback function.  It's described as optional, but--not being
> a "PWM person"--this isn't obvious to me.  Maybe the comments
> describing the field or the apply callback could define the
> semantics a little better at some point.

One of the problems I see with usage_power is that it's not well
defined. The idea is that when usage_power is true, the driver is free
to implement any setting that just matches the relative duty_cycle of
the request. So if you call pwm_apply with

	.duty_cycle = 2000
	.period = 10000
	.usage_power = true
	.polarity = PWM_POLARITY_NORMAL

you can program the hardware to implement

	.duty_cycle = 200
	.period = 1000
	.polarity = PWM_POLARITY_NORMAL

or

	.duty_cycle = 8000000
	.period = 10000000
	.polarity = PWM_POLARITY_INVERTED

The expectation is however to only deviate in a sensible manner from the
request, whatever that might mean.

I don't see much value in that field, there is only one implementing
driver and no mainline user. If you're interested you can reread the
discussions about it in the archives.

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

Re: [greybus-dev] Re: [PATCH v5] staging: greybus: introduce pwm_ops::apply

[Song Chen]

hi Alex & Uwe,

Thanks for the advices and clarifications.

在 2022/3/17 04:05, Uwe Kleine-König 写道:
> On Wed, Mar 16, 2022 at 12:20:11PM -0500, Alex Elder wrote:
>> On 3/16/22 11:29 AM, Uwe Kleine-König wrote:
>>> On Wed, Mar 16, 2022 at 10:14:30AM -0500, Alex Elder wrote:
>>>> On 3/15/22 9:21 PM, Song Chen wrote:
>>>>> diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
>>>>> index 891a6a672378..3add3032678b 100644
>>>>> --- a/drivers/staging/greybus/pwm.c
>>>>> +++ b/drivers/staging/greybus/pwm.c
>>>>> @@ -204,43 +204,54 @@ static void gb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>>>>>     	gb_pwm_deactivate_operation(pwmc, pwm->hwpwm);
>>>>>     }
>>>>> -static int gb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>>>> -			 int duty_ns, int period_ns)
>>>>> +static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>>>> +			const struct pwm_state *state)
>>>>>     {
>>>>> +	int err;
>>>>> +	bool enabled = pwm->state.enabled;
>>>>> +	u64 period = state->period;
>>>>> +	u64 duty_cycle = state->duty_cycle;
>>>>
>>>> The use of local variables here is inconsistent, and that
>>>> can be confusing.  Specifically, the "enabled" variable
>>>> represents the *current* state, while the "period" and
>>>> "duty_cycle" variables represent the *desired* state.  To
>>>> avoid confusion, if you're going to use local variables
>>>> like that, they should all represent *either* the current
>>>> state *or* the new state.  Please update your patch to
>>>> do one or the other.
>>>
>>> IMHO that it overly picky. I'm ok with the usage as is.
>>
>> I see the "enabled" flag is used in a way that I didn't
>> notice before.  Changing its name to "disabled" (to mean
>> "we have disabled the device within this function already")
>> would allow it to be used in the same way, but would make
>> it more obvious it's not just a copy of "old" device state.

Some of drivers in driver/pwn use pwm->state.enabled directly in their 
apply and others name it "enabled", see pwm-tiecap.c, pwm-berlin.c, 
pwm-vt8500.c and pwm-stm32.c.

I prefer keeping consist with those drivers, "disabled" confuses me.

>>
>>>>>     	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>>>>> -	return gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_ns, period_ns);
>>>>> -};
>>>>> +	/* set polarity */
>>>>> +	if (state->polarity != pwm->state.polarity) {
>>>>> +		if (enabled) {
>>>>> +			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
>>>>> +			enabled = false;
>>>>> +		}
>>>>> +		err = gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, state->polarity);
>>>>> +		if (err)
>>>>> +			return err;
>>>>> +	}
>>>>> -static int gb_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>>>>> -			       enum pwm_polarity polarity)
>>>>> -{
>>>>> -	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>>>>> +	if (!state->enabled) {
>>>>> +		if (enabled)
>>>>> +			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
>>>>> +		return 0;
>>>>
>>>> If you are disabling the device, you return without updating the
>>>> period and duty cycle.  But you *do* set polarity.  Is that
>>>> required by the PWM API?  (I don't actually know.)  Or can the
>>>> polarity setting be simply ignored as well if the new state is
>>>> disabled?
>>>
>>> All is well here. A disabled PWM is expected to emit the inactive level.
>>> So polarity matters, duty and period don't.
>>
>> Thanks for clarifying that.  I did not know what was expected.
>>
>>>> Also, if the polarity changed, the device will have already been
>>>> disabled above, so there's no need to do so again (and perhaps
>>>> it might be a bad thing to do twice?).
>>>
>>> That won't happen, because if the device was disabled for the polarity
>>> change, enabled = false. In fact that is the purpose of the local
>>> variable.
>>
>> Now I see, yes, the local variable gets changed when the
>> disable occurred above.
>>
>>>>> +	}
>>>>> -	return gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, polarity);
>>>>> -};
>>>>
>>>> Since you're clamping the values to 32 bits here, your comment
>>>> should explain why (because Greybus uses 32-bit values here,
>>>> while the API supports 64 bit values).  That would be a much
>>>> more useful piece of information than "set period and duty cycle".
>>>>

done, thanks.

>>>>> +	/* set period and duty cycle*/
>>>>
>>>> Include a space before "*/" in your comments.
>>>
>>> ack
>>>

done, thanks.

>>>>> +	if (period > U32_MAX)
>>>>> +		period = U32_MAX;
>>>>> -static int gb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>>>>> -{
>>>>> -	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>>>>> +	if (duty_cycle > period)
>>>>> +		duty_cycle = period;
>>>>> -	return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
>>>>> -};
>>>>> +	err = gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_cycle, period);
>>>>> +	if (err)
>>>>> +		return err;
>>>>
>>>> What if the new state set usage_power to true?  It would
>>>> be ignored here.  Is it OK to silently ignore it?  Even
>>>> if it is, a comment about that would be good to see, so
>>>> we know it's intentional.
>>>
>>> ignoring usage_power is OK. All but a single driver do it that way.
>>
>> I don't actually see anything that sets usage_power to true,
>> although "pwm-pca9685.c" tests its value.
>>
>> I guess it's an advisory parameter that's passed to the apply
>> callback function.  It's described as optional, but--not being
>> a "PWM person"--this isn't obvious to me.  Maybe the comments
>> describing the field or the apply callback could define the
>> semantics a little better at some point.
> 
> One of the problems I see with usage_power is that it's not well
> defined. The idea is that when usage_power is true, the driver is free
> to implement any setting that just matches the relative duty_cycle of
> the request. So if you call pwm_apply with
> 
> 	.duty_cycle = 2000
> 	.period = 10000
> 	.usage_power = true
> 	.polarity = PWM_POLARITY_NORMAL
> 
> you can program the hardware to implement
> 
> 	.duty_cycle = 200
> 	.period = 1000
> 	.polarity = PWM_POLARITY_NORMAL
> 
> or
> 
> 	.duty_cycle = 8000000
> 	.period = 10000000
> 	.polarity = PWM_POLARITY_INVERTED
> 
> The expectation is however to only deviate in a sensible manner from the
> request, whatever that might mean.
> 
> I don't see much value in that field, there is only one implementing
> driver and no mainline user. If you're interested you can reread the
> discussions about it in the archives.
> 
> Best regards
> Uwe
> 

what's more, "like" in commit message, removed.

Thanks

BR

Song