Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Date:   Sun, 15 Oct 2023 22:25:06 +0100
From:   Sean Young <sean@mess.org>
To:     Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Cc:     linux-media@vger.kernel.org,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Thierry Reding <thierry.reding@gmail.com>,
        Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= 
        <u.kleine-koenig@pengutronix.de>, linux-kernel@vger.kernel.org,
        linux-pwm@vger.kernel.org
Subject: Re: [PATCH v2 3/3] media: pwm-ir-tx: trigger edges from hrtimer
 interrupt context
Message-ID: <ZSxYsg/ianhkUDvY@gofer.mess.org>
References: <cover.1697193646.git.sean@mess.org>
 <1560b474f7d426bc77100665c14c3a29c3af3e75.1697193646.git.sean@mess.org>
 <e47d4d33-4689-915d-3169-5c122075df05@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <e47d4d33-4689-915d-3169-5c122075df05@gmail.com>
Precedence: bulk

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