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

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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