On Fri, Mar 10, 2023 at 10:09:47PM +0500, Khadija Kamran wrote:
Length of line 182 exceeds 100 columns in file drivers/staging/grebus/arche-platform.c, fix by removing tabs from the line.
Signed-off-by: Khadija Kamran kamrankhadijadj@gmail.com
drivers/staging/greybus/arche-platform.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c index fcbd5f71eff2..0f0fbc263f8a 100644 --- a/drivers/staging/greybus/arche-platform.c +++ b/drivers/staging/greybus/arche-platform.c @@ -179,7 +179,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);
WD_STATE_COLDBOOT_TRIG);
The original line was done deliberately so that it lines up. If we apply your patch and re-run checkpatch -f on the file then it has a new warning:
CHECK: Alignment should match open parenthesis #182: FILE: drivers/staging/greybus/arche-platform.c:182: + arche_platform_set_wake_detect_state(arche_pdata, + WD_STATE_COLDBOOT_TRIG);
Always try to think about the bigger picture. Why did the original author do it that way? The change makes checkpatch happy, but does it make the code more readable? Is there a more important readability improvement to be done here?
For example, you could re-arrange the if statements like this and pull everything in a few tabs. Don't necessarily do that. Just think about doing it. I write quite a few cleanup patches that I don't send because the next day I just decide it's not worth it.
When I look at this file, the style is not bad at all. But at the start of the file there is #if IS_ENABLED(CONFIG_USB_HSIC_USB3613). What is that? The CONFIG doesn't exist and the header doesn't exits. Probably it can be deleted.
But that raises a new question. Lukas Bulwahn is always looking for CONFIG_ entries which don't exist. I would have expected him to find this already.
Anyway, we can write our own scripts to make a list of stuff inside IS_ENABLED():
git grep IS_ENABLED | \ perl -ne 'if (/IS_ENABLED((.+?))/){ print "$1\n"}' | \ sort -u | tee CONFIG_list
Then we can go through the CONFIG_list file and see which other stuff doesn't exist.
for i in $(grep ^CONFIG CONFIG_list | cut -d '_' -f 2-) ; do \ grep -q -w "config $i$" $(find -name Kconfig) || echo $i ; \ done | tee CONFIG_not_found
I have never done this before so I don't know what you'll find. But everywhere you look if you just look closer then it raises questions which raise more questions. So it's interesting to explore. Anyway, look closely at each line in the file and follow the rabbit holes until you find something interesting to work on.
regards, dan carpenter
diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c index fcbd5f71eff2..2d9e0c41b5e3 100644 --- a/drivers/staging/greybus/arche-platform.c +++ b/drivers/staging/greybus/arche-platform.c @@ -165,43 +165,39 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) * 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 { - /* wake/detect falling */ - if (arche_pdata->wake_detect_state == WD_STATE_IDLE) { - arche_pdata->wake_detect_start = jiffies; + if (arche_pdata->wake_detect_state != WD_STATE_BOOT_INIT) + goto out_unlock; + + 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 if (arche_pdata->wake_detect_state != WD_STATE_COLDBOOT_START) { /* - * 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); + WD_STATE_COLDBOOT_TRIG); + spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); + return IRQ_WAKE_THREAD; } + } else if (arche_pdata->wake_detect_state == WD_STATE_IDLE) { + /* wake/detect falling */ + 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_unlock: spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
return IRQ_HANDLED;