[RESEND PATCH v2 1/3] rtc: ds1307: Enable battery backup on RX8130

[RESEND PATCH v2 1/3] rtc: ds1307: Enable battery backup on RX8130

[Bastian Krause]

From: Marek Vasut <marex@denx.de>

The battery backup can be disabled on this RTC, e.g. if populated right
out of production. Force the battery backup bit on to enable it.

Signed-off-by: Marek Vasut <marex@denx.de>
Reviewed-by: Bastian Krause <bst@pengutronix.de>
--
v2: Drop the custom offset, let regmap handle that
---
 drivers/rtc/rtc-ds1307.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 49702942bb08..5f0df60a71d1 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -122,6 +122,8 @@ enum ds_type {
 #define RX8130_REG_FLAG_AF		BIT(3)
 #define RX8130_REG_CONTROL0		0x1e
 #define RX8130_REG_CONTROL0_AIE		BIT(3)
+#define RX8130_REG_CONTROL1		0x1f
+#define RX8130_REG_CONTROL1_INIEN	BIT(4)
 
 #define MCP794XX_REG_CONTROL		0x07
 #	define MCP794XX_BIT_ALM0_EN	0x10
@@ -1875,6 +1877,11 @@ static int ds1307_probe(struct i2c_client *client,
 				     DS1307_REG_HOUR << 4 | 0x08, hour);
 		}
 		break;
+	case rx_8130:
+		/* make sure that the backup battery is enabled */
+		regmap_write(ds1307->regmap, RX8130_REG_CONTROL1,
+			     RX8130_REG_CONTROL1_INIEN);
+		break;
 	default:
 		break;
 	}
-- 
2.26.0.rc2

[PATCH 2/3] dt-bindings: rtc: add chargeable flag for rx8130

[Bastian Krause]

Signed-off-by: Bastian Krause <bst@pengutronix.de>
---
 Documentation/devicetree/bindings/rtc/rtc-ds1307.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
index 66f0a31ae9ce..987a0c9e0cd7 100644
--- a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
+++ b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
@@ -34,6 +34,9 @@ Optional properties:
 - trickle-diode-disable : ds1339, ds1340 and ds 1388 only
 	Do not use internal trickle charger diode
 	Should be given if internal trickle charger diode should be disabled
+- aux-voltage-chargeable: rx8130 only
+	Epsons's rx8130 supports a backup battery/supercap.
+	This flag tells	whether the battery/supercap is chargeable or not.
 
 Example:
 	ds1339: rtc@68 {
-- 
2.26.0.rc2

Re: [PATCH 2/3] dt-bindings: rtc: add chargeable flag for rx8130

[Uwe Kleine-König]

On Wed, Apr 15, 2020 at 06:37:00PM +0200, Bastian Krause wrote:
> Signed-off-by: Bastian Krause <bst@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/rtc/rtc-ds1307.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
> index 66f0a31ae9ce..987a0c9e0cd7 100644
> --- a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
> +++ b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
> @@ -34,6 +34,9 @@ Optional properties:
>  - trickle-diode-disable : ds1339, ds1340 and ds 1388 only
>  	Do not use internal trickle charger diode
>  	Should be given if internal trickle charger diode should be disabled
> +- aux-voltage-chargeable: rx8130 only
> +	Epsons's rx8130 supports a backup battery/supercap.

s/Epsons's/Epson's/

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH 2/3] dt-bindings: rtc: add chargeable flag for rx8130

[Alexandre Belloni]

On 15/04/2020 18:37:00+0200, Bastian Krause wrote:
> Signed-off-by: Bastian Krause <bst@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/rtc/rtc-ds1307.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
> index 66f0a31ae9ce..987a0c9e0cd7 100644
> --- a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
> +++ b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
> @@ -34,6 +34,9 @@ Optional properties:
>  - trickle-diode-disable : ds1339, ds1340 and ds 1388 only
>  	Do not use internal trickle charger diode
>  	Should be given if internal trickle charger diode should be disabled
> +- aux-voltage-chargeable: rx8130 only
> +	Epsons's rx8130 supports a backup battery/supercap.
> +	This flag tells	whether the battery/supercap is chargeable or not.
>  

I think we should make that a generic property and this should supersede
trickle-diode-disable which is a bit wonky as I would prefer the default
to be disabled instead of enabled with the current semantics.

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

Re: [PATCH 2/3] dt-bindings: rtc: add chargeable flag for rx8130

[Bastian Krause]

On 4/15/20 8:56 PM, Alexandre Belloni wrote:
> On 15/04/2020 18:37:00+0200, Bastian Krause wrote:
>> Signed-off-by: Bastian Krause <bst@pengutronix.de>
>> ---
>>  Documentation/devicetree/bindings/rtc/rtc-ds1307.txt | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
>> index 66f0a31ae9ce..987a0c9e0cd7 100644
>> --- a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
>> +++ b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
>> @@ -34,6 +34,9 @@ Optional properties:
>>  - trickle-diode-disable : ds1339, ds1340 and ds 1388 only
>>  	Do not use internal trickle charger diode
>>  	Should be given if internal trickle charger diode should be disabled
>> +- aux-voltage-chargeable: rx8130 only
>> +	Epsons's rx8130 supports a backup battery/supercap.
>> +	This flag tells	whether the battery/supercap is chargeable or not.
>>  
> 
> I think we should make that a generic property and this should supersede
> trickle-diode-disable which is a bit wonky as I would prefer the default
> to be disabled instead of enabled with the current semantics.

Alright, I think I know how to transform the RTC drivers.

One question about the DTs though:

This means we should remove "trickle-diode-disable" from all upstream
DTs and add "aux-voltage-chargeable" to all upstream DTs that use a RTC
compatible whose driver care in their probe function for
"trickle-diode-disable", right?

Regards,
Bastian

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/3] dt-bindings: rtc: add chargeable flag for rx8130

[Bastian Krause]

On 8/24/20 1:31 PM, Bastian Krause wrote:
> 
> On 4/15/20 8:56 PM, Alexandre Belloni wrote:
>> On 15/04/2020 18:37:00+0200, Bastian Krause wrote:
>>> Signed-off-by: Bastian Krause <bst@pengutronix.de>
>>> ---
>>>  Documentation/devicetree/bindings/rtc/rtc-ds1307.txt | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
>>> index 66f0a31ae9ce..987a0c9e0cd7 100644
>>> --- a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
>>> +++ b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
>>> @@ -34,6 +34,9 @@ Optional properties:
>>>  - trickle-diode-disable : ds1339, ds1340 and ds 1388 only
>>>  	Do not use internal trickle charger diode
>>>  	Should be given if internal trickle charger diode should be disabled
>>> +- aux-voltage-chargeable: rx8130 only
>>> +	Epsons's rx8130 supports a backup battery/supercap.
>>> +	This flag tells	whether the battery/supercap is chargeable or not.
>>>  
>>
>> I think we should make that a generic property and this should supersede
>> trickle-diode-disable which is a bit wonky as I would prefer the default
>> to be disabled instead of enabled with the current semantics.
> 
> Alright, I think I know how to transform the RTC drivers.
> 
> One question about the DTs though:
> 
> This means we should remove "trickle-diode-disable" from all upstream
> DTs and add "aux-voltage-chargeable" to all upstream DTs that use a RTC
> compatible whose driver care in their probe function for
> "trickle-diode-disable", right?

Sorry, forget that.

Here's the situation:

Currently there is a switch to explicitly disable charging, so the
default is to charge. We cannot introduce another boolean switch to turn
that the other way around, because that would change the default and
break backwards compatibility.

The only way I can think of is to introduce "aux-voltage-chargeable" not
as a boolean switch but as an integer, without any default. If this
property is not available, the drivers should simply do what they did
prior to this change (look for the legacy trickle-diode-disable, use the
default they used before).

Are you okay with that?

Some more context:

I originally tried to add a chargeable flag for rx8130. Prior to this
patch, there was no need to set "trickle-diode-disable" for this,
because the driver did not pass the chargeable flag to the RTC. With the
patch the default would have been to charge as long as
"trickle-diode-disable" is not there. So there's a change in behavior.

Regards,
Bastian

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/3] dt-bindings: rtc: add chargeable flag for rx8130

[Alexandre Belloni]

Hi,

On 24/08/2020 15:32:22+0200, Bastian Krause wrote:
> On 8/24/20 1:31 PM, Bastian Krause wrote:
> > 
> > On 4/15/20 8:56 PM, Alexandre Belloni wrote:
> >> On 15/04/2020 18:37:00+0200, Bastian Krause wrote:
> >>> Signed-off-by: Bastian Krause <bst@pengutronix.de>
> >>> ---
> >>>  Documentation/devicetree/bindings/rtc/rtc-ds1307.txt | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
> >>> index 66f0a31ae9ce..987a0c9e0cd7 100644
> >>> --- a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
> >>> +++ b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
> >>> @@ -34,6 +34,9 @@ Optional properties:
> >>>  - trickle-diode-disable : ds1339, ds1340 and ds 1388 only
> >>>  	Do not use internal trickle charger diode
> >>>  	Should be given if internal trickle charger diode should be disabled
> >>> +- aux-voltage-chargeable: rx8130 only
> >>> +	Epsons's rx8130 supports a backup battery/supercap.
> >>> +	This flag tells	whether the battery/supercap is chargeable or not.
> >>>  
> >>
> >> I think we should make that a generic property and this should supersede
> >> trickle-diode-disable which is a bit wonky as I would prefer the default
> >> to be disabled instead of enabled with the current semantics.
> > 
> > Alright, I think I know how to transform the RTC drivers.
> > 
> > One question about the DTs though:
> > 
> > This means we should remove "trickle-diode-disable" from all upstream
> > DTs and add "aux-voltage-chargeable" to all upstream DTs that use a RTC
> > compatible whose driver care in their probe function for
> > "trickle-diode-disable", right?
> 
> Sorry, forget that.
> 
> Here's the situation:
> 
> Currently there is a switch to explicitly disable charging, so the
> default is to charge. We cannot introduce another boolean switch to turn
> that the other way around, because that would change the default and
> break backwards compatibility.
> 
> The only way I can think of is to introduce "aux-voltage-chargeable" not
> as a boolean switch but as an integer, without any default. If this
> property is not available, the drivers should simply do what they did
> prior to this change (look for the legacy trickle-diode-disable, use the
> default they used before).
> 
> Are you okay with that?
> 

I agree boolean should be avoided in RTC drivers because we need a way
to express "don't change this value".

> Some more context:
> 
> I originally tried to add a chargeable flag for rx8130. Prior to this
> patch, there was no need to set "trickle-diode-disable" for this,
> because the driver did not pass the chargeable flag to the RTC. With the
> patch the default would have been to charge as long as
> "trickle-diode-disable" is not there. So there's a change in behavior.
> 

Yes, IIRC, my point was simply to move the documentation for
aux-voltage-chargeable to the generice rtc binding documentation,
Documentation/devicetree/bindings/rtc/rtc.yaml

For now, you sould keep support for trickle-diode-disable but it has to be
superseded by aux-voltage-chargeable if present. Is that more clear?


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

Re: [PATCH 2/3] dt-bindings: rtc: add chargeable flag for rx8130

[Bastian Krause]

On 8/25/20 5:32 PM, Alexandre Belloni wrote:
> Hi,
> 
> On 24/08/2020 15:32:22+0200, Bastian Krause wrote:
>> On 8/24/20 1:31 PM, Bastian Krause wrote:
>>>
>>> On 4/15/20 8:56 PM, Alexandre Belloni wrote:
>>>> On 15/04/2020 18:37:00+0200, Bastian Krause wrote:
>>>>> Signed-off-by: Bastian Krause <bst@pengutronix.de>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/rtc/rtc-ds1307.txt | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
>>>>> index 66f0a31ae9ce..987a0c9e0cd7 100644
>>>>> --- a/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
>>>>> +++ b/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
>>>>> @@ -34,6 +34,9 @@ Optional properties:
>>>>>  - trickle-diode-disable : ds1339, ds1340 and ds 1388 only
>>>>>  	Do not use internal trickle charger diode
>>>>>  	Should be given if internal trickle charger diode should be disabled
>>>>> +- aux-voltage-chargeable: rx8130 only
>>>>> +	Epsons's rx8130 supports a backup battery/supercap.
>>>>> +	This flag tells	whether the battery/supercap is chargeable or not.
>>>>>  
>>>>
>>>> I think we should make that a generic property and this should supersede
>>>> trickle-diode-disable which is a bit wonky as I would prefer the default
>>>> to be disabled instead of enabled with the current semantics.
>>>
>>> Alright, I think I know how to transform the RTC drivers.
>>>
>>> One question about the DTs though:
>>>
>>> This means we should remove "trickle-diode-disable" from all upstream
>>> DTs and add "aux-voltage-chargeable" to all upstream DTs that use a RTC
>>> compatible whose driver care in their probe function for
>>> "trickle-diode-disable", right?
>>
>> Sorry, forget that.
>>
>> Here's the situation:
>>
>> Currently there is a switch to explicitly disable charging, so the
>> default is to charge. We cannot introduce another boolean switch to turn
>> that the other way around, because that would change the default and
>> break backwards compatibility.
>>
>> The only way I can think of is to introduce "aux-voltage-chargeable" not
>> as a boolean switch but as an integer, without any default. If this
>> property is not available, the drivers should simply do what they did
>> prior to this change (look for the legacy trickle-diode-disable, use the
>> default they used before).
>>
>> Are you okay with that?
>>
> 
> I agree boolean should be avoided in RTC drivers because we need a way
> to express "don't change this value".

Alright.

>> Some more context:
>>
>> I originally tried to add a chargeable flag for rx8130. Prior to this
>> patch, there was no need to set "trickle-diode-disable" for this,
>> because the driver did not pass the chargeable flag to the RTC. With the
>> patch the default would have been to charge as long as
>> "trickle-diode-disable" is not there. So there's a change in behavior.
>>
> 
> Yes, IIRC, my point was simply to move the documentation for
> aux-voltage-chargeable to the generice rtc binding documentation,
> Documentation/devicetree/bindings/rtc/rtc.yaml
> 
> For now, you sould keep support for trickle-diode-disable but it has to be
> superseded by aux-voltage-chargeable if present. Is that more clear?

Yes, thanks for the clarification.

Should I set the deprecated flag for trickle-diode-disable in the
dt-binding yaml?

Regards,
Bastian

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/3] dt-bindings: rtc: add chargeable flag for rx8130

[Alexandre Belloni]

On 26/08/2020 10:13:04+0200, Bastian Krause wrote:
> >> Are you okay with that?
> >>
> > 
> > I agree boolean should be avoided in RTC drivers because we need a way
> > to express "don't change this value".
> 
> Alright.
> 
> >> Some more context:
> >>
> >> I originally tried to add a chargeable flag for rx8130. Prior to this
> >> patch, there was no need to set "trickle-diode-disable" for this,
> >> because the driver did not pass the chargeable flag to the RTC. With the
> >> patch the default would have been to charge as long as
> >> "trickle-diode-disable" is not there. So there's a change in behavior.
> >>
> > 
> > Yes, IIRC, my point was simply to move the documentation for
> > aux-voltage-chargeable to the generice rtc binding documentation,
> > Documentation/devicetree/bindings/rtc/rtc.yaml
> > 
> > For now, you sould keep support for trickle-diode-disable but it has to be
> > superseded by aux-voltage-chargeable if present. Is that more clear?
> 
> Yes, thanks for the clarification.
> 
> Should I set the deprecated flag for trickle-diode-disable in the
> dt-binding yaml?
> 

That's a good idea, yes.


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

[PATCH 3/3] rtc: ds1307: make backup battery chargeable optionally

[Bastian Krause]

RX8130_REG_CONTROL1_CHGEN decides whether the battery or the
supercap should be charged or not. Introduce a new dt binding for that.

Signed-off-by: Bastian Krause <bst@pengutronix.de>
---
 drivers/rtc/rtc-ds1307.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 5f0df60a71d1..451708402c3e 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -124,6 +124,7 @@ enum ds_type {
 #define RX8130_REG_CONTROL0_AIE		BIT(3)
 #define RX8130_REG_CONTROL1		0x1f
 #define RX8130_REG_CONTROL1_INIEN	BIT(4)
+#define RX8130_REG_CONTROL1_CHGEN	BIT(5)
 
 #define MCP794XX_REG_CONTROL		0x07
 #	define MCP794XX_BIT_ALM0_EN	0x10
@@ -1708,6 +1709,7 @@ static int ds1307_probe(struct i2c_client *client,
 	const struct chip_desc	*chip;
 	bool			want_irq;
 	bool			ds1307_can_wakeup_device = false;
+	bool			enable_bb_charging = false;
 	unsigned char		regs[8];
 	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
 	u8			trickle_charger_setup = 0;
@@ -1774,6 +1776,9 @@ static int ds1307_probe(struct i2c_client *client,
 	if (chip->alarm && of_property_read_bool(client->dev.of_node,
 						 "wakeup-source"))
 		ds1307_can_wakeup_device = true;
+
+	enable_bb_charging = of_property_read_bool(client->dev.of_node,
+						   "aux-voltage-chargeable");
 #endif
 
 	switch (ds1307->type) {
@@ -1879,8 +1884,13 @@ static int ds1307_probe(struct i2c_client *client,
 		break;
 	case rx_8130:
 		/* make sure that the backup battery is enabled */
-		regmap_write(ds1307->regmap, RX8130_REG_CONTROL1,
-			     RX8130_REG_CONTROL1_INIEN);
+		regs[0] = RX8130_REG_CONTROL1_INIEN;
+
+		/* set chargeable flag */
+		if (enable_bb_charging)
+			regs[0] |= RX8130_REG_CONTROL1_CHGEN;
+
+		regmap_write(ds1307->regmap, RX8130_REG_CONTROL1, regs[0]);
 		break;
 	default:
 		break;
-- 
2.26.0.rc2