Message-ID: <20251114163954.GA3399895-robh@kernel.org>
Date: Fri, 14 Nov 2025 10:39:54 -0600
From: Rob Herring <robh@...nel.org>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...ux.dev>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Mark Brown <broonie@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	linux-kernel@...r.kernel.org, Sebastian Reichel <sre@...nel.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	linux-clk@...r.kernel.org,
	Michael Turquette <mturquette@...libre.com>,
	Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
	linux-leds@...r.kernel.org, Pavel Machek <pavel@...nel.org>,
	Liam Girdwood <lgirdwood@...il.com>, linux-gpio@...r.kernel.org,
	linux-pm@...r.kernel.org, Andreas Kemnade <andreas@...nade.info>,
	Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org,
	linux-rtc@...r.kernel.org, Lee Jones <lee@...nel.org>,
	Stephen Boyd <sboyd@...nel.org>
Subject: Re: [PATCH v4 04/16] dt-bindings: power: supply: BD72720 managed
 battery

On Fri, Nov 14, 2025 at 11:04:27AM +0200, Matti Vaittinen wrote:
> On 13/11/2025 12:53, Rob Herring (Arm) wrote:
> > 
> > On Thu, 13 Nov 2025 10:52:19 +0200, Matti Vaittinen wrote:
> > > From: Matti Vaittinen <mazziesaccount@...il.com>
> > > 
> > > The BD72720 PMIC has a battery charger + coulomb counter block. These
> > > can be used to manage charging of a lithium-ion battery and to do fuel
> > > gauging.
> > > 
> > > ROHM has developed a so called "zero-correction" -algorithm to improve
> > > the fuel-gauging accuracy close to the point where battery is depleted.
> > > This relies on battery specific "VDR" tables, which are measured from
> > > the battery, and which describe the voltage drop rate. More thorough
> > > explanation about the "zero correction" and "VDR" parameters is here:
> > > https://lore.kernel.org/all/676253b9-ff69-7891-1f26-a8b5bb5a421b@fi.rohmeurope.com/
> > > 
> > > Document the VDR zero-correction specific battery properties used by the
> > > BD72720 and some other ROHM chargers.
> > > 
> > > Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
> > > Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
> > > 
> > > ---
> > > NOTE:
> > > Linus' rb-tag holds only if there's no further comments from Rob.
> > > 
> > > Revision history:
> > >   v3 =>:
> > >   - No changes
> > > 
> > >   v2 => v3:
> > >   - Constrain VDR threshold voltage to 48V
> > >   - Use standard '-bp' -suffix for the rohm,volt-drop-soc
> > > 
> > >   RFCv1 => v2:
> > >   - Add units to rohm,volt-drop-soc (tenths of %)
> > >   - Give real temperatures matching the VDR tables, instead of vague
> > >     'high', 'normal', 'low', 'very low'. (Add table of temperatures and
> > >     use number matching the right temperature index in the VDR table name).
> > >   - Fix typoed 'algorithm' in commit message.
> > > 
> > > The parameters are describing the battery voltage drop rates - so they
> > > are properties of the battery, not the charger. Thus they do not belong
> > > in the charger node.
> > > 
> 
> // snip
> 
> > My bot found errors running 'make dt_binding_check' on your patch:
> > 
> > yamllint warnings/errors:
> > 
> > dtschema/dtc warnings/errors:
> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/rohm,vdr-battery.example.dtb: battery (simple-battery): 'degrade-cycle-microamp-hours', 'rohm,volt-drop-0-microvolt', 'rohm,volt-drop-1-microvolt', 'rohm,volt-drop-2-microvolt', 'rohm,volt-drop-3-temp-microvolt', 'rohm,volt-drop-soc-bp', 'rohm,volt-drop-temperatures-millicelsius', 'rohm,voltage-vdr-thresh-microvolt' do not match any of the regexes: '^ocv-capacity-table-[0-9]+$', '^pinctrl-[0-9]+$'
> > 	from schema $id: http://devicetree.org/schemas/power/supply/battery.yaml
> > 
> 
> Odd. I am pretty sure I didn't see this when I ran the make
> dt_binding_check. Not 100% sure what happened there. I get this error now
> though when including all the bindings to the check.
> 
> Do I get this right - these errors result from the properties used in
> example not being included in the battery.yaml? So, this means that the
> check is done based on the binding (battery.yaml) where the compatible
> (simple-battery) is defined - not based on the properties which are present
> in this file where the example resides, (and which references the
> battery.yaml)?
> 
> ...
> 
> Oh... Now that I wrote it I feel like an idiot.
> 
> This approach couldn't work for the validation, right? Let's assume I had a
> VDR battery, and I added a static-battery -node for it. Running the
> validation would pick the battery.yaml based on the compatible (just as it
> does here), and be completely unaware of this vdr-battery.yaml. I have no
> idea why I thought this would work. Probably because I only thought this
> from the documentation POV.
> 
> So, as far as I understand, the only viable options are expanding the
> existing battery.yaml with these properties (which I hoped to avoid, see
> below)
> 
> >> The right place for them is the battery node, which is described by the
> >> generic "battery.yaml". I was not comfortable with adding these
> >> properties to the generic battery.yaml because they are:
> >>    - Meaningful only for those charger drivers which have the VDR
> >>      algorithm implemented. (And even though the algorithm is not charger
> >>      specific, AFAICS, it is currently only used by some ROHM PMIC
> >>      drivers).
> >>    - Technique of measuring the VDR tables for a battery is not widely
> >>      known. AFAICS, only folks at ROHM are measuring those for some
> >>      customer products. We do have those tables available for some of the
> >>      products though (Kobo?).
> 
> or, to add new compatible for the "vdr-battery".
> AFAICS, adding new compatible would require us to wither duplicate the used
> properties from battery.yaml here (as battery.yaml mandates the
> "simple-battery" - compatible) - or to split the battery.yaml in two files,
> one containing the generic properties, other containing the "simple-battery"
> -compatible and referencing the generic one. Then the "vdr-battery" could
> also reference the generic one.
> 
> Any suggestions for the next path to follow?

Probably the latter option. You could do the former and make the new 
properties conditional on the "vdr-battery" compatible. That's fine with 
small differences, but gets messy as there are more properties and 
variations.

But is "VDR" a type of battery though? Is there a certain type/chemistry 
of battery we should be describing where VDR is applicable? I don't 
think it scales well if we define battery compatibles for every 
variation of charger algorithm. Honestly I don't mind just adding 1 
property. I care more if we allow undocumented properties than 
allowing documented but invalid for the platform properties. When it 
becomes 10, 20, 30 properties, then I might start to care. If that 
happens, either we are doing a poor job of generically describing 
battery parameters or chargers and batteries are tightly coupled and 
can't be described independently.

Rob

Powered by blists - more mailing lists