[rtc-linux] Re: [PATCH] rtc-rv3029c2: Add trickle charger

["Michael Büsch" <m@bues.ch>]

[-- Attachment #1: Type: text/plain, Size: 7112 bytes --]

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.

-- 
Michael

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]