Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v5 08/11] rtc: isl1208: Add support for the built-in
> RTC on the PMIC RAA215300
>
> Hi Biju,
>
> On Mon, May 22, 2023 at 12:19 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>
> > ---
> > v4->v5:
> > * Updated commit description.
> > * Replaced "unsigned long"->"kernel_ulong_t" in isl1208_id[].
> > * -ENOENT means clock not present, so any other errors are
> propagated.
> > * Dropped bool inverted parameter from isl1208_set_xtoscb() instead
> > using xor to compute the value of xtoscb.
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> Some suggestions for improvement below...
>
> > --- a/drivers/rtc/rtc-isl1208.c
> > +++ b/drivers/rtc/rtc-isl1208.c
>
> > @@ -852,17 +861,37 @@ isl1208_probe(struct i2c_client *client)
> > isl1208->config = (struct isl1208_config *)id-
> >driver_data;
> > }
> >
> > - xin = devm_clk_get_optional(&client->dev, "xin");
> > - if (IS_ERR(xin))
> > - return PTR_ERR(xin);
> > + if (client->dev.parent->type == &i2c_client_type) {
>
> I think this deserves a comment, to explain why you are looking at the
> parent.
OK, will do.
>
> > + xin = of_clk_get_by_name(client->dev.parent->of_node,
> "xin");
> > + if (IS_ERR(xin)) {
> > + if (PTR_ERR(xin) != -ENOENT)
> > + return PTR_ERR(xin);
> > +
> > + clkin = of_clk_get_by_name(client->dev.parent-
> >of_node,
> > + "clkin");
> > + if (IS_ERR(clkin)) {
> > + if (PTR_ERR(clkin) != -ENOENT)
> > + return PTR_ERR(xin);
> > + } else {
> > + xtosb_val = 0;
> > + clk_put(clkin);
> > + }
> > + } else {
> > + clk_put(xin);
> > + }
> > + } else {
> > + xin = devm_clk_get_optional(&client->dev, "xin");
> > + if (IS_ERR(xin))
> > + return PTR_ERR(xin);
> >
> > - if (!xin) {
> > - clkin = devm_clk_get_optional(&client->dev, "clkin");
> > - if (IS_ERR(clkin))
> > - return PTR_ERR(clkin);
> > + if (!xin) {
> > + clkin = devm_clk_get_optional(&client->dev,
> "clkin");
> > + if (IS_ERR(clkin))
> > + return PTR_ERR(clkin);
> >
> > - if (clkin)
> > - xtosb_val = 0;
> > + if (clkin)
> > + xtosb_val = 0;
> > + }
>
> I think it would make the code more readable if you would spin off the
> OF vs. dev-based clock handling into a separate helper function.
> Then you can just do in the probe function:
OK.
>
> ret = isl1208_clk_present(client, "xin");
> if (ret < 0)
> return ret;
> if (!ret) {
> ret = isl1208_clk_present(client, "clkin");
> if (ret < 0)
> return ret;
> if (ret)
> xtosb_val = 0;
> }
>
> > }
> >
> > isl1208->rtc = devm_rtc_allocate_device(&client->dev);
> > @@ -882,6 +911,7 @@ isl1208_probe(struct i2c_client *client)
> > return sr;
> > }
> >
> > + xtosb_val ^= isl1208->config->has_inverted_osc_bit ? 1 : 0;
>
> As has_inverted_osc_bit is already either 0 or 1:
>
> xtosb_val ^= isl1208->config->has_inverted_osc_bit;
>
> If you don't trust XOR, or want to make the operation more clear:
I will go with clearer one.
Cheers,
Biju
>
> if (isl1208->config->has_inverted_osc_bit)
> xtosb_val = !xtosb_val;
>
> > rc = isl1208_set_xtoscb(client, sr, xtosb_val);
> > 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
