diff mbox series

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

Message ID 20220404215754.30126-2-macroalpha82@gmail.com
State Superseded
Headers show
Series power: supply: Add Support for RK817 Charger | expand

Commit Message

Chris Morgan April 4, 2022, 9:57 p.m. UTC
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(+)

Comments

Krzysztof Kozlowski April 5, 2022, 11:16 a.m. UTC | #1
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?

> +    description: |
> +      The child node for the charger to hold additional properties. If a
> +      battery is not in use, this node can be omitted.
> +    type: object
> +    properties:
> +      monitored-battery:
> +        description: |
> +          A phandle to a monitored battery node that contains a valid
> +          value for:
> +          charge-full-design-microamp-hours,
> +          charge-term-current-microamp,
> +          constant-charge-current-max-microamp,
> +          constant-charge-voltage-max-microvolt,
> +          voltage-max-design-microvolt,
> +          voltage-min-design-microvolt,
> +          and a valid ocv-capacity table.
> +
> +      rockchip,resistor-sense-micro-ohms:
> +        description: |
> +          Value in microohms of the battery sense resistor. This value is
> +          used by the driver to set the correct divisor value to translate
> +          ADC readings into the proper units of measure.
> +        enum: [10000, 20000]
> +
> +      rockchip,sleep-enter-current-microamp:
> +        description: |
> +          Value in microamps of the sleep enter current for the charger.
> +          Value is used by the driver to calibrate the relax threshold.
> +
> +      rockchip,sleep-filter-current-microamp:
> +        description:
> +          Value in microamps of the sleep filter current for the charger.
> +          Value is used by the driver to derive the sleep sample current.
> +
> +    required:
> +      - monitored-battery
> +      - rockchip,resistor-sense-micro-ohms
> +      - rockchip,sleep-enter-current-microamp
> +      - rockchip,sleep-filter-current-microamp
> +
>  allOf:
>    - if:
>        properties:
> @@ -323,6 +364,13 @@ examples:
>                  };
>              };
>  
> +            rk817_battery: battery {

The same.

> +                monitored-battery = <&battery_cell>;
> +                rockchip,resistor-sense-micro-ohms = <10000>;
> +                rockchip,sleep-enter-current-microamp = <300000>;
> +                rockchip,sleep-filter-current-microamp = <100000>;
> +            };
> +
>              rk817_codec: codec {
>                  rockchip,mic-in-differential;
>              };


Best regards,
Krzysztof
Chris Morgan April 5, 2022, 1:12 p.m. UTC | #2
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...).

> 
> > +    description: |
> > +      The child node for the charger to hold additional properties. If a
> > +      battery is not in use, this node can be omitted.
> > +    type: object
> > +    properties:
> > +      monitored-battery:
> > +        description: |
> > +          A phandle to a monitored battery node that contains a valid
> > +          value for:
> > +          charge-full-design-microamp-hours,
> > +          charge-term-current-microamp,
> > +          constant-charge-current-max-microamp,
> > +          constant-charge-voltage-max-microvolt,
> > +          voltage-max-design-microvolt,
> > +          voltage-min-design-microvolt,
> > +          and a valid ocv-capacity table.
> > +
> > +      rockchip,resistor-sense-micro-ohms:
> > +        description: |
> > +          Value in microohms of the battery sense resistor. This value is
> > +          used by the driver to set the correct divisor value to translate
> > +          ADC readings into the proper units of measure.
> > +        enum: [10000, 20000]
> > +
> > +      rockchip,sleep-enter-current-microamp:
> > +        description: |
> > +          Value in microamps of the sleep enter current for the charger.
> > +          Value is used by the driver to calibrate the relax threshold.
> > +
> > +      rockchip,sleep-filter-current-microamp:
> > +        description:
> > +          Value in microamps of the sleep filter current for the charger.
> > +          Value is used by the driver to derive the sleep sample current.
> > +
> > +    required:
> > +      - monitored-battery
> > +      - rockchip,resistor-sense-micro-ohms
> > +      - rockchip,sleep-enter-current-microamp
> > +      - rockchip,sleep-filter-current-microamp
> > +
> >  allOf:
> >    - if:
> >        properties:
> > @@ -323,6 +364,13 @@ examples:
> >                  };
> >              };
> >  
> > +            rk817_battery: battery {
> 
> The same.
> 
> > +                monitored-battery = <&battery_cell>;
> > +                rockchip,resistor-sense-micro-ohms = <10000>;
> > +                rockchip,sleep-enter-current-microamp = <300000>;
> > +                rockchip,sleep-filter-current-microamp = <100000>;
> > +            };
> > +
> >              rk817_codec: codec {
> >                  rockchip,mic-in-differential;
> >              };
> 
> 
> Best regards,
> Krzysztof

Thank you.

I forgot to note in my patch notes that this relies on this series
being upstreamed to convert the rockchip rk808 bindings from txt into
yaml:
https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20220404215754.30126-2-macroalpha82@gmail.com/

That series is mostly upstreamed (the patches fixing issues in various
devicetrees). All that remains is the main one switching to yaml.
Krzysztof Kozlowski April 5, 2022, 1:35 p.m. UTC | #3
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.

"Battery" should rather be used for the node referenced by
"monitored-battery"...

Best regards,
Krzysztof
Chris Morgan April 5, 2022, 1:54 p.m. UTC | #4
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). 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.

Thank you.

> 
> "Battery" should rather be used for the node referenced by
> "monitored-battery"...
> 
> Best regards,
> Krzysztof
Krzysztof Kozlowski April 5, 2022, 2 p.m. UTC | #5
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
Krzysztof Kozlowski April 5, 2022, 2:02 p.m. UTC | #6
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.

"Add new devicetree properties." instead of mixing tenses.

> +
> +      rockchip,sleep-filter-current-microamp:
> +        description:
> +          Value in microamps of the sleep filter current for the charger.
> +          Value is used by the driver to derive the sleep sample current.
> +
> +    required:
> +      - monitored-battery
> +      - rockchip,resistor-sense-micro-ohms
> +      - rockchip,sleep-enter-current-microamp
> +      - rockchip,sleep-filter-current-microamp

additionalProperties:false (on required-level of indentation)


Best regards,
Krzysztof
Chris Morgan April 5, 2022, 2:04 p.m. UTC | #7
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.
Rob Herring April 5, 2022, 6:23 p.m. UTC | #8
On Mon, Apr 04, 2022 at 04:57:51PM -0500, 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(+)

Doesn't apply for me. What tree is this based on?

Rob
Chris Morgan April 5, 2022, 6:37 p.m. UTC | #9
On Tue, Apr 05, 2022 at 01:23:56PM -0500, Rob Herring wrote:
> On Mon, Apr 04, 2022 at 04:57:51PM -0500, 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(+)
> 
> Doesn't apply for me. What tree is this based on?

Sorry, it relies on the following being applied (all the prerequisites
are now in master, but this one is still absent):

https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20220303203958.4904-5-macroalpha82@gmail.com/

> 
> Rob
Krzysztof Kozlowski April 6, 2022, 7:28 a.m. UTC | #10
On 05/04/2022 16:04, Chris Morgan wrote:
>>> 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.

Yes,the description of battery fits the purpose of DT.

> 
> 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.


Best regards,
Krzysztof
diff mbox series

Patch

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:
+    description: |
+      The child node for the charger to hold additional properties. If a
+      battery is not in use, this node can be omitted.
+    type: object
+    properties:
+      monitored-battery:
+        description: |
+          A phandle to a monitored battery node that contains a valid
+          value for:
+          charge-full-design-microamp-hours,
+          charge-term-current-microamp,
+          constant-charge-current-max-microamp,
+          constant-charge-voltage-max-microvolt,
+          voltage-max-design-microvolt,
+          voltage-min-design-microvolt,
+          and a valid ocv-capacity table.
+
+      rockchip,resistor-sense-micro-ohms:
+        description: |
+          Value in microohms of the battery sense resistor. This value is
+          used by the driver to set the correct divisor value to translate
+          ADC readings into the proper units of measure.
+        enum: [10000, 20000]
+
+      rockchip,sleep-enter-current-microamp:
+        description: |
+          Value in microamps of the sleep enter current for the charger.
+          Value is used by the driver to calibrate the relax threshold.
+
+      rockchip,sleep-filter-current-microamp:
+        description:
+          Value in microamps of the sleep filter current for the charger.
+          Value is used by the driver to derive the sleep sample current.
+
+    required:
+      - monitored-battery
+      - rockchip,resistor-sense-micro-ohms
+      - rockchip,sleep-enter-current-microamp
+      - rockchip,sleep-filter-current-microamp
+
 allOf:
   - if:
       properties:
@@ -323,6 +364,13 @@  examples:
                 };
             };
 
+            rk817_battery: battery {
+                monitored-battery = <&battery_cell>;
+                rockchip,resistor-sense-micro-ohms = <10000>;
+                rockchip,sleep-enter-current-microamp = <300000>;
+                rockchip,sleep-filter-current-microamp = <100000>;
+            };
+
             rk817_codec: codec {
                 rockchip,mic-in-differential;
             };