[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0d45fca3-5b6f-42e5-acec-bca2dda25f15@linux.intel.com>
Date: Wed, 2 Jul 2025 14:08:26 +0800
From: "Yan, Dongcheng" <dongcheng.yan@...ux.intel.com>
To: "Nirujogi, Pratap" <pnirujog@....com>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Hao Yao <hao.yao@...el.com>, Pratap Nirujogi <pratap.nirujogi@....com>,
mchehab@...nel.org, hverkuil@...all.nl, bryan.odonoghue@...aro.org,
krzk@...nel.org, dave.stevenson@...pberrypi.com, hdegoede@...hat.com,
jai.luthra@...asonboard.com, tomi.valkeinen@...asonboard.com,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
benjamin.chan@....com, bin.du@....com, grosikop@....com, king.li@....com,
dantony@....com, vengutta@....com, dongcheng.yan@...el.com,
jason.z.chen@...el.com, jimmy.su@...el.com
Subject: Re: [PATCH v3 RESEND] media: i2c: Add OV05C10 camera sensor driver
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
>
>
Powered by blists - more mailing lists