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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 28/07/2022 12:41, sakari.ailus@xxxxxx 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@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.

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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux