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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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.

-- 
Kind regards,

Sakari Ailus