[PATCH] rtc: abx80x: Add support for autocalibration filter capacitor

[PATCH] rtc: abx80x: Add support for autocalibration filter capacitor

[Kevin P. Fleming]

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
---
 .../bindings/rtc/abracon,abx80x.txt           |  6 ++++
 drivers/rtc/rtc-abx80x.c                      | 34 +++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
index 2405e35a1bc0..ad5d59ed6f24 100644
--- a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
+++ b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
@@ -29,3 +29,9 @@ and valid to enable charging:
  - "abracon,tc-diode": should be "standard" (0.6V) or "schottky" (0.3V)
  - "abracon,tc-resistor": should be <0>, <3>, <6> or <11>. 0 disables the output
                           resistor, the other values are in kOhm.
+
+All of the devices can have a 47pf capacitor attached to increase the
+autocalibration accuracy of their RC oscillators. To enable usage of the
+capacitor the following property has to be defined:
+
+ - "abracon,autocal-filter"
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;
+	}
+
+	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;
+	}
+
+	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

Re: [PATCH] rtc: abx80x: Add support for autocalibration filter capacitor

[Kevin P. Fleming]

Sorry, this was sent prematurely and should have been split into two
patches. It has been resent properly.

On Sat, May 30, 2020 at 8:32 AM Kevin P. Fleming <kevin+linux@km6g.us> 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
> ---
>  .../bindings/rtc/abracon,abx80x.txt           |  6 ++++
>  drivers/rtc/rtc-abx80x.c                      | 34 +++++++++++++++++++
>  2 files changed, 40 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
> index 2405e35a1bc0..ad5d59ed6f24 100644
> --- a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
> +++ b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
> @@ -29,3 +29,9 @@ and valid to enable charging:
>   - "abracon,tc-diode": should be "standard" (0.6V) or "schottky" (0.3V)
>   - "abracon,tc-resistor": should be <0>, <3>, <6> or <11>. 0 disables the output
>                            resistor, the other values are in kOhm.
> +
> +All of the devices can have a 47pf capacitor attached to increase the
> +autocalibration accuracy of their RC oscillators. To enable usage of the
> +capacitor the following property has to be defined:
> +
> + - "abracon,autocal-filter"
> 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;
> +       }
> +
> +       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;
> +       }
> +
> +       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
>

Re: [PATCH] rtc: abx80x: Add support for autocalibration filter capacitor

[Rob Herring]

On Sat, May 30, 2020 at 08:32:22AM -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
> ---
>  .../bindings/rtc/abracon,abx80x.txt           |  6 ++++
>  drivers/rtc/rtc-abx80x.c                      | 34 +++++++++++++++++++
>  2 files changed, 40 insertions(+)

Binding should be a separate patch?

> 
> diff --git a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
> index 2405e35a1bc0..ad5d59ed6f24 100644
> --- a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
> +++ b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt
> @@ -29,3 +29,9 @@ and valid to enable charging:
>   - "abracon,tc-diode": should be "standard" (0.6V) or "schottky" (0.3V)
>   - "abracon,tc-resistor": should be <0>, <3>, <6> or <11>. 0 disables the output
>                            resistor, the other values are in kOhm.
> +
> +All of the devices can have a 47pf capacitor attached to increase the
> +autocalibration accuracy of their RC oscillators. To enable usage of the
> +capacitor the following property has to be defined:
> +
> + - "abracon,autocal-filter"

Can't the standard 'quartz-load-femtofarads' property be used here? You 
might not need to know the value, but presence of the property can 
enable the feature.

Rob

Re: [PATCH] rtc: abx80x: Add support for autocalibration filter capacitor

[Kevin P. Fleming]

On Tue, Jun 9, 2020 at 6:14 PM Rob Herring <robh@kernel.org> wrote:
> > ---
> >  .../bindings/rtc/abracon,abx80x.txt           |  6 ++++
> >  drivers/rtc/rtc-abx80x.c                      | 34 +++++++++++++++++++
> >  2 files changed, 40 insertions(+)
>
> Binding should be a separate patch?

Indeed, it was re-sent with the patches separated.

> > +All of the devices can have a 47pf capacitor attached to increase the
> > +autocalibration accuracy of their RC oscillators. To enable usage of the
> > +capacitor the following property has to be defined:
> > +
> > + - "abracon,autocal-filter"
>
> Can't the standard 'quartz-load-femtofarads' property be used here? You
> might not need to know the value, but presence of the property can
> enable the feature.

On these devices the capacitor is connected to the RC oscillator, not
the crystal oscillator, so this property is controlling a different
function. I'm certainly open to suggestions for different names for
the property if that is desired.

Re: [PATCH] rtc: abx80x: Add support for autocalibration filter capacitor

[Alexandre Belloni]

Hi,

On 09/06/2020 18:23:48-0400, Kevin P. Fleming wrote:
> On Tue, Jun 9, 2020 at 6:14 PM Rob Herring <robh@kernel.org> wrote:
> > > ---
> > >  .../bindings/rtc/abracon,abx80x.txt           |  6 ++++
> > >  drivers/rtc/rtc-abx80x.c                      | 34 +++++++++++++++++++
> > >  2 files changed, 40 insertions(+)
> >
> > Binding should be a separate patch?
> 
> Indeed, it was re-sent with the patches separated.
> 
> > > +All of the devices can have a 47pf capacitor attached to increase the
> > > +autocalibration accuracy of their RC oscillators. To enable usage of the
> > > +capacitor the following property has to be defined:
> > > +
> > > + - "abracon,autocal-filter"
> >
> > Can't the standard 'quartz-load-femtofarads' property be used here? You
> > might not need to know the value, but presence of the property can
> > enable the feature.
> 
> On these devices the capacitor is connected to the RC oscillator, not
> the crystal oscillator, so this property is controlling a different
> function. I'm certainly open to suggestions for different names for
> the property if that is desired.

I agree that this is something different, quartz-load-femtofarads is
about asking the RTC to put the correct load depending on the crystal
populated on the board. Here, you want to indicate the presence or
absence of a filter capacitor.

When working with RTCs, there is one issue though: boolean properties
are not working well because there is no way to express the 3 different
conditions:
 1/ the capacitor is present, set the register
 2/ the capacitor is absent, clear the register
 3/ the device tree didn't have this property until not and the register
   may have been set or cleared using another mean, don't touch it.

As your patch is written, it only handles 1 and 3 which is probably the
safest option but then we will never have a way to clear it from the
driver. I'd say that this is not an issue but it is also something we
will never be able to change without breaking some setups.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] rtc: abx80x: Add support for autocalibration filter capacitor

[Kevin P. Fleming]

On Wed, Jun 10, 2020 at 11:16 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> When working with RTCs, there is one issue though: boolean properties
> are not working well because there is no way to express the 3 different
> conditions:
>  1/ the capacitor is present, set the register
>  2/ the capacitor is absent, clear the register
>  3/ the device tree didn't have this property until not and the register
>    may have been set or cleared using another mean, don't touch it.
>
> As your patch is written, it only handles 1 and 3 which is probably the
> safest option but then we will never have a way to clear it from the
> driver. I'd say that this is not an issue but it is also something we
> will never be able to change without breaking some setups.

I agree. I could implement this as an enumerated string option which
accepts 'yes' or 'no'. Those would cover cases 1 and 2, and the
absence of the property would be case 3. I looked through the bindings
that exist and didn't see any examples of properties configured this
way, but I think it would be understandable to users.

Re: [PATCH] rtc: abx80x: Add support for autocalibration filter capacitor

[Rob Herring]

On Fri, Jun 12, 2020 at 5:48 AM Kevin P. Fleming <kevin+linux@km6g.us> wrote:
>
> On Wed, Jun 10, 2020 at 11:16 AM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> > When working with RTCs, there is one issue though: boolean properties
> > are not working well because there is no way to express the 3 different
> > conditions:
> >  1/ the capacitor is present, set the register
> >  2/ the capacitor is absent, clear the register
> >  3/ the device tree didn't have this property until not and the register
> >    may have been set or cleared using another mean, don't touch it.
> >
> > As your patch is written, it only handles 1 and 3 which is probably the
> > safest option but then we will never have a way to clear it from the
> > driver. I'd say that this is not an issue but it is also something we
> > will never be able to change without breaking some setups.
>
> I agree. I could implement this as an enumerated string option which
> accepts 'yes' or 'no'. Those would cover cases 1 and 2, and the
> absence of the property would be case 3. I looked through the bindings
> that exist and didn't see any examples of properties configured this
> way, but I think it would be understandable to users.

It's a solved problem:

foo-feature = 0; //disable
foo-feature = 1; //enable
<no prop> // Leave feature configured as-is

Rob