Re: Suggestions on refactoring arche_platform_wd_irq() function

[Khadija Kamran <kamrankhadijadj@gmail.com>]

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
> > >
> > >
> > >
> > >
> >