| Message ID | 20230816142458.147476-10-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Add Renesas PMIC RAA215300 driver and builtin RTC support | expand |
Hi! > 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. > +++ b/drivers/rtc/rtc-isl1208.c > @@ -6,6 +6,7 @@ > @@ -175,6 +176,20 @@ isl1208_i2c_validate_client(struct i2c_client *client) > return 0; > } > > +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 ((sr & ISL1208_REG_SR_XTOSCB) == xtosb_val) > + return 0; XTOSCB bit is not bit 0, but xtosb_val is either 0 or 1. If it is 1, test will never succeed. I don't think that's intended. > + 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); Best regards, Pavel
Hi Pavel Machek, Thanks for the feedback. > Subject: Re: [PATCH 6.1.y-cip 09/13] rtc: isl1208: Add isl1208_set_xtoscb() > > Hi! > > > 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. > > > +++ b/drivers/rtc/rtc-isl1208.c > > @@ -6,6 +6,7 @@ > > @@ -175,6 +176,20 @@ isl1208_i2c_validate_client(struct i2c_client > *client) > > return 0; > > } > > > > +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 ((sr & ISL1208_REG_SR_XTOSCB) == xtosb_val) > > + return 0; > > XTOSCB bit is not bit 0, but xtosb_val is either 0 or 1. If it is 1, test > will never succeed. I don't think that's intended. As mentioned in the comment, check is to avoid unnecessary writing to I2C register if it is same value. 0 & 1 == 0 -->skip 0 & 1 == 1 -->write 1 & 1 == 0 --> write 1 & 1 == 1 -->skip Is it expected result right? Cheers, Biju > > > + 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); > > Best regards, > Pavel > -- > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Hi! > > > If using an external crystal, the XTOSCB bit needs to be set at 0 to > > > enable the crystal oscillator. > > > > > +++ b/drivers/rtc/rtc-isl1208.c > > > @@ -6,6 +6,7 @@ > > > @@ -175,6 +176,20 @@ isl1208_i2c_validate_client(struct i2c_client > > *client) > > > return 0; > > > } > > > > > > +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 ((sr & ISL1208_REG_SR_XTOSCB) == xtosb_val) > > > + return 0; > > > > XTOSCB bit is not bit 0, but xtosb_val is either 0 or 1. If it is 1, test > > will never succeed. I don't think that's intended. > > As mentioned in the comment, check is to avoid unnecessary > writing to I2C register if it is same value. > > 0 & 1 == 0 -->skip > 0 & 1 == 1 -->write > 1 & 1 == 0 --> write > 1 & 1 == 1 -->skip > > Is it expected result right? As far as I can see, you'll get #define ISL1208_REG_SR_XTOSCB (1<<6) xtosb_val = 1; 64 & 64 == 1 64 == 1 false --> write when you should be skipping. Best regards, Pavel
Hi Pavel, Thanks for the feedback. > Subject: Re: [PATCH 6.1.y-cip 09/13] rtc: isl1208: Add isl1208_set_xtoscb() > > Hi! > > > > > If using an external crystal, the XTOSCB bit needs to be set at 0 > > > > to enable the crystal oscillator. > > > > > > > +++ b/drivers/rtc/rtc-isl1208.c > > > > @@ -6,6 +6,7 @@ > > > > @@ -175,6 +176,20 @@ isl1208_i2c_validate_client(struct i2c_client > > > *client) > > > > return 0; > > > > } > > > > > > > > +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 ((sr & ISL1208_REG_SR_XTOSCB) == xtosb_val) > > > > + return 0; > > > > > > XTOSCB bit is not bit 0, but xtosb_val is either 0 or 1. If it is 1, > > > test will never succeed. I don't think that's intended. > > > > As mentioned in the comment, check is to avoid unnecessary writing to > > I2C register if it is same value. > > > > 0 & 1 == 0 -->skip > > 0 & 1 == 1 -->write > > 1 & 1 == 0 --> write > > 1 & 1 == 1 -->skip > > > > Is it expected result right? > > As far as I can see, you'll get > > #define ISL1208_REG_SR_XTOSCB (1<<6) > > xtosb_val = 1; > > 64 & 64 == 1 > 64 == 1 > false --> write > > when you should be skipping. You are correct. I need to do double negation to fix this. /* Do nothing if bit is already set to desired value */ if (!!(sr & ISL1208_REG_SR_XTOSCB) == xtosb_val) return 0; Cheers, Biju
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c index 80ecb724ae6b..915f23668228 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,20 @@ isl1208_i2c_validate_client(struct i2c_client *client) return 0; } +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 ((sr & ISL1208_REG_SR_XTOSCB) == xtosb_val) + return 0; + + 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) { @@ -511,7 +526,6 @@ isl1208_i2c_set_time(struct i2c_client *client, struct rtc_time const *tm) return 0; } - static int isl1208_rtc_set_time(struct device *dev, struct rtc_time *tm) { @@ -805,12 +819,26 @@ 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; + + clk = devm_clk_get_optional(&client->dev, name); + if (IS_ERR(clk)) + return PTR_ERR(clk); + + return !!clk; +} + static int isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id) { - int rc = 0; struct isl1208_state *isl1208; int evdet_irq = -1; + int xtosb_val = 0; + int rc = 0; + int sr; if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) return -ENODEV; @@ -835,6 +863,19 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id) 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 = 1; + } + isl1208->rtc = devm_rtc_allocate_device(&client->dev); if (IS_ERR(isl1208->rtc)) return PTR_ERR(isl1208->rtc); @@ -846,13 +887,17 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id) 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");