Re: [PATCH 15/22] media: sunxi: Remove the sun6i-csi driver implementation

[Maxime Ripard]

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