linux-kernel - Re: [PATCH v5 2/2] media: i2c: add ov2735 image 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:
 <PN3P287MB3519055FE42890F5F577B51BFF58A@PN3P287MB3519.INDP287.PROD.OUTLOOK.COM>
Date: Sat, 26 Jul 2025 06:06:05 +0000
From: Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>
To: Kieran Bingham <kieran.bingham@...asonboard.com>
CC: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	"sakari.ailus@...ux.intel.com" <sakari.ailus@...ux.intel.com>,
	"laurent.pinchart@...asonboard.com" <laurent.pinchart@...asonboard.com>,
	Himanshu Bhavani <himanshu.bhavani@...iconsignals.io>, Mauro Carvalho Chehab
	<mchehab@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Hans Verkuil
	<hverkuil@...all.nl>, Ricardo Ribalda <ribalda@...omium.org>, Bryan
 O'Donoghue <bryan.odonoghue@...aro.org>, Hans de Goede <hansg@...nel.org>,
	André Apitzsch <git@...tzsch.eu>, Benjamin Mugnier
	<benjamin.mugnier@...s.st.com>, Matthias Fend <matthias.fend@...end.at>,
	Heimir Thor Sverrisson <heimir.sverrisson@...il.com>, Sylvain Petinot
	<sylvain.petinot@...s.st.com>, Dongcheng Yan <dongcheng.yan@...el.com>,
	Jingjing Xiong <jingjing.xiong@...el.com>, Arnd Bergmann <arnd@...db.de>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 2/2] media: i2c: add ov2735 image sensor driver

> Quoting Hardevsinh Palaniya (2025-07-25 06:55:23)
> <snip>
> > > > +static int ov2735_page_access(struct ov2735 *ov2735,
> > > > +�������������������������� u32 reg, void *val, int *err, bool is_read)
> > > > +{
> > > > +���� u8 page = (reg >> CCI_REG_PRIVATE_SHIFT) & 0xff;
> > >
> > > ' & 0xff' part is redundant.
> > >
> > > > +���� u32 addr = reg & ~CCI_REG_PRIVATE_MASK;
> > > > +���� int ret = 0;
> > >
> > > How is this assignment being used?
> > >
> > > > +���� if (err && *err)
> > > > +������������ return *err;
> > > > +
> > > > +���� mutex_lock(&ov2735->page_lock);
> > > > +
> > > > +���� /* Perform page access before read/write */
> > > > +���� if (ov2735->current_page != page) {
> > > > +������������ ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, page, err);
> > > > +������������ if (ret)
> > > > +�������������������� goto err_mutex_unlock;
> > > > +������������ ov2735->current_page = page;
> > > > +���� }
> > > > +
> > > > +���� if (is_read)
> > > > +������������ ret = cci_read(ov2735->cci, addr, (u64 *)val, err);
> > > > +���� else
> > > > +������������ ret = cci_write(ov2735->cci, addr, *(u64 *)val, err);
> > >
> > > Do you really need this castings?
> >
> > Do you really think this casting is unnecessary?
> >
> 
> Yes? Well quite probably - I haven't checked myself yet but ..
> 
> 
> > Please check the definitions of cci_read/write
> >
> > without this, we can't even build the driver.
> 
> How about ... changing the function prototype of ov2735_page_access ?

Of course, changing the function prototype would work.

My intention is to keep a single ov2735_page_access() function that can
handle both read and write operations. The cci_read() function expects 
a u64 *, whereas cci_write() expects a u64 value. To support both cases
within one function, I’ve used a void *val and cast it appropriately 
depending on the operation.

If we were to remove the casting, we would need to split this into two
separate functions, one for read and one for write, even though the only 
difference between them would be a single line. I’d prefer to avoid that
redundancy and keep the code compact. 

Let me know if you see a better way to handle this without duplicating
the logic. 

Best Regards,
Hardev