Message ID | 20230625180903.142994-1-biju.das.jz@bp.renesas.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Series | regulator: raa215300: Change the scope of the variables {clkin_name, xin_name} | expand |
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
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
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) {
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(-)