All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/5] Add imx577 and imx477 compatible to imx412
@ 2022-09-15  0:35 Bryan O'Donoghue
  2022-09-15  0:35 ` [PATCH v3 1/5] media: dt-bindings: imx412: Add imx477 compatible string Bryan O'Donoghue
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Bryan O'Donoghue @ 2022-09-15  0:35 UTC (permalink / raw)
  To: sakari.ailus, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media
  Cc: bryan.odonoghue

v3:
To really get to the bottom of what was going on with the imx412, imx477
and imx577 I got a Nvida Nano with both sensors from Leopard imaging.

Interestingly the shipped driver from Leopard is called "imx477" and is
keyed to work with imx412, imx477 and imx577 with predictably the same
register init sequence.

For me aside from the physical size difference in the modules the only
discerable difference is the imx577 has a "green tinge" which is exactly
the same experience I have with the imx577 on the Qualcomm RB5 - with both
the downstream driver stack and the usptream driver.

With the modules shipped from Leopard both the imx412 and imx577 report the
same chip id of 0x0577.

I dumped the first 32 registers on both modules using the Nvidia/Leopard
stack and as expected - all of the register values match.

So I've confirmed as far as my investigation has gone, there's no
clear register we can interrogate to differentiate the parts.

- Drops renaming of the driver to imx577.c
  I continue to think this is the right thing to do but, I'm happy to
  leave that on the back burner for another time.
- Add in compatible strings for imx477 and imx577 into the dt-bindings
- Extend the imx412 per Sakari's comment re: media device naming
  https://patchwork.kernel.org/project/linux-media/patch/20220607134057.2427663-3-bryan.odonoghue@linaro.org/#24894500
- Add imx477 and imx577 with supporting code to drive media device name

Depending then if the compat string is imx412, imx477 or imx577 the media
name of the device will be imx412, imx477 or imx577 e.g. I use the
following media-ctl command depending on the specified compatible.

media-ctl -v -d /dev/media0 -V '"imx577 '22-001a'"...
media-ctl -v -d /dev/media0 -V '"imx412 '22-001a'"...

v2:
Sakari wasn't especially satisfied with the answer imx412 and imx577 have
the same init sequence but, suggested setting the string for imx577 as is
done in the ccs driver.

https://lore.kernel.org/all/20220607134057.2427663-3-bryan.odonoghue@xxxxxxxxxx/t/

I went to look at that and asked myself "how would I tell the difference
between the two silicon parts". The obvious answer is a chip identifier.

Luckily this class of IMX sensor has a chip identifier at offset 0x0016.

That looks like this for imx258, imx319 and imx355

drivers/media/i2c/imx258.c:#define IMX258_REG_CHIP_ID    0x0016
drivers/media/i2c/imx258.c:#define IMX258_CHIP_ID        0x0258

drivers/media/i2c/imx319.c:#define IMX319_REG_CHIP_ID    0x0016
drivers/media/i2c/imx319.c:#define IMX319_CHIP_ID        0x0319

drivers/media/i2c/imx355.c:#define IMX355_REG_CHIP_ID    0x0016
drivers/media/i2c/imx355.c:#define IMX355_CHIP_ID        0x0355

but then looks like this for imx412.

drivers/media/i2c/imx412.c:#define IMX412_REG_ID         0x0016
drivers/media/i2c/imx412.c:#define IMX412_ID             0x577

This made no sense at all to me, why is the imx412 driver not named imx577 ?

I went and dug into the Qualcomm camx/chi-cdk sources to find that a file
called cmk_imx577_sensor.xml has a property called sensorId which is
constrained to 0x0577.

In the Qualcomm stack this pairing of filename and identifier is
maintained for imx258, imx376, imx476, imx576, imx519, imx362, imx481,
imx318 imx334 and imx386.

Every single example I can find of a Sony IMX sensor which returns a chip
identifier at offset 0x0016 matches the driver name to the returned sensor
id both here upstream in Linux and in Qualcomm's camx stack.

The conclusion I draw from this is that imx412.c is inappropriately named.

I think the right thing to do is to rename imx412 to imx577. It is
confusing and I think wrong to pair imx412.c with a chip which identifies
as 0x0577.

v1:
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 (5):
  media: dt-bindings: imx412: Add imx477 compatible string
  media: dt-bindings: imx412: Add imx577 compatible string
  media: i2c: imx412: Assign v4l2 device subname based on compat string
  media: i2c: imx412: Add imx477 compatible string
  media: i2c: imx412: Add imx577 compatible string

 .../devicetree/bindings/media/i2c/sony,imx412.yaml    |  6 +++++-
 drivers/media/i2c/imx412.c                            | 11 ++++++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 1/5] media: dt-bindings: imx412: Add imx477 compatible string
  2022-09-15  0:35 [PATCH v3 0/5] Add imx577 and imx477 compatible to imx412 Bryan O'Donoghue
@ 2022-09-15  0:35 ` Bryan O'Donoghue
  2022-09-15  0:35 ` [PATCH v3 2/5] media: dt-bindings: imx412: Add imx577 " Bryan O'Donoghue
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Bryan O'Donoghue @ 2022-09-15  0:35 UTC (permalink / raw)
  To: sakari.ailus, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media
  Cc: bryan.odonoghue

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

Add an imx477 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..a0da469995db 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,imx477
   reg:
     description: I2C address
     maxItems: 1
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 2/5] media: dt-bindings: imx412: Add imx577 compatible string
  2022-09-15  0:35 [PATCH v3 0/5] Add imx577 and imx477 compatible to imx412 Bryan O'Donoghue
  2022-09-15  0:35 ` [PATCH v3 1/5] media: dt-bindings: imx412: Add imx477 compatible string Bryan O'Donoghue
@ 2022-09-15  0:35 ` Bryan O'Donoghue
  2022-09-15  0:35 ` [PATCH v3 3/5] media: i2c: imx412: Assign v4l2 device subname based on compat string Bryan O'Donoghue
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Bryan O'Donoghue @ 2022-09-15  0:35 UTC (permalink / raw)
  To: sakari.ailus, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media
  Cc: bryan.odonoghue

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
index a0da469995db..ebb649c5e4c5 100644
--- a/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
@@ -23,6 +23,7 @@ properties:
       - enum:
           - sony,imx412
           - sony,imx477
+          - sony,imx577
   reg:
     description: I2C address
     maxItems: 1
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 3/5] media: i2c: imx412: Assign v4l2 device subname based on compat string
  2022-09-15  0:35 [PATCH v3 0/5] Add imx577 and imx477 compatible to imx412 Bryan O'Donoghue
  2022-09-15  0:35 ` [PATCH v3 1/5] media: dt-bindings: imx412: Add imx477 compatible string Bryan O'Donoghue
  2022-09-15  0:35 ` [PATCH v3 2/5] media: dt-bindings: imx412: Add imx577 " Bryan O'Donoghue
@ 2022-09-15  0:35 ` Bryan O'Donoghue
  2022-09-15 20:54   ` Sakari Ailus
  2022-09-15  0:35 ` [PATCH v3 4/5] media: i2c: imx412: Add imx477 compatible string Bryan O'Donoghue
  2022-09-15  0:35 ` [PATCH v3 5/5] media: i2c: imx412: Add imx577 " Bryan O'Donoghue
  4 siblings, 1 reply; 13+ messages in thread
From: Bryan O'Donoghue @ 2022-09-15  0:35 UTC (permalink / raw)
  To: sakari.ailus, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media
  Cc: bryan.odonoghue

imx412, imx477 and imx577 all return the same chip-id when interrogated via
i2c. I've confirmed this myself by

- Looking at the code in Qcom and Nvidia stacks
- Running the upstream imx412 driver on imx577 with a Qcom sm8250 RB5
- Running the downstream Qcom stack on the same hardware. This uses a
  commercial licensed stack with a driver/userspace pair that make no
  differentiation between imx412, imx477 and imx577.
- Running the imx412 and imx577 on a Nvidia Nano with cameras from Leopard
  Imaging. Again this is a commercial non-upstream user-space/kernel-space
  pairing and again the same imx driver, works for both parts.

Sakari suggested we should add a new compat but that the compat string
should also set the media entity name also

https://patchwork.kernel.org/project/linux-media/patch/20220607134057.2427663-3-bryan.odonoghue@linaro.org/#24894500

Set up the .data parameter of of_device_id to pass a string which
we use to set the media entity name. Once done we can add in imx477 and
imx577 as compatible chips with the media names reflecting the directed
compat string.

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

diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
index a1394d6c1432..bc7fdb24235f 100644
--- a/drivers/media/i2c/imx412.c
+++ b/drivers/media/i2c/imx412.c
@@ -1172,6 +1172,7 @@ static int imx412_init_controls(struct imx412 *imx412)
 static int imx412_probe(struct i2c_client *client)
 {
 	struct imx412 *imx412;
+	const char *name;
 	int ret;
 
 	imx412 = devm_kzalloc(&client->dev, sizeof(*imx412), GFP_KERNEL);
@@ -1179,6 +1180,10 @@ static int imx412_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	imx412->dev = &client->dev;
+	if (dev_fwnode(&client->dev))
+		name = device_get_match_data(&client->dev);
+	else
+		return -ENODEV;
 
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&imx412->sd, client, &imx412_subdev_ops);
@@ -1218,6 +1223,8 @@ static int imx412_probe(struct i2c_client *client)
 	imx412->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	imx412->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 
+	v4l2_i2c_subdev_set_name(&imx412->sd, client, name, NULL);
+
 	/* Initialize source pad */
 	imx412->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_pads_init(&imx412->sd.entity, 1, &imx412->pad);
@@ -1281,7 +1288,7 @@ static const struct dev_pm_ops imx412_pm_ops = {
 };
 
 static const struct of_device_id imx412_of_match[] = {
-	{ .compatible = "sony,imx412" },
+	{ .compatible = "sony,imx412", .data = "imx412" },
 	{ }
 };
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 4/5] media: i2c: imx412: Add imx477 compatible string
  2022-09-15  0:35 [PATCH v3 0/5] Add imx577 and imx477 compatible to imx412 Bryan O'Donoghue
                   ` (2 preceding siblings ...)
  2022-09-15  0:35 ` [PATCH v3 3/5] media: i2c: imx412: Assign v4l2 device subname based on compat string Bryan O'Donoghue
@ 2022-09-15  0:35 ` Bryan O'Donoghue
  2022-09-15 22:03   ` Dave Stevenson
  2022-09-15  0:35 ` [PATCH v3 5/5] media: i2c: imx412: Add imx577 " Bryan O'Donoghue
  4 siblings, 1 reply; 13+ messages in thread
From: Bryan O'Donoghue @ 2022-09-15  0:35 UTC (permalink / raw)
  To: sakari.ailus, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media
  Cc: bryan.odonoghue

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

Add an imx477 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 bc7fdb24235f..1013a5afc85f 100644
--- a/drivers/media/i2c/imx412.c
+++ b/drivers/media/i2c/imx412.c
@@ -1289,6 +1289,7 @@ static const struct dev_pm_ops imx412_pm_ops = {
 
 static const struct of_device_id imx412_of_match[] = {
 	{ .compatible = "sony,imx412", .data = "imx412" },
+	{ .compatible = "sony,imx477", .data = "imx477" },
 	{ }
 };
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 5/5] media: i2c: imx412: Add imx577 compatible string
  2022-09-15  0:35 [PATCH v3 0/5] Add imx577 and imx477 compatible to imx412 Bryan O'Donoghue
                   ` (3 preceding siblings ...)
  2022-09-15  0:35 ` [PATCH v3 4/5] media: i2c: imx412: Add imx477 compatible string Bryan O'Donoghue
@ 2022-09-15  0:35 ` Bryan O'Donoghue
  2022-09-15 20:55   ` Sakari Ailus
  4 siblings, 1 reply; 13+ messages in thread
From: Bryan O'Donoghue @ 2022-09-15  0:35 UTC (permalink / raw)
  To: sakari.ailus, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media
  Cc: bryan.odonoghue

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 1013a5afc85f..776cc058edf2 100644
--- a/drivers/media/i2c/imx412.c
+++ b/drivers/media/i2c/imx412.c
@@ -1290,6 +1290,7 @@ static const struct dev_pm_ops imx412_pm_ops = {
 static const struct of_device_id imx412_of_match[] = {
 	{ .compatible = "sony,imx412", .data = "imx412" },
 	{ .compatible = "sony,imx477", .data = "imx477" },
+	{ .compatible = "sony,imx577", .data = "imx577" },
 	{ }
 };
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 3/5] media: i2c: imx412: Assign v4l2 device subname based on compat string
  2022-09-15  0:35 ` [PATCH v3 3/5] media: i2c: imx412: Assign v4l2 device subname based on compat string Bryan O'Donoghue
@ 2022-09-15 20:54   ` Sakari Ailus
  0 siblings, 0 replies; 13+ messages in thread
From: Sakari Ailus @ 2022-09-15 20:54 UTC (permalink / raw)
  To: Bryan O'Donoghue
  Cc: jacopo, paul.j.murphy, daniele.alessandrelli, mchehab, linux-media

Hi Bryan,

Thanks for the update.

On Thu, Sep 15, 2022 at 01:35:20AM +0100, Bryan O'Donoghue wrote:
> imx412, imx477 and imx577 all return the same chip-id when interrogated via
> i2c. I've confirmed this myself by
> 
> - Looking at the code in Qcom and Nvidia stacks
> - Running the upstream imx412 driver on imx577 with a Qcom sm8250 RB5
> - Running the downstream Qcom stack on the same hardware. This uses a
>   commercial licensed stack with a driver/userspace pair that make no
>   differentiation between imx412, imx477 and imx577.
> - Running the imx412 and imx577 on a Nvidia Nano with cameras from Leopard
>   Imaging. Again this is a commercial non-upstream user-space/kernel-space
>   pairing and again the same imx driver, works for both parts.
> 
> Sakari suggested we should add a new compat but that the compat string
> should also set the media entity name also
> 
> https://patchwork.kernel.org/project/linux-media/patch/20220607134057.2427663-3-bryan.odonoghue@linaro.org/#24894500
> 
> Set up the .data parameter of of_device_id to pass a string which
> we use to set the media entity name. Once done we can add in imx477 and
> imx577 as compatible chips with the media names reflecting the directed
> compat string.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/i2c/imx412.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
> index a1394d6c1432..bc7fdb24235f 100644
> --- a/drivers/media/i2c/imx412.c
> +++ b/drivers/media/i2c/imx412.c
> @@ -1172,6 +1172,7 @@ static int imx412_init_controls(struct imx412 *imx412)
>  static int imx412_probe(struct i2c_client *client)
>  {
>  	struct imx412 *imx412;
> +	const char *name;
>  	int ret;
>  
>  	imx412 = devm_kzalloc(&client->dev, sizeof(*imx412), GFP_KERNEL);
> @@ -1179,6 +1180,10 @@ static int imx412_probe(struct i2c_client *client)
>  		return -ENOMEM;
>  
>  	imx412->dev = &client->dev;
> +	if (dev_fwnode(&client->dev))
> +		name = device_get_match_data(&client->dev);

No need to make this conditional.

But you could return an error if name is NULL. It would most probably mean
a driver bug though.

> +	else
> +		return -ENODEV;
>  
>  	/* Initialize subdev */
>  	v4l2_i2c_subdev_init(&imx412->sd, client, &imx412_subdev_ops);
> @@ -1218,6 +1223,8 @@ static int imx412_probe(struct i2c_client *client)
>  	imx412->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>  	imx412->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>  
> +	v4l2_i2c_subdev_set_name(&imx412->sd, client, name, NULL);
> +
>  	/* Initialize source pad */
>  	imx412->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ret = media_entity_pads_init(&imx412->sd.entity, 1, &imx412->pad);
> @@ -1281,7 +1288,7 @@ static const struct dev_pm_ops imx412_pm_ops = {
>  };
>  
>  static const struct of_device_id imx412_of_match[] = {
> -	{ .compatible = "sony,imx412" },
> +	{ .compatible = "sony,imx412", .data = "imx412" },
>  	{ }
>  };
>  

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 5/5] media: i2c: imx412: Add imx577 compatible string
  2022-09-15  0:35 ` [PATCH v3 5/5] media: i2c: imx412: Add imx577 " Bryan O'Donoghue
@ 2022-09-15 20:55   ` Sakari Ailus
  0 siblings, 0 replies; 13+ messages in thread
From: Sakari Ailus @ 2022-09-15 20:55 UTC (permalink / raw)
  To: Bryan O'Donoghue
  Cc: jacopo, paul.j.murphy, daniele.alessandrelli, mchehab, linux-media

On Thu, Sep 15, 2022 at 01:35:22AM +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 1013a5afc85f..776cc058edf2 100644
> --- a/drivers/media/i2c/imx412.c
> +++ b/drivers/media/i2c/imx412.c
> @@ -1290,6 +1290,7 @@ static const struct dev_pm_ops imx412_pm_ops = {
>  static const struct of_device_id imx412_of_match[] = {
>  	{ .compatible = "sony,imx412", .data = "imx412" },
>  	{ .compatible = "sony,imx477", .data = "imx477" },
> +	{ .compatible = "sony,imx577", .data = "imx577" },

Please squash patche 1 with 2 and patch 4 with 5.

Then I think these should be ready for merging.

>  	{ }
>  };
>  

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 4/5] media: i2c: imx412: Add imx477 compatible string
  2022-09-15  0:35 ` [PATCH v3 4/5] media: i2c: imx412: Add imx477 compatible string Bryan O'Donoghue
@ 2022-09-15 22:03   ` Dave Stevenson
  2022-09-16  0:32     ` Bryan O'Donoghue
  0 siblings, 1 reply; 13+ messages in thread
From: Dave Stevenson @ 2022-09-15 22:03 UTC (permalink / raw)
  To: Bryan O'Donoghue
  Cc: sakari.ailus, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media

Hi Bryan

On Thu, 15 Sept 2022 at 01:36, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> The Sony imx477 uses the same silicon enabling reference code from Sony in
> the available examples provided.

Have you had any conversations with Sony on whether the same register
settings are genuinely valid for all of these sensors?

IMX477 is the sensor in the Raspberry Pi HQ camera, but the register
set that we have from Sony has many undocumented (to us at least)
registers for image quality tuning that aren't in the imx412 driver -
we're up at 417 registers vs imx412's 231 [1].

Raspberry Pi has previously had discussions with Sony regarding IMX378
vs IMX477, which is also in the same family. Whilst nearly identical
in register setup, they gave us 3 additional registers that have to be
set for IMX378 to avoid image quality issues. I wouldn't be surprised
if something similar weren't true for IMX412 and IMX577.

I'm not saying it's impossible to remove a load of duplicated register
settings by combining these sensors into one driver, but this current
patch looks a little simplistic, and probably needs to be looking to
have a struct for sensor specific registers, not just changing the
compatible string and advertised name. Or perhaps we extend that later
once the compatible string has been added?
Merging them does bring issues with regard to testing as it is
unlikely to be practical to test any new features across all variants.

  Dave

[1] https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/media/i2c/imx477.c

> Add an imx477 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 bc7fdb24235f..1013a5afc85f 100644
> --- a/drivers/media/i2c/imx412.c
> +++ b/drivers/media/i2c/imx412.c
> @@ -1289,6 +1289,7 @@ static const struct dev_pm_ops imx412_pm_ops = {
>
>  static const struct of_device_id imx412_of_match[] = {
>         { .compatible = "sony,imx412", .data = "imx412" },
> +       { .compatible = "sony,imx477", .data = "imx477" },
>         { }
>  };
>
> --
> 2.34.1
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 4/5] media: i2c: imx412: Add imx477 compatible string
  2022-09-15 22:03   ` Dave Stevenson
@ 2022-09-16  0:32     ` Bryan O'Donoghue
  2022-09-16 11:03       ` Sakari Ailus
  0 siblings, 1 reply; 13+ messages in thread
From: Bryan O'Donoghue @ 2022-09-16  0:32 UTC (permalink / raw)
  To: Dave Stevenson
  Cc: sakari.ailus, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media

On 15/09/2022 23:03, Dave Stevenson wrote:
> Hi Bryan
> 
> On Thu, 15 Sept 2022 at 01:36, Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> wrote:
>>
>> The Sony imx477 uses the same silicon enabling reference code from Sony in
>> the available examples provided.
> 
> Have you had any conversations with Sony on whether the same register
> settings are genuinely valid for all of these sensors?

There's alot to unpack here but... this shows me the main thing

https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/media/i2c/imx477.c#L37

The vendor driver reference code I have expects 0x0577. I've discussed 
with Daniele previously - that imx412 and imx577 both return 0x0577 in 
the chip id field and this imx412.c driver won't work with any chip 
reporting anything else.

Your chip though is returning 0x0477 so it will need to have its own 
upstream driver anyway.

So the conclusion I drew from the Nvidia/Leopard stack is wrong they 
call the driver imx477 but pointedly and suspiciously comment out the 
check for chip id.

And its pretty clear why, its so the "imx477" driver they have with work 
with chips identifying as 0x0577 and 0x0477.

> IMX477 is the sensor in the Raspberry Pi HQ camera, but the register
> set that we have from Sony has many undocumented (to us at least)
> registers for image quality tuning that aren't in the imx412 driver -
> we're up at 417 registers vs imx412's 231 [1].
> 
> Raspberry Pi has previously had discussions with Sony regarding IMX378
> vs IMX477, which is also in the same family. Whilst nearly identical
> in register setup, they gave us 3 additional registers that have to be
> set for IMX378 to avoid image quality issues. I wouldn't be surprised
> if something similar weren't true for IMX412 and IMX577.
> 
> I'm not saying it's impossible to remove a load of duplicated register
> settings by combining these sensors into one driver, but this current
> patch looks a little simplistic, and probably needs to be looking to
> have a struct for sensor specific registers, not just changing the
> compatible string and advertised name. Or perhaps we extend that later
> once the compatible string has been added?
> Merging them does bring issues with regard to testing as it is
> unlikely to be practical to test any new features across all variants.

No so you're right the init sequence for imx477 is different or 
different enough that Sony spun new silicon for it so you can tweak it.

So, I'll drop the imx477 for V3, thanks for catching this and FWIW I 
will take your suggestion will reach out to my board vendor and see if I 
can convince them to share documentation with me or put me in contact 
with a Sony person who is authorised to do so.

We should get a definitive answer on imx412 (v) imx577 - is it different 
silicon or is it a packaging / optics filter level difference ?

---
bod

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 4/5] media: i2c: imx412: Add imx477 compatible string
  2022-09-16  0:32     ` Bryan O'Donoghue
@ 2022-09-16 11:03       ` Sakari Ailus
  2022-09-16 11:14         ` Bryan O'Donoghue
  0 siblings, 1 reply; 13+ messages in thread
From: Sakari Ailus @ 2022-09-16 11:03 UTC (permalink / raw)
  To: Bryan O'Donoghue
  Cc: Dave Stevenson, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media

Hi Bryan,

On Fri, Sep 16, 2022 at 01:32:06AM +0100, Bryan O'Donoghue wrote:
> On 15/09/2022 23:03, Dave Stevenson wrote:
> > Hi Bryan
> > 
> > On Thu, 15 Sept 2022 at 01:36, Bryan O'Donoghue
> > <bryan.odonoghue@linaro.org> wrote:
> > > 
> > > The Sony imx477 uses the same silicon enabling reference code from Sony in
> > > the available examples provided.
> > 
> > Have you had any conversations with Sony on whether the same register
> > settings are genuinely valid for all of these sensors?
> 
> There's alot to unpack here but... this shows me the main thing
> 
> https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/media/i2c/imx477.c#L37
> 
> The vendor driver reference code I have expects 0x0577. I've discussed with
> Daniele previously - that imx412 and imx577 both return 0x0577 in the chip
> id field and this imx412.c driver won't work with any chip reporting
> anything else.
> 
> Your chip though is returning 0x0477 so it will need to have its own
> upstream driver anyway.

That doesn't matter. What does however matter is what that sensor does and
how it's controlled. It is entirely possible that earlier versions of such
a sensor simply report a wrong ID.

> 
> So the conclusion I drew from the Nvidia/Leopard stack is wrong they call
> the driver imx477 but pointedly and suspiciously comment out the check for
> chip id.
> 
> And its pretty clear why, its so the "imx477" driver they have with work
> with chips identifying as 0x0577 and 0x0477.

I wouldn't be so worried about the model ID *if* the registers (and their
values) programmed to the sensor are otherwise the same.

And even if you have small differences in the registers you'll need to
write there, you can still differentiate between the sensors based on the
compatible string.

I don't have strong opinion on the grey areas though. Still if the register
set is exactly the same, then the driver should also be the same.

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 4/5] media: i2c: imx412: Add imx477 compatible string
  2022-09-16 11:03       ` Sakari Ailus
@ 2022-09-16 11:14         ` Bryan O'Donoghue
  2022-09-16 11:18           ` Bryan O'Donoghue
  0 siblings, 1 reply; 13+ messages in thread
From: Bryan O'Donoghue @ 2022-09-16 11:14 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Dave Stevenson, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media

On 16/09/2022 12:03, Sakari Ailus wrote:
> And even if you have small differences in the registers you'll need to
> write there, you can still differentiate between the sensors based on the
> compatible string.
> 
> I don't have strong opinion on the grey areas though. Still if the register
> set is exactly the same, then the driver should also be the same.

Right now we have

- An imx412 driver that works on imx577 unmodified on Qcom hardware
- A Nvidia driver modified by Leopard imaging that ignores the chip id
   and uses the same init sequence.
   This driver is called "imx477" and I can verify it works with
   imx412 and imx577.
   The code for this driver modifies the original out of tree driver they
   had and stops validating the CHIP_ID
   So I think we can take it as read it works with imx412, imx477 and
   imx577 - I've verified the first and last is the case.

We know the upstream driver works with the Intel platform and I've 
tested/used it on Qcom with minimal change, so I'm happy to stand over 
listing both imx412 and imx577.

Its pretty clear the init sequence works for imx412, imx477 and imx577 
so, it feels to me like the right thing to do is to add in the 
compatible strings and if/when we get better chip specific data say for 
higher resolutions, add that resolution init sequence in when the compat 
matches.

---
bod

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 4/5] media: i2c: imx412: Add imx477 compatible string
  2022-09-16 11:14         ` Bryan O'Donoghue
@ 2022-09-16 11:18           ` Bryan O'Donoghue
  0 siblings, 0 replies; 13+ messages in thread
From: Bryan O'Donoghue @ 2022-09-16 11:18 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Dave Stevenson, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media

On 16/09/2022 12:14, Bryan O'Donoghue wrote:
> - A Nvidia driver modified by Leopard imaging that ignores the chip id
>    and uses the same init sequence.
>    This driver is called "imx477" and I can verify it works with
>    imx412 and imx577.
>    The code for this driver modifies the original out of tree driver they
>    had and stops validating the CHIP_ID
>    So I think we can take it as read it works with imx412, imx477 and
>    imx577 - I've verified the first and last is the case.

I should have added

- An RPI driver that works with imx477 and imx378

https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/media/i2c/imx477.c#L38

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-09-16 11:18 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-15  0:35 [PATCH v3 0/5] Add imx577 and imx477 compatible to imx412 Bryan O'Donoghue
2022-09-15  0:35 ` [PATCH v3 1/5] media: dt-bindings: imx412: Add imx477 compatible string Bryan O'Donoghue
2022-09-15  0:35 ` [PATCH v3 2/5] media: dt-bindings: imx412: Add imx577 " Bryan O'Donoghue
2022-09-15  0:35 ` [PATCH v3 3/5] media: i2c: imx412: Assign v4l2 device subname based on compat string Bryan O'Donoghue
2022-09-15 20:54   ` Sakari Ailus
2022-09-15  0:35 ` [PATCH v3 4/5] media: i2c: imx412: Add imx477 compatible string Bryan O'Donoghue
2022-09-15 22:03   ` Dave Stevenson
2022-09-16  0:32     ` Bryan O'Donoghue
2022-09-16 11:03       ` Sakari Ailus
2022-09-16 11:14         ` Bryan O'Donoghue
2022-09-16 11:18           ` Bryan O'Donoghue
2022-09-15  0:35 ` [PATCH v3 5/5] media: i2c: imx412: Add imx577 " Bryan O'Donoghue
2022-09-15 20:55   ` Sakari Ailus

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.