Re: [PATCH] rtc: abx80x: Add support for autocalibration filter capacitor

From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: "Kevin P. Fleming" <kevin+linux@km6g.us>
Cc: Rob Herring <robh@kernel.org>,
	linux-rtc@vger.kernel.org, devicetree@vger.kernel.org,
	Alessandro Zummo <a.zummo@towertech.it>
Subject: Re: [PATCH] rtc: abx80x: Add support for autocalibration filter capacitor
Date: Wed, 10 Jun 2020 17:16:19 +0200	[thread overview]
Message-ID: <20200610151619.GW3720@piout.net> (raw)
In-Reply-To: <CAE+UdorjD+2GORj3M6abgqTb8QnRZNFiyCX9PJAJc09xUBACqA@mail.gmail.com>

Hi,

On 09/06/2020 18:23:48-0400, Kevin P. Fleming wrote:
> On Tue, Jun 9, 2020 at 6:14 PM Rob Herring <robh@kernel.org> wrote:
> > > ---
> > >  .../bindings/rtc/abracon,abx80x.txt           |  6 ++++
> > >  drivers/rtc/rtc-abx80x.c                      | 34 +++++++++++++++++++
> > >  2 files changed, 40 insertions(+)
> >
> > Binding should be a separate patch?
> 
> Indeed, it was re-sent with the patches separated.
> 
> > > +All of the devices can have a 47pf capacitor attached to increase the
> > > +autocalibration accuracy of their RC oscillators. To enable usage of the
> > > +capacitor the following property has to be defined:
> > > +
> > > + - "abracon,autocal-filter"
> >
> > Can't the standard 'quartz-load-femtofarads' property be used here? You
> > might not need to know the value, but presence of the property can
> > enable the feature.
> 
> On these devices the capacitor is connected to the RC oscillator, not
> the crystal oscillator, so this property is controlling a different
> function. I'm certainly open to suggestions for different names for
> the property if that is desired.

I agree that this is something different, quartz-load-femtofarads is
about asking the RTC to put the correct load depending on the crystal
populated on the board. Here, you want to indicate the presence or
absence of a filter capacitor.

When working with RTCs, there is one issue though: boolean properties
are not working well because there is no way to express the 3 different
conditions:
 1/ the capacitor is present, set the register
 2/ the capacitor is absent, clear the register
 3/ the device tree didn't have this property until not and the register
   may have been set or cleared using another mean, don't touch it.

As your patch is written, it only handles 1 and 3 which is probably the
safest option but then we will never have a way to clear it from the
driver. I'd say that this is not an issue but it is also something we
will never be able to change without breaking some setups.

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