RE: [PATCH v6 11/11] rtc: isl1208: Add support for the built-in RTC on the PMIC RAA215300

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux