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

["Nirujogi, Pratap" <pnirujog@amd.com>]

On 6/23/2025 6:06 PM, Laurent Pinchart wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Mon, Jun 23, 2025 at 05:55:14PM -0400, Nirujogi, Pratap wrote:
>> On 6/23/2025 9:42 AM, Laurent Pinchart wrote:
>>> On Mon, Jun 23, 2025 at 01:22:00PM +0000, Sakari Ailus wrote:
>>>> On Mon, Jun 23, 2025 at 03:09:29PM +0300, Laurent Pinchart wrote:
>>>>> On Mon, Jun 16, 2025 at 07:12:28PM -0400, Nirujogi, Pratap wrote:
>>>>>> On 6/13/2025 6:02 PM, Sakari Ailus wrote:
>>>>>>> On Fri, Jun 13, 2025 at 12:55:46PM +0800, Hao Yao wrote:
>>>>>>>> Hi Pratap,
>>>>>>>>
>>>>>>>> Thanks for your patch.
>>>>>>>>
>>>>>>>> This patch is written for your camera sensor module, which seems very
>>>>>>>> different from those already applied on Dell laptops (some of "Dell Pro"
>>>>>>>> series). Looking into the driver, I think this version will break the
>>>>>>>> devices using ov05c10 sensor.
>>>>>>>
>>>>>>> There never was such a driver in upstream so nothing breaks. However, in
>>>>>>> order to support these, could you check what would it take to support them
>>>>>>> using this driver and post patches, please?
>>>>>>>
>>>>>>>> I think this patch is better to be validated on existing devices, but please
>>>>>>>> do some fixes before we can do validation. Please check my comments inline.
>>>>>>>>
>>>>>>>> On 2025/6/10 03:42, Pratap Nirujogi wrote:
>>>>>>>>> Add driver for OmniVision 5.2M OV05C10 sensor. This driver
>>>>>>>>> supports only the full size normal 2888x1808@30fps 2-lane
>>>>>>>>> sensor profile.
>>>>>>>>>
>>>>>>>>> Co-developed-by: Venkata Narendra Kumar Gutta <vengutta@amd.com>
>>>>>>>>> Signed-off-by: Venkata Narendra Kumar Gutta <vengutta@amd.com>
>>>>>>>>> Co-developed-by: Bin Du <bin.du@amd.com>
>>>>>>>>> Signed-off-by: Bin Du <bin.du@amd.com>
>>>>>>>>> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@amd.com>
>>>>>
>>>>> [snip]
>>>>>
>>>>>>>> Hi Sakari,
>>>>>>>>
>>>>>>>> Seems there are already several camera sensors using page-based registers.
>>>>>>>> Is it a good idea to add page support in CCI interface?
>>>>>>>
>>>>>>> Sounds like a good idea as such but I'm not sure how common this really is,
>>>>>>> I think I've seen a few Omnivision sensors doing this. If implemented, I
>>>>>>> think it would be nice if the page could be encoded in the register address
>>>>>>> which V4L2 CCI would store and switch page if needed only. This would
>>>>>>> require serialising accesses, too. There's some room in CCI register raw
>>>>>>> value space so this could be done without even changing that, say, with
>>>>>>> 8-bit page and 8-bit register address.
>>>>>>
>>>>>> Hi Sakari, thank you for sharing your insights and guiding us. Could you
>>>>>> please suggest if we should take up this work implementing the helpers
>>>>>> in CCI and submit the patch or is it okay to leave it as-is for now and
>>>>>> take care of updating in future once the implementation is ready.
>>>>>
>>>>> I think it can live in the driver for now. Given that the device uses
>>>>> only 8 bits of register address, I would store the page number in bits
>>>>> 15:8 instead of bits 31:24, as the CCI helpers do not make bits 27:24
>>>>> available for driver-specific purpose.
>>>>
>>>> I'd use the CCI private bits, the driver uses page numbers up to 4 so 4
>>>> bits are plenty for that. If we add pages to CCI later, this may be
>>>> refactored then.
>>>
>>> That works too.
>>
>> Thanks for your support. We will add the page number in the register
>> address 15:8 or 11:8 and will update the implementation accordingly in
>> the next version.
> 
> The CCI private bits are bits 31:28.
> 
Thanks. Will use the CCI private bits 31:28 instead of bits 15:8 / 11:8

Thanks,
Pratap

> --
> Regards,
> 
> Laurent Pinchart