Re: [PATCH v2 08/15] media: intel/ipu6: add IPU6 CSI2 receiver v4l2 sub-device

From: Andreas Helbech Kleist <andreaskleist@gmail.com>
To: "Cao, Bingbu" <bingbu.cao@intel.com>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
	"sakari.ailus@linux.intel.com" <sakari.ailus@linux.intel.com>,
	"laurent.pinchart@ideasonboard.com" 
	<laurent.pinchart@ideasonboard.com>
Cc: "andriy.shevchenko@linux.intel.com" 
	<andriy.shevchenko@linux.intel.com>,
	"hdegoede@redhat.com" <hdegoede@redhat.com>,
	"ilpo.jarvinen@linux.intel.com" <ilpo.jarvinen@linux.intel.com>,
	"claus.stovgaard@gmail.com" <claus.stovgaard@gmail.com>,
	"tfiga@chromium.org" <tfiga@chromium.org>,
	"senozhatsky@chromium.org" <senozhatsky@chromium.org>,
	"tomi.valkeinen@ideasonboard.com"
	<tomi.valkeinen@ideasonboard.com>,
	"bingbu.cao@linux.intel.com" <bingbu.cao@linux.intel.com>,
	"Qiu, Tian Shu" <tian.shu.qiu@intel.com>,
	"Wang, Hongju" <hongju.wang@intel.com>
Subject: Re: [PATCH v2 08/15] media: intel/ipu6: add IPU6 CSI2 receiver v4l2 sub-device
Date: Wed, 08 Nov 2023 16:00:02 +0100	[thread overview]
Message-ID: <a22cea646d0d8b8022c3be8e4625d6e67a3eb7bf.camel@gmail.com> (raw)
In-Reply-To: <DM8PR11MB565383A795F071F50B04D1BF99A8A@DM8PR11MB5653.namprd11.prod.outlook.com>

Bingbu,

On Wed, 2023-11-08 at 14:50 +0000, Cao, Bingbu wrote:
> > -----Original Message-----
> > On Tue, 2023-10-24 at 19:29 +0800, bingbu.cao@intel.com wrote:
> > 
> > > +#define DIV_SHIFT      8
> > > +#define CSI2_ACCINV    8
> > > +
> > > +static u32 calc_timing(s32 a, s32 b, s64 link_freq, u32 accinv)
> > > {
> > > +       return accinv * a + (accinv * b * (500000000 >>
> > > DIV_SHIFT)
> > > +                            / (int32_t)(link_freq >>
> > > DIV_SHIFT)); }
> > 
> > I think accinv should be s32 here. When accinv is unsigned, the
> > expression
> > (accinv * b) becomes unsigned, and thus negative values of b gives
> > incorrect results.
> 
> accinv is always an unsigned value, so I think we don't need change
> the
> type of argument. Following usual arithmetic conversions, I think it
> needs
> a signed integer cast here. What do you think?

I think changing the type of argument is clearer. A cast just adds
noise in my opinion. Of course the argument to
ipu6_isys_csi2_calc_timing should also be changed to match, which would
also remove any implicit casts as far as I can tell.

Now we're talking about casts. There is a cast to int32_t, but the rest
of the code uses the s32/u32 typedefs. I don't know if one or the other
is recommended for new code, but I think it should at least be
consistent across the driver.

/Andreas