linux-rtc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] rtc: pcf85363: add support for high-impedance output
@ 2023-10-25 16:21 Javier Carrasco
  2023-10-25 16:21 ` [PATCH 1/2] " Javier Carrasco
  2023-10-25 16:21 ` [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property Javier Carrasco
  0 siblings, 2 replies; 13+ messages in thread
From: Javier Carrasco @ 2023-10-25 16:21 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: linux-rtc, linux-kernel, devicetree, Javier Carrasco

This series adds support for the high-impedance (hi-Z) output mode. The
current implementation only allows two output modes: output clock
(default) and interrupt. In cases where the output is only used in
low-power modes as a clock source for other devices (PMU, PMIC, etc)
or the output is not needed at all, the hi-Z can be used to reduce power
consumption.

To model the hi-Z output, a new property "hiz-output" has been added and
the following cases have been taken into account:
- hiz-output = enabled: the output is not available in the design and
  must be kept in high-impedance mode.
- hiz-output = sleep: the output is only required in sleep (low-power)
  mode.
- hiz-output = disabled (default): for the sake of completion, this mode
  ignores the hiz-output altogether and acts as if the property was not
  defined.

Any other value will trigger a dev_warn and the default value (disabled)
will be used as it is done with other non-mandatory properties.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
Javier Carrasco (2):
      rtc: pcf85363: add support for high-impedance output
      dt-bindings: rtc: nxp,pcf8563: add hiz-output property

 .../devicetree/bindings/rtc/nxp,pcf85363.yaml      | 14 +++++
 drivers/rtc/rtc-pcf85363.c                         | 62 +++++++++++++++++++++-
 2 files changed, 75 insertions(+), 1 deletion(-)
---
base-commit: cfb67623ce281e045ec11e3eddb1b68b879b53a1
change-id: 20231024-topic-pcf85363_hiz_output-d9c3c1f5ae59

Best regards,
-- 
Javier Carrasco <javier.carrasco@wolfvision.net>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/2] rtc: pcf85363: add support for high-impedance output
  2023-10-25 16:21 [PATCH 0/2] rtc: pcf85363: add support for high-impedance output Javier Carrasco
@ 2023-10-25 16:21 ` Javier Carrasco
  2023-10-25 16:21 ` [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property Javier Carrasco
  1 sibling, 0 replies; 13+ messages in thread
From: Javier Carrasco @ 2023-10-25 16:21 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: linux-rtc, linux-kernel, devicetree, Javier Carrasco

The pcf85363 provides a high-impedance (hi-Z) mode for its output. This
mode can be used to reduce power consumption in applications where the
RTC output is only required as a clock/interrupt source when the system
runs in low-power mode (i.e. serving as a clock source for a PMU when
the system is down). If the output is not needed, it can also be
completely disabled.

This implementation adds support for a hi-Z output and also uses simple
pm operations (suspend and resume) to switch the output mode from hi-Z
in normal operation to the required operation mode in sleep mode
(currently either clock or interrupt) if the "sleep" value for the
hiz-output was set.

In order to make use of the hi-Z output modeling via device tree,
check if the new "hiz-output" property is defined and set the hi-Z
mode accordingly.

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

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index 540042b9eec8..82eaf8de8b33 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -110,9 +110,17 @@
 
 #define NVRAM_SIZE	0x40
 
+enum pcf85363_hiz_output_t {
+	PCF85363_HIZ_OFF,
+	PCF85363_HIZ_ON,
+	PCF85363_HIZ_SLEEP,
+};
+
 struct pcf85363 {
 	struct rtc_device	*rtc;
 	struct regmap		*regmap;
+
+	enum pcf85363_hiz_output_t hiz_output;
 };
 
 struct pcf85x63_config {
@@ -403,6 +411,7 @@ static int pcf85363_probe(struct i2c_client *client)
 	};
 	int ret, i, err;
 	bool wakeup_source;
+	const char *hiz_output = NULL;
 
 	if (data)
 		config = data;
@@ -433,9 +442,32 @@ static int pcf85363_probe(struct i2c_client *client)
 	pcf85363->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
 	pcf85363->rtc->range_max = RTC_TIMESTAMP_END_2099;
 
+	if (device_property_present(&client->dev, "hiz-output")) {
+		ret = device_property_read_string(&client->dev, "hiz-output",
+						  &hiz_output);
+		if (ret)
+			return ret;
+
+		if (!strcmp(hiz_output, "enabled")) {
+			pcf85363->hiz_output = PCF85363_HIZ_ON;
+		} else if (!strcmp(hiz_output, "sleep")) {
+			pcf85363->hiz_output = PCF85363_HIZ_SLEEP;
+		} else if (!strcmp(hiz_output, "disabled")) {
+			pcf85363->hiz_output = PCF85363_HIZ_OFF;
+		} else {
+			dev_warn(&client->dev, "Unknown hiz-output: %s. Assuming disabled",
+				 hiz_output);
+			pcf85363->hiz_output = PCF85363_HIZ_OFF;
+		}
+	} else {
+		pcf85363->hiz_output = PCF85363_HIZ_OFF;
+	}
 	wakeup_source = device_property_read_bool(&client->dev,
 						  "wakeup-source");
-	if (client->irq > 0 || wakeup_source) {
+	if (pcf85363->hiz_output != PCF85363_HIZ_OFF) {
+		regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
+				   PIN_IO_INTAPM, PIN_IO_INTA_HIZ);
+	} else if (client->irq > 0 || wakeup_source) {
 		regmap_write(pcf85363->regmap, CTRL_FLAGS, 0);
 		regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
 				   PIN_IO_INTAPM, PIN_IO_INTA_OUT);
@@ -474,6 +506,31 @@ static int pcf85363_probe(struct i2c_client *client)
 	return ret;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int pcf85363_suspend(struct device *dev)
+{
+	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+
+	if (pcf85363->hiz_output == PCF85363_HIZ_SLEEP)
+		regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO, PIN_IO_INTAPM,
+				   device_may_wakeup(dev) ?  PIN_IO_INTA_OUT :
+				   PIN_IO_INTA_CLK);
+
+	return 0;
+}
+
+static int pcf85363_resume(struct device *dev)
+{
+	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+
+	if (pcf85363->hiz_output == PCF85363_HIZ_SLEEP)
+		regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
+				   PIN_IO_INTAPM, PIN_IO_INTA_HIZ);
+
+	return 0;
+}
+#endif
+
 static const __maybe_unused struct of_device_id dev_ids[] = {
 	{ .compatible = "nxp,pcf85263", .data = &pcf_85263_config },
 	{ .compatible = "nxp,pcf85363", .data = &pcf_85363_config },
@@ -481,9 +538,12 @@ static const __maybe_unused struct of_device_id dev_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, dev_ids);
 
+static SIMPLE_DEV_PM_OPS(pcf85363_pm_ops, pcf85363_suspend, pcf85363_resume);
+
 static struct i2c_driver pcf85363_driver = {
 	.driver	= {
 		.name	= "pcf85363",
+		.pm	= &pcf85363_pm_ops,
 		.of_match_table = of_match_ptr(dev_ids),
 	},
 	.probe = pcf85363_probe,

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property
  2023-10-25 16:21 [PATCH 0/2] rtc: pcf85363: add support for high-impedance output Javier Carrasco
  2023-10-25 16:21 ` [PATCH 1/2] " Javier Carrasco
@ 2023-10-25 16:21 ` Javier Carrasco
  2023-10-25 22:23   ` Alexandre Belloni
  1 sibling, 1 reply; 13+ messages in thread
From: Javier Carrasco @ 2023-10-25 16:21 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: linux-rtc, linux-kernel, devicetree, Javier Carrasco

The "hiz-output" property models the RTC output as a high-impedance
(hi-Z) output.

This property is optional and if it is not defined, the output will
either act as an output clock (default mode) or as an interrupt
depending on the configuration set by other properties.

Two modes are defined in case the high-impedance is used: "enabled" and
"sleep". The former disables the RTC output completely while the latter
keeps the RTC output disabled until the system enters in sleep mode.
This option is especially relevant if the output clock is used to feed a
PMU, a PMIC or any other device required to run when the rest of the
system is down. For the sake of completeness, a "disabled" mode has been
added, which acts as if the property was not defined.

Document "hiz-output" as a non-required property.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
index 52aa3e2091e9..4b27a9154191 100644
--- a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
+++ b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
@@ -36,6 +36,19 @@ properties:
     enum: [6000, 7000, 12500]
     default: 7000
 
+  hiz-output:
+    description:
+      Use enabled if the output should stay in high-impedance. This
+      mode will mask the output as an interrupt source.
+      Use sleep if the otuput should be only active in sleep mode.
+      This mode is compatible with any other output configuration.
+      The disabled value acts as if the property was not defined.
+    enum:
+      - enabled
+      - sleep
+      - disabled
+    default: disabled
+
   start-year: true
   wakeup-source: true
 
@@ -56,5 +69,6 @@ examples:
             reg = <0x51>;
             #clock-cells = <0>;
             quartz-load-femtofarads = <12500>;
+            hiz-output = "sleep";
         };
     };

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property
  2023-10-25 16:21 ` [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property Javier Carrasco
@ 2023-10-25 22:23   ` Alexandre Belloni
  2023-10-25 22:30     ` Javier Carrasco
  0 siblings, 1 reply; 13+ messages in thread
From: Alexandre Belloni @ 2023-10-25 22:23 UTC (permalink / raw)
  To: Javier Carrasco
  Cc: Alessandro Zummo, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-rtc, linux-kernel, devicetree

Hello,

I'm sorry I never replied to your previous thread...

On 25/10/2023 18:21:55+0200, Javier Carrasco wrote:
> The "hiz-output" property models the RTC output as a high-impedance
> (hi-Z) output.
> 
> This property is optional and if it is not defined, the output will
> either act as an output clock (default mode) or as an interrupt
> depending on the configuration set by other properties.
> 
> Two modes are defined in case the high-impedance is used: "enabled" and
> "sleep". The former disables the RTC output completely while the latter
> keeps the RTC output disabled until the system enters in sleep mode.
> This option is especially relevant if the output clock is used to feed a
> PMU, a PMIC or any other device required to run when the rest of the
> system is down. For the sake of completeness, a "disabled" mode has been
> added, which acts as if the property was not defined.
> 
> Document "hiz-output" as a non-required property.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
> index 52aa3e2091e9..4b27a9154191 100644
> --- a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
> @@ -36,6 +36,19 @@ properties:
>      enum: [6000, 7000, 12500]
>      default: 7000
>  
> +  hiz-output:
> +    description:
> +      Use enabled if the output should stay in high-impedance. This
> +      mode will mask the output as an interrupt source.
> +      Use sleep if the otuput should be only active in sleep mode.
> +      This mode is compatible with any other output configuration.
> +      The disabled value acts as if the property was not defined.
> +    enum:
> +      - enabled
> +      - sleep
> +      - disabled
> +    default: disabled
> +

If instead of using a custom property, you consider this as what it
actually is: pinmuxing, then everything else comes for free. With
pinctrl, you can define different states for runtime and sleep and they
will get applied automatically instead of open coding in the driver.

Also, how you define this property means that everyone currently using
this RTC is going to have a new warning that they should just ignore.


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property
  2023-10-25 22:23   ` Alexandre Belloni
@ 2023-10-25 22:30     ` Javier Carrasco
  2023-10-25 23:23       ` Javier Carrasco
  0 siblings, 1 reply; 13+ messages in thread
From: Javier Carrasco @ 2023-10-25 22:30 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Alessandro Zummo, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-rtc, linux-kernel, devicetree

Hi Alexandre,

On 26.10.23 00:23, Alexandre Belloni wrote:
> Hello,
> 
> I'm sorry I never replied to your previous thread...
> 
> On 25/10/2023 18:21:55+0200, Javier Carrasco wrote:
>> The "hiz-output" property models the RTC output as a high-impedance
>> (hi-Z) output.
>>
>> This property is optional and if it is not defined, the output will
>> either act as an output clock (default mode) or as an interrupt
>> depending on the configuration set by other properties.
>>
>> Two modes are defined in case the high-impedance is used: "enabled" and
>> "sleep". The former disables the RTC output completely while the latter
>> keeps the RTC output disabled until the system enters in sleep mode.
>> This option is especially relevant if the output clock is used to feed a
>> PMU, a PMIC or any other device required to run when the rest of the
>> system is down. For the sake of completeness, a "disabled" mode has been
>> added, which acts as if the property was not defined.
>>
>> Document "hiz-output" as a non-required property.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
>> ---
>>  Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
>> index 52aa3e2091e9..4b27a9154191 100644
>> --- a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
>> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
>> @@ -36,6 +36,19 @@ properties:
>>      enum: [6000, 7000, 12500]
>>      default: 7000
>>  
>> +  hiz-output:
>> +    description:
>> +      Use enabled if the output should stay in high-impedance. This
>> +      mode will mask the output as an interrupt source.
>> +      Use sleep if the otuput should be only active in sleep mode.
>> +      This mode is compatible with any other output configuration.
>> +      The disabled value acts as if the property was not defined.
>> +    enum:
>> +      - enabled
>> +      - sleep
>> +      - disabled
>> +    default: disabled
>> +
> 
> If instead of using a custom property, you consider this as what it
> actually is: pinmuxing, then everything else comes for free. With
> pinctrl, you can define different states for runtime and sleep and they
> will get applied automatically instead of open coding in the driver.
> 
> Also, how you define this property means that everyone currently using
> this RTC is going to have a new warning that they should just ignore.
> 
> 
Thanks for your reply. The warning can only be triggered if the property
is defined, so in principle no one could have that warning yet. Only the
ones who actually define it and use an invalid value would get the warning.

On the other hand I did not consider your approach, which might make
this patch irrelevant. So I will have a look at it to make sure that it
achieves the same results.

Thanks again and best regards,
Javier Carrasco

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property
  2023-10-25 22:30     ` Javier Carrasco
@ 2023-10-25 23:23       ` Javier Carrasco
  2023-10-26  0:50         ` Alexandre Belloni
  0 siblings, 1 reply; 13+ messages in thread
From: Javier Carrasco @ 2023-10-25 23:23 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Alessandro Zummo, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-rtc, linux-kernel, devicetree

Hi,

On 26.10.23 00:30, Javier Carrasco wrote:
> Hi Alexandre,
> 
> On 26.10.23 00:23, Alexandre Belloni wrote:
>> Hello,
>>
>> I'm sorry I never replied to your previous thread...
>>
>> On 25/10/2023 18:21:55+0200, Javier Carrasco wrote:
>>> The "hiz-output" property models the RTC output as a high-impedance
>>> (hi-Z) output.
>>>
>>> This property is optional and if it is not defined, the output will
>>> either act as an output clock (default mode) or as an interrupt
>>> depending on the configuration set by other properties.
>>>
>>> Two modes are defined in case the high-impedance is used: "enabled" and
>>> "sleep". The former disables the RTC output completely while the latter
>>> keeps the RTC output disabled until the system enters in sleep mode.
>>> This option is especially relevant if the output clock is used to feed a
>>> PMU, a PMIC or any other device required to run when the rest of the
>>> system is down. For the sake of completeness, a "disabled" mode has been
>>> added, which acts as if the property was not defined.
>>>
>>> Document "hiz-output" as a non-required property.
>>>
>>> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
>>> ---
>>>  Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
>>> index 52aa3e2091e9..4b27a9154191 100644
>>> --- a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
>>> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
>>> @@ -36,6 +36,19 @@ properties:
>>>      enum: [6000, 7000, 12500]
>>>      default: 7000
>>>  
>>> +  hiz-output:
>>> +    description:
>>> +      Use enabled if the output should stay in high-impedance. This
>>> +      mode will mask the output as an interrupt source.
>>> +      Use sleep if the otuput should be only active in sleep mode.
>>> +      This mode is compatible with any other output configuration.
>>> +      The disabled value acts as if the property was not defined.
>>> +    enum:
>>> +      - enabled
>>> +      - sleep
>>> +      - disabled
>>> +    default: disabled
>>> +
>>
>> If instead of using a custom property, you consider this as what it
>> actually is: pinmuxing, then everything else comes for free. With
>> pinctrl, you can define different states for runtime and sleep and they
>> will get applied automatically instead of open coding in the driver.

I am not sure if your solution would cover all my needs:

1.- With pinctrl I can model the SoC pins, right? That would not stop
the RTC output though, so the 32 kHz signal would be generated anyways
even though the SoC would ignore it. That is one of the things I want to
avoid.

2.- What happens if the RTC output is a clock for an external device
that is only required when the SoC is in sleep mode? In that case I
would like the RTC driver to control the output with the modes it provides.
>>
>> Also, how you define this property means that everyone currently using
>> this RTC is going to have a new warning that they should just ignore.
>>
>>
> Thanks for your reply. The warning can only be triggered if the property
> is defined, so in principle no one could have that warning yet. Only the
> ones who actually define it and use an invalid value would get the warning.
> 
> On the other hand I did not consider your approach, which might make
> this patch irrelevant. So I will have a look at it to make sure that it
> achieves the same results.
> 
> Thanks again and best regards,
> Javier Carrasco
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property
  2023-10-25 23:23       ` Javier Carrasco
@ 2023-10-26  0:50         ` Alexandre Belloni
  2023-10-26  9:41           ` Javier Carrasco
  0 siblings, 1 reply; 13+ messages in thread
From: Alexandre Belloni @ 2023-10-26  0:50 UTC (permalink / raw)
  To: Javier Carrasco
  Cc: Alessandro Zummo, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-rtc, linux-kernel, devicetree

On 26/10/2023 01:23:21+0200, Javier Carrasco wrote:
> >>> +  hiz-output:
> >>> +    description:
> >>> +      Use enabled if the output should stay in high-impedance. This
> >>> +      mode will mask the output as an interrupt source.
> >>> +      Use sleep if the otuput should be only active in sleep mode.
> >>> +      This mode is compatible with any other output configuration.
> >>> +      The disabled value acts as if the property was not defined.
> >>> +    enum:
> >>> +      - enabled
> >>> +      - sleep
> >>> +      - disabled
> >>> +    default: disabled
> >>> +
> >>
> >> If instead of using a custom property, you consider this as what it
> >> actually is: pinmuxing, then everything else comes for free. With
> >> pinctrl, you can define different states for runtime and sleep and they
> >> will get applied automatically instead of open coding in the driver.
> 
> I am not sure if your solution would cover all my needs:
> 
> 1.- With pinctrl I can model the SoC pins, right? That would not stop
> the RTC output though, so the 32 kHz signal would be generated anyways
> even though the SoC would ignore it. That is one of the things I want to
> avoid.
> 

No, you would model the INTA pin.

> 2.- What happens if the RTC output is a clock for an external device
> that is only required when the SoC is in sleep mode? In that case I
> would like the RTC driver to control the output with the modes it provides.

Even if I doubt this is a valid use case, this would be possible as long
as the external device node has the correct pinctrl-* properties.


> >>
> >> Also, how you define this property means that everyone currently using
> >> this RTC is going to have a new warning that they should just ignore.
> >>
> >>
> > Thanks for your reply. The warning can only be triggered if the property
> > is defined, so in principle no one could have that warning yet. Only the
> > ones who actually define it and use an invalid value would get the warning.
> > 
> > On the other hand I did not consider your approach, which might make
> > this patch irrelevant. So I will have a look at it to make sure that it
> > achieves the same results.
> > 
> > Thanks again and best regards,
> > Javier Carrasco
> > 

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property
  2023-10-26  0:50         ` Alexandre Belloni
@ 2023-10-26  9:41           ` Javier Carrasco
  2023-10-26  9:56             ` Alexandre Belloni
  0 siblings, 1 reply; 13+ messages in thread
From: Javier Carrasco @ 2023-10-26  9:41 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Alessandro Zummo, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-rtc, linux-kernel, devicetree


On 26.10.23 02:50, Alexandre Belloni wrote:
> On 26/10/2023 01:23:21+0200, Javier Carrasco wrote:
>>>>> +  hiz-output:
>>>>> +    description:
>>>>> +      Use enabled if the output should stay in high-impedance. This
>>>>> +      mode will mask the output as an interrupt source.
>>>>> +      Use sleep if the otuput should be only active in sleep mode.
>>>>> +      This mode is compatible with any other output configuration.
>>>>> +      The disabled value acts as if the property was not defined.
>>>>> +    enum:
>>>>> +      - enabled
>>>>> +      - sleep
>>>>> +      - disabled
>>>>> +    default: disabled
>>>>> +
>>>>
>>>> If instead of using a custom property, you consider this as what it
>>>> actually is: pinmuxing, then everything else comes for free. With
>>>> pinctrl, you can define different states for runtime and sleep and they
>>>> will get applied automatically instead of open coding in the driver.
>>
>> I am not sure if your solution would cover all my needs:
>>
>> 1.- With pinctrl I can model the SoC pins, right? That would not stop
>> the RTC output though, so the 32 kHz signal would be generated anyways
>> even though the SoC would ignore it. That is one of the things I want to
>> avoid.
>>
> 
> No, you would model the INTA pin.
I am sorry for insisting on this topic, but if I get you right, I would
be modeling an interrupt pin (INTA) to keep it from generating a clock
signal when the RTC itself offers a high-impedance mode i.e. avoiding to
use the RTC feature.

Is that not a misuse of the INTA pin in the first place? If there was no
other option, that would be an easy fix, but why would we not implement
the hi-Z mode when it is available? If I see a pinctrl-* modeling an
interrupt pin, it is not obvious that I am doing that to stop the clock
signal and I would have to clarify it explicitly, especially if I am not
interested in the interrupt.

I would rather implement and document the hi-Z mode the RTC offers
instead of using another mode like INTA which actually can trigger
interrupts. If the implementation must be different is of course another
topic.
> 
>> 2.- What happens if the RTC output is a clock for an external device
>> that is only required when the SoC is in sleep mode? In that case I
>> would like the RTC driver to control the output with the modes it provides.
> 
> Even if I doubt this is a valid use case, this would be possible as long
> as the external device node has the correct pinctrl-* properties.
> 
> 
>>>>
>>>> Also, how you define this property means that everyone currently using
>>>> this RTC is going to have a new warning that they should just ignore.
>>>>
>>>>
>>> Thanks for your reply. The warning can only be triggered if the property
>>> is defined, so in principle no one could have that warning yet. Only the
>>> ones who actually define it and use an invalid value would get the warning.
>>>
>>> On the other hand I did not consider your approach, which might make
>>> this patch irrelevant. So I will have a look at it to make sure that it
>>> achieves the same results.
>>>
>>> Thanks again and best regards,
>>> Javier Carrasco
>>>
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property
  2023-10-26  9:41           ` Javier Carrasco
@ 2023-10-26  9:56             ` Alexandre Belloni
  2023-10-26 10:13               ` Javier Carrasco
  2024-01-30 21:34               ` Javier Carrasco
  0 siblings, 2 replies; 13+ messages in thread
From: Alexandre Belloni @ 2023-10-26  9:56 UTC (permalink / raw)
  To: Javier Carrasco
  Cc: Alessandro Zummo, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-rtc, linux-kernel, devicetree

On 26/10/2023 11:41:47+0200, Javier Carrasco wrote:
> 
> On 26.10.23 02:50, Alexandre Belloni wrote:
> > On 26/10/2023 01:23:21+0200, Javier Carrasco wrote:
> >>>>> +  hiz-output:
> >>>>> +    description:
> >>>>> +      Use enabled if the output should stay in high-impedance. This
> >>>>> +      mode will mask the output as an interrupt source.
> >>>>> +      Use sleep if the otuput should be only active in sleep mode.
> >>>>> +      This mode is compatible with any other output configuration.
> >>>>> +      The disabled value acts as if the property was not defined.
> >>>>> +    enum:
> >>>>> +      - enabled
> >>>>> +      - sleep
> >>>>> +      - disabled
> >>>>> +    default: disabled
> >>>>> +
> >>>>
> >>>> If instead of using a custom property, you consider this as what it
> >>>> actually is: pinmuxing, then everything else comes for free. With
> >>>> pinctrl, you can define different states for runtime and sleep and they
> >>>> will get applied automatically instead of open coding in the driver.
> >>
> >> I am not sure if your solution would cover all my needs:
> >>
> >> 1.- With pinctrl I can model the SoC pins, right? That would not stop
> >> the RTC output though, so the 32 kHz signal would be generated anyways
> >> even though the SoC would ignore it. That is one of the things I want to
> >> avoid.
> >>
> > 
> > No, you would model the INTA pin.
> I am sorry for insisting on this topic, but if I get you right, I would
> be modeling an interrupt pin (INTA) to keep it from generating a clock
> signal when the RTC itself offers a high-impedance mode i.e. avoiding to
> use the RTC feature.
> 
> Is that not a misuse of the INTA pin in the first place? If there was no
> other option, that would be an easy fix, but why would we not implement
> the hi-Z mode when it is available? If I see a pinctrl-* modeling an
> interrupt pin, it is not obvious that I am doing that to stop the clock
> signal and I would have to clarify it explicitly, especially if I am not
> interested in the interrupt.
> 
> I would rather implement and document the hi-Z mode the RTC offers
> instead of using another mode like INTA which actually can trigger
> interrupts. If the implementation must be different is of course another
> topic.

There is a pin named INTA, it can mux 4 different functions:

 - clock output
 - battery mode indication
 - interrupt
 - HiZ

with pinmuxing, you can select which function you want for the pin. I'm
not sure what is misused there.

Can you explain what is your actual use case? I'm starting to understand
that what you want is simply disable clock out because you are not using
the interrupt.

If we assume we are never going to use battery mode, then we could
simply used the CCF for this like the other RTC drivers.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property
  2023-10-26  9:56             ` Alexandre Belloni
@ 2023-10-26 10:13               ` Javier Carrasco
  2023-10-26 10:21                 ` Alexandre Belloni
  2024-01-30 21:34               ` Javier Carrasco
  1 sibling, 1 reply; 13+ messages in thread
From: Javier Carrasco @ 2023-10-26 10:13 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Alessandro Zummo, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-rtc, linux-kernel, devicetree

On 26.10.23 11:56, Alexandre Belloni wrote:
> On 26/10/2023 11:41:47+0200, Javier Carrasco wrote:
>>
>> On 26.10.23 02:50, Alexandre Belloni wrote:
>>> On 26/10/2023 01:23:21+0200, Javier Carrasco wrote:
>>>>>>> +  hiz-output:
>>>>>>> +    description:
>>>>>>> +      Use enabled if the output should stay in high-impedance. This
>>>>>>> +      mode will mask the output as an interrupt source.
>>>>>>> +      Use sleep if the otuput should be only active in sleep mode.
>>>>>>> +      This mode is compatible with any other output configuration.
>>>>>>> +      The disabled value acts as if the property was not defined.
>>>>>>> +    enum:
>>>>>>> +      - enabled
>>>>>>> +      - sleep
>>>>>>> +      - disabled
>>>>>>> +    default: disabled
>>>>>>> +
>>>>>>
>>>>>> If instead of using a custom property, you consider this as what it
>>>>>> actually is: pinmuxing, then everything else comes for free. With
>>>>>> pinctrl, you can define different states for runtime and sleep and they
>>>>>> will get applied automatically instead of open coding in the driver.
>>>>
>>>> I am not sure if your solution would cover all my needs:
>>>>
>>>> 1.- With pinctrl I can model the SoC pins, right? That would not stop
>>>> the RTC output though, so the 32 kHz signal would be generated anyways
>>>> even though the SoC would ignore it. That is one of the things I want to
>>>> avoid.
>>>>
>>>
>>> No, you would model the INTA pin.
>> I am sorry for insisting on this topic, but if I get you right, I would
>> be modeling an interrupt pin (INTA) to keep it from generating a clock
>> signal when the RTC itself offers a high-impedance mode i.e. avoiding to
>> use the RTC feature.
>>
>> Is that not a misuse of the INTA pin in the first place? If there was no
>> other option, that would be an easy fix, but why would we not implement
>> the hi-Z mode when it is available? If I see a pinctrl-* modeling an
>> interrupt pin, it is not obvious that I am doing that to stop the clock
>> signal and I would have to clarify it explicitly, especially if I am not
>> interested in the interrupt.
>>
>> I would rather implement and document the hi-Z mode the RTC offers
>> instead of using another mode like INTA which actually can trigger
>> interrupts. If the implementation must be different is of course another
>> topic.
> 
> There is a pin named INTA, it can mux 4 different functions:
> 
>  - clock output
>  - battery mode indication
>  - interrupt
>  - HiZ
> 
> with pinmuxing, you can select which function you want for the pin. I'm
> not sure what is misused there.
> 
> Can you explain what is your actual use case? I'm starting to understand
> that what you want is simply disable clock out because you are not using
> the interrupt.
> 
> If we assume we are never going to use battery mode, then we could
> simply used the CCF for this like the other RTC drivers.
> 
I want to model the INTA pin as a clock source that only should run in
sleep mode because its clock is only used in that mode. Therefore I want
the pin to stay in hi-Z during normal operation.

I do not want to get any interrupts from the INTA pin and the battery
mode indication is not relevant for me either. I do not know the CCF
mechanism in other RTCs though, but I think that the hi-Z mode
accomplishes exactly what I described.The assumption about the battery
mode is therefore beyond my knowledge, but my first reaction is that we
already have the hi-Z for that.

So in the end I just need a mechanism to configure INTA as hi-Z, which
the driver still does not support. There is another application where
the clock output is not required even though it is physically connected,
so hi-Z is again an interesting mode and the battery mode would be
available if it ever becomes relevant for anyone.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property
  2023-10-26 10:13               ` Javier Carrasco
@ 2023-10-26 10:21                 ` Alexandre Belloni
  2023-10-26 10:35                   ` Javier Carrasco
  0 siblings, 1 reply; 13+ messages in thread
From: Alexandre Belloni @ 2023-10-26 10:21 UTC (permalink / raw)
  To: Javier Carrasco
  Cc: Alessandro Zummo, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-rtc, linux-kernel, devicetree

On 26/10/2023 12:13:23+0200, Javier Carrasco wrote:
> I want to model the INTA pin as a clock source that only should run in
> sleep mode because its clock is only used in that mode. Therefore I want
> the pin to stay in hi-Z during normal operation.

Can you disclose what is the user of the clock, do you have a driver for
this device?

> 
> I do not want to get any interrupts from the INTA pin and the battery
> mode indication is not relevant for me either. I do not know the CCF
> mechanism in other RTCs though, but I think that the hi-Z mode
> accomplishes exactly what I described.The assumption about the battery
> mode is therefore beyond my knowledge, but my first reaction is that we
> already have the hi-Z for that.
> 
> So in the end I just need a mechanism to configure INTA as hi-Z, which
> the driver still does not support. There is another application where
> the clock output is not required even though it is physically connected,
> so hi-Z is again an interesting mode and the battery mode would be
> available if it ever becomes relevant for anyone.
> 

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property
  2023-10-26 10:21                 ` Alexandre Belloni
@ 2023-10-26 10:35                   ` Javier Carrasco
  0 siblings, 0 replies; 13+ messages in thread
From: Javier Carrasco @ 2023-10-26 10:35 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Alessandro Zummo, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-rtc, linux-kernel, devicetree

On 26.10.23 12:21, Alexandre Belloni wrote:
> On 26/10/2023 12:13:23+0200, Javier Carrasco wrote:
>> I want to model the INTA pin as a clock source that only should run in
>> sleep mode because its clock is only used in that mode. Therefore I want
>> the pin to stay in hi-Z during normal operation.
> 
> Can you disclose what is the user of the clock, do you have a driver for
> this device?
> 
It is a Rockchip PMU through its CLK32K_IN pin. I can't disclose the
exact model (yet) though, but the use case is that the PMU can run with
the RTC clock connected to that pin in low-power modes.
That pin is configured in the proper mode and maybe it could be
configured differently with pinctrl in "default" mode, but the RTC INTA
would still output the clock, which is what I want to avoid in this
particular case.
I just want to stop the clock at the RTC end i.e. write PIN_IO_INTA_HIZ
into the PIN_IO_INTAPM.
>>
>> I do not want to get any interrupts from the INTA pin and the battery
>> mode indication is not relevant for me either. I do not know the CCF
>> mechanism in other RTCs though, but I think that the hi-Z mode
>> accomplishes exactly what I described.The assumption about the battery
>> mode is therefore beyond my knowledge, but my first reaction is that we
>> already have the hi-Z for that.
>>
>> So in the end I just need a mechanism to configure INTA as hi-Z, which
>> the driver still does not support. There is another application where
>> the clock output is not required even though it is physically connected,
>> so hi-Z is again an interesting mode and the battery mode would be
>> available if it ever becomes relevant for anyone.
>>
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property
  2023-10-26  9:56             ` Alexandre Belloni
  2023-10-26 10:13               ` Javier Carrasco
@ 2024-01-30 21:34               ` Javier Carrasco
  1 sibling, 0 replies; 13+ messages in thread
From: Javier Carrasco @ 2024-01-30 21:34 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Alessandro Zummo, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-rtc, linux-kernel, devicetree

Hello Alexandre,

On 26.10.23 11:56, Alexandre Belloni wrote:

> There is a pin named INTA, it can mux 4 different functions:
> 
>  - clock output
>  - battery mode indication
>  - interrupt
>  - HiZ
> 
> with pinmuxing, you can select which function you want for the pin. I'm
> not sure what is misused there.
> 
> Can you explain what is your actual use case? I'm starting to understand
> that what you want is simply disable clock out because you are not using
> the interrupt.
> 
> If we assume we are never going to use battery mode, then we could
> simply used the CCF for this like the other RTC drivers.
> 

Could you please point me to an RTC driver that uses the CCF for a
similar application? I am not sure what you mean with the battery mode
to make use of the HiZ, and I would like to push forward with this
functionality.

Thank you.

Best regards,
Javier Carrasco

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2024-01-30 21:34 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-25 16:21 [PATCH 0/2] rtc: pcf85363: add support for high-impedance output Javier Carrasco
2023-10-25 16:21 ` [PATCH 1/2] " Javier Carrasco
2023-10-25 16:21 ` [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add hiz-output property Javier Carrasco
2023-10-25 22:23   ` Alexandre Belloni
2023-10-25 22:30     ` Javier Carrasco
2023-10-25 23:23       ` Javier Carrasco
2023-10-26  0:50         ` Alexandre Belloni
2023-10-26  9:41           ` Javier Carrasco
2023-10-26  9:56             ` Alexandre Belloni
2023-10-26 10:13               ` Javier Carrasco
2023-10-26 10:21                 ` Alexandre Belloni
2023-10-26 10:35                   ` Javier Carrasco
2024-01-30 21:34               ` Javier Carrasco

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).