Patchwork [V4] drivers/rtc/rtc-palmas.c: support for backup battery charging

login
register
mail settings
Submitter Laxman Dewangan
Date Aug. 7, 2013, 10:29 a.m.
Message ID <1375871392-871-1-git-send-email-ldewangan@nvidia.com>
Download mbox | patch
Permalink /patch/265462/
State New
Headers show

Comments

Laxman Dewangan - Aug. 7, 2013, 10:29 a.m.
Palmas series device like TPS65913, TPS80036 supports the backup battery
for powering the RTC when no other energy source is available.

The backup battery is optional, connected to the VBACKUP pin, and can be
nonrechargeable or rechargeable. The rechargeable battery can be charged
from the system supply using the backup battery charger.

Add support for enabling charging of this backup battery.  Also add the DT
binding document and the new properties to have this support.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
Reviewed-by: Stephen Warren <swarren@nvidia.com>
Reviewed-by: Felipe Balbi <balbi@ti.com>
---
Changes from V1:
- Converted DT property to boolean.
- cleanups as use "...".
Changes from V2:
- Correct the property name.
Changes from V3:
- Nit cleanups: properly capatilise the palmas to make it Palmas.
- Added reviewed by of Felipe and Stephen.

 .../devicetree/bindings/rtc/rtc-palmas.txt         |   29 ++++++++++++++++
 drivers/rtc/rtc-palmas.c                           |   35 ++++++++++++++++++++
 2 files changed, 64 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/rtc-palmas.txt
Mark Rutland - Aug. 7, 2013, 4:38 p.m.
On Wed, Aug 07, 2013 at 11:29:52AM +0100, Laxman Dewangan wrote:
> Palmas series device like TPS65913, TPS80036 supports the backup battery
> for powering the RTC when no other energy source is available.
> 
> The backup battery is optional, connected to the VBACKUP pin, and can be
> nonrechargeable or rechargeable. The rechargeable battery can be charged
> from the system supply using the backup battery charger.
> 
> Add support for enabling charging of this backup battery.  Also add the DT
> binding document and the new properties to have this support.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> Reviewed-by: Stephen Warren <swarren@nvidia.com>
> Reviewed-by: Felipe Balbi <balbi@ti.com>
> ---
> Changes from V1:
> - Converted DT property to boolean.
> - cleanups as use "...".
> Changes from V2:
> - Correct the property name.
> Changes from V3:
> - Nit cleanups: properly capatilise the palmas to make it Palmas.
> - Added reviewed by of Felipe and Stephen.
> 
>  .../devicetree/bindings/rtc/rtc-palmas.txt         |   29 ++++++++++++++++
>  drivers/rtc/rtc-palmas.c                           |   35 ++++++++++++++++++++
>  2 files changed, 64 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/rtc/rtc-palmas.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-palmas.txt b/Documentation/devicetree/bindings/rtc/rtc-palmas.txt
> new file mode 100644
> index 0000000..41bb445
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/rtc-palmas.txt
> @@ -0,0 +1,29 @@
> +Palmas RTC controller bindings
> +
> +Required properties:
> +- compatible:
> +  - "ti,palmas-rtc" for Palmas series of the RTC controller
> +- interrupt-parent: Parent interrupt device, must be handle of Palmas node.
> +- interrupts: Interrupt number of RTC submodule on device.
> +
> +Optional properties:
> +- ti,back-battery-charge-enable: The Palmas series device like TPS65913 or
> +	TPS80036 supports the battery backup for powering the RTC when main
> +	battery is removed or in very low power state. This flag will enable
> +	the backup battery charging.

I don't like the wording here as it implies that an OS *must* charge the
device, rather than that it *can* charge the device. How about:

 - ti,backup-battery-chargeable: There is a chargeable backup battery
                                 present.

> +- ti,back-battery-charge-low-current: Configure lower charging current. Device
> +	supports the charging current as < 100mA or >100mA. Low current will
> +	set as <100mA.

This is somewhat unclear as it reads as a runtime configuration choice,
rather than some instances of the device only support being changed at
low currents (as I assume is the case?). How about:

  - ti,backup-battery-low-current: The backup battery is only chargeable
                                   at currents below 100mA.

What happens if we charge at the wrong current?

Thanks,
Mark.
Laxman Dewangan - Aug. 7, 2013, 6:46 p.m.
On Wednesday 07 August 2013 10:08 PM, Mark Rutland wrote:
> On Wed, Aug 07, 2013 at 11:29:52AM +0100, Laxman Dewangan wrote:
>> +Optional properties:
>> +- ti,back-battery-charge-enable: The Palmas series device like TPS65913 or
>> +	TPS80036 supports the battery backup for powering the RTC when main
>> +	battery is removed or in very low power state. This flag will enable
>> +	the backup battery charging.
> I don't like the wording here as it implies that an OS *must* charge the
> device, rather than that it *can* charge the device. How about:
>
>   - ti,backup-battery-chargeable: There is a chargeable backup battery
>                                   present.
>

The *org* property tells whether charging should be enable or not. This 
is enabled during init and it is not the runtime configuration for 
enable/disable.
By saying "backup-battery-chargeable" means it need other 
interface/calls to start charging. It does not reflect that charging 
will be enabled by default.



>> +- ti,back-battery-charge-low-current: Configure lower charging current. Device
>> +	supports the charging current as < 100mA or >100mA. Low current will
>> +	set as <100mA.
> This is somewhat unclear as it reads as a runtime configuration choice,
> rather than some instances of the device only support being changed at
> low currents (as I assume is the case?). How about:
>
>    - ti,backup-battery-low-current: The backup battery is only chargeable
>                                     at currents below 100mA.

Hmm.. I think even if battery can be charge for more than 100mA, there 
is choice of configure it for less than 100mA.

> What happens if we charge at the wrong current?
Not much sure but I think it can create battery damage.
Mark Rutland - Aug. 8, 2013, 9:33 a.m.
On Wed, Aug 07, 2013 at 07:46:11PM +0100, Laxman Dewangan wrote:
> On Wednesday 07 August 2013 10:08 PM, Mark Rutland wrote:
> > On Wed, Aug 07, 2013 at 11:29:52AM +0100, Laxman Dewangan wrote:
> >> +Optional properties:
> >> +- ti,back-battery-charge-enable: The Palmas series device like TPS65913 or
> >> +	TPS80036 supports the battery backup for powering the RTC when main
> >> +	battery is removed or in very low power state. This flag will enable
> >> +	the backup battery charging.
> > I don't like the wording here as it implies that an OS *must* charge the
> > device, rather than that it *can* charge the device. How about:
> >
> >   - ti,backup-battery-chargeable: There is a chargeable backup battery
> >                                   present.
> >
> 
> The *org* property tells whether charging should be enable or not. This 
> is enabled during init and it is not the runtime configuration for 
> enable/disable.
> By saying "backup-battery-chargeable" means it need other 
> interface/calls to start charging. It does not reflect that charging 
> will be enabled by default.

It doesn't mean that the OS needs to provide some interface to control
charging. It simply means that it's up to the OS as to whether it
charges it (for which Linux's choice can be to always charge the
battery). An OS may choose not implement any charging code at all...

> 
> 
> 
> >> +- ti,back-battery-charge-low-current: Configure lower charging current. Device
> >> +	supports the charging current as < 100mA or >100mA. Low current will
> >> +	set as <100mA.
> > This is somewhat unclear as it reads as a runtime configuration choice,
> > rather than some instances of the device only support being changed at
> > low currents (as I assume is the case?). How about:
> >
> >    - ti,backup-battery-low-current: The backup battery is only chargeable
> >                                     at currents below 100mA.
> 
> Hmm.. I think even if battery can be charge for more than 100mA, there 
> is choice of configure it for less than 100mA.

Ok.

> 
> > What happens if we charge at the wrong current?
> Not much sure but I think it can create battery damage.

If we charge a battery supporting >100mA at a current <100mA, will that
cause damage? Or only if we charge above it's supported current?

If the latter's true, it might make sense to invert the condition and
describe that the battery may be charged at a higher current, with the
default being <100mA. That way a missing property in the dt will only
result in sub-optimal charging rather than battery damage.

Thanks,
Mark.
Laxman Dewangan - Aug. 8, 2013, 5:17 p.m.
On Thursday 08 August 2013 03:03 PM, Mark Rutland wrote:
> On Wed, Aug 07, 2013 at 07:46:11PM +0100, Laxman Dewangan wrote:
>>> What happens if we charge at the wrong current?
>> Not much sure but I think it can create battery damage.
> If we charge a battery supporting >100mA at a current <100mA, will that
> cause damage? Or only if we charge above it's supported current?

Over-current charging can only damage. Lower charging current will not 
damage, just will take more time to charge.

>
> If the latter's true, it might make sense to invert the condition and
> describe that the battery may be charged at a higher current, with the
> default being <100mA. That way a missing property in the dt will only
> result in sub-optimal charging rather than battery damage.
>
Ok, fine with this. In this case property will be 
"ti,backup-battery-charge-high-current".

Patch

diff --git a/Documentation/devicetree/bindings/rtc/rtc-palmas.txt b/Documentation/devicetree/bindings/rtc/rtc-palmas.txt
new file mode 100644
index 0000000..41bb445
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/rtc-palmas.txt
@@ -0,0 +1,29 @@ 
+Palmas RTC controller bindings
+
+Required properties:
+- compatible:
+  - "ti,palmas-rtc" for Palmas series of the RTC controller
+- interrupt-parent: Parent interrupt device, must be handle of Palmas node.
+- interrupts: Interrupt number of RTC submodule on device.
+
+Optional properties:
+- ti,back-battery-charge-enable: The Palmas series device like TPS65913 or
+	TPS80036 supports the battery backup for powering the RTC when main
+	battery is removed or in very low power state. This flag will enable
+	the backup battery charging.
+- ti,back-battery-charge-low-current: Configure lower charging current. Device
+	supports the charging current as < 100mA or >100mA. Low current will
+	set as <100mA.
+
+Example:
+	palmas: tps65913@58 {
+		...
+		palmas_rtc: rtc {
+			compatible = "ti,palmas-rtc";
+			interrupt-parent = <&palmas>;
+			interrupts = <8 0>;
+			ti,back-battery-charge-enable;
+			ti,back-battery-charge-low-current;
+		};
+		...
+	};
diff --git a/drivers/rtc/rtc-palmas.c b/drivers/rtc/rtc-palmas.c
index a1fecc8..adb6e71 100644
--- a/drivers/rtc/rtc-palmas.c
+++ b/drivers/rtc/rtc-palmas.c
@@ -238,6 +238,15 @@  static int palmas_rtc_probe(struct platform_device *pdev)
 	struct palmas *palmas = dev_get_drvdata(pdev->dev.parent);
 	struct palmas_rtc *palmas_rtc = NULL;
 	int ret;
+	bool enable_bb_charging = false;
+	bool low_bb_charging;
+
+	if (pdev->dev.of_node) {
+		enable_bb_charging = of_property_read_bool(pdev->dev.of_node,
+					"ti,back-battery-charge-enable");
+		low_bb_charging = of_property_read_bool(pdev->dev.of_node,
+					"ti,back-battery-charge-low-current");
+	}
 
 	palmas_rtc = devm_kzalloc(&pdev->dev, sizeof(struct palmas_rtc),
 			GFP_KERNEL);
@@ -254,6 +263,32 @@  static int palmas_rtc_probe(struct platform_device *pdev)
 	palmas_rtc->dev = &pdev->dev;
 	platform_set_drvdata(pdev, palmas_rtc);
 
+	if (enable_bb_charging) {
+		unsigned reg = 0;
+
+		if (low_bb_charging)
+			reg |= PALMAS_BACKUP_BATTERY_CTRL_BBS_BBC_LOW_ICHRG;
+
+		ret = palmas_update_bits(palmas, PALMAS_PMU_CONTROL_BASE,
+			PALMAS_BACKUP_BATTERY_CTRL,
+			PALMAS_BACKUP_BATTERY_CTRL_BBS_BBC_LOW_ICHRG, reg);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"BACKUP_BATTERY_CTRL update failed, %d\n", ret);
+			return ret;
+		}
+
+		ret = palmas_update_bits(palmas, PALMAS_PMU_CONTROL_BASE,
+			PALMAS_BACKUP_BATTERY_CTRL,
+			PALMAS_BACKUP_BATTERY_CTRL_BB_CHG_EN,
+			PALMAS_BACKUP_BATTERY_CTRL_BB_CHG_EN);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"BACKUP_BATTERY_CTRL update failed, %d\n", ret);
+			return ret;
+		}
+	}
+
 	/* Start RTC */
 	ret = palmas_update_bits(palmas, PALMAS_RTC_BASE, PALMAS_RTC_CTRL_REG,
 			PALMAS_RTC_CTRL_REG_STOP_RTC,