Re: [PATCH 1/4 v5] dt-bindings: Add Rockchip rk817 battery charger support

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 05/04/2022 15:54, Chris Morgan wrote:
> On Tue, Apr 05, 2022 at 03:35:04PM +0200, Krzysztof Kozlowski wrote:
>> On 05/04/2022 15:12, Chris Morgan wrote:
>>> On Tue, Apr 05, 2022 at 01:16:55PM +0200, Krzysztof Kozlowski wrote:
>>>> On 04/04/2022 23:57, Chris Morgan wrote:
>>>>> From: Chris Morgan <macromorgan@hotmail.com>
>>>>>
>>>>> Create dt-binding documentation to document rk817 battery and charger
>>>>> usage. New device-tree properties have been added.
>>>>>
>>>>> - rockchip,resistor-sense-micro-ohms: The value in microohms of the
>>>>>                                       sample resistor.
>>>>> - rockchip,sleep-enter-current-microamp: The value in microamps of the
>>>>>                                          sleep enter current.
>>>>> - rockchip,sleep-filter-current: The value in microamps of the sleep
>>>>>                                  filter current.
>>>>>
>>>>> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
>>>>> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
>>>>> ---
>>>>>  .../bindings/mfd/rockchip,rk817.yaml          | 48 +++++++++++++++++++
>>>>>  1 file changed, 48 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mfd/rockchip,rk817.yaml b/Documentation/devicetree/bindings/mfd/rockchip,rk817.yaml
>>>>> index bfc1720adc43..b949d406a487 100644
>>>>> --- a/Documentation/devicetree/bindings/mfd/rockchip,rk817.yaml
>>>>> +++ b/Documentation/devicetree/bindings/mfd/rockchip,rk817.yaml
>>>>> @@ -117,6 +117,47 @@ properties:
>>>>>          description:
>>>>>            Describes if the microphone uses differential mode.
>>>>>  
>>>>> +  battery:
>>>>
>>>> I wonder why do you call it a batter while it is a charger, isn't it?
>>>
>>> It is a driver for both the battery and charger. I'd argue about 95% of
>>> it is battery functions and the other 5% is managing the IRQs for plug
>>> removal/insertion and capturing the incoming voltage and current. In
>>> the BSP kernel these were two seperate drivers, but there was so little
>>> that needed to be done for the charger (and users probably don't need
>>> plug IRQs if they aren't using a battery anyway since the system will
>>> shut off on a plug out event due to no power...).
>>
>> What do you mean by driver for "battery"? Like some smart-battery
>> system? with embedded battery (RK817 comes with embedded battery) Or a
>> fuel gauge? Judging by power supply properties it looks like fuel gauge.
> 
> It's basically just additional registers in the rk817 PMIC. The PMIC is
> controlled via I2C and contains multiple regulators, an audio codec,
> an RTC, some GPIO, some nvram, a columb counter for the battery, and an
> ADC for measuring input current/voltage/temperature (for battery or
> charger). 

Actually you mentioned it in the cover letter - it's a PMIC, not a
battery. I doubt that it's embedded with a battery. :)

> This driver deals with the columb counter, the ADC, and a few
> bits of nvram for storing battery values to retain backwards
> compatibility with the BSP kernel and bootloader, and handling IRQs
> related to inserting or removing the charger (curiously my
> implementation ALSO has a charger sense GPIO in addition to the PMIC
> being able to sense that).
> 
> The driver itself is named rk817_charger. If you think I should change
> this from battery to "fuel gauge" or "charger" let me know and I can
> resubmit. Whatever makes it clearer for everyone.

Yeah, the property name and bindings should describe the hardware, so in
such case the hardware is rather a "charger" or "fuel-gauge". Your
"battery-cell" from DTS is probably just a "battery" (unless you expect
multiple cells?).

Best regards,
Krzysztof