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

[Chris Morgan <macroalpha82@gmail.com>]

On Tue, Apr 05, 2022 at 04:00:09PM +0200, Krzysztof Kozlowski wrote:
> 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

Okay, when v6 comes around I'll change it to be "charger" instead of
"battery" to make it more clear. There should only be a single battery
instead of multiple cells, and according to the documentation I should
be okay with describing the battery in the devicetree since it's not
something easy for the end-user to change.

I'd like to get someone to look at the meat and potatoes of the series
before I submit a v6... I did a fairly substantial rewrite of the
actual rk817_charger.c to solve for several problems and fix several
bugs I found in extended testing. One of the major changes was to
mirror the BSP in that I poll the PMIC every 8 seconds for updates
and then store it in the driver struct rather than pull each value
on demand as requested. I see other drivers doing this but I want
to make sure that's acceptable upstream.

Thank you.