[PATCH 0/2] pcf85363: support for quartz-load-femtofarads

[PATCH 0/2] pcf85363: support for quartz-load-femtofarads

[Javier Carrasco]

These patches add support for the quartz-load-femtofarads property in
the pcf85363 device driver and a description of the property's possible
values. 

The driver has been tested with a PCF85263ATT RTC and a CTS3-32.768-12.5-20
oscillator that needs a 12.5 pF load capacitor. With no property
support the 7 pF default value leads to at least 2 Hz output frequency
deviations, while setting the right value the deviation decreased to
0.15 Hz. These measurements were made with a high precision oscilloscope
(SIGLENT SDS5104X).

This modification does not affect existing designs where the
quartz-load-femtofarads is not defined because in that case the default
value is used.

Javier Carrasco (2):
  rtc: pcf85363: add support for the quartz-load-femtofarads property
  dt-bindings: rtc: nxp,pcf8563: add quartz-load-femtofarads for
    pcf85263 and pcf85363

 .../devicetree/bindings/rtc/nxp,pcf8563.yaml  | 20 ++++++++--
 drivers/rtc/rtc-pcf85363.c                    | 37 ++++++++++++++++++-
 2 files changed, 53 insertions(+), 4 deletions(-)

-- 
2.37.2


Javier Carrasco 
Research and Development

Wolfvision GmbH 
Oberes Ried 14 | 6833 Klaus | Austria 
Tel: +43 5523 52250 <tel:+43552352250> | Mail: javier.carrasco@wolfvision.net <mailto:javier.carrasco@wolfvision.net>

Website: wolfvision.com <www.wolfvision.com> 
Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria

[PATCH 1/2] rtc: pcf85363: add support for the quartz-load-femtofarads property

[Javier Carrasco]

The quartz oscillator load capacitance of the PCF85263 and PCF85363 can
be adjusted to 6 pF, 7 pF (default) and 12.5 pF with the CL[1:0] bits in
the oscillator control register (address 25h).

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 drivers/rtc/rtc-pcf85363.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index c05b722f0060..941f9264cf0a 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -101,6 +101,10 @@
 #define PIN_IO_INTA_OUT	2
 #define PIN_IO_INTA_HIZ	3
 
+#define OSC_CAP_SEL	GENMASK(1, 0)
+#define OSC_CAP_6000	0x01
+#define OSC_CAP_12500	0x02
+
 #define STOP_EN_STOP	BIT(0)
 
 #define RESET_CPR	0xa4
@@ -117,6 +121,32 @@ struct pcf85x63_config {
 	unsigned int num_nvram;
 };
 
+static int pcf85363_load_capacitance(struct pcf85363 *pcf85363, struct device_node *node)
+{
+	u32 load = 7000;
+	u8 value = 0;
+
+	of_property_read_u32(node, "quartz-load-femtofarads", &load);
+
+	switch (load) {
+	default:
+		dev_warn(&pcf85363->rtc->dev, "Unknown quartz-load-femtofarads value: %d. Assuming 7000",
+			 load);
+		fallthrough;
+	case 7000:
+		break;
+	case 6000:
+		value |= OSC_CAP_6000;
+		break;
+	case 12500:
+		value |= OSC_CAP_12500;
+		break;
+	}
+
+	return regmap_update_bits(pcf85363->regmap, CTRL_OSCILLATOR,
+				  OSC_CAP_SEL, value);
+}
+
 static int pcf85363_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
@@ -372,7 +402,7 @@ static int pcf85363_probe(struct i2c_client *client)
 			.reg_write = pcf85363_nvram_write,
 		},
 	};
-	int ret, i;
+	int ret, i, err;
 
 	if (data)
 		config = data;
@@ -394,6 +424,11 @@ static int pcf85363_probe(struct i2c_client *client)
 	if (IS_ERR(pcf85363->rtc))
 		return PTR_ERR(pcf85363->rtc);
 
+	err = pcf85363_load_capacitance(pcf85363, client->dev.of_node);
+	if (err < 0)
+		dev_warn(&client->dev, "failed to set xtal load capacitance: %d",
+			 err);
+
 	pcf85363->rtc->ops = &rtc_ops;
 	pcf85363->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
 	pcf85363->rtc->range_max = RTC_TIMESTAMP_END_2099;
-- 
2.37.2


Javier Carrasco 
Research and Development

Wolfvision GmbH 
Oberes Ried 14 | 6833 Klaus | Austria 
Tel: +43 5523 52250 <tel:+43552352250> | Mail: javier.carrasco@wolfvision.net <mailto:javier.carrasco@wolfvision.net>

Website: wolfvision.com <www.wolfvision.com> 
Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria

[PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add quartz-load-femtofarads for pcf85263 and pcf85363

[Javier Carrasco]

These RTCs are handled by the pcf85363 device driver, which now supports
the quartz-load-femtofarads property.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 .../devicetree/bindings/rtc/nxp,pcf8563.yaml  | 20 ++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
index a98b72752349..aac7f7565ba7 100644
--- a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
+++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
@@ -9,9 +9,6 @@ title: Philips PCF8563/Epson RTC8564 Real Time Clock
 maintainers:
   - Alexandre Belloni <alexandre.belloni@bootlin.com>
 
-allOf:
-  - $ref: rtc.yaml#
-
 properties:
   compatible:
     enum:
@@ -37,6 +34,23 @@ properties:
   start-year: true
   wakeup-source: true
 
+allOf:
+  - $ref: rtc.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - nxp,pcf85263
+              - nxp,pcf85363
+    then:
+      properties:
+        quartz-load-femtofarads:
+          description:
+            The capacitive load of the quartz(x-tal).
+          enum: [6000, 7000, 12500]
+          default: 7000
+
 required:
   - compatible
   - reg
-- 
2.37.2


Javier Carrasco 
Research and Development

Wolfvision GmbH 
Oberes Ried 14 | 6833 Klaus | Austria 
Tel: +43 5523 52250 <tel:+43552352250> | Mail: javier.carrasco@wolfvision.net <mailto:javier.carrasco@wolfvision.net>

Website: wolfvision.com <www.wolfvision.com> 
Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria

Re: [PATCH 1/2] rtc: pcf85363: add support for the quartz-load-femtofarads property

[Krzysztof Kozlowski]

On 13/02/2023 10:50, Javier Carrasco wrote:
> The quartz oscillator load capacitance of the PCF85263 and PCF85363 can
> be adjusted to 6 pF, 7 pF (default) and 12.5 pF with the CL[1:0] bits in
> the oscillator control register (address 25h).
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  drivers/rtc/rtc-pcf85363.c | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index c05b722f0060..941f9264cf0a 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
> @@ -101,6 +101,10 @@
>  #define PIN_IO_INTA_OUT	2
>  #define PIN_IO_INTA_HIZ	3
>  
> +#define OSC_CAP_SEL	GENMASK(1, 0)
> +#define OSC_CAP_6000	0x01
> +#define OSC_CAP_12500	0x02
> +
>  #define STOP_EN_STOP	BIT(0)
>  
>  #define RESET_CPR	0xa4
> @@ -117,6 +121,32 @@ struct pcf85x63_config {
>  	unsigned int num_nvram;
>  };
>  
> +static int pcf85363_load_capacitance(struct pcf85363 *pcf85363, struct device_node *node)
> +{
> +	u32 load = 7000;
> +	u8 value = 0;
> +
> +	of_property_read_u32(node, "quartz-load-femtofarads", &load);
> +
> +	switch (load) {
> +	default:
> +		dev_warn(&pcf85363->rtc->dev, "Unknown quartz-load-femtofarads value: %d. Assuming 7000",
> +			 load);
> +		fallthrough;
> +	case 7000:
> +		break;
> +	case 6000:
> +		value |= OSC_CAP_6000;
> +		break;
> +	case 12500:
> +		value |= OSC_CAP_12500;
> +		break;
> +	}
> +
> +	return regmap_update_bits(pcf85363->regmap, CTRL_OSCILLATOR,
> +				  OSC_CAP_SEL, value);
> +}
> +
>  static int pcf85363_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  {
>  	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> @@ -372,7 +402,7 @@ static int pcf85363_probe(struct i2c_client *client)
>  			.reg_write = pcf85363_nvram_write,
>  		},
>  	};
> -	int ret, i;
> +	int ret, i, err;
>  
>  	if (data)
>  		config = data;
> @@ -394,6 +424,11 @@ static int pcf85363_probe(struct i2c_client *client)
>  	if (IS_ERR(pcf85363->rtc))
>  		return PTR_ERR(pcf85363->rtc);
>  
> +	err = pcf85363_load_capacitance(pcf85363, client->dev.of_node);

Aren't you updating it for all variants? But the property is marked as
not valid for them.

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add quartz-load-femtofarads for pcf85263 and pcf85363

[Krzysztof Kozlowski]

On 13/02/2023 10:50, Javier Carrasco wrote:
> These RTCs are handled by the pcf85363 device driver, which now supports
> the quartz-load-femtofarads property.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  .../devicetree/bindings/rtc/nxp,pcf8563.yaml  | 20 ++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> index a98b72752349..aac7f7565ba7 100644
> --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> @@ -9,9 +9,6 @@ title: Philips PCF8563/Epson RTC8564 Real Time Clock
>  maintainers:
>    - Alexandre Belloni <alexandre.belloni@bootlin.com>
>  
> -allOf:
> -  - $ref: rtc.yaml#
> -
>  properties:
>    compatible:
>      enum:
> @@ -37,6 +34,23 @@ properties:
>    start-year: true
>    wakeup-source: true
>  
> +allOf:
> +  - $ref: rtc.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nxp,pcf85263
> +              - nxp,pcf85363
> +    then:
> +      properties:
> +        quartz-load-femtofarads:
> +          description:
> +            The capacitive load of the quartz(x-tal).
> +          enum: [6000, 7000, 12500]
> +          default: 7000

I don't think this will work. If you tested your DTS, you would see errors.

Please define the property in top-level and disallow it for other
variants. Other way to make it working would be to switch to
unevaluatedProperties, but defining properties in allOf:if:then: is not
that readable.

Best regards,
Krzysztof

Re: [PATCH 1/2] rtc: pcf85363: add support for the quartz-load-femtofarads property

[Alexandre Belloni]

On 13/02/2023 10:50:17+0100, Javier Carrasco wrote:
> The quartz oscillator load capacitance of the PCF85263 and PCF85363 can
> be adjusted to 6 pF, 7 pF (default) and 12.5 pF with the CL[1:0] bits in
> the oscillator control register (address 25h).
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  drivers/rtc/rtc-pcf85363.c | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index c05b722f0060..941f9264cf0a 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
> @@ -101,6 +101,10 @@
>  #define PIN_IO_INTA_OUT	2
>  #define PIN_IO_INTA_HIZ	3
>  
> +#define OSC_CAP_SEL	GENMASK(1, 0)
> +#define OSC_CAP_6000	0x01
> +#define OSC_CAP_12500	0x02
> +
>  #define STOP_EN_STOP	BIT(0)
>  
>  #define RESET_CPR	0xa4
> @@ -117,6 +121,32 @@ struct pcf85x63_config {
>  	unsigned int num_nvram;
>  };
>  
> +static int pcf85363_load_capacitance(struct pcf85363 *pcf85363, struct device_node *node)
> +{
> +	u32 load = 7000;
> +	u8 value = 0;
> +
> +	of_property_read_u32(node, "quartz-load-femtofarads", &load);
> +
> +	switch (load) {
> +	default:
> +		dev_warn(&pcf85363->rtc->dev, "Unknown quartz-load-femtofarads value: %d. Assuming 7000",
> +			 load);
> +		fallthrough;
> +	case 7000:
> +		break;
> +	case 6000:
> +		value |= OSC_CAP_6000;

Why are you using the |= operator?

> +		break;
> +	case 12500:
> +		value |= OSC_CAP_12500;
> +		break;
> +	}
> +
> +	return regmap_update_bits(pcf85363->regmap, CTRL_OSCILLATOR,
> +				  OSC_CAP_SEL, value);
> +}
> +
>  static int pcf85363_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  {
>  	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> @@ -372,7 +402,7 @@ static int pcf85363_probe(struct i2c_client *client)
>  			.reg_write = pcf85363_nvram_write,
>  		},
>  	};
> -	int ret, i;
> +	int ret, i, err;
>  
>  	if (data)
>  		config = data;
> @@ -394,6 +424,11 @@ static int pcf85363_probe(struct i2c_client *client)
>  	if (IS_ERR(pcf85363->rtc))
>  		return PTR_ERR(pcf85363->rtc);
>  
> +	err = pcf85363_load_capacitance(pcf85363, client->dev.of_node);
> +	if (err < 0)
> +		dev_warn(&client->dev, "failed to set xtal load capacitance: %d",
> +			 err);
> +
>  	pcf85363->rtc->ops = &rtc_ops;
>  	pcf85363->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
>  	pcf85363->rtc->range_max = RTC_TIMESTAMP_END_2099;
> -- 
> 2.37.2
> 
> 
> Javier Carrasco 
> Research and Development
> 
> Wolfvision GmbH 
> Oberes Ried 14 | 6833 Klaus | Austria 
> Tel: +43 5523 52250 <tel:+43552352250> | Mail: javier.carrasco@wolfvision.net <mailto:javier.carrasco@wolfvision.net>
> 
> Website: wolfvision.com <www.wolfvision.com> 
> Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria
> 

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

Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add quartz-load-femtofarads for pcf85263 and pcf85363

[Alexandre Belloni]

Hello,

Krzysztof's confusion is because you are changing the binding for
nxp,pcf8563 while adding support for the nxp,pcf85263/nxp,pcf85363

On 13/02/2023 10:50:18+0100, Javier Carrasco wrote:
> These RTCs are handled by the pcf85363 device driver, which now supports
> the quartz-load-femtofarads property.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  .../devicetree/bindings/rtc/nxp,pcf8563.yaml  | 20 ++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> index a98b72752349..aac7f7565ba7 100644
> --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> @@ -9,9 +9,6 @@ title: Philips PCF8563/Epson RTC8564 Real Time Clock
>  maintainers:
>    - Alexandre Belloni <alexandre.belloni@bootlin.com>
>  
> -allOf:
> -  - $ref: rtc.yaml#
> -
>  properties:
>    compatible:
>      enum:
> @@ -37,6 +34,23 @@ properties:
>    start-year: true
>    wakeup-source: true
>  
> +allOf:
> +  - $ref: rtc.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nxp,pcf85263
> +              - nxp,pcf85363
> +    then:
> +      properties:
> +        quartz-load-femtofarads:
> +          description:
> +            The capacitive load of the quartz(x-tal).
> +          enum: [6000, 7000, 12500]
> +          default: 7000
> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.37.2
> 
> 
> Javier Carrasco 
> Research and Development
> 
> Wolfvision GmbH 
> Oberes Ried 14 | 6833 Klaus | Austria 
> Tel: +43 5523 52250 <tel:+43552352250> | Mail: javier.carrasco@wolfvision.net <mailto:javier.carrasco@wolfvision.net>
> 
> Website: wolfvision.com <www.wolfvision.com> 
> Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria
> 

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

Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add quartz-load-femtofarads for pcf85263 and pcf85363

[Javier Carrasco Cruz]

Hello,

Sorry for the previous mail in html format with annoying signatures.

> Hello,
> 
> Krzysztof's confusion is because you are changing the binding for
> nxp,pcf8563 while adding support for the nxp,pcf85263/nxp,pcf85363

If that is ok I would propose a new bindings file nxp,pcf85363 for 
pcf85263 and pcf85363 in the next version.

Best regards,

Javier Carrasco

> 
> On 13/02/2023 10:50:18+0100, Javier Carrasco wrote:
>> These RTCs are handled by the pcf85363 device driver, which now supports
>> the quartz-load-femtofarads property.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
>> ---
>>   .../devicetree/bindings/rtc/nxp,pcf8563.yaml  | 20 ++++++++++++++++---
>>   1 file changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
>> index a98b72752349..aac7f7565ba7 100644
>> --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
>> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
>> @@ -9,9 +9,6 @@ title: Philips PCF8563/Epson RTC8564 Real Time Clock
>>   maintainers:
>>     - Alexandre Belloni <alexandre.belloni@bootlin.com>
>>   
>> -allOf:
>> -  - $ref: rtc.yaml#
>> -
>>   properties:
>>     compatible:
>>       enum:
>> @@ -37,6 +34,23 @@ properties:
>>     start-year: true
>>     wakeup-source: true
>>   
>> +allOf:
>> +  - $ref: rtc.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - nxp,pcf85263
>> +              - nxp,pcf85363
>> +    then:
>> +      properties:
>> +        quartz-load-femtofarads:
>> +          description:
>> +            The capacitive load of the quartz(x-tal).
>> +          enum: [6000, 7000, 12500]
>> +          default: 7000
>> +
>>   required:
>>     - compatible
>>     - reg
>> -- 
>> 2.37.2
>>
>>
>> Javier Carrasco
>> Research and Development
>>
>> Wolfvision GmbH
>> Oberes Ried 14 | 6833 Klaus | Austria
>> Tel: +43 5523 52250 <tel:+43552352250> | Mail: javier.carrasco@wolfvision.net <mailto:javier.carrasco@wolfvision.net>
>>
>> Website: wolfvision.com <www.wolfvision.com>
>> Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria
>>
> 

Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add quartz-load-femtofarads for pcf85263 and pcf85363

[Krzysztof Kozlowski]

On 14/02/2023 12:41, Javier Carrasco Cruz wrote:
> Hello,
> 
> Sorry for the previous mail in html format with annoying signatures.
> 
>> Hello,
>>
>> Krzysztof's confusion is because you are changing the binding for
>> nxp,pcf8563 while adding support for the nxp,pcf85263/nxp,pcf85363
> 
> If that is ok I would propose a new bindings file nxp,pcf85363 for 
> pcf85263 and pcf85363 in the next version.

Could be or still could be fixed in this binding. Indeed my comment in
the driver came from confusion that there are two drivers but one
binding. Having two bindings might be simpler and more readable in such
case.

Best regards,
Krzysztof