Hi Geert,
Thanks for the feedback.
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: Friday, May 19, 2023 1:54 PM
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Cc: Alessandro Zummo <a.zummo@xxxxxxxxxxxx>; Alexandre Belloni
> <alexandre.belloni@xxxxxxxxxxx>; linux-rtc@xxxxxxxxxxxxxxx; Fabrizio
> Castro <fabrizio.castro.jz@xxxxxxxxxxx>; linux-renesas-
> soc@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 08/11] rtc: isl1208: Add support for the built-in
> RTC on the PMIC RAA215300
>
> Hi Biju,
>
> On Thu, May 18, 2023 at 1:37 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 instantiate appropriate
>
> instantiates the
OK.
>
> > RTC device based on i2c_device_id.
> >
> > Enhance isl1208_set_xtoscb() to handle inverted bit and internal
> > oscillator is enabled or not is determined by the parent clock name.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v3->v4:
> > * Added support for internal oscillator enable/disable.
>
> Thanks for the update!
>
> > --- a/drivers/rtc/rtc-isl1208.c
> > +++ b/drivers/rtc/rtc-isl1208.c
>
> > @@ -852,11 +865,25 @@ isl1208_probe(struct i2c_client *client)
> > isl1208->config = (struct isl1208_config *)id-
> >driver_data;
> > }
> >
> > - xin = devm_clk_get(&client->dev, "xin");
> > - if (IS_ERR(xin)) {
> > - clkin = devm_clk_get(&client->dev, "clkin");
> > - if (!IS_ERR(clkin))
> > + if (client->dev.parent->type == &i2c_client_type) {
> > + xin = of_clk_get_by_name(client->dev.parent->of_node,
> "xin");
> > + if (IS_ERR(xin)) {
>
> -ENOENT means clock not present, so continue below.
> Any other error codes are to be propagated upstream.
Agreed.
>
> > + clkin = of_clk_get_by_name(client->dev.parent-
> >of_node, "clkin");
> > + if (IS_ERR(clkin))
>
> Likewise.
>
> > + return -ENODEV;
>
> Clock not present is not an error, as the clock is optional for DT
> backwards compatibility.
OK.
>
> > +
> > int_osc_en = false;
> > + clk_put(clkin);
> > + } else {
> > + clk_put(xin);
> > + }
> > + } else {
> > + xin = devm_clk_get(&client->dev, "xin");
> > + if (IS_ERR(xin)) {
> > + clkin = devm_clk_get(&client->dev, "clkin");
> > + if (!IS_ERR(clkin))
> > + int_osc_en = false;
> > + }
> > }
> >
> > isl1208->rtc = devm_rtc_allocate_device(&client->dev);
> > @@ -876,7 +903,8 @@ isl1208_probe(struct i2c_client *client)
> > return sr;
> > }
> >
> > - rc = isl1208_set_xtoscb(client, sr, int_osc_en);
> > + rc = isl1208_set_xtoscb(client, sr, int_osc_en,
> > +
> > + isl1208->config->has_inverted_osc_bit);
>
> No need to change isl1208_set_xtoscb() (but perhaps rename the
> int_osc_en parameter?) if you would pass "int_osc_en ^ isl1208->config-
> >has_inverted_osc_bit".
> (beware: C has no logical ^^, only bitwise ^).
OK will use "u8 xtosb_val" as the parameter.
Cheers,
Biju
>
>
> > if (rc)
> > return rc;
>
> 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
