media: i2c: Add onsemi AR0234 camera sensor driver
Subsystems (3)
v3 cover media: i2c: Add onsemi AR0234 camera sensor driver
This series adds a driver for the onsemi AR0234 CMOS image sensor. The AR0234 is a 1/2.6-inch global-shutter sensor with a 1940x1220 pixel array, capable of 1920x1200 resolution at up to 120 fps. It supports MIPI CSI-2 output with 1 to 4 data lanes, raw Bayer (8/10-bit) and monochrome formats, as well as DPCM 10->8 compression.
The driver has been tested with 2 and 4 lanes on an ARM64 Rockchip RK3568 platform with a 27 MHz external clock. Both 8-bit and 10-bit raw Bayer modes are functional.
Notes:
- 1-lane mode is currently disabled; attempts to use it produced no valid image. Further investigation is needed.
- The driver uses a private streaming flag to protect cropping changes during streaming. Is this the recommended approach, or should we rely solely on the subdev state?
- The DPCM (10->8 compression) mode is included in the code but could not be tested due to lack of suitable hardware; any testing help would be appreciated.
Changes since v2:
- Added devicetree binding documentation for the onsemi AR0234 sensor.
- Added support for 8-bit raw Bayer output (verified working).
- Added DPCM 10->8 compression mode (untested, included for completeness).
- Reworked mode handling: each mode now specifies input/output bpp, DPCM flag, MIPI data type, and link frequency index.
- Reworked link frequency handling: the driver now accepts any valid link frequencies from the device tree. It expects two frequencies - one for 8-bit mode and one for 10-bit mode - but does not enforce a fixed set; frequencies are validated by attempting PLL calculation. This makes the driver compatible with a wider range of system configurations.
- Updated ar0234_calculate_pll() to use a temporary structure and update cached PLL only on success.
Changes since v1:
- Improved error handling: use cci_write() with &ret chaining for sequential register writes, as suggested by Isaac Scott.
- Refactored format and cropping support:
Replaced static format list with dynamic cropping rectangle (struct v4l2_rect crop).
Implemented get_selection and set_selection for V4L2_SEL_TGT_CROP, allowing runtime selection of the active sensor area.
- Migrated to modern streaming model: replaced s_stream with enable_streams/disable_streams using v4l2_subdev_s_stream_helper.
- Corrected blanking constants: replaced ambiguous AR0234_HBLANK_DEF with AR0234_LINE_LENGTH_PCK_MIN; updated min/max ranges.
- Added ACPI match table (untested).
- Style fixes.
Any further comments or test results would be greatly appreciated.
Alexander Shiyan (2):
dt-bindings: media: i2c: Add onsemi AR0234 image sensor binding
media: i2c: Add onsemi AR0234 image sensor driver
.../bindings/media/i2c/onnn,ar0234.yaml | 109 ++ drivers/media/i2c/Kconfig | 12 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ar0234.c | 1309 +++++++++++++++++ 4 files changed, 1431 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml create mode 100644 drivers/media/i2c/ar0234.c
Signature · 1 line
2.52.0
RFC PATCH v3 1/2 dt-bindings: media: i2c: Add onsemi AR0234 image sensor binding
Add devicetree binding for the onsemi AR0234 CMOS image sensor.
--- .../bindings/media/i2c/onnn,ar0234.yaml | 109 ++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml
Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml new file +109 -0
@@ -0,0 +1,109 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/onnn,ar0234.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ON Semiconductor AR0234 1/2.6-inch CMOS Digital Image Sensor + +description: + The AR0234 is a 1/2.6-inch CMOS digital image sensor with a pixel + array of 1940x1220 pixels, capable of 1920x1200 resolution at up + to 120 fps. It supports MIPI CSI-2 output with 1, 2, or 4 data lanes, + and raw Bayer (8/10-bit) or monochrome output. + +properties: + compatible: + const: onnn,ar0234cs + + reg: + description: I2C device address + maxItems: 1 + + clocks: + description: Reference clock (external clock) input + maxItems: 1 + + reset-gpios: + description: Reset pin, usually active low (if needed) + maxItems: 1 + + vaa-supply: + description: Analog (2.8V) supply regulator + + vdd-supply: + description: Digital Core (1.2V) supply regulator + + vddio-supply: + description: I/O (1.8V-2.8V) supply regulator + + port: + $ref: /schemas/graph.yaml#/$defs/port-base + description: CSI-2 transmitter port + additionalProperties: false + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + properties: + data-lanes: + description: + Number of MIPI CSI-2 data lanes. Supported values: 2, 4. + minItems: 2 + maxItems: 4 + items: + enum: [1, 2, 3, 4] + + link-frequencies: + description: + Allowed MIPI link frequencies in Hz. The driver expects two + frequencies: one for 8-bit and one for 10-bit modes, + typically 360 MHz and 450 MHz, but any frequency supported + by the sensor may be used. + minItems: 2 + maxItems: 2 + items: + minimum: 360000000 + maximum: 450000000 + + required: + - data-lanes + - link-frequencies + + required: + - endpoint + +required: + - compatible + - reg + - clocks + - port + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + camera@10 { + compatible = "onnn,ar0234cs"; + reg = <0x10>; + clocks = <&clk_ext_camera>; + + vaa-supply = <®_cam_vaa>; + vdd-supply = <®_cam_vdd>; + vddio-supply = <®_cam_vddio>; + + reset-gpios = <&gpio 42 GPIO_ACTIVE_LOW>; + + port { + ar0234_ep: endpoint { + data-lanes = <1 2 3 4>; + link-frequencies = /bits/ 64 <360000000 450000000>; + }; + }; + }; + }; +...
Signature · 1 line
2.52.0
Re: [RFC PATCH v3 1/2] dt-bindings: media: i2c: Add onsemi AR0234 image sensor binding
Hi Alexander,
Thank you for the patch.
Quoted 21 lines · Alexander Shiyan
Add devicetree binding for the onsemi AR0234 CMOS image sensor.
Signed-off-by Alexander Shiyan <[email protected]>--- .../bindings/media/i2c/onnn,ar0234.yaml | 109 ++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yamlDocumentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml new file +7
-0@@ -0,0 +1,109 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/onnn,ar0234.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ON Semiconductor AR0234 1/2.6-inch CMOS Digital Image Sensor
Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml line 17
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml new file +10
-0+ +description: + The AR0234 is a 1/2.6-inch CMOS digital image sensor with a pixel + array of 1940x1220 pixels, capable of 1920x1200 resolution at up + to 120 fps. It supports MIPI CSI-2 output with 1, 2, or 4 data lanes, + and raw Bayer (8/10-bit) or monochrome output. + +properties: + compatible: + const: onnn,ar0234cs
Should we define separate compatible strings for the mono and colour variants ? I know you identify the variant at runtime in the driver, but avoid I2C communication at boot time can be beneficial (to reduce boot time, and also to avoid flashing the privacy LED on systems that have one, albeit the latter is probably less applicable to the AR0234).
Quoted 35 lines
Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml new file +35
-0+ + reg: + description: I2C device address + maxItems: 1 + + clocks: + description: Reference clock (external clock) input + maxItems: 1 + + reset-gpios: + description: Reset pin, usually active low (if needed) + maxItems: 1 + + vaa-supply: + description: Analog (2.8V) supply regulator + + vdd-supply: + description: Digital Core (1.2V) supply regulator + + vddio-supply: + description: I/O (1.8V-2.8V) supply regulator + + port: + $ref: /schemas/graph.yaml#/$defs/port-base + description: CSI-2 transmitter port + additionalProperties: false + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + properties: + data-lanes: + description: + Number of MIPI CSI-2 data lanes. Supported values: 2, 4. + minItems: 2
Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml line 62
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml new file +10
-0+ maxItems: 4 + items: + enum: [1, 2, 3, 4] + + link-frequencies: + description: + Allowed MIPI link frequencies in Hz. The driver expects two + frequencies: one for 8-bit and one for 10-bit modes, + typically 360 MHz and 450 MHz, but any frequency supported + by the sensor may be used.
What the driver supports isn't relevant for the DT bindings. The frequencies should only be limited here to the range supported by the device, regardless of the current driver implementation.
Quoted 47 lines
Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml new file +47
-0+ minItems: 2 + maxItems: 2 + items: + minimum: 360000000 + maximum: 450000000 + + required: + - data-lanes + - link-frequencies + + required: + - endpoint + +required: + - compatible + - reg + - clocks + - port + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + camera@10 { + compatible = "onnn,ar0234cs"; + reg = <0x10>; + clocks = <&clk_ext_camera>; + + vaa-supply = <®_cam_vaa>; + vdd-supply = <®_cam_vdd>; + vddio-supply = <®_cam_vddio>; + + reset-gpios = <&gpio 42 GPIO_ACTIVE_LOW>; + + port { + ar0234_ep: endpoint { + data-lanes = <1 2 3 4>; + link-frequencies = /bits/ 64 <360000000 450000000>; + }; + }; + }; + }; +...
Signature · 3 lines
Regards, Laurent Pinchart
Re: [RFC PATCH v3 1/2] dt-bindings: media: i2c: Add onsemi AR0234 image sensor binding
Hello Laurent.
Quoted 30 lines · Alexander Shiyan
Quoted 29 lines
Add devicetree binding for the onsemi AR0234 CMOS image sensor.
Signed-off-by Alexander Shiyan <[email protected]>--- .../bindings/media/i2c/onnn,ar0234.yaml | 109 ++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yamlDocumentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml new file +15
-0@@ -0,0 +1,109 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/onnn,ar0234.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ON Semiconductor AR0234 1/2.6-inch CMOS Digital Image Sensor + +description: + The AR0234 is a 1/2.6-inch CMOS digital image sensor with a pixel + array of 1940x1220 pixels, capable of 1920x1200 resolution at up + to 120 fps. It supports MIPI CSI-2 output with 1, 2, or 4 data lanes, + and raw Bayer (8/10-bit) or monochrome output. + +properties:
Quoted 8 lines
Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml line 17
Quoted 2 lines
Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml new file +2
-0+ compatible: + const: onnn,ar0234csShould we define separate compatible strings for the mono and colour variants ? I know you identify the variant at runtime in the driver, but avoid I2C communication at boot time can be beneficial (to reduce boot time, and also to avoid flashing the privacy LED on systems that have one, albeit the latter is probably less applicable to the AR0234).
We could do it like this — Color: ar0234cssc, Mono: ar0234cssm. But the current approach is more universal...
Could we add two compatible strings and keep the base one for auto-detection? For detection, it would still be good to check the identifier anyway... Or just add two compatible strings but detect connected variant in any case?
Thanks!
Re: [RFC PATCH v3 1/2] dt-bindings: media: i2c: Add onsemi AR0234 image sensor binding
Quoted 39 lines · Alexander Shiyan
Quoted 30 lines · Alexander Shiyan
Quoted 29 lines
Add devicetree binding for the onsemi AR0234 CMOS image sensor.
Signed-off-by Alexander Shiyan <[email protected]>--- .../bindings/media/i2c/onnn,ar0234.yaml | 109 ++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yamlDocumentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml new file +15
-0@@ -0,0 +1,109 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/onnn,ar0234.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ON Semiconductor AR0234 1/2.6-inch CMOS Digital Image Sensor + +description: + The AR0234 is a 1/2.6-inch CMOS digital image sensor with a pixel + array of 1940x1220 pixels, capable of 1920x1200 resolution at up + to 120 fps. It supports MIPI CSI-2 output with 1, 2, or 4 data lanes, + and raw Bayer (8/10-bit) or monochrome output. + +properties:Quoted 8 lines
Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml line 17
Quoted 2 lines
Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml new file +2
-0+ compatible: + const: onnn,ar0234csShould we define separate compatible strings for the mono and colour variants ? I know you identify the variant at runtime in the driver, but avoid I2C communication at boot time can be beneficial (to reduce boot time, and also to avoid flashing the privacy LED on systems that have one, albeit the latter is probably less applicable to the AR0234).
Quoted 5 lines
We could do it like this — Color: ar0234cssc, Mono: ar0234cssm. But the current approach is more universal...
Could we add two compatible strings and keep the base one for auto-detection? For detection, it would still be good to check the identifier anyway... Or just add two compatible strings but detect connected variant in any case?
I see multiple use cases:
1. You know at build time that you have an AR0234CS, but the model (mono or colour) is only known at runtime (e.g. a product exists in mono and colour options, with the options being otherwise identical).
This can be implemented with a single compatible string "onnn,ar0234cs" and a runtime check of the model in the driver, *or* with two compatible strings for the two models and a runtime check in the boot loader that will set the correct compatible string.
2. You know at build time what exact camera module you have, and you want to avoid powering the sensor up at boot time (e.g. boot time optimization, avoiding privacy LED flashing, ...).
This requires two separate compatible strings for the two models.
3. You know at build time what exact camera module you have, and you want a runtime sanity check.
This requires two separate compatible strings for the two models.
In order to cover all those use cases, we could use
properties:
compatible:
enum:
- onnn,ar0234cssc
- onnn,ar0234cssm
- onnn,ar0234cs
The first two compatible strings would cover use case 1 with the boot loader detection, use case 2, and use case 3. The last compatible string would cover use case 1 without the boot loader detection. This is waht the sony,imx296.yaml binding does. The sony,imx290.yaml binding, on the other hand, has deprecated the generic compatible string.
We could also use
properties:
compatible:
oneOf:
- const: onnn,ar0234cs
- items:
- enum:
- onnn,ar0234cssc
- onnn,ar0234cssm
- const: onnn,ar0234cs
if we want a fallback compatible string, which could allow systems that only case about use case 1 without boot loader detection to only match on "onnn,ar0234cs" in their driver. I don't think we have any such bindings for image sensors.
I don't think we've decided on a recommended practice.
Signature · 3 lines
Regards, Laurent Pinchart
Re: [RFC PATCH v3 1/2] dt-bindings: media: i2c: Add onsemi AR0234 image sensor binding
Quoted 48 lines · Laurent Pinchart
Quoted 39 lines · Alexander Shiyan
Quoted 30 lines · Alexander Shiyan
Quoted 29 lines
Add devicetree binding for the onsemi AR0234 CMOS image sensor.
Signed-off-by Alexander Shiyan <[email protected]>--- .../bindings/media/i2c/onnn,ar0234.yaml | 109 ++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yamlDocumentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml new file +15
-0@@ -0,0 +1,109 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/onnn,ar0234.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ON Semiconductor AR0234 1/2.6-inch CMOS Digital Image Sensor + +description: + The AR0234 is a 1/2.6-inch CMOS digital image sensor with a pixel + array of 1940x1220 pixels, capable of 1920x1200 resolution at up + to 120 fps. It supports MIPI CSI-2 output with 1, 2, or 4 data lanes, + and raw Bayer (8/10-bit) or monochrome output. + +properties:Quoted 8 lines
Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml line 17
Quoted 2 lines
Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml new file +2
-0+ compatible: + const: onnn,ar0234csShould we define separate compatible strings for the mono and colour variants ? I know you identify the variant at runtime in the driver, but avoid I2C communication at boot time can be beneficial (to reduce boot time, and also to avoid flashing the privacy LED on systems that have one, albeit the latter is probably less applicable to the AR0234).
Quoted 5 lines
We could do it like this — Color: ar0234cssc, Mono: ar0234cssm. But the current approach is more universal...
Could we add two compatible strings and keep the base one for auto-detection? For detection, it would still be good to check the identifier anyway... Or just add two compatible strings but detect connected variant in any case?I see multiple use cases:
Quoted 8 lines
1. You know at build time that you have an AR0234CS, but the model (mono or colour) is only known at runtime (e.g. a product exists in mono and colour options, with the options being otherwise identical).
This can be implemented with a single compatible string "onnn,ar0234cs" and a runtime check of the model in the driver, *or* with two compatible strings for the two models and a runtime check in the boot loader that will set the correct compatible string.
You can kinda have both of these, runtime detection on a common fallback compatible, and the bootloader selecting the right dtb/overlay etc with specific ones.
Quoted 11 lines
2. You know at build time what exact camera module you have, and you want to avoid powering the sensor up at boot time (e.g. boot time optimization, avoiding privacy LED flashing, ...).
This requires two separate compatible strings for the two models.
3. You know at build time what exact camera module you have, and you want a runtime sanity check.
This requires two separate compatible strings for the two models.
Maybe in media land this kind of thing is okay, because the likelihood of compatibility between models is low, but overall I dislike this kind of thing as it causes problems when using fallbacks. Perhaps here that doesn't matter at all because you're only doing colour/mono differentiation, not validating that this is an ar0234cs?
Quoted 27 lines
In order to cover all those use cases, we could use
properties:
compatible:
enum:
- onnn,ar0234cssc
- onnn,ar0234cssm
- onnn,ar0234csThe first two compatible strings would cover use case 1 with the boot loader detection, use case 2, and use case 3. The last compatible string would cover use case 1 without the boot loader detection. This is waht the sony,imx296.yaml binding does. The sony,imx290.yaml binding, on the other hand, has deprecated the generic compatible string.
We could also use
properties:
compatible:
oneOf:
- const: onnn,ar0234cs
- items:
- enum:
- onnn,ar0234cssc
- onnn,ar0234cssm
- const: onnn,ar0234cs
Quoted 6 lines
if we want a fallback compatible string, which could allow systems that only case about use case 1 without boot loader detection to only match on "onnn,ar0234cs" in their driver. I don't think we have any such bindings for image sensors.
I don't think we've decided on a recommended practice.
I definitely don't like the first suggestion here, the second is more acceptable. With the first, it definitely seems just like abusing the compatible property to have different behaviour for the exact same hardware.
| properties: | compatible: | items: | - enum: | - onnn,ar0234cssc | - onnn,ar0234cssm | - const: onnn,ar0234cs
Can also do this, but have all three devices in the driver, to promote completely describing a device while also supporting lazy/cheap vendors that want to reuse a dtb rather than implement two images or a selection method. Obviously it'd produce dtbs_check warnings, but it is not as if those sorts of vendors care about dtbs_check.
I mention vendors specifically here, since an end user can probably update their dtb once they figure out which device they actually have, which I think shouldn't be very difficult!
RFC PATCH v3 2/2 media: i2c: Add onsemi AR0234 image sensor driver
In reply to Alexander ShiyanAdd driver for the onsemi AR0234 CMOS image sensor.
--- drivers/media/i2c/Kconfig | 12 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ar0234.c | 1309 ++++++++++++++++++++++++++++++++++++ 3 files changed, 1322 insertions(+) create mode 100644 drivers/media/i2c/ar0234.c
drivers/media/i2c/Kconfig +12 -0
@@ -51,6 +51,18 @@ config VIDEO_ALVIUM_CSI2 To compile this driver as a module, choose M here: the module will be called alvium-csi2. +config VIDEO_AR0234 + tristate "onsemi AR0234 sensor support" + depends on ACPI || OF || COMPILE_TEST + select V4L2_CCI_I2C + select VIDEO_CCS_PLL + help + This is a Video4Linux2 sensor driver for the onsemi + AR0234 camera. + + To compile this driver as a module, choose M here: the + module will be called ar0234. + config VIDEO_AR0521 tristate "ON Semiconductor AR0521 sensor support" help
drivers/media/i2c/Makefile +1 -0
@@ -19,6 +19,7 @@ obj-$(CONFIG_VIDEO_AK7375) += ak7375.o obj-$(CONFIG_VIDEO_AK881X) += ak881x.o obj-$(CONFIG_VIDEO_ALVIUM_CSI2) += alvium-csi2.o obj-$(CONFIG_VIDEO_APTINA_PLL) += aptina-pll.o +obj-$(CONFIG_VIDEO_AR0234) += ar0234.o obj-$(CONFIG_VIDEO_AR0521) += ar0521.o obj-$(CONFIG_VIDEO_BT819) += bt819.o obj-$(CONFIG_VIDEO_BT856) += bt856.o
drivers/media/i2c/ar0234.c new file +1309 -0
1,309 lines not on this page · raw message
Signature · 1 line
2.52.0
Re: [RFC PATCH v3 2/2] media: i2c: Add onsemi AR0234 image sensor driver
Hi Alexander,
Quoted 37 lines · Alexander Shiyan
drivers/media/i2c/ar0234.c new file +37
-0+static int ar0234_identify_module (struct ar0234 *ar0234) +{ +u64 id, rev; +int ret; + + ret =cci_read (ar0234->regmap,AR0234_REG_CHIP_VERSION , &id,NULL ); + ret =cci_read (ar0234->regmap,AR0234_REG_REVISION_NUMBER , &rev, &ret); +if (ret) +return dev_err_probe (ar0234->dev, ret, +"Failed to read chip id\n" ); + +if (id ==AR0234_CHIP_ID_MONO ) + ar0234->variant =AR0234_VARIANT_MONO ; +else if (id ==AR0234_CHIP_ID ) + ar0234->variant =AR0234_VARIANT_COLOUR ; +else +return dev_err_probe (ar0234->dev, -ENODEV , +"Invalid chip id: 0x%04x\n" , (u16 )id); + +dev_info (ar0234->dev,"Success reading chip id: 0x%04x, Rev.%lld\n" , + (u16 )id, (rev >>12 ) &0xf ); + +return ret; +} + +static int ar0234_power_on (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct ar0234 *ar0234 =to_ar0234 (sd); +int ret; + + ret =regulator_bulk_enable (ARRAY_SIZE (ar0234->supplies), + ar0234->supplies); +if (ret) { +dev_err (ar0234->dev,"Failed to enable regulators\n" ); +return ret; + }
drivers/media/i2c/ar0234.c line 1125
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+ + ret =clk_prepare_enable (ar0234->clk); +if (ret) { +dev_err (ar0234->dev,"Failed to enable clock\n" ); +regulator_bulk_disable (ARRAY_SIZE (ar0234->supplies), + ar0234->supplies); +return ret; + } + +gpiod_set_value_cansleep (ar0234->reset,1 );
should be 0 to de-assert the reset pin to power on
drivers/media/i2c/ar0234.c line 1137
Quoted 12 lines
drivers/media/i2c/ar0234.c new file +12
-0+/* ~160000 EXTCLKs */ +usleep_range (27000 ,28000 ); + +return 0 ; +} + +static int ar0234_power_off (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct ar0234 *ar0234 =to_ar0234 (sd); + +gpiod_set_value_cansleep (ar0234->reset,0 );
1 to assert reset to power off
drivers/media/i2c/ar0234.c line 1145
Quoted 8 lines
drivers/media/i2c/ar0234.c new file +8
-0+regulator_bulk_disable (ARRAY_SIZE (ar0234->supplies), ar0234->supplies); +clk_disable_unprepare (ar0234->clk); +/* 100ms PwrDown until next PwrUp */ +usleep_range (100000 ,110000 ); + +return 0 ; +} +
after fixing the reset polarity locally I got the driver working with a rgb ar0234cs in 4-lane 10bit mode using a 24mhz extclk and a 448MHz link frequency.
(after the reset fix)
- Quentin
Re: [RFC PATCH v3 2/2] media: i2c: Add onsemi AR0234 image sensor driver
Hi Quentin.
Thank you for testing the driver.
If you don’t mind, could you also try the current driver version?
https://github.com/MacroGroup/linux/blob/test/drivers/media/i2c/ar0234.c
I’ve made a number of fixes, especially in the selection API code. I plan to post a non‑RFC version soon, so any additional feedback on the current code
would be very helpful to improve the driver further.
Thanks!
Quoted 2 lines · Alexander Shiyan
Quoted 1 line
+static int
ar0234_identify_module(struct ar0234 *ar0234)
...
Quoted 43 lines
drivers/media/i2c/ar0234.c line 1125
Quoted 9 lines
drivers/media/i2c/ar0234.c new file +9
-0+ ret =clk_prepare_enable (ar0234->clk); +if (ret) { +dev_err (ar0234->dev,"Failed to enable clock\n" ); +regulator_bulk_disable (ARRAY_SIZE (ar0234->supplies), + ar0234->supplies); +return ret; + } + +gpiod_set_value_cansleep (ar0234->reset,1 );should be 0 to de-assert the reset pin to power on
drivers/media/i2c/ar0234.c line 1137
Quoted 12 lines
drivers/media/i2c/ar0234.c new file +12
-0+/* ~160000 EXTCLKs */ +usleep_range (27000 ,28000 ); + +return 0 ; +} + +static int ar0234_power_off (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct ar0234 *ar0234 =to_ar0234 (sd); + +gpiod_set_value_cansleep (ar0234->reset,0 );1 to assert reset to power off
drivers/media/i2c/ar0234.c line 1145
Quoted 8 lines
drivers/media/i2c/ar0234.c new file +8
-0+regulator_bulk_disable (ARRAY_SIZE (ar0234->supplies), ar0234->supplies); +clk_disable_unprepare (ar0234->clk); +/* 100ms PwrDown until next PwrUp */ +usleep_range (100000 ,110000 ); + +return 0 ; +} +after fixing the reset polarity locally I got the driver working with a rgb ar0234cs in 4-lane 10bit mode using a 24mhz extclk and a 448MHz link frequency.
Tested-by Quentin Freimanis <[email protected]>(after the reset fix)
- Quentin
Re: [RFC PATCH v3 2/2] media: i2c: Add onsemi AR0234 image sensor driver
In reply to Alexander ShiyanHi Alexander,
(Dropping recipients whose e-mail addresses bounce)
Thank you for the patch.
Quoted 15 lines · Alexander Shiyan
Add driver for the onsemi AR0234 CMOS image sensor.
Signed-off-by Alexander Shiyan <[email protected]>--- drivers/media/i2c/Kconfig | 12 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ar0234.c | 1309 ++++++++++++++++++++++++++++++++++++ 3 files changed, 1322 insertions(+) create mode 100644 drivers/media/i2c/ar0234.cdrivers/media/i2c/Kconfig +0
-0
drivers/media/i2c/Kconfig config VIDEO_ALVIUM_CSI2 · line 58
Quoted 8 lines
drivers/media/i2c/Kconfig +5
-0@@ -51,6 +51,18 @@ config VIDEO_ALVIUM_CSI2 To compile this driver as a module, choose M here: the module will be called alvium-csi2. +config VIDEO_AR0234 + tristate "onsemi AR0234 sensor support" + depends on ACPI || OF || COMPILE_TEST + select V4L2_CCI_I2C + select VIDEO_CCS_PLL
I really like this :-)
Quoted 41 lines
drivers/media/i2c/Kconfig +7
-0+ help + This is a Video4Linux2 sensor driver for the onsemi + AR0234 camera. + + To compile this driver as a module, choose M here: the + module will be called ar0234. + config VIDEO_AR0521 tristate "ON Semiconductor AR0521 sensor support" helpdrivers/media/i2c/Makefile +1
-0@@ -19,6 +19,7 @@ obj-$(CONFIG_VIDEO_AK7375) += ak7375.o obj-$(CONFIG_VIDEO_AK881X) += ak881x.o obj-$(CONFIG_VIDEO_ALVIUM_CSI2) += alvium-csi2.o obj-$(CONFIG_VIDEO_APTINA_PLL) += aptina-pll.o +obj-$(CONFIG_VIDEO_AR0234) += ar0234.o obj-$(CONFIG_VIDEO_AR0521) += ar0521.o obj-$(CONFIG_VIDEO_BT819) += bt819.o obj-$(CONFIG_VIDEO_BT856) += bt856.odrivers/media/i2c/ar0234.c new file +13
-0@@ -0,0 +1,1309 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Driver for the onsemi AR0234 camera sensor +* +* Copyright (C) 2026 Alexander Shiyan <[email protected]> +* +* Some parts of code taken from Raspberry Pi driver ar0234.c by: +* Copyright (C) 2021, Raspberry Pi (Trading) Ltd +* Copyright (C) 2025-2026, UAB Kurokesu +* Author: Dave Stevenson <[email protected]> +* Author: Danius Kalvaitis <[email protected]> +* +* Some parts of code taken from imx290.c by:
drivers/media/i2c/ar0234.c line 23
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+* Copyright (C) 2019 FRAMOS GmbH. +* Copyright (C) 2019 Linaro Ltd. +* Author: Manivannan Sadhasivam <[email protected]> +*/ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/module.h> +#include <linux/pm_runtime.h> +#include <linux/gpio/consumer.h>
Please sort the headers alphabetically, gpio/consumer.h goes just after delay.h.
drivers/media/i2c/ar0234.c line 35
Quoted 12 lines
drivers/media/i2c/ar0234.c new file +12
-0+#include <linux/regulator/consumer.h> +#include <media/mipi-csi2.h> +#include <media/v4l2-cci.h> +#include <media/v4l2-ctrls.h> +#include <media/v4l2-event.h> +#include <media/v4l2-fwnode.h> +#include <media/v4l2-subdev.h> + +#include "ccs-pll.h" + +#define AR0234_REG_CHIP_VERSION CCI_REG16(0x3000) +# define AR0234_CHIP_ID (0x0a56)
No need for parentheses. Same below where applicable.
Quoted 60 lines
drivers/media/i2c/ar0234.c new file +60
-0+# define AR0234_CHIP_ID_MONO (0x1a56) +#define AR0234_REG_Y_ADDR_START CCI_REG16(0x3002) +#define AR0234_REG_X_ADDR_START CCI_REG16(0x3004) +#define AR0234_REG_Y_ADDR_END CCI_REG16(0x3006) +#define AR0234_REG_X_ADDR_END CCI_REG16(0x3008) +#define AR0234_REG_FRAME_LENGTH_LINES CCI_REG16(0x300a) +# define AR0234_FRAME_LENGTH_LINES_MIN (16) +# define AR0234_VBLANK_MAX (0xf000) +#define AR0234_REG_LINE_LENGTH_PCK CCI_REG16(0x300c) +# define AR0234_LINE_LENGTH_PCK_MIN (612) +# define AR0234_HBLANK_MAX (0xf000) +#define AR0234_REG_REVISION_NUMBER CCI_REG16(0x300e) +#define AR0234_REG_COARSE_INTEGRATION_TIME CCI_REG16(0x3012) +# define AR0234_EXPOSURE_MIN (2) +# define AR0234_EXPOSURE_STEP (1) +#define AR0234_REG_FINE_INTEGRATION_TIME CCI_REG16(0x3014) +#define AR0234_REG_RESET CCI_REG16(0x301a) +#define AR0234_REG_MODE_SELECT CCI_REG8(0x301c) +#define AR0234_REG_IMAGE_ORIENTATION CCI_REG8(0x301d) +#define AR0234_REG_GROUPED_PARAMETER_HOLD CCI_REG8(0x3022) +#define AR0234_REG_VT_PIX_CLK_DIV CCI_REG16(0x302a) +#define AR0234_REG_VT_SYS_CLK_DIV CCI_REG16(0x302c) +#define AR0234_REG_PRE_PLL_CLK_DIV CCI_REG16(0x302e) +#define AR0234_REG_PLL_MULTIPLIER CCI_REG16(0x3030) +#define AR0234_REG_OP_PIX_CLK_DIV CCI_REG16(0x3036) +#define AR0234_REG_OP_SYS_CLK_DIV CCI_REG16(0x3038) +#define AR0234_REG_GLOBAL_GAIN CCI_REG16(0x305e) +# define AR0234_DGTL_GAIN_MIN (0x0080) +# define AR0234_DGTL_GAIN_MAX (0x07ff) +# define AR0234_DGTL_GAIN_DEFAULT (0x0080) +# define AR0234_DGTL_GAIN_STEP (1) +#define AR0234_REG_ANALOG_GAIN CCI_REG16(0x3060) +# define AR0234_ANA_GAIN_BASE (64) +# define AR0234_ANA_GAIN_MIN (AR0234_ANA_GAIN_BASE) +# define AR0234_ANA_GAIN_MAX (16 * AR0234_ANA_GAIN_BASE) +# define AR0234_ANA_GAIN_STEP (1) +# define AR0234_ANA_GAIN_DEFAULT (AR0234_ANA_GAIN_BASE) +#define AR0234_REG_TEST_PATTERN_MODE CCI_REG16(0x3070) +# define AR0234_TEST_PATTERN_DISABLED (0) +# define AR0234_TEST_PATTERN_SOLID_COLOR (1) +# define AR0234_TEST_PATTERN_VERTICAL_COLOR_BARS (2) +# define AR0234_TEST_PATTERN_FADE_TO_GREY (3) +# define AR0234_TEST_PATTERN_WALKING_1S (256) +#define AR0234_REG_TEST_DATA_RED CCI_REG16(0x3072) +#define AR0234_REG_TEST_DATA_GREENR CCI_REG16(0x3074) +#define AR0234_REG_TEST_DATA_BLUE CCI_REG16(0x3076) +#define AR0234_REG_TEST_DATA_GREENB CCI_REG16(0x3078) +# define AR0234_TESTP_COLOUR_MIN (0) +# define AR0234_TESTP_COLOUR_MAX (0x3ff) +# define AR0234_TESTP_COLOUR_STEP (1) +#define AR0234_REG_MFR_30BA CCI_REG16(0x30ba) +# define AR0234_MFR_30BA_GAIN_BITS (x) (0x7620 | (x)) +#define AR0234_REG_DATA_FORMAT_BITS CCI_REG16(0x31ac) +# define DATA_FORMAT_BITS (x, y) (((x) << 8) | (y)) +#define AR0234_REG_SERIAL_FORMAT CCI_REG16(0x31ae) +# define DATA_FORMAT_LANES (x) (0x200 | (x)) +#define AR0234_REG_COMPANDING CCI_REG16(0x31d0) +# define COMPANDING_DPCM_EN BIT(0) +#define AR0234_REG_MIPI_CNTRL CCI_REG16(0x3354) +
drivers/media/i2c/ar0234.c line 105
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+#define AR0234_NATIVE_WIDTH (1940U) +#define AR0234_NATIVE_HEIGHT (1220U) +#define AR0234_PIXEL_ARRAY_LEFT (8U) +#define AR0234_PIXEL_ARRAY_TOP (8U) +#define AR0234_PIXEL_ARRAY_WIDTH (1920U) +#define AR0234_PIXEL_ARRAY_HEIGHT (1200U) +#define AR0234_MIN_CROP_WIDTH (4U) +#define AR0234_MIN_CROP_HEIGHT (2U) +#define AR0234_CROP_WIDTH_STEP (4U) +#define AR0234_CROP_HEIGHT_STEP (2U)
Where do those two step values come from ?
Quoted 35 lines
drivers/media/i2c/ar0234.c new file +35
-0+ +static const struct cci_reg_sequence ar0234_common_init[] = { + {AR0234_REG_FINE_INTEGRATION_TIME ,0 }, +}; + +static const char *const ar0234_test_pattern_menu[] = { +"Disabled" , +"Solid Color" , +"Vertical Color Bars" , +"Fade to Grey Vertical Color Bars" , +"Walking 1s" , +}; + +static const unsigned int ar0234_test_pattern_val[] = { +AR0234_TEST_PATTERN_DISABLED , +AR0234_TEST_PATTERN_SOLID_COLOR , +AR0234_TEST_PATTERN_VERTICAL_COLOR_BARS , +AR0234_TEST_PATTERN_FADE_TO_GREY , +AR0234_TEST_PATTERN_WALKING_1S , +}; + +static const char *const ar0234_supply_names[] = { +"vaa" , +"vdd" , +"vddio" , +}; + +enum ar0234_colour_variant { +AR0234_VARIANT_MONO , +AR0234_VARIANT_COLOUR , +AR0234_VARIANT_MAX +}; + +enum ar0234_link_freq_index { +AR0234_LINK_FREQ_IDX_BPP_8 ,
drivers/media/i2c/ar0234.c line 150
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+AR0234_LINK_FREQ_IDX_BPP_10 , +AR0234_LINK_FREQ_IDX_MAX +}; + +struct ar0234_mode { +u8 bpp_in; +u8 bpp_out; +u8 dpcm; +u8 mipi_dt; +int link_freq_index;
I think you can drop this. The driver already uses the CCS PLL calculator, so it should be able to deal with user-selectable link frequencies.
This would require testing all frequencies specified in DT at probe time to map them to bus formats, and restricting the formats accepted by the driver based on the current link frequency. The CCS driver implements such logic, and there's also an implementation in the ar0830 driver I'm working on (see [1]). Note to myself (and to Sakari too): this is a good candidate for a helper.
Quoted 49 lines
drivers/media/i2c/ar0234.c new file +49
-0+u32 code[AR0234_VARIANT_MAX ]; +}; + +static const struct ar0234_mode ar0234_modes[] = { + { + .bpp_in =8 , + .bpp_out =8 , + .dpcm =0 , + .mipi_dt =MIPI_CSI2_DT_RAW8 , + .link_freq_index =AR0234_LINK_FREQ_IDX_BPP_8 , + .code = { + [AR0234_VARIANT_MONO ] =MEDIA_BUS_FMT_Y8_1X8 , + [AR0234_VARIANT_COLOUR ] =MEDIA_BUS_FMT_SGRBG8_1X8 , + }, + }, + { + .bpp_in =10 , + .bpp_out =10 , + .dpcm =0 , + .mipi_dt =MIPI_CSI2_DT_RAW10 , + .link_freq_index =AR0234_LINK_FREQ_IDX_BPP_10 , + .code = { + [AR0234_VARIANT_MONO ] =MEDIA_BUS_FMT_Y10_1X10 , + [AR0234_VARIANT_COLOUR ] =MEDIA_BUS_FMT_SGRBG10_1X10 , + }, + }, + { + .bpp_in =10 , + .bpp_out =8 , + .dpcm =COMPANDING_DPCM_EN , + .mipi_dt =MIPI_CSI2_DT_RAW10 , + .link_freq_index =AR0234_LINK_FREQ_IDX_BPP_8 , + .code = { + [AR0234_VARIANT_COLOUR ] = +MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8 , + }, + }, +}; + +struct ar0234 { +struct device *dev; +struct clk *clk; +struct regmap *regmap; + +struct v4l2_subdev sd; +struct media_pad pad; + +struct regulator_bulk_data supplies[ARRAY_SIZE (ar0234_supply_names)]; +struct gpio_desc *reset;
drivers/media/i2c/ar0234.c line 209
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+ +unsigned int num_data_lanes; + +s64 link_freqs[AR0234_LINK_FREQ_IDX_MAX ]; + +enum ar0234_colour_variant variant; + +struct ccs_pll pll; + +struct ar0234_mode const *mode;
It would be nice to drop this, and access the information through the state. As far as I can tell, it will involve
- Looking up the mode by media bus code in .enable_streams()
- Passing the mode as a function argument to ar0234_set_link_limits(). One of the callers already has the mode pointer, the other one (ar0234_ctrls_init()) can use the default ar0234_modes[0].
- Using the default ar0234_modes[0] in ar0234_init_state(), which is a good thing to do anyway as the state should be initialized to static values that don't depend on the active configuration.
drivers/media/i2c/ar0234.c line 210
Quoted 1 line
drivers/media/i2c/ar0234.c new file +1
-0+struct v4l2_rect crop;
The crop rectangle should be accessed through the active state only.
drivers/media/i2c/ar0234.c line 212
Quoted 2 lines
drivers/media/i2c/ar0234.c new file +2
-0+ +bool streaming;
This could also possibly be replaced by v4l2_subdev_is_streaming().
Quoted 113 lines
drivers/media/i2c/ar0234.c new file +113
-0+ +struct v4l2_ctrl_handler ctrls; + +struct v4l2_ctrl *hblank; +struct v4l2_ctrl *vblank; +struct v4l2_ctrl *exposure; +struct v4l2_ctrl *pixel_rate; +struct v4l2_ctrl *link_freq; +struct v4l2_ctrl *a_gain; +struct { +struct v4l2_ctrl *hflip; +struct v4l2_ctrl *vflip; + }; +}; + +static inline struct ar0234 *to_ar0234 (struct v4l2_subdev *_sd) +{ + return container_of(_sd, struct ar0234, sd); +} + +static const struct ccs_pll_limits ar0234_pll_limits = { + .min_ext_clk_freq_hz =6000000 , + .max_ext_clk_freq_hz =54000000 , + .vt_fr = { + .min_pre_pll_clk_div =1 , + .max_pre_pll_clk_div =63 , + .min_pll_ip_clk_freq_hz =6000000 , + .max_pll_ip_clk_freq_hz =12000000 , + .min_pll_multiplier =2 , + .max_pll_multiplier =254 , + .min_pll_op_clk_freq_hz =384000000 , + .max_pll_op_clk_freq_hz =768000000 , + }, + .vt_bk = { + .min_sys_clk_div =1 , + .max_sys_clk_div =31 , + .min_sys_clk_freq_hz =6000000 , + .max_sys_clk_freq_hz =768000000 , + .min_pix_clk_div =1 , + .max_pix_clk_div =31 , + .min_pix_clk_freq_hz =6000000 , + .max_pix_clk_freq_hz =90000000 , + }, + .op_bk = { + .min_sys_clk_div =1 , + .max_sys_clk_div =31 , + .min_sys_clk_freq_hz =6000000 , + .max_sys_clk_freq_hz =768000000 , + .min_pix_clk_div =1 , + .max_pix_clk_div =31 , + .min_pix_clk_freq_hz =6000000 , + .max_pix_clk_freq_hz =90000000 , + }, +}; + +static int ar0234_calculate_pll (struct ar0234 *ar0234, +const struct ar0234_mode *mode) +{ +struct ccs_pll pll = {0 }; +int ret; + + pll.bus_type =CCS_PLL_BUS_TYPE_CSI2_DPHY ; + pll.op_lanes = ar0234->num_data_lanes; + pll.vt_lanes =1 ; + pll.csi2.lanes = ar0234->num_data_lanes; + pll.binning_horizontal =1 ; + pll.binning_vertical =1 ; + pll.scale_m =1 ; + pll.scale_n =1 ; + pll.bits_per_pixel = mode->bpp_out; + pll.flags =CCS_PLL_FLAG_LANE_SPEED_MODEL | +CCS_PLL_FLAG_EVEN_PLL_MULTIPLIER | +CCS_PLL_FLAG_FIFO_DERATING | +CCS_PLL_FLAG_FIFO_OVERRATING | +CCS_PLL_FLAG_EXT_IP_PLL_DIVIDER ; + pll.link_freq = ar0234->link_freqs[mode->link_freq_index] /2 ; + pll.ext_clk_freq_hz =clk_get_rate (ar0234->clk); + + ret =ccs_pll_calculate (ar0234->dev, &ar0234_pll_limits, &pll); +if (!ret) + ar0234->pll = pll; + +return ret; +} + +static u32 ar0234_calc_analog_gain (u32 req_gain_q6,u32 *reg_val) +{ +u32 s, t; +u32 best_gain =0 ; +u32 best_reg =0 ; +u32 min_diff =U32_MAX ; +u32 coarse_mult, fine_gain_q6, total_gain_q6, diff; + +for (s =0 ; s <=4 ; s++) { + coarse_mult = (1 << s) *AR0234_ANA_GAIN_BASE ; + +for (t =0 ; t <=15 ; t++) { +if (s ==0 || s ==2 ) { + fine_gain_q6 = + (AR0234_ANA_GAIN_BASE *32 ) / (32 - t); + }else if (s ==1 || s ==3 ) { + fine_gain_q6 = (AR0234_ANA_GAIN_BASE *16 ) / + (16 - (t /2 )); + }else { + fine_gain_q6 = (AR0234_ANA_GAIN_BASE *8 ) / + (8 - (t /4 )); + } + + total_gain_q6 = (coarse_mult * fine_gain_q6) / +AR0234_ANA_GAIN_BASE ; + +if (req_gain_q6 > total_gain_q6) + diff = req_gain_q6 - total_gain_q6;
drivers/media/i2c/ar0234.c line 335
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+else + diff = total_gain_q6 - req_gain_q6; + +if (diff < min_diff) { + min_diff = diff; + best_gain = total_gain_q6; + best_reg = (s <<4 ) | t; + } + } + }
It would be better to expose the ANALOG_GAIN register directly to userspace (or, to be precise, bits [6:0] for context A). There's no need to linearize the gain in the kernel.
Quoted 6 lines
drivers/media/i2c/ar0234.c new file +6
-0+ + *reg_val = best_reg; + +return best_gain; +} +
drivers/media/i2c/ar0234.c line 351
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+static int ar0234_set_mfr_30ba (struct ar0234 *ar0234,u32 analog_reg_val) +{ +u16 mfr_30ba_val; +u32 coarse_idx = (analog_reg_val >>4 ) &0x7 ; + +if (ar0234->pll.pixel_rate_pixel_array <=45000000 ) { +if (coarse_idx <3 ) + mfr_30ba_val =AR0234_MFR_30BA_GAIN_BITS (6 ); +else + mfr_30ba_val =AR0234_MFR_30BA_GAIN_BITS (0 );
Doesn't the cut off point also depend on the fine gain ? For 22.5 MHz I see MFR 0x30ba documented as always equal to 6, while for 45 MHz I see it as 6 util ANALOG_GAIN=0x0034 and 0 above that value.
drivers/media/i2c/ar0234.c line 358
Quoted 7 lines
drivers/media/i2c/ar0234.c new file +7
-0+ }else { +if (coarse_idx ==0 ) + mfr_30ba_val =AR0234_MFR_30BA_GAIN_BITS (2 ); +else if (coarse_idx ==1 ) + mfr_30ba_val =AR0234_MFR_30BA_GAIN_BITS (1 ); +else + mfr_30ba_val =AR0234_MFR_30BA_GAIN_BITS (0 );
Same here, the cut off point seems to 1 up to ANALOG_GAIN=0x0038 and 0 above that value.
I think you can simplify the code by checking analog_reg_val instead of calculating coarse_idx.
Quoted 8 lines
drivers/media/i2c/ar0234.c new file +8
-0+ } + +return cci_write (ar0234->regmap,AR0234_REG_MFR_30BA , + mfr_30ba_val,NULL ); +} + +static int ar0234_set_analog_gain (struct ar0234 *ar0234,u64 val) +{
drivers/media/i2c/ar0234.c line 376
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+u32 reg_val, actual_gain; +int ret; + + actual_gain =ar0234_calc_analog_gain (val, ®_val); + +if (actual_gain != val) { +__v4l2_ctrl_modify_range (ar0234->a_gain,AR0234_ANA_GAIN_MIN , +AR0234_ANA_GAIN_MAX , +AR0234_ANA_GAIN_STEP , actual_gain); +__v4l2_ctrl_s_ctrl (ar0234->a_gain, actual_gain);
Why do you need this ?
Quoted 30 lines
drivers/media/i2c/ar0234.c new file +30
-0+ } + + ret =cci_write (ar0234->regmap,AR0234_REG_GROUPED_PARAMETER_HOLD , +1 ,NULL ); +if (ret) +return ret; + + ret =ar0234_set_mfr_30ba (ar0234, reg_val); + +cci_write (ar0234->regmap,AR0234_REG_ANALOG_GAIN , reg_val, &ret); + +cci_write (ar0234->regmap,AR0234_REG_GROUPED_PARAMETER_HOLD ,0 ,NULL ); + +return ret; +} + +static int ar0234_set_ctrl (struct v4l2_ctrl *ctrl) +{ +struct ar0234 *ar0234 =container_of (ctrl->handler, + struct ar0234, ctrls); +int ret =0 ; + +if (ctrl->flags &V4L2_CTRL_FLAG_READ_ONLY ) +return 0 ; + +if (ctrl->id ==V4L2_CID_VBLANK ) { +int exposure_max = ar0234->crop.height + ctrl->val -1 ; +int exposure_val =clamp (ar0234->exposure->val, +AR0234_EXPOSURE_MIN , exposure_max); +
drivers/media/i2c/ar0234.c line 416
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+ ret =__v4l2_ctrl_modify_range (ar0234->exposure, +AR0234_EXPOSURE_MIN , + exposure_max, +AR0234_EXPOSURE_STEP , + exposure_val); +if (ret) +return ret; + } + +if (pm_runtime_get_if_in_use (ar0234->dev) ==0 )
You should use pm_runtime_get_if_active() (see
https://lore.kernel.org/linux-media/[email protected]/T/#t).
Quoted 4 lines
drivers/media/i2c/ar0234.c new file +4
-0+return 0 ; + +switch (ctrl->id) { +case V4L2_CID_HBLANK :
drivers/media/i2c/ar0234.c line 430
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+cci_write (ar0234->regmap,AR0234_REG_LINE_LENGTH_PCK , + (ar0234->crop.width /4 ) + ctrl->val, &ret); +break ; +case V4L2_CID_VBLANK : +cci_write (ar0234->regmap,AR0234_REG_FRAME_LENGTH_LINES , + ar0234->crop.height + ctrl->val, &ret); +if (ret) +break ; + ctrl = ar0234->exposure; + fallthrough;
I'd like Sakari's opinion on this.
Quoted 64 lines
drivers/media/i2c/ar0234.c new file +64
-0+case V4L2_CID_EXPOSURE : +cci_write (ar0234->regmap,AR0234_REG_COARSE_INTEGRATION_TIME , + ctrl->val, &ret); +break ; +case V4L2_CID_ANALOGUE_GAIN : + ret =ar0234_set_analog_gain (ar0234, ctrl->val); +break ; +case V4L2_CID_DIGITAL_GAIN : +cci_write (ar0234->regmap,AR0234_REG_GLOBAL_GAIN , + ctrl->val, &ret); +break ; +case V4L2_CID_TEST_PATTERN : +cci_write (ar0234->regmap,AR0234_REG_TEST_PATTERN_MODE , + ar0234_test_pattern_val[ctrl->val], &ret); +break ; +case V4L2_CID_HFLIP : +case V4L2_CID_VFLIP : +cci_write (ar0234->regmap,AR0234_REG_IMAGE_ORIENTATION , + (ar0234->vflip->val <<1 ) | ar0234->hflip->val, &ret); +break ; +case V4L2_CID_TEST_PATTERN_RED : +cci_write (ar0234->regmap,AR0234_REG_TEST_DATA_RED , + ctrl->val, &ret); +break ; +case V4L2_CID_TEST_PATTERN_GREENR : +cci_write (ar0234->regmap,AR0234_REG_TEST_DATA_GREENR , + ctrl->val, &ret); +break ; +case V4L2_CID_TEST_PATTERN_BLUE : +cci_write (ar0234->regmap,AR0234_REG_TEST_DATA_BLUE , + ctrl->val, &ret); +break ; +case V4L2_CID_TEST_PATTERN_GREENB : +cci_write (ar0234->regmap,AR0234_REG_TEST_DATA_GREENB , + ctrl->val, &ret); +break ; +default : +dev_err (ar0234->dev,"Invalid control %d\n" , ctrl->id); + ret = -EINVAL ; +break ; + } + +pm_runtime_put_autosuspend (ar0234->dev); + +return ret; +} + +static const struct v4l2_ctrl_ops ar0234_ctrl_ops = { + .s_ctrl = ar0234_set_ctrl, +}; + +static int ar0234_enum_mbus_code (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_mbus_code_enum *code) +{ +struct ar0234 *ar0234 =to_ar0234 (sd); + +if (code->index >=ARRAY_SIZE (ar0234_modes)) +return -EINVAL ; + +if (!ar0234_modes[code->index].code[ar0234->variant]) +return -EINVAL ; + + code->code = ar0234_modes[code->index].code[ar0234->variant];
drivers/media/i2c/ar0234.c line 504
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+ +return 0 ; +} + +static int ar0234_enum_frame_size (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_frame_size_enum *fse) +{ +if (fse->index >=ARRAY_SIZE (ar0234_modes)) +return -EINVAL ;
You also need to validate that fse->code is valid. I think v4l2-compliance checks for that, please include the v4l2-compliance report in the cover letter of the next version.
Quoted 45 lines
drivers/media/i2c/ar0234.c new file +45
-0+ + fse->min_width =AR0234_MIN_CROP_WIDTH ; + fse->max_width =AR0234_PIXEL_ARRAY_WIDTH ; + fse->min_height =AR0234_MIN_CROP_HEIGHT ; + fse->max_height =AR0234_PIXEL_ARRAY_HEIGHT ; + +return 0 ; +} + +static void ar0234_set_link_limits (struct ar0234 *ar0234) +{ +u64 pixel_rate = ar0234->link_freqs[ar0234->mode->link_freq_index] *2 ; + + pixel_rate *= ar0234->num_data_lanes; +do_div (pixel_rate, ar0234->mode->bpp_out); + +__v4l2_ctrl_s_ctrl_int64 (ar0234->pixel_rate, pixel_rate); + +__v4l2_ctrl_s_ctrl (ar0234->link_freq, ar0234->mode->link_freq_index); +} + +static void ar0234_set_framing_limits (struct ar0234 *ar0234) +{ +unsigned int width = ar0234->crop.width; +int hblank, hblank_min; + +__v4l2_ctrl_modify_range (ar0234->vblank,AR0234_FRAME_LENGTH_LINES_MIN , +AR0234_VBLANK_MAX ,1 , +AR0234_FRAME_LENGTH_LINES_MIN ); +__v4l2_ctrl_s_ctrl (ar0234->vblank,AR0234_FRAME_LENGTH_LINES_MIN ); + + hblank =AR0234_LINE_LENGTH_PCK_MIN - width /4 ; + hblank_min =AR0234_LINE_LENGTH_PCK_MIN -AR0234_PIXEL_ARRAY_WIDTH /4 ; +__v4l2_ctrl_modify_range (ar0234->hblank, hblank_min, +AR0234_HBLANK_MAX ,2 , hblank); +__v4l2_ctrl_s_ctrl (ar0234->hblank, hblank); +} + +static int ar0234_set_pad_format (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_format *fmt) +{ +struct ar0234 *ar0234 =to_ar0234 (sd); +struct ar0234_mode const *mode =NULL ; +unsigned int width, height;
drivers/media/i2c/ar0234.c line 559
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+int i; + +for (i =0 ; i <ARRAY_SIZE (ar0234_modes); i++) { +if (ar0234_modes[i].code[ar0234->variant] == fmt->format.code) { + mode = &ar0234_modes[i]; +break ; + } + } +if (!mode) +return -EINVAL ;
If the requested media bus code is not valid you should pick a default, not return an error.
Quoted 13 lines
drivers/media/i2c/ar0234.c new file +13
-0+ + width = clamp_t(unsigned int, round_down(fmt->format.width, +AR0234_CROP_WIDTH_STEP ), +AR0234_MIN_CROP_WIDTH ,AR0234_PIXEL_ARRAY_WIDTH ); + height = clamp_t(unsigned int, round_down(fmt->format.height, +AR0234_CROP_HEIGHT_STEP ), +AR0234_MIN_CROP_HEIGHT ,AR0234_PIXEL_ARRAY_HEIGHT ); + + fmt->format.width = width; + fmt->format.height = height; + fmt->format.field =V4L2_FIELD_NONE ; + fmt->format.colorspace =V4L2_COLORSPACE_RAW ; + fmt->format.ycbcr_enc =V4L2_YCBCR_ENC_DEFAULT ;
drivers/media/i2c/ar0234.c line 582
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+ fmt->format.quantization =V4L2_QUANTIZATION_DEFAULT ; + fmt->format.xfer_func =V4L2_XFER_FUNC_NONE ; + +if (fmt->which ==V4L2_SUBDEV_FORMAT_TRY ) { + *v4l2_subdev_state_get_format (sd_state, fmt->pad) = fmt->format; +return 0 ; + } + +if (ar0234->mode != mode) { +int ret =ar0234_calculate_pll (ar0234, mode);
I'd recommend moving this to .enable_streams().
drivers/media/i2c/ar0234.c line 592
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+ +if (ret) { +dev_err (ar0234->dev, +"PLL recalculations failed: %d\n" , ret); +return ret; + } + + ar0234->mode = mode; + +ar0234_set_link_limits (ar0234);
And this too.
drivers/media/i2c/ar0234.c line 593
Quoted 1 line
drivers/media/i2c/ar0234.c new file +1
-0+ }
Mode changes shouldn't be allowed when streaming.
Quoted 4 lines
drivers/media/i2c/ar0234.c new file +4
-0+ +if (ar0234->crop.width != width || ar0234->crop.height != height) { + ar0234->crop.width = width; + ar0234->crop.height = height;
drivers/media/i2c/ar0234.c line 607
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+ ar0234->crop.left = +min_t (u32, ar0234->crop.left,AR0234_PIXEL_ARRAY_LEFT + +AR0234_PIXEL_ARRAY_WIDTH - width); + ar0234->crop.top = +min_t (u32, ar0234->crop.top,AR0234_PIXEL_ARRAY_TOP + +AR0234_PIXEL_ARRAY_HEIGHT - height); + ar0234->crop.left = +max (ar0234->crop.left,AR0234_PIXEL_ARRAY_LEFT ); + ar0234->crop.top = +max (ar0234->crop.top,AR0234_PIXEL_ARRAY_TOP );
Propagration to the crop rectangle should be performed on the TRY format too (propagating, in that case, to the TRY crop rectangle of course).
Quoted 35 lines
drivers/media/i2c/ar0234.c new file +35
-0+ +ar0234_set_framing_limits (ar0234); + } + + *v4l2_subdev_state_get_format (sd_state, fmt->pad) = fmt->format; + +return 0 ; +} + +static int ar0234_init_state (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state) +{ +struct ar0234 *ar0234 =to_ar0234 (sd); +struct v4l2_subdev_format format = { + .which =V4L2_SUBDEV_FORMAT_TRY , + .format = { + .width = ar0234->crop.width, + .height = ar0234->crop.height, + .code = ar0234->mode->code[ar0234->variant], + }, + }; +int ret; + + ret =ar0234_set_pad_format (sd, state, &format); +if (ret) +return ret; + + *v4l2_subdev_state_get_crop (state,0 ) = ar0234->crop; + +return 0 ; +} + +static int ar0234_get_selection (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_selection *sel)
drivers/media/i2c/ar0234.c line 652
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+{ +struct ar0234 *ar0234 =to_ar0234 (sd); + +switch (sel->target) { +case V4L2_SEL_TGT_CROP : +if (sel->which ==V4L2_SUBDEV_FORMAT_TRY ) + sel->r = + *v4l2_subdev_state_get_crop (sd_state, sel->pad); +else + sel->r = ar0234->crop;
Always store the crop rectangle in the state.
Quoted 78 lines
drivers/media/i2c/ar0234.c new file +78
-0+ +return 0 ; +case V4L2_SEL_TGT_CROP_DEFAULT : +case V4L2_SEL_TGT_CROP_BOUNDS : + sel->r.left =AR0234_PIXEL_ARRAY_LEFT ; + sel->r.top =AR0234_PIXEL_ARRAY_TOP ; + sel->r.width =AR0234_PIXEL_ARRAY_WIDTH ; + sel->r.height =AR0234_PIXEL_ARRAY_HEIGHT ; + +return 0 ; +case V4L2_SEL_TGT_NATIVE_SIZE : + sel->r.top =0 ; + sel->r.left =0 ; + sel->r.width =AR0234_NATIVE_WIDTH ; + sel->r.height =AR0234_NATIVE_HEIGHT ; + +return 0 ; + } + +return -EINVAL ; +} + +static int ar0234_set_selection (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_selection *sel) +{ +struct ar0234 *ar0234 =to_ar0234 (sd); +struct v4l2_rect rect = sel->r; +struct v4l2_rect *try_crop; +struct v4l2_mbus_framefmt *try_fmt; +u32 max_left, max_top; + +if (sel->target !=V4L2_SEL_TGT_CROP ) +return -EINVAL ; + + max_left =AR0234_PIXEL_ARRAY_LEFT +AR0234_PIXEL_ARRAY_WIDTH - +AR0234_MIN_CROP_WIDTH ; + max_top =AR0234_PIXEL_ARRAY_TOP +AR0234_PIXEL_ARRAY_HEIGHT - +AR0234_MIN_CROP_HEIGHT ; + + rect.left =clamp_t (u32, rect.left,AR0234_PIXEL_ARRAY_LEFT , max_left); + rect.top =clamp_t (u32, rect.top,AR0234_PIXEL_ARRAY_TOP , max_top); + + rect.width = +clamp_t (u32,round_down (rect.width,AR0234_CROP_WIDTH_STEP ), +AR0234_MIN_CROP_WIDTH ,AR0234_PIXEL_ARRAY_WIDTH ); + rect.height = +clamp_t (u32,round_down (rect.height,AR0234_CROP_HEIGHT_STEP ), +AR0234_MIN_CROP_HEIGHT ,AR0234_PIXEL_ARRAY_HEIGHT ); + +if (rect.left + rect.width -1 > +AR0234_PIXEL_ARRAY_LEFT +AR0234_PIXEL_ARRAY_WIDTH -1 ) + rect.left =AR0234_PIXEL_ARRAY_LEFT + +AR0234_PIXEL_ARRAY_WIDTH - rect.width; +if (rect.top + rect.height -1 > +AR0234_PIXEL_ARRAY_TOP +AR0234_PIXEL_ARRAY_HEIGHT -1 ) + rect.top =AR0234_PIXEL_ARRAY_TOP + +AR0234_PIXEL_ARRAY_HEIGHT - rect.height; + +if (sel->flags &V4L2_SEL_FLAG_GE ) { +if (rect.width < sel->r.width) { +u32 new_width = rect.width +AR0234_CROP_WIDTH_STEP ; + +if (new_width <=AR0234_PIXEL_ARRAY_WIDTH ) + rect.width = new_width; + } + +if (rect.height < sel->r.height) { +u32 new_height = rect.height +AR0234_CROP_HEIGHT_STEP ; + +if (new_height <=AR0234_PIXEL_ARRAY_HEIGHT ) + rect.height = new_height; + } + } + +if (sel->flags &V4L2_SEL_FLAG_LE ) { +if (rect.width > sel->r.width && rect.width >= +AR0234_MIN_CROP_WIDTH +AR0234_CROP_WIDTH_STEP )
drivers/media/i2c/ar0234.c line 740
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+ rect.width -=AR0234_CROP_WIDTH_STEP ; + +if (rect.height > sel->r.height && rect.height >= +AR0234_MIN_CROP_HEIGHT +AR0234_CROP_HEIGHT_STEP ) + rect.height -=AR0234_CROP_HEIGHT_STEP ; + } + +if (rect.width <AR0234_MIN_CROP_WIDTH || + rect.height <AR0234_MIN_CROP_HEIGHT ) +return -EINVAL ;
The crop rectangle should be adjusted, invalid values should never return an error.
drivers/media/i2c/ar0234.c line 750
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+ +if (sel->which ==V4L2_SUBDEV_FORMAT_TRY ) { + try_crop =v4l2_subdev_state_get_crop (sd_state, sel->pad); + *try_crop = rect; + + try_fmt =v4l2_subdev_state_get_format (sd_state, sel->pad); +if (try_fmt) { + try_fmt->width = rect.width; + try_fmt->height = rect.height; + }
Here you propagate from the crop rectangle to the format for TRY, while above you propagate from the format to the crop rectangle for ACTIVE. Propagation should always operate in the same direction, for both the TRY and ACTIVE formats.
There's an effort to design a better (and better documented) crop API
(see https://lore.kernel.org/linux-media/[email protected]).
This is work in progress, so I don't want to delay this driver by
waiting for the API, but it may be a good idea to look at it.
Quoted 14 lines
drivers/media/i2c/ar0234.c new file +14
-0+ +return 0 ; + } + +if (ar0234->streaming) +return -EBUSY ; + +if (ar0234->crop.left == rect.left && ar0234->crop.top == rect.top && + ar0234->crop.width == rect.width && + ar0234->crop.height == rect.height) { + sel->r = rect; + +return 0 ; + }
drivers/media/i2c/ar0234.c line 774
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+ + ar0234->crop = rect; + + try_fmt =v4l2_subdev_state_get_format (sd_state, sel->pad); +if (try_fmt) { + try_fmt->width = rect.width; + try_fmt->height = rect.height; + } + +ar0234_set_framing_limits (ar0234);
Except for this call, the function should operate exactly the same way for TRY and ACTIVE.
Quoted 23 lines
drivers/media/i2c/ar0234.c new file +23
-0+ + sel->r = rect; + +return 0 ; +} + +static int ar0234_enable_streams (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state,u32 pad, +u64 streams_mask) +{ +struct ar0234 *ar0234 =to_ar0234 (sd); +int x_addr_start, x_addr_end, y_addr_start, y_addr_end, ret; + + ret =pm_runtime_resume_and_get (ar0234->dev); +if (ret) +return ret; + +cci_write (ar0234->regmap,AR0234_REG_PRE_PLL_CLK_DIV , + ar0234->pll.vt_fr.pre_pll_clk_div, &ret); +cci_write (ar0234->regmap,AR0234_REG_PLL_MULTIPLIER , + ar0234->pll.vt_fr.pll_multiplier, &ret); +cci_write (ar0234->regmap,AR0234_REG_VT_SYS_CLK_DIV , + ar0234->pll.vt_bk.sys_clk_div, &ret);
drivers/media/i2c/ar0234.c line 807
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+cci_write (ar0234->regmap,AR0234_REG_VT_PIX_CLK_DIV , + ar0234->pll.vt_bk.pix_clk_div, &ret); +cci_write (ar0234->regmap,AR0234_REG_OP_SYS_CLK_DIV , + ar0234->pll.op_bk.sys_clk_div, &ret); +cci_write (ar0234->regmap,AR0234_REG_OP_PIX_CLK_DIV , + ar0234->pll.op_bk.pix_clk_div, &ret); +if (ret) { +dev_err (ar0234->dev,"Failed to setup PLL\n" ); + goto start_err; + }
I'd drop this error check, and only eep the last one before __v4l2_ctrl_handler_setup().
Quoted 63 lines
drivers/media/i2c/ar0234.c new file +63
-0+ +cci_multi_reg_write (ar0234->regmap, ar0234_common_init, +ARRAY_SIZE (ar0234_common_init), &ret); + +cci_write (ar0234->regmap,AR0234_REG_COMPANDING , + ar0234->mode->dpcm, &ret); + +cci_write (ar0234->regmap,AR0234_REG_DATA_FORMAT_BITS , +DATA_FORMAT_BITS (ar0234->mode->bpp_in, ar0234->mode->bpp_out), + &ret); + +cci_write (ar0234->regmap,AR0234_REG_SERIAL_FORMAT , +DATA_FORMAT_LANES (ar0234->num_data_lanes), &ret); + +cci_write (ar0234->regmap,AR0234_REG_MIPI_CNTRL , + ar0234->mode->mipi_dt, &ret); + + x_addr_start = ar0234->crop.left; + y_addr_start = ar0234->crop.top; + x_addr_end = ar0234->crop.left + ar0234->crop.width -1 ; + y_addr_end = ar0234->crop.top + ar0234->crop.height -1 ; + +cci_write (ar0234->regmap,AR0234_REG_X_ADDR_START , x_addr_start, &ret); +cci_write (ar0234->regmap,AR0234_REG_Y_ADDR_START , y_addr_start, &ret); +cci_write (ar0234->regmap,AR0234_REG_X_ADDR_END , x_addr_end, &ret); +cci_write (ar0234->regmap,AR0234_REG_Y_ADDR_END , y_addr_end, &ret); + +if (ret) { +dev_err (ar0234->dev,"Failed to setup sensor\n" ); + goto start_err; + } + + ret =__v4l2_ctrl_handler_setup (ar0234->sd.ctrl_handler); + +cci_write (ar0234->regmap,AR0234_REG_MODE_SELECT ,1 , &ret); +if (!ret) { + ar0234->streaming = true; +return 0 ; + } + +start_err: +pm_runtime_put_autosuspend (ar0234->dev); + +return ret; +} + +static int ar0234_disable_streams (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state,u32 pad, +u64 streams_mask) +{ +struct ar0234 *ar0234 =to_ar0234 (sd); +int ret; + + ret =cci_write (ar0234->regmap,AR0234_REG_MODE_SELECT ,0 ,NULL ); + + ar0234->streaming = false; + +pm_runtime_put_autosuspend (ar0234->dev); + +return ret; +} + +static int ar0234_g_mbus_config(struct v4l2_subdev *sd, unsigned int pad_id,
drivers/media/i2c/ar0234.c line 880
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+ struct v4l2_mbus_config *config) +{ +struct ar0234 *ar0234 =to_ar0234 (sd); + + config->type =V4L2_MBUS_CSI2_DPHY ; + config->bus.mipi_csi2.flags =V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK ; + config->bus.mipi_csi2.num_data_lanes = ar0234->num_data_lanes; + +return 0 ; +}
Please also implement .get_frame_desc().
Quoted 30 lines
drivers/media/i2c/ar0234.c new file +30
-0+ +static const struct v4l2_subdev_video_ops ar0234_video_ops = { + .s_stream = v4l2_subdev_s_stream_helper, +}; + +static const struct v4l2_subdev_pad_ops ar0234_pad_ops = { + .enum_mbus_code = ar0234_enum_mbus_code, + .enum_frame_size = ar0234_enum_frame_size, + .get_fmt = v4l2_subdev_get_fmt, + .set_fmt = ar0234_set_pad_format, + .get_selection = ar0234_get_selection, + .set_selection = ar0234_set_selection, + .enable_streams = ar0234_enable_streams, + .disable_streams = ar0234_disable_streams, + .get_mbus_config = ar0234_g_mbus_config, +}; + +static const struct v4l2_subdev_core_ops ar0234_core_ops = { + .subscribe_event = v4l2_ctrl_subdev_subscribe_event, + .unsubscribe_event = v4l2_event_subdev_unsubscribe, +}; + +static const struct v4l2_subdev_ops ar0234_subdev_ops = { + .core = &ar0234_core_ops, + .video = &ar0234_video_ops, + .pad = &ar0234_pad_ops, +}; + +static const struct v4l2_subdev_internal_ops ar0234_internal_ops = { + .init_state = ar0234_init_state,
drivers/media/i2c/ar0234.c line 920
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+}; + +static const struct media_entity_operations ar0234_subdev_entity_ops = { + .link_validate = v4l2_subdev_link_validate, +}; + +static int ar0234_ctrls_init (struct ar0234 *ar0234) +{ +struct v4l2_fwnode_device_properties props; +int i, ret;
i is never negative, it can be an unsigned int.
drivers/media/i2c/ar0234.c line 926
Quoted 6 lines
drivers/media/i2c/ar0234.c new file +6
-0+ + ret =v4l2_fwnode_device_parse (ar0234->dev, &props); +if (ret) +return ret; + + ret =v4l2_ctrl_handler_init (&ar0234->ctrls,14 );
16 to account for the controls created byv4l2_ctrl_new_fwnode_properties() ?
Quoted 189 lines
drivers/media/i2c/ar0234.c new file +189
-0+if (ret) +return ret; + + ar0234->hblank =v4l2_ctrl_new_std (&ar0234->ctrls, &ar0234_ctrl_ops, +V4L2_CID_HBLANK ,0 , +AR0234_HBLANK_MAX ,2 ,0 ); + + ar0234->vblank =v4l2_ctrl_new_std (&ar0234->ctrls, &ar0234_ctrl_ops, +V4L2_CID_VBLANK ,0 , +AR0234_VBLANK_MAX ,1 ,0 ); + + ar0234->exposure =v4l2_ctrl_new_std (&ar0234->ctrls, &ar0234_ctrl_ops, +V4L2_CID_EXPOSURE , +AR0234_EXPOSURE_MIN ,U16_MAX , +AR0234_EXPOSURE_STEP ,200 ); + + ar0234->pixel_rate =v4l2_ctrl_new_std (&ar0234->ctrls, &ar0234_ctrl_ops, +V4L2_CID_PIXEL_RATE ,1 , +INT_MAX ,1 ,1 ); + + ar0234->a_gain =v4l2_ctrl_new_std (&ar0234->ctrls, &ar0234_ctrl_ops, +V4L2_CID_ANALOGUE_GAIN , +AR0234_ANA_GAIN_MIN , +AR0234_ANA_GAIN_MAX , +AR0234_ANA_GAIN_STEP , +AR0234_ANA_GAIN_DEFAULT ); + +v4l2_ctrl_new_std (&ar0234->ctrls, &ar0234_ctrl_ops, +V4L2_CID_DIGITAL_GAIN ,AR0234_DGTL_GAIN_MIN , +AR0234_DGTL_GAIN_MAX ,AR0234_DGTL_GAIN_STEP , +AR0234_DGTL_GAIN_DEFAULT ); + + ar0234->hflip =v4l2_ctrl_new_std (&ar0234->ctrls, &ar0234_ctrl_ops, +V4L2_CID_HFLIP ,0 ,1 ,1 ,0 ); + ar0234->vflip =v4l2_ctrl_new_std (&ar0234->ctrls, &ar0234_ctrl_ops, +V4L2_CID_VFLIP ,0 ,1 ,1 ,0 ); +v4l2_ctrl_cluster (2 , &ar0234->hflip); + +v4l2_ctrl_new_std_menu_items (&ar0234->ctrls, &ar0234_ctrl_ops, +V4L2_CID_TEST_PATTERN , +ARRAY_SIZE (ar0234_test_pattern_menu) -1 , +0 ,0 , ar0234_test_pattern_menu); + +for (i =0 ; i <4 ; i++) { +v4l2_ctrl_new_std (&ar0234->ctrls, &ar0234_ctrl_ops, +V4L2_CID_TEST_PATTERN_RED + i, +AR0234_TESTP_COLOUR_MIN , +AR0234_TESTP_COLOUR_MAX , +AR0234_TESTP_COLOUR_STEP , +AR0234_TESTP_COLOUR_MAX ); + } + + ar0234->link_freq = +v4l2_ctrl_new_int_menu (&ar0234->ctrls, &ar0234_ctrl_ops, +V4L2_CID_LINK_FREQ , +AR0234_LINK_FREQ_IDX_MAX -1 ,0 , + ar0234->link_freqs); +if (ar0234->link_freq) + ar0234->link_freq->flags |=V4L2_CTRL_FLAG_READ_ONLY ; + + ret =v4l2_ctrl_new_fwnode_properties (&ar0234->ctrls, &ar0234_ctrl_ops, + &props); + +if (ret) +return dev_err_probe (ar0234->dev, ret, +"Failed to add controls\n" ); + + ar0234->sd.ctrl_handler = &ar0234->ctrls; + +ar0234_set_link_limits (ar0234); +ar0234_set_framing_limits (ar0234); + +return 0 ; +} + +static int ar0234_parse_hw_config (struct ar0234 *ar0234) +{ +struct v4l2_fwnode_endpoint bus_cfg = { + .bus_type =V4L2_MBUS_CSI2_DPHY , + }; +struct fwnode_handle *ep; +unsigned int i; +int ret; + +for (i =0 ; i <ARRAY_SIZE (ar0234->supplies); i++) + ar0234->supplies[i].supply = ar0234_supply_names[i]; + + ret =devm_regulator_bulk_get (ar0234->dev, +ARRAY_SIZE (ar0234->supplies), + ar0234->supplies); +if (ret) +return dev_err_probe (ar0234->dev, ret, +"Failed to get supplies\n" ); + + ar0234->reset =devm_gpiod_get_optional (ar0234->dev,"reset" , +GPIOD_OUT_HIGH ); +if (IS_ERR (ar0234->reset)) +return dev_err_probe (ar0234->dev,PTR_ERR (ar0234->reset), +"Failed to get reset GPIO\n" ); + + ar0234->clk =devm_v4l2_sensor_clk_get (ar0234->dev,NULL ); +if (IS_ERR (ar0234->clk)) +return dev_err_probe (ar0234->dev,PTR_ERR (ar0234->clk), +"Failed to get clock\n" ); + + ep =fwnode_graph_get_next_endpoint (dev_fwnode (ar0234->dev),NULL ); +if (!ep) +return -ENXIO ; + + ret =v4l2_fwnode_endpoint_alloc_parse (ep, &bus_cfg); +fwnode_handle_put (ep); +if (ret) +return ret; + +switch (bus_cfg.bus.mipi_csi2.num_data_lanes) { +case 2 : +case 4 : + ar0234->num_data_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes; +break ; +default : + ret =dev_err_probe (ar0234->dev, -EINVAL , +"Invalid number of CSI2 data lanes %d\n" , + bus_cfg.bus.mipi_csi2.num_data_lanes); + goto done_endpoint_free; + } + +if (bus_cfg.nr_of_link_frequencies !=AR0234_LINK_FREQ_IDX_MAX ) { + ret =dev_err_probe (ar0234->dev, -EINVAL , +"Invalid number of link freq items %d\n" , + bus_cfg.nr_of_link_frequencies); + goto done_endpoint_free; + } + +for (i =0 ; i < bus_cfg.nr_of_link_frequencies; i++) { +s64 freq = bus_cfg.link_frequencies[i]; + +if (freq <360000000LL || freq >450000000LL ) { + ret =dev_err_probe (ar0234->dev, -EINVAL , +"Invalid link frequency %lli\n" , + freq); + goto done_endpoint_free; + } + + ar0234->link_freqs[i] = freq; + } + +done_endpoint_free: +v4l2_fwnode_endpoint_free (&bus_cfg); + +return ret; +} + +static int ar0234_identify_module (struct ar0234 *ar0234) +{ +u64 id, rev; +int ret; + + ret =cci_read (ar0234->regmap,AR0234_REG_CHIP_VERSION , &id,NULL ); + ret =cci_read (ar0234->regmap,AR0234_REG_REVISION_NUMBER , &rev, &ret); +if (ret) +return dev_err_probe (ar0234->dev, ret, +"Failed to read chip id\n" ); + +if (id ==AR0234_CHIP_ID_MONO ) + ar0234->variant =AR0234_VARIANT_MONO ; +else if (id ==AR0234_CHIP_ID ) + ar0234->variant =AR0234_VARIANT_COLOUR ; +else +return dev_err_probe (ar0234->dev, -ENODEV , +"Invalid chip id: 0x%04x\n" , (u16 )id); + +dev_info (ar0234->dev,"Success reading chip id: 0x%04x, Rev.%lld\n" , + (u16 )id, (rev >>12 ) &0xf ); + +return ret; +} + +static int ar0234_power_on (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct ar0234 *ar0234 =to_ar0234 (sd); +int ret; + + ret =regulator_bulk_enable (ARRAY_SIZE (ar0234->supplies), + ar0234->supplies); +if (ret) { +dev_err (ar0234->dev,"Failed to enable regulators\n" ); +return ret; + }
drivers/media/i2c/ar0234.c line 1125
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+ + ret =clk_prepare_enable (ar0234->clk); +if (ret) { +dev_err (ar0234->dev,"Failed to enable clock\n" ); +regulator_bulk_disable (ARRAY_SIZE (ar0234->supplies), + ar0234->supplies); +return ret; + } + +gpiod_set_value_cansleep (ar0234->reset,1 );
The GPIO should be set to 0 to deassert reset. Polarity inversion should be handled in DT.
drivers/media/i2c/ar0234.c line 1127
Quoted 2 lines
drivers/media/i2c/ar0234.c new file +2
-0+/* ~160000 EXTCLKs */ +usleep_range (27000 ,28000 );
fsleep() seems a good candidate here.
Quoted 4 lines
drivers/media/i2c/ar0234.c new file +4
-0+ +return 0 ; +} +
drivers/media/i2c/ar0234.c line 1141
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+static int ar0234_power_off (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct ar0234 *ar0234 =to_ar0234 (sd); + +gpiod_set_value_cansleep (ar0234->reset,0 ); +regulator_bulk_disable (ARRAY_SIZE (ar0234->supplies), ar0234->supplies); +clk_disable_unprepare (ar0234->clk); +/* 100ms PwrDown until next PwrUp */ +usleep_range (100000 ,110000 );
fsleep() here too. A possibly better option would be to record the time, and add a delay at power on time if it's too clsoe to power odown.
Quoted 129 lines
drivers/media/i2c/ar0234.c new file +129
-0+ +return 0 ; +} + +static void ar0234_subdev_cleanup (struct ar0234 *ar0234) +{ +media_entity_cleanup (&ar0234->sd.entity); +v4l2_ctrl_handler_free (&ar0234->ctrls); +} + +static int ar0234_soft_reset (struct ar0234 *ar0234) +{ +int ret; + + ret =cci_write (ar0234->regmap,AR0234_REG_RESET ,0x0001 ,NULL ); +usleep_range (2000 ,2100 ); +cci_write (ar0234->regmap,AR0234_REG_RESET ,0x2018 , &ret); +usleep_range (2000 ,2100 ); + +return ret; +} + +static int ar0234_probe (struct i2c_client *client) +{ +struct device *dev = &client->dev; +struct ar0234 *ar0234; +int ret; + + ar0234 =devm_kzalloc (dev,sizeof (*ar0234),GFP_KERNEL ); +if (!ar0234) +return -ENOMEM ; + + ar0234->dev = dev; + + ar0234->regmap =devm_cci_regmap_init_i2c (client,16 ); +if (IS_ERR (ar0234->regmap)) +return PTR_ERR (ar0234->regmap); + + ret =ar0234_parse_hw_config (ar0234); +if (ret) +return ret; + +v4l2_i2c_subdev_init (&ar0234->sd, client, &ar0234_subdev_ops); + + ret =ar0234_power_on (dev); +if (ret) + goto err_subdev; + +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 =ar0234_soft_reset (ar0234); +if (ret) + goto error_pm; + + ret =ar0234_identify_module (ar0234); +if (ret) + goto error_pm; + + ar0234->crop.left =AR0234_PIXEL_ARRAY_LEFT ; + ar0234->crop.top =AR0234_PIXEL_ARRAY_TOP ; + ar0234->crop.width =AR0234_PIXEL_ARRAY_WIDTH ; + ar0234->crop.height =AR0234_PIXEL_ARRAY_HEIGHT ; + + ar0234->mode = &ar0234_modes[0 ]; + + ret =ar0234_calculate_pll (ar0234, ar0234->mode); +if (ret) { +dev_err (ar0234->dev,"PLL calculations failed: %d\n" , ret); + goto error_pm; + } + + ret =ar0234_ctrls_init (ar0234); +if (ret) + goto error_pm; + + ar0234->sd.internal_ops = &ar0234_internal_ops; + ar0234->sd.flags |=V4L2_SUBDEV_FL_HAS_DEVNODE | +V4L2_SUBDEV_FL_HAS_EVENTS ; + ar0234->sd.entity.ops = &ar0234_subdev_entity_ops; + ar0234->sd.entity.function =MEDIA_ENT_F_CAM_SENSOR ; + + ar0234->pad.flags =MEDIA_PAD_FL_SOURCE ; + ret =media_entity_pads_init (&ar0234->sd.entity,1 , &ar0234->pad); +if (ret) { +dev_err (dev,"Failed to init entity pads: %d\n" , ret); + goto error_pm; + } + + ar0234->sd.state_lock = ar0234->ctrls.lock; + + ret =v4l2_subdev_init_finalize (&ar0234->sd); +if (ret) { +dev_err (ar0234->dev,"Subdev init error\n" ); + goto error_media; + } + + ret =v4l2_async_register_subdev_sensor (&ar0234->sd); +if (ret) { +dev_err (dev,"Failed to register sensor sub-device: %d\n" , ret); + goto error_media; + } + +pm_runtime_put_autosuspend (dev); + +return 0 ; + +error_media: +media_entity_cleanup (&ar0234->sd.entity); + +error_pm: +pm_runtime_disable (ar0234->dev); +pm_runtime_put_noidle (ar0234->dev); +ar0234_power_off (ar0234->dev); + +err_subdev: +ar0234_subdev_cleanup (ar0234); + +return ret; +} + +static void ar0234_remove (struct i2c_client *client) +{ +struct v4l2_subdev *sd =i2c_get_clientdata (client); +struct ar0234 *ar0234 =to_ar0234 (sd); +
drivers/media/i2c/ar0234.c line 1280
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+v4l2_async_unregister_subdev (sd); +ar0234_subdev_cleanup (ar0234); + +pm_runtime_disable (&client->dev); +if (!pm_runtime_status_suspended (&client->dev)) +ar0234_power_off (&client->dev); +pm_runtime_set_suspended (&client->dev); +} + +static const struct acpi_device_id ar0234_acpi_ids[]__maybe_unused = {
Is __maybe_unused needed given thatar0234_acpi_ids is referenced from +MODULE_DEVICE_TABLE() ?
drivers/media/i2c/ar0234.c line 1286
Quoted 6 lines
drivers/media/i2c/ar0234.c new file +6
-0+ { "INTC10C0" }, + { } +}; +MODULE_DEVICE_TABLE (acpi, ar0234_acpi_ids); + +static const struct of_device_id ar0234_dt_ids[]__maybe_unused = {
Same here.
Quoted 23 lines
drivers/media/i2c/ar0234.c new file +23
-0+ { .compatible = "onnn,ar0234cs" }, + { } +}; +MODULE_DEVICE_TABLE (of, ar0234_dt_ids); + +static DEFINE_RUNTIME_DEV_PM_OPS(ar0234_pm_ops, ar0234_power_off, + ar0234_power_on, NULL); + +static struct i2c_driver ar0234_i2c_driver = { + .driver = { + .name ="ar0234" , + .acpi_match_table =ACPI_PTR (ar0234_acpi_ids), + .of_match_table =of_match_ptr (ar0234_dt_ids), + .pm =pm_ptr (&ar0234_pm_ops), + }, + .probe = ar0234_probe, + .remove = ar0234_remove, +}; +module_i2c_driver (ar0234_i2c_driver); + +MODULE_DESCRIPTION ("onsemi AR0234 Camera Sensor Driver" ); +MODULE_AUTHOR ("Alexander Shiyan <[email protected]>" ); +MODULE_LICENSE ("GPL" );
Signature · 3 lines
Regards, Laurent Pinchart
Re: [RFC PATCH v3 2/2] media: i2c: Add onsemi AR0234 image sensor driver
Hello Laurent.
Quoted 4 lines
Thank you for the patch.
Quoted 2 lines · Alexander Shiyan
Add driver for the onsemi AR0234 CMOS image sensor.
Signed-off-by Alexander Shiyan <[email protected]>
...
Quoted 4 lines
Quoted 4 lines
drivers/media/i2c/ar0234.c new file +4
-0+#define AR0234_NATIVE_WIDTH (1940U) +#define AR0234_NATIVE_HEIGHT (1220U) +#define AR0234_PIXEL_ARRAY_LEFT (8U) +#define AR0234_PIXEL_ARRAY_TOP (8U)
Quoted 8 lines
drivers/media/i2c/ar0234.c line 105
Quoted 6 lines
drivers/media/i2c/ar0234.c new file +6
-0+#define AR0234_PIXEL_ARRAY_WIDTH (1920U) +#define AR0234_PIXEL_ARRAY_HEIGHT (1200U) +#define AR0234_MIN_CROP_WIDTH (4U) +#define AR0234_MIN_CROP_HEIGHT (2U) +#define AR0234_CROP_WIDTH_STEP (4U) +#define AR0234_CROP_HEIGHT_STEP (2U)Where do those two step values come from ?
The sensor’s line length register (LINE_LENGTH_PCK) is calculated as:line_length_pck = (crop->width / 4) + hblank
The width is divided by 4 because the sensor outputs 4 pixels per pixel clock. The step of 4 ensures a smooth, glitch‑free horizontal blanking range that always aligns with the sensor’s internal datapath.
AR0234_CROP_HEIGHT_STEP: The sensor currently does not impose a strict multiple‑of‑2 requirement on the height in normal mode. The step of 2 is a forward‑looking choice, intended to simplify adding row binning support later. When row binning is enabled, the datasheet (AND9812‑D, register READ_MODE) states that y_addr_start must be even. Using a step of 2 now avoids future ABI changes.
Thanks!
Re: [RFC PATCH v3 2/2] media: i2c: Add onsemi AR0234 image sensor driver
In reply to Laurent PinchartHello Laurent.
Quoted 2 lines · Alexander Shiyan
Quoted 1 line
Add driver for the onsemi AR0234 CMOS image sensor.
...
Quoted 17 lines
drivers/media/i2c/ar0234.c line 150
Quoted 12 lines
drivers/media/i2c/ar0234.c new file +12
-0+enum ar0234_link_freq_index { +AR0234_LINK_FREQ_IDX_BPP_8 , +AR0234_LINK_FREQ_IDX_BPP_10 , +AR0234_LINK_FREQ_IDX_MAX +}; + +struct ar0234_mode { +u8 bpp_in; +u8 bpp_out; +u8 dpcm; +u8 mipi_dt; +int link_freq_index;I think you can drop this. The driver already uses the CCS PLL calculator, so it should be able to deal with user-selectable link frequencies.
Quoted 8 lines
This would require testing all frequencies specified in DT at probe time to map them to bus formats, and restricting the formats accepted by the driver based on the current link frequency. The CCS driver implements such logic, and there's also an implementation in the ar0830 driver I'm working on (see [1]). Note to myself (and to Sakari too): this is a good candidate for a helper.
I reworked the driver to iterate over all link frequencies from the device tree and validate them against each mode using the CCS PLL calculator, as you suggested and as implemented in the ar0830 driver. The PLL calculations themselves succeed for both the 360 MHz and 450 MHz frequencies with both the 8‑bpp and 10‑bpp modes - the resulting bitmask was valid for all combinations.
Unfortunately, real‑hardware testing showed that the 8‑bpp mode does not produce a correct image (MIPI errors) at 450 MHz, and the 10‑bpp mode fails similarly at 360 MHz. The issue appears to be that the sensor requires different MIPI timing configurations (frame/line preambles, D‑PHY timings, etc.) for different lane rates, which are not yet implemented.
Since the PLL calculation alone is not sufficient to guarantee a working pipeline,
the automatic selection of any frequency for any mode leads to unstable operation.
For the time being I plan to revert the link frequency handling to the earlier approach where the device tree provides two specific frequencies and the driver maps them directly to the respective modes.
The DT binding will remain unchanged, so when proper MIPI timing configuration is added later we will be able to adopt the fully dynamic model without breaking existing device trees.
Thanks!
Re: [RFC PATCH v3 2/2] media: i2c: Add onsemi AR0234 image sensor driver
In reply to Alexander ShiyanQuoted 21 lines · Alexander Shiyan
drivers/media/i2c/ar0234.c new file +21
-0+ +static int ar0234_set_ctrl (struct v4l2_ctrl *ctrl) +{ +struct ar0234 *ar0234 =container_of (ctrl->handler, + struct ar0234, ctrls); +int ret =0 ; + +if (ctrl->flags &V4L2_CTRL_FLAG_READ_ONLY ) +return 0 ; + +if (ctrl->id ==V4L2_CID_VBLANK ) { +int exposure_max = ar0234->crop.height + ctrl->val -1 ; +int exposure_val =clamp (ar0234->exposure->val, +AR0234_EXPOSURE_MIN , exposure_max); + + ret =__v4l2_ctrl_modify_range (ar0234->exposure, +AR0234_EXPOSURE_MIN , + exposure_max, +AR0234_EXPOSURE_STEP , + exposure_val); +if (ret)
drivers/media/i2c/ar0234.c line 422
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+return ret; + } + +if (pm_runtime_get_if_in_use (ar0234->dev) ==0 ) +return 0 ; + +switch (ctrl->id) { +case V4L2_CID_HBLANK : +cci_write (ar0234->regmap,AR0234_REG_LINE_LENGTH_PCK , + (ar0234->crop.width /4 ) + ctrl->val, &ret);
should be (crop->width + ctrl->val) / 4, &ret) to keep the control in units of pixels
drivers/media/i2c/ar0234.c line 426
Quoted 4 lines
drivers/media/i2c/ar0234.c new file +4
-0+break ; +case V4L2_CID_VBLANK : +cci_write (ar0234->regmap,AR0234_REG_FRAME_LENGTH_LINES , + ar0234->crop.height + ctrl->val, &ret);
REG_FRAME_LENGTH_LINES seems to actually be total lines - 5
I had to make these 2 changes to be able to get the expected framerate when not using the default 120fps.
Quoted 5 lines
drivers/media/i2c/ar0234.c new file +5
-0+if (ret) +break ; + ctrl = ar0234->exposure; + fallthrough; +case V4L2_CID_EXPOSURE :
drivers/media/i2c/ar0234.c line 441
Quoted 10 lines
drivers/media/i2c/ar0234.c new file +10
-0+cci_write (ar0234->regmap,AR0234_REG_COARSE_INTEGRATION_TIME , + ctrl->val, &ret); +break ; +case V4L2_CID_ANALOGUE_GAIN : + ret =ar0234_set_analog_gain (ar0234, ctrl->val); +break ; +case V4L2_CID_DIGITAL_GAIN : +cci_write (ar0234->regmap,AR0234_REG_GLOBAL_GAIN , + ctrl->val, &ret); +break ;
- Quentin
Re: [RFC PATCH v3 2/2] media: i2c: Add onsemi AR0234 image sensor driver
Hello Quentin.
Quoted 1 line
Quoted 1 line
+static int
ar0234_set_ctrl(struct v4l2_ctrl *ctrl)
...
Quoted 7 lines
drivers/media/i2c/ar0234.c line 422
Quoted 4 lines
drivers/media/i2c/ar0234.c new file +4
-0+switch (ctrl->id) { +case V4L2_CID_HBLANK : +cci_write (ar0234->regmap,AR0234_REG_LINE_LENGTH_PCK , + (ar0234->crop.width /4 ) + ctrl->val, &ret);should be (
crop->width+ctrl->val) / 4, &ret) to keep the control in units of pixels
This also results in changes for AR0234_HBLANK_MIN and the step to control V4L2_CID_HBLANK, right?
Quoted 8 lines
drivers/media/i2c/ar0234.c line 426
Quoted 4 lines
drivers/media/i2c/ar0234.c new file +4
-0+break ; +case V4L2_CID_VBLANK : +cci_write (ar0234->regmap,AR0234_REG_FRAME_LENGTH_LINES , + ar0234->crop.height + ctrl->val, &ret);
REG_FRAME_LENGTH_LINESseems to actually be total lines - 5I had to make these 2 changes to be able to get the expected framerate when not using the default 120fps.
Yes, I found it in the datasheet, thanks.
Quoted 6 lines · Alexander Shiyan
This series adds a driver for the onsemi AR0234 CMOS image sensor. The AR0234 is a 1/2.6-inch global-shutter sensor with a 1940x1220 pixel array, capable of 1920x1200 resolution at up to 120 fps. It supports MIPI CSI-2 output with 1 to 4 data lanes, raw Bayer (8/10-bit) and monochrome formats, as well as DPCM 10->8 compression.
Quoted 7 lines
The driver has been tested with 2 and 4 lanes on an ARM64 Rockchip RK3568 platform with a 27 MHz external clock. Both 8-bit and 10-bit raw Bayer modes are functional.
Notes:
- 1-lane mode is currently disabled; attempts to use it produced no valid image. Further investigation is needed.
That's a fair limitation for the time being.
Quoted 3 lines
- The driver uses a private streaming flag to protect cropping changes during streaming. Is this the recommended approach, or should we rely solely on the subdev state?
Is there a reason not to use v4l2_subdev_is_streaming() ?
Quoted 3 lines
- The DPCM (10->8 compression) mode is included in the code but could not be tested due to lack of suitable hardware; any testing help would be appreciated.
The only upstream drivers that implement DPCM support are omap3isp and atomisp. It will be difficult to get hold of a hardware setup that include an AR0234 :-(
Quoted 44 lines
Changes since v2:
- Added devicetree binding documentation for the onsemi AR0234 sensor.
- Added support for 8-bit raw Bayer output (verified working).
- Added DPCM 10->8 compression mode (untested, included for completeness).
- Reworked mode handling: each mode now specifies input/output bpp, DPCM flag, MIPI data type, and link frequency index.
- Reworked link frequency handling: the driver now accepts any valid link frequencies from the device tree. It expects two frequencies - one for 8-bit mode and one for 10-bit mode - but does not enforce a fixed set; frequencies are validated by attempting PLL calculation. This makes the driver compatible with a wider range of system configurations.
- Updatedar0234_calculate_pll()to use a temporary structure and update cached PLL only on success.Changes since v1:
- Improved error handling: usecci_write()with &ret chaining for sequential register writes, as suggested by Isaac Scott.
- Refactored format and cropping support:
Replaced static format list with dynamic cropping rectangle (structv4l2_rectcrop).
Implementedget_selectionandset_selectionforV4L2_SEL_TGT_CROP, allowing runtime selection of the active sensor area.
- Migrated to modern streaming model: replaceds_streamwith enable_streams/disable_streams usingv4l2_subdev_s_stream_helper.
- Corrected blanking constants: replaced ambiguousAR0234_HBLANK_DEFwithAR0234_LINE_LENGTH_PCK_MIN; updated min/max ranges.
- Added ACPI match table (untested).
- Style fixes.Any further comments or test results would be greatly appreciated.
Alexander Shiyan (2):
dt-bindings: media: i2c: Add onsemi AR0234 image sensor binding
media: i2c: Add onsemi AR0234 image sensor driver.../bindings/media/i2c/onnn,ar0234.yaml | 109 ++ drivers/media/i2c/Kconfig | 12 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ar0234.c | 1309 +++++++++++++++++ 4 files changed, 1431 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml create mode 100644 drivers/media/i2c/ar0234.c
Signature · 3 lines
Regards, Laurent Pinchart