Re: [PATCH v5 2/2] media: i2c: add ov2735 image sensor driver

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

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