lkml.org 
[lkml]   [2023]   [Oct]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] media: pwm-ir-tx: trigger edges from hrtimer interrupt context
From
Date
Hi,

On 1.10.23 г. 13:40 ч., Sean Young wrote:
> The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers
> from delays as this is done in process context. Make this work in atomic
> context.
>
> This makes the driver much more precise.
>
> Signed-off-by: Sean Young <sean@mess.org>
> Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> ---
> drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
> 1 file changed, 63 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
> index c5f37c03af9c..557725a07a67 100644
> --- a/drivers/media/rc/pwm-ir-tx.c
> +++ b/drivers/media/rc/pwm-ir-tx.c
> @@ -10,6 +10,8 @@
> #include <linux/slab.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> +#include <linux/hrtimer.h>
> +#include <linux/completion.h>
> #include <media/rc-core.h>
>
> #define DRIVER_NAME "pwm-ir-tx"
> @@ -17,8 +19,13 @@
>
> struct pwm_ir {
> struct pwm_device *pwm;
> - unsigned int carrier;
> - unsigned int duty_cycle;
> + struct hrtimer timer;
> + struct completion completion;
> + uint carrier;
> + uint duty_cycle;
> + uint *txbuf;
> + uint txbuf_len;
> + uint txbuf_index;
> };
>
> static const struct of_device_id pwm_ir_of_match[] = {
> @@ -55,33 +62,65 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
> struct pwm_ir *pwm_ir = dev->priv;
> struct pwm_device *pwm = pwm_ir->pwm;
> struct pwm_state state;
> - int i;
> - ktime_t edge;
> - long delta;
> +
> + reinit_completion(&pwm_ir->completion);

You should not need that.

>
> pwm_init_state(pwm, &state);
>
> state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
> pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
> + state.enabled = false;
>
> - edge = ktime_get();
> + pwm_ir->txbuf = txbuf;
> + pwm_ir->txbuf_len = count;
> + pwm_ir->txbuf_index = 0;
>
> - for (i = 0; i < count; i++) {
> - state.enabled = !(i % 2);
> - pwm_apply_state(pwm, &state);
> + pwm_apply_state(pwm, &state);
>

ditto, first pwm control should be in the timer function

> - edge = ktime_add_us(edge, txbuf[i]);
> - delta = ktime_us_delta(edge, ktime_get());
> - if (delta > 0)
> - usleep_range(delta, delta + 10);
> - }
> + hrtimer_start(&pwm_ir->timer, 1000, HRTIMER_MODE_REL);
>

why not just call it with 0 time?

> - state.enabled = false;
> - pwm_apply_state(pwm, &state);
> + wait_for_completion(&pwm_ir->completion);
>
> return count;
> }
>
> +static enum hrtimer_restart pwm_ir_timer(struct hrtimer *timer)
> +{
> + struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
> + ktime_t now;
> +
> + /*
> + * If we happen to hit an odd latency spike, loop through the
> + * pulses until we catch up.
> + */
> + do {
> + u64 ns;
> +
> + if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) {
> + /* Stop TX here */
> + pwm_disable(pwm_ir->pwm);
> +
> + complete(&pwm_ir->completion);
> +
> + return HRTIMER_NORESTART;
> + }
> +
> + if (pwm_ir->txbuf_index % 2)
> + pwm_disable(pwm_ir->pwm);
> + else
> + pwm_enable(pwm_ir->pwm);
> +

pwm_ir->pwm->state.enabled = !(pwm_ir->txbuf_index % 2);
pwm_apply_state(pwm_ir->pwm, pwm_ir->state);

> + ns = US_TO_NS(pwm_ir->txbuf[pwm_ir->txbuf_index]);
> + hrtimer_add_expires_ns(timer, ns);
> +
> + pwm_ir->txbuf_index++;
> +
> + now = timer->base->get_time();
> + } while (hrtimer_get_expires_tv64(timer) < now);
> +
> + return HRTIMER_RESTART;
> +}
> +
> static int pwm_ir_probe(struct platform_device *pdev)
> {
> struct pwm_ir *pwm_ir;
> @@ -96,8 +135,16 @@ static int pwm_ir_probe(struct platform_device *pdev)
> if (IS_ERR(pwm_ir->pwm))
> return PTR_ERR(pwm_ir->pwm);
>
> + if (pwm_can_sleep(pwm_ir->pwm)) {
> + dev_err(&pdev->dev, "unsupported pwm device: driver can sleep\n");
> + return -ENODEV;
> + }
> +

I think we shall not limit, but use high priority thread to support
those drivers. I have that working on n900 with current (sleeping) pwm,
see my reply on the other mail. Maybe we can combine both patches in a
way to support both atomic and sleeping pwm drivers.

> pwm_ir->carrier = 38000;
> pwm_ir->duty_cycle = 50;
> + init_completion(&pwm_ir->completion);
> + hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + pwm_ir->timer.function = pwm_ir_timer;
>
> rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
> if (!rcdev)
>

Regards,
Ivo

\
 
 \ /
  Last update: 2023-10-02 07:50    [W:0.138 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site