* Suggestions on refactoring arche_platform_wd_irq() function
@ 2023-03-28 10:45 Khadija Kamran
2023-03-28 10:54 ` Julia Lawall
2023-03-28 23:28 ` Alison Schofield
0 siblings, 2 replies; 5+ messages in thread
From: Khadija Kamran @ 2023-03-28 10:45 UTC (permalink / raw)
To: outreachy
Hey Outreachy Mentors,
I am working on a check reported by checkpatch.pl saying,
CHECK: line length of 101 exceeds 100 columns
#182: FILE: drivers/staging/greybus/arche-platform.c:182:
+ WD_STATE_COLDBOOT_TRIG);
To refactor the function Ira sent me a link and suggested the use of
goto statement.
Alison guided me with this problem and asked to share the diff and start
a thread for discussion. Link to Alison's mail:
https://lore.kernel.org/outreachy/ZCHPokeiKV37uOmr@aschofie-mobl2/
Kindly review the following diff:
static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
spin_lock_irqsave(&arche_pdata->wake_lock, flags);
- if (gpiod_get_value(arche_pdata->wake_detect)) {
- /* wake/detect rising */
-
- /*
- * If wake/detect line goes high after low, within less than
- * 30msec, then standby boot sequence is initiated, which is not
- * supported/implemented as of now. So ignore it.
- */
- if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
- if (time_before(jiffies,
- arche_pdata->wake_detect_start +
- msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
- arche_platform_set_wake_detect_state(arche_pdata,
- WD_STATE_IDLE);
- } else {
- /*
- * Check we are not in middle of irq thread
- * already
- */
- 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;
- }
- }
- }
- } else {
+ if (!gpiod_get_value(arche_pdata->wake_detect)) {
/* wake/detect falling */
- if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
- arche_pdata->wake_detect_start = jiffies;
+ goto falling;
+ }
+
+ /* wake/detect rising */
+
+ /*
+ * If wake/detect line goes high after low, within less than
+ * 30msec, then standby boot sequence is initiated, which is not
+ * supported/implemented as of now. So ignore it.
+ */
+ if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
+ if (time_before(jiffies,
+ arche_pdata->wake_detect_start +
+ msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
+ arche_platform_set_wake_detect_state(arche_pdata,
+ WD_STATE_IDLE);
+ } else {
/*
- * In the beginning, when wake/detect goes low
- * (first time), we assume it is meant for coldboot
- * and set the flag. If wake/detect line stays low
- * beyond 30msec, then it is coldboot else fallback
- * to standby boot.
+ * Check we are not in middle of irq thread
+ * already
*/
- arche_platform_set_wake_detect_state(arche_pdata,
- WD_STATE_BOOT_INIT);
+ 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;
}
+falling:
+ if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
+ arche_pdata->wake_detect_start = jiffies;
+ /*
+ * In the beginning, when wake/detect goes low
+ * (first time), we assume it is meant for coldboot
+ * and set the flag. If wake/detect line stays low
+ * beyond 30msec, then it is coldboot else fallback
+ * to standby boot.
+ */
+ arche_platform_set_wake_detect_state(arche_pdata,
+ WD_STATE_BOOT_INIT);
+
+out:
spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
return IRQ_HANDLED;
Thank you!
Regards,
Khadija
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Suggestions on refactoring arche_platform_wd_irq() function
2023-03-28 10:45 Suggestions on refactoring arche_platform_wd_irq() function Khadija Kamran
@ 2023-03-28 10:54 ` Julia Lawall
2023-03-28 18:58 ` Alison Schofield
2023-03-28 23:28 ` Alison Schofield
1 sibling, 1 reply; 5+ messages in thread
From: Julia Lawall @ 2023-03-28 10:54 UTC (permalink / raw)
To: Khadija Kamran; +Cc: outreachy
On Tue, 28 Mar 2023, Khadija Kamran wrote:
> Hey Outreachy Mentors,
> I am working on a check reported by checkpatch.pl saying,
>
> CHECK: line length of 101 exceeds 100 columns
> #182: FILE: drivers/staging/greybus/arche-platform.c:182:
> + WD_STATE_COLDBOOT_TRIG);
>
>
> To refactor the function Ira sent me a link and suggested the use of
> goto statement.
>
> Alison guided me with this problem and asked to share the diff and start
> a thread for discussion. Link to Alison's mail:
> https://lore.kernel.org/outreachy/ZCHPokeiKV37uOmr@aschofie-mobl2/
Wasn't there a suggestion to reduce the size of the function names, in the
case of static (file-local) functions? For
arche_platform_set_wake_detect_state, just the name of the function is
almost as many characters as the code in its definition...
I guess the main purpose of the function is to allow placing a comment
about a locking requirement.
julia
>
> Kindly review the following diff:
>
> static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>
> spin_lock_irqsave(&arche_pdata->wake_lock, flags);
>
> - if (gpiod_get_value(arche_pdata->wake_detect)) {
> - /* wake/detect rising */
> -
> - /*
> - * If wake/detect line goes high after low, within less than
> - * 30msec, then standby boot sequence is initiated, which is not
> - * supported/implemented as of now. So ignore it.
> - */
> - if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
> - if (time_before(jiffies,
> - arche_pdata->wake_detect_start +
> - msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
> - arche_platform_set_wake_detect_state(arche_pdata,
> - WD_STATE_IDLE);
> - } else {
> - /*
> - * Check we are not in middle of irq thread
> - * already
> - */
> - 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;
> - }
> - }
> - }
> - } else {
> + if (!gpiod_get_value(arche_pdata->wake_detect)) {
> /* wake/detect falling */
> - if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> - arche_pdata->wake_detect_start = jiffies;
> + goto falling;
> + }
> +
> + /* wake/detect rising */
> +
> + /*
> + * If wake/detect line goes high after low, within less than
> + * 30msec, then standby boot sequence is initiated, which is not
> + * supported/implemented as of now. So ignore it.
> + */
> + if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
> + if (time_before(jiffies,
> + arche_pdata->wake_detect_start +
> + msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
> + arche_platform_set_wake_detect_state(arche_pdata,
> + WD_STATE_IDLE);
> + } else {
> /*
> - * In the beginning, when wake/detect goes low
> - * (first time), we assume it is meant for coldboot
> - * and set the flag. If wake/detect line stays low
> - * beyond 30msec, then it is coldboot else fallback
> - * to standby boot.
> + * Check we are not in middle of irq thread
> + * already
> */
> - arche_platform_set_wake_detect_state(arche_pdata,
> - WD_STATE_BOOT_INIT);
> + 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;
> }
>
> +falling:
> + if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> + arche_pdata->wake_detect_start = jiffies;
> + /*
> + * In the beginning, when wake/detect goes low
> + * (first time), we assume it is meant for coldboot
> + * and set the flag. If wake/detect line stays low
> + * beyond 30msec, then it is coldboot else fallback
> + * to standby boot.
> + */
> + arche_platform_set_wake_detect_state(arche_pdata,
> + WD_STATE_BOOT_INIT);
> +
> +out:
> spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
>
> return IRQ_HANDLED;
>
> Thank you!
>
> Regards,
> Khadija
>
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Suggestions on refactoring arche_platform_wd_irq() function
2023-03-28 10:54 ` Julia Lawall
@ 2023-03-28 18:58 ` Alison Schofield
2023-03-28 19:30 ` Khadija Kamran
0 siblings, 1 reply; 5+ messages in thread
From: Alison Schofield @ 2023-03-28 18:58 UTC (permalink / raw)
To: Julia Lawall; +Cc: Khadija Kamran, outreachy
On Tue, Mar 28, 2023 at 12:54:26PM +0200, Julia Lawall wrote:
>
>
> On Tue, 28 Mar 2023, Khadija Kamran wrote:
>
> > Hey Outreachy Mentors,
> > I am working on a check reported by checkpatch.pl saying,
> >
> > CHECK: line length of 101 exceeds 100 columns
> > #182: FILE: drivers/staging/greybus/arche-platform.c:182:
> > + WD_STATE_COLDBOOT_TRIG);
> >
> >
> > To refactor the function Ira sent me a link and suggested the use of
> > goto statement.
> >
> > Alison guided me with this problem and asked to share the diff and start
> > a thread for discussion. Link to Alison's mail:
> > https://lore.kernel.org/outreachy/ZCHPokeiKV37uOmr@aschofie-mobl2/
>
> Wasn't there a suggestion to reduce the size of the function names, in the
> case of static (file-local) functions? For
> arche_platform_set_wake_detect_state, just the name of the function is
> almost as many characters as the code in its definition...
Hi Julia,
Yes, Alex Elder commented about looking at the function names.
I look at it as 2 different problems:
1) This file, as a whole, and probably others in greybus, could probably
benefit from a naming rework. This file alone, that likes to preface
everything with 'arche_platform_' seems ripe for cleanup.
2) Refactoring this function in particular.
If we renamed things, sure this function would not run over 80 chars,
and would not have caught our attention so quickly. But, even with
a naming update, the indentation levels are ugly, and needless.
For Khadaji, I think the more meaningful exercise is to do the
code refactoring, just because it gives her a chance to try something
different, than the cleanups she's completed thus far.
I'll throw out a TODO item here, maybe Khadaji can pick this up rather
than picking up the actual work at the moment:
Add an item to the drivers/staging/greybus/TODO file, something like:
Consider a naming overhaul in arche-platform.c to alleviate some of
the line length issues due to prefacing most functions and data
structure with the complete 'arche_platform'
By submitting a patch with that TODO, we can find out if maintainers
are amenable to a naming overhaul. And we get the work recorded in
the TODO list.
Thanks,
Alison
>
> I guess the main purpose of the function is to allow placing a comment
> about a locking requirement.
>
> julia
>
> >
> > Kindly review the following diff:
> >
> > static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> >
> > spin_lock_irqsave(&arche_pdata->wake_lock, flags);
> >
> > - if (gpiod_get_value(arche_pdata->wake_detect)) {
> > - /* wake/detect rising */
> > -
> > - /*
> > - * If wake/detect line goes high after low, within less than
> > - * 30msec, then standby boot sequence is initiated, which is not
> > - * supported/implemented as of now. So ignore it.
> > - */
> > - if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
> > - if (time_before(jiffies,
> > - arche_pdata->wake_detect_start +
> > - msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
> > - arche_platform_set_wake_detect_state(arche_pdata,
> > - WD_STATE_IDLE);
> > - } else {
> > - /*
> > - * Check we are not in middle of irq thread
> > - * already
> > - */
> > - 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;
> > - }
> > - }
> > - }
> > - } else {
> > + if (!gpiod_get_value(arche_pdata->wake_detect)) {
> > /* wake/detect falling */
> > - if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> > - arche_pdata->wake_detect_start = jiffies;
> > + goto falling;
> > + }
> > +
> > + /* wake/detect rising */
> > +
> > + /*
> > + * If wake/detect line goes high after low, within less than
> > + * 30msec, then standby boot sequence is initiated, which is not
> > + * supported/implemented as of now. So ignore it.
> > + */
> > + if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
> > + if (time_before(jiffies,
> > + arche_pdata->wake_detect_start +
> > + msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
> > + arche_platform_set_wake_detect_state(arche_pdata,
> > + WD_STATE_IDLE);
> > + } else {
> > /*
> > - * In the beginning, when wake/detect goes low
> > - * (first time), we assume it is meant for coldboot
> > - * and set the flag. If wake/detect line stays low
> > - * beyond 30msec, then it is coldboot else fallback
> > - * to standby boot.
> > + * Check we are not in middle of irq thread
> > + * already
> > */
> > - arche_platform_set_wake_detect_state(arche_pdata,
> > - WD_STATE_BOOT_INIT);
> > + 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;
> > }
> >
> > +falling:
> > + if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> > + arche_pdata->wake_detect_start = jiffies;
> > + /*
> > + * In the beginning, when wake/detect goes low
> > + * (first time), we assume it is meant for coldboot
> > + * and set the flag. If wake/detect line stays low
> > + * beyond 30msec, then it is coldboot else fallback
> > + * to standby boot.
> > + */
> > + arche_platform_set_wake_detect_state(arche_pdata,
> > + WD_STATE_BOOT_INIT);
> > +
> > +out:
> > spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> >
> > return IRQ_HANDLED;
> >
> > Thank you!
> >
> > Regards,
> > Khadija
> >
> >
> >
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Suggestions on refactoring arche_platform_wd_irq() function
2023-03-28 18:58 ` Alison Schofield
@ 2023-03-28 19:30 ` Khadija Kamran
0 siblings, 0 replies; 5+ messages in thread
From: Khadija Kamran @ 2023-03-28 19:30 UTC (permalink / raw)
To: Alison Schofield; +Cc: Julia Lawall, outreachy
On Tue, Mar 28, 2023 at 11:58:14AM -0700, Alison Schofield wrote:
> On Tue, Mar 28, 2023 at 12:54:26PM +0200, Julia Lawall wrote:
> >
> >
> > On Tue, 28 Mar 2023, Khadija Kamran wrote:
> >
> > > Hey Outreachy Mentors,
> > > I am working on a check reported by checkpatch.pl saying,
> > >
> > > CHECK: line length of 101 exceeds 100 columns
> > > #182: FILE: drivers/staging/greybus/arche-platform.c:182:
> > > + WD_STATE_COLDBOOT_TRIG);
> > >
> > >
> > > To refactor the function Ira sent me a link and suggested the use of
> > > goto statement.
> > >
> > > Alison guided me with this problem and asked to share the diff and start
> > > a thread for discussion. Link to Alison's mail:
> > > https://lore.kernel.org/outreachy/ZCHPokeiKV37uOmr@aschofie-mobl2/
> >
> > Wasn't there a suggestion to reduce the size of the function names, in the
> > case of static (file-local) functions? For
> > arche_platform_set_wake_detect_state, just the name of the function is
> > almost as many characters as the code in its definition...
>
> Hi Julia,
>
> Yes, Alex Elder commented about looking at the function names.
> I look at it as 2 different problems:
>
> 1) This file, as a whole, and probably others in greybus, could probably
> benefit from a naming rework. This file alone, that likes to preface
> everything with 'arche_platform_' seems ripe for cleanup.
>
> 2) Refactoring this function in particular.
> If we renamed things, sure this function would not run over 80 chars,
> and would not have caught our attention so quickly. But, even with
> a naming update, the indentation levels are ugly, and needless.
>
> For Khadaji, I think the more meaningful exercise is to do the
> code refactoring, just because it gives her a chance to try something
> different, than the cleanups she's completed thus far.
>
Hey Alison,
Okay, great! Is it okay to submit a patch bsaed on the diff I sent on
your suggestion?
Link to patch with diff:
https://lore.kernel.org/outreachy/ZCLFLqNjNawO2KnO@khadija-virtual-machine/
> I'll throw out a TODO item here, maybe Khadaji can pick this up rather
> than picking up the actual work at the moment:
>
> Add an item to the drivers/staging/greybus/TODO file, something like:
>
> Consider a naming overhaul in arche-platform.c to alleviate some of
> the line length issues due to prefacing most functions and data
> structure with the complete 'arche_platform'
>
> By submitting a patch with that TODO, we can find out if maintainers
> are amenable to a naming overhaul. And we get the work recorded in
> the TODO list.
>
Okay, noted. I will submit a TODO file with the patch.
Also, just to avoid any confusion, my name is spelled 'Khadija', not
'Khadaji'. :)
Regards,
Khadija
> Thanks,
> Alison
>
>
> >
> > I guess the main purpose of the function is to allow placing a comment
> > about a locking requirement.
> >
> > julia
> >
> > >
> > > Kindly review the following diff:
> > >
> > > static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> > >
> > > spin_lock_irqsave(&arche_pdata->wake_lock, flags);
> > >
> > > - if (gpiod_get_value(arche_pdata->wake_detect)) {
> > > - /* wake/detect rising */
> > > -
> > > - /*
> > > - * If wake/detect line goes high after low, within less than
> > > - * 30msec, then standby boot sequence is initiated, which is not
> > > - * supported/implemented as of now. So ignore it.
> > > - */
> > > - if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
> > > - if (time_before(jiffies,
> > > - arche_pdata->wake_detect_start +
> > > - msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
> > > - arche_platform_set_wake_detect_state(arche_pdata,
> > > - WD_STATE_IDLE);
> > > - } else {
> > > - /*
> > > - * Check we are not in middle of irq thread
> > > - * already
> > > - */
> > > - 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;
> > > - }
> > > - }
> > > - }
> > > - } else {
> > > + if (!gpiod_get_value(arche_pdata->wake_detect)) {
> > > /* wake/detect falling */
> > > - if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> > > - arche_pdata->wake_detect_start = jiffies;
> > > + goto falling;
> > > + }
> > > +
> > > + /* wake/detect rising */
> > > +
> > > + /*
> > > + * If wake/detect line goes high after low, within less than
> > > + * 30msec, then standby boot sequence is initiated, which is not
> > > + * supported/implemented as of now. So ignore it.
> > > + */
> > > + if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
> > > + if (time_before(jiffies,
> > > + arche_pdata->wake_detect_start +
> > > + msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
> > > + arche_platform_set_wake_detect_state(arche_pdata,
> > > + WD_STATE_IDLE);
> > > + } else {
> > > /*
> > > - * In the beginning, when wake/detect goes low
> > > - * (first time), we assume it is meant for coldboot
> > > - * and set the flag. If wake/detect line stays low
> > > - * beyond 30msec, then it is coldboot else fallback
> > > - * to standby boot.
> > > + * Check we are not in middle of irq thread
> > > + * already
> > > */
> > > - arche_platform_set_wake_detect_state(arche_pdata,
> > > - WD_STATE_BOOT_INIT);
> > > + 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;
> > > }
> > >
> > > +falling:
> > > + if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> > > + arche_pdata->wake_detect_start = jiffies;
> > > + /*
> > > + * In the beginning, when wake/detect goes low
> > > + * (first time), we assume it is meant for coldboot
> > > + * and set the flag. If wake/detect line stays low
> > > + * beyond 30msec, then it is coldboot else fallback
> > > + * to standby boot.
> > > + */
> > > + arche_platform_set_wake_detect_state(arche_pdata,
> > > + WD_STATE_BOOT_INIT);
> > > +
> > > +out:
> > > spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
> > >
> > > return IRQ_HANDLED;
> > >
> > > Thank you!
> > >
> > > Regards,
> > > Khadija
> > >
> > >
> > >
> > >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Suggestions on refactoring arche_platform_wd_irq() function
2023-03-28 10:45 Suggestions on refactoring arche_platform_wd_irq() function Khadija Kamran
2023-03-28 10:54 ` Julia Lawall
@ 2023-03-28 23:28 ` Alison Schofield
1 sibling, 0 replies; 5+ messages in thread
From: Alison Schofield @ 2023-03-28 23:28 UTC (permalink / raw)
To: Khadija Kamran; +Cc: outreachy
On Tue, Mar 28, 2023 at 03:45:02PM +0500, Khadija Kamran wrote:
> Hey Outreachy Mentors,
> I am working on a check reported by checkpatch.pl saying,
>
> CHECK: line length of 101 exceeds 100 columns
> #182: FILE: drivers/staging/greybus/arche-platform.c:182:
> + WD_STATE_COLDBOOT_TRIG);
>
>
> To refactor the function Ira sent me a link and suggested the use of
> goto statement.
>
> Alison guided me with this problem and asked to share the diff and start
> a thread for discussion. Link to Alison's mail:
> https://lore.kernel.org/outreachy/ZCHPokeiKV37uOmr@aschofie-mobl2/
>
> Kindly review the following diff:
Khadaji,
When you share a diff via email, it's useful to use 'git diff' and
send that. $ git diff arche-platform.c > mychanges.diff
This diff does tuck the code in. I think it can be tucked in further
by addng that single shared exit. That allows re-ordering of the work
in the 'rising' case.
More comments inline below...
We can work that more in the chat.
Alison
>
> static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>
irqreturn_t rc = IRQ_HANDLED;
Above will help you make a single exit path. Default to IRQ_HANDLED,
and only change where needed.
> spin_lock_irqsave(&arche_pdata->wake_lock, flags);
>
> - if (gpiod_get_value(arche_pdata->wake_detect)) {
> - /* wake/detect rising */
> -
> - /*
> - * If wake/detect line goes high after low, within less than
> - * 30msec, then standby boot sequence is initiated, which is not
> - * supported/implemented as of now. So ignore it.
> - */
> - if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
> - if (time_before(jiffies,
> - arche_pdata->wake_detect_start +
> - msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
> - arche_platform_set_wake_detect_state(arche_pdata,
> - WD_STATE_IDLE);
> - }
else {
> - /*
> - * Check we are not in middle of irq thread
> - * already
> - */
> - 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;
> - }
> - }
> - }
> - } else {
> + if (!gpiod_get_value(arche_pdata->wake_detect)) {
> /* wake/detect falling */
> - if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> - arche_pdata->wake_detect_start = jiffies;
> + goto falling;
> + }
> +
> + /* wake/detect rising */
> +
> + /*
> + * If wake/detect line goes high after low, within less than
> + * 30msec, then standby boot sequence is initiated, which is not
> + * supported/implemented as of now. So ignore it.
> + */
> + if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
Notice that when (arche_pdata->wake_detect_state != WD_STATE_BOOT_INIT),
work is done! So, perhaps flip the check, and make a quick escape here.
That allows you to un-indent this next piece:
> + if (time_before(jiffies,
> + arche_pdata->wake_detect_start +
> + msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
> + arche_platform_set_wake_detect_state(arche_pdata,
> + WD_STATE_IDLE);
Another chance for a jump to out. There is no more work after this.
By jumping to out, rather than if-then-else'ing you are making the
code stream-lined, hence readable.
> + } else {
> /*
> - * In the beginning, when wake/detect goes low
> - * (first time), we assume it is meant for coldboot
> - * and set the flag. If wake/detect line stays low
> - * beyond 30msec, then it is coldboot else fallback
> - * to standby boot.
> + * Check we are not in middle of irq thread
> + * already
> */
> - arche_platform_set_wake_detect_state(arche_pdata,
> - WD_STATE_BOOT_INIT);
> + if (arche_pdata->wake_detect_state !=
> + WD_STATE_COLDBOOT_START) {
> + arche_platform_set_wake_detect_state(arche_pdata,
> + WD_STATE_COLDBOOT_TRIG)
use the single exit path, rather than unlock and exit here.
> + spin_unlock_irqrestore(&arche_pdata->wake_lock,
> + flags);
> + return IRQ_WAKE_THREAD;
rc = IRQ_WAKE_THREAD;
goto out;
> + }
> }
> + goto out;
> }
>
> +falling:
> + if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> + arche_pdata->wake_detect_start = jiffies;
> + /*
> + * In the beginning, when wake/detect goes low
> + * (first time), we assume it is meant for coldboot
> + * and set the flag. If wake/detect line stays low
> + * beyond 30msec, then it is coldboot else fallback
> + * to standby boot.
> + */
> + arche_platform_set_wake_detect_state(arche_pdata,
> + WD_STATE_BOOT_INIT);
> +
> +out:
> spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
>
> return IRQ_HANDLED;
now you'll have an 'rc' to return, which is default IRQ_HANDLED.
>
> Thank you!
>
> Regards,
> Khadija
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-28 23:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-28 10:45 Suggestions on refactoring arche_platform_wd_irq() function Khadija Kamran
2023-03-28 10:54 ` Julia Lawall
2023-03-28 18:58 ` Alison Schofield
2023-03-28 19:30 ` Khadija Kamran
2023-03-28 23:28 ` Alison Schofield
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).