On Fri, Mar 10, 2023 at 8:40 PM Dan Carpenter error27@gmail.com wrote:

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.

True, and my (private) random linux notes of the checkkconfigsymbols effort on this config states:

USB_HSIC_USB3613: Reported and maintenance of dead code is fine for staging maintainer

So, I did report it, and a quick search on lore.kernel.org (https://lore.kernel.org/all/?q=USB_HSIC_USB3613) will give us some more insight and refresh Greg's and my memory:

I reported the issue here: https://lore.kernel.org/all/CAKXUXMym0M1UNuNGUVpFr2yUwOwjkZ_sQpCD0jC8YB+hs=j...

And Greg responded:

"... It's a good example driver for those wanting to create a greybus host controller driver so it's nice to have in the tree..."

So, even though the code is dead, it is a nice example of some driver code. So, we keep it.

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.

@Dan, Thanks for pointing out my clean-up activity here!

Yes, I agree with Dan. That is certainly an interesting task to explore. If you search the mailing list, you will find another bash script of similar length that Joe Perches used a few years back. I personally use the script ./scripts/checkkconfigsymbols.py. They may differ a bit on what they report, but I fear at this point, most of its reports are false positives. I have looked at all of them, I am checking the new ones introduced, and sending out patches to clean up stuff. There are a few on my todo list, like cleaning up the definition of CONFIG_CAAM_QI. If you want to assist, please let me know. I can certainly give you a few things that are at least worth a deeper investigation and maybe also some clean up.

Lukas

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;