| Message ID | 20180907193508.24974-2-sam@ravnborg.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1,1/5] dt-binding: rtci-pcf8523: add quartz_load property | expand |
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
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.
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
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.
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
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.
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
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