| Message ID | 20200224052135.17278-5-lokeshvutla@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: omap-dmtimer: Allow for dynamic pwm period updates | expand |
On Mon, Feb 24, 2020 at 10:51:35AM +0530, Lokesh Vutla wrote: > Implement .apply callback and drop the legacy callbacks(enable, disable, > config, set_polarity). > > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > --- > drivers/pwm/pwm-omap-dmtimer.c | 141 +++++++++++++++++++-------------- > 1 file changed, 80 insertions(+), 61 deletions(-) > > diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c > index 58c61559e72f..aeda4ab12385 100644 > --- a/drivers/pwm/pwm-omap-dmtimer.c > +++ b/drivers/pwm/pwm-omap-dmtimer.c > @@ -31,8 +31,18 @@ > #define DM_TIMER_LOAD_MIN 0xfffffffe > #define DM_TIMER_MAX 0xffffffff > > +/** > + * struct pwm_omap_dmtimer_chip - Structure representing a pwm chip > + * corresponding to omap dmtimer. > + * @chip: PWM chip structure representing PWM controller > + * @mutex: Mutex to protect pwm apply state > + * @dm_timer: Pointer to omap dm timer. > + * @pdata: Pointer to omap dm timer ops. > + * dm_timer_pdev: Pointer to omap dm timer platform device > + */ > struct pwm_omap_dmtimer_chip { > struct pwm_chip chip; > + /* Mutex to protect pwm apply state */ > struct mutex mutex; > struct omap_dm_timer *dm_timer; > const struct omap_dm_timer_ops *pdata; > @@ -45,11 +55,22 @@ to_pwm_omap_dmtimer_chip(struct pwm_chip *chip) > return container_of(chip, struct pwm_omap_dmtimer_chip, chip); > } > > +/** > + * pwm_omap_dmtimer_get_clock_cycles() - Get clock cycles in a time frame > + * @clk_rate: pwm timer clock rate > + * @ns: time frame in nano seconds. > + * > + * Return number of clock cycles in a given period(ins ns). > + */ > static u32 pwm_omap_dmtimer_get_clock_cycles(unsigned long clk_rate, int ns) > { > return DIV_ROUND_CLOSEST_ULL((u64)clk_rate * ns, NSEC_PER_SEC); > } > > +/** > + * pwm_omap_dmtimer_start() - Start the pwm omap dm timer in pwm mode > + * @omap: Pointer to pwm omap dm timer chip > + */ > static void pwm_omap_dmtimer_start(struct pwm_omap_dmtimer_chip *omap) > { > /* > @@ -67,32 +88,16 @@ static void pwm_omap_dmtimer_start(struct pwm_omap_dmtimer_chip *omap) > omap->pdata->start(omap->dm_timer); > } > > -static int pwm_omap_dmtimer_enable(struct pwm_chip *chip, > - struct pwm_device *pwm) > -{ > - struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip); > - > - mutex_lock(&omap->mutex); > - omap->pdata->set_pwm(omap->dm_timer, > - pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED, > - true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE); > - > - pwm_omap_dmtimer_start(omap); > - mutex_unlock(&omap->mutex); > - > - return 0; > -} > - > -static void pwm_omap_dmtimer_disable(struct pwm_chip *chip, > - struct pwm_device *pwm) > -{ > - struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip); > - > - mutex_lock(&omap->mutex); > - omap->pdata->stop(omap->dm_timer); > - mutex_unlock(&omap->mutex); > -} > - > +/** > + * pwm_omap_dmtimer_config() - Update the configuration of pwm omap dm timer > + * @chip: Pointer to PWM controller > + * @pwm: Pointer to PWM channel > + * @duty_ns: New duty cycle in nano seconds > + * @period_ns: New period in nano seconds > + * > + * Return 0 if successfully changed the period/duty_cycle else appropriate > + * error. > + */ > static int pwm_omap_dmtimer_config(struct pwm_chip *chip, > struct pwm_device *pwm, > int duty_ns, int period_ns) > @@ -100,30 +105,26 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, > struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip); > u32 period_cycles, duty_cycles; > u32 load_value, match_value; > - struct clk *fclk; > unsigned long clk_rate; > + struct clk *fclk; > > dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n", > duty_ns, period_ns); > > - mutex_lock(&omap->mutex); > if (duty_ns == pwm_get_duty_cycle(pwm) && > - period_ns == pwm_get_period(pwm)) { > - /* No change - don't cause any transients. */ > - mutex_unlock(&omap->mutex); > + period_ns == pwm_get_period(pwm)) > return 0; > - } > > fclk = omap->pdata->get_fclk(omap->dm_timer); > if (!fclk) { > dev_err(chip->dev, "invalid pmtimer fclk\n"); > - goto err_einval; > + return -EINVAL; > } > > clk_rate = clk_get_rate(fclk); > if (!clk_rate) { > dev_err(chip->dev, "invalid pmtimer fclk rate\n"); > - goto err_einval; > + return -EINVAL; > } > > dev_dbg(chip->dev, "clk rate: %luHz\n", clk_rate); > @@ -151,7 +152,7 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, > dev_info(chip->dev, > "period %d ns too short for clock rate %lu Hz\n", > period_ns, clk_rate); > - goto err_einval; > + return -EINVAL; > } > > if (duty_cycles < 1) { > @@ -183,54 +184,72 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, > dev_dbg(chip->dev, "load value: %#08x (%d), match value: %#08x (%d)\n", > load_value, load_value, match_value, match_value); > > - mutex_unlock(&omap->mutex); > - > return 0; > - > -err_einval: > - mutex_unlock(&omap->mutex); > - > - return -EINVAL; > } > > -static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip, > - struct pwm_device *pwm, > - enum pwm_polarity polarity) > +/** > + * pwm_omap_dmtimer_apply() - Changes the state of the pwm omap dm timer. > + * @chip: Pointer to PWM controller > + * @pwm: Pointer to PWM channel > + * @state: New sate to apply > + * > + * Return 0 if successfully changed the state else appropriate error. > + */ > +static int pwm_omap_dmtimer_apply(struct pwm_chip *chip, > + struct pwm_device *pwm, > + const struct pwm_state *state) > { > struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip); > + int ret = 0; > > - /* > - * PWM core will not call set_polarity while PWM is enabled so it's > - * safe to reconfigure the timer here without stopping it first. > - */ > mutex_lock(&omap->mutex); > - omap->pdata->set_pwm(omap->dm_timer, > - polarity == PWM_POLARITY_INVERSED, > - true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE); > + > + if (pwm_is_enabled(pwm) && !state->enabled) { In my book calling PWM API functions (designed for PWM consumers) is not so nice. I would prefer you checking the hardware registers or cache the state locally instead of relying on the core here. It would be great to have a general description at the top of the driver (like for example drivers/pwm/pwm-sifive.c) that answers things like: - Does calling .stop completes the currently running period (it should)? - Does changing polarity, duty_cycle and period complete the running period? - How does the hardware behave on disable? (i.e. does it output the state the pin is at in that moment? Does it go High-Z?) Best regards Uwe
Hi Uwe, On 24/02/20 2:37 PM, Uwe Kleine-König wrote: > On Mon, Feb 24, 2020 at 10:51:35AM +0530, Lokesh Vutla wrote: >> Implement .apply callback and drop the legacy callbacks(enable, disable, >> config, set_polarity). >> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> >> --- >> drivers/pwm/pwm-omap-dmtimer.c | 141 +++++++++++++++++++-------------- >> 1 file changed, 80 insertions(+), 61 deletions(-) >> [..snip..] >> -static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip, >> - struct pwm_device *pwm, >> - enum pwm_polarity polarity) >> +/** >> + * pwm_omap_dmtimer_apply() - Changes the state of the pwm omap dm timer. >> + * @chip: Pointer to PWM controller >> + * @pwm: Pointer to PWM channel >> + * @state: New sate to apply >> + * >> + * Return 0 if successfully changed the state else appropriate error. >> + */ >> +static int pwm_omap_dmtimer_apply(struct pwm_chip *chip, >> + struct pwm_device *pwm, >> + const struct pwm_state *state) >> { >> struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip); >> + int ret = 0; >> >> - /* >> - * PWM core will not call set_polarity while PWM is enabled so it's >> - * safe to reconfigure the timer here without stopping it first. >> - */ >> mutex_lock(&omap->mutex); >> - omap->pdata->set_pwm(omap->dm_timer, >> - polarity == PWM_POLARITY_INVERSED, >> - true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE); >> + >> + if (pwm_is_enabled(pwm) && !state->enabled) { > > In my book calling PWM API functions (designed for PWM consumers) is not > so nice. I would prefer you checking the hardware registers or cache the > state locally instead of relying on the core here. .start and .stop apis does read the hardware registers and check the state before making any changes. Do you want to drop off the pwm_is_enabled(pwm) check here? > > It would be great to have a general description at the top of the driver > (like for example drivers/pwm/pwm-sifive.c) that answers things like: > > - Does calling .stop completes the currently running period (it > should)? Existing driver implementation abruptly stops the cycle. I can make changes such that it completes the currently running period. > - Does changing polarity, duty_cycle and period complete the running > period? - Polarity can be changed only when the pwm is not running. Ill add extra guards to reflect this behavior. - Changing duty_cycle and period does complete the running period and new values gets reflected in next cycle. > - How does the hardware behave on disable? (i.e. does it output the > state the pin is at in that moment? Does it go High-Z?) Now that I am making changes to complete the current period on disable, the pin goes to Low after disabling(completing the cycle). Ill add all these points as you mentioned in v2. Thanks and regards, Lokesh
Hello Lokesh, On Tue, Feb 25, 2020 at 10:31:45AM +0530, Lokesh Vutla wrote: > Hi Uwe, > > On 24/02/20 2:37 PM, Uwe Kleine-König wrote: > > On Mon, Feb 24, 2020 at 10:51:35AM +0530, Lokesh Vutla wrote: > >> Implement .apply callback and drop the legacy callbacks(enable, disable, > >> config, set_polarity). > >> > >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > >> --- > >> drivers/pwm/pwm-omap-dmtimer.c | 141 +++++++++++++++++++-------------- > >> 1 file changed, 80 insertions(+), 61 deletions(-) > >> > > [..snip..] > > >> -static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip, > >> - struct pwm_device *pwm, > >> - enum pwm_polarity polarity) > >> +/** > >> + * pwm_omap_dmtimer_apply() - Changes the state of the pwm omap dm timer. > >> + * @chip: Pointer to PWM controller > >> + * @pwm: Pointer to PWM channel > >> + * @state: New sate to apply > >> + * > >> + * Return 0 if successfully changed the state else appropriate error. > >> + */ > >> +static int pwm_omap_dmtimer_apply(struct pwm_chip *chip, > >> + struct pwm_device *pwm, > >> + const struct pwm_state *state) > >> { > >> struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip); > >> + int ret = 0; > >> > >> - /* > >> - * PWM core will not call set_polarity while PWM is enabled so it's > >> - * safe to reconfigure the timer here without stopping it first. > >> - */ > >> mutex_lock(&omap->mutex); > >> - omap->pdata->set_pwm(omap->dm_timer, > >> - polarity == PWM_POLARITY_INVERSED, > >> - true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE); > >> + > >> + if (pwm_is_enabled(pwm) && !state->enabled) { > > > > In my book calling PWM API functions (designed for PWM consumers) is not > > so nice. I would prefer you checking the hardware registers or cache the > > state locally instead of relying on the core here. > > .start and .stop apis does read the hardware registers and check the state > before making any changes. Do you want to drop off the pwm_is_enabled(pwm) check > here? The IMHO more natural approach would be to look into the hardware registers instead of asking the framework. > > It would be great to have a general description at the top of the driver > > (like for example drivers/pwm/pwm-sifive.c) that answers things like: > > > > - Does calling .stop completes the currently running period (it > > should)? > > Existing driver implementation abruptly stops the cycle. I can make changes such > that it completes the currently running period. That would be good for correctness. > > - Does changing polarity, duty_cycle and period complete the running > > period? > > - Polarity can be changed only when the pwm is not running. Ill add extra guards > to reflect this behavior. > - Changing duty_cycle and period does complete the running period and new values > gets reflected in next cycle. Is there are race with the hardware? I.e. can it happen that when a new cycle starts just when you configured the new period but not the duty_cycle yet a mixed cycle is output? > > - How does the hardware behave on disable? (i.e. does it output the > > state the pin is at in that moment? Does it go High-Z?) > > Now that I am making changes to complete the current period on disable, the pin > goes to Low after disabling(completing the cycle). > > Ill add all these points as you mentioned in v2. Great Best regards Uwe
diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c index 58c61559e72f..aeda4ab12385 100644 --- a/drivers/pwm/pwm-omap-dmtimer.c +++ b/drivers/pwm/pwm-omap-dmtimer.c @@ -31,8 +31,18 @@ #define DM_TIMER_LOAD_MIN 0xfffffffe #define DM_TIMER_MAX 0xffffffff +/** + * struct pwm_omap_dmtimer_chip - Structure representing a pwm chip + * corresponding to omap dmtimer. + * @chip: PWM chip structure representing PWM controller + * @mutex: Mutex to protect pwm apply state + * @dm_timer: Pointer to omap dm timer. + * @pdata: Pointer to omap dm timer ops. + * dm_timer_pdev: Pointer to omap dm timer platform device + */ struct pwm_omap_dmtimer_chip { struct pwm_chip chip; + /* Mutex to protect pwm apply state */ struct mutex mutex; struct omap_dm_timer *dm_timer; const struct omap_dm_timer_ops *pdata; @@ -45,11 +55,22 @@ to_pwm_omap_dmtimer_chip(struct pwm_chip *chip) return container_of(chip, struct pwm_omap_dmtimer_chip, chip); } +/** + * pwm_omap_dmtimer_get_clock_cycles() - Get clock cycles in a time frame + * @clk_rate: pwm timer clock rate + * @ns: time frame in nano seconds. + * + * Return number of clock cycles in a given period(ins ns). + */ static u32 pwm_omap_dmtimer_get_clock_cycles(unsigned long clk_rate, int ns) { return DIV_ROUND_CLOSEST_ULL((u64)clk_rate * ns, NSEC_PER_SEC); } +/** + * pwm_omap_dmtimer_start() - Start the pwm omap dm timer in pwm mode + * @omap: Pointer to pwm omap dm timer chip + */ static void pwm_omap_dmtimer_start(struct pwm_omap_dmtimer_chip *omap) { /* @@ -67,32 +88,16 @@ static void pwm_omap_dmtimer_start(struct pwm_omap_dmtimer_chip *omap) omap->pdata->start(omap->dm_timer); } -static int pwm_omap_dmtimer_enable(struct pwm_chip *chip, - struct pwm_device *pwm) -{ - struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip); - - mutex_lock(&omap->mutex); - omap->pdata->set_pwm(omap->dm_timer, - pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED, - true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE); - - pwm_omap_dmtimer_start(omap); - mutex_unlock(&omap->mutex); - - return 0; -} - -static void pwm_omap_dmtimer_disable(struct pwm_chip *chip, - struct pwm_device *pwm) -{ - struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip); - - mutex_lock(&omap->mutex); - omap->pdata->stop(omap->dm_timer); - mutex_unlock(&omap->mutex); -} - +/** + * pwm_omap_dmtimer_config() - Update the configuration of pwm omap dm timer + * @chip: Pointer to PWM controller + * @pwm: Pointer to PWM channel + * @duty_ns: New duty cycle in nano seconds + * @period_ns: New period in nano seconds + * + * Return 0 if successfully changed the period/duty_cycle else appropriate + * error. + */ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, struct pwm_device *pwm, int duty_ns, int period_ns) @@ -100,30 +105,26 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip); u32 period_cycles, duty_cycles; u32 load_value, match_value; - struct clk *fclk; unsigned long clk_rate; + struct clk *fclk; dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n", duty_ns, period_ns); - mutex_lock(&omap->mutex); if (duty_ns == pwm_get_duty_cycle(pwm) && - period_ns == pwm_get_period(pwm)) { - /* No change - don't cause any transients. */ - mutex_unlock(&omap->mutex); + period_ns == pwm_get_period(pwm)) return 0; - } fclk = omap->pdata->get_fclk(omap->dm_timer); if (!fclk) { dev_err(chip->dev, "invalid pmtimer fclk\n"); - goto err_einval; + return -EINVAL; } clk_rate = clk_get_rate(fclk); if (!clk_rate) { dev_err(chip->dev, "invalid pmtimer fclk rate\n"); - goto err_einval; + return -EINVAL; } dev_dbg(chip->dev, "clk rate: %luHz\n", clk_rate); @@ -151,7 +152,7 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, dev_info(chip->dev, "period %d ns too short for clock rate %lu Hz\n", period_ns, clk_rate); - goto err_einval; + return -EINVAL; } if (duty_cycles < 1) { @@ -183,54 +184,72 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, dev_dbg(chip->dev, "load value: %#08x (%d), match value: %#08x (%d)\n", load_value, load_value, match_value, match_value); - mutex_unlock(&omap->mutex); - return 0; - -err_einval: - mutex_unlock(&omap->mutex); - - return -EINVAL; } -static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip, - struct pwm_device *pwm, - enum pwm_polarity polarity) +/** + * pwm_omap_dmtimer_apply() - Changes the state of the pwm omap dm timer. + * @chip: Pointer to PWM controller + * @pwm: Pointer to PWM channel + * @state: New sate to apply + * + * Return 0 if successfully changed the state else appropriate error. + */ +static int pwm_omap_dmtimer_apply(struct pwm_chip *chip, + struct pwm_device *pwm, + const struct pwm_state *state) { struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip); + int ret = 0; - /* - * PWM core will not call set_polarity while PWM is enabled so it's - * safe to reconfigure the timer here without stopping it first. - */ mutex_lock(&omap->mutex); - omap->pdata->set_pwm(omap->dm_timer, - polarity == PWM_POLARITY_INVERSED, - true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE); + + if (pwm_is_enabled(pwm) && !state->enabled) { + omap->pdata->stop(omap->dm_timer); + goto unlock_mutex; + } + + if (pwm_get_polarity(pwm) != state->polarity) + omap->pdata->set_pwm(omap->dm_timer, + state->polarity == PWM_POLARITY_INVERSED, + true, + OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE); + + ret = pwm_omap_dmtimer_config(chip, pwm, state->duty_cycle, + state->period); + if (ret) + goto unlock_mutex; + + if (!pwm_is_enabled(pwm) && state->enabled) { + omap->pdata->set_pwm(omap->dm_timer, + state->polarity == PWM_POLARITY_INVERSED, + true, + OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE); + pwm_omap_dmtimer_start(omap); + } + +unlock_mutex: mutex_unlock(&omap->mutex); - return 0; + return ret; } static const struct pwm_ops pwm_omap_dmtimer_ops = { - .enable = pwm_omap_dmtimer_enable, - .disable = pwm_omap_dmtimer_disable, - .config = pwm_omap_dmtimer_config, - .set_polarity = pwm_omap_dmtimer_set_polarity, + .apply = pwm_omap_dmtimer_apply, .owner = THIS_MODULE, }; static int pwm_omap_dmtimer_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; - struct device_node *timer; - struct platform_device *timer_pdev; - struct pwm_omap_dmtimer_chip *omap; struct dmtimer_platform_data *timer_pdata; const struct omap_dm_timer_ops *pdata; + struct platform_device *timer_pdev; + struct pwm_omap_dmtimer_chip *omap; struct omap_dm_timer *dm_timer; - u32 v; + struct device_node *timer; int ret = 0; + u32 v; timer = of_parse_phandle(np, "ti,timers", 0); if (!timer)
Implement .apply callback and drop the legacy callbacks(enable, disable, config, set_polarity). Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> --- drivers/pwm/pwm-omap-dmtimer.c | 141 +++++++++++++++++++-------------- 1 file changed, 80 insertions(+), 61 deletions(-)