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
_______________________________________________
greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx
To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx