Re: [PATCH v2 1/2] dt-bindings: rtc: pcf2127: Add nxp,battery-switch-over property

[Krzysztof Kozlowski <krzk@kernel.org>]

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@kunbus.com>
>>>>> ---
>>>>>    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