[PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning

[Lee Jones]

On Wed, 12 Jan 2022, Zong Li wrote:

> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones at linaro.org> wrote:
> >
> > On Tue, 11 Jan 2022, Zong Li wrote:
> >
> > > On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones at linaro.org> wrote:
> > > >
> > > > Please improve the subject line.
> > > >
> > > > If this is a straight revert, the subject line should reflect that.
> > > >
> > > > If not, you need to give us specific information regarding the purpose
> > > > of this patch.  Please read the Git log for better, more forthcoming
> > > > examples.
> > > >
> > >
> > > It seems to me that this patch is not a straight revert, it provides
> > > another way to fix the original build warnings, just like
> > > '487dc7bb6a0c' tried to do. I guess the commit message has described
> > > what the original warnings is and what the root cause is, it also
> > > mentioned what is changed in this patch. I'm a bit confused whether we
> > > need to add fixes tag, it looks like that it might cause some
> > > misunderstanding?
> >
> > I think it's the patch description and subject that is causing the
> > misunderstanding.
> >
> 
> Yes, the subject should be made better.
> 
> > Please help me with a couple of points and I'll help you draft
> > something up.
> >
> > Firstly, what alerted you to the problem you're attempting to solve?
> >
> 
> I recently noticed the code was changed, I guess that I was missing
> something there. After tracking the log, I found that there is a build
> warning in the original implementation, and it was already fixed, but
> it seems to me that there are still some situations there, please help
> me to see the following illustration.
> 
> > > > > --- a/drivers/clk/sifive/fu540-prci.c
> > > > > +++ b/drivers/clk/sifive/fu540-prci.c
> > > > > @@ -20,7 +20,6 @@
> > > > >
> > > > >  #include <dt-bindings/clock/sifive-fu540-prci.h>
> > > > >
> > > > > -#include "fu540-prci.h"
> >
> > How is this related to the issue/patch?
> >
> 
> Let's go back to the version without '487dc7bb6a0c' fix. The
> prci_clk_fu540 variable is defined in sifive-fu540-prci.h header,
> however, fu540-prci.c includes this header but doesn't use this
> variable, so the warnings happen.
> 
> The easiest way to do it is just removing this line, then the warning
> could be fixed. But as the '487dc7bb6a0c' or this patch does, the code
> should be improved, the prci_clk_fu540 variable shouldn't be defined
> in the header, it should be moved somewhere.
> 
> > > > > +struct prci_clk_desc prci_clk_fu540 = {
> > > > > +     .clks = __prci_init_clocks_fu540,
> > > > > +     .num_clks = ARRAY_SIZE(__prci_init_clocks_fu540),
> > > > > +};
> >
> > > > > diff --git a/drivers/clk/sifive/fu540-prci.h b/drivers/clk/sifive/fu540-prci.h
> > > > > index c220677dc010..931d6cad8c1c 100644
> > > > > --- a/drivers/clk/sifive/fu540-prci.h
> > > > > +++ b/drivers/clk/sifive/fu540-prci.h
> > > > > @@ -7,10 +7,6 @@
> > > > > +extern struct prci_clk_desc prci_clk_fu540;
> >
> > > > > diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c
> > > > > index 80a288c59e56..916d2fc28b9c 100644
> > > > > --- a/drivers/clk/sifive/sifive-prci.c
> > > > > +++ b/drivers/clk/sifive/sifive-prci.c
> > > > > @@ -12,11 +12,6 @@
> > > > >  #include "fu540-prci.h"
> > > > >  #include "fu740-prci.h"
> > > > >
> > > > > -static const struct prci_clk_desc prci_clk_fu540 = {
> > > > > -     .clks = __prci_init_clocks_fu540,
> > > > > -     .num_clks = ARRAY_SIZE(__prci_init_clocks_fu540),
> > > > > -};
> > > > > -
> >
> > I'm not sure if it's you or I that is missing the point here, but
> > prci_clk_fu540 is used within *this* file itself:
> >
> 
> Here is another situation I mentioned at the beginning, if we'd like
> to put prci_clk_fu540 here, prci_clk_fu740 should be put here as well.
> I guess you didn't do that because there is a bug in the original
> code, fu740-prci.c misused the fu540-prci.h, so there is no build
> warning on fu740. FU740 still works correctly by misusing the
> fu540-prci.h header because fu740-prci.c doesn't actually use the
> prci_clk_fu740 variable, like fu540 we talked about earlier.
> 
> >  static const struct of_device_id sifive_prci_of_match[] = {
> >          {.compatible = "sifive,fu540-c000-prci", .data = &prci_clk_fu540},
> >          {.compatible = "sifive,fu740-c000-prci", .data = &prci_clk_fu740},
> >          {}
> >  };
> >
> > So why are you moving it out to somewhere it is *not* used and making
> > it an extern?  This sounds like the opposite to what you'd want?
> 
> The idea is that sifive-prci.c is the core and common part of PRCI,
> and I'd like to separate the SoCs-dependent part into SoCs-dependent
> files, such as fu540-prci.c and fu740-prci.c. The goal is if we add
> new SoCs in the future, we can just put the SoCs-dependent data
> structure in the new C file, and do as minimum modification as
> possible in the core file (i.e. sifive-prci.c). It might also help us
> to see all SoCs-dependent data in one file, then we don't need to
> cross many files. Putting these two variables in sifive-pric.c is the
> right thing to do, but that is why I separate them and make them
> extern in this patch.

I can see what you are doing, but I don't think this is the right
thing to do.  Please put the struct in the same location as it's
referenced.

And yes that should also be the case for prci_clk_fu740 and yes, it
was over-looked because it wasn't causing warnings at build time for
whatever reason.

IMHO, placing 'struct of_device_id' match tables in headers is also
odd and is likely the real cause of this strange situation.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog