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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

-- 
Regards,

Laurent Pinchart