On Fri, Mar 10, 2023 at 8:40 PM Dan Carpenter <error27@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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-bA@xxxxxxxxxxxxxx/
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;
