[PATCH] regulator: raa215300: Change the scope of the variables {clkin_name, xin_name}

[PATCH] regulator: raa215300: Change the scope of the variables {clkin_name, xin_name}

[Biju Das]

Change the scope of the variables {clkin_name, xin_name} from global->local
to fix the below warning.

drivers/regulator/raa215300.c:42:12: sparse: sparse: symbol 'xin_name' was
not declared. Should it be static?

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202306250552.Fan9WTiN-lkp@intel.com/
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/regulator/raa215300.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/raa215300.c b/drivers/regulator/raa215300.c
index 24a1c89f5dbc..f58edb888e9f 100644
--- a/drivers/regulator/raa215300.c
+++ b/drivers/regulator/raa215300.c
@@ -38,8 +38,6 @@
 #define RAA215300_REG_BLOCK_EN_RTC_EN	BIT(6)
 #define RAA215300_RTC_DEFAULT_ADDR	0x6f
 
-const char *clkin_name = "clkin";
-const char *xin_name = "xin";
 static struct clk *clk;
 
 static const struct regmap_config raa215300_regmap_config = {
@@ -71,9 +69,11 @@ static int raa215300_clk_present(struct i2c_client *client, const char *name)
 static int raa215300_i2c_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
-	const char *clk_name = xin_name;
+	const char *clkin_name = "clkin";
 	unsigned int pmic_version, val;
+	const char *xin_name = "xin";
 	struct regmap *regmap;
+	const char *clk_name;
 	int ret;
 
 	regmap = devm_regmap_init_i2c(client, &raa215300_regmap_config);
@@ -120,6 +120,8 @@ static int raa215300_i2c_probe(struct i2c_client *client)
 			return ret;
 
 		clk_name = clkin_name;
+	} else {
+		clk_name = xin_name;
 	}
 
 	if (ret) {
-- 
2.25.1

Re: [PATCH] regulator: raa215300: Change the scope of the variables {clkin_name, xin_name}

[Geert Uytterhoeven]

Hi Biju,

On Sun, Jun 25, 2023 at 8:09 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Change the scope of the variables {clkin_name, xin_name} from global->local
> to fix the below warning.
>
> drivers/regulator/raa215300.c:42:12: sparse: sparse: symbol 'xin_name' was
> not declared. Should it be static?
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202306250552.Fan9WTiN-lkp@intel.com/
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch, which is correct, so:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/regulator/raa215300.c
> +++ b/drivers/regulator/raa215300.c
> @@ -38,8 +38,6 @@
>  #define RAA215300_REG_BLOCK_EN_RTC_EN  BIT(6)
>  #define RAA215300_RTC_DEFAULT_ADDR     0x6f
>
> -const char *clkin_name = "clkin";
> -const char *xin_name = "xin";
>  static struct clk *clk;
>
>  static const struct regmap_config raa215300_regmap_config = {
> @@ -71,9 +69,11 @@ static int raa215300_clk_present(struct i2c_client *client, const char *name)
>  static int raa215300_i2c_probe(struct i2c_client *client)
>  {
>         struct device *dev = &client->dev;
> -       const char *clk_name = xin_name;
> +       const char *clkin_name = "clkin";
>         unsigned int pmic_version, val;
> +       const char *xin_name = "xin";
>         struct regmap *regmap;
> +       const char *clk_name;
>         int ret;
>
>         regmap = devm_regmap_init_i2c(client, &raa215300_regmap_config);
> @@ -120,6 +120,8 @@ static int raa215300_i2c_probe(struct i2c_client *client)
>                         return ret;
>
>                 clk_name = clkin_name;
> +       } else {
> +               clk_name = xin_name;

I'd rather invert the second if-condition and exchange the two branches,
to make the code flow easier to follow for the casual reader.

        ret = raa215300_clk_present(client, xin_name);
        if (ret < 0) {
                return ret;
        } else if (ret) {
                clk_name = xin_name;
        } else {
                ret = raa215300_clk_present(client, clkin_name);
                if (ret < 0)
                        return ret;

                clk_name = clkin_name;
        }

>         }
>
>         if (ret) {

Not introduced by this patch: the check above really checks if there is
an external clock present.  A casual reader might not notice that detail,
and add more code in between the assignment to ret and the check.
So it might be prudent to pre-initialize clk_name to NULL, and set it
to clkin_name only if ret > 0.  Then the above check can become a check
for clk_name.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH] regulator: raa215300: Change the scope of the variables {clkin_name, xin_name}

[Biju Das]

Hi Geert Uytterhoeven,

Thanks for the feedback.

> Subject: Re: [PATCH] regulator: raa215300: Change the scope of the
> variables {clkin_name, xin_name}
> 
> Hi Biju,
> 
> On Sun, Jun 25, 2023 at 8:09 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Change the scope of the variables {clkin_name, xin_name} from
> > global->local to fix the below warning.
> >
> > drivers/regulator/raa215300.c:42:12: sparse: sparse: symbol 'xin_name'
> > was not declared. Should it be static?
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes:
> 
> Thanks for your patch, which is correct, so:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> > --- a/drivers/regulator/raa215300.c
> > +++ b/drivers/regulator/raa215300.c
> > @@ -38,8 +38,6 @@
> >  #define RAA215300_REG_BLOCK_EN_RTC_EN  BIT(6)
> >  #define RAA215300_RTC_DEFAULT_ADDR     0x6f
> >
> > -const char *clkin_name = "clkin";
> > -const char *xin_name = "xin";
> >  static struct clk *clk;
> >
> >  static const struct regmap_config raa215300_regmap_config = { @@
> > -71,9 +69,11 @@ static int raa215300_clk_present(struct i2c_client
> > *client, const char *name)  static int raa215300_i2c_probe(struct
> > i2c_client *client)  {
> >         struct device *dev = &client->dev;
> > -       const char *clk_name = xin_name;
> > +       const char *clkin_name = "clkin";
> >         unsigned int pmic_version, val;
> > +       const char *xin_name = "xin";
> >         struct regmap *regmap;
> > +       const char *clk_name;
> >         int ret;
> >
> >         regmap = devm_regmap_init_i2c(client,
> > &raa215300_regmap_config); @@ -120,6 +120,8 @@ static int
> raa215300_i2c_probe(struct i2c_client *client)
> >                         return ret;
> >
> >                 clk_name = clkin_name;
> > +       } else {
> > +               clk_name = xin_name;
> 
> I'd rather invert the second if-condition and exchange the two branches,
> to make the code flow easier to follow for the casual reader.
OK.
> 
>         ret = raa215300_clk_present(client, xin_name);
>         if (ret < 0) {
>                 return ret;
>         } else if (ret) {
>                 clk_name = xin_name;
>         } else {
>                 ret = raa215300_clk_present(client, clkin_name);
>                 if (ret < 0)
>                         return ret;
> 
>                 clk_name = clkin_name;
>         }
> 
> >         }
> >
> >         if (ret) {
> 
> Not introduced by this patch: the check above really checks if there is
> an external clock present.  A casual reader might not notice that
> detail, and add more code in between the assignment to ret and the
> check.
> So it might be prudent to pre-initialize clk_name to NULL, and set it to
> clkin_name only if ret > 0.  Then the above check can become a check for
> clk_name.

Agreed. Much better. Will send V2.

Cheers,
Biju