diff mbox series

[v3,1/2] dt-bindings: rtc: pcf85263/pcf85363: add some properties

Message ID 20190903061853.19793-1-biwen.li@nxp.com
State Changes Requested, archived
Headers show
Series [v3,1/2] dt-bindings: rtc: pcf85263/pcf85363: add some properties | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Biwen Li Sept. 3, 2019, 6:18 a.m. UTC 
Add some properties for pcf85263/pcf85363 as follows:
  - interrupt-output-pin: string type
  - quartz-load-femtofarads: integer type
  - nxp,quartz-drive-strength: integer type
  - nxp,quartz-low-jitter: bool type
  - wakeup-source: bool type

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
Signed-off-by: Biwen Li <biwen.li@nxp.com>
---
Change in v3:
	- None

Change in v2:
	- Replace properties name
	  quartz-load-capacitance -> quartz-load-femtofarads
	  quartz-drive-strength -> nxp,quartz-drive-strength
	  quartz-low-jitter -> nxp,quartz-low-jitter
	- Replace drive strength name
	  PCF85263_QUARTZDRIVE_NORMAL -> PCF85263_QUARTZDRIVE_100ko
	  PCF85263_QUARTZDRIVE_LOW -> PCF85263_QUARTZDRIVE_60ko
	  PCF85263_QUARTZDRIVE_HIGH -> PCF85263_QUARTZDRIVE_500ko
	- Set default interrupt-output-pin as "INTA"

 .../devicetree/bindings/rtc/pcf85363.txt      | 29 +++++++++++++++++++
 include/dt-bindings/rtc/pcf85363.h            | 15 ++++++++++
 2 files changed, 44 insertions(+)
 create mode 100644 include/dt-bindings/rtc/pcf85363.h

Comments

Martin Fuzzey Sept. 3, 2019, 9:37 a.m. UTC | #1 
On 03/09/2019 08:18, Biwen Li wrote:
> diff --git a/Documentation/devicetree/bindings/rtc/pcf85363.txt b/Documentation/devicetree/bindings/rtc/pcf85363.txt
> index 94adc1cf93d9..588f688b30d1 100644
> --- a/Documentation/devicetree/bindings/rtc/pcf85363.txt
> +++ b/Documentation/devicetree/bindings/rtc/pcf85363.txt
> @@ -8,10 +8,39 @@ Required properties:
>   Optional properties:
>   - interrupts: IRQ line for the RTC (not implemented).
>   
> +- interrupt-output-pin: The interrupt output pin must be
> +  "INTA" or "INTB", default value is "INTA"
> +


The hardware has 2 interrupt pins which can be mapped to various 
interrupt sources (alarm1, alarm2, periodic, ...)

Currently the driver only supports alarm1.

It is even possible to use both pins for the same interrupt (eg if INTA 
were wired to the SoC, INTB to a PMIC and both used for alarm...)


So maybe it would be better to have

alarm1-interrupt-output-pin: The interrupt output pin used for the alarm 
function. Must be "INTA", "INTB" or "BOTH"

Then, if and when other types of interrupts are supported by the driver 
new properties could be added for them.



> +- quartz-load-femtofarads: The internal capacitor to select for the quartz:
> +	PCF85263_QUARTZCAP_7pF		[0]
> +	PCF85263_QUARTZCAP_6pF		[1]
> +	PCF85263_QUARTZCAP_12p5pF	[2] DEFAULT
> +


The standard DT property "quartz-load-femtofarads" takes the real 
physical value in femto Farads ie values should be 7000, 6000, 12500 
without defines.


> +- nxp,quartz-drive-strength: Drive strength for the quartz:
> +	PCF85263_QUARTZDRIVE_100ko	[0] DEFAULT
> +	PCF85263_QUARTZDRIVE_60ko	[1]
> +	PCF85263_QUARTZDRIVE_500ko	[2]
> +


Not sure about this.

Wouldn't it be better to either use a real impedence value in ohms (like 
load property above, even though it is a vendor specific value) rather 
than a define, or defines for "Low, Medium, High"?


Martin
Rob Herring Sept. 3, 2019, 5:36 p.m. UTC | #2 
On Tue, Sep 03, 2019 at 11:37:01AM +0200, Martin Fuzzey wrote:
> On 03/09/2019 08:18, Biwen Li wrote:
> > diff --git a/Documentation/devicetree/bindings/rtc/pcf85363.txt b/Documentation/devicetree/bindings/rtc/pcf85363.txt
> > index 94adc1cf93d9..588f688b30d1 100644
> > --- a/Documentation/devicetree/bindings/rtc/pcf85363.txt
> > +++ b/Documentation/devicetree/bindings/rtc/pcf85363.txt
> > @@ -8,10 +8,39 @@ Required properties:
> >   Optional properties:
> >   - interrupts: IRQ line for the RTC (not implemented).
> > +- interrupt-output-pin: The interrupt output pin must be
> > +  "INTA" or "INTB", default value is "INTA"
> > +
> 
> 
> The hardware has 2 interrupt pins which can be mapped to various interrupt
> sources (alarm1, alarm2, periodic, ...)
> 
> Currently the driver only supports alarm1.
> 
> It is even possible to use both pins for the same interrupt (eg if INTA were
> wired to the SoC, INTB to a PMIC and both used for alarm...)
> 
> 
> So maybe it would be better to have
> 
> alarm1-interrupt-output-pin: The interrupt output pin used for the alarm
> function. Must be "INTA", "INTB" or "BOTH"

That's a property per source. 2 properties possible sources (either a 
mask or list) would be my preference.

Also, whatever you end up with needs a vendor prefix.

> 
> Then, if and when other types of interrupts are supported by the driver new
> properties could be added for them.
> 
> 
> 
> > +- quartz-load-femtofarads: The internal capacitor to select for the quartz:
> > +	PCF85263_QUARTZCAP_7pF		[0]
> > +	PCF85263_QUARTZCAP_6pF		[1]
> > +	PCF85263_QUARTZCAP_12p5pF	[2] DEFAULT
> > +
> 
> 
> The standard DT property "quartz-load-femtofarads" takes the real physical
> value in femto Farads ie values should be 7000, 6000, 12500 without defines.

I believe I said this on the last version.

> 
> 
> > +- nxp,quartz-drive-strength: Drive strength for the quartz:
> > +	PCF85263_QUARTZDRIVE_100ko	[0] DEFAULT
> > +	PCF85263_QUARTZDRIVE_60ko	[1]
> > +	PCF85263_QUARTZDRIVE_500ko	[2]
> > +
> 
> 
> Not sure about this.
> 
> Wouldn't it be better to either use a real impedence value in ohms (like
> load property above, even though it is a vendor specific value) rather than
> a define, or defines for "Low, Medium, High"?
> 
> 
> Martin
> 
>
Biwen Li Sept. 10, 2019, 1:41 a.m. UTC | #3 
> 
> On 03/09/2019 08:18, Biwen Li wrote:
> > diff --git a/Documentation/devicetree/bindings/rtc/pcf85363.txt
> > b/Documentation/devicetree/bindings/rtc/pcf85363.txt
> > index 94adc1cf93d9..588f688b30d1 100644
> > --- a/Documentation/devicetree/bindings/rtc/pcf85363.txt
> > +++ b/Documentation/devicetree/bindings/rtc/pcf85363.txt
> > @@ -8,10 +8,39 @@ Required properties:
> >   Optional properties:
> >   - interrupts: IRQ line for the RTC (not implemented).
> >
> > +- interrupt-output-pin: The interrupt output pin must be
> > +  "INTA" or "INTB", default value is "INTA"
> > +
> 
> 
> The hardware has 2 interrupt pins which can be mapped to various interrupt
> sources (alarm1, alarm2, periodic, ...)
> 
> Currently the driver only supports alarm1.
> 
> It is even possible to use both pins for the same interrupt (eg if INTA were
> wired to the SoC, INTB to a PMIC and both used for alarm...)
> 
> 
> So maybe it would be better to have
> 
> alarm1-interrupt-output-pin: The interrupt output pin used for the alarm
> function. Must be "INTA", "INTB" or "BOTH"
I will fix it in v4.
> 
> Then, if and when other types of interrupts are supported by the driver new
> properties could be added for them.
> 
> 
> 
> > +- quartz-load-femtofarads: The internal capacitor to select for the quartz:
> > +     PCF85263_QUARTZCAP_7pF          [0]
> > +     PCF85263_QUARTZCAP_6pF          [1]
> > +     PCF85263_QUARTZCAP_12p5pF       [2] DEFAULT
> > +
> 
> 
> The standard DT property "quartz-load-femtofarads" takes the real physical
> value in femto Farads ie values should be 7000, 6000, 12500 without
> defines.

Ok, I will remove these defines in v4.
> 
> 
> > +- nxp,quartz-drive-strength: Drive strength for the quartz:
> > +     PCF85263_QUARTZDRIVE_100ko      [0] DEFAULT
> > +     PCF85263_QUARTZDRIVE_60ko       [1]
> > +     PCF85263_QUARTZDRIVE_500ko      [2]
> > +
> 
> 
> Not sure about this.
> 
> Wouldn't it be better to either use a real impedence value in ohms (like load
> property above, even though it is a vendor specific value) rather than a
> define, or defines for "Low, Medium, High"?
Ok, I will replace defines with a real impedence value in v4.
> 
> 
> Martin
>
Biwen Li Sept. 10, 2019, 10:21 a.m. UTC | #4 
> Caution: EXT Email
> 
> On Tue, Sep 03, 2019 at 11:37:01AM +0200, Martin Fuzzey wrote:
> > On 03/09/2019 08:18, Biwen Li wrote:
> > > diff --git a/Documentation/devicetree/bindings/rtc/pcf85363.txt
> > > b/Documentation/devicetree/bindings/rtc/pcf85363.txt
> > > index 94adc1cf93d9..588f688b30d1 100644
> > > --- a/Documentation/devicetree/bindings/rtc/pcf85363.txt
> > > +++ b/Documentation/devicetree/bindings/rtc/pcf85363.txt
> > > @@ -8,10 +8,39 @@ Required properties:
> > >   Optional properties:
> > >   - interrupts: IRQ line for the RTC (not implemented).
> > > +- interrupt-output-pin: The interrupt output pin must be
> > > +  "INTA" or "INTB", default value is "INTA"
> > > +
> >
> >
> > The hardware has 2 interrupt pins which can be mapped to various
> > interrupt sources (alarm1, alarm2, periodic, ...)
> >
> > Currently the driver only supports alarm1.
> >
> > It is even possible to use both pins for the same interrupt (eg if
> > INTA were wired to the SoC, INTB to a PMIC and both used for alarm...)
> >
> >
> > So maybe it would be better to have
> >
> > alarm1-interrupt-output-pin: The interrupt output pin used for the
> > alarm function. Must be "INTA", "INTB" or "BOTH"
> 
> That's a property per source. 2 properties possible sources (either a mask or list)
> would be my preference.
> 
> Also, whatever you end up with needs a vendor prefix.
> 
> >
> > Then, if and when other types of interrupts are supported by the
> > driver new properties could be added for them.
> >
> >
> >
> > > +- quartz-load-femtofarads: The internal capacitor to select for the quartz:
> > > +   PCF85263_QUARTZCAP_7pF          [0]
> > > +   PCF85263_QUARTZCAP_6pF          [1]
> > > +   PCF85263_QUARTZCAP_12p5pF       [2] DEFAULT
> > > +
> >
> >
> > The standard DT property "quartz-load-femtofarads" takes the real
> > physical value in femto Farads ie values should be 7000, 6000, 12500 without
> defines.
> 
> I believe I said this on the last version.
Yes, I will fix it in v4.
> 
> >
> >
> > > +- nxp,quartz-drive-strength: Drive strength for the quartz:
> > > +   PCF85263_QUARTZDRIVE_100ko      [0] DEFAULT
> > > +   PCF85263_QUARTZDRIVE_60ko       [1]
> > > +   PCF85263_QUARTZDRIVE_500ko      [2]
> > > +
> >
> >
> > Not sure about this.
> >
> > Wouldn't it be better to either use a real impedence value in ohms
> > (like load property above, even though it is a vendor specific value)
> > rather than a define, or defines for "Low, Medium, High"?
> >
> >
> > Martin
> >
> >
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/rtc/pcf85363.txt b/Documentation/devicetree/bindings/rtc/pcf85363.txt
index 94adc1cf93d9..588f688b30d1 100644
--- a/Documentation/devicetree/bindings/rtc/pcf85363.txt
+++ b/Documentation/devicetree/bindings/rtc/pcf85363.txt
@@ -8,10 +8,39 @@  Required properties:
 Optional properties:
 - interrupts: IRQ line for the RTC (not implemented).
 
+- interrupt-output-pin: The interrupt output pin must be
+  "INTA" or "INTB", default value is "INTA"
+
+- quartz-load-femtofarads: The internal capacitor to select for the quartz:
+	PCF85263_QUARTZCAP_7pF		[0]
+	PCF85263_QUARTZCAP_6pF		[1]
+	PCF85263_QUARTZCAP_12p5pF	[2] DEFAULT
+
+- nxp,quartz-drive-strength: Drive strength for the quartz:
+	PCF85263_QUARTZDRIVE_100ko	[0] DEFAULT
+	PCF85263_QUARTZDRIVE_60ko	[1]
+	PCF85263_QUARTZDRIVE_500ko	[2]
+
+- nxp,quartz-low-jitter: Boolean property, if present enables low jitter mode
+  which reduces jitter at the cost of increased power consumption.
+
+- wakeup-source: Boolean property, Please refer to
+  Documentation/devicetree/bindings/power/wakeup-source.txt
+
 Example:
 
 pcf85363: pcf85363@51 {
 	compatible = "nxp,pcf85363";
 	reg = <0x51>;
+
+	interrupt-parent = <&gpio1>;
+	interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
+
+	#include <dt-bindings/rtc/pcf85363.h>
+	wakeup-source;
+	interrupt-output-pin = "INTA";
+	quartz-load-femtofarads = <PCF85363_QUARTZCAP_12p5pF>;
+	nxp,quartz-drive-strength = <PCF85363_QUARTZDRIVE_60ko>;
+	nxp,quartz-low-jitter;
 };
 
diff --git a/include/dt-bindings/rtc/pcf85363.h b/include/dt-bindings/rtc/pcf85363.h
new file mode 100644
index 000000000000..f71b151bc481
--- /dev/null
+++ b/include/dt-bindings/rtc/pcf85363.h
@@ -0,0 +1,15 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _DT_BINDINGS_RTC_PCF85363_H
+#define _DT_BINDINGS_RTC_PCF85363_H
+
+/* Quartz capacitance */
+#define PCF85363_QUARTZCAP_7pF		0
+#define PCF85363_QUARTZCAP_6pF		1
+#define PCF85363_QUARTZCAP_12p5pF	2
+
+/* Quartz drive strength */
+#define PCF85363_QUARTZDRIVE_100ko	0
+#define PCF85363_QUARTZDRIVE_60ko	1
+#define PCF85363_QUARTZDRIVE_500ko	2
+
+#endif /* _DT_BINDINGS_RTC_PCF85363_H */