Messages in this thread | | | Date | Fri, 18 Mar 2022 07:15:34 -0500 | Subject | Re: [PATCH v6] staging: greybus: introduce pwm_ops::apply | From | Alex Elder <> |
| |
On 3/18/22 4:57 AM, Song Chen wrote: > Introduce newer .apply function in pwm_ops to replace legacy operations > including enable, disable, config and set_polarity. > > This guarantees atomic changes of the pwm controller configuration. > > Signed-off-by: Song Chen <chensong_2000@189.cn>
I had another comment suggestion but you've been through enough. Thanks for working this to completion.
Reviewed-by: Alex Elder <elder@linaro.org>
> > --- > 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. > > v6: > 1, revise commit message. > 2, explain why capping the value of period and duty to U32_MAX in > comment. > --- > drivers/staging/greybus/pwm.c | 64 ++++++++++++++++++++++------------- > 1 file changed, 40 insertions(+), 24 deletions(-) > > diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c > index 891a6a672378..ad20ec24031e 100644 > --- a/drivers/staging/greybus/pwm.c > +++ b/drivers/staging/greybus/pwm.c > @@ -204,43 +204,59 @@ 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 > + * > + * PWM privodes 64-bit period and duty_cycle, but greybus only accepts > + * 32-bit, so their values have to be limited to U32_MAX. > + */ > + 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, > }; >
| |