[PATCH v4 RESEND 0/3] Add imx577 and imx477 compatible to imx412

[PATCH v4 RESEND 0/3] Add imx577 and imx477 compatible to imx412

[Bryan O'Donoghue]

Resend:
- Cc Rob and Krzysztof

v4:
- Squashes dt-bindings addition - Sakari
- Squashes compat string addition to driver - Sakari
- Drops checking fwnode(&client->dev), checks for !name - Sakari
- Retains imx477 compat
  I think we have established that imx477 and imx577 do have additional
  settings and modes over the imx412 which, we can and hopefully will add
  in as time goes by. What we have upstream will work for all three parts.

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 (3):
  media: dt-bindings: imx412: Extend compatible strings
  media: i2c: imx412: Assign v4l2 device subname based on compat string
  media: i2c: imx412: Add new compatible strings

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

-- 
2.34.1

[PATCH v4 RESEND 1/3] media: dt-bindings: imx412: Extend compatible strings

[Bryan O'Donoghue]

Add compatible bindings for imx477 and imx577 both of which use the
same silicon enabling reference code from Sony in the available examples
provided.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 .../devicetree/bindings/media/i2c/sony,imx412.yaml          | 6 +++++-
 1 file changed, 5 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..ebb649c5e4c5 100644
--- a/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
@@ -19,7 +19,11 @@ description:
 
 properties:
   compatible:
-    const: sony,imx412
+    items:
+      - enum:
+          - sony,imx412
+          - sony,imx477
+          - sony,imx577
   reg:
     description: I2C address
     maxItems: 1
-- 
2.34.1

[PATCH v4 RESEND 2/3] media: i2c: imx412: Assign v4l2 device subname based on compat string

[Bryan O'Donoghue]

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 | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
index a1394d6c1432..9f854a1a4c2f 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,9 @@ static int imx412_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	imx412->dev = &client->dev;
+	name = device_get_match_data(&client->dev);
+	if (!name)
+		return -ENODEV;
 
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&imx412->sd, client, &imx412_subdev_ops);
@@ -1218,6 +1222,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 +1287,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

[PATCH v4 RESEND 3/3] media: i2c: imx412: Add new compatible strings

[Bryan O'Donoghue]

The Sony imx477 and imx577 use the same silicon enabling reference code
from Sony in the available examples provided as the imx412.

Add in compatible strings to differentiate the parts.

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

diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
index 9f854a1a4c2f..93f362e3b132 100644
--- a/drivers/media/i2c/imx412.c
+++ b/drivers/media/i2c/imx412.c
@@ -1288,6 +1288,8 @@ 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

Re: [PATCH v4 RESEND 3/3] media: i2c: imx412: Add new compatible strings

[Dave Stevenson]

Hi Brian

On Thu, 22 Sept 2022 at 11:43, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> The Sony imx477 and imx577 use the same silicon enabling reference code
> from Sony in the available examples provided as the imx412.
>
> Add in compatible strings to differentiate the parts.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/i2c/imx412.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
> index 9f854a1a4c2f..93f362e3b132 100644
> --- a/drivers/media/i2c/imx412.c
> +++ b/drivers/media/i2c/imx412.c
> @@ -1288,6 +1288,8 @@ 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" },

IMX477 isn't compatible with this driver at present - it has 0x0477 in
REG_ID (0x0016) instead of the 0x0577 that this driver looks for. The
driver therefore won't probe. (I don't think I've missed a patch
removing the ID check).

I know you state in your cover letter:
  I think we have established that imx477 and imx577 do have additional
  settings and modes over the imx412 which, we can and hopefully will add
  in as time goes by. What we have upstream will work for all three parts.

It may *eventually* work for all three parts, but isn't the time to
add the compatible string at the point where it is actually compatible
with the driver?

If you're adding in broken compatible strings then, as previously
mentioned, imx378 is also in the same family IIRC it streams with the
base imx477 registers but with some IQ issues. It reports 0x0378 in
REG_ID.

  Dave

> +       { .compatible = "sony,imx577", .data = "imx577" },
>         { }
>  };
>
> --
> 2.34.1
>

Re: [PATCH v4 RESEND 3/3] media: i2c: imx412: Add new compatible strings

[Bryan O'Donoghue]

On 22/09/2022 12:16, Dave Stevenson wrote:
> It may*eventually*  work for all three parts, but isn't the time to
> add the compatible string at the point where it is actually compatible
> with the driver?

Yes. I forgot about the 0x477 chip id on your part.

I'm happy enough to drop 477 from the compat string or indeed to allow 
0x0477 as a valid chip identifier in imx412.

Sakari, what would you like to do ?

---
bod

Re: [PATCH v4 RESEND 1/3] media: dt-bindings: imx412: Extend compatible strings

[Krzysztof Kozlowski]

On 22/09/2022 12:42, Bryan O'Donoghue wrote:
> Add compatible bindings for imx477 and imx577 both of which use the
> same silicon enabling reference code from Sony in the available examples
> provided.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  .../devicetree/bindings/media/i2c/sony,imx412.yaml          | 6 +++++-
>  1 file changed, 5 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..ebb649c5e4c5 100644
> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
> @@ -19,7 +19,11 @@ description:
>  
>  properties:
>    compatible:
> -    const: sony,imx412
> +    items:

No need for items. Just "enum".

Best regards,
Krzysztof

Re: [PATCH v4 RESEND 3/3] media: i2c: imx412: Add new compatible strings

[Sakari Ailus]

Hi Bryan, Dave,

On Thu, Sep 22, 2022 at 12:19:22PM +0100, Bryan O'Donoghue wrote:
> On 22/09/2022 12:16, Dave Stevenson wrote:
> > It may*eventually*  work for all three parts, but isn't the time to
> > add the compatible string at the point where it is actually compatible
> > with the driver?
> 
> Yes. I forgot about the 0x477 chip id on your part.
> 
> I'm happy enough to drop 477 from the compat string or indeed to allow
> 0x0477 as a valid chip identifier in imx412.
> 
> Sakari, what would you like to do ?

If the driver already works with all three apart from the chip ID check,
I'd just extend the check.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 RESEND 3/3] media: i2c: imx412: Add new compatible strings

[Bryan O'Donoghue]

On 22/09/2022 12:19, Bryan O'Donoghue wrote:
> On 22/09/2022 12:16, Dave Stevenson wrote:
>> It may*eventually*  work for all three parts, but isn't the time to
>> add the compatible string at the point where it is actually compatible
>> with the driver?
> 
> Yes. I forgot about the 0x477 chip id on your part.
> 
> I'm happy enough to drop 477 from the compat string or indeed to allow 
> 0x0477 as a valid chip identifier in imx412.
> 
> Sakari, what would you like to do ?
> 
> ---
> bod

Right.

So I got myself the official rpi imx477 sensor and ran the imx412/imx577 
driver on the rpi 5.19.y tree

It looks like the rpi driver configures imx477 for two MIPI data-lanes, 
whereas upstream imx412 wants four MIPI data-lanes.

So already that means the imx412 config as-is won't work.

But, we do know these sensors are very very close.

I think the right medium term thing to do is try take in the majority of 
the imx477 code - including the various test modes, and resolutions and 
support for different MIPI data-lane configurations.

Its not clear to me that the imx412/imx577 and imx378/imx477 can 
genuinely live in the same codebase though.

Anyway I think adding imx477 to the imx412 driver should be considered 
out of scope pending answering most of those questions and getting the 
code to work.

Anyway that merging of rpi imx477 and upstream imx412/imx577 code feels 
like an entirely different series.

So I'll resend this series minus the imx477 bits.

---
bod

Re: [PATCH v4 RESEND 3/3] media: i2c: imx412: Add new compatible strings

[Sakari Ailus]

Hi Bryan,

On Wed, Oct 12, 2022 at 01:56:19AM +0100, Bryan O'Donoghue wrote:
> On 22/09/2022 12:19, Bryan O'Donoghue wrote:
> > On 22/09/2022 12:16, Dave Stevenson wrote:
> > > It may*eventually*  work for all three parts, but isn't the time to
> > > add the compatible string at the point where it is actually compatible
> > > with the driver?
> > 
> > Yes. I forgot about the 0x477 chip id on your part.
> > 
> > I'm happy enough to drop 477 from the compat string or indeed to allow
> > 0x0477 as a valid chip identifier in imx412.
> > 
> > Sakari, what would you like to do ?
> > 
> > ---
> > bod
> 
> Right.
> 
> So I got myself the official rpi imx477 sensor and ran the imx412/imx577
> driver on the rpi 5.19.y tree
> 
> It looks like the rpi driver configures imx477 for two MIPI data-lanes,
> whereas upstream imx412 wants four MIPI data-lanes.
> 
> So already that means the imx412 config as-is won't work.
> 
> But, we do know these sensors are very very close.
> 
> I think the right medium term thing to do is try take in the majority of the
> imx477 code - including the various test modes, and resolutions and support
> for different MIPI data-lane configurations.
> 
> Its not clear to me that the imx412/imx577 and imx378/imx477 can genuinely
> live in the same codebase though.
> 
> Anyway I think adding imx477 to the imx412 driver should be considered out
> of scope pending answering most of those questions and getting the code to
> work.
> 
> Anyway that merging of rpi imx477 and upstream imx412/imx577 code feels like
> an entirely different series.
> 
> So I'll resend this series minus the imx477 bits.

I wonder if you saw my earlier reply... but this is certainly fine, too.

-- 
Sakari Ailus

Re: [PATCH v4 RESEND 3/3] media: i2c: imx412: Add new compatible strings

[Dave Stevenson]

Hi Bryan

On Wed, 12 Oct 2022 at 10:06, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Bryan,
>
> On Wed, Oct 12, 2022 at 01:56:19AM +0100, Bryan O'Donoghue wrote:
> > On 22/09/2022 12:19, Bryan O'Donoghue wrote:
> > > On 22/09/2022 12:16, Dave Stevenson wrote:
> > > > It may*eventually*  work for all three parts, but isn't the time to
> > > > add the compatible string at the point where it is actually compatible
> > > > with the driver?
> > >
> > > Yes. I forgot about the 0x477 chip id on your part.
> > >
> > > I'm happy enough to drop 477 from the compat string or indeed to allow
> > > 0x0477 as a valid chip identifier in imx412.
> > >
> > > Sakari, what would you like to do ?
> > >
> > > ---
> > > bod
> >
> > Right.
> >
> > So I got myself the official rpi imx477 sensor and ran the imx412/imx577
> > driver on the rpi 5.19.y tree
> >
> > It looks like the rpi driver configures imx477 for two MIPI data-lanes,
> > whereas upstream imx412 wants four MIPI data-lanes.
> >
> > So already that means the imx412 config as-is won't work.

It won't work on the Pi IMX477 camera module as-is due to only
exposing 2 data lanes, but there are 4 lane IMX477 modules around
(Arducam sell a couple).
Adding support for 2 lanes isn't a huge deal as long as you understand
the clock tree in the sensor.

> > But, we do know these sensors are very very close.

I have a conversation open with Sony about how common these sensors
are. The initial response from their Japanese team was that "common
form between sensors of different families is not a requirement
considered".

> > I think the right medium term thing to do is try take in the majority of the
> > imx477 code - including the various test modes, and resolutions and support
> > for different MIPI data-lane configurations.
> >
> > Its not clear to me that the imx412/imx577 and imx378/imx477 can genuinely
> > live in the same codebase though.

The way Sony tends to work is to give you a spreadsheet of the exact
register set required for the mode you ask them for, and they will
generally have validated the settings for image quality issues. They
tend not to go for generic configuration.

Presumably Intel as the original authors of imx412.c were given such a
spreadsheet for their particular use case.
We have such spreadsheets from Sony for our modes. As well as only
using 2 data lanes, they also drive the PLLs in a different mode (dual
vs single), so how many of those changes need to be preserved to
ensure we don't introduce image quality issues? How many of the
differences are valid for all the other supposedly common sensors? How
do we test it? How big is the can of worms?

The engineer in me loves to note where we have similarities between
sensors, but I'm also cautious in drawing too many conclusions as it
is far too easy to mess up image quality.

> > Anyway I think adding imx477 to the imx412 driver should be considered out
> > of scope pending answering most of those questions and getting the code to
> > work.
> >
> > Anyway that merging of rpi imx477 and upstream imx412/imx577 code feels like
> > an entirely different series.
> >
> > So I'll resend this series minus the imx477 bits.

Ideas On Board do have a contract to upstream our imx477 driver.
I will have a look at whether we can easily integrate it into the
existing imx412 driver, but it remains to be seen how much common
stuff remains between the sensors.

> I wonder if you saw my earlier reply... but this is certainly fine, too.

I guess that if we get there and find imx378/477 aren't common enough
with imx412, then we split off into a second compatible binding. I can
live with that.

  Dave