Hi Marek On Tue, Jul 25, 2023 at 12:35:00PM +0200, Marek Vasut wrote: > On 7/25/23 12:04, Jacopo Mondi wrote: > > Hi Marek > > Hi, > > > On Tue, Jul 25, 2023 at 11:41:18AM +0200, Marek Vasut wrote: > > > On 7/25/23 11:04, Jacopo Mondi wrote: > > > > Hi Marek > > > > > > Hi, > > > > > > > On Tue, Jul 25, 2023 at 12:22:10AM +0200, Marek Vasut wrote: > > > > > Set OV5640_REG_IO_MIPI_CTRL00 bit 2 to 1 instead of 0, since 1 means > > > > > MIPI CSI2 interface, while 0 means CPI parallel interface. > > > > > > > > > > In the ov5640_set_power_mipi() the interface should obviously be set > > > > > to MIPI CSI2 since this functions is used to power up the sensor when > > > > > operated in MIPI CSI2 mode. The sensor should not be in CPI mode in > > > > > that case. > > > > > > > > Does this actually help fixing your 'first frame corrupted issue' ? > > > > > > Yes it does. > > > Do you think it's worth mentioning it in the commit message ? > > > > I think the logic here was to power up the interface here in > > > > ov5640_set_power_mipi() with the CSI-2 interface disabled to enter > > > > LP-11 mode (something some receivers like the imx6 one are > > > > particularly sensible to). > > > > > > Per OV5640 datasheet. > > > > > > Register 0x300e bit 2 selects sensor interface mode between MIPI CSI2 and > > > CPI (parallel), it has nothing to do with LP modes . > > > > > > Register 0x3019 bits [6:4] control LP00/LP11 mode on CSI2 lines. > > > > > > > Then at stream time the CSI-2 interface is actually enabled in > > > > ov5640_set_stream_mipi() just before streaming is started. > > > > > > > > Also the register documentation is very confusing and as reported in > > > > ov5640_set_stream_mipi() it is also probably wrong (at least in the > > > > datasheet version I have). > > > > > > > > I would be particularly cautious in touching this sequence as it has > > > > been validated to work with multiple receivers. Of course if it > > > > actually fixes an issue for you it should be taken in, but ideally, as > > > > this sensor is still used in a large number of evaluation boards, it > > > > should be validated by other consumers of this driver (st, imx, > > > > microchip and rensas to name a few). Ok, I've been able to test on i.MX6Q which I was concerned for because of its sensitivness to LP-11 detection. Let alone that imx6 with ov5640 is broken on mainline because of unrelated reasons [1] I've been able to confirm that the sensor still works on this platform Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> [Test on imx6q] Tested-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> Can you confirm you have tested with iMX8MP as well ? Let me cc a few people that might be interested in testing this and give them a bit of time to do so. After that let's collect this patch! Thanks j [1] My tree looks like: 6205990fa14e media: ov5640: Enable MIPI interface in ov5640_set_power_mipi() 91f44d7815aa media: ov5640: Fix initial RESETB state and annotate timings 015497e35950 media: i2c: ov5640: Implement get_mbus_config cd4415af64e7 Revert "media: video-mux: update driver to active state" d716bd480968 Revert "media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()" > > > > > > That's quite a tall order which effectively makes it impossible to change or > > > fix anything in this driver. > > > > Really? Asking other people which use this driver to test a patch before > > merging it to make sure it doesn't break their setup makes "impossibile to > > change of fix anything" ? > > > > And it's not an order of any sort, but there are a lot of users of > > this driver and multiple times fixing a thing on one side breaks > > things for others, I'm just trying to coordinate between multiple > > developers. > > I'm afraid my expression of skepticism to the expected amount of testing has > been misinterpreted, and so was the message you were trying to convey, i.e. > communication breakdown. > > So let me rephrase. > > If the expectation is that every change to this driver has to be tested by > all aforementioned parties, then it will be very hard to get changes in, > based on my previous experience. > > Maybe let's stop this part of the thread here, and focus on the upper part?
