| Message ID | 20230602142426.438375-11-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 Fri, Jun 2, 2023 at 4:25 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > As per the HW manual, set the XTOSCB bit as follows: > > If using an external clock signal, set the XTOSCB bit as 1 to > disable the crystal oscillator. > > If using an external crystal, the XTOSCB bit needs to be set at 0 > to enable the crystal oscillator. > > Add isl1208_set_xtoscb() to set XTOSCB bit based on the clock-names > property. Fallback is enabling the internal crystal oscillator. > > While at it, introduce a variable "sr" for reading the status register > in probe() as it is reused for writing. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > v5->v6: > * Added Rb tag from Geert > * Replaced u8->int for xtosb_val parameter. > * Introduced isl1208_clk_present() for checking the presence of "xin" and > "clkin" for determining internal oscillator is enabled or not. Thanks for the update! > --- a/drivers/rtc/rtc-isl1208.c > +++ b/drivers/rtc/rtc-isl1208.c > @@ -805,12 +816,33 @@ static int isl1208_setup_irq(struct i2c_client *client, int irq) > return rc; > } > > +static int > +isl1208_clk_present(struct i2c_client *client, const char *name) > +{ > + struct clk *clk; > + int ret; > + > + clk = devm_clk_get_optional(&client->dev, name); > + if (IS_ERR(clk)) { > + ret = PTR_ERR(clk); > + } else { > + if (!clk) What about "else if"? ;-) But in this case, you can make the code even simpler (less indented) by returning early after each test. > + ret = 0; > + else > + ret = 1; > + } if (IS_ERR(clk)) return PTR_ERR(clk); return !!clk; > + > + return ret; > +} > + Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH v6 10/11] rtc: isl1208: Add isl1208_set_xtoscb() > > Hi Biju, > > On Fri, Jun 2, 2023 at 4:25 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > As per the HW manual, set the XTOSCB bit as follows: > > > > If using an external clock signal, set the XTOSCB bit as 1 to disable > > the crystal oscillator. > > > > If using an external crystal, the XTOSCB bit needs to be set at 0 to > > enable the crystal oscillator. > > > > Add isl1208_set_xtoscb() to set XTOSCB bit based on the clock-names > > property. Fallback is enabling the internal crystal oscillator. > > > > While at it, introduce a variable "sr" for reading the status register > > in probe() as it is reused for writing. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > v5->v6: > > * Added Rb tag from Geert > > * Replaced u8->int for xtosb_val parameter. > > * Introduced isl1208_clk_present() for checking the presence of "xin" > and > > "clkin" for determining internal oscillator is enabled or not. > > Thanks for the update! > > > --- a/drivers/rtc/rtc-isl1208.c > > +++ b/drivers/rtc/rtc-isl1208.c > > @@ -805,12 +816,33 @@ static int isl1208_setup_irq(struct i2c_client > *client, int irq) > > return rc; > > } > > > > +static int > > +isl1208_clk_present(struct i2c_client *client, const char *name) { > > + struct clk *clk; > > + int ret; > > + > > + clk = devm_clk_get_optional(&client->dev, name); > > + if (IS_ERR(clk)) { > > + ret = PTR_ERR(clk); > > + } else { > > + if (!clk) > > What about "else if"? ;-) > But in this case, you can make the code even simpler (less indented) by > returning early after each test. > > > + ret = 0; > > + else > > + ret = 1; > > + } > > if (IS_ERR(clk)) > return PTR_ERR(clk); > > return !!clk; OK, will change to this logic. Cheers, Biju > > > + > > + return ret; > > +} > > + > > 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
On Friday, June 2, 2023 7:24:25 AM PDT Biju Das wrote: > > +static int isl1208_set_xtoscb(struct i2c_client *client, int sr, int xtosb_val) +{ > + if (xtosb_val) > + sr &= ~ISL1208_REG_SR_XTOSCB; > + else > + sr |= ISL1208_REG_SR_XTOSCB; > + > + return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr); > +} Since the contents of this register are preserved by battery power, it will normally already have the correct value. Setting it this way adds an extra I2C transaction to every driver init to set the register to the value it's already at. It would be better to check if the bit is not set correctly, and then only set it and write to the register if it is not. You can do that like this: /* Strangely, xtosb_val of 0 means to set the bit and 1 means to clear it! * Hopefully, that is really what you want to do. Seems backward of what * I would expect. */ static int isl1208_set_xtoscb(struct i2c_client *client, int sr, int xtosb_val) { /* Do nothing if bit is already set to desired value */ if (!(st & ISL1208_REG_SR_XTOSCB) == xtosb_val) return 0; sr ^= ISL1208_REG_SR_XTOSCB; return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr); } > static int > isl1208_probe(struct i2c_client *client) > { > - int rc = 0; > struct isl1208_state *isl1208; > int evdet_irq = -1; > + int xtosb_val = 1; So you assume XTOSCB should be unset by default? Prior behavior of the driver was to preserve this bit. Maybe it was set the bootloader to the correct value? This would break such a setup. > + rc = isl1208_clk_present(client, "xin"); > + if (rc < 0) > + return rc; > + > + if (!rc) { > + rc = isl1208_clk_present(client, "clkin"); Why do you support two names for the same clock? I don't see this discussed in any of the threads.
Hi Trent Piepho, Thanks for the feedback. > Subject: Re: [PATCH v6 10/11] rtc: isl1208: Add isl1208_set_xtoscb() > > On Friday, June 2, 2023 7:24:25 AM PDT Biju Das wrote: > > > > +static int isl1208_set_xtoscb(struct i2c_client *client, int sr, int > xtosb_val) +{ > > + if (xtosb_val) > > + sr &= ~ISL1208_REG_SR_XTOSCB; > > + else > > + sr |= ISL1208_REG_SR_XTOSCB; > > + > > + return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr); } > > Since the contents of this register are preserved by battery power, it > will normally already have the correct value. Agreed. What about the cold boot and environment where there is no RTC support in bootloader? > > Setting it this way adds an extra I2C transaction to every driver init to > set the register to the value it's already at. [3] > It would be better to check if the bit is not set correctly, and then > only set it and write to the register if it is not. Agreed. > > You can do that like this: > > /* Strangely, xtosb_val of 0 means to set the bit and 1 means to clear > it! OK, But as per HW manual, see [1], 0 means :- Enable internal crystal oscillator 1 means :- Disable internal crystal oscillator <snippet> CRYSTAL OSCILLATOR ENABLE BIT (XTOSCB) This bit enables/disables the internal crystal oscillator. When the XTOSCB is set to "1", the oscillator is disabled, and the X1 pin allows for an external 32kHz signal to drive the RTC. The XTOSCB bit is set to "0" on power-up. </snippet> [1] https://www.renesas.com/us/en/document/dst/isl1208-datasheet > * Hopefully, that is really what you want to do. Seems backward of what > * I would expect. > */ Agreed. > static int isl1208_set_xtoscb(struct i2c_client *client, int sr, int > xtosb_val) { > /* Do nothing if bit is already set to desired value */ > if (!(st & ISL1208_REG_SR_XTOSCB) == xtosb_val) > return 0; It is (st & ISL1208_REG_SR_XTOSCB) == xtosb_val) right?? BIT(2) = 0 and xtosb_val =0 --> return 0 BIT(2) = 1 and xtosb_val =1 --> return 0 BIT(2) = 0 and xtosb_val = 1 --> sr ^= ISL1208_REG_SR_XTOSCB BIT(2) = 1 and xtosb_val =0 --> sr ^= ISL1208_REG_SR_XTOSCB > sr ^= ISL1208_REG_SR_XTOSCB; > return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr); } > > > > static int > > isl1208_probe(struct i2c_client *client) { > > - int rc = 0; > > struct isl1208_state *isl1208; > > int evdet_irq = -1; > > + int xtosb_val = 1; > > So you assume XTOSCB should be unset by default? Prior behavior of the > driver was to preserve this bit. Maybe it was set the bootloader to the > correct value? This would break such a setup. Cold boot, reset value is '0'. We should not assume any bootloader settings. Currently I don't have bootloader support for RTC in my environment and depending on dt settings parse the details to find it is connected to external crystal or Clock source. Normally u-boot device tree is based on kernel device tree. As you said above[3], Parse the details from kernel and see, if there is mismatch and then only override. > > > + rc = isl1208_clk_present(client, "xin"); > > + if (rc < 0) > > + return rc; > > + > > + if (!rc) { > > + rc = isl1208_clk_present(client, "clkin"); > > Why do you support two names for the same clock? I don't see this > discussed in any of the threads. Please see [2]. + Use xin, if connected to an external crystal. + Use clkin, if connected to an external clock signal. [2] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230602142426.438375-7-biju.das.jz@bp.renesas.com/ Cheers, Biju
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c index d42615fcdd9f..e0f06677b91b 100644 --- a/drivers/rtc/rtc-isl1208.c +++ b/drivers/rtc/rtc-isl1208.c @@ -6,6 +6,7 @@ */ #include <linux/bcd.h> +#include <linux/clk.h> #include <linux/i2c.h> #include <linux/module.h> #include <linux/of_device.h> @@ -175,6 +176,16 @@ isl1208_i2c_validate_client(struct i2c_client *client) return 0; } +static int isl1208_set_xtoscb(struct i2c_client *client, int sr, int xtosb_val) +{ + if (xtosb_val) + sr &= ~ISL1208_REG_SR_XTOSCB; + else + sr |= ISL1208_REG_SR_XTOSCB; + + return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr); +} + static int isl1208_i2c_get_sr(struct i2c_client *client) { @@ -805,12 +816,33 @@ static int isl1208_setup_irq(struct i2c_client *client, int irq) return rc; } +static int +isl1208_clk_present(struct i2c_client *client, const char *name) +{ + struct clk *clk; + int ret; + + clk = devm_clk_get_optional(&client->dev, name); + if (IS_ERR(clk)) { + ret = PTR_ERR(clk); + } else { + if (!clk) + ret = 0; + else + ret = 1; + } + + return ret; +} + static int isl1208_probe(struct i2c_client *client) { - int rc = 0; struct isl1208_state *isl1208; int evdet_irq = -1; + int xtosb_val = 1; + int rc = 0; + int sr; if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) return -ENODEV; @@ -837,6 +869,19 @@ isl1208_probe(struct i2c_client *client) isl1208->config = (struct isl1208_config *)id->driver_data; } + rc = isl1208_clk_present(client, "xin"); + if (rc < 0) + return rc; + + if (!rc) { + rc = isl1208_clk_present(client, "clkin"); + if (rc < 0) + return rc; + + if (rc) + xtosb_val = 0; + } + isl1208->rtc = devm_rtc_allocate_device(&client->dev); if (IS_ERR(isl1208->rtc)) return PTR_ERR(isl1208->rtc); @@ -848,13 +893,17 @@ isl1208_probe(struct i2c_client *client) isl1208->nvmem_config.size = isl1208->config->nvmem_length; isl1208->nvmem_config.priv = isl1208; - rc = isl1208_i2c_get_sr(client); - if (rc < 0) { + sr = isl1208_i2c_get_sr(client); + if (sr < 0) { dev_err(&client->dev, "reading status failed\n"); - return rc; + return sr; } - if (rc & ISL1208_REG_SR_RTCF) + rc = isl1208_set_xtoscb(client, sr, xtosb_val); + if (rc) + return rc; + + if (sr & ISL1208_REG_SR_RTCF) dev_warn(&client->dev, "rtc power failure detected, " "please set clock.\n");