[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <cf8dd3ec-e1e6-4750-b0c6-ceff710dce97@kernel.org>
Date: Mon, 28 Oct 2024 16:55:59 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Philipp Rosenberger <p.rosenberger@...bus.com>
Cc: Conor Dooley <conor@...nel.org>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, linux-rtc@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Lino Sanfilippo <l.sanfilippo@...bus.com>
Subject: Re: [PATCH v2 1/2] dt-bindings: rtc: pcf2127: Add
nxp,battery-switch-over property
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@...bus.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
Powered by blists - more mailing lists