On Thu, 9 Jan 2025 at 11:17, Dave Stevenson
<dave.stevenson@xxxxxxxxxxxxxxx> wrote:
>
> This also requires that the ranges for the exposure control
> are updated.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> ---
> drivers/media/i2c/imx415.c | 51 ++++++++++++++++++++++++++++++----------------
> 1 file changed, 33 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/i2c/imx415.c b/drivers/media/i2c/imx415.c
> index 3f7924aa1bd3..2c8c0905aa99 100644
> --- a/drivers/media/i2c/imx415.c
> +++ b/drivers/media/i2c/imx415.c
> @@ -26,6 +26,7 @@
> #define IMX415_PIXEL_ARRAY_WIDTH 3864
> #define IMX415_PIXEL_ARRAY_HEIGHT 2192
> #define IMX415_PIXEL_ARRAY_VBLANK 58
> +#define IMX415_EXPOSURE_OFFSET 8
>
> #define IMX415_NUM_CLK_PARAM_REGS 11
>
> @@ -51,6 +52,7 @@
> #define IMX415_OUTSEL CCI_REG8(0x30c0)
> #define IMX415_DRV CCI_REG8(0x30c1)
> #define IMX415_VMAX CCI_REG24_LE(0x3024)
> +#define IMX415_VMAX_MAX 0xfffff
> #define IMX415_HMAX CCI_REG16_LE(0x3028)
> #define IMX415_SHR0 CCI_REG24_LE(0x3050)
> #define IMX415_GAIN_PCG_0 CCI_REG16_LE(0x3090)
> @@ -447,7 +449,6 @@ static const struct imx415_clk_params imx415_clk_params[] = {
>
> /* all-pixel 2-lane 720 Mbps 15.74 Hz mode */
> static const struct cci_reg_sequence imx415_mode_2_720[] = {
> - { IMX415_VMAX, 0x08CA },
> { IMX415_HMAX, 0x07F0 },
> { IMX415_LANEMODE, IMX415_LANEMODE_2 },
> { IMX415_TCLKPOST, 0x006F },
> @@ -463,7 +464,6 @@ static const struct cci_reg_sequence imx415_mode_2_720[] = {
>
> /* all-pixel 2-lane 1440 Mbps 30.01 Hz mode */
> static const struct cci_reg_sequence imx415_mode_2_1440[] = {
> - { IMX415_VMAX, 0x08CA },
> { IMX415_HMAX, 0x042A },
> { IMX415_LANEMODE, IMX415_LANEMODE_2 },
> { IMX415_TCLKPOST, 0x009F },
> @@ -479,7 +479,6 @@ static const struct cci_reg_sequence imx415_mode_2_1440[] = {
>
> /* all-pixel 4-lane 891 Mbps 30 Hz mode */
> static const struct cci_reg_sequence imx415_mode_4_891[] = {
> - { IMX415_VMAX, 0x08CA },
> { IMX415_HMAX, 0x044C },
> { IMX415_LANEMODE, IMX415_LANEMODE_4 },
> { IMX415_TCLKPOST, 0x007F },
> @@ -600,6 +599,7 @@ struct imx415 {
> struct v4l2_ctrl *vblank;
> struct v4l2_ctrl *hflip;
> struct v4l2_ctrl *vflip;
> + struct v4l2_ctrl *exposure;
>
> unsigned int cur_mode;
> unsigned int num_data_lanes;
> @@ -730,17 +730,37 @@ static int imx415_s_ctrl(struct v4l2_ctrl *ctrl)
> ctrls);
> const struct v4l2_mbus_framefmt *format;
> struct v4l2_subdev_state *state;
> + u32 exposure_max;
> unsigned int vmax;
> unsigned int flip;
> int ret;
>
> - if (!pm_runtime_get_if_in_use(sensor->dev))
> - return 0;
> -
> state = v4l2_subdev_get_locked_active_state(&sensor->subdev);
> format = v4l2_subdev_state_get_format(state, 0);
>
> + if (ctrl->id == V4L2_CID_VBLANK) {
> + exposure_max = format->height + ctrl->val -
> + IMX415_EXPOSURE_OFFSET;
> + __v4l2_ctrl_modify_range(sensor->exposure,
> + sensor->exposure->minimum,
> + exposure_max, sensor->exposure->step,
> + sensor->exposure->default_value);
> + }
> +
> + if (!pm_runtime_get_if_in_use(sensor->dev))
> + return 0;
> +
> switch (ctrl->id) {
> + case V4L2_CID_VBLANK:
> + ret = cci_write(sensor->regmap, IMX415_VMAX,
> + format->height + ctrl->val, NULL);
> + if (ret)
> + return ret;
> + /*
> + * Deliberately fall through as exposure is set based on VMAX
> + * which has just changed.
> + */
> + fallthrough;
Checkpatch complains "WARNING: Prefer 'fallthrough;' over fallthrough comment"
Having a comment as to why we're wanting to fall through as well as
fallthrough; seems reasonable to me.
However what I have just noticed is that the V4L2_CID_EXPOSURE case
uses ctrl->val, which is going to be incorrect when we fall through.
I'll sort out an update in due course.
Dave
> case V4L2_CID_EXPOSURE:
> /* clamp the exposure value to VMAX. */
> vmax = format->height + sensor->vblank->cur.val;
> @@ -787,7 +807,8 @@ static int imx415_ctrls_init(struct imx415 *sensor)
> u64 pixel_rate = supported_modes[sensor->cur_mode].pixel_rate;
> u64 lane_rate = supported_modes[sensor->cur_mode].lane_rate;
> u32 exposure_max = IMX415_PIXEL_ARRAY_HEIGHT +
> - IMX415_PIXEL_ARRAY_VBLANK - 8;
> + IMX415_PIXEL_ARRAY_VBLANK -
> + IMX415_EXPOSURE_OFFSET;
> u32 hblank;
> unsigned int i;
> int ret;
> @@ -816,8 +837,9 @@ static int imx415_ctrls_init(struct imx415 *sensor)
> if (ctrl)
> ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> - v4l2_ctrl_new_std(&sensor->ctrls, &imx415_ctrl_ops, V4L2_CID_EXPOSURE,
> - 4, exposure_max, 1, exposure_max);
> + sensor->exposure = v4l2_ctrl_new_std(&sensor->ctrls, &imx415_ctrl_ops,
> + V4L2_CID_EXPOSURE, 4,
> + exposure_max, 1, exposure_max);
>
> v4l2_ctrl_new_std(&sensor->ctrls, &imx415_ctrl_ops,
> V4L2_CID_ANALOGUE_GAIN, IMX415_AGAIN_MIN,
> @@ -834,16 +856,9 @@ static int imx415_ctrls_init(struct imx415 *sensor)
> sensor->vblank = v4l2_ctrl_new_std(&sensor->ctrls, &imx415_ctrl_ops,
> V4L2_CID_VBLANK,
> IMX415_PIXEL_ARRAY_VBLANK,
> - IMX415_PIXEL_ARRAY_VBLANK, 1,
> - IMX415_PIXEL_ARRAY_VBLANK);
> - if (sensor->vblank)
> - sensor->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> + IMX415_VMAX_MAX - IMX415_PIXEL_ARRAY_HEIGHT,
> + 1, IMX415_PIXEL_ARRAY_VBLANK);
>
> - /*
> - * The pixel rate used here is a virtual value and can be used for
> - * calculating the frame rate together with hblank. It may not
> - * necessarily be the physically correct pixel clock.
> - */
> v4l2_ctrl_new_std(&sensor->ctrls, NULL, V4L2_CID_PIXEL_RATE, pixel_rate,
> pixel_rate, 1, pixel_rate);
>
>
> --
> 2.34.1
>
