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: <175345442477.2567018.13588829522231689027@ping.linuxembedded.co.uk>
Date: Fri, 25 Jul 2025 15:40:24 +0100
From: Kieran Bingham <kieran.bingham@...asonboard.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>
Cc: 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 ?

Then you might be able to build without casts ?


> > > +err_mutex_unlock:
> > > +���� mutex_unlock(&ov2735->page_lock);
> > > +���� return ret;
> > 
> > Hmm... Wouldn't be cleanup.h helpful here?
> >
> > > +}
> > 
> > ...
> > 
> > > +static int ov2735_write(struct ov2735 *ov2735, u32 reg, u64 val, int *err)
> > > +{
> > > +���� return ov2735_page_access(ov2735, reg, (void *)&val, err, false);
> > 
> > Why casting?
> > 
> > > +}
> > 
> > ...

<snip>

> > > +���� /* Apply format settings. */
> > 
> > > +���� /* Apply customized values from user */
> > 
> > Define a single style for one-line comments and use it everywhere consistently.
> 
> Are you referring to the period at the end of the comment?
>  
> > > +������������ goto error_power_off;
> > 
> > ...
> > 
> > > +���� devm_pm_runtime_set_active_enabled(ov2735->dev);
> > > +���� devm_pm_runtime_get_noresume(ov2735->dev);
> > 
> > No error checks? What's the point to use devm and what will happen if the first
> > fails, for example?
> > 
> > --
> > With Best Regards,
> > Andy Shevchenko
> 
> With all due respect,
> 
> I completely understand and appreciate the need for multiple rounds of review.
> However, where feasible, it would be helpful to receive style-related and 
> non-blocking comments earlier in the review process. Iterating on minor issues
> in later versions, especially ones that could have been addressed together 
> earlier, can become a bit frustrating at times. I hope you can understand this 
> perspective.

I certainly understand that public patch review can be slow and tedious
at times, but please remember that unless you have paid Andy to review
this work, there are no contractual 'requirements' on which bits get
reviewed first. All reviews are helpful, whereever they come in - and
the goal here is to get as many eyes on code as possible to support high
quality code being maintained in the linux kernel.


Perhaps one path to speed this up in the future might be if you might
find some time to read more kernel code and also review some other
kernel patches publicly as well. It might accelerate/help you learn the
linux coding style sooner to avoid some cycles, and provide more eyes
where we need them in the community.

Regards
--
Kieran


> Once again, thank you for your time and effort in helping improve the quality
> of the driver.
> 
> Best Regards,
> Hardev