Re: [PATCH v3] staging: greybus: merge split lines

[Alison Schofield]

On Mon, Mar 27, 2023 at 03:26:22PM +0500, Khadija Kamran wrote:
> On Tue, Mar 21, 2023 at 09:35:42AM -0700, Alison Schofield wrote:
> > On Tue, Mar 21, 2023 at 09:21:35PM +0500, Khadija Kamran wrote:
> > > On Mon, Mar 20, 2023 at 01:26:33PM +0500, Khadija Kamran wrote:
> > > > If condition and spin_unlock_...() call is split into two lines, merge
> > > > them to form a single line.
> > > > 
> > > > Suggested-by: Deepak R Varma drv@xxxxxxxxx
> > > > Signed-off-by: Khadija Kamran <kamrankhadijadj@xxxxxxxxx>
> > > > ---
> > > > 
> > > > Changes in v3:
> > > >  - Removing tab to fix line length results in a new checkpatch warning,
> > > >    so let the fix length be as it is.
> > > > Changes in v2:
> > > >  - Rephrased he subject and description
> > > >  - Merged if_condition() and spin_unlock...() into one line
> > > >  - Link to patch:
> > > >  https://lore.kernel.org/outreachy/ZAusnKYVTGvO5zoi@khadija-virtual-machine/
> > > > 
> > > > Link to first patch:
> > > > https://lore.kernel.org/outreachy/ZAtkW6g6DwPg%2FpDp@khadija-virtual-machine/
> > > > 
> > > >  drivers/staging/greybus/arche-platform.c | 6 ++----
> > > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> > > > index fcbd5f71eff2..6890710afdfc 100644
> > > > --- a/drivers/staging/greybus/arche-platform.c
> > > > +++ b/drivers/staging/greybus/arche-platform.c
> > > > @@ -176,12 +176,10 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> > > >  				 * Check we are not in middle of irq thread
> > > >  				 * already
> > > >  				 */
> > > > -				if (arche_pdata->wake_detect_state !=
> > > > -						WD_STATE_COLDBOOT_START) {
> > > > +				if (arche_pdata->wake_detect_state != WD_STATE_COLDBOOT_START) {
> > > >  					arche_platform_set_wake_detect_state(arche_pdata,
> > > >  									     WD_STATE_COLDBOOT_TRIG);
> > > > -					spin_unlock_irqrestore(&arche_pdata->wake_lock,
> > > > -							       flags);
> > > > +					spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> > > >  					return IRQ_WAKE_THREAD;
> > > >  				}
> > > >  			}
> > > > --
> > > > 2.34.1
> > > >
> > > 
> > > Hey Outreachy Mentors,
> > > 
> > > Kindly take a look at this patch and let me know if it is okay to work
> > > on this file or should I look for other cleanup patches.
> > 
> > Hi Khadija,
> > 
> > I thought you were abandoning *this* patch, and doing a refactor on
> > the function.  I'd expect that would be a new patch, probably a
> > patchset. One where you align the work based on the 'rising' and
> > 'falling' detection, 
> 
> Hey Alison,
> 
> Can you please elaborate that what do you mean by aligning on the basis
> of rising and falling detection. Are you perhaps saying that I should
> group the rising detection and group the falling detection separately?
> 
> > and perhaps a second patch that centralizes
> > the unlock and return.
> 
> To do this I should make the use of goto statement, right?
> 
> So the next patchset should be:
> Patch 1: merge split lines
> Patch 2: align on the basis of rising and falling detection
> Patch 3: use goto statement to centralize unlock and return
> 
> Kindly guide me.
> 
> Regards,
> Khadija

Hi,

Glad you are picking this back up!
I know Ira sent you some links to refactoring info. Go back and
look at those.

When we submit patches that refactor a function, we try to make
the patches obviously correct and easy to review.

I'll tell you how I approached this one, and you can see how
it works for you:

1. Edit the function until it is just how you'd like it. Hint:
   no lines over 80, minimal indentation.

{
	--snip--

	if (!gpiod_get_value(arche_pdata->wake_detect))
                goto falling;

        /* wake/detect rising */

	

falling:
	/* wake/detect falling */


out:
	spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);

        return rc;
}


2. Figure out how you can present that in patches. This function
   is just long enough that I think you have to split it up into
   two or more obvious steps, rather than throwing it into one
   patch.

How about you do Step 1, and send the diff to the Outreachy mailing
list (only) for review. Please start a new thread. 

Alison

> 
> > 
> > Is there some other concern with working on this file?
> > 
> > Alison
> > 
> > > 
> > > Thank you for your time.
> > > Regards,
> > > Khadija
> > > 
> > >