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@xxxxxxxxx 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
