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

["Alessandrelli, Daniele" <daniele.alessandrelli@intel.com>]

Hi Bryan,

On Mon, 2022-07-18 at 02:42 +0100, Bryan O'Donoghue wrote:
> 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@linaro.org/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 tried to reached out to the colleagues who wrote the driver but it
seems they are not in the company anymore.

However, I managed to get the imx412 register map documentation they
used while developing the driver and the value at offset 0x0016 is
reported to be 0x0577.

I agree this is strange, so, next week, I'll try to see if I can get my
hands on an imx412 sensor to verify the value reported by the HW.

In the meantime, would you be able to check what 16-bit value the
imx577 sensor reports at offset 0x0000 (which, on some IMX models,
seems to be another ID register)?

> 
> 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: media: Rename imx412 to imx577
>   media: i2c: imx577: Rename imx412.c to imx577.c
>   media: i2c: imx577: Fix chip identifier define name
> 
>  .../{sony,imx412.yaml => sony,imx577.yaml}    |  18 +-
>  MAINTAINERS                                   |   6 +-
>  drivers/media/i2c/Kconfig                     |   8 +-
>  drivers/media/i2c/Makefile                    |   2 +-
>  drivers/media/i2c/{imx412.c => imx577.c}      | 622 +++++++++---------
>  5 files changed, 328 insertions(+), 328 deletions(-)
>  rename Documentation/devicetree/bindings/media/i2c/{sony,imx412.yaml => sony,imx577.yaml} (83%)
>  rename drivers/media/i2c/{imx412.c => imx577.c} (55%)
>