diff mbox series

[2/5] ARM: dts: add battery phandle to cpcap_charger

Message ID 20210117224502.0f9a0e80dfd4841ad26a9914@uvos.xyz (mailing list archive)
State New, archived
Headers show
Series [1/5] power: supply: cpcap-charger: get the battery inserted infomation from cpcap-battery | expand

Commit Message

Carl Philipp Klemm Jan. 17, 2021, 9:45 p.m. UTC
This is required for 
	power: supply: cpcap-charger: get the battery inserted 
		infomation from cpcap-battery

Signed-off-by: Carl Philipp Klemm <philipp@uvos.xyz>
---
 arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi | 1 +
 1 file changed, 1 insertion(+)

Comments

Tony Lindgren March 24, 2021, 11:54 a.m. UTC | #1
* Carl Philipp Klemm <philipp@uvos.xyz> [210117 23:45]:
> --- a/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
> +++ b/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
> @@ -61,6 +61,7 @@ &cpcap_adc 2 &cpcap_adc 5
>  			io-channel-names = "battdetb", "battp",
>  					   "vbus", "chg_isense",
>  					   "batti";
> +			battery = <&cpcap_battery>;
>  		};

This seems like good standard stuff to have, picking up this patch into
omap-for-v5.13/dt thanks.

Tony
Sebastian Reichel March 24, 2021, 3:42 p.m. UTC | #2
Hi,

On Wed, Mar 24, 2021 at 01:54:02PM +0200, Tony Lindgren wrote:
> * Carl Philipp Klemm <philipp@uvos.xyz> [210117 23:45]:
> > --- a/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
> > +++ b/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
> > @@ -61,6 +61,7 @@ &cpcap_adc 2 &cpcap_adc 5
> >  			io-channel-names = "battdetb", "battp",
> >  					   "vbus", "chg_isense",
> >  					   "batti";
> > +			battery = <&cpcap_battery>;
> >  		};
> 
> This seems like good standard stuff to have, picking up this patch into
> omap-for-v5.13/dt thanks.
> 
> Tony

This was btw. one of the patches that finally made me convert all
the binding files to YAML. This patch will now result in warning
being printed when you run 'make dtbs_check', since the binding has
not been updated.

I did not yet have time to review the patchset properly, so I have
not yet replied (partely, because of being busy with the YAML
stuff).

I think the patch is also wrong, since the information is already
described in DT - just the other way around: The battery references
the charger. This provides all required information to the kernel
for a 1:1 link.

-- Sebastian
Tony Lindgren March 24, 2021, 4:29 p.m. UTC | #3
Hi,

* Sebastian Reichel <sre@kernel.org> [210324 15:43]:
> On Wed, Mar 24, 2021 at 01:54:02PM +0200, Tony Lindgren wrote:
> > * Carl Philipp Klemm <philipp@uvos.xyz> [210117 23:45]:
> > > --- a/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
> > > +++ b/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
> > > @@ -61,6 +61,7 @@ &cpcap_adc 2 &cpcap_adc 5
> > >  			io-channel-names = "battdetb", "battp",
> > >  					   "vbus", "chg_isense",
> > >  					   "batti";
> > > +			battery = <&cpcap_battery>;
> > >  		};
> > 
> > This seems like good standard stuff to have, picking up this patch into
> > omap-for-v5.13/dt thanks.
> 
> This was btw. one of the patches that finally made me convert all
> the binding files to YAML. This patch will now result in warning
> being printed when you run 'make dtbs_check', since the binding has
> not been updated.
> 
> I did not yet have time to review the patchset properly, so I have
> not yet replied (partely, because of being busy with the YAML
> stuff).
> 
> I think the patch is also wrong, since the information is already
> described in DT - just the other way around: The battery references
> the charger. This provides all required information to the kernel
> for a 1:1 link.

OK, I will drop this patch.

Regards,

Tony
Sebastian Reichel March 24, 2021, 11:19 p.m. UTC | #4
Hi,

On Wed, Mar 24, 2021 at 05:21:52PM +0100, H. Nikolaus Schaller wrote:
> > Am 24.03.2021 um 16:42 schrieb Sebastian Reichel <sre@kernel.org>:
> > I think the patch is also wrong, since the information is already
> > described in DT - just the other way around: The battery references
> > the charger.
> 
> Just curious for other devices to be properly defined:
> 
> Does a battery have its own driver?
> Can it be addressed (through I2C or similar mechanisms)?

Linux power-supply subsystem expects chargers and batteries
with battery being smart battery. I guess this has been
designed following the ACPI specs. On embedded devices this
usually means battery = fuel gauge.

> Is it closer to the processor (being the root node of DTS) or
> farther away than the chargers?

It depends? :) There are systems with smart batteries reachable
via I2C and gpio-charger device, which cannot be controlled and
just having an enable gpio (and thus being in the root node).
OTOH there are systems, that lack a proper fuel gauge and have
advanced I2C chargers.

> My observations is that usually chargers have drivers and need to
> reference battery information to adapt their behaviour.

I believe you are talking about chemistry information e.t.c.
that's available from the simple-battery node?

> So IMHO it would be more natural to have a charger reference the
> battery.

Direction was not something I came up with. I took over the
subsystem years ago when this was already in place. It's a
core thing in the subsystem and definetly cannot be changed
anymore:

Documentation/devicetree/bindings/power/supply/power-supply.yaml

In general sometimes the battery needs charger info and sometimes
charger needs fuel gauge info. The thing is, that this does not
mean we need phandles in both directions. One phandle is enough
to have the required information, everything else can be handled
by kernel frameworks. That's preferred, since it does not create
ABI.

-- Sebastian
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi b/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
index 08a7d3ce383f..842eaa7b89e5 100644
--- a/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
+++ b/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
@@ -61,6 +61,7 @@  &cpcap_adc 2 &cpcap_adc 5
 			io-channel-names = "battdetb", "battp",
 					   "vbus", "chg_isense",
 					   "batti";
+			battery = <&cpcap_battery>;
 		};
 
 		cpcap_regulator: regulator {