From: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org,
linux-phy@lists.infradead.org, linux-clk@vger.kernel.org,
linux-staging@lists.linux.dev, Yong Deng <yong.deng@magewell.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Maxime Ripard <mripard@kernel.org>,
Sakari Ailus <sakari.ailus@linux.intel.com>,
Hans Verkuil <hans.verkuil@cisco.com>,
Chen-Yu Tsai <wens@csie.org>,
Jernej Skrabec <jernej.skrabec@gmail.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Helen Koike <helen.koike@collabora.com>,
Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
Rob Herring <robh@kernel.org>
Subject: Re: [PATCH v2 07/66] dt-bindings: media: sun6i-a31-csi: Add MIPI CSI-2 input port
Date: Mon, 14 Feb 2022 17:10:05 +0100 [thread overview]
Message-ID: <Ygp+3URjxyvRBn/5@aptenodytes> (raw)
In-Reply-To: <YgbNfKiNkPmvaT1k@pendragon.ideasonboard.com>
[-- Attachment #1: Type: text/plain, Size: 6466 bytes --]
Hi,
On Fri 11 Feb 22, 22:56, Laurent Pinchart wrote:
> On Fri, Feb 11, 2022 at 05:10:06PM +0100, Paul Kocialkowski wrote:
> > Hi Laurent,
> >
> > Thanks for the review!
> >
> > On Mon 07 Feb 22, 18:03, Laurent Pinchart wrote:
> > > Hi Paul,
> > >
> > > Thank you for the patch.
> > >
> > > On Sat, Feb 05, 2022 at 07:53:30PM +0100, Paul Kocialkowski wrote:
> > > > The A31 CSI controller supports two distinct input interfaces:
> > > > parallel and an external MIPI CSI-2 bridge. The parallel interface
> > > > is often connected to a set of hardware pins while the MIPI CSI-2
> > > > bridge is an internal FIFO-ish link. As a result, these two inputs
> > > > are distinguished as two different ports.
> > > >
> > > > Note that only one of the two may be present on a controller instance.
> > > > For example, the V3s has one controller dedicated to MIPI-CSI2 and one
> > > > dedicated to parallel.
> > >
> > > Is it that only one of the two is present, or only one of the two is
> > > connected ? In the latter case I'd make both ports required, but with
> > > only one of them connected.
> >
> > There are situations where the actual pins for parallel (port@0) are missing
> > and the controller is dedicated to its mipi csi-2 bridge (port@1), cases where
> > the two are present and cases where the mipi csi-2 bridge doesn't exist.
> > So all in all it's really legit that only one port may be defined.
>
> The port could still exist internally in the IP core though. Of course
> that's hard to tell.
Yes that's true, the bit to switch input to mipi csi-2 is there in all cases
but I don't think it makes sense to expose the port in situations where
no controller is attached.
> > > > Update the binding with an explicit ports node that holds two distinct
> > > > port nodes: one for parallel input and one for MIPI CSI-2.
> > > >
> > > > This is backward-compatible with the single-port approach that was
> > > > previously taken for representing the parallel interface port, which
> > > > stays enumerated as fwnode port 0.
> > > >
> > > > Note that additional ports may be added in the future, especially to
> > > > support feeding the CSI controller's output to the ISP.
> > > >
> > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > > Acked-by: Maxime Ripard <mripard@kernel.org>
> > > > ---
> > > > .../media/allwinner,sun6i-a31-csi.yaml | 60 +++++++++++++++----
> > > > 1 file changed, 47 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> > > > index 8b568072a069..3cc61866ea89 100644
> > > > --- a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> > > > @@ -61,6 +61,34 @@ properties:
> > > >
> > > > additionalProperties: false
> > > >
> > > > + ports:
> > > > + $ref: /schemas/graph.yaml#/properties/ports
> > > > +
> > > > + properties:
> > > > + port@0:
> > > > + $ref: "#/properties/port"
> > > > + unevaluatedProperties: false
> > > > +
> > > > + port@1:
> > > > + $ref: /schemas/graph.yaml#/$defs/port-base
> > > > + description: MIPI CSI-2 bridge input port
> > > > +
> > > > + properties:
> > > > + reg:
> > > > + const: 1
> > > > +
> > > > + endpoint:
> > > > + $ref: video-interfaces.yaml#
> > > > + unevaluatedProperties: false
> > > > +
> > > > + additionalProperties: false
> > > > +
> > > > + anyOf:
> > > > + - required:
> > > > + - port@0
> > > > + - required:
> > > > + - port@1
> > > > +
> > > > required:
> > > > - compatible
> > > > - reg
> > >
> > > Shouldn't you specify that either port or ports is required, but not
> > > both ? I'd also add a comment in the port node to tell it's deprecated,
> > > and that ports should be used instead.
> >
> > Yes I agree on both points. I guess that should be a:
> >
> > oneOf:
> > - required:
> > - ports
> > - required:
> > - port
> >
> > (but feel free to correct me).
> >
> > > > @@ -89,19 +117,25 @@ examples:
> > > > "ram";
> > > > resets = <&ccu RST_BUS_CSI>;
> > > >
> > > > - port {
> > > > - /* Parallel bus endpoint */
> > > > - csi1_ep: endpoint {
> > > > - remote-endpoint = <&adv7611_ep>;
> > > > - bus-width = <16>;
> > > > -
> > > > - /*
> > > > - * If hsync-active/vsync-active are missing,
> > > > - * embedded BT.656 sync is used.
> > > > - */
> > > > - hsync-active = <0>; /* Active low */
> > > > - vsync-active = <0>; /* Active low */
> > > > - pclk-sample = <1>; /* Rising */
> > > > + ports {
> > > > + #address-cells = <1>;
> > > > + #size-cells = <0>;
> > > > +
> > > > + port@0 {
> > > > + reg = <0>;
> > > > + /* Parallel bus endpoint */
> > > > + csi1_ep: endpoint {
> > > > + remote-endpoint = <&adv7611_ep>;
> > > > + bus-width = <16>;
> > > > +
> > > > + /*
> > > > + * If hsync-active/vsync-active are missing,
> > > > + * embedded BT.656 sync is used.
> > > > + */
> > > > + hsync-active = <0>; /* Active low */
> > > > + vsync-active = <0>; /* Active low */
> > > > + pclk-sample = <1>; /* Rising */
> > >
> > > Wrong indentation.
> >
> > The double-space before /* Rising */ or something with the heading indent?
>
> The heading has one extra space for all three lines, they should be
> aligned to the / of /*, not to the *.
Oh that's true, good catch thanks!
Paul
> > > > + };
> > > > };
> > > > };
> > > > };
>
> --
> Regards,
>
> Laurent Pinchart
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]
next prev parent reply other threads:[~2022-02-14 16:10 UTC|newest]
Thread overview: 141+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-05 18:53 [PATCH v2 00/66] Allwinner A31/A83T MIPI CSI-2 Support and A31 ISP Support Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 01/66] ARM: dts: sun8i: v3s: Move the csi1 block to follow address order Paul Kocialkowski
2022-02-07 9:24 ` (subset) " Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 02/66] dt-bindings: interconnect: sunxi: Add V3s mbus compatible Paul Kocialkowski
2022-02-05 20:14 ` Samuel Holland
2022-02-07 8:43 ` Paul Kocialkowski
2022-02-07 8:50 ` Jernej Škrabec
2022-02-07 9:44 ` Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 03/66] clk: sunxi-ng: v3s: Export the MBUS clock to the public header Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 04/66] ARM: dts: sun8i: v3s: Add mbus node to represent the interconnect Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 05/66] dt-bindings: sun6i-a31-mipi-dphy: Add optional direction property Paul Kocialkowski
2022-02-11 15:03 ` Rob Herring
2022-02-11 15:12 ` Paul Kocialkowski
2022-02-11 20:47 ` Laurent Pinchart
2022-02-05 18:53 ` [PATCH v2 06/66] phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI CSI-2 Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 07/66] dt-bindings: media: sun6i-a31-csi: Add MIPI CSI-2 input port Paul Kocialkowski
2022-02-07 16:03 ` Laurent Pinchart
2022-02-11 16:10 ` Paul Kocialkowski
2022-02-11 20:56 ` Laurent Pinchart
2022-02-14 16:10 ` Paul Kocialkowski [this message]
2022-02-05 18:53 ` [PATCH v2 08/66] dt-bindings: media: Add Allwinner A31 MIPI CSI-2 bindings documentation Paul Kocialkowski
2022-02-07 16:09 ` Laurent Pinchart
2022-02-11 16:03 ` Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 09/66] media: sunxi: Add support for the A31 MIPI CSI-2 controller Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 10/66] MAINTAINERS: Add entry for the Allwinner A31 MIPI CSI-2 bridge driver Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 11/66] ARM: dts: sun8i: v3s: Add nodes for MIPI CSI-2 support Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 12/66] dt-bindings: media: Add Allwinner A83T MIPI CSI-2 bindings documentation Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 13/66] media: sunxi: Add support for the A83T MIPI CSI-2 controller Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 14/66] MAINTAINERS: Add entry for the Allwinner A83T MIPI CSI-2 bridge Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 15/66] ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node Paul Kocialkowski
2022-02-05 18:53 ` [PATCH NOT FOR MERGE v2 16/66] ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865 Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 17/66] media: sun6i-csi: Define and use driver name and (reworked) description Paul Kocialkowski
2022-02-07 9:10 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 18/66] media: sun6i-csi: Refactor main driver data structures Paul Kocialkowski
2022-02-07 9:11 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 19/66] media: sun6i-csi: Grab bus clock instead of passing it to regmap Paul Kocialkowski
2022-02-09 9:20 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 20/66] media: sun6i-csi: Tidy up platform code Paul Kocialkowski
2022-02-07 9:13 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 21/66] media: sun6i-csi: Always set exclusive module clock rate Paul Kocialkowski
2022-02-07 9:14 ` Maxime Ripard
2022-02-11 16:29 ` Paul Kocialkowski
2022-02-14 16:31 ` Sakari Ailus
2022-03-01 15:39 ` Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 22/66] media: sun6i-csi: Use runtime pm for clocks and reset Paul Kocialkowski
2022-02-09 9:22 ` Maxime Ripard
2022-02-11 16:01 ` Paul Kocialkowski
2022-02-11 16:41 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 23/66] media: sun6i-csi: Tidy up v4l2 code Paul Kocialkowski
2022-02-07 9:55 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 24/66] media: sun6i-csi: Tidy up video code Paul Kocialkowski
2022-02-07 9:56 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 25/66] media: sun6i-csi: Pass and store csi device directly in " Paul Kocialkowski
2022-02-07 9:58 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 26/66] media: sun6i-csi: Register the media device after creation Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 27/66] media: sun6i-csi: Add media ops with link notify callback Paul Kocialkowski
2022-02-14 16:40 ` Sakari Ailus
2022-02-15 10:01 ` Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 28/66] media: sun6i-csi: Introduce and use video helper functions Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 29/66] media: sun6i-csi: Move csi buffer definition to main header file Paul Kocialkowski
2022-02-09 9:23 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 30/66] media: sun6i-csi: Add bridge v4l2 subdev with port management Paul Kocialkowski
2022-02-09 9:24 ` Maxime Ripard
2022-02-11 15:43 ` Paul Kocialkowski
2022-02-11 16:44 ` Maxime Ripard
2022-02-14 18:12 ` Sakari Ailus
2022-03-02 14:59 ` Paul Kocialkowski
2022-03-03 22:43 ` Sakari Ailus
2022-03-04 9:00 ` Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 31/66] media: sun6i-csi: Rename sun6i_video to sun6i_csi_capture Paul Kocialkowski
2022-02-09 9:25 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 32/66] media: sun6i-csi: Add capture state using vsync for page flip Paul Kocialkowski
2022-02-09 9:26 ` Maxime Ripard
2022-02-11 15:40 ` Paul Kocialkowski
2022-02-11 16:45 ` Maxime Ripard
2022-02-05 18:53 ` [PATCH v2 33/66] media: sun6i-csi: Rework register definitions, invert misleading fields Paul Kocialkowski
2022-02-09 9:39 ` Maxime Ripard
2022-02-11 15:17 ` Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 34/66] media: sun6i-csi: Add dimensions and format helpers to capture Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 35/66] media: sun6i-csi: Implement address configuration without indirection Paul Kocialkowski
2022-02-05 18:53 ` [PATCH v2 36/66] media: sun6i-csi: Split stream sequences and irq code in capture Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 37/66] media: sun6i-csi: Move power management to runtime pm " Paul Kocialkowski
2022-02-14 18:30 ` Sakari Ailus
2022-02-15 9:56 ` Paul Kocialkowski
2022-02-15 10:04 ` Laurent Pinchart
2022-02-15 10:21 ` Paul Kocialkowski
2022-02-15 21:21 ` Sakari Ailus
2022-02-05 18:54 ` [PATCH v2 38/66] media: sun6i-csi: Move register configuration to capture Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 39/66] media: sun6i-csi: Rework capture format management with helper Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 40/66] media: sun6i-csi: Remove custom format helper and rework configure Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 41/66] media: sun6i-csi: Add bridge dimensions and format helpers Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 42/66] media: sun6i-csi: Get mbus code from bridge instead of storing it Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 43/66] media: sun6i-csi: Tidy capture configure code Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 44/66] media: sun6i-csi: Introduce bridge format structure, list and helper Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 45/66] media: sun6i-csi: Introduce capture " Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 46/66] media: sun6i-csi: Configure registers from format tables Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 47/66] media: sun6i-csi: Introduce format match structure, list and helper Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 48/66] media: sun6i-csi: Implement capture link validation with logic Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 49/66] media: sun6i-csi: Get bridge subdev directly in capture stream ops Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 50/66] media: sun6i-csi: Move hardware control to the bridge Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 51/66] media: sun6i-csi: Unset bridge source on capture streamon fail Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 52/66] media: sun6i-csi: Rename the capture video device to sun6i-csi-capture Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 53/66] media: sun6i-csi: Cleanup headers and includes, update copyright lines Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 54/66] media: sun6i-csi: Add support for MIPI CSI-2 to the bridge code Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 55/66] media: sun6i-csi: Only configure capture when streaming Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 56/66] media: sun6i-csi: Add extra checks to the interrupt routine Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 57/66] media: sun6i-csi: Request a shared interrupt Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 58/66] media: sun6i-csi: Detect the availability of the ISP Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 59/66] media: sun6i-csi: Add support for hooking to the isp devices Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 60/66] MAINTAINERS: Add myself as sun6i-csi maintainer and rename/move entry Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 61/66] dt-bindings: media: Add Allwinner A31 ISP bindings documentation Paul Kocialkowski
2022-02-07 15:51 ` Laurent Pinchart
2022-02-11 15:13 ` Rob Herring
2022-02-11 20:52 ` Laurent Pinchart
2022-02-14 16:28 ` Paul Kocialkowski
2022-02-14 17:10 ` Laurent Pinchart
2022-02-14 16:18 ` Paul Kocialkowski
2022-02-14 17:09 ` Laurent Pinchart
2022-02-15 10:10 ` Paul Kocialkowski
2022-02-15 10:16 ` Laurent Pinchart
2022-03-01 15:38 ` Paul Kocialkowski
2022-03-04 12:01 ` Laurent Pinchart
2022-03-04 13:57 ` Paul Kocialkowski
2022-03-04 14:09 ` Laurent Pinchart
2022-03-11 14:17 ` Paul Kocialkowski
2022-03-13 16:15 ` Laurent Pinchart
2022-03-15 9:49 ` Paul Kocialkowski
2022-03-15 10:13 ` Laurent Pinchart
2022-02-05 18:54 ` [PATCH v2 62/66] dt-bindings: media: sun6i-a31-csi: Add ISP output port Paul Kocialkowski
2022-02-07 16:04 ` Laurent Pinchart
2022-02-11 15:16 ` Rob Herring
2022-02-05 18:54 ` [PATCH v2 63/66] staging: media: Add support for the Allwinner A31 ISP Paul Kocialkowski
2022-02-07 16:16 ` Laurent Pinchart
2022-03-01 15:58 ` Paul Kocialkowski
2022-03-02 8:51 ` Laurent Pinchart
2022-03-02 13:23 ` Paul Kocialkowski
2022-03-02 13:33 ` Laurent Pinchart
2022-03-02 15:10 ` Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 64/66] MAINTAINERS: Add entry for the Allwinner A31 ISP driver Paul Kocialkowski
2022-02-05 18:54 ` [PATCH v2 65/66] ARM: dts: sun8i: v3s: Add support for the ISP Paul Kocialkowski
2022-02-05 18:54 ` [PATCH NOT FOR MERGE v2 66/66] of: Mark interconnects property supplier as optional Paul Kocialkowski
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=Ygp+3URjxyvRBn/5@aptenodytes \
--to=paul.kocialkowski@bootlin.com \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=hans.verkuil@cisco.com \
--cc=helen.koike@collabora.com \
--cc=jernej.skrabec@gmail.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-phy@lists.infradead.org \
--cc=linux-staging@lists.linux.dev \
--cc=linux-sunxi@lists.linux.dev \
--cc=mchehab@kernel.org \
--cc=mripard@kernel.org \
--cc=robh+dt@kernel.org \
--cc=robh@kernel.org \
--cc=sakari.ailus@linux.intel.com \
--cc=thomas.petazzoni@bootlin.com \
--cc=wens@csie.org \
--cc=yong.deng@magewell.com \
/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).