Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v6 11/11] rtc: isl1208: Add support for the built-in
> RTC on the PMIC RAA215300
>
> Hi Biju,
>
> On Fri, Jun 2, 2023 at 4:25 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > The built-in RTC found on PMIC RAA215300 is the same as ISL1208.
> > However, the external oscillator bit is inverted on PMIC version 0x11.
> > The PMIC driver detects PMIC version and instantiates the RTC device
> > based on i2c_device_id.
> >
> > The internal oscillator is enabled or not is determined by the parent
> > clock name.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > ---
> > v5->v6:
> > * Added Rb tag from Geert.
> > * Parsing of parent node is moved from probe->isl1208_clk_present()
> > * Added comment for parsing parent node for getting clock resource.
> > * Replaced XOR->NOT to make the operation more clear for the inverted
> case.
>
> Thanks for the update!
>
> > --- a/drivers/rtc/rtc-isl1208.c
> > +++ b/drivers/rtc/rtc-isl1208.c
>
> > @@ -822,14 +831,32 @@ isl1208_clk_present(struct i2c_client *client,
> const char *name)
> > struct clk *clk;
> > int ret;
> >
> > - clk = devm_clk_get_optional(&client->dev, name);
> > - if (IS_ERR(clk)) {
> > - ret = PTR_ERR(clk);
> > - } else {
> > - if (!clk)
> > - ret = 0;
> > - else
> > + /*
> > + * For the built-in RTC found on PMIC RAA21530, enabling of
> the
> > + * internal oscillator is based on the parent clock. So parse
> the
> > + * parent node to get the details.
> > + */
> > + if (client->dev.parent->type == &i2c_client_type) {
> > + clk = of_clk_get_by_name(client->dev.parent->of_node,
> name);
> > + if (IS_ERR(clk)) {
> > + if (PTR_ERR(clk) != -ENOENT)
> > + ret = PTR_ERR(clk);
> > + else
> > + ret = 0;
> > + } else {
> > + clk_put(clk);
> > ret = 1;
> > + }
>
> Perhaps reshuffle the conditions to decrease indentation?
> And return early?
>
> if (!IS_ERR(clk)) {
> clk_put(clk);
> return 1;
> }
>
> if (PTR_ERR(clk) != -ENOENT)
> return PTR_ERR(clk);
>
> return 0;
Agreed.
>
>
> > + } else {
> > + clk = devm_clk_get_optional(&client->dev, name);
> > + if (IS_ERR(clk)) {
> > + ret = PTR_ERR(clk);
> > + } else {
> > + if (!clk)
> > + ret = 0;
> > + else
> > + ret = 1;
> > + }
>
> Same comments as [PATCH v6 10/11].
OK, will do this change in v7.
Cheers,
Biju
>
> > }
> >
> > return ret;
>
> 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
