Re: [PATCH v2 3/3] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Ivaylo Dimitrov]

On 13.10.23 г. 13:46 ч., Sean Young wrote:
> This makes the driver much more precise.
>
> Signed-off-by: Sean Young <sean@xxxxxxxx>
> ---
>   drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++++++++--
>   1 file changed, 76 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
> index c5f37c03af9c..3e801fa8ee2c 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,14 @@
>   
>   struct pwm_ir {
>   	struct pwm_device *pwm;
> -	unsigned int carrier;
> -	unsigned int duty_cycle;
> +	struct hrtimer timer;
> +	struct completion completion;

what about 'struct completion tx_done'?

> +	struct pwm_state *state;
> +	uint carrier;
> +	uint duty_cycle;

With my c++ developer hat on, I think either 'u32' or 'unsigned int' is 
more proper type for carrier and duty_cycle. Both s_tx_duty_cycle and 
s_tx_carrier are declared with second parameter of type u32, maybe 
that's what have to be used all over the place if you are to change from 
'unsigned int'. But better leave as it is, pwm_set_relative_duty_cycle() 
takes 'unsigned int' anyway.

> +	uint *txbuf;
> +	uint txbuf_len;
> +	uint txbuf_index;

OTOH, it is (*tx_ir)(struct rc_dev *dev, unsigned *txbuf, unsigned n), 
so maybe you should use 'unsigned' or 'unsigned int' for those.

I know at the end all those will be compiled to same type, but still :)

>   };
>   
>   static const struct of_device_id pwm_ir_of_match[] = {
> @@ -82,6 +90,62 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
>   	return count;
>   }
>   
> +static int pwm_ir_tx_atomic(struct rc_dev *dev, unsigned int *txbuf,
> +			    unsigned int count)
> +{
> +	struct pwm_ir *pwm_ir = dev->priv;
> +	struct pwm_device *pwm = pwm_ir->pwm;
> +	struct pwm_state state;
> +
> +	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);
> +
> +	pwm_ir->txbuf = txbuf;
> +	pwm_ir->txbuf_len = count;
> +	pwm_ir->txbuf_index = 0;
> +	pwm_ir->state = &state;
> +
> +	hrtimer_start(&pwm_ir->timer, 0, HRTIMER_MODE_REL);
> +
> +	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;
> +
> +		pwm_ir->state->enabled = !(pwm_ir->txbuf_index % 2);
> +		pwm_apply_state_atomic(pwm_ir->pwm, pwm_ir->state);
> +
> +		if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) {
> +			complete(&pwm_ir->completion);
> +
> +			return HRTIMER_NORESTART;
> +		}
> +
> +		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;
> @@ -103,10 +167,19 @@ static int pwm_ir_probe(struct platform_device *pdev)
>   	if (!rcdev)
>   		return -ENOMEM;
>   
> +	if (pwm_is_atomic(pwm_ir->pwm)) {
> +		init_completion(&pwm_ir->completion);
> +		hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +		pwm_ir->timer.function = pwm_ir_timer;
> +		rcdev->tx_ir = pwm_ir_tx_atomic;
> +	} else {
> +		dev_info(&pdev->dev, "tx will not be accurate as pwm device does not support atomic mode");
> +		rcdev->tx_ir = pwm_ir_tx;
> +	}
> +
>   	rcdev->priv = pwm_ir;
>   	rcdev->driver_name = DRIVER_NAME;
>   	rcdev->device_name = DEVICE_NAME;
> -	rcdev->tx_ir = pwm_ir_tx;
>   	rcdev->s_tx_duty_cycle = pwm_ir_set_duty_cycle;
>   	rcdev->s_tx_carrier = pwm_ir_set_carrier;
>