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 <paul.kocialkowski@xxxxxxxxxxx> 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:
signature.asc
Description: PGP signature
