* [PATCH v3] staging: greybus: merge split lines @ 2023-03-20 8:26 Khadija Kamran 2023-03-21 16:21 ` Khadija Kamran 2023-03-22 9:08 ` Greg Kroah-Hartman 0 siblings, 2 replies; 7+ messages in thread From: Khadija Kamran @ 2023-03-20 8:26 UTC (permalink / raw) To: outreachy Cc: Vaibhav Hiremath, Johan Hovold, Alex Elder, Greg Kroah-Hartman, greybus-dev, linux-staging, linux-kernel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] staging: greybus: merge split lines 2023-03-20 8:26 [PATCH v3] staging: greybus: merge split lines Khadija Kamran @ 2023-03-21 16:21 ` Khadija Kamran 2023-03-21 16:35 ` Alison Schofield 2023-03-22 9:08 ` Greg Kroah-Hartman 1 sibling, 1 reply; 7+ messages in thread From: Khadija Kamran @ 2023-03-21 16:21 UTC (permalink / raw) To: outreachy Cc: Vaibhav Hiremath, Johan Hovold, Alex Elder, Greg Kroah-Hartman, greybus-dev, linux-staging, linux-kernel 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. Thank you for your time. Regards, Khadija ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] staging: greybus: merge split lines 2023-03-21 16:21 ` Khadija Kamran @ 2023-03-21 16:35 ` Alison Schofield 2023-03-27 10:26 ` Khadija Kamran 0 siblings, 1 reply; 7+ messages in thread From: Alison Schofield @ 2023-03-21 16:35 UTC (permalink / raw) To: Khadija Kamran Cc: outreachy, Vaibhav Hiremath, Johan Hovold, Alex Elder, Greg Kroah-Hartman, greybus-dev, linux-staging, linux-kernel 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, and perhaps a second patch that centralizes the unlock and return. Is there some other concern with working on this file? Alison > > Thank you for your time. > Regards, > Khadija > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] staging: greybus: merge split lines 2023-03-21 16:35 ` Alison Schofield @ 2023-03-27 10:26 ` Khadija Kamran 2023-03-27 17:17 ` Alison Schofield 0 siblings, 1 reply; 7+ messages in thread From: Khadija Kamran @ 2023-03-27 10:26 UTC (permalink / raw) To: Alison Schofield Cc: outreachy, Vaibhav Hiremath, Johan Hovold, Alex Elder, Greg Kroah-Hartman, greybus-dev, linux-staging, linux-kernel 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 > > Is there some other concern with working on this file? > > Alison > > > > > Thank you for your time. > > Regards, > > Khadija > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] staging: greybus: merge split lines 2023-03-27 10:26 ` Khadija Kamran @ 2023-03-27 17:17 ` Alison Schofield 2023-03-27 21:00 ` Khadija Kamran 0 siblings, 1 reply; 7+ messages in thread From: Alison Schofield @ 2023-03-27 17:17 UTC (permalink / raw) To: Khadija Kamran Cc: outreachy, Vaibhav Hiremath, Johan Hovold, Alex Elder, Greg Kroah-Hartman, greybus-dev, linux-staging, linux-kernel 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 > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] staging: greybus: merge split lines 2023-03-27 17:17 ` Alison Schofield @ 2023-03-27 21:00 ` Khadija Kamran 0 siblings, 0 replies; 7+ messages in thread From: Khadija Kamran @ 2023-03-27 21:00 UTC (permalink / raw) To: Alison Schofield Cc: outreachy, Vaibhav Hiremath, Johan Hovold, Alex Elder, Greg Kroah-Hartman, greybus-dev, linux-staging, linux-kernel On Mon, Mar 27, 2023 at 10:17:22AM -0700, Alison Schofield wrote: > 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. > Hey Alison, I am sorry about sending a new patch instead of sending a diff for discussion. I realize that I did not read your message carefully and misunderstood its contents. Let me start a new thread. Sorry for the inconvenience. Regards, Khadija > Alison > > > > > > > > > Is there some other concern with working on this file? > > > > > > Alison > > > > > > > > > > > Thank you for your time. > > > > Regards, > > > > Khadija > > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] staging: greybus: merge split lines 2023-03-20 8:26 [PATCH v3] staging: greybus: merge split lines Khadija Kamran 2023-03-21 16:21 ` Khadija Kamran @ 2023-03-22 9:08 ` Greg Kroah-Hartman 1 sibling, 0 replies; 7+ messages in thread From: Greg Kroah-Hartman @ 2023-03-22 9:08 UTC (permalink / raw) To: Khadija Kamran Cc: outreachy, Vaibhav Hiremath, Johan Hovold, Alex Elder, greybus-dev, linux-staging, linux-kernel On Mon, Mar 20, 2023 at 01:26:26PM +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 You need to properly quote email addresses for our tools to handle them, put a <> around them like you did here: > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com> See? Please fix up and resend. thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-03-27 21:00 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-20 8:26 [PATCH v3] staging: greybus: merge split lines Khadija Kamran 2023-03-21 16:21 ` Khadija Kamran 2023-03-21 16:35 ` Alison Schofield 2023-03-27 10:26 ` Khadija Kamran 2023-03-27 17:17 ` Alison Schofield 2023-03-27 21:00 ` Khadija Kamran 2023-03-22 9:08 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).