[PATCH 0/2] Add imx577 compatible to imx412

[PATCH 0/2] Add imx577 compatible to imx412

[Bryan O'Donoghue]

Right now the imx412 and imx577 are code and pin compatible however, they
are distinct pieces of silicon.

Document imx577 as a compatible enum and add the compat string to imx412.c.
This allows us to differentiate these chips in DTS and potentially to apply
any future imx412 or imx577 specific changes appropriately.

Bryan O'Donoghue (2):
  media: dt-bindings: imx412: Add imx577 compatible string
  media: i2c: imx412: Add imx577 compatible string

 Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml | 5 ++++-
 drivers/media/i2c/imx412.c                                   | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.36.1

[PATCH 1/2] media: dt-bindings: imx412: Add imx577 compatible string

[Bryan O'Donoghue]

The Sony IMX577 uses the same silicon enabling reference code from Sony in
the available examples provided.

Add an imx577 compatible string to allow for chip differentiation and
accurate description of hardware in dts.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
index 26d1807d0bb6..d1561841ccbc 100644
--- a/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
@@ -19,7 +19,10 @@ description:
 
 properties:
   compatible:
-    const: sony,imx412
+    items:
+      - enum:
+          - sony,imx412
+          - sony,imx577
   reg:
     description: I2C address
     maxItems: 1
-- 
2.36.1

Re: [PATCH 1/2] media: dt-bindings: imx412: Add imx577 compatible string

[Rob Herring]

On Tue, 07 Jun 2022 14:40:56 +0100, Bryan O'Donoghue wrote:
> The Sony IMX577 uses the same silicon enabling reference code from Sony in
> the available examples provided.
> 
> Add an imx577 compatible string to allow for chip differentiation and
> accurate description of hardware in dts.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/2] media: dt-bindings: imx412: Add imx577 compatible string

[Kieran Bingham]

Hi Bryan,

Quoting Bryan O'Donoghue (2022-06-07 14:40:56)
> The Sony IMX577 uses the same silicon enabling reference code from Sony in
> the available examples provided.
> 
> Add an imx577 compatible string to allow for chip differentiation and
> accurate description of hardware in dts.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>


Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
> index 26d1807d0bb6..d1561841ccbc 100644
> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
> @@ -19,7 +19,10 @@ description:
>  
>  properties:
>    compatible:
> -    const: sony,imx412
> +    items:
> +      - enum:
> +          - sony,imx412
> +          - sony,imx577
>    reg:
>      description: I2C address
>      maxItems: 1
> -- 
> 2.36.1
>

[PATCH 2/2] media: i2c: imx412: Add imx577 compatible string

[Bryan O'Donoghue]

The Sony IMX577 uses the same silicon enabling reference code from Sony in
the available examples provided.

Add an imx577 compatible string and re-use the existing imx412 code.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/i2c/imx412.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
index a1394d6c1432..3b7011ab0a8f 100644
--- a/drivers/media/i2c/imx412.c
+++ b/drivers/media/i2c/imx412.c
@@ -1282,6 +1282,7 @@ static const struct dev_pm_ops imx412_pm_ops = {
 
 static const struct of_device_id imx412_of_match[] = {
 	{ .compatible = "sony,imx412" },
+	{ .compatible = "sony,imx577" },
 	{ }
 };
 
-- 
2.36.1

Re: [PATCH 2/2] media: i2c: imx412: Add imx577 compatible string

[Kieran Bingham]

Hi Bryan,

Quoting Bryan O'Donoghue (2022-06-07 14:40:57)
> The Sony IMX577 uses the same silicon enabling reference code from Sony in
> the available examples provided.
> 
> Add an imx577 compatible string and re-use the existing imx412 code.
> 

As discussed in the cover letter, I think this is a reasonable way to
accomodate the imx577 (and certainly better than letting any real
hardware be defined as if it's an imx412).

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/i2c/imx412.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
> index a1394d6c1432..3b7011ab0a8f 100644
> --- a/drivers/media/i2c/imx412.c
> +++ b/drivers/media/i2c/imx412.c
> @@ -1282,6 +1282,7 @@ static const struct dev_pm_ops imx412_pm_ops = {
>  
>  static const struct of_device_id imx412_of_match[] = {
>         { .compatible = "sony,imx412" },
> +       { .compatible = "sony,imx577" },
>         { }
>  };
>  
> -- 
> 2.36.1
>

Re: [PATCH 2/2] media: i2c: imx412: Add imx577 compatible string

[Alessandrelli, Daniele]

On Tue, 2022-06-07 at 14:40 +0100, Bryan O'Donoghue wrote:
> The Sony IMX577 uses the same silicon enabling reference code from Sony in
> the available examples provided.
> 
> Add an imx577 compatible string and re-use the existing imx412 code.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

I've read the discussion following the cover letter and I also tend to
agree that this change makes sense, at least for now.

If in the future somebody wants to add IMX577-specific features, they
can always create a separate driver for IMX577, if that's preferable.

Reviewed-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>

> ---
>  drivers/media/i2c/imx412.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
> index a1394d6c1432..3b7011ab0a8f 100644
> --- a/drivers/media/i2c/imx412.c
> +++ b/drivers/media/i2c/imx412.c
> @@ -1282,6 +1282,7 @@ static const struct dev_pm_ops imx412_pm_ops = {
>  
>  static const struct of_device_id imx412_of_match[] = {
>         { .compatible = "sony,imx412" },
> +       { .compatible = "sony,imx577" },
>         { }
>  };
>  

Re: [PATCH 2/2] media: i2c: imx412: Add imx577 compatible string

[Sakari Ailus]

Hi Bryan,

On Tue, Jun 07, 2022 at 02:40:57PM +0100, Bryan O'Donoghue wrote:
> The Sony IMX577 uses the same silicon enabling reference code from Sony in
> the available examples provided.
> 
> Add an imx577 compatible string and re-use the existing imx412 code.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/i2c/imx412.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
> index a1394d6c1432..3b7011ab0a8f 100644
> --- a/drivers/media/i2c/imx412.c
> +++ b/drivers/media/i2c/imx412.c
> @@ -1282,6 +1282,7 @@ static const struct dev_pm_ops imx412_pm_ops = {
>  
>  static const struct of_device_id imx412_of_match[] = {
>  	{ .compatible = "sony,imx412" },
> +	{ .compatible = "sony,imx577" },

Thanks for the patch.

Surely the sensors still have some differences.

Even if the same registers would work as-is (the imx577 might still benefit
from different MSRs?), the user should know which sensor it is. I.e. please
set the media entity name accordingly. See e.g. the CCS driver for an
example.

>  	{ }
>  };
>  

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/2] media: i2c: imx412: Add imx577 compatible string

[Bryan O'Donoghue]

On 14/06/2022 05:00, Sakari Ailus wrote:
> Thanks for the patch.
> 
> Surely the sensors still have some differences.

They do, they absolutely do, the imx577 has a whole bunch of extra modes.

We don't have any reference code or access to documentation for those modes.

My reference is the qualcomm camx code for the rb5 board, which includes 
a imx577 sensor. That stack uses the same init code as for the 412.

So for that baseline mode, the imx412 driver works perfectly.

> Even if the same registers would work as-is (the imx577 might still benefit
> from different MSRs?), the user should know which sensor it is. I.e. please
> set the media entity name accordingly. See e.g. the CCS driver for an
> example.

Agreed, I'll do that.

---
bod

Re: [PATCH 0/2] Add imx577 compatible to imx412

[Kieran Bingham]

Quoting Bryan O'Donoghue (2022-06-07 14:40:55)
> Right now the imx412 and imx577 are code and pin compatible however, they
> are distinct pieces of silicon.
> 
> Document imx577 as a compatible enum and add the compat string to imx412.c.
> This allows us to differentiate these chips in DTS and potentially to apply
> any future imx412 or imx577 specific changes appropriately.

While I think this is probably fine to get things working, there seems to
be quite a few extra features in the IMX577 [0] compared with the IMX412 [1]. 

[0] https://www.sony-semicon.co.jp/products/common/pdf/IMX412-AACK_Flyer03.pdf
[1] https://www.sony-semicon.co.jp/products/common/pdf/IMX577-AACK_Flyer.pdf

I certainly agree though that it's better to declare the camera
correctly in the DT - so it's better to add this support than 'fake' the
DT to say it's an IMX412 on the platform.


wdiff  /tmp/imx412 /tmp/imx577 

[-IMX412-AACK-] {+IMX577-AACK+}
Diagonal 7.857 mm (Type 1/2.3) 12.3 Mega-Pixel CMOS Image Sensor with Square
Pixel for Color Cameras
Description

[-IMX412-AACK-]{+The IMX577-AACK+} is a diagonal 7.857 mm (Type 1/2.3)
12.3 Mega-pixel CMOS active pixel type stacked image sensor with a
square pixel array.

{+It adopts Sony’s Stacked CMOS Image Sensor technology to achieve high
speed image capturing by column parallel A/D converter circuits and high
sensitivity and low noise image (comparing with conventional CMOS image
sensor) through the backside illuminated imaging pixel structure.+}

R, G, and B pigment primary color mosaic filter is employed. It equips
an electronic shutter with variable integration time. It operates with
three power supply voltages: analog [-2.75-] {+2.8+} V, digital 1.05 V
and 1.8 V for input/output interface and achieves low power consumption.

[-(Applications: Surveillance cameras)-]

{+In addition, this product is designed for use in consumer use
camcorder. When using this for another application, Sony Semiconductor
Solutions Corporation does not guarantee the quality and reliability of
product. Therefore, don't use this for applications other than consumer
use camcorder.  In addition, individual specification change cannot be
supported because this is a standard product.  Consult your Sony
Semiconductor Solutions Corporation sales representative if you have any
questions.+}


Features
◆ Back-illuminated and stacked CMOS image sensor
◆ Digital Overlap High Dynamic Range (DOL-HDR) mode with raw data [-output-] {+output.+}
◆ High signal to noise ratio [-(SNR)-] {+(SNR).+}
◆ Full resolution @60 frame/s (Normal), 4K2K @60 frame/s (Normal), 1080p @240 frame/s
Full resolution @40 frame/s (12 bit Normal), Full resolution @30 frame/s (DOL-HDR, 2 frame)
◆ Output video format of [-RAW12/10-] {+RAW12/10/8, COMP8.+}
◆ [-Low-] Power [-Streaming-] {+Save+} Mode with MIPI ULPS operation
◆ Pixel binning readout and V sub-sampling [-function-] {+function.+}
◆ Independent flipping and [-mirroring-] {+mirroring.+}
◆ Input clock frequency [-6, 12, 18, 24 or-] {+6 to+} 27 MHz
◆ CSI-2 serial data output (MIPI 2lane/4lane, Max. 2.1 Gbps/lane, D-PHY spec. ver. 1.2 compliant)
◆ 2-wire serial [-communication-] {+communication.+}
◆ Two PLLs for independent clock generation for pixel control and data output [-interface-] {+interface.+}
◆ Defect Pixel Correction (DPC)
◆ {+Ambient Light Sensor (ALS)
◆+} Fast mode [-transition-] {+transition.+} (on the fly)
◆ Dual sensor synchronization operation (Multi camera compatible)
◆ 7 k bit of OTP ROM for [-users-] {+users.
◆ Built-in temperature sensor+}
◆ 10-bit/12-bit A/D conversion on chip
◆ Horizontal Low Power [-analog-] {+Analog+} Cropping
{+◆ Window Scanning mode+}
◆ 92-pin high-precision ceramic package


So picking out the diffs from that:

IMX412
 - Designed for Surveillance cameras
 - Defined input clocks of 6, 12, 18, 24 or 27MHz

IMX577
 + Designed for Camcorder use cases
 + May have an extra RAW8, COMP8 output format.
 + Less definition on input clock '6 to 27MHz' (may be the same)
 + Has an Ambiant light sensor
 + Has Built in temperature sensor
 + Window scanning mode

The extra RAW modes, ALS and temperature sensor may be key
differentiators in the future to be aware of. Perhaps they can be
handled by the compatible string though.

If only we had better/more open drivers/documentation for these
components I bet they could be factored out for better support.

--
Kieran



> 
> Bryan O'Donoghue (2):
>   media: dt-bindings: imx412: Add imx577 compatible string
>   media: i2c: imx412: Add imx577 compatible string
> 
>  Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml | 5 ++++-
>  drivers/media/i2c/imx412.c                                   | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> -- 
> 2.36.1
>

Re: [PATCH 0/2] Add imx577 compatible to imx412

[Bryan O'Donoghue]

On 07/06/2022 22:56, Kieran Bingham wrote:
> Quoting Bryan O'Donoghue (2022-06-07 14:40:55)
>> Right now the imx412 and imx577 are code and pin compatible however, they
>> are distinct pieces of silicon.
>>
>> Document imx577 as a compatible enum and add the compat string to imx412.c.
>> This allows us to differentiate these chips in DTS and potentially to apply
>> any future imx412 or imx577 specific changes appropriately.
> While I think this is probably fine to get things working, there seems to
> be quite a few extra features in the IMX577 [0] compared with the IMX412 [1].
> 
> [0]https://www.sony-semicon.co.jp/products/common/pdf/IMX412-AACK_Flyer03.pdf
> [1]https://www.sony-semicon.co.jp/products/common/pdf/IMX577-AACK_Flyer.pdf
> 
> I certainly agree though that it's better to declare the camera
> correctly in the DT - so it's better to add this support than 'fake' the
> DT to say it's an IMX412 on the platform.

Agreed the specs differ but the only silicon enabling code I have access 
to - from the qcom camx release is a 1:1 for 412 and 577.

The init process works for me on rb5 to obtain 10 bit bayer @ 30 fps.

Agreed the 577 does a whole bunch of extra stuff I just have no 
reference either in PDF or code on how to do any of it.

Its "good enough" for me in 412 mode.

---
bod

Re: [PATCH 0/2] Add imx577 compatible to imx412

[Sakari Ailus]

Hi Kieran,

On Tue, Jun 07, 2022 at 10:56:32PM +0100, Kieran Bingham wrote:
> So picking out the diffs from that:
> 
> IMX412
>  - Designed for Surveillance cameras
>  - Defined input clocks of 6, 12, 18, 24 or 27MHz

Probably the spec just these while other frequencies in the range would
work equally well --- with different register configuration.

The sensor might work with the CCS driver, too, with MSR configuration
taken from this driver.

-- 
Sakari Ailus