| Message ID | 20220718014215.1240114-1-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add imx577 compatible to imx412 | expand |
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%) >
On 21/07/2022 16:15, Alessandrelli, Daniele wrote: > 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)? Looks like it is zero. [ 10.422064] imx577 20-001a: Register @ 0x0000 0x0000 register @ 0x0016 0x0577
On Thu, 2022-07-21 at 16:15 +0100, Daniele Alessandrelli wrote: > 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. I managed to get one of the imx412 sensors that were used for the development of this driver and I can confirm that the CHIP ID reported by the HW is 0x577 (as specified in the datasheet I was given) However, somebody [1] on the internet is reporting that their imx412 shows a different ID, so it's possible that different batches of imx412 have different IDs (perhaps Sony fixed the ID inconsistency at some point). But I'd like to see more evidence of this. [1] https://discuss.96boards.org/t/imx412-driver-troubleshooting/11350 Anyway, regardless of the ID, I think this driver shouldn't be renamed because it was written for imx412, using an imx412. If there exist other imx412 devices with a different ID, I think we should just add support for that ID to the driver. > > 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%) > > >
On 27/07/2022 21:12, Alessandrelli, Daniele wrote: > On Thu, 2022-07-21 at 16:15 +0100, Daniele Alessandrelli wrote: >> 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. > > I managed to get one of the imx412 sensors that were used for the > development of this driver and I can confirm that the CHIP ID reported > by the HW is 0x577 (as specified in the datasheet I was given) Did you happen to check the other offset we discussed, the one you mentioned @ 0x0000 ? [ 10.422064] imx577 20-001a: Register @ 0x0000 0x0000 register @ 0x0016 0x0577 > However, somebody [1] on the internet is reporting that their imx412 > shows a different ID, so it's possible that different batches of imx412 > have different IDs (perhaps Sony fixed the ID inconsistency at some > point). But I'd like to see more evidence of this. > > [1] https://discuss.96boards.org/t/imx412-driver-troubleshooting/11350 > > Anyway, regardless of the ID, I think this driver shouldn't be renamed > because it was written for imx412, using an imx412. Can't say I agree there. Currently imx412.c - Uses the imx577 init sequence - Uses the imx577 register id if any thing imx412 should be considered a special case of imx577 not the reverse. ./oem/qcom/sensor/imx577/imx577_sensor.xml<registerAddr>0x0136</registerAddr> <registerData>0x18</registerData> <registerAddr>0x0137</registerAddr> <registerData>0x00</registerData><registerAddr>0x3C7E</registerAddr> <registerData>0x01</registerData> <registerAddr>0x3C7F</registerAddr> <registerData>0x02</registerData> <registerAddr>0x38A8</registerAddr> <registerData>0x1F</registerData> <registerAddr>0x38A9</registerAddr> <registerData>0xFF</registerData> <registerAddr>0x38AA</registerAddr> <registerData>0x1F</registerData> <registerAddr>0x38AB</registerAddr> <registerData>0xFF</registerData> <registerAddr>0x55D4</registerAddr> <registerData>0x00</registerData> <registerAddr>0x55D5</registerAddr> <registerData>0x00</registerData> ... drivers/media/i2c/imx412.c static const struct imx412_reg mode_4056x3040_regs[] = { {0x0136, 0x18}, {0x0137, 0x00}, {0x3c7e, 0x08}, {0x3c7f, 0x02}, {0x38a8, 0x1f}, {0x38a9, 0xff}, {0x38aa, 0x1f}, {0x38ab, 0xff}, {0x55d4, 0x00}, {0x55d5, 0x00}, ... drivers/media/i2c/imx258.c:#define IMX258_CHIP_ID 0x0258 drivers/media/i2c/imx319.c:#define IMX319_CHIP_ID 0x0319 drivers/media/i2c/imx355.c:#define IMX355_CHIP_ID 0x0355 and then make this departure drivers/media/i2c/imx412.c:#define IMX577_ID 0x577 I honestly think we should rename to imx577 here, it seems very illogical to have the imx577 init sequence, the imx577 register identifier but break with the established naming convention and call the driver imx412. --- bod
Hi Bryan, On Thu, Jul 28, 2022 at 03:01:38AM +0100, Bryan O'Donoghue wrote: > On 27/07/2022 21:12, Alessandrelli, Daniele wrote: > > On Thu, 2022-07-21 at 16:15 +0100, Daniele Alessandrelli wrote: > > > 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. > > > > I managed to get one of the imx412 sensors that were used for the > > development of this driver and I can confirm that the CHIP ID reported > > by the HW is 0x577 (as specified in the datasheet I was given) > > Did you happen to check the other offset we discussed, the one you mentioned > @ 0x0000 ? > > [ 10.422064] imx577 20-001a: Register @ 0x0000 0x0000 register @ 0x0016 > 0x0577 > > > However, somebody [1] on the internet is reporting that their imx412 > > shows a different ID, so it's possible that different batches of imx412 > > have different IDs (perhaps Sony fixed the ID inconsistency at some > > point). But I'd like to see more evidence of this. > > > > [1] https://discuss.96boards.org/t/imx412-driver-troubleshooting/11350 > > > > Anyway, regardless of the ID, I think this driver shouldn't be renamed > > because it was written for imx412, using an imx412. > > Can't say I agree there. > > Currently imx412.c > > - Uses the imx577 init sequence > - Uses the imx577 register id > > if any thing imx412 should be considered a special case of imx577 not the > reverse. > > ./oem/qcom/sensor/imx577/imx577_sensor.xml<registerAddr>0x0136</registerAddr> > <registerData>0x18</registerData> > <registerAddr>0x0137</registerAddr> > <registerData>0x00</registerData><registerAddr>0x3C7E</registerAddr> > <registerData>0x01</registerData> > <registerAddr>0x3C7F</registerAddr> > <registerData>0x02</registerData> > <registerAddr>0x38A8</registerAddr> > <registerData>0x1F</registerData> > <registerAddr>0x38A9</registerAddr> > <registerData>0xFF</registerData> > <registerAddr>0x38AA</registerAddr> > <registerData>0x1F</registerData> > <registerAddr>0x38AB</registerAddr> > <registerData>0xFF</registerData> > <registerAddr>0x55D4</registerAddr> > <registerData>0x00</registerData> > <registerAddr>0x55D5</registerAddr> > <registerData>0x00</registerData> > ... > > drivers/media/i2c/imx412.c > static const struct imx412_reg mode_4056x3040_regs[] = { > {0x0136, 0x18}, > {0x0137, 0x00}, > {0x3c7e, 0x08}, > {0x3c7f, 0x02}, > {0x38a8, 0x1f}, > {0x38a9, 0xff}, > {0x38aa, 0x1f}, > {0x38ab, 0xff}, > {0x55d4, 0x00}, > {0x55d5, 0x00}, > ... > > drivers/media/i2c/imx258.c:#define IMX258_CHIP_ID 0x0258 > drivers/media/i2c/imx319.c:#define IMX319_CHIP_ID 0x0319 > drivers/media/i2c/imx355.c:#define IMX355_CHIP_ID 0x0355 > > and then make this departure > > drivers/media/i2c/imx412.c:#define IMX577_ID 0x577 > > I honestly think we should rename to imx577 here, it seems very illogical to > have the imx577 init sequence, the imx577 register identifier but break with > the established naming convention and call the driver imx412. The device is called IMX412 according to Sony, so similarly the driver is called imx412. What's in the model ID register is another matter. On IMX577 support, please simply add a new compatible string for it and mention in driver's Kconfig help text it supports both. If the driver would be renamed, MODULE_ALIAS() needs to be added. But I'm not sure how useful renaming would be: neither IMX412 or IMX577 is more generic than the other.
On 28/07/2022 12:41, sakari.ailus@iki.fi wrote: > Hi Bryan, > > On Thu, Jul 28, 2022 at 03:01:38AM +0100, Bryan O'Donoghue wrote: >> On 27/07/2022 21:12, Alessandrelli, Daniele wrote: >>> On Thu, 2022-07-21 at 16:15 +0100, Daniele Alessandrelli wrote: >>>> 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. >>> >>> I managed to get one of the imx412 sensors that were used for the >>> development of this driver and I can confirm that the CHIP ID reported >>> by the HW is 0x577 (as specified in the datasheet I was given) >> >> Did you happen to check the other offset we discussed, the one you mentioned >> @ 0x0000 ? >> >> [ 10.422064] imx577 20-001a: Register @ 0x0000 0x0000 register @ 0x0016 >> 0x0577 >> >>> However, somebody [1] on the internet is reporting that their imx412 >>> shows a different ID, so it's possible that different batches of imx412 >>> have different IDs (perhaps Sony fixed the ID inconsistency at some >>> point). But I'd like to see more evidence of this. >>> >>> [1] https://discuss.96boards.org/t/imx412-driver-troubleshooting/11350 >>> >>> Anyway, regardless of the ID, I think this driver shouldn't be renamed >>> because it was written for imx412, using an imx412. >> >> Can't say I agree there. >> >> Currently imx412.c >> >> - Uses the imx577 init sequence >> - Uses the imx577 register id >> >> if any thing imx412 should be considered a special case of imx577 not the >> reverse. >> >> ./oem/qcom/sensor/imx577/imx577_sensor.xml<registerAddr>0x0136</registerAddr> >> <registerData>0x18</registerData> >> <registerAddr>0x0137</registerAddr> >> <registerData>0x00</registerData><registerAddr>0x3C7E</registerAddr> >> <registerData>0x01</registerData> >> <registerAddr>0x3C7F</registerAddr> >> <registerData>0x02</registerData> >> <registerAddr>0x38A8</registerAddr> >> <registerData>0x1F</registerData> >> <registerAddr>0x38A9</registerAddr> >> <registerData>0xFF</registerData> >> <registerAddr>0x38AA</registerAddr> >> <registerData>0x1F</registerData> >> <registerAddr>0x38AB</registerAddr> >> <registerData>0xFF</registerData> >> <registerAddr>0x55D4</registerAddr> >> <registerData>0x00</registerData> >> <registerAddr>0x55D5</registerAddr> >> <registerData>0x00</registerData> >> ... >> >> drivers/media/i2c/imx412.c >> static const struct imx412_reg mode_4056x3040_regs[] = { >> {0x0136, 0x18}, >> {0x0137, 0x00}, >> {0x3c7e, 0x08}, >> {0x3c7f, 0x02}, >> {0x38a8, 0x1f}, >> {0x38a9, 0xff}, >> {0x38aa, 0x1f}, >> {0x38ab, 0xff}, >> {0x55d4, 0x00}, >> {0x55d5, 0x00}, >> ... >> >> drivers/media/i2c/imx258.c:#define IMX258_CHIP_ID 0x0258 >> drivers/media/i2c/imx319.c:#define IMX319_CHIP_ID 0x0319 >> drivers/media/i2c/imx355.c:#define IMX355_CHIP_ID 0x0355 >> >> and then make this departure >> >> drivers/media/i2c/imx412.c:#define IMX577_ID 0x577 >> >> I honestly think we should rename to imx577 here, it seems very illogical to >> have the imx577 init sequence, the imx577 register identifier but break with >> the established naming convention and call the driver imx412. > > The device is called IMX412 according to Sony, so similarly the driver is > called imx412. The device I have is called an imx577 by Sony and reports 0x577 in the chip identifier field. What's in the model ID register is another matter. On IMX577 > support, please simply add a new compatible string for it and mention in > driver's Kconfig help text it supports both. I'm happy to add a compat string, np. It seems to me, given our established naming convention for these chips => drivers/media/i2c/imx258.c:#define IMX258_CHIP_ID 0x0258 drivers/media/i2c/imx319.c:#define IMX319_CHIP_ID 0x0319 drivers/media/i2c/imx355.c:#define IMX355_CHIP_ID 0x0355 there _ought_ to be two compat strings "imx412" and "imx577" living inside of a driver called imx577.c not imx412.c --- bod
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 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%)