Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
To: Maxime Ripard <maxime@cerno.tech>
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org,
	linux-sunxi@googlegroups.com,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Rob Herring <robh+dt@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
	Yong Deng <yong.deng@magewell.com>,
	Kishon Vijay Abraham I <kishon@ti.com>,
	Vinod Koul <vkoul@kernel.org>,
	Helen Koike <helen.koike@collabora.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Hans Verkuil <hans.verkuil@cisco.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	kevin.lhopital@hotmail.com
Subject: Re: [PATCH 07/14] dt-bindings: media: i2c: Add A31 MIPI CSI-2 bindings documentation
Date: Wed, 4 Nov 2020 11:48:27 +0100
Message-ID: <20201104104827.GD285779@aptenodytes> (raw)
In-Reply-To: <20201027184459.eberpkr52kay3du6@gilmour.lan>


[-- Attachment #1: Type: text/plain, Size: 7822 bytes --]

Hi,

On Tue 27 Oct 20, 19:44, Maxime Ripard wrote:
> On Tue, Oct 27, 2020 at 10:52:21AM +0100, Paul Kocialkowski wrote:
> > Hi,
> > 
> > On Mon 26 Oct 20, 17:14, Maxime Ripard wrote:
> > > i2c? :)
> > 
> > Oops, good catch!
> >  
> > > On Fri, Oct 23, 2020 at 07:45:39PM +0200, Paul Kocialkowski wrote:
> > > > This introduces YAML bindings documentation for the A31 MIPI CSI-2
> > > > controller.
> > > > 
> > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > ---
> > > >  .../media/allwinner,sun6i-a31-mipi-csi2.yaml  | 168 ++++++++++++++++++
> > > >  1 file changed, 168 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
> > > > new file mode 100644
> > > > index 000000000000..9adc0bc27033
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
> > > > @@ -0,0 +1,168 @@
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/media/allwinner,sun6i-a31-mipi-csi2.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Allwinner A31 MIPI CSI-2 Device Tree Bindings
> > > > +
> > > > +maintainers:
> > > > +  - Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    oneOf:
> > > > +      - const: allwinner,sun6i-a31-mipi-csi2
> > > > +      - items:
> > > > +          - const: allwinner,sun8i-v3s-mipi-csi2
> > > > +          - const: allwinner,sun6i-a31-mipi-csi2
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  clocks:
> > > > +    items:
> > > > +      - description: Bus Clock
> > > > +      - description: Module Clock
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: bus
> > > > +      - const: mod
> > > > +
> > > > +  phys:
> > > > +    items:
> > > > +      - description: MIPI D-PHY
> > > > +
> > > > +  phy-names:
> > > > +    items:
> > > > +      - const: dphy
> > > > +
> > > > +  resets:
> > > > +    maxItems: 1
> > > > +
> > > > +  # See ./video-interfaces.txt for details
> > > > +  ports:
> > > > +    type: object
> > > > +
> > > > +    properties:
> > > > +      port@0:
> > > > +        type: object
> > > > +        description: Input port, connect to a MIPI CSI-2 sensor
> > > > +
> > > > +        properties:
> > > > +          reg:
> > > > +            const: 0
> > > > +
> > > > +          endpoint:
> > > > +            type: object
> > > > +
> > > > +            properties:
> > > > +              remote-endpoint: true
> > > > +
> > > > +              bus-type:
> > > > +                const: 4
> > > > +
> > > > +              clock-lanes:
> > > > +                maxItems: 1
> > > > +
> > > > +              data-lanes:
> > > > +                minItems: 1
> > > > +                maxItems: 4
> > > > +
> > > > +            required:
> > > > +              - bus-type
> > > > +              - data-lanes
> > > > +              - remote-endpoint
> > > > +
> > > > +            additionalProperties: false
> > > > +
> > > > +        required:
> > > > +          - endpoint
> > > > +
> > > > +        additionalProperties: false
> > > > +
> > > > +      port@1:
> > > > +        type: object
> > > > +        description: Output port, connect to a CSI controller
> > > > +
> > > > +        properties:
> > > > +          reg:
> > > > +            const: 1
> > > > +
> > > > +          endpoint:
> > > > +            type: object
> > > > +
> > > > +            properties:
> > > > +              remote-endpoint: true
> > > > +
> > > > +              bus-type:
> > > > +                const: 4
> > > 
> > > That one seems a bit weird. If the input and output ports are using the
> > > same format, what is that "bridge" supposed to be doing?
> > 
> > Fair enough. What this represents is the internal link (likely a FIFO) between
> > the two controllers. It is definitely not a MIPI CSI-2 bus but there's no
> > mbus type for an internal link (probably because it's not a bus after all).
> > 
> > Note that on the CSI controller side, we need the bus-type to be set to 4 for it
> > to properly select the MIPI CSI-2 input. So it just felt more logical to have
> > the same on the other side of the endpoint. On the other hand, we can just
> > remove it on the MIPI CSI-2 controller side since it won't check it and have it
> > fallback to the unknown mbus type.
> > 
> > But that would make the types inconsistent on the two sides of the link.
> > I don't think V4L2 will complain about it at the moment, but it would also make
> > sense that it does eventually.
> > 
> > What do you think?
> 
> There's still the same issue though, it doesn't make any sense that a
> bridge doesn't change the bus type. If it really did, we wouldn't need
> that in the first place.

Yes I agreee.

> What you want to check in your driver is whether the subdev you're
> connected to has a sink pad that uses MIPI-CSI

I'm not really sure that's possible, but if it is it would indeed be the most
appropriate solution. If it's not, we still need to know that we need to feed
from MIPI CSI-2 so I don't see any other option than report MIPI CSI-2 on both
ends of MIPI CSI-2 controller.

But there's still the question of what media bus type should be reported for
the CSI <-> MIPI CSI-2 link. I'm fine with unknown but we could also add a
generic internal bus type for this case.

Paul

> Maxime
> 
> > > > +            additionalProperties: false
> > > > +
> > > > +        required:
> > > > +          - endpoint
> > > > +
> > > > +        additionalProperties: false
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - interrupts
> > > > +  - clocks
> > > > +  - clock-names
> > > > +  - resets
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > > +    #include <dt-bindings/clock/sun8i-v3s-ccu.h>
> > > > +    #include <dt-bindings/reset/sun8i-v3s-ccu.h>
> > > > +
> > > > +    mipi_csi2: mipi-csi2@1cb1000 {
> > > 
> > > The unit name should be pretty standard, with the list here:
> > > 
> > > https://github.com/devicetree-org/devicetree-specification/blob/master/source/chapter2-devicetree-basics.rst#generic-names-recommendation
> > > 
> > > there's nothing really standing out for us in that list, but given that
> > > there's dsi, we should stick with csi
> > 
> > Then what really surprises me is that the CSI controllers are called "camera",
> > not "csi". If "camera" is supposed to cover both image sensor and camera sensor
> > interfaces, it would probably fit MIPI CSI-2 as well.
> > 
> > I see lots of names with -controller for controllers with specific devices
> > attached, like "nand-controller" or "lcd-controller". Maybe using
> > "camera-controller" for the CSI and MIPI CSI-2 controllers would make the most
> > sense, while keeping "camera" for the actual image sensors.
> > 
> > What do you think?
> 
> If you really want to discuss this, feel free to open a PR for the DT
> spec and add it. However, I still think this csi would be best here:
> it's neither a camera nor a camera controller
> 
> maxime



-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply index

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-23 17:45 [PATCH 00/14] Allwinner MIPI CSI-2 support for A31/V3s/A83T Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 01/14] phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes Paul Kocialkowski
2020-10-23 18:18   ` [linux-sunxi] " Jernej Škrabec
2020-10-24  8:31     ` Paul Kocialkowski
2020-10-30 22:44   ` Helen Koike
2020-10-23 17:45 ` [PATCH 02/14] phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI CSI-2 Paul Kocialkowski
2020-10-26 15:38   ` Maxime Ripard
2020-10-27  9:23     ` Paul Kocialkowski
2020-10-27 18:28       ` Maxime Ripard
2020-11-04 10:53         ` Paul Kocialkowski
2020-10-30 22:44   ` Helen Koike
2020-11-04 10:54     ` Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 03/14] media: sun6i-csi: Support an optional dedicated memory pool Paul Kocialkowski
2020-10-26 15:41   ` Maxime Ripard
2020-10-27  9:26     ` Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 04/14] media: sun6i-csi: Fix the image storage bpp for 10/12-bit Bayer formats Paul Kocialkowski
2020-10-30 22:45   ` Helen Koike
2020-11-04 10:56     ` Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 05/14] media: sun6i-csi: Only configure the interface data width for parallel Paul Kocialkowski
2020-10-26 16:00   ` Maxime Ripard
2020-10-27  9:31     ` Paul Kocialkowski
2020-10-27 18:31       ` Maxime Ripard
2020-10-23 17:45 ` [PATCH 06/14] media: sun6i-csi: Support feeding from the MIPI CSI-2 controller Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 07/14] dt-bindings: media: i2c: Add A31 MIPI CSI-2 bindings documentation Paul Kocialkowski
2020-10-26 16:14   ` Maxime Ripard
2020-10-27  9:52     ` Paul Kocialkowski
2020-10-27 18:44       ` Maxime Ripard
2020-11-04 10:48         ` Paul Kocialkowski [this message]
2020-11-04 16:53           ` Maxime Ripard
2020-10-30 16:33   ` Rob Herring
2020-10-30 16:56   ` Sakari Ailus
2020-10-23 17:45 ` [PATCH 08/14] media: sunxi: Add support for the A31 MIPI CSI-2 controller Paul Kocialkowski
2020-10-26  8:39   ` Dan Carpenter
2020-10-26 16:54   ` Maxime Ripard
2020-11-04 11:34     ` Paul Kocialkowski
2020-11-04 18:56       ` Maxime Ripard
2020-11-05 14:52         ` Paul Kocialkowski
2020-10-30 22:45   ` Helen Koike
2020-11-02  9:21     ` Maxime Ripard
2020-11-04 11:17       ` Paul Kocialkowski
2020-11-04 16:38         ` Helen Koike
2020-11-04 18:45           ` Maxime Ripard
2020-11-05 14:14             ` Helen Koike
2020-11-05  8:45   ` Sakari Ailus
2020-11-05 14:55     ` Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 09/14] ARM: dts: sun8i: v3s: Add CSI0 camera interface node Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 10/14] ARM: dts: sun8i: v3s: Add MIPI D-PHY and MIPI CSI-2 interface nodes Paul Kocialkowski
2020-10-26 16:55   ` Maxime Ripard
2020-10-23 17:45 ` [PATCH 11/14] dt-bindings: media: i2c: Add A83T MIPI CSI-2 bindings documentation Paul Kocialkowski
2020-10-26 16:56   ` Maxime Ripard
2020-11-04 10:33     ` Paul Kocialkowski
2020-11-05  8:48   ` Sakari Ailus
2020-10-23 17:45 ` [PATCH 12/14] media: sunxi: Add support for the A83T MIPI CSI-2 controller Paul Kocialkowski
2020-10-26  8:53   ` Dan Carpenter
2020-10-26 17:00   ` Maxime Ripard
2020-11-04 10:37     ` Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 13/14] ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 14/14] media: sunxi: sun8i-a83t-mipi-csi2: Avoid using the (unsolicited) interrupt Paul Kocialkowski
2020-10-26 16:57   ` Maxime Ripard
2020-10-26 17:20 ` [PATCH 00/14] Allwinner MIPI CSI-2 support for A31/V3s/A83T Maxime Ripard
2020-10-30 22:44 ` Helen Koike
2020-11-02  9:17   ` Maxime Ripard
2020-11-04 11:11   ` Paul Kocialkowski
2020-11-04 11:14     ` Paul Kocialkowski
2020-11-04 16:36     ` Helen Koike
2020-11-05 14:58       ` 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=20201104104827.GD285779@aptenodytes \
    --to=paul.kocialkowski@bootlin.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hans.verkuil@cisco.com \
    --cc=helen.koike@collabora.com \
    --cc=hverkuil@xs4all.nl \
    --cc=kevin.lhopital@hotmail.com \
    --cc=kishon@ti.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-sunxi@googlegroups.com \
    --cc=maxime@cerno.tech \
    --cc=mchehab@kernel.org \
    --cc=p.zabel@pengutronix.de \
    --cc=robh+dt@kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    --cc=thomas.petazzoni@bootlin.com \
    --cc=vkoul@kernel.org \
    --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

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git