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: <31ba098c-9f1e-4710-b6b3-042a510cda52@amd.com>
Date: Thu, 26 Jun 2025 14:14:06 -0400
From: "Nirujogi, Pratap" <pnirujog@....com>
To: Kieran Bingham <kieran.bingham@...asonboard.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

Hi Kieran,

On 6/26/2025 7:11 AM, Kieran Bingham wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> 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)
> 
Initally we too felt the existing cci_write() / cci_read() should be 
updated to support pages, but worried about the potential regressions 
with other drivers that are based on existing implementation. We felt 
adding new helper functions in V4L2 CCI — while keeping the original 
cci_write() and cci_read() unchanged — would be a safer and more stable 
approach.

Thanks,
Pratap

> --
> 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