media: Add Sony IMX585 image sensor support

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +16 -0

+
+  clocks:
+    description: Clock frequency 74.25MHz, 37.125MHz, 72MHz, 27MHz, 24MHz
+    maxItems: 1
+
+  vana-supply:
+    description: Analog power supply (3.3V)
+
+  vddl-supply:
+    description: Interface power supply (1.8V)
+
+  vdig-supply:
+    description: Digital power supply (1.1V)
+
+  reset-gpios:
+    description: Sensor reset (XCLR) GPIO

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +13 -0

+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            oneOf:

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +10 -0

+
+        required:
+          - data-lanes
+          - link-frequencies
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - port

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml line 19

Quoted 8 lines · Will Whang

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +8 -0

+description:
+  IMX585 sensor is a Sony CMOS sensor with 4K and FHD outputs.
+
+properties:
+  compatible:
+    enum:
+      - sony,imx585
+      - sony,imx585-mono

I don't understand this second compatible. Is this different hardware? Can you point me to "mono" datasheet?

Your description should explain this. Commit msg as well, instead of speaking about driver (in fact drop all driver related comments).

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml line 26

Quoted 7 lines

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +7 -0

+
+  reg:
+    maxItems: 1
+
+  assigned-clocks: true
+  assigned-clock-parents: true
+  assigned-clock-rates: true

Drop all three.

Quoted 16 lines

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +16 -0

+
+  clocks:
+    description: Clock frequency 74.25MHz, 37.125MHz, 72MHz, 27MHz, 24MHz
+    maxItems: 1
+
+  vana-supply:
+    description: Analog power supply (3.3V)
+
+  vddl-supply:
+    description: Interface power supply (1.8V)
+
+  vdig-supply:
+    description: Digital power supply (1.1V)
+
+  reset-gpios:
+    description: Sensor reset (XCLR) GPIO

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml line 52

Quoted 10 lines

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +10 -0

+    maxItems: 1
+
+  sony,sync-mode:
+    description: |
+      Select the synchronisation mode of the sensor
+        0 – internal sync, leader (default)
+        1 – internal sync, follower
+        2 – external sync
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [ 0, 1, 2 ]

Previous comments not applied.

Quoted 17 lines

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +17 -0

+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            oneOf:
+              - items:
+                  - const: 1
+                  - const: 2
+              - items:

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml line 75

Quoted 6 lines

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +6 -0

+                  - const: 1
+                  - const: 2
+                  - const: 3
+                  - const: 4
+
+          link-frequencies: true

Drop

Quoted 11 lines

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +10 -0

+
+        required:
+          - data-lanes
+          - link-frequencies
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - port

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml line 95

Quoted 10 lines

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +10 -0

+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        imx585@1a {

Nothing improved.

You replied that you applied comment, but send the same.

Best regards,
Krzysztof

Hi Krzysztof,
Reply inline.
Thanks,
Will Whang

Quoted 14 lines · Krzysztof Kozlowski

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml line 19

Quoted 8 lines · Will Whang

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +8 -0

+description:
+  IMX585 sensor is a Sony CMOS sensor with 4K and FHD outputs.
+
+properties:
+  compatible:
+    enum:
+      - sony,imx585
+      - sony,imx585-mono

I don't understand this second compatible. Is this different hardware? Can you point me to "mono" datasheet?

Quoted 3 lines

Your description should explain this. Commit msg as well, instead of speaking about driver (in fact drop all driver related comments).

Mono version of this sensor is basically just removing the bayer
filter, so the sensor itself actually doesn't know if it is color or
mono and from my knowledge there are no registers programmed in the
factory that will show the variant and model number. (That is why when
the driver probing it only test blacklevel register because there are
no ID registers)
Originally in V1 patch I've made the switch between color and mono in
dtoverlay config but reviewer comments is to move it to compatible
string and not property.(https://lore.kernel.org/linux-media/[email protected]/)

In this case, what would you recommend?

compatible:
enum:
- sony,imx585
- sony,imx585-mono
description: IMX585 has two variants, color and mono which the
driver supports both.

Quoted 23 lines · Will Whang

Hi Krzysztof,
Reply inline.
Thanks,
Will Whang

Quoted 14 lines · Krzysztof Kozlowski

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml line 19

Quoted 8 lines · Will Whang

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +8 -0

+description:
+  IMX585 sensor is a Sony CMOS sensor with 4K and FHD outputs.
+
+properties:
+  compatible:
+    enum:
+      - sony,imx585
+      - sony,imx585-mono

I don't understand this second compatible. Is this different hardware? Can you point me to "mono" datasheet?

Quoted 3 lines

Your description should explain this. Commit msg as well, instead of speaking about driver (in fact drop all driver related comments).

Quoted 9 lines

Mono version of this sensor is basically just removing the bayer
filter, so the sensor itself actually doesn't know if it is color or
mono and from my knowledge there are no registers programmed in the
factory that will show the variant and model number. (That is why when
the driver probing it only test blacklevel register because there are
no ID registers)
Originally in V1 patch I've made the switch between color and mono in
dtoverlay config but reviewer comments is to move it to compatible
string and not property.(https://lore.kernel.org/linux-media/[email protected]/)

You only partially answer and judging by mentioning driver below:

Quoted 9 lines

In this case, what would you recommend?

compatible:
enum:
- sony,imx585
- sony,imx585-mono
description: IMX585 has two variants, color and mono which the
driver supports both.

... I still have doubts that you really understand what I am asking. Is this one device or two different devices?

Best regards,
Krzysztof

Hi Krzysztof,

Reply inline,
Thanks,
Will Whang

Quoted 48 lines · Krzysztof Kozlowski

Quoted 23 lines · Will Whang

Hi Krzysztof,
Reply inline.
Thanks,
Will Whang

Quoted 14 lines · Krzysztof Kozlowski

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml line 19

Quoted 8 lines · Will Whang

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +8 -0

+description:
+  IMX585 sensor is a Sony CMOS sensor with 4K and FHD outputs.
+
+properties:
+  compatible:
+    enum:
+      - sony,imx585
+      - sony,imx585-mono

I don't understand this second compatible. Is this different hardware? Can you point me to "mono" datasheet?

Quoted 3 lines

Your description should explain this. Commit msg as well, instead of speaking about driver (in fact drop all driver related comments).

Quoted 9 lines

Mono version of this sensor is basically just removing the bayer
filter, so the sensor itself actually doesn't know if it is color or
mono and from my knowledge there are no registers programmed in the
factory that will show the variant and model number. (That is why when
the driver probing it only test blacklevel register because there are
no ID registers)
Originally in V1 patch I've made the switch between color and mono in
dtoverlay config but reviewer comments is to move it to compatible
string and not property.(https://lore.kernel.org/linux-media/[email protected]/)

You only partially answer and judging by mentioning driver below:

Quoted 9 lines

In this case, what would you recommend?

compatible:
enum:
- sony,imx585
- sony,imx585-mono
description: IMX585 has two variants, color and mono which the
driver supports both.

Quoted 2 lines

... I still have doubts that you really understand what I am asking. Is this one device or two different devices?

One device that has two variants: IMX585-AAMJ1 (Mono) and IMX585-AAQJ1 (Color). Silicon-wise the difference is just with or without bayer filter.

Quoted 43 lines · Will Whang

Quoted 41 lines · Krzysztof Kozlowski

Quoted 17 lines · Will Whang

Quoted 13 lines · Krzysztof Kozlowski

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml line 19

Quoted 8 lines · Will Whang

Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +8 -0

+description:
+  IMX585 sensor is a Sony CMOS sensor with 4K and FHD outputs.
+
+properties:
+  compatible:
+    enum:
+      - sony,imx585
+      - sony,imx585-mono

I don't understand this second compatible. Is this different hardware? Can you point me to "mono" datasheet?

Quoted 3 lines

Your description should explain this. Commit msg as well, instead of speaking about driver (in fact drop all driver related comments).

Quoted 9 lines

Mono version of this sensor is basically just removing the bayer
filter, so the sensor itself actually doesn't know if it is color or
mono and from my knowledge there are no registers programmed in the
factory that will show the variant and model number. (That is why when
the driver probing it only test blacklevel register because there are
no ID registers)
Originally in V1 patch I've made the switch between color and mono in
dtoverlay config but reviewer comments is to move it to compatible
string and not property.(https://lore.kernel.org/linux-media/[email protected]/)

You only partially answer and judging by mentioning driver below:

Quoted 8 lines

In this case, what would you recommend?

compatible:
enum:
- sony,imx585
- sony,imx585-mono
description: IMX585 has two variants, color and mono which the
driver supports both.

... I still have doubts that you really understand what I am asking. Is this one device or two different devices?

Quoted 3 lines

One device that has two variants: IMX585-AAMJ1 (Mono) and IMX585-AAQJ1 (Color). Silicon-wise the difference is just with or without bayer filter.

Then I would propose to use sony,imx585-aamj1 and -aaqj1 with short explanation either in comment or description about difference in RGB mosaic filter.

Quoted 48 lines · Krzysztof Kozlowski

Quoted 46 lines · Will Whang

Quoted 39 lines · Krzysztof Kozlowski

Quoted 17 lines · Will Whang

Quoted 13 lines · Krzysztof Kozlowski

Review

Quoted 8 lines · Will Whang

(quoted diff) +8 -0

+description:
+  IMX585 sensor is a Sony CMOS sensor with 4K and FHD outputs.
+
+properties:
+  compatible:
+    enum:
+      - sony,imx585
+      - sony,imx585-mono

I don't understand this second compatible. Is this different hardware? Can you point me to "mono" datasheet?

Quoted 3 lines

Your description should explain this. Commit msg as well, instead of speaking about driver (in fact drop all driver related comments).

Quoted 9 lines

Mono version of this sensor is basically just removing the bayer
filter, so the sensor itself actually doesn't know if it is color or
mono and from my knowledge there are no registers programmed in the
factory that will show the variant and model number. (That is why when
the driver probing it only test blacklevel register because there are
no ID registers)
Originally in V1 patch I've made the switch between color and mono in
dtoverlay config but reviewer comments is to move it to compatible
string and not property.(https://lore.kernel.org/linux-media/[email protected]/)

You only partially answer and judging by mentioning driver below:

Quoted 8 lines

In this case, what would you recommend?

compatible:
enum:
- sony,imx585
- sony,imx585-mono
description: IMX585 has two variants, color and mono which the
driver supports both.

Quoted 2 lines

... I still have doubts that you really understand what I am asking. Is this one device or two different devices?

One device that has two variants: IMX585-AAMJ1 (Mono) and IMX585-AAQJ1 (Color). Silicon-wise the difference is just with or without bayer filter.

Quoted 3 lines

Then I would propose to use sony,imx585-aamj1 and -aaqj1 with short explanation either in comment or description about difference in RGB mosaic filter.

Works for me. We could possibly omit the "j1" suffix too.

--
Regards,

Laurent Pinchart

Quoted 50 lines · Laurent Pinchart

Quoted 48 lines · Krzysztof Kozlowski

Quoted 46 lines · Will Whang

Quoted 39 lines · Krzysztof Kozlowski

Quoted 17 lines · Will Whang

Quoted 13 lines · Krzysztof Kozlowski

Review

Quoted 8 lines · Will Whang

(quoted diff) +8 -0

+description:
+  IMX585 sensor is a Sony CMOS sensor with 4K and FHD outputs.
+
+properties:
+  compatible:
+    enum:
+      - sony,imx585
+      - sony,imx585-mono

I don't understand this second compatible. Is this different hardware? Can you point me to "mono" datasheet?

Quoted 3 lines

Your description should explain this. Commit msg as well, instead of speaking about driver (in fact drop all driver related comments).

Quoted 9 lines

Mono version of this sensor is basically just removing the bayer
filter, so the sensor itself actually doesn't know if it is color or
mono and from my knowledge there are no registers programmed in the
factory that will show the variant and model number. (That is why when
the driver probing it only test blacklevel register because there are
no ID registers)
Originally in V1 patch I've made the switch between color and mono in
dtoverlay config but reviewer comments is to move it to compatible
string and not property.(https://lore.kernel.org/linux-media/[email protected]/)

You only partially answer and judging by mentioning driver below:

Quoted 8 lines

In this case, what would you recommend?

compatible:
enum:
- sony,imx585
- sony,imx585-mono
description: IMX585 has two variants, color and mono which the
driver supports both.

Quoted 2 lines

... I still have doubts that you really understand what I am asking. Is this one device or two different devices?

One device that has two variants: IMX585-AAMJ1 (Mono) and IMX585-AAQJ1 (Color). Silicon-wise the difference is just with or without bayer filter.

Quoted 6 lines

Quoted 3 lines

Then I would propose to use sony,imx585-aamj1 and -aaqj1 with short explanation either in comment or description about difference in RGB mosaic filter.

Works for me. We could possibly omit the "j1" suffix too.

My thinking is that imx585 and imx585-mono are easier to comprehend than IMX585-AAM and IMX585-AAQ.
Because in dtoverlay for the users/me they will have to know what is the exact name instead of easy to remember name.

dtoverlay=imx585-aam
is not as nice as
dtoverlay=imx585-mono

which is what it does, a mono variant of the sensor.

I really don't understand the standard for compatible string naming here, is there something I missed? Is it required to use the full name of the sensor parts number as a compatible string?

Quoted 63 lines · Will Whang

Quoted 50 lines · Laurent Pinchart

Quoted 48 lines · Krzysztof Kozlowski

Quoted 46 lines · Will Whang

Quoted 39 lines · Krzysztof Kozlowski

Quoted 17 lines · Will Whang

Quoted 13 lines · Krzysztof Kozlowski

Review

Quoted 8 lines · Will Whang

(quoted diff) +8 -0

+description:
+  IMX585 sensor is a Sony CMOS sensor with 4K and FHD outputs.
+
+properties:
+  compatible:
+    enum:
+      - sony,imx585
+      - sony,imx585-mono

I don't understand this second compatible. Is this different hardware? Can you point me to "mono" datasheet?

Quoted 3 lines

Your description should explain this. Commit msg as well, instead of speaking about driver (in fact drop all driver related comments).

Quoted 9 lines

Mono version of this sensor is basically just removing the bayer
filter, so the sensor itself actually doesn't know if it is color or
mono and from my knowledge there are no registers programmed in the
factory that will show the variant and model number. (That is why when
the driver probing it only test blacklevel register because there are
no ID registers)
Originally in V1 patch I've made the switch between color and mono in
dtoverlay config but reviewer comments is to move it to compatible
string and not property.(https://lore.kernel.org/linux-media/[email protected]/)

You only partially answer and judging by mentioning driver below:

Quoted 8 lines

In this case, what would you recommend?

compatible:
enum:
- sony,imx585
- sony,imx585-mono
description: IMX585 has two variants, color and mono which the
driver supports both.

Quoted 2 lines

... I still have doubts that you really understand what I am asking. Is this one device or two different devices?

One device that has two variants: IMX585-AAMJ1 (Mono) and IMX585-AAQJ1 (Color). Silicon-wise the difference is just with or without bayer filter.

Quoted 6 lines

Quoted 3 lines

Then I would propose to use sony,imx585-aamj1 and -aaqj1 with short explanation either in comment or description about difference in RGB mosaic filter.

Works for me. We could possibly omit the "j1" suffix too.

My thinking is that imx585 and imx585-mono are easier to comprehend than IMX585-AAM and IMX585-AAQ.
Because in dtoverlay for the users/me they will have to know what is the exact name instead of easy to remember name.

Quoted 3 lines

dtoverlay=imx585-aam
is not as nice as
dtoverlay=imx585-mono

I have datasheet for AAQ, so how above is easier for me to figure out which compatible I am using?

Quoted 6 lines

which is what it does, a mono variant of the sensor.

I really don't understand the standard for compatible string naming here, is there something I missed? Is it required to use the full name of the sensor parts number as a compatible string?

It's not part number. You have there different models. We don't add prose to compatibles, but use device or model names.

Best regards,
Krzysztof

Quoted 65 lines · Krzysztof Kozlowski

Quoted 63 lines · Will Whang

Quoted 50 lines · Laurent Pinchart

Quoted 48 lines · Krzysztof Kozlowski

Quoted 46 lines · Will Whang

Quoted 39 lines · Krzysztof Kozlowski

Quoted 17 lines · Will Whang

Quoted 13 lines · Krzysztof Kozlowski

Review

Quoted 8 lines · Will Whang

(quoted diff) +8 -0

+description:
+  IMX585 sensor is a Sony CMOS sensor with 4K and FHD outputs.
+
+properties:
+  compatible:
+    enum:
+      - sony,imx585
+      - sony,imx585-mono

I don't understand this second compatible. Is this different hardware? Can you point me to "mono" datasheet?

Quoted 3 lines

Your description should explain this. Commit msg as well, instead of speaking about driver (in fact drop all driver related comments).

Quoted 9 lines

Mono version of this sensor is basically just removing the bayer
filter, so the sensor itself actually doesn't know if it is color or
mono and from my knowledge there are no registers programmed in the
factory that will show the variant and model number. (That is why when
the driver probing it only test blacklevel register because there are
no ID registers)
Originally in V1 patch I've made the switch between color and mono in
dtoverlay config but reviewer comments is to move it to compatible
string and not property.(https://lore.kernel.org/linux-media/[email protected]/)

You only partially answer and judging by mentioning driver below:

Quoted 8 lines

In this case, what would you recommend?

compatible:
enum:
- sony,imx585
- sony,imx585-mono
description: IMX585 has two variants, color and mono which the
driver supports both.

Quoted 2 lines

... I still have doubts that you really understand what I am asking. Is this one device or two different devices?

One device that has two variants: IMX585-AAMJ1 (Mono) and IMX585-AAQJ1 (Color). Silicon-wise the difference is just with or without bayer filter.

Quoted 6 lines

Quoted 3 lines

Then I would propose to use sony,imx585-aamj1 and -aaqj1 with short explanation either in comment or description about difference in RGB mosaic filter.

Works for me. We could possibly omit the "j1" suffix too.

My thinking is that imx585 and imx585-mono are easier to comprehend than IMX585-AAM and IMX585-AAQ.
Because in dtoverlay for the users/me they will have to know what is the exact name instead of easy to remember name.

Quoted 7 lines

Quoted 3 lines

dtoverlay=imx585-aam
is not as nice as
dtoverlay=imx585-mono

I have datasheet for AAQ, so how above is easier for me to figure out which compatible I am using?

I propose this:

compatible:
enum:
- sony,imx585
- sony,imx585-mono
- sony,imx585-AAQJ1
- sony,imx585-AAMJ1

description: IMX585 has two variants, color (IMX585-AAQ) and mono
(IMX585-AAM) which
the driver supports both.

Description is there for a reason, dtoverlay has description also. See sony,imx296.yaml as an example.
If you are looking at AAQ you know it is a color sensor and all the color sensors from sony can be used with imx+three numbers in the current list.
This is following the established convention.

drivers/media/i2c/imx585.c new file +18 -0

+
+/* Update analogue gain limits based on mode/HDR/HCG */
+static void imx585_update_gain_limits(struct imx585 *imx585)
+{
+	const bool hcg_on = imx585->hcg;
+	const bool clear_hdr = imx585->clear_hdr;
+	const u32 min = hcg_on ? IMX585_ANA_GAIN_MIN_HCG : IMX585_ANA_GAIN_MIN_NORMAL;
+	const u32 max = clear_hdr ? IMX585_ANA_GAIN_MAX_HDR : IMX585_ANA_GAIN_MAX_NORMAL;
+	u32 cur = imx585->gain->val;
+
+	__v4l2_ctrl_modify_range(imx585->gain, min, max, IMX585_ANA_GAIN_STEP,
+				 clamp(cur, min, max));
+
+	if (cur < min || cur > max)
+		__v4l2_ctrl_s_ctrl(imx585->gain, clamp(cur, min, max));
+}
+
+/* Recompute per-mode timing limits (HMAX/VMAX) from link/lanes/HDR */

drivers/media/i2c/imx585.c new file +29 -0

+	}
+}
+
+static void imx585_set_framing_limits(struct imx585 *imx585,
+				      const struct imx585_mode *mode)
+{
+	u64 pixel_rate;
+	u64 max_hblank;
+
+	imx585_update_hmax(imx585);
+
+	imx585->vmax = mode->min_vmax;
+	imx585->hmax = mode->min_hmax;
+
+	/* Pixel rate proxy: width * clock / min_hmax */
+	pixel_rate = (u64)mode->width * IMX585_PIXEL_RATE;
+	do_div(pixel_rate, mode->min_hmax);
+	__v4l2_ctrl_modify_range(imx585->pixel_rate, pixel_rate, pixel_rate, 1,
+				 pixel_rate);
+
+	max_hblank = (u64)IMX585_HMAX_MAX * pixel_rate;
+	do_div(max_hblank, IMX585_PIXEL_RATE);
+	max_hblank -= mode->width;
+
+	__v4l2_ctrl_modify_range(imx585->hblank, 0, max_hblank, 1, 0);
+	__v4l2_ctrl_s_ctrl(imx585->hblank, 0);
+
+	__v4l2_ctrl_modify_range(imx585->vblank,
+				 mode->min_vmax - mode->height,

drivers/media/i2c/imx585.c new file +33 -0

+}
+
+/* --------------------------------------------------------------------------
+ * Controls
+ * --------------------------------------------------------------------------
+ */
+
+static int imx585_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct imx585 *imx585 = container_of(ctrl->handler, struct imx585, ctrl_handler);
+	const struct imx585_mode *mode, *mode_list;
+	struct v4l2_subdev_state *state;
+	struct v4l2_mbus_framefmt *fmt;
+	unsigned int num_modes;
+	int ret = 0;
+
+	state = v4l2_subdev_get_locked_active_state(&imx585->sd);
+	fmt = v4l2_subdev_state_get_format(state, 0);
+
+	get_mode_table(imx585, fmt->code, &mode_list, &num_modes);
+	mode = v4l2_find_nearest_size(mode_list, num_modes, width, height,
+				      fmt->width, fmt->height);
+
+	switch (ctrl->id) {
+	case V4L2_CID_WIDE_DYNAMIC_RANGE:
+		if (imx585->clear_hdr != ctrl->val) {
+			u32 code;
+
+			imx585->clear_hdr = ctrl->val;
+
+			v4l2_ctrl_activate(imx585->datasel_th_ctrl,  imx585->clear_hdr);
+			v4l2_ctrl_activate(imx585->datasel_bk_ctrl,  imx585->clear_hdr);
+			v4l2_ctrl_activate(imx585->gdc_th_ctrl,      imx585->clear_hdr);

drivers/media/i2c/imx585.c new file +4 -0

+
+			code = imx585->mono ? MEDIA_BUS_FMT_Y12_1X12
+					    : MEDIA_BUS_FMT_SRGGB12_1X12;
+			get_mode_table(imx585, code, &mode_list, &num_modes);

drivers/media/i2c/imx585.c new file +10 -0

+		}
+		break;
+	default:
+		break;
+	}
+
 
+
+static int imx585_check_module_exists(struct imx585 *imx585)
+{
+	int ret;

drivers/media/i2c/imx585.c new file +9 -0

+	return 0;
+}
+
+static int imx585_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct imx585 *imx585;
+	const char *sync_mode;
+	int ret, i;

drivers/media/i2c/imx585.c new file +15 -0

+
+	ret = imx585_check_hwcfg(dev, imx585);
+	if (ret)
+		return ret;
+
+	imx585->regmap = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(imx585->regmap))
+		return dev_err_probe(dev, PTR_ERR(imx585->regmap), "CCI init failed\n");
+
+	imx585->xclk = devm_clk_get(dev, NULL);
+	if (IS_ERR(imx585->xclk))
+		return dev_err_probe(dev, PTR_ERR(imx585->xclk), "xclk missing\n");
+
+	imx585->xclk_freq = clk_get_rate(imx585->xclk);
+	for (i = 0; i < ARRAY_SIZE(imx585_inck_table); ++i) {

drivers/media/i2c/imx585.c new file +108 -0

+
+	ret = imx585_get_regulators(imx585);
+	if (ret)
+		return dev_err_probe(dev, ret, "regulators\n");
+
+	imx585->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+
+	/* Power on to probe the device */
+	ret = imx585_power_on(dev);
+	if (ret)
+		return ret;
+
+	ret = imx585_check_module_exists(imx585);
+	if (ret)
+		goto err_power_off;
+
+	pm_runtime_set_active(dev);
+	pm_runtime_get_noresume(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+
+	ret = imx585_init_controls(imx585);
+	if (ret)
+		goto err_pm;
+
+	imx585->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	imx585->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+	imx585->sd.internal_ops = &imx585_internal_ops;
+
+	imx585->pad.flags = MEDIA_PAD_FL_SOURCE;
+
+	ret = media_entity_pads_init(&imx585->sd.entity, 1, &imx585->pad);
+	if (ret) {
+		dev_err(dev, "entity pads init failed: %d\n", ret);
+		goto err_ctrls;
+	}
+
+	imx585->sd.state_lock = imx585->ctrl_handler.lock;
+	ret = v4l2_subdev_init_finalize(&imx585->sd);
+	if (ret) {
+		dev_err_probe(dev, ret, "subdev init\n");
+		goto err_entity;
+	}
+
+	ret = v4l2_async_register_subdev_sensor(&imx585->sd);
+	if (ret) {
+		dev_err(dev, "sensor subdev register failed: %d\n", ret);
+		goto err_entity;
+	}
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+	return 0;
+
+err_entity:
+	media_entity_cleanup(&imx585->sd.entity);
+err_ctrls:
+	imx585_free_controls(imx585);
+err_pm:
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+err_power_off:
+	imx585_power_off(dev);
+	return ret;
+}
+
+static void imx585_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx585 *imx585 = to_imx585(sd);
+
+	v4l2_async_unregister_subdev(sd);
+	v4l2_subdev_cleanup(sd);
+	media_entity_cleanup(&sd->entity);
+	imx585_free_controls(imx585);
+
+	pm_runtime_disable(imx585->clientdev);
+	if (!pm_runtime_status_suspended(imx585->clientdev))
+		imx585_power_off(imx585->clientdev);
+	pm_runtime_set_suspended(imx585->clientdev);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(imx585_pm_ops, imx585_power_off,
+				 imx585_power_on, NULL);
+
+static const struct of_device_id imx585_of_match[] = {
+	{ .compatible = "sony,imx585" },
+	{ .compatible = "sony,imx585-mono" }, /* monochrome variant */
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx585_of_match);
+
+static struct i2c_driver imx585_i2c_driver = {
+	.driver = {
+		.name  = "imx585",
+		.pm    = pm_ptr(&imx585_pm_ops),
+		.of_match_table = imx585_of_match,
+	},
+	.probe  = imx585_probe,
+	.remove = imx585_remove,
+};
+module_i2c_driver(imx585_i2c_driver);
+
+MODULE_AUTHOR("Will Whang <[email protected]>");
+MODULE_AUTHOR("Tetsuya Nomura <[email protected]>");
+MODULE_DESCRIPTION("Sony IMX585 sensor driver");
+MODULE_LICENSE("GPL");

Signature · 1 line

2.39.5

drivers/media/i2c/imx585.c new file +17 -0

+
+/* --------------------------------------------------------------------------
+ * Power / runtime PM
+ * --------------------------------------------------------------------------
+ */
+
+static int imx585_power_on(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct imx585 *imx585 = to_imx585(sd);
+	int ret;
+
+	dev_dbg(imx585->clientdev, "power_on\n");
+
+	ret = regulator_bulk_enable(IMX585_NUM_SUPPLIES, imx585->supplies);
+	if (ret) {
+		dev_err(imx585->clientdev, "Failed to enable regulators\n");

drivers/media/i2c/imx585.c new file +7 -0

+	usleep_range(IMX585_XCLR_MIN_DELAY_US,
+		     IMX585_XCLR_MIN_DELAY_US + IMX585_XCLR_DELAY_RANGE_US);
+	return 0;
+
+reg_off:
+	regulator_bulk_disable(IMX585_NUM_SUPPLIES, imx585->supplies);
+	return ret;

Quoted 17 lines · Will Whang

drivers/media/i2c/imx585.c new file +17 -0

+
+/* --------------------------------------------------------------------------
+ * Power / runtime PM
+ * --------------------------------------------------------------------------
+ */
+
+static int imx585_power_on(struct device *dev)
+{
+     struct v4l2_subdev *sd = dev_get_drvdata(dev);
+     struct imx585 *imx585 = to_imx585(sd);
+     int ret;
+
+     dev_dbg(imx585->clientdev, "power_on\n");
+
+     ret = regulator_bulk_enable(IMX585_NUM_SUPPLIES, imx585->supplies);
+     if (ret) {
+             dev_err(imx585->clientdev, "Failed to enable regulators\n");

drivers/media/i2c/imx585.c line 1414

Quoted 10 lines

drivers/media/i2c/imx585.c new file +10 -0

+             return ret;
+     }
+
+     ret = clk_prepare_enable(imx585->xclk);
+     if (ret) {
+             dev_err(imx585->clientdev, "Failed to enable clock\n");
+             goto reg_off;
+     }
+
+     gpiod_set_value_cansleep(imx585->reset_gpio, 1);

You asserted reset gpio causing it to enter reset and you call this "power on"?

Quoted 12 lines

drivers/media/i2c/imx585.c new file +12 -0

+     usleep_range(IMX585_XCLR_MIN_DELAY_US,
+                  IMX585_XCLR_MIN_DELAY_US + IMX585_XCLR_DELAY_RANGE_US);
+     return 0;
+
+reg_off:
+     regulator_bulk_disable(IMX585_NUM_SUPPLIES, imx585->supplies);
+     return ret;
+}
+
+static int imx585_power_off(struct device *dev)
+{
+     struct v4l2_subdev *sd = dev_get_drvdata(dev);

drivers/media/i2c/imx585.c line 1431

Quoted 5 lines

drivers/media/i2c/imx585.c new file +5 -0

+     struct imx585 *imx585 = to_imx585(sd);
+
+     dev_dbg(imx585->clientdev, "power_off\n");
+
+     gpiod_set_value_cansleep(imx585->reset_gpio, 0);

And here device comes up, but you call it power off? Your functions or reset gpio code are completely reversed/wrong.

Best regards,
Krzysztof

Quoted 45 lines · Krzysztof Kozlowski

Quoted 17 lines · Will Whang

drivers/media/i2c/imx585.c new file +17 -0

+
+/* --------------------------------------------------------------------------
+ * Power / runtime PM
+ * --------------------------------------------------------------------------
+ */
+
+static int imx585_power_on(struct device *dev)
+{
+     struct v4l2_subdev *sd = dev_get_drvdata(dev);
+     struct imx585 *imx585 = to_imx585(sd);
+     int ret;
+
+     dev_dbg(imx585->clientdev, "power_on\n");
+
+     ret = regulator_bulk_enable(IMX585_NUM_SUPPLIES, imx585->supplies);
+     if (ret) {
+             dev_err(imx585->clientdev, "Failed to enable regulators\n");

drivers/media/i2c/imx585.c line 1414

Quoted 10 lines

drivers/media/i2c/imx585.c new file +10 -0

+             return ret;
+     }
+
+     ret = clk_prepare_enable(imx585->xclk);
+     if (ret) {
+             dev_err(imx585->clientdev, "Failed to enable clock\n");
+             goto reg_off;
+     }
+
+     gpiod_set_value_cansleep(imx585->reset_gpio, 1);

You asserted reset gpio causing it to enter reset and you call this "power on"?

Quoted 12 lines

drivers/media/i2c/imx585.c new file +12 -0

+     usleep_range(IMX585_XCLR_MIN_DELAY_US,
+                  IMX585_XCLR_MIN_DELAY_US + IMX585_XCLR_DELAY_RANGE_US);
+     return 0;
+
+reg_off:
+     regulator_bulk_disable(IMX585_NUM_SUPPLIES, imx585->supplies);
+     return ret;
+}
+
+static int imx585_power_off(struct device *dev)
+{
+     struct v4l2_subdev *sd = dev_get_drvdata(dev);

Quoted 8 lines

drivers/media/i2c/imx585.c line 1431

Quoted 5 lines

drivers/media/i2c/imx585.c new file +5 -0

+     struct imx585 *imx585 = to_imx585(sd);
+
+     dev_dbg(imx585->clientdev, "power_off\n");
+
+     gpiod_set_value_cansleep(imx585->reset_gpio, 0);

And here device comes up, but you call it power off? Your functions or reset gpio code are completely reversed/wrong.

Reset pin High -> Run normally
Reset pin Low -> Reset state

See drivers/media/i2c/imx219.c with the same logic:

static int imx219_power_on(struct device *dev) {
struct v4l2_subdev *sd = dev_get_drvdata(dev); struct imx219 *imx219 = to_imx219(sd); int ret;

ret = regulator_bulk_enable(IMX219_NUM_SUPPLIES,
imx219->supplies);
if (ret) {
dev_err(dev, "%s: failed to enable regulators\n", __func__);
return ret;
}

ret = clk_prepare_enable(imx219->xclk); if (ret) {
dev_err(dev, "%s: failed to enable clock\n", __func__);
goto reg_off;
}

gpiod_set_value_cansleep(imx219->reset_gpio, 1); usleep_range(IMX219_XCLR_MIN_DELAY_US,

    IMX219_XCLR_MIN_DELAY_US + IMX219_XCLR_DELAY_RANGE_US);

return 0;

reg_off:
regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);

return ret;
}

static int imx219_power_off(struct device *dev) {
struct v4l2_subdev *sd = dev_get_drvdata(dev); struct imx219 *imx219 = to_imx219(sd);

gpiod_set_value_cansleep(imx219->reset_gpio, 0); regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies); clk_disable_unprepare(imx219->xclk);

return 0;
}

I really don't understand why this is a problem, it is up to the chip designer to decide
what to do with reset behavior and not the reviewers.

The new IMX585 V4L2 sub-device driver introduces several driver-specific controls for configuring Clear-HDR blending, gradation compression thresholds, and HCG enabling. This patch adds an rst document under Documentation/userspace-api/media/drivers/ that details each control, allowed values, and their effects.

Signed-off-by Will Whang <[email protected]>

---
 .../userspace-api/media/drivers/imx585.rst    | 122 ++++++++++++++++++
 .../userspace-api/media/drivers/index.rst     |   1 +
 MAINTAINERS                                   |   1 +
 3 files changed, 124 insertions(+)
 create mode 100644 Documentation/userspace-api/media/drivers/imx585.rst

Documentation/userspace-api/media/drivers/imx585.rst new file +101 -0

new file mode 100644
index 000000000..9f7c16f30
@@ -0,0 +1,122 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+Sony IMX585 driver
+==================
+
+The IMX585 image-sensor driver provides the following *driver-specific*
+V4L2 controls.  They are visible only when the IMX585 driver is loaded
+and sit in the sensor-private control class.
+
+HDR data blending
+-----------------
+
+``V4L2_CID_IMX585_HDR_DATASEL_TH``  (``U16[2]``)
+    Lower/upper **thresholds** (0 – 4095) that decide which exposure is
+    chosen—or blended—for each pixel in Clear-HDR mode.
+
+``V4L2_CID_IMX585_HDR_DATASEL_BK``  (menu)
+    **Blending ratio** between the long-gain (LG) and
+    high-gain (HG) read-outs.
+
+    .. flat-table::
+       :stub-columns: 0
+       :widths: 1 5
+
+       * - ``0``
+         - HG ½, LG ½
+       * - ``1``
+         - HG ¾, LG ¼
+       * - ``2``     # duplicate ratio present in the datasheet
+         - HG ½, LG ½
+       * - ``3``
+         - HG ⅞, LG ⅛
+       * - ``4``
+         - HG 15⁄16, LG 1⁄16
+       * - ``5``     # second 50/50 entry as documented
+         - **2ⁿᵈ** HG ½, LG ½
+       * - ``6``
+         - HG 1⁄16, LG 15⁄16
+       * - ``7``
+         - HG ⅛, LG ⅞
+       * - ``8``
+         - HG ¼, LG ¾
+
+Gradation compression
+---------------------
+
+``V4L2_CID_IMX585_HDR_GRAD_TH``  (``U32[2]``)
+    17-bit **break-points** (0 – 0x1ffff) that shape the 16-bit
+    gradation-compression curve.
+
+``V4L2_CID_IMX585_HDR_GRAD_COMP_L``  (menu)
+    See V4L2_CID_IMX585_HDR_GRAD_COMP_H
+
+``V4L2_CID_IMX585_HDR_GRAD_COMP_H``  (menu)
+    **Compression ratios** below the first break-point and between the
+    two break-points, respectively.
+
+    .. flat-table::
+        :stub-columns: 0
+        :widths: 1 4
+
+        * - ``0``
+          - 1 : 1
+        * - ``1``
+          - 1 : 2
+        * - ``2``
+          - 1 : 4   *(default for COMP_L)*
+        * - ``3``
+          - 1 : 8
+        * - ``4``
+          - 1 : 16
+        * - ``5``
+          - 1 : 32
+        * - ``6``
+          - 1 : 64  *(default for COMP_H)*
+        * - ``7``
+          - 1 : 128
+        * - ``8``
+          - 1 : 256
+        * - ``9``
+          - 1 : 512
+        * - ``10``
+          - 1 : 1024
+        * - ``11``
+          - 1 : 2048
+
+Gain settings
+-------------
+
+``V4L2_CID_IMX585_HDR_GAIN``  (menu)
+    **Additional gain** (in dB) applied to the high-gain path when
+    Clear-HDR is active.
+
+    .. flat-table::
+        :stub-columns: 0
+        :widths: 1 3
+
+        * - ``0``
+          - +0 dB
+        * - ``1``
+          - +6 dB

Documentation/userspace-api/media/drivers/imx585.rst new file +8 -0

+
+Notes
+-----
+
+* Controls are writable while streaming; changes take effect from the
+  next frame.
+* HDR-specific controls are hidden when HDR is disabled.
+* Inter-control dependencies are enforced by the driver.

Documentation/userspace-api/media/drivers/index.rst +1 -0

index d706cb47b..87912acfb 100644
@@ -32,6 +32,7 @@ For more details see the file COPYING in the source distribution of Linux.
        cx2341x-uapi
        dw100
        imx-uapi
+       imx585
        max2175
        npcm-video
        omap3isp-uapi

MAINTAINERS +1 -1

index 175f5236a..42e32b6ba 100644
@@ -23183,6 +23183,7 @@ M:      Will Whang <[email protected]>
 L:     [email protected]
 S:     Maintained
 F:     Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
+F:     Documentation/userspace-api/media/drivers/imx585.rst
 F:     drivers/media/i2c/imx585.c
 F:     include/uapi/linux/imx585.h
 
--

2.39.5

Hi Will

Thanks for the patches.

Quoted 122 lines · Will Whang

The new IMX585 V4L2 sub-device driver introduces several driver-specific controls for configuring Clear-HDR blending, gradation compression thresholds, and HCG enabling. This patch adds an rst document under Documentation/userspace-api/media/drivers/ that details each control, allowed values, and their effects.

Signed-off-by Will Whang <[email protected]>

---
 .../userspace-api/media/drivers/imx585.rst    | 122 ++++++++++++++++++
 .../userspace-api/media/drivers/index.rst     |   1 +
 MAINTAINERS                                   |   1 +
 3 files changed, 124 insertions(+)
 create mode 100644 Documentation/userspace-api/media/drivers/imx585.rst

Documentation/userspace-api/media/drivers/imx585.rst new file +101 -0

new file mode 100644
index 000000000..9f7c16f30
@@ -0,0 +1,122 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+Sony IMX585 driver
+==================
+
+The IMX585 image-sensor driver provides the following *driver-specific*
+V4L2 controls.  They are visible only when the IMX585 driver is loaded
+and sit in the sensor-private control class.
+
+HDR data blending
+-----------------
+
+``V4L2_CID_IMX585_HDR_DATASEL_TH``  (``U16[2]``)
+    Lower/upper **thresholds** (0 – 4095) that decide which exposure is
+    chosen—or blended—for each pixel in Clear-HDR mode.
+
+``V4L2_CID_IMX585_HDR_DATASEL_BK``  (menu)
+    **Blending ratio** between the long-gain (LG) and
+    high-gain (HG) read-outs.
+
+    .. flat-table::
+       :stub-columns: 0
+       :widths: 1 5
+
+       * - ``0``
+         - HG ½, LG ½
+       * - ``1``
+         - HG ¾, LG ¼
+       * - ``2``     # duplicate ratio present in the datasheet
+         - HG ½, LG ½
+       * - ``3``
+         - HG ⅞, LG ⅛
+       * - ``4``
+         - HG 15⁄16, LG 1⁄16
+       * - ``5``     # second 50/50 entry as documented
+         - **2ⁿᵈ** HG ½, LG ½
+       * - ``6``
+         - HG 1⁄16, LG 15⁄16
+       * - ``7``
+         - HG ⅛, LG ⅞
+       * - ``8``
+         - HG ¼, LG ¾
+
+Gradation compression
+---------------------
+
+``V4L2_CID_IMX585_HDR_GRAD_TH``  (``U32[2]``)
+    17-bit **break-points** (0 – 0x1ffff) that shape the 16-bit
+    gradation-compression curve.
+
+``V4L2_CID_IMX585_HDR_GRAD_COMP_L``  (menu)
+    See V4L2_CID_IMX585_HDR_GRAD_COMP_H
+
+``V4L2_CID_IMX585_HDR_GRAD_COMP_H``  (menu)
+    **Compression ratios** below the first break-point and between the
+    two break-points, respectively.
+
+    .. flat-table::
+        :stub-columns: 0
+        :widths: 1 4
+
+        * - ``0``
+          - 1 : 1
+        * - ``1``
+          - 1 : 2
+        * - ``2``
+          - 1 : 4   *(default for COMP_L)*
+        * - ``3``
+          - 1 : 8
+        * - ``4``
+          - 1 : 16
+        * - ``5``
+          - 1 : 32
+        * - ``6``
+          - 1 : 64  *(default for COMP_H)*
+        * - ``7``
+          - 1 : 128
+        * - ``8``
+          - 1 : 256
+        * - ``9``
+          - 1 : 512
+        * - ``10``
+          - 1 : 1024
+        * - ``11``
+          - 1 : 2048
+
+Gain settings
+-------------
+
+``V4L2_CID_IMX585_HDR_GAIN``  (menu)
+    **Additional gain** (in dB) applied to the high-gain path when
+    Clear-HDR is active.
+
+    .. flat-table::
+        :stub-columns: 0
+        :widths: 1 3
+
+        * - ``0``
+          - +0 dB
+        * - ``1``
+          - +6 dB

Documentation/userspace-api/media/drivers/imx585.rst line 111

Quoted 10 lines

Documentation/userspace-api/media/drivers/imx585.rst new file +10 -0

+        * - ``2``
+          - +12 dB *(default)*
+        * - ``3``
+          - +18 dB
+        * - ``4``
+          - +24 dB
+        * - ``5``
+          - +29.1 dB
+
+``V4L2_CID_IMX585_HCG_GAIN``  (boolean)

HCG stands for High Conversion Gain, so we've got Gain repeated in the name.

Spell it out as V4L2_CID_IMX585_HIGH_CONV_GAIN, or call it CONVERSION_GAIN and use an enum control?

Documentation/userspace-api/media/drivers/imx585.rst line 114

Quoted 3 lines

Documentation/userspace-api/media/drivers/imx585.rst new file +3 -0

+    Toggle **High-Conversion-Gain** mode.
+
+    *0 = LCG (default), 1 = HCG.*

An HCG / LCG control would also be applicable for IMX290 [1], so it would be nice if this could be a generic control instead of imx585 specific.

I never got a good description as to the benefit HCG was meant to give. The datasheet for IMX290 says the conversion efficiency ratio between HCG and LCG is 2, but not why that is any better than adding 6dB of analog gain.

Sony's website [2] states
"Sony’s Super High Conversion Gain technology is designed to amplify electrical signals immediately after the conversion from photons, when the noise levels are relatively low. In this way, it reduces the overall noise after amplification. As a result, lower-noise images, compared to conventional technology, can be captured even in a low-illuminance environment. Lower noise levels in images also help to enhance the accuracy in visual or AI-assisted image recognition." From that one would presume you'd always want it on (lower noise = good), unless needing the minimum exposure time and the image was already over-exposed.
I'm guessing you have no additional information based on your description text.

Dave

[1] Also IMX327, IMX462, and IMX662 which are in the same family, IMX678 (ratio of 2.6), and quite probably most of the Sony Starvis or Starvis 2 ranges.
[2] https://www.sony-semicon.com/en/technology/security/index.html

Quoted 37 lines

Documentation/userspace-api/media/drivers/imx585.rst new file +8 -0

+
+Notes
+-----
+
+* Controls are writable while streaming; changes take effect from the
+  next frame.
+* HDR-specific controls are hidden when HDR is disabled.
+* Inter-control dependencies are enforced by the driver.

Documentation/userspace-api/media/drivers/index.rst +1 -0

index d706cb47b..87912acfb 100644
@@ -32,6 +32,7 @@ For more details see the file COPYING in the source distribution of Linux.
        cx2341x-uapi
        dw100
        imx-uapi
+       imx585
        max2175
        npcm-video
        omap3isp-uapi

MAINTAINERS +1 -1

index 175f5236a..42e32b6ba 100644
@@ -23183,6 +23183,7 @@ M:      Will Whang <[email protected]>
 L:     [email protected]
 S:     Maintained
 F:     Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
+F:     Documentation/userspace-api/media/drivers/imx585.rst
 F:     drivers/media/i2c/imx585.c
 F:     include/uapi/linux/imx585.h
 
--

2.39.5

Quoted 147 lines · Dave Stevenson

Quoted 122 lines · Will Whang

The new IMX585 V4L2 sub-device driver introduces several driver-specific controls for configuring Clear-HDR blending, gradation compression thresholds, and HCG enabling. This patch adds an rst document under Documentation/userspace-api/media/drivers/ that details each control, allowed values, and their effects.

Signed-off-by Will Whang <[email protected]>

---
 .../userspace-api/media/drivers/imx585.rst    | 122 ++++++++++++++++++
 .../userspace-api/media/drivers/index.rst     |   1 +
 MAINTAINERS                                   |   1 +
 3 files changed, 124 insertions(+)
 create mode 100644 Documentation/userspace-api/media/drivers/imx585.rst

Documentation/userspace-api/media/drivers/imx585.rst new file +101 -0

new file mode 100644
index 000000000..9f7c16f30
@@ -0,0 +1,122 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+Sony IMX585 driver
+==================
+
+The IMX585 image-sensor driver provides the following *driver-specific*
+V4L2 controls.  They are visible only when the IMX585 driver is loaded
+and sit in the sensor-private control class.
+
+HDR data blending
+-----------------
+
+``V4L2_CID_IMX585_HDR_DATASEL_TH``  (``U16[2]``)
+    Lower/upper **thresholds** (0 – 4095) that decide which exposure is
+    chosen—or blended—for each pixel in Clear-HDR mode.
+
+``V4L2_CID_IMX585_HDR_DATASEL_BK``  (menu)
+    **Blending ratio** between the long-gain (LG) and
+    high-gain (HG) read-outs.
+
+    .. flat-table::
+       :stub-columns: 0
+       :widths: 1 5
+
+       * - ``0``
+         - HG ½, LG ½
+       * - ``1``
+         - HG ¾, LG ¼
+       * - ``2``     # duplicate ratio present in the datasheet
+         - HG ½, LG ½
+       * - ``3``
+         - HG ⅞, LG ⅛
+       * - ``4``
+         - HG 15⁄16, LG 1⁄16
+       * - ``5``     # second 50/50 entry as documented
+         - **2ⁿᵈ** HG ½, LG ½
+       * - ``6``
+         - HG 1⁄16, LG 15⁄16
+       * - ``7``
+         - HG ⅛, LG ⅞
+       * - ``8``
+         - HG ¼, LG ¾
+
+Gradation compression
+---------------------
+
+``V4L2_CID_IMX585_HDR_GRAD_TH``  (``U32[2]``)
+    17-bit **break-points** (0 – 0x1ffff) that shape the 16-bit
+    gradation-compression curve.
+
+``V4L2_CID_IMX585_HDR_GRAD_COMP_L``  (menu)
+    See V4L2_CID_IMX585_HDR_GRAD_COMP_H
+
+``V4L2_CID_IMX585_HDR_GRAD_COMP_H``  (menu)
+    **Compression ratios** below the first break-point and between the
+    two break-points, respectively.
+
+    .. flat-table::
+        :stub-columns: 0
+        :widths: 1 4
+
+        * - ``0``
+          - 1 : 1
+        * - ``1``
+          - 1 : 2
+        * - ``2``
+          - 1 : 4   *(default for COMP_L)*
+        * - ``3``
+          - 1 : 8
+        * - ``4``
+          - 1 : 16
+        * - ``5``
+          - 1 : 32
+        * - ``6``
+          - 1 : 64  *(default for COMP_H)*
+        * - ``7``
+          - 1 : 128
+        * - ``8``
+          - 1 : 256
+        * - ``9``
+          - 1 : 512
+        * - ``10``
+          - 1 : 1024
+        * - ``11``
+          - 1 : 2048
+
+Gain settings
+-------------
+
+``V4L2_CID_IMX585_HDR_GAIN``  (menu)
+    **Additional gain** (in dB) applied to the high-gain path when
+    Clear-HDR is active.
+
+    .. flat-table::
+        :stub-columns: 0
+        :widths: 1 3
+
+        * - ``0``
+          - +0 dB
+        * - ``1``
+          - +6 dB

Documentation/userspace-api/media/drivers/imx585.rst line 111

Quoted 10 lines

Documentation/userspace-api/media/drivers/imx585.rst new file +10 -0

+        * - ``2``
+          - +12 dB *(default)*
+        * - ``3``
+          - +18 dB
+        * - ``4``
+          - +24 dB
+        * - ``5``
+          - +29.1 dB
+
+``V4L2_CID_IMX585_HCG_GAIN``  (boolean)

HCG stands for High Conversion Gain, so we've got Gain repeated in the name.

Spell it out as V4L2_CID_IMX585_HIGH_CONV_GAIN, or call it CONVERSION_GAIN and use an enum control?

Documentation/userspace-api/media/drivers/imx585.rst line 114

Quoted 3 lines

Documentation/userspace-api/media/drivers/imx585.rst new file +3 -0

+    Toggle **High-Conversion-Gain** mode.
+
+    *0 = LCG (default), 1 = HCG.*

An HCG / LCG control would also be applicable for IMX290 [1], so it would be nice if this could be a generic control instead of imx585 specific.

Quoted 4 lines

I never got a good description as to the benefit HCG was meant to give. The datasheet for IMX290 says the conversion efficiency ratio between HCG and LCG is 2, but not why that is any better than adding 6dB of analog gain.

What I've learned is that HCG is actually a 2nd stage amplifier, so instead of using one for high gain, you can split it into two lower gain stages that lower the noise.

This is also why ClearHDR and HCG have the conflict, because it is basically sampling after the 1st gain stage and 2nd gain stage as High/Low gain images.

I have thought about making it more generic because IMX662 and IMX678 are going to be the next patch once this one passes that also has this function (the two are basically stripped down versions from IMX585 that the driver can be adapted to easily). I intended for the upcoming IMX662/IMX678 driver to use IMX585's V4L2 HCG control also.

But given that I'm new to Linux kernel developments, or more specifically, V4L2, I really don't have an idea how to do so in a way the patch will be accepted.
Or I can make this more generic name to replace IMX585 for STARVIS2, for example: V4L2_CID_STARVIS2_HIGH_CONV_GAIN

Quoted 13 lines

Sony's website [2] states
"Sony’s Super High Conversion Gain technology is designed to amplify electrical signals immediately after the conversion from photons, when the noise levels are relatively low. In this way, it reduces the overall noise after amplification. As a result, lower-noise images, compared to conventional technology, can be captured even in a low-illuminance environment. Lower noise levels in images also help to enhance the accuracy in visual or AI-assisted image recognition." From that one would presume you'd always want it on (lower noise = good), unless needing the minimum exposure time and the image was already over-exposed.
I'm guessing you have no additional information based on your description text.

Quoted 6 lines

Dave

[1] Also IMX327, IMX462, and IMX662 which are in the same family, IMX678 (ratio of 2.6), and quite probably most of the Sony Starvis or Starvis 2 ranges.
[2] https://www.sony-semicon.com/en/technology/security/index.html

Yeah I think Starvis 2 series all have this capability.

Quoted 32 lines

Quoted 32 lines

Documentation/userspace-api/media/drivers/imx585.rst new file +8 -0

+
+Notes
+-----
+
+* Controls are writable while streaming; changes take effect from the
+  next frame.
+* HDR-specific controls are hidden when HDR is disabled.
+* Inter-control dependencies are enforced by the driver.

Documentation/userspace-api/media/drivers/index.rst +1 -0

index d706cb47b..87912acfb 100644
@@ -32,6 +32,7 @@ For more details see the file COPYING in the source distribution of Linux.
        cx2341x-uapi
        dw100
        imx-uapi
+       imx585
        max2175
        npcm-video
        omap3isp-uapi

MAINTAINERS +1 -0

index 175f5236a..42e32b6ba 100644
@@ -23183,6 +23183,7 @@ M:      Will Whang <[email protected]>
 L:     [email protected]
 S:     Maintained
 F:     Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
+F:     Documentation/userspace-api/media/drivers/imx585.rst
 F:     drivers/media/i2c/imx585.c
 F:     include/uapi/linux/imx585.h
 

Hi Will,

Quoted 148 lines · Will Whang

Quoted 147 lines · Dave Stevenson

Quoted 122 lines · Will Whang

The new IMX585 V4L2 sub-device driver introduces several driver-specific controls for configuring Clear-HDR blending, gradation compression thresholds, and HCG enabling. This patch adds an rst document under Documentation/userspace-api/media/drivers/ that details each control, allowed values, and their effects.

Signed-off-by Will Whang <[email protected]>

---
 .../userspace-api/media/drivers/imx585.rst    | 122 ++++++++++++++++++
 .../userspace-api/media/drivers/index.rst     |   1 +
 MAINTAINERS                                   |   1 +
 3 files changed, 124 insertions(+)
 create mode 100644 Documentation/userspace-api/media/drivers/imx585.rst

Documentation/userspace-api/media/drivers/imx585.rst new file +101 -0

new file mode 100644
index 000000000..9f7c16f30
@@ -0,0 +1,122 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+Sony IMX585 driver
+==================
+
+The IMX585 image-sensor driver provides the following *driver-specific*
+V4L2 controls.  They are visible only when the IMX585 driver is loaded
+and sit in the sensor-private control class.
+
+HDR data blending
+-----------------
+
+``V4L2_CID_IMX585_HDR_DATASEL_TH``  (``U16[2]``)
+    Lower/upper **thresholds** (0 – 4095) that decide which exposure is
+    chosen—or blended—for each pixel in Clear-HDR mode.
+
+``V4L2_CID_IMX585_HDR_DATASEL_BK``  (menu)
+    **Blending ratio** between the long-gain (LG) and
+    high-gain (HG) read-outs.
+
+    .. flat-table::
+       :stub-columns: 0
+       :widths: 1 5
+
+       * - ``0``
+         - HG ½, LG ½
+       * - ``1``
+         - HG ¾, LG ¼
+       * - ``2``     # duplicate ratio present in the datasheet
+         - HG ½, LG ½
+       * - ``3``
+         - HG ⅞, LG ⅛
+       * - ``4``
+         - HG 15⁄16, LG 1⁄16
+       * - ``5``     # second 50/50 entry as documented
+         - **2ⁿᵈ** HG ½, LG ½
+       * - ``6``
+         - HG 1⁄16, LG 15⁄16
+       * - ``7``
+         - HG ⅛, LG ⅞
+       * - ``8``
+         - HG ¼, LG ¾
+
+Gradation compression
+---------------------
+
+``V4L2_CID_IMX585_HDR_GRAD_TH``  (``U32[2]``)
+    17-bit **break-points** (0 – 0x1ffff) that shape the 16-bit
+    gradation-compression curve.
+
+``V4L2_CID_IMX585_HDR_GRAD_COMP_L``  (menu)
+    See V4L2_CID_IMX585_HDR_GRAD_COMP_H
+
+``V4L2_CID_IMX585_HDR_GRAD_COMP_H``  (menu)
+    **Compression ratios** below the first break-point and between the
+    two break-points, respectively.
+
+    .. flat-table::
+        :stub-columns: 0
+        :widths: 1 4
+
+        * - ``0``
+          - 1 : 1
+        * - ``1``
+          - 1 : 2
+        * - ``2``
+          - 1 : 4   *(default for COMP_L)*
+        * - ``3``
+          - 1 : 8
+        * - ``4``
+          - 1 : 16
+        * - ``5``
+          - 1 : 32
+        * - ``6``
+          - 1 : 64  *(default for COMP_H)*
+        * - ``7``
+          - 1 : 128
+        * - ``8``
+          - 1 : 256
+        * - ``9``
+          - 1 : 512
+        * - ``10``
+          - 1 : 1024
+        * - ``11``
+          - 1 : 2048
+
+Gain settings
+-------------
+
+``V4L2_CID_IMX585_HDR_GAIN``  (menu)
+    **Additional gain** (in dB) applied to the high-gain path when
+    Clear-HDR is active.
+
+    .. flat-table::
+        :stub-columns: 0
+        :widths: 1 3
+
+        * - ``0``
+          - +0 dB
+        * - ``1``
+          - +6 dB

Documentation/userspace-api/media/drivers/imx585.rst line 111

Quoted 10 lines

Documentation/userspace-api/media/drivers/imx585.rst new file +10 -0

+        * - ``2``
+          - +12 dB *(default)*
+        * - ``3``
+          - +18 dB
+        * - ``4``
+          - +24 dB
+        * - ``5``
+          - +29.1 dB
+
+``V4L2_CID_IMX585_HCG_GAIN``  (boolean)

HCG stands for High Conversion Gain, so we've got Gain repeated in the name.

Spell it out as V4L2_CID_IMX585_HIGH_CONV_GAIN, or call it CONVERSION_GAIN and use an enum control?

Documentation/userspace-api/media/drivers/imx585.rst line 114

Quoted 3 lines

Documentation/userspace-api/media/drivers/imx585.rst new file +3 -0

+    Toggle **High-Conversion-Gain** mode.
+
+    *0 = LCG (default), 1 = HCG.*

An HCG / LCG control would also be applicable for IMX290 [1], so it would be nice if this could be a generic control instead of imx585 specific.

Quoted 8 lines

Quoted 4 lines

I never got a good description as to the benefit HCG was meant to give. The datasheet for IMX290 says the conversion efficiency ratio between HCG and LCG is 2, but not why that is any better than adding 6dB of analog gain.

What I've learned is that HCG is actually a 2nd stage amplifier, so instead of using one for high gain, you can split it into two lower gain stages that lower the noise.

DCG as a term is a bit confusing. It was initially used to describe an inter-frame HDR technique, where two different conversion gains are applied to separate frames by switching on and off an extra capacitor on the pixel's floating diffusion node. I don't know how it evolved from there, but just adding a second amplifier doesn't seem to be a very effective way to lower noise.

If anyone does some research on the topic and finds clear information, please share them.

Quoted 10 lines

This is also why ClearHDR and HCG have the conflict, because it is basically sampling after the 1st gain stage and 2nd gain stage as High/Low gain images.

I have thought about making it more generic because IMX662 and IMX678 are going to be the next patch once this one passes that also has this function (the two are basically stripped down versions from IMX585 that the driver can be adapted to easily). I intended for the upcoming IMX662/IMX678 driver to use IMX585's V4L2 HCG control also.

Quoted 5 lines

But given that I'm new to Linux kernel developments, or more specifically, V4L2, I really don't have an idea how to do so in a way the patch will be accepted.
Or I can make this more generic name to replace IMX585 for STARVIS2, for example: V4L2_CID_STARVIS2_HIGH_CONV_GAIN

We should really try to standardize V4L2 controls for HDR support in
sensors. See https://lore.kernel.org/linux-media/[email protected]
for an attempt at doing so (for some of the controls at least). Could
you share your feedback in that mail thread ?

The proposal doesn't address the sensor-side blending controls. For those, I recommend considering how they should be handled from userspace. We don't want to have per-sensor code there if we can avoid it, and that should drive the API design.

As for what you call gradation above, that's not strictly speaking limited to HDR, right ? If my understanding is right, this is about applying a compression curve to lower the bandwidth by lowering the number of bits per pixel while preserving more dynamic range in the lower and middle parts of the pixel value range. Is that correct ? If so, the term "companding" is also often used for this feature. I think we have three needs:

- Enable/disable companding (which includes configuring the bit depth of the output format)

- Obtaining the companding curve applied by the sensor (as the host will have to apply the inverse expansion curve)

- Optionally, modifying the companing curve.

I'd like standard controls for those.

Quoted 46 lines

Quoted 13 lines

Sony's website [2] states
"Sony’s Super High Conversion Gain technology is designed to amplify electrical signals immediately after the conversion from photons, when the noise levels are relatively low. In this way, it reduces the overall noise after amplification. As a result, lower-noise images, compared to conventional technology, can be captured even in a low-illuminance environment. Lower noise levels in images also help to enhance the accuracy in visual or AI-assisted image recognition." From that one would presume you'd always want it on (lower noise = good), unless needing the minimum exposure time and the image was already over-exposed.
I'm guessing you have no additional information based on your description text.

Quoted 6 lines

Dave

[1] Also IMX327, IMX462, and IMX662 which are in the same family, IMX678 (ratio of 2.6), and quite probably most of the Sony Starvis or Starvis 2 ranges.
[2] https://www.sony-semicon.com/en/technology/security/index.html

Yeah I think Starvis 2 series all have this capability.

Quoted 24 lines

Quoted 24 lines

Documentation/userspace-api/media/drivers/imx585.rst new file +8 -0

+
+Notes
+-----
+
+* Controls are writable while streaming; changes take effect from the
+  next frame.
+* HDR-specific controls are hidden when HDR is disabled.
+* Inter-control dependencies are enforced by the driver.

Documentation/userspace-api/media/drivers/index.rst +1 -0

index d706cb47b..87912acfb 100644
@@ -32,6 +32,7 @@ For more details see the file COPYING in the source distribution of Linux.
        cx2341x-uapi
        dw100
        imx-uapi
+       imx585
        max2175
        npcm-video
        omap3isp-uapi

MAINTAINERS +0 -0

index 175f5236a..42e32b6ba 100644

Quoted 8 lines

Quoted 8 lines

MAINTAINERS M: Will Whang <[email protected]> · line 23189

Quoted 8 lines

MAINTAINERS +1 -0

@@ -23183,6 +23183,7 @@ M:      Will Whang <[email protected]>
 L:     [email protected]
 S:     Maintained
 F:     Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
+F:     Documentation/userspace-api/media/drivers/imx585.rst
 F:     drivers/media/i2c/imx585.c
 F:     include/uapi/linux/imx585.h
 

--
Regards,

Laurent Pinchart