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

[PATCH v3 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

v3 (Dave)
- Check __v4l2_ctrl_modify_range() return value
- Make LINK_FREQ readonly to avoid additional churn in s_ctrl
- Fix trivial early return in s_ctrl
- Use exposure's default value when modifying the controls' limits
- Change the exposure default to 0x70 to match the register default value

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

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 | 352 ++++++++++++++++++++++++++-----------
 1 file changed, 250 insertions(+), 102 deletions(-)

--
2.38.1

[PATCH v3 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 e408049f6312..45fcf3798ad2 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -799,6 +799,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);
@@ -865,6 +883,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.38.1

[PATCH v3 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 45fcf3798ad2..20a87dde2289 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
@@ -455,6 +462,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:
@@ -498,6 +509,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.38.1

[PATCH v3 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 20a87dde2289..bcbd6ffd8832 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.38.1

[PATCH v3 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 bcbd6ffd8832..d30f7a1c7651 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.38.1

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

[Jacopo Mondi]

Refuse unsupported controls by returning -EINVAL in the s_ctrl
operation. While at it, remove a the default switch case in the first
switch as it effectively is now a no-op.

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

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index d30f7a1c7651..acb9509d7708 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -514,9 +514,6 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
 		sensor->total_height = sensor->fmt.width +
 			sensor->ctrls.vblank->val;
 		break;
-	default:
-		ret = -EINVAL;
-		break;
 	}
 
 	/* access the sensor only if it's powered up */
@@ -546,6 +543,11 @@ 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);
+		ret = -EINVAL;
+		break;
 	}
 
 	pm_runtime_put(&sensor->i2c_client->dev);
-- 
2.38.1

[PATCH v3 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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index acb9509d7708..c9c578b223ef 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 {
@@ -570,6 +574,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;
+	struct v4l2_ctrl *link_freq;
 	int ret;
 
 	v4l2_ctrl_handler_init(hdl, 32);
@@ -608,6 +613,12 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
 	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0,
 					    65535, 1, 360);
 
+	link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
+					ARRAY_SIZE(ar0521_link_frequencies) - 1,
+					0, ar0521_link_frequencies);
+	if (link_freq)
+		link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
 	ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops,
 					V4L2_CID_TEST_PATTERN,
 					ARRAY_SIZE(test_pattern_menu) - 1,
-- 
2.38.1

[PATCH v3 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.

Update the exposure control limits when a new blanking value is
applied and change the controls initialization to use valid values for the
default format.

The exposure control default value is changed to report the default
value of register 0x3012.

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

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index c9c578b223ef..0ef4acac1bd3 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_length_lines */
+#define AR0521_TOTAL_WIDTH_MAX      65532u /* 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,6 +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 max_vblank, max_hblank, exposure_max;
+	int ret;
 
 	ar0521_adj_fmt(&format->format);
 
@@ -493,30 +494,73 @@ 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;
+	ret = __v4l2_ctrl_modify_range(sensor->ctrls.hblank,
+				       sensor->ctrls.hblank->minimum,
+				       max_hblank, sensor->ctrls.hblank->step,
+				       sensor->ctrls.hblank->minimum);
+	if (ret)
+		goto unlock;
+
+	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.hblank,
+				 sensor->ctrls.hblank->minimum);
+	if (ret)
+		goto unlock;
+
+	max_vblank = AR0521_TOTAL_HEIGHT_MAX - sensor->fmt.height;
+	ret = __v4l2_ctrl_modify_range(sensor->ctrls.vblank,
+				       sensor->ctrls.vblank->minimum,
+				       max_vblank, sensor->ctrls.vblank->step,
+				       sensor->ctrls.vblank->minimum);
+	if (ret)
+		goto unlock;
+
+	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank,
+				 sensor->ctrls.vblank->minimum);
+	if (ret)
+		goto unlock;
+
+	exposure_max = sensor->fmt.height + AR0521_HEIGHT_BLANKING_MIN - 4;
+	ret = __v4l2_ctrl_modify_range(sensor->ctrls.exposure,
+				       sensor->ctrls.exposure->minimum,
+				       exposure_max,
+				       sensor->ctrls.exposure->step,
+				       sensor->ctrls.exposure->default_value);
+unlock:
 	mutex_unlock(&sensor->lock);
-	return 0;
+
+	return ret;
 }
 
 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 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;
+		exp_max = sensor->fmt.height + ctrl->val - 4;
+		__v4l2_ctrl_modify_range(sensor->ctrls.exposure,
+					 sensor->ctrls.exposure->minimum,
+					 exp_max, sensor->ctrls.exposure->step,
+					 sensor->ctrls.exposure->default_value);
 		break;
 	}
 
@@ -574,6 +618,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;
 	struct v4l2_ctrl *link_freq;
 	int ret;
 
@@ -595,11 +640,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);
 
@@ -609,9 +660,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, 0x70);
 
 	link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
 					ARRAY_SIZE(ar0521_link_frequencies) - 1,
-- 
2.38.1

[PATCH v3 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 0ef4acac1bd3..323f583e2029 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.38.1

[PATCH v3 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 geomerty configuration in the ar0521_set_stream() caller and rename
ar0521_write_mode() to ar0521_pll_config().

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 323f583e2029..91fa4cba12f6 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.38.1

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

[Jacopo Mondi]

I just noticed patch 10/10 was missing.

Now sent in reply to this thread.

While at it, gentle ping to see if this can make it for 6.2 ?

Thanks
  j

On Fri, Nov 04, 2022 at 03:24:43PM +0100, Jacopo Mondi wrote:
> 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
>
> v3 (Dave)
> - Check __v4l2_ctrl_modify_range() return value
> - Make LINK_FREQ readonly to avoid additional churn in s_ctrl
> - Fix trivial early return in s_ctrl
> - Use exposure's default value when modifying the controls' limits
> - Change the exposure default to 0x70 to match the register default value
>
> 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
>
> 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 | 352 ++++++++++++++++++++++++++-----------
>  1 file changed, 250 insertions(+), 102 deletions(-)
>
> --
> 2.38.1
>

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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Nov 21, 2022 at 06:49:33PM +0100, Jacopo Mondi wrote:
> I just noticed patch 10/10 was missing.
> 
> Now sent in reply to this thread.
> 
> While at it, gentle ping to see if this can make it for 6.2 ?

I'm drowning in reviews, so you'll need another volunteer.

> On Fri, Nov 04, 2022 at 03:24:43PM +0100, Jacopo Mondi wrote:
> > 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
> >
> > v3 (Dave)
> > - Check __v4l2_ctrl_modify_range() return value
> > - Make LINK_FREQ readonly to avoid additional churn in s_ctrl
> > - Fix trivial early return in s_ctrl
> > - Use exposure's default value when modifying the controls' limits
> > - Change the exposure default to 0x70 to match the register default value
> >
> > 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
> >
> > 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 | 352 ++++++++++++++++++++++++++-----------
> >  1 file changed, 250 insertions(+), 102 deletions(-)
> >

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent, Jacopo,

On Tue, Nov 22, 2022 at 01:38:41AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Mon, Nov 21, 2022 at 06:49:33PM +0100, Jacopo Mondi wrote:
> > I just noticed patch 10/10 was missing.
> > 
> > Now sent in reply to this thread.
> > 
> > While at it, gentle ping to see if this can make it for 6.2 ?
> 
> I'm drowning in reviews, so you'll need another volunteer.

I rather thought the set is good as there are no comments for almost three
weeks. It's in my tree now, I expect to send a PR soon...

-- 
Regards,

Sakari Ailus

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

[Krzysztof Hałasa]

Hi Jacopo and Co.,

Jacopo Mondi <jacopo@jmondi.org> writes:

> I just noticed patch 10/10 was missing.
>
> Now sent in reply to this thread.
>
> While at it, gentle ping to see if this can make it for 6.2 ?

I understand this has been tested on actual hw. I'd like to check how
does it work in my setup as well, but it is currently out of question.
This will eventually happen, though. For now I guess I can
Acked-by: Krzysztof Hałasa <khalasa@piap.pl>
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa