| Message ID | 20160301213322.661fe771@wiggum |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On 01/03/2016 at 21:33:22 +0100, Michael Büsch wrote : > This adds a device tree property to enable the tricke charger at > some to be configured resistance. > If the property is left out, the trickle charger is disabled. > > The kconfig name is also changed to include the chip number as > there are other MicroCrystal RTCs. > This should have been another patch and will conflict with my patch doing exactly that, see rtc-next. > Also a "rv3029" device ID was added, because the C2 suffix does > not appear in latest chip versions and datasheet versions. > This change should also be part of another patch. You may as well stop adding that suffix to newly added functions and macros. I'd also take a preliminary patch removing the c2 prefix from current functions and macros. Unfortunately, we can't rename the file because this will break scripts loading modules. > /* Register map */ > /* control section */ > #define RV3029C2_ONOFF_CTRL 0x00 > +#define RV3029C2_ONOFF_CTRL_WE (1 << 0) > +#define RV3029C2_ONOFF_CTRL_TE (1 << 1) > +#define RV3029C2_ONOFF_CTRL_TAR (1 << 2) > +#define RV3029C2_ONOFF_CTRL_EERE (1 << 3) > +#define RV3029C2_ONOFF_CTRL_SRON (1 << 4) > +#define RV3029C2_ONOFF_CTRL_TD0 (1 << 5) > +#define RV3029C2_ONOFF_CTRL_TD1 (1 << 6) > +#define RV3029C2_ONOFF_CTRL_CLKINT (1 << 7) You didn't use --strict for checkpatch, please use BIT() and also correct the alignments issues. > #define RV3029C2_IRQ_CTRL 0x01 > #define RV3029C2_IRQ_CTRL_AIE (1 << 0) > +#define RV3029C2_IRQ_CTRL_TIE (1 << 1) > +#define RV3029C2_IRQ_CTRL_V1IE (1 << 2) > +#define RV3029C2_IRQ_CTRL_V2IE (1 << 3) > +#define RV3029C2_IRQ_CTRL_SRIE (1 << 4) I'm pretty sure many you are adding many of those but not using them yet. > /* user ram section */ > #define RV3029C2_USR1_RAM_PAGE 0x38 > @@ -84,6 +111,7 @@ > #define RV3029C2_USR2_RAM_PAGE 0x3C > #define RV3029C2_USR2_SECTION_LEN 0x04 > > + spurious blank line > static int > rv3029c2_i2c_read_regs(struct i2c_client *client, u8 reg, u8 *buf, > unsigned len) > @@ -114,6 +142,24 @@ rv3029c2_i2c_write_regs(struct i2c_client *client, u8 reg, u8 const buf[], > } > > static int > +rv3029c2_i2c_maskset_reg(struct i2c_client *client, u8 reg, u8 mask, u8 set) I'd prefer that function named _update_bits > +{ > + u8 buf[1]; couldn't that be u8 buf? > + int ret; > + > + ret = rv3029c2_i2c_read_regs(client, reg, buf, 1); > + if (ret < 0) > + return ret; > + buf[0] &= mask; > + buf[0] |= set; > + ret = rv3029c2_i2c_write_regs(client, reg, buf, 1); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int > rv3029c2_i2c_get_sr(struct i2c_client *client, u8 *buf) > { > int ret = rv3029c2_i2c_read_regs(client, RV3029C2_STATUS, buf, 1); > @@ -138,6 +184,128 @@ rv3029c2_i2c_set_sr(struct i2c_client *client, u8 val) > return 0; > } > > +static int rv3029c2_eeprom_busywait(struct i2c_client *client) > +{ > + int i, ret; > + u8 sr; > + > + for (i = 100; i > 0; i--) { > + ret = rv3029c2_i2c_get_sr(client, &sr); > + if (ret < 0) > + break; > + if (!(sr & RV3029C2_STATUS_EEBUSY)) > + break; > + usleep_range(1000, 10000); > + } > + if (i <= 0) { > + dev_err(&client->dev, "EEPROM busy wait timeout.\n"); > + return -ETIMEDOUT; > + } > + > + return ret; > +} > + > +static int rv3029c2_eeprom_exit(struct i2c_client *client) > +{ > + /* Re-enable eeprom refresh */ > + return rv3029c2_i2c_maskset_reg(client, RV3029C2_ONOFF_CTRL, > + 0xFF, RV3029C2_ONOFF_CTRL_EERE); > +} > + > +static int rv3029c2_eeprom_enter(struct i2c_client *client) > +{ > + int i, ret; > + u8 sr; > + > + /* Check whether we are in the allowed voltage range. */ > + for (i = 100; i > 0; i--) { > + ret = rv3029c2_i2c_get_sr(client, &sr); > + if (ret < 0) > + return ret; > + if (!(sr & (RV3029C2_STATUS_VLOW1 | RV3029C2_STATUS_VLOW2))) > + break; > + > + sr &= ~RV3029C2_STATUS_VLOW1; > + sr &= ~RV3029C2_STATUS_VLOW2; > + ret = rv3029c2_i2c_set_sr(client, sr); > + if (ret < 0) > + return ret; I wouldn't reset the VLOW1 and VLOW2 flags here. Just bail out if the voltage is not sufficient. I don't think that condition will actually get better simply by waiting. Proper VLOW1 and VLOW2 handling for that RTC is something I have in the pipe. > + usleep_range(1000, 10000); > + } > + if (i <= 0) { > + dev_err(&client->dev, "EEPROM voltage wait timeout.\n"); > + return -ETIMEDOUT; > + } > + > + /* Disable eeprom refresh. */ > + ret = rv3029c2_i2c_maskset_reg(client, RV3029C2_ONOFF_CTRL, > + (u8)~RV3029C2_ONOFF_CTRL_EERE, 0); > + if (ret < 0) > + return ret; > + > + /* Wait for previous eeprom accesses to finish. */ > + ret = rv3029c2_eeprom_busywait(client); > + if (ret < 0) > + rv3029c2_eeprom_exit(client); > + > + return ret; > +} > + > +static int rv3029c2_eeprom_read(struct i2c_client *client, u8 reg, > + u8 buf[], size_t len) > +{ > + int ret, err; > + size_t i; > + > + ret = rv3029c2_eeprom_enter(client); > + if (ret < 0) > + return ret; > + > + for (i = 0; i < len; i++) { > + ret = rv3029c2_i2c_read_regs(client, reg, &buf[i], 1); > + if (ret < 0) > + break; > + } > + > + err = rv3029c2_eeprom_exit(client); > + if (err < 0) > + return err; > + > + return ret; > +} > + > +static int rv3029c2_eeprom_write(struct i2c_client *client, u8 reg, > + u8 const buf[], size_t len) > +{ > + int ret, err; > + size_t i; > + u8 tmp; > + > + ret = rv3029c2_eeprom_enter(client); > + if (ret < 0) > + return ret; > + > + for (i = 0; i < len; i++) { > + ret = rv3029c2_i2c_read_regs(client, reg, &tmp, 1); > + if (ret < 0) > + break; > + if (tmp != buf[i]) { > + ret = rv3029c2_i2c_write_regs(client, reg, &buf[i], 1); > + if (ret < 0) > + break; > + } > + ret = rv3029c2_eeprom_busywait(client); > + if (ret < 0) > + break; > + } > + > + err = rv3029c2_eeprom_exit(client); > + if (err < 0) > + return err; > + > + return ret; > +} > + > static int > rv3029c2_i2c_read_time(struct i2c_client *client, struct rtc_time *tm) > { > @@ -230,22 +398,13 @@ static int rv3029c2_rtc_i2c_alarm_set_irq(struct i2c_client *client, > int enable) > { > int ret; > - u8 buf[1]; > - > - /* enable AIE irq */ > - ret = rv3029c2_i2c_read_regs(client, RV3029C2_IRQ_CTRL, buf, 1); > - if (ret < 0) { > - dev_err(&client->dev, "can't read INT reg\n"); > - return ret; > - } > - if (enable) > - buf[0] |= RV3029C2_IRQ_CTRL_AIE; > - else > - buf[0] &= ~RV3029C2_IRQ_CTRL_AIE; > > - ret = rv3029c2_i2c_write_regs(client, RV3029C2_IRQ_CTRL, buf, 1); > + /* enable/disable AIE irq */ > + ret = rv3029c2_i2c_maskset_reg(client, RV3029C2_IRQ_CTRL, > + (u8)~RV3029C2_IRQ_CTRL_AIE, > + (enable ? RV3029C2_IRQ_CTRL_AIE : 0)); > if (ret < 0) { > - dev_err(&client->dev, "can't set INT reg\n"); > + dev_err(&client->dev, "can't update INT reg\n"); I would also put that particular change in a separate patch, with the _update_bits addition. > return ret; > } > > @@ -286,20 +445,11 @@ static int rv3029c2_rtc_i2c_set_alarm(struct i2c_client *client, > return ret; > > if (alarm->enabled) { > - u8 buf[1]; > - > /* clear AF flag */ > - ret = rv3029c2_i2c_read_regs(client, RV3029C2_IRQ_FLAGS, > - buf, 1); > - if (ret < 0) { > - dev_err(&client->dev, "can't read alarm flag\n"); > - return ret; > - } > - buf[0] &= ~RV3029C2_IRQ_FLAGS_AF; > - ret = rv3029c2_i2c_write_regs(client, RV3029C2_IRQ_FLAGS, > - buf, 1); > + ret = rv3029c2_i2c_maskset_reg(client, RV3029C2_IRQ_FLAGS, > + (u8)~RV3029C2_IRQ_FLAGS_AF, 0); > if (ret < 0) { > - dev_err(&client->dev, "can't set alarm flag\n"); > + dev_err(&client->dev, "can't clear alarm flag\n"); Ditto. > return ret; > } > /* enable AIE irq */ > @@ -372,6 +522,109 @@ static int rv3029c2_rtc_set_time(struct device *dev, struct rtc_time *tm) > return rv3029c2_i2c_set_time(to_i2c_client(dev), tm); > } > > +static const struct rv3029c2_trickle_tab_elem { > + u32 r; /* resistance in ohms */ > + u8 conf; /* trickle config bits */ > +} rv3029c2_trickle_tab[] = { > + { > + .r = 1076, > + .conf = RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_5K | > + RV3029C2_TRICKLE_20K | RV3029C2_TRICKLE_80K, > + }, { > + .r = 1091, > + .conf = RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_5K | > + RV3029C2_TRICKLE_20K, > + }, { > + .r = 1137, > + .conf = RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_5K | > + RV3029C2_TRICKLE_80K, > + }, { > + .r = 1154, > + .conf = RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_5K, > + }, { > + .r = 1371, > + .conf = RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_20K | > + RV3029C2_TRICKLE_80K, > + }, { > + .r = 1395, > + .conf = RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_20K, > + }, { > + .r = 1472, > + .conf = RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_80K, > + }, { > + .r = 1500, > + .conf = RV3029C2_TRICKLE_1K, > + }, { > + .r = 3810, > + .conf = RV3029C2_TRICKLE_5K | RV3029C2_TRICKLE_20K | > + RV3029C2_TRICKLE_80K, > + }, { > + .r = 4000, > + .conf = RV3029C2_TRICKLE_5K | RV3029C2_TRICKLE_20K, > + }, { > + .r = 4706, > + .conf = RV3029C2_TRICKLE_5K | RV3029C2_TRICKLE_80K, > + }, { > + .r = 5000, > + .conf = RV3029C2_TRICKLE_5K, > + }, { > + .r = 16000, > + .conf = RV3029C2_TRICKLE_20K | RV3029C2_TRICKLE_80K, > + }, { > + .r = 20000, > + .conf = RV3029C2_TRICKLE_20K, > + }, { > + .r = 80000, > + .conf = RV3029C2_TRICKLE_80K, > + }, > +}; > + > +static int rv3029c2_of_init(struct i2c_client *client) I'd name that function rv3029_trickle_config or so. Other features parsed from DT can be implemented using other functions. > +{ > + struct device_node *of_node = client->dev.of_node; > + const struct rv3029c2_trickle_tab_elem *elem; > + int i, err; > + u32 ohms; > + u8 eectrl; > + > + if (!of_node) > + return 0; > + > + /* Configure the trickle charger. */ > + err = rv3029c2_eeprom_read(client, RV3029C2_CONTROL_E2P_EECTRL, > + &eectrl, 1); > + if (err < 0) { > + dev_err(&client->dev, "Failed to read trickle " > + "charger config\n"); > + return err; > + } > + err = of_property_read_u32(of_node, "trickle-resistor-ohms", &ohms); > + if (err) { > + /* Disable trickle charger. */ > + eectrl &= ~RV3029C2_TRICKLE_MASK; I wouldn't touch it at all, some people may set it in the bootloader and don't expect linux to change the value. > + } else { > + /* Enable trickle charger. */ > + for (i = 0; i < ARRAY_SIZE(rv3029c2_trickle_tab); i++) { > + elem = &rv3029c2_trickle_tab[i]; > + if (elem->r >= ohms) > + break; > + } > + eectrl &= ~RV3029C2_TRICKLE_MASK; > + eectrl |= elem->conf; > + dev_info(&client->dev, "Trickle charger enabled at %d ohms " > + "resistance.\n", elem->r); > + } > + err = rv3029c2_eeprom_write(client, RV3029C2_CONTROL_E2P_EECTRL, > + &eectrl, 1); > + if (err < 0) { > + dev_err(&client->dev, "Failed to write trickle " > + "charger config\n"); Those strings should be only on one line to stay greppable. > + return err; > + } > + > + return 0; > +} > + > static const struct rtc_class_ops rv3029c2_rtc_ops = { > .read_time = rv3029c2_rtc_read_time, > .set_time = rv3029c2_rtc_set_time, > @@ -381,6 +634,7 @@ static const struct rtc_class_ops rv3029c2_rtc_ops = { > > static struct i2c_device_id rv3029c2_id[] = { > { "rv3029c2", 0 }, > + { "rv3029", 0 }, > { } > }; > MODULE_DEVICE_TABLE(i2c, rv3029c2_id); > @@ -401,6 +655,12 @@ static int rv3029c2_probe(struct i2c_client *client, > return rc; > } > > + rc = rv3029c2_of_init(client); > + if (rc < 0) { > + dev_err(&client->dev, "OF init failed.\n"); > + return rc; I don't think this should be a hard failure, the RTC can function without that particular configuration, the previous error message is enough (no need for two messages).
On Tue, 1 Mar 2016 22:36:55 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > The kconfig name is also changed to include the chip number as > > there are other MicroCrystal RTCs. > > > > This should have been another patch and will conflict with my patch > doing exactly that, see rtc-next. Ok, will remove that. > > Also a "rv3029" device ID was added, because the C2 suffix does > > not appear in latest chip versions and datasheet versions. > > > > This change should also be part of another patch. Well, ok. > You may as well stop adding that suffix to newly added functions and > macros. > I'd also take a preliminary patch removing the c2 prefix from current > functions and macros. > > Unfortunately, we can't rename the file because this will break scripts > loading modules. > > > /* Register map */ > > /* control section */ > > #define RV3029C2_ONOFF_CTRL 0x00 > > +#define RV3029C2_ONOFF_CTRL_WE (1 << 0) > > +#define RV3029C2_ONOFF_CTRL_TE (1 << 1) > > +#define RV3029C2_ONOFF_CTRL_TAR (1 << 2) > > +#define RV3029C2_ONOFF_CTRL_EERE (1 << 3) > > +#define RV3029C2_ONOFF_CTRL_SRON (1 << 4) > > +#define RV3029C2_ONOFF_CTRL_TD0 (1 << 5) > > +#define RV3029C2_ONOFF_CTRL_TD1 (1 << 6) > > +#define RV3029C2_ONOFF_CTRL_CLKINT (1 << 7) > > You didn't use --strict for checkpatch, please use BIT() and also > correct the alignments issues. It's what the current code already does. But I can change it all to BIT. > > #define RV3029C2_IRQ_CTRL 0x01 > > #define RV3029C2_IRQ_CTRL_AIE (1 << 0) > > +#define RV3029C2_IRQ_CTRL_TIE (1 << 1) > > +#define RV3029C2_IRQ_CTRL_V1IE (1 << 2) > > +#define RV3029C2_IRQ_CTRL_V2IE (1 << 3) > > +#define RV3029C2_IRQ_CTRL_SRIE (1 << 4) > > I'm pretty sure many you are adding many of those but not using them > yet. Yes. I'm adding all missing register definitions. I didn't really see the need for splitting this further. > > /* user ram section */ > > #define RV3029C2_USR1_RAM_PAGE 0x38 > > @@ -84,6 +111,7 @@ > > #define RV3029C2_USR2_RAM_PAGE 0x3C > > #define RV3029C2_USR2_SECTION_LEN 0x04 > > > > + > > spurious blank line Hm yes. Just adds some space to separate the reg definitions. > > static int > > +rv3029c2_i2c_maskset_reg(struct i2c_client *client, u8 reg, u8 mask, u8 set) > > I'd prefer that function named _update_bits I prefer maskset, because that's just what it does. Mask and set. But well, I'll just change it because it doesn't matter much. > > +{ > > + u8 buf[1]; > > couldn't that be u8 buf? Well, yes. Some other functions do it like this, too. But I'll change it, because I already did it in a sane way for sr and such, too. > > + /* Check whether we are in the allowed voltage range. */ > > + for (i = 100; i > 0; i--) { > > + ret = rv3029c2_i2c_get_sr(client, &sr); > > + if (ret < 0) > > + return ret; > > + if (!(sr & (RV3029C2_STATUS_VLOW1 | RV3029C2_STATUS_VLOW2))) > > + break; > > + > > + sr &= ~RV3029C2_STATUS_VLOW1; > > + sr &= ~RV3029C2_STATUS_VLOW2; > > + ret = rv3029c2_i2c_set_sr(client, sr); > > + if (ret < 0) > > + return ret; > > I wouldn't reset the VLOW1 and VLOW2 flags here. Just bail out if the > voltage is not sufficient. I don't think that condition will actually > get better simply by waiting. I do actually think so that we should clear them at least once (that is retry once). According to the data sheet these bits will not reset automatically and might be left over from machine start (brown out). > > static int > > rv3029c2_i2c_read_time(struct i2c_client *client, struct rtc_time *tm) > > { > > @@ -230,22 +398,13 @@ static int rv3029c2_rtc_i2c_alarm_set_irq(struct i2c_client *client, > > int enable) > > { > > int ret; > > - u8 buf[1]; > > - > > - /* enable AIE irq */ > > - ret = rv3029c2_i2c_read_regs(client, RV3029C2_IRQ_CTRL, buf, 1); > > - if (ret < 0) { > > - dev_err(&client->dev, "can't read INT reg\n"); > > - return ret; > > - } > > - if (enable) > > - buf[0] |= RV3029C2_IRQ_CTRL_AIE; > > - else > > - buf[0] &= ~RV3029C2_IRQ_CTRL_AIE; > > > > - ret = rv3029c2_i2c_write_regs(client, RV3029C2_IRQ_CTRL, buf, 1); > > + /* enable/disable AIE irq */ > > + ret = rv3029c2_i2c_maskset_reg(client, RV3029C2_IRQ_CTRL, > > + (u8)~RV3029C2_IRQ_CTRL_AIE, > > + (enable ? RV3029C2_IRQ_CTRL_AIE : 0)); > > if (ret < 0) { > > - dev_err(&client->dev, "can't set INT reg\n"); > > + dev_err(&client->dev, "can't update INT reg\n"); > > I would also put that particular change in a separate patch, with the > _update_bits addition. > Ditto. Ok. This will require a separate patch that just adds the update_bits function which the tricke change depends on then. > > +static int rv3029c2_of_init(struct i2c_client *client) > > I'd name that function rv3029_trickle_config or so. Other features > parsed from DT can be implemented using other functions. Ok. > > + err = of_property_read_u32(of_node, "trickle-resistor-ohms", &ohms); > > + if (err) { > > + /* Disable trickle charger. */ > > + eectrl &= ~RV3029C2_TRICKLE_MASK; > > I wouldn't touch it at all, some people may set it in the bootloader and > don't expect linux to change the value. But that would mean there is no way to disable it once it got enabled. So I'd rather keep it like this or use special values (like a huge value) like we already discussed. > > + } else { > > + /* Enable trickle charger. */ > > + for (i = 0; i < ARRAY_SIZE(rv3029c2_trickle_tab); i++) { > > + elem = &rv3029c2_trickle_tab[i]; > > + if (elem->r >= ohms) > > + break; > > + } > > + eectrl &= ~RV3029C2_TRICKLE_MASK; > > + eectrl |= elem->conf; > > + dev_info(&client->dev, "Trickle charger enabled at %d ohms " > > + "resistance.\n", elem->r); > > + } > > + err = rv3029c2_eeprom_write(client, RV3029C2_CONTROL_E2P_EECTRL, > > + &eectrl, 1); > > + if (err < 0) { > > + dev_err(&client->dev, "Failed to write trickle " > > + "charger config\n"); > > Those strings should be only on one line to stay greppable. Ok. The line break is a left over from a code reformat. > > + rc = rv3029c2_of_init(client); > > + if (rc < 0) { > > + dev_err(&client->dev, "OF init failed.\n"); > > + return rc; > > I don't think this should be a hard failure, the RTC can function > without that particular configuration, the previous error message is > enough (no need for two messages). Only 50% agree here. :) And RTC without a working backup battery is not really that useful. But I can remove the error checking of course. Thanks for the comments.
On 01/03/2016 at 22:54:01 +0100, Michael Büsch wrote : > > > > + /* Check whether we are in the allowed voltage range. */ > > > + for (i = 100; i > 0; i--) { > > > + ret = rv3029c2_i2c_get_sr(client, &sr); > > > + if (ret < 0) > > > + return ret; > > > + if (!(sr & (RV3029C2_STATUS_VLOW1 | RV3029C2_STATUS_VLOW2))) > > > + break; > > > + > > > + sr &= ~RV3029C2_STATUS_VLOW1; > > > + sr &= ~RV3029C2_STATUS_VLOW2; > > > + ret = rv3029c2_i2c_set_sr(client, sr); > > > + if (ret < 0) > > > + return ret; > > > > I wouldn't reset the VLOW1 and VLOW2 flags here. Just bail out if the > > voltage is not sufficient. I don't think that condition will actually > > get better simply by waiting. > > > I do actually think so that we should clear them at least once (that is > retry once). According to the data sheet these bits will not reset > automatically and might be left over from machine start (brown out). > No, this should be handled differently. I have some pending patches to handle those bits. The main issue is that if you reset those bits there, there is now way to know whether the date has been set or is invalid. I think the best way to handle this case is to call the function again when the date/time are set and we have to reset VLOW1/2. > > > + err = of_property_read_u32(of_node, "trickle-resistor-ohms", &ohms); > > > + if (err) { > > > + /* Disable trickle charger. */ > > > + eectrl &= ~RV3029C2_TRICKLE_MASK; > > > > I wouldn't touch it at all, some people may set it in the bootloader and > > don't expect linux to change the value. > > But that would mean there is no way to disable it once it got enabled. > So I'd rather keep it like this or use special values (like a huge > value) like we already discussed. > Agreed, keep it that way. > > > + rc = rv3029c2_of_init(client); > > > + if (rc < 0) { > > > + dev_err(&client->dev, "OF init failed.\n"); > > > + return rc; > > > > I don't think this should be a hard failure, the RTC can function > > without that particular configuration, the previous error message is > > enough (no need for two messages). > > Only 50% agree here. :) > And RTC without a working backup battery is not really that useful. > But I can remove the error checking of course. > As proposed, I think we can retry to set trickle charging when necessary, when setting the date.
On Wed, 2 Mar 2016 00:07:45 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > I wouldn't reset the VLOW1 and VLOW2 flags here. Just bail out if the > > > voltage is not sufficient. I don't think that condition will actually > > > get better simply by waiting. > > > > > > I do actually think so that we should clear them at least once (that is > > retry once). According to the data sheet these bits will not reset > > automatically and might be left over from machine start (brown out). > > > > No, this should be handled differently. I have some pending patches to > handle those bits. The main issue is that if you reset those bits there, > there is now way to know whether the date has been set or is invalid. > > I think the best way to handle this case is to call the function again > when the date/time are set and we have to reset VLOW1/2. Yes, right. But as of now we do not have VLOW handling. So I don't see an issue with clearing the bits here. The clearing can be removed, if VLOW handling is added eventually. The issue currently is: We need to check VLOW before accessing the eeprom, because otherwise it would mean data corruption. This is especially true for writing. And if we read VLOW, we need a way to reset the bits from the probable initial brown out. Otherwise we can never access the eeprom as this check would hit often. So when are your VLOW patches going to be available?
On 02/03/2016 at 07:26:27 +0100, Michael Büsch wrote : > On Wed, 2 Mar 2016 00:07:45 +0100 > Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > > > I wouldn't reset the VLOW1 and VLOW2 flags here. Just bail out if the > > > > voltage is not sufficient. I don't think that condition will actually > > > > get better simply by waiting. > > > > > > > > > I do actually think so that we should clear them at least once (that is > > > retry once). According to the data sheet these bits will not reset > > > automatically and might be left over from machine start (brown out). > > > > > > > No, this should be handled differently. I have some pending patches to > > handle those bits. The main issue is that if you reset those bits there, > > there is now way to know whether the date has been set or is invalid. > > > > I think the best way to handle this case is to call the function again > > when the date/time are set and we have to reset VLOW1/2. > > Yes, right. But as of now we do not have VLOW handling. So I don't see > an issue with clearing the bits here. > The clearing can be removed, if VLOW handling is added eventually. > > The issue currently is: We need to check VLOW before accessing the > eeprom, because otherwise it would mean data corruption. This is > especially true for writing. And if we read VLOW, we need a way to > reset the bits from the probable initial brown out. Otherwise we can > never access the eeprom as this check would hit often. > Ok, that is fine. > So when are your VLOW patches going to be available? > I'll probably rebase on top of your changes.
This series eventually adds support for the trickle charger mechanism to the MicroCrystal rv3029 RTC driver. This series includes the changes from the first review round. The tricke charger functionality was verified on a rv3029 connected to a RaspberryPi. The resistance between VCC and the battery pin was measured for some 'trickle-resistor-ohms' settings. The series is also available in this git repository: git://git.bues.ch/linux.git rv3029
On 04/03/2016 at 20:02:56 +0100, Michael Büsch wrote : > This series eventually adds support for the trickle charger mechanism > to the MicroCrystal rv3029 RTC driver. > > This series includes the changes from the first review round. > The tricke charger functionality was verified on a rv3029 connected to > a RaspberryPi. The resistance between VCC and the battery pin was > measured for some 'trickle-resistor-ohms' settings. > > The series is also available in this git repository: > This is mostly good. I still have two small comments.
This series eventually adds support for the trickle charger mechanism to the MicroCrystal rv3029 RTC driver. This series includes the changes from the first and second review round. The tricke charger functionality was verified on a rv3029 connected to a RaspberryPi. The resistance between VCC and the battery pin was measured for some 'trickle-resistor-ohms' settings. The series is also available in this git repository: git://git.bues.ch/linux.git rv3029
On 04/03/2016 at 22:36:54 +0100, Michael Büsch wrote : > This series eventually adds support for the trickle charger mechanism > to the MicroCrystal rv3029 RTC driver. > > This series includes the changes from the first and second review round. > > The tricke charger functionality was verified on a rv3029 connected to > a RaspberryPi. The resistance between VCC and the battery pin was > measured for some 'trickle-resistor-ohms' settings. > > The series is also available in this git repository: > > git://git.bues.ch/linux.git rv3029 > All applied, thanks!
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index 376322f..c37f535 100644 --- a/drivers/rtc/Kconfig +++ b/drivers/rtc/Kconfig @@ -595,7 +595,7 @@ config RTC_DRV_EM3027 will be called rtc-em3027. config RTC_DRV_RV3029C2 - tristate "Micro Crystal RTC" + tristate "Micro Crystal RV3029(C2)" help If you say yes here you get support for the Micro Crystal RV3029-C2 RTC chips. diff --git a/drivers/rtc/rtc-rv3029c2.c b/drivers/rtc/rtc-rv3029c2.c index e9ac5a4..036e205 100644 --- a/drivers/rtc/rtc-rv3029c2.c +++ b/drivers/rtc/rtc-rv3029c2.c @@ -2,30 +2,46 @@ * Micro Crystal RV-3029C2 rtc class driver * * Author: Gregory Hermant <gregory.hermant@calao-systems.com> + * Michael Buesch <m@bues.ch> * * based on previously existing rtc class drivers * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as * published by the Free Software Foundation. - * - * NOTE: Currently this driver only supports the bare minimum for read - * and write the RTC and alarms. The extra features provided by this chip - * (trickle charger, eeprom, T° compensation) are unavailable. */ #include <linux/module.h> #include <linux/i2c.h> #include <linux/bcd.h> #include <linux/rtc.h> +#include <linux/delay.h> +#include <linux/of.h> + /* Register map */ /* control section */ #define RV3029C2_ONOFF_CTRL 0x00 +#define RV3029C2_ONOFF_CTRL_WE (1 << 0) +#define RV3029C2_ONOFF_CTRL_TE (1 << 1) +#define RV3029C2_ONOFF_CTRL_TAR (1 << 2) +#define RV3029C2_ONOFF_CTRL_EERE (1 << 3) +#define RV3029C2_ONOFF_CTRL_SRON (1 << 4) +#define RV3029C2_ONOFF_CTRL_TD0 (1 << 5) +#define RV3029C2_ONOFF_CTRL_TD1 (1 << 6) +#define RV3029C2_ONOFF_CTRL_CLKINT (1 << 7) #define RV3029C2_IRQ_CTRL 0x01 #define RV3029C2_IRQ_CTRL_AIE (1 << 0) +#define RV3029C2_IRQ_CTRL_TIE (1 << 1) +#define RV3029C2_IRQ_CTRL_V1IE (1 << 2) +#define RV3029C2_IRQ_CTRL_V2IE (1 << 3) +#define RV3029C2_IRQ_CTRL_SRIE (1 << 4) #define RV3029C2_IRQ_FLAGS 0x02 #define RV3029C2_IRQ_FLAGS_AF (1 << 0) +#define RV3029C2_IRQ_FLAGS_TF (1 << 1) +#define RV3029C2_IRQ_FLAGS_V1IF (1 << 2) +#define RV3029C2_IRQ_FLAGS_V2IF (1 << 3) +#define RV3029C2_IRQ_FLAGS_SRF (1 << 4) #define RV3029C2_STATUS 0x03 #define RV3029C2_STATUS_VLOW1 (1 << 2) #define RV3029C2_STATUS_VLOW2 (1 << 3) @@ -33,6 +49,7 @@ #define RV3029C2_STATUS_PON (1 << 5) #define RV3029C2_STATUS_EEBUSY (1 << 7) #define RV3029C2_RST_CTRL 0x04 +#define RV3029C2_RST_CTRL_SYSR (1 << 4) #define RV3029C2_CONTROL_SECTION_LEN 0x05 /* watch section */ @@ -67,16 +84,26 @@ /* eeprom data section */ #define RV3029C2_E2P_EEDATA1 0x28 #define RV3029C2_E2P_EEDATA2 0x29 +#define RV3029C2_E2PDATA_SECTION_LEN 0x02 /* eeprom control section */ #define RV3029C2_CONTROL_E2P_EECTRL 0x30 -#define RV3029C2_TRICKLE_1K (1<<0) /* 1K resistance */ -#define RV3029C2_TRICKLE_5K (1<<1) /* 5K resistance */ -#define RV3029C2_TRICKLE_20K (1<<2) /* 20K resistance */ -#define RV3029C2_TRICKLE_80K (1<<3) /* 80K resistance */ -#define RV3029C2_CONTROL_E2P_XTALOFFSET 0x31 -#define RV3029C2_CONTROL_E2P_QCOEF 0x32 -#define RV3029C2_CONTROL_E2P_TURNOVER 0x33 +#define RV3029C2_EECTRL_THP (1 << 0) /* temp scan interval */ +#define RV3029C2_EECTRL_THE (1 << 1) /* thermometer enable */ +#define RV3029C2_EECTRL_FD0 (1 << 2) /* CLKOUT */ +#define RV3029C2_EECTRL_FD1 (1 << 3) /* CLKOUT */ +#define RV3029C2_TRICKLE_1K (1 << 4) /* 1.5K resistance */ +#define RV3029C2_TRICKLE_5K (1 << 5) /* 5K resistance */ +#define RV3029C2_TRICKLE_20K (1 << 6) /* 20K resistance */ +#define RV3029C2_TRICKLE_80K (1 << 7) /* 80K resistance */ +#define RV3029C2_TRICKLE_MASK (RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_5K |\ + RV3029C2_TRICKLE_20K | RV3029C2_TRICKLE_80K) +#define RV3029C2_TRICKLE_SHIFT 4 +#define RV3029C2_CONTROL_E2P_XOFFS 0x31 /* XTAL offset */ +#define RV3029C2_CONTROL_E2P_XOFFS_SIGN (1 << 7) /* Sign: 1->pos, 0->neg */ +#define RV3029C2_CONTROL_E2P_QCOEF 0x32 /* XTAL temp drift coef */ +#define RV3029C2_CONTROL_E2P_TURNOVER 0x33 /* XTAL turnover temp (in *C) */ +#define RV3029C2_CONTROL_E2P_TOV_MASK 0x3F /* XTAL turnover temp mask */ /* user ram section */ #define RV3029C2_USR1_RAM_PAGE 0x38 @@ -84,6 +111,7 @@ #define RV3029C2_USR2_RAM_PAGE 0x3C #define RV3029C2_USR2_SECTION_LEN 0x04 + static int rv3029c2_i2c_read_regs(struct i2c_client *client, u8 reg, u8 *buf, unsigned len) @@ -114,6 +142,24 @@ rv3029c2_i2c_write_regs(struct i2c_client *client, u8 reg, u8 const buf[], } static int +rv3029c2_i2c_maskset_reg(struct i2c_client *client, u8 reg, u8 mask, u8 set) +{ + u8 buf[1]; + int ret; + + ret = rv3029c2_i2c_read_regs(client, reg, buf, 1); + if (ret < 0) + return ret; + buf[0] &= mask; + buf[0] |= set; + ret = rv3029c2_i2c_write_regs(client, reg, buf, 1); + if (ret < 0) + return ret; + + return 0; +} + +static int rv3029c2_i2c_get_sr(struct i2c_client *client, u8 *buf) { int ret = rv3029c2_i2c_read_regs(client, RV3029C2_STATUS, buf, 1); @@ -138,6 +184,128 @@ rv3029c2_i2c_set_sr(struct i2c_client *client, u8 val) return 0; } +static int rv3029c2_eeprom_busywait(struct i2c_client *client) +{ + int i, ret; + u8 sr; + + for (i = 100; i > 0; i--) { + ret = rv3029c2_i2c_get_sr(client, &sr); + if (ret < 0) + break; + if (!(sr & RV3029C2_STATUS_EEBUSY)) + break; + usleep_range(1000, 10000); + } + if (i <= 0) { + dev_err(&client->dev, "EEPROM busy wait timeout.\n"); + return -ETIMEDOUT; + } + + return ret; +} + +static int rv3029c2_eeprom_exit(struct i2c_client *client) +{ + /* Re-enable eeprom refresh */ + return rv3029c2_i2c_maskset_reg(client, RV3029C2_ONOFF_CTRL, + 0xFF, RV3029C2_ONOFF_CTRL_EERE); +} + +static int rv3029c2_eeprom_enter(struct i2c_client *client) +{ + int i, ret; + u8 sr; + + /* Check whether we are in the allowed voltage range. */ + for (i = 100; i > 0; i--) { + ret = rv3029c2_i2c_get_sr(client, &sr); + if (ret < 0) + return ret; + if (!(sr & (RV3029C2_STATUS_VLOW1 | RV3029C2_STATUS_VLOW2))) + break; + + sr &= ~RV3029C2_STATUS_VLOW1; + sr &= ~RV3029C2_STATUS_VLOW2; + ret = rv3029c2_i2c_set_sr(client, sr); + if (ret < 0) + return ret; + usleep_range(1000, 10000); + } + if (i <= 0) { + dev_err(&client->dev, "EEPROM voltage wait timeout.\n"); + return -ETIMEDOUT; + } + + /* Disable eeprom refresh. */ + ret = rv3029c2_i2c_maskset_reg(client, RV3029C2_ONOFF_CTRL, + (u8)~RV3029C2_ONOFF_CTRL_EERE, 0); + if (ret < 0) + return ret; + + /* Wait for previous eeprom accesses to finish. */ + ret = rv3029c2_eeprom_busywait(client); + if (ret < 0) + rv3029c2_eeprom_exit(client); + + return ret; +} + +static int rv3029c2_eeprom_read(struct i2c_client *client, u8 reg, + u8 buf[], size_t len) +{ + int ret, err; + size_t i; + + ret = rv3029c2_eeprom_enter(client); + if (ret < 0) + return ret; + + for (i = 0; i < len; i++) { + ret = rv3029c2_i2c_read_regs(client, reg, &buf[i], 1); + if (ret < 0) + break; + } + + err = rv3029c2_eeprom_exit(client); + if (err < 0) + return err; + + return ret; +} + +static int rv3029c2_eeprom_write(struct i2c_client *client, u8 reg, + u8 const buf[], size_t len) +{ + int ret, err; + size_t i; + u8 tmp; + + ret = rv3029c2_eeprom_enter(client); + if (ret < 0) + return ret; + + for (i = 0; i < len; i++) { + ret = rv3029c2_i2c_read_regs(client, reg, &tmp, 1); + if (ret < 0) + break; + if (tmp != buf[i]) { + ret = rv3029c2_i2c_write_regs(client, reg, &buf[i], 1); + if (ret < 0) + break; + } + ret = rv3029c2_eeprom_busywait(client); + if (ret < 0) + break; + } + + err = rv3029c2_eeprom_exit(client); + if (err < 0) + return err; + + return ret; +} + static int rv3029c2_i2c_read_time(struct i2c_client *client, struct rtc_time *tm) { @@ -230,22 +398,13 @@ static int rv3029c2_rtc_i2c_alarm_set_irq(struct i2c_client *client, int enable) { int ret; - u8 buf[1]; - - /* enable AIE irq */ - ret = rv3029c2_i2c_read_regs(client, RV3029C2_IRQ_CTRL, buf, 1); - if (ret < 0) { - dev_err(&client->dev, "can't read INT reg\n"); - return ret; - } - if (enable) - buf[0] |= RV3029C2_IRQ_CTRL_AIE; - else - buf[0] &= ~RV3029C2_IRQ_CTRL_AIE; - ret = rv3029c2_i2c_write_regs(client, RV3029C2_IRQ_CTRL, buf, 1); + /* enable/disable AIE irq */ + ret = rv3029c2_i2c_maskset_reg(client, RV3029C2_IRQ_CTRL, + (u8)~RV3029C2_IRQ_CTRL_AIE, + (enable ? RV3029C2_IRQ_CTRL_AIE : 0)); if (ret < 0) { - dev_err(&client->dev, "can't set INT reg\n"); + dev_err(&client->dev, "can't update INT reg\n"); return ret; } @@ -286,20 +445,11 @@ static int rv3029c2_rtc_i2c_set_alarm(struct i2c_client *client, return ret; if (alarm->enabled) { - u8 buf[1]; - /* clear AF flag */ - ret = rv3029c2_i2c_read_regs(client, RV3029C2_IRQ_FLAGS, - buf, 1); - if (ret < 0) { - dev_err(&client->dev, "can't read alarm flag\n"); - return ret; - } - buf[0] &= ~RV3029C2_IRQ_FLAGS_AF; - ret = rv3029c2_i2c_write_regs(client, RV3029C2_IRQ_FLAGS, - buf, 1); + ret = rv3029c2_i2c_maskset_reg(client, RV3029C2_IRQ_FLAGS, + (u8)~RV3029C2_IRQ_FLAGS_AF, 0); if (ret < 0) { - dev_err(&client->dev, "can't set alarm flag\n"); + dev_err(&client->dev, "can't clear alarm flag\n"); return ret; } /* enable AIE irq */ @@ -372,6 +522,109 @@ static int rv3029c2_rtc_set_time(struct device *dev, struct rtc_time *tm) return rv3029c2_i2c_set_time(to_i2c_client(dev), tm); } +static const struct rv3029c2_trickle_tab_elem { + u32 r; /* resistance in ohms */ + u8 conf; /* trickle config bits */ +} rv3029c2_trickle_tab[] = { + { + .r = 1076, + .conf = RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_5K | + RV3029C2_TRICKLE_20K | RV3029C2_TRICKLE_80K, + }, { + .r = 1091, + .conf = RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_5K | + RV3029C2_TRICKLE_20K, + }, { + .r = 1137, + .conf = RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_5K | + RV3029C2_TRICKLE_80K, + }, { + .r = 1154, + .conf = RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_5K, + }, { + .r = 1371, + .conf = RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_20K | + RV3029C2_TRICKLE_80K, + }, { + .r = 1395, + .conf = RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_20K, + }, { + .r = 1472, + .conf = RV3029C2_TRICKLE_1K | RV3029C2_TRICKLE_80K, + }, { + .r = 1500, + .conf = RV3029C2_TRICKLE_1K, + }, { + .r = 3810, + .conf = RV3029C2_TRICKLE_5K | RV3029C2_TRICKLE_20K | + RV3029C2_TRICKLE_80K, + }, { + .r = 4000, + .conf = RV3029C2_TRICKLE_5K | RV3029C2_TRICKLE_20K, + }, { + .r = 4706, + .conf = RV3029C2_TRICKLE_5K | RV3029C2_TRICKLE_80K, + }, { + .r = 5000, + .conf = RV3029C2_TRICKLE_5K, + }, { + .r = 16000, + .conf = RV3029C2_TRICKLE_20K | RV3029C2_TRICKLE_80K, + }, { + .r = 20000, + .conf = RV3029C2_TRICKLE_20K, + }, { + .r = 80000, + .conf = RV3029C2_TRICKLE_80K, + }, +}; + +static int rv3029c2_of_init(struct i2c_client *client) +{ + struct device_node *of_node = client->dev.of_node; + const struct rv3029c2_trickle_tab_elem *elem; + int i, err; + u32 ohms; + u8 eectrl; + + if (!of_node) + return 0; + + /* Configure the trickle charger. */ + err = rv3029c2_eeprom_read(client, RV3029C2_CONTROL_E2P_EECTRL, + &eectrl, 1); + if (err < 0) { + dev_err(&client->dev, "Failed to read trickle " + "charger config\n"); + return err; + } + err = of_property_read_u32(of_node, "trickle-resistor-ohms", &ohms); + if (err) { + /* Disable trickle charger. */ + eectrl &= ~RV3029C2_TRICKLE_MASK; + } else { + /* Enable trickle charger. */ + for (i = 0; i < ARRAY_SIZE(rv3029c2_trickle_tab); i++) { + elem = &rv3029c2_trickle_tab[i]; + if (elem->r >= ohms) + break; + } + eectrl &= ~RV3029C2_TRICKLE_MASK; + eectrl |= elem->conf; + dev_info(&client->dev, "Trickle charger enabled at %d ohms " + "resistance.\n", elem->r); + } + err = rv3029c2_eeprom_write(client, RV3029C2_CONTROL_E2P_EECTRL, + &eectrl, 1); + if (err < 0) { + dev_err(&client->dev, "Failed to write trickle " + "charger config\n"); + return err; + } + + return 0; +} + static const struct rtc_class_ops rv3029c2_rtc_ops = { .read_time = rv3029c2_rtc_read_time, .set_time = rv3029c2_rtc_set_time, @@ -381,6 +634,7 @@ static const struct rtc_class_ops rv3029c2_rtc_ops = { static struct i2c_device_id rv3029c2_id[] = { { "rv3029c2", 0 }, + { "rv3029", 0 }, { } }; MODULE_DEVICE_TABLE(i2c, rv3029c2_id); @@ -401,6 +655,12 @@ static int rv3029c2_probe(struct i2c_client *client, return rc; } + rc = rv3029c2_of_init(client); + if (rc < 0) { + dev_err(&client->dev, "OF init failed.\n"); + return rc; + } + rtc = devm_rtc_device_register(&client->dev, client->name, &rv3029c2_rtc_ops, THIS_MODULE); @@ -423,5 +683,6 @@ static struct i2c_driver rv3029c2_driver = { module_i2c_driver(rv3029c2_driver); MODULE_AUTHOR("Gregory Hermant <gregory.hermant@calao-systems.com>"); +MODULE_AUTHOR("Michael Buesch <m@bues.ch>"); MODULE_DESCRIPTION("Micro Crystal RV3029C2 RTC driver"); MODULE_LICENSE("GPL");
This adds a device tree property to enable the tricke charger at some to be configured resistance. If the property is left out, the trickle charger is disabled. The kconfig name is also changed to include the chip number as there are other MicroCrystal RTCs. Also a "rv3029" device ID was added, because the C2 suffix does not appear in latest chip versions and datasheet versions. Signed-off-by: Michael Buesch <m@bues.ch> --- drivers/rtc/Kconfig | 2 +- drivers/rtc/rtc-rv3029c2.c | 335 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 299 insertions(+), 38 deletions(-)