[PATCH v6 0/2] On Semi AR0521 sensor driver

[PATCH v6 0/2] On Semi AR0521 sensor driver

[Krzysztof Hałasa]

Rob, Mauro, media subsystem reviewers,

This is the 6th version of my On Semi AR0521 sensor driver.

The documentation patch (1/2) hasn't been changed from v4:

 onnn,ar0521.yaml |  112
 1 file changed, 112 insertions(+)

The actual driver (2/2) changes:

 MAINTAINERS                |    7
 drivers/media/i2c/Kconfig  |   13
 drivers/media/i2c/Makefile |    1
 drivers/media/i2c/ar0521.c | 1051
 4 files changed, 1072 insertions(+)

- I reformatted the code to fit in 80 columns. Nobody should be asked to
  make his code worse (and the 80-column version IS worse), and multiple
  high-profile Linux developers (including the top one) appear to share
  my opinion, but nevertheless - if it's something that will make it go
  in, I won't care.

- Basically the same applies to the // comments.

- I have removed the "interval" support (frames per second).
  Unfortunately this cripples the driver further a bit - the userspace
  will not be able to set precise frame timings needed for broadcast
  quality video. I will have to keep a private patch for that.
  Another effect of this change is that the pixel clock is now fixed at
  184 MHz, which by default produces ca. 30 FPS at 2560x1920. This may
  be problematic on systems with less than 4 MIPI lanes, and/or on ones
  which can't support higher frequency MIPI bus (the previous version
  used a calculated clock). Perhaps it will be possible to fix this
  issue in the future, with a couple of core V4L2 changes.

- the driver now provides the .pre_streamon() for setting LP-11 state on
  MIPI data and clock lanes. This is compatible with i.MX6 receiver.

- s_power() converted to SET_RUNTIME_PM_OPS().

- the "initial" I2C registers have been all converted to a table of
  multi-register files, to minimize time spent on I2C bus.

- a lot of smaller changes suggested by Laurent, Sakari, Jacopo and
  possibly others.
-- 
Krzysztof "Chris" Hałasa

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

[PATCH v6 1/2] dt-binding: media: document ON Semi AR0521 sensor bindings

[Krzysztof Hałasa]

This file documents DT bindings for the AR0521 camera sensor driver.

Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Rob Herring <robh@kernel.org>

diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
new file mode 100644
index 000000000000..b617cc5c6a9f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
@@ -0,0 +1,112 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/onnn,ar0521.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ON Semiconductor AR0521 MIPI CSI-2 sensor
+
+maintainers:
+  - Krzysztof Hałasa <khalasa@piap.pl>
+
+description: |-
+  The AR0521 is a raw CMOS image sensor with MIPI CSI-2 and
+  I2C-compatible control interface.
+
+properties:
+  compatible:
+    const: onnn,ar0521
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: extclk
+
+  vaa-supply:
+    description:
+      Definition of the regulator used as analog (2.7 V) voltage supply.
+
+  vdd-supply:
+    description:
+      Definition of the regulator used as digital core (1.2 V) voltage supply.
+
+  vdd_io-supply:
+    description:
+      Definition of the regulator used as digital I/O (1.8 V) voltage supply.
+
+  reset-gpios:
+    description: reset GPIO, usually active low
+    maxItems: 1
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    unevaluatedProperties: false
+    description: |
+      Video output port.
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          bus-type:
+            const: 4
+          data-lanes:
+            anyOf:
+              - items:
+                  - const: 1
+              - items:
+                  - const: 1
+                  - const: 2
+              - items:
+                  - const: 1
+                  - const: 2
+                  - const: 3
+                  - const: 4
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - vaa-supply
+  - vdd-supply
+  - vdd_io-supply
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/clock/imx6qdl-clock.h>
+
+    i2c {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            ar0521: camera-sensor@36 {
+                    compatible = "onnn,ar0521";
+                    reg = <0x36>;
+                    pinctrl-names = "default";
+                    pinctrl-0 = <&pinctrl_mipi_camera>;
+                    clocks = <&clks IMX6QDL_CLK_CKO>;
+                    clock-names = "extclk";
+                    reset-gpios = <&gpio1 7 GPIO_ACTIVE_LOW>;
+                    vaa-supply = <&reg_2p7v>;
+                    vdd-supply = <&reg_1p2v>;
+                    vdd_io-supply = <&reg_1p8v>;
+
+                    port {
+                           mipi_camera_to_mipi_csi2: endpoint {
+                                    remote-endpoint = <&mipi_csi2_in>;
+                                    data-lanes = <1 2 3 4>;
+                            };
+                    };
+            };
+    };

[PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

The driver has been extensively tested in an i.MX6-based system.
AR0521 is a 5.7 mm x 4.3 mm, 5 MPix RGGB MIPI/HiSPi BSI CMOS sensor
from On Semiconductor.

Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>

diff --git a/MAINTAINERS b/MAINTAINERS
index 38235471fed0..472a49e6df61 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1383,6 +1383,13 @@ 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:	Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
+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 69c56e24a612..741ae073623e 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -733,6 +733,19 @@ config VIDEO_APTINA_PLL
 config VIDEO_CCS_PLL
 	tristate
 
+config VIDEO_AR0521
+	tristate "ON Semiconductor AR0521 sensor support"
+	depends on I2C && VIDEO_V4L2
+	select MEDIA_CONTROLLER
+	select VIDEO_V4L2_SUBDEV_API
+	select V4L2_FWNODE
+	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 b01f6cd05ee8..4e4986e16071 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -118,6 +118,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_HI846)	+= hi846.o
 obj-$(CONFIG_VIDEO_IMX208)	+= imx208.o
diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
new file mode 100644
index 000000000000..f113c1b163de
--- /dev/null
+++ b/drivers/media/i2c/ar0521.c
@@ -0,0 +1,1051 @@
+// 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/delay.h>
+#include <linux/pm_runtime.h>
+
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+/* External clock (extclk) frequencies */
+#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_RATE	 (184 * 1000 * 1000)
+#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     38u /* 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 {
+		struct v4l2_ctrl *gain;
+		struct v4l2_ctrl *red_balance;
+		struct v4l2_ctrl *blue_balance;
+	};
+	struct {
+		struct v4l2_ctrl *hblank;
+		struct v4l2_ctrl *vblank;
+	};
+	struct v4l2_ctrl *pixrate;
+	struct v4l2_ctrl *exposure;
+	struct v4l2_ctrl *test_pattern;
+};
+
+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 ar0521_ctrls ctrls;
+	unsigned int lane_count;
+	u16 total_width;
+	u16 total_height;
+	u16 pll_pre;
+	u16 pll_mult;
+	u16 pll_pre2;
+	u16 pll_mult2;
+	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 */
+	};
+	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_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\n",
+		sensor->fmt.width, sensor->fmt.height, sensor->total_width,
+		sensor->total_height);
+	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;
+
+		/* Exit LP-11 mode on clock and data lanes */
+		ret = ar0521_write_reg(sensor, AR0521_REG_HISPI_CONTROL_STATUS,
+				       0);
+		if (ret)
+			return ret;
+
+		/* Start streaming */
+		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;
+
+		/* Stop streaming */
+		return ar0521_write_reg(sensor, AR0521_REG_RESET,
+					AR0521_REG_RESET_DEFAULTS);
+	}
+
+}
+
+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 */
+	u16 total_width = max(sensor->fmt.width + AR0521_WIDTH_BLANKING_MIN,
+			      AR0521_TOTAL_WIDTH_MIN);
+	u16 total_height = sensor->fmt.height + AR0521_HEIGHT_BLANKING_MIN;
+
+	/* Calculate approximate pixel clock first */
+	u64 pix_clk = AR0521_PIXEL_CLOCK_RATE;
+
+	/* PLL1 drives pixel clock - dual rate */
+	pix_clk = calc_pll(sensor, 1, pix_clk * (DIV / 2), &sensor->pll_pre,
+			   &sensor->pll_mult);
+	pix_clk = div64_round(pix_clk, (DIV / 2));
+	calc_pll(sensor, 2, pix_clk * (DIV / 2) * speed_mod, &sensor->pll_pre2,
+		 &sensor->pll_mult2);
+
+	sensor->total_width = total_width;
+	sensor->total_height = total_height;
+}
+
+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);
+	v4l2_ctrl_cluster(3, &ctrls->gain);
+
+	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);
+	v4l2_ctrl_cluster(2, &ctrls->hblank);
+
+	/* 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_RATE);
+
+	/* Manual exposure time */
+	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0,
+					    65535, 1, 360);
+
+	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;
+}
+
+#define ARRAY_AND_SIZE(a)	(a), ARRAY_SIZE(a)
+#define REGS(...)		{ARRAY_AND_SIZE(((const __be16[]){__VA_ARGS__}))}
+
+static const struct initial_reg {
+	const __be16 *data; /* data[0] is register address */
+	unsigned int count;
+} initial_regs[] = {
+	REGS(be(0x0112), be(0x0808)), /* 8-bit/8-bit mode */
+
+	/* PEDESTAL+2 :+2 is a workaround for 10bit mode +0.5 rounding */
+	REGS(be(0x301E), be(0x00AA)),
+
+	/* corrections_recommended_bayer */
+	REGS(be(0x3042),
+	     be(0x0004),  /* 3042: RNC: enable b/w rnc mode */
+	     be(0x4580)), /* 3044: RNC: enable row noise correction */
+
+	REGS(be(0x30D2),
+	     be(0x0000),  /* 30D2: CRM/CC: enable crm on Visible and CC rows */
+	     be(0x0000),  /* 30D4: CC: CC enabled with 16 samples per column */
+	     /* 30D6: CC: bw mode enabled/12 bit data resolution/bw mode */
+	     be(0x2FFF)),
+
+	REGS(be(0x30DA),
+	     be(0x0FFF),  /* 30DA: CC: column correction clip level 2 is 0 */
+	     be(0x0FFF),  /* 30DC: CC: column correction clip level 3 is 0 */
+	     be(0x0000)), /* 30DE: CC: Group FPN correction */
+
+	/* RNC: rnc scaling factor = * 54 / 64 (32 / 38 * 64 = 53.9) */
+	REGS(be(0x30EE), be(0x1136)),
+	REGS(be(0x30FA), be(0xFD00)), /* GPIO0 = flash, GPIO1 = shutter */
+	REGS(be(0x3120), be(0x0005)), /* p1 dither enabled for 10bit mode */
+	REGS(be(0x3172), be(0x0206)), /* txlo clk divider options */
+	/* FDOC:fdoc settings with fdoc every frame turned of */
+	REGS(be(0x3180), be(0x9434)),
+
+	REGS(be(0x31B0),
+	     be(0x008B),  /* 31B0: frame_preamble - FIXME check WRT lanes# */
+	     be(0x0050)), /* 31B2: line_preamble - FIXME check WRT lanes# */
+
+	/* don't use continuous clock mode while shut down */
+	REGS(be(0x31BC), be(0x068C)),
+	REGS(be(0x31E0), be(0x0781)), /* Fuse/2DDC: enable 2ddc */
+
+	/* analog_setup_recommended_10bit */
+	REGS(be(0x341A), be(0x4735)), /* Samp&Hold pulse in ADC */
+	REGS(be(0x3420), be(0x4735)), /* Samp&Hold pulse in ADC */
+	REGS(be(0x3426), be(0x8A1A)), /* ADC offset distribution pulse */
+	REGS(be(0x342A), be(0x0018)), /* pulse_config */
+
+	/* pixel_timing_recommended */
+	REGS(be(0x3D00),
+	     /* 3D00 */ be(0x043E), be(0x4760), be(0xFFFF), be(0xFFFF),
+	     /* 3D08 */ be(0x8000), be(0x0510), be(0xAF08), be(0x0252),
+	     /* 3D10 */ be(0x486F), be(0x5D5D), be(0x8056), be(0x8313),
+	     /* 3D18 */ be(0x0087), be(0x6A48), be(0x6982), be(0x0280),
+	     /* 3D20 */ be(0x8359), be(0x8D02), be(0x8020), be(0x4882),
+	     /* 3D28 */ be(0x4269), be(0x6A95), be(0x5988), be(0x5A83),
+	     /* 3D30 */ be(0x5885), be(0x6280), be(0x6289), be(0x6097),
+	     /* 3D38 */ be(0x5782), be(0x605C), be(0xBF18), be(0x0961),
+	     /* 3D40 */ be(0x5080), be(0x2090), be(0x4390), be(0x4382),
+	     /* 3D48 */ be(0x5F8A), be(0x5D5D), be(0x9C63), be(0x8063),
+	     /* 3D50 */ be(0xA960), be(0x9757), be(0x8260), be(0x5CFF),
+	     /* 3D58 */ be(0xBF10), be(0x1681), be(0x0802), be(0x8000),
+	     /* 3D60 */ be(0x141C), be(0x6000), be(0x6022), be(0x4D80),
+	     /* 3D68 */ be(0x5C97), be(0x6A69), be(0xAC6F), be(0x4645),
+	     /* 3D70 */ be(0x4400), be(0x0513), be(0x8069), be(0x6AC6),
+	     /* 3D78 */ be(0x5F95), be(0x5F70), be(0x8040), be(0x4A81),
+	     /* 3D80 */ be(0x0300), be(0xE703), be(0x0088), be(0x4A83),
+	     /* 3D88 */ be(0x40FF), be(0xFFFF), be(0xFD70), be(0x8040),
+	     /* 3D90 */ be(0x4A85), be(0x4FA8), be(0x4F8C), be(0x0070),
+	     /* 3D98 */ be(0xBE47), be(0x8847), be(0xBC78), be(0x6B89),
+	     /* 3DA0 */ be(0x6A80), be(0x6986), be(0x6B8E), be(0x6B80),
+	     /* 3DA8 */ be(0x6980), be(0x6A88), be(0x7C9F), be(0x866B),
+	     /* 3DB0 */ be(0x8765), be(0x46FF), be(0xE365), be(0xA679),
+	     /* 3DB8 */ be(0x4A40), be(0x4580), be(0x44BC), be(0x7000),
+	     /* 3DC0 */ be(0x8040), be(0x0802), be(0x10EF), be(0x0104),
+	     /* 3DC8 */ be(0x3860), be(0x5D5D), be(0x5682), be(0x1300),
+	     /* 3DD0 */ be(0x8648), be(0x8202), be(0x8082), be(0x598A),
+	     /* 3DD8 */ be(0x0280), be(0x2048), be(0x3060), be(0x8042),
+	     /* 3DE0 */ be(0x9259), be(0x865A), be(0x8258), be(0x8562),
+	     /* 3DE8 */ be(0x8062), be(0x8560), be(0x9257), be(0x8221),
+	     /* 3DF0 */ be(0x10FF), be(0xB757), be(0x9361), be(0x1019),
+	     /* 3DF8 */ be(0x8020), be(0x9043), be(0x8E43), be(0x845F),
+	     /* 3E00 */ be(0x835D), be(0x805D), be(0x8163), be(0x8063),
+	     /* 3E08 */ be(0xA060), be(0x9157), be(0x8260), be(0x5CFF),
+	     /* 3E10 */ be(0xFFFF), be(0xFFE5), be(0x1016), be(0x2048),
+	     /* 3E18 */ be(0x0802), be(0x1C60), be(0x0014), be(0x0060),
+	     /* 3E20 */ be(0x2205), be(0x8120), be(0x908F), be(0x6A80),
+	     /* 3E28 */ be(0x6982), be(0x5F9F), be(0x6F46), be(0x4544),
+	     /* 3E30 */ be(0x0005), be(0x8013), be(0x8069), be(0x6A80),
+	     /* 3E38 */ be(0x7000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3E40 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3E48 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3E50 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3E58 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3E60 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3E68 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3E70 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3E78 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3E80 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3E88 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3E90 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3E98 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3EA0 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3EA8 */ be(0x0000), be(0x0000), be(0x0000), be(0x0000),
+	     /* 3EB0 */ be(0x0000), be(0x0000), be(0x0000)),
+
+	REGS(be(0x3EB6), be(0x004C)), /* ECL */
+
+	REGS(be(0x3EBA),
+	     be(0xAAAD),  /* 3EBA */
+	     be(0x0086)), /* 3EBC: Bias currents for FSC/ECL */
+
+	REGS(be(0x3EC0),
+	     be(0x1E00),  /* 3EC0: SFbin/SH mode settings */
+	     be(0x100A),  /* 3EC2: CLK divider for ramp for 10 bit 400MH */
+	     be(0x3300),  /* 3EC4: FSC clamps for HDR mode and adc comp power
+			     down co */
+	     be(0xEA44),  /* 3EC6: VLN and clk gating controls */
+	     be(0x6F6F),  /* 3EC8: Txl0 and Txlo1 settings for normal mode */
+	     be(0x2F4A),  /* 3ECA: CDAC/Txlo2/RSTGHI/RSTGLO settings */
+	     be(0x0506),  /* 3ECC: RSTDHI/RSTDLO/CDAC/TXHI settings */
+	     be(0x203B),  /* 3ECE: Ramp buffer settings and Booster enable
+			     (bits 0-5) */
+	     be(0x13F0),  /* 3ED0: TXLO from atest/sf bin settings */
+	     be(0xA53D),  /* 3ED2: Ramp offset */
+	     be(0x862F),  /* 3ED4: TXLO open loop/row driver settings */
+	     be(0x4081),  /* 3ED6: Txlatch fr cfpn rows/vln bias */
+	     be(0x8003),  /* 3ED8: Ramp step setting for 10 bit 400 Mhz */
+	     be(0xA580),  /* 3EDA: Ramp Offset */
+	     be(0xC000),  /* 3EDC: over range for rst and under range for sig */
+	     be(0xC103)), /* 3EDE: over range for sig and col dec clk settings */
+
+	/* corrections_recommended_bayer */
+	REGS(be(0x3F00),
+	     be(0x0017),  /* 3F00: BM_T0 */
+	     be(0x02DD),  /* 3F02: BM_T1 */
+	     /* 3F04: if Ana_gain less than 2, use noise_floor0, multipl */
+	     be(0x0020),
+	     /* 3F06: if Ana_gain between 4 and 7, use noise_floor2 and */
+	     be(0x0040),
+	     /* 3F08: if Ana_gain between 4 and 7, use noise_floor2 and */
+	     be(0x0070),
+	     /* 3F0A: Define noise_floor0(low address) and noise_floor1 */
+	     be(0x0101),
+	     be(0x0302)), /* 3F0C: Define noise_floor2 and noise_floor3 */
+
+	REGS(be(0x3F10),
+	     be(0x0505),  /* 3F10: single k factor 0 */
+	     be(0x0505),  /* 3F12: single k factor 1 */
+	     be(0x0505),  /* 3F14: single k factor 2 */
+	     be(0x01FF),  /* 3F16: cross factor 0 */
+	     be(0x01FF),  /* 3F18: cross factor 1 */
+	     be(0x01FF),  /* 3F1A: cross factor 2 */
+	     be(0x0022)), /* 3F1E */
+
+	/* GTH_THRES_RTN: 4max,4min filtered out of every 46 samples and */
+	REGS(be(0x3F2C), be(0x442E)),
+
+	REGS(be(0x3F3E),
+	     be(0x0000),  /* 3F3E: Switch ADC from 12 bit to 10 bit mode */
+	     be(0x1511),  /* 3F40: couple k factor 0 */
+	     be(0x1511),  /* 3F42: couple k factor 1 */
+	     be(0x0707)), /* 3F44: couple k factor 2 */
+};
+
+static void ar0521_power_off(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ar0521_dev *sensor = to_ar0521_dev(sd);
+	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 device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ar0521_dev *sensor = to_ar0521_dev(sd);
+	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)
+		/* deassert RESET signal */
+		gpiod_set_value(sensor->reset_gpio, 0);
+	usleep_range(4500, 5000); /* min 45000 clocks */
+
+	for (cnt = 0; cnt < ARRAY_SIZE(initial_regs); cnt++)
+		if (ar0521_write_regs(sensor, initial_regs[cnt].data,
+				      initial_regs[cnt].count))
+			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(dev);
+	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_pre_streamon(struct v4l2_subdev *sd, u32 flags)
+{
+	struct ar0521_dev *sensor = to_ar0521_dev(sd);
+	int ret;
+
+	if (!(flags & V4L2_SUBDEV_PRE_STREAMON_FL_MANUAL_LP))
+		return 0;
+
+	/* 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 LP-11 */
+	return ar0521_write_reg(sensor, AR0521_REG_RESET,
+				AR0521_REG_RESET_DEFAULTS |
+				AR0521_REG_RESET_STREAM);
+}
+
+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,
+};
+
+static const struct v4l2_subdev_video_ops ar0521_video_ops = {
+	.s_stream = ar0521_s_stream,
+	.pre_streamon = ar0521_pre_streamon,
+};
+
+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.width = AR0521_WIDTH_MAX;
+	sensor->fmt.height = AR0521_HEIGHT_MAX;
+	ar0521_adj_fmt(&sensor->fmt);
+
+	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);
+	}
+
+	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;
+	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;
+
+	/* Turn on the device and enable runtime PM */
+	ret = ar0521_power_on(&client->dev);
+	if (ret)
+		goto disable;
+	pm_runtime_set_active(&client->dev);
+	pm_runtime_enable(&client->dev);
+	dev_dbg(dev, "AR0521 driver initialized, master clock frequency: %u Hz, %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:
+	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);
+	if (!pm_runtime_status_suspended(&client->dev))
+		ar0521_power_off(&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)
+	SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL)
+};
+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");

Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor

[Joe Perches]

On Thu, 2021-12-23 at 08:06 +0100, Krzysztof Hałasa wrote:
> The driver has been extensively tested in an i.MX6-based system.
> AR0521 is a 5.7 mm x 4.3 mm, 5 MPix RGGB MIPI/HiSPi BSI CMOS sensor
> from On Semiconductor.

trivial notes:

> diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
[]
> +/* External clock (extclk) frequencies */
> +#define AR0521_EXTCLK_MIN	  (10 * 1000 * 1000)

Generally, adding a prefix like AR0521_ to defines that are
locally defined in a single file unnecessarily increases
identifier length.

It makes using short line lengths difficult.

e.g. Using this identifier anywhere

> +#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80

Many of the 80 column line lengths and line wrapping used in this
file are not really nice to read.  I believe you don't have to be
strict about 80 column lines.

> +#define be		cpu_to_be16

It's a pity there's no way to declare an array with all members
having a specific endianness.  Making sure all elements in these
arrays are declared with be() is tedious.

> +#define AR0521_NUM_SUPPLIES ARRAY_SIZE(ar0521_supply_names)

It's almost always better to use ARRAY_SIZE directly and not
use a #define for the array size.

> +static int ar0521_set_gains(struct ar0521_dev *sensor)
> +{
[]
> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);

ftrace works and perhaps all the similar debug logging uses aren't
really necessary.

Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor

[Jacopo Mondi]

Hi Joe,
  sorry to jump in

On Thu, Dec 23, 2021 at 09:49:58AM -0800, Joe Perches wrote:
> On Thu, 2021-12-23 at 08:06 +0100, Krzysztof Hałasa wrote:
> > The driver has been extensively tested in an i.MX6-based system.
> > AR0521 is a 5.7 mm x 4.3 mm, 5 MPix RGGB MIPI/HiSPi BSI CMOS sensor
> > from On Semiconductor.
>
> trivial notes:
>
> > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> []
> > +/* External clock (extclk) frequencies */
> > +#define AR0521_EXTCLK_MIN	  (10 * 1000 * 1000)
>
> Generally, adding a prefix like AR0521_ to defines that are
> locally defined in a single file unnecessarily increases
> identifier length.
>
> It makes using short line lengths difficult.
>
> e.g. Using this identifier anywhere
>
> > +#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80
>
> Many of the 80 column line lengths and line wrapping used in this
> file are not really nice to read.  I believe you don't have to be
> strict about 80 column lines.
>

Krzysztof first version had much longer lines, and in facts it has
been asked by me to reduce them to 80 cols. The media subsystem
requires to validate patches with

        ./scripts/checkpatch.pl --strict --max-line-length=80

We longly debated this and I believe it's now generally accepted to go
over 80 when it makes sense, but not regularly span to 120 cols like
in the previous version.

I think this 80-cols limit not being an hard limit anymore is doing
more worse than good, as each subsystem applies a different rule, and
I know how frustrating is for Krzysztof to be pushed in different
direction, as the same happened to me when I contributed to other
subsystems and I've been asked to span to 100 cols while I was trying
to stay in 80 no matter what.

Thanks
   j

> > +#define be		cpu_to_be16
>
> It's a pity there's no way to declare an array with all members
> having a specific endianness.  Making sure all elements in these
> arrays are declared with be() is tedious.
>
> > +#define AR0521_NUM_SUPPLIES ARRAY_SIZE(ar0521_supply_names)
>
> It's almost always better to use ARRAY_SIZE directly and not
> use a #define for the array size.
>
> > +static int ar0521_set_gains(struct ar0521_dev *sensor)
> > +{
> []
> > +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
>
> ftrace works and perhaps all the similar debug logging uses aren't
> really necessary.
>
>

Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor

[Joe Perches]

On Thu, 2021-12-23 at 19:48 +0100, Jacopo Mondi wrote:
> Hi Joe,
>   sorry to jump in

No worries.  It's all just a bikeshed and doesn't really matter
to the correctness of the code.

> On Thu, Dec 23, 2021 at 09:49:58AM -0800, Joe Perches wrote:
> > On Thu, 2021-12-23 at 08:06 +0100, Krzysztof Hałasa wrote:
> > > The driver has been extensively tested in an i.MX6-based system.
> > > AR0521 is a 5.7 mm x 4.3 mm, 5 MPix RGGB MIPI/HiSPi BSI CMOS sensor
> > > from On Semiconductor.
> > 
> > trivial notes:
> > 
> > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> > []
> > > +/* External clock (extclk) frequencies */
> > > +#define AR0521_EXTCLK_MIN	  (10 * 1000 * 1000)
> > 
> > Generally, adding a prefix like AR0521_ to defines that are
> > locally defined in a single file unnecessarily increases
> > identifier length.
> > 
> > It makes using short line lengths difficult.
> > 
> > e.g. Using this identifier anywhere
> > 
> > > +#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80
> > 
> > Many of the 80 column line lengths and line wrapping used in this
> > file are not really nice to read.  I believe you don't have to be
> > strict about 80 column lines.
> > 
> 
> Krzysztof first version had much longer lines, and in facts it has
> been asked by me to reduce them to 80 cols. The media subsystem
> requires to validate patches with
> 
>         ./scripts/checkpatch.pl --strict --max-line-length=80
> 
> We longly debated this and I believe it's now generally accepted to go
> over 80 when it makes sense, but not regularly span to 120 cols like
> in the previous version.

IMO: Many of the lines here could be lengthened to < 100 to
improve readability.

> I think this 80-cols limit not being an hard limit anymore is doing
> more worse than good, as each subsystem applies a different rule, and
> I know how frustrating is for Krzysztof to be pushed in different
> direction, as the same happened to me when I contributed to other
> subsystems and I've been asked to span to 100 cols while I was trying
> to stay in 80 no matter what.

Up to you all.

But there's a tension between long identifiers and short lines.

And anything using a 55 character identifier basically guarantees
that the code will exceed 80 columns.

Using identifiers with 10 character or so is generally OK, but
there are dozens of longer identifiers specific to this code.

I'd suggest because of these long identifiers that the code
be restricted to 100 columns, but not strictly at 80.

And there are quite a few long lines in drivers/media/i2c and
espcially for drivers/media/

A few of them are grotesquely long.
Probably all of them are historic and don't warrant change.

Just for i2c:

$ git ls-files -- 'drivers/media/i2c/*.[ch]' | \
  xargs awk '{print length($0); }' | \
  sort -rn | uniq -c
      2 143
      1 141
      1 123
      4 118
      2 114
      1 111
      1 110
      1 109
      1 107
      1 105
      4 104
      2 102
      3 101
      1 100
      2 99
     11 98
      7 97
      8 96
      4 95
     11 94
      8 93
     19 92
     13 91
     28 90
     20 89
     28 88
     39 87
     18 86
     33 85
     42 84
     86 83
     38 82
     47 81
    167 80
    110 79
    155 78
    363 77
    230 76
    219 75
    217 74
    427 73
    695 72
    471 71
    538 70
    525 69
    679 68
    661 67
   1046 66
    757 65
   1002 64
    942 63
   1053 62
    967 61
   1018 60
   1132 59
   1307 58
   1366 57
   3206 56
   1240 55
   2191 54
   1829 53
   1719 52
   1503 51
   1795 50
   1714 49
   1640 48
   1567 47
   1550 46
   1880 45
   2155 44
   1780 43
   1880 42
   1854 41
   1962 40
   2031 39
   2009 38
   2022 37
   2240 36
   2252 35
   2152 34
   2178 33
   2074 32
   2185 31
   2462 30
   2478 29
   2186 28
   1988 27
   1846 26
   1926 25
   2177 24
   2048 23
   1804 22
   1267 21
   1414 20
   1563 19
   6154 18
   3619 17
   7222 16
   1682 15
   2685 14
   3037 13
   2142 12
   1704 11
   3013 10
   3191 9
   1609 8
    230 7
    461 6
    571 5
    878 4
   2790 3
   6524 2
   7732 1
  24729 0

And for all of drivers/media:

$ git ls-files -- 'drivers/media/*.[ch]' | \
  xargs awk '{print length($0);}' | \
  sort -rn | uniq -c
      1 338
      1 314
      1 268
      1 261
      1 255
      1 254
      1 242
      1 234
      1 228
      1 213
      1 207
      1 205
      1 198
      2 197
      3 192
      2 188
      2 177
      1 174
      2 172
      2 169
      3 168
      2 167
      1 166
      1 165
      1 164
      3 163
      2 162
      2 161
      2 160
      6 158
     10 157
      3 156
      2 155
      3 154
      2 153
     12 152
      8 151
     49 150
      4 148
      2 147
      3 146
      3 145
      5 144
      5 143
      1 142
      6 141
      7 140
      8 139
      6 138
     10 137
     14 136
     13 135
     14 134
     13 133
     11 132
      7 131
      6 130
     15 129
     21 128
     17 127
     13 126
     10 125
     13 124
     12 123
     25 122
     20 121
     25 120
     15 119
     18 118
     20 117
     23 116
     30 115
     23 114
     26 113
     35 112
     35 111
     40 110
     60 109
     50 108
     72 107
     42 106
     47 105
    105 104
     72 103
     90 102
    110 101
    144 100
     79 99
    122 98
    226 97
    644 96
    115 95
    135 94
    135 93
    166 92
    210 91
    227 90
    218 89
    208 88
    279 87
    292 86
   1260 85
   1122 84
    879 83
   1288 82
   1489 81
   2505 80
   6241 79
   3653 78
   5268 77
   2012 76
   2168 75
   2279 74
   3297 73
   4343 72
   3741 71
   4018 70
   4360 69
   4487 68
   4433 67
   6014 66
   6098 65
   6547 64
   6661 63
   7312 62
   7684 61
   7610 60
   8157 59
   9052 58
  10047 57
  12064 56
   9904 55
  11075 54
  11271 53
  13259 52
  11585 51
  15036 50
  13930 49
  15159 48
  14221 47
  13349 46
  14243 45
  15887 44
  17829 43
  16620 42
  17759 41
  17569 40
  16653 39
  17386 38
  17480 37
  18296 36
  18205 35
  18782 34
  18352 33
  18137 32
  19556 31
  19229 30
  19403 29
  19570 28
  19447 27
  19581 26
  19255 25
  19300 24
  17038 23
  18523 22
  15609 21
  16188 20
  14634 19
  19426 18
  20979 17
  21548 16
  13476 15
  16713 14
  18914 13
  17577 12
  12828 11
  19525 10
  21665 9
  13912 8
   3261 7
   5375 6
   6756 5
   8260 4
  23448 3
  48708 2
  55786 1
 182329 0

Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor

[Joe Perches]

On Thu, 2021-12-23 at 19:48 +0100, Jacopo Mondi wrote:
> The media subsystem requires to validate patches with
> 
>         ./scripts/checkpatch.pl --strict --max-line-length=80
> 
> We longly debated this and I believe it's now generally accepted to go
> over 80 when it makes sense, but not regularly span to 120 cols like
> in the previous version.

Where is this documented and do you have a link to the debate?

The archive for the i2c mailing list doesn't show much debate:

https://lore.kernel.org/linux-i2c/?q=%2280+columns%22
https://lore.kernel.org/linux-i2c/?q=%22line+length%22

Perhaps there should be a MAINTAINERS P: entry for this requirement.

From MAINTAINERS:

	P: Subsystem Profile document for more details submitting
	   patches to the given subsystem. This is either an in-tree file,
	   or a URI. See Documentation/maintainer/maintainer-entry-profile.rst
	   for details.

Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor

[Joe Perches]

On Thu, 2021-12-23 at 12:13 -0800, Joe Perches wrote:
> On Thu, 2021-12-23 at 19:48 +0100, Jacopo Mondi wrote:
> > The media subsystem requires to validate patches with
> > 
> >         ./scripts/checkpatch.pl --strict --max-line-length=80
> > 
> > We longly debated this and I believe it's now generally accepted to go
> > over 80 when it makes sense, but not regularly span to 120 cols like
> > in the previous version.
[]
> Perhaps there should be a MAINTAINERS P: entry for this requirement.
> 
> From MAINTAINERS:
> 
> 	P: Subsystem Profile document for more details submitting
> 	   patches to the given subsystem. This is either an in-tree file,
> 	   or a URI. See Documentation/maintainer/maintainer-entry-profile.rst
> 	   for details.

Perhaps:
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index cd55b83878e05..bbfcb8e7eef06 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11977,6 +11977,7 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 W:	https://linuxtv.org
 Q:	http://patchwork.kernel.org/project/linux-media/list/
+P:	https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/maintainer-entry-profile.html
 T:	git git://linuxtv.org/media_tree.git
 F:	Documentation/admin-guide/media/
 F:	Documentation/devicetree/bindings/media/

Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor

[Jacopo Mondi]

Hi Joe

On Thu, Dec 23, 2021 at 12:13:10PM -0800, Joe Perches wrote:
> On Thu, 2021-12-23 at 19:48 +0100, Jacopo Mondi wrote:
> > The media subsystem requires to validate patches with
> >
> >         ./scripts/checkpatch.pl --strict --max-line-length=80
> >
> > We longly debated this and I believe it's now generally accepted to go
> > over 80 when it makes sense, but not regularly span to 120 cols like
> > in the previous version.
>
> Where is this documented and do you have a link to the debate?

It's in the subsystem maintainer profile
Documentation/driver-api/media/maintainer-entry-profile.rst

Where of course some exceptions are listed but it's anyway enforced
that "efforts should be made towards staying within 80
characters per line"

    - on strings, as they shouldn't be broken due to line length limits;
    - when a function or variable name need to have a big identifier name,
      which keeps hard to honor the 80 columns limit;
    - on arithmetic expressions, when breaking lines makes them harder to
      read;
    - when they avoid a line to end with an open parenthesis or an open
      bracket.

The debate I mentioned was specifically on the previous version of the
driver where me and Krzysztof shown quite different understanding of
coding style requirements.
https://patchwork.linuxtv.org/project/linux-media/patch/m3fstfoexa.fsf@t19.piap.pl/

That lead me to submit this
https://patchwork.linuxtv.org/project/linux-media/patch/20211013092005.14268-1-jacopo@jmondi.org/
which I never managed to re-send, my bad.


>
> The archive for the i2c mailing list doesn't show much debate:
>
> https://lore.kernel.org/linux-i2c/?q=%2280+columns%22
> https://lore.kernel.org/linux-i2c/?q=%22line+length%22
>
> Perhaps there should be a MAINTAINERS P: entry for this requirement.
>
> From MAINTAINERS:
>
> 	P: Subsystem Profile document for more details submitting
> 	   patches to the given subsystem. This is either an in-tree file,
> 	   or a URI. See Documentation/maintainer/maintainer-entry-profile.rst
> 	   for details.
>
>

Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor

[Joe Perches]

On Fri, 2021-12-24 at 10:22 +0100, Jacopo Mondi wrote:
> Hi Joe

hi again.

> On Thu, Dec 23, 2021 at 12:13:10PM -0800, Joe Perches wrote:
> > On Thu, 2021-12-23 at 19:48 +0100, Jacopo Mondi wrote:
> > > The media subsystem requires to validate patches with
> > > 
> > >         ./scripts/checkpatch.pl --strict --max-line-length=80
> > > 
> > > We longly debated this and I believe it's now generally accepted to go
> > > over 80 when it makes sense, but not regularly span to 120 cols like
> > > in the previous version.
> > 
> > Where is this documented and do you have a link to the debate?
> 
> It's in the subsystem maintainer profile
> Documentation/driver-api/media/maintainer-entry-profile.rst
> 
> Where of course some exceptions are listed but it's anyway enforced
> that "efforts should be made towards staying within 80
> characters per line"
> 
>     - on strings, as they shouldn't be broken due to line length limits;
>     - when a function or variable name need to have a big identifier name,
>       which keeps hard to honor the 80 columns limit;
>     - on arithmetic expressions, when breaking lines makes them harder to
>       read;
>     - when they avoid a line to end with an open parenthesis or an open
>       bracket.
> 
> The debate I mentioned was specifically on the previous version of the
> driver where me and Krzysztof shown quite different understanding of
> coding style requirements.
> https://patchwork.linuxtv.org/project/linux-media/patch/m3fstfoexa.fsf@t19.piap.pl/

Thanks for that.

> That lead me to submit this
> https://patchwork.linuxtv.org/project/linux-media/patch/20211013092005.14268-1-jacopo@jmondi.org/

That too.

FWIW, I believe using more than 100 columns or so makes it more
difficult to track quickly and efficiently to the next line.
Reading with multiple visual saccades on a single line is slow.

And IMO:

o reverse xmas tree declarations is quite a poor style requirement
o single line c99 // comments should be encouraged/preferred
o identifiers longer than 20 characters or so should be discouraged

Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

Hello Joe,

Joe Perches <joe@perches.com> writes:

>> +/* External clock (extclk) frequencies */
>> +#define AR0521_EXTCLK_MIN	  (10 * 1000 * 1000)
>
> Generally, adding a prefix like AR0521_ to defines that are
> locally defined in a single file unnecessarily increases
> identifier length.

Right. In general, I don't do that (for that very reason), however in
drivers/media this looks like a common practice and I didn't want to
break it.

> e.g. Using this identifier anywhere
>
>> +#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80

Right. However, such a name helps looking this up in the docs.
E.g. the register name in the docs is "hispi_control_status" and the
bitfield is "framer_test_mode" or something like that.
Since it's just one register (+ value) and it actually fits in 80
columns without too much problems, I'd rather like to leave it
unchanged.

> Many of the 80 column line lengths and line wrapping used in this
> file are not really nice to read.  I believe you don't have to be
> strict about 80 column lines.

Well, personally I think we could all switch to VT100's 132 columns.
Introduced in '78 :-) That's what I currently use for non-kernel tasks
(not the VT100 but just the line length). OTOH I'm using that emacs
wrapping mode so longer lines aren't a problem either.
But here, in drivers/media, I'm told 80 column is strict.

>> +#define be		cpu_to_be16
>
> It's a pity there's no way to declare an array with all members
> having a specific endianness.  Making sure all elements in these
> arrays are declared with be() is tedious.

Right. Unfortunately anything else would mean recoding.

>> +#define AR0521_NUM_SUPPLIES ARRAY_SIZE(ar0521_supply_names)
>
> It's almost always better to use ARRAY_SIZE directly and not
> use a #define for the array size.

It's another custom in drivers/media, but I guess I don't have to follow
it closely, do I? I never liked the #define.

>> +static int ar0521_set_gains(struct ar0521_dev *sensor)
>> +{
> []
>> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
>
> ftrace works and perhaps all the similar debug logging uses aren't
> really necessary.

TBH I've never used ftrace.
It appears that it can't show the arguments, can it?
If not, I'd rather leave these dev_dbg()s in place - like other
drivers/media/* in fact.
However obviously the code without deb_dbg()s would be cleaner, so if
ftrace can show the (formatted) arguments, I'm all for it.

Thanks for looking at this,
-- 
Krzysztof "Chris" Hałasa

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

Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor

[Krzysztof Hałasa]

Being that "stubborn developer"...

I think all those always increasing restrictions are a dead end and
serve no useful purpose.

I strongly oppose restrictions which are just for the sake of
uniformity. I think the coding style rules make sense only as long as
they increase readability, and should never extend further. And in
particular, we should NEVER sacrifice readability for uniformity.

Unfortunately one can't measure readability easily, and what's better
for one, is worse for another (think - vision problems).

Yes, some arbitrary basic rules like tab size and brace style are
needed, and there are certain rules needed for correctness (like
"type* ptr" vs "type *ptr" which is quite visible in "type* a, b")
but the rest should serve the readability. I.e., we shouldn't want to
have all code identical - we should want it to be good.

E.g., for me, it's better to have 99 worse source files (like wildly
formatted to 80 columns) and 1 (perhaps simply more recent) file with
a better, cleaner formatting than to have 100 of the former kind.

But what do I know...
-- 
Krzysztof "Chris" Hałasa

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

Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor

[kernel test robot]

Hi "Krzysztof,

I love your patch! Yet something to improve:

[auto build test ERROR on media-tree/master]
[also build test ERROR on linus/master v5.16-rc7 next-20211224]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Krzysztof-Ha-asa/On-Semi-AR0521-sensor-driver/20211223-150758
base:   git://linuxtv.org/media_tree.git master
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20220101/202201010737.V5A5o9x5-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/664482ab74a2331a7a7ead9256b0455cfc3334c7
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Krzysztof-Ha-asa/On-Semi-AR0521-sensor-driver/20211223-150758
        git checkout 664482ab74a2331a7a7ead9256b0455cfc3334c7
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/device.h:25,
                    from include/linux/pm_runtime.h:11,
                    from drivers/media/i2c/ar0521.c:10:
>> drivers/media/i2c/ar0521.c:1029:21: error: initialization of 'int (*)(struct device *)' from incompatible pointer type 'void (*)(struct device *)' [-Werror=incompatible-pointer-types]
    1029 |  SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL)
         |                     ^~~~~~~~~~~~~~~~
   include/linux/pm.h:341:21: note: in definition of macro 'SET_RUNTIME_PM_OPS'
     341 |  .runtime_suspend = suspend_fn, \
         |                     ^~~~~~~~~~
   drivers/media/i2c/ar0521.c:1029:21: note: (near initialization for 'ar0521_pm_ops.runtime_suspend')
    1029 |  SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL)
         |                     ^~~~~~~~~~~~~~~~
   include/linux/pm.h:341:21: note: in definition of macro 'SET_RUNTIME_PM_OPS'
     341 |  .runtime_suspend = suspend_fn, \
         |                     ^~~~~~~~~~
   cc1: some warnings being treated as errors


vim +1029 drivers/media/i2c/ar0521.c

  1026	
  1027	static const struct dev_pm_ops ar0521_pm_ops = {
  1028		SET_SYSTEM_SLEEP_PM_OPS(ar0521_suspend, ar0521_resume)
> 1029		SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL)
  1030	};
  1031	static const struct of_device_id ar0521_dt_ids[] = {
  1032		{.compatible = "onnn,ar0521"},
  1033		{}
  1034	};
  1035	MODULE_DEVICE_TABLE(of, ar0521_dt_ids);
  1036	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2992 bytes --]

Hi "Krzysztof,

I love your patch! Yet something to improve:

[auto build test ERROR on media-tree/master]
[also build test ERROR on linus/master v5.16-rc7 next-20211224]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Krzysztof-Ha-asa/On-Semi-AR0521-sensor-driver/20211223-150758
base:   git://linuxtv.org/media_tree.git master
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20220101/202201010737.V5A5o9x5-lkp(a)intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/664482ab74a2331a7a7ead9256b0455cfc3334c7
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Krzysztof-Ha-asa/On-Semi-AR0521-sensor-driver/20211223-150758
        git checkout 664482ab74a2331a7a7ead9256b0455cfc3334c7
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/device.h:25,
                    from include/linux/pm_runtime.h:11,
                    from drivers/media/i2c/ar0521.c:10:
>> drivers/media/i2c/ar0521.c:1029:21: error: initialization of 'int (*)(struct device *)' from incompatible pointer type 'void (*)(struct device *)' [-Werror=incompatible-pointer-types]
    1029 |  SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL)
         |                     ^~~~~~~~~~~~~~~~
   include/linux/pm.h:341:21: note: in definition of macro 'SET_RUNTIME_PM_OPS'
     341 |  .runtime_suspend = suspend_fn, \
         |                     ^~~~~~~~~~
   drivers/media/i2c/ar0521.c:1029:21: note: (near initialization for 'ar0521_pm_ops.runtime_suspend')
    1029 |  SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL)
         |                     ^~~~~~~~~~~~~~~~
   include/linux/pm.h:341:21: note: in definition of macro 'SET_RUNTIME_PM_OPS'
     341 |  .runtime_suspend = suspend_fn, \
         |                     ^~~~~~~~~~
   cc1: some warnings being treated as errors


vim +1029 drivers/media/i2c/ar0521.c

  1026	
  1027	static const struct dev_pm_ops ar0521_pm_ops = {
  1028		SET_SYSTEM_SLEEP_PM_OPS(ar0521_suspend, ar0521_resume)
> 1029		SET_RUNTIME_PM_OPS(ar0521_power_off, ar0521_power_on, NULL)
  1030	};
  1031	static const struct of_device_id ar0521_dt_ids[] = {
  1032		{.compatible = "onnn,ar0521"},
  1033		{}
  1034	};
  1035	MODULE_DEVICE_TABLE(of, ar0521_dt_ids);
  1036	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org