[PATCH v2 0/4] media: Add Sony IMX585 image sensor support

[PATCH v2 0/4] media: Add Sony IMX585 image sensor support

[Will Whang]

Hi all,

This is v2 of the IMX585 series. It adds ClearHDR controls, fixes mono
format handling, and ensures HCG can’t be enabled while HDR is active.

Changes in v2:
  - Move to V4L2 active State API.
  - Shift the I2C registers read/write to v4l2-cci.
  - Remove IR Filter switch support.
  - Various log/trace noise trimmed or moved to debug print.
  - Lock/ignore HCG when HDR is enabled; mark control inactive.
  - Using compatible string for mono sensor.
  - dt-bindings: drop redundant maxItems/minItems/names,
    add "sony,imx585-mono", fix quoted string lint.

Features
==========
  * 4-lane or 2-lane MIPI-CSI-2 up to 2079 Mbps/lane
  * 4 K @ 60 fps 12-bit linear, 4 K @ 30 fps 16-bit Clear-HDR,
    4 K @ 30 fps 12-bit gradient compression Clear-HDR
    1080p binning mode, mono hardware variant, HCG support.
  * New Sensor Dependent V4L2 controls for
    HDR blending, grad-compression, HCG enable and Enable ClearHDR.
  * Blacklevel adjustments through V4L2_CID_BRIGHTNESS.
  * Multi Camera synchronization mode support.

Testing
==========
  - Platform: Raspberry Pi 5, 4 lanes
  - Formats: Y12/Y16 (mono), SRGGB12/SRGGB16 (color)
  - Verified HDR on/off toggling updates ranges (exposure/gain/HMAX/VMAX)
  - HCG is inactive and ignored when HDR=1; active in normal mode

Series layout
=============
  1. **dt-bindings: media: Add Sony IMX585 CMOS image sensor**
  2. **media: uapi: Add custom IMX585 control IDs**
  3. **media: i2c: imx585: Add Sony IMX585 image-sensor driver**
  4. **media: docs: Add userspace-API guide for the IMX585 driver**

Link to v1: https://lore.kernel.org/linux-media/20250702063836.3984-1-will@willwhang.com/

Thanks for reviewing!

Signed-off-by: Will Whang <will@willwhang.com>

Will Whang (4):
  dt-bindings: media: Add Sony IMX585 CMOS image sensor
  media: uapi: Add custom IMX585 control IDs
  media: i2c: imx585: Add Sony IMX585 image-sensor driver
  media: docs: Add userspace-API guide for the IMX585 driver

 .../bindings/media/i2c/sony,imx585.yaml       |  116 ++
 .../userspace-api/media/drivers/imx585.rst    |  122 ++
 .../userspace-api/media/drivers/index.rst     |    1 +
 MAINTAINERS                                   |    9 +
 drivers/media/i2c/Kconfig                     |    9 +
 drivers/media/i2c/Makefile                    |    1 +
 drivers/media/i2c/imx585.c                    | 1768 +++++++++++++++++
 include/uapi/linux/imx585.h                   |   20 +
 include/uapi/linux/v4l2-controls.h            |    6 +
 9 files changed, 2052 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
 create mode 100644 Documentation/userspace-api/media/drivers/imx585.rst
 create mode 100644 drivers/media/i2c/imx585.c
 create mode 100644 include/uapi/linux/imx585.h

-- 
2.39.5

[PATCH v2 1/4] dt-bindings: media: Add Sony IMX585 CMOS image sensor

[Will Whang]

Document the devicetree binding for the Sony IMX585.  The schema
covers the CSI-2 data-lanes, the optional 'mono-mode' flag,
and the internal-sync properties used by the driver.

Signed-off-by: Will Whang <will@willwhang.com>
---
 .../bindings/media/i2c/sony,imx585.yaml       | 116 ++++++++++++++++++
 MAINTAINERS                                   |   6 +
 2 files changed, 122 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
new file mode 100644
index 000000000..020b7cfb9
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
@@ -0,0 +1,116 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/sony,imx585.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sony IMX585 CMOS image sensor
+
+maintainers:
+  - Will Whang <will@willwhang.com>
+
+description:
+  IMX585 sensor is a Sony CMOS sensor with 4K and FHD outputs.
+
+properties:
+  compatible:
+    enum:
+      - sony,imx585
+      - sony,imx585-mono
+
+  reg:
+    maxItems: 1
+
+  assigned-clocks: true
+  assigned-clock-parents: true
+  assigned-clock-rates: true
+
+  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
+    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 ]
+
+  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:
+                  - const: 1
+                  - const: 2
+                  - const: 3
+                  - const: 4
+
+          link-frequencies: true
+
+        required:
+          - data-lanes
+          - link-frequencies
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - port
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        imx585@1a {
+            compatible = "sony,imx585";
+            reg = <0x1a>;
+            clocks = <&imx585_clk>;
+
+            assigned-clocks = <&imx585_clk>;
+            assigned-clock-rates = <24000000>;
+
+            vana-supply = <&camera_vadd_3v3>;
+            vdig-supply = <&camera_vdd1_1v8>;
+            vddl-supply = <&camera_vdd2_1v1>;
+
+            port {
+                imx585: endpoint {
+                    remote-endpoint = <&cam>;
+                    data-lanes = <1 2 3 4>;
+                    link-frequencies = /bits/ 64 <720000000>;
+                };
+            };
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 24c557ee0..ef04ee4ec 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23178,6 +23178,12 @@ T:	git git://linuxtv.org/media.git
 F:	Documentation/devicetree/bindings/media/i2c/sony,imx415.yaml
 F:	drivers/media/i2c/imx415.c
 
+SONY IMX585 SENSOR DRIVER
+M:	Will Whang <will@willwhang.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
+
 SONY MEMORYSTICK SUBSYSTEM
 M:	Maxim Levitsky <maximlevitsky@gmail.com>
 M:	Alex Dubov <oakad@yahoo.com>
-- 
2.39.5

Re: [PATCH v2 1/4] dt-bindings: media: Add Sony IMX585 CMOS image sensor

[Krzysztof Kozlowski]

On Sun, Aug 10, 2025 at 11:09:18PM +0100, Will Whang wrote:
> +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).

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

Drop all three.

> +
> +  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
> +    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.

> +
> +  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:
> +                  - const: 1
> +                  - const: 2
> +                  - const: 3
> +                  - const: 4
> +
> +          link-frequencies: true

Drop

> +
> +        required:
> +          - data-lanes
> +          - link-frequencies
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - port
> +
> +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

Re: [PATCH v2 1/4] dt-bindings: media: Add Sony IMX585 CMOS image sensor

[Will Whang]

Hi Krzysztof,
Reply inline.
Thanks,
Will Whang

On Mon, Aug 11, 2025 at 1:01 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Sun, Aug 10, 2025 at 11:09:18PM +0100, Will Whang wrote:
> > +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).
>
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/20250703175121.GA17709@pendragon.ideasonboard.com/)

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.

Something like this?


> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  assigned-clocks: true
> > +  assigned-clock-parents: true
> > +  assigned-clock-rates: true
>
> Drop all three.
>
> > +
> > +  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
> > +    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.
>
> > +
> > +  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:
> > +                  - const: 1
> > +                  - const: 2
> > +                  - const: 3
> > +                  - const: 4
> > +
> > +          link-frequencies: true
>
> Drop
Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml and
sony,283.yaml doesn't seem to drop this?
Just to double check if my understanding is correct, you want me to
remove the line link-frequencies: true?

>
> > +
> > +        required:
> > +          - data-lanes
> > +          - link-frequencies
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - port
> > +
> > +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
>

Re: [PATCH v2 1/4] dt-bindings: media: Add Sony IMX585 CMOS image sensor

[Krzysztof Kozlowski]

On 12/08/2025 04:47, Will Whang wrote:
> Hi Krzysztof,
> Reply inline.
> Thanks,
> Will Whang
> 
> On Mon, Aug 11, 2025 at 1:01 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On Sun, Aug 10, 2025 at 11:09:18PM +0100, Will Whang wrote:
>>> +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).
>>
> 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/20250703175121.GA17709@pendragon.ideasonboard.com/)

You only partially answer and judging by mentioning driver below:


> 
> 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

Re: [PATCH v2 1/4] dt-bindings: media: Add Sony IMX585 CMOS image sensor

[Will Whang]

Hi Krzysztof,

Reply inline,
Thanks,
Will Whang

On Mon, Aug 11, 2025 at 11:23 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 12/08/2025 04:47, Will Whang wrote:
> > Hi Krzysztof,
> > Reply inline.
> > Thanks,
> > Will Whang
> >
> > On Mon, Aug 11, 2025 at 1:01 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On Sun, Aug 10, 2025 at 11:09:18PM +0100, Will Whang wrote:
> >>> +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).
> >>
> > 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/20250703175121.GA17709@pendragon.ideasonboard.com/)
>
> You only partially answer and judging by mentioning driver below:
>
>
> >
> > 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?
One device that has two variants: IMX585-AAMJ1 (Mono) and IMX585-AAQJ1
(Color). Silicon-wise the difference is just with or without bayer
filter.
>
>
> Best regards,
> Krzysztof

Re: [PATCH v2 1/4] dt-bindings: media: Add Sony IMX585 CMOS image sensor

[Krzysztof Kozlowski]

On 12/08/2025 08:31, Will Whang wrote:
> Hi Krzysztof,
> 
> Reply inline,
> Thanks,
> Will Whang
> 
> On Mon, Aug 11, 2025 at 11:23 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 12/08/2025 04:47, Will Whang wrote:
>>> Hi Krzysztof,
>>> Reply inline.
>>> Thanks,
>>> Will Whang
>>>
>>> On Mon, Aug 11, 2025 at 1:01 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>
>>>> On Sun, Aug 10, 2025 at 11:09:18PM +0100, Will Whang wrote:
>>>>> +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).
>>>>
>>> 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/20250703175121.GA17709@pendragon.ideasonboard.com/)
>>
>> You only partially answer and judging by mentioning driver below:
>>
>>
>>>
>>> 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?
> 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.

Best regards,
Krzysztof

Re: [PATCH v2 1/4] dt-bindings: media: Add Sony IMX585 CMOS image sensor

[Laurent Pinchart]

On Tue, Aug 12, 2025 at 08:47:12AM +0200, Krzysztof Kozlowski wrote:
> On 12/08/2025 08:31, Will Whang wrote:
> > On Mon, Aug 11, 2025 at 11:23 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >> On 12/08/2025 04:47, Will Whang wrote:
> >>> On Mon, Aug 11, 2025 at 1:01 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>> On Sun, Aug 10, 2025 at 11:09:18PM +0100, Will Whang wrote:
> >>>>> +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).
> >>>>
> >>> 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/20250703175121.GA17709@pendragon.ideasonboard.com/)
> >>
> >> You only partially answer and judging by mentioning driver below:
> >>
> >>> 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?
> > 
> > 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.

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/4] dt-bindings: media: Add Sony IMX585 CMOS image sensor

[Will Whang]

On Tue, Aug 12, 2025 at 2:56 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tue, Aug 12, 2025 at 08:47:12AM +0200, Krzysztof Kozlowski wrote:
> > On 12/08/2025 08:31, Will Whang wrote:
> > > On Mon, Aug 11, 2025 at 11:23 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >> On 12/08/2025 04:47, Will Whang wrote:
> > >>> On Mon, Aug 11, 2025 at 1:01 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >>>> On Sun, Aug 10, 2025 at 11:09:18PM +0100, Will Whang wrote:
> > >>>>> +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).
> > >>>>
> > >>> 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/20250703175121.GA17709@pendragon.ideasonboard.com/)
> > >>
> > >> You only partially answer and judging by mentioning driver below:
> > >>
> > >>> 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?
> > >
> > > 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.
>
> 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?

> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 1/4] dt-bindings: media: Add Sony IMX585 CMOS image sensor

[Krzysztof Kozlowski]

On 13/08/2025 06:30, Will Whang wrote:
> On Tue, Aug 12, 2025 at 2:56 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> On Tue, Aug 12, 2025 at 08:47:12AM +0200, Krzysztof Kozlowski wrote:
>>> On 12/08/2025 08:31, Will Whang wrote:
>>>> On Mon, Aug 11, 2025 at 11:23 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>> On 12/08/2025 04:47, Will Whang wrote:
>>>>>> On Mon, Aug 11, 2025 at 1:01 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>>>> On Sun, Aug 10, 2025 at 11:09:18PM +0100, Will Whang wrote:
>>>>>>>> +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).
>>>>>>>
>>>>>> 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/20250703175121.GA17709@pendragon.ideasonboard.com/)
>>>>>
>>>>> You only partially answer and judging by mentioning driver below:
>>>>>
>>>>>> 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?
>>>>
>>>> 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.
>>
>> 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

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

> 
> 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

Re: [PATCH v2 1/4] dt-bindings: media: Add Sony IMX585 CMOS image sensor

[Will Whang]

On Tue, Aug 12, 2025 at 11:08 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 13/08/2025 06:30, Will Whang wrote:
> > On Tue, Aug 12, 2025 at 2:56 AM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> >>
> >> On Tue, Aug 12, 2025 at 08:47:12AM +0200, Krzysztof Kozlowski wrote:
> >>> On 12/08/2025 08:31, Will Whang wrote:
> >>>> On Mon, Aug 11, 2025 at 11:23 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>>> On 12/08/2025 04:47, Will Whang wrote:
> >>>>>> On Mon, Aug 11, 2025 at 1:01 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>>>>> On Sun, Aug 10, 2025 at 11:09:18PM +0100, Will Whang wrote:
> >>>>>>>> +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).
> >>>>>>>
> >>>>>> 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/20250703175121.GA17709@pendragon.ideasonboard.com/)
> >>>>>
> >>>>> You only partially answer and judging by mentioning driver below:
> >>>>>
> >>>>>> 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?
> >>>>
> >>>> 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.
> >>
> >> 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
>
> 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.

> >
> > 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

Re: [PATCH v2 1/4] dt-bindings: media: Add Sony IMX585 CMOS image sensor

[Krzysztof Kozlowski]

On 13/08/2025 08:38, Will Whang wrote:
> On Tue, Aug 12, 2025 at 11:08 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 13/08/2025 06:30, Will Whang wrote:
>>> On Tue, Aug 12, 2025 at 2:56 AM Laurent Pinchart
>>> <laurent.pinchart@ideasonboard.com> wrote:
>>>>
>>>> On Tue, Aug 12, 2025 at 08:47:12AM +0200, Krzysztof Kozlowski wrote:
>>>>> On 12/08/2025 08:31, Will Whang wrote:
>>>>>> On Mon, Aug 11, 2025 at 11:23 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>>>> On 12/08/2025 04:47, Will Whang wrote:
>>>>>>>> On Mon, Aug 11, 2025 at 1:01 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>>>>>> On Sun, Aug 10, 2025 at 11:09:18PM +0100, Will Whang wrote:
>>>>>>>>>> +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).
>>>>>>>>>
>>>>>>>> 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/20250703175121.GA17709@pendragon.ideasonboard.com/)
>>>>>>>
>>>>>>> You only partially answer and judging by mentioning driver below:
>>>>>>>
>>>>>>>> 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?
>>>>>>
>>>>>> 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.
>>>>
>>>> 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
>>
>> 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.

So why four compatibles? BTW, driver does not matter.

> 
> 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.
Which? Sorry, point me anywhere to convention that we do not use proper
full model names but shortened version for some variant of a device?
Such approach lead to many issues in the past, for example current
Risc-v reset mess where contributor wants to remove completely
incomplete compatible :/

Best regards,
Krzysztof

[PATCH v2 2/4] media: uapi: Add custom IMX585 control IDs

[Will Whang]

Reserve a private control ID block and define the helper enums
used by the upcoming IMX585 driver.

Signed-off-by: Will Whang <will@willwhang.com>
---
 MAINTAINERS                        |  1 +
 include/uapi/linux/imx585.h        | 20 ++++++++++++++++++++
 include/uapi/linux/v4l2-controls.h |  6 ++++++
 3 files changed, 27 insertions(+)
 create mode 100644 include/uapi/linux/imx585.h

diff --git a/MAINTAINERS b/MAINTAINERS
index ef04ee4ec..e6aeac0c5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23183,6 +23183,7 @@ M:	Will Whang <will@willwhang.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
+F:	include/uapi/linux/imx585.h
 
 SONY MEMORYSTICK SUBSYSTEM
 M:	Maxim Levitsky <maximlevitsky@gmail.com>
diff --git a/include/uapi/linux/imx585.h b/include/uapi/linux/imx585.h
new file mode 100644
index 000000000..30e12df88
--- /dev/null
+++ b/include/uapi/linux/imx585.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * IMX585 V4L2 controls user space header file.
+ *
+ */
+
+#ifndef __UAPI_IMX585_H_
+#define __UAPI_IMX585_H_
+
+#include <linux/v4l2-controls.h>
+
+#define V4L2_CID_IMX585_HDR_DATASEL_TH   (V4L2_CID_USER_IMX585_BASE + 0)
+#define V4L2_CID_IMX585_HDR_DATASEL_BK   (V4L2_CID_USER_IMX585_BASE + 1)
+#define V4L2_CID_IMX585_HDR_GRAD_TH      (V4L2_CID_USER_IMX585_BASE + 2)
+#define V4L2_CID_IMX585_HDR_GRAD_COMP_L  (V4L2_CID_USER_IMX585_BASE + 3)
+#define V4L2_CID_IMX585_HDR_GRAD_COMP_H  (V4L2_CID_USER_IMX585_BASE + 4)
+#define V4L2_CID_IMX585_HDR_GAIN         (V4L2_CID_USER_IMX585_BASE + 5)
+#define V4L2_CID_IMX585_HCG_GAIN         (V4L2_CID_USER_IMX585_BASE + 6)
+
+#endif /* __UAPI_IMX585_H_ */
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index f836512e9..091a044e5 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -228,6 +228,12 @@ enum v4l2_colorfx {
  */
 #define V4L2_CID_USER_RKISP1_BASE		(V4L2_CID_USER_BASE + 0x1220)
 
+/*
+ * The base for IMX585 driver controls.
+ * We reserve 16 controls for this driver.
+ */
+#define V4L2_CID_USER_IMX585_BASE               (V4L2_CID_USER_BASE + 0x1230)
+
 /* MPEG-class control IDs */
 /* The MPEG controls are applicable to all codec controls
  * and the 'MPEG' part of the define is historical */
-- 
2.39.5

[PATCH v2 3/4] media: i2c: imx585: Add Sony IMX585 image-sensor driver

[Will Whang]

Implements support for:
  * 4-lane / 2-lane CSI-2
  * 12-bit linear, 12-bit HDR-GC and 16-bit Clear-HDR modes
  * Mono variant switch, HCG, custom HDR controls
  * Tested on Raspberry Pi 4/5 with 24 MHz clock.

Signed-off-by: Will Whang <will@willwhang.com>
---
 MAINTAINERS                |    1 +
 drivers/media/i2c/Kconfig  |    9 +
 drivers/media/i2c/Makefile |    1 +
 drivers/media/i2c/imx585.c | 1768 ++++++++++++++++++++++++++++++++++++
 4 files changed, 1779 insertions(+)
 create mode 100644 drivers/media/i2c/imx585.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e6aeac0c5..175f5236a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23183,6 +23183,7 @@ M:	Will Whang <will@willwhang.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
+F:	drivers/media/i2c/imx585.c
 F:	include/uapi/linux/imx585.h
 
 SONY MEMORYSTICK SUBSYSTEM
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 6237fe804..3dc3eea36 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -267,6 +267,15 @@ config VIDEO_IMX415
 	  To compile this driver as a module, choose M here: the
 	  module will be called imx415.
 
+config VIDEO_IMX585
+	tristate "Sony IMX585 sensor support"
+	help
+	  This is a Video4Linux2 sensor driver for the Sony
+	  IMX585 camera.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called imx585.
+
 config VIDEO_MAX9271_LIB
 	tristate
 
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 5873d2943..887d19ca7 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_VIDEO_IMX335) += imx335.o
 obj-$(CONFIG_VIDEO_IMX355) += imx355.o
 obj-$(CONFIG_VIDEO_IMX412) += imx412.o
 obj-$(CONFIG_VIDEO_IMX415) += imx415.o
+obj-$(CONFIG_VIDEO_IMX585) += imx585.o
 obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o
 obj-$(CONFIG_VIDEO_ISL7998X) += isl7998x.o
 obj-$(CONFIG_VIDEO_KS0127) += ks0127.o
diff --git a/drivers/media/i2c/imx585.c b/drivers/media/i2c/imx585.c
new file mode 100644
index 000000000..163cf68b2
--- /dev/null
+++ b/drivers/media/i2c/imx585.c
@@ -0,0 +1,1768 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * A V4L2 driver for Sony IMX585 camera.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_graph.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <linux/unaligned.h>
+
+#include <media/v4l2-cci.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-mediabus.h>
+
+/* --------------------------------------------------------------------------
+ * Driver-local custom controls
+ * --------------------------------------------------------------------------
+ */
+
+#ifndef V4L2_CID_USER_IMX585_BASE
+#define V4L2_CID_USER_IMX585_BASE (V4L2_CID_USER_BASE + 0x2000)
+#endif
+
+#define V4L2_CID_IMX585_HDR_DATASEL_TH  (V4L2_CID_USER_IMX585_BASE + 0)
+#define V4L2_CID_IMX585_HDR_DATASEL_BK  (V4L2_CID_USER_IMX585_BASE + 1)
+#define V4L2_CID_IMX585_HDR_GRAD_TH     (V4L2_CID_USER_IMX585_BASE + 2)
+#define V4L2_CID_IMX585_HDR_GRAD_COMP_L (V4L2_CID_USER_IMX585_BASE + 3)
+#define V4L2_CID_IMX585_HDR_GRAD_COMP_H (V4L2_CID_USER_IMX585_BASE + 4)
+#define V4L2_CID_IMX585_HDR_GAIN        (V4L2_CID_USER_IMX585_BASE + 5)
+#define V4L2_CID_IMX585_HCG_GAIN        (V4L2_CID_USER_IMX585_BASE + 6)
+
+/* --------------------------------------------------------------------------
+ * Registers / limits
+ * --------------------------------------------------------------------------
+ */
+
+/* Standby or streaming mode */
+#define IMX585_REG_MODE_SELECT          CCI_REG8(0x3000)
+#define IMX585_MODE_STANDBY             0x01
+#define IMX585_MODE_STREAMING           0x00
+#define IMX585_STREAM_DELAY_US          25000
+#define IMX585_STREAM_DELAY_RANGE_US    1000
+
+/* Initialisation delay between XCLR low->high and the moment sensor is ready */
+#define IMX585_XCLR_MIN_DELAY_US        500000
+#define IMX585_XCLR_DELAY_RANGE_US      1000
+
+/* Leader mode and XVS/XHS direction */
+#define IMX585_REG_XMSTA                CCI_REG8(0x3002)
+#define IMX585_REG_XXS_DRV              CCI_REG8(0x30a6)
+#define IMX585_REG_EXTMODE              CCI_REG8(0x30ce)
+#define IMX585_REG_XXS_OUTSEL           CCI_REG8(0x30a4)
+
+/* XVS pulse length, 2^n H with n=0~3 */
+#define IMX585_REG_XVSLNG               CCI_REG8(0x30cc)
+/* XHS pulse length, 16*(2^n) Clock with n=0~3 */
+#define IMX585_REG_XHSLNG               CCI_REG8(0x30cd)
+
+/* Clock selection */
+#define IMX585_INCK_SEL                 CCI_REG8(0x3014)
+
+/* Link speed selector */
+#define IMX585_DATARATE_SEL             CCI_REG8(0x3015)
+
+/* BIN mode: 0x01 mono bin, 0x00 color */
+#define IMX585_BIN_MODE                 CCI_REG8(0x3019)
+
+/* Lane Count */
+#define IMX585_LANEMODE                 CCI_REG8(0x3040)
+
+/* VMAX internal VBLANK */
+#define IMX585_REG_VMAX                 CCI_REG24_LE(0x3028)
+#define IMX585_VMAX_MAX                 0xfffff
+#define IMX585_VMAX_DEFAULT             2250
+
+/* HMAX internal HBLANK */
+#define IMX585_REG_HMAX                 CCI_REG16_LE(0x302c)
+#define IMX585_HMAX_MAX                 0xffff
+
+/* SHR internal (coarse exposure) */
+#define IMX585_REG_SHR                  CCI_REG24_LE(0x3050)
+#define IMX585_SHR_MIN                  8
+#define IMX585_SHR_MIN_HDR              10
+#define IMX585_SHR_MAX                  0xfffff
+
+/* Exposure control (lines) */
+#define IMX585_EXPOSURE_MIN             2
+#define IMX585_EXPOSURE_STEP            1
+#define IMX585_EXPOSURE_DEFAULT         1000
+#define IMX585_EXPOSURE_MAX             49865
+
+/* HDR threshold / blending / compression */
+#define IMX585_REG_EXP_TH_H             CCI_REG16_LE(0x36d0)
+#define IMX585_REG_EXP_TH_L             CCI_REG16_LE(0x36d4)
+#define IMX585_REG_EXP_BK               CCI_REG8(0x36e2)
+#define IMX585_REG_CCMP_EN              CCI_REG8(0x36ef)
+#define IMX585_REG_CCMP1_EXP            CCI_REG24_LE(0x36e8)
+#define IMX585_REG_CCMP2_EXP            CCI_REG24_LE(0x36e4)
+#define IMX585_REG_ACMP1_EXP            CCI_REG8(0x36ee)
+#define IMX585_REG_ACMP2_EXP            CCI_REG8(0x36ec)
+#define IMX585_REG_EXP_GAIN             CCI_REG8(0x3081)
+
+/* Black level control */
+#define IMX585_REG_BLKLEVEL             CCI_REG16_LE(0x30dc)
+#define IMX585_BLKLEVEL_DEFAULT         50
+
+/* Digital Clamp */
+#define IMX585_REG_DIGITAL_CLAMP        CCI_REG8(0x3458)
+
+/* Analog gain control */
+#define IMX585_REG_ANALOG_GAIN          CCI_REG16_LE(0x306c)
+#define IMX585_REG_FDG_SEL0             CCI_REG8(0x3030)
+#define IMX585_ANA_GAIN_MIN_NORMAL      0
+#define IMX585_ANA_GAIN_MIN_HCG         34
+#define IMX585_ANA_GAIN_MAX_HDR         80
+#define IMX585_ANA_GAIN_MAX_NORMAL      240
+#define IMX585_ANA_GAIN_STEP            1
+#define IMX585_ANA_GAIN_DEFAULT         0
+
+/* Flip */
+#define IMX585_FLIP_WINMODEH            CCI_REG8(0x3020)
+#define IMX585_FLIP_WINMODEV            CCI_REG8(0x3021)
+
+/* Pixel rate helper (sensor line clock proxy used below) */
+#define IMX585_PIXEL_RATE               74250000U
+
+/* Native and active array */
+#define IMX585_NATIVE_WIDTH             3856U
+#define IMX585_NATIVE_HEIGHT            2180U
+#define IMX585_PIXEL_ARRAY_LEFT         8U
+#define IMX585_PIXEL_ARRAY_TOP          8U
+#define IMX585_PIXEL_ARRAY_WIDTH        3840U
+#define IMX585_PIXEL_ARRAY_HEIGHT       2160U
+
+/* Link frequency setup */
+enum {
+	IMX585_LINK_FREQ_297MHZ,   /* 594 Mbps/lane  */
+	IMX585_LINK_FREQ_360MHZ,   /* 720 Mbps/lane  */
+	IMX585_LINK_FREQ_445MHZ,   /* 891 Mbps/lane  */
+	IMX585_LINK_FREQ_594MHZ,   /* 1188 Mbps/lane */
+	IMX585_LINK_FREQ_720MHZ,   /* 1440 Mbps/lane */
+	IMX585_LINK_FREQ_891MHZ,   /* 1782 Mbps/lane */
+	IMX585_LINK_FREQ_1039MHZ,  /* 2079 Mbps/lane */
+	IMX585_LINK_FREQ_1188MHZ,  /* 2376 Mbps/lane */
+};
+
+static const u8 link_freqs_reg_value[] = {
+	[IMX585_LINK_FREQ_297MHZ]  = 0x07,
+	[IMX585_LINK_FREQ_360MHZ]  = 0x06,
+	[IMX585_LINK_FREQ_445MHZ]  = 0x05,
+	[IMX585_LINK_FREQ_594MHZ]  = 0x04,
+	[IMX585_LINK_FREQ_720MHZ]  = 0x03,
+	[IMX585_LINK_FREQ_891MHZ]  = 0x02,
+	[IMX585_LINK_FREQ_1039MHZ] = 0x01,
+	[IMX585_LINK_FREQ_1188MHZ] = 0x00,
+};
+
+static const u64 link_freqs[] = {
+	[IMX585_LINK_FREQ_297MHZ]  = 297000000ULL,
+	[IMX585_LINK_FREQ_360MHZ]  = 360000000ULL,
+	[IMX585_LINK_FREQ_445MHZ]  = 445500000ULL,
+	[IMX585_LINK_FREQ_594MHZ]  = 594000000ULL,
+	[IMX585_LINK_FREQ_720MHZ]  = 720000000ULL,
+	[IMX585_LINK_FREQ_891MHZ]  = 891000000ULL,
+	[IMX585_LINK_FREQ_1039MHZ] = 1039500000ULL,
+	[IMX585_LINK_FREQ_1188MHZ] = 1188000000ULL,
+};
+
+/* min HMAX for 4-lane 4K full res mode, x2 for 2-lane, /2 for FHD */
+static const u16 HMAX_table_4lane_4K[] = {
+	[IMX585_LINK_FREQ_297MHZ]  = 1584,
+	[IMX585_LINK_FREQ_360MHZ]  = 1320,
+	[IMX585_LINK_FREQ_445MHZ]  = 1100,
+	[IMX585_LINK_FREQ_594MHZ]  = 792,
+	[IMX585_LINK_FREQ_720MHZ]  = 660,
+	[IMX585_LINK_FREQ_891MHZ]  = 550,
+	[IMX585_LINK_FREQ_1039MHZ] = 440,
+	[IMX585_LINK_FREQ_1188MHZ] = 396,
+};
+
+struct imx585_inck_cfg {
+	u32 xclk_hz;
+	u8  inck_sel;
+};
+
+static const struct imx585_inck_cfg imx585_inck_table[] = {
+	{ 74250000, 0x00 },
+	{ 37125000, 0x01 },
+	{ 72000000, 0x02 },
+	{ 27000000, 0x03 },
+	{ 24000000, 0x04 },
+};
+
+static const char * const hdr_gain_adder_menu[] = {
+	"+0dB", "+6dB", "+12dB", "+18dB", "+24dB", "+29.1dB",
+};
+
+/* Keep the order as in datasheet, there are two 50/50 for some reasons */
+static const char * const hdr_data_blender_menu[] = {
+	"HG 1/2, LG 1/2",
+	"HG 3/4, LG 1/4",
+	"HG 1/2, LG 1/2",
+	"HG 7/8, LG 1/8",
+	"HG 15/16, LG 1/16",
+	"2nd HG 1/2, LG 1/2",
+	"HG 1/16, LG 15/16",
+	"HG 1/8, LG 7/8",
+	"HG 1/4, LG 3/4",
+};
+
+static const char * const grad_compression_slope_menu[] = {
+	"1/1", "1/2", "1/4",  "1/8",   "1/16", "1/32",
+	"1/64", "1/128", "1/256", "1/512", "1/1024", "1/2048",
+};
+
+enum {
+	SYNC_INT_LEADER,
+	SYNC_INT_FOLLOWER,
+	SYNC_EXTERNAL,
+};
+
+static const char * const sync_mode_menu[] = {
+	"Internal Sync Leader Mode",
+	"External Sync Leader Mode",
+	"Follower Mode",
+};
+
+/* Mode description */
+struct imx585_mode {
+	unsigned int width;
+	unsigned int height;
+
+	u8  hmax_div;       /* per-mode scaling of min HMAX */
+	u16 min_hmax;       /* computed at runtime */
+	u32 min_vmax;       /* computed at runtime (fits 20-bit) */
+
+	struct v4l2_rect crop;
+
+	struct {
+		unsigned int num_of_regs;
+		const struct cci_reg_sequence *regs;
+	} reg_list;
+};
+
+/* --------------------------------------------------------------------------
+ * Register tables
+ * --------------------------------------------------------------------------
+ */
+
+static const struct cci_reg_sequence common_regs[] = {
+	{ CCI_REG8(0x3002), 0x01 },
+	{ CCI_REG8(0x3069), 0x00 },
+	{ CCI_REG8(0x3074), 0x64 },
+	{ CCI_REG8(0x30d5), 0x04 }, /* DIG_CLP_VSTART */
+	{ CCI_REG8(0x3030), 0x00 }, /* FDG_SEL0 LCG (HCG=0x01) */
+	{ CCI_REG8(0x30a6), 0x00 }, /* XVS_DRV [1:0] Hi-Z */
+	{ CCI_REG8(0x3081), 0x00 }, /* EXP_GAIN reset */
+	{ CCI_REG8(0x303a), 0x03 }, /* Disable embedded data */
+
+	/* The remaining blocks are datasheet-recommended settings */
+	{ CCI_REG8(0x3460), 0x21 }, { CCI_REG8(0x3478), 0xa1 },
+	{ CCI_REG8(0x347c), 0x01 }, { CCI_REG8(0x3480), 0x01 },
+	{ CCI_REG8(0x3a4e), 0x14 }, { CCI_REG8(0x3a52), 0x14 },
+	{ CCI_REG8(0x3a56), 0x00 }, { CCI_REG8(0x3a5a), 0x00 },
+	{ CCI_REG8(0x3a5e), 0x00 }, { CCI_REG8(0x3a62), 0x00 },
+	{ CCI_REG8(0x3a6a), 0x20 }, { CCI_REG8(0x3a6c), 0x42 },
+	{ CCI_REG8(0x3a6e), 0xa0 }, { CCI_REG8(0x3b2c), 0x0c },
+	{ CCI_REG8(0x3b30), 0x1c }, { CCI_REG8(0x3b34), 0x0c },
+	{ CCI_REG8(0x3b38), 0x1c }, { CCI_REG8(0x3ba0), 0x0c },
+	{ CCI_REG8(0x3ba4), 0x1c }, { CCI_REG8(0x3ba8), 0x0c },
+	{ CCI_REG8(0x3bac), 0x1c }, { CCI_REG8(0x3d3c), 0x11 },
+	{ CCI_REG8(0x3d46), 0x0b }, { CCI_REG8(0x3de0), 0x3f },
+	{ CCI_REG8(0x3de1), 0x08 }, { CCI_REG8(0x3e14), 0x87 },
+	{ CCI_REG8(0x3e16), 0x91 }, { CCI_REG8(0x3e18), 0x91 },
+	{ CCI_REG8(0x3e1a), 0x87 }, { CCI_REG8(0x3e1c), 0x78 },
+	{ CCI_REG8(0x3e1e), 0x50 }, { CCI_REG8(0x3e20), 0x50 },
+	{ CCI_REG8(0x3e22), 0x50 }, { CCI_REG8(0x3e24), 0x87 },
+	{ CCI_REG8(0x3e26), 0x91 }, { CCI_REG8(0x3e28), 0x91 },
+	{ CCI_REG8(0x3e2a), 0x87 }, { CCI_REG8(0x3e2c), 0x78 },
+	{ CCI_REG8(0x3e2e), 0x50 }, { CCI_REG8(0x3e30), 0x50 },
+	{ CCI_REG8(0x3e32), 0x50 }, { CCI_REG8(0x3e34), 0x87 },
+	{ CCI_REG8(0x3e36), 0x91 }, { CCI_REG8(0x3e38), 0x91 },
+	{ CCI_REG8(0x3e3a), 0x87 }, { CCI_REG8(0x3e3c), 0x78 },
+	{ CCI_REG8(0x3e3e), 0x50 }, { CCI_REG8(0x3e40), 0x50 },
+	{ CCI_REG8(0x3e42), 0x50 }, { CCI_REG8(0x4054), 0x64 },
+	{ CCI_REG8(0x4148), 0xfe }, { CCI_REG8(0x4149), 0x05 },
+	{ CCI_REG8(0x414a), 0xff }, { CCI_REG8(0x414b), 0x05 },
+	{ CCI_REG8(0x420a), 0x03 }, { CCI_REG8(0x4231), 0x08 },
+	{ CCI_REG8(0x423d), 0x9c }, { CCI_REG8(0x4242), 0xb4 },
+	{ CCI_REG8(0x4246), 0xb4 }, { CCI_REG8(0x424e), 0xb4 },
+	{ CCI_REG8(0x425c), 0xb4 }, { CCI_REG8(0x425e), 0xb6 },
+	{ CCI_REG8(0x426c), 0xb4 }, { CCI_REG8(0x426e), 0xb6 },
+	{ CCI_REG8(0x428c), 0xb4 }, { CCI_REG8(0x428e), 0xb6 },
+	{ CCI_REG8(0x4708), 0x00 }, { CCI_REG8(0x4709), 0x00 },
+	{ CCI_REG8(0x470a), 0xff }, { CCI_REG8(0x470b), 0x03 },
+	{ CCI_REG8(0x470c), 0x00 }, { CCI_REG8(0x470d), 0x00 },
+	{ CCI_REG8(0x470e), 0xff }, { CCI_REG8(0x470f), 0x03 },
+	{ CCI_REG8(0x47eb), 0x1c }, { CCI_REG8(0x47f0), 0xa6 },
+	{ CCI_REG8(0x47f2), 0xa6 }, { CCI_REG8(0x47f4), 0xa0 },
+	{ CCI_REG8(0x47f6), 0x96 }, { CCI_REG8(0x4808), 0xa6 },
+	{ CCI_REG8(0x480a), 0xa6 }, { CCI_REG8(0x480c), 0xa0 },
+	{ CCI_REG8(0x480e), 0x96 }, { CCI_REG8(0x492c), 0xb2 },
+	{ CCI_REG8(0x4930), 0x03 }, { CCI_REG8(0x4932), 0x03 },
+	{ CCI_REG8(0x4936), 0x5b }, { CCI_REG8(0x4938), 0x82 },
+	{ CCI_REG8(0x493e), 0x23 }, { CCI_REG8(0x4ba8), 0x1c },
+	{ CCI_REG8(0x4ba9), 0x03 }, { CCI_REG8(0x4bac), 0x1c },
+	{ CCI_REG8(0x4bad), 0x1c }, { CCI_REG8(0x4bae), 0x1c },
+	{ CCI_REG8(0x4baf), 0x1c }, { CCI_REG8(0x4bb0), 0x1c },
+	{ CCI_REG8(0x4bb1), 0x1c }, { CCI_REG8(0x4bb2), 0x1c },
+	{ CCI_REG8(0x4bb3), 0x1c }, { CCI_REG8(0x4bb4), 0x1c },
+	{ CCI_REG8(0x4bb8), 0x03 }, { CCI_REG8(0x4bb9), 0x03 },
+	{ CCI_REG8(0x4bba), 0x03 }, { CCI_REG8(0x4bbb), 0x03 },
+	{ CCI_REG8(0x4bbc), 0x03 }, { CCI_REG8(0x4bbd), 0x03 },
+	{ CCI_REG8(0x4bbe), 0x03 }, { CCI_REG8(0x4bbf), 0x03 },
+	{ CCI_REG8(0x4bc0), 0x03 }, { CCI_REG8(0x4c14), 0x87 },
+	{ CCI_REG8(0x4c16), 0x91 }, { CCI_REG8(0x4c18), 0x91 },
+	{ CCI_REG8(0x4c1a), 0x87 }, { CCI_REG8(0x4c1c), 0x78 },
+	{ CCI_REG8(0x4c1e), 0x50 }, { CCI_REG8(0x4c20), 0x50 },
+	{ CCI_REG8(0x4c22), 0x50 }, { CCI_REG8(0x4c24), 0x87 },
+	{ CCI_REG8(0x4c26), 0x91 }, { CCI_REG8(0x4c28), 0x91 },
+	{ CCI_REG8(0x4c2a), 0x87 }, { CCI_REG8(0x4c2c), 0x78 },
+	{ CCI_REG8(0x4c2e), 0x50 }, { CCI_REG8(0x4c30), 0x50 },
+	{ CCI_REG8(0x4c32), 0x50 }, { CCI_REG8(0x4c34), 0x87 },
+	{ CCI_REG8(0x4c36), 0x91 }, { CCI_REG8(0x4c38), 0x91 },
+	{ CCI_REG8(0x4c3a), 0x87 }, { CCI_REG8(0x4c3c), 0x78 },
+	{ CCI_REG8(0x4c3e), 0x50 }, { CCI_REG8(0x4c40), 0x50 },
+	{ CCI_REG8(0x4c42), 0x50 }, { CCI_REG8(0x4d12), 0x1f },
+	{ CCI_REG8(0x4d13), 0x1e }, { CCI_REG8(0x4d26), 0x33 },
+	{ CCI_REG8(0x4e0e), 0x59 }, { CCI_REG8(0x4e14), 0x55 },
+	{ CCI_REG8(0x4e16), 0x59 }, { CCI_REG8(0x4e1e), 0x3b },
+	{ CCI_REG8(0x4e20), 0x47 }, { CCI_REG8(0x4e22), 0x54 },
+	{ CCI_REG8(0x4e26), 0x81 }, { CCI_REG8(0x4e2c), 0x7d },
+	{ CCI_REG8(0x4e2e), 0x81 }, { CCI_REG8(0x4e36), 0x63 },
+	{ CCI_REG8(0x4e38), 0x6f }, { CCI_REG8(0x4e3a), 0x7c },
+	{ CCI_REG8(0x4f3a), 0x3c }, { CCI_REG8(0x4f3c), 0x46 },
+	{ CCI_REG8(0x4f3e), 0x59 }, { CCI_REG8(0x4f42), 0x64 },
+	{ CCI_REG8(0x4f44), 0x6e }, { CCI_REG8(0x4f46), 0x81 },
+	{ CCI_REG8(0x4f4a), 0x82 }, { CCI_REG8(0x4f5a), 0x81 },
+	{ CCI_REG8(0x4f62), 0xaa }, { CCI_REG8(0x4f72), 0xa9 },
+	{ CCI_REG8(0x4f78), 0x36 }, { CCI_REG8(0x4f7a), 0x41 },
+	{ CCI_REG8(0x4f7c), 0x61 }, { CCI_REG8(0x4f7d), 0x01 },
+	{ CCI_REG8(0x4f7e), 0x7c }, { CCI_REG8(0x4f7f), 0x01 },
+	{ CCI_REG8(0x4f80), 0x77 }, { CCI_REG8(0x4f82), 0x7b },
+	{ CCI_REG8(0x4f88), 0x37 }, { CCI_REG8(0x4f8a), 0x40 },
+	{ CCI_REG8(0x4f8c), 0x62 }, { CCI_REG8(0x4f8d), 0x01 },
+	{ CCI_REG8(0x4f8e), 0x76 }, { CCI_REG8(0x4f8f), 0x01 },
+	{ CCI_REG8(0x4f90), 0x5e }, { CCI_REG8(0x4f91), 0x02 },
+	{ CCI_REG8(0x4f92), 0x69 }, { CCI_REG8(0x4f93), 0x02 },
+	{ CCI_REG8(0x4f94), 0x89 }, { CCI_REG8(0x4f95), 0x02 },
+	{ CCI_REG8(0x4f96), 0xa4 }, { CCI_REG8(0x4f97), 0x02 },
+	{ CCI_REG8(0x4f98), 0x9f }, { CCI_REG8(0x4f99), 0x02 },
+	{ CCI_REG8(0x4f9a), 0xa3 }, { CCI_REG8(0x4f9b), 0x02 },
+	{ CCI_REG8(0x4fa0), 0x5f }, { CCI_REG8(0x4fa1), 0x02 },
+	{ CCI_REG8(0x4fa2), 0x68 }, { CCI_REG8(0x4fa3), 0x02 },
+	{ CCI_REG8(0x4fa4), 0x8a }, { CCI_REG8(0x4fa5), 0x02 },
+	{ CCI_REG8(0x4fa6), 0x9e }, { CCI_REG8(0x4fa7), 0x02 },
+	{ CCI_REG8(0x519e), 0x79 }, { CCI_REG8(0x51a6), 0xa1 },
+	{ CCI_REG8(0x51f0), 0xac }, { CCI_REG8(0x51f2), 0xaa },
+	{ CCI_REG8(0x51f4), 0xa5 }, { CCI_REG8(0x51f6), 0xa0 },
+	{ CCI_REG8(0x5200), 0x9b }, { CCI_REG8(0x5202), 0x91 },
+	{ CCI_REG8(0x5204), 0x87 }, { CCI_REG8(0x5206), 0x82 },
+	{ CCI_REG8(0x5208), 0xac }, { CCI_REG8(0x520a), 0xaa },
+	{ CCI_REG8(0x520c), 0xa5 }, { CCI_REG8(0x520e), 0xa0 },
+	{ CCI_REG8(0x5210), 0x9b }, { CCI_REG8(0x5212), 0x91 },
+	{ CCI_REG8(0x5214), 0x87 }, { CCI_REG8(0x5216), 0x82 },
+	{ CCI_REG8(0x5218), 0xac }, { CCI_REG8(0x521a), 0xaa },
+	{ CCI_REG8(0x521c), 0xa5 }, { CCI_REG8(0x521e), 0xa0 },
+	{ CCI_REG8(0x5220), 0x9b }, { CCI_REG8(0x5222), 0x91 },
+	{ CCI_REG8(0x5224), 0x87 }, { CCI_REG8(0x5226), 0x82 },
+};
+
+static const struct cci_reg_sequence common_clearHDR_mode[] = {
+	{ CCI_REG8(0x301a), 0x10 }, /* WDMODE: Clear HDR */
+	{ CCI_REG8(0x3024), 0x02 }, /* COMBI_EN */
+	{ CCI_REG8(0x3069), 0x02 },
+	{ CCI_REG8(0x3074), 0x63 },
+	{ CCI_REG8(0x3930), 0xe6 }, /* DUR[15:8] (12-bit) */
+	{ CCI_REG8(0x3931), 0x00 }, /* DUR[7:0]  (12-bit) */
+	{ CCI_REG8(0x3a4c), 0x61 }, { CCI_REG8(0x3a4d), 0x02 },
+	{ CCI_REG8(0x3a50), 0x70 }, { CCI_REG8(0x3a51), 0x02 },
+	{ CCI_REG8(0x3e10), 0x17 }, /* ADTHEN */
+	{ CCI_REG8(0x493c), 0x41 }, /* 10-bit HDR */
+	{ CCI_REG8(0x4940), 0x41 }, /* 12-bit HDR */
+	{ CCI_REG8(0x3081), 0x02 }, /* EXP_GAIN: +12 dB default */
+};
+
+static const struct cci_reg_sequence common_normal_mode[] = {
+	{ CCI_REG8(0x301a), 0x00 }, /* WDMODE: Normal */
+	{ CCI_REG8(0x3024), 0x00 }, /* COMBI_EN */
+	{ CCI_REG8(0x3069), 0x00 },
+	{ CCI_REG8(0x3074), 0x64 },
+	{ CCI_REG8(0x3930), 0x0c }, /* DUR[15:8] (12-bit) */
+	{ CCI_REG8(0x3931), 0x01 }, /* DUR[7:0]  (12-bit) */
+	{ CCI_REG8(0x3a4c), 0x39 }, { CCI_REG8(0x3a4d), 0x01 },
+	{ CCI_REG8(0x3a50), 0x48 }, { CCI_REG8(0x3a51), 0x01 },
+	{ CCI_REG8(0x3e10), 0x10 }, /* ADTHEN */
+	{ CCI_REG8(0x493c), 0x23 }, /* 10-bit Normal */
+	{ CCI_REG8(0x4940), 0x23 }, /* 12-bit Normal */
+};
+
+/* All-pixel 4K, 12-bit */
+static const struct cci_reg_sequence mode_4k_regs_12bit[] = {
+	{ CCI_REG8(0x301b), 0x00 }, /* ADDMODE non-binning */
+	{ CCI_REG8(0x3022), 0x02 }, /* ADBIT 12-bit */
+	{ CCI_REG8(0x3023), 0x01 }, /* MDBIT 12-bit */
+	{ CCI_REG8(0x30d5), 0x04 }, /* DIG_CLP_VSTART non-binning */
+};
+
+/* 2x2 binned 1080p, 12-bit */
+static const struct cci_reg_sequence mode_1080_regs_12bit[] = {
+	{ CCI_REG8(0x301b), 0x01 }, /* ADDMODE binning */
+	{ CCI_REG8(0x3022), 0x02 }, /* ADBIT 12-bit */
+	{ CCI_REG8(0x3023), 0x01 }, /* MDBIT 12-bit */
+	{ CCI_REG8(0x30d5), 0x02 }, /* DIG_CLP_VSTART binning */
+};
+
+/* --------------------------------------------------------------------------
+ * Mode list
+ * --------------------------------------------------------------------------
+ * Default:
+ *   12Bit - FHD, 4K
+ * ClearHDR Enabled:
+ *   12bit + Gradation compression
+ *   16bit - FHD, 4K
+ *
+ * Gradation compression is available on 12bit
+ * With Default option, only 12bit mode is exposed
+ * With ClearHDR enabled via parameters,
+ *   12bit will be with Gradation compression enabled
+ *   16bit mode exposed
+ *
+ * Technically, because the sensor is actually binning
+ * in digital domain, its readout speed is the same
+ * between 4K and FHD. However, through testing it is
+ * possible to "overclock" the FHD mode, thus leaving the
+ * hmax_div option for those who want to try.
+ * Also, note that FHD and 4K mode shared the same VMAX.
+ */
+
+static struct imx585_mode supported_modes[] = {
+	{
+		/* 1080p60 2x2 binning */
+		.width = 1928,
+		.height = 1090,
+		.hmax_div = 1,
+		.min_hmax = 366,            /* overwritten at runtime */
+		.min_vmax = IMX585_VMAX_DEFAULT,
+		.crop = {
+			.left = IMX585_PIXEL_ARRAY_LEFT,
+			.top = IMX585_PIXEL_ARRAY_TOP,
+			.width = IMX585_PIXEL_ARRAY_WIDTH,
+			.height = IMX585_PIXEL_ARRAY_HEIGHT,
+		},
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1080_regs_12bit),
+			.regs = mode_1080_regs_12bit,
+		},
+	},
+	{
+		/* 4K60 All pixel */
+		.width = 3856,
+		.height = 2180,
+		.hmax_div = 1,
+		.min_hmax = 550,            /* overwritten at runtime */
+		.min_vmax = IMX585_VMAX_DEFAULT,
+		.crop = {
+			.left = IMX585_PIXEL_ARRAY_LEFT,
+			.top = IMX585_PIXEL_ARRAY_TOP,
+			.width = IMX585_PIXEL_ARRAY_WIDTH,
+			.height = IMX585_PIXEL_ARRAY_HEIGHT,
+		},
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_4k_regs_12bit),
+			.regs = mode_4k_regs_12bit,
+		},
+	},
+};
+
+/* Formats exposed per mode/bit depth */
+static const u32 codes_normal[] = {
+	MEDIA_BUS_FMT_SRGGB12_1X12,
+	MEDIA_BUS_FMT_SGRBG12_1X12,
+	MEDIA_BUS_FMT_SGBRG12_1X12,
+	MEDIA_BUS_FMT_SBGGR12_1X12,
+};
+
+static const u32 codes_clearhdr[] = {
+	/* 16-bit first */
+	MEDIA_BUS_FMT_SRGGB16_1X16,
+	MEDIA_BUS_FMT_SGRBG16_1X16,
+	MEDIA_BUS_FMT_SGBRG16_1X16,
+	MEDIA_BUS_FMT_SBGGR16_1X16,
+	/* then 12-bit */
+	MEDIA_BUS_FMT_SRGGB12_1X12,
+	MEDIA_BUS_FMT_SGRBG12_1X12,
+	MEDIA_BUS_FMT_SGBRG12_1X12,
+	MEDIA_BUS_FMT_SBGGR12_1X12,
+};
+
+static const u32 mono_codes[] = {
+	MEDIA_BUS_FMT_Y16_1X16,
+	MEDIA_BUS_FMT_Y12_1X12,
+};
+
+/* Regulators */
+static const char * const imx585_supply_name[] = {
+	"vana", /* 3.3V analog */
+	"vdig", /* 1.1V core   */
+	"vddl", /* 1.8V I/O    */
+};
+
+#define IMX585_NUM_SUPPLIES ARRAY_SIZE(imx585_supply_name)
+
+/* --------------------------------------------------------------------------
+ * State
+ * --------------------------------------------------------------------------
+ */
+
+struct imx585 {
+	struct v4l2_subdev sd;
+	struct media_pad pad;
+	struct device *clientdev;
+	struct regmap *regmap;
+
+	struct clk *xclk;
+	u32 xclk_freq;
+	u8  inck_sel_val;
+
+	unsigned int lane_count;
+	unsigned int link_freq_idx;
+
+	struct gpio_desc *reset_gpio;
+	struct regulator_bulk_data supplies[IMX585_NUM_SUPPLIES];
+
+	struct v4l2_ctrl_handler ctrl_handler;
+
+	/* Controls */
+	struct v4l2_ctrl *pixel_rate;
+	struct v4l2_ctrl *link_freq;
+	struct v4l2_ctrl *exposure;
+	struct v4l2_ctrl *gain;
+	struct v4l2_ctrl *hcg_ctrl;
+	struct v4l2_ctrl *vflip;
+	struct v4l2_ctrl *hflip;
+	struct v4l2_ctrl *vblank;
+	struct v4l2_ctrl *hblank;
+	struct v4l2_ctrl *blacklevel;
+
+	/* HDR controls */
+	struct v4l2_ctrl *hdr_mode;
+	struct v4l2_ctrl *datasel_th_ctrl;
+	struct v4l2_ctrl *datasel_bk_ctrl;
+	struct v4l2_ctrl *gdc_th_ctrl;
+	struct v4l2_ctrl *gdc_exp_ctrl_l;
+	struct v4l2_ctrl *gdc_exp_ctrl_h;
+	struct v4l2_ctrl *hdr_gain_ctrl;
+
+	/* Flags/params */
+	bool hcg;
+	bool mono;
+	bool clear_hdr;
+
+	/*
+	 * Sync Mode
+	 * 0 = Internal Sync Leader Mode
+	 * 1 = External Sync Leader Mode
+	 * 2 = Follower Mode
+	 * The datasheet wording is very confusing but basically:
+	 * Leader Mode = Sensor using internal clock to drive the sensor
+	 * But with external sync mode you can send a XVS input so the sensor
+	 * will try to align with it.
+	 * For Follower mode it is purely driven by external clock.
+	 * In this case you need to drive both XVS and XHS.
+	 */
+	u8   sync_mode;
+
+	u16  hmax;
+	u32  vmax;
+
+	bool streaming;
+	bool common_regs_written;
+};
+
+/* Helpers */
+
+static inline struct imx585 *to_imx585(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct imx585, sd);
+}
+
+static inline void get_mode_table(struct imx585 *imx585, unsigned int code,
+				  const struct imx585_mode **mode_list,
+				  unsigned int *num_modes)
+{
+	*mode_list = NULL;
+	*num_modes = 0;
+
+	if (imx585->mono) {
+		/* --- Mono paths --- */
+		if (code == MEDIA_BUS_FMT_Y16_1X16 && imx585->clear_hdr) {
+			*mode_list = supported_modes;
+			*num_modes = ARRAY_SIZE(supported_modes);
+		}
+		if (code == MEDIA_BUS_FMT_Y12_1X12) {
+			*mode_list = supported_modes;
+			*num_modes = ARRAY_SIZE(supported_modes);
+		}
+	} else {
+		/* --- Color paths --- */
+		switch (code) {
+		/* 16-bit */
+		case MEDIA_BUS_FMT_SRGGB16_1X16:
+		case MEDIA_BUS_FMT_SGRBG16_1X16:
+		case MEDIA_BUS_FMT_SGBRG16_1X16:
+		case MEDIA_BUS_FMT_SBGGR16_1X16:
+		/* 12-bit */
+		case MEDIA_BUS_FMT_SRGGB12_1X12:
+		case MEDIA_BUS_FMT_SGRBG12_1X12:
+		case MEDIA_BUS_FMT_SGBRG12_1X12:
+		case MEDIA_BUS_FMT_SBGGR12_1X12:
+			*mode_list = supported_modes;
+			*num_modes = ARRAY_SIZE(supported_modes);
+			break;
+		default:
+			*mode_list = NULL;
+			*num_modes = 0;
+		}
+	}
+}
+
+static u32 imx585_get_format_code(struct imx585 *imx585, u32 code)
+{
+	unsigned int i;
+
+	if (imx585->mono) {
+		for (i = 0; i < ARRAY_SIZE(mono_codes); i++)
+			if (mono_codes[i] == code)
+				return mono_codes[i];
+		return mono_codes[0];
+	}
+
+	if (imx585->clear_hdr) {
+		for (i = 0; i < ARRAY_SIZE(codes_clearhdr); i++)
+			if (codes_clearhdr[i] == code)
+				return codes_clearhdr[i];
+		return codes_clearhdr[0];
+	}
+
+	for (i = 0; i < ARRAY_SIZE(codes_normal); i++)
+		if (codes_normal[i] == code)
+			return codes_normal[i];
+	return codes_normal[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 */
+static void imx585_update_hmax(struct imx585 *imx585)
+{
+	const u32 base_4lane = HMAX_table_4lane_4K[imx585->link_freq_idx];
+	const u32 lane_scale = (imx585->lane_count == 2) ? 2 : 1;
+	const u32 factor     = base_4lane * lane_scale;
+	const u32 hdr_scale  = imx585->clear_hdr ? 2 : 1;
+	unsigned int i;
+
+	dev_info(imx585->clientdev, "Update minimum HMAX: base=%u lane_scale=%u hdr_scale=%u\n",
+		 base_4lane, lane_scale, hdr_scale);
+
+	for (i = 0; i < ARRAY_SIZE(supported_modes); ++i) {
+		u32 h = factor / supported_modes[i].hmax_div;
+		u32 v = IMX585_VMAX_DEFAULT * hdr_scale;
+
+		supported_modes[i].min_hmax = h;
+		supported_modes[i].min_vmax = v;
+
+		dev_info(imx585->clientdev, " mode %ux%u -> VMAX=%u HMAX=%u\n",
+			 supported_modes[i].width, supported_modes[i].height, v, h);
+	}
+}
+
+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,
+				 IMX585_VMAX_MAX - mode->height,
+				 1, mode->min_vmax - mode->height);
+	__v4l2_ctrl_s_ctrl(imx585->vblank, mode->min_vmax - mode->height);
+
+	__v4l2_ctrl_modify_range(imx585->exposure, IMX585_EXPOSURE_MIN,
+				 imx585->vmax - IMX585_SHR_MIN_HDR, 1,
+				 IMX585_EXPOSURE_DEFAULT);
+
+	dev_info(imx585->clientdev, "Framing: VMAX=%u HMAX=%u pixel_rate=%llu\n",
+		 imx585->vmax, imx585->hmax, pixel_rate);
+}
+
+/* --------------------------------------------------------------------------
+ * 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);
+			v4l2_ctrl_activate(imx585->gdc_exp_ctrl_h,   imx585->clear_hdr);
+			v4l2_ctrl_activate(imx585->gdc_exp_ctrl_l,   imx585->clear_hdr);
+			v4l2_ctrl_activate(imx585->hdr_gain_ctrl,    imx585->clear_hdr);
+			v4l2_ctrl_activate(imx585->hcg_ctrl,        !imx585->clear_hdr);
+
+			/* Disable HCG in ClearHDR mode */
+			imx585->hcg = imx585->clear_hdr ? 0 : imx585->hcg;
+			__v4l2_ctrl_s_ctrl(imx585->hcg_ctrl, imx585->hcg);
+			imx585_update_gain_limits(imx585);
+			dev_info(imx585->clientdev, "HDR=%u, HCG=%u\n", ctrl->val, imx585->hcg);
+
+			code = imx585->mono ? MEDIA_BUS_FMT_Y12_1X12
+					    : MEDIA_BUS_FMT_SRGGB12_1X12;
+			get_mode_table(imx585, code, &mode_list, &num_modes);
+			mode = v4l2_find_nearest_size(mode_list, num_modes, width, height,
+						      fmt->width, fmt->height);
+			imx585_set_framing_limits(imx585, mode);
+		}
+		break;
+	case V4L2_CID_IMX585_HCG_GAIN:
+		if (!imx585->clear_hdr) {
+			imx585->hcg = ctrl->val;
+			imx585_update_gain_limits(imx585);
+			dev_info(imx585->clientdev, "HCG=%u\n", ctrl->val);
+		}
+		break;
+	default:
+		break;
+	}
+
+	/* Apply control only when powered (runtime active). */
+	if (!pm_runtime_get_if_active(imx585->clientdev))
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_EXPOSURE: {
+		u32 shr = (imx585->vmax - ctrl->val) & ~1U; /* SHR always a multiple of 2 */
+
+		dev_dbg(imx585->clientdev, "EXPOSURE=%u -> SHR=%u (VMAX=%u HMAX=%u)\n",
+			ctrl->val, shr, imx585->vmax, imx585->hmax);
+
+		ret = cci_write(imx585->regmap, IMX585_REG_SHR, shr, NULL);
+		if (ret)
+			dev_err_ratelimited(imx585->clientdev, "SHR write failed (%d)\n", ret);
+		break;
+	}
+	case V4L2_CID_IMX585_HCG_GAIN:
+		if (!imx585->clear_hdr) {
+			ret = cci_write(imx585->regmap, IMX585_REG_FDG_SEL0, ctrl->val, NULL);
+			if (ret)
+				dev_err_ratelimited(imx585->clientdev,
+						    "FDG_SEL0 write failed (%d)\n", ret);
+			dev_info(imx585->clientdev, "HCG write reg=%u\n", ctrl->val);
+		}
+		break;
+	case V4L2_CID_ANALOGUE_GAIN:
+		dev_dbg(imx585->clientdev, "ANALOG_GAIN=%u (%s)\n",
+			ctrl->val, imx585->hcg ? "HCG" : "LCG");
+
+		ret = cci_write(imx585->regmap, IMX585_REG_ANALOG_GAIN, ctrl->val, NULL);
+		if (ret)
+			dev_err_ratelimited(imx585->clientdev, "Gain write failed (%d)\n", ret);
+		break;
+	case V4L2_CID_VBLANK: {
+		u32 current_exposure = imx585->exposure->cur.val;
+		const u32 min_shr = imx585->clear_hdr ? IMX585_SHR_MIN_HDR : IMX585_SHR_MIN;
+
+		imx585->vmax = (mode->height + ctrl->val) & ~1U;
+
+		current_exposure = clamp_t(u32, current_exposure,
+					   IMX585_EXPOSURE_MIN, imx585->vmax - min_shr);
+		__v4l2_ctrl_modify_range(imx585->exposure,
+					 IMX585_EXPOSURE_MIN, imx585->vmax - min_shr, 1,
+					 current_exposure);
+
+		dev_info(imx585->clientdev, "VBLANK=%u -> VMAX=%u\n", ctrl->val, imx585->vmax);
+
+		ret = cci_write(imx585->regmap, IMX585_REG_VMAX, imx585->vmax, NULL);
+		if (ret)
+			dev_err_ratelimited(imx585->clientdev, "VMAX write failed (%d)\n", ret);
+		break;
+	}
+	case V4L2_CID_HBLANK: {
+		u64 pixel_rate = (u64)mode->width * IMX585_PIXEL_RATE;
+		u64 hmax;
+
+		do_div(pixel_rate, mode->min_hmax);
+		hmax = (u64)(mode->width + ctrl->val) * IMX585_PIXEL_RATE;
+		do_div(hmax, pixel_rate);
+		imx585->hmax = (u32)hmax;
+
+		dev_info(imx585->clientdev, "HBLANK=%u -> HMAX=%u\n", ctrl->val, imx585->hmax);
+
+		ret = cci_write(imx585->regmap, IMX585_REG_HMAX, imx585->hmax, NULL);
+		if (ret)
+			dev_err_ratelimited(imx585->clientdev, "HMAX write failed (%d)\n", ret);
+		break;
+	}
+	case V4L2_CID_HFLIP:
+		ret = cci_write(imx585->regmap, IMX585_FLIP_WINMODEH, ctrl->val, NULL);
+		if (ret)
+			dev_err_ratelimited(imx585->clientdev, "HFLIP write failed (%d)\n", ret);
+		break;
+	case V4L2_CID_VFLIP:
+		ret = cci_write(imx585->regmap, IMX585_FLIP_WINMODEV, ctrl->val, NULL);
+		if (ret)
+			dev_err_ratelimited(imx585->clientdev, "VFLIP write failed (%d)\n", ret);
+		break;
+	case V4L2_CID_BRIGHTNESS: {
+		u16 blacklevel = min_t(u32, ctrl->val, 4095);
+
+		ret = cci_write(imx585->regmap, IMX585_REG_BLKLEVEL, blacklevel, NULL);
+		if (ret)
+			dev_err_ratelimited(imx585->clientdev, "BLKLEVEL write failed (%d)\n", ret);
+		break;
+	}
+	case V4L2_CID_WIDE_DYNAMIC_RANGE: /* Handled above */
+		break;
+	case V4L2_CID_IMX585_HDR_DATASEL_TH: {
+		const u16 *th = (const u16 *)ctrl->p_new.p;
+
+		ret = cci_write(imx585->regmap, IMX585_REG_EXP_TH_H, th[0], NULL);
+		if (!ret)
+			ret = cci_write(imx585->regmap, IMX585_REG_EXP_TH_L, th[1], NULL);
+		if (ret)
+			dev_err_ratelimited(imx585->clientdev, "HDR TH write failed (%d)\n", ret);
+		break;
+	}
+	case V4L2_CID_IMX585_HDR_DATASEL_BK:
+		ret = cci_write(imx585->regmap, IMX585_REG_EXP_BK, ctrl->val, NULL);
+		if (ret)
+			dev_err_ratelimited(imx585->clientdev, "HDR BK write failed (%d)\n", ret);
+		break;
+	case V4L2_CID_IMX585_HDR_GRAD_TH: {
+		const u32 *thr = (const u32 *)ctrl->p_new.p;
+
+		ret = cci_write(imx585->regmap, IMX585_REG_CCMP1_EXP, thr[0], NULL);
+		if (!ret)
+			ret = cci_write(imx585->regmap, IMX585_REG_CCMP2_EXP, thr[1], NULL);
+		if (ret)
+			dev_err_ratelimited(imx585->clientdev,
+					    "HDR grad TH write failed (%d)\n", ret);
+		break;
+	}
+	case V4L2_CID_IMX585_HDR_GRAD_COMP_L:
+		ret = cci_write(imx585->regmap, IMX585_REG_ACMP1_EXP, ctrl->val, NULL);
+		if (ret)
+			dev_err_ratelimited(imx585->clientdev,
+					    "HDR grad low write failed (%d)\n", ret);
+		break;
+	case V4L2_CID_IMX585_HDR_GRAD_COMP_H:
+		ret = cci_write(imx585->regmap, IMX585_REG_ACMP2_EXP, ctrl->val, NULL);
+		if (ret)
+			dev_err_ratelimited(imx585->clientdev,
+					    "HDR grad high write failed (%d)\n", ret);
+		break;
+	case V4L2_CID_IMX585_HDR_GAIN:
+		ret = cci_write(imx585->regmap, IMX585_REG_EXP_GAIN, ctrl->val, NULL);
+		if (ret)
+			dev_err_ratelimited(imx585->clientdev,
+					    "HDR gain write failed (%d)\n", ret);
+		break;
+	default:
+		dev_info(imx585->clientdev, "Unhandled ctrl %s: id=0x%x, val=0x%x\n",
+			 ctrl->name, ctrl->id, ctrl->val);
+		break;
+	}
+
+	pm_runtime_put(imx585->clientdev);
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops imx585_ctrl_ops = {
+	.s_ctrl = imx585_set_ctrl,
+};
+
+static const u16 hdr_thresh_def[2] = { 512, 1024 };
+static const struct v4l2_ctrl_config imx585_cfg_datasel_th = {
+	.ops       = &imx585_ctrl_ops,
+	.id        = V4L2_CID_IMX585_HDR_DATASEL_TH,
+	.name      = "HDR Data Selection Threshold",
+	.type      = V4L2_CTRL_TYPE_U16,
+	.min       = 0,
+	.max       = 0x0FFF,
+	.step      = 1,
+	.def       = 0,
+	.dims      = { 2 },
+	.elem_size = sizeof(u16),
+};
+
+static const struct v4l2_ctrl_config imx585_cfg_datasel_bk = {
+	.ops   = &imx585_ctrl_ops,
+	.id    = V4L2_CID_IMX585_HDR_DATASEL_BK,
+	.name  = "HDR Data Blending Mode",
+	.type  = V4L2_CTRL_TYPE_MENU,
+	.max   = ARRAY_SIZE(hdr_data_blender_menu) - 1,
+	.def   = 0,
+	.qmenu = hdr_data_blender_menu,
+};
+
+static const u32 grad_thresh_def[2] = { 500, 11500 };
+static const struct v4l2_ctrl_config imx585_cfg_grad_th = {
+	.ops       = &imx585_ctrl_ops,
+	.id        = V4L2_CID_IMX585_HDR_GRAD_TH,
+	.name      = "HDR Gradient Compression Threshold (16-bit)",
+	.type      = V4L2_CTRL_TYPE_U32,
+	.min       = 0,
+	.max       = 0x1FFFF,
+	.step      = 1,
+	.def       = 0,
+	.dims      = { 2 },
+	.elem_size = sizeof(u32),
+};
+
+static const struct v4l2_ctrl_config imx585_cfg_grad_exp_l = {
+	.ops   = &imx585_ctrl_ops,
+	.id    = V4L2_CID_IMX585_HDR_GRAD_COMP_L,
+	.name  = "HDR Gradient Compression Ratio Low",
+	.type  = V4L2_CTRL_TYPE_MENU,
+	.min   = 0,
+	.max   = ARRAY_SIZE(grad_compression_slope_menu) - 1,
+	.def   = 2,
+	.qmenu = grad_compression_slope_menu,
+};
+
+static const struct v4l2_ctrl_config imx585_cfg_grad_exp_h = {
+	.ops   = &imx585_ctrl_ops,
+	.id    = V4L2_CID_IMX585_HDR_GRAD_COMP_H,
+	.name  = "HDR Gradient Compression Ratio High",
+	.type  = V4L2_CTRL_TYPE_MENU,
+	.min   = 0,
+	.max   = ARRAY_SIZE(grad_compression_slope_menu) - 1,
+	.def   = 6,
+	.qmenu = grad_compression_slope_menu,
+};
+
+static const struct v4l2_ctrl_config imx585_cfg_hdr_gain = {
+	.ops   = &imx585_ctrl_ops,
+	.id    = V4L2_CID_IMX585_HDR_GAIN,
+	.name  = "HDR Gain Adder (dB)",
+	.type  = V4L2_CTRL_TYPE_MENU,
+	.min   = 0,
+	.max   = ARRAY_SIZE(hdr_gain_adder_menu) - 1,
+	.def   = 2,
+	.qmenu = hdr_gain_adder_menu,
+};
+
+static const struct v4l2_ctrl_config imx585_cfg_hcg = {
+	.ops  = &imx585_ctrl_ops,
+	.id   = V4L2_CID_IMX585_HCG_GAIN,
+	.name = "HCG Enable",
+	.type = V4L2_CTRL_TYPE_BOOLEAN,
+	.min  = 0,
+	.max  = 1,
+	.step = 1,
+	.def  = 0,
+};
+
+static int imx585_init_controls(struct imx585 *imx585)
+{
+	struct v4l2_ctrl_handler *hdl = &imx585->ctrl_handler;
+	struct v4l2_fwnode_device_properties props;
+	int ret;
+
+	ret = v4l2_ctrl_handler_init(hdl, 32);
+	if (ret)
+		return ret;
+
+	/* Read-only, updated per mode */
+	imx585->pixel_rate = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
+					       V4L2_CID_PIXEL_RATE,
+					       1, UINT_MAX, 1, 1);
+
+	imx585->link_freq =
+		v4l2_ctrl_new_int_menu(hdl, &imx585_ctrl_ops, V4L2_CID_LINK_FREQ,
+				       0, 0, &link_freqs[imx585->link_freq_idx]);
+	if (imx585->link_freq)
+		imx585->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	imx585->vblank = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
+					   V4L2_CID_VBLANK, 0, 0xFFFFF, 1, 0);
+	imx585->hblank = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
+					   V4L2_CID_HBLANK, 0, 0xFFFF, 1, 0);
+	imx585->blacklevel = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
+					       V4L2_CID_BRIGHTNESS, 0, 0xFFFF, 1,
+					       IMX585_BLKLEVEL_DEFAULT);
+
+	imx585->exposure = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
+					     V4L2_CID_EXPOSURE,
+					     IMX585_EXPOSURE_MIN, IMX585_EXPOSURE_MAX,
+					     IMX585_EXPOSURE_STEP, IMX585_EXPOSURE_DEFAULT);
+
+	imx585->gain = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
+					 IMX585_ANA_GAIN_MIN_NORMAL, IMX585_ANA_GAIN_MAX_NORMAL,
+					 IMX585_ANA_GAIN_STEP, IMX585_ANA_GAIN_DEFAULT);
+
+	imx585->hflip = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
+					  V4L2_CID_HFLIP, 0, 1, 1, 0);
+	imx585->vflip = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
+					  V4L2_CID_VFLIP, 0, 1, 1, 0);
+
+	imx585->hdr_mode = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
+					     V4L2_CID_WIDE_DYNAMIC_RANGE, 0, 1, 1, 0);
+	imx585->datasel_th_ctrl = v4l2_ctrl_new_custom(hdl, &imx585_cfg_datasel_th, NULL);
+	imx585->datasel_bk_ctrl = v4l2_ctrl_new_custom(hdl, &imx585_cfg_datasel_bk, NULL);
+	imx585->gdc_th_ctrl     = v4l2_ctrl_new_custom(hdl, &imx585_cfg_grad_th, NULL);
+	imx585->gdc_exp_ctrl_l  = v4l2_ctrl_new_custom(hdl, &imx585_cfg_grad_exp_l, NULL);
+	imx585->gdc_exp_ctrl_h  = v4l2_ctrl_new_custom(hdl, &imx585_cfg_grad_exp_h, NULL);
+	imx585->hdr_gain_ctrl   = v4l2_ctrl_new_custom(hdl, &imx585_cfg_hdr_gain, NULL);
+	imx585->hcg_ctrl        = v4l2_ctrl_new_custom(hdl, &imx585_cfg_hcg, NULL);
+
+	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);
+	v4l2_ctrl_activate(imx585->gdc_exp_ctrl_l,   imx585->clear_hdr);
+	v4l2_ctrl_activate(imx585->gdc_exp_ctrl_h,   imx585->clear_hdr);
+	v4l2_ctrl_activate(imx585->hdr_gain_ctrl,    imx585->clear_hdr);
+	/* HCG is disabled if ClearHDR is enabled */
+	v4l2_ctrl_activate(imx585->hcg_ctrl,        !imx585->clear_hdr);
+
+	if (hdl->error) {
+		ret = hdl->error;
+		dev_err(imx585->clientdev, "control init failed (%d)\n", ret);
+		goto err_free;
+	}
+
+	ret = v4l2_fwnode_device_parse(imx585->clientdev, &props);
+	if (ret)
+		goto err_free;
+
+	ret = v4l2_ctrl_new_fwnode_properties(hdl, &imx585_ctrl_ops, &props);
+	if (ret)
+		goto err_free;
+
+	/* Set the default value for ClearHDR thresholds */
+	memcpy(imx585->datasel_th_ctrl->p_cur.p, hdr_thresh_def, sizeof(hdr_thresh_def));
+	memcpy(imx585->datasel_th_ctrl->p_new.p, hdr_thresh_def, sizeof(hdr_thresh_def));
+	memcpy(imx585->gdc_th_ctrl->p_cur.p, grad_thresh_def, sizeof(grad_thresh_def));
+	memcpy(imx585->gdc_th_ctrl->p_new.p, grad_thresh_def, sizeof(grad_thresh_def));
+
+	imx585->hdr_mode->flags |= V4L2_CTRL_FLAG_UPDATE | V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+	imx585->sd.ctrl_handler = hdl;
+	return 0;
+
+err_free:
+	v4l2_ctrl_handler_free(hdl);
+	return ret;
+}
+
+static void imx585_free_controls(struct imx585 *imx585)
+{
+	v4l2_ctrl_handler_free(imx585->sd.ctrl_handler);
+}
+
+/* --------------------------------------------------------------------------
+ * Pad ops / formats
+ * --------------------------------------------------------------------------
+ */
+
+static int imx585_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *sd_state,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct imx585 *imx585 = to_imx585(sd);
+	unsigned int entries;
+	const u32 *tbl;
+
+	if (imx585->mono) {
+		if (imx585->clear_hdr) {
+			if (code->index > 1)
+				return -EINVAL;
+			code->code = mono_codes[code->index];
+			return 0;
+		}
+		if (code->index)
+			return -EINVAL;
+		code->code = MEDIA_BUS_FMT_Y12_1X12;
+		return 0;
+	}
+
+	if (imx585->clear_hdr) {
+		tbl = codes_clearhdr;
+		entries = ARRAY_SIZE(codes_clearhdr) / 4;
+	} else {
+		tbl = codes_normal;
+		entries = ARRAY_SIZE(codes_normal) / 4;
+	}
+
+	if (code->index >= entries)
+		return -EINVAL;
+
+	code->code = imx585_get_format_code(imx585, tbl[code->index * 4]);
+	return 0;
+}
+
+static int imx585_enum_frame_size(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *sd_state,
+				  struct v4l2_subdev_frame_size_enum *fse)
+{
+	struct imx585 *imx585 = to_imx585(sd);
+	const struct imx585_mode *mode_list;
+	unsigned int num_modes;
+
+	get_mode_table(imx585, fse->code, &mode_list, &num_modes);
+	if (fse->index >= num_modes)
+		return -EINVAL;
+	if (fse->code != imx585_get_format_code(imx585, fse->code))
+		return -EINVAL;
+
+	fse->min_width  = mode_list[fse->index].width;
+	fse->max_width  = fse->min_width;
+	fse->min_height = mode_list[fse->index].height;
+	fse->max_height = fse->min_height;
+
+	return 0;
+}
+
+static int imx585_set_pad_format(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *sd_state,
+				 struct v4l2_subdev_format *fmt)
+{
+	struct imx585 *imx585 = to_imx585(sd);
+	const struct imx585_mode *mode_list, *mode;
+	unsigned int num_modes;
+	struct v4l2_mbus_framefmt *format;
+
+	get_mode_table(imx585, fmt->format.code, &mode_list, &num_modes);
+	mode = v4l2_find_nearest_size(mode_list, num_modes, width, height,
+				      fmt->format.width, fmt->format.height);
+
+	fmt->format.width        = mode->width;
+	fmt->format.height       = mode->height;
+	fmt->format.field        = V4L2_FIELD_NONE;
+	fmt->format.colorspace   = V4L2_COLORSPACE_RAW;
+	fmt->format.ycbcr_enc    = V4L2_YCBCR_ENC_601;
+	fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	fmt->format.xfer_func    = V4L2_XFER_FUNC_NONE;
+
+	format = v4l2_subdev_state_get_format(sd_state, 0);
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		imx585_set_framing_limits(imx585, mode);
+
+	*format = fmt->format;
+	return 0;
+}
+
+/* --------------------------------------------------------------------------
+ * Stream on/off
+ * --------------------------------------------------------------------------
+ */
+
+static int imx585_enable_streams(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *state, u32 pad,
+				 u64 streams_mask)
+{
+	struct imx585 *imx585 = to_imx585(sd);
+	const struct imx585_mode *mode_list, *mode;
+	struct v4l2_subdev_state *st;
+	struct v4l2_mbus_framefmt *fmt;
+	unsigned int n_modes;
+	int ret;
+
+	ret = pm_runtime_get_sync(imx585->clientdev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(imx585->clientdev);
+		return ret;
+	}
+
+	ret = cci_multi_reg_write(imx585->regmap, common_regs,
+				  ARRAY_SIZE(common_regs), NULL);
+	if (ret) {
+		dev_err(imx585->clientdev, "Failed to write common settings\n");
+		goto err_rpm_put;
+	}
+
+	ret = cci_write(imx585->regmap, IMX585_INCK_SEL, imx585->inck_sel_val, NULL);
+	if (!ret)
+		ret = cci_write(imx585->regmap, IMX585_REG_BLKLEVEL, IMX585_BLKLEVEL_DEFAULT, NULL);
+	if (!ret)
+		ret = cci_write(imx585->regmap, IMX585_DATARATE_SEL,
+				link_freqs_reg_value[imx585->link_freq_idx], NULL);
+	if (ret)
+		goto err_rpm_put;
+
+	ret = cci_write(imx585->regmap, IMX585_LANEMODE,
+			(imx585->lane_count == 2) ? 0x01 : 0x03, NULL);
+	if (ret)
+		goto err_rpm_put;
+
+	/* Mono bin flag (datasheet: 0x01 mono, 0x00 color) */
+	ret = cci_write(imx585->regmap, IMX585_BIN_MODE, imx585->mono ? 0x01 : 0x00, NULL);
+	if (ret)
+		goto err_rpm_put;
+
+	/* Sync configuration */
+	if (imx585->sync_mode == SYNC_INT_FOLLOWER) {
+		dev_info(imx585->clientdev, "Internal sync follower: XVS input\n");
+		cci_write(imx585->regmap, IMX585_REG_EXTMODE, 0x01, NULL);
+		cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x03, NULL); /* XHS out, XVS in */
+		cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x08, NULL); /* disable XVS OUT */
+	} else if (imx585->sync_mode == SYNC_INT_LEADER) {
+		dev_info(imx585->clientdev, "Internal sync leader: XVS/XHS output\n");
+		cci_write(imx585->regmap, IMX585_REG_EXTMODE, 0x00, NULL);
+		cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x00, NULL); /* XHS/XVS out */
+		cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x0A, NULL);
+	} else {
+		dev_info(imx585->clientdev, "Follower: XVS/XHS input\n");
+		cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x0F, NULL); /* inputs */
+		cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x00, NULL);
+	}
+
+	imx585->common_regs_written = true;
+
+	/* Select mode */
+	st  = v4l2_subdev_get_locked_active_state(&imx585->sd);
+	fmt = v4l2_subdev_state_get_format(st, 0);
+
+	get_mode_table(imx585, fmt->code, &mode_list, &n_modes);
+	mode = v4l2_find_nearest_size(mode_list, n_modes, width, height,
+				      fmt->width, fmt->height);
+
+	ret = cci_multi_reg_write(imx585->regmap, mode->reg_list.regs,
+				  mode->reg_list.num_of_regs, NULL);
+	if (ret) {
+		dev_err(imx585->clientdev, "Failed to write mode registers\n");
+		goto err_rpm_put;
+	}
+
+	if (imx585->clear_hdr) {
+		ret = cci_multi_reg_write(imx585->regmap, common_clearHDR_mode,
+					  ARRAY_SIZE(common_clearHDR_mode), NULL);
+		if (ret) {
+			dev_err(imx585->clientdev, "Failed to set ClearHDR regs\n");
+			goto err_rpm_put;
+		}
+		/* 16-bit: linear; 12-bit: enable gradation compression */
+		switch (fmt->code) {
+		case MEDIA_BUS_FMT_SRGGB16_1X16:
+		case MEDIA_BUS_FMT_SGRBG16_1X16:
+		case MEDIA_BUS_FMT_SGBRG16_1X16:
+		case MEDIA_BUS_FMT_SBGGR16_1X16:
+		case MEDIA_BUS_FMT_Y16_1X16:
+			cci_write(imx585->regmap, IMX585_REG_CCMP_EN, 0x00, NULL);
+			cci_write(imx585->regmap, CCI_REG8(0x3023), 0x03, NULL); /* MDBIT 16-bit */
+			break;
+		default:
+			cci_write(imx585->regmap, IMX585_REG_CCMP_EN, 0x01, NULL);
+			break;
+		}
+	} else {
+		ret = cci_multi_reg_write(imx585->regmap, common_normal_mode,
+					  ARRAY_SIZE(common_normal_mode), NULL);
+		if (ret) {
+			dev_err(imx585->clientdev, "Failed to set normal regs\n");
+			goto err_rpm_put;
+		}
+	}
+
+	/* Disable digital clamp */
+	cci_write(imx585->regmap, IMX585_REG_DIGITAL_CLAMP, 0x00, NULL);
+
+	/* Apply user controls after writing the base tables */
+	ret = __v4l2_ctrl_handler_setup(imx585->sd.ctrl_handler);
+	if (ret) {
+		dev_err(imx585->clientdev, "Control handler setup failed\n");
+		goto err_rpm_put;
+	}
+
+	if (imx585->sync_mode != SYNC_EXTERNAL)
+		cci_write(imx585->regmap, IMX585_REG_XMSTA, 0x00, NULL);
+
+	ret = cci_write(imx585->regmap, IMX585_REG_MODE_SELECT, IMX585_MODE_STREAMING, NULL);
+	if (ret)
+		goto err_rpm_put;
+
+	dev_info(imx585->clientdev, "Streaming started\n");
+	usleep_range(IMX585_STREAM_DELAY_US,
+		     IMX585_STREAM_DELAY_US + IMX585_STREAM_DELAY_RANGE_US);
+
+	/* vflip, hflip and HDR cannot change during streaming */
+	__v4l2_ctrl_grab(imx585->vflip, true);
+	__v4l2_ctrl_grab(imx585->hflip, true);
+	__v4l2_ctrl_grab(imx585->hdr_mode, true);
+
+	return 0;
+
+err_rpm_put:
+	pm_runtime_mark_last_busy(imx585->clientdev);
+	pm_runtime_put_autosuspend(imx585->clientdev);
+	return ret;
+}
+
+static int imx585_disable_streams(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *state, u32 pad,
+				  u64 streams_mask)
+{
+	struct imx585 *imx585 = to_imx585(sd);
+	int ret;
+
+	ret = cci_write(imx585->regmap, IMX585_REG_MODE_SELECT, IMX585_MODE_STANDBY, NULL);
+	if (ret)
+		dev_err(imx585->clientdev, "Failed to stop streaming\n");
+
+	__v4l2_ctrl_grab(imx585->vflip, false);
+	__v4l2_ctrl_grab(imx585->hflip, false);
+	__v4l2_ctrl_grab(imx585->hdr_mode, false);
+
+	pm_runtime_mark_last_busy(imx585->clientdev);
+	pm_runtime_put_autosuspend(imx585->clientdev);
+
+	return ret;
+}
+
+/* --------------------------------------------------------------------------
+ * 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");
+		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);
+	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);
+	struct imx585 *imx585 = to_imx585(sd);
+
+	dev_dbg(imx585->clientdev, "power_off\n");
+
+	gpiod_set_value_cansleep(imx585->reset_gpio, 0);
+	regulator_bulk_disable(IMX585_NUM_SUPPLIES, imx585->supplies);
+	clk_disable_unprepare(imx585->xclk);
+
+	return 0;
+}
+
+/* --------------------------------------------------------------------------
+ * Selection / state
+ * --------------------------------------------------------------------------
+ */
+
+static int imx585_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *sd_state,
+				struct v4l2_subdev_selection *sel)
+{
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+		sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
+		return 0;
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+		sel->r.left = 0;
+		sel->r.top = 0;
+		sel->r.width = IMX585_NATIVE_WIDTH;
+		sel->r.height = IMX585_NATIVE_HEIGHT;
+		return 0;
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.left = IMX585_PIXEL_ARRAY_LEFT;
+		sel->r.top = IMX585_PIXEL_ARRAY_TOP;
+		sel->r.width = IMX585_PIXEL_ARRAY_WIDTH;
+		sel->r.height = IMX585_PIXEL_ARRAY_HEIGHT;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int imx585_init_state(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_state *state)
+{
+	struct v4l2_rect *crop;
+	struct imx585 *imx585 = to_imx585(sd);
+	struct v4l2_subdev_format fmt = {
+		.which  = V4L2_SUBDEV_FORMAT_TRY,
+		.pad    = 0,
+		.format = {
+			.code   = imx585->mono ? MEDIA_BUS_FMT_Y12_1X12
+					    : MEDIA_BUS_FMT_SRGGB12_1X12,
+			.width  = supported_modes[0].width,
+			.height = supported_modes[0].height,
+		},
+	};
+
+	imx585_set_pad_format(sd, state, &fmt);
+
+	crop = v4l2_subdev_state_get_crop(state, 0);
+	*crop = supported_modes[0].crop;
+
+	return 0;
+}
+
+/* --------------------------------------------------------------------------
+ * Subdev ops
+ * --------------------------------------------------------------------------
+ */
+
+static const struct v4l2_subdev_video_ops imx585_video_ops = {
+	.s_stream = v4l2_subdev_s_stream_helper,
+};
+
+static const struct v4l2_subdev_pad_ops imx585_pad_ops = {
+	.enum_mbus_code = imx585_enum_mbus_code,
+	.get_fmt        = v4l2_subdev_get_fmt,
+	.set_fmt        = imx585_set_pad_format,
+	.get_selection  = imx585_get_selection,
+	.enum_frame_size = imx585_enum_frame_size,
+	.enable_streams  = imx585_enable_streams,
+	.disable_streams = imx585_disable_streams,
+};
+
+static const struct v4l2_subdev_internal_ops imx585_internal_ops = {
+	.init_state = imx585_init_state,
+};
+
+static const struct v4l2_subdev_ops imx585_subdev_ops = {
+	.video = &imx585_video_ops,
+	.pad   = &imx585_pad_ops,
+};
+
+/* --------------------------------------------------------------------------
+ * Probe / remove
+ * --------------------------------------------------------------------------
+ */
+
+static int imx585_check_hwcfg(struct device *dev, struct imx585 *imx585)
+{
+	struct fwnode_handle *endpoint;
+	struct v4l2_fwnode_endpoint ep = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY,
+	};
+	int ret = -EINVAL;
+	int i;
+
+	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
+	if (!endpoint) {
+		dev_err(dev, "endpoint node not found\n");
+		return -EINVAL;
+	}
+
+	if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep)) {
+		dev_err(dev, "could not parse endpoint\n");
+		goto out_put;
+	}
+
+	if (ep.bus.mipi_csi2.num_data_lanes != 2 &&
+	    ep.bus.mipi_csi2.num_data_lanes != 4) {
+		dev_err(dev, "only 2 or 4 data lanes supported\n");
+		goto out_free;
+	}
+	imx585->lane_count = ep.bus.mipi_csi2.num_data_lanes;
+	dev_info(dev, "Data lanes: %u\n", imx585->lane_count);
+
+	if (!ep.nr_of_link_frequencies) {
+		dev_err(dev, "link-frequency property missing\n");
+		goto out_free;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(link_freqs); i++) {
+		if (link_freqs[i] == ep.link_frequencies[0]) {
+			imx585->link_freq_idx = i;
+			break;
+		}
+	}
+	if (i == ARRAY_SIZE(link_freqs)) {
+		dev_err(dev, "unsupported link frequency: %llu\n",
+			(unsigned long long)ep.link_frequencies[0]);
+		goto out_free;
+	}
+
+	dev_info(dev, "Link speed: %llu Hz\n",
+		 (unsigned long long)ep.link_frequencies[0]);
+
+	ret = 0;
+
+out_free:
+	v4l2_fwnode_endpoint_free(&ep);
+out_put:
+	fwnode_handle_put(endpoint);
+	return ret;
+}
+
+static int imx585_get_regulators(struct imx585 *imx585)
+{
+	unsigned int i;
+
+	for (i = 0; i < IMX585_NUM_SUPPLIES; i++)
+		imx585->supplies[i].supply = imx585_supply_name[i];
+
+	return devm_regulator_bulk_get(imx585->clientdev,
+				       IMX585_NUM_SUPPLIES, imx585->supplies);
+}
+
+static int imx585_check_module_exists(struct imx585 *imx585)
+{
+	int ret;
+	u64 val;
+
+	/* No chip-id register; read a known register as a presence test */
+	ret = cci_read(imx585->regmap, IMX585_REG_BLKLEVEL, &val, NULL);
+	if (ret) {
+		dev_err(imx585->clientdev, "register read failed (%d)\n", ret);
+		return ret;
+	}
+
+	dev_info(imx585->clientdev, "Sensor detected\n");
+	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;
+
+	imx585 = devm_kzalloc(dev, sizeof(*imx585), GFP_KERNEL);
+	if (!imx585)
+		return -ENOMEM;
+
+	v4l2_i2c_subdev_init(&imx585->sd, client, &imx585_subdev_ops);
+	imx585->clientdev = dev;
+
+	imx585->mono = of_device_is_compatible(dev->of_node, "sony,imx585-mono");
+	dev_info(dev, "mono=%d\n", imx585->mono);
+
+	imx585->sync_mode = SYNC_INT_LEADER;
+	if (!device_property_read_string(dev, "sony,sync-mode", &sync_mode)) {
+		if (!strcmp(sync_mode, "internal-follower"))
+			imx585->sync_mode = SYNC_INT_FOLLOWER;
+		else if (!strcmp(sync_mode, "external"))
+			imx585->sync_mode = SYNC_EXTERNAL;
+	}
+	dev_info(dev, "sync-mode: %s\n", sync_mode_menu[imx585->sync_mode]);
+
+	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) {
+		if (imx585_inck_table[i].xclk_hz == imx585->xclk_freq) {
+			imx585->inck_sel_val = imx585_inck_table[i].inck_sel;
+			break;
+		}
+	}
+	if (i == ARRAY_SIZE(imx585_inck_table))
+		return dev_err_probe(dev, -EINVAL, "unsupported XCLK %u Hz\n", imx585->xclk_freq);
+
+	dev_info(dev, "XCLK %u Hz -> INCK_SEL 0x%02x\n",
+		 imx585->xclk_freq, imx585->inck_sel_val);
+
+	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 <will@willwhang.com>");
+MODULE_AUTHOR("Tetsuya Nomura <tetsuya.nomura@soho-enterprise.com>");
+MODULE_DESCRIPTION("Sony IMX585 sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.39.5

Re: [PATCH v2 3/4] media: i2c: imx585: Add Sony IMX585 image-sensor driver

[Krzysztof Kozlowski]

On Sun, Aug 10, 2025 at 11:09:20PM +0100, Will Whang wrote:
> +
> +/* 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 */
> +static void imx585_update_hmax(struct imx585 *imx585)
> +{
> +	const u32 base_4lane = HMAX_table_4lane_4K[imx585->link_freq_idx];
> +	const u32 lane_scale = (imx585->lane_count == 2) ? 2 : 1;
> +	const u32 factor     = base_4lane * lane_scale;
> +	const u32 hdr_scale  = imx585->clear_hdr ? 2 : 1;
> +	unsigned int i;
> +
> +	dev_info(imx585->clientdev, "Update minimum HMAX: base=%u lane_scale=%u hdr_scale=%u\n",
> +		 base_4lane, lane_scale, hdr_scale);

Drop, driver should be silent on success. Could be dev_dbg.


> +
> +	for (i = 0; i < ARRAY_SIZE(supported_modes); ++i) {
> +		u32 h = factor / supported_modes[i].hmax_div;
> +		u32 v = IMX585_VMAX_DEFAULT * hdr_scale;
> +
> +		supported_modes[i].min_hmax = h;
> +		supported_modes[i].min_vmax = v;
> +
> +		dev_info(imx585->clientdev, " mode %ux%u -> VMAX=%u HMAX=%u\n",
> +			 supported_modes[i].width, supported_modes[i].height, v, h);

How many obvious/standard debugs do you need?

> +	}
> +}
> +
> +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,
> +				 IMX585_VMAX_MAX - mode->height,
> +				 1, mode->min_vmax - mode->height);
> +	__v4l2_ctrl_s_ctrl(imx585->vblank, mode->min_vmax - mode->height);
> +
> +	__v4l2_ctrl_modify_range(imx585->exposure, IMX585_EXPOSURE_MIN,
> +				 imx585->vmax - IMX585_SHR_MIN_HDR, 1,
> +				 IMX585_EXPOSURE_DEFAULT);
> +
> +	dev_info(imx585->clientdev, "Framing: VMAX=%u HMAX=%u pixel_rate=%llu\n",
> +		 imx585->vmax, imx585->hmax, pixel_rate);

Here as well...

> +}
> +
> +/* --------------------------------------------------------------------------
> + * 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);
> +			v4l2_ctrl_activate(imx585->gdc_exp_ctrl_h,   imx585->clear_hdr);
> +			v4l2_ctrl_activate(imx585->gdc_exp_ctrl_l,   imx585->clear_hdr);
> +			v4l2_ctrl_activate(imx585->hdr_gain_ctrl,    imx585->clear_hdr);
> +			v4l2_ctrl_activate(imx585->hcg_ctrl,        !imx585->clear_hdr);
> +
> +			/* Disable HCG in ClearHDR mode */
> +			imx585->hcg = imx585->clear_hdr ? 0 : imx585->hcg;
> +			__v4l2_ctrl_s_ctrl(imx585->hcg_ctrl, imx585->hcg);
> +			imx585_update_gain_limits(imx585);
> +			dev_info(imx585->clientdev, "HDR=%u, HCG=%u\n", ctrl->val, imx585->hcg);

Drop

> +
> +			code = imx585->mono ? MEDIA_BUS_FMT_Y12_1X12
> +					    : MEDIA_BUS_FMT_SRGGB12_1X12;
> +			get_mode_table(imx585, code, &mode_list, &num_modes);
> +			mode = v4l2_find_nearest_size(mode_list, num_modes, width, height,
> +						      fmt->width, fmt->height);
> +			imx585_set_framing_limits(imx585, mode);
> +		}
> +		break;
> +	case V4L2_CID_IMX585_HCG_GAIN:
> +		if (!imx585->clear_hdr) {
> +			imx585->hcg = ctrl->val;
> +			imx585_update_gain_limits(imx585);
> +			dev_info(imx585->clientdev, "HCG=%u\n", ctrl->val);

Your driver is way too noisy.

Above comment - drop all dev_info - applies EVERYWHERE, especially to
standard controls. Printing info message, just because someone set some v4l2
control is too noisy and does not warrant dev_dbg, imo.

> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +

> +
> +static int imx585_check_module_exists(struct imx585 *imx585)
> +{
> +	int ret;
> +	u64 val;
> +
> +	/* No chip-id register; read a known register as a presence test */
> +	ret = cci_read(imx585->regmap, IMX585_REG_BLKLEVEL, &val, NULL);
> +	if (ret) {
> +		dev_err(imx585->clientdev, "register read failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	dev_info(imx585->clientdev, "Sensor detected\n");

Drop. It is obvious (already known) due to lack of error message.

> +	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;
> +
> +	imx585 = devm_kzalloc(dev, sizeof(*imx585), GFP_KERNEL);
> +	if (!imx585)
> +		return -ENOMEM;
> +
> +	v4l2_i2c_subdev_init(&imx585->sd, client, &imx585_subdev_ops);
> +	imx585->clientdev = dev;
> +
> +	imx585->mono = of_device_is_compatible(dev->of_node, "sony,imx585-mono");
> +	dev_info(dev, "mono=%d\n", imx585->mono);

Heh? So you debug driver matching and probing?


> +
> +	imx585->sync_mode = SYNC_INT_LEADER;
> +	if (!device_property_read_string(dev, "sony,sync-mode", &sync_mode)) {
> +		if (!strcmp(sync_mode, "internal-follower"))
> +			imx585->sync_mode = SYNC_INT_FOLLOWER;
> +		else if (!strcmp(sync_mode, "external"))
> +			imx585->sync_mode = SYNC_EXTERNAL;
> +	}
> +	dev_info(dev, "sync-mode: %s\n", sync_mode_menu[imx585->sync_mode]);

No, drop. This is STATIC coming from DT. It will be always like that,
what is the point of debugging DT?

> +
> +	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) {
> +		if (imx585_inck_table[i].xclk_hz == imx585->xclk_freq) {
> +			imx585->inck_sel_val = imx585_inck_table[i].inck_sel;
> +			break;
> +		}
> +	}
> +	if (i == ARRAY_SIZE(imx585_inck_table))
> +		return dev_err_probe(dev, -EINVAL, "unsupported XCLK %u Hz\n", imx585->xclk_freq);
> +
> +	dev_info(dev, "XCLK %u Hz -> INCK_SEL 0x%02x\n",
> +		 imx585->xclk_freq, imx585->inck_sel_val);

No, drop

> +
> +	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 <will@willwhang.com>");
> +MODULE_AUTHOR("Tetsuya Nomura <tetsuya.nomura@soho-enterprise.com>");
> +MODULE_DESCRIPTION("Sony IMX585 sensor driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.39.5
> 

Re: [PATCH v2 3/4] media: i2c: imx585: Add Sony IMX585 image-sensor driver

[Krzysztof Kozlowski]

On Sun, Aug 10, 2025 at 11:09:20PM +0100, Will Whang wrote:
> +
> +/* --------------------------------------------------------------------------
> + * 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");
> +		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"?

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

Re: [PATCH v2 3/4] media: i2c: imx585: Add Sony IMX585 image-sensor driver

[Will Whang]

On Mon, Aug 11, 2025 at 1:06 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Sun, Aug 10, 2025 at 11:09:20PM +0100, Will Whang wrote:
> > +
> > +/* --------------------------------------------------------------------------
> > + * 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");
> > +             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"?
>
> > +     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);
> > +     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.

I'm simply writing the code following the datasheets here.


> Best regards,
> Krzysztof
>

Re: [PATCH v2 3/4] media: i2c: imx585: Add Sony IMX585 image-sensor driver

[Krzysztof Kozlowski]

On 16/08/2025 21:44, Will Whang wrote:
> On Mon, Aug 11, 2025 at 1:06 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On Sun, Aug 10, 2025 at 11:09:20PM +0100, Will Whang wrote:
>>> +
>>> +/* --------------------------------------------------------------------------
>>> + * 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");
>>> +             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"?
>>
>>> +     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);
>>> +     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

This is not how resets work. Logical reset value high means it is
asserted and device does not work.

Read carefully your datasheet. Because if you claim above this is not a
reset line, but some other.

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

Great, so 10 year old buggy code is the example, but all other - 90% of
correct drivers - are not?

> 
> 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.

I explained to you. You are mixing logical level with line level. This
is the problem, because (again I said it):

It does not work.

Your code or your DTS is incorrect.



Best regards,
Krzysztof

[PATCH v2 4/4] media: docs: Add userspace-API guide for the IMX585 driver

[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 <will@willwhang.com>
---
 .../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

diff --git a/Documentation/userspace-api/media/drivers/imx585.rst b/Documentation/userspace-api/media/drivers/imx585.rst
new file mode 100644
index 000000000..9f7c16f30
--- /dev/null
+++ b/Documentation/userspace-api/media/drivers/imx585.rst
@@ -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
+        * - ``2``
+          - +12 dB *(default)*
+        * - ``3``
+          - +18 dB
+        * - ``4``
+          - +24 dB
+        * - ``5``
+          - +29.1 dB
+
+``V4L2_CID_IMX585_HCG_GAIN``  (boolean)
+    Toggle **High-Conversion-Gain** mode.
+
+    *0 = LCG (default), 1 = HCG.*
+
+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.
diff --git a/Documentation/userspace-api/media/drivers/index.rst b/Documentation/userspace-api/media/drivers/index.rst
index d706cb47b..87912acfb 100644
--- a/Documentation/userspace-api/media/drivers/index.rst
+++ b/Documentation/userspace-api/media/drivers/index.rst
@@ -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
diff --git a/MAINTAINERS b/MAINTAINERS
index 175f5236a..42e32b6ba 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23183,6 +23183,7 @@ M:	Will Whang <will@willwhang.com>
 L:	linux-media@vger.kernel.org
 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

Re: [PATCH v2 4/4] media: docs: Add userspace-API guide for the IMX585 driver

[Dave Stevenson]

Hi Will

Thanks for the patches.

On Sun, 10 Aug 2025 at 23:11, Will Whang <will@willwhang.com> wrote:
>
> 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 <will@willwhang.com>
> ---
>  .../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
>
> diff --git a/Documentation/userspace-api/media/drivers/imx585.rst b/Documentation/userspace-api/media/drivers/imx585.rst
> new file mode 100644
> index 000000000..9f7c16f30
> --- /dev/null
> +++ b/Documentation/userspace-api/media/drivers/imx585.rst
> @@ -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
> +        * - ``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?

> +    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

> +
> +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.
> diff --git a/Documentation/userspace-api/media/drivers/index.rst b/Documentation/userspace-api/media/drivers/index.rst
> index d706cb47b..87912acfb 100644
> --- a/Documentation/userspace-api/media/drivers/index.rst
> +++ b/Documentation/userspace-api/media/drivers/index.rst
> @@ -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
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 175f5236a..42e32b6ba 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23183,6 +23183,7 @@ M:      Will Whang <will@willwhang.com>
>  L:     linux-media@vger.kernel.org
>  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
>
>

Re: [PATCH v2 4/4] media: docs: Add userspace-API guide for the IMX585 driver

[Will Whang]

Hi Dave,
Reply inline.
Thanks,
Will Whang

On Mon, Aug 11, 2025 at 7:24 AM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Will
>
> Thanks for the patches.
>
> On Sun, 10 Aug 2025 at 23:11, Will Whang <will@willwhang.com> wrote:
> >
> > 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 <will@willwhang.com>
> > ---
> >  .../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
> >
> > diff --git a/Documentation/userspace-api/media/drivers/imx585.rst b/Documentation/userspace-api/media/drivers/imx585.rst
> > new file mode 100644
> > index 000000000..9f7c16f30
> > --- /dev/null
> > +++ b/Documentation/userspace-api/media/drivers/imx585.rst
> > @@ -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
> > +        * - ``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?
>
> > +    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.
>
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

> 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
Yeah I think Starvis 2 series all have this capability.
>
> > +
> > +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.
> > diff --git a/Documentation/userspace-api/media/drivers/index.rst b/Documentation/userspace-api/media/drivers/index.rst
> > index d706cb47b..87912acfb 100644
> > --- a/Documentation/userspace-api/media/drivers/index.rst
> > +++ b/Documentation/userspace-api/media/drivers/index.rst
> > @@ -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
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 175f5236a..42e32b6ba 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -23183,6 +23183,7 @@ M:      Will Whang <will@willwhang.com>
> >  L:     linux-media@vger.kernel.org
> >  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
> >
> >

Re: [PATCH v2 4/4] media: docs: Add userspace-API guide for the IMX585 driver

[Laurent Pinchart]

Hi Will,

On Mon, Aug 11, 2025 at 07:31:13PM -0700, Will Whang wrote:
> On Mon, Aug 11, 2025 at 7:24 AM Dave Stevenson wrote:
> > On Sun, 10 Aug 2025 at 23:11, Will Whang wrote:
> > >
> > > 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 <will@willwhang.com>
> > > ---
> > >  .../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
> > >
> > > diff --git a/Documentation/userspace-api/media/drivers/imx585.rst b/Documentation/userspace-api/media/drivers/imx585.rst
> > > new file mode 100644
> > > index 000000000..9f7c16f30
> > > --- /dev/null
> > > +++ b/Documentation/userspace-api/media/drivers/imx585.rst
> > > @@ -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
> > > +        * - ``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?
> >
> > > +    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.
> 
> 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.

> 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

We should really try to standardize V4L2 controls for HDR support in
sensors. See https://lore.kernel.org/linux-media/20250710220544.89066-1-mirela.rabulea@nxp.com
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.

> > 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
> 
> Yeah I think Starvis 2 series all have this capability.
> 
> > > +
> > > +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.
> > > diff --git a/Documentation/userspace-api/media/drivers/index.rst b/Documentation/userspace-api/media/drivers/index.rst
> > > index d706cb47b..87912acfb 100644
> > > --- a/Documentation/userspace-api/media/drivers/index.rst
> > > +++ b/Documentation/userspace-api/media/drivers/index.rst
> > > @@ -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
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 175f5236a..42e32b6ba 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -23183,6 +23183,7 @@ M:      Will Whang <will@willwhang.com>
> > >  L:     linux-media@vger.kernel.org
> > >  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

Re: [PATCH v2 4/4] media: docs: Add userspace-API guide for the IMX585 driver

[Will Whang]

Hi Laurent,
Reply inline.

Thanks,
Will Whang

On Tue, Aug 12, 2025 at 4:28 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Will,
>
> On Mon, Aug 11, 2025 at 07:31:13PM -0700, Will Whang wrote:
> > On Mon, Aug 11, 2025 at 7:24 AM Dave Stevenson wrote:
> > > On Sun, 10 Aug 2025 at 23:11, Will Whang wrote:
> > > >
> > > > 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 <will@willwhang.com>
> > > > ---
> > > >  .../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
> > > >
> > > > diff --git a/Documentation/userspace-api/media/drivers/imx585.rst b/Documentation/userspace-api/media/drivers/imx585.rst
> > > > new file mode 100644
> > > > index 000000000..9f7c16f30
> > > > --- /dev/null
> > > > +++ b/Documentation/userspace-api/media/drivers/imx585.rst
> > > > @@ -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
> > > > +        * - ``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?
> > >
> > > > +    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.
> >
> > 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.
>
From my understanding, DOL-HDR (Digital Overlapping), multi-frame HDR
or stagger HDR are all using different exposure times to separate
high/low brightness image frames. DCG with multiframe HDR is kinda
weird because the main reason to have DCG is that you can have two
gains with one exposure/charge so both high/low gain images are
exactly from the same sample and not time delayed. If multi-frame HDR
is acceptable then using two exposure times is way easier to implement
then DCG just for multi-frame HDR.

Now that I think about it and see the register naming, I think HCG
control is for a DCG enabled sensor to select between high/low
conversion gain channels to use for the SDR result.

> > 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
>
> We should really try to standardize V4L2 controls for HDR support in
> sensors. See https://lore.kernel.org/linux-media/20250710220544.89066-1-mirela.rabulea@nxp.com
> for an attempt at doing so (for some of the controls at least). Could
> you share your feedback in that mail thread ?
>

My counter argument will be that HDR controls are way too diverse
vendor to vendor and even sensor to sensor that having a sensor-series
based HDR controls would suit better.

I think it might be better to focus on how to handle multiple image
channels from one sensor because what makes this (IMX585) sensor
unique (and thus, the implementations will be somewhat unique) is the
capability to merge the image on the sensor directly and output as a
12bit compressed or 16bit linear image. Without the ability to handle
multiple MIPI virtual channels for far more common DOL-HDR or
multi-frame based HDR sensors, there isn't a lot of need to
standardize V4L2 HDR controls.

> 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.
>

Can you recommend a way to handle the blending controls from userspace?
I will be glad to move these controls to module parameters then V4L2
controls if we don't want per-sensor code. In the worst case I'll
remove the ClearHDR functions and HCG controls for this driver so the
patches go through.

> 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.
>

Let me describe the data flow here:

           +-----------+              +-----------+
           | High gain |              | Low  gain |
           +-----------+              +-----------+
              [12b]  |                   [12b]  |
                     v                           v
         +-----------------------------------------------+
         |              Linear merging stage          |
         |           ( DATA_SEL, DATA_BK)        |
         +-----------------------------------------------+
                                   |
                         (16bit output)
                                   |
                                   v
         +-----------------------------------------------+
         |       Gradient Compression stage      |
         |        (GRAD_TH, GRAD_COMP)      |
         +-----------------------------------------------+
                                   |
                         (12bit output)
                                   |
                                   v

So both 12 bit compressed and 16 bit linear are all based on the HDR
result and even though this is exactly the same as companding, I still
think we need sensor dependent control here.
From what I can see from the public datasheets, OX03C10 has a similar
data compression algorithm called piecewise linear (PWL), which acts
on 24bit(!) 3-frame HDR results with 32(!) curves.
On the Sony side I don't see this capability on any sensor other than IMX585.

> > > 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
> >
> > Yeah I think Starvis 2 series all have this capability.
> >
> > > > +
> > > > +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.
> > > > diff --git a/Documentation/userspace-api/media/drivers/index.rst b/Documentation/userspace-api/media/drivers/index.rst
> > > > index d706cb47b..87912acfb 100644
> > > > --- a/Documentation/userspace-api/media/drivers/index.rst
> > > > +++ b/Documentation/userspace-api/media/drivers/index.rst
> > > > @@ -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
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 175f5236a..42e32b6ba 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -23183,6 +23183,7 @@ M:      Will Whang <will@willwhang.com>
> > > >  L:     linux-media@vger.kernel.org
> > > >  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