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

[Sean Young <sean@mess.org>]

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