Re: [PATCH 02/17] media: atomisp: pci: fix punit_ddr_dvfs_enable() argument for mrfld_power up case

[Dan Carpenter <dan.carpenter@oracle.com>]

On Mon, Nov 08, 2021 at 11:48:42PM +0900, Tsuchiya Yuto wrote:
> > > diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > > index 0511c454e769..f5362554638e 100644
> > > --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > > +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > > @@ -711,15 +711,15 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
> > >  
> > >  	dev_dbg(isp->dev, "IUNIT power-%s.\n", enable ? "on" : "off");
> > >  
> > > -	/*WA:Enable DVFS*/
> > > +	/* WA for PUNIT, if DVFS enabled, ISP timeout observed */
> > >  	if (IS_CHT && enable)
> > > -		punit_ddr_dvfs_enable(true);
> > > +		punit_ddr_dvfs_enable(false);
> > >  
> > >  	/*
> > >  	 * FIXME:WA for ECS28A, with this sleep, CTS
> > >  	 * android.hardware.camera2.cts.CameraDeviceTest#testCameraDeviceAbort
> > >  	 * PASS, no impact on other platforms
> > > -	*/
> > > +	 */
> >         ^^^
> > 
> > >  	if (IS_BYT && enable)
> > >  		msleep(10);
> > >  
> > > @@ -727,7 +727,7 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
> > >  	iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, MRFLD_ISPSSPM0,
> > >  			val, MRFLD_ISPSSPM0_ISPSSC_MASK);
> > >  
> > > -	/*WA:Enable DVFS*/
> > > +	/* WA:Enable DVFS */
> >         ^^^^^^^^^^^^^^^^^^^
> > These two white space changes are unrelated.  Please do them in a
> > separate patch.
> 
> Thank you for the review :-)
> 
> I thought these space fixes were too trivial for a separate patch, so
> I made the fix while at it. I have no strong reason not to separate the
> space fix. I'll do so in v2.

Trivial white space patches are fine.  We get thousands of those.

This goes through Mauro's tree instead of Greg's and he is definitely
more flexible than Greg.  Plus the atomisp stuff is broken so no one
cares.

But the rules are really clear and it does help in reviewing when people
follow them.

When I'm reviewing this patch, the patch description says it is a fix so
I review that differently.  The fix is very simple and it changes true
to false.

The other patch would have been a "change comments" patch.  We get
thousands of those as I mentioned.  My concern there is that a UMN
researcher will try to trick us by hiding code in a "change comments"
patch so I have a script to review those.  It takes one second to run.

But then when I was reviewing this patch I first had to spot the change
from true to false before I could review it.  That's where most of the
time was wasted.

regards,
dan carpenter