RE: [PATCH v5 2/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver

Hello Laurent,

-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sent: Wednesday, February 1, 2023 6:42 PM
To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
[off-list ref]
Cc: sakari.ailus@iki.fi; hverkuil@xs4all.nl; mchehab@kernel.org; iwamatsu
nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ) [off-list ref];
robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
rafael.j.wysocki@intel.com; broonie@kernel.org; linux-media@vger.kernel.org;
linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
devicetree@vger.kernel.org
Subject: Re: [PATCH v5 2/6] media: platform: visconti: Add Toshiba Visconti
Video Input Interface driver

Hello Ishikawa-san,

On Wed, Feb 01, 2023 at 02:02:43AM +0000, yuji2.ishikawa@toshiba.co.jp wrote:

quoted

Hello Sakari,

Sorry for sending the reply again.
My mail agent posted the previous one with HTML format.

Thank you for reviewing and your comments.

quoted

-----Original Message-----
From: Sakari Ailus sakari.ailus@iki.fi
Sent: Wednesday, January 18, 2023 7:40 AM
To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
yuji2.ishikawa@toshiba.co.jp
Cc: Hans Verkuil hverkuil@xs4all.nl; Laurent Pinchart
laurent.pinchart@ideasonboard.com; Mauro Carvalho Chehab
mchehab@kernel.org; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
nobuhiro1.iwamatsu@toshiba.co.jp; Rob Herring robh+dt@kernel.org;
Krzysztof Kozlowski krzysztof.kozlowski+dt@linaro.org; Rafael J .
Wysocki rafael.j.wysocki@intel.com; Mark Brown broonie@kernel.org;
linux-media@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
Subject: Re: [PATCH v5 2/6] media: platform: visconti: Add Toshiba
Visconti Video Input Interface driver

[snip]

quoted

quoted

quoted

diff --git a/drivers/media/platform/visconti/hwd_viif_reg.h

b/drivers/media/platform/visconti/hwd_viif_reg.h
new file mode 100644
index 00000000000..b7f43c5fe95

--- /dev/null
+++ b/drivers/media/platform/visconti/hwd_viif_reg.h

@@ -0,0 +1,2802 @@
+/* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
+/* Toshiba Visconti Video Capture Support
+ *
+ * (C) Copyright 2022 TOSHIBA CORPORATION
+ * (C) Copyright 2022 Toshiba Electronic Devices & Storage
+Corporation  */
+
+#ifndef HWD_VIIF_REG_H
+#define HWD_VIIF_REG_H
+
+/**
+ * struct hwd_viif_csi2host_reg - Registers for VIIF CSI2HOST
+control  */ struct hwd_viif_csi2host_reg {
+    u32 RESERVED_A_1;
+    u32 CSI2RX_NLANES;
+    u32 CSI2RX_RESETN;
+    u32 CSI2RX_INT_ST_MAIN;
+    u32 CSI2RX_DATA_IDS_1;
+    u32 CSI2RX_DATA_IDS_2;
+    u32 RESERVED_B_1[10];
+    u32 CSI2RX_PHY_SHUTDOWNZ;
+    u32 CSI2RX_PHY_RSTZ;
+    u32 CSI2RX_PHY_RX;
+    u32 CSI2RX_PHY_STOPSTATE;
+    u32 CSI2RX_PHY_TESTCTRL0;
+    u32 CSI2RX_PHY_TESTCTRL1;
+    u32 RESERVED_B_2[34];
+    u32 CSI2RX_INT_ST_PHY_FATAL;
+    u32 CSI2RX_INT_MSK_PHY_FATAL;
+    u32 CSI2RX_INT_FORCE_PHY_FATAL;
+    u32 RESERVED_B_3[1];
+    u32 CSI2RX_INT_ST_PKT_FATAL;
+    u32 CSI2RX_INT_MSK_PKT_FATAL;
+    u32 CSI2RX_INT_FORCE_PKT_FATAL;
+    u32 RESERVED_B_4[1];
+    u32 CSI2RX_INT_ST_FRAME_FATAL;
+    u32 CSI2RX_INT_MSK_FRAME_FATAL;
+    u32 CSI2RX_INT_FORCE_FRAME_FATAL;
+    u32 RESERVED_B_5[1];
+    u32 CSI2RX_INT_ST_PHY;
+    u32 CSI2RX_INT_MSK_PHY;
+    u32 CSI2RX_INT_FORCE_PHY;
+    u32 RESERVED_B_6[1];
+    u32 CSI2RX_INT_ST_PKT;
+    u32 CSI2RX_INT_MSK_PKT;
+    u32 CSI2RX_INT_FORCE_PKT;
+    u32 RESERVED_B_7[1];
+    u32 CSI2RX_INT_ST_LINE;
+    u32 CSI2RX_INT_MSK_LINE;
+    u32 CSI2RX_INT_FORCE_LINE;
+    u32 RESERVED_B_8[113];
+    u32 RESERVED_A_2;
+    u32 RESERVED_A_3;
+    u32 RESERVED_A_4;
+    u32 RESERVED_A_5;
+    u32 RESERVED_A_6;
+    u32 RESERVED_B_9[58];
+    u32 RESERVED_A_7;

These should be lower case, they're struct members.

This way of defining a hardware register interface is highly
unconventional. I'm not saying no to it, not now at least, but
something should be done to make this more robust against accidental
changes: adding a field in the middle changes the address of
anything that comes after it, and it's really difficult to say from
the code alone that the address of a given register is what it's intended to be.

Maybe pahole would still help?

quoted

quoted

But some documentation would be needed in that case.

I wonder what others think.

I understand the risk.
I'll remove these struct-style definition and introduce macro style definition.
I've hesitated this migration simply because it seemed difficult to
complete without any defects especially on calculating the offset of each

member.

quoted

I try find a series of operations that will complete the migration safely.

I agree with you about the migration risk. Maybe a script that parses the header
file and generates macros would take less time to implement than doing it
manually, and would be safer ?

Thank you for the advice.
I'm also thinking about generating macro definitions from headers.
The pahole tool might help me checking if the generated macros are correct.

quoted

quoted

quoted

+};
+

--
Regards,

Laurent Pinchart

Regards,
Yuji Ishikawa