[PATCH v3 0/3] rtc: Set M41T82 & M41T83 xtal load capacitance from DT

[PATCH v3 0/3] rtc: Set M41T82 & M41T83 xtal load capacitance from DT

[Dennis Lambe Jr]

Other than adding a sign-off to one of the changelogs, this is a RESEND.

Alexandre Belloni, what do you need for this before you'd want to apply
it? In case it's additional reviewers, I have CC'd some more
potentially-interested parties this time and updated Atsushi Nemoto's
email address to one that's hopefully more current.

I think the original author listed in the header for this driver,
Alexander Bigga, is inaccurate. It looks to me like his name got copied
over by Atsushi Nemoto when he created m41t82.c by deriving it from a
similar driver. At any rate, Alexander Bigga's listed email address
bounces, I didn't find a newer one for him, and he doesn't show up in
the kernel commit log after 2007. I don't think he can be considered the
maintainer for this driver anymore if he ever was.

Changes in v3:
* dt-bindings: added Krzysztof Kozlowski sign-off to changelog

Changes in v2:
* dt-bindings: remove accidental wakeup-sources line
    suggested by Krzysztof Kozlowski
* spelling fixes in changelogs

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 changes `#ifdef CONFIG_OF` to `if (IS_ENABLED(CONFIG_OF))`
in m41t80_probe() so that I don't need to use __maybe_unused on my new
functions and variables.

Patch 2 is the dt-bindings.

Patch 3 is the actual feature implementation.

The desired capacitance comes from the quartz-load-femtofarads property,
following the example of two other RTC ICs that have adjustable internal
load capacitance, the NXP pcf85063 and pcf8523. The m41t82 and m41t83
support much finer-grained control over the capacitance than those
chips, and ST 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.

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

Previous versions:
v1: https://lore.kernel.org/linux-rtc/20221219190915.3912384-1-dennis@sparkcharge.io/T/

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    | 16 ++++
 drivers/rtc/rtc-m41t80.c                      | 84 +++++++++++++++++--
 2 files changed, 92 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH v3 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
undesirable if there's nothing inherently DT-specific about those
functions.

Signed-off-by: Dennis Lambe Jr <dennis@sparkcharge.io>
---

Notes:
    v1 -> v2: spelling fix in changelog

 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 v3 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 capacitance.

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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---

Notes:
    v2 -> v3:
    	added "Reviewed-by: Krzysztof Kozlowski" to changelog
    
    v1 -> v2:
    	remove accidental wakeup-sources line
    		suggested by Krzysztof Kozlowski
    	spelling fix in changelog

 .../devicetree/bindings/rtc/st,m41t80.yaml       | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/st,m41t80.yaml b/Documentation/devicetree/bindings/rtc/st,m41t80.yaml
index fc9c6da6483f..6673adf6e99b 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.
@@ -46,6 +51,17 @@ properties:
 
 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 v3 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 v3 0/3] rtc: Set M41T82 & M41T83 xtal load capacitance from DT

[Alexandre Belloni]

On 19/01/2023 21:39:00+0000, Dennis Lambe Jr wrote:
> Other than adding a sign-off to one of the changelogs, this is a RESEND.
> 
> Alexandre Belloni, what do you need for this before you'd want to apply
> it? In case it's additional reviewers, I have CC'd some more
> potentially-interested parties this time and updated Atsushi Nemoto's
> email address to one that's hopefully more current.
> 

I need to find time to think about it because while setting the analog
trimming statically from the device tree solves your immediate problem,
it will also remove the possibility to handle it from userspace later
on. I would really prefer this uses the offset interface or a better
interface that unfortunately doesn't exist yet.

> I think the original author listed in the header for this driver,
> Alexander Bigga, is inaccurate. It looks to me like his name got copied
> over by Atsushi Nemoto when he created m41t82.c by deriving it from a
> similar driver. At any rate, Alexander Bigga's listed email address
> bounces, I didn't find a newer one for him, and he doesn't show up in
> the kernel commit log after 2007. I don't think he can be considered the
> maintainer for this driver anymore if he ever was.
> 
> Changes in v3:
> * dt-bindings: added Krzysztof Kozlowski sign-off to changelog
> 
> Changes in v2:
> * dt-bindings: remove accidental wakeup-sources line
>     suggested by Krzysztof Kozlowski
> * spelling fixes in changelogs
> 
> 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 changes `#ifdef CONFIG_OF` to `if (IS_ENABLED(CONFIG_OF))`
> in m41t80_probe() so that I don't need to use __maybe_unused on my new
> functions and variables.
> 
> Patch 2 is the dt-bindings.
> 
> Patch 3 is the actual feature implementation.
> 
> The desired capacitance comes from the quartz-load-femtofarads property,
> following the example of two other RTC ICs that have adjustable internal
> load capacitance, the NXP pcf85063 and pcf8523. The m41t82 and m41t83
> support much finer-grained control over the capacitance than those
> chips, and ST 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.
> 
> 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
> 
> Previous versions:
> v1: https://lore.kernel.org/linux-rtc/20221219190915.3912384-1-dennis@sparkcharge.io/T/
> 
> 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    | 16 ++++
>  drivers/rtc/rtc-m41t80.c                      | 84 +++++++++++++++++--
>  2 files changed, 92 insertions(+), 8 deletions(-)
> 
> -- 
> 2.25.1
> 

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

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

[Alexandre Belloni]

On 19/01/2023 21:39:01+0000, Dennis Lambe Jr wrote:
> 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
> undesirable if there's nothing inherently DT-specific about those
> functions.
> 
> Signed-off-by: Dennis Lambe Jr <dennis@sparkcharge.io>
> ---
> 
> Notes:
>     v1 -> v2: spelling fix in changelog
> 
>  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");
> +	}
> +

A way better patch would switch to fwnode_property_read_bool

>  	if (client->irq > 0) {
>  		rc = devm_request_threaded_irq(&client->dev, client->irq,
>  					       NULL, m41t80_handle_irq,
> -- 
> 2.25.1
> 

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

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

[Dennis Lambe]

On Thu, Jan 19, 2023 at 5:21 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:

> > -#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");
> > +     }
> > +
>
> A way better patch would switch to fwnode_property_read_bool

If you like that better, I'll make sure that's how I do it in future
revs of the patchset. I didn't know if it was appropriate since I
don't know if it would ever make sense to call acpi_dev_prop_get on
"wakeup-source" or "quartz-load-femtofarads", or if that kind of
consideration should even matter when choosing to use fwnode_* instead
of of_*.
-- 
Dennis Lambe (He/Him)
Lead Firmware Engineer
sparkcharge.io

Re: [PATCH v3 0/3] rtc: Set M41T82 & M41T83 xtal load capacitance from DT

[Dennis Lambe]

On Thu, Jan 19, 2023 at 5:18 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:

> I need to find time to think about it because while setting the analog
> trimming statically from the device tree solves your immediate problem,
> it will also remove the possibility to handle it from userspace later
> on. I would really prefer this uses the offset interface or a better
> interface that unfortunately doesn't exist yet.

Thanks for letting me know what you're thinking about this. I think I
see what you're getting at.

However, I think this is more complex than either of us had
considered. The M41T82 has two different calibration capabilities:

1. Digital calibration. This looks to me like it behaves similarly to
the digital calibration feature of the M41T00, which ds1307.c exposes
through the offset interface. The M41T8x driver doesn't currently
expose the digital calibration register at all, but if it did I would
agree that the offset interface looks appropriate.

2. Analog calibration -- that's what the datasheet calls it, but the
range on it is very big -- 3.5 pF all the way up to 17.4 pF -- and
their reference design uses it as the only xtal load capacitance in
the circuit. Most of the values you could set for this would be wildly
inappropriate for any given design's choice of xtal oscillator.

Between these, I don't know if you'd want to expose just one, the
other, or some synthesis of both via the offset interface or some new
interface.

I'd make the case that the xtal's required load capacitance is a
hardware requirement that's appropriate to configure via the Device
Tree. Even if you did want to allow some amount of runtime fine-tuning
of this register, you'd still want to document a rational starting
value chosen based on the hardware.

I agree with you, though, that if a runtime fine-tuning feature were
added, we'd have to find a way to choose whether to initialize the
register on boot or not, so that we didn't overwrite the fine-tuning.

Just to demonstrate something that could work, and would be
backward-compatible with this patchset, here's a hypothetical design:
* dt-bindings: add quartz-load-femtofarad-tuning-min and
quartz-load-femtofarad-tuning-max
* Limit run-time tuning adjustments to be within that range
* Only overwrite the analog calibration register on start-up if its
value is outside that range

After thinking through all this, I'd still advocate for merging this
patchset in some form and leaving integration with runtime APIs as a
potential future enhancement. I look forward to hearing your thoughts
about it.
-- 
Dennis Lambe (He/Him)
Lead Firmware Engineer
sparkcharge.io

Re: [PATCH v3 0/3] rtc: Set M41T82 & M41T83 xtal load capacitance from DT

[Alexandre Belloni]

On 19/01/2023 18:27:44-0500, Dennis Lambe wrote:
> On Thu, Jan 19, 2023 at 5:18 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> 
> > I need to find time to think about it because while setting the analog
> > trimming statically from the device tree solves your immediate problem,
> > it will also remove the possibility to handle it from userspace later
> > on. I would really prefer this uses the offset interface or a better
> > interface that unfortunately doesn't exist yet.
> 
> Thanks for letting me know what you're thinking about this. I think I
> see what you're getting at.
> 
> However, I think this is more complex than either of us had
> considered. The M41T82 has two different calibration capabilities:
> 
> 1. Digital calibration. This looks to me like it behaves similarly to
> the digital calibration feature of the M41T00, which ds1307.c exposes
> through the offset interface. The M41T8x driver doesn't currently
> expose the digital calibration register at all, but if it did I would
> agree that the offset interface looks appropriate.
> 
> 2. Analog calibration -- that's what the datasheet calls it, but the
> range on it is very big -- 3.5 pF all the way up to 17.4 pF -- and
> their reference design uses it as the only xtal load capacitance in
> the circuit. Most of the values you could set for this would be wildly
> inappropriate for any given design's choice of xtal oscillator.
> 
> Between these, I don't know if you'd want to expose just one, the
> other, or some synthesis of both via the offset interface or some new
> interface.
> 
> I'd make the case that the xtal's required load capacitance is a
> hardware requirement that's appropriate to configure via the Device
> Tree. Even if you did want to allow some amount of runtime fine-tuning
> of this register, you'd still want to document a rational starting
> value chosen based on the hardware.
> 
> I agree with you, though, that if a runtime fine-tuning feature were
> added, we'd have to find a way to choose whether to initialize the
> register on boot or not, so that we didn't overwrite the fine-tuning.
> 
> Just to demonstrate something that could work, and would be
> backward-compatible with this patchset, here's a hypothetical design:
> * dt-bindings: add quartz-load-femtofarad-tuning-min and
> quartz-load-femtofarad-tuning-max
> * Limit run-time tuning adjustments to be within that range
> * Only overwrite the analog calibration register on start-up if its
> value is outside that range
> 
> After thinking through all this, I'd still advocate for merging this
> patchset in some form and leaving integration with runtime APIs as a
> potential future enhancement. I look forward to hearing your thoughts
> about it.

I specifically referred to analog trimming in my reply because I knew it
could do digital trimming and I also said that we will probably need a
new interface for this.
The main existing issue is that the register changes the capacitance but
the datasheet doesn't give the actual effect in ppm which doesn't
integrate well with the existing userspace tooling.

I advocate against merging as is without more thought because changing
anything later will mean breaking the DT ABI and this is not allowed.


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

Re: [PATCH v3 0/3] rtc: Set M41T82 & M41T83 xtal load capacitance from DT

[Atsushi Nemoto]

On 2023/01/20 6:39, Dennis Lambe Jr wrote:
> In case it's additional reviewers, I have CC'd some more
> potentially-interested parties this time and updated Atsushi Nemoto's
> email address to one that's hopefully more current.

My addresses are both still active (dispite the slow response).
Ether is fine.

> I think the original author listed in the header for this driver,
> Alexander Bigga, is inaccurate. It looks to me like his name got copied
> over by Atsushi Nemoto when he created m41t82.c by deriving it from a
> similar driver. At any rate, Alexander Bigga's listed email address
> bounces, I didn't find a newer one for him, and he doesn't show up in
> the kernel commit log after 2007. I don't think he can be considered the
> maintainer for this driver anymore if he ever was.

In 2006-2007, I wrote rtc-m41t80 driver based on Alexander Bigga's
rtc-m41txx driver with his cooperation and review, then upstreamed it.

About your patchset, I have no idea, since I have not touched this device
and driver for a long time (>10 years) ...

---
Atsushi Nemoto

Re: [PATCH v3 0/3] rtc: Set M41T82 & M41T83 xtal load capacitance from DT

[Dennis Lambe]

On Thu, Jan 19, 2023 at 6:52 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> On 19/01/2023 18:27:44-0500, Dennis Lambe wrote:
> > 2. Analog calibration -- that's what the datasheet calls it, but the
> > range on it is very big -- 3.5 pF all the way up to 17.4 pF -- and
> > their reference design uses it as the only xtal load capacitance in
> > the circuit. Most of the values you could set for this would be wildly
> > inappropriate for any given design's choice of xtal oscillator.

I think I should start with an apology, almost everything I wrote
yesterday was in error one way or another. I was conflating this RTC
with I'm also working with. I see now that the datasheet for this one
clearly states that it's only to be used with 12.5 pF crystals, and
that the analog adjustments are meant exclusively for calibration. I
get what you mean about wanting it to be a new runtime interface and
it not making sense to put in the DeviceTree.

I also see what you mean about the datasheet not providing a good
capacitance vs. ppm table. The graph is approximate at best, and ST's
appnote recommends an iterative tuning procedure rather than just
assuming a certain value gives exactly a certain ppm adjustment. I see
why you would want to avoid using 'offset' for this.

I'll hold off on submitting any more patches for this until you've had
a chance to think about how you would want a new interface to work.
Would it be useful to you if I start working up a patchset that makes
a new rtc sysfs attribute and wires the m41t80 driver into it so that
I'm ready to adapt it to whatever naming, scaling, semantics,
interpretation, etc. you decide is right for it?

> I advocate against merging as is without more thought because changing
> anything later will mean breaking the DT ABI and this is not allowed.

Me too, thanks for taking the time to get through to me about it.
-- 

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