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

[Zong Li]

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.

Many thanks for your reply.

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