* Re: [PATCH] staging: greybus: refactor arche_platform_wd_irq() function
2023-03-27 20:18 [PATCH] staging: greybus: refactor arche_platform_wd_irq() function Khadija Kamran
@ 2023-03-27 20:34 ` Alex Elder
2023-03-27 20:49 ` Alison Schofield
2023-03-28 7:33 ` Dan Carpenter
2 siblings, 0 replies; 6+ messages in thread
From: Alex Elder @ 2023-03-27 20:34 UTC (permalink / raw)
To: Khadija Kamran, outreachy
Cc: Vaibhav Hiremath, Johan Hovold, Alex Elder, Greg Kroah-Hartman,
greybus-dev, linux-staging, linux-kernel
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@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;
> }
>
> /*
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging: greybus: refactor arche_platform_wd_irq() function
2023-03-27 20:18 [PATCH] staging: greybus: refactor arche_platform_wd_irq() function Khadija Kamran
2023-03-27 20:34 ` Alex Elder
@ 2023-03-27 20:49 ` Alison Schofield
2023-03-28 7:33 ` Dan Carpenter
2 siblings, 0 replies; 6+ messages in thread
From: Alison Schofield @ 2023-03-27 20:49 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 28, 2023 at 01:18:53AM +0500, Khadija Kamran wrote:
> Refactor function by adding goto statement. This reduces the
> indentation and fixes the issue reported by checkpatch.pl script.
It seems our emails may have crossed on this issue.
See: https://lore.kernel.org/outreachy/ZBrFqtyY%2FHydnBdT@kroah.com/T/#m5ac406a1ebef0ee5c013b6db75ce1368e292c179
Alison
>
> "CHECK: line length of 101 exceeds 100 columns"
>
> Signed-off-by: Khadija Kamran <kamrankhadijadj@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;
> }
>
> /*
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging: greybus: refactor arche_platform_wd_irq() function
2023-03-27 20:18 [PATCH] staging: greybus: refactor arche_platform_wd_irq() function Khadija Kamran
2023-03-27 20:34 ` Alex Elder
2023-03-27 20:49 ` Alison Schofield
@ 2023-03-28 7:33 ` Dan Carpenter
2023-03-28 23:04 ` Alison Schofield
2 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2023-03-28 7:33 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 28, 2023 at 01:18:53AM +0500, 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"
>
> Signed-off-by: Khadija Kamran <kamrankhadijadj@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;
I don't like this goto.
It suggests that calling arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG);
is a part of a shared exit path when that is not true so it is sort of
lying to the reader and making things harder to understand.
Unlocking is shared. goto unlock is fine. But that doesn't help with
the very long lines. (I am not saying that goto unlock is a worthwhile
patch to send but I would definitely have found it tolerable).
regards,
dan carpenter
> }
> }
> }
> @@ -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;
> }
>
> /*
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging: greybus: refactor arche_platform_wd_irq() function
2023-03-28 7:33 ` Dan Carpenter
@ 2023-03-28 23:04 ` Alison Schofield
2023-03-30 15:05 ` Khadija Kamran
0 siblings, 1 reply; 6+ messages in thread
From: Alison Schofield @ 2023-03-28 23:04 UTC (permalink / raw)
To: Khadija Kamran, Dan Carpenter
Cc: outreachy, Vaibhav Hiremath, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, greybus-dev, linux-staging, linux-kernel
On Tue, Mar 28, 2023 at 10:33:23AM +0300, Dan Carpenter wrote:
> On Tue, Mar 28, 2023 at 01:18:53AM +0500, 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"
> >
> > Signed-off-by: Khadija Kamran <kamrankhadijadj@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;
>
> I don't like this goto.
>
> It suggests that calling arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG);
> is a part of a shared exit path when that is not true so it is sort of
> lying to the reader and making things harder to understand.
>
> Unlocking is shared. goto unlock is fine. But that doesn't help with
> the very long lines. (I am not saying that goto unlock is a worthwhile
> patch to send but I would definitely have found it tolerable).
>
> regards,
> dan carpenter
Khadija,
While you are reworking this, just wanted to note that, once 'out:'
becomes the single exit path for the function, it won't be a lie
any more. So you will have addressed Dan's concern.
Alison
>
> > }
> > }
> > }
> > @@ -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;
> > }
> >
> > /*
> > --
> > 2.34.1
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging: greybus: refactor arche_platform_wd_irq() function
2023-03-28 23:04 ` Alison Schofield
@ 2023-03-30 15:05 ` Khadija Kamran
0 siblings, 0 replies; 6+ messages in thread
From: Khadija Kamran @ 2023-03-30 15:05 UTC (permalink / raw)
To: Alison Schofield
Cc: Dan Carpenter, outreachy, Vaibhav Hiremath, Johan Hovold,
Alex Elder, Greg Kroah-Hartman, greybus-dev, linux-staging,
linux-kernel
On Tue, Mar 28, 2023 at 04:04:33PM -0700, Alison Schofield wrote:
> On Tue, Mar 28, 2023 at 10:33:23AM +0300, Dan Carpenter wrote:
> > On Tue, Mar 28, 2023 at 01:18:53AM +0500, 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"
> > >
> > > Signed-off-by: Khadija Kamran <kamrankhadijadj@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;
> >
> > I don't like this goto.
> >
> > It suggests that calling arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG);
> > is a part of a shared exit path when that is not true so it is sort of
> > lying to the reader and making things harder to understand.
> >
> > Unlocking is shared. goto unlock is fine. But that doesn't help with
> > the very long lines. (I am not saying that goto unlock is a worthwhile
> > patch to send but I would definitely have found it tolerable).
> >
> > regards,
> > dan carpenter
>
>
> Khadija,
> While you are reworking this, just wanted to note that, once 'out:'
> becomes the single exit path for the function, it won't be a lie
> any more. So you will have addressed Dan's concern.
> Alison
>
Hey Alison,
Yes, I understand. Thank you. :)
Regards,
Khadija
>
> >
> > > }
> > > }
> > > }
> > > @@ -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;
> > > }
> > >
> > > /*
> > > --
> > > 2.34.1
> > >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread