[PATCH v1 0/5] add quartz load support to NXP rtc drivers

[PATCH v1 0/5] add quartz load support to NXP rtc drivers

[Sam Ravnborg]

The current rtc drivers for NXP pcf8523 and pcf85063
have different configuration of the quartz load.

pcf8523 hardcode the quartz load to 12.5 Pf
pcf85063 uses the reset default of 7 pF

Introduce a new property to the bindings for the two
rtc devices to specify if the quartz load is 12.5 pF.

Update all in-kernel DT files using the pcf8523 to specify this property.
And finally update the drivers to support the property.

NOTE:
This has the side-effect that any users of the
pcf8523 driver using out-of-tree DTS files needs to
add the new property to get the same behaviour as before.

	Sam

Søren Andersen (5):
      dt-binding: rtci-pcf8523: add binding with quartz_load property
      dt-binding: rtc-pcf85063: add binding with quartz load property
      dts: add nxp,quartz_load_12.5pf to all pcf8523 nodes
      rtc: pcf8523: add quartz load configuration from DT
      rtc: pcf85063: add quartz load configuration from DT

 .../devicetree/bindings/rtc/nxp,pcf85063.txt       | 19 +++++++++++++
 .../devicetree/bindings/rtc/nxp,pcf8523.txt        | 19 +++++++++++++
 .../devicetree/bindings/trivial-devices.txt        |  2 --
 arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts       |  1 +
 arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts         |  1 +
 arch/arm/boot/dts/imx6q-h100.dts                   |  1 +
 arch/arm/boot/dts/imx6q-novena.dts                 |  1 +
 arch/arm/boot/dts/imx6qdl-cubox-i.dtsi             |  1 +
 arch/arm/boot/dts/imx6qdl-hummingboard.dtsi        |  1 +
 arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi       |  1 +
 drivers/rtc/rtc-pcf85063.c                         | 31 ++++++++++++++++++++++
 drivers/rtc/rtc-pcf8523.c                          |  6 ++---
 12 files changed, 79 insertions(+), 5 deletions(-)

[PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

[Sam Ravnborg]

From: Søren Andersen <san@skov.dk>

The NXP pcf8523 supports two different quartz loads.
- 7 pF (default)
- 12.5 pF (minimum power consumption)

The pcf8523 needs to know the size of the quartz load,
otherwise the the RTC will have a bad precision.

The default for the rtc (after power-on) is 7 pF.
Add a property that tells if the external capacitor is 12.5 pF.

Signed-off-by: Søren Andersen <san@skov.dk>
Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 19 +++++++++++++++++++
 Documentation/devicetree/bindings/trivial-devices.txt |  1 -
 2 files changed, 19 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt

diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
new file mode 100644
index 000000000000..7c5e93f5077c
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
@@ -0,0 +1,19 @@
+* NXP PCF8523 Real Time Clock
+
+NXP PCF8523 Real Time Clock
+
+Required properties:
+- compatible: Should contain "nxp,pcf8523".
+- reg: I2C address for chip.
+
+Optional property:
+- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
+  which differ from the default value of 7 pF
+
+Example:
+
+pcf8523: pcf8523@68 {
+	compatible = "nxp,pcf85063";
+	reg = <0x68>;
+	nxp,quartz_load_12.5pF;
+};
diff --git a/Documentation/devicetree/bindings/trivial-devices.txt b/Documentation/devicetree/bindings/trivial-devices.txt
index 763a2808a95c..297dc82b67ad 100644
--- a/Documentation/devicetree/bindings/trivial-devices.txt
+++ b/Documentation/devicetree/bindings/trivial-devices.txt
@@ -169,7 +169,6 @@ nxp,pca9556		Octal SMBus and I2C registered interface
 nxp,pca9557		8-bit I2C-bus and SMBus I/O port with reset
 nxp,pcf2127		Real-time clock
 nxp,pcf2129		Real-time clock
-nxp,pcf8523		Real-time Clock
 nxp,pcf8563		Real-time clock/calendar
 nxp,pcf85063		Tiny Real-Time Clock
 oki,ml86v7667		OKI ML86V7667 video decoder
-- 
2.12.0

[PATCH v1 2/5] dt-binding: rtc-pcf85063: add quartz load property

[Sam Ravnborg]

From: Søren Andersen <san@skov.dk>

The NXP pcf85063 support two different quartz loads.
- 7 pF (default)
- 12.5 pF (minimum power consumption)

The pcf85063 needs to know the size of the external capacitor,
otherwise the RTC will have a bad precision (hours/week).

The power-on default is 7 pF
Add a property that tells if the external capacitor is 12.5 pF

Signed-off-by: Søren Andersen <san@skov.dk>
Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 .../devicetree/bindings/rtc/nxp,pcf85063.txt          | 19 +++++++++++++++++++
 Documentation/devicetree/bindings/trivial-devices.txt |  1 -
 2 files changed, 19 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf85063.txt

diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf85063.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf85063.txt
new file mode 100644
index 000000000000..22ebb2ce52c1
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/nxp,pcf85063.txt
@@ -0,0 +1,19 @@
+* NXP PCF85063 Real Time Clock
+
+NXP PCF85063 Real Time Clock
+
+Required properties:
+- compatible: Should contain "nxp,pcf85063".
+- reg: I2C address for chip.
+
+Optional property:
+- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
+  which differ from the default value of 7 pF
+
+Example:
+
+pcf85063: pcf85063@51 {
+	compatible = "nxp,pcf85063";
+	reg = <0x51>;
+	nxp,quartz_load_12.5pF;
+};
diff --git a/Documentation/devicetree/bindings/trivial-devices.txt b/Documentation/devicetree/bindings/trivial-devices.txt
index 297dc82b67ad..440632682e31 100644
--- a/Documentation/devicetree/bindings/trivial-devices.txt
+++ b/Documentation/devicetree/bindings/trivial-devices.txt
@@ -170,7 +170,6 @@ nxp,pca9557		8-bit I2C-bus and SMBus I/O port with reset
 nxp,pcf2127		Real-time clock
 nxp,pcf2129		Real-time clock
 nxp,pcf8563		Real-time clock/calendar
-nxp,pcf85063		Tiny Real-Time Clock
 oki,ml86v7667		OKI ML86V7667 video decoder
 ovti,ov5642		OV5642: Color CMOS QSXGA (5-megapixel) Image Sensor with OmniBSI and Embedded TrueFocus
 pericom,pt7c4338	Real-time Clock Module
-- 
2.12.0

[PATCH v1 3/5] dts: add nxp,quartz_load_12.5pf to all pcf8523 nodes

[Sam Ravnborg]

From: Søren Andersen <san@skov.dk>

Add "nxp,quartz_load_12.5pf" to all pcf8523 nodes.

The device tree binding for the pcf8523 now includes
a property "nxp,quartz_load_12.5pf" to specify
that the connected x-tal has an internal capacitance
of 12.5 pF.
Add this to all pcf8523 nodes, because this was implicit before,
at least in the Linux kernel where the driver was hardcoded
to set the rtc to use 12.5 pF.

Signed-off-by: Søren Andersen <san@skov.dk>
Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Joel Stanley <joel@jms.id.au>
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Russell King <linux@armlinux.org.uk>
---
 arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts | 1 +
 arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts   | 1 +
 arch/arm/boot/dts/imx6q-h100.dts             | 1 +
 arch/arm/boot/dts/imx6q-novena.dts           | 1 +
 arch/arm/boot/dts/imx6qdl-cubox-i.dtsi       | 1 +
 arch/arm/boot/dts/imx6qdl-hummingboard.dtsi  | 1 +
 arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi | 1 +
 7 files changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts b/arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts
index d598b6391362..d1a0e3521e2b 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts
@@ -200,6 +200,7 @@
 	rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	tmp75@48 {
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
index 2c5aa90a546d..b5088414e610 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
@@ -201,6 +201,7 @@
 	rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	ucd90160@64 {
diff --git a/arch/arm/boot/dts/imx6q-h100.dts b/arch/arm/boot/dts/imx6q-h100.dts
index 714e09e04dcb..3b6116069b11 100644
--- a/arch/arm/boot/dts/imx6q-h100.dts
+++ b/arch/arm/boot/dts/imx6q-h100.dts
@@ -173,6 +173,7 @@
 	rtc: pcf8523@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	sgtl5000: sgtl5000@a {
diff --git a/arch/arm/boot/dts/imx6q-novena.dts b/arch/arm/boot/dts/imx6q-novena.dts
index fcd824dc485b..6a2eaaa6e220 100644
--- a/arch/arm/boot/dts/imx6q-novena.dts
+++ b/arch/arm/boot/dts/imx6q-novena.dts
@@ -257,6 +257,7 @@
 	rtc: pcf8523@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	sbs_battery: bq20z75@b {
diff --git a/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi b/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
index 9332a31e6c8b..e0f601bce3d3 100644
--- a/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
@@ -143,6 +143,7 @@
 	rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 };
 
diff --git a/arch/arm/boot/dts/imx6qdl-hummingboard.dtsi b/arch/arm/boot/dts/imx6qdl-hummingboard.dtsi
index 0e64016e765f..7848b818078e 100644
--- a/arch/arm/boot/dts/imx6qdl-hummingboard.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-hummingboard.dtsi
@@ -189,6 +189,7 @@
 	rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	/* Pro baseboard model */
diff --git a/arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi b/arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi
index c413f9c3540f..7bc264234085 100644
--- a/arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi
@@ -222,6 +222,7 @@
 	pcf8523: rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	sgtl5000: codec@a {
-- 
2.12.0

[PATCH v1 4/5] rtc: pcf8523: external capacitor configuration

[Sam Ravnborg]

From: Søren Andersen <san@skov.dk>

Add support for specifying the quartz load in the DT node.
The pcf8523 may use either a 7 pF or an 12.5 pF xtal.
If the rtc has the wrong configuration the time will
drift several hours/week.

If nothing is specified in DT then the factory default of 7 pF is used.
This is a change compared to before where the driver assumed 12.5 pF.

Note: all kernel dts trees are updated with the new property.

Signed-off-by: Søren Andersen <san@skov.dk>
Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-pcf8523.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-pcf8523.c b/drivers/rtc/rtc-pcf8523.c
index 453615f8ac9a..3c19b4d21a2d 100644
--- a/drivers/rtc/rtc-pcf8523.c
+++ b/drivers/rtc/rtc-pcf8523.c
@@ -85,7 +85,7 @@ static int pcf8523_write(struct i2c_client *client, u8 reg, u8 value)
 	return 0;
 }
 
-static int pcf8523_select_capacitance(struct i2c_client *client, bool high)
+static int pcf8523_select_capacitance(struct i2c_client *client)
 {
 	u8 value;
 	int err;
@@ -94,7 +94,7 @@ static int pcf8523_select_capacitance(struct i2c_client *client, bool high)
 	if (err < 0)
 		return err;
 
-	if (!high)
+	if (!device_property_present(&client->dev, "nxp,quartz_load_12.5pf"))
 		value &= ~REG_CONTROL1_CAP_SEL;
 	else
 		value |= REG_CONTROL1_CAP_SEL;
@@ -331,7 +331,7 @@ static int pcf8523_probe(struct i2c_client *client,
 	if (!pcf)
 		return -ENOMEM;
 
-	err = pcf8523_select_capacitance(client, true);
+	err = pcf8523_select_capacitance(client);
 	if (err < 0)
 		return err;
 
-- 
2.12.0

[PATCH v1 5/5] rtc: pcf85063: external capacitor configuration

[Sam Ravnborg]

From: Søren Andersen <san@skov.dk>

Add support for specifying the quartz load in the DT node.
The pcf85063 may use either a 7 pF or an 12.5 pF xtal.
If the rtc has the wrong configuration the time will
drift several hours/week.

If nothing is specified in DT then the factory default of 7 pF is used.

Signed-off-by: Søren Andersen <san@skov.dk>
Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-pcf85063.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
index 283c2335b01b..38163446664f 100644
--- a/drivers/rtc/rtc-pcf85063.c
+++ b/drivers/rtc/rtc-pcf85063.c
@@ -27,6 +27,7 @@
 */
 
 #define PCF85063_REG_CTRL1		0x00 /* status */
+#define PCF85063_REG_CTRL1_CAP_SEL	BIT(0)
 #define PCF85063_REG_CTRL1_STOP		BIT(5)
 #define PCF85063_REG_CTRL2		0x01
 
@@ -180,6 +181,31 @@ static const struct rtc_class_ops pcf85063_rtc_ops = {
 	.set_time	= pcf85063_rtc_set_time
 };
 
+static int pcf85063_select_capacitance(struct i2c_client *client)
+{
+	int rc;
+	u8 reg;
+
+	rc = i2c_smbus_read_byte_data(client, PCF85063_REG_CTRL1);
+	if (rc < 0) {
+		dev_err(&client->dev, "Failing to read Control1 reg\n");
+		return -EIO;
+	}
+
+	if (device_property_present(&client->dev, "nxp,quartz_load_12.5pf"))
+		reg = rc |= PCF85063_REG_CTRL1_CAP_SEL;
+	else
+		reg = rc &= ~PCF85063_REG_CTRL1_CAP_SEL;
+
+	rc = i2c_smbus_write_byte_data(client, PCF85063_REG_CTRL1, reg);
+	if (rc < 0) {
+		dev_err(&client->dev, "Failing to configure device\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static int pcf85063_probe(struct i2c_client *client,
 				const struct i2c_device_id *id)
 {
@@ -197,6 +223,11 @@ static int pcf85063_probe(struct i2c_client *client,
 		return err;
 	}
 
+	err = pcf85063_select_capacitance(client);
+	if (err < 0)
+		dev_warn(&client->dev,
+			 "Capacitance  setup failed. Trying to continue\n");
+
 	rtc = devm_rtc_device_register(&client->dev,
 				       pcf85063_driver.driver.name,
 				       &pcf85063_rtc_ops, THIS_MODULE);
-- 
2.12.0

Re: [PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

[Sam Ravnborg]

Hi all.

> +Optional property:
> +- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
> +  which differ from the default value of 7 pF
> +
> +Example:
> +
> +pcf8523: pcf8523@68 {
> +	compatible = "nxp,pcf85063";
> +	reg = <0x68>;
> +	nxp,quartz_load_12.5pF;
> +};

The property described in the bindings uses capital F.
But the intent was to use a lower-case f, so the property
is all lower-case.

Will fix in v2.
This goes for both this binding file and the nxp,pcf85063.txt.
The dts files and the implmentation uses lower-case f already.

	Sam

Re: [PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

[Alexandre Belloni]

Hi,

You can remove 'rtci-' from the subject.

On 07/09/2018 21:35:04+0200, Sam Ravnborg wrote:
> From: Søren Andersen <san@skov.dk>
> 
> The NXP pcf8523 supports two different quartz loads.
> - 7 pF (default)
> - 12.5 pF (minimum power consumption)
> 
> The pcf8523 needs to know the size of the quartz load,
> otherwise the the RTC will have a bad precision.
> 
> The default for the rtc (after power-on) is 7 pF.
> Add a property that tells if the external capacitor is 12.5 pF.
> 
> Signed-off-by: Søren Andersen <san@skov.dk>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 19 +++++++++++++++++++
>  Documentation/devicetree/bindings/trivial-devices.txt |  1 -
>  2 files changed, 19 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> new file mode 100644
> index 000000000000..7c5e93f5077c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> @@ -0,0 +1,19 @@
> +* NXP PCF8523 Real Time Clock
> +
> +NXP PCF8523 Real Time Clock
> +
> +Required properties:
> +- compatible: Should contain "nxp,pcf8523".
> +- reg: I2C address for chip.
> +
> +Optional property:
> +- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
> +  which differ from the default value of 7 pF
> +

The boolean properties usually don't work well for RTCs because people
usually want to keep any previous configuration that may have been done
at the factory or in the bootloader so I would use:

nxp,quartz_load_fF and this would be either 7000 or 12500.


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

Re: [PATCH v1 2/5] dt-binding: rtc-pcf85063: add quartz load property

[Alexandre Belloni]

Hi,

You can remove 'rtc-' from the part name in the subject.

On 07/09/2018 21:35:05+0200, Sam Ravnborg wrote:
> From: Søren Andersen <san@skov.dk>
> 
> The NXP pcf85063 support two different quartz loads.
> - 7 pF (default)
> - 12.5 pF (minimum power consumption)
> 
> The pcf85063 needs to know the size of the external capacitor,
> otherwise the RTC will have a bad precision (hours/week).
> 
> The power-on default is 7 pF
> Add a property that tells if the external capacitor is 12.5 pF
> 
> Signed-off-by: Søren Andersen <san@skov.dk>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  .../devicetree/bindings/rtc/nxp,pcf85063.txt          | 19 +++++++++++++++++++
>  Documentation/devicetree/bindings/trivial-devices.txt |  1 -
>  2 files changed, 19 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf85063.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf85063.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf85063.txt
> new file mode 100644
> index 000000000000..22ebb2ce52c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf85063.txt
> @@ -0,0 +1,19 @@
> +* NXP PCF85063 Real Time Clock
> +
> +NXP PCF85063 Real Time Clock
> +
> +Required properties:
> +- compatible: Should contain "nxp,pcf85063".
> +- reg: I2C address for chip.
> +
> +Optional property:
> +- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
> +  which differ from the default value of 7 pF
> +
> +Example:
> +
> +pcf85063: pcf85063@51 {
> +	compatible = "nxp,pcf85063";
> +	reg = <0x51>;
> +	nxp,quartz_load_12.5pF;

Same comment as the previous patch, a value in fF is more useful. It
could also probably be made a generic rtc property.


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

Re: [PATCH v1 3/5] dts: add nxp,quartz_load_12.5pf to all pcf8523 nodes

[Alexandre Belloni]

On 07/09/2018 21:35:06+0200, Sam Ravnborg wrote:
> From: Søren Andersen <san@skov.dk>
> 
> Add "nxp,quartz_load_12.5pf" to all pcf8523 nodes.
> 
> The device tree binding for the pcf8523 now includes
> a property "nxp,quartz_load_12.5pf" to specify
> that the connected x-tal has an internal capacitance
> of 12.5 pF.
> Add this to all pcf8523 nodes, because this was implicit before,
> at least in the Linux kernel where the driver was hardcoded
> to set the rtc to use 12.5 pF.
> 

Well, this patch is not needed as we should not break the DT ABI. See
comment on the next patch...

> Signed-off-by: Søren Andersen <san@skov.dk>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Russell King <linux@armlinux.org.uk>
> ---
>  arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts | 1 +
>  arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts   | 1 +
>  arch/arm/boot/dts/imx6q-h100.dts             | 1 +
>  arch/arm/boot/dts/imx6q-novena.dts           | 1 +
>  arch/arm/boot/dts/imx6qdl-cubox-i.dtsi       | 1 +
>  arch/arm/boot/dts/imx6qdl-hummingboard.dtsi  | 1 +
>  arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi | 1 +
>  7 files changed, 7 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts b/arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts
> index d598b6391362..d1a0e3521e2b 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts
> @@ -200,6 +200,7 @@
>  	rtc@68 {
>  		compatible = "nxp,pcf8523";
>  		reg = <0x68>;
> +		nxp,quartz_load_12.5pf;
>  	};
>  
>  	tmp75@48 {
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> index 2c5aa90a546d..b5088414e610 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> @@ -201,6 +201,7 @@
>  	rtc@68 {
>  		compatible = "nxp,pcf8523";
>  		reg = <0x68>;
> +		nxp,quartz_load_12.5pf;
>  	};
>  
>  	ucd90160@64 {
> diff --git a/arch/arm/boot/dts/imx6q-h100.dts b/arch/arm/boot/dts/imx6q-h100.dts
> index 714e09e04dcb..3b6116069b11 100644
> --- a/arch/arm/boot/dts/imx6q-h100.dts
> +++ b/arch/arm/boot/dts/imx6q-h100.dts
> @@ -173,6 +173,7 @@
>  	rtc: pcf8523@68 {
>  		compatible = "nxp,pcf8523";
>  		reg = <0x68>;
> +		nxp,quartz_load_12.5pf;
>  	};
>  
>  	sgtl5000: sgtl5000@a {
> diff --git a/arch/arm/boot/dts/imx6q-novena.dts b/arch/arm/boot/dts/imx6q-novena.dts
> index fcd824dc485b..6a2eaaa6e220 100644
> --- a/arch/arm/boot/dts/imx6q-novena.dts
> +++ b/arch/arm/boot/dts/imx6q-novena.dts
> @@ -257,6 +257,7 @@
>  	rtc: pcf8523@68 {
>  		compatible = "nxp,pcf8523";
>  		reg = <0x68>;
> +		nxp,quartz_load_12.5pf;
>  	};
>  
>  	sbs_battery: bq20z75@b {
> diff --git a/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi b/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
> index 9332a31e6c8b..e0f601bce3d3 100644
> --- a/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
> @@ -143,6 +143,7 @@
>  	rtc@68 {
>  		compatible = "nxp,pcf8523";
>  		reg = <0x68>;
> +		nxp,quartz_load_12.5pf;
>  	};
>  };
>  
> diff --git a/arch/arm/boot/dts/imx6qdl-hummingboard.dtsi b/arch/arm/boot/dts/imx6qdl-hummingboard.dtsi
> index 0e64016e765f..7848b818078e 100644
> --- a/arch/arm/boot/dts/imx6qdl-hummingboard.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-hummingboard.dtsi
> @@ -189,6 +189,7 @@
>  	rtc@68 {
>  		compatible = "nxp,pcf8523";
>  		reg = <0x68>;
> +		nxp,quartz_load_12.5pf;
>  	};
>  
>  	/* Pro baseboard model */
> diff --git a/arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi b/arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi
> index c413f9c3540f..7bc264234085 100644
> --- a/arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi
> @@ -222,6 +222,7 @@
>  	pcf8523: rtc@68 {
>  		compatible = "nxp,pcf8523";
>  		reg = <0x68>;
> +		nxp,quartz_load_12.5pf;
>  	};
>  
>  	sgtl5000: codec@a {
> -- 
> 2.12.0
> 

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

Re: [PATCH v1 4/5] rtc: pcf8523: external capacitor configuration

[Alexandre Belloni]

On 07/09/2018 21:35:07+0200, Sam Ravnborg wrote:
> From: Søren Andersen <san@skov.dk>
> 
> Add support for specifying the quartz load in the DT node.
> The pcf8523 may use either a 7 pF or an 12.5 pF xtal.
> If the rtc has the wrong configuration the time will
> drift several hours/week.
> 
> If nothing is specified in DT then the factory default of 7 pF is used.
> This is a change compared to before where the driver assumed 12.5 pF.
> 
> Note: all kernel dts trees are updated with the new property.
> 
> Signed-off-by: Søren Andersen <san@skov.dk>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  drivers/rtc/rtc-pcf8523.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pcf8523.c b/drivers/rtc/rtc-pcf8523.c
> index 453615f8ac9a..3c19b4d21a2d 100644
> --- a/drivers/rtc/rtc-pcf8523.c
> +++ b/drivers/rtc/rtc-pcf8523.c
> @@ -85,7 +85,7 @@ static int pcf8523_write(struct i2c_client *client, u8 reg, u8 value)
>  	return 0;
>  }
>  
> -static int pcf8523_select_capacitance(struct i2c_client *client, bool high)
> +static int pcf8523_select_capacitance(struct i2c_client *client)
>  {
>  	u8 value;
>  	int err;
> @@ -94,7 +94,7 @@ static int pcf8523_select_capacitance(struct i2c_client *client, bool high)
>  	if (err < 0)
>  		return err;
>  
> -	if (!high)
> +	if (!device_property_present(&client->dev, "nxp,quartz_load_12.5pf"))

OK, this is super annoying we unfortunately can't change what was done
previously without breaking the DT ABI. Honestly, I suspect the 12.5pF
value is wrong for most boards anyway.

What I would do is use the value from DT (so either 7000fF or 12500fF)
and set it if present.
If not present, set 12.5pF and print a warning that this differs from
the RTC default.

Hopefully, at some point, we could leave it as it is as the value should
persist across reboot.

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

Re: [PATCH v1 5/5] rtc: pcf85063: external capacitor configuration

[Alexandre Belloni]

On 07/09/2018 21:35:08+0200, Sam Ravnborg wrote:
> From: Søren Andersen <san@skov.dk>
> 
> Add support for specifying the quartz load in the DT node.
> The pcf85063 may use either a 7 pF or an 12.5 pF xtal.
> If the rtc has the wrong configuration the time will
> drift several hours/week.
> 
> If nothing is specified in DT then the factory default of 7 pF is used.
> 
> Signed-off-by: Søren Andersen <san@skov.dk>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  drivers/rtc/rtc-pcf85063.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> index 283c2335b01b..38163446664f 100644
> --- a/drivers/rtc/rtc-pcf85063.c
> +++ b/drivers/rtc/rtc-pcf85063.c
> @@ -27,6 +27,7 @@
>  */
>  
>  #define PCF85063_REG_CTRL1		0x00 /* status */
> +#define PCF85063_REG_CTRL1_CAP_SEL	BIT(0)
>  #define PCF85063_REG_CTRL1_STOP		BIT(5)
>  #define PCF85063_REG_CTRL2		0x01
>  
> @@ -180,6 +181,31 @@ static const struct rtc_class_ops pcf85063_rtc_ops = {
>  	.set_time	= pcf85063_rtc_set_time
>  };
>  
> +static int pcf85063_select_capacitance(struct i2c_client *client)
> +{
> +	int rc;
> +	u8 reg;
> +
> +	rc = i2c_smbus_read_byte_data(client, PCF85063_REG_CTRL1);
> +	if (rc < 0) {
> +		dev_err(&client->dev, "Failing to read Control1 reg\n");
> +		return -EIO;
> +	}
> +
> +	if (device_property_present(&client->dev, "nxp,quartz_load_12.5pf"))
> +		reg = rc |= PCF85063_REG_CTRL1_CAP_SEL;
> +	else
> +		reg = rc &= ~PCF85063_REG_CTRL1_CAP_SEL;

That one is more straight forward, set it to the value from DT or leave
it alone.


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

Re: [PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

[Sam Ravnborg]

Hi Alexandre.

On Thu, Sep 13, 2018 at 09:05:16PM +0200, Alexandre Belloni wrote:
> Hi,
> 
> You can remove 'rtci-' from the subject.

The 'i' part was me fooling around in vi.
After submitting this serie I read the proper subject would be:
(from bindings/submitting-patches.txt)

dt-bindings: rtc: <short description>

I will use this in next submission so it is clear this is rtc related.

> 
> On 07/09/2018 21:35:04+0200, Sam Ravnborg wrote:
> > From: Søren Andersen <san@skov.dk>
> > 
> > The NXP pcf8523 supports two different quartz loads.
> > - 7 pF (default)
> > - 12.5 pF (minimum power consumption)
> > 
> > The pcf8523 needs to know the size of the quartz load,
> > otherwise the the RTC will have a bad precision.
> > 
> > The default for the rtc (after power-on) is 7 pF.
> > Add a property that tells if the external capacitor is 12.5 pF.
> > 
> > Signed-off-by: Søren Andersen <san@skov.dk>
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Alessandro Zummo <a.zummo@towertech.it>
> > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > ---
> >  Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 19 +++++++++++++++++++
> >  Documentation/devicetree/bindings/trivial-devices.txt |  1 -
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > new file mode 100644
> > index 000000000000..7c5e93f5077c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > @@ -0,0 +1,19 @@
> > +* NXP PCF8523 Real Time Clock
> > +
> > +NXP PCF8523 Real Time Clock
> > +
> > +Required properties:
> > +- compatible: Should contain "nxp,pcf8523".
> > +- reg: I2C address for chip.
> > +
> > +Optional property:
> > +- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
> > +  which differ from the default value of 7 pF
> > +
> 
> The boolean properties usually don't work well for RTCs because people
> usually want to keep any previous configuration that may have been done
> at the factory or in the bootloader so I would use:
> 
> nxp,quartz_load_fF and this would be either 7000 or 12500.
We had is implmented like this (using pF) in the beginning but
then went for the simpler property.
Will add a fF property as you suggest and avoid breaking the existing drivers.
We will check a few of the boards to see if the current configuration
of the pcf8523 driver looks wrong, and if so we will print
the warnings as suggested.

I think the above covers feedback on all patches.
And thanks for the feedback!

	Sam

Re: [PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

[Alexandre Belloni]

On 13/09/2018 22:44:12+0200, Sam Ravnborg wrote:
> > The boolean properties usually don't work well for RTCs because people
> > usually want to keep any previous configuration that may have been done
> > at the factory or in the bootloader so I would use:
> > 
> > nxp,quartz_load_fF and this would be either 7000 or 12500.
> We had is implmented like this (using pF) in the beginning but
> then went for the simpler property.
> Will add a fF property as you suggest and avoid breaking the existing drivers.
> We will check a few of the boards to see if the current configuration
> of the pcf8523 driver looks wrong, and if so we will print
> the warnings as suggested.
> 
> I think the above covers feedback on all patches.
> And thanks for the feedback!
> 

Hint: look at the cubox-i and the hummingboard schematics. I'm
definitely not an analog expert but the two capacitors on the cubox-i
are making me think that the setting should be different from the
hummingboard.

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

Re: [PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

[Rob Herring]

On Thu, Sep 13, 2018 at 09:05:16PM +0200, Alexandre Belloni wrote:
> Hi,
> 
> You can remove 'rtci-' from the subject.
> 
> On 07/09/2018 21:35:04+0200, Sam Ravnborg wrote:
> > From: Søren Andersen <san@skov.dk>
> > 
> > The NXP pcf8523 supports two different quartz loads.
> > - 7 pF (default)
> > - 12.5 pF (minimum power consumption)
> > 
> > The pcf8523 needs to know the size of the quartz load,
> > otherwise the the RTC will have a bad precision.
> > 
> > The default for the rtc (after power-on) is 7 pF.
> > Add a property that tells if the external capacitor is 12.5 pF.
> > 
> > Signed-off-by: Søren Andersen <san@skov.dk>
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Alessandro Zummo <a.zummo@towertech.it>
> > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > ---
> >  Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 19 +++++++++++++++++++
> >  Documentation/devicetree/bindings/trivial-devices.txt |  1 -
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > new file mode 100644
> > index 000000000000..7c5e93f5077c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > @@ -0,0 +1,19 @@
> > +* NXP PCF8523 Real Time Clock
> > +
> > +NXP PCF8523 Real Time Clock
> > +
> > +Required properties:
> > +- compatible: Should contain "nxp,pcf8523".
> > +- reg: I2C address for chip.
> > +
> > +Optional property:
> > +- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
> > +  which differ from the default value of 7 pF
> > +
> 
> The boolean properties usually don't work well for RTCs because people
> usually want to keep any previous configuration that may have been done
> at the factory or in the bootloader so I would use:
> 
> nxp,quartz_load_fF and this would be either 7000 or 12500.

Use '-' rather than '_'.

There's already a defined unit suffix of '-picofarads'. If you need 
'-femtofarads', then please add to property-units.txt. Though I'd prefer 
not as wouldn't a value of 12 (or 13) be close enough for the needs 
here?

Rob

Re: [PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

[Alexandre Belloni]

On 26/09/2018 10:47:08-0500, Rob Herring wrote:
> > > +Optional property:
> > > +- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
> > > +  which differ from the default value of 7 pF
> > > +
> > 
> > The boolean properties usually don't work well for RTCs because people
> > usually want to keep any previous configuration that may have been done
> > at the factory or in the bootloader so I would use:
> > 
> > nxp,quartz_load_fF and this would be either 7000 or 12500.
> 
> Use '-' rather than '_'.
> 
> There's already a defined unit suffix of '-picofarads'. If you need 
> '-femtofarads', then please add to property-units.txt. Though I'd prefer 
> not as wouldn't a value of 12 (or 13) be close enough for the needs 
> here?
> 

For that particular RTC, yes but the isl1208 has 0.5pF steps and the
x1205 has 0.25pf steps. I'd prefer having a single generic property for
all the RTCs: quartz-load-femtofarads.

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

Re: [PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

[Sam Ravnborg]

On Wed, Sep 26, 2018 at 08:51:21PM +0200, Alexandre Belloni wrote:
> On 26/09/2018 10:47:08-0500, Rob Herring wrote:
> > > > +Optional property:
> > > > +- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
> > > > +  which differ from the default value of 7 pF
> > > > +
> > > 
> > > The boolean properties usually don't work well for RTCs because people
> > > usually want to keep any previous configuration that may have been done
> > > at the factory or in the bootloader so I would use:
> > > 
> > > nxp,quartz_load_fF and this would be either 7000 or 12500.
> > 
> > Use '-' rather than '_'.
> > 
> > There's already a defined unit suffix of '-picofarads'. If you need 
> > '-femtofarads', then please add to property-units.txt. Though I'd prefer 
> > not as wouldn't a value of 12 (or 13) be close enough for the needs 
> > here?
> > 
> 
> For that particular RTC, yes but the isl1208 has 0.5pF steps and the
> x1205 has 0.25pf steps. I'd prefer having a single generic property for
> all the RTCs: quartz-load-femtofarads.

I will change to use this, and update property-units.txt too, when I get
back to the patch set in a week or so.

	Sam