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.
> + 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:
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:
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@xxxxxxxxxxxxxx
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
