[PATCH v2 00/10] media: ar0521: Add analog gain, rework clock tree

[PATCH v2 00/10] media: ar0521: Add analog gain, rework clock tree

[Jacopo Mondi]

Hello,
  this series adds a few features to the ar0521 driver to enable its usage
with libcamera.

In particular:
- enum_frame_sizes
- global analog gain control
- LINK_FREQ
- Rework blanking handligs

v2:

- I have dropped the most controverse part that allows to change the link
  frequency to obtain 60FPS. It can be eventually be applied on top.
- Use register 0x3028 to control analog gain not to overwrite the global digital
  gain.
- Fix the HBLANK/VBLANK max by using the values read from on-chip registers.
- Fix handling of LINK_FREQ in s_cltr (but do not make the control read only).
- Fix errors reported by 0-days:
  - use do_div() for 64-bit division
  - declare variables in function scope and not in case scope in s_ctrl

Thanks
   j

Jacopo Mondi (10):
  media: ar0521: Implement enum_frame_sizes
  media: ar0521: Add V4L2_CID_ANALOG_GAIN
  media: ar0521: Set maximum resolution to 2592x1944
  media: ar0521: Rework PLL computation
  media: ar0521: Refuse unsupported controls
  media: ar0521: Add LINK_FREQ control
  media: ar0521: Adjust exposure and blankings limits
  media: ar0521: Setup controls at s_stream time
  media: ar0521: Rework startup sequence
  media: ar0521: Tab-align definitions

 drivers/media/i2c/ar0521.c | 348 ++++++++++++++++++++++++++-----------
 1 file changed, 245 insertions(+), 103 deletions(-)

--
2.37.3

[PATCH v2 01/10] media: ar0521: Implement enum_frame_sizes

[Jacopo Mondi]

Implement the enum_frame_size pad operation.

The sensor supports a continuous size range of resolutions.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ar0521.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index c6ab531532be..0daa61df2603 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -798,6 +798,24 @@ static int ar0521_enum_mbus_code(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int ar0521_enum_frame_size(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *sd_state,
+				  struct v4l2_subdev_frame_size_enum *fse)
+{
+	if (fse->index)
+		return -EINVAL;
+
+	if (fse->code != MEDIA_BUS_FMT_SGRBG8_1X8)
+		return -EINVAL;
+
+	fse->min_width = AR0521_WIDTH_MIN;
+	fse->max_width = AR0521_WIDTH_MAX;
+	fse->min_height = AR0521_HEIGHT_MIN;
+	fse->max_height = AR0521_HEIGHT_MAX;
+
+	return 0;
+}
+
 static int ar0521_pre_streamon(struct v4l2_subdev *sd, u32 flags)
 {
 	struct ar0521_dev *sensor = to_ar0521_dev(sd);
@@ -864,6 +882,7 @@ static const struct v4l2_subdev_video_ops ar0521_video_ops = {
 
 static const struct v4l2_subdev_pad_ops ar0521_pad_ops = {
 	.enum_mbus_code = ar0521_enum_mbus_code,
+	.enum_frame_size = ar0521_enum_frame_size,
 	.get_fmt = ar0521_get_fmt,
 	.set_fmt = ar0521_set_fmt,
 };
-- 
2.37.3

[PATCH v2 02/10] media: ar0521: Add V4L2_CID_ANALOG_GAIN

[Jacopo Mondi]

Add support for V4L2_CID_ANALOG_GAIN. The control programs the global
gain register which applies to all color channels.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ar0521.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index 0daa61df2603..ba169f0218a9 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -35,6 +35,11 @@
 #define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
 #define AR0521_TOTAL_WIDTH_MIN	     2968u
 
+#define AR0521_ANA_GAIN_MIN		0x00
+#define AR0521_ANA_GAIN_MAX		0x3f
+#define AR0521_ANA_GAIN_STEP		0x01
+#define AR0521_ANA_GAIN_DEFAULT		0x00
+
 /* AR0521 registers */
 #define AR0521_REG_VT_PIX_CLK_DIV		0x0300
 #define AR0521_REG_FRAME_LENGTH_LINES		0x0340
@@ -50,6 +55,8 @@
 #define   AR0521_REG_RESET_RESTART		  BIT(1)
 #define   AR0521_REG_RESET_INIT			  BIT(0)
 
+#define AR0521_REG_ANA_GAIN_CODE_GLOBAL		0x3028
+
 #define AR0521_REG_GREEN1_GAIN			0x3056
 #define AR0521_REG_BLUE_GAIN			0x3058
 #define AR0521_REG_RED_GAIN			0x305A
@@ -456,6 +463,10 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_VBLANK:
 		ret = ar0521_set_geometry(sensor);
 		break;
+	case V4L2_CID_ANALOGUE_GAIN:
+		ret = ar0521_write_reg(sensor, AR0521_REG_ANA_GAIN_CODE_GLOBAL,
+				       ctrl->val);
+		break;
 	case V4L2_CID_GAIN:
 	case V4L2_CID_RED_BALANCE:
 	case V4L2_CID_BLUE_BALANCE:
@@ -499,6 +510,11 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
 	/* We can use our own mutex for the ctrl lock */
 	hdl->lock = &sensor->lock;
 
+	/* Analog gain */
+	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
+			  AR0521_ANA_GAIN_MIN, AR0521_ANA_GAIN_MAX,
+			  AR0521_ANA_GAIN_STEP, AR0521_ANA_GAIN_DEFAULT);
+
 	/* Manual gain */
 	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 511, 1, 0);
 	ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_RED_BALANCE,
-- 
2.37.3

[PATCH v2 03/10] media: ar0521: Set maximum resolution to 2592x1944

[Jacopo Mondi]

Change the largest visibile resolution to 2592x1944, which corresponds
to the active pixel array area size. Take into account the horizontal
and vertical limits when programming the visible sizes to skip
dummy/inactive pixels.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ar0521.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index ba169f0218a9..dfa4de0f4996 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -26,10 +26,17 @@
 #define AR0521_PIXEL_CLOCK_MIN	 (168 * 1000 * 1000)
 #define AR0521_PIXEL_CLOCK_MAX	 (414 * 1000 * 1000)
 
+#define AR0521_NATIVE_WIDTH		2604u
+#define AR0521_NATIVE_HEIGHT		1964u
+#define AR0521_MIN_X_ADDR_START		0u
+#define AR0521_MIN_Y_ADDR_START		0u
+#define AR0521_MAX_X_ADDR_END		2603u
+#define AR0521_MAX_Y_ADDR_END		1955u
+
 #define AR0521_WIDTH_MIN	       8u
-#define AR0521_WIDTH_MAX	    2608u
+#define AR0521_WIDTH_MAX	    2592u
 #define AR0521_HEIGHT_MIN	       8u
-#define AR0521_HEIGHT_MAX	    1958u
+#define AR0521_HEIGHT_MAX	    1944u
 
 #define AR0521_WIDTH_BLANKING_MIN     572u
 #define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
@@ -176,13 +183,17 @@ static int ar0521_write_reg(struct ar0521_dev *sensor, u16 reg, u16 val)
 
 static int ar0521_set_geometry(struct ar0521_dev *sensor)
 {
+	/* Center the image in the visible output window. */
+	u16 x = clamp((AR0521_WIDTH_MAX - sensor->fmt.width) / 2,
+		       AR0521_MIN_X_ADDR_START, AR0521_MAX_X_ADDR_END);
+	u16 y = clamp(((AR0521_HEIGHT_MAX - sensor->fmt.height) / 2) & ~1,
+		       AR0521_MIN_Y_ADDR_START, AR0521_MAX_Y_ADDR_END);
+
 	/* All dimensions are unsigned 12-bit integers */
-	u16 x = (AR0521_WIDTH_MAX - sensor->fmt.width) / 2;
-	u16 y = ((AR0521_HEIGHT_MAX - sensor->fmt.height) / 2) & ~1;
 	__be16 regs[] = {
 		be(AR0521_REG_FRAME_LENGTH_LINES),
-		be(sensor->total_height),
-		be(sensor->total_width),
+		be(sensor->fmt.height + sensor->ctrls.vblank->val),
+		be(sensor->fmt.width + sensor->ctrls.hblank->val),
 		be(x),
 		be(y),
 		be(x + sensor->fmt.width - 1),
-- 
2.37.3

[PATCH v2 04/10] media: ar0521: Rework PLL computation

[Jacopo Mondi]

Rework the PLL computation procedure to take into account the currently
configured format bpp and the number of data lanes.

Comment the PLL configuration procedure with information provided by the
sensor chip manual and remove the hardcoded divider from the pixel clock
calculation.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ar0521.c | 113 +++++++++++++++++++++++++++----------
 1 file changed, 84 insertions(+), 29 deletions(-)

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index dfa4de0f4996..e673424880ac 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -21,7 +21,7 @@
 #define AR0521_PLL_MIN		 (320 * 1000 * 1000)
 #define AR0521_PLL_MAX		(1280 * 1000 * 1000)
 
-/* Effective pixel clocks, the registers may be DDR */
+/* Effective pixel sample rate on the pixel array. */
 #define AR0521_PIXEL_CLOCK_RATE	 (184 * 1000 * 1000)
 #define AR0521_PIXEL_CLOCK_MIN	 (168 * 1000 * 1000)
 #define AR0521_PIXEL_CLOCK_MAX	 (414 * 1000 * 1000)
@@ -123,10 +123,14 @@ struct ar0521_dev {
 	unsigned int lane_count;
 	u16 total_width;
 	u16 total_height;
-	u16 pll_pre;
-	u16 pll_mult;
-	u16 pll_pre2;
-	u16 pll_mult2;
+	struct {
+		u16 pre;
+		u16 mult;
+		u16 pre2;
+		u16 mult2;
+		u16 vt_pix;
+	} pll;
+
 	bool streaming;
 };
 
@@ -151,6 +155,16 @@ static u32 div64_round_up(u64 v, u32 d)
 	return div_u64(v + d - 1, d);
 }
 
+static int ar0521_code_to_bpp(struct ar0521_dev *sensor)
+{
+	switch (sensor->fmt.code) {
+	case MEDIA_BUS_FMT_SGRBG8_1X8:
+		return 8;
+	}
+
+	return -EINVAL;
+}
+
 /* Data must be BE16, the first value is the register address */
 static int ar0521_write_regs(struct ar0521_dev *sensor, const __be16 *data,
 			     unsigned int count)
@@ -226,8 +240,7 @@ static int ar0521_set_gains(struct ar0521_dev *sensor)
 	return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
 }
 
-static u32 calc_pll(struct ar0521_dev *sensor, int num, u32 freq, u16 *pre_ptr,
-		    u16 *mult_ptr)
+static u32 calc_pll(struct ar0521_dev *sensor, u32 freq, u16 *pre_ptr, u16 *mult_ptr)
 {
 	u16 pre = 1, mult = 1, new_pre;
 	u32 pll = AR0521_PLL_MAX + 1;
@@ -262,37 +275,79 @@ static u32 calc_pll(struct ar0521_dev *sensor, int num, u32 freq, u16 *pre_ptr,
 	return pll;
 }
 
-#define DIV 4
 static void ar0521_calc_mode(struct ar0521_dev *sensor)
 {
-	unsigned int speed_mod = 4 / sensor->lane_count; /* 1 with 4 DDR lanes */
-	u16 total_width = max(sensor->fmt.width + AR0521_WIDTH_BLANKING_MIN,
-			      AR0521_TOTAL_WIDTH_MIN);
-	u16 total_height = sensor->fmt.height + AR0521_HEIGHT_BLANKING_MIN;
-
-	/* Calculate approximate pixel clock first */
-	u64 pix_clk = AR0521_PIXEL_CLOCK_RATE;
-
-	/* PLL1 drives pixel clock - dual rate */
-	pix_clk = calc_pll(sensor, 1, pix_clk * (DIV / 2), &sensor->pll_pre,
-			   &sensor->pll_mult);
-	pix_clk = div64_round(pix_clk, (DIV / 2));
-	calc_pll(sensor, 2, pix_clk * (DIV / 2) * speed_mod, &sensor->pll_pre2,
-		 &sensor->pll_mult2);
-
-	sensor->total_width = total_width;
-	sensor->total_height = total_height;
+	unsigned int pixel_clock;
+	u16 pre, mult;
+	u32 vco;
+	int bpp;
+
+	/*
+	 * PLL1 and PLL2 are computed equally even if the application note
+	 * suggests a slower PLL1 clock. Maintain pll1 and pll2 divider and
+	 * multiplier separated to later specialize the calculation procedure.
+	 *
+	 * PLL1:
+	 * - mclk -> / pre_div1 * pre_mul1 = VCO1 = COUNTER_CLOCK
+	 *
+	 * PLL2:
+	 * - mclk -> / pre_div * pre_mul = VCO
+	 *
+	 *   VCO -> / vt_pix = PIXEL_CLOCK
+	 *   VCO -> / vt_pix / 2 = WORD_CLOCK
+	 *   VCO -> / op_sys = SERIAL_CLOCK
+	 *
+	 * With:
+	 * - vt_pix = bpp / 2
+	 * - WORD_CLOCK = PIXEL_CLOCK / 2
+	 * - SERIAL_CLOCK = MIPI data rate (Mbps / lane) = WORD_CLOCK * bpp
+	 *   NOTE: this implies the MIPI clock is divided internally by 2
+	 *         to account for DDR.
+	 *
+	 * As op_sys_div is fixed to 1:
+	 *
+	 * SERIAL_CLOCK = VCO
+	 * VCO = 2 * MIPI_CLK
+	 * VCO = PIXEL_CLOCK * bpp / 2
+	 *
+	 * In the clock tree:
+	 * MIPI_CLK = PIXEL_CLOCK * bpp / 2 / 2
+	 *
+	 * Generic pixel_rate to bus clock frequencey equation:
+	 * MIPI_CLK = V4L2_CID_PIXEL_RATE * bpp / lanes / 2
+	 *
+	 * From which we derive the PIXEL_CLOCK to use in the clock tree:
+	 * PIXEL_CLOCK = V4L2_CID_PIXEL_RATE * 2 / lanes
+	 *
+	 * Documented clock ranges:
+	 *   WORD_CLOCK = (35MHz - 120 MHz)
+	 *   PIXEL_CLOCK = (84MHz - 207MHz)
+	 *   VCO = (320MHz - 1280MHz)
+	 *
+	 * TODO: in case we have less data lanes we have to reduce the desired
+	 * VCO not to exceed the limits specified by the datasheet and
+	 * consequentially reduce the obtained pixel clock.
+	 */
+	pixel_clock = AR0521_PIXEL_CLOCK_RATE * 2 / sensor->lane_count;
+	bpp = ar0521_code_to_bpp(sensor);
+	sensor->pll.vt_pix = bpp / 2;
+	vco = pixel_clock * sensor->pll.vt_pix;
+
+	calc_pll(sensor, vco, &pre, &mult);
+
+	sensor->pll.pre = sensor->pll.pre2 = pre;
+	sensor->pll.mult = sensor->pll.mult2 = mult;
 }
 
 static int ar0521_write_mode(struct ar0521_dev *sensor)
 {
 	__be16 pll_regs[] = {
 		be(AR0521_REG_VT_PIX_CLK_DIV),
-		/* 0x300 */ be(4), /* vt_pix_clk_div = number of bits / 2 */
+		/* 0x300 */ be(sensor->pll.vt_pix), /* vt_pix_clk_div = bpp / 2 */
 		/* 0x302 */ be(1), /* vt_sys_clk_div */
-		/* 0x304 */ be((sensor->pll_pre2 << 8) | sensor->pll_pre),
-		/* 0x306 */ be((sensor->pll_mult2 << 8) | sensor->pll_mult),
-		/* 0x308 */ be(8), /* op_pix_clk_div = 2 * vt_pix_clk_div */
+		/* 0x304 */ be((sensor->pll.pre2 << 8) | sensor->pll.pre),
+		/* 0x306 */ be((sensor->pll.mult2 << 8) | sensor->pll.mult),
+		/* 0x308 */ be(sensor->pll.vt_pix * 2), /* op_pix_clk_div = 2 * vt_pix_clk_div */
 		/* 0x30A */ be(1)  /* op_sys_clk_div */
 	};
 	int ret;
-- 
2.37.3

[PATCH v2 05/10] media: ar0521: Refuse unsupported controls

[Jacopo Mondi]

Refuse unsupported controls by returning -EINVAL in the s_ctrl
operation.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ar0521.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index e673424880ac..fcd852760750 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -547,6 +547,10 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE,
 				       ctrl->val);
 		break;
+	default:
+		dev_err(&sensor->i2c_client->dev,
+			"Unsupported control %x\n", ctrl->id);
+		return -EINVAL;
 	}
 
 	pm_runtime_put(&sensor->i2c_client->dev);
-- 
2.37.3

[PATCH v2 06/10] media: ar0521: Add LINK_FREQ control

[Jacopo Mondi]

Add support for V4L2_CID_LINK_FREQ which currently reports a single
hard-coded frequency which depends on the fixed pixel clock.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ar0521.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index fcd852760750..2310346f11d5 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -89,6 +89,10 @@ static const char * const ar0521_supply_names[] = {
 	"vaa",		/* Analog (2.7V) supply */
 };
 
+static const s64 ar0521_link_frequencies[] = {
+	184000000,
+};
+
 struct ar0521_ctrls {
 	struct v4l2_ctrl_handler handler;
 	struct {
@@ -547,6 +551,13 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE,
 				       ctrl->val);
 		break;
+	case V4L2_CID_LINK_FREQ:
+		/*
+		 * Link frequency index is used at PLL configuration time,
+		 * nothing to do here.
+		 */
+		ret = 0;
+		break;
 	default:
 		dev_err(&sensor->i2c_client->dev,
 			"Unsupported control %x\n", ctrl->id);
@@ -611,6 +622,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
 	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0,
 					    65535, 1, 360);
 
+	v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
+			       ARRAY_SIZE(ar0521_link_frequencies) - 1,
+			       0, ar0521_link_frequencies);
+
 	ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops,
 					V4L2_CID_TEST_PATTERN,
 					ARRAY_SIZE(test_pattern_menu) - 1,
-- 
2.37.3

[PATCH v2 07/10] media: ar0521: Adjust exposure and blankings limits

[Jacopo Mondi]

Adjust the control limits for V4L2_CID_VBLANK, V4L2_CID_HBLANK and
V4L2_CID_EXPOSURE when a new format is applied to the sensor.

Also update the exposure control when a new blanking value is applied.

Also change the controls initialization to use valid values for the
default format.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ar0521.c | 79 ++++++++++++++++++++++++++++----------
 1 file changed, 59 insertions(+), 20 deletions(-)

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index 2310346f11d5..854f4ccd9c3d 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -40,7 +40,8 @@
 
 #define AR0521_WIDTH_BLANKING_MIN     572u
 #define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
-#define AR0521_TOTAL_WIDTH_MIN	     2968u
+#define AR0521_TOTAL_HEIGHT_MAX     65535u /* max_frame_lenght_lines */
+#define AR0521_TOTAL_WIDTH_MAX      65532u /* max_line_lenght_pck */
 
 #define AR0521_ANA_GAIN_MIN		0x00
 #define AR0521_ANA_GAIN_MAX		0x3f
@@ -125,8 +126,6 @@ struct ar0521_dev {
 	struct v4l2_mbus_framefmt fmt;
 	struct ar0521_ctrls ctrls;
 	unsigned int lane_count;
-	u16 total_width;
-	u16 total_height;
 	struct {
 		u16 pre;
 		u16 mult;
@@ -483,7 +482,8 @@ static int ar0521_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_format *format)
 {
 	struct ar0521_dev *sensor = to_ar0521_dev(sd);
-	int ret = 0;
+	int exposure_max, exposure_val;
+	int max_vblank, max_hblank;
 
 	ar0521_adj_fmt(&format->format);
 
@@ -494,33 +494,64 @@ static int ar0521_set_fmt(struct v4l2_subdev *sd,
 
 		fmt = v4l2_subdev_get_try_format(sd, sd_state, 0 /* pad */);
 		*fmt = format->format;
-	} else {
-		sensor->fmt = format->format;
-		ar0521_calc_mode(sensor);
+
+		mutex_unlock(&sensor->lock);
+
+		return 0;
 	}
 
+	sensor->fmt = format->format;
+	ar0521_calc_mode(sensor);
+
+	/*
+	 * Update the exposure and blankings limits. Blankings are also reset
+	 * to the minimum.
+	 */
+	max_hblank = AR0521_TOTAL_WIDTH_MAX - sensor->fmt.width;
+	__v4l2_ctrl_modify_range(sensor->ctrls.hblank,
+				 sensor->ctrls.hblank->minimum,
+				 max_hblank, sensor->ctrls.hblank->step,
+				 sensor->ctrls.hblank->minimum);
+	__v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, sensor->ctrls.hblank->minimum);
+
+	max_vblank = AR0521_TOTAL_HEIGHT_MAX - sensor->fmt.height;
+	__v4l2_ctrl_modify_range(sensor->ctrls.vblank,
+				 sensor->ctrls.vblank->minimum,
+				 max_vblank, sensor->ctrls.vblank->step,
+				 sensor->ctrls.vblank->minimum);
+	__v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, sensor->ctrls.vblank->minimum);
+
+	exposure_max = sensor->fmt.height + AR0521_HEIGHT_BLANKING_MIN - 4;
+	exposure_val = min(sensor->ctrls.exposure->val, exposure_max);
+	__v4l2_ctrl_modify_range(sensor->ctrls.exposure,
+				 sensor->ctrls.exposure->minimum,
+				 exposure_max, sensor->ctrls.exposure->step,
+				 exposure_val);
+	__v4l2_ctrl_s_ctrl(sensor->ctrls.exposure, exposure_val);
+
 	mutex_unlock(&sensor->lock);
-	return ret;
+
+	return 0;
 }
 
 static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
 	struct ar0521_dev *sensor = to_ar0521_dev(sd);
+	int exp_max;
+	int exp_val;
 	int ret;
 
 	/* v4l2_ctrl_lock() locks our own mutex */
 
 	switch (ctrl->id) {
-	case V4L2_CID_HBLANK:
 	case V4L2_CID_VBLANK:
-		sensor->total_width = sensor->fmt.width +
-			sensor->ctrls.hblank->val;
-		sensor->total_height = sensor->fmt.width +
-			sensor->ctrls.vblank->val;
-		break;
-	default:
-		ret = -EINVAL;
+		exp_max = sensor->fmt.height + ctrl->val - 4;
+		exp_val = min(sensor->ctrls.exposure->val, exp_max);
+		__v4l2_ctrl_modify_range(sensor->ctrls.exposure,
+					 sensor->ctrls.exposure->minimum,
+					 exp_max, sensor->ctrls.exposure->step,
+					 exp_val);
 		break;
 	}
 
@@ -584,6 +615,7 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
 	const struct v4l2_ctrl_ops *ops = &ar0521_ctrl_ops;
 	struct ar0521_ctrls *ctrls = &sensor->ctrls;
 	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
+	int max_vblank, max_hblank, exposure_max;
 	int ret;
 
 	v4l2_ctrl_handler_init(hdl, 32);
@@ -604,11 +636,17 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
 						-512, 511, 1, 0);
 	v4l2_ctrl_cluster(3, &ctrls->gain);
 
+	/* Initialize blanking limits using the default 2592x1944 format. */
+	max_hblank = AR0521_TOTAL_WIDTH_MAX - AR0521_WIDTH_MAX;
 	ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
-					  AR0521_WIDTH_BLANKING_MIN, 4094, 1,
+					  AR0521_WIDTH_BLANKING_MIN,
+					  max_hblank, 1,
 					  AR0521_WIDTH_BLANKING_MIN);
+
+	max_vblank = AR0521_TOTAL_HEIGHT_MAX - AR0521_HEIGHT_MAX;
 	ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
-					  AR0521_HEIGHT_BLANKING_MIN, 4094, 2,
+					  AR0521_HEIGHT_BLANKING_MIN,
+					  max_vblank, 2,
 					  AR0521_HEIGHT_BLANKING_MIN);
 	v4l2_ctrl_cluster(2, &ctrls->hblank);
 
@@ -618,9 +656,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
 					   AR0521_PIXEL_CLOCK_MAX, 1,
 					   AR0521_PIXEL_CLOCK_RATE);
 
-	/* Manual exposure time */
+	/* Manual exposure time: max exposure time = visible + blank - 4 */
+	exposure_max = AR0521_HEIGHT_MAX + AR0521_HEIGHT_BLANKING_MIN - 4;
 	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0,
-					    65535, 1, 360);
+					    exposure_max, 1, 360);
 
 	v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
 			       ARRAY_SIZE(ar0521_link_frequencies) - 1,
-- 
2.37.3

[PATCH v2 08/10] media: ar0521: Setup controls at s_stream time

[Jacopo Mondi]

Setup all the registered controls at s_stream(1) time instead of
manually configure gains.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ar0521.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index 854f4ccd9c3d..cf2d2746657e 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -399,7 +399,7 @@ static int ar0521_set_stream(struct ar0521_dev *sensor, bool on)
 		if (ret)
 			goto err;
 
-		ret = ar0521_set_gains(sensor);
+		ret =  __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
 		if (ret)
 			goto err;
 
-- 
2.37.3

[PATCH v2 09/10] media: ar0521: Rework startup sequence

[Jacopo Mondi]

The ar0521_write_mode() function explicitly programs the exposure time
register and the test pattern register, which are now setup by the call
to __v4l2_ctrl_handler_setup() in ar0521_set_stream().

Removing those register writes from ar0521_write_mode() reduces the
function to two operations: geometry configuration and pll
configuration.

Move those operations in the ar0521_set_stream() caller and remove
ar0521_write_mode(). However maintain the ar0521_calc_pll() function
separated as it is used during pad format configuration to update the
PIXEL_RATE control value.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ar0521.c | 50 ++++++++++++--------------------------
 1 file changed, 16 insertions(+), 34 deletions(-)

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index cf2d2746657e..5560dc3d6605 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -278,7 +278,7 @@ static u32 calc_pll(struct ar0521_dev *sensor, u32 freq, u16 *pre_ptr, u16 *mult
 	return pll;
 }
 
-static void ar0521_calc_mode(struct ar0521_dev *sensor)
+static void ar0521_calc_pll(struct ar0521_dev *sensor)
 {
 	unsigned int pixel_clock;
 	u16 pre, mult;
@@ -342,7 +342,7 @@ static void ar0521_calc_mode(struct ar0521_dev *sensor)
 	sensor->pll.mult = sensor->pll.mult2 = mult;
 }
 
-static int ar0521_write_mode(struct ar0521_dev *sensor)
+static int ar0521_pll_config(struct ar0521_dev *sensor)
 {
 	__be16 pll_regs[] = {
 		be(AR0521_REG_VT_PIX_CLK_DIV),
@@ -353,36 +353,9 @@ static int ar0521_write_mode(struct ar0521_dev *sensor)
 		/* 0x308 */ be(sensor->pll.vt_pix * 2), /* op_pix_clk_div = 2 * vt_pix_clk_div */
 		/* 0x30A */ be(1)  /* op_sys_clk_div */
 	};
-	int ret;
-
-	/* Stop streaming for just a moment */
-	ret = ar0521_write_reg(sensor, AR0521_REG_RESET,
-			       AR0521_REG_RESET_DEFAULTS);
-	if (ret)
-		return ret;
-
-	ret = ar0521_set_geometry(sensor);
-	if (ret)
-		return ret;
-
-	ret = ar0521_write_regs(sensor, pll_regs, ARRAY_SIZE(pll_regs));
-	if (ret)
-		return ret;
-
-	ret = ar0521_write_reg(sensor, AR0521_REG_COARSE_INTEGRATION_TIME,
-			       sensor->ctrls.exposure->val);
-	if (ret)
-		return ret;
-
-	ret = ar0521_write_reg(sensor, AR0521_REG_RESET,
-			       AR0521_REG_RESET_DEFAULTS |
-			       AR0521_REG_RESET_STREAM);
-	if (ret)
-		return ret;
 
-	ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE,
-			       sensor->ctrls.test_pattern->val);
-	return ret;
+	ar0521_calc_pll(sensor);
+	return ar0521_write_regs(sensor, pll_regs, ARRAY_SIZE(pll_regs));
 }
 
 static int ar0521_set_stream(struct ar0521_dev *sensor, bool on)
@@ -394,8 +367,17 @@ static int ar0521_set_stream(struct ar0521_dev *sensor, bool on)
 		if (ret < 0)
 			return ret;
 
-		ar0521_calc_mode(sensor);
-		ret = ar0521_write_mode(sensor);
+		/* Stop streaming for just a moment */
+		ret = ar0521_write_reg(sensor, AR0521_REG_RESET,
+				       AR0521_REG_RESET_DEFAULTS);
+		if (ret)
+			return ret;
+
+		ret = ar0521_set_geometry(sensor);
+		if (ret)
+			return ret;
+
+		ret = ar0521_pll_config(sensor);
 		if (ret)
 			goto err;
 
@@ -501,7 +483,7 @@ static int ar0521_set_fmt(struct v4l2_subdev *sd,
 	}
 
 	sensor->fmt = format->format;
-	ar0521_calc_mode(sensor);
+	ar0521_calc_pll(sensor);
 
 	/*
 	 * Update the exposure and blankings limits. Blankings are also reset
-- 
2.37.3

[PATCH v2 10/10] media: ar0521: Tab-align definitions

[Jacopo Mondi]

Align some register and constant definitions using tab in place of
mixed tab+spaces.

Cosmetic change only.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ar0521.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index 5560dc3d6605..608d0795e98e 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -14,17 +14,17 @@
 #include <media/v4l2-subdev.h>
 
 /* External clock (extclk) frequencies */
-#define AR0521_EXTCLK_MIN	  (10 * 1000 * 1000)
-#define AR0521_EXTCLK_MAX	  (48 * 1000 * 1000)
+#define AR0521_EXTCLK_MIN		(10 * 1000 * 1000)
+#define AR0521_EXTCLK_MAX		(48 * 1000 * 1000)
 
 /* PLL and PLL2 */
-#define AR0521_PLL_MIN		 (320 * 1000 * 1000)
-#define AR0521_PLL_MAX		(1280 * 1000 * 1000)
+#define AR0521_PLL_MIN			(320 * 1000 * 1000)
+#define AR0521_PLL_MAX			(1280 * 1000 * 1000)
 
 /* Effective pixel sample rate on the pixel array. */
-#define AR0521_PIXEL_CLOCK_RATE	 (184 * 1000 * 1000)
-#define AR0521_PIXEL_CLOCK_MIN	 (168 * 1000 * 1000)
-#define AR0521_PIXEL_CLOCK_MAX	 (414 * 1000 * 1000)
+#define AR0521_PIXEL_CLOCK_RATE		(184 * 1000 * 1000)
+#define AR0521_PIXEL_CLOCK_MIN		(168 * 1000 * 1000)
+#define AR0521_PIXEL_CLOCK_MAX		(414 * 1000 * 1000)
 
 #define AR0521_NATIVE_WIDTH		2604u
 #define AR0521_NATIVE_HEIGHT		1964u
@@ -33,15 +33,15 @@
 #define AR0521_MAX_X_ADDR_END		2603u
 #define AR0521_MAX_Y_ADDR_END		1955u
 
-#define AR0521_WIDTH_MIN	       8u
-#define AR0521_WIDTH_MAX	    2592u
-#define AR0521_HEIGHT_MIN	       8u
-#define AR0521_HEIGHT_MAX	    1944u
+#define AR0521_WIDTH_MIN		8u
+#define AR0521_WIDTH_MAX		2592u
+#define AR0521_HEIGHT_MIN		8u
+#define AR0521_HEIGHT_MAX		1944u
 
-#define AR0521_WIDTH_BLANKING_MIN     572u
-#define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
-#define AR0521_TOTAL_HEIGHT_MAX     65535u /* max_frame_lenght_lines */
-#define AR0521_TOTAL_WIDTH_MAX      65532u /* max_line_lenght_pck */
+#define AR0521_WIDTH_BLANKING_MIN	572u
+#define AR0521_HEIGHT_BLANKING_MIN	38u /* must be even */
+#define AR0521_TOTAL_HEIGHT_MAX		65535u /* max_frame_lenght_lines */
+#define AR0521_TOTAL_WIDTH_MAX		65532u /* max_line_lenght_pck */
 
 #define AR0521_ANA_GAIN_MIN		0x00
 #define AR0521_ANA_GAIN_MAX		0x3f
-- 
2.37.3

Re: [PATCH v2 02/10] media: ar0521: Add V4L2_CID_ANALOG_GAIN

[Dave Stevenson]

Hi Jacopo

On Sat, 22 Oct 2022 at 11:47, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Add support for V4L2_CID_ANALOG_GAIN. The control programs the global
> gain register which applies to all color channels.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ar0521.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> index 0daa61df2603..ba169f0218a9 100644
> --- a/drivers/media/i2c/ar0521.c
> +++ b/drivers/media/i2c/ar0521.c
> @@ -35,6 +35,11 @@
>  #define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
>  #define AR0521_TOTAL_WIDTH_MIN      2968u
>
> +#define AR0521_ANA_GAIN_MIN            0x00
> +#define AR0521_ANA_GAIN_MAX            0x3f

The register reference I have says it is u3.4 format, which would make
the max value 0x7f rather than 0x3f.

Functionally it makes no real difference, but a max gain of nearly x7
15/16ths  is preferable to nearly x3 15/16ths.

If it is u3.4 I'd have expected the minimum to be 0x10 to avoid a gain
of less than x1 (does it even work?)

Looking at the listed m0, c0, m1, c1 values for gain (1, 0, 0, and 4
respectively), that maps to a formula:
gain = code / 4
Min and max codes are 0 and 0x1f, so that implies it will do a gain of
less than x1, and goes up to x7.75.

So much contradictory information!!

I'm happy to add a R-B tag for the code side, but the limits seem to
be a little all over the place. I'd be looking at taking some test
images with fixed exposure time at each gain code to work out what
value is actually x1, x2, x4, etc. Doing so does require knowing the
black level and applying an appropriate correction to the raw values,
or extrapolating from the results.

  Dave

> +#define AR0521_ANA_GAIN_STEP           0x01
> +#define AR0521_ANA_GAIN_DEFAULT                0x00
> +
>  /* AR0521 registers */
>  #define AR0521_REG_VT_PIX_CLK_DIV              0x0300
>  #define AR0521_REG_FRAME_LENGTH_LINES          0x0340
> @@ -50,6 +55,8 @@
>  #define   AR0521_REG_RESET_RESTART               BIT(1)
>  #define   AR0521_REG_RESET_INIT                          BIT(0)
>
> +#define AR0521_REG_ANA_GAIN_CODE_GLOBAL                0x3028
> +
>  #define AR0521_REG_GREEN1_GAIN                 0x3056
>  #define AR0521_REG_BLUE_GAIN                   0x3058
>  #define AR0521_REG_RED_GAIN                    0x305A
> @@ -456,6 +463,10 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
>         case V4L2_CID_VBLANK:
>                 ret = ar0521_set_geometry(sensor);
>                 break;
> +       case V4L2_CID_ANALOGUE_GAIN:
> +               ret = ar0521_write_reg(sensor, AR0521_REG_ANA_GAIN_CODE_GLOBAL,
> +                                      ctrl->val);
> +               break;
>         case V4L2_CID_GAIN:
>         case V4L2_CID_RED_BALANCE:
>         case V4L2_CID_BLUE_BALANCE:
> @@ -499,6 +510,11 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
>         /* We can use our own mutex for the ctrl lock */
>         hdl->lock = &sensor->lock;
>
> +       /* Analog gain */
> +       v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> +                         AR0521_ANA_GAIN_MIN, AR0521_ANA_GAIN_MAX,
> +                         AR0521_ANA_GAIN_STEP, AR0521_ANA_GAIN_DEFAULT);
> +
>         /* Manual gain */
>         ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 511, 1, 0);
>         ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_RED_BALANCE,
> --
> 2.37.3
>

Re: [PATCH v2 02/10] media: ar0521: Add V4L2_CID_ANALOG_GAIN

[Jacopo Mondi]

Hi Dave,

On Mon, Oct 24, 2022 at 01:13:58PM +0100, Dave Stevenson wrote:
> Hi Jacopo
>
> On Sat, 22 Oct 2022 at 11:47, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Add support for V4L2_CID_ANALOG_GAIN. The control programs the global
> > gain register which applies to all color channels.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/i2c/ar0521.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> > index 0daa61df2603..ba169f0218a9 100644
> > --- a/drivers/media/i2c/ar0521.c
> > +++ b/drivers/media/i2c/ar0521.c
> > @@ -35,6 +35,11 @@
> >  #define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
> >  #define AR0521_TOTAL_WIDTH_MIN      2968u
> >
> > +#define AR0521_ANA_GAIN_MIN            0x00
> > +#define AR0521_ANA_GAIN_MAX            0x3f
>
> The register reference I have says it is u3.4 format, which would make
> the max value 0x7f rather than 0x3f.
>
> Functionally it makes no real difference, but a max gain of nearly x7
> 15/16ths  is preferable to nearly x3 15/16ths.
>
> If it is u3.4 I'd have expected the minimum to be 0x10 to avoid a gain
> of less than x1 (does it even work?)
>

The value of the 0x3280 register is written in the 7 low bits of the
0x305e register. Whatever is written to 0x305e is similalrly reflected
in the 0x3280 one. The 0x305e[6:4] bits are the coarse analog gain
value and the lower 4 bits are the fine analog gain value. I wonder if
u3.4 is used there to describe the that, even if u3.4 has a different
meaning

> Looking at the listed m0, c0, m1, c1 values for gain (1, 0, 0, and 4
> respectively), that maps to a formula:
> gain = code / 4
> Min and max codes are 0 and 0x1f, so that implies it will do a gain of
> less than x1, and goes up to x7.75.

The sensor does not use the CCS gain model, but an exponential
piecewise function documented as a table of register values/gains in
the application manual.

The sensor exposes a list of CCS-compatible registers, including
"0x0204 analogue_gain_code_global", from which I presume one should
contorl the gain if the CCS compatible model works. From my tests,
writing to that register is a no-op.

I presume the CCS compatible interface is not plumbed, or maybe it
depdends on the FW version, or else :)

>
> So much contradictory information!!
>

Yes :)

> I'm happy to add a R-B tag for the code side, but the limits seem to
> be a little all over the place. I'd be looking at taking some test
> images with fixed exposure time at each gain code to work out what
> value is actually x1, x2, x4, etc. Doing so does require knowing the
> black level and applying an appropriate correction to the raw values,
> or extrapolating from the results.
>
>   Dave
>
> > +#define AR0521_ANA_GAIN_STEP           0x01
> > +#define AR0521_ANA_GAIN_DEFAULT                0x00
> > +
> >  /* AR0521 registers */
> >  #define AR0521_REG_VT_PIX_CLK_DIV              0x0300
> >  #define AR0521_REG_FRAME_LENGTH_LINES          0x0340
> > @@ -50,6 +55,8 @@
> >  #define   AR0521_REG_RESET_RESTART               BIT(1)
> >  #define   AR0521_REG_RESET_INIT                          BIT(0)
> >
> > +#define AR0521_REG_ANA_GAIN_CODE_GLOBAL                0x3028
> > +
> >  #define AR0521_REG_GREEN1_GAIN                 0x3056
> >  #define AR0521_REG_BLUE_GAIN                   0x3058
> >  #define AR0521_REG_RED_GAIN                    0x305A
> > @@ -456,6 +463,10 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
> >         case V4L2_CID_VBLANK:
> >                 ret = ar0521_set_geometry(sensor);
> >                 break;
> > +       case V4L2_CID_ANALOGUE_GAIN:
> > +               ret = ar0521_write_reg(sensor, AR0521_REG_ANA_GAIN_CODE_GLOBAL,
> > +                                      ctrl->val);
> > +               break;
> >         case V4L2_CID_GAIN:
> >         case V4L2_CID_RED_BALANCE:
> >         case V4L2_CID_BLUE_BALANCE:
> > @@ -499,6 +510,11 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
> >         /* We can use our own mutex for the ctrl lock */
> >         hdl->lock = &sensor->lock;
> >
> > +       /* Analog gain */
> > +       v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> > +                         AR0521_ANA_GAIN_MIN, AR0521_ANA_GAIN_MAX,
> > +                         AR0521_ANA_GAIN_STEP, AR0521_ANA_GAIN_DEFAULT);
> > +
> >         /* Manual gain */
> >         ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 511, 1, 0);
> >         ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_RED_BALANCE,
> > --
> > 2.37.3
> >

Re: [PATCH v2 05/10] media: ar0521: Refuse unsupported controls

[Dave Stevenson]

Hi Jacopo

On Sat, 22 Oct 2022 at 11:13, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Refuse unsupported controls by returning -EINVAL in the s_ctrl
> operation.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>
> ---
>  drivers/media/i2c/ar0521.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> index e673424880ac..fcd852760750 100644
> --- a/drivers/media/i2c/ar0521.c
> +++ b/drivers/media/i2c/ar0521.c
> @@ -547,6 +547,10 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
>                 ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE,
>                                        ctrl->val);
>                 break;
> +       default:
> +               dev_err(&sensor->i2c_client->dev,
> +                       "Unsupported control %x\n", ctrl->id);
> +               return -EINVAL;
>         }
>
>         pm_runtime_put(&sensor->i2c_client->dev);

In the default case you've returned without doing the pm_runtime_put,
so pm is going to be unbalanced.

default:
  dev_err(...)
  ret = -EINVAL;
  break;
would avoid that.

  Dave

> --
> 2.37.3
>

Re: [PATCH v2 05/10] media: ar0521: Refuse unsupported controls

[Jacopo Mondi]

Ups, trivial mistake sorry

Thanks for spotting

On Mon, Oct 24, 2022 at 02:00:28PM +0100, Dave Stevenson wrote:
> Hi Jacopo
>
> On Sat, 22 Oct 2022 at 11:13, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Refuse unsupported controls by returning -EINVAL in the s_ctrl
> > operation.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > ---
> >  drivers/media/i2c/ar0521.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> > index e673424880ac..fcd852760750 100644
> > --- a/drivers/media/i2c/ar0521.c
> > +++ b/drivers/media/i2c/ar0521.c
> > @@ -547,6 +547,10 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
> >                 ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE,
> >                                        ctrl->val);
> >                 break;
> > +       default:
> > +               dev_err(&sensor->i2c_client->dev,
> > +                       "Unsupported control %x\n", ctrl->id);
> > +               return -EINVAL;
> >         }
> >
> >         pm_runtime_put(&sensor->i2c_client->dev);
>
> In the default case you've returned without doing the pm_runtime_put,
> so pm is going to be unbalanced.
>
> default:
>   dev_err(...)
>   ret = -EINVAL;
>   break;
> would avoid that.
>
>   Dave
>
> > --
> > 2.37.3
> >

Re: [PATCH v2 06/10] media: ar0521: Add LINK_FREQ control

[Dave Stevenson]

Hi Jacopo

On Sat, 22 Oct 2022 at 11:38, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Add support for V4L2_CID_LINK_FREQ which currently reports a single
> hard-coded frequency which depends on the fixed pixel clock.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ar0521.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> index fcd852760750..2310346f11d5 100644
> --- a/drivers/media/i2c/ar0521.c
> +++ b/drivers/media/i2c/ar0521.c
> @@ -89,6 +89,10 @@ static const char * const ar0521_supply_names[] = {
>         "vaa",          /* Analog (2.7V) supply */
>  };
>
> +static const s64 ar0521_link_frequencies[] = {
> +       184000000,
> +};
> +
>  struct ar0521_ctrls {
>         struct v4l2_ctrl_handler handler;
>         struct {
> @@ -547,6 +551,13 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
>                 ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE,
>                                        ctrl->val);
>                 break;
> +       case V4L2_CID_LINK_FREQ:
> +               /*
> +                * Link frequency index is used at PLL configuration time,
> +                * nothing to do here.
> +                */
> +               ret = 0;
> +               break;

Defining as read only removes the need for this, but it depends on
whether you expect to have userspace changing it in the future.

As it stands it works, so:

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

>         default:
>                 dev_err(&sensor->i2c_client->dev,
>                         "Unsupported control %x\n", ctrl->id);
> @@ -611,6 +622,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
>         ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0,
>                                             65535, 1, 360);
>
> +       v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
> +                              ARRAY_SIZE(ar0521_link_frequencies) - 1,
> +                              0, ar0521_link_frequencies);
> +
>         ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops,
>                                         V4L2_CID_TEST_PATTERN,
>                                         ARRAY_SIZE(test_pattern_menu) - 1,
> --
> 2.37.3
>

Re: [PATCH v2 07/10] media: ar0521: Adjust exposure and blankings limits

[Dave Stevenson]

Hi Jacopo

On Sat, 22 Oct 2022 at 11:20, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Adjust the control limits for V4L2_CID_VBLANK, V4L2_CID_HBLANK and
> V4L2_CID_EXPOSURE when a new format is applied to the sensor.
>
> Also update the exposure control when a new blanking value is applied.
>
> Also change the controls initialization to use valid values for the
> default format.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ar0521.c | 79 ++++++++++++++++++++++++++++----------
>  1 file changed, 59 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> index 2310346f11d5..854f4ccd9c3d 100644
> --- a/drivers/media/i2c/ar0521.c
> +++ b/drivers/media/i2c/ar0521.c
> @@ -40,7 +40,8 @@
>
>  #define AR0521_WIDTH_BLANKING_MIN     572u
>  #define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
> -#define AR0521_TOTAL_WIDTH_MIN      2968u
> +#define AR0521_TOTAL_HEIGHT_MAX     65535u /* max_frame_lenght_lines */

s/max_frame_lenght_lines/max_frame_length_lines

> +#define AR0521_TOTAL_WIDTH_MAX      65532u /* max_line_lenght_pck */

s/max_line_lenght_pck /max_line_length_pck

>
>  #define AR0521_ANA_GAIN_MIN            0x00
>  #define AR0521_ANA_GAIN_MAX            0x3f
> @@ -125,8 +126,6 @@ struct ar0521_dev {
>         struct v4l2_mbus_framefmt fmt;
>         struct ar0521_ctrls ctrls;
>         unsigned int lane_count;
> -       u16 total_width;
> -       u16 total_height;
>         struct {
>                 u16 pre;
>                 u16 mult;
> @@ -483,7 +482,8 @@ static int ar0521_set_fmt(struct v4l2_subdev *sd,
>                           struct v4l2_subdev_format *format)
>  {
>         struct ar0521_dev *sensor = to_ar0521_dev(sd);
> -       int ret = 0;
> +       int exposure_max, exposure_val;
> +       int max_vblank, max_hblank;
>
>         ar0521_adj_fmt(&format->format);
>
> @@ -494,33 +494,64 @@ static int ar0521_set_fmt(struct v4l2_subdev *sd,
>
>                 fmt = v4l2_subdev_get_try_format(sd, sd_state, 0 /* pad */);
>                 *fmt = format->format;
> -       } else {
> -               sensor->fmt = format->format;
> -               ar0521_calc_mode(sensor);
> +
> +               mutex_unlock(&sensor->lock);
> +
> +               return 0;
>         }
>
> +       sensor->fmt = format->format;
> +       ar0521_calc_mode(sensor);
> +
> +       /*
> +        * Update the exposure and blankings limits. Blankings are also reset
> +        * to the minimum.
> +        */
> +       max_hblank = AR0521_TOTAL_WIDTH_MAX - sensor->fmt.width;
> +       __v4l2_ctrl_modify_range(sensor->ctrls.hblank,
> +                                sensor->ctrls.hblank->minimum,
> +                                max_hblank, sensor->ctrls.hblank->step,
> +                                sensor->ctrls.hblank->minimum);

__v4l2_ctrl_modify_range does return an error code. Is there any value
in checking it?
The only time I've really seen it return an error is when the code is
messed up and the default provided is out of range, but that would be
a driver bug and not something caused by userspace.

It looks like the rest of the code is correct, but I haven't had time
to follow it completely. I'll try and do that tomorrow.

  Dave

> +       __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, sensor->ctrls.hblank->minimum);
> +
> +       max_vblank = AR0521_TOTAL_HEIGHT_MAX - sensor->fmt.height;
> +       __v4l2_ctrl_modify_range(sensor->ctrls.vblank,
> +                                sensor->ctrls.vblank->minimum,
> +                                max_vblank, sensor->ctrls.vblank->step,
> +                                sensor->ctrls.vblank->minimum);
> +       __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, sensor->ctrls.vblank->minimum);
> +
> +       exposure_max = sensor->fmt.height + AR0521_HEIGHT_BLANKING_MIN - 4;
> +       exposure_val = min(sensor->ctrls.exposure->val, exposure_max);
> +       __v4l2_ctrl_modify_range(sensor->ctrls.exposure,
> +                                sensor->ctrls.exposure->minimum,
> +                                exposure_max, sensor->ctrls.exposure->step,
> +                                exposure_val);
> +       __v4l2_ctrl_s_ctrl(sensor->ctrls.exposure, exposure_val);
> +
>         mutex_unlock(&sensor->lock);
> -       return ret;
> +
> +       return 0;
>  }
>
>  static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>         struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
>         struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +       int exp_max;
> +       int exp_val;
>         int ret;
>
>         /* v4l2_ctrl_lock() locks our own mutex */
>
>         switch (ctrl->id) {
> -       case V4L2_CID_HBLANK:
>         case V4L2_CID_VBLANK:
> -               sensor->total_width = sensor->fmt.width +
> -                       sensor->ctrls.hblank->val;
> -               sensor->total_height = sensor->fmt.width +
> -                       sensor->ctrls.vblank->val;
> -               break;
> -       default:
> -               ret = -EINVAL;
> +               exp_max = sensor->fmt.height + ctrl->val - 4;
> +               exp_val = min(sensor->ctrls.exposure->val, exp_max);
> +               __v4l2_ctrl_modify_range(sensor->ctrls.exposure,
> +                                        sensor->ctrls.exposure->minimum,
> +                                        exp_max, sensor->ctrls.exposure->step,
> +                                        exp_val);
>                 break;
>         }
>
> @@ -584,6 +615,7 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
>         const struct v4l2_ctrl_ops *ops = &ar0521_ctrl_ops;
>         struct ar0521_ctrls *ctrls = &sensor->ctrls;
>         struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> +       int max_vblank, max_hblank, exposure_max;
>         int ret;
>
>         v4l2_ctrl_handler_init(hdl, 32);
> @@ -604,11 +636,17 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
>                                                 -512, 511, 1, 0);
>         v4l2_ctrl_cluster(3, &ctrls->gain);
>
> +       /* Initialize blanking limits using the default 2592x1944 format. */
> +       max_hblank = AR0521_TOTAL_WIDTH_MAX - AR0521_WIDTH_MAX;
>         ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
> -                                         AR0521_WIDTH_BLANKING_MIN, 4094, 1,
> +                                         AR0521_WIDTH_BLANKING_MIN,
> +                                         max_hblank, 1,
>                                           AR0521_WIDTH_BLANKING_MIN);
> +
> +       max_vblank = AR0521_TOTAL_HEIGHT_MAX - AR0521_HEIGHT_MAX;
>         ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
> -                                         AR0521_HEIGHT_BLANKING_MIN, 4094, 2,
> +                                         AR0521_HEIGHT_BLANKING_MIN,
> +                                         max_vblank, 2,
>                                           AR0521_HEIGHT_BLANKING_MIN);
>         v4l2_ctrl_cluster(2, &ctrls->hblank);
>
> @@ -618,9 +656,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
>                                            AR0521_PIXEL_CLOCK_MAX, 1,
>                                            AR0521_PIXEL_CLOCK_RATE);
>
> -       /* Manual exposure time */
> +       /* Manual exposure time: max exposure time = visible + blank - 4 */
> +       exposure_max = AR0521_HEIGHT_MAX + AR0521_HEIGHT_BLANKING_MIN - 4;
>         ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0,
> -                                           65535, 1, 360);
> +                                           exposure_max, 1, 360);
>
>         v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
>                                ARRAY_SIZE(ar0521_link_frequencies) - 1,
> --
> 2.37.3
>

Re: [PATCH v2 02/10] media: ar0521: Add V4L2_CID_ANALOG_GAIN

[Sakari Ailus]

Hi Jacopo,

On Mon, Oct 24, 2022 at 02:31:05PM +0200, Jacopo Mondi wrote:
> Hi Dave,
> 
> On Mon, Oct 24, 2022 at 01:13:58PM +0100, Dave Stevenson wrote:
> > Hi Jacopo
> >
> > On Sat, 22 Oct 2022 at 11:47, Jacopo Mondi <jacopo@jmondi.org> wrote:
> > >
> > > Add support for V4L2_CID_ANALOG_GAIN. The control programs the global
> > > gain register which applies to all color channels.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  drivers/media/i2c/ar0521.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> > > index 0daa61df2603..ba169f0218a9 100644
> > > --- a/drivers/media/i2c/ar0521.c
> > > +++ b/drivers/media/i2c/ar0521.c
> > > @@ -35,6 +35,11 @@
> > >  #define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
> > >  #define AR0521_TOTAL_WIDTH_MIN      2968u
> > >
> > > +#define AR0521_ANA_GAIN_MIN            0x00
> > > +#define AR0521_ANA_GAIN_MAX            0x3f
> >
> > The register reference I have says it is u3.4 format, which would make
> > the max value 0x7f rather than 0x3f.
> >
> > Functionally it makes no real difference, but a max gain of nearly x7
> > 15/16ths  is preferable to nearly x3 15/16ths.
> >
> > If it is u3.4 I'd have expected the minimum to be 0x10 to avoid a gain
> > of less than x1 (does it even work?)
> >
> 
> The value of the 0x3280 register is written in the 7 low bits of the
> 0x305e register. Whatever is written to 0x305e is similalrly reflected
> in the 0x3280 one. The 0x305e[6:4] bits are the coarse analog gain
> value and the lower 4 bits are the fine analog gain value. I wonder if
> u3.4 is used there to describe the that, even if u3.4 has a different
> meaning
> 
> > Looking at the listed m0, c0, m1, c1 values for gain (1, 0, 0, and 4
> > respectively), that maps to a formula:
> > gain = code / 4
> > Min and max codes are 0 and 0x1f, so that implies it will do a gain of
> > less than x1, and goes up to x7.75.
> 
> The sensor does not use the CCS gain model, but an exponential
> piecewise function documented as a table of register values/gains in
> the application manual.
> 
> The sensor exposes a list of CCS-compatible registers, including
> "0x0204 analogue_gain_code_global", from which I presume one should
> contorl the gain if the CCS compatible model works. From my tests,
> writing to that register is a no-op.
> 
> I presume the CCS compatible interface is not plumbed, or maybe it
> depdends on the FW version, or else :)

It is also possible this is a sensor developed based on an earlier one that
was compatible with e.g. SMIA. If the newer CCS-only registers aren't
defined, it is unlikely it would be compatible.

-- 
Regards,

Sakari Ailus

Ps. this would be a good time to get patches towards upstream. :-)

Re: [PATCH v2 07/10] media: ar0521: Adjust exposure and blankings limits

[Dave Stevenson]

Hi Jacopo

Sorry for the two part review and delay.

On Mon, 24 Oct 2022 at 18:47, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Jacopo
>
> On Sat, 22 Oct 2022 at 11:20, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Adjust the control limits for V4L2_CID_VBLANK, V4L2_CID_HBLANK and
> > V4L2_CID_EXPOSURE when a new format is applied to the sensor.
> >
> > Also update the exposure control when a new blanking value is applied.
> >
> > Also change the controls initialization to use valid values for the
> > default format.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/i2c/ar0521.c | 79 ++++++++++++++++++++++++++++----------
> >  1 file changed, 59 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> > index 2310346f11d5..854f4ccd9c3d 100644
> > --- a/drivers/media/i2c/ar0521.c
> > +++ b/drivers/media/i2c/ar0521.c
> > @@ -40,7 +40,8 @@
> >
> >  #define AR0521_WIDTH_BLANKING_MIN     572u
> >  #define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
> > -#define AR0521_TOTAL_WIDTH_MIN      2968u
> > +#define AR0521_TOTAL_HEIGHT_MAX     65535u /* max_frame_lenght_lines */
>
> s/max_frame_lenght_lines/max_frame_length_lines
>
> > +#define AR0521_TOTAL_WIDTH_MAX      65532u /* max_line_lenght_pck */
>
> s/max_line_lenght_pck /max_line_length_pck
>
> >
> >  #define AR0521_ANA_GAIN_MIN            0x00
> >  #define AR0521_ANA_GAIN_MAX            0x3f
> > @@ -125,8 +126,6 @@ struct ar0521_dev {
> >         struct v4l2_mbus_framefmt fmt;
> >         struct ar0521_ctrls ctrls;
> >         unsigned int lane_count;
> > -       u16 total_width;
> > -       u16 total_height;
> >         struct {
> >                 u16 pre;
> >                 u16 mult;
> > @@ -483,7 +482,8 @@ static int ar0521_set_fmt(struct v4l2_subdev *sd,
> >                           struct v4l2_subdev_format *format)
> >  {
> >         struct ar0521_dev *sensor = to_ar0521_dev(sd);
> > -       int ret = 0;
> > +       int exposure_max, exposure_val;
> > +       int max_vblank, max_hblank;
> >
> >         ar0521_adj_fmt(&format->format);
> >
> > @@ -494,33 +494,64 @@ static int ar0521_set_fmt(struct v4l2_subdev *sd,
> >
> >                 fmt = v4l2_subdev_get_try_format(sd, sd_state, 0 /* pad */);
> >                 *fmt = format->format;
> > -       } else {
> > -               sensor->fmt = format->format;
> > -               ar0521_calc_mode(sensor);
> > +
> > +               mutex_unlock(&sensor->lock);
> > +
> > +               return 0;
> >         }
> >
> > +       sensor->fmt = format->format;
> > +       ar0521_calc_mode(sensor);
> > +
> > +       /*
> > +        * Update the exposure and blankings limits. Blankings are also reset
> > +        * to the minimum.
> > +        */
> > +       max_hblank = AR0521_TOTAL_WIDTH_MAX - sensor->fmt.width;
> > +       __v4l2_ctrl_modify_range(sensor->ctrls.hblank,
> > +                                sensor->ctrls.hblank->minimum,
> > +                                max_hblank, sensor->ctrls.hblank->step,
> > +                                sensor->ctrls.hblank->minimum);
>
> __v4l2_ctrl_modify_range does return an error code. Is there any value
> in checking it?
> The only time I've really seen it return an error is when the code is
> messed up and the default provided is out of range, but that would be
> a driver bug and not something caused by userspace.
>
> It looks like the rest of the code is correct, but I haven't had time
> to follow it completely. I'll try and do that tomorrow.
>
>   Dave
>
> > +       __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, sensor->ctrls.hblank->minimum);
> > +
> > +       max_vblank = AR0521_TOTAL_HEIGHT_MAX - sensor->fmt.height;
> > +       __v4l2_ctrl_modify_range(sensor->ctrls.vblank,
> > +                                sensor->ctrls.vblank->minimum,
> > +                                max_vblank, sensor->ctrls.vblank->step,
> > +                                sensor->ctrls.vblank->minimum);
> > +       __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, sensor->ctrls.vblank->minimum);
> > +
> > +       exposure_max = sensor->fmt.height + AR0521_HEIGHT_BLANKING_MIN - 4;
> > +       exposure_val = min(sensor->ctrls.exposure->val, exposure_max);
> > +       __v4l2_ctrl_modify_range(sensor->ctrls.exposure,
> > +                                sensor->ctrls.exposure->minimum,
> > +                                exposure_max, sensor->ctrls.exposure->step,
> > +                                exposure_val);
> > +       __v4l2_ctrl_s_ctrl(sensor->ctrls.exposure, exposure_val);

I've stared at this and tried to work it through several times over,
and I can't convince myself the behaviour over the default value for
this control is correct.

At start of day you create the control with a default value of 360.
Magic number picked from somewhere.

When the mode is changed, the default is changed to be the current
value (not the current default) clipped by the max permitted based on
height & VBLANK.
Why should the default change based on the current value? If 360 was
the default at start of day, shouldn't it continue to be the default
(assuming it is in range)?

__v4l2_ctrl_modify_range would reset the current value to the default
if the new maximum limit is lower than current, so that bit is almost
handled for you.

It's only the def value passed in to __v4l2_ctrl_modify_range that I
think is my only issue.

> > +
> >         mutex_unlock(&sensor->lock);
> > -       return ret;
> > +
> > +       return 0;
> >  }
> >
> >  static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
> >  {
> >         struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> >         struct ar0521_dev *sensor = to_ar0521_dev(sd);
> > +       int exp_max;
> > +       int exp_val;
> >         int ret;
> >
> >         /* v4l2_ctrl_lock() locks our own mutex */
> >
> >         switch (ctrl->id) {
> > -       case V4L2_CID_HBLANK:
> >         case V4L2_CID_VBLANK:
> > -               sensor->total_width = sensor->fmt.width +
> > -                       sensor->ctrls.hblank->val;
> > -               sensor->total_height = sensor->fmt.width +
> > -                       sensor->ctrls.vblank->val;
> > -               break;
> > -       default:
> > -               ret = -EINVAL;
> > +               exp_max = sensor->fmt.height + ctrl->val - 4;
> > +               exp_val = min(sensor->ctrls.exposure->val, exp_max);
> > +               __v4l2_ctrl_modify_range(sensor->ctrls.exposure,
> > +                                        sensor->ctrls.exposure->minimum,
> > +                                        exp_max, sensor->ctrls.exposure->step,
> > +                                        exp_val);

Same here. Should the advertised default value for exposure change
based on VBLANK setting?

  Dave

> >                 break;
> >         }
> >
> > @@ -584,6 +615,7 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
> >         const struct v4l2_ctrl_ops *ops = &ar0521_ctrl_ops;
> >         struct ar0521_ctrls *ctrls = &sensor->ctrls;
> >         struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> > +       int max_vblank, max_hblank, exposure_max;
> >         int ret;
> >
> >         v4l2_ctrl_handler_init(hdl, 32);
> > @@ -604,11 +636,17 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
> >                                                 -512, 511, 1, 0);
> >         v4l2_ctrl_cluster(3, &ctrls->gain);
> >
> > +       /* Initialize blanking limits using the default 2592x1944 format. */
> > +       max_hblank = AR0521_TOTAL_WIDTH_MAX - AR0521_WIDTH_MAX;
> >         ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
> > -                                         AR0521_WIDTH_BLANKING_MIN, 4094, 1,
> > +                                         AR0521_WIDTH_BLANKING_MIN,
> > +                                         max_hblank, 1,
> >                                           AR0521_WIDTH_BLANKING_MIN);
> > +
> > +       max_vblank = AR0521_TOTAL_HEIGHT_MAX - AR0521_HEIGHT_MAX;
> >         ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
> > -                                         AR0521_HEIGHT_BLANKING_MIN, 4094, 2,
> > +                                         AR0521_HEIGHT_BLANKING_MIN,
> > +                                         max_vblank, 2,
> >                                           AR0521_HEIGHT_BLANKING_MIN);
> >         v4l2_ctrl_cluster(2, &ctrls->hblank);
> >
> > @@ -618,9 +656,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
> >                                            AR0521_PIXEL_CLOCK_MAX, 1,
> >                                            AR0521_PIXEL_CLOCK_RATE);
> >
> > -       /* Manual exposure time */
> > +       /* Manual exposure time: max exposure time = visible + blank - 4 */
> > +       exposure_max = AR0521_HEIGHT_MAX + AR0521_HEIGHT_BLANKING_MIN - 4;
> >         ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0,
> > -                                           65535, 1, 360);
> > +                                           exposure_max, 1, 360);
> >
> >         v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
> >                                ARRAY_SIZE(ar0521_link_frequencies) - 1,
> > --
> > 2.37.3
> >

Re: [PATCH v2 07/10] media: ar0521: Adjust exposure and blankings limits

[Jacopo Mondi]

Hi Dave

On Tue, Oct 25, 2022 at 06:45:41PM +0100, Dave Stevenson wrote:
> Hi Jacopo
>
> Sorry for the two part review and delay.
>
> On Mon, 24 Oct 2022 at 18:47, Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi Jacopo
> >
> > On Sat, 22 Oct 2022 at 11:20, Jacopo Mondi <jacopo@jmondi.org> wrote:
> > >
> > > Adjust the control limits for V4L2_CID_VBLANK, V4L2_CID_HBLANK and
> > > V4L2_CID_EXPOSURE when a new format is applied to the sensor.
> > >
> > > Also update the exposure control when a new blanking value is applied.
> > >
> > > Also change the controls initialization to use valid values for the
> > > default format.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  drivers/media/i2c/ar0521.c | 79 ++++++++++++++++++++++++++++----------
> > >  1 file changed, 59 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> > > index 2310346f11d5..854f4ccd9c3d 100644
> > > --- a/drivers/media/i2c/ar0521.c
> > > +++ b/drivers/media/i2c/ar0521.c
> > > @@ -40,7 +40,8 @@
> > >
> > >  #define AR0521_WIDTH_BLANKING_MIN     572u
> > >  #define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
> > > -#define AR0521_TOTAL_WIDTH_MIN      2968u
> > > +#define AR0521_TOTAL_HEIGHT_MAX     65535u /* max_frame_lenght_lines */
> >
> > s/max_frame_lenght_lines/max_frame_length_lines
> >
> > > +#define AR0521_TOTAL_WIDTH_MAX      65532u /* max_line_lenght_pck */
> >
> > s/max_line_lenght_pck /max_line_length_pck
> >
> > >
> > >  #define AR0521_ANA_GAIN_MIN            0x00
> > >  #define AR0521_ANA_GAIN_MAX            0x3f
> > > @@ -125,8 +126,6 @@ struct ar0521_dev {
> > >         struct v4l2_mbus_framefmt fmt;
> > >         struct ar0521_ctrls ctrls;
> > >         unsigned int lane_count;
> > > -       u16 total_width;
> > > -       u16 total_height;
> > >         struct {
> > >                 u16 pre;
> > >                 u16 mult;
> > > @@ -483,7 +482,8 @@ static int ar0521_set_fmt(struct v4l2_subdev *sd,
> > >                           struct v4l2_subdev_format *format)
> > >  {
> > >         struct ar0521_dev *sensor = to_ar0521_dev(sd);
> > > -       int ret = 0;
> > > +       int exposure_max, exposure_val;
> > > +       int max_vblank, max_hblank;
> > >
> > >         ar0521_adj_fmt(&format->format);
> > >
> > > @@ -494,33 +494,64 @@ static int ar0521_set_fmt(struct v4l2_subdev *sd,
> > >
> > >                 fmt = v4l2_subdev_get_try_format(sd, sd_state, 0 /* pad */);
> > >                 *fmt = format->format;
> > > -       } else {
> > > -               sensor->fmt = format->format;
> > > -               ar0521_calc_mode(sensor);
> > > +
> > > +               mutex_unlock(&sensor->lock);
> > > +
> > > +               return 0;
> > >         }
> > >
> > > +       sensor->fmt = format->format;
> > > +       ar0521_calc_mode(sensor);
> > > +
> > > +       /*
> > > +        * Update the exposure and blankings limits. Blankings are also reset
> > > +        * to the minimum.
> > > +        */
> > > +       max_hblank = AR0521_TOTAL_WIDTH_MAX - sensor->fmt.width;
> > > +       __v4l2_ctrl_modify_range(sensor->ctrls.hblank,
> > > +                                sensor->ctrls.hblank->minimum,
> > > +                                max_hblank, sensor->ctrls.hblank->step,
> > > +                                sensor->ctrls.hblank->minimum);
> >
> > __v4l2_ctrl_modify_range does return an error code. Is there any value
> > in checking it?
> > The only time I've really seen it return an error is when the code is
> > messed up and the default provided is out of range, but that would be
> > a driver bug and not something caused by userspace.
> >
> > It looks like the rest of the code is correct, but I haven't had time
> > to follow it completely. I'll try and do that tomorrow.
> >
> >   Dave
> >
> > > +       __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, sensor->ctrls.hblank->minimum);
> > > +
> > > +       max_vblank = AR0521_TOTAL_HEIGHT_MAX - sensor->fmt.height;
> > > +       __v4l2_ctrl_modify_range(sensor->ctrls.vblank,
> > > +                                sensor->ctrls.vblank->minimum,
> > > +                                max_vblank, sensor->ctrls.vblank->step,
> > > +                                sensor->ctrls.vblank->minimum);
> > > +       __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, sensor->ctrls.vblank->minimum);
> > > +
> > > +       exposure_max = sensor->fmt.height + AR0521_HEIGHT_BLANKING_MIN - 4;
> > > +       exposure_val = min(sensor->ctrls.exposure->val, exposure_max);
> > > +       __v4l2_ctrl_modify_range(sensor->ctrls.exposure,
> > > +                                sensor->ctrls.exposure->minimum,
> > > +                                exposure_max, sensor->ctrls.exposure->step,
> > > +                                exposure_val);
> > > +       __v4l2_ctrl_s_ctrl(sensor->ctrls.exposure, exposure_val);
>
> I've stared at this and tried to work it through several times over,
> and I can't convince myself the behaviour over the default value for
> this control is correct.
>
> At start of day you create the control with a default value of 360.
> Magic number picked from somewhere.

360 was there already, and I agree it would make more sense to use the
register default value of 0x70

>
> When the mode is changed, the default is changed to be the current
> value (not the current default) clipped by the max permitted based on
> height & VBLANK.
> Why should the default change based on the current value? If 360 was
> the default at start of day, shouldn't it continue to be the default
> (assuming it is in range)?
>
> __v4l2_ctrl_modify_range would reset the current value to the default
> if the new maximum limit is lower than current, so that bit is almost
> handled for you.
>
> It's only the def value passed in to __v4l2_ctrl_modify_range that I
> think is my only issue.
>

I agree the default doesn't need to be changed. To be honest, I'm not
even sure how the default value should be interpreted for userspace,
but if we there report the register defualt value I agree it should
not be changed when a new mode is applied.

Thanks
  j

> > > +
> > >         mutex_unlock(&sensor->lock);
> > > -       return ret;
> > > +
> > > +       return 0;
> > >  }
> > >
> > >  static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
> > >  {
> > >         struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> > >         struct ar0521_dev *sensor = to_ar0521_dev(sd);
> > > +       int exp_max;
> > > +       int exp_val;
> > >         int ret;
> > >
> > >         /* v4l2_ctrl_lock() locks our own mutex */
> > >
> > >         switch (ctrl->id) {
> > > -       case V4L2_CID_HBLANK:
> > >         case V4L2_CID_VBLANK:
> > > -               sensor->total_width = sensor->fmt.width +
> > > -                       sensor->ctrls.hblank->val;
> > > -               sensor->total_height = sensor->fmt.width +
> > > -                       sensor->ctrls.vblank->val;
> > > -               break;
> > > -       default:
> > > -               ret = -EINVAL;
> > > +               exp_max = sensor->fmt.height + ctrl->val - 4;
> > > +               exp_val = min(sensor->ctrls.exposure->val, exp_max);
> > > +               __v4l2_ctrl_modify_range(sensor->ctrls.exposure,
> > > +                                        sensor->ctrls.exposure->minimum,
> > > +                                        exp_max, sensor->ctrls.exposure->step,
> > > +                                        exp_val);
>
> Same here. Should the advertised default value for exposure change
> based on VBLANK setting?
>
>   Dave
>
> > >                 break;
> > >         }
> > >
> > > @@ -584,6 +615,7 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
> > >         const struct v4l2_ctrl_ops *ops = &ar0521_ctrl_ops;
> > >         struct ar0521_ctrls *ctrls = &sensor->ctrls;
> > >         struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> > > +       int max_vblank, max_hblank, exposure_max;
> > >         int ret;
> > >
> > >         v4l2_ctrl_handler_init(hdl, 32);
> > > @@ -604,11 +636,17 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
> > >                                                 -512, 511, 1, 0);
> > >         v4l2_ctrl_cluster(3, &ctrls->gain);
> > >
> > > +       /* Initialize blanking limits using the default 2592x1944 format. */
> > > +       max_hblank = AR0521_TOTAL_WIDTH_MAX - AR0521_WIDTH_MAX;
> > >         ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
> > > -                                         AR0521_WIDTH_BLANKING_MIN, 4094, 1,
> > > +                                         AR0521_WIDTH_BLANKING_MIN,
> > > +                                         max_hblank, 1,
> > >                                           AR0521_WIDTH_BLANKING_MIN);
> > > +
> > > +       max_vblank = AR0521_TOTAL_HEIGHT_MAX - AR0521_HEIGHT_MAX;
> > >         ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
> > > -                                         AR0521_HEIGHT_BLANKING_MIN, 4094, 2,
> > > +                                         AR0521_HEIGHT_BLANKING_MIN,
> > > +                                         max_vblank, 2,
> > >                                           AR0521_HEIGHT_BLANKING_MIN);
> > >         v4l2_ctrl_cluster(2, &ctrls->hblank);
> > >
> > > @@ -618,9 +656,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
> > >                                            AR0521_PIXEL_CLOCK_MAX, 1,
> > >                                            AR0521_PIXEL_CLOCK_RATE);
> > >
> > > -       /* Manual exposure time */
> > > +       /* Manual exposure time: max exposure time = visible + blank - 4 */
> > > +       exposure_max = AR0521_HEIGHT_MAX + AR0521_HEIGHT_BLANKING_MIN - 4;
> > >         ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0,
> > > -                                           65535, 1, 360);
> > > +                                           exposure_max, 1, 360);
> > >
> > >         v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
> > >                                ARRAY_SIZE(ar0521_link_frequencies) - 1,
> > > --
> > > 2.37.3
> > >