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:
signature.asc
Description: PGP signature
