[PATCH v5] Driver for ON Semi AR0521 camera sensor

[PATCH v5] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

The driver has been extensively tested in an i.MX6-based system.

Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
---
Changes from v4:
- removed struct v4l2_fwnode_endpoint from struct ar0521_dev
  (leaving only MIPI lane_count)
- converted power_count to pm_runtime_*()
- removed extra debug/formatting
- (get|set)_*() don't check for pad# (using pad 0 only)
- suspend/resume support
- removed static int debug, now using dev_dbg() only (+dynamic debug)
- trivial changes

diff --git a/MAINTAINERS b/MAINTAINERS
index ee91c5472bc1..29115fdc5d4a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1358,6 +1358,12 @@ S:	Supported
 W:	http://www.aquantia.com
 F:	drivers/net/ethernet/aquantia/atlantic/aq_ptp*
 
+AR0521 ON SEMICONDUCTOR CAMERA SENSOR DRIVER
+M:	Krzysztof Hałasa <khalasa@piap.pl>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+F:	drivers/media/i2c/ar0521.c
+
 ARASAN NAND CONTROLLER DRIVER
 M:	Miquel Raynal <miquel.raynal@bootlin.com>
 M:	Naga Sureshkumar Relli <nagasure@xilinx.com>
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index adb348aa8396..af031c45a5a4 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -730,6 +730,16 @@ config VIDEO_APTINA_PLL
 config VIDEO_CCS_PLL
 	tristate
 
+config VIDEO_AR0521
+	tristate "ON Semiconductor AR0521 sensor support"
+	depends on I2C && VIDEO_V4L2
+	help
+	  This is a Video4Linux2 sensor driver for the ON Semiconductor
+	  AR0521 camera.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ar0521.
+
 config VIDEO_HI556
 	tristate "Hynix Hi-556 sensor support"
 	depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 5ac8d639e5ca..3163f9a081ac 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -117,6 +117,7 @@ obj-$(CONFIG_VIDEO_I2C)		+= video-i2c.o
 obj-$(CONFIG_VIDEO_ML86V7667)	+= ml86v7667.o
 obj-$(CONFIG_VIDEO_OV2659)	+= ov2659.o
 obj-$(CONFIG_VIDEO_TC358743)	+= tc358743.o
+obj-$(CONFIG_VIDEO_AR0521)	+= ar0521.o
 obj-$(CONFIG_VIDEO_HI556)	+= hi556.o
 obj-$(CONFIG_VIDEO_IMX208)	+= imx208.o
 obj-$(CONFIG_VIDEO_IMX214)	+= imx214.o
diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
new file mode 100644
index 000000000000..27a4c362de52
--- /dev/null
+++ b/drivers/media/i2c/ar0521.c
@@ -0,0 +1,1047 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021 Sieć Badawcza Łukasiewicz - Przemysłowy Instytut Automatyki i Pomiarów PIAP
+ * Written by Krzysztof Hałasa
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/ctype.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+// External clock (extclk) frequencies
+#define AR0521_EXTCLK_RATE	  (27 * 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)
+
+// effective pixel clocks, the registers may be DDR
+#define AR0521_PIXEL_CLOCK_MIN	 (168 * 1000 * 1000)
+#define AR0521_PIXEL_CLOCK_MAX	 (414 * 1000 * 1000)
+
+#define AR0521_WIDTH_MIN	       8u
+#define AR0521_WIDTH_MAX	    2608u
+#define AR0521_HEIGHT_MIN	       8u
+#define AR0521_HEIGHT_MAX	    1958u
+
+#define AR0521_WIDTH_BLANKING_MIN     572u
+#define AR0521_HEIGHT_BLANKING_MIN     28u // must be even
+#define AR0521_TOTAL_WIDTH_MIN	     2968u
+
+// AR0521 registers
+#define AR0521_REG_VT_PIX_CLK_DIV		0x0300
+#define AR0521_REG_FRAME_LENGTH_LINES		0x0340
+
+#define AR0521_REG_CHIP_ID			0x3000
+#define AR0521_REG_COARSE_INTEGRATION_TIME	0x3012
+#define AR0521_REG_ROW_SPEED			0x3016
+#define AR0521_REG_EXTRA_DELAY			0x3018
+#define AR0521_REG_RESET			0x301A
+#define   AR0521_REG_RESET_DEFAULTS		  0x0238
+#define   AR0521_REG_RESET_GROUP_PARAM_HOLD	  0x8000
+#define   AR0521_REG_RESET_STREAM		  BIT(2)
+#define   AR0521_REG_RESET_RESTART		  BIT(1)
+#define   AR0521_REG_RESET_INIT			  BIT(0)
+
+#define AR0521_REG_GREEN1_GAIN			0x3056
+#define AR0521_REG_BLUE_GAIN			0x3058
+#define AR0521_REG_RED_GAIN			0x305A
+#define AR0521_REG_GREEN2_GAIN			0x305C
+#define AR0521_REG_GLOBAL_GAIN			0x305E
+
+#define AR0521_REG_HISPI_TEST_MODE		0x3066
+#define AR0521_REG_HISPI_TEST_MODE_LP11		  0x0004
+
+#define AR0521_REG_TEST_PATTERN_MODE		0x3070
+
+#define AR0521_REG_SERIAL_FORMAT		0x31AE
+#define AR0521_REG_SERIAL_FORMAT_MIPI		  0x0200
+
+#define AR0521_REG_HISPI_CONTROL_STATUS		0x31C6
+#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80
+
+#define be		cpu_to_be16
+
+static const char * const ar0521_supply_names[] = {
+	"vdd_io",	// I/O (1.8V) supply
+	"vdd",		// Core, PLL and MIPI (1.2V) supply
+	"vaa",		// Analog (2.7V) supply
+};
+
+#define AR0521_NUM_SUPPLIES ARRAY_SIZE(ar0521_supply_names)
+
+struct ar0521_ctrls {
+	struct v4l2_ctrl_handler handler;
+	struct v4l2_ctrl *exposure;
+	struct v4l2_ctrl *gain, *red_balance, *blue_balance;
+	struct v4l2_ctrl *test_pattern;
+	struct v4l2_ctrl *hblank, *vblank, *pixrate;
+};
+
+struct ar0521_dev {
+	struct i2c_client *i2c_client;
+	struct v4l2_subdev sd;
+	struct media_pad pad;
+	struct clk *extclk;
+	u32 extclk_freq;
+
+	struct regulator *supplies[AR0521_NUM_SUPPLIES];
+	struct gpio_desc *reset_gpio;
+
+	// lock to protect all members below
+	struct mutex lock;
+
+	struct v4l2_mbus_framefmt fmt;
+	struct v4l2_fract frame_interval, current_frame_interval;
+	struct ar0521_ctrls ctrls;
+	u32 pix_clk;
+	unsigned int lane_count;
+	u16 total_width, total_height, pll_pre, pll_mult, pll_pre2, pll_mult2, extra_delay;
+	bool streaming;
+};
+
+static inline struct ar0521_dev *to_ar0521_dev(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct ar0521_dev, sd);
+}
+
+static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
+{
+	return &container_of(ctrl->handler, struct ar0521_dev, ctrls.handler)->sd;
+}
+
+static u32 div64_round(u64 v, u32 d)
+{
+	return div_u64(v + (d >> 1), d);
+}
+
+static u32 div64_round_up(u64 v, u32 d)
+{
+	return div_u64(v + d - 1, d);
+}
+
+// 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)
+{
+	struct i2c_client *client = sensor->i2c_client;
+	struct i2c_msg msg;
+	int ret;
+
+	if (!pm_runtime_get_if_in_use(&client->dev))
+		return 0;
+
+	msg.addr = client->addr;
+	msg.flags = client->flags;
+	msg.buf = (u8 *)data;
+	msg.len = count * sizeof(*data);
+
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	pm_runtime_put(&client->dev);
+
+	if (ret < 0) {
+		v4l2_err(&sensor->sd, "%s: I2C write error\n", __func__);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ar0521_write_reg(struct ar0521_dev *sensor, u16 reg, u16 val)
+{
+	__be16 buf[2] = {be(reg), be(val)};
+
+	return ar0521_write_regs(sensor, buf, 2);
+}
+
+static int ar0521_set_geometry(struct ar0521_dev *sensor)
+{
+	// 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(x),
+		be(y),
+		be(x + sensor->fmt.width - 1),
+		be(y + sensor->fmt.height - 1),
+		be(sensor->fmt.width),
+		be(sensor->fmt.height)
+	};
+
+	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
+
+	return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
+}
+
+static int ar0521_set_gains(struct ar0521_dev *sensor)
+{
+	int green = sensor->ctrls.gain->val;
+	int red = max(green + sensor->ctrls.red_balance->val, 0);
+	int blue = max(green + sensor->ctrls.blue_balance->val, 0);
+	unsigned int gain = min(red, min(green, blue));
+	unsigned int analog = min(gain, 64u); // range is 0 - 127
+	__be16 regs[5];
+
+	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
+
+	red   = min(red   - analog + 64, 511u);
+	green = min(green - analog + 64, 511u);
+	blue  = min(blue  - analog + 64, 511u);
+	regs[0] = be(AR0521_REG_GREEN1_GAIN);
+	regs[1] = be(green << 7 | analog);
+	regs[2] = be(blue  << 7 | analog);
+	regs[3] = be(red   << 7 | analog);
+	regs[4] = be(green << 7 | analog);
+
+	return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
+}
+
+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
+		/* 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
+		/* 0x30A */ be(1)  // op_sys_clk_div
+	};
+	u32 num = sensor->current_frame_interval.numerator;
+	u32 denom = sensor->current_frame_interval.denominator;
+	int ret;
+
+	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
+
+	// 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_EXTRA_DELAY, sensor->extra_delay);
+	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);
+	if (ret)
+		return ret;
+
+	dev_dbg(&sensor->i2c_client->dev,
+		"AR0521: %ux%u, total %ux%u, pixel clock %u MHz, %u (%u/%u) FPS\n",
+		sensor->fmt.width, sensor->fmt.height, sensor->total_width, sensor->total_height,
+		sensor->pix_clk, (num + denom / 2) / denom, num, denom);
+	return 0;
+}
+
+static int ar0521_set_stream(struct ar0521_dev *sensor, bool on)
+{
+	int ret;
+
+	dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, on);
+
+	ret = ar0521_write_mode(sensor);
+	if (ret)
+		return ret;
+
+	if (on) {
+		ret = ar0521_set_gains(sensor);
+		if (ret)
+			return ret;
+
+		// normal output on clock and data lanes
+		ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS, 0);
+		if (ret)
+			return ret;
+	} else {
+		// reset gain, the sensor may produce all white pixels without this
+		ret = ar0521_write_reg(sensor, AR0521_REG_GLOBAL_GAIN, 0x2000);
+		if (ret)
+			return ret;
+
+		// set LP-11 on clock and data lanes
+		ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS,
+				       AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE);
+		if (ret)
+			return ret;
+	}
+
+	// start streaming (possibly with LP-11 on all lines)
+	return ar0521_write_reg(sensor, AR0521_REG_RESET,
+				AR0521_REG_RESET_DEFAULTS |
+				AR0521_REG_RESET_RESTART |
+				AR0521_REG_RESET_STREAM);
+}
+
+static u32 calc_pll(struct ar0521_dev *sensor, int num, u32 freq, u16 *pre_ptr, u16 *mult_ptr)
+{
+	u16 pre = 1, mult = 1, new_pre;
+	u32 pll = AR0521_PLL_MAX + 1;
+
+	for (new_pre = 1; new_pre < 64; new_pre++) {
+		u32 new_pll;
+		u32 new_mult = div64_round_up((u64)freq * new_pre, sensor->extclk_freq);
+
+		if (new_mult < 32)
+			continue; // minimum value
+		if (new_mult > 254)
+			break; // maximum, larger pre won't work either
+		if (sensor->extclk_freq * (u64)new_mult < AR0521_PLL_MIN * new_pre)
+			continue;
+		if (sensor->extclk_freq * (u64)new_mult > AR0521_PLL_MAX * new_pre)
+			break; // larger pre won't work either
+		new_pll = div64_round_up(sensor->extclk_freq * (u64)new_mult, new_pre);
+		if (new_pll < pll) {
+			pll = new_pll;
+			pre = new_pre;
+			mult = new_mult;
+		}
+	}
+
+	pll = div64_round(sensor->extclk_freq * (u64)mult, pre);
+	*pre_ptr = pre;
+	*mult_ptr = mult;
+	return pll;
+}
+
+static void ar0521_adj_fmt(struct v4l2_mbus_framefmt *fmt)
+{
+	fmt->width = clamp(ALIGN(fmt->width, 4), AR0521_WIDTH_MIN, AR0521_WIDTH_MAX);
+	fmt->height = clamp(ALIGN(fmt->height, 4), AR0521_HEIGHT_MIN, AR0521_HEIGHT_MAX);
+	fmt->code = MEDIA_BUS_FMT_SGRBG8_1X8;
+	fmt->field = V4L2_FIELD_NONE;
+	fmt->colorspace = V4L2_COLORSPACE_SRGB;
+	fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;
+}
+
+#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
+	u64 pix_clk; // for calculations
+	u32 pixels, num, denom, new_total_height, new_pixels;
+	u16 total_width, total_height;
+
+	total_width = max(sensor->fmt.width + AR0521_WIDTH_BLANKING_MIN, AR0521_TOTAL_WIDTH_MIN);
+	total_height = sensor->fmt.height + AR0521_HEIGHT_BLANKING_MIN;
+
+	pixels = total_width * total_height;
+	num = sensor->frame_interval.numerator;
+	denom = sensor->frame_interval.denominator;
+
+	// calculate approximate pixel clock first
+	pix_clk = div64_round_up(pixels * (u64)num, denom);
+	if (pix_clk > AR0521_PIXEL_CLOCK_MAX) {
+		u32 cnt;
+		// have to recalculate FPS
+		num = pix_clk = AR0521_PIXEL_CLOCK_MAX;
+		denom = pixels;
+		// try to reduce the numbers a bit
+		for (cnt = 2; cnt * cnt < denom; cnt++) {
+			while (num % cnt == 0 && denom % cnt == 0) {
+				num /= cnt;
+				denom /= cnt;
+			}
+		}
+	} else if (pix_clk < AR0521_PIXEL_CLOCK_MIN)
+		// we will compensate with total_height and extra_delay
+		pix_clk = AR0521_PIXEL_CLOCK_MIN;
+
+	sensor->current_frame_interval.numerator = num;
+	sensor->current_frame_interval.denominator = denom;
+
+	// 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);
+
+	// let's see if we can do better
+	new_total_height = (div64_round((u64)pix_clk * denom, num) / total_width) & ~1; // must be even
+	if (new_total_height > total_height) {
+		total_height = new_total_height;
+		pixels = total_width * total_height;
+	}
+
+	// maybe there is still room for improvement
+	new_pixels = div64_round(pix_clk * denom, num);
+	sensor->extra_delay = 0;
+	if (new_pixels > pixels)
+		sensor->extra_delay = new_pixels - pixels;
+
+	sensor->pix_clk = pix_clk;
+	sensor->total_width = total_width;
+	sensor->total_height = total_height;
+}
+
+static int ar0521_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state,
+			  struct v4l2_subdev_format *format)
+{
+	struct ar0521_dev *sensor = to_ar0521_dev(sd);
+	struct v4l2_mbus_framefmt *fmt;
+
+	dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, format->which);
+
+	mutex_lock(&sensor->lock);
+
+	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
+		fmt = v4l2_subdev_get_try_format(&sensor->sd, sd_state, 0 /* pad */);
+	else
+		fmt = &sensor->fmt;
+
+	format->format = *fmt;
+
+	mutex_unlock(&sensor->lock);
+	return 0;
+}
+
+static int ar0521_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state,
+			  struct v4l2_subdev_format *format)
+{
+	struct ar0521_dev *sensor = to_ar0521_dev(sd);
+	int ret = 0;
+
+	dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, format->which);
+
+	ar0521_adj_fmt(&format->format);
+
+	mutex_lock(&sensor->lock);
+
+	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+		struct v4l2_mbus_framefmt *fmt;
+
+		fmt = v4l2_subdev_get_try_format(sd, sd_state, 0 /* pad */);
+		*fmt = format->format;
+	} else {
+		sensor->fmt = format->format;
+		ar0521_calc_mode(sensor);
+		ret = ar0521_write_mode(sensor);
+	}
+
+	mutex_unlock(&sensor->lock);
+	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 ret;
+
+	// v4l2_ctrl_lock() locks our own mutex
+
+	dev_dbg(&sensor->i2c_client->dev, "%s(0x%X)\n", __func__, ctrl->id);
+
+	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;
+		ret = ar0521_set_geometry(sensor);
+		break;
+	case V4L2_CID_GAIN:
+	case V4L2_CID_RED_BALANCE:
+	case V4L2_CID_BLUE_BALANCE:
+		ret = ar0521_set_gains(sensor);
+		break;
+	case V4L2_CID_EXPOSURE:
+		ret = ar0521_write_reg(sensor, AR0521_REG_COARSE_INTEGRATION_TIME, ctrl->val);
+		break;
+	case V4L2_CID_TEST_PATTERN:
+		ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE, ctrl->val);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops ar0521_ctrl_ops = {
+	.s_ctrl = ar0521_s_ctrl,
+};
+
+static const char * const test_pattern_menu[] = {
+	"Disabled",
+	"Solid color",
+	"Color bars",
+	"Faded color bars"
+};
+
+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 ret;
+
+	v4l2_ctrl_handler_init(hdl, 32);
+
+	// we can use our own mutex for the ctrl lock
+	hdl->lock = &sensor->lock;
+
+	// 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, -512, 511, 1, 0);
+	ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BLUE_BALANCE, -512, 511, 1, 0);
+
+	// alternate for frame interval
+	ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, AR0521_WIDTH_BLANKING_MIN, 4094, 1, AR0521_WIDTH_BLANKING_MIN);
+	ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, AR0521_HEIGHT_BLANKING_MIN, 4094, 2, AR0521_HEIGHT_BLANKING_MIN);
+	// Read-only
+	ctrls->pixrate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE, AR0521_PIXEL_CLOCK_MIN, AR0521_PIXEL_CLOCK_MAX, 1, AR0521_PIXEL_CLOCK_MIN);
+
+	// manual exposure time
+	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0, 65535, 1, 0);
+
+	ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
+							   ARRAY_SIZE(test_pattern_menu) - 1,
+							   0, 0, test_pattern_menu);
+
+	if (hdl->error) {
+		ret = hdl->error;
+		goto free_ctrls;
+	}
+
+	sensor->sd.ctrl_handler = hdl;
+	return 0;
+
+free_ctrls:
+	v4l2_ctrl_handler_free(hdl);
+	return ret;
+}
+
+static const struct initial_reg {
+	u16 addr, value;
+} initial_regs[] = {
+	// corrections_recommended_bayer
+	{0x3042, 0x0004}, // RNC:enable b/w rnc mode
+	{0x3044, 0x4580}, // RNC:enable row noise correction
+	{0x30EE, 0x1136}, // RNC:rnc scaling factor-->initial recommended setting
+	{0x3120, 0x0001}, // recommended setting for dither
+	{0x3F2C, 0x442E}, // GTH_THRES_RTN: 7max,7min filtered out of every 46
+	{0x30D2, 0x0000}, // CRM/CC: enable crm on Visible and CC rows
+	{0x30D4, 0x0000}, // CC: CC enabled with 16 samples per column
+	{0x30D6, 0x2FFF}, // CC: bw mode enabled/12 bit data resolution/bw mode
+	{0x30DA, 0x0FFF}, // CC: column correction clip level 2 is 0
+	{0x30DC, 0x0FFF}, // CC: column correction clip level 3 is 0
+	{0x30DE, 0x0000}, // CC: Group FPN correction
+	{0x31E0, 0x0781}, // Fuse/2DDC: enable 2ddc
+	{0x3180, 0x9434}, // FDOC:fdoc settings with fdoc every frame turned of
+	{0x3172, 0x0206}, // txlo clk divider options
+	{0x3F00, 0x0017}, // BM_T0
+	{0x3F02, 0x02DD}, // BM_T1
+	{0x3F04, 0x0020}, // if Ana_gain less than 2, use noise_floor0, multipl
+	{0x3F06, 0x0040}, // if Ana_gain between 4 and 7, use noise_floor2 and
+	{0x3F08, 0x0070}, // if Ana_gain between 4 and 7, use noise_floor2 and
+	{0x3F0A, 0x0101}, // Define noise_floor0(low address) and noise_floor1
+	{0x3F0C, 0x0302}, // Define noise_floor2 and noise_floor3
+	{0x3F1E, 0x0022},
+	{0x3F1A, 0x01FF}, // cross factor 2
+	{0x3F14, 0x0505}, // single k factor 2
+	{0x3F44, 0x0707}, // couple k factor 2
+	{0x3F18, 0x01FF}, // cross factor 1
+	{0x3F12, 0x0505}, // single k factor 1
+	{0x3F42, 0x1511}, // couple k factor 1
+	{0x3F16, 0x01FF}, // cross factor 0
+	{0x3F10, 0x0505}, // single k factor 0
+	{0x3F40, 0x1511}, // couple k factor 0
+
+	// analog_setup_recommended_12bit
+	{0x3EB6, 0x004C}, // ECL
+	{0x3EBA, 0xAAAA},
+	{0x3EBC, 0x0086}, // Bias currents for FSC/ECL
+	{0x3EC0, 0x1E00}, // SFbin/SH mode settings
+	{0x3EC2, 0x100B}, // CLK divider for ramp for 12 bit 400MHz mode only
+	{0x3EC4, 0x3300}, // FSC clamps for HDR mode and adc comp power down co
+	{0x3EC6, 0xEA44}, // VLN and clk gating controls
+	{0x3EC8, 0x6F6F}, // Txl0 and Txlo1 settings for normal mode
+	{0x3ECA, 0x2F4A}, // CDAC/Txlo2/RSTGHI/RSTGLO settings
+	{0x3ECC, 0x0506}, // RSTDHI/RSTDLO/CDAC/TXHI settings
+	{0x3ECE, 0x203B}, // Ramp buffer settings and Booster enable (bits 0-5)
+	{0x3ED0, 0x13F0}, // TXLO from atest/sf bin settings
+	{0x3ED2, 0x9A3D}, // Booster settings for reference rows/columns
+	{0x3ED4, 0x862F}, // TXLO open loop/row driver settings
+	{0x3ED6, 0x4081}, // Txlatch fr cfpn rows/vln bias
+	{0x3ED8, 0x4003}, // Ramp step setting for 12 bit 400 Mhz mode
+	{0x3EDA, 0x9A80}, // ramp offset for T1/normal and rst under range
+	{0x3EDC, 0xC000}, // over range for rst and under range for sig
+	{0x3EDE, 0xC103}, // over range for sig and col dec clk settings
+	{0x3426, 0x1600}, // ADC offset distribution pulse
+	{0x342A, 0x0038}, // pulse_config
+	{0x3F3E, 0x0001}, // Switch ADC from 10 bit to 12 bit mode
+	{0x341A, 0x6051},
+	{0x3420, 0x6051},
+
+	// analog_setup_recommended_10bit
+	{0x3EC2, 0x100A}, // CLK divider for ramp for 10 bit 400MH
+	{0x3ED8, 0x8003}, // Ramp step setting for 10 bit 400 Mhz
+	{0x341A, 0x4735}, // Samp&Hold pulse in ADC
+	{0x3420, 0x4735}, // Samp&Hold pulse in ADC
+	{0x3426, 0x8A1A}, // ADC offset distribution pulse
+	{0x342A, 0x0018}, // pulse_config
+	{0x3ED2, 0xA53D}, // Ramp offset
+	{0x3EDA, 0xA580}, // Ramp Offset
+	{0x3EBA, 0xAAAD},
+	{0x3EB6, 0x004C},
+	{0x3F3E, 0x0000}, // Switch ADC from 12 bit to 10 bit mode
+
+	// new RNC 10bit
+	{0x30EE, 0x1136}, // RNC:rnc scaling factor=*54/64 (32/38*64=53.9)
+	{0x3F2C, 0x442E}, // GTH_THRES_RTN: 4max,4min filtered out of every 46 samples and
+	// for 10bit mode
+	{0x301E, 0x00AA}, // PEDESTAL+2 :+2 is a workaround for 10bit mode +0.5 Rounding
+	{0x3120, 0x0005}, // p1 dither enabled for 10bit mode
+
+	{0x0112, 0x0808}, // 8-bit/8-bit mode
+	{0x31BC, 0x068C}, // don't use continuous clock mode while shut down
+	{0x30FA, 0xFD00}, // GPIO0 = flash, GPIO1 = shutter
+	{0x31B0, 0x008B}, // frame_preamble - FIXME check WRT lanes#
+	{0x31B2, 0x0050}, // line_preamble - FIXME check WRT lanes#
+};
+
+static __be16 pixel_timing_recommended[] = {
+	be(0x3D00), // first register address
+	/* 3D00 */ be(0x043E), be(0x4760), be(0xFFFF), be(0xFFFF), be(0x8000), be(0x0510), be(0xAF08), be(0x0252),
+	/* 3D10 */ be(0x486F), be(0x5D5D), be(0x8056), be(0x8313), be(0x0087), be(0x6A48), be(0x6982), be(0x0280),
+	/* 3D20 */ be(0x8359), be(0x8D02), be(0x8020), be(0x4882), be(0x4269), be(0x6A95), be(0x5988), be(0x5A83),
+	/* 3D30 */ be(0x5885), be(0x6280), be(0x6289), be(0x6097), be(0x5782), be(0x605C), be(0xBF18), be(0x0961),
+	/* 3D40 */ be(0x5080), be(0x2090), be(0x4390), be(0x4382), be(0x5F8A), be(0x5D5D), be(0x9C63), be(0x8063),
+	/* 3D50 */ be(0xA960), be(0x9757), be(0x8260), be(0x5CFF), be(0xBF10), be(0x1681), be(0x0802), be(0x8000),
+	/* 3D60 */ be(0x141C), be(0x6000), be(0x6022), be(0x4D80), be(0x5C97), be(0x6A69), be(0xAC6F), be(0x4645),
+	/* 3D70 */ be(0x4400), be(0x0513), be(0x8069), be(0x6AC6), be(0x5F95), be(0x5F70), be(0x8040), be(0x4A81),
+	/* 3D80 */ be(0x0300), be(0xE703), be(0x0088), be(0x4A83), be(0x40FF), be(0xFFFF), be(0xFD70), be(0x8040),
+	/* 3D90 */ be(0x4A85), be(0x4FA8), be(0x4F8C), be(0x0070), be(0xBE47), be(0x8847), be(0xBC78), be(0x6B89),
+	/* 3DA0 */ be(0x6A80), be(0x6986), be(0x6B8E), be(0x6B80), be(0x6980), be(0x6A88), be(0x7C9F), be(0x866B),
+	/* 3DB0 */ be(0x8765), be(0x46FF), be(0xE365), be(0xA679), be(0x4A40), be(0x4580), be(0x44BC), be(0x7000),
+	/* 3DC0 */ be(0x8040), be(0x0802), be(0x10EF), be(0x0104), be(0x3860), be(0x5D5D), be(0x5682), be(0x1300),
+	/* 3DD0 */ be(0x8648), be(0x8202), be(0x8082), be(0x598A), be(0x0280), be(0x2048), be(0x3060), be(0x8042),
+	/* 3DE0 */ be(0x9259), be(0x865A), be(0x8258), be(0x8562), be(0x8062), be(0x8560), be(0x9257), be(0x8221),
+	/* 3DF0 */ be(0x10FF), be(0xB757), be(0x9361), be(0x1019), be(0x8020), be(0x9043), be(0x8E43), be(0x845F),
+	/* 3E00 */ be(0x835D), be(0x805D), be(0x8163), be(0x8063), be(0xA060), be(0x9157), be(0x8260), be(0x5CFF),
+	/* 3E10 */ be(0xFFFF), be(0xFFE5), be(0x1016), be(0x2048), be(0x0802), be(0x1C60), be(0x0014), be(0x0060),
+	/* 3E20 */ be(0x2205), be(0x8120), be(0x908F), be(0x6A80), be(0x6982), be(0x5F9F), be(0x6F46), be(0x4544),
+	/* 3E30 */ be(0x0005), be(0x8013), be(0x8069), be(0x6A80), be(0x7000), be(0x0000), be(0x0000), be(0x0000),
+	/* 3E40 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	/* 3E50 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	/* 3E60 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	/* 3E70 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	/* 3E80 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	/* 3E90 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	/* 3EA0 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	/* 3EB0 */ be(0x0000), be(0x0000), be(0x0000)};
+
+static void ar0521_power_off(struct ar0521_dev *sensor)
+{
+	int i;
+
+	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
+	clk_disable_unprepare(sensor->extclk);
+
+	if (sensor->reset_gpio)
+		gpiod_set_value(sensor->reset_gpio, 1); // assert RESET signal
+
+	for (i = AR0521_NUM_SUPPLIES - 1; i >= 0; i--) {
+		if (sensor->supplies[i])
+			regulator_disable(sensor->supplies[i]);
+	}
+}
+
+static int ar0521_power_on(struct ar0521_dev *sensor)
+{
+	unsigned int cnt;
+	int ret;
+
+	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
+	for (cnt = 0; cnt < AR0521_NUM_SUPPLIES; cnt++)
+		if (sensor->supplies[cnt]) {
+			ret = regulator_enable(sensor->supplies[cnt]);
+			if (ret < 0)
+				goto off;
+
+			usleep_range(1000, 1500); // min 1 ms
+		}
+
+	ret = clk_prepare_enable(sensor->extclk);
+	if (ret < 0) {
+		v4l2_err(&sensor->sd, "error enabling sensor clock\n");
+		goto off;
+	}
+	usleep_range(1000, 1500); // min 1 ms
+
+	if (sensor->reset_gpio)
+		gpiod_set_value(sensor->reset_gpio, 0); // deassert RESET signal
+	usleep_range(4500, 5000); // min 45000 clocks
+
+	for (cnt = 0; cnt < ARRAY_SIZE(initial_regs); cnt++)
+		if (ar0521_write_reg(sensor, initial_regs[cnt].addr, initial_regs[cnt].value))
+			goto off;
+
+	ret = ar0521_write_regs(sensor, pixel_timing_recommended, ARRAY_SIZE(pixel_timing_recommended));
+	if (ret)
+		goto off;
+
+	ret = ar0521_write_reg(sensor, AR0521_REG_SERIAL_FORMAT, AR0521_REG_SERIAL_FORMAT_MIPI | sensor->lane_count);
+	if (ret)
+		goto off;
+
+	// set MIPI test mode - disabled for now
+	ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_TEST_MODE,
+			       ((0x40 << sensor->lane_count) - 0x40) | AR0521_REG_HISPI_TEST_MODE_LP11);
+	if (ret)
+		goto off;
+
+	ret = ar0521_write_reg(sensor, AR0521_REG_ROW_SPEED, 0x110 | 4 / sensor->lane_count);
+	if (ret)
+		goto off;
+
+	ar0521_calc_mode(sensor);
+
+	ret = ar0521_set_stream(sensor, 0);
+	if (ret)
+		goto off;
+
+	return 0;
+off:
+	ar0521_power_off(sensor);
+	return ret;
+}
+
+static int ar0521_s_power(struct v4l2_subdev *sd, int on)
+{
+	struct ar0521_dev *sensor = to_ar0521_dev(sd);
+	struct i2c_client *client = sensor->i2c_client;
+	int ret;
+
+	dev_dbg(&client->dev, "%s(%s)\n", __func__, on ? "on" : "off");
+	mutex_lock(&sensor->lock);
+
+	if (on) {
+		ret = pm_runtime_resume_and_get(&client->dev);
+		if (ret == 0) {
+			ret = ar0521_power_on(sensor);
+			if (ret)
+				pm_runtime_put(&client->dev);
+		}
+	} else {
+		ret = pm_runtime_put(&client->dev);
+		if (ret == 0)
+			ar0521_power_off(sensor);
+	}
+
+	mutex_unlock(&sensor->lock);
+	return ret;
+}
+
+static int ar0521_enum_mbus_code(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct ar0521_dev *sensor = to_ar0521_dev(sd);
+
+	if (code->index)
+		return -EINVAL;
+
+	code->code = sensor->fmt.code;
+	dev_dbg(&sensor->i2c_client->dev, "%s() = %X\n", __func__, code->code);
+	return 0;
+}
+
+static int ar0521_g_frame_interval(struct v4l2_subdev *sd, struct v4l2_subdev_frame_interval *fi)
+{
+	struct ar0521_dev *sensor = to_ar0521_dev(sd);
+
+	mutex_lock(&sensor->lock);
+	fi->interval = sensor->current_frame_interval;
+	mutex_unlock(&sensor->lock);
+	dev_dbg(&sensor->i2c_client->dev, "%s() = %u/%u\n", __func__,
+		fi->interval.numerator, fi->interval.denominator);
+	return 0;
+}
+
+static int ar0521_s_frame_interval(struct v4l2_subdev *sd, struct v4l2_subdev_frame_interval *fi)
+{
+	struct ar0521_dev *sensor = to_ar0521_dev(sd);
+	int ret;
+
+	dev_dbg(&sensor->i2c_client->dev, "%s(%u/%u)\n", __func__,
+		fi->interval.numerator, fi->interval.denominator);
+	mutex_lock(&sensor->lock);
+
+	if (sensor->streaming) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	sensor->frame_interval = fi->interval;
+	ar0521_calc_mode(sensor);
+	ret = ar0521_write_mode(sensor);
+out:
+	mutex_unlock(&sensor->lock);
+	return ret;
+}
+
+static int ar0521_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct ar0521_dev *sensor = to_ar0521_dev(sd);
+	int ret;
+
+	dev_dbg(&sensor->i2c_client->dev, "%s(%i)\n", __func__, enable);
+	mutex_lock(&sensor->lock);
+
+	ret = ar0521_set_stream(sensor, enable);
+	if (!ret)
+		sensor->streaming = enable;
+
+	mutex_unlock(&sensor->lock);
+	return ret;
+}
+
+static const struct v4l2_subdev_core_ops ar0521_core_ops = {
+	.log_status = v4l2_ctrl_subdev_log_status,
+	.s_power = ar0521_s_power,
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_video_ops ar0521_video_ops = {
+	.g_frame_interval = ar0521_g_frame_interval,
+	.s_frame_interval = ar0521_s_frame_interval,
+	.s_stream = ar0521_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops ar0521_pad_ops = {
+	.enum_mbus_code = ar0521_enum_mbus_code,
+	.get_fmt = ar0521_get_fmt,
+	.set_fmt = ar0521_set_fmt,
+};
+
+static const struct v4l2_subdev_ops ar0521_subdev_ops = {
+	.core = &ar0521_core_ops,
+	.video = &ar0521_video_ops,
+	.pad = &ar0521_pad_ops,
+};
+
+static int __maybe_unused ar0521_suspend(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ar0521_dev *sensor = to_ar0521_dev(sd);
+
+	if (sensor->streaming)
+		ar0521_set_stream(sensor, 0);
+
+	return 0;
+}
+
+static int __maybe_unused ar0521_resume(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ar0521_dev *sensor = to_ar0521_dev(sd);
+
+	if (sensor->streaming)
+		return ar0521_set_stream(sensor, 1);
+
+	return 0;
+}
+
+static int ar0521_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	struct v4l2_fwnode_endpoint ep = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY
+	};
+	struct device *dev = &client->dev;
+	struct fwnode_handle *endpoint;
+	struct ar0521_dev *sensor;
+	unsigned int cnt;
+	int ret;
+
+	dev_dbg(dev, "%s()\n", __func__);
+	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
+	if (!sensor)
+		return -ENOMEM;
+
+	sensor->i2c_client = client;
+	sensor->fmt.code = MEDIA_BUS_FMT_SGRBG8_1X8;
+	sensor->fmt.width = AR0521_WIDTH_MAX;
+	sensor->fmt.height = AR0521_HEIGHT_MAX;
+	sensor->fmt.field = V4L2_FIELD_NONE;
+	sensor->frame_interval.numerator = 30;
+	sensor->frame_interval.denominator = 1;
+
+	endpoint = fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), NULL);
+	if (!endpoint) {
+		dev_err(dev, "endpoint node not found\n");
+		return -EINVAL;
+	}
+
+	ret = v4l2_fwnode_endpoint_parse(endpoint, &ep);
+	fwnode_handle_put(endpoint);
+	if (ret) {
+		dev_err(dev, "could not parse endpoint\n");
+		return ret;
+	}
+
+	if (ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
+		dev_err(dev, "invalid bus type, must be MIPI CSI2\n");
+		return -EINVAL;
+	}
+
+	sensor->lane_count = ep.bus.mipi_csi2.num_data_lanes;
+	switch (sensor->lane_count) {
+	case 1:
+	case 2:
+	case 4:
+		break;
+	default:
+		dev_err(dev, "invalid number of MIPI data lanes\n");
+		return -EINVAL;
+	}
+
+	// get master clock (extclk)
+	sensor->extclk = devm_clk_get(dev, "extclk");
+	if (IS_ERR(sensor->extclk)) {
+		dev_err(dev, "failed to get extclk\n");
+		return PTR_ERR(sensor->extclk);
+	}
+
+	ret = clk_set_rate(sensor->extclk, AR0521_EXTCLK_RATE);
+	if (ret < 0) {
+		dev_err(dev, "error setting clock rate\n");
+		return ret;
+	}
+
+	sensor->extclk_freq = clk_get_rate(sensor->extclk);
+
+	if (sensor->extclk_freq < AR0521_EXTCLK_MIN ||
+	    sensor->extclk_freq > AR0521_EXTCLK_MAX) {
+		dev_err(dev, "extclk frequency out of range: %u Hz\n", sensor->extclk_freq);
+		return -EINVAL;
+	}
+
+	// request optional reset pin (usually active low) and assert it
+	sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+
+	v4l2_i2c_subdev_init(&sensor->sd, client, &ar0521_subdev_ops);
+
+	sensor->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
+	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
+	sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
+	if (ret)
+		return ret;
+
+	for (cnt = 0; cnt < AR0521_NUM_SUPPLIES; cnt++) {
+		struct regulator *supply = devm_regulator_get(dev, ar0521_supply_names[cnt]);
+
+		if (IS_ERR(supply)) {
+			dev_info(dev, "no %s regulator found: %li\n", ar0521_supply_names[cnt], PTR_ERR(supply));
+			return PTR_ERR(supply);
+		}
+		sensor->supplies[cnt] = supply;
+	}
+
+	mutex_init(&sensor->lock);
+
+	ret = ar0521_init_controls(sensor);
+	if (ret)
+		goto entity_cleanup;
+
+	ar0521_adj_fmt(&sensor->fmt);
+
+	ret = v4l2_async_register_subdev(&sensor->sd);
+	if (ret)
+		goto free_ctrls;
+
+	// Enable runtime PM and turn off the device
+	pm_runtime_set_active(&client->dev);
+	pm_runtime_enable(&client->dev);
+	pm_runtime_idle(&client->dev);
+	dev_dbg(dev, "AR0521 driver initialized, master clock frequency: %u MHz, %u MIPI data lanes\n",
+		sensor->extclk_freq, sensor->lane_count);
+	return 0;
+
+free_ctrls:
+	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
+entity_cleanup:
+	media_entity_cleanup(&sensor->sd.entity);
+	mutex_destroy(&sensor->lock);
+	return ret;
+}
+
+static int ar0521_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ar0521_dev *sensor = to_ar0521_dev(sd);
+
+	v4l2_async_unregister_subdev(&sensor->sd);
+	media_entity_cleanup(&sensor->sd.entity);
+	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+	mutex_destroy(&sensor->lock);
+	return 0;
+}
+
+static const struct dev_pm_ops ar0521_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ar0521_suspend, ar0521_resume)
+};
+static const struct of_device_id ar0521_dt_ids[] = {
+	{.compatible = "onnn,ar0521"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, ar0521_dt_ids);
+
+static struct i2c_driver ar0521_i2c_driver = {
+	.driver = {
+		.name  = "ar0521",
+		.pm = &ar0521_pm_ops,
+		.of_match_table	= ar0521_dt_ids,
+	},
+	.probe    = ar0521_probe,
+	.remove   = ar0521_remove,
+};
+
+module_i2c_driver(ar0521_i2c_driver);
+
+MODULE_DESCRIPTION("AR0521 MIPI Camera subdev driver");
+MODULE_AUTHOR("Krzysztof Hałasa <khalasa@piap.pl>");
+MODULE_LICENSE("GPL v2");

-- 
Krzysztof "Chris" Hałasa

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

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Sakari Ailus]

Hi Krzysztof,

Thanks for the update.

Where's the corresponding DT binding patch? Ideally it would be part of the
same set.

On Tue, Oct 05, 2021 at 02:05:05PM +0200, Krzysztof Hałasa wrote:
> The driver has been extensively tested in an i.MX6-based system.
> 
> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
> ---
> Changes from v4:
> - removed struct v4l2_fwnode_endpoint from struct ar0521_dev
>   (leaving only MIPI lane_count)
> - converted power_count to pm_runtime_*()
> - removed extra debug/formatting
> - (get|set)_*() don't check for pad# (using pad 0 only)
> - suspend/resume support
> - removed static int debug, now using dev_dbg() only (+dynamic debug)
> - trivial changes
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ee91c5472bc1..29115fdc5d4a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1358,6 +1358,12 @@ S:	Supported
>  W:	http://www.aquantia.com
>  F:	drivers/net/ethernet/aquantia/atlantic/aq_ptp*
>  
> +AR0521 ON SEMICONDUCTOR CAMERA SENSOR DRIVER
> +M:	Krzysztof Hałasa <khalasa@piap.pl>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +F:	drivers/media/i2c/ar0521.c
> +
>  ARASAN NAND CONTROLLER DRIVER
>  M:	Miquel Raynal <miquel.raynal@bootlin.com>
>  M:	Naga Sureshkumar Relli <nagasure@xilinx.com>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index adb348aa8396..af031c45a5a4 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -730,6 +730,16 @@ config VIDEO_APTINA_PLL
>  config VIDEO_CCS_PLL
>  	tristate
>  
> +config VIDEO_AR0521
> +	tristate "ON Semiconductor AR0521 sensor support"
> +	depends on I2C && VIDEO_V4L2
> +	help
> +	  This is a Video4Linux2 sensor driver for the ON Semiconductor
> +	  AR0521 camera.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ar0521.
> +
>  config VIDEO_HI556
>  	tristate "Hynix Hi-556 sensor support"
>  	depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 5ac8d639e5ca..3163f9a081ac 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -117,6 +117,7 @@ obj-$(CONFIG_VIDEO_I2C)		+= video-i2c.o
>  obj-$(CONFIG_VIDEO_ML86V7667)	+= ml86v7667.o
>  obj-$(CONFIG_VIDEO_OV2659)	+= ov2659.o
>  obj-$(CONFIG_VIDEO_TC358743)	+= tc358743.o
> +obj-$(CONFIG_VIDEO_AR0521)	+= ar0521.o
>  obj-$(CONFIG_VIDEO_HI556)	+= hi556.o
>  obj-$(CONFIG_VIDEO_IMX208)	+= imx208.o
>  obj-$(CONFIG_VIDEO_IMX214)	+= imx214.o
> diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> new file mode 100644
> index 000000000000..27a4c362de52
> --- /dev/null
> +++ b/drivers/media/i2c/ar0521.c
> @@ -0,0 +1,1047 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Sieć Badawcza Łukasiewicz - Przemysłowy Instytut Automatyki i Pomiarów PIAP
> + * Written by Krzysztof Hałasa
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +#include <linux/ctype.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +
> +// External clock (extclk) frequencies
> +#define AR0521_EXTCLK_RATE	  (27 * 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)
> +
> +// effective pixel clocks, the registers may be DDR
> +#define AR0521_PIXEL_CLOCK_MIN	 (168 * 1000 * 1000)
> +#define AR0521_PIXEL_CLOCK_MAX	 (414 * 1000 * 1000)
> +
> +#define AR0521_WIDTH_MIN	       8u
> +#define AR0521_WIDTH_MAX	    2608u
> +#define AR0521_HEIGHT_MIN	       8u
> +#define AR0521_HEIGHT_MAX	    1958u
> +
> +#define AR0521_WIDTH_BLANKING_MIN     572u
> +#define AR0521_HEIGHT_BLANKING_MIN     28u // must be even

Please use /* */ for comments. The SPDX tag is an exception.

> +#define AR0521_TOTAL_WIDTH_MIN	     2968u
> +
> +// AR0521 registers
> +#define AR0521_REG_VT_PIX_CLK_DIV		0x0300
> +#define AR0521_REG_FRAME_LENGTH_LINES		0x0340
> +
> +#define AR0521_REG_CHIP_ID			0x3000
> +#define AR0521_REG_COARSE_INTEGRATION_TIME	0x3012
> +#define AR0521_REG_ROW_SPEED			0x3016
> +#define AR0521_REG_EXTRA_DELAY			0x3018
> +#define AR0521_REG_RESET			0x301A
> +#define   AR0521_REG_RESET_DEFAULTS		  0x0238
> +#define   AR0521_REG_RESET_GROUP_PARAM_HOLD	  0x8000
> +#define   AR0521_REG_RESET_STREAM		  BIT(2)
> +#define   AR0521_REG_RESET_RESTART		  BIT(1)
> +#define   AR0521_REG_RESET_INIT			  BIT(0)
> +
> +#define AR0521_REG_GREEN1_GAIN			0x3056
> +#define AR0521_REG_BLUE_GAIN			0x3058
> +#define AR0521_REG_RED_GAIN			0x305A
> +#define AR0521_REG_GREEN2_GAIN			0x305C
> +#define AR0521_REG_GLOBAL_GAIN			0x305E
> +
> +#define AR0521_REG_HISPI_TEST_MODE		0x3066
> +#define AR0521_REG_HISPI_TEST_MODE_LP11		  0x0004
> +
> +#define AR0521_REG_TEST_PATTERN_MODE		0x3070
> +
> +#define AR0521_REG_SERIAL_FORMAT		0x31AE
> +#define AR0521_REG_SERIAL_FORMAT_MIPI		  0x0200
> +
> +#define AR0521_REG_HISPI_CONTROL_STATUS		0x31C6
> +#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80
> +
> +#define be		cpu_to_be16
> +
> +static const char * const ar0521_supply_names[] = {
> +	"vdd_io",	// I/O (1.8V) supply
> +	"vdd",		// Core, PLL and MIPI (1.2V) supply
> +	"vaa",		// Analog (2.7V) supply
> +};
> +
> +#define AR0521_NUM_SUPPLIES ARRAY_SIZE(ar0521_supply_names)
> +
> +struct ar0521_ctrls {
> +	struct v4l2_ctrl_handler handler;
> +	struct v4l2_ctrl *exposure;
> +	struct v4l2_ctrl *gain, *red_balance, *blue_balance;
> +	struct v4l2_ctrl *test_pattern;
> +	struct v4l2_ctrl *hblank, *vblank, *pixrate;
> +};
> +
> +struct ar0521_dev {
> +	struct i2c_client *i2c_client;
> +	struct v4l2_subdev sd;
> +	struct media_pad pad;
> +	struct clk *extclk;
> +	u32 extclk_freq;
> +
> +	struct regulator *supplies[AR0521_NUM_SUPPLIES];
> +	struct gpio_desc *reset_gpio;
> +
> +	// lock to protect all members below
> +	struct mutex lock;
> +
> +	struct v4l2_mbus_framefmt fmt;
> +	struct v4l2_fract frame_interval, current_frame_interval;
> +	struct ar0521_ctrls ctrls;
> +	u32 pix_clk;
> +	unsigned int lane_count;
> +	u16 total_width, total_height, pll_pre, pll_mult, pll_pre2, pll_mult2, extra_delay;

Please wrap your lines at 80 or earlier, unless a sound reason exists to do
otherwise.

> +	bool streaming;
> +};
> +
> +static inline struct ar0521_dev *to_ar0521_dev(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct ar0521_dev, sd);
> +}
> +
> +static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> +{
> +	return &container_of(ctrl->handler, struct ar0521_dev, ctrls.handler)->sd;
> +}
> +
> +static u32 div64_round(u64 v, u32 d)
> +{
> +	return div_u64(v + (d >> 1), d);
> +}
> +
> +static u32 div64_round_up(u64 v, u32 d)
> +{
> +	return div_u64(v + d - 1, d);
> +}
> +
> +// 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)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	struct i2c_msg msg;
> +	int ret;
> +
> +	if (!pm_runtime_get_if_in_use(&client->dev))
> +		return 0;
> +
> +	msg.addr = client->addr;
> +	msg.flags = client->flags;
> +	msg.buf = (u8 *)data;
> +	msg.len = count * sizeof(*data);
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	pm_runtime_put(&client->dev);
> +
> +	if (ret < 0) {
> +		v4l2_err(&sensor->sd, "%s: I2C write error\n", __func__);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ar0521_write_reg(struct ar0521_dev *sensor, u16 reg, u16 val)
> +{
> +	__be16 buf[2] = {be(reg), be(val)};
> +
> +	return ar0521_write_regs(sensor, buf, 2);
> +}
> +
> +static int ar0521_set_geometry(struct ar0521_dev *sensor)
> +{
> +	// 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(x),
> +		be(y),
> +		be(x + sensor->fmt.width - 1),
> +		be(y + sensor->fmt.height - 1),
> +		be(sensor->fmt.width),
> +		be(sensor->fmt.height)
> +	};
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> +
> +	return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
> +}
> +
> +static int ar0521_set_gains(struct ar0521_dev *sensor)
> +{
> +	int green = sensor->ctrls.gain->val;
> +	int red = max(green + sensor->ctrls.red_balance->val, 0);
> +	int blue = max(green + sensor->ctrls.blue_balance->val, 0);
> +	unsigned int gain = min(red, min(green, blue));
> +	unsigned int analog = min(gain, 64u); // range is 0 - 127
> +	__be16 regs[5];
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> +
> +	red   = min(red   - analog + 64, 511u);
> +	green = min(green - analog + 64, 511u);
> +	blue  = min(blue  - analog + 64, 511u);
> +	regs[0] = be(AR0521_REG_GREEN1_GAIN);
> +	regs[1] = be(green << 7 | analog);
> +	regs[2] = be(blue  << 7 | analog);
> +	regs[3] = be(red   << 7 | analog);
> +	regs[4] = be(green << 7 | analog);
> +
> +	return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
> +}
> +
> +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
> +		/* 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
> +		/* 0x30A */ be(1)  // op_sys_clk_div
> +	};
> +	u32 num = sensor->current_frame_interval.numerator;
> +	u32 denom = sensor->current_frame_interval.denominator;
> +	int ret;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> +
> +	// 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_EXTRA_DELAY, sensor->extra_delay);
> +	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);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(&sensor->i2c_client->dev,
> +		"AR0521: %ux%u, total %ux%u, pixel clock %u MHz, %u (%u/%u) FPS\n",
> +		sensor->fmt.width, sensor->fmt.height, sensor->total_width, sensor->total_height,
> +		sensor->pix_clk, (num + denom / 2) / denom, num, denom);
> +	return 0;
> +}
> +
> +static int ar0521_set_stream(struct ar0521_dev *sensor, bool on)
> +{
> +	int ret;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, on);
> +
> +	ret = ar0521_write_mode(sensor);
> +	if (ret)
> +		return ret;
> +
> +	if (on) {
> +		ret = ar0521_set_gains(sensor);
> +		if (ret)
> +			return ret;
> +
> +		// normal output on clock and data lanes
> +		ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS, 0);
> +		if (ret)
> +			return ret;
> +	} else {
> +		// reset gain, the sensor may produce all white pixels without this
> +		ret = ar0521_write_reg(sensor, AR0521_REG_GLOBAL_GAIN, 0x2000);
> +		if (ret)
> +			return ret;
> +
> +		// set LP-11 on clock and data lanes
> +		ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS,
> +				       AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	// start streaming (possibly with LP-11 on all lines)
> +	return ar0521_write_reg(sensor, AR0521_REG_RESET,
> +				AR0521_REG_RESET_DEFAULTS |
> +				AR0521_REG_RESET_RESTART |
> +				AR0521_REG_RESET_STREAM);
> +}
> +
> +static u32 calc_pll(struct ar0521_dev *sensor, int num, u32 freq, u16 *pre_ptr, u16 *mult_ptr)
> +{
> +	u16 pre = 1, mult = 1, new_pre;
> +	u32 pll = AR0521_PLL_MAX + 1;
> +
> +	for (new_pre = 1; new_pre < 64; new_pre++) {
> +		u32 new_pll;
> +		u32 new_mult = div64_round_up((u64)freq * new_pre, sensor->extclk_freq);
> +
> +		if (new_mult < 32)
> +			continue; // minimum value
> +		if (new_mult > 254)
> +			break; // maximum, larger pre won't work either
> +		if (sensor->extclk_freq * (u64)new_mult < AR0521_PLL_MIN * new_pre)
> +			continue;
> +		if (sensor->extclk_freq * (u64)new_mult > AR0521_PLL_MAX * new_pre)
> +			break; // larger pre won't work either
> +		new_pll = div64_round_up(sensor->extclk_freq * (u64)new_mult, new_pre);
> +		if (new_pll < pll) {
> +			pll = new_pll;
> +			pre = new_pre;
> +			mult = new_mult;
> +		}
> +	}
> +
> +	pll = div64_round(sensor->extclk_freq * (u64)mult, pre);
> +	*pre_ptr = pre;
> +	*mult_ptr = mult;
> +	return pll;
> +}
> +
> +static void ar0521_adj_fmt(struct v4l2_mbus_framefmt *fmt)
> +{
> +	fmt->width = clamp(ALIGN(fmt->width, 4), AR0521_WIDTH_MIN, AR0521_WIDTH_MAX);
> +	fmt->height = clamp(ALIGN(fmt->height, 4), AR0521_HEIGHT_MIN, AR0521_HEIGHT_MAX);
> +	fmt->code = MEDIA_BUS_FMT_SGRBG8_1X8;
> +	fmt->field = V4L2_FIELD_NONE;
> +	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +	fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +}
> +
> +#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
> +	u64 pix_clk; // for calculations
> +	u32 pixels, num, denom, new_total_height, new_pixels;
> +	u16 total_width, total_height;
> +
> +	total_width = max(sensor->fmt.width + AR0521_WIDTH_BLANKING_MIN, AR0521_TOTAL_WIDTH_MIN);
> +	total_height = sensor->fmt.height + AR0521_HEIGHT_BLANKING_MIN;
> +
> +	pixels = total_width * total_height;
> +	num = sensor->frame_interval.numerator;
> +	denom = sensor->frame_interval.denominator;
> +
> +	// calculate approximate pixel clock first
> +	pix_clk = div64_round_up(pixels * (u64)num, denom);
> +	if (pix_clk > AR0521_PIXEL_CLOCK_MAX) {
> +		u32 cnt;
> +		// have to recalculate FPS
> +		num = pix_clk = AR0521_PIXEL_CLOCK_MAX;
> +		denom = pixels;
> +		// try to reduce the numbers a bit
> +		for (cnt = 2; cnt * cnt < denom; cnt++) {
> +			while (num % cnt == 0 && denom % cnt == 0) {
> +				num /= cnt;
> +				denom /= cnt;
> +			}
> +		}
> +	} else if (pix_clk < AR0521_PIXEL_CLOCK_MIN)
> +		// we will compensate with total_height and extra_delay
> +		pix_clk = AR0521_PIXEL_CLOCK_MIN;
> +
> +	sensor->current_frame_interval.numerator = num;
> +	sensor->current_frame_interval.denominator = denom;
> +
> +	// 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);
> +
> +	// let's see if we can do better
> +	new_total_height = (div64_round((u64)pix_clk * denom, num) / total_width) & ~1; // must be even
> +	if (new_total_height > total_height) {
> +		total_height = new_total_height;
> +		pixels = total_width * total_height;
> +	}
> +
> +	// maybe there is still room for improvement
> +	new_pixels = div64_round(pix_clk * denom, num);
> +	sensor->extra_delay = 0;
> +	if (new_pixels > pixels)
> +		sensor->extra_delay = new_pixels - pixels;
> +
> +	sensor->pix_clk = pix_clk;
> +	sensor->total_width = total_width;
> +	sensor->total_height = total_height;
> +}
> +
> +static int ar0521_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state,
> +			  struct v4l2_subdev_format *format)
> +{
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +	struct v4l2_mbus_framefmt *fmt;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, format->which);
> +
> +	mutex_lock(&sensor->lock);
> +
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> +		fmt = v4l2_subdev_get_try_format(&sensor->sd, sd_state, 0 /* pad */);
> +	else
> +		fmt = &sensor->fmt;
> +
> +	format->format = *fmt;
> +
> +	mutex_unlock(&sensor->lock);
> +	return 0;
> +}
> +
> +static int ar0521_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state,
> +			  struct v4l2_subdev_format *format)
> +{
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +	int ret = 0;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, format->which);
> +
> +	ar0521_adj_fmt(&format->format);
> +
> +	mutex_lock(&sensor->lock);
> +
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> +		struct v4l2_mbus_framefmt *fmt;
> +
> +		fmt = v4l2_subdev_get_try_format(sd, sd_state, 0 /* pad */);
> +		*fmt = format->format;
> +	} else {
> +		sensor->fmt = format->format;
> +		ar0521_calc_mode(sensor);
> +		ret = ar0521_write_mode(sensor);
> +	}
> +
> +	mutex_unlock(&sensor->lock);
> +	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 ret;
> +
> +	// v4l2_ctrl_lock() locks our own mutex
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s(0x%X)\n", __func__, ctrl->id);

Please make sure the sensor is powered on before accessing it. See e.g.
imx219_set_ctrl() how to do this.

> +
> +	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;
> +		ret = ar0521_set_geometry(sensor);
> +		break;
> +	case V4L2_CID_GAIN:
> +	case V4L2_CID_RED_BALANCE:
> +	case V4L2_CID_BLUE_BALANCE:
> +		ret = ar0521_set_gains(sensor);
> +		break;
> +	case V4L2_CID_EXPOSURE:
> +		ret = ar0521_write_reg(sensor, AR0521_REG_COARSE_INTEGRATION_TIME, ctrl->val);
> +		break;
> +	case V4L2_CID_TEST_PATTERN:
> +		ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE, ctrl->val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops ar0521_ctrl_ops = {
> +	.s_ctrl = ar0521_s_ctrl,
> +};
> +
> +static const char * const test_pattern_menu[] = {
> +	"Disabled",
> +	"Solid color",
> +	"Color bars",
> +	"Faded color bars"
> +};
> +
> +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 ret;
> +
> +	v4l2_ctrl_handler_init(hdl, 32);
> +
> +	// we can use our own mutex for the ctrl lock
> +	hdl->lock = &sensor->lock;
> +
> +	// 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, -512, 511, 1, 0);
> +	ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BLUE_BALANCE, -512, 511, 1, 0);
> +
> +	// alternate for frame interval
> +	ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, AR0521_WIDTH_BLANKING_MIN, 4094, 1, AR0521_WIDTH_BLANKING_MIN);
> +	ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, AR0521_HEIGHT_BLANKING_MIN, 4094, 2, AR0521_HEIGHT_BLANKING_MIN);
> +	// Read-only
> +	ctrls->pixrate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE, AR0521_PIXEL_CLOCK_MIN, AR0521_PIXEL_CLOCK_MAX, 1, AR0521_PIXEL_CLOCK_MIN);
> +
> +	// manual exposure time
> +	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0, 65535, 1, 0);
> +
> +	ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> +							   ARRAY_SIZE(test_pattern_menu) - 1,
> +							   0, 0, test_pattern_menu);
> +
> +	if (hdl->error) {
> +		ret = hdl->error;
> +		goto free_ctrls;
> +	}
> +
> +	sensor->sd.ctrl_handler = hdl;
> +	return 0;
> +
> +free_ctrls:
> +	v4l2_ctrl_handler_free(hdl);
> +	return ret;
> +}
> +
> +static const struct initial_reg {
> +	u16 addr, value;
> +} initial_regs[] = {
> +	// corrections_recommended_bayer
> +	{0x3042, 0x0004}, // RNC:enable b/w rnc mode
> +	{0x3044, 0x4580}, // RNC:enable row noise correction
> +	{0x30EE, 0x1136}, // RNC:rnc scaling factor-->initial recommended setting
> +	{0x3120, 0x0001}, // recommended setting for dither
> +	{0x3F2C, 0x442E}, // GTH_THRES_RTN: 7max,7min filtered out of every 46
> +	{0x30D2, 0x0000}, // CRM/CC: enable crm on Visible and CC rows
> +	{0x30D4, 0x0000}, // CC: CC enabled with 16 samples per column
> +	{0x30D6, 0x2FFF}, // CC: bw mode enabled/12 bit data resolution/bw mode
> +	{0x30DA, 0x0FFF}, // CC: column correction clip level 2 is 0
> +	{0x30DC, 0x0FFF}, // CC: column correction clip level 3 is 0
> +	{0x30DE, 0x0000}, // CC: Group FPN correction
> +	{0x31E0, 0x0781}, // Fuse/2DDC: enable 2ddc
> +	{0x3180, 0x9434}, // FDOC:fdoc settings with fdoc every frame turned of
> +	{0x3172, 0x0206}, // txlo clk divider options
> +	{0x3F00, 0x0017}, // BM_T0
> +	{0x3F02, 0x02DD}, // BM_T1
> +	{0x3F04, 0x0020}, // if Ana_gain less than 2, use noise_floor0, multipl
> +	{0x3F06, 0x0040}, // if Ana_gain between 4 and 7, use noise_floor2 and
> +	{0x3F08, 0x0070}, // if Ana_gain between 4 and 7, use noise_floor2 and
> +	{0x3F0A, 0x0101}, // Define noise_floor0(low address) and noise_floor1
> +	{0x3F0C, 0x0302}, // Define noise_floor2 and noise_floor3
> +	{0x3F1E, 0x0022},
> +	{0x3F1A, 0x01FF}, // cross factor 2
> +	{0x3F14, 0x0505}, // single k factor 2
> +	{0x3F44, 0x0707}, // couple k factor 2
> +	{0x3F18, 0x01FF}, // cross factor 1
> +	{0x3F12, 0x0505}, // single k factor 1
> +	{0x3F42, 0x1511}, // couple k factor 1
> +	{0x3F16, 0x01FF}, // cross factor 0
> +	{0x3F10, 0x0505}, // single k factor 0
> +	{0x3F40, 0x1511}, // couple k factor 0
> +
> +	// analog_setup_recommended_12bit
> +	{0x3EB6, 0x004C}, // ECL
> +	{0x3EBA, 0xAAAA},
> +	{0x3EBC, 0x0086}, // Bias currents for FSC/ECL
> +	{0x3EC0, 0x1E00}, // SFbin/SH mode settings
> +	{0x3EC2, 0x100B}, // CLK divider for ramp for 12 bit 400MHz mode only
> +	{0x3EC4, 0x3300}, // FSC clamps for HDR mode and adc comp power down co
> +	{0x3EC6, 0xEA44}, // VLN and clk gating controls
> +	{0x3EC8, 0x6F6F}, // Txl0 and Txlo1 settings for normal mode
> +	{0x3ECA, 0x2F4A}, // CDAC/Txlo2/RSTGHI/RSTGLO settings
> +	{0x3ECC, 0x0506}, // RSTDHI/RSTDLO/CDAC/TXHI settings
> +	{0x3ECE, 0x203B}, // Ramp buffer settings and Booster enable (bits 0-5)
> +	{0x3ED0, 0x13F0}, // TXLO from atest/sf bin settings
> +	{0x3ED2, 0x9A3D}, // Booster settings for reference rows/columns
> +	{0x3ED4, 0x862F}, // TXLO open loop/row driver settings
> +	{0x3ED6, 0x4081}, // Txlatch fr cfpn rows/vln bias
> +	{0x3ED8, 0x4003}, // Ramp step setting for 12 bit 400 Mhz mode
> +	{0x3EDA, 0x9A80}, // ramp offset for T1/normal and rst under range
> +	{0x3EDC, 0xC000}, // over range for rst and under range for sig
> +	{0x3EDE, 0xC103}, // over range for sig and col dec clk settings
> +	{0x3426, 0x1600}, // ADC offset distribution pulse
> +	{0x342A, 0x0038}, // pulse_config
> +	{0x3F3E, 0x0001}, // Switch ADC from 10 bit to 12 bit mode
> +	{0x341A, 0x6051},
> +	{0x3420, 0x6051},
> +
> +	// analog_setup_recommended_10bit
> +	{0x3EC2, 0x100A}, // CLK divider for ramp for 10 bit 400MH
> +	{0x3ED8, 0x8003}, // Ramp step setting for 10 bit 400 Mhz
> +	{0x341A, 0x4735}, // Samp&Hold pulse in ADC
> +	{0x3420, 0x4735}, // Samp&Hold pulse in ADC
> +	{0x3426, 0x8A1A}, // ADC offset distribution pulse
> +	{0x342A, 0x0018}, // pulse_config
> +	{0x3ED2, 0xA53D}, // Ramp offset
> +	{0x3EDA, 0xA580}, // Ramp Offset
> +	{0x3EBA, 0xAAAD},
> +	{0x3EB6, 0x004C},
> +	{0x3F3E, 0x0000}, // Switch ADC from 12 bit to 10 bit mode
> +
> +	// new RNC 10bit
> +	{0x30EE, 0x1136}, // RNC:rnc scaling factor=*54/64 (32/38*64=53.9)
> +	{0x3F2C, 0x442E}, // GTH_THRES_RTN: 4max,4min filtered out of every 46 samples and
> +	// for 10bit mode
> +	{0x301E, 0x00AA}, // PEDESTAL+2 :+2 is a workaround for 10bit mode +0.5 Rounding
> +	{0x3120, 0x0005}, // p1 dither enabled for 10bit mode
> +
> +	{0x0112, 0x0808}, // 8-bit/8-bit mode
> +	{0x31BC, 0x068C}, // don't use continuous clock mode while shut down
> +	{0x30FA, 0xFD00}, // GPIO0 = flash, GPIO1 = shutter
> +	{0x31B0, 0x008B}, // frame_preamble - FIXME check WRT lanes#
> +	{0x31B2, 0x0050}, // line_preamble - FIXME check WRT lanes#
> +};
> +
> +static __be16 pixel_timing_recommended[] = {
> +	be(0x3D00), // first register address
> +	/* 3D00 */ be(0x043E), be(0x4760), be(0xFFFF), be(0xFFFF), be(0x8000), be(0x0510), be(0xAF08), be(0x0252),
> +	/* 3D10 */ be(0x486F), be(0x5D5D), be(0x8056), be(0x8313), be(0x0087), be(0x6A48), be(0x6982), be(0x0280),
> +	/* 3D20 */ be(0x8359), be(0x8D02), be(0x8020), be(0x4882), be(0x4269), be(0x6A95), be(0x5988), be(0x5A83),
> +	/* 3D30 */ be(0x5885), be(0x6280), be(0x6289), be(0x6097), be(0x5782), be(0x605C), be(0xBF18), be(0x0961),
> +	/* 3D40 */ be(0x5080), be(0x2090), be(0x4390), be(0x4382), be(0x5F8A), be(0x5D5D), be(0x9C63), be(0x8063),
> +	/* 3D50 */ be(0xA960), be(0x9757), be(0x8260), be(0x5CFF), be(0xBF10), be(0x1681), be(0x0802), be(0x8000),
> +	/* 3D60 */ be(0x141C), be(0x6000), be(0x6022), be(0x4D80), be(0x5C97), be(0x6A69), be(0xAC6F), be(0x4645),
> +	/* 3D70 */ be(0x4400), be(0x0513), be(0x8069), be(0x6AC6), be(0x5F95), be(0x5F70), be(0x8040), be(0x4A81),
> +	/* 3D80 */ be(0x0300), be(0xE703), be(0x0088), be(0x4A83), be(0x40FF), be(0xFFFF), be(0xFD70), be(0x8040),
> +	/* 3D90 */ be(0x4A85), be(0x4FA8), be(0x4F8C), be(0x0070), be(0xBE47), be(0x8847), be(0xBC78), be(0x6B89),
> +	/* 3DA0 */ be(0x6A80), be(0x6986), be(0x6B8E), be(0x6B80), be(0x6980), be(0x6A88), be(0x7C9F), be(0x866B),
> +	/* 3DB0 */ be(0x8765), be(0x46FF), be(0xE365), be(0xA679), be(0x4A40), be(0x4580), be(0x44BC), be(0x7000),
> +	/* 3DC0 */ be(0x8040), be(0x0802), be(0x10EF), be(0x0104), be(0x3860), be(0x5D5D), be(0x5682), be(0x1300),
> +	/* 3DD0 */ be(0x8648), be(0x8202), be(0x8082), be(0x598A), be(0x0280), be(0x2048), be(0x3060), be(0x8042),
> +	/* 3DE0 */ be(0x9259), be(0x865A), be(0x8258), be(0x8562), be(0x8062), be(0x8560), be(0x9257), be(0x8221),
> +	/* 3DF0 */ be(0x10FF), be(0xB757), be(0x9361), be(0x1019), be(0x8020), be(0x9043), be(0x8E43), be(0x845F),
> +	/* 3E00 */ be(0x835D), be(0x805D), be(0x8163), be(0x8063), be(0xA060), be(0x9157), be(0x8260), be(0x5CFF),
> +	/* 3E10 */ be(0xFFFF), be(0xFFE5), be(0x1016), be(0x2048), be(0x0802), be(0x1C60), be(0x0014), be(0x0060),
> +	/* 3E20 */ be(0x2205), be(0x8120), be(0x908F), be(0x6A80), be(0x6982), be(0x5F9F), be(0x6F46), be(0x4544),
> +	/* 3E30 */ be(0x0005), be(0x8013), be(0x8069), be(0x6A80), be(0x7000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3E40 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3E50 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3E60 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3E70 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3E80 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3E90 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3EA0 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3EB0 */ be(0x0000), be(0x0000), be(0x0000)};
> +
> +static void ar0521_power_off(struct ar0521_dev *sensor)
> +{
> +	int i;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> +	clk_disable_unprepare(sensor->extclk);
> +
> +	if (sensor->reset_gpio)
> +		gpiod_set_value(sensor->reset_gpio, 1); // assert RESET signal
> +
> +	for (i = AR0521_NUM_SUPPLIES - 1; i >= 0; i--) {
> +		if (sensor->supplies[i])
> +			regulator_disable(sensor->supplies[i]);
> +	}
> +}
> +
> +static int ar0521_power_on(struct ar0521_dev *sensor)
> +{
> +	unsigned int cnt;
> +	int ret;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> +	for (cnt = 0; cnt < AR0521_NUM_SUPPLIES; cnt++)
> +		if (sensor->supplies[cnt]) {
> +			ret = regulator_enable(sensor->supplies[cnt]);
> +			if (ret < 0)
> +				goto off;
> +
> +			usleep_range(1000, 1500); // min 1 ms
> +		}
> +
> +	ret = clk_prepare_enable(sensor->extclk);
> +	if (ret < 0) {
> +		v4l2_err(&sensor->sd, "error enabling sensor clock\n");
> +		goto off;
> +	}
> +	usleep_range(1000, 1500); // min 1 ms
> +
> +	if (sensor->reset_gpio)
> +		gpiod_set_value(sensor->reset_gpio, 0); // deassert RESET signal
> +	usleep_range(4500, 5000); // min 45000 clocks
> +
> +	for (cnt = 0; cnt < ARRAY_SIZE(initial_regs); cnt++)
> +		if (ar0521_write_reg(sensor, initial_regs[cnt].addr, initial_regs[cnt].value))
> +			goto off;
> +
> +	ret = ar0521_write_regs(sensor, pixel_timing_recommended, ARRAY_SIZE(pixel_timing_recommended));
> +	if (ret)
> +		goto off;
> +
> +	ret = ar0521_write_reg(sensor, AR0521_REG_SERIAL_FORMAT, AR0521_REG_SERIAL_FORMAT_MIPI | sensor->lane_count);
> +	if (ret)
> +		goto off;
> +
> +	// set MIPI test mode - disabled for now
> +	ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_TEST_MODE,
> +			       ((0x40 << sensor->lane_count) - 0x40) | AR0521_REG_HISPI_TEST_MODE_LP11);
> +	if (ret)
> +		goto off;
> +
> +	ret = ar0521_write_reg(sensor, AR0521_REG_ROW_SPEED, 0x110 | 4 / sensor->lane_count);
> +	if (ret)
> +		goto off;
> +
> +	ar0521_calc_mode(sensor);
> +
> +	ret = ar0521_set_stream(sensor, 0);
> +	if (ret)
> +		goto off;
> +
> +	return 0;
> +off:
> +	ar0521_power_off(sensor);
> +	return ret;
> +}
> +
> +static int ar0521_s_power(struct v4l2_subdev *sd, int on)

Please drop the s_power callback and rely on runtime PM. You should still
power on the sensor in probe() and off in remove() to allow the driver to
operate without runtime PM. Please see e.g. drivers/media/i2c/imx219.c for
an example.

> +{
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +	struct i2c_client *client = sensor->i2c_client;
> +	int ret;
> +
> +	dev_dbg(&client->dev, "%s(%s)\n", __func__, on ? "on" : "off");
> +	mutex_lock(&sensor->lock);
> +
> +	if (on) {
> +		ret = pm_runtime_resume_and_get(&client->dev);
> +		if (ret == 0) {
> +			ret = ar0521_power_on(sensor);
> +			if (ret)
> +				pm_runtime_put(&client->dev);
> +		}
> +	} else {
> +		ret = pm_runtime_put(&client->dev);
> +		if (ret == 0)
> +			ar0521_power_off(sensor);
> +	}
> +
> +	mutex_unlock(&sensor->lock);
> +	return ret;
> +}
> +
> +static int ar0521_enum_mbus_code(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state,
> +				 struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> +	if (code->index)
> +		return -EINVAL;
> +
> +	code->code = sensor->fmt.code;
> +	dev_dbg(&sensor->i2c_client->dev, "%s() = %X\n", __func__, code->code);
> +	return 0;
> +}
> +
> +static int ar0521_g_frame_interval(struct v4l2_subdev *sd, struct v4l2_subdev_frame_interval *fi)
> +{
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> +	mutex_lock(&sensor->lock);
> +	fi->interval = sensor->current_frame_interval;
> +	mutex_unlock(&sensor->lock);
> +	dev_dbg(&sensor->i2c_client->dev, "%s() = %u/%u\n", __func__,
> +		fi->interval.numerator, fi->interval.denominator);
> +	return 0;
> +}
> +
> +static int ar0521_s_frame_interval(struct v4l2_subdev *sd, struct v4l2_subdev_frame_interval *fi)
> +{
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +	int ret;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s(%u/%u)\n", __func__,
> +		fi->interval.numerator, fi->interval.denominator);
> +	mutex_lock(&sensor->lock);
> +
> +	if (sensor->streaming) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	sensor->frame_interval = fi->interval;
> +	ar0521_calc_mode(sensor);
> +	ret = ar0521_write_mode(sensor);
> +out:
> +	mutex_unlock(&sensor->lock);
> +	return ret;
> +}

Please drop *frame_interval() callbacks. See
https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html for an
explanation.

Let me know if you have questions.

> +
> +static int ar0521_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +	int ret;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s(%i)\n", __func__, enable);
> +	mutex_lock(&sensor->lock);
> +
> +	ret = ar0521_set_stream(sensor, enable);
> +	if (!ret)
> +		sensor->streaming = enable;
> +
> +	mutex_unlock(&sensor->lock);
> +	return ret;
> +}
> +
> +static const struct v4l2_subdev_core_ops ar0521_core_ops = {
> +	.log_status = v4l2_ctrl_subdev_log_status,
> +	.s_power = ar0521_s_power,
> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> +static const struct v4l2_subdev_video_ops ar0521_video_ops = {
> +	.g_frame_interval = ar0521_g_frame_interval,
> +	.s_frame_interval = ar0521_s_frame_interval,
> +	.s_stream = ar0521_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops ar0521_pad_ops = {
> +	.enum_mbus_code = ar0521_enum_mbus_code,
> +	.get_fmt = ar0521_get_fmt,
> +	.set_fmt = ar0521_set_fmt,
> +};
> +
> +static const struct v4l2_subdev_ops ar0521_subdev_ops = {
> +	.core = &ar0521_core_ops,
> +	.video = &ar0521_video_ops,
> +	.pad = &ar0521_pad_ops,
> +};
> +
> +static int __maybe_unused ar0521_suspend(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> +	if (sensor->streaming)
> +		ar0521_set_stream(sensor, 0);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ar0521_resume(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> +	if (sensor->streaming)
> +		return ar0521_set_stream(sensor, 1);
> +
> +	return 0;
> +}
> +
> +static int ar0521_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	struct v4l2_fwnode_endpoint ep = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY
> +	};
> +	struct device *dev = &client->dev;
> +	struct fwnode_handle *endpoint;
> +	struct ar0521_dev *sensor;
> +	unsigned int cnt;
> +	int ret;
> +
> +	dev_dbg(dev, "%s()\n", __func__);
> +	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> +	if (!sensor)
> +		return -ENOMEM;
> +
> +	sensor->i2c_client = client;
> +	sensor->fmt.code = MEDIA_BUS_FMT_SGRBG8_1X8;
> +	sensor->fmt.width = AR0521_WIDTH_MAX;
> +	sensor->fmt.height = AR0521_HEIGHT_MAX;
> +	sensor->fmt.field = V4L2_FIELD_NONE;
> +	sensor->frame_interval.numerator = 30;
> +	sensor->frame_interval.denominator = 1;
> +
> +	endpoint = fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), NULL);

Please use:

	endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
						   FWNODE_GRAPH_ENDPOINT_NEXT);

> +	if (!endpoint) {
> +		dev_err(dev, "endpoint node not found\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = v4l2_fwnode_endpoint_parse(endpoint, &ep);
> +	fwnode_handle_put(endpoint);
> +	if (ret) {
> +		dev_err(dev, "could not parse endpoint\n");
> +		return ret;
> +	}
> +
> +	if (ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
> +		dev_err(dev, "invalid bus type, must be MIPI CSI2\n");
> +		return -EINVAL;
> +	}
> +
> +	sensor->lane_count = ep.bus.mipi_csi2.num_data_lanes;
> +	switch (sensor->lane_count) {
> +	case 1:
> +	case 2:
> +	case 4:
> +		break;
> +	default:
> +		dev_err(dev, "invalid number of MIPI data lanes\n");
> +		return -EINVAL;
> +	}
> +
> +	// get master clock (extclk)
> +	sensor->extclk = devm_clk_get(dev, "extclk");
> +	if (IS_ERR(sensor->extclk)) {
> +		dev_err(dev, "failed to get extclk\n");
> +		return PTR_ERR(sensor->extclk);
> +	}
> +
> +	ret = clk_set_rate(sensor->extclk, AR0521_EXTCLK_RATE);
> +	if (ret < 0) {
> +		dev_err(dev, "error setting clock rate\n");
> +		return ret;
> +	}
> +
> +	sensor->extclk_freq = clk_get_rate(sensor->extclk);
> +
> +	if (sensor->extclk_freq < AR0521_EXTCLK_MIN ||
> +	    sensor->extclk_freq > AR0521_EXTCLK_MAX) {
> +		dev_err(dev, "extclk frequency out of range: %u Hz\n", sensor->extclk_freq);
> +		return -EINVAL;
> +	}
> +
> +	// request optional reset pin (usually active low) and assert it
> +	sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +
> +	v4l2_i2c_subdev_init(&sensor->sd, client, &ar0521_subdev_ops);
> +
> +	sensor->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> +	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> +	if (ret)
> +		return ret;
> +
> +	for (cnt = 0; cnt < AR0521_NUM_SUPPLIES; cnt++) {
> +		struct regulator *supply = devm_regulator_get(dev, ar0521_supply_names[cnt]);
> +
> +		if (IS_ERR(supply)) {
> +			dev_info(dev, "no %s regulator found: %li\n", ar0521_supply_names[cnt], PTR_ERR(supply));
> +			return PTR_ERR(supply);
> +		}
> +		sensor->supplies[cnt] = supply;
> +	}
> +
> +	mutex_init(&sensor->lock);
> +
> +	ret = ar0521_init_controls(sensor);
> +	if (ret)
> +		goto entity_cleanup;
> +
> +	ar0521_adj_fmt(&sensor->fmt);
> +
> +	ret = v4l2_async_register_subdev(&sensor->sd);
> +	if (ret)
> +		goto free_ctrls;
> +
> +	// Enable runtime PM and turn off the device
> +	pm_runtime_set_active(&client->dev);
> +	pm_runtime_enable(&client->dev);
> +	pm_runtime_idle(&client->dev);
> +	dev_dbg(dev, "AR0521 driver initialized, master clock frequency: %u MHz, %u MIPI data lanes\n",
> +		sensor->extclk_freq, sensor->lane_count);
> +	return 0;
> +
> +free_ctrls:
> +	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> +entity_cleanup:
> +	media_entity_cleanup(&sensor->sd.entity);
> +	mutex_destroy(&sensor->lock);
> +	return ret;
> +}
> +
> +static int ar0521_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> +	v4l2_async_unregister_subdev(&sensor->sd);
> +	media_entity_cleanup(&sensor->sd.entity);
> +	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +	mutex_destroy(&sensor->lock);
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops ar0521_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(ar0521_suspend, ar0521_resume)
> +};
> +static const struct of_device_id ar0521_dt_ids[] = {
> +	{.compatible = "onnn,ar0521"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ar0521_dt_ids);
> +
> +static struct i2c_driver ar0521_i2c_driver = {
> +	.driver = {
> +		.name  = "ar0521",
> +		.pm = &ar0521_pm_ops,
> +		.of_match_table	= ar0521_dt_ids,
> +	},
> +	.probe    = ar0521_probe,
> +	.remove   = ar0521_remove,
> +};
> +
> +module_i2c_driver(ar0521_i2c_driver);
> +
> +MODULE_DESCRIPTION("AR0521 MIPI Camera subdev driver");
> +MODULE_AUTHOR("Krzysztof Hałasa <khalasa@piap.pl>");
> +MODULE_LICENSE("GPL v2");

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

Hi Sakari,

Thanks for your input.

> Where's the corresponding DT binding patch? Ideally it would be part of the
> same set.

Well I've sent it a moment before this one. Will make them a set next
time.

>> +#define AR0521_WIDTH_BLANKING_MIN     572u
>> +#define AR0521_HEIGHT_BLANKING_MIN     28u // must be even
>
> Please use /* */ for comments. The SPDX tag is an exception.

As far as I know, this is no longer the case, the C99 comments are now
permitted and maybe even encouraged. Or was I dreaming?

checkpatch doesn't protest either.

> Please wrap your lines at 80 or earlier, unless a sound reason exists to do
> otherwise.

This limitation appears to be lifted as well, after all those years.
Is there a specific reason to still use it here? Yes, lines longer than
80 chars make the code much more readable (for my eyes, at least).
Yes, I know there is some "soft" limit, and I trim lines when it makes
them better in my opinion.

>> +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 ret;
>> +
>> +	// v4l2_ctrl_lock() locks our own mutex
>> +
>> +	dev_dbg(&sensor->i2c_client->dev, "%s(0x%X)\n", __func__, ctrl->id);
>
> Please make sure the sensor is powered on before accessing it. See e.g.
> imx219_set_ctrl() how to do this.

I do, the lower-level IO function ar0521_write_regs() checks for this.
However, I identified a case when pm_runtime_* isn't available
(CONFIG_*), so I will post corrected patch.

> Please drop the s_power callback and rely on runtime PM.

Ok

> Please drop *frame_interval() callbacks. See
> https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html for an
> explanation.
>
> Let me know if you have questions.

I already wrote about this, you must have missed it:

...However, it apparently isn't as flexible as *frame_interval() -
I can't control the precise timings:
- the V4L2_CID_PIXEL_RATE is discrete and R/O (i.e. the application
  can't control it)
- even if I could somehow control pixel rate, frame interval is
  calculated as (width + h_blanking) * (height + v_blanking) /
  pixel_rate, which may be a bit coarse for precise video.
  With *frame_interval(), I compensate with per-frame "extra" delay
  (in single pixels, not whole H or V lines).

If the (userspace) application can control pixel rate and the "extra"
timing (well maybe pixel rate and the total number of pixels including
blanking and "extra") then I would be more than happy dropping
frame_interval().

I guess I could easily do that myself, if there is consensus about it.

E.g. V4L2_CID_PIXEL_RATE would not be forced R/O (and discrete) anymore
and I would invent a V4L2_CID_TOTAL_PIXELS or something.

The V4L2_CID_PIXEL_RATE issue may be somehow offset by the
V4L2_CID_LINK_FREQ, but the latter is "menu" type and thus not very
useful (am I to populate it with 250 values 1 MHz apart?).

Perhaps the receiver could publish it's input frequency range instead,
then the transmitter would set a fixed value? I don't know. And this
doesn't cover a case where the user needs a slower rate than max(tx, rx)
for some reason.

We should decide something about this, though.

I look forward for your comments,
-- 
Krzysztof "Chris" Hałasa

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

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Jacopo Mondi]

Hi Krzysztof,

   I've been testing this driver in the last few days, thanks for your
effort in upstreaming it!

I'll separately comment on what I had to change to have it working for
my use case, but let me continue the discussion from where it was left
pending here to add my 2 cents.

On Thu, Oct 07, 2021 at 11:11:09AM +0200, Krzysztof Hałasa wrote:
> Hi Sakari,
>
> Thanks for your input.
>
> > Where's the corresponding DT binding patch? Ideally it would be part of the
> > same set.
>
> Well I've sent it a moment before this one. Will make them a set next
> time.
>
> >> +#define AR0521_WIDTH_BLANKING_MIN     572u
> >> +#define AR0521_HEIGHT_BLANKING_MIN     28u // must be even
> >
> > Please use /* */ for comments. The SPDX tag is an exception.
>
> As far as I know, this is no longer the case, the C99 comments are now
> permitted and maybe even encouraged. Or was I dreaming?
>
> checkpatch doesn't protest either.

To my understanding the C99 standard added support for the //
commenting style and tollerate them, but they're still from C++ and I
see very few places where they're used in the kernel, and per as far I
know they're still not allowed by the coding style
https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting

Looking at how you used comments in the driver I think you could get
rid of most // comments easily, the register tables might be an
exception but I would really try to remove them from there as well.


>
> > Please wrap your lines at 80 or earlier, unless a sound reason exists to do
> > otherwise.
>
> This limitation appears to be lifted as well, after all those years.
> Is there a specific reason to still use it here? Yes, lines longer than
> 80 chars make the code much more readable (for my eyes, at least).
> Yes, I know there is some "soft" limit, and I trim lines when it makes
> them better in my opinion.
>

In my personal opinion lifting that restriction caused more pain than
anything, as different subsystem are now imposing different
requirements. Here everything has been so far pretty strict about
going over 80-cols, but I think there are situation where it makes
sense in example

static int a_very_long_function_name(struct a_long_struct_name with_a_long_list_of_args)
{

}

Would read bad if written

static int
a_very_long_function_name(
        struct a_long_struct_name with_a_long_list_of_args)
{

}

but here you have multiple cases where it would be very easy to stay
in the 80 cols limit

	ret = ar0521_write_regs(sensor, pixel_timing_recommended, ARRAY_SIZE(pixel_timing_recommended));
	if (ret)
		goto off;


should be

	ret = ar0521_write_regs(sensor, pixel_timing_recommended,
                                ARRAY_SIZE(pixel_timing_recommended));
	if (ret)
		goto off;

The register tables again could easily be shrinked in 80 cols (also
because you have them in two different styles).

My suggestion is: aim to 80 cols whenever possible, if it forces you
to do things like the above shown function declaration you can go a
little over that

As reported here
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
if you go over 100 you should ask yourself what are you doing :)

> >> +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 ret;
> >> +
> >> +	// v4l2_ctrl_lock() locks our own mutex
> >> +
> >> +	dev_dbg(&sensor->i2c_client->dev, "%s(0x%X)\n", __func__, ctrl->id);
> >
> > Please make sure the sensor is powered on before accessing it. See e.g.
> > imx219_set_ctrl() how to do this.
>
> I do, the lower-level IO function ar0521_write_regs() checks for this.

yes it returns immediately

        if (!sensor->power_count)
		return 0;

But I would rather move to runtime_pm and

         if (!pm_runtime_get_if_in_use(&client->dev))
                    return 0;

at the very beginning of s_ctrl and call __v4l2_ctrl_handler_setup()
at s_stream() time to apply the cached controls values before
streaming starts

> However, I identified a case when pm_runtime_* isn't available
> (CONFIG_*), so I will post corrected patch.
>
> > Please drop the s_power callback and rely on runtime PM.
>
> Ok
>
> > Please drop *frame_interval() callbacks. See
> > https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html for an
> > explanation.
> >
> > Let me know if you have questions.
>
> I already wrote about this, you must have missed it:
>
> ...However, it apparently isn't as flexible as *frame_interval() -
> I can't control the precise timings:
> - the V4L2_CID_PIXEL_RATE is discrete and R/O (i.e. the application
>   can't control it)
> - even if I could somehow control pixel rate, frame interval is
>   calculated as (width + h_blanking) * (height + v_blanking) /
>   pixel_rate, which may be a bit coarse for precise video.
>   With *frame_interval(), I compensate with per-frame "extra" delay
>   (in single pixels, not whole H or V lines).
>
> If the (userspace) application can control pixel rate and the "extra"
> timing (well maybe pixel rate and the total number of pixels including
> blanking and "extra") then I would be more than happy dropping
> frame_interval().
>
> I guess I could easily do that myself, if there is consensus about it.
>
> E.g. V4L2_CID_PIXEL_RATE would not be forced R/O (and discrete) anymore
> and I would invent a V4L2_CID_TOTAL_PIXELS or something.
>
> The V4L2_CID_PIXEL_RATE issue may be somehow offset by the
> V4L2_CID_LINK_FREQ, but the latter is "menu" type and thus not very
> useful (am I to populate it with 250 values 1 MHz apart?).
>
> Perhaps the receiver could publish it's input frequency range instead,
> then the transmitter would set a fixed value? I don't know. And this
> doesn't cover a case where the user needs a slower rate than max(tx, rx)
> for some reason.
>
> We should decide something about this, though.

The sensor frame rate is configured by userspace by changing the
blankings through the V4L2_CID_[VH]BLANK.

You are right the current definition is akward to work with, as there
is no way to set the 'total pixels' like you have suggested, but it's
rather userspace that knowing the desired total sizes has to compute
the blankings by subtracting the visible sizes (plus the mandatory min
blanking sizes).

Thanks
   j

>
> I look forward for your comments,
> --
> Krzysztof "Chris" Hałasa
>
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Jacopo Mondi]

Hi Krzysztof

On Tue, Oct 05, 2021 at 02:05:05PM +0200, Krzysztof Hałasa wrote:
> The driver has been extensively tested in an i.MX6-based system.

That's a give for code submitted for inclusions, right ? right ??? :)

Which frame sizes have you tested it with ? I know from the code the
driver supports a single mode, but if you want to add these
information to the commit message I would report it.

I sincerely would have gone for

"Add support for ON Semi AR0521 image sensor through the V4L2
subsystem.

ON Semi AR0521 is a RAW image sensor with a total visible sizes of
...x... in Raw Bayer SGRBG 8, 10 and 12 bit modes.

Tested on i.MX6 based system by capturing ....

>
> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
> ---
> Changes from v4:
> - removed struct v4l2_fwnode_endpoint from struct ar0521_dev
>   (leaving only MIPI lane_count)
> - converted power_count to pm_runtime_*()
> - removed extra debug/formatting
> - (get|set)_*() don't check for pad# (using pad 0 only)
> - suspend/resume support
> - removed static int debug, now using dev_dbg() only (+dynamic debug)
> - trivial changes
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ee91c5472bc1..29115fdc5d4a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1358,6 +1358,12 @@ S:	Supported
>  W:	http://www.aquantia.com
>  F:	drivers/net/ethernet/aquantia/atlantic/aq_ptp*
>
> +AR0521 ON SEMICONDUCTOR CAMERA SENSOR DRIVER
> +M:	Krzysztof Hałasa <khalasa@piap.pl>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +F:	drivers/media/i2c/ar0521.c

The dt binding files should be named here.

> +
>  ARASAN NAND CONTROLLER DRIVER
>  M:	Miquel Raynal <miquel.raynal@bootlin.com>
>  M:	Naga Sureshkumar Relli <nagasure@xilinx.com>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index adb348aa8396..af031c45a5a4 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -730,6 +730,16 @@ config VIDEO_APTINA_PLL
>  config VIDEO_CCS_PLL
>  	tristate
>
> +config VIDEO_AR0521
> +	tristate "ON Semiconductor AR0521 sensor support"
> +	depends on I2C && VIDEO_V4L2

select CONFIG_MEDIA_CONTROLLER as you register a media entity
select VIDEO_V4L2_SUBDEV_API as you expose a devnode to userspace
select V4L2_FWNODE as you parse the OF node and endpoint

> +	help
> +	  This is a Video4Linux2 sensor driver for the ON Semiconductor
> +	  AR0521 camera.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ar0521.
> +
>  config VIDEO_HI556
>  	tristate "Hynix Hi-556 sensor support"
>  	depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 5ac8d639e5ca..3163f9a081ac 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -117,6 +117,7 @@ obj-$(CONFIG_VIDEO_I2C)		+= video-i2c.o
>  obj-$(CONFIG_VIDEO_ML86V7667)	+= ml86v7667.o
>  obj-$(CONFIG_VIDEO_OV2659)	+= ov2659.o
>  obj-$(CONFIG_VIDEO_TC358743)	+= tc358743.o
> +obj-$(CONFIG_VIDEO_AR0521)	+= ar0521.o

seems like we've given up with alphabetical ordering a long time ago.
Sigh.

>  obj-$(CONFIG_VIDEO_HI556)	+= hi556.o
>  obj-$(CONFIG_VIDEO_IMX208)	+= imx208.o
>  obj-$(CONFIG_VIDEO_IMX214)	+= imx214.o
> diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> new file mode 100644
> index 000000000000..27a4c362de52
> --- /dev/null
> +++ b/drivers/media/i2c/ar0521.c
> @@ -0,0 +1,1047 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Sieć Badawcza Łukasiewicz - Przemysłowy Instytut Automatyki i Pomiarów PIAP
> + * Written by Krzysztof Hałasa
> + */
> +

Alphabetically sort includes please

> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>

Do you need clk-provider.h ?

> +#include <linux/clkdev.h>

Do you need clkdev ?

> +#include <linux/ctype.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>

Do you need device ?

> +#include <linux/i2c.h>
> +#include <linux/init.h>

Do you need init ?

> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>

Do you need slab ?

> +#include <linux/types.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>

blank line please

> +#include <media/v4l2-async.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>

Do you need v4l2-device ?

> +#include <media/v4l2-event.h>

Are events actually used ?

> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +
> +// External clock (extclk) frequencies

As commented in the other email, use C90 style comments please.

> +#define AR0521_EXTCLK_RATE	  (27 * 1000 * 1000)

You try to set the external clock to this frequency, but your PLL code
does not rely on this value being actually set, am I wrong ? So why
would you try to change it in first place ?


> +#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)
> +
> +// effective pixel clocks, the registers may be DDR

Be consistent with comments starting with capital or lowercase
letters. It's usually "start with capital letter, end with full stop."

> +#define AR0521_PIXEL_CLOCK_MIN	 (168 * 1000 * 1000)
> +#define AR0521_PIXEL_CLOCK_MAX	 (414 * 1000 * 1000)
> +
> +#define AR0521_WIDTH_MIN	       8u
> +#define AR0521_WIDTH_MAX	    2608u
> +#define AR0521_HEIGHT_MIN	       8u
> +#define AR0521_HEIGHT_MAX	    1958u
> +
> +#define AR0521_WIDTH_BLANKING_MIN     572u
> +#define AR0521_HEIGHT_BLANKING_MIN     28u // must be even
> +#define AR0521_TOTAL_WIDTH_MIN	     2968u

Isn't this TOTAL_WIDTH_MAX ?

> +
> +// AR0521 registers
> +#define AR0521_REG_VT_PIX_CLK_DIV		0x0300
> +#define AR0521_REG_FRAME_LENGTH_LINES		0x0340
> +
> +#define AR0521_REG_CHIP_ID			0x3000
> +#define AR0521_REG_COARSE_INTEGRATION_TIME	0x3012
> +#define AR0521_REG_ROW_SPEED			0x3016
> +#define AR0521_REG_EXTRA_DELAY			0x3018
> +#define AR0521_REG_RESET			0x301A

You won't like this as it will require quite some changes, but we use
lowercase letters for hexadecimal values.

> +#define   AR0521_REG_RESET_DEFAULTS		  0x0238

By default BIT(5) is 0. Should this be 0x0218 ?

> +#define   AR0521_REG_RESET_GROUP_PARAM_HOLD	  0x8000
> +#define   AR0521_REG_RESET_STREAM		  BIT(2)
> +#define   AR0521_REG_RESET_RESTART		  BIT(1)
> +#define   AR0521_REG_RESET_INIT			  BIT(0)
> +
> +#define AR0521_REG_GREEN1_GAIN			0x3056
> +#define AR0521_REG_BLUE_GAIN			0x3058
> +#define AR0521_REG_RED_GAIN			0x305A
> +#define AR0521_REG_GREEN2_GAIN			0x305C
> +#define AR0521_REG_GLOBAL_GAIN			0x305E
> +
> +#define AR0521_REG_HISPI_TEST_MODE		0x3066
> +#define AR0521_REG_HISPI_TEST_MODE_LP11		  0x0004
> +
> +#define AR0521_REG_TEST_PATTERN_MODE		0x3070
> +
> +#define AR0521_REG_SERIAL_FORMAT		0x31AE
> +#define AR0521_REG_SERIAL_FORMAT_MIPI		  0x0200
> +
> +#define AR0521_REG_HISPI_CONTROL_STATUS		0x31C6
> +#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80
> +
> +#define be		cpu_to_be16
> +
> +static const char * const ar0521_supply_names[] = {
> +	"vdd_io",	// I/O (1.8V) supply
> +	"vdd",		// Core, PLL and MIPI (1.2V) supply
> +	"vaa",		// Analog (2.7V) supply
> +};
> +
> +#define AR0521_NUM_SUPPLIES ARRAY_SIZE(ar0521_supply_names)
> +
> +struct ar0521_ctrls {
> +	struct v4l2_ctrl_handler handler;
> +	struct v4l2_ctrl *exposure;
> +	struct v4l2_ctrl *gain, *red_balance, *blue_balance;
> +	struct v4l2_ctrl *test_pattern;
> +	struct v4l2_ctrl *hblank, *vblank, *pixrate;
> +};
> +
> +struct ar0521_dev {
> +	struct i2c_client *i2c_client;
> +	struct v4l2_subdev sd;
> +	struct media_pad pad;
> +	struct clk *extclk;
> +	u32 extclk_freq;
> +
> +	struct regulator *supplies[AR0521_NUM_SUPPLIES];
> +	struct gpio_desc *reset_gpio;
> +
> +	// lock to protect all members below
> +	struct mutex lock;
> +
> +	struct v4l2_mbus_framefmt fmt;
> +	struct v4l2_fract frame_interval, current_frame_interval;

As a general rule one variable per line. Even more so in structures
declaration.

> +	struct ar0521_ctrls ctrls;
> +	u32 pix_clk;
> +	unsigned int lane_count;
> +	u16 total_width, total_height, pll_pre, pll_mult, pll_pre2, pll_mult2, extra_delay;
> +	bool streaming;
> +};
> +
> +static inline struct ar0521_dev *to_ar0521_dev(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct ar0521_dev, sd);
> +}
> +
> +static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> +{
> +	return &container_of(ctrl->handler, struct ar0521_dev, ctrls.handler)->sd;
> +}
> +
> +static u32 div64_round(u64 v, u32 d)
> +{
> +	return div_u64(v + (d >> 1), d);
> +}
> +
> +static u32 div64_round_up(u64 v, u32 d)
> +{
> +	return div_u64(v + d - 1, d);
> +}
> +
> +// 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)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	struct i2c_msg msg;
> +	int ret;
> +
> +	if (!pm_runtime_get_if_in_use(&client->dev))
> +		return 0;

Oh, in my previous email I commented looking at v4 probably, not v5.

Anyway, I feel this check should really be in the caller.
Also, does this cause a power up/down sequence for every transaction ?

> +
> +	msg.addr = client->addr;
> +	msg.flags = client->flags;
> +	msg.buf = (u8 *)data;
> +	msg.len = count * sizeof(*data);

I see you write data of arbitrary length and that's why you need the
ugly be() in the registers definition and in all data you pass to this
function. That's ugly and I wonder if it could be avoided.

You could write them one register at the time so that you could

        __be16 be_data = cpu_to_be16(data);

        ..

        msg.buf = (u8 *)&be_data;
        ...

at the expense of one transaction per register write (like you do when
you write initial_reg[]). Sounds like a little price to pay for a
nicer driver but I might be underestimating it (or being to concerned
about the use of be() everywhere :)

> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	pm_runtime_put(&client->dev);
> +
> +	if (ret < 0) {
> +		v4l2_err(&sensor->sd, "%s: I2C write error\n", __func__);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ar0521_write_reg(struct ar0521_dev *sensor, u16 reg, u16 val)
> +{
> +	__be16 buf[2] = {be(reg), be(val)};
> +
> +	return ar0521_write_regs(sensor, buf, 2);
> +}
> +
> +static int ar0521_set_geometry(struct ar0521_dev *sensor)
> +{
> +	// 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(x),
> +		be(y),
> +		be(x + sensor->fmt.width - 1),
> +		be(y + sensor->fmt.height - 1),
> +		be(sensor->fmt.width),
> +		be(sensor->fmt.height)
> +	};
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> +

My datasheet version also describes registers from 0x3002 to 0x300a
to be about timings. I had to add

	__be16 timings[] = {
		be(0x3002),	be(y),
		/* 0x3004 */	be(x),
		/* 0x3006 */	be(y + sensor->fmt.height - 1),
		/* 0x3008 */	be(x + sensor->fmt.width - 1),
		/* 0x300a */	be(sensor->total_height),
		/* Documented as 'twice the number of pixel clocks in one row' */
		/* 0x300c */	be(2 * sensor->total_width),
	};
	ar0521_write_regs(sensor, timings, ARRAY_SIZE(timings));

To this function to be able to get images out from the sensor.

Also, be careful about register 0x300c which you don't seem to program
at the moment. The register is described as

   The number of pixel clock periods in one line (row) time. This
   includes visible pixels and horizontal blanking time. Need to set
   twice value of the number of pixel clock in one line row time.

And it seems the integration time depends on this register value

        - CIT = 0x3012
        - LLPCK = 1/2 × reg_300C
        - Integration Time = (CIT × LLPCK) / pix_clk

Are you able to control exposure properly with your version ?

> +	return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
> +}
> +
> +static int ar0521_set_gains(struct ar0521_dev *sensor)
> +{
> +	int green = sensor->ctrls.gain->val;
> +	int red = max(green + sensor->ctrls.red_balance->val, 0);
> +	int blue = max(green + sensor->ctrls.blue_balance->val, 0);

unsigned ?

> +	unsigned int gain = min(red, min(green, blue));
> +	unsigned int analog = min(gain, 64u); // range is 0 - 127

Nit: when possible declare variables in reverse xmas-tree order

> +	__be16 regs[5];
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> +
> +	red   = min(red   - analog + 64, 511u);
> +	green = min(green - analog + 64, 511u);
> +	blue  = min(blue  - analog + 64, 511u);
> +	regs[0] = be(AR0521_REG_GREEN1_GAIN);
> +	regs[1] = be(green << 7 | analog);
> +	regs[2] = be(blue  << 7 | analog);
> +	regs[3] = be(red   << 7 | analog);
> +	regs[4] = be(green << 7 | analog);
> +
> +	return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
> +}
> +
> +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
> +		/* 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
> +		/* 0x30A */ be(1)  // op_sys_clk_div
> +	};
> +	u32 num = sensor->current_frame_interval.numerator;
> +	u32 denom = sensor->current_frame_interval.denominator;
> +	int ret;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);

tracepoints debug are not that useful if they do not report what has
been written, aren't they ?

> +
> +	// 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);

I comment here but that's mostly about the exposure control
definition. You initialize it with a value of 1, which means 1 line of
exposure which result in very dark images. I know userspace should be
in control of this, but a more sensible default value should probably
be used. Do you have a 'default' mode ? Could you set the default
exposure to something a bit larger (I know it's hard to define what a
sensible value could be, but 1 line is certainly very small)

> +	if (ret)
> +		return ret;
> +
> +	ret = ar0521_write_reg(sensor, AR0521_REG_EXTRA_DELAY, sensor->extra_delay);
> +	if (ret)
> +		return ret;
> +
> +	ret = ar0521_write_reg(sensor, AR0521_REG_RESET, AR0521_REG_RESET_DEFAULTS | AR0521_REG_RESET_STREAM);
> +	if (ret)
> +		return ret;

This part I don't get. As far as I can see and can read, setting the
AR0521_REG_RESET_STREAM bit in AR0521_REG_RESET_DEFAULTS as the effect
of:

Setting this bit places the sensor in streaming mode.
Clearing this bit places the sensor in a low power mode. The result of clearing
this bit depends upon the operating mode of the sensor. Entry and exit from
streaming mode can also be controlled from the signal interface

I would have expected to see this bit set/cleared at s_stream() time
(I've done so and things work better, otherwise I get crippled
images).

> +
> +	ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE, sensor->ctrls.test_pattern->val);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(&sensor->i2c_client->dev,
> +		"AR0521: %ux%u, total %ux%u, pixel clock %u MHz, %u (%u/%u) FPS\n",
> +		sensor->fmt.width, sensor->fmt.height, sensor->total_width, sensor->total_height,
> +		sensor->pix_clk, (num + denom / 2) / denom, num, denom);

Empty line before return statements

> +	return 0;
> +}
> +
> +static int ar0521_set_stream(struct ar0521_dev *sensor, bool on)
> +{
> +	int ret;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, on);
> +
> +	ret = ar0521_write_mode(sensor);
> +	if (ret)
> +		return ret;
> +
> +	if (on) {
> +		ret = ar0521_set_gains(sensor);
> +		if (ret)
> +			return ret;
> +
> +		// normal output on clock and data lanes
> +		ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS, 0);

This register controls the HiSPI interface while my understanding is
that everything is MIPI CSI-2 in the rest of the driver. Why is it
here ? does it play any role in your setup ?

> +		if (ret)
> +			return ret;
> +	} else {
> +		// reset gain, the sensor may produce all white pixels without this
> +		ret = ar0521_write_reg(sensor, AR0521_REG_GLOBAL_GAIN, 0x2000);
> +		if (ret)
> +			return ret;
> +
> +		// set LP-11 on clock and data lanes
> +		ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS,
> +				       AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE);

Same question as above, why the HiSPI register ? And why enabling test
mode ?

> +		if (ret)
> +			return ret;
> +	}
> +
> +	// start streaming (possibly with LP-11 on all lines)

So you fall down here even in the case s_stream(0) ?

> +	return ar0521_write_reg(sensor, AR0521_REG_RESET,
> +				AR0521_REG_RESET_DEFAULTS |
> +				AR0521_REG_RESET_RESTART |
> +				AR0521_REG_RESET_STREAM);
> +}

I have changes set_stream to be

static int ar0521_set_stream(struct ar0521_dev *sensor, bool on)
{
	int ret;

	v4l2_dbg(2, debug, &sensor->sd, "%s(%u)\n", __func__, on);

	ret = ar0521_write_mode(sensor);
	if (ret)
		return ret;

	if (on) {
		ret = ar0521_set_gains(sensor);
		if (ret)
			return ret;

		// normal output on clock and data lanes
		ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS, 0);
		if (ret)
			return ret;

		// start streaming (possibly with LP-11 on all lines)
		return ar0521_write_reg(sensor, AR0521_REG_RESET,
					AR0521_REG_RESET_DEFAULTS | AR0521_REG_RESET_STREAM);
	} else {
		// reset gain, the sensor may produce all white pixels without this
		ret = ar0521_write_reg(sensor, AR0521_REG_GLOBAL_GAIN, 0x2000);
		if (ret)
			return ret;

		// set LP-11 on clock and data lanes
		ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS,
				       AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE);
		if (ret)
			return ret;

		// start streaming (possibly with LP-11 on all lines)
		return ar0521_write_reg(sensor, AR0521_REG_RESET, AR0521_REG_RESET_DEFAULTS);
	}
}

And I think I could remove AR0521_REG_HISPI_CONTROL_STATUS (but have
not tested that so far)

> +
> +static u32 calc_pll(struct ar0521_dev *sensor, int num, u32 freq, u16 *pre_ptr, u16 *mult_ptr)
> +{
> +	u16 pre = 1, mult = 1, new_pre;
> +	u32 pll = AR0521_PLL_MAX + 1;
> +
> +	for (new_pre = 1; new_pre < 64; new_pre++) {
> +		u32 new_pll;
> +		u32 new_mult = div64_round_up((u64)freq * new_pre, sensor->extclk_freq);
> +
> +		if (new_mult < 32)
> +			continue; // minimum value
> +		if (new_mult > 254)
> +			break; // maximum, larger pre won't work either
> +		if (sensor->extclk_freq * (u64)new_mult < AR0521_PLL_MIN * new_pre)
> +			continue;
> +		if (sensor->extclk_freq * (u64)new_mult > AR0521_PLL_MAX * new_pre)
> +			break; // larger pre won't work either
> +		new_pll = div64_round_up(sensor->extclk_freq * (u64)new_mult, new_pre);
> +		if (new_pll < pll) {
> +			pll = new_pll;
> +			pre = new_pre;
> +			mult = new_mult;
> +		}
> +	}
> +
> +	pll = div64_round(sensor->extclk_freq * (u64)mult, pre);
> +	*pre_ptr = pre;
> +	*mult_ptr = mult;
> +	return pll;
> +}
> +
> +static void ar0521_adj_fmt(struct v4l2_mbus_framefmt *fmt)
> +{
> +	fmt->width = clamp(ALIGN(fmt->width, 4), AR0521_WIDTH_MIN, AR0521_WIDTH_MAX);
> +	fmt->height = clamp(ALIGN(fmt->height, 4), AR0521_HEIGHT_MIN, AR0521_HEIGHT_MAX);
> +	fmt->code = MEDIA_BUS_FMT_SGRBG8_1X8;
> +	fmt->field = V4L2_FIELD_NONE;
> +	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +	fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +}
> +
> +#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
> +	u64 pix_clk; // for calculations
> +	u32 pixels, num, denom, new_total_height, new_pixels;
> +	u16 total_width, total_height;
> +
> +	total_width = max(sensor->fmt.width + AR0521_WIDTH_BLANKING_MIN, AR0521_TOTAL_WIDTH_MIN);
> +	total_height = sensor->fmt.height + AR0521_HEIGHT_BLANKING_MIN;
> +
> +	pixels = total_width * total_height;
> +	num = sensor->frame_interval.numerator;
> +	denom = sensor->frame_interval.denominator;
> +
> +	// calculate approximate pixel clock first
> +	pix_clk = div64_round_up(pixels * (u64)num, denom);
> +	if (pix_clk > AR0521_PIXEL_CLOCK_MAX) {
> +		u32 cnt;
> +		// have to recalculate FPS
> +		num = pix_clk = AR0521_PIXEL_CLOCK_MAX;
> +		denom = pixels;
> +		// try to reduce the numbers a bit
> +		for (cnt = 2; cnt * cnt < denom; cnt++) {
> +			while (num % cnt == 0 && denom % cnt == 0) {
> +				num /= cnt;
> +				denom /= cnt;
> +			}
> +		}
> +	} else if (pix_clk < AR0521_PIXEL_CLOCK_MIN)
> +		// we will compensate with total_height and extra_delay
> +		pix_clk = AR0521_PIXEL_CLOCK_MIN;
> +
> +	sensor->current_frame_interval.numerator = num;
> +	sensor->current_frame_interval.denominator = denom;
> +
> +	// 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);
> +
> +	// let's see if we can do better
> +	new_total_height = (div64_round((u64)pix_clk * denom, num) / total_width) & ~1; // must be even
> +	if (new_total_height > total_height) {
> +		total_height = new_total_height;
> +		pixels = total_width * total_height;
> +	}
> +
> +	// maybe there is still room for improvement
> +	new_pixels = div64_round(pix_clk * denom, num);
> +	sensor->extra_delay = 0;
> +	if (new_pixels > pixels)
> +		sensor->extra_delay = new_pixels - pixels;
> +
> +	sensor->pix_clk = pix_clk;
> +	sensor->total_width = total_width;
> +	sensor->total_height = total_height;
> +}


I've not yet looked into the PLL part, but I get a framerate of 16 FPS
in 1080p while 30 where expected. I think it's due to the fact I know
program 0x300c but I need more testing.

Before introducing 0x300c I got 30 FPS but the frame content was
mangled (or completely black)

> +
> +static int ar0521_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state,
> +			  struct v4l2_subdev_format *format)
> +{
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +	struct v4l2_mbus_framefmt *fmt;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, format->which);
> +
> +	mutex_lock(&sensor->lock);
> +
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> +		fmt = v4l2_subdev_get_try_format(&sensor->sd, sd_state, 0 /* pad */);
> +	else
> +		fmt = &sensor->fmt;
> +
> +	format->format = *fmt;
> +
> +	mutex_unlock(&sensor->lock);
> +	return 0;
> +}
> +
> +static int ar0521_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state,
> +			  struct v4l2_subdev_format *format)
> +{
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +	int ret = 0;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, format->which);
> +
> +	ar0521_adj_fmt(&format->format);
> +
> +	mutex_lock(&sensor->lock);
> +
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> +		struct v4l2_mbus_framefmt *fmt;
> +
> +		fmt = v4l2_subdev_get_try_format(sd, sd_state, 0 /* pad */);
> +		*fmt = format->format;
> +	} else {
> +		sensor->fmt = format->format;
> +		ar0521_calc_mode(sensor);
> +		ret = ar0521_write_mode(sensor);

Do you need to do so ? the mode is programmed at s_stream() time,
isn't it enough ? Same for the other call to write_mode() above or
set_geometry() below.

> +	}
> +
> +	mutex_unlock(&sensor->lock);
> +	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 ret;
> +
> +	// v4l2_ctrl_lock() locks our own mutex
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s(0x%X)\n", __func__, ctrl->id);
> +
> +	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;
> +		ret = ar0521_set_geometry(sensor);
> +		break;
> +	case V4L2_CID_GAIN:
> +	case V4L2_CID_RED_BALANCE:
> +	case V4L2_CID_BLUE_BALANCE:
> +		ret = ar0521_set_gains(sensor);
> +		break;
> +	case V4L2_CID_EXPOSURE:
> +		ret = ar0521_write_reg(sensor, AR0521_REG_COARSE_INTEGRATION_TIME, ctrl->val);
> +		break;
> +	case V4L2_CID_TEST_PATTERN:
> +		ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE, ctrl->val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops ar0521_ctrl_ops = {
> +	.s_ctrl = ar0521_s_ctrl,
> +};
> +
> +static const char * const test_pattern_menu[] = {
> +	"Disabled",
> +	"Solid color",
> +	"Color bars",
> +	"Faded color bars"
> +};
> +
> +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 ret;
> +
> +	v4l2_ctrl_handler_init(hdl, 32);
> +
> +	// we can use our own mutex for the ctrl lock
> +	hdl->lock = &sensor->lock;
> +
> +	// 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, -512, 511, 1, 0);
> +	ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BLUE_BALANCE, -512, 511, 1, 0);

Seems like you handle these together, should these be clusters ? Same
for the blankings

> +
> +	// alternate for frame interval
> +	ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, AR0521_WIDTH_BLANKING_MIN, 4094, 1, AR0521_WIDTH_BLANKING_MIN);
> +	ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, AR0521_HEIGHT_BLANKING_MIN, 4094, 2, AR0521_HEIGHT_BLANKING_MIN);

How nicer would this be in 80-cols ? :)

> +	// Read-only
> +	ctrls->pixrate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE, AR0521_PIXEL_CLOCK_MIN, AR0521_PIXEL_CLOCK_MAX, 1, AR0521_PIXEL_CLOCK_MIN);
> +
> +	// manual exposure time
> +	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0, 65535, 1, 0);
> +
> +	ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> +							   ARRAY_SIZE(test_pattern_menu) - 1,
> +							   0, 0, test_pattern_menu);
> +
> +	if (hdl->error) {
> +		ret = hdl->error;
> +		goto free_ctrls;
> +	}
> +
> +	sensor->sd.ctrl_handler = hdl;
> +	return 0;
> +
> +free_ctrls:
> +	v4l2_ctrl_handler_free(hdl);
> +	return ret;
> +}
> +
> +static const struct initial_reg {
> +	u16 addr, value;
> +} initial_regs[] = {
> +	// corrections_recommended_bayer
> +	{0x3042, 0x0004}, // RNC:enable b/w rnc mode
> +	{0x3044, 0x4580}, // RNC:enable row noise correction
> +	{0x30EE, 0x1136}, // RNC:rnc scaling factor-->initial recommended setting
> +	{0x3120, 0x0001}, // recommended setting for dither
> +	{0x3F2C, 0x442E}, // GTH_THRES_RTN: 7max,7min filtered out of every 46
> +	{0x30D2, 0x0000}, // CRM/CC: enable crm on Visible and CC rows
> +	{0x30D4, 0x0000}, // CC: CC enabled with 16 samples per column
> +	{0x30D6, 0x2FFF}, // CC: bw mode enabled/12 bit data resolution/bw mode
> +	{0x30DA, 0x0FFF}, // CC: column correction clip level 2 is 0
> +	{0x30DC, 0x0FFF}, // CC: column correction clip level 3 is 0
> +	{0x30DE, 0x0000}, // CC: Group FPN correction
> +	{0x31E0, 0x0781}, // Fuse/2DDC: enable 2ddc
> +	{0x3180, 0x9434}, // FDOC:fdoc settings with fdoc every frame turned of
> +	{0x3172, 0x0206}, // txlo clk divider options
> +	{0x3F00, 0x0017}, // BM_T0
> +	{0x3F02, 0x02DD}, // BM_T1
> +	{0x3F04, 0x0020}, // if Ana_gain less than 2, use noise_floor0, multipl
> +	{0x3F06, 0x0040}, // if Ana_gain between 4 and 7, use noise_floor2 and
> +	{0x3F08, 0x0070}, // if Ana_gain between 4 and 7, use noise_floor2 and
> +	{0x3F0A, 0x0101}, // Define noise_floor0(low address) and noise_floor1
> +	{0x3F0C, 0x0302}, // Define noise_floor2 and noise_floor3
> +	{0x3F1E, 0x0022},
> +	{0x3F1A, 0x01FF}, // cross factor 2
> +	{0x3F14, 0x0505}, // single k factor 2
> +	{0x3F44, 0x0707}, // couple k factor 2
> +	{0x3F18, 0x01FF}, // cross factor 1
> +	{0x3F12, 0x0505}, // single k factor 1
> +	{0x3F42, 0x1511}, // couple k factor 1
> +	{0x3F16, 0x01FF}, // cross factor 0
> +	{0x3F10, 0x0505}, // single k factor 0
> +	{0x3F40, 0x1511}, // couple k factor 0
> +
> +	// analog_setup_recommended_12bit
> +	{0x3EB6, 0x004C}, // ECL
> +	{0x3EBA, 0xAAAA},
> +	{0x3EBC, 0x0086}, // Bias currents for FSC/ECL
> +	{0x3EC0, 0x1E00}, // SFbin/SH mode settings
> +	{0x3EC2, 0x100B}, // CLK divider for ramp for 12 bit 400MHz mode only
> +	{0x3EC4, 0x3300}, // FSC clamps for HDR mode and adc comp power down co
> +	{0x3EC6, 0xEA44}, // VLN and clk gating controls
> +	{0x3EC8, 0x6F6F}, // Txl0 and Txlo1 settings for normal mode
> +	{0x3ECA, 0x2F4A}, // CDAC/Txlo2/RSTGHI/RSTGLO settings
> +	{0x3ECC, 0x0506}, // RSTDHI/RSTDLO/CDAC/TXHI settings
> +	{0x3ECE, 0x203B}, // Ramp buffer settings and Booster enable (bits 0-5)
> +	{0x3ED0, 0x13F0}, // TXLO from atest/sf bin settings
> +	{0x3ED2, 0x9A3D}, // Booster settings for reference rows/columns
> +	{0x3ED4, 0x862F}, // TXLO open loop/row driver settings
> +	{0x3ED6, 0x4081}, // Txlatch fr cfpn rows/vln bias
> +	{0x3ED8, 0x4003}, // Ramp step setting for 12 bit 400 Mhz mode
> +	{0x3EDA, 0x9A80}, // ramp offset for T1/normal and rst under range
> +	{0x3EDC, 0xC000}, // over range for rst and under range for sig
> +	{0x3EDE, 0xC103}, // over range for sig and col dec clk settings
> +	{0x3426, 0x1600}, // ADC offset distribution pulse
> +	{0x342A, 0x0038}, // pulse_config
> +	{0x3F3E, 0x0001}, // Switch ADC from 10 bit to 12 bit mode
> +	{0x341A, 0x6051},
> +	{0x3420, 0x6051},
> +
> +	// analog_setup_recommended_10bit
> +	{0x3EC2, 0x100A}, // CLK divider for ramp for 10 bit 400MH
> +	{0x3ED8, 0x8003}, // Ramp step setting for 10 bit 400 Mhz
> +	{0x341A, 0x4735}, // Samp&Hold pulse in ADC
> +	{0x3420, 0x4735}, // Samp&Hold pulse in ADC
> +	{0x3426, 0x8A1A}, // ADC offset distribution pulse
> +	{0x342A, 0x0018}, // pulse_config
> +	{0x3ED2, 0xA53D}, // Ramp offset
> +	{0x3EDA, 0xA580}, // Ramp Offset
> +	{0x3EBA, 0xAAAD},
> +	{0x3EB6, 0x004C},
> +	{0x3F3E, 0x0000}, // Switch ADC from 12 bit to 10 bit mode
> +
> +	// new RNC 10bit
> +	{0x30EE, 0x1136}, // RNC:rnc scaling factor=*54/64 (32/38*64=53.9)
> +	{0x3F2C, 0x442E}, // GTH_THRES_RTN: 4max,4min filtered out of every 46 samples and
> +	// for 10bit mode
> +	{0x301E, 0x00AA}, // PEDESTAL+2 :+2 is a workaround for 10bit mode +0.5 Rounding
> +	{0x3120, 0x0005}, // p1 dither enabled for 10bit mode
> +
> +	{0x0112, 0x0808}, // 8-bit/8-bit mode
> +	{0x31BC, 0x068C}, // don't use continuous clock mode while shut down
> +	{0x30FA, 0xFD00}, // GPIO0 = flash, GPIO1 = shutter
> +	{0x31B0, 0x008B}, // frame_preamble - FIXME check WRT lanes#
> +	{0x31B2, 0x0050}, // line_preamble - FIXME check WRT lanes#
> +};
> +
> +static __be16 pixel_timing_recommended[] = {

Have you got any idea what these do ? I have these registers not
documented.

> +	be(0x3D00), // first register address
> +	/* 3D00 */ be(0x043E), be(0x4760), be(0xFFFF), be(0xFFFF), be(0x8000), be(0x0510), be(0xAF08), be(0x0252),
> +	/* 3D10 */ be(0x486F), be(0x5D5D), be(0x8056), be(0x8313), be(0x0087), be(0x6A48), be(0x6982), be(0x0280),
> +	/* 3D20 */ be(0x8359), be(0x8D02), be(0x8020), be(0x4882), be(0x4269), be(0x6A95), be(0x5988), be(0x5A83),
> +	/* 3D30 */ be(0x5885), be(0x6280), be(0x6289), be(0x6097), be(0x5782), be(0x605C), be(0xBF18), be(0x0961),
> +	/* 3D40 */ be(0x5080), be(0x2090), be(0x4390), be(0x4382), be(0x5F8A), be(0x5D5D), be(0x9C63), be(0x8063),
> +	/* 3D50 */ be(0xA960), be(0x9757), be(0x8260), be(0x5CFF), be(0xBF10), be(0x1681), be(0x0802), be(0x8000),
> +	/* 3D60 */ be(0x141C), be(0x6000), be(0x6022), be(0x4D80), be(0x5C97), be(0x6A69), be(0xAC6F), be(0x4645),
> +	/* 3D70 */ be(0x4400), be(0x0513), be(0x8069), be(0x6AC6), be(0x5F95), be(0x5F70), be(0x8040), be(0x4A81),
> +	/* 3D80 */ be(0x0300), be(0xE703), be(0x0088), be(0x4A83), be(0x40FF), be(0xFFFF), be(0xFD70), be(0x8040),
> +	/* 3D90 */ be(0x4A85), be(0x4FA8), be(0x4F8C), be(0x0070), be(0xBE47), be(0x8847), be(0xBC78), be(0x6B89),
> +	/* 3DA0 */ be(0x6A80), be(0x6986), be(0x6B8E), be(0x6B80), be(0x6980), be(0x6A88), be(0x7C9F), be(0x866B),
> +	/* 3DB0 */ be(0x8765), be(0x46FF), be(0xE365), be(0xA679), be(0x4A40), be(0x4580), be(0x44BC), be(0x7000),
> +	/* 3DC0 */ be(0x8040), be(0x0802), be(0x10EF), be(0x0104), be(0x3860), be(0x5D5D), be(0x5682), be(0x1300),
> +	/* 3DD0 */ be(0x8648), be(0x8202), be(0x8082), be(0x598A), be(0x0280), be(0x2048), be(0x3060), be(0x8042),
> +	/* 3DE0 */ be(0x9259), be(0x865A), be(0x8258), be(0x8562), be(0x8062), be(0x8560), be(0x9257), be(0x8221),
> +	/* 3DF0 */ be(0x10FF), be(0xB757), be(0x9361), be(0x1019), be(0x8020), be(0x9043), be(0x8E43), be(0x845F),
> +	/* 3E00 */ be(0x835D), be(0x805D), be(0x8163), be(0x8063), be(0xA060), be(0x9157), be(0x8260), be(0x5CFF),
> +	/* 3E10 */ be(0xFFFF), be(0xFFE5), be(0x1016), be(0x2048), be(0x0802), be(0x1C60), be(0x0014), be(0x0060),
> +	/* 3E20 */ be(0x2205), be(0x8120), be(0x908F), be(0x6A80), be(0x6982), be(0x5F9F), be(0x6F46), be(0x4544),
> +	/* 3E30 */ be(0x0005), be(0x8013), be(0x8069), be(0x6A80), be(0x7000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3E40 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3E50 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3E60 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3E70 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3E80 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3E90 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3EA0 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000), be(0x0000),
> +	/* 3EB0 */ be(0x0000), be(0x0000), be(0x0000)};

closing brace here please

> +
> +static void ar0521_power_off(struct ar0521_dev *sensor)
> +{
> +	int i;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> +	clk_disable_unprepare(sensor->extclk);
> +
> +	if (sensor->reset_gpio)
> +		gpiod_set_value(sensor->reset_gpio, 1); // assert RESET signal
> +
> +	for (i = AR0521_NUM_SUPPLIES - 1; i >= 0; i--) {
> +		if (sensor->supplies[i])
> +			regulator_disable(sensor->supplies[i]);
> +	}
> +}
> +
> +static int ar0521_power_on(struct ar0521_dev *sensor)
> +{
> +	unsigned int cnt;
> +	int ret;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> +	for (cnt = 0; cnt < AR0521_NUM_SUPPLIES; cnt++)
> +		if (sensor->supplies[cnt]) {
> +			ret = regulator_enable(sensor->supplies[cnt]);
> +			if (ret < 0)
> +				goto off;
> +
> +			usleep_range(1000, 1500); // min 1 ms
> +		}
> +
> +	ret = clk_prepare_enable(sensor->extclk);
> +	if (ret < 0) {
> +		v4l2_err(&sensor->sd, "error enabling sensor clock\n");
> +		goto off;
> +	}
> +	usleep_range(1000, 1500); // min 1 ms
> +
> +	if (sensor->reset_gpio)
> +		gpiod_set_value(sensor->reset_gpio, 0); // deassert RESET signal
> +	usleep_range(4500, 5000); // min 45000 clocks
> +
> +	for (cnt = 0; cnt < ARRAY_SIZE(initial_regs); cnt++)
> +		if (ar0521_write_reg(sensor, initial_regs[cnt].addr, initial_regs[cnt].value))
> +			goto off;
> +
> +	ret = ar0521_write_regs(sensor, pixel_timing_recommended, ARRAY_SIZE(pixel_timing_recommended));
> +	if (ret)
> +		goto off;
> +
> +	ret = ar0521_write_reg(sensor, AR0521_REG_SERIAL_FORMAT, AR0521_REG_SERIAL_FORMAT_MIPI | sensor->lane_count);
> +	if (ret)
> +		goto off;
> +
> +	// set MIPI test mode - disabled for now
> +	ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_TEST_MODE,
> +			       ((0x40 << sensor->lane_count) - 0x40) | AR0521_REG_HISPI_TEST_MODE_LP11);

As far as I can tell this write has no effect.

Define test mode to be applied to mipi/ccp interface if test_en is asserted

and asfaict you never assert test_en

It also seems a register meant to be used for the bringup of the
sensor as it allows to place the CSI-2 lanes in a known state for
testing. I don't think we need it.

> +	if (ret)
> +		goto off;
> +
> +	ret = ar0521_write_reg(sensor, AR0521_REG_ROW_SPEED, 0x110 | 4 / sensor->lane_count);

I wasn't able to interpret this register right

pc_speed
Slows down the internal pixel clock frequency relative to the system
clock frequency.  A programmed value of N gives a pixel clock period
of N system clocks.  Only values 1, 2 and 4 are supported.

Shouldn't this be part of the PLL calculation ?

> +	if (ret)
> +		goto off;
> +
> +	ar0521_calc_mode(sensor);
> +
> +	ret = ar0521_set_stream(sensor, 0);
> +	if (ret)
> +		goto off;
> +
> +	return 0;
> +off:
> +	ar0521_power_off(sensor);
> +	return ret;
> +}
> +
> +static int ar0521_s_power(struct v4l2_subdev *sd, int on)
> +{
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +	struct i2c_client *client = sensor->i2c_client;
> +	int ret;
> +
> +	dev_dbg(&client->dev, "%s(%s)\n", __func__, on ? "on" : "off");
> +	mutex_lock(&sensor->lock);
> +
> +	if (on) {
> +		ret = pm_runtime_resume_and_get(&client->dev);
> +		if (ret == 0) {
> +			ret = ar0521_power_on(sensor);
> +			if (ret)
> +				pm_runtime_put(&client->dev);
> +		}
> +	} else {
> +		ret = pm_runtime_put(&client->dev);
> +		if (ret == 0)
> +			ar0521_power_off(sensor);
> +	}
> +
> +	mutex_unlock(&sensor->lock);
> +	return ret;
> +}
> +
> +static int ar0521_enum_mbus_code(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state,
> +				 struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> +	if (code->index)
> +		return -EINVAL;
> +
> +	code->code = sensor->fmt.code;
> +	dev_dbg(&sensor->i2c_client->dev, "%s() = %X\n", __func__, code->code);
> +	return 0;
> +}
> +
> +static int ar0521_g_frame_interval(struct v4l2_subdev *sd, struct v4l2_subdev_frame_interval *fi)
> +{
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> +	mutex_lock(&sensor->lock);
> +	fi->interval = sensor->current_frame_interval;
> +	mutex_unlock(&sensor->lock);
> +	dev_dbg(&sensor->i2c_client->dev, "%s() = %u/%u\n", __func__,
> +		fi->interval.numerator, fi->interval.denominator);
> +	return 0;
> +}
> +
> +static int ar0521_s_frame_interval(struct v4l2_subdev *sd, struct v4l2_subdev_frame_interval *fi)
> +{
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +	int ret;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s(%u/%u)\n", __func__,
> +		fi->interval.numerator, fi->interval.denominator);
> +	mutex_lock(&sensor->lock);
> +
> +	if (sensor->streaming) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	sensor->frame_interval = fi->interval;
> +	ar0521_calc_mode(sensor);
> +	ret = ar0521_write_mode(sensor);
> +out:
> +	mutex_unlock(&sensor->lock);
> +	return ret;
> +}
> +
> +static int ar0521_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +	int ret;
> +
> +	dev_dbg(&sensor->i2c_client->dev, "%s(%i)\n", __func__, enable);
> +	mutex_lock(&sensor->lock);
> +
> +	ret = ar0521_set_stream(sensor, enable);
> +	if (!ret)
> +		sensor->streaming = enable;
> +
> +	mutex_unlock(&sensor->lock);
> +	return ret;
> +}
> +
> +static const struct v4l2_subdev_core_ops ar0521_core_ops = {
> +	.log_status = v4l2_ctrl_subdev_log_status,
> +	.s_power = ar0521_s_power,
> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,

I easily get lost when it's about event, but they do not seem to be
supported by the driver

> +};
> +
> +static const struct v4l2_subdev_video_ops ar0521_video_ops = {
> +	.g_frame_interval = ar0521_g_frame_interval,
> +	.s_frame_interval = ar0521_s_frame_interval,
> +	.s_stream = ar0521_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops ar0521_pad_ops = {
> +	.enum_mbus_code = ar0521_enum_mbus_code,
> +	.get_fmt = ar0521_get_fmt,
> +	.set_fmt = ar0521_set_fmt,
> +};
> +
> +static const struct v4l2_subdev_ops ar0521_subdev_ops = {
> +	.core = &ar0521_core_ops,
> +	.video = &ar0521_video_ops,
> +	.pad = &ar0521_pad_ops,
> +};
> +
> +static int __maybe_unused ar0521_suspend(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> +	if (sensor->streaming)
> +		ar0521_set_stream(sensor, 0);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ar0521_resume(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> +	if (sensor->streaming)
> +		return ar0521_set_stream(sensor, 1);
> +
> +	return 0;
> +}

Shouldn't suspend/resume do what your s_power does instead of just
stopping/starting the streaming ?

> +
> +static int ar0521_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	struct v4l2_fwnode_endpoint ep = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY
> +	};
> +	struct device *dev = &client->dev;
> +	struct fwnode_handle *endpoint;
> +	struct ar0521_dev *sensor;
> +	unsigned int cnt;
> +	int ret;
> +
> +	dev_dbg(dev, "%s()\n", __func__);
> +	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> +	if (!sensor)
> +		return -ENOMEM;
> +
> +	sensor->i2c_client = client;
> +	sensor->fmt.code = MEDIA_BUS_FMT_SGRBG8_1X8;
> +	sensor->fmt.width = AR0521_WIDTH_MAX;
> +	sensor->fmt.height = AR0521_HEIGHT_MAX;
> +	sensor->fmt.field = V4L2_FIELD_NONE;
> +	sensor->frame_interval.numerator = 30;
> +	sensor->frame_interval.denominator = 1;

isn't this what adj_fmt() does ?

> +
> +	endpoint = fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), NULL);
> +	if (!endpoint) {
> +		dev_err(dev, "endpoint node not found\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = v4l2_fwnode_endpoint_parse(endpoint, &ep);
> +	fwnode_handle_put(endpoint);
> +	if (ret) {
> +		dev_err(dev, "could not parse endpoint\n");
> +		return ret;
> +	}
> +
> +	if (ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
> +		dev_err(dev, "invalid bus type, must be MIPI CSI2\n");
> +		return -EINVAL;
> +	}
> +
> +	sensor->lane_count = ep.bus.mipi_csi2.num_data_lanes;
> +	switch (sensor->lane_count) {
> +	case 1:
> +	case 2:
> +	case 4:
> +		break;
> +	default:
> +		dev_err(dev, "invalid number of MIPI data lanes\n");
> +		return -EINVAL;
> +	}
> +
> +	// get master clock (extclk)
> +	sensor->extclk = devm_clk_get(dev, "extclk");
> +	if (IS_ERR(sensor->extclk)) {
> +		dev_err(dev, "failed to get extclk\n");
> +		return PTR_ERR(sensor->extclk);
> +	}
> +
> +	ret = clk_set_rate(sensor->extclk, AR0521_EXTCLK_RATE);
> +	if (ret < 0) {
> +		dev_err(dev, "error setting clock rate\n");
> +		return ret;
> +	}
> +
> +	sensor->extclk_freq = clk_get_rate(sensor->extclk);
> +
> +	if (sensor->extclk_freq < AR0521_EXTCLK_MIN ||
> +	    sensor->extclk_freq > AR0521_EXTCLK_MAX) {
> +		dev_err(dev, "extclk frequency out of range: %u Hz\n", sensor->extclk_freq);
> +		return -EINVAL;
> +	}
> +
> +	// request optional reset pin (usually active low) and assert it
> +	sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +
> +	v4l2_i2c_subdev_init(&sensor->sd, client, &ar0521_subdev_ops);
> +
> +	sensor->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> +	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> +	if (ret)
> +		return ret;
> +
> +	for (cnt = 0; cnt < AR0521_NUM_SUPPLIES; cnt++) {
> +		struct regulator *supply = devm_regulator_get(dev, ar0521_supply_names[cnt]);
> +
> +		if (IS_ERR(supply)) {
> +			dev_info(dev, "no %s regulator found: %li\n", ar0521_supply_names[cnt], PTR_ERR(supply));
> +			return PTR_ERR(supply);
> +		}
> +		sensor->supplies[cnt] = supply;
> +	}
> +
> +	mutex_init(&sensor->lock);
> +
> +	ret = ar0521_init_controls(sensor);
> +	if (ret)
> +		goto entity_cleanup;
> +
> +	ar0521_adj_fmt(&sensor->fmt);
> +
> +	ret = v4l2_async_register_subdev(&sensor->sd);
> +	if (ret)
> +		goto free_ctrls;
> +
> +	// Enable runtime PM and turn off the device
> +	pm_runtime_set_active(&client->dev);
> +	pm_runtime_enable(&client->dev);
> +	pm_runtime_idle(&client->dev);
> +	dev_dbg(dev, "AR0521 driver initialized, master clock frequency: %u MHz, %u MIPI data lanes\n",
> +		sensor->extclk_freq, sensor->lane_count);
> +	return 0;
> +
> +free_ctrls:
> +	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> +entity_cleanup:
> +	media_entity_cleanup(&sensor->sd.entity);
> +	mutex_destroy(&sensor->lock);
> +	return ret;
> +}
> +
> +static int ar0521_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> +
> +	v4l2_async_unregister_subdev(&sensor->sd);
> +	media_entity_cleanup(&sensor->sd.entity);
> +	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);

set_suspended() then disable maybe ?

Thank you again for your contribution!
   j

> +	mutex_destroy(&sensor->lock);
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops ar0521_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(ar0521_suspend, ar0521_resume)
> +};
> +static const struct of_device_id ar0521_dt_ids[] = {
> +	{.compatible = "onnn,ar0521"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ar0521_dt_ids);
> +
> +static struct i2c_driver ar0521_i2c_driver = {
> +	.driver = {
> +		.name  = "ar0521",
> +		.pm = &ar0521_pm_ops,
> +		.of_match_table	= ar0521_dt_ids,
> +	},
> +	.probe    = ar0521_probe,
> +	.remove   = ar0521_remove,
> +};
> +
> +module_i2c_driver(ar0521_i2c_driver);
> +
> +MODULE_DESCRIPTION("AR0521 MIPI Camera subdev driver");
> +MODULE_AUTHOR("Krzysztof Hałasa <khalasa@piap.pl>");
> +MODULE_LICENSE("GPL v2");
>
> --
> Krzysztof "Chris" Hałasa
>
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Randy Dunlap]

On 10/9/21 2:07 AM, Jacopo Mondi wrote:
> Hi Krzysztof,
> 
>     I've been testing this driver in the last few days, thanks for your
> effort in upstreaming it!
> 
> I'll separately comment on what I had to change to have it working for
> my use case, but let me continue the discussion from where it was left
> pending here to add my 2 cents.
> 
> On Thu, Oct 07, 2021 at 11:11:09AM +0200, Krzysztof Hałasa wrote:
>> Hi Sakari,
>>
>> Thanks for your input.
>>
>>> Where's the corresponding DT binding patch? Ideally it would be part of the
>>> same set.
>>
>> Well I've sent it a moment before this one. Will make them a set next
>> time.
>>
>>>> +#define AR0521_WIDTH_BLANKING_MIN     572u
>>>> +#define AR0521_HEIGHT_BLANKING_MIN     28u // must be even
>>>
>>> Please use /* */ for comments. The SPDX tag is an exception.
>>
>> As far as I know, this is no longer the case, the C99 comments are now
>> permitted and maybe even encouraged. Or was I dreaming?
>>
>> checkpatch doesn't protest either.
> 
> To my understanding the C99 standard added support for the //
> commenting style and tollerate them, but they're still from C++ and I
> see very few places where they're used in the kernel, and per as far I
> know they're still not allowed by the coding style
> https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting

http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html

Maybe we should update coding-style then.

> 
> Looking at how you used comments in the driver I think you could get
> rid of most // comments easily, the register tables might be an
> exception but I would really try to remove them from there as well.
> 
> 
>>
>>> Please wrap your lines at 80 or earlier, unless a sound reason exists to do
>>> otherwise.
>>
>> This limitation appears to be lifted as well, after all those years.
>> Is there a specific reason to still use it here? Yes, lines longer than
>> 80 chars make the code much more readable (for my eyes, at least).
>> Yes, I know there is some "soft" limit, and I trim lines when it makes
>> them better in my opinion.
>>
> 
> In my personal opinion lifting that restriction caused more pain than
> anything, as different subsystem are now imposing different
> requirements. Here everything has been so far pretty strict about
> going over 80-cols, but I think there are situation where it makes
> sense in example
> 

[snip]

> 
> My suggestion is: aim to 80 cols whenever possible, if it forces you
> to do things like the above shown function declaration you can go a
> little over that

Yes, 80 max is still preferred. Up to 100 may be tolerable in some
cases.

> As reported here
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> if you go over 100 you should ask yourself what are you doing :)




-- 
~Randy

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Sakari Ailus]

Hi Randy, others,

On Sat, Oct 09, 2021 at 01:18:50PM -0700, Randy Dunlap wrote:
> On 10/9/21 2:07 AM, Jacopo Mondi wrote:
> > Hi Krzysztof,
> > 
> >     I've been testing this driver in the last few days, thanks for your
> > effort in upstreaming it!
> > 
> > I'll separately comment on what I had to change to have it working for
> > my use case, but let me continue the discussion from where it was left
> > pending here to add my 2 cents.
> > 
> > On Thu, Oct 07, 2021 at 11:11:09AM +0200, Krzysztof Hałasa wrote:
> > > Hi Sakari,
> > > 
> > > Thanks for your input.
> > > 
> > > > Where's the corresponding DT binding patch? Ideally it would be part of the
> > > > same set.
> > > 
> > > Well I've sent it a moment before this one. Will make them a set next
> > > time.
> > > 
> > > > > +#define AR0521_WIDTH_BLANKING_MIN     572u
> > > > > +#define AR0521_HEIGHT_BLANKING_MIN     28u // must be even
> > > > 
> > > > Please use /* */ for comments. The SPDX tag is an exception.
> > > 
> > > As far as I know, this is no longer the case, the C99 comments are now
> > > permitted and maybe even encouraged. Or was I dreaming?
> > > 
> > > checkpatch doesn't protest either.
> > 
> > To my understanding the C99 standard added support for the //
> > commenting style and tollerate them, but they're still from C++ and I
> > see very few places where they're used in the kernel, and per as far I
> > know they're still not allowed by the coding style
> > https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
> 
> http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html
> 
> Maybe we should update coding-style then.

That's not really a statement for C++ (//) comments as it's against other
/* ... */ multi-line comment styles some of which are used by the
networking stack.

I haven't read all the discussion but I see coding-style.rst still
documents a different multi-line comment style for the network stack.

What comes to // comments, almost all the cases that exist in the kernel
currently outside SPDX tags are either adjacent copyright notices or the
AMD DRM driver. Based on this, I'd rather not adopt that style in a sensor
driver.

-- 
Regards,

Sakari Ailus

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

Hi Jacopo,

Thanks for your input.

Jacopo Mondi <jacopo@jmondi.org> writes:

> To my understanding the C99 standard added support for the //
> commenting style and tollerate them, but they're still from C++

Sure. Not everything coming from C++ is bad.

> and I
> see very few places where they're used in the kernel,

It's not going to change if no one uses //.

> and per as far I
> know they're still not allowed by the coding style
> https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting

As Randy wrote, perhaps we need to bring the coding-style up to date?

> Looking at how you used comments in the driver I think you could get
> rid of most // comments easily, the register tables might be an
> exception but I would really try to remove them from there as well.

I could. The question is "why?" IMHO the C++ style is (in places I use
it) better than the /* */. Why should I use the worse thing?

> In my personal opinion lifting that restriction caused more pain than
> anything, as different subsystem are now imposing different
> requirements.

I think it was always the restriction causing more harm than good.
It's not like the "spirit" behind it was wrong - no. The oversided lines
SHOULD be avoided. It was the hard limit which was wrong: a) the limit
itself (80) was definitely inadequate, and b) the hard limit should have
never existed. 8-character tabs only made this worse (e.g. I use 4-chars
tabs outside the kernel).

This is all about readability, right? Hard rules don't play well with
it.

Things like:
                                       fst_tx_dma(card,
                                                   card->tx_dma_handle_card,
                                                   BUF_OFFSET(txBuffer[pi]
                                                              [port->txpos][0]),
                                                   skb->len);
Is this better, isn't it?
However I do realize my opinion may be a bit distorted since I have some
vision problems.

> 	ret = ar0521_write_regs(sensor, pixel_timing_recommended, ARRAY_SIZE(pixel_timing_recommended));
> 	if (ret)
> 		goto off;
>
>
> should be
>
> 	ret = ar0521_write_regs(sensor, pixel_timing_recommended,
>                                 ARRAY_SIZE(pixel_timing_recommended));
> 	if (ret)
> 		goto off;

Do you consider the second one BETTER? I definitely don't (though it
this case the difference is small). If it's worse, why should I use it?

Also, in such cases I try to align the arguments (ARRAY_SIZE right below
sensor). Still IMHO worse than #1.

> if you go over 100 you should ask yourself what are you doing :)

I do. Sometimes the answer is I'm doing the right thing :-)
And sometimes I change the code. You won't see it because it's already
changed.

> The sensor frame rate is configured by userspace by changing the
> blankings through the V4L2_CID_[VH]BLANK.
>
> You are right the current definition is akward to work with, as there
> is no way to set the 'total pixels' like you have suggested, but it's
> rather userspace that knowing the desired total sizes has to compute
> the blankings by subtracting the visible sizes (plus the mandatory min
> blanking sizes).

But it can't do that, can it?
This could be adequate for a sensor with fixed pixel clock. Here we can
control pixel clocks at will, how is the driver going to know what pixel
clock should it use? Also, the "extra delay" can't be set with
V4L2_CID_[VH]BLANK, it needs interval-based timings or the "total pixel"
or something alike.
-- 
Krzysztof "Chris" Hałasa

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

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

Sakari Ailus <sakari.ailus@iki.fi> writes:

>> http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html
>
> That's not really a statement for C++ (//) comments as it's against other
> /* ... */ multi-line comment styles some of which are used by the
> networking stack.

It appears it's about both multi-line and single-line comments, though:
(c)
// This can be a single line. Or many. Your choice.
                 ^^^^^^^^^^^
-- 
Krzysztof "Chris" Hałasa

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

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Laurent Pinchart]

Hi Krzysztof,

On Mon, Oct 11, 2021 at 08:20:32AM +0200, Krzysztof Hałasa wrote:
> Hi Jacopo,
> 
> Thanks for your input.
> 
> Jacopo Mondi <jacopo@jmondi.org> writes:
> 
> > To my understanding the C99 standard added support for the //
> > commenting style and tollerate them, but they're still from C++
> 
> Sure. Not everything coming from C++ is bad.
> 
> > and I
> > see very few places where they're used in the kernel,
> 
> It's not going to change if no one uses //.
> 
> > and per as far I
> > know they're still not allowed by the coding style
> > https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
> 
> As Randy wrote, perhaps we need to bring the coding-style up to date?
> 
> > Looking at how you used comments in the driver I think you could get
> > rid of most // comments easily, the register tables might be an
> > exception but I would really try to remove them from there as well.
> 
> I could. The question is "why?" IMHO the C++ style is (in places I use
> it) better than the /* */. Why should I use the worse thing?

It's also a matter of consistency, to try and unify the coding style
across similar drivers in a subsytem. In this case, the media system
frowns upon C++-style comments.

> > In my personal opinion lifting that restriction caused more pain than
> > anything, as different subsystem are now imposing different
> > requirements.
> 
> I think it was always the restriction causing more harm than good.
> It's not like the "spirit" behind it was wrong - no. The oversided lines
> SHOULD be avoided. It was the hard limit which was wrong: a) the limit
> itself (80) was definitely inadequate, and b) the hard limit should have
> never existed. 8-character tabs only made this worse (e.g. I use 4-chars
> tabs outside the kernel).
> 
> This is all about readability, right? Hard rules don't play well with
> it.
> 
> Things like:
>                                        fst_tx_dma(card,
>                                                    card->tx_dma_handle_card,
>                                                    BUF_OFFSET(txBuffer[pi]
>                                                               [port->txpos][0]),
>                                                    skb->len);
> Is this better, isn't it?
> However I do realize my opinion may be a bit distorted since I have some
> vision problems.
> 
> > 	ret = ar0521_write_regs(sensor, pixel_timing_recommended, ARRAY_SIZE(pixel_timing_recommended));
> > 	if (ret)
> > 		goto off;
> >
> >
> > should be
> >
> > 	ret = ar0521_write_regs(sensor, pixel_timing_recommended,
> >                                 ARRAY_SIZE(pixel_timing_recommended));
> > 	if (ret)
> > 		goto off;
> 
> Do you consider the second one BETTER? I definitely don't (though it
> this case the difference is small). If it's worse, why should I use it?

I find the second option much more readable, yes.

Code is written once and read often, so you should consider the coding
style in use in the subsystem.

> Also, in such cases I try to align the arguments (ARRAY_SIZE right below
> sensor). Still IMHO worse than #1.
> 
> > if you go over 100 you should ask yourself what are you doing :)
> 
> I do. Sometimes the answer is I'm doing the right thing :-)
> And sometimes I change the code. You won't see it because it's already
> changed.
> 
> > The sensor frame rate is configured by userspace by changing the
> > blankings through the V4L2_CID_[VH]BLANK.
> >
> > You are right the current definition is akward to work with, as there
> > is no way to set the 'total pixels' like you have suggested, but it's
> > rather userspace that knowing the desired total sizes has to compute
> > the blankings by subtracting the visible sizes (plus the mandatory min
> > blanking sizes).
> 
> But it can't do that, can it?
> This could be adequate for a sensor with fixed pixel clock. Here we can
> control pixel clocks at will, how is the driver going to know what pixel
> clock should it use? Also, the "extra delay" can't be set with
> V4L2_CID_[VH]BLANK, it needs interval-based timings or the "total pixel"
> or something alike.

Additional controls may be needed, I haven't studied this particular
sensor in details, but in general frame rate is controlled explicitly
through low-level parameters for raw sensors, which means controlling
h/v blank and possibly the pixel clock from userspace. The
.g_frame_interval() and .s_frame_interval() operations are deprecated
(and should never have been used) for raw sensors.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

Jacopo Mondi <jacopo@jmondi.org> writes:

>> The driver has been extensively tested in an i.MX6-based system.
>
> That's a give for code submitted for inclusions, right ? right ??? :)

Of course not :-)

Untested code is submitted - and merged - all the time.
For example, you can't test the code if you don't have access to the
respective hardware.

IMO this is right. The change should be applied if it's an overall
improvement (statistically), otherwise the development would stall.

It means someone has to fix possible fallout, sure. I've done such fixes
many times (as I have some quite unique hw), and I still think it's
right.

> Which frame sizes have you tested it with ? I know from the code the
> driver supports a single mode, but if you want to add these
> information to the commit message I would report it.

The driver is quite flexible, I was using mostly modes like 2560x1440
and 2560x1920, but not only these. p25 rather than p30, though.

>> +#define AR0521_EXTCLK_RATE	  (27 * 1000 * 1000)
>
> You try to set the external clock to this frequency, but your PLL code
> does not rely on this value being actually set, am I wrong ? So why
> would you try to change it in first place ?

Change from what?
This is used to clk_set_rate(sensor->extclk, AR0521_EXTCLK_RATE);
Do you want me to skip this step? What clock (if any) would I get then?

> Be consistent with comments starting with capital or lowercase
> letters. It's usually "start with capital letter, end with full stop."

I try to make them as good as I can, instead of blindly following
anything, but I will review the comments of course.

>> +#define AR0521_TOTAL_WIDTH_MIN	     2968u
>
> Isn't this TOTAL_WIDTH_MAX ?

No:
total_width = max(sensor->fmt.width + AR0521_WIDTH_BLANKING_MIN, AR0521_TOTAL_WIDTH_MIN);
But this needs verifying.

> You won't like this as it will require quite some changes, but we use
> lowercase letters for hexadecimal values.

Define "we" :-)
For me, uppercase hex digits are more visible. Perhaps because they are
full height? Sight problems maybe.

>> +#define   AR0521_REG_RESET_DEFAULTS		  0x0238
>
> By default BIT(5) is 0. Should this be 0x0218 ?

reset_lpf_enable
To prevent reset from POR glitch, enable digital low pass filter.

Why do think it should be 0?

>> +	struct v4l2_mbus_framefmt fmt;
>> +	struct v4l2_fract frame_interval, current_frame_interval;
>
> As a general rule one variable per line. Even more so in structures
> declaration.

Why?

>> +static int ar0521_write_regs(struct ar0521_dev *sensor, const __be16 *data, unsigned int count)
>> +{
>> +	struct i2c_client *client = sensor->i2c_client;
>> +	struct i2c_msg msg;
>> +	int ret;
>> +
>> +	if (!pm_runtime_get_if_in_use(&client->dev))
>> +		return 0;
>
> Oh, in my previous email I commented looking at v4 probably, not v5.
>
> Anyway, I feel this check should really be in the caller.

There are multiple callers - the code would get more complicated which
is bad.

> Also, does this cause a power up/down sequence for every transaction ?

No. Why would it?
If I get this right (and the tests support it), this fragment would
return if not in use (= not powered up).

> You could write them one register at the time so that you could
[]
> at the expense of one transaction per register write (like you do when
> you write initial_reg[]). Sounds like a little price to pay for a
> nicer driver but I might be underestimating it (or being to concerned
> about the use of be() everywhere :)

Even on otherwise idle I2C bus this would add unnecessary latency.
I prefer to have a much faster code, even if using that be() macro
(which isn't ugly IMHO).
For example, at 100 kb/s, the most common rate, transferring the
pixel_timing_recommended[] only should normally take about 50 ms
(depends on the controller and if it adds any/long inter-byte delays).
With single transfers, instead of 16-bit per registers, you have to send
start, devsel, 16-bit address, 16-bit value and stop. This could easily
mean 150 ms total (much more with slow controller). Do we want to add
100 ms of latency only to get rid of the "ugly" macro? I don't think so.

On a busy bus (or e.g. "realtime"), the short writes would be
a disaster (and even with the long ones it isn't obvious).

> I had to add
>
> 	__be16 timings[] = {
> 		be(0x3002),	be(y),
> 		/* 0x3004 */	be(x),
> 		/* 0x3006 */	be(y + sensor->fmt.height - 1),
> 		/* 0x3008 */	be(x + sensor->fmt.width - 1),
> 		/* 0x300a */	be(sensor->total_height),
> 		/* Documented as 'twice the number of pixel clocks in one row' */
> 		/* 0x300c */	be(2 * sensor->total_width),
> 	};
> 	ar0521_write_regs(sensor, timings, ARRAY_SIZE(timings));
>
> To this function to be able to get images out from the sensor.

Interesting. I don't write them via 0x3002 but use 0x340 (the primary
ones) instead, in set_geometry(). But then, the total_width, perhaps
I calculate it incorrectly (the 2x factor) and thus it doesn't work in
certain modes. I don't remember why it's done the ways it is, will have
to look at it.
I understand what you use is 1920x1080p30, right?

> Also, be careful about register 0x300c which you don't seem to program
> at the moment.

I do, it's register 0x342.

> And it seems the integration time depends on this register value
>
>         - CIT = 0x3012
>         - LLPCK = 1/2 × reg_300C
>         - Integration Time = (CIT × LLPCK) / pix_clk
>
> Are you able to control exposure properly with your version ?

Definitely.

>> +static int ar0521_set_gains(struct ar0521_dev *sensor)
>> +{
>> +	int green = sensor->ctrls.gain->val;
>> +	int red = max(green + sensor->ctrls.red_balance->val, 0);
>> +	int blue = max(green + sensor->ctrls.blue_balance->val, 0);
>
> unsigned ?

The ctrls.* (R and B) can be negative. I'm not sure mixing types here
would be the right thing (I'd have to check with the C standard, and
anyone reading this later would probably need it as well).
With signed ints it's simply correct.

>> +	unsigned int gain = min(red, min(green, blue));
>> +	unsigned int analog = min(gain, 64u); // range is 0 - 127
>
> Nit: when possible declare variables in reverse xmas-tree order

Not very possible here, right? Unless you want to split into
declarations and calculations. Readability IMHO suffers.

>> +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
>> +		/* 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
>> +		/* 0x30A */ be(1)  // op_sys_clk_div
>> +	};
>> +	u32 num = sensor->current_frame_interval.numerator;
>> +	u32 denom = sensor->current_frame_interval.denominator;
>> +	int ret;
>> +
>> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
>
> tracepoints debug are not that useful if they do not report what has
> been written, aren't they ?

This is only to see what the upper layer is doing. I have already
removed (moved to private commit) the I2C write debug routine. Out of
sight, out of mind :-(

> I comment here but that's mostly about the exposure control
> definition. You initialize it with a value of 1, which means 1 line of
> exposure which result in very dark images. I know userspace should be
> in control of this, but a more sensible default value should probably
> be used. Do you have a 'default' mode ? Could you set the default
> exposure to something a bit larger (I know it's hard to define what a
> sensible value could be, but 1 line is certainly very small)

What do you propose?
I'm using automatic exposure, the application reads the whole image and
decides.

>> +	ret = ar0521_write_reg(sensor, AR0521_REG_RESET, AR0521_REG_RESET_DEFAULTS | AR0521_REG_RESET_STREAM);

> This part I don't get. As far as I can see and can read, setting the
> AR0521_REG_RESET_STREAM bit in AR0521_REG_RESET_DEFAULTS as the effect
> of:
>
> Setting this bit places the sensor in streaming mode.
> Clearing this bit places the sensor in a low power mode. The result of clearing
> this bit depends upon the operating mode of the sensor. Entry and exit from
> streaming mode can also be controlled from the signal interface
>
> I would have expected to see this bit set/cleared at s_stream() time
> (I've done so and things work better, otherwise I get crippled
> images).

This is needed for operation with i.MX6 CSI. Apparently the i.MX6
doesn't like the low power state of the sensor, it needs the LP-11 (low
power with both LVDS lines high) on all data and clock lanes before
the stream is started.

When you disable streaming, the clock lanes are apparently in LP-11
state, however data lanes are LP-00.

>> +	ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE, sensor->ctrls.test_pattern->val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_dbg(&sensor->i2c_client->dev,
>> +		"AR0521: %ux%u, total %ux%u, pixel clock %u MHz, %u (%u/%u) FPS\n",
>> +		sensor->fmt.width, sensor->fmt.height, sensor->total_width, sensor->total_height,
>> +		sensor->pix_clk, (num + denom / 2) / denom, num, denom);
>
> Empty line before return statements

Why?

>> +		// normal output on clock and data lanes
>> +		ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS, 0);
>
> This register controls the HiSPI interface while my understanding is
> that everything is MIPI CSI-2 in the rest of the driver. Why is it
> here ? does it play any role in your setup ?

Yes, it also controls MIPI mode.

>> +	// start streaming (possibly with LP-11 on all lines)
>
> So you fall down here even in the case s_stream(0) ?
>
>> +	return ar0521_write_reg(sensor, AR0521_REG_RESET,
>> +				AR0521_REG_RESET_DEFAULTS |
>> +				AR0521_REG_RESET_RESTART |
>> +				AR0521_REG_RESET_STREAM);
>> +}

That's correct. It's the only way I have found.

> Before introducing 0x300c I got 30 FPS but the frame content was
> mangled (or completely black)

This could be due to the line_length_pck/line_length_pck_ (0x342/0x300C)
register. I will look at it.
TBH I've written this driver in 2017 (didn't have time for the whole
submission process back then, not that the situation changed much).
This means I will have to check the details.

>> +static int ar0521_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state,
>> +			  struct v4l2_subdev_format *format)
>> +{
>> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
>> +	int ret = 0;
>> +
>> +	dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, format->which);
>> +
>> +	ar0521_adj_fmt(&format->format);
>> +
>> +	mutex_lock(&sensor->lock);
>> +
>> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>> +		struct v4l2_mbus_framefmt *fmt;
>> +
>> +		fmt = v4l2_subdev_get_try_format(sd, sd_state, 0 /* pad */);
>> +		*fmt = format->format;
>> +	} else {
>> +		sensor->fmt = format->format;
>> +		ar0521_calc_mode(sensor);
>> +		ret = ar0521_write_mode(sensor);
>
> Do you need to do so ? the mode is programmed at s_stream() time,
> isn't it enough ? Same for the other call to write_mode() above or
> set_geometry() below.

I wonder if you can call set_fmt() etc. when the sensor is streaming.
Why not?

>> +	// 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, -512, 511, 1, 0);
>> +	ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BLUE_BALANCE, -512, 511, 1, 0);
>
> Seems like you handle these together, should these be clusters ? Same
> for the blankings

An example maybe? A file, existing driver maybe?

>> +	// alternate for frame interval
>> +	ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, AR0521_WIDTH_BLANKING_MIN, 4094, 1, AR0521_WIDTH_BLANKING_MIN);
>> +	ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, AR0521_HEIGHT_BLANKING_MIN, 4094, 2, AR0521_HEIGHT_BLANKING_MIN);
>
> How nicer would this be in 80-cols ? :)

Depends on one's display window :-)
I can change these ones I guess.

>> +static __be16 pixel_timing_recommended[] = {
>
> Have you got any idea what these do ? I have these registers not
> documented.

Unfortunately I know nothing about these.

>> +	// set MIPI test mode - disabled for now
>> +	ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_TEST_MODE,
>> +			       ((0x40 << sensor->lane_count) - 0x40) | AR0521_REG_HISPI_TEST_MODE_LP11);
>
> As far as I can tell this write has no effect.

That's right :-)
It will have its effect later, when the AR0521_REG_HISPI_CONTROL_STATUS
is written.

> Define test mode to be applied to mipi/ccp interface if test_en is asserted
>
> and asfaict you never assert test_en

I do, it's called
AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE and it simply
won't work on i.MX6 without this.

>> +	if (ret)
>> +		goto off;
>> +
>> +	ret = ar0521_write_reg(sensor, AR0521_REG_ROW_SPEED, 0x110 | 4 / sensor->lane_count);
>
> I wasn't able to interpret this register right
>
> pc_speed
> Slows down the internal pixel clock frequency relative to the system
> clock frequency.  A programmed value of N gives a pixel clock period
> of N system clocks.  Only values 1, 2 and 4 are supported.
>
> Shouldn't this be part of the PLL calculation ?

Well, this doesn't seem to be part of the PLL, and it's constant (as
long as MIPI lane# is constant). Why do you think it belongs there?

You know, I try not to throw unnecessary traffic at I2C bus, too.

>> +static const struct v4l2_subdev_core_ops ar0521_core_ops = {
>> +	.log_status = v4l2_ctrl_subdev_log_status,
>> +	.s_power = ar0521_s_power,
>> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
>> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
>
> I easily get lost when it's about event, but they do not seem to be
> supported by the driver

TBH, I don't know. It's what other driver(s) do.

>> +static int __maybe_unused ar0521_suspend(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
>> +
>> +	if (sensor->streaming)
>> +		ar0521_set_stream(sensor, 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused ar0521_resume(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
>> +
>> +	if (sensor->streaming)
>> +		return ar0521_set_stream(sensor, 1);
>> +
>> +	return 0;
>> +}
>
> Shouldn't suspend/resume do what your s_power does instead of just
> stopping/starting the streaming ?

That's what imx219 driver does. TBH I don't know how is it all supposed
to work. The system I'm using this with has no real suspend/resume
capability. Sakari has suggested that I look at imx219 and so I did.

>> +	sensor->i2c_client = client;
>> +	sensor->fmt.code = MEDIA_BUS_FMT_SGRBG8_1X8;
>> +	sensor->fmt.width = AR0521_WIDTH_MAX;
>> +	sensor->fmt.height = AR0521_HEIGHT_MAX;
>> +	sensor->fmt.field = V4L2_FIELD_NONE;
>> +	sensor->frame_interval.numerator = 30;
>> +	sensor->frame_interval.denominator = 1;
>
> isn't this what adj_fmt() does ?

Partially, it is.

>> +static int ar0521_remove(struct i2c_client *client)
>> +{
>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
>> +
>> +	v4l2_async_unregister_subdev(&sensor->sd);
>> +	media_entity_cleanup(&sensor->sd.entity);
>> +	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
>> +	pm_runtime_disable(&client->dev);
>> +	pm_runtime_set_suspended(&client->dev);
>
> set_suspended() then disable maybe ?

Other drivers seem to do it the above way but I don't know the
difference.
-- 
Krzysztof "Chris" Hałasa

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

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Jacopo Mondi]

Hi Krzysztof,

On Mon, Oct 11, 2021 at 02:19:53PM +0200, Krzysztof Hałasa wrote:
> Jacopo Mondi <jacopo@jmondi.org> writes:
>
> >> The driver has been extensively tested in an i.MX6-based system.
> >
> > That's a give for code submitted for inclusions, right ? right ??? :)
>
> Of course not :-)
>
> Untested code is submitted - and merged - all the time.
> For example, you can't test the code if you don't have access to the
> respective hardware.
>
> IMO this is right. The change should be applied if it's an overall
> improvement (statistically), otherwise the development would stall.
>
> It means someone has to fix possible fallout, sure. I've done such fixes
> many times (as I have some quite unique hw), and I still think it's
> right.
>

I'll pretend I haven't read this not to be dragged in tangential
discussions. My whole point was such a commit message is useless as I
assume if you submit a driver for inclusion you have tested it.

> > Which frame sizes have you tested it with ? I know from the code the
> > driver supports a single mode, but if you want to add these
> > information to the commit message I would report it.
>
> The driver is quite flexible, I was using mostly modes like 2560x1440
> and 2560x1920, but not only these. p25 rather than p30, though.
>

That's more useful to know

> >> +#define AR0521_EXTCLK_RATE	  (27 * 1000 * 1000)
> >
> > You try to set the external clock to this frequency, but your PLL code
> > does not rely on this value being actually set, am I wrong ? So why
> > would you try to change it in first place ?
>
> Change from what?
> This is used to clk_set_rate(sensor->extclk, AR0521_EXTCLK_RATE);
> Do you want me to skip this step? What clock (if any) would I get then?
>

I think the clock rate should be assigned in DTS and you should verify
it falls in the supported clock ranges.

> > Be consistent with comments starting with capital or lowercase
> > letters. It's usually "start with capital letter, end with full stop."
>
> I try to make them as good as I can, instead of blindly following
> anything, but I will review the comments of course.
>
> >> +#define AR0521_TOTAL_WIDTH_MIN	     2968u
> >
> > Isn't this TOTAL_WIDTH_MAX ?
>
> No:
> total_width = max(sensor->fmt.width + AR0521_WIDTH_BLANKING_MIN, AR0521_TOTAL_WIDTH_MIN);
> But this needs verifying.

There's usually a minimum amount of blankings to be respected, not a
total, even less if the sensor capable of producing smaller modes
through subsampling. Is this sensor different ? I haven't found any
mention in the datasheet.

>
> > You won't like this as it will require quite some changes, but we use
> > lowercase letters for hexadecimal values.
>
> Define "we" :-)
> For me, uppercase hex digits are more visible. Perhaps because they are
> full height? Sight problems maybe.

Everyone has preferences. I do have mine, and they DON'T MATTER when I
submit code for inclusion.

"We" is me reporting what I've been told in these years from people
that spent their time reviewing my code and making sure the result is
consistent among the whole subsystem. The style of this driver is
totally alien to any convention in place here, and if you can ignore
my comments as I'm just a random idiot on the internet, you have been
told the same by the other people that cared enough to look into your
code.

It all really sounds like "it's better my way, please don't annoy me".
I don't find this a productive way to operate in a cooperative space :)

>
> >> +#define   AR0521_REG_RESET_DEFAULTS		  0x0238
> >
> > By default BIT(5) is 0. Should this be 0x0218 ?
>
> reset_lpf_enable
> To prevent reset from POR glitch, enable digital low pass filter.
>
> Why do think it should be 0?
>

Because it's 0 by default ?
Although seems like a desirable feature.

I would have defined BIT(5) separately and enabled it explicitly to
make it visible, but I understand it's not required.

> >> +	struct v4l2_mbus_framefmt fmt;
> >> +	struct v4l2_fract frame_interval, current_frame_interval;
> >
> > As a general rule one variable per line. Even more so in structures
> > declaration.
>
> Why?
>

/(=.=)\

> >> +static int ar0521_write_regs(struct ar0521_dev *sensor, const __be16 *data, unsigned int count)
> >> +{
> >> +	struct i2c_client *client = sensor->i2c_client;
> >> +	struct i2c_msg msg;
> >> +	int ret;
> >> +
> >> +	if (!pm_runtime_get_if_in_use(&client->dev))
> >> +		return 0;
> >
> > Oh, in my previous email I commented looking at v4 probably, not v5.
> >
> > Anyway, I feel this check should really be in the caller.
>
> There are multiple callers - the code would get more complicated which
> is bad.
>

I think the contrary, but ok.

> > Also, does this cause a power up/down sequence for every transaction ?
>
> No. Why would it?
> If I get this right (and the tests support it), this fragment would
> return if not in use (= not powered up).

You're right I missed the 'if_in_use()' part

>
> > You could write them one register at the time so that you could
> []
> > at the expense of one transaction per register write (like you do when
> > you write initial_reg[]). Sounds like a little price to pay for a
> > nicer driver but I might be underestimating it (or being to concerned
> > about the use of be() everywhere :)
>
> Even on otherwise idle I2C bus this would add unnecessary latency.
> I prefer to have a much faster code, even if using that be() macro
> (which isn't ugly IMHO).

'be()' is bad as it is only used in this driver, modifying the
register tables requires to handle the be() macro. I had tested this
by moving register tables to and from a different driver and I had to
manually insert the be() thing and remove it on the other way around.

For one that writes code there are tens that reads, use and modify it.
And you should be mostly concerned about them.

> For example, at 100 kb/s, the most common rate, transferring the
> pixel_timing_recommended[] only should normally take about 50 ms
> (depends on the controller and if it adds any/long inter-byte delays).
> With single transfers, instead of 16-bit per registers, you have to send
> start, devsel, 16-bit address, 16-bit value and stop. This could easily
> mean 150 ms total (much more with slow controller). Do we want to add
> 100 ms of latency only to get rid of the "ugly" macro? I don't think so.
>
> On a busy bus (or e.g. "realtime"), the short writes would be
> a disaster (and even with the long ones it isn't obvious).

The long table writes should be done once at power-up time, not in any
hot path. The number or registers that should written at s_stream()
time (the only hot path here) is minimal, so I personally would not
care.

That said, if the efficiency argument holds for others, feel free to
keep it the way it is.

>
> > I had to add
> >
> > 	__be16 timings[] = {
> > 		be(0x3002),	be(y),
> > 		/* 0x3004 */	be(x),
> > 		/* 0x3006 */	be(y + sensor->fmt.height - 1),
> > 		/* 0x3008 */	be(x + sensor->fmt.width - 1),
> > 		/* 0x300a */	be(sensor->total_height),
> > 		/* Documented as 'twice the number of pixel clocks in one row' */
> > 		/* 0x300c */	be(2 * sensor->total_width),
> > 	};
> > 	ar0521_write_regs(sensor, timings, ARRAY_SIZE(timings));
> >
> > To this function to be able to get images out from the sensor.
>
> Interesting. I don't write them via 0x3002 but use 0x340 (the primary
> ones) instead, in set_geometry(). But then, the total_width, perhaps
> I calculate it incorrectly (the 2x factor) and thus it doesn't work in
> certain modes. I don't remember why it's done the ways it is, will have
> to look at it.
> I understand what you use is 1920x1080p30, right?

I tested with that yes, I can try remove the already programmed totals
and see if it's only 0x300c that makes a difference

>
> > Also, be careful about register 0x300c which you don't seem to program
> > at the moment.
>
> I do, it's register 0x342.

Do you know why registers are mirrored ? I haven't find any reference
to register shadowing in the manual.

>
> > And it seems the integration time depends on this register value
> >
> >         - CIT = 0x3012
> >         - LLPCK = 1/2 × reg_300C
> >         - Integration Time = (CIT × LLPCK) / pix_clk
> >
> > Are you able to control exposure properly with your version ?
>
> Definitely.
>
> >> +static int ar0521_set_gains(struct ar0521_dev *sensor)
> >> +{
> >> +	int green = sensor->ctrls.gain->val;
> >> +	int red = max(green + sensor->ctrls.red_balance->val, 0);
> >> +	int blue = max(green + sensor->ctrls.blue_balance->val, 0);
> >
> > unsigned ?
>
> The ctrls.* (R and B) can be negative. I'm not sure mixing types here
> would be the right thing (I'd have to check with the C standard, and
> anyone reading this later would probably need it as well).
> With signed ints it's simply correct.

Ah, I missed the v4l2 ctrls definition.

>
> >> +	unsigned int gain = min(red, min(green, blue));
> >> +	unsigned int analog = min(gain, 64u); // range is 0 - 127
> >
> > Nit: when possible declare variables in reverse xmas-tree order
>
> Not very possible here, right? Unless you want to split into
> declarations and calculations. Readability IMHO suffers.

Not here, but in general it is.

>
> >> +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
> >> +		/* 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
> >> +		/* 0x30A */ be(1)  // op_sys_clk_div
> >> +	};
> >> +	u32 num = sensor->current_frame_interval.numerator;
> >> +	u32 denom = sensor->current_frame_interval.denominator;
> >> +	int ret;
> >> +
> >> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
> >
> > tracepoints debug are not that useful if they do not report what has
> > been written, aren't they ?
>
> This is only to see what the upper layer is doing. I have already
> removed (moved to private commit) the I2C write debug routine. Out of
> sight, out of mind :-(
>
> > I comment here but that's mostly about the exposure control
> > definition. You initialize it with a value of 1, which means 1 line of
> > exposure which result in very dark images. I know userspace should be
> > in control of this, but a more sensible default value should probably
> > be used. Do you have a 'default' mode ? Could you set the default
> > exposure to something a bit larger (I know it's hard to define what a
> > sensible value could be, but 1 line is certainly very small)
>
> What do you propose?

Being this a raw sensor, it's probably fine to assume exposure will be
manually adjusted later. I'll defer this to maintainers.

> I'm using automatic exposure, the application reads the whole image and
> decides.
>
> >> +	ret = ar0521_write_reg(sensor, AR0521_REG_RESET, AR0521_REG_RESET_DEFAULTS | AR0521_REG_RESET_STREAM);
>
> > This part I don't get. As far as I can see and can read, setting the
> > AR0521_REG_RESET_STREAM bit in AR0521_REG_RESET_DEFAULTS as the effect
> > of:
> >
> > Setting this bit places the sensor in streaming mode.
> > Clearing this bit places the sensor in a low power mode. The result of clearing
> > this bit depends upon the operating mode of the sensor. Entry and exit from
> > streaming mode can also be controlled from the signal interface
> >
> > I would have expected to see this bit set/cleared at s_stream() time
> > (I've done so and things work better, otherwise I get crippled
> > images).
>
> This is needed for operation with i.MX6 CSI. Apparently the i.MX6
> doesn't like the low power state of the sensor, it needs the LP-11 (low
> power with both LVDS lines high) on all data and clock lanes before
> the stream is started.
>
> When you disable streaming, the clock lanes are apparently in LP-11
> state, however data lanes are LP-00.
>

I see. Reading this and your below reply I get you need to enable test
mode, force the lines in LP-11 state and then disable the test mode
and start streaming. Feels like an ack, but maybe that's how the
sensor is intended to be operated.

I anyway think doing this here is not the best idea. "We" have a
pre_streamon() operation but it's also usually done at s_stream(0)
which you can call at device node open time to coax the lines in
LP-11. You already do most of these things, so I would move the test
mode handling to s_stream(0) and start/stop stream by just toggling
the RESET_STREAM bit as suggested in the s_stream() code snippet I've
shared in my previous reply.

> >> +	ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE, sensor->ctrls.test_pattern->val);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	dev_dbg(&sensor->i2c_client->dev,
> >> +		"AR0521: %ux%u, total %ux%u, pixel clock %u MHz, %u (%u/%u) FPS\n",
> >> +		sensor->fmt.width, sensor->fmt.height, sensor->total_width, sensor->total_height,
> >> +		sensor->pix_clk, (num + denom / 2) / denom, num, denom);
> >
> > Empty line before return statements
>
> Why?
>
> >> +		// normal output on clock and data lanes
> >> +		ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS, 0);
> >
> > This register controls the HiSPI interface while my understanding is
> > that everything is MIPI CSI-2 in the rest of the driver. Why is it
> > here ? does it play any role in your setup ?
>
> Yes, it also controls MIPI mode.
>

Maybe by accident, as a consequence enabling/disabling the test modes on
the CSI-2 lanes ?

> >> +	// start streaming (possibly with LP-11 on all lines)
> >
> > So you fall down here even in the case s_stream(0) ?
> >
> >> +	return ar0521_write_reg(sensor, AR0521_REG_RESET,
> >> +				AR0521_REG_RESET_DEFAULTS |
> >> +				AR0521_REG_RESET_RESTART |
> >> +				AR0521_REG_RESET_STREAM);
> >> +}
>
> That's correct. It's the only way I have found.
>

I think it's wrong. RESTART as per its definition interrupts the frame
and re starts the stream. START enables/disable streaming. You set
them both, even at s_stream(0) time, and things might work by
accident because you enable/disable the test mode at s_stream() time.

As suggested I would rather coax the data lanes in LP-11 state at
s_stream(0) time by enabling test mode after having disabled streaming
by clearing the RESET_STREAM bit in AR0521_REG_RESET. Similarly at
s_stream(1) time I would disable the test mode (I bet it could be done
by clearing bits [9:6] in 0x3066 without touching the HiSpi control
register) and the start streaming by setting RESET_STREAM.


> > Before introducing 0x300c I got 30 FPS but the frame content was
> > mangled (or completely black)
>
> This could be due to the line_length_pck/line_length_pck_ (0x342/0x300C)
> register. I will look at it.
> TBH I've written this driver in 2017 (didn't have time for the whole
> submission process back then, not that the situation changed much).
> This means I will have to check the details.

Thanks, I will run more experiments as well.

>
> >> +static int ar0521_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state,
> >> +			  struct v4l2_subdev_format *format)
> >> +{
> >> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> >> +	int ret = 0;
> >> +
> >> +	dev_dbg(&sensor->i2c_client->dev, "%s(%u)\n", __func__, format->which);
> >> +
> >> +	ar0521_adj_fmt(&format->format);
> >> +
> >> +	mutex_lock(&sensor->lock);
> >> +
> >> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> >> +		struct v4l2_mbus_framefmt *fmt;
> >> +
> >> +		fmt = v4l2_subdev_get_try_format(sd, sd_state, 0 /* pad */);
> >> +		*fmt = format->format;
> >> +	} else {
> >> +		sensor->fmt = format->format;
> >> +		ar0521_calc_mode(sensor);
> >> +		ret = ar0521_write_mode(sensor);
> >
> > Do you need to do so ? the mode is programmed at s_stream() time,
> > isn't it enough ? Same for the other call to write_mode() above or
> > set_geometry() below.
>
> I wonder if you can call set_fmt() etc. when the sensor is streaming.
> Why not?

Because
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-subdev-g-fmt.html?highlight=subdev_s_fmt#c.V4L.VIDIOC_SUBDEV_S_FMT

EBUSY
The format can’t be changed because the pad is currently busy. This
can be caused, for instance, by an active video stream on the pad. The
ioctl must not be retried without performing another action to fix the
problem first. Only returned by VIDIOC_SUBDEV_S_FMT

>
> >> +	// 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, -512, 511, 1, 0);
> >> +	ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BLUE_BALANCE, -512, 511, 1, 0);
> >
> > Seems like you handle these together, should these be clusters ? Same
> > for the blankings
>
> An example maybe? A file, existing driver maybe?

$ git grep v4l2_ctrl_cluster drivers/media/i2c/ | wc -l
17

>
> >> +	// alternate for frame interval
> >> +	ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, AR0521_WIDTH_BLANKING_MIN, 4094, 1, AR0521_WIDTH_BLANKING_MIN);
> >> +	ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, AR0521_HEIGHT_BLANKING_MIN, 4094, 2, AR0521_HEIGHT_BLANKING_MIN);
> >
> > How nicer would this be in 80-cols ? :)
>
> Depends on one's display window :-)
> I can change these ones I guess.
>
> >> +static __be16 pixel_timing_recommended[] = {
> >
> > Have you got any idea what these do ? I have these registers not
> > documented.
>
> Unfortunately I know nothing about these.
>
> >> +	// set MIPI test mode - disabled for now
> >> +	ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_TEST_MODE,
> >> +			       ((0x40 << sensor->lane_count) - 0x40) | AR0521_REG_HISPI_TEST_MODE_LP11);
> >
> > As far as I can tell this write has no effect.
>
> That's right :-)
> It will have its effect later, when the AR0521_REG_HISPI_CONTROL_STATUS
> is written.
>
> > Define test mode to be applied to mipi/ccp interface if test_en is asserted
> >
> > and asfaict you never assert test_en
>
> I do, it's called
> AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE and it simply
> won't work on i.MX6 without this.
>
> >> +	if (ret)
> >> +		goto off;
> >> +
> >> +	ret = ar0521_write_reg(sensor, AR0521_REG_ROW_SPEED, 0x110 | 4 / sensor->lane_count);
> >
> > I wasn't able to interpret this register right
> >
> > pc_speed
> > Slows down the internal pixel clock frequency relative to the system
> > clock frequency.  A programmed value of N gives a pixel clock period
> > of N system clocks.  Only values 1, 2 and 4 are supported.
> >
> > Shouldn't this be part of the PLL calculation ?
>
> Well, this doesn't seem to be part of the PLL, and it's constant (as
> long as MIPI lane# is constant). Why do you think it belongs there?

Because seems like a clock downscaler intended to be used to adapt the CSI-2
clock frequency (lane dependent) to the pixel clock and could be
programmed once.

Also note that the number of data lanes in use can actually be modified at
run time but it's not something that has to be considered here.

>
> You know, I try not to throw unnecessary traffic at I2C bus, too.

So this could moved to the register tables ? :)

>
> >> +static const struct v4l2_subdev_core_ops ar0521_core_ops = {
> >> +	.log_status = v4l2_ctrl_subdev_log_status,
> >> +	.s_power = ar0521_s_power,
> >> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> >> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> >
> > I easily get lost when it's about event, but they do not seem to be
> > supported by the driver
>
> TBH, I don't know. It's what other driver(s) do.
>

i think you can remove any reference to events

> >> +static int __maybe_unused ar0521_suspend(struct device *dev)
> >> +{
> >> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> >> +
> >> +	if (sensor->streaming)
> >> +		ar0521_set_stream(sensor, 0);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int __maybe_unused ar0521_resume(struct device *dev)
> >> +{
> >> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> >> +
> >> +	if (sensor->streaming)
> >> +		return ar0521_set_stream(sensor, 1);
> >> +
> >> +	return 0;
> >> +}
> >
> > Shouldn't suspend/resume do what your s_power does instead of just
> > stopping/starting the streaming ?
>
> That's what imx219 driver does. TBH I don't know how is it all supposed
> to work. The system I'm using this with has no real suspend/resume
> capability. Sakari has suggested that I look at imx219 and so I did.
>

I would move the content of s_power() here, let's see what Sakari
thinks

> >> +	sensor->i2c_client = client;
> >> +	sensor->fmt.code = MEDIA_BUS_FMT_SGRBG8_1X8;
> >> +	sensor->fmt.width = AR0521_WIDTH_MAX;
> >> +	sensor->fmt.height = AR0521_HEIGHT_MAX;
> >> +	sensor->fmt.field = V4L2_FIELD_NONE;
> >> +	sensor->frame_interval.numerator = 30;
> >> +	sensor->frame_interval.denominator = 1;
> >
> > isn't this what adj_fmt() does ?
>
> Partially, it is.
>

So please reuse it instead of open-coding this.
You should probably also support init_cfg as you expose a subdev video
device and you can have an init_format() or similar to be called there
for the try format and called here for the active format.

> >> +static int ar0521_remove(struct i2c_client *client)
> >> +{
> >> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> >> +
> >> +	v4l2_async_unregister_subdev(&sensor->sd);
> >> +	media_entity_cleanup(&sensor->sd.entity);
> >> +	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> >> +	pm_runtime_disable(&client->dev);
> >> +	pm_runtime_set_suspended(&client->dev);
> >
> > set_suspended() then disable maybe ?
>
> Other drivers seem to do it the above way but I don't know the
> difference.

Maybe I'm wrong but calling set_suspend() after pm_runtime() had been
disabled seems pointless. A minor anyway as it's in the driver's
remove function.

Thanks
   j

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

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Daniel Scally]

Hi Jacopo

On 11/10/2021 15:34, Jacopo Mondi wrote:
>>>> +static int ar0521_remove(struct i2c_client *client)
>>>> +{
>>>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>>> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
>>>> +
>>>> +	v4l2_async_unregister_subdev(&sensor->sd);
>>>> +	media_entity_cleanup(&sensor->sd.entity);
>>>> +	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
>>>> +	pm_runtime_disable(&client->dev);
>>>> +	pm_runtime_set_suspended(&client->dev);
>>> set_suspended() then disable maybe ?
>> Other drivers seem to do it the above way but I don't know the
>> difference.
> Maybe I'm wrong but calling set_suspend() after pm_runtime() had been
> disabled seems pointless. A minor anyway as it's in the driver's
> remove function.
>

fwiw, the kernel doc [1] for pm_runtime_set_suspended() does say that
it's not valid to call it for devices where runtime PM is still enabled.


[1]
https://elixir.bootlin.com/linux/latest/source/include/linux/pm_runtime.h#L510

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Jacopo Mondi]

Hi Daniel,

On Mon, Oct 11, 2021 at 11:22:10PM +0100, Daniel Scally wrote:
> Hi Jacopo
>
> On 11/10/2021 15:34, Jacopo Mondi wrote:
> >>>> +static int ar0521_remove(struct i2c_client *client)
> >>>> +{
> >>>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >>>> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
> >>>> +
> >>>> +	v4l2_async_unregister_subdev(&sensor->sd);
> >>>> +	media_entity_cleanup(&sensor->sd.entity);
> >>>> +	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> >>>> +	pm_runtime_disable(&client->dev);
> >>>> +	pm_runtime_set_suspended(&client->dev);
> >>> set_suspended() then disable maybe ?
> >> Other drivers seem to do it the above way but I don't know the
> >> difference.
> > Maybe I'm wrong but calling set_suspend() after pm_runtime() had been
> > disabled seems pointless. A minor anyway as it's in the driver's
> > remove function.
> >
>
> fwiw, the kernel doc [1] for pm_runtime_set_suspended() does say that
> it's not valid to call it for devices where runtime PM is still enabled.
>

Ah, great! thanks for pointing it out, it was very well visible in the
documentation :) Sorry for the noise!

Cheers
   j

>
> [1]
> https://elixir.bootlin.com/linux/latest/source/include/linux/pm_runtime.h#L510
>

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

Hi Laurent,

I understand it me who wants the code merged, and therefore I'm in the
lost position, but this is the matter of principia and thus I need to
voice my opposition :-)

>> I could. The question is "why?" IMHO the C++ style is (in places I use
>> it) better than the /* */. Why should I use the worse thing?
>
> It's also a matter of consistency, to try and unify the coding style
> across similar drivers in a subsytem. In this case, the media system
> frowns upon C++-style comments.

Ok. So it's better-worse vs consistent-different. IMHO, consistency is
a good thing, when you don't have to sacrifice quality. Things should be
consistently good (rather than consistent for the sake of it), no?

Perhaps you don't consider (these) old-C comments worse, though. I do,
for the simple reason, they use more space. I give up because it's
a small thing, but I shouldn't have to.

> Code is written once and read often, so you should consider the coding
> style in use in the subsystem.

I do. But... don't you think the "rules" go WAY too far?
We have the coding-style for consistency. IMHO it also goes too far from
time to time (and, as shown, I'm not exactly alone in this). But coding
style is based on some consensus, discussions etc. It follows the ever
changing needs (like evolution of display devices).
We have the checkpatch (imperfect, of course) which is meant to check
for potentially harmful deviations.

Now there is the "subsystem coding style". What is it based on? Who can
challenge it and how? Who defines it? Where is it defined so it can be
improved?

>> But it can't do that, can it?
>> This could be adequate for a sensor with fixed pixel clock. Here we can
>> control pixel clocks at will, how is the driver going to know what pixel
>> clock should it use? Also, the "extra delay" can't be set with
>> V4L2_CID_[VH]BLANK, it needs interval-based timings or the "total pixel"
>> or something alike.
>
> Additional controls may be needed, I haven't studied this particular
> sensor in details, but in general frame rate is controlled explicitly
> through low-level parameters for raw sensors, which means controlling
> h/v blank and possibly the pixel clock from userspace. The
> .g_frame_interval() and .s_frame_interval() operations are deprecated
> (and should never have been used) for raw sensors.

The problem is *interval() are currently the only way to accurately
control the timings. There is simply no other way - short of creating
private controls or changing the upper levels.
Now I don't say we shouldn't change the upper levels - but all I want
ATM is having the driver for a simple MIPI sensor merged, not fixing the
entire Universe. Trying to have the driver merged already takes a lot of
effort.

Sure, I can remove the *interval(), crippling the driver another bit.
Should I do it?
-- 
Krzysztof "Chris" Hałasa

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

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

Jacopo,

> I'll pretend I haven't read this not to be dragged in tangential
> discussions. My whole point was such a commit message is useless as I
> assume if you submit a driver for inclusion you have tested it.

Fine, I will remove that very commit comment.

> I think the clock rate should be assigned in DTS and you should verify
> it falls in the supported clock ranges.

Which driver does that, please?
clk_set_rate() appears to be used by multiple drivers, I was thinking
I can use it.
I can see clock frequencies in DTS, but they usually refer to fixed
clocks (e.g. crystal-based). The clock I use is IMX6QDL_CLK_CKO, some
sort of a shared generic clock from inside of the CPU. I don't know if
I could even set its frequency in DTS.

>> total_width = max(sensor->fmt.width + AR0521_WIDTH_BLANKING_MIN, AR0521_TOTAL_WIDTH_MIN);
>> But this needs verifying.
>
> There's usually a minimum amount of blankings to be respected, not a
> total, even less if the sensor capable of producing smaller modes
> through subsampling. Is this sensor different ? I haven't found any
> mention in the datasheet.

I got these values experimentally. But it was few years ago and I have
to look at them again.

> Everyone has preferences. I do have mine, and they DON'T MATTER when I
> submit code for inclusion.

This is a straw man argument, I don't force anyone to my prefs (like
e.g. 4-chars tabs). What we have here is a written coding-style
document, and documented amendments from high profile developers.
Now there are additional, informal and previously unknown to me
requirements, of unknown source, which I'm apparently required to
follow, so my questions shouldn't came as a surprise, should they?

> "We" is me reporting what I've been told in these years from people
> that spent their time reviewing my code and making sure the result is
> consistent among the whole subsystem. The style of this driver is
> totally alien to any convention in place here, and if you can ignore
> my comments as I'm just a random idiot on the internet, you have been
> told the same by the other people that cared enough to look into your
> code.

Well I must say I wasn't told that "style of this driver is totally
alien to any convention in place here" by anyone else yet.

> It all really sounds like "it's better my way, please don't annoy me".
> I don't find this a productive way to operate in a cooperative space
> :)

I have already changed a lot of stuff, based on the feedback by several
people, and will probably change a lot more. Is it really like you say?
I don't think so.
Now swap the seats and tell me that what you say isn't now true.

Think about it.

>> Even on otherwise idle I2C bus this would add unnecessary latency.
>> I prefer to have a much faster code, even if using that be() macro
>> (which isn't ugly IMHO).
>
> 'be()' is bad as it is only used in this driver, modifying the
> register tables requires to handle the be() macro. I had tested this
> by moving register tables to and from a different driver and I had to
> manually insert the be() thing and remove it on the other way around.
>
> For one that writes code there are tens that reads, use and modify it.
> And you should be mostly concerned about them.

I think I have to be most concerned about the operation of the driver.
What good is a nicely looking code if it doesn't work?
Also, I would think the author (me in this case) is the person who reads
his/her code most often, though obviously it may be much less that all
others combined.

I'd love to get rid of the be(), though. What do you propose instead?

> The long table writes should be done once at power-up time, not in any
> hot path.

Power-up isn't "once" anymore. It looks like it's to be done before
every start of capture operation (my device will not do that).

> That said, if the efficiency argument holds for others, feel free to
> keep it the way it is.

It's just how the 1982 hardware thing work.

> I tested with that yes, I can try remove the already programmed totals
> and see if it's only 0x300c that makes a difference

Just tested it and it works for me in 1920x1080p30 without any changes.
Would it be possible it's the gain/exposure settings? If not, what exact
clock frequency (for the chip) do you use?
(I haven't yet a chance to test this 0x300c register, but will do).

> Do you know why registers are mirrored ? I haven't find any reference
> to register shadowing in the manual.

I think the first set follows some standard, but it lacks something -
thus a new set. Or maybe it's how the chip was build from blocks - I
don't know.

>> When you disable streaming, the clock lanes are apparently in LP-11
>> state, however data lanes are LP-00.
>
> I see. Reading this and your below reply I get you need to enable test
> mode, force the lines in LP-11 state and then disable the test mode
> and start streaming. Feels like an ack, but maybe that's how the
> sensor is intended to be operated.

I don't think so. I think, in proprietary development, nobody cares
about what does the chip send while not streaming.

> I anyway think doing this here is not the best idea. "We" have a
> pre_streamon() operation but it's also usually done at s_stream(0)
> which you can call at device node open time to coax the lines in
> LP-11. You already do most of these things, so I would move the test
> mode handling to s_stream(0) and start/stop stream by just toggling
> the RESET_STREAM bit as suggested in the s_stream() code snippet I've
> shared in my previous reply.

I don't get it completely. However I see I could use the pre_stream()
to put the sensor in LP-11 mode (doing this in s_stream(0) would then be
pointless). I will see if it works.
RESET_STREAM must be already set to 1 (ACTIVE STREAMING) before
s_stream(1) is called.

>> > This register controls the HiSPI interface while my understanding is
>> > that everything is MIPI CSI-2 in the rest of the driver. Why is it
>> > here ? does it play any role in your setup ?
>>
>> Yes, it also controls MIPI mode.
>
> Maybe by accident, as a consequence enabling/disabling the test modes on
> the CSI-2 lanes ?

I guess it simply enables/disables the chosen test mode in the CSI-2
lanes :-)
I've chosen the test mode to be, well, LP-11 on all lines.

>> >> +	return ar0521_write_reg(sensor, AR0521_REG_RESET,
>> >> +				AR0521_REG_RESET_DEFAULTS |
>> >> +				AR0521_REG_RESET_RESTART |
>> >> +				AR0521_REG_RESET_STREAM);
>> >> +}
>>
>> That's correct. It's the only way I have found.
>
> I think it's wrong. RESTART as per its definition interrupts the frame
> and re starts the stream. START enables/disable streaming. You set
> them both, even at s_stream(0) time, and things might work by
> accident because you enable/disable the test mode at s_stream() time.
>
> As suggested I would rather coax the data lanes in LP-11 state at
> s_stream(0) time by enabling test mode after having disabled streaming
> by clearing the RESET_STREAM bit in AR0521_REG_RESET.

How would you do that?
If you disable streaming, LP-11 is gone.
You need STREAMING to actually "stream" LP-11.

> Similarly at
> s_stream(1) time I would disable the test mode (I bet it could be done
> by clearing bits [9:6] in 0x3066 without touching the HiSpi control
> register) and the start streaming by setting RESET_STREAM.

You lost your bet :-)
Clearing these bits disables test on data lanes only.
To disable clock lane test you have to clear 0x31C6.
This is what I do in s_stream(1):

                // normal output on clock and data lanes
                ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS, 0);

0x31C6 = AR0521_REG_HISPI_CONTROL_STATUS which is a misnomer and perhaps
I should call it AR0521_REG_MIPI_AND_HISPI_CONTROL_STATUS.

>> I wonder if you can call set_fmt() etc. when the sensor is streaming.
>> Why not?
>
> Because
> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-subdev-g-fmt.html?highlight=subdev_s_fmt#c.V4L.VIDIOC_SUBDEV_S_FMT
>
> EBUSY
> The format can’t be changed because the pad is currently busy. This
> can be caused, for instance, by an active video stream on the pad. The
> ioctl must not be retried without performing another action to fix the
> problem first. Only returned by VIDIOC_SUBDEV_S_FMT

But it doesn't say EBUSY MUST BE returned when the sensor is streaming,
only that it MAY BE returned. Looking at the code, I can see nothing
forcing the EBUSY (subdev_do_ioctl(VIDIOC_SUBDEV_S_FMT) ->
v4l2_subdev_call(set_fmt)). It also appears several drivers update
registers in their set_fmt(), so I think it's the desirable action
there.

>> >> +	ret = ar0521_write_reg(sensor, AR0521_REG_ROW_SPEED, 0x110 | 4 / sensor->lane_count);
>> >
>> > I wasn't able to interpret this register right
>> >
>> > pc_speed
>> > Slows down the internal pixel clock frequency relative to the system
>> > clock frequency.  A programmed value of N gives a pixel clock period
>> > of N system clocks.  Only values 1, 2 and 4 are supported.
>> >
>> > Shouldn't this be part of the PLL calculation ?
>>
>> Well, this doesn't seem to be part of the PLL, and it's constant (as
>> long as MIPI lane# is constant). Why do you think it belongs there?
>
> Because seems like a clock downscaler intended to be used to adapt the CSI-2
> clock frequency (lane dependent) to the pixel clock and could be
> programmed once.

Exactly. This "programmed once" (per power-up) is exactly why I set it
in ar0521_power_on() instead of calc_pll().
First, calc_pll is calculations-only, no hw access (suggested by Laurent
IIRC, and it plays well). Second, calc_pll() is called multiple times.

> Also note that the number of data lanes in use can actually be modified at
> run time but it's not something that has to be considered here.

Yes, #lanes is constant here, though variable lane count could be
useful - for tests, and in my other (unpublished) experiments. I don't
know if it was possible back then, when I wrote the driver.

[initial_regs]

>> You know, I try not to throw unnecessary traffic at I2C bus, too.
>
> So this could moved to the register tables ? :)

The truth is, I've considered doing exactly that. But they're quite
fragmented (pixel_timing_recommended is linear) and well, perhaps I
didn't want to complicate code too much. I may still do it, though.
Guess I should.

> i think you can remove any reference to events

Good :-)

> You should probably also support init_cfg as you expose a subdev video
> device and you can have an init_format() or similar to be called there
> for the try format and called here for the active format.

Will have a look.

> Maybe I'm wrong but calling set_suspend() after pm_runtime() had been
> disabled seems pointless. A minor anyway as it's in the driver's
> remove function.

Daniel already posted a comment about this, however the whole
pm_runtime*() in this driver is a bit fragile and I'd welcome someone
who know how does this PM stuff work looking at it...
... with the following quick patch applied on top of v5:

-	pm_runtime_idle(&client->dev);
+	if (pm_runtime_idle(&client->dev) == -ENOSYS) {
+		ret = ar0521_power_on(sensor);
+		if (ret)
+			goto disable;
+	}
 	dev_dbg(dev, "AR0521 driver initialized, master clock frequency: %u MHz, %u MIPI data lanes\n",
 		sensor->extclk_freq, sensor->lane_count);
 	return 0;
 
+disable:
+	v4l2_async_unregister_subdev(&sensor->sd);
+	media_entity_cleanup(&sensor->sd.entity);
 free_ctrls:
 	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
 entity_cleanup:

(Power is only needed after probe()).

I'll post v6 eventually, of course, but have to squeeze this in
somewhere.
-- 
Krzysztof "Chris" Hałasa

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

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Sakari Ailus]

Hi Krzysztof,

On Tue, Oct 12, 2021 at 02:24:17PM +0200, Krzysztof Hałasa wrote:
> Jacopo,
> 
> > I'll pretend I haven't read this not to be dragged in tangential
> > discussions. My whole point was such a commit message is useless as I
> > assume if you submit a driver for inclusion you have tested it.
> 
> Fine, I will remove that very commit comment.
> 
> > I think the clock rate should be assigned in DTS and you should verify
> > it falls in the supported clock ranges.
> 
> Which driver does that, please?
> clk_set_rate() appears to be used by multiple drivers, I was thinking
> I can use it.
> I can see clock frequencies in DTS, but they usually refer to fixed
> clocks (e.g. crystal-based). The clock I use is IMX6QDL_CLK_CKO, some
> sort of a shared generic clock from inside of the CPU. I don't know if
> I could even set its frequency in DTS.

Please see:

	https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html

Generally camera sensor drivers that set the clock in drivers themselves
are (very) old.

-- 
Regards,

Sakari Ailus

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

Hi Sakari,

> 	https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html

Ok:
"8.2.2. Devicetree

The currently preferred way to achieve this is using assigned-clocks,
assigned-clock-parents and assigned-clock-rates properties. See
Documentation/devicetree/bindings/clock/clock-bindings.txt for more
information. The driver then gets the frequency using clk_get_rate()."

Let's see:
Documentation/devicetree/bindings/clock/clock-bindings.txt:

"==Assigned clock parents and rates==

Some platforms may require initial configuration of default parent clocks
and clock frequencies. Such a configuration can be specified in a device tree
node through assigned-clocks, assigned-clock-parents and assigned-clock-rates
properties. The assigned-clock-parents property should contain a list of parent
clocks in the form of a phandle and clock specifier pair and the
assigned-clock-rates property should contain a list of frequencies in Hz. Both
these properties should correspond to the clocks listed in the assigned-clocks
property."

So I'm after "assigned-clock-rates", right?

"Configuring a clock's parent and rate through the device node that consumes
the clock can be done only for clocks that have a single user. Specifying
conflicting parent or rate configuration in multiple consumer nodes for
a shared clock is forbidden."

This sounds a bit problematic, the clock I use is at least potentially
shared by multiple parts of the system, depending on current (run time)
configuration. I am/was getting different frequencies depending of the
particular system (all based on the same i.MX6* SoC, but with different
peripherals used/enabled). I think it's quite a common situation.

> Generally camera sensor drivers that set the clock in drivers themselves
> are (very) old.

Let's have a look... ov9282 is (one of) the newest drivers. It does:
#define OV9282_INCLK_RATE    24000000

        /* Get sensor input clock */
        ov9282->inclk = devm_clk_get(ov9282->dev, NULL);
        if (IS_ERR(ov9282->inclk)) {
                dev_err(ov9282->dev, "could not get inclk");
                return PTR_ERR(ov9282->inclk);
        }

        rate = clk_get_rate(ov9282->inclk);
        if (rate != OV9282_INCLK_RATE) {
                dev_err(ov9282->dev, "inclk frequency mismatch");
                return -EINVAL;
        }

$ git grep -l ov9282
Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml
MAINTAINERS
drivers/media/i2c/Kconfig
drivers/media/i2c/Makefile
drivers/media/i2c/ov9282.c

  clocks:
    description: Clock frequency from 6 to 27MHz

No in-tree DTS exists, but the single frequency (both in the driver -
this one can be fixed - and in the DTS) is rather limiting. Maybe
another:

imx412, imx335, imx334, imx258 - same here.
imx208 is ACPI-based.

Which driver should I consult?
-- 
Krzysztof "Chris" Hałasa

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

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Jacopo Mondi]

Hi Krzysztof

On Tue, Oct 12, 2021 at 02:24:17PM +0200, Krzysztof Hałasa wrote:
> Jacopo,
>
> > I'll pretend I haven't read this not to be dragged in tangential
> > discussions. My whole point was such a commit message is useless as I
> > assume if you submit a driver for inclusion you have tested it.
>
> Fine, I will remove that very commit comment.
>
> > I think the clock rate should be assigned in DTS and you should verify
> > it falls in the supported clock ranges.
>
> Which driver does that, please?
> clk_set_rate() appears to be used by multiple drivers, I was thinking
> I can use it.
> I can see clock frequencies in DTS, but they usually refer to fixed
> clocks (e.g. crystal-based). The clock I use is IMX6QDL_CLK_CKO, some
> sort of a shared generic clock from inside of the CPU. I don't know if
> I could even set its frequency in DTS.
>
> >> total_width = max(sensor->fmt.width + AR0521_WIDTH_BLANKING_MIN, AR0521_TOTAL_WIDTH_MIN);
> >> But this needs verifying.
> >
> > There's usually a minimum amount of blankings to be respected, not a
> > total, even less if the sensor capable of producing smaller modes
> > through subsampling. Is this sensor different ? I haven't found any
> > mention in the datasheet.
>
> I got these values experimentally. But it was few years ago and I have
> to look at them again.
>
> > Everyone has preferences. I do have mine, and they DON'T MATTER when I
> > submit code for inclusion.
>
> This is a straw man argument, I don't force anyone to my prefs (like
> e.g. 4-chars tabs). What we have here is a written coding-style
> document, and documented amendments from high profile developers.
> Now there are additional, informal and previously unknown to me
> requirements, of unknown source, which I'm apparently required to
> follow, so my questions shouldn't came as a surprise, should they?
>

You have never been told before, while submitting code to Linux, not
to use C++ comments ? Are you surprised someone contests that ?

> > "We" is me reporting what I've been told in these years from people
> > that spent their time reviewing my code and making sure the result is
> > consistent among the whole subsystem. The style of this driver is
> > totally alien to any convention in place here, and if you can ignore
> > my comments as I'm just a random idiot on the internet, you have been
> > told the same by the other people that cared enough to look into your
> > code.
>
> Well I must say I wasn't told that "style of this driver is totally
> alien to any convention in place here" by anyone else yet.
>

It is.

No driver in media (which I'm aware of) uses C++ comments.
Your one is entirely commented with C++ comments.

They all try to stay in the 80-cols limit.
Yours have lines that span to 140 cols and goes regularly over 100.

Lowercase hex is preferred (based on the review I read on the list)
but not enforced, I give you that.

> > It all really sounds like "it's better my way, please don't annoy me".
> > I don't find this a productive way to operate in a cooperative space
> > :)
>
> I have already changed a lot of stuff, based on the feedback by several
> people, and will probably change a lot more. Is it really like you say?
> I don't think so.
> Now swap the seats and tell me that what you say isn't now true.
>
> Think about it.
>

Ok, I give up then, feels like a waste of time reviewing a driver
(for the only sake of code consistency) and have every single comment
contested.

I don't have interest in blocking this driver, the contrary, if it's
fine for everyone else the way it is. Let me just point you to

Documentation/driver-api/media/maintainer-entry-profile.rst

and has there suggested have the patch go through
./scripts/checkpatch.pl --strict --max-line-length=80

Then I'll stop bothering about style and I'll let maintainers decide
what to pick up.

> >> Even on otherwise idle I2C bus this would add unnecessary latency.
> >> I prefer to have a much faster code, even if using that be() macro
> >> (which isn't ugly IMHO).
> >
> > 'be()' is bad as it is only used in this driver, modifying the
> > register tables requires to handle the be() macro. I had tested this
> > by moving register tables to and from a different driver and I had to
> > manually insert the be() thing and remove it on the other way around.
> >
> > For one that writes code there are tens that reads, use and modify it.
> > And you should be mostly concerned about them.
>
> I think I have to be most concerned about the operation of the driver.
> What good is a nicely looking code if it doesn't work?
> Also, I would think the author (me in this case) is the person who reads
> his/her code most often, though obviously it may be much less that all
> others combined.
>
> I'd love to get rid of the be(), though. What do you propose instead?
>

Mode based sensor drivers usually rely on long register tables, whose
writing is an expensive operation to be done at streamon time. Power
up is usually done at devnode open time but you relay on the legacy
s_power() here, so it's in control of the receiver driver which
depending on the implemenation might call it at open() time or stream
on. Sorry, I didn't notice that, has you register a devnode I assumed
you had an open() function, which you don't.

You have two tables of registers:
- the initial settings (70 register) that you write in pairs (which
  means 280 bytes on the bus)
- recommended timings (216 registers) that you write in a single i2c
  transaction. A third of those registers is 0x0000 (no be() is
  needed).

There are more or less 15 other occurrences of be(), which are for
registers writes at streamon time (timings, pll and gains mostly).

The efficiency argument holds as long as we are in an hot path and I
understand writing 216 registers in pairs has an overhead which to
me, at open devnode open time is marginal, but if done at streamon time
should be avoided.

Please keep the suggested timings table declared as big endian in
place then. I would not use be() to write the handful of registers for
pll and gains at runtime, but up to you.

> > The long table writes should be done once at power-up time, not in any
> > hot path.
>
> Power-up isn't "once" anymore. It looks like it's to be done before
> every start of capture operation (my device will not do that).
>

As said, as your driver registers a subdev devnode, but also support
operations with a devnode-centric receivers, you're not in control of
when s_power() is called.

> > That said, if the efficiency argument holds for others, feel free to
> > keep it the way it is.
>
> It's just how the 1982 hardware thing work.
>
> > I tested with that yes, I can try remove the already programmed totals
> > and see if it's only 0x300c that makes a difference
>
> Just tested it and it works for me in 1920x1080p30 without any changes.
> Would it be possible it's the gain/exposure settings? If not, what exact
> clock frequency (for the chip) do you use?

24Mhz

> (I haven't yet a chance to test this 0x300c register, but will do).

I tested it again, and programming the right 0x300c (or the mirror
register) makes a difference between having completely dark images or
good ones.

Also I got mangled frames before I changed the streamon sequence to
what I've shared. But I need to verify that again as the receiver code
was moving too.

>
> > Do you know why registers are mirrored ? I haven't find any reference
> > to register shadowing in the manual.
>
> I think the first set follows some standard, but it lacks something -
> thus a new set. Or maybe it's how the chip was build from blocks - I
> don't know.
>

The difference is that the 0x3xxx ones are frame synchronized and
apply to 'bad frames' too.

> >> When you disable streaming, the clock lanes are apparently in LP-11
> >> state, however data lanes are LP-00.
> >
> > I see. Reading this and your below reply I get you need to enable test
> > mode, force the lines in LP-11 state and then disable the test mode
> > and start streaming. Feels like an ack, but maybe that's how the
> > sensor is intended to be operated.
>
> I don't think so. I think, in proprietary development, nobody cares
> about what does the chip send while not streaming.
>

afaict only imx6 has this check enforced (but I might be wrong)

> > I anyway think doing this here is not the best idea. "We" have a
> > pre_streamon() operation but it's also usually done at s_stream(0)
> > which you can call at device node open time to coax the lines in
> > LP-11. You already do most of these things, so I would move the test
> > mode handling to s_stream(0) and start/stop stream by just toggling
> > the RESET_STREAM bit as suggested in the s_stream() code snippet I've
> > shared in my previous reply.
>
> I don't get it completely. However I see I could use the pre_stream()
> to put the sensor in LP-11 mode (doing this in s_stream(0) would then be
> pointless). I will see if it works.

Be aware that relying on pre_streamon() requires the receiver driver
to be instrumented to call the operation. Doing that at s_stream(0)
time works with all receivers.

> RESET_STREAM must be already set to 1 (ACTIVE STREAMING) before
> s_stream(1) is called.
>
> >> > This register controls the HiSPI interface while my understanding is
> >> > that everything is MIPI CSI-2 in the rest of the driver. Why is it
> >> > here ? does it play any role in your setup ?
> >>
> >> Yes, it also controls MIPI mode.
> >
> > Maybe by accident, as a consequence enabling/disabling the test modes on
> > the CSI-2 lanes ?
>
> I guess it simply enables/disables the chosen test mode in the CSI-2
> lanes :-)
> I've chosen the test mode to be, well, LP-11 on all lines.
>
> >> >> +	return ar0521_write_reg(sensor, AR0521_REG_RESET,
> >> >> +				AR0521_REG_RESET_DEFAULTS |
> >> >> +				AR0521_REG_RESET_RESTART |
> >> >> +				AR0521_REG_RESET_STREAM);
> >> >> +}
> >>
> >> That's correct. It's the only way I have found.
> >
> > I think it's wrong. RESTART as per its definition interrupts the frame
> > and re starts the stream. START enables/disable streaming. You set
> > them both, even at s_stream(0) time, and things might work by
> > accident because you enable/disable the test mode at s_stream() time.
> >
> > As suggested I would rather coax the data lanes in LP-11 state at
> > s_stream(0) time by enabling test mode after having disabled streaming
> > by clearing the RESET_STREAM bit in AR0521_REG_RESET.
>
> How would you do that?
> If you disable streaming, LP-11 is gone.
> You need STREAMING to actually "stream" LP-11.
>

Even for test mode ? So for you streamoff is:
- Enable test mode (programmed to be LP-11)
- Start stream
?

Anyway, should the AR0521_REG_RESET_RESTART bit be dropped ?

> > Similarly at
> > s_stream(1) time I would disable the test mode (I bet it could be done
> > by clearing bits [9:6] in 0x3066 without touching the HiSpi control
> > register) and the start streaming by setting RESET_STREAM.
>
> You lost your bet :-)
> Clearing these bits disables test on data lanes only.

Ack

> To disable clock lane test you have to clear 0x31C6.
> This is what I do in s_stream(1):
>
>                 // normal output on clock and data lanes
>                 ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS, 0);
>
> 0x31C6 = AR0521_REG_HISPI_CONTROL_STATUS which is a misnomer and perhaps
> I should call it AR0521_REG_MIPI_AND_HISPI_CONTROL_STATUS.
>

I don't have a way to test LP-11 state, but it feels really weird to
have to go through a test mode, programmed through a register
dedicated to HiSPi handling, to be able to have lanes in that state.
But sensors are weird, and if this works for you it's fine.

I would just try to centralize it in s_stream()

> >> I wonder if you can call set_fmt() etc. when the sensor is streaming.
> >> Why not?
> >
> > Because
> > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-subdev-g-fmt.html?highlight=subdev_s_fmt#c.V4L.VIDIOC_SUBDEV_S_FMT
> >
> > EBUSY
> > The format can’t be changed because the pad is currently busy. This
> > can be caused, for instance, by an active video stream on the pad. The
> > ioctl must not be retried without performing another action to fix the
> > problem first. Only returned by VIDIOC_SUBDEV_S_FMT
>
> But it doesn't say EBUSY MUST BE returned when the sensor is streaming,
> only that it MAY BE returned. Looking at the code, I can see nothing
> forcing the EBUSY (subdev_do_ioctl(VIDIOC_SUBDEV_S_FMT) ->

There's nothing in the core that has the notion of 'active streaming'

> v4l2_subdev_call(set_fmt)). It also appears several drivers update
> registers in their set_fmt(), so I think it's the desirable action
> there.
>

I hardly see a case where changing format on the sensor through an
operation on the subdev while streaming, is a good idea.

Also, you have a devnode, so you can change the format on the subdev
only, without informing the receiver.

Also, your s_fmt() and s_frame_interval() call write_mode() which
stops the streaming and it doesn't get restarted. I think that's
wrong and it's an undesired side effect.

(Also had a chat with Hans about this, the takeaway is that it's a
really bad idea and you need very strong reasons to allow that. It
could be considered for extreme cases like changing the color spaces of
reducing the image size as the allocated buffers are big enough but
again, you need very strong reasons to do so)

> >> >> +	ret = ar0521_write_reg(sensor, AR0521_REG_ROW_SPEED, 0x110 | 4 / sensor->lane_count);
> >> >
> >> > I wasn't able to interpret this register right
> >> >
> >> > pc_speed
> >> > Slows down the internal pixel clock frequency relative to the system
> >> > clock frequency.  A programmed value of N gives a pixel clock period
> >> > of N system clocks.  Only values 1, 2 and 4 are supported.
> >> >
> >> > Shouldn't this be part of the PLL calculation ?
> >>
> >> Well, this doesn't seem to be part of the PLL, and it's constant (as
> >> long as MIPI lane# is constant). Why do you think it belongs there?
> >
> > Because seems like a clock downscaler intended to be used to adapt the CSI-2
> > clock frequency (lane dependent) to the pixel clock and could be
> > programmed once.
>
> Exactly. This "programmed once" (per power-up) is exactly why I set it
> in ar0521_power_on() instead of calc_pll().
> First, calc_pll is calculations-only, no hw access (suggested by Laurent
> IIRC, and it plays well). Second, calc_pll() is called multiple times.
>
> > Also note that the number of data lanes in use can actually be modified at
> > run time but it's not something that has to be considered here.
>
> Yes, #lanes is constant here, though variable lane count could be
> useful - for tests, and in my other (unpublished) experiments. I don't
> know if it was possible back then, when I wrote the driver.
>
> [initial_regs]
>
> >> You know, I try not to throw unnecessary traffic at I2C bus, too.
> >
> > So this could moved to the register tables ? :)
>
> The truth is, I've considered doing exactly that. But they're quite
> fragmented (pixel_timing_recommended is linear) and well, perhaps I
> didn't want to complicate code too much. I may still do it, though.
> Guess I should.
>
> > i think you can remove any reference to events
>
> Good :-)
>
> > You should probably also support init_cfg as you expose a subdev video
> > device and you can have an init_format() or similar to be called there
> > for the try format and called here for the active format.
>
> Will have a look.
>
> > Maybe I'm wrong but calling set_suspend() after pm_runtime() had been
> > disabled seems pointless. A minor anyway as it's in the driver's
> > remove function.
>
> Daniel already posted a comment about this, however the whole
> pm_runtime*() in this driver is a bit fragile and I'd welcome someone
> who know how does this PM stuff work looking at it...
> ... with the following quick patch applied on top of v5:
>
> -	pm_runtime_idle(&client->dev);
> +	if (pm_runtime_idle(&client->dev) == -ENOSYS) {
> +		ret = ar0521_power_on(sensor);
> +		if (ret)
> +			goto disable;
> +	}
>  	dev_dbg(dev, "AR0521 driver initialized, master clock frequency: %u MHz, %u MIPI data lanes\n",
>  		sensor->extclk_freq, sensor->lane_count);
>  	return 0;
>
> +disable:
> +	v4l2_async_unregister_subdev(&sensor->sd);
> +	media_entity_cleanup(&sensor->sd.entity);
>  free_ctrls:
>  	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
>  entity_cleanup:
>
> (Power is only needed after probe()).
>
> I'll post v6 eventually, of course, but have to squeeze this in
> somewhere.
> --
> Krzysztof "Chris" Hałasa
>
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

Jacopo Mondi <jacopo@jmondi.org> writes:

> You have never been told before, while submitting code to Linux, not
> to use C++ comments ? Are you surprised someone contests that ?

Surprised, far from it. Linus has decided C++ comments are ok.
I simply follow his advice (C++ comments are, after all, technically
a little bit better).

> No driver in media (which I'm aware of) uses C++ comments.
> Your one is entirely commented with C++ comments.
>
> They all try to stay in the 80-cols limit.
> Yours have lines that span to 140 cols and goes regularly over 100.

Linus has already (in 2016 IIRC) said 80 cols are BS. This is BTW what
several people (me included) postulated long before. Would it make sense
to use this limit now?

If the above constitutes being "alien", well, don't worry about it.

> Ok, I give up then, feels like a waste of time reviewing a driver
> (for the only sake of code consistency) and have every single comment
> contested.

Please note a didn't contest your every comment. Actually, I have
contested only those... which I contested, by definition. I have
(perhaps too silently) accepted the rest.

> Documentation/driver-api/media/maintainer-entry-profile.rst
>
> and has there suggested have the patch go through
> ./scripts/checkpatch.pl --strict --max-line-length=80

You suggest I'm to fix this (.rst) file first? I think I can at least
try.

>> I'd love to get rid of the be(), though. What do you propose instead?
>
> Mode based sensor drivers usually rely on long register tables, whose
> writing is an expensive operation to be done at streamon time. Power
> up is usually done at devnode open time but you relay on the legacy
> s_power() here,

It's been suggested I get rid of it, and I'm going to do exactly this.

> so it's in control of the receiver driver which
> depending on the implemenation might call it at open() time or stream
> on. Sorry, I didn't notice that, has you register a devnode I assumed
> you had an open() function, which you don't.

Should I have one? Why? Are other drivers expected to have an open()?
Shouldn't I register a devnode?

> The efficiency argument holds as long as we are in an hot path and I
> understand writing 216 registers in pairs has an overhead which to
> me, at open devnode open time is marginal, but if done at streamon time
> should be avoided.

It *may*be* marginal in some cases, but it you have a single I^2 bus for
a bunch of devices, some of them e.g. MEMS, it may be as well critical.

>> Just tested it and it works for me in 1920x1080p30 without any changes.
>> Would it be possible it's the gain/exposure settings? If not, what exact
>> clock frequency (for the chip) do you use?
>
> 24Mhz

I will try to use that.
What SoC (or MIPI receiver) are you using?

> The difference is that the 0x3xxx ones are frame synchronized and
> apply to 'bad frames' too.

Is it stated in the docs?

>> I don't think so. I think, in proprietary development, nobody cares
>> about what does the chip send while not streaming.
>>
>
> afaict only imx6 has this check enforced (but I might be wrong)

Possibly only in the official tree (not the FSL/NXP).

>> How would you do that?
>> If you disable streaming, LP-11 is gone.
>> You need STREAMING to actually "stream" LP-11.
>
> Even for test mode ? So for you streamoff is:
> - Enable test mode (programmed to be LP-11)
> - Start stream
> ?

That's correct. LP-11 here *is* a test mode.

> Anyway, should the AR0521_REG_RESET_RESTART bit be dropped ?

Not sure. Why do you think so?

> I don't have a way to test LP-11 state,

On i.MX6 you can read a MIPI RX status register. IIRC the results may be
a bit unclear, though - using an oscilloscope removes any doubt.

>> But it doesn't say EBUSY MUST BE returned when the sensor is streaming,
>> only that it MAY BE returned. Looking at the code, I can see nothing
>> forcing the EBUSY (subdev_do_ioctl(VIDIOC_SUBDEV_S_FMT) ->
>
> There's nothing in the core that has the notion of 'active streaming'

Come on. It appears Linux (from top to the bottom) will accept set_fmt
while streaming. With certain (most?) drivers only, that's it. Not that
I actually tested it, but the v4l2 core code suggests it.
So I'm either to return -EBUSY, or - as others, probably most drivers -
update the registers. I can't just drop it on the floor, and let the
driver apply it on the next s_stream(1)... can I?

> I hardly see a case where changing format on the sensor through an
> operation on the subdev while streaming, is a good idea.

I'm not in control of this.

> Also, your s_fmt() and s_frame_interval() call write_mode() which
> stops the streaming and it doesn't get restarted. I think that's
> wrong and it's an undesired side effect.

That would be wrong indeed, but I can't see it in the code.
write_mode() stop streaming only momentarily, I can't avoid this.
s_frame_interval() returns -EBUSY if streaming (which I guess I should
remove). If not for the return, it wouldn't stop streaming either.

I will do some experiments, though.

> (Also had a chat with Hans about this, the takeaway is that it's a
> really bad idea and you need very strong reasons to allow that. It
> could be considered for extreme cases like changing the color spaces of
> reducing the image size as the allocated buffers are big enough but
> again, you need very strong reasons to do so)

Ah, buffers are a different story. You can't, for example, request
buffers which streaming etc. This is a completely different territory.

Now... I don't have buffers :-)

It's a MIPI sensor, the output is a bunch of LVDS lines.
Certain devices (like THC63LVD104C, an LVDS -> parallel receiver) simply
always stream (well, not in powerdown and not without incoming clock).
They don't even notice that some format has maybe changed.
A MIPI sensor is a bit smarter than that, but buffers - it's the
receiver's problem.
-- 
Krzysztof "Chris" Hałasa

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

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Jacopo Mondi]

HI Krzysztof

On Wed, Oct 13, 2021 at 02:55:54PM +0200, Krzysztof Hałasa wrote:
> Jacopo Mondi <jacopo@jmondi.org> writes:
>
> > You have never been told before, while submitting code to Linux, not
> > to use C++ comments ? Are you surprised someone contests that ?
>
> Surprised, far from it. Linus has decided C++ comments are ok.
> I simply follow his advice (C++ comments are, after all, technically
> a little bit better).
>
> > No driver in media (which I'm aware of) uses C++ comments.
> > Your one is entirely commented with C++ comments.
> >
> > They all try to stay in the 80-cols limit.
> > Yours have lines that span to 140 cols and goes regularly over 100.
>
> Linus has already (in 2016 IIRC) said 80 cols are BS. This is BTW what
> several people (me included) postulated long before. Would it make sense
> to use this limit now?
>

Apparently, for the subsystem maintainers, yes.

> If the above constitutes being "alien", well, don't worry about it.
>
> > Ok, I give up then, feels like a waste of time reviewing a driver
> > (for the only sake of code consistency) and have every single comment
> > contested.
>
> Please note a didn't contest your every comment. Actually, I have
> contested only those... which I contested, by definition. I have
> (perhaps too silently) accepted the rest.
>
> > Documentation/driver-api/media/maintainer-entry-profile.rst
> >
> > and has there suggested have the patch go through
> > ./scripts/checkpatch.pl --strict --max-line-length=80
>
> You suggest I'm to fix this (.rst) file first? I think I can at least
> try.

No, I suggest you stick to what has been there recorded not longer
than 2 years ago.

If you prefer to go the other way around and change the subsystem
profile first, please go ahead.

>
> >> I'd love to get rid of the be(), though. What do you propose instead?
> >
> > Mode based sensor drivers usually rely on long register tables, whose
> > writing is an expensive operation to be done at streamon time. Power
> > up is usually done at devnode open time but you relay on the legacy
> > s_power() here,
>
> It's been suggested I get rid of it, and I'm going to do exactly this.
>
> > so it's in control of the receiver driver which
> > depending on the implemenation might call it at open() time or stream
> > on. Sorry, I didn't notice that, has you register a devnode I assumed
> > you had an open() function, which you don't.
>
> Should I have one? Why? Are other drivers expected to have an open()?

If you register a devnode, userspace can open it.

To each open file handled is associated a try format, which should be
initialized to some meaningful default.

The same format should (or could) be used for your active format.

Userspace that makes use of the V4L2 subdev API won't be able to fully
operate on this driver in its current state.

> Shouldn't I register a devnode?
>

Depends on your use case and how much you care about supporting
complex use cases.

If you operate with a legacy implementation where everything goes
through the video device node, you don't care about the subdev
devnode.

If you operate in a media-controller environment, where userspace is
in control of configuring each component on the pipeline, you should
register a devnode.

> > The efficiency argument holds as long as we are in an hot path and I
> > understand writing 216 registers in pairs has an overhead which to
> > me, at open devnode open time is marginal, but if done at streamon time
> > should be avoided.
>
> It *may*be* marginal in some cases, but it you have a single I^2 bus for
> a bunch of devices, some of them e.g. MEMS, it may be as well critical.
>
> >> Just tested it and it works for me in 1920x1080p30 without any changes.
> >> Would it be possible it's the gain/exposure settings? If not, what exact
> >> clock frequency (for the chip) do you use?
> >
> > 24Mhz
>
> I will try to use that.
> What SoC (or MIPI receiver) are you using?
>
> > The difference is that the 0x3xxx ones are frame synchronized and
> > apply to 'bad frames' too.
>
> Is it stated in the docs?
>

Yes, in the right-most columns of the register tables.

> >> I don't think so. I think, in proprietary development, nobody cares
> >> about what does the chip send while not streaming.
> >>
> >
> > afaict only imx6 has this check enforced (but I might be wrong)
>
> Possibly only in the official tree (not the FSL/NXP).
>
> >> How would you do that?
> >> If you disable streaming, LP-11 is gone.
> >> You need STREAMING to actually "stream" LP-11.
> >
> > Even for test mode ? So for you streamoff is:
> > - Enable test mode (programmed to be LP-11)
> > - Start stream
> > ?
>
> That's correct. LP-11 here *is* a test mode.
>
> > Anyway, should the AR0521_REG_RESET_RESTART bit be dropped ?
>
> Not sure. Why do you think so?
>

Because it's documented as:

Setting this bit causes the sensor to truncate the current frame at
the end of the current row and start resetting (integrating) the first
row. The delay before the first valid frame is read out is equal to
the integration time.

and since you're moving from test mode to stream mode, there's no
frame integration going on.

> > I don't have a way to test LP-11 state,
>
> On i.MX6 you can read a MIPI RX status register. IIRC the results may be
> a bit unclear, though - using an oscilloscope removes any doubt.
>
> >> But it doesn't say EBUSY MUST BE returned when the sensor is streaming,
> >> only that it MAY BE returned. Looking at the code, I can see nothing
> >> forcing the EBUSY (subdev_do_ioctl(VIDIOC_SUBDEV_S_FMT) ->
> >
> > There's nothing in the core that has the notion of 'active streaming'
>
> Come on. It appears Linux (from top to the bottom) will accept set_fmt

What do you mean with "Linux from top to the bottom" ?

What I meant is that the core cannot prevent ioctls and subdev
operations to be called on the sensor while streaming, as the 'is
streaming' state is only kept in the driver and there's no state
keeping in the V4L2 core.

> while streaming. With certain (most?) drivers only, that's it. Not that
> I actually tested it, but the v4l2 core code suggests it.

spec says no.
reviewers say no.
maintainers say no.

But if you think you're right, please go ahead.

> So I'm either to return -EBUSY, or - as others, probably most drivers -

maybe they just assume they knew better when they got being told
not to do so during review.

> update the registers. I can't just drop it on the floor, and let the
> driver apply it on the next s_stream(1)... can I?
>
> > I hardly see a case where changing format on the sensor through an
> > operation on the subdev while streaming, is a good idea.
>
> I'm not in control of this.
>

As you're not in control of how your driver will be used once merged

That's why there are specs, reviews and collective knowledge that
helps enforce a consistent behavior.

> > Also, your s_fmt() and s_frame_interval() call write_mode() which
> > stops the streaming and it doesn't get restarted. I think that's
> > wrong and it's an undesired side effect.
>
> That would be wrong indeed, but I can't see it in the code.
> write_mode() stop streaming only momentarily, I can't avoid this.

Yes you can, by not allowing s_fmt while streaming.

Stopping and restarting stream in the sensor, through a subdev ioctl
behind the back of the rest of the capture pipeline is just calling
for troubles.

There's a potential point of failure in every single part of the
capture pipeline, from the on-going transfer in the SoC's DMA engines
to the CSI-2 receiver port.

Even without the stop/restart sequence, what happens if you increase
the frame size which is output from the sensor without re-negotiating
buffers or image formats ?

There's no single valid reason to allow such a use case, if not making
out of it a matter of principle like you're doing.

> s_frame_interval() returns -EBUSY if streaming (which I guess I should
> remove). If not for the return, it wouldn't stop streaming either.
>
> I will do some experiments, though.
>
> > (Also had a chat with Hans about this, the takeaway is that it's a
> > really bad idea and you need very strong reasons to allow that. It
> > could be considered for extreme cases like changing the color spaces of
> > reducing the image size as the allocated buffers are big enough but
> > again, you need very strong reasons to do so)
>
> Ah, buffers are a different story. You can't, for example, request
> buffers which streaming etc. This is a completely different territory.
>
> Now... I don't have buffers :-)
>
> It's a MIPI sensor, the output is a bunch of LVDS lines.
> Certain devices (like THC63LVD104C, an LVDS -> parallel receiver) simply
> always stream (well, not in powerdown and not without incoming clock).
> They don't even notice that some format has maybe changed.
> A MIPI sensor is a bit smarter than that, but buffers - it's the
> receiver's problem.
> --
> Krzysztof "Chris" Hałasa
>
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

Jacopo,

> To each open file handled is associated a try format, which should be
> initialized to some meaningful default.

I will look at it then.

> If you operate with a legacy implementation where everything goes
> through the video device node, you don't care about the subdev
> devnode.

This can't be done in case of a sensor driver, right? Output from the
sensor is MIPI CSI, there is no connection to the memory bus.

BTW: What SoC (or MIPI receiver etc.) are you using?

>> > The difference is that the 0x3xxx ones are frame synchronized and
>> > apply to 'bad frames' too.
>>
>> Is it stated in the docs?
>
> Yes, in the right-most columns of the register tables.

Right.
Interesting - they are marked both "frame sync'd" and "bad frame" (not
all of them), where the 0x340 ones are both "N". I will try some tests,
but such a combination looks a bit suspicious.
(I guess I already tested it, but don't remember the results).

> Because it's documented as:
>
> Setting this bit causes the sensor to truncate the current frame at
> the end of the current row and start resetting (integrating) the first
> row. The delay before the first valid frame is read out is equal to
> the integration time.
>
> and since you're moving from test mode to stream mode, there's no
> frame integration going on.

But there is :-)
Remember we're streaming, the whole sensor is working. It's just the
LP-11 on the output lines.

> What do you mean with "Linux from top to the bottom" ?

Userspace + ioctl on top, the driver/hw on the bottom.

> What I meant is that the core cannot prevent ioctls and subdev
> operations to be called on the sensor while streaming, as the 'is
> streaming' state is only kept in the driver and there's no state
> keeping in the V4L2 core.

Exactly, that's what I thought.

>> while streaming. With certain (most?) drivers only, that's it. Not that
>> I actually tested it, but the v4l2 core code suggests it.
>
> spec says no.

I'm not aware of it. The specs say a driver *is*allowed* to return
EBUSY, if it can't handle the condition.

> reviewers say no.

Haven't seen this either. The existing code (other drivers) suggests
otherwise.

> maintainers say no.

Ditto.
Buffers are a completely different thing.

>> So I'm either to return -EBUSY, or - as others, probably most drivers -
>
> maybe they just assume they knew better when they got being told
> not to do so during review.

All of the others are wrong?
Maybe nobody told them otherwise - because there was no reason?

>> > I hardly see a case where changing format on the sensor through an
>> > operation on the subdev while streaming, is a good idea.
>>
>> I'm not in control of this.
>
> As you're not in control of how your driver will be used once merged

Exactly, it's the very same thing.

> That's why there are specs, reviews and collective knowledge that
> helps enforce a consistent behavior.

That's correct. Now you tell me I'm not to write to the hw in set_fmt(),
while I can clearly see other drivers do exactly that, and nobody else
suggests they (I) shouldn't.

> There's a potential point of failure in every single part of the
> capture pipeline, from the on-going transfer in the SoC's DMA engines
> to the CSI-2 receiver port.

I'm not in control of this, why should I set a policy for them? I don't
even know if there is a SoC and DMA engines - maybe this goes straight
to the antenna or *SDI transmitter? :-)

> Even without the stop/restart sequence, what happens if you increase
> the frame size which is output from the sensor without re-negotiating
> buffers or image formats ?

*I* don't change frame sizes. It's the upper levels which are making
such decisions. My code can only comply or return an error.
Perhaps the upper levels know what they're doing?

If they are wrong after all, well - a misprogrammed i.MX6 will corrupt
the frames, the output stream will lose sync, and the userspace can get
I/O errors on ioctls. The userspace will get what it asked for.

This is BTW completely orthogonal to the -EBUSY on set_fmt(). The
effects will be exactly the same if the e.g. geometry changes come when
the sensor is not streaming.

> There's no single valid reason to allow such a use case, if not making
> out of it a matter of principle like you're doing.

Then why other drivers do exactly that? Eg. all imx*.
Including the newest one imx412, merged 2021-08-04, and:
    Signed-off-by: Martina Krasteva <martinax.krasteva@intel.com>
    Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
    Acked-by: Paul J. Murphy <paul.j.murphy@intel.com>
    Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
    Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Are they really all wrong? Really?

BTW principles are important to me, yes. One of them is "allow unless
required otherwise".
-- 
Krzysztof "Chris" Hałasa

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

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Jacopo Mondi]

On Thu, Oct 14, 2021 at 07:43:02AM +0200, Krzysztof Hałasa wrote:
> Jacopo,
>
> > To each open file handled is associated a try format, which should be
> > initialized to some meaningful default.
>
> I will look at it then.
>
> > If you operate with a legacy implementation where everything goes
> > through the video device node, you don't care about the subdev
> > devnode.
>
> This can't be done in case of a sensor driver, right? Output from the
> sensor is MIPI CSI, there is no connection to the memory bus.
>
> BTW: What SoC (or MIPI receiver etc.) are you using?
>
> >> > The difference is that the 0x3xxx ones are frame synchronized and
> >> > apply to 'bad frames' too.
> >>
> >> Is it stated in the docs?
> >
> > Yes, in the right-most columns of the register tables.
>
> Right.
> Interesting - they are marked both "frame sync'd" and "bad frame" (not
> all of them), where the 0x340 ones are both "N". I will try some tests,
> but such a combination looks a bit suspicious.
> (I guess I already tested it, but don't remember the results).
>
> > Because it's documented as:
> >
> > Setting this bit causes the sensor to truncate the current frame at
> > the end of the current row and start resetting (integrating) the first
> > row. The delay before the first valid frame is read out is equal to
> > the integration time.
> >
> > and since you're moving from test mode to stream mode, there's no
> > frame integration going on.
>
> But there is :-)
> Remember we're streaming, the whole sensor is working. It's just the
> LP-11 on the output lines.
>
> > What do you mean with "Linux from top to the bottom" ?
>
> Userspace + ioctl on top, the driver/hw on the bottom.
>
> > What I meant is that the core cannot prevent ioctls and subdev
> > operations to be called on the sensor while streaming, as the 'is
> > streaming' state is only kept in the driver and there's no state
> > keeping in the V4L2 core.
>
> Exactly, that's what I thought.
>
> >> while streaming. With certain (most?) drivers only, that's it. Not that
> >> I actually tested it, but the v4l2 core code suggests it.
> >
> > spec says no.
>
> I'm not aware of it. The specs say a driver *is*allowed* to return
> EBUSY, if it can't handle the condition.
>
> > reviewers say no.
>
> Haven't seen this either. The existing code (other drivers) suggests
> otherwise.
>
> > maintainers say no.
>
> Ditto.
> Buffers are a completely different thing.
>
> >> So I'm either to return -EBUSY, or - as others, probably most drivers -
> >
> > maybe they just assume they knew better when they got being told
> > not to do so during review.
>
> All of the others are wrong?
> Maybe nobody told them otherwise - because there was no reason?
>
> >> > I hardly see a case where changing format on the sensor through an
> >> > operation on the subdev while streaming, is a good idea.
> >>
> >> I'm not in control of this.
> >
> > As you're not in control of how your driver will be used once merged
>
> Exactly, it's the very same thing.
>
> > That's why there are specs, reviews and collective knowledge that
> > helps enforce a consistent behavior.
>
> That's correct. Now you tell me I'm not to write to the hw in set_fmt(),
> while I can clearly see other drivers do exactly that, and nobody else
> suggests they (I) shouldn't.
>
> > There's a potential point of failure in every single part of the
> > capture pipeline, from the on-going transfer in the SoC's DMA engines
> > to the CSI-2 receiver port.
>
> I'm not in control of this, why should I set a policy for them? I don't
> even know if there is a SoC and DMA engines - maybe this goes straight
> to the antenna or *SDI transmitter? :-)
>
> > Even without the stop/restart sequence, what happens if you increase
> > the frame size which is output from the sensor without re-negotiating
> > buffers or image formats ?
>
> *I* don't change frame sizes. It's the upper levels which are making
> such decisions. My code can only comply or return an error.
> Perhaps the upper levels know what they're doing?
>
> If they are wrong after all, well - a misprogrammed i.MX6 will corrupt
> the frames, the output stream will lose sync, and the userspace can get
> I/O errors on ioctls. The userspace will get what it asked for.
>
> This is BTW completely orthogonal to the -EBUSY on set_fmt(). The
> effects will be exactly the same if the e.g. geometry changes come when
> the sensor is not streaming.
>

No, this isn't true. Your s_fmt() implementation stops then restart the
stream. It has an undocumented side effect and will cause undefined
behaviour.

> > There's no single valid reason to allow such a use case, if not making
> > out of it a matter of principle like you're doing.
>
> Then why other drivers do exactly that? Eg. all imx*.
> Including the newest one imx412, merged 2021-08-04, and:
>     Signed-off-by: Martina Krasteva <martinax.krasteva@intel.com>
>     Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
>     Acked-by: Paul J. Murphy <paul.j.murphy@intel.com>
>     Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>     Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>
> Are they really all wrong? Really?
>
> BTW principles are important to me, yes. One of them is "allow unless
> required otherwise".

And that's how you will end up accepting the fact a s_fmt() has the
slightly disruptive side effect of stopping/restarting the stream
behind the back of the other components of the capture pipeline.

If your s_fmt() has to stop and restart streaming to take effect,
it means userspace should instead stop the stream, change
the format where opportune in the pipeline, and then restart the stream.
This allows the pipeline validation to take place, it allows buffers
to be negotiated correctly, it makes it impossible to write
application that rely on a side effect.

There's a lot going on about correctness and avoiding undefined
behaviours in the kernel these days. This is an example why I think a
language change won't solve much, not when it comes to correctness
towards user space. Allowing drivers to implement side-effects and
what clearly is an example of undefined behaviour is the recipe to
allow userspace to shot in their foot and makes it impossible to write
portable software with a predictable behaviour.

I still haven't heard a single reason why you want this, if not again,
for a matter of principles, which is by the way the less possible
productive way to carry on a converstation that should be based on
facts and reasons, not a fight to impose your mindset no
matter what.

Anyway, since we're not going anywhere here, I'll let this upscale to
anyone that will pick this driver up.

Thanks again for the contribution, I hope to see this driver in soon.
Cheers
   j

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

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

Jacopo Mondi <jacopo@jmondi.org> writes:

>> This is BTW completely orthogonal to the -EBUSY on set_fmt(). The
>> effects will be exactly the same if the e.g. geometry changes come when
>> the sensor is not streaming.
>>
>
> No, this isn't true. Your s_fmt() implementation stops then restart the
> stream. It has an undocumented side effect and will cause undefined
> behaviour.

It will cause *at*most* a corrupted frame. On a MIPI link. That's right.
Such a corrupted frame will *at*most* cause some transient IO error - it
must not cause anything serious, because corrupted frames on MIPI can
happen for multiple reasons, some of which simply cannot be avoided.
BTW I will see if it's actually the case - chances are, there is no
corruption, but I tested it years ago and haven't yet checked my notes.

In fact those set_fmt() in other drivers may - or may not - cause
corrupted frames just the same.

> If your s_fmt() has to stop and restart streaming to take effect,
> it means userspace should instead stop the stream, change
> the format where opportune in the pipeline, and then restart the
> stream.

Maybe. This is a sensor driver - not userspace.
If the userspace uses it as a part of "frame grabber", it will certainly
do exactly that (nothing else would make sense in practice).

Unfortunately this all will have to wait a bit, so thanks for your help,
expect a new patch in few weeks.
-- 
Krzysztof "Chris" Hałasa

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

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Sakari Ailus]

Hi Krzysztof,

On Wed, Oct 13, 2021 at 07:39:07AM +0200, Krzysztof Hałasa wrote:
> Hi Sakari,
> 
> > 	https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html
> 
> Ok:
> "8.2.2. Devicetree
> 
> The currently preferred way to achieve this is using assigned-clocks,
> assigned-clock-parents and assigned-clock-rates properties. See
> Documentation/devicetree/bindings/clock/clock-bindings.txt for more
> information. The driver then gets the frequency using clk_get_rate()."
> 
> Let's see:
> Documentation/devicetree/bindings/clock/clock-bindings.txt:
> 
> "==Assigned clock parents and rates==
> 
> Some platforms may require initial configuration of default parent clocks
> and clock frequencies. Such a configuration can be specified in a device tree
> node through assigned-clocks, assigned-clock-parents and assigned-clock-rates
> properties. The assigned-clock-parents property should contain a list of parent
> clocks in the form of a phandle and clock specifier pair and the
> assigned-clock-rates property should contain a list of frequencies in Hz. Both
> these properties should correspond to the clocks listed in the assigned-clocks
> property."
> 
> So I'm after "assigned-clock-rates", right?
> 
> "Configuring a clock's parent and rate through the device node that consumes
> the clock can be done only for clocks that have a single user. Specifying
> conflicting parent or rate configuration in multiple consumer nodes for
> a shared clock is forbidden."
> 
> This sounds a bit problematic, the clock I use is at least potentially
> shared by multiple parts of the system, depending on current (run time)
> configuration. I am/was getting different frequencies depending of the
> particular system (all based on the same i.MX6* SoC, but with different
> peripherals used/enabled). I think it's quite a common situation.

This was discussed some time ago before I wrote the documentation. The
conclusion back then was that it's just fine, and such cases would need to
be addressed when they turn up. We haven't had any yet as far as I know.

> 
> > Generally camera sensor drivers that set the clock in drivers themselves
> > are (very) old.
> 
> Let's have a look... ov9282 is (one of) the newest drivers. It does:
> #define OV9282_INCLK_RATE    24000000
> 
>         /* Get sensor input clock */
>         ov9282->inclk = devm_clk_get(ov9282->dev, NULL);
>         if (IS_ERR(ov9282->inclk)) {
>                 dev_err(ov9282->dev, "could not get inclk");
>                 return PTR_ERR(ov9282->inclk);
>         }
> 
>         rate = clk_get_rate(ov9282->inclk);
>         if (rate != OV9282_INCLK_RATE) {
>                 dev_err(ov9282->dev, "inclk frequency mismatch");
>                 return -EINVAL;
>         }
> 
> $ git grep -l ov9282
> Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml
> MAINTAINERS
> drivers/media/i2c/Kconfig
> drivers/media/i2c/Makefile
> drivers/media/i2c/ov9282.c
> 
>   clocks:
>     description: Clock frequency from 6 to 27MHz
> 
> No in-tree DTS exists, but the single frequency (both in the driver -
> this one can be fixed - and in the DTS) is rather limiting. Maybe
> another:
> 
> imx412, imx335, imx334, imx258 - same here.
> imx208 is ACPI-based.
> 
> Which driver should I consult?

The drivers you're looking at are based on register lists so they usually
support just a single frequency. The sensors are not limited to this
frequency however, which is why you see the frequency in DT bindings, too.

-- 
Regards,

Sakari Ailus

Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

Hi Sakari,

> The drivers you're looking at are based on register lists so they usually
> support just a single frequency. The sensors are not limited to this
> frequency however, which is why you see the frequency in DT bindings, too.

Does that mean that drivers supporting a frequency range (rather than a
single frequency) don't need the range (nor single frequency) in DT?

While it's true that the AR0521 driver can (and has to) work with
different frequencies (on different systems), I think I can specify
a single frequency per system. But - should I really do that?
-- 
Krzysztof "Chris" Hałasa

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