| Message ID | 20230518113643.420806-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 Thu, May 18, 2023 at 1:37 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 instantiate appropriate instantiates the > 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@bp.renesas.com> > --- > 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. > + 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. > + > 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 ^). > 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
Hi Geert, Thanks for the feedback. > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Friday, May 19, 2023 1:54 PM > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni > <alexandre.belloni@bootlin.com>; linux-rtc@vger.kernel.org; Fabrizio > Castro <fabrizio.castro.jz@renesas.com>; linux-renesas- > soc@vger.kernel.org > 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@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 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@bp.renesas.com> > > --- > > 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
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c index 5f91a3ca5920..597e0126155f 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 = (unsigned long)&config_isl1208 }, { "isl1209", .driver_data = (unsigned long)&config_isl1209 }, { "isl1218", .driver_data = (unsigned long)&config_isl1218 }, { "isl1219", .driver_data = (unsigned long)&config_isl1219 }, + { "raa215300_a0", .driver_data = (unsigned long)&config_raa215300_a0 }, { } }; MODULE_DEVICE_TABLE(i2c, isl1208_id); @@ -176,12 +185,16 @@ isl1208_i2c_validate_client(struct i2c_client *client) return 0; } -static int isl1208_set_xtoscb(struct i2c_client *client, int sr, bool int_osc_en) +static int isl1208_set_xtoscb(struct i2c_client *client, int sr, + bool int_osc_en, bool inverted) { + int xtosb_set = sr | ISL1208_REG_SR_XTOSCB; + int xtosb_clr = sr & ~ISL1208_REG_SR_XTOSCB; + if (int_osc_en) - sr &= ~ISL1208_REG_SR_XTOSCB; + sr = inverted ? xtosb_set : xtosb_clr; else - sr |= ISL1208_REG_SR_XTOSCB; + sr = inverted ? xtosb_clr : xtosb_set; return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr); } @@ -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)) { + clkin = of_clk_get_by_name(client->dev.parent->of_node, "clkin"); + if (IS_ERR(clkin)) + return -ENODEV; + 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); 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 instantiate appropriate 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@bp.renesas.com> --- 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 | 44 ++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 8 deletions(-)