Re: [PATCH v4 00/12] media: ov5640: Misc cleanup and improvements

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 2800 bytes --]

Hello Maxime,

On Thu, Oct 11, 2018 at 11:20:55AM +0200, 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,

I'm sorry to report this breaks CSI-2 capture on my i.MX6 testing
platform.

I have been testing the whole afternoon with different configurations,
but I have not been able yet to find out the root of the problem.

In the meantime, I have some comments on the patches, and I'll reply
to them singularly.

Thanks
   j

> Maxime
>
> Changes from v3:
>   - Rebased on current Sakari tree
>   - Fixed an error when changing only the framerate
>
> 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 (12):
>   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
>   ov5640: Enforce a mode change when changing the framerate
>
>  drivers/media/i2c/ov5640.c | 679 ++++++++++++++++++++-----------------
>  1 file changed, 374 insertions(+), 305 deletions(-)
>
> --
> 2.19.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]