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

From: Lee Jones <lee.jones@linaro.org>
To: Zong Li <zong.li@sifive.com>
Cc: Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	linux-clk <linux-clk@vger.kernel.org>,
	linux-riscv <linux-riscv@lists.infradead.org>
Subject: Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
Date: Tue, 11 Jan 2022 09:32:44 +0000	[thread overview]
Message-ID: <Yd1OvFZ4pKw+aTgv@google.com> (raw)
In-Reply-To: <CANXhq0ookagQTZZrNduP5DjXs2awQdRkUxNzTWU=-dz+TVuUwg@mail.gmail.com>

On Tue, 11 Jan 2022, Zong Li wrote:

> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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.

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?

> > > --- 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?

> > > +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:

 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?

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