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@mailo.com Signed-off-by: Khadija Kamran kamrankhadijadj@gmail.com
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
-
Alison Schofield