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@xxxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> > Closes:
>
> Thanks for your patch, which is correct, so:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> > --- 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
