[2/2] rtc: abx80x: Add support for autocalibration filter capacitor

All of the parts supported by this driver can make use of a
small capacitor to improve the accuracy of the autocalibration
process for their RC oscillators. If a capacitor is connected,
a configuration register must be set to enable its use, so a
new Device Tree property has been added for that purpose.

Signed-off-by: Kevin P. Fleming <kevin+linux@km6g.us>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Rob Herring <robh+dt@kernel.org>
To: linux-rtc@vger.kernel.org
To: devicetree@vger.kernel.org
---
 drivers/rtc/rtc-abx80x.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

On 30/05/2020 08:49:00-0400, Kevin P. Fleming wrote:
> All of the parts supported by this driver can make use of a
> small capacitor to improve the accuracy of the autocalibration
> process for their RC oscillators. If a capacitor is connected,
> a configuration register must be set to enable its use, so a
> new Device Tree property has been added for that purpose.
> 
> Signed-off-by: Kevin P. Fleming <kevin+linux@km6g.us>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> To: linux-rtc@vger.kernel.org
> To: devicetree@vger.kernel.org
> ---
>  drivers/rtc/rtc-abx80x.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 3521d8e8dc38..be5a814e8c0b 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
> @@ -76,6 +76,9 @@
>  #define ABX8XX_CFG_KEY_OSC	0xa1
>  #define ABX8XX_CFG_KEY_MISC	0x9d
>  
> +#define ABX8XX_REG_AFCTRL	0x26
> +#define ABX8XX_AUTOCAL_FILTER_ENABLE	0xa0
> +
>  #define ABX8XX_REG_ID0		0x28
>  
>  #define ABX8XX_REG_OUT_CTRL	0x30
> @@ -130,6 +133,31 @@ static int abx80x_is_rc_mode(struct i2c_client *client)
>  	return (flags & ABX8XX_OSS_OMODE) ? 1 : 0;
>  }
>  
> +static int abx80x_enable_autocal_filter(struct i2c_client *client)
> +{
> +	int err;
> +
> +	/*
> +	 * Write the configuration key register to enable access to the AFCTRL
> +	 * register
> +	 */
> +	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_CFG_KEY,
> +					ABX8XX_CFG_KEY_MISC);
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write configuration key\n");
> +		return -EIO;
> +	}

I'd like to avoid having more error messages in the driver (and whole
subsystem). Can you move the ABX8XX_REG_CFG_KEY setting earlier in
abx80x_probe so you don't have to do it here and avoid duplication the
error message?

This would also make the separate function superfluous.

> +
> +	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_AFCTRL,
> +					ABX8XX_AUTOCAL_FILTER_ENABLE);
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write autocal filter register\n");
> +		return -EIO;

The RTC can still work if this fails and the rror is transient, maybe
just warn and continue. It will be set on the next probe.


> +	}
> +
> +	return 0;
> +}
> +
>  static int abx80x_enable_trickle_charger(struct i2c_client *client,
>  					 u8 trickle_cfg)
>  {
> @@ -825,6 +853,12 @@ static int abx80x_probe(struct i2c_client *client,
>  			return err;
>  	}
>  
> +	if (of_property_read_bool(np, "abracon,autocal_filter")) {
> +		err = abx80x_enable_autocal_filter(client);
> +		if (err)
> +			return err;
> +	}
> +
>  	if (client->irq > 0) {
>  		dev_info(&client->dev, "IRQ %d supplied\n", client->irq);
>  		err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> -- 
> 2.26.2
>

On Wed, Jun 10, 2020 at 11:22 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> I'd like to avoid having more error messages in the driver (and whole
> subsystem). Can you move the ABX8XX_REG_CFG_KEY setting earlier in
> abx80x_probe so you don't have to do it here and avoid duplication the
> error message?
>

Based on my reading of the app manual this won't work properly, as
setting the configuration key only allows writing to one register, and
then the key is reset. It has to be set to allow enabling the trickle
charger, and also to allow enabling the autocalibration filter
capacitor.

> The RTC can still work if this fails and the rror is transient, maybe
> just warn and continue. It will be set on the next probe.

Will fix in the next version of the patch.

Thanks for the review!

On 12/06/2020 07:55:53-0400, Kevin P. Fleming wrote:
> On Wed, Jun 10, 2020 at 11:22 AM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> > I'd like to avoid having more error messages in the driver (and whole
> > subsystem). Can you move the ABX8XX_REG_CFG_KEY setting earlier in
> > abx80x_probe so you don't have to do it here and avoid duplication the
> > error message?
> >
> 
> Based on my reading of the app manual this won't work properly, as
> setting the configuration key only allows writing to one register, and
> then the key is reset. It has to be set to allow enabling the trickle
> charger, and also to allow enabling the autocalibration filter
> capacitor.
> 

That is correct and I forgot about that. Can you move just setting the
key to a function as a preliminary patch?

> > The RTC can still work if this fails and the rror is transient, maybe
> > just warn and continue. It will be set on the next probe.
> 
> Will fix in the next version of the patch.
> 
> Thanks for the review!