Message ID | 1421203412-5159-1-git-send-email-zyw@rock-chips.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Jan 13, 2015 at 6:43 PM, Chris Zhong <zyw@rock-chips.com> wrote: > After we set the GET_TIME bit, the rtc time couldn't be read immediately, > we should wait up to 31.25 us, about one cycle of 32khz. Otherwise reading > RTC time will return a old time. If clear the GET_TIME bit after setting, > the time of i2c transfer certainly more than 31.25us. > > Signed-off-by: Chris Zhong <zyw@rock-chips.com> > > --- > > drivers/rtc/rtc-rk808.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/rtc/rtc-rk808.c b/drivers/rtc/rtc-rk808.c > index df42257..8dae322 100644 > --- a/drivers/rtc/rtc-rk808.c > +++ b/drivers/rtc/rtc-rk808.c > @@ -67,15 +67,20 @@ static int rk808_rtc_readtime(struct device *dev, struct rtc_time *tm) > /* Force an update of the shadowed registers right now */ > ret = regmap_update_bits(rk808->regmap, RK808_RTC_CTRL_REG, > BIT_RTC_CTRL_REG_RTC_GET_TIME, > - 0); > + BIT_RTC_CTRL_REG_RTC_GET_TIME); > if (ret) { > dev_err(dev, "Failed to update bits rtc_ctrl: %d\n", ret); > return ret; > } > > + /* After we set the GET_TIME bit, the rtc time couldn't be read > + * immediately, we should wait up to 31.25 us, about one cycle of > + * 32khz. If we clear the GET_TIME bit here, the time of i2c transfer > + * certainly more than 31.25us. > + */ Chris, it looks like you swapped the set and the clear of this bit, and you're relying on the fact that the i2c transaction takes a certain amount of time after the RTC_GET_TIME BIT is set. I'm not sure how long it actually takes, but why not just put in a usleep() for the minimum wait time? > ret = regmap_update_bits(rk808->regmap, RK808_RTC_CTRL_REG, > BIT_RTC_CTRL_REG_RTC_GET_TIME, > - BIT_RTC_CTRL_REG_RTC_GET_TIME); > + 0); > if (ret) { > dev_err(dev, "Failed to update bits rtc_ctrl: %d\n", ret); > return ret; > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
Sonny, > Chris, it looks like you swapped the set and the clear of this bit, > and you're relying on the fact that the i2c transaction takes a > certain amount of time after the RTC_GET_TIME BIT is set. I'm not > sure how long it actually takes, but why not just put in a usleep() > for the minimum wait time? I think we are safe. At 400kHz (the max speed of this part) each bit can be transferred no faster than 2.5us. In order to do a valid i2c transaction we need to _at least_ write the address of the device and the data onto the bus, which is 16 bits. 16 * 2.5us = 40us. That's above the 31.25us Personally I think what Chris has is fine, with the comment. -Doug
Chris, On Tue, Jan 13, 2015 at 6:43 PM, Chris Zhong <zyw@rock-chips.com> wrote: > + /* After we set the GET_TIME bit, the rtc time couldn't be read > + * immediately, we should wait up to 31.25 us, about one cycle of > + * 32khz. If we clear the GET_TIME bit here, the time of i2c transfer > + * certainly more than 31.25us. > + */ No idea what Andrew thinks of this, but other multiline comments in this file look slightly different (there's a line containing just /*). > ret = regmap_update_bits(rk808->regmap, RK808_RTC_CTRL_REG, > BIT_RTC_CTRL_REG_RTC_GET_TIME, > - BIT_RTC_CTRL_REG_RTC_GET_TIME); > + 0); > if (ret) { > dev_err(dev, "Failed to update bits rtc_ctrl: %d\n", ret); > return ret; Above is just a nit. I'll leave it up to Andrew to say whether he wants you to spin, or he'll fix it up, or he doesn't care and he'll land it as-is. Reviewed-by: Doug Anderson <dianders@chromium.org>
On Wed, Jan 14, 2015 at 10:36 AM, Doug Anderson <dianders@chromium.org> wrote: > Sonny, > >> Chris, it looks like you swapped the set and the clear of this bit, >> and you're relying on the fact that the i2c transaction takes a >> certain amount of time after the RTC_GET_TIME BIT is set. I'm not >> sure how long it actually takes, but why not just put in a usleep() >> for the minimum wait time? > > I think we are safe. > > At 400kHz (the max speed of this part) each bit can be transferred no > faster than 2.5us. In order to do a valid i2c transaction we need to > _at least_ write the address of the device and the data onto the bus, > which is 16 bits. 16 * 2.5us = 40us. That's above the 31.25us > > Personally I think what Chris has is fine, with the comment. Ok, I'm fine with that if we're sure it's slow enough. Comment explaining would certainly help. > > -Doug
diff --git a/drivers/rtc/rtc-rk808.c b/drivers/rtc/rtc-rk808.c index df42257..8dae322 100644 --- a/drivers/rtc/rtc-rk808.c +++ b/drivers/rtc/rtc-rk808.c @@ -67,15 +67,20 @@ static int rk808_rtc_readtime(struct device *dev, struct rtc_time *tm) /* Force an update of the shadowed registers right now */ ret = regmap_update_bits(rk808->regmap, RK808_RTC_CTRL_REG, BIT_RTC_CTRL_REG_RTC_GET_TIME, - 0); + BIT_RTC_CTRL_REG_RTC_GET_TIME); if (ret) { dev_err(dev, "Failed to update bits rtc_ctrl: %d\n", ret); return ret; } + /* After we set the GET_TIME bit, the rtc time couldn't be read + * immediately, we should wait up to 31.25 us, about one cycle of + * 32khz. If we clear the GET_TIME bit here, the time of i2c transfer + * certainly more than 31.25us. + */ ret = regmap_update_bits(rk808->regmap, RK808_RTC_CTRL_REG, BIT_RTC_CTRL_REG_RTC_GET_TIME, - BIT_RTC_CTRL_REG_RTC_GET_TIME); + 0); if (ret) { dev_err(dev, "Failed to update bits rtc_ctrl: %d\n", ret); return ret;
After we set the GET_TIME bit, the rtc time couldn't be read immediately, we should wait up to 31.25 us, about one cycle of 32khz. Otherwise reading RTC time will return a old time. If clear the GET_TIME bit after setting, the time of i2c transfer certainly more than 31.25us. Signed-off-by: Chris Zhong <zyw@rock-chips.com> --- drivers/rtc/rtc-rk808.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)