Re: [PATCH v3 RESEND] media: i2c: Add OV05C10 camera sensor driver

[Yan, Dongcheng]

On 7/3/2025 12:47 AM, Nirujogi, Pratap wrote:
> Hi Sakari, Dongcheng,
> 
> On 7/2/2025 2:12 AM, Sakari Ailus wrote:
>> Caution: This message originated from an External Source. Use proper
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> Hi Dongcheng, Pratap,
>>
>> On Wed, Jul 02, 2025 at 02:08:26PM +0800, Yan, Dongcheng wrote:
>>> Hi Pratap,
>>>
>>> On 7/1/2025 6:31 AM, Nirujogi, Pratap wrote:
>>>> Hi Sakari, Hi Laurent,
>>>>
>>>> On 6/29/2025 3:40 AM, Sakari Ailus wrote:
>>>>> Caution: This message originated from an External Source. Use proper
>>>>> caution when opening attachments, clicking links, or responding.
>>>>>
>>>>>
>>>>> Hi Pratap,
>>>>>
>>>>> On 6/17/25 01:33, Nirujogi, Pratap wrote:
>>>>> ...
>>>>>>>>>> +static const struct cci_reg_sequence ov05c10_2888x1808_regs[]
>>>>>>>>>> = {
>>>>>>>>>> + { CCI_REG8(0xfd),  0x00 },
>>>>>>>>>> + { CCI_REG8(0x20),  0x00 },
>>>>>>>>>> + { CCI_REG8(0xfd),  0x00 },
>>>>>>>>>> + { CCI_REG8(0x20),  0x0b },
>>>>>>>>>> + { CCI_REG8(0xc1),  0x09 },
>>>>>>>>>> + { CCI_REG8(0x21),  0x06 },
>>>>>>>>>> + { CCI_REG8(0x14),  0x78 },
>>>>>>>>>> + { CCI_REG8(0xe7),  0x03 },
>>>>>>>>>> + { CCI_REG8(0xe7),  0x00 },
>>>>>>>>>> + { CCI_REG8(0x21),  0x00 },
>>>>>>>>>> + { CCI_REG8(0xfd),  0x01 },
>>>>>>>>>> + { CCI_REG8(0x03),  0x00 },
>>>>>>>>>> + { CCI_REG8(0x04),  0x06 },
>>>>>>>>>> + { CCI_REG8(0x05),  0x07 },
>>>>>>>>>> + { CCI_REG8(0x06),  0x44 },
>>>>>>>>>> + { CCI_REG8(0x07),  0x08 },
>>>>>>>>>> + { CCI_REG8(0x1b),  0x01 },
>>>>>>>>>> + { CCI_REG8(0x24),  0xff },
>>>>>>>>>> + { CCI_REG8(0x32),  0x03 },
>>>>>>>>>> + { CCI_REG8(0x42),  0x5d },
>>>>>>>>>> + { CCI_REG8(0x43),  0x08 },
>>>>>>>>>> + { CCI_REG8(0x44),  0x81 },
>>>>>>>>>> + { CCI_REG8(0x46),  0x5f },
>>>>>>>>>> + { CCI_REG8(0x48),  0x18 },
>>>>>>>>>> + { CCI_REG8(0x49),  0x04 },
>>>>>>>>>> + { CCI_REG8(0x5c),  0x18 },
>>>>>>>>>> + { CCI_REG8(0x5e),  0x13 },
>>>>>>>>>> + { CCI_REG8(0x70),  0x15 },
>>>>>>>>>> + { CCI_REG8(0x77),  0x35 },
>>>>>>>>>> + { CCI_REG8(0x79),  0x00 },
>>>>>>>>>> + { CCI_REG8(0x7b),  0x08 },
>>>>>>>>>> + { CCI_REG8(0x7d),  0x08 },
>>>>>>>>>> + { CCI_REG8(0x7e),  0x08 },
>>>>>>>>>> + { CCI_REG8(0x7f),  0x08 },
>>>>>>>>>> + { CCI_REG8(0x90),  0x37 },
>>>>>>>>>> + { CCI_REG8(0x91),  0x05 },
>>>>>>>>>> + { CCI_REG8(0x92),  0x18 },
>>>>>>>>>> + { CCI_REG8(0x93),  0x27 },
>>>>>>>>>> + { CCI_REG8(0x94),  0x05 },
>>>>>>>>>> + { CCI_REG8(0x95),  0x38 },
>>>>>>>>>> + { CCI_REG8(0x9b),  0x00 },
>>>>>>>>>> + { CCI_REG8(0x9c),  0x06 },
>>>>>>>>>> + { CCI_REG8(0x9d),  0x28 },
>>>>>>>>>> + { CCI_REG8(0x9e),  0x06 },
>>>>>>>>>> + { CCI_REG8(0xb2),  0x0f },
>>>>>>>>>> + { CCI_REG8(0xb3),  0x29 },
>>>>>>>>>> + { CCI_REG8(0xbf),  0x3c },
>>>>>>>>>> + { CCI_REG8(0xc2),  0x04 },
>>>>>>>>>> + { CCI_REG8(0xc4),  0x00 },
>>>>>>>>>> + { CCI_REG8(0xca),  0x20 },
>>>>>>>>>> + { CCI_REG8(0xcb),  0x20 },
>>>>>>>>>> + { CCI_REG8(0xcc),  0x28 },
>>>>>>>>>> + { CCI_REG8(0xcd),  0x28 },
>>>>>>>>>> + { CCI_REG8(0xce),  0x20 },
>>>>>>>>>> + { CCI_REG8(0xcf),  0x20 },
>>>>>>>>>> + { CCI_REG8(0xd0),  0x2a },
>>>>>>>>>> + { CCI_REG8(0xd1),  0x2a },
>>>>>>>>>> + { CCI_REG8(0xfd),  0x0f },
>>>>>>>>>> + { CCI_REG8(0x00),  0x00 },
>>>>>>>>>> + { CCI_REG8(0x01),  0xa0 },
>>>>>>>>>> + { CCI_REG8(0x02),  0x48 },
>>>>>>>>>> + { CCI_REG8(0x07),  0x8f },
>>>>>>>>>> + { CCI_REG8(0x08),  0x70 },
>>>>>>>>>> + { CCI_REG8(0x09),  0x01 },
>>>>>>>>>> + { CCI_REG8(0x0b),  0x40 },
>>>>>>>>>> + { CCI_REG8(0x0d),  0x07 },
>>>>>>>>>> + { CCI_REG8(0x11),  0x33 },
>>>>>>>>>> + { CCI_REG8(0x12),  0x77 },
>>>>>>>>>> + { CCI_REG8(0x13),  0x66 },
>>>>>>>>>> + { CCI_REG8(0x14),  0x65 },
>>>>>>>>>> + { CCI_REG8(0x15),  0x37 },
>>>>>>>>>> + { CCI_REG8(0x16),  0xbf },
>>>>>>>>>> + { CCI_REG8(0x17),  0xff },
>>>>>>>>>> + { CCI_REG8(0x18),  0xff },
>>>>>>>>>> + { CCI_REG8(0x19),  0x12 },
>>>>>>>>>> + { CCI_REG8(0x1a),  0x10 },
>>>>>>>>>> + { CCI_REG8(0x1c),  0x77 },
>>>>>>>>>> + { CCI_REG8(0x1d),  0x77 },
>>>>>>>>>> + { CCI_REG8(0x20),  0x0f },
>>>>>>>>>> + { CCI_REG8(0x21),  0x0f },
>>>>>>>>>> + { CCI_REG8(0x22),  0x0f },
>>>>>>>>>> + { CCI_REG8(0x23),  0x0f },
>>>>>>>>>> + { CCI_REG8(0x2b),  0x20 },
>>>>>>>>>> + { CCI_REG8(0x2c),  0x20 },
>>>>>>>>>> + { CCI_REG8(0x2d),  0x04 },
>>>>>>>>>> + { CCI_REG8(0xfd),  0x03 },
>>>>>>>>>> + { CCI_REG8(0x9d),  0x0f },
>>>>>>>>>> + { CCI_REG8(0x9f),  0x40 },
>>>>>>>>>> + { CCI_REG8(0xfd),  0x00 },
>>>>>>>>>> + { CCI_REG8(0x20),  0x1b },
>>>>>>>>>> + { CCI_REG8(0xfd),  0x04 },
>>>>>>>>>> + { CCI_REG8(0x19),  0x60 },
>>>>>>>>>> + { CCI_REG8(0xfd),  0x02 },
>>>>>>>>>> + { CCI_REG8(0x75),  0x05 },
>>>>>>>>>> + { CCI_REG8(0x7f),  0x06 },
>>>>>>>>>> + { CCI_REG8(0x9a),  0x03 },
>>>>>>>>>> + { CCI_REG8(0xa2),  0x07 },
>>>>>>>>>> + { CCI_REG8(0xa3),  0x10 },
>>>>>>>>>> + { CCI_REG8(0xa5),  0x02 },
>>>>>>>>>> + { CCI_REG8(0xa6),  0x0b },
>>>>>>>>>> + { CCI_REG8(0xa7),  0x48 },
>>>>>>>>>> + { CCI_REG8(0xfd),  0x07 },
>>>>>>>>>> + { CCI_REG8(0x42),  0x00 },
>>>>>>>>>> + { CCI_REG8(0x43),  0x80 },
>>>>>>>>>> + { CCI_REG8(0x44),  0x00 },
>>>>>>>>>> + { CCI_REG8(0x45),  0x80 },
>>>>>>>>>> + { CCI_REG8(0x46),  0x00 },
>>>>>>>>>> + { CCI_REG8(0x47),  0x80 },
>>>>>>>>>> + { CCI_REG8(0x48),  0x00 },
>>>>>>>>>> + { CCI_REG8(0x49),  0x80 },
>>>>>>>>>> + { CCI_REG8(0x00),  0xf7 },
>>>>>>>>>> + { CCI_REG8(0xfd),  0x00 },
>>>>>>>>>> + { CCI_REG8(0xe7),  0x03 },
>>>>>>>>>> + { CCI_REG8(0xe7),  0x00 },
>>>>>>>>>> + { CCI_REG8(0xfd),  0x00 },
>>>>>>>>>> + { CCI_REG8(0x93),  0x18 },
>>>>>>>>>> + { CCI_REG8(0x94),  0xff },
>>>>>>>>>> + { CCI_REG8(0x95),  0xbd },
>>>>>>>>>> + { CCI_REG8(0x96),  0x1a },
>>>>>>>>>> + { CCI_REG8(0x98),  0x04 },
>>>>>>>>>> + { CCI_REG8(0x99),  0x08 },
>>>>>>>>>> + { CCI_REG8(0x9b),  0x10 },
>>>>>>>>>> + { CCI_REG8(0x9c),  0x3f },
>>>>>>>>>> + { CCI_REG8(0xa1),  0x05 },
>>>>>>>>>> + { CCI_REG8(0xa4),  0x2f },
>>>>>>>>>> + { CCI_REG8(0xc0),  0x0c },
>>>>>>>>>> + { CCI_REG8(0xc1),  0x08 },
>>>>>>>>>> + { CCI_REG8(0xc2),  0x00 },
>>>>>>>>>> + { CCI_REG8(0xb6),  0x20 },
>>>>>>>>>> + { CCI_REG8(0xbb),  0x80 },
>>>>>>>>>> + { CCI_REG8(0xfd),  0x00 },
>>>>>>>>>> + { CCI_REG8(0xa0),  0x01 },
>>>>>>>>>> + { CCI_REG8(0xfd),  0x01 },
>>>>>>>
>>>>>>> Please replace these with names macros where possible. I'm sure
>>>>>>> quite a
>>>>>>> few of the registers configured here are documented in the
>>>>>>> datasheet.
>>>>>>> The registers that configure the mode (analog crop, digital crop,
>>>>>>> binning, skipping, ...) should be computed dynamically from the
>>>>>>> subdev
>>>>>>> pad format and selection rectangles, not hardcoded.
>>>>>>>
>>>>>> I agree, but we get the sensor settings based on our requirements
>>>>>> from
>>>>>> the vendor, i will check if we can get some more info regarding the
>>>>>> crop, binning, skipping etc...
>>>>>
>>>>> Some of this infomation should be available in the datasheet. Use at
>>>>> least the register names that can be found, for those that can't
>>>>> there's
>>>>> not much that could be done.
>>>>>
>>>> Sorry to say that I don't have the details in this case. We have
>>>> previously reached out to the sensor vendor, but they are not
>>>> willing to
>>>> disclose any of these details. We hope for your understanding of the
>>>> constraints we're facing and truly value your support.
>>>>
>>>
>>> If you have a spec of OV05C10 (I assume you do, as the developer of this
>>> driver), it is not a issue.
>>> Take P0:0x14 as an example, it's named as DPLL_NC_SEL in spec and set to
>>> 0x78 in your reglist ov05c10_2888x1808_regs. If define all named
>>> registers rather than the confusing magic hardcode, the driver code will
>>> be more readable and easy to review.
>>> I think this is what Sakari thought.
>>
>> Yes. And even if it happens that a register write slips to a wrong list,
>> we can fix it later.
>>
> I agree with the suggestion on proper naming of register offsets, but
> unfortunately we lack access to the spec.

Do you mean this driver is developed without spec? Noticing that
OV05C10_*CTL_PAGE and OV05C10_REG_* are defined quite standard, I never
doubt it. Excuse me for being a little straightforward, I even doubt
whether this driver can work properly.

 We are completely relying on
> the sensor vendor for these sequences, which they are not willing to
> share the details.
> 
> Thanks,
> Pratap
> 
>> -- 
>> Kind regards,
>>
>> Sakari Ailus
>