Hi Laurent,
On Tue, Feb 01, 2022 at 09:07:35PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Jan 31, 2022 at 03:32:33PM +0100, Jacopo Mondi wrote:
> > The ov5640_mode_init_data is a fictional sensor more which is used to
> > program the initial sensor settings.
> >
> > It is only used to initialize the sensor and can be replaced
> > it with a throw-away mode which just wraps the register table.
> >
> > Also rename the register table to drop the format from the name to make
> > it clear an actual sensor mode has to be applied after the initial
> > programming.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
> > ---
> > drivers/media/i2c/ov5640.c | 32 +++++---------------------------
> > 1 file changed, 5 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index d966cca78e92..1e2f37c93f0d 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -348,7 +348,7 @@ static inline bool ov5640_is_mipi(struct ov5640_dev *sensor)
> > * over i2c.
> > */
> > /* YUV422 UYVY VGA@30fps */
> > -static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> > +static const struct reg_value ov5640_init_setting[] = {
> > {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> > {0x3103, 0x03, 0, 0}, {0x3630, 0x36, 0, 0},
> > {0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
> > @@ -586,30 +586,6 @@ static const struct reg_value ov5640_setting_QSXGA_2592_1944[] = {
> > {0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 70},
> > };
> >
> > -/* power-on sensor init reg table */
> > -static const struct ov5640_mode_info ov5640_mode_init_data = {
> > - .id = 0,
> > - .dn_mode = SUBSAMPLING,
> > - .pixel_rate = OV5640_PIXEL_RATE_96M,
> > - .analog_crop = {
> > - .left = 0,
> > - .top = 4,
> > - .width = 2623,
> > - .height = 1947,
> > - },
> > - .crop = {
> > - .left = 16,
> > - .top = 6,
> > - .width = 640,
> > - .height = 480,
> > - },
> > - .ppl = 1896,
> > - .vblank_def = 504,
> > - .reg_data = ov5640_init_setting_30fps_VGA,
> > - .reg_data_size = ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
> > - .max_fps = OV5640_30_FPS
> > -};
> > -
> > static const struct ov5640_mode_info
> > ov5640_mode_data[OV5640_NUM_MODES] = {
> > {
> > @@ -2117,13 +2093,15 @@ static int ov5640_set_framefmt(struct ov5640_dev *sensor,
> > /* restore the last set video mode after chip power-on */
> > static int ov5640_restore_mode(struct ov5640_dev *sensor)
> > {
> > + struct ov5640_mode_info init_data = {};
> > int ret;
> >
> > /* first load the initial register values */
> > - ret = ov5640_load_regs(sensor, &ov5640_mode_init_data);
> > + init_data.reg_data = ov5640_init_setting;
> > + init_data.reg_data_size = ARRAY_SIZE(ov5640_init_setting);
> > + ret = ov5640_load_regs(sensor, &init_data);
>
> Could we change ov5640_load_regs() to take a reg_value array and size as
> parameters, to avoid the local init_data variable ?
>
ov5640_load_regs() calls ov5640_set_timings() which wants a mode.
I can de-couple the two, even more so because here I'm passing in a
mode with invalid timings, which is not an issue as the actual ones
will be programmed at s_stream() time, but might be confusing when
reading the code in future.
> > if (ret < 0)
> > return ret;
> > - sensor->last_mode = &ov5640_mode_init_data;
> >
> > ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3f,
> > (ilog2(OV5640_SCLK2X_ROOT_DIV) << 2) |
>
> --
> Regards,
>
> Laurent Pinchart
