From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: yuji2.ishikawa@toshiba.co.jp
Cc: sakari.ailus@iki.fi, hverkuil@xs4all.nl, mchehab@kernel.org,
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
Date: Wed, 1 Feb 2023 11:41:56 +0200 [thread overview]
Message-ID: <Y9oz5MDMmopBq5h9@pendragon.ideasonboard.com> (raw)
In-Reply-To: <TYAPR01MB62010040B750701D253C47CB92D19@TYAPR01MB6201.jpnprd01.prod.outlook.com>
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 ?
> > > +};
> > > +
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2023-02-01 9:42 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-11 2:24 [PATCH v5 0/6] Add Toshiba Visconti Video Input Interface driver Yuji Ishikawa
2023-01-11 2:24 ` [PATCH v5 1/6] dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input Interface bindings Yuji Ishikawa
2023-01-11 9:19 ` Krzysztof Kozlowski
2023-01-11 12:48 ` yuji2.ishikawa
2023-01-17 15:26 ` Laurent Pinchart
2023-01-17 15:42 ` Krzysztof Kozlowski
2023-01-17 15:58 ` Laurent Pinchart
2023-01-17 17:01 ` Krzysztof Kozlowski
2023-01-22 19:25 ` Laurent Pinchart
2023-01-30 9:06 ` yuji2.ishikawa
2023-02-01 9:45 ` Laurent Pinchart
2023-02-01 11:24 ` yuji2.ishikawa
2023-01-11 2:24 ` [PATCH v5 2/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver Yuji Ishikawa
2023-01-11 15:30 ` kernel test robot
2023-01-11 22:55 ` kernel test robot
2023-01-17 10:01 ` Hans Verkuil
2023-01-25 12:12 ` yuji2.ishikawa
2023-01-17 22:39 ` Sakari Ailus
2023-02-01 2:02 ` yuji2.ishikawa
2023-02-01 9:41 ` Laurent Pinchart [this message]
2023-02-01 11:22 ` yuji2.ishikawa
2023-01-18 0:52 ` Laurent Pinchart
2023-02-02 4:37 ` yuji2.ishikawa
2023-01-11 2:24 ` [PATCH v5 3/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver user interace Yuji Ishikawa
2023-01-17 11:47 ` Hans Verkuil
2023-01-18 1:04 ` Laurent Pinchart
2023-01-25 10:20 ` yuji2.ishikawa
2023-01-26 20:49 ` Laurent Pinchart
2023-02-02 4:52 ` yuji2.ishikawa
2023-02-02 7:58 ` Laurent Pinchart
2023-01-26 1:25 ` yuji2.ishikawa
2023-01-26 8:01 ` Hans Verkuil
2023-01-27 12:47 ` yuji2.ishikawa
2023-01-11 2:24 ` [PATCH v5 4/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver v4l2 controls handler Yuji Ishikawa
2023-01-17 11:19 ` Hans Verkuil
2023-01-26 0:38 ` yuji2.ishikawa
2023-01-26 8:39 ` Hans Verkuil
2023-01-26 11:35 ` Laurent Pinchart
2023-02-03 1:35 ` yuji2.ishikawa
2023-02-02 12:42 ` yuji2.ishikawa
2023-01-11 2:24 ` [PATCH v5 5/6] documentation: media: add documentation for Toshiba Visconti Video Input Interface driver Yuji Ishikawa
2023-01-11 2:24 ` [PATCH v5 6/6] MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface Yuji Ishikawa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y9oz5MDMmopBq5h9@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=hverkuil@xs4all.nl \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=nobuhiro1.iwamatsu@toshiba.co.jp \
--cc=rafael.j.wysocki@intel.com \
--cc=robh+dt@kernel.org \
--cc=sakari.ailus@iki.fi \
--cc=yuji2.ishikawa@toshiba.co.jp \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).