Messages in this thread | | | Subject | Re: [PATCH v6] staging: greybus: introduce pwm_ops::apply | From | chensong <> | Date | Fri, 18 Mar 2022 20:58:12 +0800 |
| |
hi Alex,
On 2022/3/18 下午8:15, Alex Elder wrote: > 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.
sorry about that, i probably missed it somehow. Thanks for the understanding.
Song
> > 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, >> }; > >
| |