Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 4 Sep 2020 12:44:14 +0200
From:   Jacopo Mondi <jacopo@jmondi.org>
To:     Sakari Ailus <sakari.ailus@linux.intel.com>
Cc:     Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
        Steve Longerbeam <slongerbeam@gmail.com>,
        Jacopo Mondi <jacopo+renesas@jmondi.org>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Paul <paul.kocialkowski@bootlin.com>,
        Hugues Fruchet <hugues.fruchet@st.com>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-renesas-soc@vger.kernel.org,
        Prabhakar <prabhakar.csengg@gmail.com>
Subject: Re: [PATCH v3 1/3] media: i2c: ov5640: Enable data pins on poweron
 for DVP mode
Message-ID: <20200904104414.ilqc2qhustefiwdx@uno.localdomain>
References: <20200813171337.5540-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <20200813171337.5540-2-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <20200901221052.GC32646@paasikivi.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200901221052.GC32646@paasikivi.fi.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Prabhakar, Sakari,

On Wed, Sep 02, 2020 at 01:10:53AM +0300, Sakari Ailus wrote:
> Hi Prabhakar,
>
> My apologies for the late reply.
>
> On Thu, Aug 13, 2020 at 06:13:35PM +0100, Lad Prabhakar wrote:
> > During testing this sensor on iW-RainboW-G21D-Qseven platform in 8-bit DVP
> > mode with rcar-vin bridge noticed the capture worked fine for the first run
> > (with yavta), but for subsequent runs the bridge driver waited for the
> > frame to be captured. Debugging further noticed the data lines were
> > enabled/disabled in stream on/off callback and dumping the register
> > contents 0x3017/0x3018 in ov5640_set_stream_dvp() reported the correct
> > values, but yet frame capturing failed.
> >
> > To get around this issue the following actions are performed for
> > parallel mode (DVP):
> > 1: Keeps the sensor in software power down mode and is woken up only in
> >    ov5640_set_stream_dvp() callback.
>
> I'd suppose with s_power, the main driver would power the device off
> when it's not streaming.
>
> > 2: Enables data lines in s_power callback
> > 3: Configures HVP lines in s_power callback instead of configuring
> >    everytime in ov5640_set_stream_dvp().
> > 4: Disables MIPI interface.
>
> Could you split this into two (or even more) patches so that the first
> refactors the receiver setup and another one changes how it actually works?
> That way this would be quite a bit easier to review.
>
> While some of the above seem entirely reasonable, the changes are vast and
> testing should be done on different boards to make sure things won't break.
> That said, this depends on others who have the hardware.

I left it as a comment during review of v2, but now more formally:

For CSI-2 capture operations:
Tested-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>
> --
> Kind regards,
>
> Sakari Ailus