On 3/27/23 3:18 PM, Khadija Kamran wrote:
> Refactor function by adding goto statement. This reduces the
> indentation and fixes the issue reported by checkpatch.pl script.
>
> "CHECK: line length of 101 exceeds 100 columns"
Looking at this entire function, it seems a great deal of it
has somewhat wide lines. Part of the problem is that it
relies on arche_platform_set_wake_detect_state(), which is
36 characters long all by itself.
In any case, the line that is identified is the widest, of
course, by 10 or more characters. But changing that one
line doesn't substantially improve things.
I'm reluctant to suggest this, because I don't want a lot
of "code churn" patches to follow based on this, but...
One could rename arche_platform_set_wake_detect_state()
to be just set_wake_detect_state(). It's private to
its source file (arche-platform.c) and therefore the
"arche_plaform_" prefix isn't really necessary. And
perhaps the result would be code that is a little more
readable, because its lines aren't so long.
I'd be happy to hear others' thoughts on this.
-Alex
>
> Signed-off-by: Khadija Kamran <kamrankhadijadj(a)gmail.com>
> ---
> drivers/staging/greybus/arche-platform.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index fcbd5f71eff2..c7d3b6f7368f 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -178,11 +178,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> */
> 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);
> - return IRQ_WAKE_THREAD;
> + goto out;
> }
> }
> }
> @@ -205,6 +201,11 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
>
> return IRQ_HANDLED;
> +
> +out:
> + arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG);
> + spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> + return IRQ_WAKE_THREAD;
> }
>
> /*
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(a)mailo.com
> > > > Signed-off-by: Khadija Kamran <kamrankhadijadj(a)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-machin…
> > > >
> > > > 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
> > >
> > >
Convert macros to a static inline function, to make the relevant
types apparent in the definition and to benefit from the type
checking performed by the compiler at call sites.
Changes in v2: Change patch subjects, noted by Alison Schofield
<alison.schofield(a)intel.com>
Changes in v3: Change patch subjects to length 80 chars,
noted by Alison Schofield <alison.schofield(a)intel.com>
Sumitra Sharma (3):
staging: greybus: Inline gpio_chip_to_gb_gpio_controller()
staging: greybus: Inline gb_audio_manager_module()
staging: greybus: Inline pwm_chip_to_gb_pwm_chip()
drivers/staging/greybus/audio_manager_module.c | 7 +++++--
drivers/staging/greybus/gpio.c | 7 +++++--
drivers/staging/greybus/pwm.c | 6 ++++--
3 files changed, 14 insertions(+), 6 deletions(-)
--
2.25.1
Convert macros to a static inline function, to make the relevant
types apparent in the definition and to benefit from the type
checking performed by the compiler at call sites.
CHanges in v2: Change patch subjects, noted by Alison Schofield
<alison.schofield(a)intel.com>
Sumitra Sharma (3):
Staging: greybus: Use inline function for macro
gpio_chip_to_gb_gpio_controller
Staging: greybus: Use inline function for gb_audio_manager_module
Staging: greybus: Use inline function for pwm_chip_to_gb_pwm_chip
drivers/staging/greybus/audio_manager_module.c | 7 +++++--
drivers/staging/greybus/gpio.c | 7 +++++--
drivers/staging/greybus/pwm.c | 6 ++++--
3 files changed, 14 insertions(+), 6 deletions(-)
--
2.25.1
Convert macros to a static inline function, to make the relevant
types apparent in the definition and to benefit from the type
checking performed by the compiler at call sites.
Sumitra Sharma (3):
Staging: greybus: Convert macro gpio_chip_to_gb_gpio_controller to an
inline function
Staging: greybus: Convert macro struct gb_audio_manager_module to an
inline function
Staging: greybus: Convert macro struct pwm_chip_to_gb_pwm_chip to an
inline function
drivers/staging/greybus/audio_manager_module.c | 7 +++++--
drivers/staging/greybus/gpio.c | 7 +++++--
drivers/staging/greybus/pwm.c | 6 ++++--
3 files changed, 14 insertions(+), 6 deletions(-)
--
2.25.1
If condition and spin_unlock_...() call is split into two lines, merge
them to form a single line.
Suggested-by: Deepak R Varma drv(a)mailo.com
Signed-off-by: Khadija Kamran <kamrankhadijadj(a)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-machin…
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
