| Message ID | 1560b474f7d426bc77100665c14c3a29c3af3e75.1697193646.git.sean@mess.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve pwm-ir-tx precision | expand |
On 13.10.23 г. 13:46 ч., Sean Young wrote: > This makes the driver much more precise. > > Signed-off-by: Sean Young <sean@mess.org> > --- > 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; > >
On Sun, Oct 15, 2023 at 09:31:34AM +0300, Ivaylo Dimitrov wrote: > On 13.10.23 г. 13:46 ч., Sean Young wrote: > > This makes the driver much more precise. > > > > Signed-off-by: Sean Young <sean@mess.org> > > --- > > 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'? Agreed, that's much better. > > + 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. I much prefer the rust way of u64/u32/u16/u8/usize and simply no int/short/long types at all. int is useful when your compiler needs to work on weird architectures with non-power-of-two register sizes like the pdp-9 (18 bits anyone?), but on contemporary cpus there is really no need for int: int is always a 32 bit value. So I'm all for banishing int in every form, but for now the kernel uses unsigned int and u32 interchangably, so it's hard to be consistent with this. > > + 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 :) Maybe it's time for tx_ir to be defined with u32 types and do away with this madness. However, as it stands I agree with your points. I guess it's best to be consistent with the apis this driver implements/uses. Thanks, Sean
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; + struct pwm_state *state; + uint carrier; + uint duty_cycle; + uint *txbuf; + uint txbuf_len; + uint txbuf_index; }; 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;
This makes the driver much more precise. Signed-off-by: Sean Young <sean@mess.org> --- drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 3 deletions(-)