Messages in this thread | | | From | <> | Subject | RE: [PATCH v5 2/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver | Date | Wed, 1 Feb 2023 11:22:14 +0000 |
| |
Hello Laurent,
> -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: Wednesday, February 1, 2023 6:42 PM > To: ishikawa yuji(石川 悠司 ○RDC□AITC○EA開) > <yuji2.ishikawa@toshiba.co.jp> > Cc: sakari.ailus@iki.fi; hverkuil@xs4all.nl; mchehab@kernel.org; iwamatsu > nobuhiro(岩松 信洋 □SWC◯ACT) <nobuhiro1.iwamatsu@toshiba.co.jp>; > 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: > > 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. > > > > > -----Original Message----- > > > From: Sakari Ailus sakari.ailus@iki.fi > > > Sent: Wednesday, January 18, 2023 7:40 AM > > > To: ishikawa yuji(石川 悠司 ○RDC□AITC○EA開) > > > 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(岩松 信洋 □SWC◯ACT) > > > 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] > > > > > 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? > > > 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. > > 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.
> > > > +}; > > > > + > > -- > Regards, > > Laurent Pinchart
Regards, Yuji Ishikawa
| |