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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <175093628786.4005407.10292502794888309807@ping.linuxembedded.co.uk>
Date: Thu, 26 Jun 2025 12:11:27 +0100
From: Kieran Bingham <kieran.bingham@...asonboard.com>
To: "Nirujogi, Pratap" <pnirujog@....com>, Laurent Pinchart <laurent.pinchart@...asonboard.com>, Sakari Ailus <sakari.ailus@...ux.intel.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, Svetoslav.Stoilov@....com, Yana.Zheleva@....com
Subject: Re: [PATCH v3 RESEND] media: i2c: Add OV05C10 camera sensor driver

Quoting Nirujogi, Pratap (2025-06-25 23:06:01)
> Hi Sakari, Hi Laurent,
> 
> On 6/23/2025 5:55 PM, Nirujogi, Pratap wrote:
> [...]
> >>>> 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.
> > 
> I would like to share the approach we are taking to implement the CCI 
> helpers that support page value. Could you please review the steps and 
> let us know if they make sense or if any adjustments are needed?
> 
> 1: Add new macros to embed page value into the register address.
> 
> Ex:
> #define CCI_PAGE_REG8(x, p)             ((1 << CCI_REG_WIDTH_SHIFT) | (p << 
> CCI_REG_PRIVATE_SHIFT) | (x))
> #define CCI_PAGE_REG16(x, p)            ((2 << CCI_REG_WIDTH_SHIFT) | (p << 
> CCI_REG_PRIVATE_SHIFT) | (x))
> 
> 2: Create V4L2 CCI context. Initialize page control reg, current_page, 
> regmap etc.
> 
> Ex:
> struct v4l2_cci_ctx {
>         struct mutex lock;
>         struct regmap *map;
>         s16 current_page;
>         u8 page_ctrl_reg;
> }
> 
> 3: Introduce new CCI helpers - cci_pwrite() and cci_pread() to handle 
> register read-writes updating the page control register as necessary.

Out of curiosity - but couldn't the existing cci_write and cci_read
already be used by the users - and then the default behaviour is that
the page isn't modified if there is no page_ctrl_reg - and by default
CCI_REG() will simply have (initilised) as page 0 - so the pages will
never change on those calls?

Then the users can indeed define

#define TEST_PATTERN_PAGE 5
#define TEST_PATTERN_COLOUR_BARS BIT(3)

#define MY_TEST_PATTERN_REG CCI_PAGE_REG8(0x33, TEST_PATTERN_PAGE)

and can call 
 cci_write(regmap, MY_TEST_PATTERN_REG, TEST_PATTERN_COLOUR_BARS, &ret);

with everything handled transparently ?


Or do you envisage more complications with the types of pages that might
be supportable ?

(I perfectly understand if I'm wishing for an unreachable utopia -
because I haven't considered something implicit about the page handling
that I haven't yet used :D)

--
Kieran


> int cci_pwrite(void *data, u32 reg, u64 val, int *err)
> {
>         /* get v4l2_cci_ctx context from data */
> 
>         /* get page value from reg */
> 
>         /* acquire mutex */
> 
>         /* update cci page control reg, save current page value */
>         
>         /* do cci_write */
> 
>         /* release mutex */
> }
> 
> Similar steps for cci_pread() as well.
> 
> Thanks,
> Pratap