[PATCH v2 0/2] media: i2c: Add driver for Sony IMX728

[PATCH v2 0/2] media: i2c: Add driver for Sony IMX728

[Spencer Hill]

Add a v4l2 sensor driver for Sony IMX728

v1->v2:
 * Elaborate on device details in dt-bindings.
 * Add links and voltage supplies to dt-bindings.
 * Re-order so that dt-bindings are applied first.
 * Move pattern gen config into v4l2_ctrls.
 * Switch to using CCI functions to access the sensor.
 * Merge header into c file.
 * Move verbose prints to dev_dbg.
 * Add missing v4l2 format params.
 * Added support for software reset if no hardware one is available.
 * Move register constants into defines, these are vague as Sony
   requested that I not use register names from the datasheet.

v4l2-compliance 1.24.1, 64 bits, 64-bit time_t

Compliance test for device /dev/v4l-subdev4:

Driver Info:
        Driver version   : 6.1.80
        Capabilities     : 0x00000002
                Streams Support

Required ioctls:
        test VIDIOC_SUDBEV_QUERYCAP: OK
        test invalid ioctls: OK

Allow for multiple opens:
        test second /dev/v4l-subdev4 open: OK
        test VIDIOC_SUBDEV_QUERYCAP: OK
        test for unlimited opens: OK

Debug ioctls:
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
        test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
        test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls:
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
        test VIDIOC_QUERYCTRL: OK
        test VIDIOC_G/S_CTRL: OK
        test VIDIOC_G/S/TRY_EXT_CTRLS: OK
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 12 Private Controls: 0

Format ioctls:
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported)
        test VIDIOC_G/S_PARM: OK (Not Supported)
        test VIDIOC_G_FBUF: OK (Not Supported)
        test VIDIOC_G_FMT: OK (Not Supported)
        test VIDIOC_TRY_FMT: OK (Not Supported)
        test VIDIOC_S_FMT: OK (Not Supported)
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK (Not Supported)

Codec ioctls:
        test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
        test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported)
        test VIDIOC_EXPBUF: OK (Not Supported)
        test Requests: OK (Not Supported)

Total for device /dev/v4l-subdev4: 43, Succeeded: 43, Failed: 0, Warnings: 0

Spencer Hill (2):
  media: i2c: Add driver for Sony IMX728
  media: dt-bindings: Add Sony IMX728

 .../bindings/media/i2c/sony,imx728.yaml       |   78 +
 MAINTAINERS                                   |    9 +
 drivers/media/i2c/Kconfig                     |   11 +
 drivers/media/i2c/Makefile                    |    1 +
 drivers/media/i2c/imx728.c                    | 1167 ++++++
 drivers/media/i2c/imx728.h                    | 3458 +++++++++++++++++
 6 files changed, 4724 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx728.yaml
 create mode 100644 drivers/media/i2c/imx728.c
 create mode 100644 drivers/media/i2c/imx728.h

--
2.40.1

---
Spencer Hill (2):
      media: dt-bindings: Add Sony IMX728
      media: i2c: Add driver for Sony IMX728

 .../devicetree/bindings/media/i2c/sony,imx728.yaml |  119 +
 MAINTAINERS                                        |    7 +
 drivers/media/i2c/Kconfig                          |   11 +
 drivers/media/i2c/Makefile                         |    1 +
 drivers/media/i2c/imx728.c                         | 4660 ++++++++++++++++++++
 5 files changed, 4798 insertions(+)
---
base-commit: 8771b7f31b7fff91a998e6afdb60650d4bac59a5
change-id: 20240627-imx728-driver-a8d905cafd2d

Best regards,
--
Spencer Hill <shill@d3engineering.com>

Please be aware that this email includes email addresses outside of the organization.

[PATCH v2 1/2] media: dt-bindings: Add Sony IMX728

[Spencer Hill]

Add bindings for Sony IMX728.

Signed-off-by: Spencer Hill <shill@d3engineering.com>
---
 .../devicetree/bindings/media/i2c/sony,imx728.yaml | 119 +++++++++++++++++++++
 MAINTAINERS                                        |   7 ++
 2 files changed, 126 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx728.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx728.yaml
new file mode 100644
index 000000000000..1b14dbcc473a
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx728.yaml
@@ -0,0 +1,119 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/sony,imx728.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sony IMX728 Camera Sensor
+
+maintainers:
+  - Spencer Hill <shill@d3engineering.com>
+
+description:
+  The Sony IMX728 is a 1/1.72-Inch CMOS Solid-state image sensor with a
+  color square pixel array and 8.39M active pixels. It is programmed
+  through an I2C interface.
+
+  The sensor can output up to 3840x2160 at a maximum of 45 frames/s over
+  a CSI-2 serial interface. It supports RAW24/20/16/12 and 10.
+
+properties:
+  compatible:
+    enum:
+      - sony,imx728
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: Clock frequency from 18 to 30MHz
+    maxItems: 1
+
+  clock-names:
+    const: inck
+
+  reset-gpios:
+    maxItems: 1
+    description:
+      Specifier for the GPIO connected to the XCLR (System Reset) pin.
+
+  avdd-supply:
+    description: Analog power supply (3.3V)
+
+  dvdd-supply:
+    description: Digital core power supply (1.1V)
+
+  ovdd-supply:
+    description: Digital I/O power supply (1.8V)
+
+  port:
+    $ref: /schemas/graph.yaml#/properties/port
+    additionalProperties: false
+
+    properties:
+      endpoint:
+        $ref: ../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:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        clock-frequency = <400000>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        camera@1a {
+            compatible = "sony,imx728";
+            reg = <0x1a>;
+
+            clocks = <&fixed_clock>;
+            clock-names = "inck";
+
+            reset-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
+
+            avdd-supply = <&camera_vdda_3v3>;
+            dvdd-supply = <&camera_vddd_1v1>;
+            ovdd-supply = <&camera_vddo_1v8>;
+
+            port {
+                camera1: endpoint {
+                    remote-endpoint = <&csi2_phy0>;
+                    data-lanes = <1 2 3 4>;
+                    link-frequencies = /bits/ 64 <800000000>;
+                };
+            };
+        };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index ef6be9d95143..a2811249ac8c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20589,6 +20589,13 @@ T:     git git://linuxtv.org/media_tree.git
 F:     Documentation/devicetree/bindings/media/i2c/sony,imx415.yaml
 F:     drivers/media/i2c/imx415.c

+SONY IMX728 SENSOR DRIVER
+M:     Spencer Hill <shill@d3engineering.com>
+L:     linux-media@vger.kernel.org
+S:     Maintained
+F:     Documentation/devicetree/bindings/media/i2c/sony,imx728.yaml
+F:     drivers/media/i2c/imx728.c
+
 SONY MEMORYSTICK SUBSYSTEM
 M:     Maxim Levitsky <maximlevitsky@gmail.com>
 M:     Alex Dubov <oakad@yahoo.com>

--
2.43.0

Please be aware that this email includes email addresses outside of the organization.

Re: [PATCH v2 2/2] media: i2c: Add driver for Sony IMX728

[Markus Elfring]

…
> +++ b/drivers/media/i2c/imx728.c
> @@ -0,0 +1,4660 @@
…
> +static int imx728_set_fmt(struct v4l2_subdev *sd,
> +                         struct v4l2_subdev_state *state,
> +                         struct v4l2_subdev_format *fmt)
> +{
…
> +       mutex_lock(&imx728->lock);
> +
> +       fmt->format.field = V4L2_FIELD_NONE;
…
> +       *format = fmt->format;
> +
> +done:
> +       mutex_unlock(&imx728->lock);
> +
> +       return ret;
> +}
…

Under which circumstances would you become interested to apply a statement
like “guard(mutex)(&imx728->lock);”?
https://elixir.bootlin.com/linux/v6.10-rc5/source/include/linux/mutex.h#L196

Regards,
Markus

Re: [PATCH v2 2/2] media: i2c: Add driver for Sony IMX728

[Alexander Stein]

Hi,

Am Freitag, 28. Juni 2024, 23:17:01 CEST schrieb Spencer Hill:
> Add a driver for the Sony IMX728 image sensor.
> 
> Signed-off-by: Spencer Hill <shill@d3engineering.com>
> ---
>  drivers/media/i2c/Kconfig  |   11 +
>  drivers/media/i2c/Makefile |    1 +
>  drivers/media/i2c/imx728.c | 4660 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 4672 insertions(+)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index c6d3ee472d81..46b6463c558a 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -233,6 +233,17 @@ config VIDEO_IMX415
>           To compile this driver as a module, choose M here: the
>           module will be called imx415.
> 
> +config VIDEO_IMX728
> +       tristate "Sony IMX728 sensor support"
> +       depends on OF_GPIO
> +       select V4L2_CCI_I2C
> +       help
> +         This is a Video4Linux2 sensor driver for the Sony
> +         IMX728 camera.
> +
> +         To compile this driver as a module, choose M here: the
> +         module will be called imx728.
> +
>  config VIDEO_MAX9271_LIB
>         tristate
> 
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index dfbe6448b549..1188420ee1b4 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -56,6 +56,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_IMX728) += imx728.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/imx728.c b/drivers/media/i2c/imx728.c
> new file mode 100644
> index 000000000000..190f54aaf4e9
> --- /dev/null
> +++ b/drivers/media/i2c/imx728.c
> @@ -0,0 +1,4660 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sony IMX728 CMOS Image Sensor Driver
> + *
> + * Copyright (c) 2024 Define Design Deploy Corp
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/clk.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/v4l2-mediabus.h>
> +#include <linux/videodev2.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-cci.h>
> +
> +#define IMX728_FRAMERATE_MAX           30

Bindings state the framerate is up to 45 fps, to this should be
set accordingly.

> +#define IMX728_FRAMERATE_DEFAULT       30
> +#define IMX728_FRAMERATE_MIN           10
> +
> +#define IMX728_PIXEL_ARRAY_WIDTH       3857
> +#define IMX728_PIXEL_ARRAY_HEIGHT      2177
> +#define IMX728_PIXEL_ARRAY_MARGIN_TOP  9
> +#define IMX728_PIXEL_ARRAY_MARGIN_LEFT 8
> +#define IMX728_PIXEL_ARRAY_RECORDING_WIDTH     3840
> +#define IMX728_PIXEL_ARRAY_RECORDING_HEIGHT    2160
> +
> +#define IMX728_PIXEL_RATE              248832000
> +#define IMX728_LINK_FREQ               800000000
> +
> +#define IMX728_EXPOSURE_DEFAULT                10000
> +
> +#define IMX728_PM_IDLE_TIMEOUT         1000
> +
> +#define IMX728_REG_STATE       CCI_REG8(0x2CAC)
> +#define IMX728_REG_PG_00       CCI_REG16_LE(0x1A2A)
> +#define IMX728_REG_PG_01       CCI_REG24_LE(0x1A30)
> +#define IMX728_REG_PG_02       CCI_REG24_LE(0x1A38)
> +#define IMX728_REG_PG_03       CCI_REG8(0xB58F)
> +#define IMX728_REG_PG_04       CCI_REG8(0xB6C5)
> +#define IMX728_REG_PG_05       CCI_REG16_LE(0x1A2C)
> +#define IMX728_REG_PG_06       CCI_REG8(0xB58E)
> +#define IMX728_REG_PG_07       CCI_REG8(0xB6C4)
> +#define IMX728_REG_EXPOSURE_00 CCI_REG32_LE(0x98DC)
> +#define IMX728_REG_EXPOSURE_01 CCI_REG32_LE(0x98E4)
> +#define IMX728_REG_EXPOSURE_02 CCI_REG32_LE(0x98EC)
> +#define IMX728_REG_AGAIN_00    CCI_REG32_LE(0x98F8)
> +#define IMX728_REG_AGAIN_01    CCI_REG32_LE(0x98FC)
> +#define IMX728_REG_AGAIN_02    CCI_REG32_LE(0x9900)
> +#define IMX728_REG_AGAIN_03    CCI_REG32_LE(0x9904)
> +#define IMX728_REG_AGAIN_04    CCI_REG32_LE(0x9908)
> +#define IMX728_REG_FLIP                CCI_REG8(0x9651)
> +#define IMX728_REG_HFLIP       CCI_REG8(0xB67C)
> +#define IMX728_REG_VFLIP       CCI_REG8(0xB67D)
> +#define IMX728_REG_VMINOR      CCI_REG8(0x6000)
> +#define IMX728_REG_VMAJOR      CCI_REG8(0x6002)
> +#define IMX728_REG_RESET_0     CCI_REG8(0xB661)
> +#define IMX728_REG_RESET_1     CCI_REG8(0x95C5)
> +#define IMX728_REG_INCK_0      CCI_REG8(0x1B20)
> +#define IMX728_REG_INCK_1      CCI_REG8(0x1B1C)
> +#define IMX728_REG_SLEEP       CCI_REG8(0x1B05)
> +#define IMX728_REG_REGMAP      CCI_REG8(0xFFFF)
> +#define IMX728_REG_HDR_00      CCI_REG32_LE(0x9C60)
> +#define IMX728_REG_HDR_01      CCI_REG32_LE(0x9C6C)
> +#define IMX728_REG_HDR_02      CCI_REG32_LE(0x9C64)
> +#define IMX728_REG_HDR_03      CCI_REG32_LE(0x9C70)
> +#define IMX728_REG_HDR_04      CCI_REG16_LE(0x9C68)
> +#define IMX728_REG_HDR_05      CCI_REG16_LE(0x9C74)
> +#define IMX728_REG_HDR_06      CCI_REG16_LE(0x9C6A)
> +#define IMX728_REG_HDR_07      CCI_REG16_LE(0x9C76)
> +#define IMX728_REG_AE_MODE     CCI_REG8(0x98AC)
> +#define IMX728_REG_AWBMODE     CCI_REG8(0xA248)
> +#define IMX728_REG_AWB_EN      CCI_REG8(0x1808)
> +#define IMX728_REG_UNIT_00     CCI_REG8(0x98E0)
> +#define IMX728_REG_UNIT_01     CCI_REG8(0x98E8)
> +#define IMX728_REG_UNIT_02     CCI_REG8(0x98F0)
> +#define IMX728_REG_MD_00       CCI_REG8(0x1708)
> +#define IMX728_REG_MD_01       CCI_REG8(0x1709)
> +#define IMX728_REG_MD_02       CCI_REG8(0x170A)
> +#define IMX728_REG_MD_03       CCI_REG8(0x1B40)
> +#define IMX728_REG_MODE_SEL    CCI_REG16_LE(0x9728)
> +#define IMX728_REG_OUT_MODE    CCI_REG8(0xEC7E)
> +#define IMX728_REG_OB_0                CCI_REG16_LE(0xEC12)
> +#define IMX728_REG_OB_1                CCI_REG8(0xEC14)
> +#define IMX728_REG_SKEW                CCI_REG8(0x1761)
> +#define IMX728_REG_SUBP_0      CCI_REG8(0x9714)
> +#define IMX728_REG_SUBP_1      CCI_REG8(0xB684)
> +#define IMX728_REG_STREAM_00   CCI_REG8(0x9789)
> +#define IMX728_REG_STREAM_01   CCI_REG8(0x95C1)
> +#define IMX728_REG_STREAM_02   CCI_REG8(0x1B04)

Can you sort them by register address?

> +#define IMX728_REG_CTRL_POINT_X(i) CCI_REG32(0xA198 + (i) * 8)
> +#define IMX728_REG_CTRL_POINT_Y(i) (IMX728_REG_CTRL_POINT_X(i) + 4)

>[snip]

Best regards,
Alexander

> --
> 2.43.0
> 
> Please be aware that this email includes email addresses outside of the organization.
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v2 1/2] media: dt-bindings: Add Sony IMX728

[Krzysztof Kozlowski]

On 28/06/2024 23:17, Spencer Hill wrote:
> Add bindings for Sony IMX728.
> 
> Signed-off-by: Spencer Hill <shill@d3engineering.com>


> +
> +  clocks:
> +    description: Clock frequency from 18 to 30MHz
> +    maxItems: 1
> +
> +  clock-names:
> +    const: inck

clock-names do not seem that useful - name is pretty obvious. Drop.

> +
> +  reset-gpios:
> +    maxItems: 1
> +    description:
> +      Specifier for the GPIO connected to the XCLR (System Reset) pin.
> +
> +  avdd-supply:
> +    description: Analog power supply (3.3V)
> +
> +  dvdd-supply:
> +    description: Digital core power supply (1.1V)
> +
> +  ovdd-supply:
> +    description: Digital I/O power supply (1.8V)
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/properties/port
> +    additionalProperties: false
> +
> +    properties:
> +      endpoint:
> +        $ref: ../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, not needed.

> +
> +        required:
> +          - data-lanes
> +          - link-frequencies
> +
> +    required:
> +      - endpoint
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        clock-frequency = <400000>;

Drop

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        camera@1a {
> +            compatible = "sony,imx728";
> +            reg = <0x1a>;
> +
> +            clocks = <&fixed_clock>;
> +            clock-names = "inck";
> +
> +            reset-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> +
> +            avdd-supply = <&camera_vdda_3v3>;
> +            dvdd-supply = <&camera_vddd_1v1>;
> +            ovdd-supply = <&camera_vddo_1v8>;
> +
> +            port {
> +                camera1: endpoint {
> +                    remote-endpoint = <&csi2_phy0>;
> +                    data-lanes = <1 2 3 4>;
> +                    link-frequencies = /bits/ 64 <800000000>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ef6be9d95143..a2811249ac8c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20589,6 +20589,13 @@ T:     git git://linuxtv.org/media_tree.git
>  F:     Documentation/devicetree/bindings/media/i2c/sony,imx415.yaml
>  F:     drivers/media/i2c/imx415.c
> 
> +SONY IMX728 SENSOR DRIVER
> +M:     Spencer Hill <shill@d3engineering.com>
> +L:     linux-media@vger.kernel.org
> +S:     Maintained
> +F:     Documentation/devicetree/bindings/media/i2c/sony,imx728.yaml
> +F:     drivers/media/i2c/imx728.c

There is no such file. Patches must be bisectable.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] media: i2c: Add driver for Sony IMX728

[Dave Stevenson]

Hi Alexander

On Mon, 1 Jul 2024 at 10:25, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi,
>
> Am Freitag, 28. Juni 2024, 23:17:01 CEST schrieb Spencer Hill:
> > Add a driver for the Sony IMX728 image sensor.
> >
> > Signed-off-by: Spencer Hill <shill@d3engineering.com>
> > ---
> >  drivers/media/i2c/Kconfig  |   11 +
> >  drivers/media/i2c/Makefile |    1 +
> >  drivers/media/i2c/imx728.c | 4660 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 4672 insertions(+)
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index c6d3ee472d81..46b6463c558a 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -233,6 +233,17 @@ config VIDEO_IMX415
> >           To compile this driver as a module, choose M here: the
> >           module will be called imx415.
> >
> > +config VIDEO_IMX728
> > +       tristate "Sony IMX728 sensor support"
> > +       depends on OF_GPIO
> > +       select V4L2_CCI_I2C
> > +       help
> > +         This is a Video4Linux2 sensor driver for the Sony
> > +         IMX728 camera.
> > +
> > +         To compile this driver as a module, choose M here: the
> > +         module will be called imx728.
> > +
> >  config VIDEO_MAX9271_LIB
> >         tristate
> >
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index dfbe6448b549..1188420ee1b4 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -56,6 +56,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_IMX728) += imx728.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/imx728.c b/drivers/media/i2c/imx728.c
> > new file mode 100644
> > index 000000000000..190f54aaf4e9
> > --- /dev/null
> > +++ b/drivers/media/i2c/imx728.c
> > @@ -0,0 +1,4660 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Sony IMX728 CMOS Image Sensor Driver
> > + *
> > + * Copyright (c) 2024 Define Design Deploy Corp
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/clk.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/types.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/v4l2-mediabus.h>
> > +#include <linux/videodev2.h>
> > +#include <media/v4l2-subdev.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-event.h>
> > +#include <media/v4l2-cci.h>
> > +
> > +#define IMX728_FRAMERATE_MAX           30
>
> Bindings state the framerate is up to 45 fps, to this should be
> set accordingly.

Not necessarily. The binding describes the hardware, not the
functionality implemented by the driver.

The link frequency defined may not be sufficient to support 45fps -
perfectly valid.
The binding also lists that it can produce RAW24/20/16/12 and 10, but
the driver only currently implements RAW10. Hopefully the others get
added at a later date.

The bindings for many sensors advertise a wide range of potential
input clock frequencies, but most only implement a 1 or 2. (I'm
slightly surprised this sensor appears to be able to configure all
PLLs just by being told the frequency of the clock, but it's
possible).
Many sensors could support using a different number of CSI2 data lanes
and reference it in their bindings, but currently don't.

AIUI All of those are acceptable to be stated in the bindings but not
currently implemented in the drivers.


What does appear to be an issue here is that setting the frame
interval appears to never be programmed into the sensor, so you have
no frame rate control (imx728->fps is never read other than in
imx728_get_frame_interval). There's no point in advertising the
capability if it isn't connected up.
If frame rate/interval control was hooked up, it'd be nice if that
also set the maximum exposure time based on the frame interval.

  Dave

> > +#define IMX728_FRAMERATE_DEFAULT       30
> > +#define IMX728_FRAMERATE_MIN           10
> > +
> > +#define IMX728_PIXEL_ARRAY_WIDTH       3857
> > +#define IMX728_PIXEL_ARRAY_HEIGHT      2177
> > +#define IMX728_PIXEL_ARRAY_MARGIN_TOP  9
> > +#define IMX728_PIXEL_ARRAY_MARGIN_LEFT 8
> > +#define IMX728_PIXEL_ARRAY_RECORDING_WIDTH     3840
> > +#define IMX728_PIXEL_ARRAY_RECORDING_HEIGHT    2160
> > +
> > +#define IMX728_PIXEL_RATE              248832000
> > +#define IMX728_LINK_FREQ               800000000
> > +
> > +#define IMX728_EXPOSURE_DEFAULT                10000
> > +
> > +#define IMX728_PM_IDLE_TIMEOUT         1000
> > +
> > +#define IMX728_REG_STATE       CCI_REG8(0x2CAC)
> > +#define IMX728_REG_PG_00       CCI_REG16_LE(0x1A2A)
> > +#define IMX728_REG_PG_01       CCI_REG24_LE(0x1A30)
> > +#define IMX728_REG_PG_02       CCI_REG24_LE(0x1A38)
> > +#define IMX728_REG_PG_03       CCI_REG8(0xB58F)
> > +#define IMX728_REG_PG_04       CCI_REG8(0xB6C5)
> > +#define IMX728_REG_PG_05       CCI_REG16_LE(0x1A2C)
> > +#define IMX728_REG_PG_06       CCI_REG8(0xB58E)
> > +#define IMX728_REG_PG_07       CCI_REG8(0xB6C4)
> > +#define IMX728_REG_EXPOSURE_00 CCI_REG32_LE(0x98DC)
> > +#define IMX728_REG_EXPOSURE_01 CCI_REG32_LE(0x98E4)
> > +#define IMX728_REG_EXPOSURE_02 CCI_REG32_LE(0x98EC)
> > +#define IMX728_REG_AGAIN_00    CCI_REG32_LE(0x98F8)
> > +#define IMX728_REG_AGAIN_01    CCI_REG32_LE(0x98FC)
> > +#define IMX728_REG_AGAIN_02    CCI_REG32_LE(0x9900)
> > +#define IMX728_REG_AGAIN_03    CCI_REG32_LE(0x9904)
> > +#define IMX728_REG_AGAIN_04    CCI_REG32_LE(0x9908)
> > +#define IMX728_REG_FLIP                CCI_REG8(0x9651)
> > +#define IMX728_REG_HFLIP       CCI_REG8(0xB67C)
> > +#define IMX728_REG_VFLIP       CCI_REG8(0xB67D)
> > +#define IMX728_REG_VMINOR      CCI_REG8(0x6000)
> > +#define IMX728_REG_VMAJOR      CCI_REG8(0x6002)
> > +#define IMX728_REG_RESET_0     CCI_REG8(0xB661)
> > +#define IMX728_REG_RESET_1     CCI_REG8(0x95C5)
> > +#define IMX728_REG_INCK_0      CCI_REG8(0x1B20)
> > +#define IMX728_REG_INCK_1      CCI_REG8(0x1B1C)
> > +#define IMX728_REG_SLEEP       CCI_REG8(0x1B05)
> > +#define IMX728_REG_REGMAP      CCI_REG8(0xFFFF)
> > +#define IMX728_REG_HDR_00      CCI_REG32_LE(0x9C60)
> > +#define IMX728_REG_HDR_01      CCI_REG32_LE(0x9C6C)
> > +#define IMX728_REG_HDR_02      CCI_REG32_LE(0x9C64)
> > +#define IMX728_REG_HDR_03      CCI_REG32_LE(0x9C70)
> > +#define IMX728_REG_HDR_04      CCI_REG16_LE(0x9C68)
> > +#define IMX728_REG_HDR_05      CCI_REG16_LE(0x9C74)
> > +#define IMX728_REG_HDR_06      CCI_REG16_LE(0x9C6A)
> > +#define IMX728_REG_HDR_07      CCI_REG16_LE(0x9C76)
> > +#define IMX728_REG_AE_MODE     CCI_REG8(0x98AC)
> > +#define IMX728_REG_AWBMODE     CCI_REG8(0xA248)
> > +#define IMX728_REG_AWB_EN      CCI_REG8(0x1808)
> > +#define IMX728_REG_UNIT_00     CCI_REG8(0x98E0)
> > +#define IMX728_REG_UNIT_01     CCI_REG8(0x98E8)
> > +#define IMX728_REG_UNIT_02     CCI_REG8(0x98F0)
> > +#define IMX728_REG_MD_00       CCI_REG8(0x1708)
> > +#define IMX728_REG_MD_01       CCI_REG8(0x1709)
> > +#define IMX728_REG_MD_02       CCI_REG8(0x170A)
> > +#define IMX728_REG_MD_03       CCI_REG8(0x1B40)
> > +#define IMX728_REG_MODE_SEL    CCI_REG16_LE(0x9728)
> > +#define IMX728_REG_OUT_MODE    CCI_REG8(0xEC7E)
> > +#define IMX728_REG_OB_0                CCI_REG16_LE(0xEC12)
> > +#define IMX728_REG_OB_1                CCI_REG8(0xEC14)
> > +#define IMX728_REG_SKEW                CCI_REG8(0x1761)
> > +#define IMX728_REG_SUBP_0      CCI_REG8(0x9714)
> > +#define IMX728_REG_SUBP_1      CCI_REG8(0xB684)
> > +#define IMX728_REG_STREAM_00   CCI_REG8(0x9789)
> > +#define IMX728_REG_STREAM_01   CCI_REG8(0x95C1)
> > +#define IMX728_REG_STREAM_02   CCI_REG8(0x1B04)
>
> Can you sort them by register address?
>
> > +#define IMX728_REG_CTRL_POINT_X(i) CCI_REG32(0xA198 + (i) * 8)
> > +#define IMX728_REG_CTRL_POINT_Y(i) (IMX728_REG_CTRL_POINT_X(i) + 4)
>
> >[snip]
>
> Best regards,
> Alexander
>
> > --
> > 2.43.0
> >
> > Please be aware that this email includes email addresses outside of the organization.
> >
>
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>

Re: [PATCH v2 1/2] media: dt-bindings: Add Sony IMX728

[Rob Herring (Arm)]

On Fri, 28 Jun 2024 17:17:00 -0400, Spencer Hill wrote:
> Add bindings for Sony IMX728.
> 
> Signed-off-by: Spencer Hill <shill@d3engineering.com>
> ---
>  .../devicetree/bindings/media/i2c/sony,imx728.yaml | 119 +++++++++++++++++++++
>  MAINTAINERS                                        |   7 ++
>  2 files changed, 126 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/media/i2c/sony,imx728.yaml:62:17: [warning] wrong indentation: expected 18 but found 16 (indentation)
./Documentation/devicetree/bindings/media/i2c/sony,imx728.yaml:65:17: [warning] wrong indentation: expected 18 but found 16 (indentation)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/i2c/sony,imx728.example.dtb: camera@1a: port:endpoint: Unevaluated properties are not allowed ('data-lanes', 'link-frequencies' were unexpected)
	from schema $id: http://devicetree.org/schemas/media/i2c/sony,imx728.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240628-imx728-driver-v2-1-80efa6774286@d3engineering.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 1/2] media: dt-bindings: Add Sony IMX728

[Krzysztof Kozlowski]

On 01/07/2024 11:36, Krzysztof Kozlowski wrote:
> On 28/06/2024 23:17, Spencer Hill wrote:
>> Add bindings for Sony IMX728.
>>
>> Signed-off-by: Spencer Hill <shill@d3engineering.com>
> 
> 
>> +
>> +  clocks:
>> +    description: Clock frequency from 18 to 30MHz
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    const: inck
> 
> clock-names do not seem that useful - name is pretty obvious. Drop.

... and the binding was not tested. You must test them before asking
maintainers to review. Otherwise we will just ignore the patches. :(


Best regards,
Krzysztof