From: Khadija Kamran <kamrankhadijadj@gmail.com>
To: Alison Schofield <alison.schofield@intel.com>
Cc: Julia Lawall <julia.lawall@inria.fr>, outreachy@lists.linux.dev
Subject: Re: Suggestions on refactoring arche_platform_wd_irq() function
Date: Wed, 29 Mar 2023 00:30:56 +0500 [thread overview]
Message-ID: <ZCNAcPBFFpLMYd4D@khadija-virtual-machine> (raw)
In-Reply-To: <ZCM4xlkzDK+lVjX0@aschofie-mobl2>
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
> > >
> > >
> > >
> > >
> >
next prev parent reply other threads:[~2023-03-28 19:31 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2023-03-28 23:28 ` Alison Schofield
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZCNAcPBFFpLMYd4D@khadija-virtual-machine \
--to=kamrankhadijadj@gmail.com \
--cc=alison.schofield@intel.com \
--cc=julia.lawall@inria.fr \
--cc=outreachy@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).