| Message ID | 20200607181840.13536-11-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API | expand |
On Sun, Jun 07, 2020 at 08:18:35PM +0200, Hans de Goede wrote: > Replace the enable, disable and config pwm_ops with an apply op, > to support the new atomic PWM API. ... > -static int crc_pwm_calc_clk_div(int period_ns) > +static int crc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > { > - int clk_div; > - > - clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_MHZ); > - /* clk_div 1 - 128, maps to register values 0-127 */ > - if (clk_div > 0) > - clk_div--; > - > - return clk_div; > -} ... > + clk_div = PWM_BASE_CLK_MHZ * state->period / > + (256 * NSEC_PER_MHZ); > + /* clk_div 1 - 128, maps to register values 0-127 */ > + if (clk_div > 0) > + clk_div--; And again... :-(
Hi, On 6/9/20 1:32 PM, Andy Shevchenko wrote: > On Sun, Jun 07, 2020 at 08:18:35PM +0200, Hans de Goede wrote: >> Replace the enable, disable and config pwm_ops with an apply op, >> to support the new atomic PWM API. > > ... > >> -static int crc_pwm_calc_clk_div(int period_ns) >> +static int crc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, >> + const struct pwm_state *state) >> { >> - int clk_div; >> - >> - clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_MHZ); >> - /* clk_div 1 - 128, maps to register values 0-127 */ >> - if (clk_div > 0) >> - clk_div--; >> - >> - return clk_div; >> -} > > ... > >> + clk_div = PWM_BASE_CLK_MHZ * state->period / >> + (256 * NSEC_PER_MHZ); >> + /* clk_div 1 - 128, maps to register values 0-127 */ >> + if (clk_div > 0) >> + clk_div--; > > And again... :-( Well yes I cannot help it that the original code, as submitted by Intel, was of very questionable quality, so instead of just converting it to the atomic PWM API I had to do a ton of bugfixes first... I tried to do this all in small bits rather then in a single big rewrite the buggy <beep> commit to make life easier for reviewers. I can introduce the crc_pwm_calc_clk_div helper earlier as you suggested in an earlier mail. I guess I could also keep the helper here, and then fold it into the function in a later commit (*). Would that work for you ? Regards, Hans *) Because having a helper for 3 lines of code when it is used only once is not helpful IMHO, it only makes it harder to figure out what the code is exactly doing when readin the code.
On Tue, Jun 09, 2020 at 03:44:18PM +0200, Hans de Goede wrote: > On 6/9/20 1:32 PM, Andy Shevchenko wrote: > > On Sun, Jun 07, 2020 at 08:18:35PM +0200, Hans de Goede wrote: ... > > And again... :-( > > Well yes I cannot help it that the original code, as submitted by Intel, > was of very questionable quality, so instead of just converting it to the > atomic PWM API I had to do a ton of bugfixes first... I tried to do > this all in small bits rather then in a single big rewrite the buggy > <beep> commit to make life easier for reviewers. Yes, I know about that old code quality, sorry, we were not at Intel that time (or were just right-less newbies). > I can introduce the crc_pwm_calc_clk_div helper earlier as you suggested > in an earlier mail. I guess I could also keep the helper here, and then > fold it into the function in a later commit (*). > > Would that work for you ? Definitely. > *) Because having a helper for 3 lines of code when it is used only > once is not helpful IMHO, it only makes it harder to figure out what > the code is exactly doing when readin the code. At least it will reduce churn to just 1) introduce foo(); 2) do many changes with foo() being used; 3) drop foo() *if* it's not needed / makes little sense.
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index 53734bcf67e1..58c7e9ef7278 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -41,70 +41,81 @@ static inline struct crystalcove_pwm *to_crc_pwm(struct pwm_chip *pc) return container_of(pc, struct crystalcove_pwm, chip); } -static int crc_pwm_calc_clk_div(int period_ns) +static int crc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) { - int clk_div; - - clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_MHZ); - /* clk_div 1 - 128, maps to register values 0-127 */ - if (clk_div > 0) - clk_div--; - - return clk_div; -} - -static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm) -{ - struct crystalcove_pwm *crc_pwm = to_crc_pwm(c); - int div = crc_pwm_calc_clk_div(pwm_get_period(pwm)); - - regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, div | PWM_OUTPUT_ENABLE); - regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 1); - - return 0; -} - -static void crc_pwm_disable(struct pwm_chip *c, struct pwm_device *pwm) -{ - struct crystalcove_pwm *crc_pwm = to_crc_pwm(c); - int div = crc_pwm_calc_clk_div(pwm_get_period(pwm)); - - regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 0); - regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, div); -} - -static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm, - int duty_ns, int period_ns) -{ - struct crystalcove_pwm *crc_pwm = to_crc_pwm(c); + struct crystalcove_pwm *crc_pwm = to_crc_pwm(chip); + int err, clk_div, level, pwm_output_enable; struct device *dev = crc_pwm->chip.dev; - int level; - if (period_ns > PWM_MAX_PERIOD_NS) { + if (state->period > PWM_MAX_PERIOD_NS) { dev_err(dev, "un-supported period_ns\n"); return -EINVAL; } - if (pwm_get_period(pwm) != period_ns) { - int clk_div = crc_pwm_calc_clk_div(period_ns); + if (state->polarity != PWM_POLARITY_NORMAL) + return -ENOTSUPP; + + if (pwm_is_enabled(pwm) && !state->enabled) { + err = regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 0); + if (err) { + dev_err(dev, "Error writing BACKLIGHT_EN %d\n", err); + return err; + } + } + + if (pwm_get_duty_cycle(pwm) != state->duty_cycle || + pwm_get_period(pwm) != state->period) { + level = state->duty_cycle * PWM_MAX_LEVEL / state->period; + err = regmap_write(crc_pwm->regmap, PWM0_DUTY_CYCLE, level); + if (err) { + dev_err(dev, "Error writing PWM0_DUTY_CYCLE %d\n", err); + return err; + } + } + + if (pwm_is_enabled(pwm) && state->enabled && + pwm_get_period(pwm) != state->period) { /* changing the clk divisor, clear PWM_OUTPUT_ENABLE first */ - regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, 0); - regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, - clk_div | PWM_OUTPUT_ENABLE); + err = regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, 0); + if (err) { + dev_err(dev, "Error writing PWM0_CLK_DIV %d\n", err); + return err; + } } - /* change the pwm duty cycle */ - level = duty_ns * PWM_MAX_LEVEL / period_ns; - regmap_write(crc_pwm->regmap, PWM0_DUTY_CYCLE, level); + if (pwm_get_period(pwm) != state->period || + pwm_is_enabled(pwm) != state->enabled) { + clk_div = PWM_BASE_CLK_MHZ * state->period / + (256 * NSEC_PER_MHZ); + /* clk_div 1 - 128, maps to register values 0-127 */ + if (clk_div > 0) + clk_div--; + + pwm_output_enable = state->enabled ? PWM_OUTPUT_ENABLE : 0; + + err = regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, + clk_div | pwm_output_enable); + if (err) { + dev_err(dev, "Error writing PWM0_CLK_DIV %d\n", err); + return err; + } + } + + if (!pwm_is_enabled(pwm) && state->enabled) { + err = regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 1); + if (err) { + dev_err(dev, "Error writing BACKLIGHT_EN %d\n", err); + return err; + } + } return 0; } static const struct pwm_ops crc_pwm_ops = { - .config = crc_pwm_config, - .enable = crc_pwm_enable, - .disable = crc_pwm_disable, + .apply = crc_pwm_apply, }; static int crystalcove_pwm_probe(struct platform_device *pdev)
Replace the enable, disable and config pwm_ops with an apply op, to support the new atomic PWM API. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/pwm/pwm-crc.c | 107 +++++++++++++++++++++++------------------- 1 file changed, 59 insertions(+), 48 deletions(-)