On 11/8/23 10:50 PM, Cao, Bingbu wrote:
> Andreas,
>
> ------------------------------------------------------------------------
> BRs,
> Bingbu Cao
>
>> -----Original Message-----
>> From: Andreas Helbech Kleist <andreaskleist@xxxxxxxxx>
>> Sent: Wednesday, November 8, 2023 7:26 PM
>> To: Cao, Bingbu <bingbu.cao@xxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
>> sakari.ailus@xxxxxxxxxxxxxxx; laurent.pinchart@xxxxxxxxxxxxxxxx
>> Cc: andriy.shevchenko@xxxxxxxxxxxxxxx; hdegoede@xxxxxxxxxx;
>> ilpo.jarvinen@xxxxxxxxxxxxxxx; claus.stovgaard@xxxxxxxxx;
>> tfiga@xxxxxxxxxxxx; senozhatsky@xxxxxxxxxxxx;
>> tomi.valkeinen@xxxxxxxxxxxxxxxx; bingbu.cao@xxxxxxxxxxxxxxx; Qiu, Tian Shu
>> <tian.shu.qiu@xxxxxxxxx>; Wang, Hongju <hongju.wang@xxxxxxxxx>
>> Subject: Re: [PATCH v2 08/15] media: intel/ipu6: add IPU6 CSI2 receiver
>> v4l2 sub-device
>>
>> Hi Bingbu,
>>
>> On Tue, 2023-10-24 at 19:29 +0800, bingbu.cao@xxxxxxxxx wrote:
>>> From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>>>
>>> Input system CSI2 receiver is exposed as a v4l2 sub-device.
>>> Each CSI2 sub-device represent one single CSI2 hardware port which be
>>> linked with external sub-device such camera sensor by linked with ISYS
>>> CSI2's sink pad. CSI2 source pad is linked to the sink pad of video
>>> capture device.
>>>
>>> Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>>> ---
>>> drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 654
>>> ++++++++++++++++++
>>> drivers/media/pci/intel/ipu6/ipu6-isys-csi2.h | 81 +++
>>> .../media/pci/intel/ipu6/ipu6-isys-subdev.c | 379 ++++++++++
>>> .../media/pci/intel/ipu6/ipu6-isys-subdev.h | 58 ++
>>> .../intel/ipu6/ipu6-platform-isys-csi2-reg.h | 187 +++++
>>> 5 files changed, 1359 insertions(+)
>>> create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
>>> create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.h
>>> create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-subdev.c
>>> create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-subdev.h
>>> create mode 100644 drivers/media/pci/intel/ipu6/ipu6-platform-isys-
>>> csi2-reg.h
>>>
>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
>>> b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
>>> new file mode 100644
>>> index 000000000000..0dedf3f51850
>>> --- /dev/null
>>> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
>> ...
>>>
>>> +
>>> +#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?
However, I can also follow your comments to change to s32. Thanks!
>
>
>>
>> This is different from e.g. https://github.com/intel/ipu6-drivers.
>>
>> /Andreas
--
Best regards,
Bingbu Cao
