[PATCH v2 00/20] media: i2c: imx290: Miscellaneous improvements

[PATCH v2 00/20] media: i2c: imx290: Miscellaneous improvements

[Laurent Pinchart]

Hello,

This patch series gathers miscellaneous improvements for the imx290
driver. The most notable changes on the kernel side is patch 08/20 that
simplifies register access, and on the userspace API side patches 15/20,
16/20 and 19/20 that extend the driver with controls and selection
rectangles required by libcamera.

Laurent Pinchart (20):
  media: dt-bindings: Convert imx290.txt to YAML
  media: i2c: imx290: Use device lock for the control handler
  media: i2c: imx290: Print error code when I2C transfer fails
  media: i2c: imx290: Replace macro with explicit ARRAY_SIZE()
  media: i2c: imx290: Drop imx290_write_buffered_reg()
  media: i2c: imx290: Drop regmap cache
  media: i2c: imx290: Specify HMAX values in decimal
  media: i2c: imx290: Support variable-sized registers
  media: i2c: imx290: Correct register sizes
  media: i2c: imx290: Simplify error handling when writing registers
  media: i2c: imx290: Define more register macros
  media: i2c: imx290: Add exposure time control
  media: i2c: imx290: Fix max gain value
  media: i2c: imx290: Split control initialization to separate function
  media: i2c: imx290: Implement HBLANK and VBLANK controls
  media: i2c: imx290: Create controls for fwnode properties
  media: i2c: imx290: Move registers with fixed value to init array
  media: i2c: imx290: Factor out format retrieval to separate function
  media: i2c: imx290: Add crop selection targets support
  media: i2c: imx290: Replace GAIN control with ANALOGUE_GAIN

 .../devicetree/bindings/media/i2c/imx290.txt  |  57 --
 .../bindings/media/i2c/sony,imx290.yaml       | 129 +++
 MAINTAINERS                                   |   2 +-
 drivers/media/i2c/imx290.c                    | 784 ++++++++++--------
 4 files changed, 591 insertions(+), 381 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/media/i2c/imx290.txt
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml

-- 
Regards,

Laurent Pinchart

[PATCH v2 01/20] media: dt-bindings: Convert imx290.txt to YAML

[Laurent Pinchart]

Convert the Sony IMX290 DT binding from text to YAML. Add Manivannan as
a maintainer given that he is listed in MAINTAINERS for the file, as
volunteering myself.

The name of the input clock, "xclk", is wrong as the hardware manual
names it INCK. As the device has a single clock, the name could be
omitted, but that would require a corresponding change to the driver and
is thus a candidate for further patches.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../devicetree/bindings/media/i2c/imx290.txt  |  57 --------
 .../bindings/media/i2c/sony,imx290.yaml       | 129 ++++++++++++++++++
 MAINTAINERS                                   |   2 +-
 3 files changed, 130 insertions(+), 58 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/media/i2c/imx290.txt
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/imx290.txt b/Documentation/devicetree/bindings/media/i2c/imx290.txt
deleted file mode 100644
index a3cc21410f7c..000000000000
--- a/Documentation/devicetree/bindings/media/i2c/imx290.txt
+++ /dev/null
@@ -1,57 +0,0 @@
-* Sony IMX290 1/2.8-Inch CMOS Image Sensor
-
-The Sony IMX290 is a 1/2.8-Inch CMOS Solid-state image sensor with
-Square Pixel for Color Cameras. It is programmable through I2C and 4-wire
-interfaces. The sensor output is available via CMOS logic parallel SDR output,
-Low voltage LVDS DDR output and CSI-2 serial data output. The CSI-2 bus is the
-default. No bindings have been defined for the other busses.
-
-Required Properties:
-- compatible: Should be "sony,imx290"
-- reg: I2C bus address of the device
-- clocks: Reference to the xclk clock.
-- clock-names: Should be "xclk".
-- clock-frequency: Frequency of the xclk clock in Hz.
-- vdddo-supply: Sensor digital IO regulator.
-- vdda-supply: Sensor analog regulator.
-- vddd-supply: Sensor digital core regulator.
-
-Optional Properties:
-- reset-gpios: Sensor reset GPIO
-
-The imx290 device node should contain one 'port' child node with
-an 'endpoint' subnode. For further reading on port node refer to
-Documentation/devicetree/bindings/media/video-interfaces.txt.
-
-Required Properties on endpoint:
-- data-lanes: check ../video-interfaces.txt
-- link-frequencies: check ../video-interfaces.txt
-- remote-endpoint: check ../video-interfaces.txt
-
-Example:
-	&i2c1 {
-		...
-		imx290: camera-sensor@1a {
-			compatible = "sony,imx290";
-			reg = <0x1a>;
-
-			reset-gpios = <&msmgpio 35 GPIO_ACTIVE_LOW>;
-			pinctrl-names = "default";
-			pinctrl-0 = <&camera_rear_default>;
-
-			clocks = <&gcc GCC_CAMSS_MCLK0_CLK>;
-			clock-names = "xclk";
-			clock-frequency = <37125000>;
-
-			vdddo-supply = <&camera_vdddo_1v8>;
-			vdda-supply = <&camera_vdda_2v8>;
-			vddd-supply = <&camera_vddd_1v5>;
-
-			port {
-				imx290_ep: endpoint {
-					data-lanes = <1 2 3 4>;
-					link-frequencies = /bits/ 64 <445500000>;
-					remote-endpoint = <&csiphy0_ep>;
-				};
-			};
-		};
diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
new file mode 100644
index 000000000000..21377daae026
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
@@ -0,0 +1,129 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/sony,imx290.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sony IMX290 1/2.8-Inch CMOS Image Sensor
+
+maintainers:
+  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+
+description: |-
+  The Sony IMX290 is a 1/2.8-Inch CMOS Solid-state image sensor with Square
+  Pixel for Color Cameras. It is programmable through I2C and 4-wire
+  interfaces. The sensor output is available via CMOS logic parallel SDR
+  output, Low voltage LVDS DDR output and CSI-2 serial data output. The CSI-2
+  bus is the default. No bindings have been defined for the other busses.
+
+properties:
+  compatible:
+    enum:
+      - sony,imx290
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    description: Input clock (37.125 MHz or 74.25 MHz)
+    items:
+      - const: xclk
+
+  clock-frequency:
+    description: Frequency of the xclk clock in Hz
+
+  vdda-supply:
+    description: Analog power supply (2.9V)
+
+  vddd-supply:
+    description: Digital core power supply (1.2V)
+
+  vdddo-supply:
+    description: Digital I/O power supply (1.8V)
+
+  reset-gpios:
+    description: Sensor reset (XCLR) GPIO
+    maxItems: 1
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    description: |
+      Video output port
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            anyOf:
+              - items:
+                  - const: 1
+                  - const: 2
+              - items:
+                  - const: 1
+                  - const: 2
+                  - const: 3
+                  - const: 4
+
+          link-frequencies: true
+
+        required:
+          - data-lanes
+          - link-frequencies
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - clock-frequency
+  - vdda-supply
+  - vddd-supply
+  - vdddo-supply
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        imx290: camera-sensor@1a {
+            compatible = "sony,imx290";
+            reg = <0x1a>;
+
+            pinctrl-names = "default";
+            pinctrl-0 = <&camera_rear_default>;
+
+            clocks = <&gcc 90>;
+            clock-names = "xclk";
+            clock-frequency = <37125000>;
+
+            vdddo-supply = <&camera_vdddo_1v8>;
+            vdda-supply = <&camera_vdda_2v8>;
+            vddd-supply = <&camera_vddd_1v5>;
+
+            reset-gpios = <&msmgpio 35 GPIO_ACTIVE_LOW>;
+
+            port {
+                imx290_ep: endpoint {
+                    data-lanes = <1 2 3 4>;
+                    link-frequencies = /bits/ 64 <445500000>;
+                    remote-endpoint = <&csiphy0_ep>;
+                };
+            };
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 72b9654f764c..c431fd20e89b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18982,7 +18982,7 @@ M:	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
-F:	Documentation/devicetree/bindings/media/i2c/imx290.txt
+F:	Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
 F:	drivers/media/i2c/imx290.c
 
 SONY IMX319 SENSOR DRIVER
-- 
Regards,

Laurent Pinchart

[PATCH v2 02/20] media: i2c: imx290: Use device lock for the control handler

[Laurent Pinchart]

The link frequency and pixel rate controls are set without holding the
control handler lock, resulting in kernel warnings. As the value of
those controls depend on the format, the simplest fix is to use the
device lock for the control handler.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 99f2a50d39a4..d97a5fb1d501 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1043,6 +1043,7 @@ static int imx290_probe(struct i2c_client *client)
 	imx290_entity_init_cfg(&imx290->sd, NULL);
 
 	v4l2_ctrl_handler_init(&imx290->ctrls, 4);
+	imx290->ctrls.lock = &imx290->lock;
 
 	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
 			  V4L2_CID_GAIN, 0, 72, 1, 0);
-- 
Regards,

Laurent Pinchart

[PATCH v2 03/20] media: i2c: imx290: Print error code when I2C transfer fails

[Laurent Pinchart]

Knowing why I2C transfers fail is useful for debugging. Extend the error
message to print the error code.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index d97a5fb1d501..64bd43813dbf 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -370,7 +370,8 @@ static inline int __always_unused imx290_read_reg(struct imx290 *imx290, u16 add
 
 	ret = regmap_read(imx290->regmap, addr, &regval);
 	if (ret) {
-		dev_err(imx290->dev, "I2C read failed for addr: %x\n", addr);
+		dev_err(imx290->dev, "Failed to read register 0x%04x: %d\n",
+			addr, ret);
 		return ret;
 	}
 
@@ -385,7 +386,8 @@ static int imx290_write_reg(struct imx290 *imx290, u16 addr, u8 value)
 
 	ret = regmap_write(imx290->regmap, addr, value);
 	if (ret) {
-		dev_err(imx290->dev, "I2C write failed for addr: %x\n", addr);
+		dev_err(imx290->dev, "Failed to write register 0x%04x: %d\n",
+			addr, ret);
 		return ret;
 	}
 
-- 
Regards,

Laurent Pinchart

[PATCH v2 04/20] media: i2c: imx290: Replace macro with explicit ARRAY_SIZE()

[Laurent Pinchart]

Use ARRAY_SIZE(imx290->supplies) for code that needs the size of the
array, instead of relying on the IMX290_NUM_SUPPLIES. The result is less
error-prone as it ties the size to the array.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 64bd43813dbf..0f32f391b2e7 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -790,10 +790,10 @@ static int imx290_get_regulators(struct device *dev, struct imx290 *imx290)
 {
 	unsigned int i;
 
-	for (i = 0; i < IMX290_NUM_SUPPLIES; i++)
+	for (i = 0; i < ARRAY_SIZE(imx290->supplies); i++)
 		imx290->supplies[i].supply = imx290_supply_name[i];
 
-	return devm_regulator_bulk_get(dev, IMX290_NUM_SUPPLIES,
+	return devm_regulator_bulk_get(dev, ARRAY_SIZE(imx290->supplies),
 				       imx290->supplies);
 }
 
@@ -852,7 +852,8 @@ static int imx290_power_on(struct device *dev)
 		return ret;
 	}
 
-	ret = regulator_bulk_enable(IMX290_NUM_SUPPLIES, imx290->supplies);
+	ret = regulator_bulk_enable(ARRAY_SIZE(imx290->supplies),
+				    imx290->supplies);
 	if (ret) {
 		dev_err(dev, "Failed to enable regulators\n");
 		clk_disable_unprepare(imx290->xclk);
@@ -876,7 +877,7 @@ static int imx290_power_off(struct device *dev)
 
 	clk_disable_unprepare(imx290->xclk);
 	gpiod_set_value_cansleep(imx290->rst_gpio, 1);
-	regulator_bulk_disable(IMX290_NUM_SUPPLIES, imx290->supplies);
+	regulator_bulk_disable(ARRAY_SIZE(imx290->supplies), imx290->supplies);
 
 	return 0;
 }
-- 
Regards,

Laurent Pinchart

[PATCH v2 05/20] media: i2c: imx290: Drop imx290_write_buffered_reg()

[Laurent Pinchart]

The imx290_write_buffered_reg() function wraps a register write with
register hold, to enable changing multiple registers synchronously. It
is used for the gain only, which is an 8-bit register, defeating its
purpose.

The feature is useful, but should be implemented differently. Drop the
function for now, to prepare for a rework of register access.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 32 +-------------------------------
 1 file changed, 1 insertion(+), 31 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 0f32f391b2e7..5646f1704a1e 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -413,41 +413,11 @@ static int imx290_set_register_array(struct imx290 *imx290,
 	return 0;
 }
 
-static int imx290_write_buffered_reg(struct imx290 *imx290, u16 address_low,
-				     u8 nr_regs, u32 value)
-{
-	unsigned int i;
-	int ret;
-
-	ret = imx290_write_reg(imx290, IMX290_REGHOLD, 0x01);
-	if (ret) {
-		dev_err(imx290->dev, "Error setting hold register\n");
-		return ret;
-	}
-
-	for (i = 0; i < nr_regs; i++) {
-		ret = imx290_write_reg(imx290, address_low + i,
-				       (u8)(value >> (i * 8)));
-		if (ret) {
-			dev_err(imx290->dev, "Error writing buffered registers\n");
-			return ret;
-		}
-	}
-
-	ret = imx290_write_reg(imx290, IMX290_REGHOLD, 0x00);
-	if (ret) {
-		dev_err(imx290->dev, "Error setting hold register\n");
-		return ret;
-	}
-
-	return ret;
-}
-
 static int imx290_set_gain(struct imx290 *imx290, u32 value)
 {
 	int ret;
 
-	ret = imx290_write_buffered_reg(imx290, IMX290_GAIN, 1, value);
+	ret = imx290_write_reg(imx290, IMX290_GAIN, value);
 	if (ret)
 		dev_err(imx290->dev, "Unable to write gain\n");
 
-- 
Regards,

Laurent Pinchart

[PATCH v2 06/20] media: i2c: imx290: Drop regmap cache

[Laurent Pinchart]

Only two registers are ever read, and once only. There's no need to
cache values.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 5646f1704a1e..e1f91b4275c3 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -97,7 +97,6 @@ static const struct imx290_pixfmt imx290_formats[] = {
 static const struct regmap_config imx290_regmap_config = {
 	.reg_bits = 16,
 	.val_bits = 8,
-	.cache_type = REGCACHE_RBTREE,
 };
 
 static const char * const imx290_test_pattern_menu[] = {
-- 
Regards,

Laurent Pinchart

[PATCH v2 07/20] media: i2c: imx290: Specify HMAX values in decimal

[Laurent Pinchart]

The HMAX value specifies the total line length in pixels. It's thus more
readable in decimal than hexadecimal. Fix it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx290.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index e1f91b4275c3..711282126c34 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -307,7 +307,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
 	{
 		.width = 1920,
 		.height = 1080,
-		.hmax = 0x1130,
+		.hmax = 4400,
 		.link_freq_index = FREQ_INDEX_1080P,
 		.data = imx290_1080p_settings,
 		.data_size = ARRAY_SIZE(imx290_1080p_settings),
@@ -315,7 +315,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
 	{
 		.width = 1280,
 		.height = 720,
-		.hmax = 0x19c8,
+		.hmax = 6600,
 		.link_freq_index = FREQ_INDEX_720P,
 		.data = imx290_720p_settings,
 		.data_size = ARRAY_SIZE(imx290_720p_settings),
@@ -326,7 +326,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
 	{
 		.width = 1920,
 		.height = 1080,
-		.hmax = 0x0898,
+		.hmax = 2200,
 		.link_freq_index = FREQ_INDEX_1080P,
 		.data = imx290_1080p_settings,
 		.data_size = ARRAY_SIZE(imx290_1080p_settings),
@@ -334,7 +334,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
 	{
 		.width = 1280,
 		.height = 720,
-		.hmax = 0x0ce4,
+		.hmax = 3300,
 		.link_freq_index = FREQ_INDEX_720P,
 		.data = imx290_720p_settings,
 		.data_size = ARRAY_SIZE(imx290_720p_settings),
-- 
Regards,

Laurent Pinchart

[PATCH v2 08/20] media: i2c: imx290: Support variable-sized registers

[Laurent Pinchart]

The IMX290 has registers of different sizes. To simplify the code,
handle this in the read/write functions instead of in the callers by
encoding the register size in the symbolic name macros. All registers
are defined as 8-bit for now, a subsequent change will move to larger
registers where applicable.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Use IMX290_REG_SIZE_SHIFT in IMX290_REG_*BIT() macros
- Use unsigned integers in IMX290_REG_*BIT() macros
---
 drivers/media/i2c/imx290.c | 352 +++++++++++++++++++------------------
 1 file changed, 180 insertions(+), 172 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 711282126c34..6e4662caec62 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -22,22 +22,28 @@
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
-#define IMX290_STANDBY 0x3000
-#define IMX290_REGHOLD 0x3001
-#define IMX290_XMSTA 0x3002
-#define IMX290_FR_FDG_SEL 0x3009
-#define IMX290_BLKLEVEL_LOW 0x300a
-#define IMX290_BLKLEVEL_HIGH 0x300b
-#define IMX290_GAIN 0x3014
-#define IMX290_HMAX_LOW 0x301c
-#define IMX290_HMAX_HIGH 0x301d
-#define IMX290_PGCTRL 0x308c
-#define IMX290_PHY_LANE_NUM 0x3407
-#define IMX290_CSI_LANE_MODE 0x3443
+#define IMX290_REG_SIZE_SHIFT				16
+#define IMX290_REG_ADDR_MASK				0xffff
+#define IMX290_REG_8BIT(n)				((1U << IMX290_REG_SIZE_SHIFT) | (n))
+#define IMX290_REG_16BIT(n)				((2U << IMX290_REG_SIZE_SHIFT) | (n))
+#define IMX290_REG_24BIT(n)				((3U << IMX290_REG_SIZE_SHIFT) | (n))
 
-#define IMX290_PGCTRL_REGEN BIT(0)
-#define IMX290_PGCTRL_THRU BIT(1)
-#define IMX290_PGCTRL_MODE(n) ((n) << 4)
+#define IMX290_STANDBY					IMX290_REG_8BIT(0x3000)
+#define IMX290_REGHOLD					IMX290_REG_8BIT(0x3001)
+#define IMX290_XMSTA					IMX290_REG_8BIT(0x3002)
+#define IMX290_FR_FDG_SEL				IMX290_REG_8BIT(0x3009)
+#define IMX290_BLKLEVEL_LOW				IMX290_REG_8BIT(0x300a)
+#define IMX290_BLKLEVEL_HIGH				IMX290_REG_8BIT(0x300b)
+#define IMX290_GAIN					IMX290_REG_8BIT(0x3014)
+#define IMX290_HMAX_LOW					IMX290_REG_8BIT(0x301c)
+#define IMX290_HMAX_HIGH				IMX290_REG_8BIT(0x301d)
+#define IMX290_PGCTRL					IMX290_REG_8BIT(0x308c)
+#define IMX290_PHY_LANE_NUM				IMX290_REG_8BIT(0x3407)
+#define IMX290_CSI_LANE_MODE				IMX290_REG_8BIT(0x3443)
+
+#define IMX290_PGCTRL_REGEN				BIT(0)
+#define IMX290_PGCTRL_THRU				BIT(1)
+#define IMX290_PGCTRL_MODE(n)				((n) << 4)
 
 static const char * const imx290_supply_name[] = {
 	"vdda",
@@ -48,7 +54,7 @@ static const char * const imx290_supply_name[] = {
 #define IMX290_NUM_SUPPLIES ARRAY_SIZE(imx290_supply_name)
 
 struct imx290_regval {
-	u16 reg;
+	u32 reg;
 	u8 val;
 };
 
@@ -111,163 +117,163 @@ static const char * const imx290_test_pattern_menu[] = {
 };
 
 static const struct imx290_regval imx290_global_init_settings[] = {
-	{ 0x3007, 0x00 },
-	{ 0x3018, 0x65 },
-	{ 0x3019, 0x04 },
-	{ 0x301a, 0x00 },
-	{ 0x3444, 0x20 },
-	{ 0x3445, 0x25 },
-	{ 0x303a, 0x0c },
-	{ 0x3040, 0x00 },
-	{ 0x3041, 0x00 },
-	{ 0x303c, 0x00 },
-	{ 0x303d, 0x00 },
-	{ 0x3042, 0x9c },
-	{ 0x3043, 0x07 },
-	{ 0x303e, 0x49 },
-	{ 0x303f, 0x04 },
-	{ 0x304b, 0x0a },
-	{ 0x300f, 0x00 },
-	{ 0x3010, 0x21 },
-	{ 0x3012, 0x64 },
-	{ 0x3016, 0x09 },
-	{ 0x3070, 0x02 },
-	{ 0x3071, 0x11 },
-	{ 0x309b, 0x10 },
-	{ 0x309c, 0x22 },
-	{ 0x30a2, 0x02 },
-	{ 0x30a6, 0x20 },
-	{ 0x30a8, 0x20 },
-	{ 0x30aa, 0x20 },
-	{ 0x30ac, 0x20 },
-	{ 0x30b0, 0x43 },
-	{ 0x3119, 0x9e },
-	{ 0x311c, 0x1e },
-	{ 0x311e, 0x08 },
-	{ 0x3128, 0x05 },
-	{ 0x313d, 0x83 },
-	{ 0x3150, 0x03 },
-	{ 0x317e, 0x00 },
-	{ 0x32b8, 0x50 },
-	{ 0x32b9, 0x10 },
-	{ 0x32ba, 0x00 },
-	{ 0x32bb, 0x04 },
-	{ 0x32c8, 0x50 },
-	{ 0x32c9, 0x10 },
-	{ 0x32ca, 0x00 },
-	{ 0x32cb, 0x04 },
-	{ 0x332c, 0xd3 },
-	{ 0x332d, 0x10 },
-	{ 0x332e, 0x0d },
-	{ 0x3358, 0x06 },
-	{ 0x3359, 0xe1 },
-	{ 0x335a, 0x11 },
-	{ 0x3360, 0x1e },
-	{ 0x3361, 0x61 },
-	{ 0x3362, 0x10 },
-	{ 0x33b0, 0x50 },
-	{ 0x33b2, 0x1a },
-	{ 0x33b3, 0x04 },
+	{ IMX290_REG_8BIT(0x3007), 0x00 },
+	{ IMX290_REG_8BIT(0x3018), 0x65 },
+	{ IMX290_REG_8BIT(0x3019), 0x04 },
+	{ IMX290_REG_8BIT(0x301a), 0x00 },
+	{ IMX290_REG_8BIT(0x3444), 0x20 },
+	{ IMX290_REG_8BIT(0x3445), 0x25 },
+	{ IMX290_REG_8BIT(0x303a), 0x0c },
+	{ IMX290_REG_8BIT(0x3040), 0x00 },
+	{ IMX290_REG_8BIT(0x3041), 0x00 },
+	{ IMX290_REG_8BIT(0x303c), 0x00 },
+	{ IMX290_REG_8BIT(0x303d), 0x00 },
+	{ IMX290_REG_8BIT(0x3042), 0x9c },
+	{ IMX290_REG_8BIT(0x3043), 0x07 },
+	{ IMX290_REG_8BIT(0x303e), 0x49 },
+	{ IMX290_REG_8BIT(0x303f), 0x04 },
+	{ IMX290_REG_8BIT(0x304b), 0x0a },
+	{ IMX290_REG_8BIT(0x300f), 0x00 },
+	{ IMX290_REG_8BIT(0x3010), 0x21 },
+	{ IMX290_REG_8BIT(0x3012), 0x64 },
+	{ IMX290_REG_8BIT(0x3016), 0x09 },
+	{ IMX290_REG_8BIT(0x3070), 0x02 },
+	{ IMX290_REG_8BIT(0x3071), 0x11 },
+	{ IMX290_REG_8BIT(0x309b), 0x10 },
+	{ IMX290_REG_8BIT(0x309c), 0x22 },
+	{ IMX290_REG_8BIT(0x30a2), 0x02 },
+	{ IMX290_REG_8BIT(0x30a6), 0x20 },
+	{ IMX290_REG_8BIT(0x30a8), 0x20 },
+	{ IMX290_REG_8BIT(0x30aa), 0x20 },
+	{ IMX290_REG_8BIT(0x30ac), 0x20 },
+	{ IMX290_REG_8BIT(0x30b0), 0x43 },
+	{ IMX290_REG_8BIT(0x3119), 0x9e },
+	{ IMX290_REG_8BIT(0x311c), 0x1e },
+	{ IMX290_REG_8BIT(0x311e), 0x08 },
+	{ IMX290_REG_8BIT(0x3128), 0x05 },
+	{ IMX290_REG_8BIT(0x313d), 0x83 },
+	{ IMX290_REG_8BIT(0x3150), 0x03 },
+	{ IMX290_REG_8BIT(0x317e), 0x00 },
+	{ IMX290_REG_8BIT(0x32b8), 0x50 },
+	{ IMX290_REG_8BIT(0x32b9), 0x10 },
+	{ IMX290_REG_8BIT(0x32ba), 0x00 },
+	{ IMX290_REG_8BIT(0x32bb), 0x04 },
+	{ IMX290_REG_8BIT(0x32c8), 0x50 },
+	{ IMX290_REG_8BIT(0x32c9), 0x10 },
+	{ IMX290_REG_8BIT(0x32ca), 0x00 },
+	{ IMX290_REG_8BIT(0x32cb), 0x04 },
+	{ IMX290_REG_8BIT(0x332c), 0xd3 },
+	{ IMX290_REG_8BIT(0x332d), 0x10 },
+	{ IMX290_REG_8BIT(0x332e), 0x0d },
+	{ IMX290_REG_8BIT(0x3358), 0x06 },
+	{ IMX290_REG_8BIT(0x3359), 0xe1 },
+	{ IMX290_REG_8BIT(0x335a), 0x11 },
+	{ IMX290_REG_8BIT(0x3360), 0x1e },
+	{ IMX290_REG_8BIT(0x3361), 0x61 },
+	{ IMX290_REG_8BIT(0x3362), 0x10 },
+	{ IMX290_REG_8BIT(0x33b0), 0x50 },
+	{ IMX290_REG_8BIT(0x33b2), 0x1a },
+	{ IMX290_REG_8BIT(0x33b3), 0x04 },
 };
 
 static const struct imx290_regval imx290_1080p_settings[] = {
 	/* mode settings */
-	{ 0x3007, 0x00 },
-	{ 0x303a, 0x0c },
-	{ 0x3414, 0x0a },
-	{ 0x3472, 0x80 },
-	{ 0x3473, 0x07 },
-	{ 0x3418, 0x38 },
-	{ 0x3419, 0x04 },
-	{ 0x3012, 0x64 },
-	{ 0x3013, 0x00 },
-	{ 0x305c, 0x18 },
-	{ 0x305d, 0x03 },
-	{ 0x305e, 0x20 },
-	{ 0x305f, 0x01 },
-	{ 0x315e, 0x1a },
-	{ 0x3164, 0x1a },
-	{ 0x3480, 0x49 },
+	{ IMX290_REG_8BIT(0x3007), 0x00 },
+	{ IMX290_REG_8BIT(0x303a), 0x0c },
+	{ IMX290_REG_8BIT(0x3414), 0x0a },
+	{ IMX290_REG_8BIT(0x3472), 0x80 },
+	{ IMX290_REG_8BIT(0x3473), 0x07 },
+	{ IMX290_REG_8BIT(0x3418), 0x38 },
+	{ IMX290_REG_8BIT(0x3419), 0x04 },
+	{ IMX290_REG_8BIT(0x3012), 0x64 },
+	{ IMX290_REG_8BIT(0x3013), 0x00 },
+	{ IMX290_REG_8BIT(0x305c), 0x18 },
+	{ IMX290_REG_8BIT(0x305d), 0x03 },
+	{ IMX290_REG_8BIT(0x305e), 0x20 },
+	{ IMX290_REG_8BIT(0x305f), 0x01 },
+	{ IMX290_REG_8BIT(0x315e), 0x1a },
+	{ IMX290_REG_8BIT(0x3164), 0x1a },
+	{ IMX290_REG_8BIT(0x3480), 0x49 },
 	/* data rate settings */
-	{ 0x3405, 0x10 },
-	{ 0x3446, 0x57 },
-	{ 0x3447, 0x00 },
-	{ 0x3448, 0x37 },
-	{ 0x3449, 0x00 },
-	{ 0x344a, 0x1f },
-	{ 0x344b, 0x00 },
-	{ 0x344c, 0x1f },
-	{ 0x344d, 0x00 },
-	{ 0x344e, 0x1f },
-	{ 0x344f, 0x00 },
-	{ 0x3450, 0x77 },
-	{ 0x3451, 0x00 },
-	{ 0x3452, 0x1f },
-	{ 0x3453, 0x00 },
-	{ 0x3454, 0x17 },
-	{ 0x3455, 0x00 },
+	{ IMX290_REG_8BIT(0x3405), 0x10 },
+	{ IMX290_REG_8BIT(0x3446), 0x57 },
+	{ IMX290_REG_8BIT(0x3447), 0x00 },
+	{ IMX290_REG_8BIT(0x3448), 0x37 },
+	{ IMX290_REG_8BIT(0x3449), 0x00 },
+	{ IMX290_REG_8BIT(0x344a), 0x1f },
+	{ IMX290_REG_8BIT(0x344b), 0x00 },
+	{ IMX290_REG_8BIT(0x344c), 0x1f },
+	{ IMX290_REG_8BIT(0x344d), 0x00 },
+	{ IMX290_REG_8BIT(0x344e), 0x1f },
+	{ IMX290_REG_8BIT(0x344f), 0x00 },
+	{ IMX290_REG_8BIT(0x3450), 0x77 },
+	{ IMX290_REG_8BIT(0x3451), 0x00 },
+	{ IMX290_REG_8BIT(0x3452), 0x1f },
+	{ IMX290_REG_8BIT(0x3453), 0x00 },
+	{ IMX290_REG_8BIT(0x3454), 0x17 },
+	{ IMX290_REG_8BIT(0x3455), 0x00 },
 };
 
 static const struct imx290_regval imx290_720p_settings[] = {
 	/* mode settings */
-	{ 0x3007, 0x10 },
-	{ 0x303a, 0x06 },
-	{ 0x3414, 0x04 },
-	{ 0x3472, 0x00 },
-	{ 0x3473, 0x05 },
-	{ 0x3418, 0xd0 },
-	{ 0x3419, 0x02 },
-	{ 0x3012, 0x64 },
-	{ 0x3013, 0x00 },
-	{ 0x305c, 0x20 },
-	{ 0x305d, 0x00 },
-	{ 0x305e, 0x20 },
-	{ 0x305f, 0x01 },
-	{ 0x315e, 0x1a },
-	{ 0x3164, 0x1a },
-	{ 0x3480, 0x49 },
+	{ IMX290_REG_8BIT(0x3007), 0x10 },
+	{ IMX290_REG_8BIT(0x303a), 0x06 },
+	{ IMX290_REG_8BIT(0x3414), 0x04 },
+	{ IMX290_REG_8BIT(0x3472), 0x00 },
+	{ IMX290_REG_8BIT(0x3473), 0x05 },
+	{ IMX290_REG_8BIT(0x3418), 0xd0 },
+	{ IMX290_REG_8BIT(0x3419), 0x02 },
+	{ IMX290_REG_8BIT(0x3012), 0x64 },
+	{ IMX290_REG_8BIT(0x3013), 0x00 },
+	{ IMX290_REG_8BIT(0x305c), 0x20 },
+	{ IMX290_REG_8BIT(0x305d), 0x00 },
+	{ IMX290_REG_8BIT(0x305e), 0x20 },
+	{ IMX290_REG_8BIT(0x305f), 0x01 },
+	{ IMX290_REG_8BIT(0x315e), 0x1a },
+	{ IMX290_REG_8BIT(0x3164), 0x1a },
+	{ IMX290_REG_8BIT(0x3480), 0x49 },
 	/* data rate settings */
-	{ 0x3405, 0x10 },
-	{ 0x3446, 0x4f },
-	{ 0x3447, 0x00 },
-	{ 0x3448, 0x2f },
-	{ 0x3449, 0x00 },
-	{ 0x344a, 0x17 },
-	{ 0x344b, 0x00 },
-	{ 0x344c, 0x17 },
-	{ 0x344d, 0x00 },
-	{ 0x344e, 0x17 },
-	{ 0x344f, 0x00 },
-	{ 0x3450, 0x57 },
-	{ 0x3451, 0x00 },
-	{ 0x3452, 0x17 },
-	{ 0x3453, 0x00 },
-	{ 0x3454, 0x17 },
-	{ 0x3455, 0x00 },
+	{ IMX290_REG_8BIT(0x3405), 0x10 },
+	{ IMX290_REG_8BIT(0x3446), 0x4f },
+	{ IMX290_REG_8BIT(0x3447), 0x00 },
+	{ IMX290_REG_8BIT(0x3448), 0x2f },
+	{ IMX290_REG_8BIT(0x3449), 0x00 },
+	{ IMX290_REG_8BIT(0x344a), 0x17 },
+	{ IMX290_REG_8BIT(0x344b), 0x00 },
+	{ IMX290_REG_8BIT(0x344c), 0x17 },
+	{ IMX290_REG_8BIT(0x344d), 0x00 },
+	{ IMX290_REG_8BIT(0x344e), 0x17 },
+	{ IMX290_REG_8BIT(0x344f), 0x00 },
+	{ IMX290_REG_8BIT(0x3450), 0x57 },
+	{ IMX290_REG_8BIT(0x3451), 0x00 },
+	{ IMX290_REG_8BIT(0x3452), 0x17 },
+	{ IMX290_REG_8BIT(0x3453), 0x00 },
+	{ IMX290_REG_8BIT(0x3454), 0x17 },
+	{ IMX290_REG_8BIT(0x3455), 0x00 },
 };
 
 static const struct imx290_regval imx290_10bit_settings[] = {
-	{ 0x3005, 0x00},
-	{ 0x3046, 0x00},
-	{ 0x3129, 0x1d},
-	{ 0x317c, 0x12},
-	{ 0x31ec, 0x37},
-	{ 0x3441, 0x0a},
-	{ 0x3442, 0x0a},
-	{ 0x300a, 0x3c},
-	{ 0x300b, 0x00},
+	{ IMX290_REG_8BIT(0x3005), 0x00},
+	{ IMX290_REG_8BIT(0x3046), 0x00},
+	{ IMX290_REG_8BIT(0x3129), 0x1d},
+	{ IMX290_REG_8BIT(0x317c), 0x12},
+	{ IMX290_REG_8BIT(0x31ec), 0x37},
+	{ IMX290_REG_8BIT(0x3441), 0x0a},
+	{ IMX290_REG_8BIT(0x3442), 0x0a},
+	{ IMX290_REG_8BIT(0x300a), 0x3c},
+	{ IMX290_REG_8BIT(0x300b), 0x00},
 };
 
 static const struct imx290_regval imx290_12bit_settings[] = {
-	{ 0x3005, 0x01 },
-	{ 0x3046, 0x01 },
-	{ 0x3129, 0x00 },
-	{ 0x317c, 0x00 },
-	{ 0x31ec, 0x0e },
-	{ 0x3441, 0x0c },
-	{ 0x3442, 0x0c },
-	{ 0x300a, 0xf0 },
-	{ 0x300b, 0x00 },
+	{ IMX290_REG_8BIT(0x3005), 0x01 },
+	{ IMX290_REG_8BIT(0x3046), 0x01 },
+	{ IMX290_REG_8BIT(0x3129), 0x00 },
+	{ IMX290_REG_8BIT(0x317c), 0x00 },
+	{ IMX290_REG_8BIT(0x31ec), 0x0e },
+	{ IMX290_REG_8BIT(0x3441), 0x0c },
+	{ IMX290_REG_8BIT(0x3442), 0x0c },
+	{ IMX290_REG_8BIT(0x300a), 0xf0 },
+	{ IMX290_REG_8BIT(0x300b), 0x00 },
 };
 
 /* supported link frequencies */
@@ -362,33 +368,35 @@ static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
 	return container_of(_sd, struct imx290, sd);
 }
 
-static inline int __always_unused imx290_read_reg(struct imx290 *imx290, u16 addr, u8 *value)
+static int __always_unused imx290_read_reg(struct imx290 *imx290, u32 addr, u32 *value)
 {
-	unsigned int regval;
+	u8 data[3] = { 0, 0, 0 };
 	int ret;
 
-	ret = regmap_read(imx290->regmap, addr, &regval);
-	if (ret) {
-		dev_err(imx290->dev, "Failed to read register 0x%04x: %d\n",
-			addr, ret);
+	ret = regmap_raw_read(imx290->regmap, addr & IMX290_REG_ADDR_MASK,
+			      data, (addr >> IMX290_REG_SIZE_SHIFT) & 3);
+	if (ret < 0) {
+		dev_err(imx290->dev, "%u-bit read from 0x%04x failed: %d\n",
+			 ((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
+			 addr & IMX290_REG_ADDR_MASK, ret);
 		return ret;
 	}
 
-	*value = regval & 0xff;
-
+	*value = (data[2] << 16) | (data[1] << 8) | data[0];
 	return 0;
 }
 
-static int imx290_write_reg(struct imx290 *imx290, u16 addr, u8 value)
+static int imx290_write_reg(struct imx290 *imx290, u32 addr, u32 value)
 {
+	u8 data[3] = { value & 0xff, (value >> 8) & 0xff, value >> 16 };
 	int ret;
 
-	ret = regmap_write(imx290->regmap, addr, value);
-	if (ret) {
-		dev_err(imx290->dev, "Failed to write register 0x%04x: %d\n",
-			addr, ret);
-		return ret;
-	}
+	ret = regmap_raw_write(imx290->regmap, addr & IMX290_REG_ADDR_MASK,
+			       data, (addr >> IMX290_REG_SIZE_SHIFT) & 3);
+	if (ret < 0)
+		dev_err(imx290->dev, "%u-bit write to 0x%04x failed: %d\n",
+			 ((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
+			 addr & IMX290_REG_ADDR_MASK, ret);
 
 	return ret;
 }
-- 
Regards,

Laurent Pinchart

[PATCH v2 09/20] media: i2c: imx290: Correct register sizes

[Laurent Pinchart]

Define registers with the appropriate size, using the variable-size
register access mechanism that has just been introduced. This simplifies
the code.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx290.c | 39 +++++++++-----------------------------
 1 file changed, 9 insertions(+), 30 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 6e4662caec62..cfcfa04e83e1 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -32,12 +32,11 @@
 #define IMX290_REGHOLD					IMX290_REG_8BIT(0x3001)
 #define IMX290_XMSTA					IMX290_REG_8BIT(0x3002)
 #define IMX290_FR_FDG_SEL				IMX290_REG_8BIT(0x3009)
-#define IMX290_BLKLEVEL_LOW				IMX290_REG_8BIT(0x300a)
-#define IMX290_BLKLEVEL_HIGH				IMX290_REG_8BIT(0x300b)
+#define IMX290_BLKLEVEL					IMX290_REG_16BIT(0x300a)
 #define IMX290_GAIN					IMX290_REG_8BIT(0x3014)
-#define IMX290_HMAX_LOW					IMX290_REG_8BIT(0x301c)
-#define IMX290_HMAX_HIGH				IMX290_REG_8BIT(0x301d)
+#define IMX290_HMAX					IMX290_REG_16BIT(0x301c)
 #define IMX290_PGCTRL					IMX290_REG_8BIT(0x308c)
+#define IMX290_CHIP_ID					IMX290_REG_16BIT(0x319a)
 #define IMX290_PHY_LANE_NUM				IMX290_REG_8BIT(0x3407)
 #define IMX290_CSI_LANE_MODE				IMX290_REG_8BIT(0x3443)
 
@@ -461,8 +460,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	case V4L2_CID_TEST_PATTERN:
 		if (ctrl->val) {
-			imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW, 0x00);
-			imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
+			imx290_write_reg(imx290, IMX290_BLKLEVEL, 0);
 			usleep_range(10000, 11000);
 			imx290_write_reg(imx290, IMX290_PGCTRL,
 					 (u8)(IMX290_PGCTRL_REGEN |
@@ -472,12 +470,11 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 			imx290_write_reg(imx290, IMX290_PGCTRL, 0x00);
 			usleep_range(10000, 11000);
 			if (imx290->bpp == 10)
-				imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW,
+				imx290_write_reg(imx290, IMX290_BLKLEVEL,
 						 0x3c);
 			else /* 12 bits per pixel */
-				imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW,
+				imx290_write_reg(imx290, IMX290_BLKLEVEL,
 						 0xf0);
-			imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
 		}
 		break;
 	default:
@@ -669,25 +666,6 @@ static int imx290_write_current_format(struct imx290 *imx290)
 	return 0;
 }
 
-static int imx290_set_hmax(struct imx290 *imx290, u32 val)
-{
-	int ret;
-
-	ret = imx290_write_reg(imx290, IMX290_HMAX_LOW, (val & 0xff));
-	if (ret) {
-		dev_err(imx290->dev, "Error setting HMAX register\n");
-		return ret;
-	}
-
-	ret = imx290_write_reg(imx290, IMX290_HMAX_HIGH, ((val >> 8) & 0xff));
-	if (ret) {
-		dev_err(imx290->dev, "Error setting HMAX register\n");
-		return ret;
-	}
-
-	return 0;
-}
-
 /* Start streaming */
 static int imx290_start_streaming(struct imx290 *imx290)
 {
@@ -716,8 +694,9 @@ static int imx290_start_streaming(struct imx290 *imx290)
 		dev_err(imx290->dev, "Could not set current mode\n");
 		return ret;
 	}
-	ret = imx290_set_hmax(imx290, imx290->current_mode->hmax);
-	if (ret < 0)
+
+	ret = imx290_write_reg(imx290, IMX290_HMAX, imx290->current_mode->hmax);
+	if (ret)
 		return ret;
 
 	/* Apply customized values from user */
-- 
Regards,

Laurent Pinchart

[PATCH v2 10/20] media: i2c: imx290: Simplify error handling when writing registers

[Laurent Pinchart]

Error handling for register writes requires checking the error status of
every single write. This makes the code complex, or incorrect when the
checks are omitted. Simplify this by passing a pointer to an error code
to the imx290_write_reg() function, which allows writing multiple
registers in a row and only checking for errors at the end.

While at it, rename imx290_write_reg() to imx290_write() as there's
nothing else than registers to write, and rename imx290_read_reg()
accordingly.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 86 ++++++++++++++------------------------
 1 file changed, 32 insertions(+), 54 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index cfcfa04e83e1..0f26da5c2e54 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -367,7 +367,7 @@ static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
 	return container_of(_sd, struct imx290, sd);
 }
 
-static int __always_unused imx290_read_reg(struct imx290 *imx290, u32 addr, u32 *value)
+static int __always_unused imx290_read(struct imx290 *imx290, u32 addr, u32 *value)
 {
 	u8 data[3] = { 0, 0, 0 };
 	int ret;
@@ -385,17 +385,23 @@ static int __always_unused imx290_read_reg(struct imx290 *imx290, u32 addr, u32
 	return 0;
 }
 
-static int imx290_write_reg(struct imx290 *imx290, u32 addr, u32 value)
+static int imx290_write(struct imx290 *imx290, u32 addr, u32 value, int *err)
 {
 	u8 data[3] = { value & 0xff, (value >> 8) & 0xff, value >> 16 };
 	int ret;
 
+	if (err && *err)
+		return *err;
+
 	ret = regmap_raw_write(imx290->regmap, addr & IMX290_REG_ADDR_MASK,
 			       data, (addr >> IMX290_REG_SIZE_SHIFT) & 3);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(imx290->dev, "%u-bit write to 0x%04x failed: %d\n",
 			 ((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
 			 addr & IMX290_REG_ADDR_MASK, ret);
+		if (err)
+			*err = ret;
+	}
 
 	return ret;
 }
@@ -408,7 +414,7 @@ static int imx290_set_register_array(struct imx290 *imx290,
 	int ret;
 
 	for (i = 0; i < num_settings; ++i, ++settings) {
-		ret = imx290_write_reg(imx290, settings->reg, settings->val);
+		ret = imx290_write(imx290, settings->reg, settings->val, NULL);
 		if (ret < 0)
 			return ret;
 	}
@@ -419,29 +425,16 @@ static int imx290_set_register_array(struct imx290 *imx290,
 	return 0;
 }
 
-static int imx290_set_gain(struct imx290 *imx290, u32 value)
-{
-	int ret;
-
-	ret = imx290_write_reg(imx290, IMX290_GAIN, value);
-	if (ret)
-		dev_err(imx290->dev, "Unable to write gain\n");
-
-	return ret;
-}
-
 /* Stop streaming */
 static int imx290_stop_streaming(struct imx290 *imx290)
 {
-	int ret;
+	int ret = 0;
 
-	ret = imx290_write_reg(imx290, IMX290_STANDBY, 0x01);
-	if (ret < 0)
-		return ret;
+	imx290_write(imx290, IMX290_STANDBY, 0x01, &ret);
 
 	msleep(30);
 
-	return imx290_write_reg(imx290, IMX290_XMSTA, 0x01);
+	return imx290_write(imx290, IMX290_XMSTA, 0x01, &ret);
 }
 
 static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
@@ -456,25 +449,25 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case V4L2_CID_GAIN:
-		ret = imx290_set_gain(imx290, ctrl->val);
+		ret = imx290_write(imx290, IMX290_GAIN, ctrl->val, NULL);
 		break;
 	case V4L2_CID_TEST_PATTERN:
 		if (ctrl->val) {
-			imx290_write_reg(imx290, IMX290_BLKLEVEL, 0);
+			imx290_write(imx290, IMX290_BLKLEVEL, 0, &ret);
 			usleep_range(10000, 11000);
-			imx290_write_reg(imx290, IMX290_PGCTRL,
-					 (u8)(IMX290_PGCTRL_REGEN |
-					 IMX290_PGCTRL_THRU |
-					 IMX290_PGCTRL_MODE(ctrl->val)));
+			imx290_write(imx290, IMX290_PGCTRL,
+				     (u8)(IMX290_PGCTRL_REGEN |
+				     IMX290_PGCTRL_THRU |
+				     IMX290_PGCTRL_MODE(ctrl->val)), &ret);
 		} else {
-			imx290_write_reg(imx290, IMX290_PGCTRL, 0x00);
+			imx290_write(imx290, IMX290_PGCTRL, 0x00, &ret);
 			usleep_range(10000, 11000);
 			if (imx290->bpp == 10)
-				imx290_write_reg(imx290, IMX290_BLKLEVEL,
-						 0x3c);
+				imx290_write(imx290, IMX290_BLKLEVEL, 0x3c,
+					     &ret);
 			else /* 12 bits per pixel */
-				imx290_write_reg(imx290, IMX290_BLKLEVEL,
-						 0xf0);
+				imx290_write(imx290, IMX290_BLKLEVEL, 0xf0,
+					     &ret);
 		}
 		break;
 	default:
@@ -695,7 +688,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
 		return ret;
 	}
 
-	ret = imx290_write_reg(imx290, IMX290_HMAX, imx290->current_mode->hmax);
+	ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax,
+			   NULL);
 	if (ret)
 		return ret;
 
@@ -706,14 +700,12 @@ static int imx290_start_streaming(struct imx290 *imx290)
 		return ret;
 	}
 
-	ret = imx290_write_reg(imx290, IMX290_STANDBY, 0x00);
-	if (ret < 0)
-		return ret;
+	imx290_write(imx290, IMX290_STANDBY, 0x00, &ret);
 
 	msleep(30);
 
 	/* Start streaming */
-	return imx290_write_reg(imx290, IMX290_XMSTA, 0x00);
+	return imx290_write(imx290, IMX290_XMSTA, 0x00, &ret);
 }
 
 static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
@@ -772,27 +764,13 @@ static int imx290_set_data_lanes(struct imx290 *imx290)
 		 * validated in probe itself
 		 */
 		dev_err(imx290->dev, "Lane configuration not supported\n");
-		ret = -EINVAL;
-		goto exit;
+		return -EINVAL;
 	}
 
-	ret = imx290_write_reg(imx290, IMX290_PHY_LANE_NUM, laneval);
-	if (ret) {
-		dev_err(imx290->dev, "Error setting Physical Lane number register\n");
-		goto exit;
-	}
-
-	ret = imx290_write_reg(imx290, IMX290_CSI_LANE_MODE, laneval);
-	if (ret) {
-		dev_err(imx290->dev, "Error setting CSI Lane mode register\n");
-		goto exit;
-	}
-
-	ret = imx290_write_reg(imx290, IMX290_FR_FDG_SEL, frsel);
-	if (ret)
-		dev_err(imx290->dev, "Error setting FR/FDG SEL register\n");
+	imx290_write(imx290, IMX290_PHY_LANE_NUM, laneval, &ret);
+	imx290_write(imx290, IMX290_CSI_LANE_MODE, laneval, &ret);
+	imx290_write(imx290, IMX290_FR_FDG_SEL, frsel, &ret);
 
-exit:
 	return ret;
 }
 
-- 
Regards,

Laurent Pinchart

[PATCH v2 11/20] media: i2c: imx290: Define more register macros

[Laurent Pinchart]

Define macros for all registers programmed by the driver for which
documentation is available to increase readability. This starts making
use of 16-bit registers in the register arrays, so the value field has
to be increased to 32 bits.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx290.c | 219 +++++++++++++++++++++----------------
 1 file changed, 124 insertions(+), 95 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 0f26da5c2e54..93fd043669dc 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -31,14 +31,73 @@
 #define IMX290_STANDBY					IMX290_REG_8BIT(0x3000)
 #define IMX290_REGHOLD					IMX290_REG_8BIT(0x3001)
 #define IMX290_XMSTA					IMX290_REG_8BIT(0x3002)
+#define IMX290_ADBIT					IMX290_REG_8BIT(0x3005)
+#define IMX290_ADBIT_10BIT				(0 << 0)
+#define IMX290_ADBIT_12BIT				(1 << 0)
+#define IMX290_CTRL_07					IMX290_REG_8BIT(0x3007)
+#define IMX290_VREVERSE					BIT(0)
+#define IMX290_HREVERSE					BIT(1)
+#define IMX290_WINMODE_1080P				(0 << 4)
+#define IMX290_WINMODE_720P				(1 << 4)
+#define IMX290_WINMODE_CROP				(4 << 4)
 #define IMX290_FR_FDG_SEL				IMX290_REG_8BIT(0x3009)
 #define IMX290_BLKLEVEL					IMX290_REG_16BIT(0x300a)
 #define IMX290_GAIN					IMX290_REG_8BIT(0x3014)
+#define IMX290_VMAX					IMX290_REG_24BIT(0x3018)
 #define IMX290_HMAX					IMX290_REG_16BIT(0x301c)
+#define IMX290_SHS1					IMX290_REG_24BIT(0x3020)
+#define IMX290_WINWV_OB					IMX290_REG_8BIT(0x303a)
+#define IMX290_WINPV					IMX290_REG_16BIT(0x303c)
+#define IMX290_WINWV					IMX290_REG_16BIT(0x303e)
+#define IMX290_WINPH					IMX290_REG_16BIT(0x3040)
+#define IMX290_WINWH					IMX290_REG_16BIT(0x3042)
+#define IMX290_OUT_CTRL					IMX290_REG_8BIT(0x3046)
+#define IMX290_ODBIT_10BIT				(0 << 0)
+#define IMX290_ODBIT_12BIT				(1 << 0)
+#define IMX290_OPORTSEL_PARALLEL			(0x0 << 4)
+#define IMX290_OPORTSEL_LVDS_2CH			(0xd << 4)
+#define IMX290_OPORTSEL_LVDS_4CH			(0xe << 4)
+#define IMX290_OPORTSEL_LVDS_8CH			(0xf << 4)
+#define IMX290_XSOUTSEL					IMX290_REG_8BIT(0x304b)
+#define IMX290_XSOUTSEL_XVSOUTSEL_HIGH			(0 << 0)
+#define IMX290_XSOUTSEL_XVSOUTSEL_VSYNC			(2 << 0)
+#define IMX290_XSOUTSEL_XHSOUTSEL_HIGH			(0 << 2)
+#define IMX290_XSOUTSEL_XHSOUTSEL_HSYNC			(2 << 2)
+#define IMX290_INCKSEL1					IMX290_REG_8BIT(0x305c)
+#define IMX290_INCKSEL2					IMX290_REG_8BIT(0x305d)
+#define IMX290_INCKSEL3					IMX290_REG_8BIT(0x305e)
+#define IMX290_INCKSEL4					IMX290_REG_8BIT(0x305f)
 #define IMX290_PGCTRL					IMX290_REG_8BIT(0x308c)
+#define IMX290_ADBIT1					IMX290_REG_8BIT(0x3129)
+#define IMX290_ADBIT1_10BIT				0x1d
+#define IMX290_ADBIT1_12BIT				0x00
+#define IMX290_INCKSEL5					IMX290_REG_8BIT(0x315e)
+#define IMX290_INCKSEL6					IMX290_REG_8BIT(0x3164)
+#define IMX290_ADBIT2					IMX290_REG_8BIT(0x317c)
+#define IMX290_ADBIT2_10BIT				0x12
+#define IMX290_ADBIT2_12BIT				0x00
 #define IMX290_CHIP_ID					IMX290_REG_16BIT(0x319a)
+#define IMX290_ADBIT3					IMX290_REG_16BIT(0x31ec)
+#define IMX290_ADBIT3_10BIT				0x37
+#define IMX290_ADBIT3_12BIT				0x0e
+#define IMX290_REPETITION				IMX290_REG_8BIT(0x3405)
 #define IMX290_PHY_LANE_NUM				IMX290_REG_8BIT(0x3407)
+#define IMX290_OPB_SIZE_V				IMX290_REG_8BIT(0x3414)
+#define IMX290_Y_OUT_SIZE				IMX290_REG_16BIT(0x3418)
+#define IMX290_CSI_DT_FMT				IMX290_REG_16BIT(0x3441)
+#define IMX290_CSI_DT_FMT_RAW10				0x0a0a
+#define IMX290_CSI_DT_FMT_RAW12				0x0c0c
 #define IMX290_CSI_LANE_MODE				IMX290_REG_8BIT(0x3443)
+#define IMX290_EXTCK_FREQ				IMX290_REG_16BIT(0x3444)
+#define IMX290_TCLKPOST					IMX290_REG_16BIT(0x3446)
+#define IMX290_THSZERO					IMX290_REG_16BIT(0x3448)
+#define IMX290_THSPREPARE				IMX290_REG_16BIT(0x344a)
+#define IMX290_TCLKTRAIL				IMX290_REG_16BIT(0x344c)
+#define IMX290_THSTRAIL					IMX290_REG_16BIT(0x344e)
+#define IMX290_TCLKZERO					IMX290_REG_16BIT(0x3450)
+#define IMX290_TCLKPREPARE				IMX290_REG_16BIT(0x3452)
+#define IMX290_TLPX					IMX290_REG_16BIT(0x3454)
+#define IMX290_X_OUT_SIZE				IMX290_REG_16BIT(0x3472)
 
 #define IMX290_PGCTRL_REGEN				BIT(0)
 #define IMX290_PGCTRL_THRU				BIT(1)
@@ -54,7 +113,7 @@ static const char * const imx290_supply_name[] = {
 
 struct imx290_regval {
 	u32 reg;
-	u8 val;
+	u32 val;
 };
 
 struct imx290_mode {
@@ -116,22 +175,16 @@ static const char * const imx290_test_pattern_menu[] = {
 };
 
 static const struct imx290_regval imx290_global_init_settings[] = {
-	{ IMX290_REG_8BIT(0x3007), 0x00 },
-	{ IMX290_REG_8BIT(0x3018), 0x65 },
-	{ IMX290_REG_8BIT(0x3019), 0x04 },
-	{ IMX290_REG_8BIT(0x301a), 0x00 },
-	{ IMX290_REG_8BIT(0x3444), 0x20 },
-	{ IMX290_REG_8BIT(0x3445), 0x25 },
-	{ IMX290_REG_8BIT(0x303a), 0x0c },
-	{ IMX290_REG_8BIT(0x3040), 0x00 },
-	{ IMX290_REG_8BIT(0x3041), 0x00 },
-	{ IMX290_REG_8BIT(0x303c), 0x00 },
-	{ IMX290_REG_8BIT(0x303d), 0x00 },
-	{ IMX290_REG_8BIT(0x3042), 0x9c },
-	{ IMX290_REG_8BIT(0x3043), 0x07 },
-	{ IMX290_REG_8BIT(0x303e), 0x49 },
-	{ IMX290_REG_8BIT(0x303f), 0x04 },
-	{ IMX290_REG_8BIT(0x304b), 0x0a },
+	{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
+	{ IMX290_VMAX, 1125 },
+	{ IMX290_EXTCK_FREQ, 0x2520 },
+	{ IMX290_WINWV_OB, 12 },
+	{ IMX290_WINPH, 0 },
+	{ IMX290_WINPV, 0 },
+	{ IMX290_WINWH, 1948 },
+	{ IMX290_WINWV, 1097 },
+	{ IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC |
+			   IMX290_XSOUTSEL_XHSOUTSEL_HSYNC },
 	{ IMX290_REG_8BIT(0x300f), 0x00 },
 	{ IMX290_REG_8BIT(0x3010), 0x21 },
 	{ IMX290_REG_8BIT(0x3012), 0x64 },
@@ -177,102 +230,78 @@ static const struct imx290_regval imx290_global_init_settings[] = {
 
 static const struct imx290_regval imx290_1080p_settings[] = {
 	/* mode settings */
-	{ IMX290_REG_8BIT(0x3007), 0x00 },
-	{ IMX290_REG_8BIT(0x303a), 0x0c },
-	{ IMX290_REG_8BIT(0x3414), 0x0a },
-	{ IMX290_REG_8BIT(0x3472), 0x80 },
-	{ IMX290_REG_8BIT(0x3473), 0x07 },
-	{ IMX290_REG_8BIT(0x3418), 0x38 },
-	{ IMX290_REG_8BIT(0x3419), 0x04 },
+	{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
+	{ IMX290_WINWV_OB, 12 },
+	{ IMX290_OPB_SIZE_V, 10 },
+	{ IMX290_X_OUT_SIZE, 1920 },
+	{ IMX290_Y_OUT_SIZE, 1080 },
 	{ IMX290_REG_8BIT(0x3012), 0x64 },
 	{ IMX290_REG_8BIT(0x3013), 0x00 },
-	{ IMX290_REG_8BIT(0x305c), 0x18 },
-	{ IMX290_REG_8BIT(0x305d), 0x03 },
-	{ IMX290_REG_8BIT(0x305e), 0x20 },
-	{ IMX290_REG_8BIT(0x305f), 0x01 },
-	{ IMX290_REG_8BIT(0x315e), 0x1a },
-	{ IMX290_REG_8BIT(0x3164), 0x1a },
+	{ IMX290_INCKSEL1, 0x18 },
+	{ IMX290_INCKSEL2, 0x03 },
+	{ IMX290_INCKSEL3, 0x20 },
+	{ IMX290_INCKSEL4, 0x01 },
+	{ IMX290_INCKSEL5, 0x1a },
+	{ IMX290_INCKSEL6, 0x1a },
 	{ IMX290_REG_8BIT(0x3480), 0x49 },
 	/* data rate settings */
-	{ IMX290_REG_8BIT(0x3405), 0x10 },
-	{ IMX290_REG_8BIT(0x3446), 0x57 },
-	{ IMX290_REG_8BIT(0x3447), 0x00 },
-	{ IMX290_REG_8BIT(0x3448), 0x37 },
-	{ IMX290_REG_8BIT(0x3449), 0x00 },
-	{ IMX290_REG_8BIT(0x344a), 0x1f },
-	{ IMX290_REG_8BIT(0x344b), 0x00 },
-	{ IMX290_REG_8BIT(0x344c), 0x1f },
-	{ IMX290_REG_8BIT(0x344d), 0x00 },
-	{ IMX290_REG_8BIT(0x344e), 0x1f },
-	{ IMX290_REG_8BIT(0x344f), 0x00 },
-	{ IMX290_REG_8BIT(0x3450), 0x77 },
-	{ IMX290_REG_8BIT(0x3451), 0x00 },
-	{ IMX290_REG_8BIT(0x3452), 0x1f },
-	{ IMX290_REG_8BIT(0x3453), 0x00 },
-	{ IMX290_REG_8BIT(0x3454), 0x17 },
-	{ IMX290_REG_8BIT(0x3455), 0x00 },
+	{ IMX290_REPETITION, 0x10 },
+	{ IMX290_TCLKPOST, 87 },
+	{ IMX290_THSZERO, 55 },
+	{ IMX290_THSPREPARE, 31 },
+	{ IMX290_TCLKTRAIL, 31 },
+	{ IMX290_THSTRAIL, 31 },
+	{ IMX290_TCLKZERO, 119 },
+	{ IMX290_TCLKPREPARE, 31 },
+	{ IMX290_TLPX, 23 },
 };
 
 static const struct imx290_regval imx290_720p_settings[] = {
 	/* mode settings */
-	{ IMX290_REG_8BIT(0x3007), 0x10 },
-	{ IMX290_REG_8BIT(0x303a), 0x06 },
-	{ IMX290_REG_8BIT(0x3414), 0x04 },
-	{ IMX290_REG_8BIT(0x3472), 0x00 },
-	{ IMX290_REG_8BIT(0x3473), 0x05 },
-	{ IMX290_REG_8BIT(0x3418), 0xd0 },
-	{ IMX290_REG_8BIT(0x3419), 0x02 },
+	{ IMX290_CTRL_07, IMX290_WINMODE_720P },
+	{ IMX290_WINWV_OB, 6 },
+	{ IMX290_OPB_SIZE_V, 4 },
+	{ IMX290_X_OUT_SIZE, 1280 },
+	{ IMX290_Y_OUT_SIZE, 720 },
 	{ IMX290_REG_8BIT(0x3012), 0x64 },
 	{ IMX290_REG_8BIT(0x3013), 0x00 },
-	{ IMX290_REG_8BIT(0x305c), 0x20 },
-	{ IMX290_REG_8BIT(0x305d), 0x00 },
-	{ IMX290_REG_8BIT(0x305e), 0x20 },
-	{ IMX290_REG_8BIT(0x305f), 0x01 },
-	{ IMX290_REG_8BIT(0x315e), 0x1a },
-	{ IMX290_REG_8BIT(0x3164), 0x1a },
+	{ IMX290_INCKSEL1, 0x20 },
+	{ IMX290_INCKSEL2, 0x00 },
+	{ IMX290_INCKSEL3, 0x20 },
+	{ IMX290_INCKSEL4, 0x01 },
+	{ IMX290_INCKSEL5, 0x1a },
+	{ IMX290_INCKSEL6, 0x1a },
 	{ IMX290_REG_8BIT(0x3480), 0x49 },
 	/* data rate settings */
-	{ IMX290_REG_8BIT(0x3405), 0x10 },
-	{ IMX290_REG_8BIT(0x3446), 0x4f },
-	{ IMX290_REG_8BIT(0x3447), 0x00 },
-	{ IMX290_REG_8BIT(0x3448), 0x2f },
-	{ IMX290_REG_8BIT(0x3449), 0x00 },
-	{ IMX290_REG_8BIT(0x344a), 0x17 },
-	{ IMX290_REG_8BIT(0x344b), 0x00 },
-	{ IMX290_REG_8BIT(0x344c), 0x17 },
-	{ IMX290_REG_8BIT(0x344d), 0x00 },
-	{ IMX290_REG_8BIT(0x344e), 0x17 },
-	{ IMX290_REG_8BIT(0x344f), 0x00 },
-	{ IMX290_REG_8BIT(0x3450), 0x57 },
-	{ IMX290_REG_8BIT(0x3451), 0x00 },
-	{ IMX290_REG_8BIT(0x3452), 0x17 },
-	{ IMX290_REG_8BIT(0x3453), 0x00 },
-	{ IMX290_REG_8BIT(0x3454), 0x17 },
-	{ IMX290_REG_8BIT(0x3455), 0x00 },
+	{ IMX290_REPETITION, 0x10 },
+	{ IMX290_TCLKPOST, 79 },
+	{ IMX290_THSZERO, 47 },
+	{ IMX290_THSPREPARE, 23 },
+	{ IMX290_TCLKTRAIL, 23 },
+	{ IMX290_THSTRAIL, 23 },
+	{ IMX290_TCLKZERO, 87 },
+	{ IMX290_TCLKPREPARE, 23 },
+	{ IMX290_TLPX, 23 },
 };
 
 static const struct imx290_regval imx290_10bit_settings[] = {
-	{ IMX290_REG_8BIT(0x3005), 0x00},
-	{ IMX290_REG_8BIT(0x3046), 0x00},
-	{ IMX290_REG_8BIT(0x3129), 0x1d},
-	{ IMX290_REG_8BIT(0x317c), 0x12},
-	{ IMX290_REG_8BIT(0x31ec), 0x37},
-	{ IMX290_REG_8BIT(0x3441), 0x0a},
-	{ IMX290_REG_8BIT(0x3442), 0x0a},
-	{ IMX290_REG_8BIT(0x300a), 0x3c},
-	{ IMX290_REG_8BIT(0x300b), 0x00},
+	{ IMX290_ADBIT, IMX290_ADBIT_10BIT },
+	{ IMX290_OUT_CTRL, IMX290_ODBIT_10BIT },
+	{ IMX290_ADBIT1, IMX290_ADBIT1_10BIT },
+	{ IMX290_ADBIT2, IMX290_ADBIT2_10BIT },
+	{ IMX290_ADBIT3, IMX290_ADBIT3_10BIT },
+	{ IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW10 },
+	{ IMX290_BLKLEVEL, 60 },
 };
 
 static const struct imx290_regval imx290_12bit_settings[] = {
-	{ IMX290_REG_8BIT(0x3005), 0x01 },
-	{ IMX290_REG_8BIT(0x3046), 0x01 },
-	{ IMX290_REG_8BIT(0x3129), 0x00 },
-	{ IMX290_REG_8BIT(0x317c), 0x00 },
-	{ IMX290_REG_8BIT(0x31ec), 0x0e },
-	{ IMX290_REG_8BIT(0x3441), 0x0c },
-	{ IMX290_REG_8BIT(0x3442), 0x0c },
-	{ IMX290_REG_8BIT(0x300a), 0xf0 },
-	{ IMX290_REG_8BIT(0x300b), 0x00 },
+	{ IMX290_ADBIT, IMX290_ADBIT_12BIT },
+	{ IMX290_OUT_CTRL, IMX290_ODBIT_12BIT },
+	{ IMX290_ADBIT1, IMX290_ADBIT1_12BIT },
+	{ IMX290_ADBIT2, IMX290_ADBIT2_12BIT },
+	{ IMX290_ADBIT3, IMX290_ADBIT3_12BIT },
+	{ IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW12 },
+	{ IMX290_BLKLEVEL, 240 },
 };
 
 /* supported link frequencies */
-- 
Regards,

Laurent Pinchart

[PATCH v2 12/20] media: i2c: imx290: Add exposure time control

[Laurent Pinchart]

Support configuring the exposure time, which is expressed as the
complement of the exposure time (frame period minus integration time).
The frame period is currently fixed.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx290.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 93fd043669dc..a3d1394819ec 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -103,6 +103,8 @@
 #define IMX290_PGCTRL_THRU				BIT(1)
 #define IMX290_PGCTRL_MODE(n)				((n) << 4)
 
+#define IMX290_VMAX_DEFAULT				1125
+
 static const char * const imx290_supply_name[] = {
 	"vdda",
 	"vddd",
@@ -176,7 +178,7 @@ static const char * const imx290_test_pattern_menu[] = {
 
 static const struct imx290_regval imx290_global_init_settings[] = {
 	{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
-	{ IMX290_VMAX, 1125 },
+	{ IMX290_VMAX, IMX290_VMAX_DEFAULT },
 	{ IMX290_EXTCK_FREQ, 0x2520 },
 	{ IMX290_WINWV_OB, 12 },
 	{ IMX290_WINPH, 0 },
@@ -480,6 +482,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_GAIN:
 		ret = imx290_write(imx290, IMX290_GAIN, ctrl->val, NULL);
 		break;
+
+	case V4L2_CID_EXPOSURE:
+		ret = imx290_write(imx290, IMX290_SHS1,
+				   IMX290_VMAX_DEFAULT - ctrl->val - 1, NULL);
+		break;
+
 	case V4L2_CID_TEST_PATTERN:
 		if (ctrl->val) {
 			imx290_write(imx290, IMX290_BLKLEVEL, 0, &ret);
@@ -1008,12 +1016,16 @@ static int imx290_probe(struct i2c_client *client)
 	 */
 	imx290_entity_init_cfg(&imx290->sd, NULL);
 
-	v4l2_ctrl_handler_init(&imx290->ctrls, 4);
+	v4l2_ctrl_handler_init(&imx290->ctrls, 5);
 	imx290->ctrls.lock = &imx290->lock;
 
 	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
 			  V4L2_CID_GAIN, 0, 72, 1, 0);
 
+	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+			  V4L2_CID_EXPOSURE, 1, IMX290_VMAX_DEFAULT - 2, 1,
+			  IMX290_VMAX_DEFAULT - 2);
+
 	imx290->link_freq =
 		v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
 				       V4L2_CID_LINK_FREQ,
-- 
Regards,

Laurent Pinchart

[PATCH v2 13/20] media: i2c: imx290: Fix max gain value

[Laurent Pinchart]

The gain is expressed in multiple of 0.3dB, as a value between 0.0dB
and 72.0dB. Gains between 0.0dB and 30.0dB (included) apply analog gain
only, higher gains from 30.3dB to 72dB apply additional digital gain.

The maximum gain value is erroneously set to 72. Increase it to 100 to
cover the whole analog gain range. Support for digital gain can be added
separately if needed.

The IMX327 and IMX462 are largely compatible with the IMX290, but have
an analog gain range of 0.0dB to 29.4dB and 42dB of digital gain. When
support for those sensors gets added to the driver, the gain control
should be adjusted accordingly.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes since v1:

- Limit gain to 100
- Add comment
---
 drivers/media/i2c/imx290.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index a3d1394819ec..43ac6244c3a2 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1019,8 +1019,21 @@ static int imx290_probe(struct i2c_client *client)
 	v4l2_ctrl_handler_init(&imx290->ctrls, 5);
 	imx290->ctrls.lock = &imx290->lock;
 
+	/*
+	 * The sensor has an analog gain and a digital gain, both controlled
+	 * through a single gain value, expressed in 0.3dB increments. Values
+	 * from 0.0dB (0) to 30.0dB (100) apply analog gain only, higher values
+	 * up to 72.0dB (240) add further digital gain. Limit the range to
+	 * analog gain only, support for digital gain can be added separately
+	 * if needed.
+	 *
+	 * The IMX327 and IMX462 are largely compatible with the IMX290, but
+	 * have an analog gain range of 0.0dB to 29.4dB and 42dB of digital
+	 * gain. When support for those sensors gets added to the driver, the
+	 * gain control should be adjusted accordingly.
+	 */
 	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
-			  V4L2_CID_GAIN, 0, 72, 1, 0);
+			  V4L2_CID_GAIN, 0, 100, 1, 0);
 
 	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
 			  V4L2_CID_EXPOSURE, 1, IMX290_VMAX_DEFAULT - 2, 1,
-- 
Regards,

Laurent Pinchart

[PATCH v2 14/20] media: i2c: imx290: Split control initialization to separate function

[Laurent Pinchart]

The imx290_probe() function is too large. Split control initialzation to
a dedicated function to increase code readability.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 109 +++++++++++++++++++++----------------
 1 file changed, 61 insertions(+), 48 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 43ac6244c3a2..5deeab594e9b 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -878,6 +878,62 @@ static const struct media_entity_operations imx290_subdev_entity_ops = {
 	.link_validate = v4l2_subdev_link_validate,
 };
 
+static int imx290_ctrl_init(struct imx290 *imx290)
+{
+	int ret;
+
+	v4l2_ctrl_handler_init(&imx290->ctrls, 5);
+	imx290->ctrls.lock = &imx290->lock;
+
+	/*
+	 * The sensor has an analog gain and a digital gain, both controlled
+	 * through a single gain value, expressed in 0.3dB increments. Values
+	 * from 0.0dB (0) to 30.0dB (100) apply analog gain only, higher values
+	 * up to 72.0dB (240) add further digital gain. Limit the range to
+	 * analog gain only, support for digital gain can be added separately
+	 * if needed.
+	 *
+	 * The IMX327 and IMX462 are largely compatible with the IMX290, but
+	 * have an analog gain range of 0.0dB to 29.4dB and 42dB of digital
+	 * gain. When support for those sensors gets added to the driver, the
+	 * gain control should be adjusted accordingly.
+	 */
+	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+			  V4L2_CID_GAIN, 0, 100, 1, 0);
+
+	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+			  V4L2_CID_EXPOSURE, 1, IMX290_VMAX_DEFAULT - 2, 1,
+			  IMX290_VMAX_DEFAULT - 2);
+
+	imx290->link_freq =
+		v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
+				       V4L2_CID_LINK_FREQ,
+				       imx290_link_freqs_num(imx290) - 1, 0,
+				       imx290_link_freqs_ptr(imx290));
+	if (imx290->link_freq)
+		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+					       V4L2_CID_PIXEL_RATE,
+					       1, INT_MAX, 1,
+					       imx290_calc_pixel_rate(imx290));
+
+	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(imx290_test_pattern_menu) - 1,
+				     0, 0, imx290_test_pattern_menu);
+
+	imx290->sd.ctrl_handler = &imx290->ctrls;
+
+	if (imx290->ctrls.error) {
+		ret = imx290->ctrls.error;
+		v4l2_ctrl_handler_free(&imx290->ctrls);
+		return ret;
+	}
+
+	return 0;
+}
+
 /*
  * Returns 0 if all link frequencies used by the driver for the given number
  * of MIPI data lanes are mentioned in the device tree, or the value of the
@@ -1016,54 +1072,10 @@ static int imx290_probe(struct i2c_client *client)
 	 */
 	imx290_entity_init_cfg(&imx290->sd, NULL);
 
-	v4l2_ctrl_handler_init(&imx290->ctrls, 5);
-	imx290->ctrls.lock = &imx290->lock;
-
-	/*
-	 * The sensor has an analog gain and a digital gain, both controlled
-	 * through a single gain value, expressed in 0.3dB increments. Values
-	 * from 0.0dB (0) to 30.0dB (100) apply analog gain only, higher values
-	 * up to 72.0dB (240) add further digital gain. Limit the range to
-	 * analog gain only, support for digital gain can be added separately
-	 * if needed.
-	 *
-	 * The IMX327 and IMX462 are largely compatible with the IMX290, but
-	 * have an analog gain range of 0.0dB to 29.4dB and 42dB of digital
-	 * gain. When support for those sensors gets added to the driver, the
-	 * gain control should be adjusted accordingly.
-	 */
-	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
-			  V4L2_CID_GAIN, 0, 100, 1, 0);
-
-	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
-			  V4L2_CID_EXPOSURE, 1, IMX290_VMAX_DEFAULT - 2, 1,
-			  IMX290_VMAX_DEFAULT - 2);
-
-	imx290->link_freq =
-		v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
-				       V4L2_CID_LINK_FREQ,
-				       imx290_link_freqs_num(imx290) - 1, 0,
-				       imx290_link_freqs_ptr(imx290));
-	if (imx290->link_freq)
-		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-
-	imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
-					       V4L2_CID_PIXEL_RATE,
-					       1, INT_MAX, 1,
-					       imx290_calc_pixel_rate(imx290));
-
-	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
-				     V4L2_CID_TEST_PATTERN,
-				     ARRAY_SIZE(imx290_test_pattern_menu) - 1,
-				     0, 0, imx290_test_pattern_menu);
-
-	imx290->sd.ctrl_handler = &imx290->ctrls;
-
-	if (imx290->ctrls.error) {
-		dev_err(dev, "Control initialization error %d\n",
-			imx290->ctrls.error);
-		ret = imx290->ctrls.error;
-		goto free_ctrl;
+	ret = imx290_ctrl_init(imx290);
+	if (ret < 0) {
+		dev_err(dev, "Control initialization error %d\n", ret);
+		goto free_mutex;
 	}
 
 	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
@@ -1104,6 +1116,7 @@ static int imx290_probe(struct i2c_client *client)
 	media_entity_cleanup(&imx290->sd.entity);
 free_ctrl:
 	v4l2_ctrl_handler_free(&imx290->ctrls);
+free_mutex:
 	mutex_destroy(&imx290->lock);
 free_err:
 	v4l2_fwnode_endpoint_free(&ep);
-- 
Regards,

Laurent Pinchart

[PATCH v2 15/20] media: i2c: imx290: Implement HBLANK and VBLANK controls

[Laurent Pinchart]

Add support for the V4L2_CID_HBLANK and V4L2_CID_VBLANK controls to the
imx290 driver. Make the controls read-only to start with, to report the
values to userspace for timing calculation.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Drop incorrect comment
---
 drivers/media/i2c/imx290.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 5deeab594e9b..c5cecfd5d24c 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -146,6 +146,8 @@ struct imx290 {
 	struct v4l2_ctrl_handler ctrls;
 	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *pixel_rate;
+	struct v4l2_ctrl *hblank;
+	struct v4l2_ctrl *vblank;
 
 	struct mutex lock;
 };
@@ -642,6 +644,20 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 		if (imx290->pixel_rate)
 			__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
 						 imx290_calc_pixel_rate(imx290));
+
+		if (imx290->hblank) {
+			unsigned int hblank = mode->hmax - mode->width;
+
+			__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank,
+						 1, hblank);
+		}
+
+		if (imx290->vblank) {
+			unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
+
+			__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank,
+						 1, vblank);
+		}
 	}
 
 	*format = fmt->format;
@@ -880,9 +896,10 @@ static const struct media_entity_operations imx290_subdev_entity_ops = {
 
 static int imx290_ctrl_init(struct imx290 *imx290)
 {
+	unsigned int blank;
 	int ret;
 
-	v4l2_ctrl_handler_init(&imx290->ctrls, 5);
+	v4l2_ctrl_handler_init(&imx290->ctrls, 7);
 	imx290->ctrls.lock = &imx290->lock;
 
 	/*
@@ -923,6 +940,20 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 				     ARRAY_SIZE(imx290_test_pattern_menu) - 1,
 				     0, 0, imx290_test_pattern_menu);
 
+	blank = imx290->current_mode->hmax - imx290->current_mode->width;
+	imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+					   V4L2_CID_HBLANK, blank, blank, 1,
+					   blank);
+	if (imx290->hblank)
+		imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	blank = IMX290_VMAX_DEFAULT - imx290->current_mode->height;
+	imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+					   V4L2_CID_VBLANK, blank, blank, 1,
+					   blank);
+	if (imx290->vblank)
+		imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
 	imx290->sd.ctrl_handler = &imx290->ctrls;
 
 	if (imx290->ctrls.error) {
-- 
Regards,

Laurent Pinchart

[PATCH v2 16/20] media: i2c: imx290: Create controls for fwnode properties

[Laurent Pinchart]

Create the V4L2_CID_ORIENTATION and V4L2_CID_ROTATION controls to
expose the corresponding fwnode properties.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index c5cecfd5d24c..93eab6c96ca0 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -896,10 +896,15 @@ static const struct media_entity_operations imx290_subdev_entity_ops = {
 
 static int imx290_ctrl_init(struct imx290 *imx290)
 {
+	struct v4l2_fwnode_device_properties props;
 	unsigned int blank;
 	int ret;
 
-	v4l2_ctrl_handler_init(&imx290->ctrls, 7);
+	ret = v4l2_fwnode_device_parse(imx290->dev, &props);
+	if (ret < 0)
+		return ret;
+
+	v4l2_ctrl_handler_init(&imx290->ctrls, 9);
 	imx290->ctrls.lock = &imx290->lock;
 
 	/*
@@ -954,6 +959,9 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 	if (imx290->vblank)
 		imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
+	v4l2_ctrl_new_fwnode_properties(&imx290->ctrls, &imx290_ctrl_ops,
+					&props);
+
 	imx290->sd.ctrl_handler = &imx290->ctrls;
 
 	if (imx290->ctrls.error) {
-- 
Regards,

Laurent Pinchart

[PATCH v2 17/20] media: i2c: imx290: Move registers with fixed value to init array

[Laurent Pinchart]

Registers 0x3012, 0x3013 and 0x3480 are not documented and are set in
the per-mode register arrays with values indentical for all modes. Move
them to the common array.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx290.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 93eab6c96ca0..0b34d60f8ce2 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -192,6 +192,7 @@ static const struct imx290_regval imx290_global_init_settings[] = {
 	{ IMX290_REG_8BIT(0x300f), 0x00 },
 	{ IMX290_REG_8BIT(0x3010), 0x21 },
 	{ IMX290_REG_8BIT(0x3012), 0x64 },
+	{ IMX290_REG_8BIT(0x3013), 0x00 },
 	{ IMX290_REG_8BIT(0x3016), 0x09 },
 	{ IMX290_REG_8BIT(0x3070), 0x02 },
 	{ IMX290_REG_8BIT(0x3071), 0x11 },
@@ -230,6 +231,7 @@ static const struct imx290_regval imx290_global_init_settings[] = {
 	{ IMX290_REG_8BIT(0x33b0), 0x50 },
 	{ IMX290_REG_8BIT(0x33b2), 0x1a },
 	{ IMX290_REG_8BIT(0x33b3), 0x04 },
+	{ IMX290_REG_8BIT(0x3480), 0x49 },
 };
 
 static const struct imx290_regval imx290_1080p_settings[] = {
@@ -239,15 +241,12 @@ static const struct imx290_regval imx290_1080p_settings[] = {
 	{ IMX290_OPB_SIZE_V, 10 },
 	{ IMX290_X_OUT_SIZE, 1920 },
 	{ IMX290_Y_OUT_SIZE, 1080 },
-	{ IMX290_REG_8BIT(0x3012), 0x64 },
-	{ IMX290_REG_8BIT(0x3013), 0x00 },
 	{ IMX290_INCKSEL1, 0x18 },
 	{ IMX290_INCKSEL2, 0x03 },
 	{ IMX290_INCKSEL3, 0x20 },
 	{ IMX290_INCKSEL4, 0x01 },
 	{ IMX290_INCKSEL5, 0x1a },
 	{ IMX290_INCKSEL6, 0x1a },
-	{ IMX290_REG_8BIT(0x3480), 0x49 },
 	/* data rate settings */
 	{ IMX290_REPETITION, 0x10 },
 	{ IMX290_TCLKPOST, 87 },
@@ -267,15 +266,12 @@ static const struct imx290_regval imx290_720p_settings[] = {
 	{ IMX290_OPB_SIZE_V, 4 },
 	{ IMX290_X_OUT_SIZE, 1280 },
 	{ IMX290_Y_OUT_SIZE, 720 },
-	{ IMX290_REG_8BIT(0x3012), 0x64 },
-	{ IMX290_REG_8BIT(0x3013), 0x00 },
 	{ IMX290_INCKSEL1, 0x20 },
 	{ IMX290_INCKSEL2, 0x00 },
 	{ IMX290_INCKSEL3, 0x20 },
 	{ IMX290_INCKSEL4, 0x01 },
 	{ IMX290_INCKSEL5, 0x1a },
 	{ IMX290_INCKSEL6, 0x1a },
-	{ IMX290_REG_8BIT(0x3480), 0x49 },
 	/* data rate settings */
 	{ IMX290_REPETITION, 0x10 },
 	{ IMX290_TCLKPOST, 79 },
-- 
Regards,

Laurent Pinchart

[PATCH v2 18/20] media: i2c: imx290: Factor out format retrieval to separate function

[Laurent Pinchart]

The driver duplicates the same pattern to access the try or active
format in multiple locations. Factor it out to a separate function.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Avoid returning NULL from imx290_get_pad_format()
---
 drivers/media/i2c/imx290.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 0b34d60f8ce2..b0ff0e8ed45a 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -519,6 +519,16 @@ static const struct v4l2_ctrl_ops imx290_ctrl_ops = {
 	.s_ctrl = imx290_set_ctrl,
 };
 
+static struct v4l2_mbus_framefmt *
+imx290_get_pad_format(struct imx290 *imx290, struct v4l2_subdev_state *state,
+		      u32 which)
+{
+	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		return &imx290->current_format;
+	else
+		return v4l2_subdev_get_try_format(&imx290->sd, state, 0);
+}
+
 static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
@@ -562,12 +572,7 @@ static int imx290_get_fmt(struct v4l2_subdev *sd,
 
 	mutex_lock(&imx290->lock);
 
-	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
-		framefmt = v4l2_subdev_get_try_format(&imx290->sd, sd_state,
-						      fmt->pad);
-	else
-		framefmt = &imx290->current_format;
-
+	framefmt = imx290_get_pad_format(imx290, sd_state, fmt->which);
 	fmt->format = *framefmt;
 
 	mutex_unlock(&imx290->lock);
@@ -627,10 +632,9 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 	fmt->format.code = imx290_formats[i].code;
 	fmt->format.field = V4L2_FIELD_NONE;
 
-	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
-		format = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
-	} else {
-		format = &imx290->current_format;
+	format = imx290_get_pad_format(imx290, sd_state, fmt->which);
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		imx290->current_mode = mode;
 		imx290->bpp = imx290_formats[i].bpp;
 
-- 
Regards,

Laurent Pinchart

[PATCH v2 19/20] media: i2c: imx290: Add crop selection targets support

[Laurent Pinchart]

Implement read-only access to crop selection rectangles to expose the
analogue crop rectangle. The public (leaked) IMX290 documentation is not
very clear on how cropping is implemented and configured exactly, so
the margins may not be entirely accurate.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Fix effective bottom margin value in diagram
---
 drivers/media/i2c/imx290.c | 94 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index b0ff0e8ed45a..53db66068538 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -105,6 +105,53 @@
 
 #define IMX290_VMAX_DEFAULT				1125
 
+
+/*
+ * The IMX290 pixel array is organized as follows:
+ *
+ *     +------------------------------------+
+ *     |           Optical Black            |     }  Vertical effective optical black (10)
+ * +---+------------------------------------+---+
+ * |   |                                    |   | }  Effective top margin (8)
+ * |   |   +----------------------------+   |   | \
+ * |   |   |                            |   |   |  |
+ * |   |   |                            |   |   |  |
+ * |   |   |                            |   |   |  |
+ * |   |   |    Recording Pixel Area    |   |   |  | Recommended height (1080)
+ * |   |   |                            |   |   |  |
+ * |   |   |                            |   |   |  |
+ * |   |   |                            |   |   |  |
+ * |   |   +----------------------------+   |   | /
+ * |   |                                    |   | }  Effective bottom margin (9)
+ * +---+------------------------------------+---+
+ *  <-> <-> <--------------------------> <-> <->
+ *                                            \----  Ignored right margin (4)
+ *                                        \--------  Effective right margin (9)
+ *                       \-------------------------  Recommended width (1920)
+ *       \-----------------------------------------  Effective left margin (8)
+ *   \---------------------------------------------  Ignored left margin (4)
+ *
+ * The optical black lines are output over CSI-2 with a separate data type.
+ *
+ * The pixel array is meant to have 1920x1080 usable pixels after image
+ * processing in an ISP. It has 8 (9) extra active pixels usable for color
+ * processing in the ISP on the top and left (bottom and right) sides of the
+ * image. In addition, 4 additional pixels are present on the left and right
+ * sides of the image, documented as "ignored area".
+ *
+ * As far as is understood, all pixels of the pixel array (ignored area, color
+ * processing margins and recording area) can be output by the sensor.
+ */
+
+#define IMX290_PIXEL_ARRAY_WIDTH			1945
+#define IMX290_PIXEL_ARRAY_HEIGHT			1097
+#define IMX920_PIXEL_ARRAY_MARGIN_LEFT			12
+#define IMX920_PIXEL_ARRAY_MARGIN_RIGHT			13
+#define IMX920_PIXEL_ARRAY_MARGIN_TOP			8
+#define IMX920_PIXEL_ARRAY_MARGIN_BOTTOM		9
+#define IMX290_PIXEL_ARRAY_RECORDING_WIDTH		1920
+#define IMX290_PIXEL_ARRAY_RECORDING_HEIGHT		1080
+
 static const char * const imx290_supply_name[] = {
 	"vdda",
 	"vddd",
@@ -667,6 +714,52 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int imx290_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *sd_state,
+				struct v4l2_subdev_selection *sel)
+{
+	struct imx290 *imx290 = to_imx290(sd);
+	struct v4l2_mbus_framefmt *format;
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP: {
+		format = imx290_get_pad_format(imx290, sd_state, sel->which);
+
+		mutex_lock(&imx290->lock);
+
+		sel->r.top = IMX920_PIXEL_ARRAY_MARGIN_TOP
+			   + (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT - format->height) / 2;
+		sel->r.left = IMX920_PIXEL_ARRAY_MARGIN_LEFT
+			    + (IMX290_PIXEL_ARRAY_RECORDING_WIDTH - format->width) / 2;
+		sel->r.width = format->width;
+		sel->r.height = format->height;
+
+		mutex_unlock(&imx290->lock);
+		return 0;
+	}
+
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = IMX290_PIXEL_ARRAY_WIDTH;
+		sel->r.height = IMX290_PIXEL_ARRAY_HEIGHT;
+
+		return 0;
+
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		sel->r.top = IMX920_PIXEL_ARRAY_MARGIN_TOP;
+		sel->r.left = IMX920_PIXEL_ARRAY_MARGIN_LEFT;
+		sel->r.width = IMX290_PIXEL_ARRAY_RECORDING_WIDTH;
+		sel->r.height = IMX290_PIXEL_ARRAY_RECORDING_HEIGHT;
+
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
 static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
 				  struct v4l2_subdev_state *sd_state)
 {
@@ -883,6 +976,7 @@ static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
 	.enum_frame_size = imx290_enum_frame_size,
 	.get_fmt = imx290_get_fmt,
 	.set_fmt = imx290_set_fmt,
+	.get_selection = imx290_get_selection,
 };
 
 static const struct v4l2_subdev_ops imx290_subdev_ops = {
-- 
Regards,

Laurent Pinchart

[PATCH v2 20/20] media: i2c: imx290: Replace GAIN control with ANALOGUE_GAIN

[Laurent Pinchart]

The IMX290 gain register controls the analogue gain. Replace the
V4L2_CID_GAIN control with V4L2_CID_ANALOGUE_GAIN.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 53db66068538..ffc072535be0 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -524,7 +524,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 		return 0;
 
 	switch (ctrl->id) {
-	case V4L2_CID_GAIN:
+	case V4L2_CID_ANALOGUE_GAIN:
 		ret = imx290_write(imx290, IMX290_GAIN, ctrl->val, NULL);
 		break;
 
@@ -1015,7 +1015,7 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 	 * gain control should be adjusted accordingly.
 	 */
 	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
-			  V4L2_CID_GAIN, 0, 100, 1, 0);
+			  V4L2_CID_ANALOGUE_GAIN, 0, 100, 1, 0);
 
 	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
 			  V4L2_CID_EXPOSURE, 1, IMX290_VMAX_DEFAULT - 2, 1,
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 01/20] media: dt-bindings: Convert imx290.txt to YAML

[Krzysztof Kozlowski]

On 16/10/2022 02:15, Laurent Pinchart wrote:
> Convert the Sony IMX290 DT binding from text to YAML. Add Manivannan as
> a maintainer given that he is listed in MAINTAINERS for the file, as
> volunteering myself.
> 
> The name of the input clock, "xclk", is wrong as the hardware manual
> names it INCK. As the device has a single clock, the name could be
> omitted, but that would require a corresponding change to the driver and
> is thus a candidate for further patches.
> 

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 17/20] media: i2c: imx290: Move registers with fixed value to init array

[Alexander Stein]

Hello Laurent,

thanks for the updated series.

Am Sonntag, 16. Oktober 2022, 08:15:20 CEST schrieb Laurent Pinchart:
> Registers 0x3012, 0x3013 and 0x3480 are not documented and are set in
> the per-mode register arrays with values indentical for all modes. Move
> them to the common array.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/imx290.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 93eab6c96ca0..0b34d60f8ce2 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -192,6 +192,7 @@ static const struct imx290_regval
> imx290_global_init_settings[] = { { IMX290_REG_8BIT(0x300f), 0x00 },
>  	{ IMX290_REG_8BIT(0x3010), 0x21 },
>  	{ IMX290_REG_8BIT(0x3012), 0x64 },
> +	{ IMX290_REG_8BIT(0x3013), 0x00 },
>  	{ IMX290_REG_8BIT(0x3016), 0x09 },
>  	{ IMX290_REG_8BIT(0x3070), 0x02 },
>  	{ IMX290_REG_8BIT(0x3071), 0x11 },
> @@ -230,6 +231,7 @@ static const struct imx290_regval
> imx290_global_init_settings[] = { { IMX290_REG_8BIT(0x33b0), 0x50 },
>  	{ IMX290_REG_8BIT(0x33b2), 0x1a },
>  	{ IMX290_REG_8BIT(0x33b3), 0x04 },
> +	{ IMX290_REG_8BIT(0x3480), 0x49 },
>  };
> 
>  static const struct imx290_regval imx290_1080p_settings[] = {
> @@ -239,15 +241,12 @@ static const struct imx290_regval
> imx290_1080p_settings[] = { { IMX290_OPB_SIZE_V, 10 },
>  	{ IMX290_X_OUT_SIZE, 1920 },
>  	{ IMX290_Y_OUT_SIZE, 1080 },
> -	{ IMX290_REG_8BIT(0x3012), 0x64 },
> -	{ IMX290_REG_8BIT(0x3013), 0x00 },
>  	{ IMX290_INCKSEL1, 0x18 },
>  	{ IMX290_INCKSEL2, 0x03 },
>  	{ IMX290_INCKSEL3, 0x20 },
>  	{ IMX290_INCKSEL4, 0x01 },
>  	{ IMX290_INCKSEL5, 0x1a },
>  	{ IMX290_INCKSEL6, 0x1a },
> -	{ IMX290_REG_8BIT(0x3480), 0x49 },
>  	/* data rate settings */
>  	{ IMX290_REPETITION, 0x10 },
>  	{ IMX290_TCLKPOST, 87 },
> @@ -267,15 +266,12 @@ static const struct imx290_regval
> imx290_720p_settings[] = { { IMX290_OPB_SIZE_V, 4 },
>  	{ IMX290_X_OUT_SIZE, 1280 },
>  	{ IMX290_Y_OUT_SIZE, 720 },
> -	{ IMX290_REG_8BIT(0x3012), 0x64 },
> -	{ IMX290_REG_8BIT(0x3013), 0x00 },
>  	{ IMX290_INCKSEL1, 0x20 },
>  	{ IMX290_INCKSEL2, 0x00 },
>  	{ IMX290_INCKSEL3, 0x20 },
>  	{ IMX290_INCKSEL4, 0x01 },
>  	{ IMX290_INCKSEL5, 0x1a },
>  	{ IMX290_INCKSEL6, 0x1a },
> -	{ IMX290_REG_8BIT(0x3480), 0x49 },
>  	/* data rate settings */
>  	{ IMX290_REPETITION, 0x10 },
>  	{ IMX290_TCLKPOST, 79 },

Acked-by:  Alexander Stein <alexander.stein@ew.tq-group.com>

Re: [PATCH v2 18/20] media: i2c: imx290: Factor out format retrieval to separate function

[Alexander Stein]

Hello Laurent,

thanks for the updated patch.

Am Sonntag, 16. Oktober 2022, 08:15:21 CEST schrieb Laurent Pinchart:
> The driver duplicates the same pattern to access the try or active
> format in multiple locations. Factor it out to a separate function.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Avoid returning NULL from imx290_get_pad_format()
> ---
>  drivers/media/i2c/imx290.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 0b34d60f8ce2..b0ff0e8ed45a 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -519,6 +519,16 @@ static const struct v4l2_ctrl_ops imx290_ctrl_ops = {
>  	.s_ctrl = imx290_set_ctrl,
>  };
> 
> +static struct v4l2_mbus_framefmt *
> +imx290_get_pad_format(struct imx290 *imx290, struct v4l2_subdev_state
> *state, +		      u32 which)
> +{
> +	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +		return &imx290->current_format;
> +	else
> +		return v4l2_subdev_get_try_format(&imx290->sd, state, 
0);
> +}
> +

v4l2_subdev_get_try_format can return NULL, which would be dereferenced later 
on. But this happens only if state is NULL itself, which will raise a WARN_ON 
anyway. So i guess this is fine.

Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>

>  static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state 
*sd_state,
>  				 struct v4l2_subdev_mbus_code_enum 
*code)
> @@ -562,12 +572,7 @@ static int imx290_get_fmt(struct v4l2_subdev *sd,
> 
>  	mutex_lock(&imx290->lock);
> 
> -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> -		framefmt = v4l2_subdev_get_try_format(&imx290->sd, 
sd_state,
> -						      fmt-
>pad);
> -	else
> -		framefmt = &imx290->current_format;
> -
> +	framefmt = imx290_get_pad_format(imx290, sd_state, fmt->which);
>  	fmt->format = *framefmt;
> 
>  	mutex_unlock(&imx290->lock);
> @@ -627,10 +632,9 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>  	fmt->format.code = imx290_formats[i].code;
>  	fmt->format.field = V4L2_FIELD_NONE;
> 
> -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		format = v4l2_subdev_get_try_format(sd, sd_state, fmt-
>pad);
> -	} else {
> -		format = &imx290->current_format;
> +	format = imx290_get_pad_format(imx290, sd_state, fmt->which);
> +
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>  		imx290->current_mode = mode;
>  		imx290->bpp = imx290_formats[i].bpp;

Re: [PATCH v2 11/20] media: i2c: imx290: Define more register macros

[Alexander Stein]

Hello Laurent,

thanks for the updated series.

Am Sonntag, 16. Oktober 2022, 08:15:14 CEST schrieb Laurent Pinchart:
> Define macros for all registers programmed by the driver for which
> documentation is available to increase readability. This starts making
> use of 16-bit registers in the register arrays, so the value field has
> to be increased to 32 bits.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/imx290.c | 219 +++++++++++++++++++++----------------
>  1 file changed, 124 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 0f26da5c2e54..93fd043669dc 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -31,14 +31,73 @@
>  #define IMX290_STANDBY					
IMX290_REG_8BIT(0x3000)
>  #define IMX290_REGHOLD					
IMX290_REG_8BIT(0x3001)
>  #define IMX290_XMSTA					
IMX290_REG_8BIT(0x3002)
> +#define IMX290_ADBIT					
IMX290_REG_8BIT(0x3005)
> +#define IMX290_ADBIT_10BIT				(0 << 0)
> +#define IMX290_ADBIT_12BIT				(1 << 0)
> +#define IMX290_CTRL_07					
IMX290_REG_8BIT(0x3007)
> +#define IMX290_VREVERSE					
BIT(0)
> +#define IMX290_HREVERSE					
BIT(1)
> +#define IMX290_WINMODE_1080P				(0 << 4)
> +#define IMX290_WINMODE_720P				(1 << 4)
> +#define IMX290_WINMODE_CROP				(4 << 4)
>  #define IMX290_FR_FDG_SEL				
IMX290_REG_8BIT(0x3009)
>  #define IMX290_BLKLEVEL					
IMX290_REG_16BIT(0x300a)
>  #define IMX290_GAIN					
IMX290_REG_8BIT(0x3014)
> +#define IMX290_VMAX					
IMX290_REG_24BIT(0x3018)
>  #define IMX290_HMAX					
IMX290_REG_16BIT(0x301c)
> +#define IMX290_SHS1					
IMX290_REG_24BIT(0x3020)
> +#define IMX290_WINWV_OB					
IMX290_REG_8BIT(0x303a)
> +#define IMX290_WINPV					
IMX290_REG_16BIT(0x303c)
> +#define IMX290_WINWV					
IMX290_REG_16BIT(0x303e)
> +#define IMX290_WINPH					
IMX290_REG_16BIT(0x3040)
> +#define IMX290_WINWH					
IMX290_REG_16BIT(0x3042)
> +#define IMX290_OUT_CTRL					
IMX290_REG_8BIT(0x3046)
> +#define IMX290_ODBIT_10BIT				(0 << 0)
> +#define IMX290_ODBIT_12BIT				(1 << 0)
> +#define IMX290_OPORTSEL_PARALLEL			(0x0 << 4)
> +#define IMX290_OPORTSEL_LVDS_2CH			(0xd << 4)
> +#define IMX290_OPORTSEL_LVDS_4CH			(0xe << 4)
> +#define IMX290_OPORTSEL_LVDS_8CH			(0xf << 4)
> +#define IMX290_XSOUTSEL					
IMX290_REG_8BIT(0x304b)
> +#define IMX290_XSOUTSEL_XVSOUTSEL_HIGH			(0 << 0)
> +#define IMX290_XSOUTSEL_XVSOUTSEL_VSYNC			(2 << 0)
> +#define IMX290_XSOUTSEL_XHSOUTSEL_HIGH			(0 << 2)
> +#define IMX290_XSOUTSEL_XHSOUTSEL_HSYNC			(2 << 2)
> +#define IMX290_INCKSEL1					
IMX290_REG_8BIT(0x305c)
> +#define IMX290_INCKSEL2					
IMX290_REG_8BIT(0x305d)
> +#define IMX290_INCKSEL3					
IMX290_REG_8BIT(0x305e)
> +#define IMX290_INCKSEL4					
IMX290_REG_8BIT(0x305f)
>  #define IMX290_PGCTRL					
IMX290_REG_8BIT(0x308c)
> +#define IMX290_ADBIT1					
IMX290_REG_8BIT(0x3129)
> +#define IMX290_ADBIT1_10BIT				0x1d
> +#define IMX290_ADBIT1_12BIT				0x00
> +#define IMX290_INCKSEL5					
IMX290_REG_8BIT(0x315e)
> +#define IMX290_INCKSEL6					
IMX290_REG_8BIT(0x3164)
> +#define IMX290_ADBIT2					
IMX290_REG_8BIT(0x317c)
> +#define IMX290_ADBIT2_10BIT				0x12
> +#define IMX290_ADBIT2_12BIT				0x00
>  #define IMX290_CHIP_ID					
IMX290_REG_16BIT(0x319a)
> +#define IMX290_ADBIT3					
IMX290_REG_16BIT(0x31ec)

AFAICS this is only 8 bits wide.

Thanks
Alexander

> +#define IMX290_ADBIT3_10BIT				0x37
> +#define IMX290_ADBIT3_12BIT				0x0e
> +#define IMX290_REPETITION				
IMX290_REG_8BIT(0x3405)
>  #define IMX290_PHY_LANE_NUM				
IMX290_REG_8BIT(0x3407)
> +#define IMX290_OPB_SIZE_V				
IMX290_REG_8BIT(0x3414)
> +#define IMX290_Y_OUT_SIZE				
IMX290_REG_16BIT(0x3418)
> +#define IMX290_CSI_DT_FMT				
IMX290_REG_16BIT(0x3441)
> +#define IMX290_CSI_DT_FMT_RAW10				
0x0a0a
> +#define IMX290_CSI_DT_FMT_RAW12				
0x0c0c
>  #define IMX290_CSI_LANE_MODE				
IMX290_REG_8BIT(0x3443)
> +#define IMX290_EXTCK_FREQ				
IMX290_REG_16BIT(0x3444)
> +#define IMX290_TCLKPOST					
IMX290_REG_16BIT(0x3446)
> +#define IMX290_THSZERO					
IMX290_REG_16BIT(0x3448)
> +#define IMX290_THSPREPARE				
IMX290_REG_16BIT(0x344a)
> +#define IMX290_TCLKTRAIL				
IMX290_REG_16BIT(0x344c)
> +#define IMX290_THSTRAIL					
IMX290_REG_16BIT(0x344e)
> +#define IMX290_TCLKZERO					
IMX290_REG_16BIT(0x3450)
> +#define IMX290_TCLKPREPARE				
IMX290_REG_16BIT(0x3452)
> +#define IMX290_TLPX					
IMX290_REG_16BIT(0x3454)
> +#define IMX290_X_OUT_SIZE				
IMX290_REG_16BIT(0x3472)
> 
>  #define IMX290_PGCTRL_REGEN				BIT(0)
>  #define IMX290_PGCTRL_THRU				BIT(1)
> @@ -54,7 +113,7 @@ static const char * const imx290_supply_name[] = {
> 
>  struct imx290_regval {
>  	u32 reg;
> -	u8 val;
> +	u32 val;
>  };
> 
>  struct imx290_mode {
> @@ -116,22 +175,16 @@ static const char * const imx290_test_pattern_menu[] =
> { };
> 
>  static const struct imx290_regval imx290_global_init_settings[] = {
> -	{ IMX290_REG_8BIT(0x3007), 0x00 },
> -	{ IMX290_REG_8BIT(0x3018), 0x65 },
> -	{ IMX290_REG_8BIT(0x3019), 0x04 },
> -	{ IMX290_REG_8BIT(0x301a), 0x00 },
> -	{ IMX290_REG_8BIT(0x3444), 0x20 },
> -	{ IMX290_REG_8BIT(0x3445), 0x25 },
> -	{ IMX290_REG_8BIT(0x303a), 0x0c },
> -	{ IMX290_REG_8BIT(0x3040), 0x00 },
> -	{ IMX290_REG_8BIT(0x3041), 0x00 },
> -	{ IMX290_REG_8BIT(0x303c), 0x00 },
> -	{ IMX290_REG_8BIT(0x303d), 0x00 },
> -	{ IMX290_REG_8BIT(0x3042), 0x9c },
> -	{ IMX290_REG_8BIT(0x3043), 0x07 },
> -	{ IMX290_REG_8BIT(0x303e), 0x49 },
> -	{ IMX290_REG_8BIT(0x303f), 0x04 },
> -	{ IMX290_REG_8BIT(0x304b), 0x0a },
> +	{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
> +	{ IMX290_VMAX, 1125 },
> +	{ IMX290_EXTCK_FREQ, 0x2520 },
> +	{ IMX290_WINWV_OB, 12 },
> +	{ IMX290_WINPH, 0 },
> +	{ IMX290_WINPV, 0 },
> +	{ IMX290_WINWH, 1948 },
> +	{ IMX290_WINWV, 1097 },
> +	{ IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC |
> +			   IMX290_XSOUTSEL_XHSOUTSEL_HSYNC },
>  	{ IMX290_REG_8BIT(0x300f), 0x00 },
>  	{ IMX290_REG_8BIT(0x3010), 0x21 },
>  	{ IMX290_REG_8BIT(0x3012), 0x64 },
> @@ -177,102 +230,78 @@ static const struct imx290_regval
> imx290_global_init_settings[] = {
> 
>  static const struct imx290_regval imx290_1080p_settings[] = {
>  	/* mode settings */
> -	{ IMX290_REG_8BIT(0x3007), 0x00 },
> -	{ IMX290_REG_8BIT(0x303a), 0x0c },
> -	{ IMX290_REG_8BIT(0x3414), 0x0a },
> -	{ IMX290_REG_8BIT(0x3472), 0x80 },
> -	{ IMX290_REG_8BIT(0x3473), 0x07 },
> -	{ IMX290_REG_8BIT(0x3418), 0x38 },
> -	{ IMX290_REG_8BIT(0x3419), 0x04 },
> +	{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
> +	{ IMX290_WINWV_OB, 12 },
> +	{ IMX290_OPB_SIZE_V, 10 },
> +	{ IMX290_X_OUT_SIZE, 1920 },
> +	{ IMX290_Y_OUT_SIZE, 1080 },
>  	{ IMX290_REG_8BIT(0x3012), 0x64 },
>  	{ IMX290_REG_8BIT(0x3013), 0x00 },
> -	{ IMX290_REG_8BIT(0x305c), 0x18 },
> -	{ IMX290_REG_8BIT(0x305d), 0x03 },
> -	{ IMX290_REG_8BIT(0x305e), 0x20 },
> -	{ IMX290_REG_8BIT(0x305f), 0x01 },
> -	{ IMX290_REG_8BIT(0x315e), 0x1a },
> -	{ IMX290_REG_8BIT(0x3164), 0x1a },
> +	{ IMX290_INCKSEL1, 0x18 },
> +	{ IMX290_INCKSEL2, 0x03 },
> +	{ IMX290_INCKSEL3, 0x20 },
> +	{ IMX290_INCKSEL4, 0x01 },
> +	{ IMX290_INCKSEL5, 0x1a },
> +	{ IMX290_INCKSEL6, 0x1a },
>  	{ IMX290_REG_8BIT(0x3480), 0x49 },
>  	/* data rate settings */
> -	{ IMX290_REG_8BIT(0x3405), 0x10 },
> -	{ IMX290_REG_8BIT(0x3446), 0x57 },
> -	{ IMX290_REG_8BIT(0x3447), 0x00 },
> -	{ IMX290_REG_8BIT(0x3448), 0x37 },
> -	{ IMX290_REG_8BIT(0x3449), 0x00 },
> -	{ IMX290_REG_8BIT(0x344a), 0x1f },
> -	{ IMX290_REG_8BIT(0x344b), 0x00 },
> -	{ IMX290_REG_8BIT(0x344c), 0x1f },
> -	{ IMX290_REG_8BIT(0x344d), 0x00 },
> -	{ IMX290_REG_8BIT(0x344e), 0x1f },
> -	{ IMX290_REG_8BIT(0x344f), 0x00 },
> -	{ IMX290_REG_8BIT(0x3450), 0x77 },
> -	{ IMX290_REG_8BIT(0x3451), 0x00 },
> -	{ IMX290_REG_8BIT(0x3452), 0x1f },
> -	{ IMX290_REG_8BIT(0x3453), 0x00 },
> -	{ IMX290_REG_8BIT(0x3454), 0x17 },
> -	{ IMX290_REG_8BIT(0x3455), 0x00 },
> +	{ IMX290_REPETITION, 0x10 },
> +	{ IMX290_TCLKPOST, 87 },
> +	{ IMX290_THSZERO, 55 },
> +	{ IMX290_THSPREPARE, 31 },
> +	{ IMX290_TCLKTRAIL, 31 },
> +	{ IMX290_THSTRAIL, 31 },
> +	{ IMX290_TCLKZERO, 119 },
> +	{ IMX290_TCLKPREPARE, 31 },
> +	{ IMX290_TLPX, 23 },
>  };
> 
>  static const struct imx290_regval imx290_720p_settings[] = {
>  	/* mode settings */
> -	{ IMX290_REG_8BIT(0x3007), 0x10 },
> -	{ IMX290_REG_8BIT(0x303a), 0x06 },
> -	{ IMX290_REG_8BIT(0x3414), 0x04 },
> -	{ IMX290_REG_8BIT(0x3472), 0x00 },
> -	{ IMX290_REG_8BIT(0x3473), 0x05 },
> -	{ IMX290_REG_8BIT(0x3418), 0xd0 },
> -	{ IMX290_REG_8BIT(0x3419), 0x02 },
> +	{ IMX290_CTRL_07, IMX290_WINMODE_720P },
> +	{ IMX290_WINWV_OB, 6 },
> +	{ IMX290_OPB_SIZE_V, 4 },
> +	{ IMX290_X_OUT_SIZE, 1280 },
> +	{ IMX290_Y_OUT_SIZE, 720 },
>  	{ IMX290_REG_8BIT(0x3012), 0x64 },
>  	{ IMX290_REG_8BIT(0x3013), 0x00 },
> -	{ IMX290_REG_8BIT(0x305c), 0x20 },
> -	{ IMX290_REG_8BIT(0x305d), 0x00 },
> -	{ IMX290_REG_8BIT(0x305e), 0x20 },
> -	{ IMX290_REG_8BIT(0x305f), 0x01 },
> -	{ IMX290_REG_8BIT(0x315e), 0x1a },
> -	{ IMX290_REG_8BIT(0x3164), 0x1a },
> +	{ IMX290_INCKSEL1, 0x20 },
> +	{ IMX290_INCKSEL2, 0x00 },
> +	{ IMX290_INCKSEL3, 0x20 },
> +	{ IMX290_INCKSEL4, 0x01 },
> +	{ IMX290_INCKSEL5, 0x1a },
> +	{ IMX290_INCKSEL6, 0x1a },
>  	{ IMX290_REG_8BIT(0x3480), 0x49 },
>  	/* data rate settings */
> -	{ IMX290_REG_8BIT(0x3405), 0x10 },
> -	{ IMX290_REG_8BIT(0x3446), 0x4f },
> -	{ IMX290_REG_8BIT(0x3447), 0x00 },
> -	{ IMX290_REG_8BIT(0x3448), 0x2f },
> -	{ IMX290_REG_8BIT(0x3449), 0x00 },
> -	{ IMX290_REG_8BIT(0x344a), 0x17 },
> -	{ IMX290_REG_8BIT(0x344b), 0x00 },
> -	{ IMX290_REG_8BIT(0x344c), 0x17 },
> -	{ IMX290_REG_8BIT(0x344d), 0x00 },
> -	{ IMX290_REG_8BIT(0x344e), 0x17 },
> -	{ IMX290_REG_8BIT(0x344f), 0x00 },
> -	{ IMX290_REG_8BIT(0x3450), 0x57 },
> -	{ IMX290_REG_8BIT(0x3451), 0x00 },
> -	{ IMX290_REG_8BIT(0x3452), 0x17 },
> -	{ IMX290_REG_8BIT(0x3453), 0x00 },
> -	{ IMX290_REG_8BIT(0x3454), 0x17 },
> -	{ IMX290_REG_8BIT(0x3455), 0x00 },
> +	{ IMX290_REPETITION, 0x10 },
> +	{ IMX290_TCLKPOST, 79 },
> +	{ IMX290_THSZERO, 47 },
> +	{ IMX290_THSPREPARE, 23 },
> +	{ IMX290_TCLKTRAIL, 23 },
> +	{ IMX290_THSTRAIL, 23 },
> +	{ IMX290_TCLKZERO, 87 },
> +	{ IMX290_TCLKPREPARE, 23 },
> +	{ IMX290_TLPX, 23 },
>  };
> 
>  static const struct imx290_regval imx290_10bit_settings[] = {
> -	{ IMX290_REG_8BIT(0x3005), 0x00},
> -	{ IMX290_REG_8BIT(0x3046), 0x00},
> -	{ IMX290_REG_8BIT(0x3129), 0x1d},
> -	{ IMX290_REG_8BIT(0x317c), 0x12},
> -	{ IMX290_REG_8BIT(0x31ec), 0x37},
> -	{ IMX290_REG_8BIT(0x3441), 0x0a},
> -	{ IMX290_REG_8BIT(0x3442), 0x0a},
> -	{ IMX290_REG_8BIT(0x300a), 0x3c},
> -	{ IMX290_REG_8BIT(0x300b), 0x00},
> +	{ IMX290_ADBIT, IMX290_ADBIT_10BIT },
> +	{ IMX290_OUT_CTRL, IMX290_ODBIT_10BIT },
> +	{ IMX290_ADBIT1, IMX290_ADBIT1_10BIT },
> +	{ IMX290_ADBIT2, IMX290_ADBIT2_10BIT },
> +	{ IMX290_ADBIT3, IMX290_ADBIT3_10BIT },
> +	{ IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW10 },
> +	{ IMX290_BLKLEVEL, 60 },
>  };
> 
>  static const struct imx290_regval imx290_12bit_settings[] = {
> -	{ IMX290_REG_8BIT(0x3005), 0x01 },
> -	{ IMX290_REG_8BIT(0x3046), 0x01 },
> -	{ IMX290_REG_8BIT(0x3129), 0x00 },
> -	{ IMX290_REG_8BIT(0x317c), 0x00 },
> -	{ IMX290_REG_8BIT(0x31ec), 0x0e },
> -	{ IMX290_REG_8BIT(0x3441), 0x0c },
> -	{ IMX290_REG_8BIT(0x3442), 0x0c },
> -	{ IMX290_REG_8BIT(0x300a), 0xf0 },
> -	{ IMX290_REG_8BIT(0x300b), 0x00 },
> +	{ IMX290_ADBIT, IMX290_ADBIT_12BIT },
> +	{ IMX290_OUT_CTRL, IMX290_ODBIT_12BIT },
> +	{ IMX290_ADBIT1, IMX290_ADBIT1_12BIT },
> +	{ IMX290_ADBIT2, IMX290_ADBIT2_12BIT },
> +	{ IMX290_ADBIT3, IMX290_ADBIT3_12BIT },
> +	{ IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW12 },
> +	{ IMX290_BLKLEVEL, 240 },
>  };
> 
>  /* supported link frequencies */

Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>

[PATCH v2.1 11/20] media: i2c: imx290: Define more register macros

[Laurent Pinchart]

Define macros for all registers programmed by the driver for which
documentation is available to increase readability. This starts making
use of 16-bit registers in the register arrays, so the value field has
to be increased to 32 bits.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes since v2:

- Fix ADBIT3 register width
---
 drivers/media/i2c/imx290.c | 219 +++++++++++++++++++++----------------
 1 file changed, 124 insertions(+), 95 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index eb007c40ff55..c7b55953f5b1 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -31,14 +31,73 @@
 #define IMX290_STANDBY					IMX290_REG_8BIT(0x3000)
 #define IMX290_REGHOLD					IMX290_REG_8BIT(0x3001)
 #define IMX290_XMSTA					IMX290_REG_8BIT(0x3002)
+#define IMX290_ADBIT					IMX290_REG_8BIT(0x3005)
+#define IMX290_ADBIT_10BIT				(0 << 0)
+#define IMX290_ADBIT_12BIT				(1 << 0)
+#define IMX290_CTRL_07					IMX290_REG_8BIT(0x3007)
+#define IMX290_VREVERSE					BIT(0)
+#define IMX290_HREVERSE					BIT(1)
+#define IMX290_WINMODE_1080P				(0 << 4)
+#define IMX290_WINMODE_720P				(1 << 4)
+#define IMX290_WINMODE_CROP				(4 << 4)
 #define IMX290_FR_FDG_SEL				IMX290_REG_8BIT(0x3009)
 #define IMX290_BLKLEVEL					IMX290_REG_16BIT(0x300a)
 #define IMX290_GAIN					IMX290_REG_8BIT(0x3014)
+#define IMX290_VMAX					IMX290_REG_24BIT(0x3018)
 #define IMX290_HMAX					IMX290_REG_16BIT(0x301c)
+#define IMX290_SHS1					IMX290_REG_24BIT(0x3020)
+#define IMX290_WINWV_OB					IMX290_REG_8BIT(0x303a)
+#define IMX290_WINPV					IMX290_REG_16BIT(0x303c)
+#define IMX290_WINWV					IMX290_REG_16BIT(0x303e)
+#define IMX290_WINPH					IMX290_REG_16BIT(0x3040)
+#define IMX290_WINWH					IMX290_REG_16BIT(0x3042)
+#define IMX290_OUT_CTRL					IMX290_REG_8BIT(0x3046)
+#define IMX290_ODBIT_10BIT				(0 << 0)
+#define IMX290_ODBIT_12BIT				(1 << 0)
+#define IMX290_OPORTSEL_PARALLEL			(0x0 << 4)
+#define IMX290_OPORTSEL_LVDS_2CH			(0xd << 4)
+#define IMX290_OPORTSEL_LVDS_4CH			(0xe << 4)
+#define IMX290_OPORTSEL_LVDS_8CH			(0xf << 4)
+#define IMX290_XSOUTSEL					IMX290_REG_8BIT(0x304b)
+#define IMX290_XSOUTSEL_XVSOUTSEL_HIGH			(0 << 0)
+#define IMX290_XSOUTSEL_XVSOUTSEL_VSYNC			(2 << 0)
+#define IMX290_XSOUTSEL_XHSOUTSEL_HIGH			(0 << 2)
+#define IMX290_XSOUTSEL_XHSOUTSEL_HSYNC			(2 << 2)
+#define IMX290_INCKSEL1					IMX290_REG_8BIT(0x305c)
+#define IMX290_INCKSEL2					IMX290_REG_8BIT(0x305d)
+#define IMX290_INCKSEL3					IMX290_REG_8BIT(0x305e)
+#define IMX290_INCKSEL4					IMX290_REG_8BIT(0x305f)
 #define IMX290_PGCTRL					IMX290_REG_8BIT(0x308c)
+#define IMX290_ADBIT1					IMX290_REG_8BIT(0x3129)
+#define IMX290_ADBIT1_10BIT				0x1d
+#define IMX290_ADBIT1_12BIT				0x00
+#define IMX290_INCKSEL5					IMX290_REG_8BIT(0x315e)
+#define IMX290_INCKSEL6					IMX290_REG_8BIT(0x3164)
+#define IMX290_ADBIT2					IMX290_REG_8BIT(0x317c)
+#define IMX290_ADBIT2_10BIT				0x12
+#define IMX290_ADBIT2_12BIT				0x00
 #define IMX290_CHIP_ID					IMX290_REG_16BIT(0x319a)
+#define IMX290_ADBIT3					IMX290_REG_8BIT(0x31ec)
+#define IMX290_ADBIT3_10BIT				0x37
+#define IMX290_ADBIT3_12BIT				0x0e
+#define IMX290_REPETITION				IMX290_REG_8BIT(0x3405)
 #define IMX290_PHY_LANE_NUM				IMX290_REG_8BIT(0x3407)
+#define IMX290_OPB_SIZE_V				IMX290_REG_8BIT(0x3414)
+#define IMX290_Y_OUT_SIZE				IMX290_REG_16BIT(0x3418)
+#define IMX290_CSI_DT_FMT				IMX290_REG_16BIT(0x3441)
+#define IMX290_CSI_DT_FMT_RAW10				0x0a0a
+#define IMX290_CSI_DT_FMT_RAW12				0x0c0c
 #define IMX290_CSI_LANE_MODE				IMX290_REG_8BIT(0x3443)
+#define IMX290_EXTCK_FREQ				IMX290_REG_16BIT(0x3444)
+#define IMX290_TCLKPOST					IMX290_REG_16BIT(0x3446)
+#define IMX290_THSZERO					IMX290_REG_16BIT(0x3448)
+#define IMX290_THSPREPARE				IMX290_REG_16BIT(0x344a)
+#define IMX290_TCLKTRAIL				IMX290_REG_16BIT(0x344c)
+#define IMX290_THSTRAIL					IMX290_REG_16BIT(0x344e)
+#define IMX290_TCLKZERO					IMX290_REG_16BIT(0x3450)
+#define IMX290_TCLKPREPARE				IMX290_REG_16BIT(0x3452)
+#define IMX290_TLPX					IMX290_REG_16BIT(0x3454)
+#define IMX290_X_OUT_SIZE				IMX290_REG_16BIT(0x3472)
 
 #define IMX290_PGCTRL_REGEN				BIT(0)
 #define IMX290_PGCTRL_THRU				BIT(1)
@@ -54,7 +113,7 @@ static const char * const imx290_supply_name[] = {
 
 struct imx290_regval {
 	u32 reg;
-	u8 val;
+	u32 val;
 };
 
 struct imx290_mode {
@@ -116,22 +175,16 @@ static const char * const imx290_test_pattern_menu[] = {
 };
 
 static const struct imx290_regval imx290_global_init_settings[] = {
-	{ IMX290_REG_8BIT(0x3007), 0x00 },
-	{ IMX290_REG_8BIT(0x3018), 0x65 },
-	{ IMX290_REG_8BIT(0x3019), 0x04 },
-	{ IMX290_REG_8BIT(0x301a), 0x00 },
-	{ IMX290_REG_8BIT(0x3444), 0x20 },
-	{ IMX290_REG_8BIT(0x3445), 0x25 },
-	{ IMX290_REG_8BIT(0x303a), 0x0c },
-	{ IMX290_REG_8BIT(0x3040), 0x00 },
-	{ IMX290_REG_8BIT(0x3041), 0x00 },
-	{ IMX290_REG_8BIT(0x303c), 0x00 },
-	{ IMX290_REG_8BIT(0x303d), 0x00 },
-	{ IMX290_REG_8BIT(0x3042), 0x9c },
-	{ IMX290_REG_8BIT(0x3043), 0x07 },
-	{ IMX290_REG_8BIT(0x303e), 0x49 },
-	{ IMX290_REG_8BIT(0x303f), 0x04 },
-	{ IMX290_REG_8BIT(0x304b), 0x0a },
+	{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
+	{ IMX290_VMAX, 1125 },
+	{ IMX290_EXTCK_FREQ, 0x2520 },
+	{ IMX290_WINWV_OB, 12 },
+	{ IMX290_WINPH, 0 },
+	{ IMX290_WINPV, 0 },
+	{ IMX290_WINWH, 1948 },
+	{ IMX290_WINWV, 1097 },
+	{ IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC |
+			   IMX290_XSOUTSEL_XHSOUTSEL_HSYNC },
 	{ IMX290_REG_8BIT(0x300f), 0x00 },
 	{ IMX290_REG_8BIT(0x3010), 0x21 },
 	{ IMX290_REG_8BIT(0x3012), 0x64 },
@@ -177,102 +230,78 @@ static const struct imx290_regval imx290_global_init_settings[] = {
 
 static const struct imx290_regval imx290_1080p_settings[] = {
 	/* mode settings */
-	{ IMX290_REG_8BIT(0x3007), 0x00 },
-	{ IMX290_REG_8BIT(0x303a), 0x0c },
-	{ IMX290_REG_8BIT(0x3414), 0x0a },
-	{ IMX290_REG_8BIT(0x3472), 0x80 },
-	{ IMX290_REG_8BIT(0x3473), 0x07 },
-	{ IMX290_REG_8BIT(0x3418), 0x38 },
-	{ IMX290_REG_8BIT(0x3419), 0x04 },
+	{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
+	{ IMX290_WINWV_OB, 12 },
+	{ IMX290_OPB_SIZE_V, 10 },
+	{ IMX290_X_OUT_SIZE, 1920 },
+	{ IMX290_Y_OUT_SIZE, 1080 },
 	{ IMX290_REG_8BIT(0x3012), 0x64 },
 	{ IMX290_REG_8BIT(0x3013), 0x00 },
-	{ IMX290_REG_8BIT(0x305c), 0x18 },
-	{ IMX290_REG_8BIT(0x305d), 0x03 },
-	{ IMX290_REG_8BIT(0x305e), 0x20 },
-	{ IMX290_REG_8BIT(0x305f), 0x01 },
-	{ IMX290_REG_8BIT(0x315e), 0x1a },
-	{ IMX290_REG_8BIT(0x3164), 0x1a },
+	{ IMX290_INCKSEL1, 0x18 },
+	{ IMX290_INCKSEL2, 0x03 },
+	{ IMX290_INCKSEL3, 0x20 },
+	{ IMX290_INCKSEL4, 0x01 },
+	{ IMX290_INCKSEL5, 0x1a },
+	{ IMX290_INCKSEL6, 0x1a },
 	{ IMX290_REG_8BIT(0x3480), 0x49 },
 	/* data rate settings */
-	{ IMX290_REG_8BIT(0x3405), 0x10 },
-	{ IMX290_REG_8BIT(0x3446), 0x57 },
-	{ IMX290_REG_8BIT(0x3447), 0x00 },
-	{ IMX290_REG_8BIT(0x3448), 0x37 },
-	{ IMX290_REG_8BIT(0x3449), 0x00 },
-	{ IMX290_REG_8BIT(0x344a), 0x1f },
-	{ IMX290_REG_8BIT(0x344b), 0x00 },
-	{ IMX290_REG_8BIT(0x344c), 0x1f },
-	{ IMX290_REG_8BIT(0x344d), 0x00 },
-	{ IMX290_REG_8BIT(0x344e), 0x1f },
-	{ IMX290_REG_8BIT(0x344f), 0x00 },
-	{ IMX290_REG_8BIT(0x3450), 0x77 },
-	{ IMX290_REG_8BIT(0x3451), 0x00 },
-	{ IMX290_REG_8BIT(0x3452), 0x1f },
-	{ IMX290_REG_8BIT(0x3453), 0x00 },
-	{ IMX290_REG_8BIT(0x3454), 0x17 },
-	{ IMX290_REG_8BIT(0x3455), 0x00 },
+	{ IMX290_REPETITION, 0x10 },
+	{ IMX290_TCLKPOST, 87 },
+	{ IMX290_THSZERO, 55 },
+	{ IMX290_THSPREPARE, 31 },
+	{ IMX290_TCLKTRAIL, 31 },
+	{ IMX290_THSTRAIL, 31 },
+	{ IMX290_TCLKZERO, 119 },
+	{ IMX290_TCLKPREPARE, 31 },
+	{ IMX290_TLPX, 23 },
 };
 
 static const struct imx290_regval imx290_720p_settings[] = {
 	/* mode settings */
-	{ IMX290_REG_8BIT(0x3007), 0x10 },
-	{ IMX290_REG_8BIT(0x303a), 0x06 },
-	{ IMX290_REG_8BIT(0x3414), 0x04 },
-	{ IMX290_REG_8BIT(0x3472), 0x00 },
-	{ IMX290_REG_8BIT(0x3473), 0x05 },
-	{ IMX290_REG_8BIT(0x3418), 0xd0 },
-	{ IMX290_REG_8BIT(0x3419), 0x02 },
+	{ IMX290_CTRL_07, IMX290_WINMODE_720P },
+	{ IMX290_WINWV_OB, 6 },
+	{ IMX290_OPB_SIZE_V, 4 },
+	{ IMX290_X_OUT_SIZE, 1280 },
+	{ IMX290_Y_OUT_SIZE, 720 },
 	{ IMX290_REG_8BIT(0x3012), 0x64 },
 	{ IMX290_REG_8BIT(0x3013), 0x00 },
-	{ IMX290_REG_8BIT(0x305c), 0x20 },
-	{ IMX290_REG_8BIT(0x305d), 0x00 },
-	{ IMX290_REG_8BIT(0x305e), 0x20 },
-	{ IMX290_REG_8BIT(0x305f), 0x01 },
-	{ IMX290_REG_8BIT(0x315e), 0x1a },
-	{ IMX290_REG_8BIT(0x3164), 0x1a },
+	{ IMX290_INCKSEL1, 0x20 },
+	{ IMX290_INCKSEL2, 0x00 },
+	{ IMX290_INCKSEL3, 0x20 },
+	{ IMX290_INCKSEL4, 0x01 },
+	{ IMX290_INCKSEL5, 0x1a },
+	{ IMX290_INCKSEL6, 0x1a },
 	{ IMX290_REG_8BIT(0x3480), 0x49 },
 	/* data rate settings */
-	{ IMX290_REG_8BIT(0x3405), 0x10 },
-	{ IMX290_REG_8BIT(0x3446), 0x4f },
-	{ IMX290_REG_8BIT(0x3447), 0x00 },
-	{ IMX290_REG_8BIT(0x3448), 0x2f },
-	{ IMX290_REG_8BIT(0x3449), 0x00 },
-	{ IMX290_REG_8BIT(0x344a), 0x17 },
-	{ IMX290_REG_8BIT(0x344b), 0x00 },
-	{ IMX290_REG_8BIT(0x344c), 0x17 },
-	{ IMX290_REG_8BIT(0x344d), 0x00 },
-	{ IMX290_REG_8BIT(0x344e), 0x17 },
-	{ IMX290_REG_8BIT(0x344f), 0x00 },
-	{ IMX290_REG_8BIT(0x3450), 0x57 },
-	{ IMX290_REG_8BIT(0x3451), 0x00 },
-	{ IMX290_REG_8BIT(0x3452), 0x17 },
-	{ IMX290_REG_8BIT(0x3453), 0x00 },
-	{ IMX290_REG_8BIT(0x3454), 0x17 },
-	{ IMX290_REG_8BIT(0x3455), 0x00 },
+	{ IMX290_REPETITION, 0x10 },
+	{ IMX290_TCLKPOST, 79 },
+	{ IMX290_THSZERO, 47 },
+	{ IMX290_THSPREPARE, 23 },
+	{ IMX290_TCLKTRAIL, 23 },
+	{ IMX290_THSTRAIL, 23 },
+	{ IMX290_TCLKZERO, 87 },
+	{ IMX290_TCLKPREPARE, 23 },
+	{ IMX290_TLPX, 23 },
 };
 
 static const struct imx290_regval imx290_10bit_settings[] = {
-	{ IMX290_REG_8BIT(0x3005), 0x00},
-	{ IMX290_REG_8BIT(0x3046), 0x00},
-	{ IMX290_REG_8BIT(0x3129), 0x1d},
-	{ IMX290_REG_8BIT(0x317c), 0x12},
-	{ IMX290_REG_8BIT(0x31ec), 0x37},
-	{ IMX290_REG_8BIT(0x3441), 0x0a},
-	{ IMX290_REG_8BIT(0x3442), 0x0a},
-	{ IMX290_REG_8BIT(0x300a), 0x3c},
-	{ IMX290_REG_8BIT(0x300b), 0x00},
+	{ IMX290_ADBIT, IMX290_ADBIT_10BIT },
+	{ IMX290_OUT_CTRL, IMX290_ODBIT_10BIT },
+	{ IMX290_ADBIT1, IMX290_ADBIT1_10BIT },
+	{ IMX290_ADBIT2, IMX290_ADBIT2_10BIT },
+	{ IMX290_ADBIT3, IMX290_ADBIT3_10BIT },
+	{ IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW10 },
+	{ IMX290_BLKLEVEL, 60 },
 };
 
 static const struct imx290_regval imx290_12bit_settings[] = {
-	{ IMX290_REG_8BIT(0x3005), 0x01 },
-	{ IMX290_REG_8BIT(0x3046), 0x01 },
-	{ IMX290_REG_8BIT(0x3129), 0x00 },
-	{ IMX290_REG_8BIT(0x317c), 0x00 },
-	{ IMX290_REG_8BIT(0x31ec), 0x0e },
-	{ IMX290_REG_8BIT(0x3441), 0x0c },
-	{ IMX290_REG_8BIT(0x3442), 0x0c },
-	{ IMX290_REG_8BIT(0x300a), 0xf0 },
-	{ IMX290_REG_8BIT(0x300b), 0x00 },
+	{ IMX290_ADBIT, IMX290_ADBIT_12BIT },
+	{ IMX290_OUT_CTRL, IMX290_ODBIT_12BIT },
+	{ IMX290_ADBIT1, IMX290_ADBIT1_12BIT },
+	{ IMX290_ADBIT2, IMX290_ADBIT2_12BIT },
+	{ IMX290_ADBIT3, IMX290_ADBIT3_12BIT },
+	{ IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW12 },
+	{ IMX290_BLKLEVEL, 240 },
 };
 
 /* supported link frequencies */
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 18/20] media: i2c: imx290: Factor out format retrieval to separate function

[Laurent Pinchart]

Hi Alexander,

On Mon, Oct 17, 2022 at 07:55:28AM +0200, Alexander Stein wrote:
> Am Sonntag, 16. Oktober 2022, 08:15:21 CEST schrieb Laurent Pinchart:
> > The driver duplicates the same pattern to access the try or active
> > format in multiple locations. Factor it out to a separate function.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Avoid returning NULL from imx290_get_pad_format()
> > ---
> >  drivers/media/i2c/imx290.c | 24 ++++++++++++++----------
> >  1 file changed, 14 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 0b34d60f8ce2..b0ff0e8ed45a 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -519,6 +519,16 @@ static const struct v4l2_ctrl_ops imx290_ctrl_ops = {
> >  	.s_ctrl = imx290_set_ctrl,
> >  };
> > 
> > +static struct v4l2_mbus_framefmt *
> > +imx290_get_pad_format(struct imx290 *imx290, struct v4l2_subdev_state *state,
> > +		      u32 which)
> > +{
> > +	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > +		return &imx290->current_format;
> > +	else
> > +		return v4l2_subdev_get_try_format(&imx290->sd, state, 0);
> > +}
> > +
> 
> v4l2_subdev_get_try_format can return NULL, which would be dereferenced later 
> on. But this happens only if state is NULL itself, which will raise a WARN_ON 
> anyway. So i guess this is fine.

I'll add a patch on top of the series to convert the driver to the V4L2
active state, that should simplify all this.

> Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> 
> >  static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
> >  				 struct v4l2_subdev_state *sd_state,
> >  				 struct v4l2_subdev_mbus_code_enum *code)
> > @@ -562,12 +572,7 @@ static int imx290_get_fmt(struct v4l2_subdev *sd,
> > 
> >  	mutex_lock(&imx290->lock);
> > 
> > -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> > -		framefmt = v4l2_subdev_get_try_format(&imx290->sd, sd_state,
> > -						      fmt->pad);
> > -	else
> > -		framefmt = &imx290->current_format;
> > -
> > +	framefmt = imx290_get_pad_format(imx290, sd_state, fmt->which);
> >  	fmt->format = *framefmt;
> > 
> >  	mutex_unlock(&imx290->lock);
> > @@ -627,10 +632,9 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> >  	fmt->format.code = imx290_formats[i].code;
> >  	fmt->format.field = V4L2_FIELD_NONE;
> > 
> > -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > -		format = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> > -	} else {
> > -		format = &imx290->current_format;
> > +	format = imx290_get_pad_format(imx290, sd_state, fmt->which);
> > +
> > +	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> >  		imx290->current_mode = mode;
> >  		imx290->bpp = imx290_formats[i].bpp;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 01/20] media: dt-bindings: Convert imx290.txt to YAML

[Dave Stevenson]

Hi Laurent

On Sun, 16 Oct 2022 at 07:15, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Convert the Sony IMX290 DT binding from text to YAML. Add Manivannan as
> a maintainer given that he is listed in MAINTAINERS for the file, as
> volunteering myself.
>
> The name of the input clock, "xclk", is wrong as the hardware manual
> names it INCK. As the device has a single clock, the name could be
> omitted, but that would require a corresponding change to the driver and
> is thus a candidate for further patches.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../devicetree/bindings/media/i2c/imx290.txt  |  57 --------
>  .../bindings/media/i2c/sony,imx290.yaml       | 129 ++++++++++++++++++
>  MAINTAINERS                                   |   2 +-
>  3 files changed, 130 insertions(+), 58 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/media/i2c/imx290.txt
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/imx290.txt b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> deleted file mode 100644
> index a3cc21410f7c..000000000000
> --- a/Documentation/devicetree/bindings/media/i2c/imx290.txt
> +++ /dev/null
> @@ -1,57 +0,0 @@
> -* Sony IMX290 1/2.8-Inch CMOS Image Sensor
> -
> -The Sony IMX290 is a 1/2.8-Inch CMOS Solid-state image sensor with
> -Square Pixel for Color Cameras. It is programmable through I2C and 4-wire
> -interfaces. The sensor output is available via CMOS logic parallel SDR output,
> -Low voltage LVDS DDR output and CSI-2 serial data output. The CSI-2 bus is the
> -default. No bindings have been defined for the other busses.
> -
> -Required Properties:
> -- compatible: Should be "sony,imx290"
> -- reg: I2C bus address of the device
> -- clocks: Reference to the xclk clock.
> -- clock-names: Should be "xclk".
> -- clock-frequency: Frequency of the xclk clock in Hz.
> -- vdddo-supply: Sensor digital IO regulator.
> -- vdda-supply: Sensor analog regulator.
> -- vddd-supply: Sensor digital core regulator.
> -
> -Optional Properties:
> -- reset-gpios: Sensor reset GPIO
> -
> -The imx290 device node should contain one 'port' child node with
> -an 'endpoint' subnode. For further reading on port node refer to
> -Documentation/devicetree/bindings/media/video-interfaces.txt.
> -
> -Required Properties on endpoint:
> -- data-lanes: check ../video-interfaces.txt
> -- link-frequencies: check ../video-interfaces.txt
> -- remote-endpoint: check ../video-interfaces.txt
> -
> -Example:
> -       &i2c1 {
> -               ...
> -               imx290: camera-sensor@1a {
> -                       compatible = "sony,imx290";
> -                       reg = <0x1a>;
> -
> -                       reset-gpios = <&msmgpio 35 GPIO_ACTIVE_LOW>;
> -                       pinctrl-names = "default";
> -                       pinctrl-0 = <&camera_rear_default>;
> -
> -                       clocks = <&gcc GCC_CAMSS_MCLK0_CLK>;
> -                       clock-names = "xclk";
> -                       clock-frequency = <37125000>;
> -
> -                       vdddo-supply = <&camera_vdddo_1v8>;
> -                       vdda-supply = <&camera_vdda_2v8>;
> -                       vddd-supply = <&camera_vddd_1v5>;
> -
> -                       port {
> -                               imx290_ep: endpoint {
> -                                       data-lanes = <1 2 3 4>;
> -                                       link-frequencies = /bits/ 64 <445500000>;
> -                                       remote-endpoint = <&csiphy0_ep>;
> -                               };
> -                       };
> -               };
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
> new file mode 100644
> index 000000000000..21377daae026
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
> @@ -0,0 +1,129 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/sony,imx290.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sony IMX290 1/2.8-Inch CMOS Image Sensor
> +
> +maintainers:
> +  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> +description: |-
> +  The Sony IMX290 is a 1/2.8-Inch CMOS Solid-state image sensor with Square
> +  Pixel for Color Cameras. It is programmable through I2C and 4-wire
> +  interfaces. The sensor output is available via CMOS logic parallel SDR
> +  output, Low voltage LVDS DDR output and CSI-2 serial data output. The CSI-2
> +  bus is the default. No bindings have been defined for the other busses.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - sony,imx290
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    description: Input clock (37.125 MHz or 74.25 MHz)
> +    items:
> +      - const: xclk
> +
> +  clock-frequency:
> +    description: Frequency of the xclk clock in Hz
> +
> +  vdda-supply:
> +    description: Analog power supply (2.9V)
> +
> +  vddd-supply:
> +    description: Digital core power supply (1.2V)
> +
> +  vdddo-supply:
> +    description: Digital I/O power supply (1.8V)
> +
> +  reset-gpios:
> +    description: Sensor reset (XCLR) GPIO
> +    maxItems: 1
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    description: |
> +      Video output port
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml#
> +        unevaluatedProperties: false
> +
> +        properties:
> +          data-lanes:
> +            anyOf:
> +              - items:
> +                  - const: 1
> +                  - const: 2
> +              - items:
> +                  - const: 1
> +                  - const: 2
> +                  - const: 3
> +                  - const: 4
> +
> +          link-frequencies: true
> +
> +        required:
> +          - data-lanes
> +          - link-frequencies
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - clock-frequency
> +  - vdda-supply
> +  - vddd-supply
> +  - vdddo-supply
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        imx290: camera-sensor@1a {
> +            compatible = "sony,imx290";
> +            reg = <0x1a>;
> +
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&camera_rear_default>;
> +
> +            clocks = <&gcc 90>;
> +            clock-names = "xclk";
> +            clock-frequency = <37125000>;
> +
> +            vdddo-supply = <&camera_vdddo_1v8>;
> +            vdda-supply = <&camera_vdda_2v8>;
> +            vddd-supply = <&camera_vddd_1v5>;
> +
> +            reset-gpios = <&msmgpio 35 GPIO_ACTIVE_LOW>;
> +
> +            port {
> +                imx290_ep: endpoint {
> +                    data-lanes = <1 2 3 4>;
> +                    link-frequencies = /bits/ 64 <445500000>;

Minor nit that this won't work with the current Linux driver due to a
driver restriction implementing the recommended settings from Sony.
OV8865 has the same restrictions and notes it in the binding[1]. I
don't know if this is the preferred approach or not.

  Dave

[1] https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/media/i2c/ov8856.yaml#L78

> +                    remote-endpoint = <&csiphy0_ep>;
> +                };
> +            };
> +        };
> +    };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 72b9654f764c..c431fd20e89b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18982,7 +18982,7 @@ M:      Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>  L:     linux-media@vger.kernel.org
>  S:     Maintained
>  T:     git git://linuxtv.org/media_tree.git
> -F:     Documentation/devicetree/bindings/media/i2c/imx290.txt
> +F:     Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
>  F:     drivers/media/i2c/imx290.c
>
>  SONY IMX319 SENSOR DRIVER
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 01/20] media: dt-bindings: Convert imx290.txt to YAML

[Sakari Ailus]

Hi Dave,

On Mon, Oct 17, 2022 at 02:57:58PM +0100, Dave Stevenson wrote:
> > +                    link-frequencies = /bits/ 64 <445500000>;
> 
> Minor nit that this won't work with the current Linux driver due to a
> driver restriction implementing the recommended settings from Sony.
> OV8865 has the same restrictions and notes it in the binding[1]. I
> don't know if this is the preferred approach or not.

The DT describes hardware and is not bound by (current) driver limitations.
So I'd say that if the hardware supports multiple frequencies, the
supported ones need to be listed here.

I'd also like to merge the series, it's been out of tree for long already.

-- 
Kind regards,

Sakari Ailus