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@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 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%) >
