Hi Maxime & all OV5640 stakeholders, I've just pushed a new patchset also related to rate/pixel clock handling [1], based on your V3 great work: > media: ov5640: Adjust the clock based on the expected rate > media: ov5640: Remove the clocks registers initialization > media: ov5640: Remove redundant defines > media: ov5640: Remove redundant register setup > media: ov5640: Compute the clock rate at runtime > media: ov5640: Remove pixel clock rates > media: ov5640: Enhance FPS handling This is working perfectly fine on my parallel setup and allows me to well support VGA@30fps (instead 27) and also support XGA(1024x768)@15fps that I never seen working before. So at least for the parallel setup, this serie is working fine for all the discrete resolutions and framerate exposed by the driver for the moment: * QCIF 176x144 15/30fps * QVGA 320x240 15/30fps * VGA 640x480 15/30fps * 480p 720x480 15/30fps * XGA 1024x768 15/30fps * 720p 1280x720 15/30fps * 1080p 1920x1080 15/30fps * 5Mp 2592x1944 15fps Moreover I'm not clear on relationship between rate and pixel clock frequency. I've understood that to DVP_PCLK_DIVIDER (0x3824) register and VFIFO_CTRL0C (0x460c) affects the effective pixel clock frequency. All the resolutions up to 720x576 are forcing a manual value of 2 for divider (0x460c=0x22), but including 720p and more, the divider value is controlled by "auto-mode" (0x460c=0x20)... from what I measured and understood, for those resolutions, the divider must be set to 1 in order that your rate computation match the effective pixel clock on output, see [2]. So I wonder if this PCLK divider register should be included or not into your rate computation, what do you think ? [1] OV5640: reduce rate according to maximum pixel clock https://www.spinics.net/lists/linux-media/msg140958.html: [2] media: ov5640: move parallel port pixel clock divider out of registers set https://www.spinics.net/lists/linux-media/msg140960.html BR, Hugues. On 05/17/2018 10:53 AM, Maxime Ripard wrote: > Hi, > > Here is a "small" series that mostly cleans up the ov5640 driver code, > slowly getting rid of the big data array for more understandable code > (hopefully). > > The biggest addition would be the clock rate computation at runtime, > instead of relying on those arrays to setup the clock tree > properly. As a side effect, it fixes the framerate that was off by > around 10% on the smaller resolutions, and we now support 60fps. > > This also introduces a bunch of new features. > > Let me know what you think, > Maxime > > Changes from v2: > - Rebased on latest Sakari PR > - Fixed the issues reported by Hugues: improper FPS returned for > formats, improper rounding of the FPS, some with his suggestions, > some by simplifying the logic. > - Expanded the clock tree comments based on the feedback from Samuel > Bobrowicz and Loic Poulain > - Merged some of the changes made by Samuel Bobrowicz to fix the > MIPI rate computation, fix the call sites of the > ov5640_set_timings function, the auto-exposure calculation call, > etc. > - Split the patches into smaller ones in order to make it more > readable (hopefully) > > Changes from v1: > - Integrated Hugues' suggestions to fix v4l2-compliance > - Fixed the bus width with JPEG > - Dropped the clock rate calculation loops for something simpler as > suggested by Sakari > - Cache the exposure value instead of using the control value > - Rebased on top of 4.17 > > Maxime Ripard (11): > media: ov5640: Adjust the clock based on the expected rate > media: ov5640: Remove the clocks registers initialization > media: ov5640: Remove redundant defines > media: ov5640: Remove redundant register setup > media: ov5640: Compute the clock rate at runtime > media: ov5640: Remove pixel clock rates > media: ov5640: Enhance FPS handling > media: ov5640: Make the return rate type more explicit > media: ov5640: Make the FPS clamping / rounding more extendable > media: ov5640: Add 60 fps support > media: ov5640: Remove duplicate auto-exposure setup > > Samuel Bobrowicz (1): > media: ov5640: Fix timings setup code > > drivers/media/i2c/ov5640.c | 701 +++++++++++++++++++++---------------- > 1 file changed, 392 insertions(+), 309 deletions(-) >
