| Message ID | 20220131143245.128089-10-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov5640: Rework the clock tree programming for MIPI | expand |
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@jmondi.org> > --- > 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 ? > 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) |
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@jmondi.org> > > --- > > 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
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); 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) |
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@jmondi.org> --- drivers/media/i2c/ov5640.c | 32 +++++--------------------------- 1 file changed, 5 insertions(+), 27 deletions(-)