| Message ID | 20230522101849.297499-9-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add Renesas PMIC RAA215300 and built-in RTC support | expand |
Hi Biju, On Mon, May 22, 2023 at 12:19 PM Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> > --- > 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@glider.be> 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
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@bp.renesas.com> > 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@bp.renesas.com> > > --- > > 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@glider.be> > > 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
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c index 6ca977595977..5c68120ff543 100644 --- a/drivers/rtc/rtc-isl1208.c +++ b/drivers/rtc/rtc-isl1208.c @@ -74,6 +74,7 @@ struct isl1208_config { unsigned int nvmem_length; unsigned has_tamper:1; unsigned has_timestamp:1; + unsigned has_inverted_osc_bit:1; }; static const struct isl1208_config config_isl1208 = { @@ -100,11 +101,19 @@ static const struct isl1208_config config_isl1219 = { .has_timestamp = true }; +static const struct isl1208_config config_raa215300_a0 = { + .nvmem_length = 2, + .has_tamper = false, + .has_timestamp = false, + .has_inverted_osc_bit = true +}; + static const struct i2c_device_id isl1208_id[] = { { "isl1208", .driver_data = (kernel_ulong_t)&config_isl1208 }, { "isl1209", .driver_data = (kernel_ulong_t)&config_isl1209 }, { "isl1218", .driver_data = (kernel_ulong_t)&config_isl1218 }, { "isl1219", .driver_data = (kernel_ulong_t)&config_isl1219 }, + { "raa215300_a0", .driver_data = (kernel_ulong_t)&config_raa215300_a0 }, { } }; MODULE_DEVICE_TABLE(i2c, isl1208_id); @@ -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) { + 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; + } } 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; rc = isl1208_set_xtoscb(client, sr, xtosb_val); if (rc) return rc;
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@bp.renesas.com> --- 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. v3->v4: * Added support for internal oscillator enable/disable. v2->v3: * RTC device is instantiated by PMIC driver and dropped isl1208_probe_helper(). * Added "TYPE_RAA215300_RTC_A0" to handle inverted oscillator bit case. RFC->v2: * Dropped compatible "renesas,raa215300-isl1208" and "renesas,raa215300-pmic" property. * Updated the comment polarity->bit for External Oscillator. * Added raa215300_rtc_probe_helper() for registering raa215300_rtc device and added the helper function isl1208_probe_helper() to share the code. --- drivers/rtc/rtc-isl1208.c | 48 +++++++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 9 deletions(-)