| Message ID | 20200530123222.361104-1-kevin+linux@km6g.us |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtc: abx80x: Add support for autocalibration filter capacitor | expand |
Sorry, this was sent prematurely and should have been split into two patches. It has been resent properly. On Sat, May 30, 2020 at 8:32 AM Kevin P. Fleming <kevin+linux@km6g.us> wrote: > > All of the parts supported by this driver can make use of a > small capacitor to improve the accuracy of the autocalibration > process for their RC oscillators. If a capacitor is connected, > a configuration register must be set to enable its use, so a > new Device Tree property has been added for that purpose. > > Signed-off-by: Kevin P. Fleming <kevin+linux@km6g.us> > Cc: Alessandro Zummo <a.zummo@towertech.it> > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> > Cc: Rob Herring <robh+dt@kernel.org> > To: linux-rtc@vger.kernel.org > To: devicetree@vger.kernel.org > --- > .../bindings/rtc/abracon,abx80x.txt | 6 ++++ > drivers/rtc/rtc-abx80x.c | 34 +++++++++++++++++++ > 2 files changed, 40 insertions(+) > > diff --git a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt > index 2405e35a1bc0..ad5d59ed6f24 100644 > --- a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt > +++ b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt > @@ -29,3 +29,9 @@ and valid to enable charging: > - "abracon,tc-diode": should be "standard" (0.6V) or "schottky" (0.3V) > - "abracon,tc-resistor": should be <0>, <3>, <6> or <11>. 0 disables the output > resistor, the other values are in kOhm. > + > +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" > diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c > index 3521d8e8dc38..be5a814e8c0b 100644 > --- a/drivers/rtc/rtc-abx80x.c > +++ b/drivers/rtc/rtc-abx80x.c > @@ -76,6 +76,9 @@ > #define ABX8XX_CFG_KEY_OSC 0xa1 > #define ABX8XX_CFG_KEY_MISC 0x9d > > +#define ABX8XX_REG_AFCTRL 0x26 > +#define ABX8XX_AUTOCAL_FILTER_ENABLE 0xa0 > + > #define ABX8XX_REG_ID0 0x28 > > #define ABX8XX_REG_OUT_CTRL 0x30 > @@ -130,6 +133,31 @@ static int abx80x_is_rc_mode(struct i2c_client *client) > return (flags & ABX8XX_OSS_OMODE) ? 1 : 0; > } > > +static int abx80x_enable_autocal_filter(struct i2c_client *client) > +{ > + int err; > + > + /* > + * Write the configuration key register to enable access to the AFCTRL > + * register > + */ > + err = i2c_smbus_write_byte_data(client, ABX8XX_REG_CFG_KEY, > + ABX8XX_CFG_KEY_MISC); > + if (err < 0) { > + dev_err(&client->dev, "Unable to write configuration key\n"); > + return -EIO; > + } > + > + err = i2c_smbus_write_byte_data(client, ABX8XX_REG_AFCTRL, > + ABX8XX_AUTOCAL_FILTER_ENABLE); > + if (err < 0) { > + dev_err(&client->dev, "Unable to write autocal filter register\n"); > + return -EIO; > + } > + > + return 0; > +} > + > static int abx80x_enable_trickle_charger(struct i2c_client *client, > u8 trickle_cfg) > { > @@ -825,6 +853,12 @@ static int abx80x_probe(struct i2c_client *client, > return err; > } > > + if (of_property_read_bool(np, "abracon,autocal_filter")) { > + err = abx80x_enable_autocal_filter(client); > + if (err) > + return err; > + } > + > if (client->irq > 0) { > dev_info(&client->dev, "IRQ %d supplied\n", client->irq); > err = devm_request_threaded_irq(&client->dev, client->irq, NULL, > -- > 2.26.2 >
On Sat, May 30, 2020 at 08:32:22AM -0400, Kevin P. Fleming wrote: > All of the parts supported by this driver can make use of a > small capacitor to improve the accuracy of the autocalibration > process for their RC oscillators. If a capacitor is connected, > a configuration register must be set to enable its use, so a > new Device Tree property has been added for that purpose. > > Signed-off-by: Kevin P. Fleming <kevin+linux@km6g.us> > Cc: Alessandro Zummo <a.zummo@towertech.it> > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> > Cc: Rob Herring <robh+dt@kernel.org> > To: linux-rtc@vger.kernel.org > To: devicetree@vger.kernel.org > --- > .../bindings/rtc/abracon,abx80x.txt | 6 ++++ > drivers/rtc/rtc-abx80x.c | 34 +++++++++++++++++++ > 2 files changed, 40 insertions(+) Binding should be a separate patch? > > diff --git a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt > index 2405e35a1bc0..ad5d59ed6f24 100644 > --- a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt > +++ b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt > @@ -29,3 +29,9 @@ and valid to enable charging: > - "abracon,tc-diode": should be "standard" (0.6V) or "schottky" (0.3V) > - "abracon,tc-resistor": should be <0>, <3>, <6> or <11>. 0 disables the output > resistor, the other values are in kOhm. > + > +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. Rob
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.
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.
On Wed, Jun 10, 2020 at 11:16 AM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > 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. I agree. I could implement this as an enumerated string option which accepts 'yes' or 'no'. Those would cover cases 1 and 2, and the absence of the property would be case 3. I looked through the bindings that exist and didn't see any examples of properties configured this way, but I think it would be understandable to users.
On Fri, Jun 12, 2020 at 5:48 AM Kevin P. Fleming <kevin+linux@km6g.us> wrote: > > On Wed, Jun 10, 2020 at 11:16 AM Alexandre Belloni > <alexandre.belloni@bootlin.com> wrote: > > 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. > > I agree. I could implement this as an enumerated string option which > accepts 'yes' or 'no'. Those would cover cases 1 and 2, and the > absence of the property would be case 3. I looked through the bindings > that exist and didn't see any examples of properties configured this > way, but I think it would be understandable to users. It's a solved problem: foo-feature = 0; //disable foo-feature = 1; //enable <no prop> // Leave feature configured as-is Rob
diff --git a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt index 2405e35a1bc0..ad5d59ed6f24 100644 --- a/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt +++ b/Documentation/devicetree/bindings/rtc/abracon,abx80x.txt @@ -29,3 +29,9 @@ and valid to enable charging: - "abracon,tc-diode": should be "standard" (0.6V) or "schottky" (0.3V) - "abracon,tc-resistor": should be <0>, <3>, <6> or <11>. 0 disables the output resistor, the other values are in kOhm. + +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" diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c index 3521d8e8dc38..be5a814e8c0b 100644 --- a/drivers/rtc/rtc-abx80x.c +++ b/drivers/rtc/rtc-abx80x.c @@ -76,6 +76,9 @@ #define ABX8XX_CFG_KEY_OSC 0xa1 #define ABX8XX_CFG_KEY_MISC 0x9d +#define ABX8XX_REG_AFCTRL 0x26 +#define ABX8XX_AUTOCAL_FILTER_ENABLE 0xa0 + #define ABX8XX_REG_ID0 0x28 #define ABX8XX_REG_OUT_CTRL 0x30 @@ -130,6 +133,31 @@ static int abx80x_is_rc_mode(struct i2c_client *client) return (flags & ABX8XX_OSS_OMODE) ? 1 : 0; } +static int abx80x_enable_autocal_filter(struct i2c_client *client) +{ + int err; + + /* + * Write the configuration key register to enable access to the AFCTRL + * register + */ + err = i2c_smbus_write_byte_data(client, ABX8XX_REG_CFG_KEY, + ABX8XX_CFG_KEY_MISC); + if (err < 0) { + dev_err(&client->dev, "Unable to write configuration key\n"); + return -EIO; + } + + err = i2c_smbus_write_byte_data(client, ABX8XX_REG_AFCTRL, + ABX8XX_AUTOCAL_FILTER_ENABLE); + if (err < 0) { + dev_err(&client->dev, "Unable to write autocal filter register\n"); + return -EIO; + } + + return 0; +} + static int abx80x_enable_trickle_charger(struct i2c_client *client, u8 trickle_cfg) { @@ -825,6 +853,12 @@ static int abx80x_probe(struct i2c_client *client, return err; } + if (of_property_read_bool(np, "abracon,autocal_filter")) { + err = abx80x_enable_autocal_filter(client); + if (err) + return err; + } + if (client->irq > 0) { dev_info(&client->dev, "IRQ %d supplied\n", client->irq); err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
All of the parts supported by this driver can make use of a small capacitor to improve the accuracy of the autocalibration process for their RC oscillators. If a capacitor is connected, a configuration register must be set to enable its use, so a new Device Tree property has been added for that purpose. Signed-off-by: Kevin P. Fleming <kevin+linux@km6g.us> Cc: Alessandro Zummo <a.zummo@towertech.it> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> Cc: Rob Herring <robh+dt@kernel.org> To: linux-rtc@vger.kernel.org To: devicetree@vger.kernel.org --- .../bindings/rtc/abracon,abx80x.txt | 6 ++++ drivers/rtc/rtc-abx80x.c | 34 +++++++++++++++++++ 2 files changed, 40 insertions(+)