From: Maxime Ripard <firstname.lastname@example.org> To: Paul Kocialkowski <email@example.com> Cc: firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, Yong Deng <firstname.lastname@example.org>, Mauro Carvalho Chehab <email@example.com>, Rob Herring <firstname.lastname@example.org>, Sakari Ailus <email@example.com>, Hans Verkuil <firstname.lastname@example.org>, Chen-Yu Tsai <email@example.com>, Jernej Skrabec <firstname.lastname@example.org>, Greg Kroah-Hartman <email@example.com>, Helen Koike <firstname.lastname@example.org>, Laurent Pinchart <email@example.com>, Thomas Petazzoni <firstname.lastname@example.org> Subject: Re: [PATCH 15/22] media: sunxi: Remove the sun6i-csi driver implementation Date: Mon, 13 Sep 2021 10:17:07 +0200 [thread overview] Message-ID: <20210913081707.3pjcfuwan46pbdep@gilmour> (raw) In-Reply-To: <email@example.com> [-- Attachment #1: Type: text/plain, Size: 3381 bytes --] On Fri, Sep 10, 2021 at 08:41:40PM +0200, Paul Kocialkowski wrote: > As described in the commit adding support for the new sun6i-csi driver, > a complete rewrite was necessary to support the Allwinner A31 ISP as > well as fix a number of issues with the current implementation. > > Farewell and thanks for all the pixels! > > Signed-off-by: Paul Kocialkowski <firstname.lastname@example.org> For completeness, this is what the other commit log mentions: > While adapting the sun6i-csi driver for MIPI CSI-2 support was > possible, it became clear that adding support for the ISP required > very heavy changes to the driver which were quite hard to break down > into a series of subsequent changes. > The first major difficulty comes from the lack of v4l2 subdev that > acts a bridge, separate from the video node representing the DMA > engine. To support the ISP, only parts of the hardware must be > configured (excluding aspects related to the DMA output), which made > the separation a hard requirement. > Another significant difficulty was the specific dance that is required > to have both the ISP and CSI device be part of the same media device. > Because the ISP and CSI are two different hardware blocks, they have > two distinct drivers that will each try to register their own v4l2 > and media devices, resulting in two distinct pipelines. When the ISP > is in use, we actually want the CSI driver to register with the ISP's > v4l2 and media devices while keeping the ability to register its own > when the ISP is not in use. This is done by: > 1. Having the CSI driver check whether the ISP is available, using > sun6i_csi_isp_detect(); > 2. If not, it can register when its own async subdevs are ready, using > sun6i_csi_v4l2_complete(); > 3. If so, it will register its bridge as an async subdev which will > be picked-up by the ISP driver (from the fwnode graph link); > 4. When the subdev becomes bound to the ISP's v4l2 device, we can > then access that device (and the associated media device) to > complete registration of the capture video node, using > sun6i_csi_isp_complete(); > Besides the logic rework, other issues were identified and resolved: > - The sync mechanism for buffer flipping was based on the frame done > interrupt, which is too late (next frame is already being processed). > This lead to requiring 3 buffers to start and writing two addresses > when starting. Using vsync as a sync point seems to be the correct > approach and allows using only two buffers without tearing; > - Using devm_regmap_init_mmio_clk was incorrect since the reset also > comes into play; > - Some register definitions were inverted compared to their actual > effect (which was inherited from the Allwinner documentation and > code): comments were added where relevant; > - The deprecated v4l2_async_notifier_parse_fwnode_endpoints() helper > is no longer used by the driver; With that being said, NAK. Having heavy changes to a driver is completely fine, and is kind of expected really with such a big change. Breaking all possibility of bisection and throwing away years of stabilization and maintenance isn't. And all those small bug fixes you mention at the end are just that: small bug fixes that can be done on the current driver just fine too. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --]
next prev parent reply other threads:[~2021-09-13 8:17 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-10 18:41 [PATCH 00/22] Allwinner A31/A83T MIPI CSI-2 Support and A31 ISP Support Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 01/22] clk: sunxi-ng: v3s: Make the ISP PLL clock public Paul Kocialkowski 2021-09-13 7:54 ` Maxime Ripard 2021-09-13 8:53 ` Paul Kocialkowski 2021-09-16 16:30 ` Maxime Ripard 2021-09-10 18:41 ` [PATCH 02/22] ARM: dts: sun8i: v3s: Parent the CSI module clock to the ISP PLL Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 03/22] dt-bindings: sun6i-a31-mipi-dphy: Add optional direction property Paul Kocialkowski 2021-09-13 8:00 ` Maxime Ripard 2021-09-14 7:39 ` Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 04/22] phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI CSI-2 Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 05/22] dt-bindings: media: sun6i-a31-csi: Add MIPI CSI-2 input port Paul Kocialkowski 2021-09-13 8:09 ` Maxime Ripard 2021-09-14 7:43 ` Paul Kocialkowski 2021-09-14 12:06 ` Maxime Ripard 2021-09-10 18:41 ` [PATCH 06/22] dt-bindings: media: Add Allwinner A31 MIPI CSI-2 bindings documentation Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 07/22] media: sunxi: Add support for the A31 MIPI CSI-2 controller Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 08/22] MAINTAINERS: Add entry for the Allwinner A31 MIPI CSI-2 bridge driver Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 09/22] ARM: dts: sun8i: v3s: Add nodes for MIPI CSI-2 support Paul Kocialkowski 2021-09-11 2:32 ` Samuel Holland 2021-09-13 7:44 ` Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 10/22] dt-bindings: media: Add Allwinner A83T MIPI CSI-2 bindings documentation Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 11/22] media: sunxi: Add support for the A83T MIPI CSI-2 controller Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 12/22] MAINTAINERS: Add entry for the Allwinner A83T MIPI CSI-2 bridge Paul Kocialkowski 2021-09-10 18:41 ` [PATCH NOT FOR MERGE 13/22] ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node Paul Kocialkowski 2021-09-11 2:53 ` Chen-Yu Tsai 2021-09-13 7:45 ` Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 14/22] ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865 Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 15/22] media: sunxi: Remove the sun6i-csi driver implementation Paul Kocialkowski 2021-09-13 8:17 ` Maxime Ripard [this message] 2021-09-14 8:04 ` Paul Kocialkowski 2021-09-15 19:51 ` Sakari Ailus 2021-09-10 18:41 ` [PATCH 16/22] media: sunxi: Introduce a rewritten sun6i-csi driver Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 17/22] dt-bindings: media: Add Allwinner A31 ISP bindings documentation Paul Kocialkowski 2021-09-13 8:18 ` Maxime Ripard 2021-09-14 7:44 ` Paul Kocialkowski 2021-09-14 12:07 ` Maxime Ripard 2021-09-10 18:41 ` [PATCH 18/22] dt-bindings: media: sun6i-a31-csi: Add ISP output port Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 19/22] soc: sunxi: mbus: Add A31 ISP compatibles to the list Paul Kocialkowski 2021-09-11 2:36 ` Samuel Holland 2021-09-13 7:45 ` Paul Kocialkowski 2021-09-13 8:32 ` Maxime Ripard 2021-09-10 18:41 ` [PATCH 20/22] staging: media: Add support for the Allwinner A31 ISP Paul Kocialkowski 2021-09-13 8:31 ` Maxime Ripard 2021-09-14 7:50 ` Paul Kocialkowski 2021-09-14 11:11 ` Laurent Pinchart 2021-09-14 11:48 ` Maxime Ripard 2021-09-10 18:41 ` [PATCH 21/22] MAINTAINERS: Add entry for the Allwinner A31 ISP driver Paul Kocialkowski 2021-09-10 18:41 ` [PATCH 22/22] ARM: dts: sun8i: v3s: Add support for the ISP 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=20210913081707.3pjcfuwan46pbdep@gilmour \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH 15/22] media: sunxi: Remove the sun6i-csi driver implementation' \ /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
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).