[PATCH 0/3] rtc: m41t80: set xtal load capacitance from DT

[PATCH 0/3] rtc: m41t80: set xtal load capacitance from DT

[Dennis Lambe Jr]

The m41t82 and m41t83 have an adjustable internal capacitance that 
defaults to 25 pF per xtal pin. This patch series adds the ability to 
configure it via the devicetree.

Patch 1 just switches the CONFIG_OF-dependent block in m41t80_probe() 
from an ifdef guard to an if(IS_ENABLED(...)) guard, so that I don't 
need to use __maybe_used on my new functions and variables.

Patch 2 is the DeviceTree YAML changes.

Patch 3 is the actual added functionality.

The desired capacitance comes from quartz-load-capacitance property, 
following the example of two other RTC ICs that have adjustable internal 
load capacitance, the NXP pcf85063 and pcf8523.

The m41t82 supports much finer-grained control over the capacitance than 
those chips and calls the feature "analog calibration", but it looks to 
me like it's essentially the same kind of thing.

My use case for this is:

ST specifies not to add any additional external load capacitance[1], but 
the MikroElektronika RTC 9 Click board[2] has a 22 pF cap on each xtal 
pin[3]. The resulting combined capacitance appears to be outside of the 
operating range of the xtal, because when power is removed from the 
boards I'm testing with, the RTC reports an Oscillator-Fail flag on the 
next power on.

I found I could work around the problem by reducing the internal load 
capacitance as low as it will go.

I have tested on the VersaLogic Zebra, an NXP i.MX6 ARM. I made sure it 
compiles cleanly on amd64 with CONFIG_OF=no.

References:
[1] https://www.st.com/resource/en/application_note/an3060-applications-guide-for-serial-realtime-clocks-rtcs-stmicroelectronics.pdf
[2] https://www.mikroe.com/rtc-9-click
[3] https://download.mikroe.com/documents/add-on-boards/click/rtc-9/rtc-9-click-schematic-v100.pdf

Dennis Lambe Jr (3):
  rtc: m41t80: probe: use IS_ENABLED for CONFIG_OF
  dt-bindings: m41t80: add xtal load capacitance
  rtc: m41t80: set xtal load capacitance from DT

 .../devicetree/bindings/rtc/st,m41t80.yaml    | 18 ++++
 drivers/rtc/rtc-m41t80.c                      | 84 +++++++++++++++++--
 2 files changed, 94 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH 1/3] rtc: m41t80: probe: use IS_ENABLED for CONFIG_OF

[Dennis Lambe Jr]

The style guide recommends IS_ENABLED rather than ifdef for wrapping
conditional code wherever possible.

Functions that are only called on DeviceTree platforms would otherwise
need to be cluttered up with __maybe_unused, which is especially
undesireable if there's nothing inherently DT-specific about those
functions.

Signed-off-by: Dennis Lambe Jr <dennis@sparkcharge.io>
---
 drivers/rtc/rtc-m41t80.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index 494052dbd39f..f963b76e5fc0 100644
--- a/drivers/rtc/rtc-m41t80.c
+++ b/drivers/rtc/rtc-m41t80.c
@@ -909,10 +909,11 @@ static int m41t80_probe(struct i2c_client *client)
 	if (IS_ERR(m41t80_data->rtc))
 		return PTR_ERR(m41t80_data->rtc);
 
-#ifdef CONFIG_OF
-	wakeup_source = of_property_read_bool(client->dev.of_node,
-					      "wakeup-source");
-#endif
+	if (IS_ENABLED(CONFIG_OF)) {
+		wakeup_source = of_property_read_bool(client->dev.of_node,
+						      "wakeup-source");
+	}
+
 	if (client->irq > 0) {
 		rc = devm_request_threaded_irq(&client->dev, client->irq,
 					       NULL, m41t80_handle_irq,
-- 
2.25.1

[PATCH 2/3] dt-bindings: m41t80: add xtal load capacitance

[Dennis Lambe Jr]

The ST m41t82 and m41t83 support programmable load capacitance from 3.5
pF to 17.4 pF. The hardware defaults to 12.5 pF.

The accuracy of the xtal can be calibrated precisely by adjusting the
load capacicance.

Add default, minimum, and maximum for the standard rtc property
quartz-load-femtofarads on compatible devices.

Signed-off-by: Dennis Lambe Jr <dennis@sparkcharge.io>
---
 .../devicetree/bindings/rtc/st,m41t80.yaml     | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/st,m41t80.yaml b/Documentation/devicetree/bindings/rtc/st,m41t80.yaml
index fc9c6da6483f..6b72580dc031 100644
--- a/Documentation/devicetree/bindings/rtc/st,m41t80.yaml
+++ b/Documentation/devicetree/bindings/rtc/st,m41t80.yaml
@@ -33,6 +33,11 @@ properties:
   "#clock-cells":
     const: 1
 
+  quartz-load-femtofarads:
+    default: 12500
+    minimum: 3500
+    maximum: 17375
+
   clock-output-names:
     maxItems: 1
     description: From common clock binding to override the default output clock name.
@@ -44,8 +49,21 @@ properties:
       clock-frequency:
         const: 32768
 
+  wakeup-source: true
+
 allOf:
   - $ref: rtc.yaml
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - st,m41t82
+                - st,m41t83
+    then:
+      properties:
+        quartz-load-femtofarads: false
 
 unevaluatedProperties: false
 
-- 
2.25.1

[PATCH 3/3] rtc: m41t80: set xtal load capacitance from DT

[Dennis Lambe Jr]

Add support for specifying the xtal load capacitance in the DT node for
devices with an Analog Calibration register.

the m41t82 and m41t83 support xtal load capacitance from 3.5 pF to 17.4
pF.

If no xtal load capacitance is specified, the battery-backed register
won't be modified. The hardware defaults to 12.5 pF on reset.

Signed-off-by: Dennis Lambe Jr <dennis@sparkcharge.io>
---
 drivers/rtc/rtc-m41t80.c | 75 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 71 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index f963b76e5fc0..85bde7130a4d 100644
--- a/drivers/rtc/rtc-m41t80.c
+++ b/drivers/rtc/rtc-m41t80.c
@@ -44,12 +44,17 @@
 #define M41T80_REG_ALARM_MIN	0x0d
 #define M41T80_REG_ALARM_SEC	0x0e
 #define M41T80_REG_FLAGS	0x0f
+#define M41T80_REG_AC		0x12
 #define M41T80_REG_SQW		0x13
 
 #define M41T80_DATETIME_REG_SIZE	(M41T80_REG_YEAR + 1)
 #define M41T80_ALARM_REG_SIZE	\
 	(M41T80_REG_ALARM_SEC + 1 - M41T80_REG_ALARM_MON)
 
+#define M41T80_AC_MIN		 3500
+#define M41T80_AC_MAX		17375
+#define M41T80_AC_DEFAULT	12500
+
 #define M41T80_SQW_MAX_FREQ	32768
 
 #define M41T80_SEC_ST		BIT(7)	/* ST: Stop Bit */
@@ -68,6 +73,7 @@
 #define M41T80_FEATURE_SQ	BIT(2)	/* Squarewave feature */
 #define M41T80_FEATURE_WD	BIT(3)	/* Extra watchdog resolution */
 #define M41T80_FEATURE_SQ_ALT	BIT(4)	/* RSx bits are in reg 4 */
+#define M41T80_FEATURE_AC	BIT(5) /* Analog calibration */
 
 static const struct i2c_device_id m41t80_id[] = {
 	{ "m41t62", M41T80_FEATURE_SQ | M41T80_FEATURE_SQ_ALT },
@@ -75,8 +81,10 @@ static const struct i2c_device_id m41t80_id[] = {
 	{ "m41t80", M41T80_FEATURE_SQ },
 	{ "m41t81", M41T80_FEATURE_HT | M41T80_FEATURE_SQ},
 	{ "m41t81s", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
-	{ "m41t82", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
-	{ "m41t83", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
+	{ "m41t82", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ
+		    | M41T80_FEATURE_AC },
+	{ "m41t83", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ
+		    | M41T80_FEATURE_AC },
 	{ "m41st84", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
 	{ "m41st85", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
 	{ "m41st87", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
@@ -108,11 +116,13 @@ static const __maybe_unused struct of_device_id m41t80_of_match[] = {
 	},
 	{
 		.compatible = "st,m41t82",
-		.data = (void *)(M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ)
+		.data = (void *)(M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ
+				 | M41T80_FEATURE_AC)
 	},
 	{
 		.compatible = "st,m41t83",
-		.data = (void *)(M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ)
+		.data = (void *)(M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ
+				 | M41T80_FEATURE_AC)
 	},
 	{
 		.compatible = "st,m41t84",
@@ -405,6 +415,54 @@ static const struct rtc_class_ops m41t80_rtc_ops = {
 	.alarm_irq_enable = m41t80_alarm_irq_enable,
 };
 
+static u8 to_sign_magnitude_u8(int n)
+{
+	if (n < 0)
+		return 0x80 | -n;
+	return n;
+}
+
+static int m41t80_encode_ac(int quartz_load)
+{
+	if (quartz_load < M41T80_AC_MIN || quartz_load > M41T80_AC_MAX)
+		return -EINVAL;
+
+	/*
+	 * register representation is the per-capacitor offset from its default
+	 * value in units of 1/4 pF, in sign-magnitude form.
+	 */
+	return to_sign_magnitude_u8((quartz_load - M41T80_AC_DEFAULT) / 125);
+}
+
+static int m41t80_set_ac(struct m41t80_data *m41t80_data, int quartz_load)
+{
+	struct i2c_client *client = m41t80_data->client;
+	struct device *dev = &client->dev;
+	int ret;
+	int ac;
+
+	if (!(m41t80_data->features & M41T80_FEATURE_AC)) {
+		dev_err(dev, "analog calibration requested but not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	ac = m41t80_encode_ac(quartz_load);
+	if (ac < 0) {
+		dev_err(dev, "quartz load %d fF out of range\n",
+			quartz_load);
+		return ac;
+	}
+
+	ret = i2c_smbus_write_byte_data(client, M41T80_REG_AC, ac);
+	if (ret < 0) {
+		dev_err(dev, "Can't set AC register\n");
+		return ret;
+	}
+
+	dev_info(dev, "quartz load set to %d fF (AC=0x%x)\n", quartz_load, ac);
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int m41t80_suspend(struct device *dev)
 {
@@ -883,6 +941,7 @@ static int m41t80_probe(struct i2c_client *client)
 	struct rtc_time tm;
 	struct m41t80_data *m41t80_data = NULL;
 	bool wakeup_source = false;
+	u32 quartz_load = M41T80_AC_DEFAULT;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK |
 				     I2C_FUNC_SMBUS_BYTE_DATA)) {
@@ -912,6 +971,14 @@ static int m41t80_probe(struct i2c_client *client)
 	if (IS_ENABLED(CONFIG_OF)) {
 		wakeup_source = of_property_read_bool(client->dev.of_node,
 						      "wakeup-source");
+
+		rc = of_property_read_u32(client->dev.of_node,
+					  "quartz-load-femtofarads",
+					  &quartz_load);
+		if (!rc)
+			m41t80_set_ac(m41t80_data, quartz_load);
+		else if (rc != -EINVAL)
+			dev_err(&client->dev, "quartz-load-femtofarads property value is missing or invalid\n");
 	}
 
 	if (client->irq > 0) {
-- 
2.25.1

Re: [PATCH 2/3] dt-bindings: m41t80: add xtal load capacitance

[Krzysztof Kozlowski]

On 19/12/2022 20:09, Dennis Lambe Jr wrote:
> The ST m41t82 and m41t83 support programmable load capacitance from 3.5
> pF to 17.4 pF. The hardware defaults to 12.5 pF.
> 
> The accuracy of the xtal can be calibrated precisely by adjusting the
> load capacicance.
> 
> Add default, minimum, and maximum for the standard rtc property
> quartz-load-femtofarads on compatible devices.
> 
> Signed-off-by: Dennis Lambe Jr <dennis@sparkcharge.io>
> ---
>  .../devicetree/bindings/rtc/st,m41t80.yaml     | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/st,m41t80.yaml b/Documentation/devicetree/bindings/rtc/st,m41t80.yaml
> index fc9c6da6483f..6b72580dc031 100644
> --- a/Documentation/devicetree/bindings/rtc/st,m41t80.yaml
> +++ b/Documentation/devicetree/bindings/rtc/st,m41t80.yaml
> @@ -33,6 +33,11 @@ properties:
>    "#clock-cells":
>      const: 1
>  
> +  quartz-load-femtofarads:
> +    default: 12500
> +    minimum: 3500
> +    maximum: 17375
> +
>    clock-output-names:
>      maxItems: 1
>      description: From common clock binding to override the default output clock name.
> @@ -44,8 +49,21 @@ properties:
>        clock-frequency:
>          const: 32768
>  
> +  wakeup-source: true

Why do you need it here? It's already accepted in rtc.yaml. Adding it is
not explained in commit msg.

> +
>  allOf:


Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: m41t80: add xtal load capacitance

[Dennis Lambe]

On Tue, Dec 20, 2022 at 5:52 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> > +  wakeup-source: true
>
> Why do you need it here? It's already accepted in rtc.yaml. Adding it is
> not explained in commit msg.

This shouldn't have been included in the patch, my mistake. In the
next revision of the patch series I'll take this line out. Sorry about
that, thanks for catching it.

Aside from that, does this patch look good to you? It passed
dt_binding_check and reflects my understanding of the logic needed,
but dt-binding yaml isn't something I'm super familiar with,
especially the if: not: properties: contains: enum block. The
intention is to indicate that the `quartz-load-femtofarads` property
is only applicable to the m41t82 and m41t83, but not the other devices
supported by the rtc-m41t80 driver.
-- 

Dennis Lambe (He/Him)
Lead Firmware Engineer
sparkcharge.io