Re: [PATCH 0/5] media: Allwinner A10 CSI support

Hi,

On Wed, Nov 14, 2018 at 11:24:48AM +0800, Chen-Yu Tsai wrote:

On Tue, Nov 13, 2018 at 4:24 PM Maxime Ripard [off-list ref] wrote:

quoted

Here is a series introducing the support for the A10 (and SoCs of the same
generation) CMOS Sensor Interface (called CSI, not to be confused with
MIPI-CSI, which isn't support by that IP).

That interface is pretty straightforward, but the driver has a few issues
that I wanted to bring up:

  * The only board I've been testing this with has an ov5640 sensor
    attached, which doesn't work with the upstream driver. Copying the
    Allwinner init sequence works though, and this is how it has been
    tested. Testing with a second sensor would allow to see if it's an
    issue on the CSI side or the sensor side.
  * When starting a capture, the last buffer to capture will fail due to
    double buffering being used, and we don't have a next buffer for the
    last frame. I'm not sure how to deal with that though. It seems like
    some drivers use a scratch buffer in such a case, some don't care, so
    I'm not sure which solution should be preferred.
  * We don't have support for the ISP at the moment, but this can be added
    eventually.

  * How to model the CSI module clock isn't really clear to me. It looks
    like it goes through the CSI controller and then is muxed to one of the
    CSI pin so that it can clock the sensor. I'm not quite sure how to
    model it, if it should be a clock, the CSI driver being a clock
    provider, or if the sensor should just use the module clock directly.

Which clock are you talking about? MCLK? This seems to be fed directly from
the CCU, as there doesn't seem to be controls for it within the CSI hardware
block, and the diagram doesn't list it either. IMO you don't have to model it.
The camera sensor device node would just take a reference to it directly.

Yeah, that what I went for, I guess we agree :)

You would probably enable the (separate) pinmux setting in the CSI
controller node.

I'll change that.

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com