[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZSxYsg/ianhkUDvY@gofer.mess.org>
Date: Sun, 15 Oct 2023 22:25:06 +0100
From: Sean Young <sean@...s.org>
To: Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>
Cc: linux-media@...r.kernel.org,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Thierry Reding <thierry.reding@...il.com>,
Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>, linux-kernel@...r.kernel.org,
linux-pwm@...r.kernel.org
Subject: Re: [PATCH v2 3/3] media: pwm-ir-tx: trigger edges from hrtimer
interrupt context
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@...s.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
Powered by blists - more mailing lists