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

["Yan, Dongcheng" <dongcheng.yan@linux.intel.com>]

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.

Thanks,
Dongcheng

> Thanks,
> Pratap
> 
>> -- 
>> Regards,
>>
>> Sakari Ailus
> 
>