On 28/10/2024 15:54, Philipp Rosenberger wrote: > On 24.10.24 09:25, Krzysztof Kozlowski wrote: >> On Thu, Oct 24, 2024 at 09:11:04AM +0200, Philipp Rosenberger wrote: >>> On 22.10.24 18:35, Conor Dooley wrote: >>>> On Tue, Oct 22, 2024 at 11:28:54AM +0200, Philipp Rosenberger wrote: >>>>> The nxp,battery-switch-over property is used to control the switch-over, >>>>> battery low detection and extra power fail detection functions. >>>>> >>>>> The PCF2131 has a different default value for the PWRMNG bits. It is set >>>>> to 0x7: battery switch-over function is disabled, only one power supply >>>>> (VDD); battery low detection function is disabled. >>>>> This is the opposite of the default of the PCF2127/PCA2129 and PCF2129. >>>>> With the nxp,battery-switch-over the behavior can be controlled through >>>>> the device tree. >>>>> >>>>> Signed-off-by: Philipp Rosenberger <p.rosenberger@xxxxxxxxxx> >>>>> --- >>>>> Documentation/devicetree/bindings/rtc/nxp,pcf2127.yaml | 10 ++++++++++ >>>>> 1 file changed, 10 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf2127.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf2127.yaml >>>>> index 2d9fe5a75b06..5739c3e371e7 100644 >>>>> --- a/Documentation/devicetree/bindings/rtc/nxp,pcf2127.yaml >>>>> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf2127.yaml >>>>> @@ -30,6 +30,16 @@ properties: >>>>> reset-source: true >>>>> + nxp,battery-switch-over: >>>>> + description: >>>>> + Battery and power related configuration. This property is used to set the >>>>> + PWRMNG bits of the Control_3 register to control the battery switch-over, >>>>> + battery low detection and extra power fail detection functions. >>>>> + The actual supported functions depend on the device capabilities. >>>>> + $ref: /schemas/types.yaml#/definitions/uint8 >>>>> + minimum: 0 >>>>> + maximum: 7 >>>> >>>> Beyond the fact that I dislike register-content properties like this, where >>>> it is not possible to grok the meaning by reading the property, what >>> >>> Yes, I'm not satisfied with this solution myself. >>> There are three different functions, which can be configured in the >>> register: >>> - battery switch-over mode: standard; direct; disabled >>> - battery low detection: enabled; disabled >>> - extra power fail detection: enabled; disabled >>> >>> I'm not sure what a proper way is to implement this in the devicetree. >>> >>>> even makes this suitable for DT in the first place? Reading the commit >>>> message this sounds like software policy, and that different users of >>>> the same board might want to configure these register bits in different >>>> ways. >>> >>> It is less a software policy, but a configuration how the hardware is >>> implemented. If the device has no battery, it is possible to disable the >>> battery switch-over function. In this case the V_BAT must be connected to >>> ground. >> >> monitored-battery property already tells you this. > > If I understand this correctly, the monitored-battery property is meant > for rechargeable batteries, not for a simple button cell to back up an RTC. Uh, right, this is not a charger. Then property, when described as real hardware characteristic, is fine, e.g. "nxp,no-backup-battery". > >> >>> If a battery is connected, the battery switchover will only work if the >>> battery switch-over function is in standard mode or direct switching mode. >>> Until now the driver has just ignored the PWRMNG bits. As the default was >>> battery switching in standard mode. Thus all use cases worked good enough. >>> Battery switching was working if a battery was connected. If no battery was >>> connected it did no real harm (the rtc may have used a tiny bit more power >>> then needed, I guess). >> >> Why driver cannot use standard mode always? Or other way? > > This would overwrite any configuration set by a bootloader/firmware. For > the older chips (pre PCF2131) this was no problem. As the reset default, > was "battery switch-over in standard mode". The driver just left the > whole battery switch-over configuration untouched. > If we decide to change the battery switch-over configuration > unconditionally, this could overwrite any third-party configuration. I still don't get why this is a problem. Why device cannot be configured completely? > >>> With the new PCF2131 the default has changed to battery switch-over >>> disabled. Now even with a battery attached, the rtc will lose time after a >>> power cycle. >>> I guess I should describe this better in the commit message. >> >> In any case this is pcf2131 related, right? So compatible implies it. > > The reason, why this property is necessary for our devices is the new > PCF2131. But the function is not limited to this device. > > I’m considering simplifying this to just a property that informs the > driver that a backup battery is available. If the property is available, > the driver will enable the battery switch-over function; otherwise, it > will not touch the configuration. Sure. Best regards, Krzysztof
