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@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'?
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
